Bug 39168 – Canvas: 2d.fillStyle.parse.system.html fails

• Status: RESOLVED
• Resolution: FIXED
• Component: Canvas

[Chang Shu]

The above test case fails on all platforms.

[Chang Shu]

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. :)

[Dirk Schulze]

(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.

[Jan Erik Hanssen]

Created attachment 59458 [details]
Proposed patch

[Darin Adler]

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.

[Jan Erik Hanssen]

(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.

[Jan Erik Hanssen]

Created attachment 59595 [details]
Proposed patch (updated)

[Philip Taylor]

(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.)

[Jan Erik Hanssen]

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.

[WebKit Review Bot]

Attachment 64372 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3744159

[Jan Erik Hanssen]

Created attachment 64752 [details]
Proposed patch v4

Fix a Chrome build issue

[Andreas Kling]

(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.)

[Darin Adler]

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".

[Julien Chaffraix]

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.

[Julien Chaffraix]

Created attachment 66790 [details]
New iteration: Taking Andreas and Darin's comments

[WebKit Review Bot]

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.

[Julien Chaffraix]

Created attachment 66803 [details]
Same as previously without the style issue (added #include not in the right order)

[Andreas Kling]

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?

[Julien Chaffraix]

> > +    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.

[Adam Barth]

> 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.

[Julien Chaffraix]

Created attachment 67286 [details]
Another iteration: Added the page NULL-check as suggested by Andreas

[Andreas Kling]

Comment on attachment 67286 [details]
Another iteration: Added the page NULL-check as suggested by Andreas

This looks very good.
r=me

[WebKit Review Bot]

http://trac.webkit.org/changeset/67570 might have broken Qt Linux Release

[Julien Chaffraix]

(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

[Julien Chaffraix]

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.

[Eric Seidel (no email)]

Bots appear still borked.

[Julien Chaffraix]

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...

[Andreas Kling]

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.

[Andreas Kling]

Created attachment 82267 [details]
Proposed patch

Updated patch based on the work by Julien and Jan-Erik.

[Andreas Kling]

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.

[Andreas Kling]

Created attachment 82274 [details]
Proposed patch v2

[Dirk Schulze]

Comment on attachment 82274 [details]
Proposed patch v2

r=me

[Andreas Kling]

Comment on attachment 82274 [details]
Proposed patch v2

Clearing flags on attachment: 82274

Committed r78442: <http://trac.webkit.org/changeset/78442>

[Andreas Kling]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

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

[Andreas Kling]

(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.)