Bug 157097 - [css-grid] Store auto-repeat information in style
Summary: [css-grid] Store auto-repeat information in style
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sergio Villar Senin
URL:
Keywords:
Depends on:
Blocks: 60731
  Show dependency treegraph
 
Reported: 2016-04-27 12:18 PDT by Sergio Villar Senin
Modified: 2016-04-28 02:28 PDT (History)
6 users (show)

See Also:


Attachments
Patch (26.66 KB, patch)
2016-04-27 12:27 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch for landing (26.81 KB, patch)
2016-04-28 02:17 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sergio Villar Senin 2016-04-27 12:18:34 PDT
[css-grid] Store auto-repeat information in style
Comment 1 Sergio Villar Senin 2016-04-27 12:27:23 PDT
Created attachment 277526 [details]
Patch
Comment 2 Darin Adler 2016-04-27 15:48:11 PDT
Comment on attachment 277526 [details]
Patch

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

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1055
> +    const Vector<GridTrackSize>& trackSizes = isRowAxis ? style.gridColumns() : style.gridRows();
> +    const Vector<GridTrackSize>& autoRepeatTrackSizes = isRowAxis ? style.gridAutoRepeatColumns() : style.gridAutoRepeatRows();
> +    const OrderedNamedGridLinesMap& orderedNamedGridLines = isRowAxis ? style.orderedNamedGridColumnLines() : style.orderedNamedGridRowLines();

I think auto& really works well to make code like this less wordy

> Source/WebCore/css/StyleBuilderConverter.h:162
> +    static bool createGridTrackList(CSSValue&, Vector<GridTrackSize>& trackSizes, NamedGridLinesMap&, OrderedNamedGridLinesMap&,
> +        Vector<GridTrackSize>& autoRepeatTrackSizes, NamedGridLinesMap& autoRepeatNamedLines, OrderedNamedGridLinesMap& autoRepeatOrderedNamedLines,
> +        unsigned& autoRepeatInsertionPoint, AutoRepeatType&, StyleResolver&);

There are so many arguments here now that I think we may need to make some kind of class instead of just a function. With 10 different arguments that all are references, so all seem to be in/out, I think we are really looking at too many. Maybe this could still be a function, but return a struct with all the return valeus instead of using out arguments.

> Source/WebCore/css/StyleBuilderConverter.h:864
> +        NamedGridLinesMap::AddResult result = namedGridLines.add(namedGridLine, Vector<unsigned>());

Should either use auto here, or combine this with the next line.

> Source/WebCore/css/StyleBuilderConverter.h:866
> +        OrderedNamedGridLinesMap::AddResult orderedResult = orderedNamedGridLines.add(currentNamedGridLine, Vector<String>());

Should either use auto here, or combine this with the next line.

> Source/WebCore/rendering/style/StyleGridData.h:108
> +    AutoRepeatType m_autoRepeatRowsType;
>  private:

WebKit style calls for a blank line here before private
Comment 3 Sergio Villar Senin 2016-04-28 02:17:21 PDT
Created attachment 277608 [details]
Patch for landing

Added a new struct which stores all the tracks data. Also created a new macro to store all the track info in style suitable for both rows and columns
Comment 4 Sergio Villar Senin 2016-04-28 02:28:40 PDT
Landed on r200182