Summary: | Fix GridTrackSize::operator== | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Hans Wennborg <hans> | ||||||||
Component: | New Bugs | Assignee: | 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
Hans Wennborg
2013-03-16 12:41:09 PDT
Created attachment 193443 [details]
Patch
Julien, would you like to take a look? Would be nice to test. Ojan/tony would know how. 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. (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! 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. Created attachment 193591 [details]
Test case
Created attachment 193628 [details]
Patch
Comment on attachment 193628 [details]
Patch
Hot damn! Thanks for the patch and test! (I assume your test fails before the patch...)
Comment on attachment 193628 [details] Patch Clearing flags on attachment: 193628 Committed r146117: <http://trac.webkit.org/changeset/146117> All reviewed patches have been landed. Closing bug. 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 (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. (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). |