Bug 112501

Summary: Fix GridTrackSize::operator==
Product: WebKit Reporter: Hans Wennborg <hans>
Component: New BugsAssignee: Hans Wennborg <hans>
Status: RESOLVED FIXED    
Severity: Normal CC: eric, esprehn+autocc, jchaffraix, ojan.autocc, simon.fraser, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 112574    
Bug Blocks: 60731    
Attachments:
Description Flags
Patch
none
Test case
none
Patch none

Hans Wennborg
Reported 2013-03-16 12:41:09 PDT
Fix GridTrackSize::operator==
Attachments
Patch (1.59 KB, patch)
2013-03-16 12:45 PDT, Hans Wennborg 		no flags
DetailsFormatted DiffDiff
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
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Hans Wennborg
Comment 1 2013-03-16 12:45:28 PDT
Created attachment 193443 [details] Patch
Hans Wennborg
Comment 2 2013-03-16 12:47:07 PDT
Julien, would you like to take a look?
Eric Seidel (no email)
Comment 3 2013-03-16 12:52:53 PDT
Would be nice to test. Ojan/tony would know how.
Julien Chaffraix
Comment 4 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.
Hans Wennborg
Comment 5 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!
Julien Chaffraix
Comment 6 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.
Julien Chaffraix
Comment 7 2013-03-18 10:07:44 PDT
Created attachment 193591 [details] Test case
Hans Wennborg
Comment 8 2013-03-18 12:36:37 PDT
Created attachment 193628 [details] Patch
Eric Seidel (no email)
Comment 9 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...)
WebKit Review Bot
Comment 10 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>
WebKit Review Bot
Comment 11 2013-03-18 13:58:32 PDT
All reviewed patches have been landed. Closing bug.
Julien Chaffraix
Comment 13 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.
Hans Wennborg
Comment 14 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).
Note You need to log in before you can comment on or make changes to this bug.