Bug 39168

Summary: Canvas: 2d.fillStyle.parse.system.html fails
Product: WebKit Reporter: Chang Shu <cshu>
Component: CanvasAssignee: Andreas Kling <kling>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dglazkov, eric, excors, heldercorreia, jchaffraix, jhanssen, kling, krit, laszlo.gombos, mdelaney7, webkit.review.bot
Priority: P2 Keywords: HTML5
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 20553, 45482    
Attachments:
Description Flags
Proposed patch
darin: review-, darin: commit-queue-
Proposed patch (updated)
none
Proposed patch v3
none
Proposed patch v4
darin: review-, darin: commit-queue-
New iteration: Taking Andreas and Darin's comments
none
Same as previously without the style issue (added #include not in the right order)
none
Another iteration: Added the page NULL-check as suggested by Andreas
none
Another iteration: Same as previously but added a work-around for the lack of 'transparent' color support
none
Proposed patch
none
Proposed patch v2
none
Proposed patch v2 none

Description Chang Shu 2010-05-15 13:22:55 PDT
The above test case fails on all platforms.
Comment 1 Chang Shu 2010-05-15 13:25:40 PDT
The problem of this test case is actually because of ThreeDDarkShadow, which is not supported in WebKit. If changing it to some other named colors, the test case would work. I am wondering if Philip can make a change to the test case. :)
Comment 2 Dirk Schulze 2010-05-24 11:23:14 PDT
(In reply to comment #1)
> The problem of this test case is actually because of ThreeDDarkShadow, which is not supported in WebKit. If changing it to some other named colors, the test case would work. I am wondering if Philip can make a change to the test case. :)

I doubt that he'll make a change, since the test checks what a browser is doing on setting an invalid string to fillStyle :-) And fillStyle (as well as strokeStyle) should ignore this value, we don't do it but replace the current style with the invalid string and validate it later. Thats why we give back transparent black.
Comment 3 Jan Erik Hanssen 2010-06-22 18:40:24 PDT
Created attachment 59458 [details]
Proposed patch
Comment 4 Darin Adler 2010-06-23 09:18:58 PDT
Comment on attachment 59458 [details]
Proposed patch

Looks pretty good, and needs a bit of work.

> -static PassRefPtr<CanvasStyle> toHTMLCanvasStyle(ExecState* exec, JSValue value)
> +static PassRefPtr<CanvasStyle> toHTMLCanvasStyle(ExecState* exec, JSValue value, CanvasRenderingContext2D* ctx)

Please use words, not abbreviations, for argument and variable names in new code. Hence this would be named "context" rather than "ctx".

This is too much code to have in a JavaScript binding function like toHTMLCanvasStyle. The code should be structured so that the conversion from JavaScript data structures to WebCore ones is done quickly, and then a function called that is outside the binding to do the work. The binding handles only JavaScript-specific issues, not language independent ones.

The old code fit this structure, with the function being CanvasStyle::create. The new code still should be calling only a single function that creates a CanvasStyle object. It can be one that takes more arguments and does more work.

In fact, I think all this code belongs inside CanvasStyle::create, which should be changed to take an additional argument.

> +        String colorString(ustringToString(asString(value)->value(exec)));
> +        RefPtr<CanvasStyle> style = CanvasStyle::create(colorString);
> +        if (!style) {

The WebKit project uses early return style. So this would be:

    if (style)
        return style.release();

rather than nesting the next section of the function inside braces.

> +            Page* page = 0;
> +            Document* doc = ctx->canvas()->document();
> +            if (doc)
> +                page = doc->page();

Again, please use words rather than abbreviations. Specifically please use "document" rather than "doc". The way we normally write the code to get to a document and then its page is:

    Document* document = context->canvas()->document();
    Page* page = document ? document->page() : 0;

> +            RefPtr<RenderTheme> theme = RenderTheme::themeForPage(page);

I see, of course, that this test suite includes a test for uses of system colors in canvas. I'm a bit surprised that system colors are supported, though. Are you sure this is the correct behavior? Could you point me to HTML5 canvas documentation that makes this clear?

> +                CSSParserString cssColor;
> +                cssColor.characters = const_cast<UChar*>(colorString.characters());
> +                cssColor.length = colorString.length();
> +                int id = cssValueKeywordID(cssColor);
> +                if (id > 0) {
> +                    Color color = theme->systemColor(id);
> +                    return CanvasStyle::create(color.rgb());
> +                }

The details of how CSS parsing works should be inside the CSSParser class. Perhaps we can add a new function called parseSystemColor. Or a new version of parseColor that handles system colors as well and is given an additional argument. Calling cssValueKeywordID directly is not a good separation of responsibilities here.

> -    int RGBValue;
> +    int ARGBValue;

Since these values are now going to fill all 32 bits I think the type should be unsigned rather than int.
Comment 5 Jan Erik Hanssen 2010-06-23 18:26:07 PDT
(In reply to comment #4)
> (From update of attachment 59458 [details])
> Looks pretty good, and needs a bit of work.

Thanks for the constructive feedback, I'll propose an updated patch shortly.

> I see, of course, that this test suite includes a test for uses of system 
> colors in canvas. I'm a bit surprised that system colors are supported, though. 
> Are you sure this is the correct behavior? Could you point me to HTML5 canvas 
> documentation that makes this clear?

The documentation in question is located at http://philip.html5.org/tests/canvas/suite/tests/spec.html#dom-context-2d-fillstyle specifically in the part that reads "On setting, strings must be parsed as CSS <color> values and the color assigned", and afaik CSS color values includes system colors.
Comment 6 Jan Erik Hanssen 2010-06-23 18:29:07 PDT
Created attachment 59595 [details]
Proposed patch (updated)
Comment 7 Philip Taylor 2010-06-24 04:21:06 PDT
(In reply to comment #2)
> (In reply to comment #1)
> > The problem of this test case is actually because of ThreeDDarkShadow, which is not supported in WebKit. If changing it to some other named colors, the test case would work. I am wondering if Philip can make a change to the test case. :)
> 
> I doubt that he'll make a change, since the test checks what a browser is doing on setting an invalid string to fillStyle :-)

It's not testing invalid strings - it's testing that ThreeDDarkShadow is a valid colour keyword, and that it's not ignored when assigning to fillStyle (by testing that fillStyle doesn't remain as #FF0000 or #ff0000 or #f00 after the assignment).

(This would give an incorrect 'fail' if the browser decided that ThreeDDarkShadow should be a bright red colour, which would be legitimate for the browser but rather improbable.)

HTML5 just says it should be a CSS3 <color> value, which is defined in http://www.w3.org/TR/css3-color/#colorunits . No profile is specified, so I believe all the different <color> types must be supported, which includes the (deprecated) System Colors.

(I don't see this as a useful feature and I wouldn't care if the canvas spec was updated to exclude system colours, but for consistency with HTML and CSS it seems better to include them.)
Comment 8 Jan Erik Hanssen 2010-08-13 13:50:53 PDT
Created attachment 64372 [details]
Proposed patch v3

Completely forgot about this bug, updated the patch to work with the changes made to CanvasRenderingContext2D since the last patch upload.
Comment 9 WebKit Review Bot 2010-08-14 01:37:48 PDT
Attachment 64372 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3744159
Comment 10 Jan Erik Hanssen 2010-08-18 12:13:21 PDT
Created attachment 64752 [details]
Proposed patch v4

Fix a Chrome build issue
Comment 11 Andreas Kling 2010-08-18 13:01:57 PDT
(In reply to comment #10)
> Created an attachment (id=64752) [details]
> Proposed patch v4

This is looking pretty sane, but should be split into two separate changes IMO. (System colors and the "transparent" issue.)
Comment 12 Darin Adler 2010-08-29 13:31:24 PDT
Comment on attachment 64752 [details]
Proposed patch v4

Nice work. Getting closer.

>      if (value->cssValueType() == CSSValue::CSS_PRIMITIVE_VALUE) {
>          CSSPrimitiveValue* primitiveValue = static_cast<CSSPrimitiveValue*>(value);
> -        color = primitiveValue->getRGBA32Value();
> +        if (primitiveValue->primitiveType() == CSSPrimitiveValue::CSS_RGBCOLOR) {
> +            color = primitiveValue->getRGBA32Value();
> +            return true;
> +        }
> +        return false;
>      }
>  
>      return true;

It seems strange that if the value type is not a primitive the function returns true, but if it's a primitive that is not an RGB color it returns false. Is that correct? Are both code paths covered by a test case?

Our normal style is to use early return, rather than nested ifs.

> +bool CSSParser::parseSystemColor(RGBA32& color, const String& string, PassRefPtr<RenderTheme> theme)

The theme argument should be a raw pointer, not a PassRefPtr, because this function uses the theme, but does not take ownership of it.

> +    RefPtr<RenderTheme> renderTheme(theme);

And there's no need for this local variable.

> +    int id = cssValueKeywordID(cssColor);
> +    if (id > 0) {
> +        color = renderTheme->systemColor(id).rgb();
> +        return true;
> +    }
> +    return false;

We'd normally do early return here, rather than nesting the success case inside an if statement.

> +        static bool parseSystemColor(RGBA32& color, const String&, PassRefPtr<RenderTheme> theme);

There's no need for the argument names "color and "theme" here. The types make them clear without names.

> +    Document* document = canvas()->document();
> +    Page* page = document ? document->page() : 0;
> +    RefPtr<RenderTheme> theme = RenderTheme::themeForPage(page);
> +    setStrokeStyle(CanvasStyle::createFromString(color, theme.release()));

Creating a new RenderTheme here every time this function is called is a massive performance mistake that will make our canvas a lot slower. What we need to do is call page->theme() instead.

> +PassRefPtr<CanvasStyle> CanvasStyle::createFromString(const String& color, PassRefPtr<RenderTheme> theme)

This argument should be a raw pointer, not a PassRefPtr. This function does not take ownership of a theme.

For best performance, the functions should not go from the Document* to the theme. Instead, we should pass a Document* in rather than a RenderTheme*. The code to go from the document to the page and then to the theme should be inside the CSSParser::parseSystemColor function.

> -    int RGBValue;
> +    unsigned int ARGBValue;

We use just "unsigned", not "unsigned int". Another possibility would be "uint32_t".
Comment 13 Julien Chaffraix 2010-09-01 21:29:07 PDT
Taking the bug as I started to work on a fix (taking Darin's comments into account).

I intend to split the 'transparent' part as suggested by Andrea. This would enable testing both changes as CSSParser::parseColor would not lie about whether it parsed the color or not.
Comment 14 Julien Chaffraix 2010-09-07 16:53:51 PDT
Created attachment 66790 [details]
New iteration: Taking Andreas and Darin's comments
Comment 15 WebKit Review Bot 2010-09-07 16:55:52 PDT
Attachment 66790 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/css/CSSParser.cpp:67:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 16 Julien Chaffraix 2010-09-07 17:34:05 PDT
Created attachment 66803 [details]
Same as previously without the style issue (added #include not in the right order)
Comment 17 Andreas Kling 2010-09-10 02:59:39 PDT
Comment on attachment 66803 [details]
Same as previously without the style issue (added #include not in the right order)

> +    color = document->page()->theme()->systemColor(id).rgb();

There's no way page() can be NULL here?
Comment 18 Julien Chaffraix 2010-09-10 18:50:45 PDT
> > +    color = document->page()->theme()->systemColor(id).rgb();
> 
> There's no way page() can be NULL here?

You are right. I thought we always had a frame - thus a page - but it looks like there is some case where this is not the case. I will modify the code.
Comment 19 Adam Barth 2010-09-10 19:07:37 PDT
> You are right. I thought we always had a frame - thus a page - but it looks like there is some case where this is not the case. I will modify the code.

Note that, in theory, Frame can outlive Page.
Comment 20 Julien Chaffraix 2010-09-10 19:11:09 PDT
Created attachment 67286 [details]
Another iteration: Added the page NULL-check as suggested by Andreas
Comment 21 Andreas Kling 2010-09-11 07:26:51 PDT
Comment on attachment 67286 [details]
Another iteration: Added the page NULL-check as suggested by Andreas

This looks very good.
r=me
Comment 22 WebKit Review Bot 2010-09-15 15:57:03 PDT
http://trac.webkit.org/changeset/67570 might have broken Qt Linux Release
Comment 23 Julien Chaffraix 2010-09-15 16:00:15 PDT
(In reply to comment #22)
> http://trac.webkit.org/changeset/67570 might have broken Qt Linux Release

Indeed, reverted the patch in r67574 as it also broke those 2 tests on Gtk:

- fast/canvas/canvas-color-serialization.html
- fast/canvas/set-colors.html
Comment 24 Julien Chaffraix 2010-09-15 16:01:11 PDT
Comment on attachment 67286 [details]
Another iteration: Added the page NULL-check as suggested by Andreas

Clearing the review flag while I investigate the failing tests.
Comment 25 Eric Seidel (no email) 2010-09-15 17:02:28 PDT
Bots appear still borked.
Comment 26 Julien Chaffraix 2010-09-22 19:16:12 PDT
Created attachment 68492 [details]
Another iteration: Same as previously but added a work-around for the lack of 'transparent' color support

> Bots appear still borked.

This was due to a bad revert on my side. I will use sheriff-bot next time, sorry for the breakage...
Comment 27 Andreas Kling 2010-10-06 23:00:39 PDT
Comment on attachment 68492 [details]
Another iteration: Same as previously but added a work-around for the lack of 'transparent' color support

Removing from review queue.
Comment 28 Andreas Kling 2011-02-13 10:10:25 PST
Created attachment 82267 [details]
Proposed patch

Updated patch based on the work by Julien and Jan-Erik.
Comment 29 Andreas Kling 2011-02-13 13:11:22 PST
Created attachment 82273 [details]
Proposed patch v2

Updated with adjustments from Dirk Schulze (IRC):
- Pass Document* rather than HTMLCanvasElement* to parseColor().
- Avoid unnecessary RenderTheme.h include in CanvasRenderingContext2D.cpp.
Comment 30 Andreas Kling 2011-02-13 13:13:06 PST
Created attachment 82274 [details]
Proposed patch v2
Comment 31 Dirk Schulze 2011-02-13 13:34:38 PST
Comment on attachment 82274 [details]
Proposed patch v2

r=me
Comment 32 Andreas Kling 2011-02-13 13:41:28 PST
Comment on attachment 82274 [details]
Proposed patch v2

Clearing flags on attachment: 82274

Committed r78442: <http://trac.webkit.org/changeset/78442>
Comment 33 Andreas Kling 2011-02-13 13:41:39 PST
All reviewed patches have been landed.  Closing bug.
Comment 34 WebKit Review Bot 2011-02-13 15:37:09 PST
http://trac.webkit.org/changeset/78442 might have broken SnowLeopard Intel Release (Tests)
The following tests are not passing:
canvas/philip/tests/2d.fillStyle.parse.system.html
Comment 35 Andreas Kling 2011-02-13 16:25:48 PST
(In reply to comment #34)
> http://trac.webkit.org/changeset/78442 might have broken SnowLeopard Intel Release (Tests)
> The following tests are not passing:
> canvas/philip/tests/2d.fillStyle.parse.system.html

Fix landed in <http://trac.webkit.org/changeset/78446> (mac platform was tracking failures.)