Bug 134544 - [CSS Grid Layout] Support sparse in auto-placement algorithm
Summary: [CSS Grid Layout] Support sparse in auto-placement algorithm
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Manuel Rego Casasnovas (Holidays - back 1st Sep)
URL:
Keywords:
Depends on: 134057
Blocks: 103316 134842
  Show dependency treegraph
 
Reported: 2014-07-02 04:41 PDT by Manuel Rego Casasnovas (Holidays - back 1st Sep)
Modified: 2014-07-14 14:24 PDT (History)
6 users (show)

See Also:


Attachments
Patch (52.09 KB, patch)
2014-07-11 13:58 PDT, Manuel Rego Casasnovas (Holidays - back 1st Sep)
no flags Details | Formatted Diff | Diff
Patch for landing (52.01 KB, patch)
2014-07-14 13:44 PDT, Manuel Rego Casasnovas (Holidays - back 1st Sep)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Manuel Rego Casasnovas (Holidays - back 1st Sep) 2014-07-02 04:41:13 PDT
Once the new grid-auto-flow syntax is ready (bug #134057) we should implement proper support for sparse mode in auto-placement algorithm.

This is being addressed in Blink (https://codereview.chromium.org/362733002/) and should be ported to WebKit.
Comment 1 Manuel Rego Casasnovas (Holidays - back 1st Sep) 2014-07-11 13:58:59 PDT
Created attachment 234778 [details]
Patch
Comment 2 Sergio Villar Senin 2014-07-14 09:55:16 PDT
Comment on attachment 234778 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=234778&action=review

Awesome patch! As usual :) some comments.

> Source/WebCore/ChangeLog:8
> +        Sparse mode for auto-placement algorithm was not implemented yet.

Remove this line, it adds nothing.

> Source/WebCore/ChangeLog:10
> +        This patch implements sparse mode for auto-placement algorithm. It keeps

Let's explain that the algorithm is "sparse" by default, otherwise people might get confused as the change does not add parsing for any new keyword.

> Source/WebCore/ChangeLog:14
> +        If we're in dense mode it resets the cursor after each item.

Maybe add some text here mentioning that the old code was using the dense mode by default. Otherwise it looks like we're implementing both sparse and dense.

> Source/WebCore/rendering/RenderGrid.cpp:764
> +        // If grid-auto-flow is dense, reset auto-placement cursor.

Let's remove this comment. The code is totally self-explanatory.

> Source/WebCore/rendering/RenderGrid.cpp:765
> +        if (style().isGridAutoFlowAlgorithmDense()) {

Actually there is no need to call it for every single item, we can store the value in a var outside the for loop and use it here.

> Source/WebCore/rendering/RenderGrid.cpp:772
> +void RenderGrid::placeAutoMajorAxisItemOnGrid(RenderBox* gridItem, std::pair<size_t, size_t>& autoPlacementCursor)

I think it'd be a good idea to give the std::pair a good type name and use it instead. AutoPlacementCursor sounds good to me :)

> Source/WebCore/rendering/RenderGrid.cpp:823
> +    // Move auto-placement cursor to the new position.

No need for this comment.

> Source/WebCore/rendering/RenderGrid.h:80
> +    void placeAutoMajorAxisItemOnGrid(RenderBox*, std::pair<size_t, size_t>& autoPlacementCursor);

Ditto the std::pair. Note that the argument name won't be needed.
Comment 3 Manuel Rego Casasnovas (Holidays - back 1st Sep) 2014-07-14 13:44:39 PDT
Created attachment 234876 [details]
Patch for landing
Comment 4 WebKit Commit Bot 2014-07-14 14:24:12 PDT
Comment on attachment 234876 [details]
Patch for landing

Clearing flags on attachment: 234876

Committed r171082: <http://trac.webkit.org/changeset/171082>
Comment 5 WebKit Commit Bot 2014-07-14 14:24:15 PDT
All reviewed patches have been landed.  Closing bug.