[webkit-reviews] review granted: [Bug 135878] Implement CanvasRenderingContext2D direction attribute : [Attachment 236594] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Vivek Galatage <vivekg at webkit.org>'s
request for review:
Bug 135878: Implement CanvasRenderingContext2D direction attribute
https://bugs.webkit.org/show_bug.cgi?id=135878

Attachment 236594: Patch
https://bugs.webkit.org/attachment.cgi?id=236594&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=236594&action=review


review+ but please don’t check in those PNG files

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:165
>      m_stateStack.first() = State();
> +    m_stateStack.first().m_direction = inheritedDirection(*canvas());

Might be cleaner to have a State constructor that takes a TextDirection to make
these two be a single line.

Using constructor delegation we could have the default constructor call the one
that takes a TextDirection, so we wouldn’t have to repeat them. Or we could
simply have the TextDirection argument have a default value of LTR and have a
single constructor be both the default one and the one that takes
TextDirection.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2168
> +    return (state().m_direction == RTL) ? "rtl" : "ltr";

For better performance it should be ASCIILiteral("rtl") : ASCIILiteral("ltr");

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2171
> +void CanvasRenderingContext2D::setDirection(const String& s)

Would be nicer to name the argument "string" or "directionString" rather than
just the letter "s". We frown on the use of letters instead of words for
variable names.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2176
> +    if (s != "inherit" && s != "rtl" && s != "ltr")
> +	   return;
> +
> +    TextDirection direction = (s == "inherit") ?
inheritedDirection(*canvas()) : (s == "rtl") ? RTL : LTR;

Would be nicer to combine the direction computation with the validation:

    if (s == "inherit")
	direction = inheritedDirection(*canvas());
    else if (s == "rtl")
	direction = RTL;
    else if (s == "ltr")
	direction = LTR;
    else
	return;

That way we do only 3 string equality comparisons instead of 5.

> LayoutTests/ChangeLog:11
> +	   * fast/canvas/canvas-direction-expected.txt: Added.
> +	   * fast/canvas/canvas-direction.html: Added.
> +	   * platform/gtk/fast/canvas/canvas-direction-expected.png: Added.
> +	   * platform/mac/fast/canvas/canvas-direction-expected.png: Added.

I don’t understand why we have the expected.png files here. If this is a
text-only test, no png file should be written out or read.

> LayoutTests/fast/canvas/canvas-direction.html:53
> +  testRunner.dumpAsText(true);

I don’t think the dumpAsText function takes an argument, does it? Should indent
by 4 spaces, not 2.