As explained here http://lists.w3.org/Archives/Public/www-style/2014Mar/0512.html we sometimes need to consider that some tracks are infinitely growable when they are really not, in order to produce better results. One example is this case: * Two "auto" tracks (i.e. "minmax(min-content, max-content) minmax(min-content, max-content)") * Item 1 is in track 1, and has min-content = max-content = 10. * Item 2 spans tracks 1 and 2, and has min-content = 30, max-content = 100. Our current implementation will outcome [45px, 55px]. But with this change that would be [10, 90] which is considered more "natural" from the algorithm author's POV.
Created attachment 250342 [details] Patch
Comment on attachment 250342 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=250342&action=review > Source/WebCore/rendering/RenderGrid.cpp:76 > + bool growthLimitIsInfiniteOrTrackIsInfinitelyGrowable() const I wonder if there’s a shorter name for this that could still be just as clear. > Source/WebCore/rendering/RenderGrid.cpp:726 > + auto* tracksToGrowBeyondGrowthLimits = sizingData.growBeyondGrowthLimitsTracks.isEmpty() ? &sizingData.filteredTracks : &sizingData.growBeyondGrowthLimitsTracks; This could be a reference instead of a pointer. > Source/WebCore/rendering/RenderGrid.cpp:735 > + if ((track.*trackSize)() == infinity && track.plannedSize() != infinity) > + track.setInfinitelyGrowable(true); > + else if (track.infinitelyGrowable()) > + track.setInfinitelyGrowable(false); This should be written more simply: track.setInfinitelyGrowable((track.*trackSize)() == infinity && track.plannedSize() != infinity);
(In reply to comment #2) > Comment on attachment 250342 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=250342&action=review > > > Source/WebCore/rendering/RenderGrid.cpp:76 > > + bool growthLimitIsInfiniteOrTrackIsInfinitelyGrowable() const > > I wonder if there’s a shorter name for this that could still be just as > clear. I think I can use trackIsInfinitelyGrowable() because that includes the case of growth limit being infinite. > > > Source/WebCore/rendering/RenderGrid.cpp:726 > > + auto* tracksToGrowBeyondGrowthLimits = sizingData.growBeyondGrowthLimitsTracks.isEmpty() ? &sizingData.filteredTracks : &sizingData.growBeyondGrowthLimitsTracks; > > This could be a reference instead of a pointer. Yes it's just used as an argument of a function that requires a pointer (as it could be nullptr) > > Source/WebCore/rendering/RenderGrid.cpp:735 > > + if ((track.*trackSize)() == infinity && track.plannedSize() != infinity) > > + track.setInfinitelyGrowable(true); > > + else if (track.infinitelyGrowable()) > > + track.setInfinitelyGrowable(false); > > This should be written more simply: > > track.setInfinitelyGrowable((track.*trackSize)() == infinity && > track.plannedSize() != infinity); Yeah. In any case I'm changing it now that we have explicit phases of the algorithm added in https://bugs.webkit.org/show_bug.cgi?id=143481
Comment on attachment 250342 [details] Patch Clearing the r+ as I'm going to attach a different version of the patch based on the changes added in https://bugs.webkit.org/show_bug.cgi?id=143481. Basically we have now extra information about the step/phase of the algorithm that is being run, so we can take better decisions.
Created attachment 250519 [details] Patch
Comment on attachment 250342 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=250342&action=review >>> Source/WebCore/rendering/RenderGrid.cpp:726 >>> + auto* tracksToGrowBeyondGrowthLimits = sizingData.growBeyondGrowthLimitsTracks.isEmpty() ? &sizingData.filteredTracks : &sizingData.growBeyondGrowthLimitsTracks; >> >> This could be a reference instead of a pointer. > > Yes it's just used as an argument of a function that requires a pointer (as it could be nullptr) And when passing to that function, you can use & right at that point (&tracksToGrowBeyondGrowthLimits); it’s better to keep locals as references right until they get to the point where they need to become pointers, especially if the code might be refactored later.
Comment on attachment 250519 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=250519&action=review > Source/WebCore/rendering/RenderGrid.cpp:839 > + auto* tracksToGrowBeyondGrowthLimits = sizingData.growBeyondGrowthLimitsTracks.isEmpty() ? &sizingData.filteredTracks : &sizingData.growBeyondGrowthLimitsTracks; Looking at this expression, I wish there was a function to do this that took a sizingData argument, or even a member function. I still stand by my suggestion that we use a reference here and put the & on the next line of code instead of this one.
Committed r182704: <http://trac.webkit.org/changeset/182704>
(In reply to comment #8) > Committed r182704: <http://trac.webkit.org/changeset/182704> It broke the build everywhere. Could you fix it?
ping?
(In reply to comment #9) > (In reply to comment #8) > > Committed r182704: <http://trac.webkit.org/changeset/182704> > > It broke the build everywhere. Could you fix it? Should be fixed. Sorry for the inconvenience, a last minute change as usual :)