[Webkit-unassigned] [Bug 39168] http://philip.html5.org/tests/canvas/suite/tests/2d.fillStyle.parse.system.html fails

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=39168


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #59458|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #4 from Darin Adler <darin at apple.com>  2010-06-23 09:18:58 PST ---
(From update of attachment 59458)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.