Bug 75630 – getComputedStyle for border-radius is not implemented.

• Component: CSS
• Status: RESOLVED FIXED

[Alexis Menard (darktears)]

getComputedStyle for border-radius is not implemented.

[Alexis Menard (darktears)]

Created attachment 121291 [details]
Patch

[Alexis Menard (darktears)]

I need some feedback here whether it is the right approach, especially in the change regarding CSSPrimitiveValue.

Thanks.

[Tony Chang]

Comment on attachment 121291 [details]
Patch

Per IRC discussion, CSS_UNQUOTED_STRING feels hacky.  We should instead extend CSSListValue so it knows how to use '/' as a separator.  Then the above could be represented as 2 lists in a list.

[Alexis Menard (darktears)]

Created attachment 121304 [details]
Patch

[Alexis Menard (darktears)]

Comment on attachment 121304 [details]
Patch

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

> Source/WebCore/css/CSSValue.h:188
> +    unsigned char m_valueListType : ValueListTypeBits; // CSSValueList type

This is a controversial change with the patch, CSSValue will not fit in 16 bits anymore. Any suggestions?

[Alexis Menard (darktears)]

Created attachment 121308 [details]
Patch

[Tony Chang]

One way to get the extra bit back would be to split CSSValueList into two classes: CSSValueListCommaSeparated and CSSValueListSpaceSeparated.  This would allow us to remove m_isSpaceSeparatedValueList from CSSValue.h and move that bit into m_classType.  This gives us 32 additional CSSValue class types.

This would be a bit tricky because most of the code for CSSValueListCommaSeparated and CSSValueListSpaceSeparated would want to share code.  Maybe one could inherit from the other?

Also WebKitCSSFilterValue and WebKitCSSTransformValue inherit from CSSValueList and it looks like FilterValue can use spaces or commas (maybe it would have to be split into two as well?).

If we can't find a place to get the extra bit back, we could go back to the first patch which allows us to make the '/' a primitive type.

[Tony Chang]

Ryosuke suggested keeping a single CSSValueList, but have 2 different m_classType values for spaces vs commas.  This seems less invasive. I can try it in a separate patch tomorrow.

[WebKit Review Bot]

Comment on attachment 121308 [details]
Patch

Attachment 121308 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11144160

[Alexis Menard (darktears)]

(In reply to comment #8)
> Ryosuke suggested keeping a single CSSValueList, but have 2 different m_classType values for spaces vs commas.  This seems less invasive. I can try it in a separate patch tomorrow.

Problem is WebKitCSSFilter and WebKitCSSTransform have their own class type and the former can either be space or comma separated if I'm not mistaken. The reason of them being special class is when querying the CSS text value it will static_cast them to the right type to call customCSSText.

[Alexis Menard (darktears)]

(In reply to comment #10)
> (In reply to comment #8)
> > Ryosuke suggested keeping a single CSSValueList, but have 2 different m_classType values for spaces vs commas.  This seems less invasive. I can try it in a separate patch tomorrow.
> 
> Problem is WebKitCSSFilter and WebKitCSSTransform have their own class type and the former can either be space or comma separated if I'm not mistaken. The reason of them being special class is when querying the CSS text value it will static_cast them to the right type to call customCSSText.

There is one alternative there :

https://bugs.webkit.org/show_bug.cgi?id=75714

which is to remove the inheritance of RefCounted and allows us to modify how the refcount is handle.

After the patch linked I could reduce m_refCount to be 15 bits and change bool m_isSpaceSeparatedValueList : 1; to be a enum with 3 values (Comma, Spaces, Slash) of 2 bits.

[Alexis Menard (darktears)]

(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #8)
> > > Ryosuke suggested keeping a single CSSValueList, but have 2 different m_classType values for spaces vs commas.  This seems less invasive. I can try it in a separate patch tomorrow.
> > 
> > Problem is WebKitCSSFilter and WebKitCSSTransform have their own class type and the former can either be space or comma separated if I'm not mistaken. The reason of them being special class is when querying the CSS text value it will static_cast them to the right type to call customCSSText.
> 
> There is one alternative there :
> 
> https://bugs.webkit.org/show_bug.cgi?id=75714
> 
> which is to remove the inheritance of RefCounted and allows us to modify how the refcount is handle.
> 
> After the patch linked I could reduce m_refCount to be 15 bits and change bool m_isSpaceSeparatedValueList : 1; to be a enum with 3 values (Comma, Spaces, Slash) of 2 bits.

I think a better looking alternative than https://bugs.webkit.org/show_bug.cgi?id=75725 and https://bugs.webkit.org/show_bug.cgi?id=75714 could be https://bugs.webkit.org/show_bug.cgi?id=75841

[Alexis Menard (darktears)]

Created attachment 121652 [details]
Patch

[WebKit Review Bot]

Comment on attachment 121652 [details]
Patch

Clearing flags on attachment: 121652

Committed r104469: <http://trac.webkit.org/changeset/104469>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.