[webkit-reviews] review granted: [Bug 136098] Canvas direction should reflect change in dir attribute and also across save/restore operations : [Attachment 236966] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Aug 22 09:06:17 PDT 2014


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 236966: Patch
https://bugs.webkit.org/attachment.cgi?id=236966&action=review

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


If this becomes a performance problem we might some day want to add code to
cache the inherited direction. It’s not trivial invalidate such a cache
correctly, but the extra cost of updating style and calling computedStyle every
time we draw text might make canvas text drawing slow enough that such an
optimization would be worth while.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2158
> +TextDirection CanvasRenderingContext2D::toTextDirection(Direction direction,
RenderStyle*& computedStyle) const

The inline keyword needs to be here on the definition, rather than in the
header file on the declaration.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2161
> +    canvas()->document().updateStyleIfNeeded();
> +    computedStyle = canvas()->computedStyle();

This work is wasteful when 1) the caller is not going to use computedStyle and
2) the direction is not Inherit. One way to fix that is to change the
computedStyle out argument to make it optional; change it to a RenderStyle**
with a default of nullptr. It would also complicate the logic a bit but it
would make the direction getter significantly more efficient when the direction
is set explicitly to LTR or RTL.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2176
> +    RenderStyle* computedStyle = nullptr;

I don’t think you need to initialize this, since it’s an out parameter.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2295
> +    RenderStyle* computedStyle = nullptr;

Ditto.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.h:234
> +	   Rtl,
> +	   Ltr

These should be all capitals. We capitalize acronyms.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.h:348
> +    inline TextDirection toTextDirection(Direction, RenderStyle*&
computedStyle) const;

The inline keyword should be omitted here. If you do a little research you’ll
find that information in C++ documentation.


More information about the webkit-reviews mailing list