Support currentTransform in 2D Canvas.

Support currentTransform in 2D Canvas.

Canvas v5 has a currentTransform that is an SVGMatrix.
http://www.whatwg.org/specs/web-apps/current-work/multipage/the-canvas-element.html#dom-context-2d-currenttransform

[Immutable] extended attribute is used, because canvas must not change CTM in
the following case.
    var matrix = context.currentTransform;
    matrix.a = 2;

BUG=277107
TEST=fast/canvas/canvas-currentTransform.html

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=159303

[Justin Novosad, 2013-09-19 19:23:48]

https://codereview.chromium.org/24233004/diff/1/LayoutTests/fast/canvas/scrip...
File LayoutTests/fast/canvas/script-tests/canvas-currentTransform2.js (right):

https://codereview.chromium.org/24233004/diff/1/LayoutTests/fast/canvas/scrip...
LayoutTests/fast/canvas/script-tests/canvas-currentTransform2.js:25: matrix =
ctx.currentTransform;
Should use a different variable here to test that the assignment is actually
doing something.

https://codereview.chromium.org/24233004/diff/1/LayoutTests/fast/canvas/scrip...
LayoutTests/fast/canvas/script-tests/canvas-currentTransform2.js:43: matrix =
ctx.currentTransform;
Same here

https://codereview.chromium.org/24233004/diff/1/LayoutTests/fast/canvas/scrip...
LayoutTests/fast/canvas/script-tests/canvas-currentTransform2.js:67: matrix =
ctx.currentTransform;
Same here.

https://codereview.chromium.org/24233004/diff/1/Source/core/html/canvas/Canva...
File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):

https://codereview.chromium.org/24233004/diff/1/Source/core/html/canvas/Canva...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:67: #include
"core/svg/SVGMatrix.h"
Are you sure this include is necessary?

https://codereview.chromium.org/24233004/diff/1/Source/core/html/canvas/Canva...
File Source/core/html/canvas/CanvasRenderingContext2D.h (right):

https://codereview.chromium.org/24233004/diff/1/Source/core/html/canvas/Canva...
Source/core/html/canvas/CanvasRenderingContext2D.h:121: SVGMatrix
currentTransform() const;
IIRC, you can't return an incomplete type.  If this code compiles as is, you
probably don't need the above forward declaration. Also, it is returning by
value, which may not be the best thing to do here.  Maybe you could make this
function an inline so that the compiler can optimize-out the transient copy of
the return value. http://en.wikipedia.org/wiki/Return_value_optimization

[Justin Novosad, 2013-09-19 19:30:07]

https://codereview.chromium.org/24233004/diff/10001/Source/core/html/canvas/C...
File Source/core/html/canvas/CanvasRenderingContext2D.h (right):

https://codereview.chromium.org/24233004/diff/10001/Source/core/html/canvas/C...
Source/core/html/canvas/CanvasRenderingContext2D.h:121: SVGMatrix
currentTransform() const;
Would you still need the custom binding if this returned a
PassRefPtr<SVGMatrix>, which would imply making SVGMatrix RefCounted.  I'm not
saying that's the right thing to do. I'm just asking if that was tried or
considered

[dshwang, 2013-09-19 19:47:03]

Thank you for review :)

On 2013/09/19 19:23:48, junov wrote:
>
https://codereview.chromium.org/24233004/diff/1/LayoutTests/fast/canvas/scrip...
> File LayoutTests/fast/canvas/script-tests/canvas-currentTransform2.js (right):
> 
>
https://codereview.chromium.org/24233004/diff/1/LayoutTests/fast/canvas/scrip...
> LayoutTests/fast/canvas/script-tests/canvas-currentTransform2.js:25: matrix =
> ctx.currentTransform;
> Should use a different variable here to test that the assignment is actually
> doing something.

I add following code to check the assignment correctly.
    matrix.a = NaN;
    matrix.b = NaN;
    matrix.c = NaN;
    matrix.d = NaN;
    matrix.e = NaN;
    matrix.f = NaN;

 
>
https://codereview.chromium.org/24233004/diff/1/Source/core/html/canvas/Canva...
> File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):
> 
>
https://codereview.chromium.org/24233004/diff/1/Source/core/html/canvas/Canva...
> Source/core/html/canvas/CanvasRenderingContext2D.cpp:67: #include
> "core/svg/SVGMatrix.h"
> Are you sure this include is necessary?
> 
>
https://codereview.chromium.org/24233004/diff/1/Source/core/html/canvas/Canva...
> File Source/core/html/canvas/CanvasRenderingContext2D.h (right):
> 
>
https://codereview.chromium.org/24233004/diff/1/Source/core/html/canvas/Canva...
> Source/core/html/canvas/CanvasRenderingContext2D.h:121: SVGMatrix
> currentTransform() const;
> IIRC, you can't return an incomplete type.  If this code compiles as is, you
> probably don't need the above forward declaration. Also, it is returning by
> value, which may not be the best thing to do here.  Maybe you could make this
> function an inline so that the compiler can optimize-out the transient copy of
> the return value. http://en.wikipedia.org/wiki/Return_value_optimization

Good to know. Done.


On 2013/09/19 19:30:07, junov wrote:
>
https://codereview.chromium.org/24233004/diff/10001/Source/core/html/canvas/C...
> File Source/core/html/canvas/CanvasRenderingContext2D.h (right):
> 
>
https://codereview.chromium.org/24233004/diff/10001/Source/core/html/canvas/C...
> Source/core/html/canvas/CanvasRenderingContext2D.h:121: SVGMatrix
> currentTransform() const;
> Would you still need the custom binding if this returned a
> PassRefPtr<SVGMatrix>, which would imply making SVGMatrix RefCounted.  I'm not
> saying that's the right thing to do. I'm just asking if that was tried or
> considered

Interesting idea!
measureText() returns PassRefPtr.
in idl
TextMetrics measureText(DOMString text);
in cpp
PassRefPtr<TextMetrics> measureText(const String& text);

to do, TextMetrics needs to extend ScriptWrappable.
class TextMetrics : public RefCounted<TextMetrics>, public ScriptWrappable {}

I'm not sure whether it is possible or not. If I use ImplementedAs custom idl
attribute, it may be possible.
I'm surveying it now.

[Justin Novosad, 2013-09-19 19:52:05]

Thanks for investigating deeper.  I think it is very inconvenient that we would
need custom bindings for such a simple case.  It would be great to fix this once
and for all.

[dshwang, 2013-09-19 20:19:33]

On 2013/09/19 19:47:03, dshwang wrote:
>
https://codereview.chromium.org/24233004/diff/1/Source/core/html/canvas/Canva...
> > Source/core/html/canvas/CanvasRenderingContext2D.h:121: SVGMatrix
> > currentTransform() const;
> > IIRC, you can't return an incomplete type.  If this code compiles as is, you
> > probably don't need the above forward declaration. Also, it is returning by
> > value, which may not be the best thing to do here.  Maybe you could make
this
> > function an inline so that the compiler can optimize-out the transient copy
of
> > the return value. http://en.wikipedia.org/wiki/Return_value_optimization
> 
> Good to know. Done.
> 

My OS is ubuntu 13.04 and I use gcc (Ubuntu/Linaro 4.7.3-1ubuntu1) 4.7.3
Unfortunately, gcc 4.7.3 does not make Return value optimization in this case.
There is one SVGMatrix copy construction regardless of this change.

[dshwang, 2013-09-19 20:30:59]

@ch.dumez Could you advise me how to avoid custom binding in this case?

[do-not-use, 2013-09-20 05:01:22]

On 2013/09/19 20:30:59, dshwang wrote:
> @ch.dumez Could you advise me how to avoid custom binding in this case?
 
The bindings generator already has special casing for SVGPropertyTearOff so it
should work with minimal work. I am out of office so I cannot look in into it
now. I'll try to find some time. Otherwise haraken likely knows.

[dshwang, 2013-09-20 05:06:33]

On 2013/09/20 05:01:22, Christophe Dumez (OOO) wrote:
> On 2013/09/19 20:30:59, dshwang wrote:
> > @ch.dumez Could you advise me how to avoid custom binding in this case?
>  
> The bindings generator already has special casing for SVGPropertyTearOff so it
> should work with minimal work. I am out of office so I cannot look in into it
> now. I'll try to find some time. Otherwise haraken likely knows.

@ch.dumez thx for explanation.

@haraken Could you advise me how to avoid custom binding in this case?

[haraken, 2013-09-20 07:07:19]

https://codereview.chromium.org/24233004/diff/23001/Source/core/html/canvas/C...
File Source/core/html/canvas/CanvasRenderingContext2D.idl (right):

https://codereview.chromium.org/24233004/diff/23001/Source/core/html/canvas/C...
Source/core/html/canvas/CanvasRenderingContext2D.idl:33:
[EnabledAtRuntime=ExperimentalCanvasFeatures, Custom] attribute SVGMatrix
currentTransform;

What code is generated if you omit [Custom]? How is the generated code different
from the custom binding code you wrote?

I think you need to search "TearOff" in code_generator_v8.pm and tweak the code
around it, so that the generator can generate the exact code you want.

[do-not-use, 2013-09-20 07:31:40]

This should hopefully set you in the right direction:
http://pastebin.com/Em6CcZ00

No custom code, it builds and the test passes. I also did not edit the bindings
generators at all.
We should probably do something in updateCurrentTransformation() though... You
can use the SVG code as reference.

[do-not-use, 2013-09-20 07:43:58]

On 2013/09/20 07:31:40, Christophe Dumez (OOO) wrote:
> This should hopefully set you in the right direction:
> http://pastebin.com/Em6CcZ00
> 
> No custom code, it builds and the test passes. I also did not edit the
bindings
> generators at all.
> We should probably do something in updateCurrentTransformation() though... You
> can use the SVG code as reference.

Also, we can probably do something like this in currentTransform():
SVGMatrix& currentTransform()
{
    return static_cast<SVGMatrix&>(modifyableState().m_transform);
}

To avoid the local member. Feel free to experiment :)

[do-not-use, 2013-09-20 08:25:37]

On 2013/09/20 07:43:58, Christophe Dumez (OOO) wrote:
> On 2013/09/20 07:31:40, Christophe Dumez (OOO) wrote:
> > This should hopefully set you in the right direction:
> > http://pastebin.com/Em6CcZ00
> > 
> > No custom code, it builds and the test passes. I also did not edit the
> bindings
> > generators at all.
> > We should probably do something in updateCurrentTransformation() though...
You
> > can use the SVG code as reference.
> 
> Also, we can probably do something like this in currentTransform():
> SVGMatrix& currentTransform()
> {
>     return static_cast<SVGMatrix&>(modifyableState().m_transform);
> }
> 
> To avoid the local member. Feel free to experiment :)

Oh, now that I see your description, it seems you don't want to update the
transform if the matrix is later changed on JS side.
Then, using a local member and leaving the updateCurrentTransform() method empty
(as in my patch) is probably the right thing to do. Just add a comment in there
to explain why it is empty.

[dshwang, 2013-09-20 10:29:03]

> What code is generated if you omit [Custom]? How is the generated code
different
> from the custom binding code you wrote?

The code that uses strange API: http://pastebin.com/Em6CcZ00 


On 2013/09/20 07:31:40, Christophe Dumez (OOO) wrote:
> This should hopefully set you in the right direction:
> http://pastebin.com/Em6CcZ00
> 
> No custom code, it builds and the test passes. I also did not edit the
bindings
> generators at all.
> We should probably do something in updateCurrentTransformation() though... You
> can use the SVG code as reference.

Thank you for your deep investigation.

Your code is great. However, cpp part is strange.
I understand it is only way both to avoid custom binding and to satisfy
SVGTransform.

Another option is to change code_generator_v8.pm in order to generate what I
type in custom file, but we need to make another custom binding for
SVGTransform.

I'll follow ch.dumez's instruction. Thank you again.

[dshwang, 2013-09-20 11:38:51]

https://codereview.chromium.org/24233004/diff/35001/Source/core/html/canvas/C...
File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):

https://codereview.chromium.org/24233004/diff/35001/Source/core/html/canvas/C...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:711:
modifiableState().m_transform = newTransform;
when setting non-invertible matrix, currentTransform should keep the value of
non-invertible matrix.

For example,
context.currentTrasform = SVGMatrix(0,0,0,1,0,0);

currentTrasform must be [0,0,0,1,0,0] and the invertible flag of the context
must be set to false.

New layout test covers it.

[do-not-use, 2013-09-20 14:39:15]

On 2013/09/20 10:29:03, dshwang wrote:
> > What code is generated if you omit [Custom]? How is the generated code
> different
> > from the custom binding code you wrote?
> 
> The code that uses strange API: http://pastebin.com/Em6CcZ00 
> 
> 
> On 2013/09/20 07:31:40, Christophe Dumez (OOO) wrote:
> > This should hopefully set you in the right direction:
> > http://pastebin.com/Em6CcZ00
> > 
> > No custom code, it builds and the test passes. I also did not edit the
> bindings
> > generators at all.
> > We should probably do something in updateCurrentTransformation() though...
You
> > can use the SVG code as reference.
> 
> Thank you for your deep investigation.
> 
> Your code is great. However, cpp part is strange.
> I understand it is only way both to avoid custom binding and to satisfy
> SVGTransform.
> 
> Another option is to change code_generator_v8.pm in order to generate what I
> type in custom file, but we need to make another custom binding for
> SVGTransform.
> 
> I'll follow ch.dumez's instruction. Thank you again.

I believe you want to use the [Immutable] IDL extended attribute on your new
attribute. This should do what you expect (i.e. use a SVGPropertyTearOff but
less awkward API since the returned object is not editable from JS).

[dshwang, 2013-09-20 14:46:04]

On 2013/09/20 14:39:15, Christophe Dumez (OOO) wrote:
> I believe you want to use the [Immutable] IDL extended attribute on your new
> attribute. This should do what you expect (i.e. use a SVGPropertyTearOff but
> less awkward API since the returned object is not editable from JS).

Yep, it is almost what I want.

However, in http://www.chromium.org/blink/webidl/blink-idl-extended-attributes
[Immutable], [MasqueradesAsUndefined], [DoNotCheckSignature]
Might be deprecated. Discussion is on-going.

Is there alternative?

[do-not-use, 2013-09-20 14:50:03]

On 2013/09/20 14:46:04, dshwang wrote:
> On 2013/09/20 14:39:15, Christophe Dumez (OOO) wrote:
> > I believe you want to use the [Immutable] IDL extended attribute on your new
> > attribute. This should do what you expect (i.e. use a SVGPropertyTearOff but
> > less awkward API since the returned object is not editable from JS).
> 
> Yep, it is almost what I want.
> 
> However, in http://www.chromium.org/blink/webidl/blink-idl-extended-attributes
> [Immutable], [MasqueradesAsUndefined], [DoNotCheckSignature]
> Might be deprecated. Discussion is on-going.
> 
> Is there alternative?

No alternative. You should use this one.
SVG IDL needs some clean up but it should not affect your work.

[dshwang, 2013-09-20 14:51:00]

On 2013/09/20 14:46:04, dshwang wrote:
> On 2013/09/20 14:39:15, Christophe Dumez (OOO) wrote:
> > I believe you want to use the [Immutable] IDL extended attribute on your new
> > attribute. This should do what you expect (i.e. use a SVGPropertyTearOff but
> > less awkward API since the returned object is not editable from JS).
> 
> Yep, it is almost what I want.
> 
> However, in http://www.chromium.org/blink/webidl/blink-idl-extended-attributes
> [Immutable], [MasqueradesAsUndefined], [DoNotCheckSignature]
> Might be deprecated. Discussion is on-going.
> 
> Is there alternative?

Immutable is fantastic. It generates the same code to the custom binding in
PatchSet1.
@haraken Can I use [Immutable]?

[haraken, 2013-09-20 14:53:32]

> Immutable is fantastic. It generates the same code to the custom binding in
> PatchSet1.
> @haraken Can I use [Immutable]?

Yes. [Immutable] is already used in a couple of SVG attributes (but only in SVG
attributes).

[do-not-use, 2013-09-20 14:55:02]

On 2013/09/20 14:51:00, dshwang wrote:
> On 2013/09/20 14:46:04, dshwang wrote:
> > On 2013/09/20 14:39:15, Christophe Dumez (OOO) wrote:
> > > I believe you want to use the [Immutable] IDL extended attribute on your
new
> > > attribute. This should do what you expect (i.e. use a SVGPropertyTearOff
but
> > > less awkward API since the returned object is not editable from JS).
> > 
> > Yep, it is almost what I want.
> > 
> > However, in
http://www.chromium.org/blink/webidl/blink-idl-extended-attributes
> > [Immutable], [MasqueradesAsUndefined], [DoNotCheckSignature]
> > Might be deprecated. Discussion is on-going.
> > 
> > Is there alternative?
> 
> Immutable is fantastic. It generates the same code to the custom binding in
> PatchSet1.
> @haraken Can I use [Immutable]?

The doc says it *might* be deprecated. At this point, it is not deprecated and
there is no alternative AFAIK.
Immutable is already used in SVG IDL for cases where the returned SVGPoint /
SVGMatrix is not "updatable".
I am fairly sure it is the right thing to use here (although Immutable will
likely get removed or renamed in the future).

[haraken, 2013-09-20 14:58:11]

> The doc says it *might* be deprecated. At this point, it is not deprecated and
> there is no alternative AFAIK.
> Immutable is already used in SVG IDL for cases where the returned SVGPoint /
> SVGMatrix is not "updatable".
> I am fairly sure it is the right thing to use here (although Immutable will
> likely get removed or renamed in the future).

Yeah, I think we cannot remove [Immutable] until we get rid of TearOffs from SVG
and align SVG's architecture with the architecture of other DOM objects.
kouhei-san is starting the rearchitecturing, but you don't need to wait for it.

[dshwang, 2013-09-20 15:03:26]

On 2013/09/20 14:55:02, Christophe Dumez (OOO) wrote:
> The doc says it *might* be deprecated. At this point, it is not deprecated and
> there is no alternative AFAIK.
> Immutable is already used in SVG IDL for cases where the returned SVGPoint /
> SVGMatrix is not "updatable".
> I am fairly sure it is the right thing to use here (although Immutable will
> likely get removed or renamed in the future).

On 2013/09/20 14:58:11, haraken wrote:
> Yeah, I think we cannot remove [Immutable] until we get rid of TearOffs from
SVG
> and align SVG's architecture with the architecture of other DOM objects.
> kouhei-san is starting the rearchitecturing, but you don't need to wait for
it.

Thx @ch.dumez and @haraken for quick&kind answers :)

[dshwang, 2013-09-27 16:36:15]

http://crrev.com/25037006/ blocks this CL.
I submitted CL about svg setter generation (http://crrev.com/25037006/ ) to make
this CL succinct.

[krit, 2013-09-27 19:08:12]

On 2013/09/20 14:58:11, haraken wrote:
> > The doc says it *might* be deprecated. At this point, it is not deprecated
and
> > there is no alternative AFAIK.
> > Immutable is already used in SVG IDL for cases where the returned SVGPoint /
> > SVGMatrix is not "updatable".
> > I am fairly sure it is the right thing to use here (although Immutable will
> > likely get removed or renamed in the future).
> 
> Yeah, I think we cannot remove [Immutable] until we get rid of TearOffs from
SVG
> and align SVG's architecture with the architecture of other DOM objects.
> kouhei-san is starting the rearchitecturing, but you don't need to wait for
it.

The new DOMMatrix specification will specify two interfaces. One immutable
DOMMatrix and one mutable. This makes it fully compatible with WebIDL. Special
hacks are not needed.

I would still be carful with proceeding with SVGMatrix. Even is SVGMatrix will
be aliased to DOMMatrix, we need to keep the type [SVGMatrix] around for some
time. Using SVGMatrix at this point could encourage users to use SVGMatrix more
instead of less. I won't review this patch but would like everyone to keep that
in mind. The specification makes great progress and the SVG and CSS WG agreed on
most things already. I expect to have a stable spec at the end of the year. (It
might not be in CR though.)

[dshwang, 2013-09-30 14:51:24]

On 2013/09/27 19:08:12, krit wrote:
> The new DOMMatrix specification will specify two interfaces. One immutable
> DOMMatrix and one mutable. This makes it fully compatible with WebIDL. Special
> hacks are not needed.
> 
> I would still be carful with proceeding with SVGMatrix. Even is SVGMatrix will
> be aliased to DOMMatrix, we need to keep the type [SVGMatrix] around for some
> time. Using SVGMatrix at this point could encourage users to use SVGMatrix
more
> instead of less. I won't review this patch but would like everyone to keep
that
> in mind. The specification makes great progress and the SVG and CSS WG agreed
on
> most things already. I expect to have a stable spec at the end of the year.
(It
> might not be in CR though.)

Thank you your explanation. I'm keeping my eyes open for DOMMatrix, so I
migrated your WebKit bug to chromium 3 weeks ago.
https://code.google.com/p/chromium/issues/detail?id=289522

This implementation is under runtime flag: ExperimentalCanvasFeatures
After DOMMatrix is stablized and canvas spec uses DOMMatrix, I'll apply
DOMMatrix on Canvas. I guess we can ship it at that time.

[dshwang, 2013-10-09 16:11:42]

now binding code is simple.

junov@ , could you review? :)

[Justin Novosad, 2013-10-09 17:17:03]

https://codereview.chromium.org/24233004/diff/54001/Source/core/html/canvas/C...
File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):

https://codereview.chromium.org/24233004/diff/54001/Source/core/html/canvas/C...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:706:
modifiableState().m_transform = newTransform;
Interesting bug you fixed here, but is this the right way to fix it?  I am
guessing this is to fix the case of a setCurrentTransform/currentTransform
round-trip with a non-invertible matrix? With this change, in the case where the
transform is non-invertible, the transform will now be retained in the canvas
state, but won't be applied to the context because the call to c->concatCTM
below will be skipped. I think that is wrong. The canvas state and the graphics
context state should remain in sync. Probably not a big deal given Blink's
implementation of 2D canvas does not currently handle non-invertible transforms
correctly, but it is something we should eventually fix, and we can start right
here.

[dshwang, 2013-10-09 17:48:19]

Thank you for review. :)

https://codereview.chromium.org/24233004/diff/54001/Source/core/html/canvas/C...
File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):

https://codereview.chromium.org/24233004/diff/54001/Source/core/html/canvas/C...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:706:
modifiableState().m_transform = newTransform;
On 2013/10/09 17:17:04, junov wrote:
> Interesting bug you fixed here, but is this the right way to fix it?  I am
> guessing this is to fix the case of a setCurrentTransform/currentTransform
> round-trip with a non-invertible matrix?
exactly. This change is only for currentTransform with a non-invertible matrix.


> With this change, in the case where the
> transform is non-invertible, the transform will now be retained in the canvas
> state, but won't be applied to the context because the call to c->concatCTM
> below will be skipped. I think that is wrong. The canvas state and the
graphics
> context state should remain in sync. Probably not a big deal given Blink's
> implementation of 2D canvas does not currently handle non-invertible
transforms
> correctly, but it is something we should eventually fix, and we can start
right
> here.

We already have some inconsistent states between canvas and GraphicsContext
For example,
1. canvas calls save() lazily
2. When Canvas.currentTransform is identity, GraphicsContext.CTM is
HTMLCanvasElement::baseTransform()

In addtion, we never use non-invertible modifiableState().m_transform because we
always check modifiableState().m_invertibleCTM before using
modifiableState().m_transform.
IMO there will be nothing that we should eventually fix.


Nevertheless, if we are afraid this approach, there would be two options.
1. Add new variable to trace currentTransform although non-invertible.
  modifiableState().m_transform will be consistent with js world
  modifiableState().m_effectiveTransform will be consistent with GraphicsContext

2. Make GraphicsContext handle non-invertible matrix.
GraphicsContext can not handle non-invertible matrix. GraphicsContext just calls
SkCanvas::concat().
Introduce m_invertibleCTM in GraphicsContext and check m_invertibleCTM in every
methods before using SkCanvas like CanvasRenderingContext2D.

WDYT?

[Justin Novosad, 2013-10-09 17:52:25]

Ok, I take back my previous comment. This is fine. We'll worry about invertible
matrices the day we decide to support them.

lgtm (but I'm not an OWNER)

[Stephen White, 2013-10-09 17:53:56]

LGTM

[dshwang, 2013-10-10 04:22:40]

Thank you for your review :)

[commit-bot: I haz the power, 2013-10-10 04:22:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dongseong.hwang@intel.com/24233004/54001

[commit-bot: I haz the power, 2013-10-10 06:17:34]

Change committed as 159303