Bug 127837 – [CSS Grid Layout] getComputedStyle() not using author's order when showing named grid lines

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Sergio Villar Senin]

It's easy to check that with a declaration like:

-webkit-grid-definition-columns:  "first" "nav" 10px "last";

the return value of getComputedStyle() is

"nav" "first" 10px "last"

It isn't really critical because the name refers to the same position independently of the order, but it looks weird from the author's POV as the names are not in the order they were written. The reason why it happens is because we're just taking the names from a HashMap where we store the information about the named grid lines, so the insertion order is not guaranteed at all.

[Sergio Villar Senin]

Created attachment 223959 [details]
Patch

[Andreas Kling]

Comment on attachment 223959 [details]
Patch

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

> Source/WebCore/rendering/style/RenderStyle.h:1313
> +    void setOrderedNamedGridColumnLines(const OrderedNamedGridLinesMap& orderedNamedGridColumnLines) { SET_VAR(rareNonInheritedData.access()->m_grid, m_orderedNamedGridColumnLines, orderedNamedGridColumnLines); }
> +    void setOrderedNamedGridRowLines(const OrderedNamedGridLinesMap& orderedNamedGridRowLines) { SET_VAR(rareNonInheritedData.access()->m_grid, m_orderedNamedGridRowLines, orderedNamedGridRowLines); }

SET_VAR(foo.access()->bar, ...) is a bad pattern since it will always cause a copy-on-write of the "foo" object.
The point of the SET_VAR macro is to avoid creating objects when overwriting with identical values, and we're losing that optimization here (and in the code surrounding it too!)

> Source/WebCore/rendering/style/StyleGridData.h:66
>      NamedGridLinesMap m_namedGridColumnLines;
>      NamedGridLinesMap m_namedGridRowLines;
>  
> +    OrderedNamedGridLinesMap m_orderedNamedGridColumnLines;
> +    OrderedNamedGridLinesMap m_orderedNamedGridRowLines;

This object is starting to look really heavy. Are we sure this is the right direction to be going?

[Sergio Villar Senin]

(In reply to comment #2)
> (From update of attachment 223959 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=223959&action=review

> > Source/WebCore/rendering/style/StyleGridData.h:66
> >      NamedGridLinesMap m_namedGridColumnLines;
> >      NamedGridLinesMap m_namedGridRowLines;
> >  
> > +    OrderedNamedGridLinesMap m_orderedNamedGridColumnLines;
> > +    OrderedNamedGridLinesMap m_orderedNamedGridRowLines;
> 
> This object is starting to look really heavy. Are we sure this is the right direction to be going?

That's indeed a good question. I tried to include the ordering information in the already available structures (mainly because the vector of ordered named grid lines is just used for getcomputedstyle() but the resulting code was much more complicated. Perhaps I could try again, even penalizing a bit the readability of that uncommon use case, in exchange of a lighter object. Sounds like a plan?

[Sergio Villar Senin]

(In reply to comment #2)
> (From update of attachment 223959 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=223959&action=review
> 
> > Source/WebCore/rendering/style/StyleGridData.h:66
> >      NamedGridLinesMap m_namedGridColumnLines;
> >      NamedGridLinesMap m_namedGridRowLines;
> >  
> > +    OrderedNamedGridLinesMap m_orderedNamedGridColumnLines;
> > +    OrderedNamedGridLinesMap m_orderedNamedGridRowLines;
> 
> This object is starting to look really heavy. Are we sure this is the right direction to be going?

BTW I'm going to remove the NamedGridLinesMap objects in a follow up patch (for bug 130010) so this wouldn't be a big deal.

[Dave Hyatt]

Comment on attachment 223959 [details]
Patch

r=me

[Sergio Villar Senin]

Committed r165742: <http://trac.webkit.org/changeset/165742>