Bug 75630 - getComputedStyle for border-radius is not implemented.
Summary: getComputedStyle for border-radius is not implemented.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Alexis Menard (darktears)
URL:
Keywords:
Depends on: 75725
Blocks: 13658
  Show dependency treegraph
 
Reported: 2012-01-05 09:59 PST by Alexis Menard (darktears)
Modified: 2012-01-09 11:27 PST (History)
8 users (show)

See Also:


Attachments
Patch (24.97 KB, patch)
2012-01-05 10:02 PST, Alexis Menard (darktears)
no flags Details | Formatted Diff | Diff
Patch (28.59 KB, patch)
2012-01-05 11:36 PST, Alexis Menard (darktears)
no flags Details | Formatted Diff | Diff
Patch (28.74 KB, patch)
2012-01-05 11:52 PST, Alexis Menard (darktears)
no flags Details | Formatted Diff | Diff
Patch (23.40 KB, patch)
2012-01-09 05:59 PST, Alexis Menard (darktears)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexis Menard (darktears) 2012-01-05 09:59:47 PST
getComputedStyle for border-radius is not implemented.
Comment 1 Alexis Menard (darktears) 2012-01-05 10:02:14 PST
Created attachment 121291 [details]
Patch
Comment 2 Alexis Menard (darktears) 2012-01-05 10:03:37 PST
I need some feedback here whether it is the right approach, especially in the change regarding CSSPrimitiveValue.

Thanks.
Comment 3 Tony Chang 2012-01-05 10:57:35 PST
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.
Comment 4 Alexis Menard (darktears) 2012-01-05 11:36:26 PST
Created attachment 121304 [details]
Patch
Comment 5 Alexis Menard (darktears) 2012-01-05 11:38:06 PST
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?
Comment 6 Alexis Menard (darktears) 2012-01-05 11:52:08 PST
Created attachment 121308 [details]
Patch
Comment 7 Tony Chang 2012-01-05 12:04:07 PST
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.
Comment 8 Tony Chang 2012-01-05 16:22:29 PST
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.
Comment 9 WebKit Review Bot 2012-01-05 17:51:43 PST
Comment on attachment 121308 [details]
Patch

Attachment 121308 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11144160
Comment 10 Alexis Menard (darktears) 2012-01-06 05:48:02 PST
(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.
Comment 11 Alexis Menard (darktears) 2012-01-06 10:45:56 PST
(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.
Comment 12 Alexis Menard (darktears) 2012-01-09 04:06:17 PST
(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
Comment 13 Alexis Menard (darktears) 2012-01-09 05:59:26 PST
Created attachment 121652 [details]
Patch
Comment 14 WebKit Review Bot 2012-01-09 11:27:09 PST
Comment on attachment 121652 [details]
Patch

Clearing flags on attachment: 121652

Committed r104469: <http://trac.webkit.org/changeset/104469>
Comment 15 WebKit Review Bot 2012-01-09 11:27:17 PST
All reviewed patches have been landed.  Closing bug.