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

[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 59458: Proposed patch
https://bugs.webkit.org/attachment.cgi?id=59458&action=review

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