[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 #64752|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #12 from Darin Adler <darin at apple.com>  2010-08-29 13:31:24 PST ---
(From update of attachment 64752)
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".

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