Bug 104700

Created attachment 178869 [details] Proposed implementation: Feedback seeked on whether testing logical width and logical height independently is sufficient.

Comment on attachment 178869 [details] Proposed implementation: Feedback seeked on whether testing logical width and logical height independently is sufficient. Attachment 178869 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15278498

Comment on attachment 178869 [details] Proposed implementation: Feedback seeked on whether testing logical width and logical height independently is sufficient. View in context: https://bugs.webkit.org/attachment.cgi?id=178869&action=review Mac failure is real: /Volumes/Data/EWS/WebKit/Source/WebCore/rendering/RenderGrid.cpp:192:63: error: unused parameter 'direction' [-Werror,-Wunused-parameter] void RenderGrid::distributeSpaceToTracks(TrackSizingDirection direction, Vector<GridTrack>& tracks, LayoutUnit availableLogicalSpace) > Source/WebCore/ChangeLog:9 > + to resolve any sizing function that doesn't size based on the content (mincontent, maxcontent). Nit: min-content, max-content. > Source/WebCore/rendering/RenderGrid.cpp:125 > + const Length& minTrackLength = trackStyles[i].minTrackBreadth(); > + const Length& maxTrackLength = trackStyles[i].maxTrackBreadth(); I think most of the time we don't bother with using a const reference since Length is pretty small (actually, looks like it's trying to be small, but failing-- I'll fix that in a separate patch). > Source/WebCore/rendering/RenderGrid.cpp:155 > + const Length& minTrackBreadth = trackStyles[i].minTrackBreadth(); > + const Length& maxTrackBreadth = trackStyles[i].maxTrackBreadth(); Nit: Maybe drop the const &. > Source/WebCore/rendering/RenderGrid.cpp:160 > + // Ignore the max track breadth if it is smaller than the min track breadth. Remove this comment. It describe's the code and doesn't answer 'why'. > Source/WebCore/rendering/RenderGrid.cpp:162 > + if (track.m_maxBreadth < track.m_usedBreadth) > + track.m_maxBreadth = track.m_usedBreadth; Nit: This could be track.m_maxBreadth = std::max(track.m_maxBreadth, track.m_usedBreadth). > Source/WebCore/rendering/RenderGrid.cpp:179 > + // FIXME: We stil need to support calc() here (bug 103761). Nit: stil -> still > Source/WebCore/rendering/RenderGrid.cpp:187 > +bool RenderGrid::sortByGridTrackGrowthPotential(GridTrack* track1, GridTrack* track2) Does this function need to be part of RenderGrid? I would declare it static to the cpp file. > Source/WebCore/rendering/RenderGrid.cpp:194 > + const unsigned tracksSize = tracks.size(); size_t and I would drop the const. Most other uses of const are for global values. > Source/WebCore/rendering/RenderGrid.cpp:196 > + for (unsigned i = 0; i < tracksSize; ++i) size_t > Source/WebCore/rendering/RenderGrid.cpp:201 > + for (unsigned i = 0; i < tracksSize; ++i) { size_t

(In reply to comment #3) > actually, looks like it's trying to be small, but failing-- I'll fix that in a separate patch. Oh, I'm wrong, it's already bit packed well.

Comment on attachment 178869 [details] Proposed implementation: Feedback seeked on whether testing logical width and logical height independently is sufficient. View in context: https://bugs.webkit.org/attachment.cgi?id=178869&action=review >> Source/WebCore/rendering/RenderGrid.cpp:125 >> + const Length& maxTrackLength = trackStyles[i].maxTrackBreadth(); > > I think most of the time we don't bother with using a const reference since Length is pretty small (actually, looks like it's trying to be small, but failing-- I'll fix that in a separate patch). True, but it's always better to avoid a copy if we can avoid it, which we can in this case. >> Source/WebCore/rendering/RenderGrid.cpp:162 >> + track.m_maxBreadth = track.m_usedBreadth; > > Nit: This could be track.m_maxBreadth = std::max(track.m_maxBreadth, track.m_usedBreadth). It can! >> Source/WebCore/rendering/RenderGrid.cpp:187 >> +bool RenderGrid::sortByGridTrackGrowthPotential(GridTrack* track1, GridTrack* track2) > > Does this function need to be part of RenderGrid? I would declare it static to the cpp file. Unfortunately it has to be because we are accessing GridTrack which is declared as a private class inside RenderGrid. We could make GridTrack publicly declared which would remove this need though. We could also pull it out of RenderGrid to avoid having to rewrite that to RenderGrid::GridTrack. Doing those 2 would probably be better though but I don't feel strongly either way. >> Source/WebCore/rendering/RenderGrid.cpp:201 >> + for (unsigned i = 0; i < tracksSize; ++i) { > > size_t There is an issue with using size_t here: LayoutUnit doesn't handle size_t in cross-platform manner. See the FIXME at the beginning of LayoutUnit and https://bugs.webkit.org/show_bug.cgi?id=83848#c19 for more details. It's possible to fix that by introducing the operator/() for all primitive types. Maybe it's time to nix this FIXME.

(In reply to comment #5) > (From update of attachment 178869 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=178869&action=review > > >> Source/WebCore/rendering/RenderGrid.cpp:187 > >> +bool RenderGrid::sortByGridTrackGrowthPotential(GridTrack* track1, GridTrack* track2) > > > > Does this function need to be part of RenderGrid? I would declare it static to the cpp file. > > Unfortunately it has to be because we are accessing GridTrack which is declared as a private class inside RenderGrid. We could make GridTrack publicly declared which would remove this need though. We could also pull it out of RenderGrid to avoid having to rewrite that to RenderGrid::GridTrack. Doing those 2 would probably be better though but I don't feel strongly either way. I think that would be a good refactor.

Created attachment 179182 [details] Updated change: made GridTrack public in WebCore namespace and switched the code to use size_t.

CC'ing Darin & Emil as they may have some comment on the LayoutUnit change.

Comment on attachment 179182 [details] Updated change: made GridTrack public in WebCore namespace and switched the code to use size_t. Attachment 179182 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/15313088

Comment on attachment 179182 [details] Updated change: made GridTrack public in WebCore namespace and switched the code to use size_t. Attachment 179182 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/15316107

FWIW, I would do the LayoutUnit change in a separate bug first.

Comment on attachment 179182 [details] Updated change: made GridTrack public in WebCore namespace and switched the code to use size_t. View in context: https://bugs.webkit.org/attachment.cgi?id=179182&action=review > Source/WebCore/platform/LayoutUnit.h:73 > LayoutUnit(int value) { setValue(value); } Please remove the FIXME comment on line 67-70 as you are in effect adding support for size_t. Yay! > Source/WebCore/platform/LayoutUnit.h:88 > + m_value = clampTo<unsigned long long>(value * kFixedPointDenominator, static_cast<unsigned long long>(INT_MIN), static_cast<unsigned long long>(INT_MAX)); Assigning the return value from clampTo<unsigned long long> to m_value do an implicit cast from an unsigned long long to a signed int. This will likely trigger a compiler warning if SATURATED_LAYOUT_ARITHMETIC is enabled. You'll might want to add a static_cast or do clampTo<int>. > Source/WebCore/platform/LayoutUnit.h:305 > + static bool isInBounds(unsigned long value) Instead of adding two new isInBounds methods how about using the unsigned version directly and do a cast the REPORT_OVERFLOW call in the subpixel disabled constructors? I.e. REPORT_OVERFLOW(isInBounds(static_cast<unsigned>(value)));

Tony & Emil, thanks for the comment, it sounds like I should just fix bug 83848 for good instead of cutting corners by implementing half of it. Marking this bug as blocked on bug 83848.

Created attachment 181574 [details] Updated patch, now that the size_t operations are supported.

Comment on attachment 181574 [details] Updated patch, now that the size_t operations are supported. Thanks!

Comment on attachment 181574 [details] Updated patch, now that the size_t operations are supported. Attachment 181574 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/15743768

Comment on attachment 181574 [details] Updated patch, now that the size_t operations are supported. Attachment 181574 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/15731906

Comment on attachment 181574 [details] Updated patch, now that the size_t operations are supported. Attachment 181574 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/15739902

Comment on attachment 181574 [details] Updated patch, now that the size_t operations are supported. Attachment 181574 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15754044

Comment on attachment 181574 [details] Updated patch, now that the size_t operations are supported. View in context: https://bugs.webkit.org/attachment.cgi?id=181574&action=review > Source/WebCore/rendering/RenderGrid.cpp:153 > + LayoutUnit availableLogicalSpace = (direction == ForColumns) ? availableLogicalWidth() : availableLogicalHeight(); The EWS is choking on this line. I forgot to update it to availableLogicalHeight(IncludeMarginBorderPadding).

Created attachment 181581 [details] Updated patch for the EWS.

Comment on attachment 181581 [details] Updated patch for the EWS. Clearing flags on attachment: 181581 Committed r139025: <http://trac.webkit.org/changeset/139025>

All reviewed patches have been landed. Closing bug.

The newly added test seems to be timing out: http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=fast%2Fcss-grid-layout%2Fpercent-resolution-grid-item.html%2Cfast%2Fcss-grid-layout%2Fbreadth-size-resolution-grid.html%2Cfast%2Fcss-grid-layout%2Fplace-cell-by-index.html%2Cfast%2Fcss-grid-layout%2Fpercent-padding-margin-resolution-grid-item.html%2Cfast%2Fcss-grid-layout%2Fgrid-columns-rows-get-set-multiple.html%2Cfast%2Fcss-grid-layout%2Fcalc-resolution-grid-item.html

(In reply to comment #24) > The newly added test seems to be timing out: Oh, it _made_ some tests start time out. Bad stuff?

(In reply to comment #25) > (In reply to comment #24) > > The newly added test seems to be timing out: > > Oh, it _made_ some tests start time out. Bad stuff? The timeout only impacts Win7 Dbg(1), all the other platforms are just fine, including Win7 Dbg(2). The fact that another bot with a similar configuration is fine with the change makes me think the bot is acting up and the change is fine.

(In reply to comment #26) > (In reply to comment #25) > > (In reply to comment #24) > > > The newly added test seems to be timing out: > > > > Oh, it _made_ some tests start time out. Bad stuff? > > The timeout only impacts Win7 Dbg(1), all the other platforms are just fine, including Win7 Dbg(2). The fact that another bot with a similar configuration is fine with the change makes me think the bot is acting up and the change is fine. Win7 Dbg(1) and (2) run two different sets of tests.

The tests that forced this bug re-opened are passing AFAICT. Closing back.