Bug 77001

Summary: border-image should not crash when the source is not specified.
Product: WebKit Reporter: Alexis Menard (darktears) <menard>
Component: New BugsAssignee: Alexis Menard (darktears) <menard>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, kling, koivisto, macpherson, tony, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 76876    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch for landing none

Description Alexis Menard (darktears) 2012-01-25 03:24:53 PST
border-image should not crash when the source is not specified.
Comment 1 Alexis Menard (darktears) 2012-01-25 03:33:48 PST
Created attachment 123913 [details]
Patch
Comment 2 Andreas Kling 2012-01-25 08:02:51 PST
Comment on attachment 123913 [details]
Patch

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

With this change, mapNinePieceImage() becomes very liberal in which argument order it accepts. Are you sure this won't lead to situations where incorrect values slip through the crack?

> Source/WebCore/ChangeLog:10
> +        may happen that you have no value set for it. CSSBorderImage::createBorderImageValue

There is no CSSBorderImage class, the method is simply WebCore::createBorderImageValue.

> Source/WebCore/css/CSSStyleSelector.cpp:4322
> +        if (current->isPrimitiveValue() && !current->isImageValue() && !current->isImageGeneratorValue())

If isPrimitiveValue() is true, isImageGeneratorValue() is going to be false, no need to check the latter.
Comment 3 Alexis Menard (darktears) 2012-01-25 08:42:09 PST
Created attachment 123946 [details]
Patch
Comment 4 Alexis Menard (darktears) 2012-01-25 08:48:08 PST
(In reply to comment #2)
> (From update of attachment 123913 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=123913&action=review
> 
> With this change, mapNinePieceImage() becomes very liberal in which argument order it accepts. Are you sure this won't lead to situations where incorrect values slip through the crack?

Well before r105502

http://trac.webkit.org/changeset/105502/trunk/Source/WebCore/css/CSSStyleSelector.cpp

It was already liberal on the order as it was just taking the CSSBorderImageValue object as parameter.

> 
> > Source/WebCore/ChangeLog:10
> > +        may happen that you have no value set for it. CSSBorderImage::createBorderImageValue
> 
> There is no CSSBorderImage class, the method is simply WebCore::createBorderImageValue.

Fixed.

> 
> > Source/WebCore/css/CSSStyleSelector.cpp:4322
> > +        if (current->isPrimitiveValue() && !current->isImageValue() && !current->isImageGeneratorValue())
> 
> If isPrimitiveValue() is true, isImageGeneratorValue() is going to be false, no need to check the latter.

Good catch. Fixed.
Comment 5 Alexis Menard (darktears) 2012-01-25 09:09:26 PST
Created attachment 123951 [details]
Patch
Comment 6 Andreas Kling 2012-01-25 09:12:29 PST
Comment on attachment 123951 [details]
Patch

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

Looks good, r=me.

> Source/WebCore/css/CSSStyleSelector.cpp:4320
> +        } else if (current->isPrimitiveValue())
> +            // Set the appropriate rules for stretch/round/repeat of the slices.
> +            mapNinePieceImageRepeat(current, image);

Please add { } here, since the comment makes it two lines.
Comment 7 Alexis Menard (darktears) 2012-01-25 09:14:16 PST
Created attachment 123953 [details]
Patch for landing
Comment 8 WebKit Review Bot 2012-01-25 10:54:56 PST
Comment on attachment 123953 [details]
Patch for landing

Clearing flags on attachment: 123953

Committed r105898: <http://trac.webkit.org/changeset/105898>
Comment 9 WebKit Review Bot 2012-01-25 10:55:01 PST
All reviewed patches have been landed.  Closing bug.