Bug 72531 - Add limited parsing support for grid-columns and grid-rows
Summary: Add limited parsing support for grid-columns and grid-rows
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Julien Chaffraix
URL:
Keywords:
Depends on:
Blocks: 60731
  Show dependency treegraph
 
Reported: 2011-11-16 12:01 PST by Julien Chaffraix
Modified: 2011-11-28 14:59 PST (History)
5 users (show)

See Also:


Attachments
WIP: early review appreciated on naming and such, doesn't build on all platforms. (31.15 KB, patch)
2011-11-16 14:17 PST, Julien Chaffraix
no flags Details | Formatted Diff | Diff
Proposed patch 1: Fully building this time, after the renamings. (39.34 KB, patch)
2011-11-16 15:57 PST, Julien Chaffraix
no flags Details | Formatted Diff | Diff
Patch for landing (39.34 KB, patch)
2011-11-28 13:33 PST, Julien Chaffraix
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Julien Chaffraix 2011-11-16 12:01:59 PST
This is about implementing a small part of the spec's grammar for <track-list> (section 6.5).

My goal is to implement the following subset as a first step:

<track-list> := <length> | <percentage> | 'none' | 'auto'

We will then increase our supported set to better match the spec.
Comment 1 Julien Chaffraix 2011-11-16 14:17:59 PST
Created attachment 115446 [details]
WIP: early review appreciated on naming and such, doesn't build on all platforms.
Comment 2 Ojan Vafai 2011-11-16 14:44:32 PST
Comment on attachment 115446 [details]
WIP: early review appreciated on naming and such, doesn't build on all platforms.

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

> Source/WebCore/ChangeLog:16
> +        Helper function to convert our Rendertyle information to a proper CSSValue.

typo: Rendertyle

> Source/WebCore/css/CSSParser.cpp:3506
> +bool CSSParser::parseTrackList(int propId, bool important)

parseGridTrackList maybe?

> Source/WebCore/rendering/style/StyleRareNonInheritedData.h:118
> +    DataRef<StyleGridElementData> m_gridLayout;

As discussed in person, call this StyleGridData. Maybe just call this m_grid? That matches better the names of the other member variables here.
Comment 3 Julien Chaffraix 2011-11-16 15:49:05 PST
Comment on attachment 115446 [details]
WIP: early review appreciated on naming and such, doesn't build on all platforms.

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

>> Source/WebCore/ChangeLog:16
>> +        Helper function to convert our Rendertyle information to a proper CSSValue.
> 
> typo: Rendertyle

Good catch!

>> Source/WebCore/css/CSSParser.cpp:3506
>> +bool CSSParser::parseTrackList(int propId, bool important)
> 
> parseGridTrackList maybe?

It is coherent with valueForGridTrackList. Will change.

>> Source/WebCore/rendering/style/StyleRareNonInheritedData.h:118
>> +    DataRef<StyleGridElementData> m_gridLayout;
> 
> As discussed in person, call this StyleGridData. Maybe just call this m_grid? That matches better the names of the other member variables here.

Will be renamed. I would have leaned towards m_gridData as it would make more sense at a call site (we are not manipulating the grid but the grid data). However consistency is more important.
Comment 4 Julien Chaffraix 2011-11-16 15:57:37 PST
Created attachment 115467 [details]
Proposed patch 1: Fully building this time, after the renamings.
Comment 5 Julien Chaffraix 2011-11-17 10:12:25 PST
Tab, we had a discussion yesterday about what getComputedStyle should return. There seems to be no real convention (our code is messy and looks like a per-case choice). What's your opinion on our results? Should we come back to the spec and have it amended? Thanks!
Comment 6 Tab Atkins 2011-11-22 12:43:04 PST
(In reply to comment #5)
> Tab, we had a discussion yesterday about what getComputedStyle should return. There seems to be no real convention (our code is messy and looks like a per-case choice). What's your opinion on our results? Should we come back to the spec and have it amended? Thanks!

According to Alex Mogilevsky, one of the implementors on IE, it returns the percentage in getComputedStyle.  In other words, it just returns the computed value.

It looks like this is what we're doing, but I'm not entirely sure.  If not, we should match them.  I've asked them to put these details into the spec.
Comment 7 Tony Chang 2011-11-28 10:04:31 PST
Comment on attachment 115467 [details]
Proposed patch 1: Fully building this time, after the renamings.

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

> LayoutTests/fast/css-grid-layout/grid-columns-rows-get-set.html:28
> +    -webkit-grid-columns: 10em;
> +    -webkit-grid-rows: 15em;

These might resolve to different pixel amounts depending on the platform, but we can rebaseline if that's the case.
Comment 8 Julien Chaffraix 2011-11-28 10:18:29 PST
Comment on attachment 115467 [details]
Proposed patch 1: Fully building this time, after the renamings.

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

>> LayoutTests/fast/css-grid-layout/grid-columns-rows-get-set.html:28
>> +    -webkit-grid-rows: 15em;
> 
> These might resolve to different pixel amounts depending on the platform, but we can rebaseline if that's the case.

Good point, maybe we should use the Ahem font with a fixed size then to mitigate this risk?
Comment 9 Tony Chang 2011-11-28 11:11:03 PST
(In reply to comment #8)
> (From update of attachment 115467 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=115467&action=review
> 
> >> LayoutTests/fast/css-grid-layout/grid-columns-rows-get-set.html:28
> >> +    -webkit-grid-rows: 15em;
> > 
> > These might resolve to different pixel amounts depending on the platform, but we can rebaseline if that's the case.
> 
> Good point, maybe we should use the Ahem font with a fixed size then to mitigate this risk?

That sounds like a good idea to me.
Comment 10 Julien Chaffraix 2011-11-28 13:33:51 PST
Created attachment 116809 [details]
Patch for landing
Comment 11 WebKit Review Bot 2011-11-28 14:59:40 PST
Comment on attachment 116809 [details]
Patch for landing

Clearing flags on attachment: 116809

Committed r101288: <http://trac.webkit.org/changeset/101288>
Comment 12 WebKit Review Bot 2011-11-28 14:59:44 PST
All reviewed patches have been landed.  Closing bug.