Implement canvas2d direction attribute

Implement canvas2d direction attribute

The attribute, 'direction', represents the text directionality.
This is an important attribute for the bi-directional text within the CanvasRenderingContext2D.
The allowed set of values are 'rtl', 'ltr' and 'inherit'.

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

Intent to implement: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/KWKkTpKiXXM
BUG=277209
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=180984

[vivekg, 2014-08-18 08:09:07]

PTAL, thank you!

[esprehn, 2014-08-18 19:22:44]

You can't use ::computedStyle() without first updating the style of the
document, otherwise the values it reads are stale.

HTMLCanvasElement::getContext would need to call
document().updateStyleIfNeeded() now.

Why do we need to eagerly compute this value though? It seems like something we
should compute on demand, why would the canvas have a value frozen in time from
the first time you call getContext?

https://codereview.chromium.org/468483003/diff/100001/Source/core/html/canvas...
File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):

https://codereview.chromium.org/468483003/diff/100001/Source/core/html/canvas...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:2032: return
(state().m_direction == RTL) ? "rtl" : "ltr";
no parens

https://codereview.chromium.org/468483003/diff/100001/Source/core/html/canvas...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:2035: void
CanvasRenderingContext2D::setDirection(const String& s)
Don't use single letter variable bames.

https://codereview.chromium.org/468483003/diff/100001/Source/core/html/canvas...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:2040: TextDirection
direction = (s == "inherit") ? inheritedDirection(*canvas()) : (s == "rtl") ?
RTL : LTR;
Don't nest ternaries.

[vivekg, 2014-08-18 19:37:40]

On 2014/08/18 at 19:22:44, esprehn wrote:
> Why do we need to eagerly compute this value though? It seems like something
we should compute on demand, why would the canvas have a value frozen in time
from the first time you call getContext?
>
 
The initial value of the direction attribute is 'inherit' i.e. it will be
containing elements' direction attribute as per the spec [1]. Hence we need to
invoke computedStyle for deducing the direction value.

[1]
http://www.whatwg.org/specs/web-apps/current-work/multipage/scripting.html#do...

[vivekg, 2014-08-18 19:38:41]

Thank you @esprehn, I will make all the necessary changes as you suggested.

[vivekg, 2014-08-19 17:40:17]

A friendly ping, thank you.

[esprehn, 2014-08-19 17:46:53]

https://codereview.chromium.org/468483003/diff/120001/Source/core/html/canvas...
File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):

https://codereview.chromium.org/468483003/diff/120001/Source/core/html/canvas...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:2038: direction =
inheritedDirection(*canvas());
This is now how inheritance is supposed to work, it's supposed to be live, so
that if the parent value changes then it changes here too. The getter needs to
resolve inherit into the correct value when reading, not when here in the
setter.

[vivekg, 2014-08-20 08:58:54]

On 2014/08/19 17:46:53, esprehn wrote:
>
https://codereview.chromium.org/468483003/diff/120001/Source/core/html/canvas...
> File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):
> 
>
https://codereview.chromium.org/468483003/diff/120001/Source/core/html/canvas...
> Source/core/html/canvas/CanvasRenderingContext2D.cpp:2038: direction =
> inheritedDirection(*canvas());
> This is now how inheritance is supposed to work, it's supposed to be live, so
> that if the parent value changes then it changes here too. The getter needs to
> resolve inherit into the correct value when reading, not when here in the
> setter.

Done, PTAL. Thank you!

[vivekg, 2014-08-21 12:11:26]

Updated the patch to handle previously missed out save/restore context
operations with respect to the direction.

[Stephen White, 2014-08-21 14:04:38]

https://codereview.chromium.org/468483003/diff/200001/Source/core/html/canvas...
File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):

https://codereview.chromium.org/468483003/diff/200001/Source/core/html/canvas...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:249:
CanvasRenderingContext2D::State::State(Direction direction)
This seems out of keeping with the other State parameters, none of 
which are passed as constructor arguments. What makes Direction 
special?

[vivekg, 2014-08-21 14:05:42]

On 2014/08/21 at 14:04:38, senorblanco wrote:
>
https://codereview.chromium.org/468483003/diff/200001/Source/core/html/canvas...
> File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):
> 
>
https://codereview.chromium.org/468483003/diff/200001/Source/core/html/canvas...
> Source/core/html/canvas/CanvasRenderingContext2D.cpp:249:
CanvasRenderingContext2D::State::State(Direction direction)
> This seems out of keeping with the other State parameters, none of 
> which are passed as constructor arguments. What makes Direction 
> special?

Sorry my bad, it was a previous patch not cleaned up. I will clear this up.

[vivekg, 2014-08-21 14:13:24]

Uploaded cleaned up patch, PTAL! Thanks.

[Stephen White, 2014-08-21 14:17:43]

LGTM, but please wait for esprehn for approval on the CSS bits.

[vivekg, 2014-08-21 14:19:32]

On 2014/08/21 at 14:17:43, senorblanco wrote:
> LGTM, but please wait for esprehn for approval on the CSS bits.

yeah sure, Thank you! :)

[vivekg, 2014-08-22 10:08:20]

Updated the patch slightly to introduce toTextDirection method to avoid code
duplication. PTAL, thank you!

[Stephen White, 2014-08-22 17:45:35]

https://codereview.chromium.org/468483003/diff/260001/Source/core/html/canvas...
File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):

https://codereview.chromium.org/468483003/diff/260001/Source/core/html/canvas...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:2022: TextDirection
CanvasRenderingContext2D::toTextDirection(Direction direction, RenderStyle*&
computedStyle, bool updateRenderTree) const
I'm not fond of non-const references, although they are not banned by the blink
style guide. Would you mind using a RenderStyle** instead, for clarity at the
call site?

https://codereview.chromium.org/468483003/diff/260001/Source/core/html/canvas...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:2188: TextDirection
direction = toTextDirection(state().m_direction, computedStyle, false);
Passing a boolean constant is not self-documenting: "Prefer enums to bools on
function parameters if callers are likely to be passing constants, since named
constants are easier to read at the call site."

Could you just get rid of this argument altogether, and put the code at the call
site where it's needed?

[Justin Novosad, 2014-08-22 18:15:02]

https://codereview.chromium.org/468483003/diff/260001/Source/core/html/canvas...
File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):

https://codereview.chromium.org/468483003/diff/260001/Source/core/html/canvas...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:2022: TextDirection
CanvasRenderingContext2D::toTextDirection(Direction direction, RenderStyle*&
computedStyle, bool updateRenderTree) const
Does Blink really need to have an additional enum (Direction)? RTL vs. Rtl is a
bit unclear. I mean blink already has enough enums for this: WritingDirection,
TextDirection, WebTextDirection.

https://codereview.chromium.org/468483003/diff/260001/Source/core/html/canvas...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:2050: else if
(directionString == "rtl")
This literal is in more than one place. A constant would be appropriate.

[vivekg, 2014-08-26 11:31:52]

vivekg@chromium.org changed reviewers:
+ vivekg@chromium.org

[vivekg, 2014-08-26 11:31:52]

Thank you all for the review. I have updated the patch with the comments
addressed, PTAL.

https://codereview.chromium.org/468483003/diff/260001/Source/core/html/canvas...
File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):

https://codereview.chromium.org/468483003/diff/260001/Source/core/html/canvas...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:2022: TextDirection
CanvasRenderingContext2D::toTextDirection(Direction direction, RenderStyle*&
computedStyle, bool updateRenderTree) const
On 2014/08/22 17:45:35, Stephen White wrote:
> I'm not fond of non-const references, although they are not banned by the
blink
> style guide. Would you mind using a RenderStyle** instead, for clarity at the
> call site?

Done.

https://codereview.chromium.org/468483003/diff/260001/Source/core/html/canvas...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:2022: TextDirection
CanvasRenderingContext2D::toTextDirection(Direction direction, RenderStyle*&
computedStyle, bool updateRenderTree) const
On 2014/08/22 18:15:02, junov wrote:
> Does Blink really need to have an additional enum (Direction)? RTL vs. Rtl is
a
> bit unclear. I mean blink already has enough enums for this: WritingDirection,
> TextDirection, WebTextDirection.

We wanted to be explicit with "inherit" hence ended up adding the Direction enum
which is only visible to CanvasRenderingContext2D being private.

https://codereview.chromium.org/468483003/diff/260001/Source/core/html/canvas...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:2050: else if
(directionString == "rtl")
On 2014/08/22 18:15:02, junov wrote:
> This literal is in more than one place. A constant would be appropriate.

Done.

https://codereview.chromium.org/468483003/diff/260001/Source/core/html/canvas...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:2188: TextDirection
direction = toTextDirection(state().m_direction, computedStyle, false);
On 2014/08/22 17:45:35, Stephen White wrote:
> Passing a boolean constant is not self-documenting: "Prefer enums to bools on
> function parameters if callers are likely to be passing constants, since named
> constants are easier to read at the call site."
> 
> Could you just get rid of this argument altogether, and put the code at the
call
> site where it's needed?

Done.

[Justin Novosad, 2014-08-26 15:01:49]

https://codereview.chromium.org/468483003/diff/320001/LayoutTests/TestExpecta...
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/468483003/diff/320001/LayoutTests/TestExpecta...
LayoutTests/TestExpectations:625: crbug.com/277209
virtual/gpu/fast/canvas/canvas-direction.html [ NeedsRebaseline ]
Why is this needed? You already have an "-expected.txt" file and it is a
dumpAsText test.

https://codereview.chromium.org/468483003/diff/320001/Source/core/html/canvas...
File Source/core/html/canvas/CanvasRenderingContext2D.idl (right):

https://codereview.chromium.org/468483003/diff/320001/Source/core/html/canvas...
Source/core/html/canvas/CanvasRenderingContext2D.idl:162: attribute DOMString
direction; // "inherit", "rtl", "ltr" (default: "inherit")
This feature was only approved for implementation.  You need to hide it behind a
flag until an intent to ship is approved by the API owners on blink-dev. You can
just add [RuntimeEnabled=ExperimentalCanvasFeatures] at the beginning of this
line.

[vivekg, 2014-08-27 06:24:35]

On 2014/08/26 at 15:01:49, junov wrote:
>
https://codereview.chromium.org/468483003/diff/320001/LayoutTests/TestExpecta...
> File LayoutTests/TestExpectations (right):
> 
>
https://codereview.chromium.org/468483003/diff/320001/LayoutTests/TestExpecta...
> LayoutTests/TestExpectations:625: crbug.com/277209
virtual/gpu/fast/canvas/canvas-direction.html [ NeedsRebaseline ]
> Why is this needed? You already have an "-expected.txt" file and it is a
dumpAsText test.
> 

This came from a wrong local branch. Sorry have corrected it now.

>
https://codereview.chromium.org/468483003/diff/320001/Source/core/html/canvas...
> File Source/core/html/canvas/CanvasRenderingContext2D.idl (right):
> 
>
https://codereview.chromium.org/468483003/diff/320001/Source/core/html/canvas...
> Source/core/html/canvas/CanvasRenderingContext2D.idl:162: attribute DOMString
direction; // "inherit", "rtl", "ltr" (default: "inherit")
> This feature was only approved for implementation.  You need to hide it behind
a flag until an intent to ship is approved by the API owners on blink-dev. You
can just add [RuntimeEnabled=ExperimentalCanvasFeatures] at the beginning of
this line.

Done.

[Justin Novosad, 2014-08-27 14:25:33]

lgtm

[vivekg, 2014-08-27 14:35:28]

The CQ bit was checked by vivekg@chromium.org

[vivekg, 2014-08-27 14:35:41]

On 2014/08/27 at 14:25:33, junov wrote:
> lgtm

Thank you.

[commit-bot: I haz the power, 2014-08-27 14:35:55]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vivek.vg@samsung.com/468483003/340001

[commit-bot: I haz the power, 2014-08-27 15:29:31]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_gpu_retina_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_retina_tr...)

[commit-bot: I haz the power, 2014-08-27 15:56:21]

Committed patchset #10 (id:340001) as 180984