Bug 135878 – Implement CanvasRenderingContext2D direction attribute

• Status: RESOLVED
• Resolution: FIXED
• Component: Canvas

[Vivek Galatage]

WIP: Implement CanvasRenderingContext2D direction attribute

[Vivek Galatage]

Created attachment 236518 [details]
Patch

[Vivek Galatage]

Created attachment 236575 [details]
Patch

[Vivek Galatage]

Please take a look, thank you!

[Vivek Galatage]

Link to WHATWG specificiation URL: http://www.whatwg.org/specs/web-apps/current-work/multipage/scripting.html#dom-context-2d-direction

[Darin Adler]

Comment on attachment 236575 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=236575&action=review

If you’re going to do all that whitespace trimming, maybe that can be a separate patch that you land first. It’s not great to combine that with the substantive change.

Test coverage is not sufficient. There are bugs in the code that the tests did not detect.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:107
> +static bool parseDirection(const String& direction)

A function that does the isValidDirection operation should not be named “parse” because it doesn’t do the parsing. It doesn’t return the value of what’s parsed. So please don’t name it “parse”. But also, it doesn’t make sense to have this function at all. The code just below the one call site for this function handles all the three direction values anyway, so this function should not be added.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:109
> +    return direction == "inherit" ? true : direction == "rtl" ? true : direction == "ltr" ? true : false;

The || operator does the same thing as "? true :" so you should use that:

    return direction == "inherit" || direction == "rtl" || direction == "ltr";

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:121
> +    setDirection("inherit");

We don’t want to pass ourselves a string that we need to parse. We should find another way to share the code that reads the direction from the canvas and write it into the state. Probably simply:

    m_stateStack.first().m_direction = inheritedDirection(*canvas);

The inheritedDirection function would take an HTMLCanvasElement& and also be used in the setDirection function.

This work needs to be done in CanvasRenderingContext2D::reset as well, not just in the constructor. Need test coverage for the behavior of reset.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2169
> +    if (!parseDirection(direction))
> +        return;

There’s no need for this. Instead the function should compute the direction first, returning if the direction string is invalid. Then it can call realizeSaves, and only then it can set modifiableState().m_direction.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2173
> +        modifiableState().m_direction = canvas()->computedStyle() ? canvas()->computedStyle()->direction() : LTR;

The computedStyle function does a lot of work. It’s not a good idea to call it twice. Please use a local variable.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.h:263
> +        TextDirection m_direction;

The default constructor for State needs to initialize this. We don’t want to leave it uninitialized. The copy constructor for State and the assignment operator for state need to copy this.

> LayoutTests/ChangeLog:15
> +        * fast/canvas/canvas-direction-inherited-rtl-expected.txt: Added.
> +        * fast/canvas/canvas-direction-inherited-rtl.html: Added.
> +        * fast/canvas/canvas-direction-inherited-unspecified-expected.txt: Added.
> +        * fast/canvas/canvas-direction-inherited-unspecified.html: Added.
> +        * fast/canvas/canvas-direction-rtl-override-with-ltr-expected.txt: Added.
> +        * fast/canvas/canvas-direction-rtl-override-with-ltr.html: Added.
> +        * fast/canvas/canvas-direction-unspecified-override-with-rtl-expected.txt: Added.
> +        * fast/canvas/canvas-direction-unspecified-override-with-rtl.html: Added.

These should not all be separate tests. You should combine them into a single test.

Also, we need more test coverage. For example, we need to test invalid values such as "INHERITED", "RTL", and "LTR" to make sure that the handling of direction is case sensitive.

[Vivek Galatage]

Created attachment 236594 [details]
Patch

[Vivek Galatage]

Thank you Darin for the review. Have updated the patch with all your comments incorporated. Please take a look.

[Vivek Galatage]

(In reply to comment #7)
> Thank you Darin for the review. Have updated the patch with all your comments incorporated. Please take a look.

A friendly ping! Thank you.

[Darin Adler]

Comment on attachment 236594 [details]
Patch

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.

[Vivek Galatage]

Created attachment 236778 [details]
Patch

[Darin Adler]

Comment on attachment 236778 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=236778&action=review

> Source/WebCore/html/canvas/CanvasRenderingContext2D.h:232
> +        State(TextDirection = LTR);

Might be ever so slightly better to put an "explicit" here.

[Vivek Galatage]

Created attachment 236780 [details]
Patch

[Vivek Galatage]

(In reply to comment #11)
> (From update of attachment 236778 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=236778&action=review
> 
> > Source/WebCore/html/canvas/CanvasRenderingContext2D.h:232
> > +        State(TextDirection = LTR);
> 
> Might be ever so slightly better to put an "explicit" here.

Thank you very much for your help and time for the quick reviews :)

[WebKit Commit Bot]

Comment on attachment 236780 [details]
Patch

Clearing flags on attachment: 236780

Committed r172723: <http://trac.webkit.org/changeset/172723>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.