[webkit-reviews] review denied: [Bug 135878] Implement CanvasRenderingContext2D direction attribute : [Attachment 236575] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Aug 13 22:36:34 PDT 2014


Darin Adler <darin at apple.com> has denied 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 236575: Patch
https://bugs.webkit.org/attachment.cgi?id=236575&action=review

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


More information about the webkit-reviews mailing list