Bug 75563 – remove CSSBorderImageValue

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Tony Chang]

border-image is a CSS shorthand but has a custom CSSValue.  This was because it became a shorthand at the last minute and done to maintain backwards compat with -webkit-border-image.

We could get rid of CSSBorderImageValue as long as we maintain that getComputedStyle for -webkit-border-image still works. This would involve some rewriting/refactoring of the parsing code for -webkit-border-image and border-image.

[Alexis Menard (darktears)]

I'll take that one if nobody object.

[Alexis Menard (darktears)]

Created attachment 122661 [details]
Patch

[Gustavo Noronha (kov)]

Comment on attachment 122661 [details]
Patch

Attachment 122661 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11254242

[Gyuyoung Kim]

Comment on attachment 122661 [details]
Patch

Attachment 122661 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11255247

[Alexis Menard (darktears)]

Created attachment 122666 [details]
Patch

[WebKit Review Bot]

Attachment 122666 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebCore/css/CSSStyleSelector.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alexis Menard (darktears)]

(In reply to comment #6)
> Attachment 122666 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
> 
> Source/WebCore/css/CSSStyleSelector.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
> Total errors found: 1 in 6 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

I kept it grouped despite the order being wrong originally.

[Tony Chang]

Comment on attachment 122666 [details]
Patch

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

I think you're missing the file deletes.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:508
> +        RefPtr<CSSValueList> listSlash = CSSValueList::createSlashSeparated();
> +        if (imageSlices)
> +            listSlash->append(imageSlices.release());

The existing test doesn't seem to include test cases for this slash separated list.  Can you add some test cases before this change to make sure we're not breaking anything?

Hyatt made it sound like getComputedStyle on webkit-border-image may be different from getComputedStyle on border-image.

> Source/WebCore/css/CSSParser.cpp:5404
> +            list->append(m_repeat);
> +        return list.release();

It looks like this still creates a persistent value for the border-image instead of being a shorthand (i.e., setting the specific properties instead of setting border-image).  In the long run, I think we want this to just set the specific properties, but this is OK for now.

[Alexis Menard (darktears)]

Comment on attachment 122666 [details]
Patch

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

>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:508
>> +            listSlash->append(imageSlices.release());
> 
> The existing test doesn't seem to include test cases for this slash separated list.  Can you add some test cases before this change to make sure we're not breaking anything?
> 
> Hyatt made it sound like getComputedStyle on webkit-border-image may be different from getComputedStyle on border-image.

It seems like according to what I found on google that you can only have one / in the shorthand.

http://developer.apple.com/library/safari/#documentation/appleapplications/reference/SafariCSSRef/Articles/StandardCSSProperties.html

So yes I can add a test case first with the slash to make sure we don't break anything before this patch.

>> Source/WebCore/css/CSSParser.cpp:5404
>> +        return list.release();
> 
> It looks like this still creates a persistent value for the border-image instead of being a shorthand (i.e., setting the specific properties instead of setting border-image).  In the long run, I think we want this to just set the specific properties, but this is OK for now.

Definitively possible as a following patch.

[Alexis Menard (darktears)]

(In reply to comment #9)
> (From update of attachment 122666 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=122666&action=review
> 
> >> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:508
> >> +            listSlash->append(imageSlices.release());
> > 
> > The existing test doesn't seem to include test cases for this slash separated list.  Can you add some test cases before this change to make sure we're not breaking anything?
> > 
> > Hyatt made it sound like getComputedStyle on webkit-border-image may be different from getComputedStyle on border-image.
> 
> It seems like according to what I found on google that you can only have one / in the shorthand.
> 

https://bugs.webkit.org/show_bug.cgi?id=76473 is adding more tests.

> http://developer.apple.com/library/safari/#documentation/appleapplications/reference/SafariCSSRef/Articles/StandardCSSProperties.html
> 
> So yes I can add a test case first with the slash to make sure we don't break anything before this patch.
> 
> >> Source/WebCore/css/CSSParser.cpp:5404
> >> +        return list.release();
> > 
> > It looks like this still creates a persistent value for the border-image instead of being a shorthand (i.e., setting the specific properties instead of setting border-image).  In the long run, I think we want this to just set the specific properties, but this is OK for now.
> 
> Definitively possible as a following patch.

[Alexis Menard (darktears)]

Created attachment 122897 [details]
Patch

[WebKit Review Bot]

Attachment 122897 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebCore/css/CSSStyleSelector.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Tony Chang]

Comment on attachment 122897 [details]
Patch

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

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:505
> +    RefPtr<CSSValueList> list = CSSValueList::createSpaceSeparated();
> +    list->append(imageValue.release());
> +
> +    if (borderSlices || outset) {

This code appears to be duplicated in CSSParser.cpp.  Can we make a helper function for it?  Maybe put it in CSSBorderImageValue.* for now.  In the long run, I think we want to get rid of the code in CSSParser.cpp and then can move the code into CSSComputedStyleDeclaration.cpp.

> Source/WebCore/css/CSSStyleSelector.cpp:4326
> +            // Set the appropriate rules for stretch/round/repeat of the slices

Nit: add a period.

[Alexis Menard (darktears)]

Created attachment 123137 [details]
Patch

[WebKit Review Bot]

Attachment 123137 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebCore/css/CSSStyleSelector.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alexis Menard (darktears)]

(In reply to comment #14)
> Created an attachment (id=123137) [details]
> Patch

Tony, I'm working already on the following patch which keep -webkit-border-image to be a normal CSS property (with a value) as -webkit-border-image doesn't have longhands and "transform" border-image to be a shorthand, i.e. expand and correctly set the longhands.

[Tony Chang]

Comment on attachment 123137 [details]
Patch

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

> Source/WebCore/css/CSSBorderImage.h:31
> +PassRefPtr<CSSValueList> createBorderImageValue(PassRefPtr<CSSValue> image, PassRefPtr<CSSValue> imageSlice, PassRefPtr<CSSValue> borderSlice,
> +                                            PassRefPtr<CSSValue> outset, PassRefPtr<CSSValue> repeat);

Nit: This doesn't look like it's indented properly.

[Alexis Menard (darktears)]

Created attachment 123273 [details]
Patch for landing

[WebKit Review Bot]

Attachment 123273 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebCore/css/CSSStyleSelector.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 123273 [details]
Patch for landing

Clearing flags on attachment: 123273

Committed r105502: <http://trac.webkit.org/changeset/105502>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.