Bug 112501 – Fix GridTrackSize::operator==

• Component: New Bugs
• Status: RESOLVED FIXED

[Hans Wennborg]

Fix GridTrackSize::operator==

[Hans Wennborg]

Created attachment 193443 [details]
Patch

[Hans Wennborg]

Julien, would you like to take a look?

[Eric Seidel (no email)]

Would be nice to test. Ojan/tony would know how.

[Julien Chaffraix]

As Eric pointed out, this should be tested. operator== is used during style changes to see if we should relayout the grid element so it should be possible to test that.

Here is the outline of how this would work:
* Set grid-columns to minmax(10px, 20px), layout and check the sizes.
* Set grid-columns to minmax(10px, 30px), layout and check that the width changed.

We have 0 testing for dynamic updates of grid-{columns|rows} so it's a good idea to add more testing that just this narrow case. Let me see if I can come up with some good testing.

[Hans Wennborg]

(In reply to comment #4)
> We have 0 testing for dynamic updates of grid-{columns|rows} so it's a good idea to add more testing that just this narrow case. Let me see if I can come up with some good testing.

Sounds great, thanks!

[Julien Chaffraix]

Actually, we have another bug in the StyleGridData::operator=:

bool operator==(const StyleGridData& o) const
{
    return m_gridColumns == o.m_gridColumns && m_gridRows == o.m_gridRows && m_gridAutoFlow != o.m_gridAutoFlow;
}

This bug is hiding what Hans is trying to fix as any grid-{rows|columns} will still trigger a full layout as grid-auto-flow didn't change. I am on it, then we should be able to test this bug.

[Julien Chaffraix]

Created attachment 193591 [details]
Test case

[Hans Wennborg]

Created attachment 193628 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 193628 [details]
Patch

Hot damn!  Thanks for the patch and test!  (I assume your test fails before the patch...)

[WebKit Review Bot]

Comment on attachment 193628 [details]
Patch

Clearing flags on attachment: 193628

Committed r146117: <http://trac.webkit.org/changeset/146117>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Fraser (smfr)]

Might this have caused a this test failure?
http://build.webkit.org/results/Apple%20MountainLion%20Debug%20WK1%20(Tests)/r146109%20(6473)/fast/frames/flattening/frameset-flattening-grid-pretty-diff.html

[Julien Chaffraix]

(In reply to comment #12)
> Might this have caused a this test failure?
> http://build.webkit.org/results/Apple%20MountainLion%20Debug%20WK1%20(Tests)/r146109%20(6473)/fast/frames/flattening/frameset-flattening-grid-pretty-diff.html

You need to opt into the grid layout parsing code at runtime. This test doesn't opt in nor does the output / test shows anything grid related so I would think it is not your culprit.

[Hans Wennborg]

(In reply to comment #9)
> (From update of attachment 193628 [details])
> Hot damn!  Thanks for the patch and test!  (I assume your test fails before the patch...)

Thank Julien for the test :)

Yes, it fails without my patch (though the patch in Bug 112574 was required to get there).