[webkit-reviews] review granted: [Bug 136098] Canvas direction should reflect change in dir attribute and also across save/restore operations : [Attachment 236926] 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 136098: Canvas direction should reflect change in dir attribute and also
across save/restore operations
https://bugs.webkit.org/show_bug.cgi?id=136098

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

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


> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2181
> +    TextDirection direction;
> +    switch (state().m_direction) {
> +    case DirectionInherit:
> +	   direction = inheritedDirection(*canvas());
> +	   break;
> +    case DirectionRTL:
> +	   direction = RTL;
> +	   break;
> +    case DirectionLTR:
> +	   direction = LTR;
> +	   break;
> +    default:
> +	   ASSERT_NOT_REACHED();
> +	   break;
> +    }

Would be nicer to share this switch statement with drawTextInternal. But it
might require too much rearranging to do that cleanly.

Also, best pattern for such switch statements is to not have a default. The way
we do that is to put the switch into a function, use return, and then put the
ASSERT_NOT_REACHED after the switch statement. This makes it so that if we add
a new enumeration value, we get a compile time warning rather than just a
runtime assertion.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.h:231
> +    enum Direction {

Why not use "enum class" instead of enum? We are doing that in most new code, I
believe.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.h:232
> +	   DirectionInherit = -1,

Seems unnecessary to give this a special value of -1. We should just let the
enum values be the default ones starting with zero; no reason not to!