Bug 112501 - Fix GridTrackSize::operator==
Summary: Fix GridTrackSize::operator==
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Hans Wennborg
URL:
Keywords:
Depends on: 112574
Blocks: 60731
  Show dependency treegraph
 
Reported: 2013-03-16 12:41 PDT by Hans Wennborg
Modified: 2013-03-19 02:48 PDT (History)
6 users (show)

See Also:


Attachments
Patch (1.59 KB, patch)
2013-03-16 12:45 PDT, Hans Wennborg
no flags Details | Formatted Diff | Diff
Test case (3.08 KB, text/html)
2013-03-18 10:07 PDT, Julien Chaffraix
no flags Details
Patch (6.50 KB, patch)
2013-03-18 12:36 PDT, Hans Wennborg
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hans Wennborg 2013-03-16 12:41:09 PDT
Fix GridTrackSize::operator==
Comment 1 Hans Wennborg 2013-03-16 12:45:28 PDT
Created attachment 193443 [details]
Patch
Comment 2 Hans Wennborg 2013-03-16 12:47:07 PDT
Julien, would you like to take a look?
Comment 3 Eric Seidel (no email) 2013-03-16 12:52:53 PDT
Would be nice to test. Ojan/tony would know how.
Comment 4 Julien Chaffraix 2013-03-18 07:46:02 PDT
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.
Comment 5 Hans Wennborg 2013-03-18 07:49:26 PDT
(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!
Comment 6 Julien Chaffraix 2013-03-18 08:16:15 PDT
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.
Comment 7 Julien Chaffraix 2013-03-18 10:07:44 PDT
Created attachment 193591 [details]
Test case
Comment 8 Hans Wennborg 2013-03-18 12:36:37 PDT
Created attachment 193628 [details]
Patch
Comment 9 Eric Seidel (no email) 2013-03-18 13:35:09 PDT
Comment on attachment 193628 [details]
Patch

Hot damn!  Thanks for the patch and test!  (I assume your test fails before the patch...)
Comment 10 WebKit Review Bot 2013-03-18 13:58:29 PDT
Comment on attachment 193628 [details]
Patch

Clearing flags on attachment: 193628

Committed r146117: <http://trac.webkit.org/changeset/146117>
Comment 11 WebKit Review Bot 2013-03-18 13:58:32 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Julien Chaffraix 2013-03-18 14:45:41 PDT
(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.
Comment 14 Hans Wennborg 2013-03-19 02:48:17 PDT
(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).