Closed Bug 1411765 Opened 7 years ago Closed 7 years ago

Fix reflow status when size is exactly the available size and undo comment changes

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: neerja, Assigned: neerja)

References

Details

Attachments

(6 files)

This is a follow up bug filed for adding fantasai's patch here:
https://public.etherpad-mozilla.org/p/patches-accepted

The detailed explanation for this can be found in fantasai's comment here:
Bug 447660 Comment 9.
Comment on attachment 8922095 [details]
Bug 1411765 - Part1:Convert an Incomplete reflow status to OverflowIncomplete also when our size is exactly the available size, not just less.

https://reviewboard.mozilla.org/r/193098/#review198374

r=mats with the nits fixed, assuming it passed regression tests on Try on all platforms

::: commit-message-a97fb:2
(Diff revision 1)
> +Bug 1411765 - Undo comment changes in changeset b9c248025d37 and add new fragmentation test by fantasai. r?mats
> +

The current commit message only says things that are irrelevant.  The relevant part of this patch is that we change "<" to "<=".  So please change it to something like:
"Bug 1411765 - Convert an Incomplete reflow status to OverflowIncomplete also when our size is exactly the available size, not just less.  r=mats"

(The "add new fragmentation test" is irrelevant because when a code change is accompanied by a reftest then it's obvious that it's testing the new layout.
The "by fantasai" is irrelevant because the patch meta data already names the patch author.
The "Undo comment changes in changeset b9c248025d37" is irrelevant because it follows that this patch is addressing those XXX comments and it doesn't really matter which exact revision a code comment was added.)

::: layout/generic/nsBlockFrame.cpp
(Diff revision 1)
>  
>    if (aStatus->IsIncomplete() &&
> -      aFinalSize.BSize(wm) < aReflowInput.AvailableBSize()) {
> -    // We fit in the available space - change status to OVERFLOW_INCOMPLETE.
> -    // XXXmats why didn't Reflow report OVERFLOW_INCOMPLETE in the first place?
> +      aFinalSize.BSize(wm) <= aReflowInput.AvailableBSize()) {
> +    // We ran out of height on this page but we're incomplete
> +    // Set status to complete except for overflow
> -    // XXXmats and why exclude the case when our size == AvailableBSize?

nit: please add a "." at the end of both lines above to make them proper sentences.

::: layout/reftests/w3c-css/submitted/break3/reftest.list:1
(Diff revision 1)
> +== moz-block-fragmentation-001.html moz-block-fragmentation-001-ref.html

The raw patch says: "\ No newline at end of file"
Please add it.
Attachment #8922095 - Flags: review?(mats) → review+
The bug title implies this a comment+test change but there's a code change at line 7494. Please make sure that's intentional & OK. Thx!
Summary: Undo comment changes made while re-enabling float breaking in columns → Fix reflow status when size is exactly the available size and undo comment changes
(In reply to Mats Palmgren (:mats) from comment #2)

Thanks Mats! 
I fixed everything you said and the try run has some failures with grid-fragmentation + some crashes so I will look at those next week. 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c0d3f1273108&selectedJob=139728791
Hi Mats,

I don't really understand grid fragmentation but from what I see in grid-fragmentation-019.html (and most other failures look similar to this), the difference between this and its reference is a small border fragment that you can see here:

https://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/reftest-analyzer.xhtml#logurl=https://queue.taskcluster.net/v1/task/SRAioVt_SQqYt-cpwhXlQw/runs/0/artifacts/public/logs/live_backing.log&only_show_unexpected=1

This border fragment can be removed from the *reference* grid-fragmentation-019-ref.html without affecting the validity of the test like so:
https://public.etherpad-mozilla.org/p/grid-fragmentation-019-ref.htm_border_frag_removed

This makes me think that this is an existing bug in grid fragmentation code and the tests were written assuming this behavior.
I checked this on Chrome and Safari and they seem to do something much different. I attached a screenshot of this. 
 
What do you think?
Flags: needinfo?(mats)
(In reply to Neerja Pancholi[:neerja] from comment #7)
> Hi Mats,
> This border fragment can be removed from the *reference*
> grid-fragmentation-019-ref.html without affecting the validity of the test
> like so:
> https://public.etherpad-mozilla.org/p/grid-fragmentation-019-ref.
> htm_border_frag_removed

Etherpad isn't a good tool for communicating changes like this.
You should've just created a patch as usual.

> This makes me think that this is an existing bug in grid fragmentation code
> and the tests were written assuming this behavior.

The Grid fragmentation code is correct, but the grid-fragmentation-019 tests
are affected by this block layout bug, so the reference layout is wrong.
As you suggest, removing the black border fragment in the third column in
the reference is the right fix.  Since I created the patch while investigating
this I might as well attach it here.  Please merge this into your patch and
land it (assuming everything else was green).

> I checked this on Chrome and Safari and they seem to do something much
> different. I attached a screenshot of this.

The grid fragmentation code in Blink/WebKit is very much *not* implementing
what the Grid spec says, so their layout is almost always wrong.
We do implement grid fragmentation per spec.
Flags: needinfo?(mats)
(In reply to Mats Palmgren (:mats) from comment #8)

Thanks Mats! I will keep what you mentioned in mind.

Other than this extra border fragment problem, there are two other problems of note with the try build:

1. Assertion failure -
layout/generic/crashtests/399412-1.html | assertion count 1 is more than expected 0 assertions
ASSERTION: overflow containers must be zero-block-size: 'finalSize.BSize(wm) == 0', file /Users/npancholi/builds/mozilla-central/layout/generic/nsBlockFrame.cpp, line 1676

I tried debugging this one and it is really not obvious to me so I will create a secondary bug for this.

2. Reftest failure with continuation in the wrong place - 
grid-fragmentation-dyn1-006.html

This can be seen here (2nd test in link)
https://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/reftest-analyzer.xhtml#logurl=https://queue.taskcluster.net/v1/task/WQ7xUZ2MSbmPDHgk3TW6qQ/runs/0/artifacts/public/logs/live_backing.log&only_show_unexpected=1

This is not reproducible on my 10.13 machine with the latest Nightly release but consistently fails on try.

So I think the options here are to either remove the '<=' change for now and take this patch with just comment changes
or
I can mark these problems as expected/fuzzed for now + change the reference file above to remove the border fragment
and
In both cases, file a secondary bug to investigate the final solution. 

As the reviewer, which one of these sounds most reasonable to you? Or neither?
(In reply to Neerja Pancholi[:neerja] from comment #9)
> 1. Assertion failure -
> layout/generic/crashtests/399412-1.html | assertion count 1 is more than
> expected 0 assertions

That assertion is already known to fail for some tests (bug 574889)
so I think it's OK to mark this as assertimg in the manifest with
a comment referencing that bug.

> 2. Reftest failure with continuation in the wrong place - 
> grid-fragmentation-dyn1-006.html

Oh, I missed this one the last time.  I can reproduce it locally
so I'll look into it...
Flags: needinfo?(mats)
(In reply to Mats Palmgren (:mats) from comment #10)

Thanks for taking a look at this Mats. :)
I will update the patch with these changes (minus the fix for #2) tomorrow.
Please merge this patch too.  That test should now pass.
I'll deal with it in bug 1415301.
Flags: needinfo?(mats)
(In reply to Mats Palmgren (:mats) from comment #12)
> Created attachment 8926076 [details] [diff] [review]
> disable-failing-grid-fragmentation-test
> 
> Please merge this patch too.  That test should now pass.
> I'll deal with it in bug 1415301.

Done.
Try run looks good. There are a few failures but none of them seem related.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9e69496ca64d1d781982572b2d6417bfcac0fdd8&selectedJob=142831867
Comment on attachment 8926185 [details]
Bug 1411765 - Part2:Remove extra border fragment from ref file to match it with new expected output of grid fragmentation tests and change assertion count for crashtests.

https://reviewboard.mozilla.org/r/197434/#review202664
Attachment #8926185 - Flags: review?(mats) → review+
Comment on attachment 8926186 [details]
Bug 1411765 - Part3:Disable part of failing test grid-fragmentation-dyn1-006.html.

https://reviewboard.mozilla.org/r/197436/#review202666

Thanks!
Attachment #8926186 - Flags: review?(mats) → review+
Pushed by npancholi@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0590cfeeeaa3
Part1:Convert an Incomplete reflow status to OverflowIncomplete also when our size is exactly the available size, not just less.  r=mats
https://hg.mozilla.org/integration/autoland/rev/4288a7814f71
Part2:Remove extra border fragment from ref file to match it with new expected output of grid fragmentation tests and change assertion count for crashtests. r=mats
https://hg.mozilla.org/integration/autoland/rev/a22464e2572f
Part3:Disable part of failing test grid-fragmentation-dyn1-006.html. r=mats
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: