Robert O'Callahan (:roc) (email my personal email if necessary) Reporter
	Description • 16 years ago

We turned this off in bug 413048. One day when it's safe to turn it on again, we should probably do so.

	Mats Palmgren (inactive) Assignee
	Comment 1 • 10 years ago

We've fixed a ton of bugs in the past six years so I think we should try to enable this
again behind a pref.  I pushed a Try run (together with the fixes in bug 1108104):
https://tbpl.mozilla.org/?tree=Try&rev=b517a952e46d

There are a few assertions that we need to fix before enabling it though:

layout/generic/crashtests/404219-2.html
layout/generic/crashtests/468207-1.html
layout/generic/crashtests/600100.xhtml
layout/generic/crashtests/810726.html
layout/generic/crashtests/840787.html

	Mats Palmgren (inactive) Assignee
	Comment 3 • 10 years ago

Fixes this assertion:
overflow container must not have computedBSizeLeftOver: '!( IS_TRUE_OVERFLOW_CONTAINER(this) && computedBSizeLeftOver )', layout\generic\nsBlockFrame.cpp, line 7199

	Mats Palmgren (inactive) Assignee
	Comment 4 • 10 years ago

This fixes a few bugs when pushing/pulling float continuations between columns.
(see the patch description for details)

https://tbpl.mozilla.org/?tree=Try&rev=e6ec37ed6cca
(other tests were green in an earlier run, so just ref/crashtests here)

	Robert O'Callahan (:roc) (email my personal email if necessary) Reporter
	Comment 5 • 10 years ago

Comment on attachment 8535960 [details] [diff] [review]
part 3 - When collecting / pushing a float, make sure to also pick up its next-in-flows that are in same block.

Review of attachment 8535960 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsBlockFrame.cpp
@@ +7354,5 @@
> +      nsIFrame* nif = e.get()->GetNextInFlow();
> +      MOZ_ASSERT(!nif || (!mFloats.ContainsFrame(nif) && !mFrames.ContainsFrame(nif)));
> +    }
> +  }
> +  

trailing whitespace

	Mats Palmgren (inactive) Assignee
	Comment 6 • 10 years ago

> trailing whitespace

Fixed.

https://hg.mozilla.org/integration/mozilla-inbound/rev/3c8aa35169bf
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9c248025d37
https://hg.mozilla.org/integration/mozilla-inbound/rev/56d432ad6ed6

	Ryan VanderMeulen [:RyanVM]
	Comment 7 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/3c8aa35169bf
https://hg.mozilla.org/mozilla-central/rev/b9c248025d37
https://hg.mozilla.org/mozilla-central/rev/56d432ad6ed6

	Mats Palmgren (inactive) Assignee
	Comment 8 • 9 years ago

Filed bug 1148104 to enable this in RELEASE builds and let it ride the trains.

	fantasai
	Comment 9 • 7 years ago

I don't really understand why the following change was made in patch #2, since rewrapping a line of text and replacing comments with questions seems unrelated to any of the code changes to fixing this bug.

-  if (NS_FRAME_IS_NOT_COMPLETE(*aStatus)
-      && aFinalSize.BSize(wm) < aReflowState.AvailableBSize()) {
-    // We ran out of height on this page but we're incomplete
-    // Set status to complete except for overflow
+  if (NS_FRAME_IS_NOT_COMPLETE(*aStatus) &&
+      aFinalSize.BSize(wm) < aReflowState.AvailableBSize()) {
+    // We fit in the available space - change status to OVERFLOW_INCOMPLETE.
+    // XXXmats why didn't Reflow report OVERFLOW_INCOMPLETE in the first place?
+    // XXXmats and why exclude the case when our size == AvailableBSize?
     NS_FRAME_SET_OVERFLOW_INCOMPLETE(*aStatus);
   }
Imho the new comment is less helpful than the original: our content doesn't fit, so we're not complete and need a continuation to continue placing our content--but we ran out of computed height, that is why we need to change the reflow status to OVERFLOW_INCOMPLETE to indicate this state.

To answer the first question, Reflow didn't report OVERFLOW_INCOMPLETE rather than NOT_COMPLETE because it didn't know we ran out of computed height: we only know that after we compute the height in this function.

To answer the second question, this seems like an error and likely should be, as you seem to suspect, <= rather than < in the comparison.

p.s. Probably questions should be directed to the person responsible for answering rather than left in the source for her to discover several years later. ;)

	Jet Villegas (inactive)
	Comment 10 • 7 years ago

Neerja: @fantasai graciously posted a patch for fixing the issue in comment 9 here:
https://public.etherpad-mozilla.org/p/patches-accepted

Can you file a follow-up bug and get it reviewed/landed? Thx!

	Neerja Pancholi[:neerja]
	Comment 11 • 7 years ago

(In reply to Jet Villegas (:jet) from comment #10)
> Neerja: @fantasai graciously posted a patch for fixing the issue in comment
> 9 here:
> https://public.etherpad-mozilla.org/p/patches-accepted
> 
> Can you file a follow-up bug and get it reviewed/landed? Thx!

Created follow up Bug 1411765 for this.