WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
112501
Fix GridTrackSize::operator==
https://bugs.webkit.org/show_bug.cgi?id=112501
Summary
Fix GridTrackSize::operator==
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
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
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.
Simon Fraser (smfr)
Comment 12
2013-03-18 14:23:02 PDT
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
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.
Top of Page
Format For Printing
XML
Clone This Bug