Implement CRC2D.scrollPathIntoView() on Canvas

Implement CRC2D.scrollPathIntoView() on Canvas

The method's behavior is very similar to element.scrollIntoView().
This is just a another version about path on canvas.
('path' means both current default path and path object)
This is especially useful on devices with small screens,
where the whole canvas might not be visible at once.

Intent to Implement:
  https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/KLFSBeP42VQ

The spec reference is:
  http://www.w3.org/TR/2dcontext/#dom-context-2d-scrollpathintoview
  http://www.whatwg.org/specs/web-apps/current-work/#dom-context-2d-scrollpathintoview

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

[zino, 2014-03-20 16:03:27]

Could you please review?

[Rik, 2014-03-20 16:22:19]

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

https://codereview.chromium.org/196243007/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:1114:
scrollPathIntoViewInternal(m_path);
Does this take the current CTM into account?
I believe m_path is in user coordinate space.

https://codereview.chromium.org/196243007/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:1129: if
(!state().m_invertibleCTM || !canvas() || path.isEmpty())
scrollPathIntoView() should take ctm into account.
According to the spec, scrollPathIntoView(Path path) should not. (This feels
like a spec bug)

Even though you check, it does look like you're using the CTM.

[Justin Novosad, 2014-03-20 20:03:12]

https://codereview.chromium.org/196243007/diff/40001/LayoutTests/fast/canvas/...
File LayoutTests/fast/canvas/canvas-scroll-path-into-view.html (right):

https://codereview.chromium.org/196243007/diff/40001/LayoutTests/fast/canvas/...
LayoutTests/fast/canvas/canvas-scroll-path-into-view.html:100:
scrollTest(container, context, path);
Need additional test case with CTM != identity.  Among other things, test
rotation on rectangular and curvy paths

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

https://codereview.chromium.org/196243007/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:1129: if
(!state().m_invertibleCTM || !canvas() || path.isEmpty())
On 2014/03/20 16:22:19, Rik wrote:
> scrollPathIntoView() should take ctm into account.
> According to the spec, scrollPathIntoView(Path path) should not. (This feels
> like a spec bug)

I agree, all the other methods on CanvasRenderingContext2D that take a path
object use the CTM. ctx.fill(myPath) followed by ctx.scrollPathIntoView(myPath)
should line-up.

Also, no need to check !canvas(). CRC2D holds a counted reference on the canvas
element that it was created from, so the canvas will always continue to exist as
long as the CRC2D is alive, even if the canvas element was removed from the
document and the JS heap.

https://codereview.chromium.org/196243007/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:2190: rect =
LayoutRect(transformedBounds);
This works if the transform has only translation and scale.  If there is
rotation, you need to transform the path first and then take the bounds of the
transformed path, which is not equivalent in that case.

[Rik, 2014-03-20 20:24:51]

On 2014/03/20 20:03:12, junov wrote:
>
https://codereview.chromium.org/196243007/diff/40001/LayoutTests/fast/canvas/...
> File LayoutTests/fast/canvas/canvas-scroll-path-into-view.html (right):
> 
>
https://codereview.chromium.org/196243007/diff/40001/LayoutTests/fast/canvas/...
> LayoutTests/fast/canvas/canvas-scroll-path-into-view.html:100:
> scrollTest(container, context, path);
> Need additional test case with CTM != identity.  Among other things, test
> rotation on rectangular and curvy paths
> 
>
https://codereview.chromium.org/196243007/diff/40001/Source/core/html/canvas/...
> File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):
> 
>
https://codereview.chromium.org/196243007/diff/40001/Source/core/html/canvas/...
> Source/core/html/canvas/CanvasRenderingContext2D.cpp:1129: if
> (!state().m_invertibleCTM || !canvas() || path.isEmpty())
> On 2014/03/20 16:22:19, Rik wrote:
> > scrollPathIntoView() should take ctm into account.
> > According to the spec, scrollPathIntoView(Path path) should not. (This feels
> > like a spec bug)
> 
> I agree, all the other methods on CanvasRenderingContext2D that take a path
> object use the CTM. ctx.fill(myPath) followed by
ctx.scrollPathIntoView(myPath)
> should line-up.

Yes. Once this is clarified, it will be OK to apply the ctm to the incoming
path.
@zino, I previously missed where the ctm was applied to the path so you can
ignore that comment/

[zino, 2014-03-27 17:50:17]

Thanks to everyone.
I addressed all your comments and I uploaded a new patch set.
Could you please review again?

[Justin Novosad, 2014-03-27 21:26:21]

Much better test!
Just a few minor requests.
Otherwise lgtm.

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

https://codereview.chromium.org/196243007/diff/100001/Source/core/html/canvas...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:1136:
transformedPath.transform(state().m_transform);
I am fine with submitting this, but please create a new bug to track the fact
that there is a discrepancy between this implementation and the specification.
The specification still does not say that the Path2D flavor of
scrollPathIntoView is supposed to transform the path by the CTM, but it probably
should.

https://codereview.chromium.org/196243007/diff/100001/Source/core/html/canvas...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:1152: // TODO: should
implement "inform the user" that the caret and/or
If there isn't already a bug for this, please create one, and specify the URL
(crbug.com/xxx) in this comment

[zino, 2014-03-31 13:45:23]

On 2014/03/27 21:26:21, junov wrote:
> Much better test!
> Just a few minor requests.
> Otherwise lgtm.
> 
>
https://codereview.chromium.org/196243007/diff/100001/Source/core/html/canvas...
> File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):
> 
>
https://codereview.chromium.org/196243007/diff/100001/Source/core/html/canvas...
> Source/core/html/canvas/CanvasRenderingContext2D.cpp:1136:
> transformedPath.transform(state().m_transform);
> I am fine with submitting this, but please create a new bug to track the fact
> that there is a discrepancy between this implementation and the specification.
> The specification still does not say that the Path2D flavor of
> scrollPathIntoView is supposed to transform the path by the CTM, but it
probably
> should.
> 
>
https://codereview.chromium.org/196243007/diff/100001/Source/core/html/canvas...
> Source/core/html/canvas/CanvasRenderingContext2D.cpp:1152: // TODO: should
> implement "inform the user" that the caret and/or
> If there isn't already a bug for this, please create one, and specify the URL
> (crbug.com/xxx) in this comment

Thank you for review & lgtm :)

I created new bug and I added comment about "inform the user".

By the way, I'm not still understanding because I don't have enough knowledges.
I think it seems the specifications says that the path must be transformed
according to the CTM of CRC2D when intended path is path object.

The specification reference: 
"When the intended path is a Path object, the coordinates and lines of its
subpaths must be transformed according to the CanvasRenderingContext2D object's
current transformation matrix when used by these methods (without affecting the
Path object itself)."

In case of fill (or stroke, etc), when fill method is called, we have only to
draw the path(without transformation).
Because the coordinate system of GC(GraphicsContext) has already been transfomed
by the methods such as translate, rotate and so on.

However, in case of scrollPathIntoView, The method is not drawing API and it
doesn't use the coordinate system of GC.
So, we should use a temporary path object in order to transform. (Because we
don't use GC and GC's coordinate system in scrollPathIntoView)

Please share your opinion.

Thank you!

[Justin Novosad, 2014-03-31 17:14:01]

On 2014/03/31 13:45:23, zino wrote:

> 
> Thank you for review & lgtm :)
> 
> I created new bug and I added comment about "inform the user".
> 
> By the way, I'm not still understanding because I don't have enough
knowledges.
> I think it seems the specifications says that the path must be transformed
> according to the CTM of CRC2D when intended path is path object.

You are correct. The definition of scrollPathIntoView uses the term "intended
path", which is defined above. So it is already clear in the spec that Path
object is transformed by the CTM.  Thanks for pointing that out.

lgtm

[zino, 2014-04-01 00:33:07]

The CQ bit was checked by jinho.bang@samsung.com

[commit-bot: I haz the power, 2014-04-01 00:33:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jinho.bang@samsung.com/196243007/120001

[commit-bot: I haz the power, 2014-04-01 01:05:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-01 01:05:20]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_rel

[zino, 2014-04-01 05:29:20]

The CQ bit was checked by jinho.bang@samsung.com

[commit-bot: I haz the power, 2014-04-01 05:29:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jinho.bang@samsung.com/196243007/140001

[commit-bot: I haz the power, 2014-04-01 05:39:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-01 05:39:44]

Try jobs failed on following builders:
  tryserver.blink on win_blink_compile_dbg

[zino, 2014-04-01 06:45:29]

The CQ bit was checked by jinho.bang@samsung.com

[commit-bot: I haz the power, 2014-04-01 06:45:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jinho.bang@samsung.com/196243007/140001

[commit-bot: I haz the power, 2014-04-01 07:21:26]

Change committed as 170524