[webkit-reviews] review denied: [Bug 39168] http://philip.html5.org/tests/canvas/suite/tests/2d.fillStyle.parse.system.html fails : [Attachment 64752] Proposed patch v4

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Jan Erik Hanssen
<jhanssen at codeaurora.org>'s request for review:
Bug 39168:
http://philip.html5.org/tests/canvas/suite/tests/2d.fillStyle.parse.system.html
fails
https://bugs.webkit.org/show_bug.cgi?id=39168

Attachment 64752: Proposed patch v4
https://bugs.webkit.org/attachment.cgi?id=64752&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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".