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.
I'll take that one if nobody object.
Created attachment 122661 [details] Patch
Comment on attachment 122661 [details] Patch Attachment 122661 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11254242
Comment on attachment 122661 [details] Patch Attachment 122661 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/11255247
Created attachment 122666 [details] Patch
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.
(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.
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.
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.
(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.
Created attachment 122897 [details] Patch
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.
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.
Created attachment 123137 [details] Patch
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.
(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.
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.
Created attachment 123273 [details] Patch for landing
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.
Comment on attachment 123273 [details] Patch for landing Clearing flags on attachment: 123273 Committed r105502: <http://trac.webkit.org/changeset/105502>
All reviewed patches have been landed. Closing bug.