[webkit-reviews] review denied: [Bug 136098] CanvasRenderingContext2D direction should reflect change in Element's dir attribute : [Attachment 236862] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Vivek Galatage <vivekg at webkit.org>'s
request for review:
Bug 136098: CanvasRenderingContext2D direction should reflect change in
Element's dir attribute
https://bugs.webkit.org/show_bug.cgi?id=136098

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

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


Doesn’t work right with save/restore. After this sequence:

    context.direction = "inherit";
    context.save();
    context.direction = "rtl";
    context.restore();

the context will be in non-inherited state, but should be in inherited state.
The correct way to fix this is to change m_direction in State to have a
separate state for inherit, so perhaps not be a TextDirection at all, or to add
a boolean there, or have it be an Optional<TextDirection> or something like
that.

Need test cases that cover the behavior with save/restore.

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

This is no longer a useful thing to do. It wastefully computes a direction that
will never be used since drawTextInternal will re-compute the direction every
time.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2294
> +    if (m_hasInheritedDirection && computedStyle)

If computedStyle is null then we should use LTR, not the direction that already
happens to be in the state.

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

It’s not helpful to store a new direction in the context, since we just have to
recompute it every time anyway.