Bug 72531

Created attachment 115446 [details] WIP: early review appreciated on naming and such, doesn't build on all platforms.

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 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.

Created attachment 115467 [details] Proposed patch 1: Fully building this time, after the renamings.

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!

(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 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 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?

(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.

Created attachment 116809 [details] Patch for landing

Comment on attachment 116809 [details] Patch for landing Clearing flags on attachment: 116809 Committed r101288: <http://trac.webkit.org/changeset/101288>

All reviewed patches have been landed. Closing bug.