CanvasPathMethods should be shared with CRC2D and Path2D

CanvasPathMethods should be shared with CRC2D and Path2D

All methods in CanvasPathMethods should be shared with CanvasRenderingContext2D and Path2D.
CanvasPathMethods has been differently defined in CRC2D and Path2D until now.
It is against the spec and it makes it more difficult to maintain.
Actually, CRC2D.idl has defined ellipse function but Path2D.idl has omitted the function.
Following codes will make trouble.

  var path = new Path();
  path.ellipse(100, 100, 50, 50, 0, 0, 6.28, false); // undefined error

To solve this problem, it should be changed to share CanvasPathMethods with CRC2D and Path2D.
And to prevent happening same problem, we should add layout test with respect to all Path2D methods.

Spec:
  http://www.w3.org/html/wg/drafts/2dcontext/html5_canvas/#canvaspathmethods

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

[zino, 2014-03-09 02:08:30]

Review please. Thank you :)

[Inactive, 2014-03-09 04:26:36]

The changes are according to spec and the change looks good technically.
However, please:
- Add a link to the spec in your CL description
(http://www.w3.org/html/wg/drafts/2dcontext/html5_canvas/#canvaspathmethods)
- Please confirm that we behave the same way as other browsers (mandatory
arguments AND new method exposed).

This change will need API owner approval as it is web-exposed and non-trivial.

https://codereview.chromium.org/191923002/diff/20001/LayoutTests/fast/canvas/...
File LayoutTests/fast/canvas/script-tests/canvas-path-object.js (right):

https://codereview.chromium.org/191923002/diff/20001/LayoutTests/fast/canvas/...
LayoutTests/fast/canvas/script-tests/canvas-path-object.js:15: shouldBe("typeof
path.ellipse", '"function"');
You likely need to rebaseline as the test as well. I don't see the new
-expected.txt in your CL.

https://codereview.chromium.org/191923002/diff/20001/Source/core/html/canvas/...
File Source/core/html/canvas/CanvasPathMethods.h (right):

https://codereview.chromium.org/191923002/diff/20001/Source/core/html/canvas/...
Source/core/html/canvas/CanvasPathMethods.h:55: // CanvasPathMethods JS API
Should end with a period '.'

https://codereview.chromium.org/191923002/diff/20001/Source/core/html/canvas/...
File Source/core/html/canvas/CanvasPathMethods.idl (right):

https://codereview.chromium.org/191923002/diff/20001/Source/core/html/canvas/...
Source/core/html/canvas/CanvasPathMethods.idl:2: * Copyright (C) 2014 Samsung
Electronics, Inc.  All rights reserved.
Please use this license for new files:
http://www.chromium.org/blink/coding-style#TOC-License

https://codereview.chromium.org/191923002/diff/20001/Source/core/html/canvas/...
Source/core/html/canvas/CanvasPathMethods.idl:33: void moveTo(float x, float y);
A lot of arguments were optional for the methods on Path and now are mandatory
(as per spec). Could you please confirm that this matches the behavior of other
browsers?

https://codereview.chromium.org/191923002/diff/20001/Source/core/html/canvas/...
Source/core/html/canvas/CanvasPathMethods.idl:40: [RaisesException] void
ellipse(float x, float y, float radiusX, float radiusY, float rotation, float
startAngle, float endAngle, [Default=Undefined] optional boolean anticlockwise);
So this method is now exposed on Path as well. This is a web-exposed behavior
change. How do other browsers behave?

[zino, 2014-03-10 07:17:19]

Thank you for detailed explanation :)

On 2014/03/09 04:26:36, Chris Dumez wrote:
> The changes are according to spec and the change looks good technically.
> However, please:
> - Add a link to the spec in your CL description

Done.

> (http://www.w3.org/html/wg/drafts/2dcontext/html5_canvas/#canvaspathmethods)
> - Please confirm that we behave the same way as other browsers (mandatory
> arguments AND new method exposed).
> 
> This change will need API owner approval as it is web-exposed and non-trivial.

How can I request approval to API owner?
(Intent to Implement? mail? others? I don't know well. could you please give me
some guide T.T)

> 
>
https://codereview.chromium.org/191923002/diff/20001/LayoutTests/fast/canvas/...
> File LayoutTests/fast/canvas/script-tests/canvas-path-object.js (right):
> 
>
https://codereview.chromium.org/191923002/diff/20001/LayoutTests/fast/canvas/...
> LayoutTests/fast/canvas/script-tests/canvas-path-object.js:15:
shouldBe("typeof
> path.ellipse", '"function"');
> You likely need to rebaseline as the test as well. I don't see the new
> -expected.txt in your CL.

Done.

> 
>
https://codereview.chromium.org/191923002/diff/20001/Source/core/html/canvas/...
> File Source/core/html/canvas/CanvasPathMethods.h (right):
> 
>
https://codereview.chromium.org/191923002/diff/20001/Source/core/html/canvas/...
> Source/core/html/canvas/CanvasPathMethods.h:55: // CanvasPathMethods JS API
> Should end with a period '.'

Done.

> 
>
https://codereview.chromium.org/191923002/diff/20001/Source/core/html/canvas/...
> File Source/core/html/canvas/CanvasPathMethods.idl (right):
> 
>
https://codereview.chromium.org/191923002/diff/20001/Source/core/html/canvas/...
> Source/core/html/canvas/CanvasPathMethods.idl:2: * Copyright (C) 2014 Samsung
> Electronics, Inc.  All rights reserved.
> Please use this license for new files:
> http://www.chromium.org/blink/coding-style#TOC-License

Done.

> 
>
https://codereview.chromium.org/191923002/diff/20001/Source/core/html/canvas/...
> Source/core/html/canvas/CanvasPathMethods.idl:33: void moveTo(float x, float
y);
> A lot of arguments were optional for the methods on Path and now are mandatory
> (as per spec). Could you please confirm that this matches the behavior of
other
> browsers?

The basic part of Path was already implemented in Webkit.
(https://bugs.webkit.org/show_bug.cgi?id=97333)
It seems that current Path.idl has been created at that time.

A few months later, Path methods on CRC2D was updated by @dshwang's patch to
match the spec.
(https://chromiumcodereview.appspot.com/14654002,
 https://bugs.webkit.org/show_bug.cgi?id=115723)
BTW, It seems that Path wasn't updated at that time.

So, I think his patch(as per spec) is right and it should be applied.

> 
>
https://codereview.chromium.org/191923002/diff/20001/Source/core/html/canvas/...
> Source/core/html/canvas/CanvasPathMethods.idl:40: [RaisesException] void
> ellipse(float x, float y, float radiusX, float radiusY, float rotation, float
> startAngle, float endAngle, [Default=Undefined] optional boolean
anticlockwise);
> So this method is now exposed on Path as well. This is a web-exposed behavior
> change. How do other browsers behave?

"ellipse" function was implemented by @dshwang.
(https://chromiumcodereview.appspot.com/14298022/)
Other browsers weren't implemented yet.

By the same token, I think this method should be exposed on Path.

[Inactive, 2014-03-10 15:36:06]

On 2014/03/10 07:17:19, zino wrote:
> Thank you for detailed explanation :)
> 
> On 2014/03/09 04:26:36, Chris Dumez wrote:
> > The changes are according to spec and the change looks good technically.
> > However, please:
> > - Add a link to the spec in your CL description
> 
> Done.
> 
> > (http://www.w3.org/html/wg/drafts/2dcontext/html5_canvas/#canvaspathmethods)
> > - Please confirm that we behave the same way as other browsers (mandatory
> > arguments AND new method exposed).
> > 
> > This change will need API owner approval as it is web-exposed and
non-trivial.
> 
> How can I request approval to API owner?
> (Intent to Implement? mail? others? I don't know well. could you please give
me
> some guide T.T)

This is a small change so I don't think you need an intent to implement. You
merely need an LGTM on this patch from an API Owner (eseidel is an API Owner for
e.g.).
The full list of API Owners is at Source/OWNERS.

Please add your explanation about compatibility risks to the Changelog as well
to facilitate review.

[Inactive, 2014-03-10 15:37:17]

On 2014/03/10 15:36:06, Chris Dumez wrote:
> On 2014/03/10 07:17:19, zino wrote:
> > Thank you for detailed explanation :)
> > 
> > On 2014/03/09 04:26:36, Chris Dumez wrote:
> > > The changes are according to spec and the change looks good technically.
> > > However, please:
> > > - Add a link to the spec in your CL description
> > 
> > Done.
> > 
> > >
(http://www.w3.org/html/wg/drafts/2dcontext/html5_canvas/#canvaspathmethods)
> > > - Please confirm that we behave the same way as other browsers (mandatory
> > > arguments AND new method exposed).
> > > 
> > > This change will need API owner approval as it is web-exposed and
> non-trivial.
> > 
> > How can I request approval to API owner?
> > (Intent to Implement? mail? others? I don't know well. could you please give
> me
> > some guide T.T)
> 
> This is a small change so I don't think you need an intent to implement. You
> merely need an LGTM on this patch from an API Owner (eseidel is an API Owner
for
> e.g.).
> The full list of API Owners is at Source/OWNERS.
> 
> Please add your explanation about compatibility risks to the Changelog as well
> to facilitate review.

arg.., I l-g-t-m'ed inadvertently. Please don't land without API Owner approval.

[Justin Novosad, 2014-03-10 18:15:37]

https://codereview.chromium.org/191923002/diff/40001/Source/core/html/canvas/...
File Source/core/html/canvas/CanvasPathMethods.h (right):

https://codereview.chromium.org/191923002/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/CanvasPathMethods.h:56: static void
closePath(CanvasPathMethods& object)
How is adding this indirection better than what we had before?

https://codereview.chromium.org/191923002/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/CanvasPathMethods.h:78: Path m_path;
I think is is something that should be refactored: this interface class should
not be storing a path. The fact that there is an m_path here and in CRC2D is
confusing.

[Justin Novosad, 2014-03-10 18:31:19]

I am not sure I understand how this change affects spec compliance in a tangible
way.  Does it really matter from a spec-compliance stand point that the IDL
interface inheritance structure matches the spec?  In other words, is it
possible to write a layout_test that would be fixed by this change?

[Inactive, 2014-03-10 18:39:55]

On 2014/03/10 18:31:19, junov wrote:
> I am not sure I understand how this change affects spec compliance in a
tangible
> way.  Does it really matter from a spec-compliance stand point that the IDL
> interface inheritance structure matches the spec?  In other words, is it
> possible to write a layout_test that would be fixed by this change?

The web-exposed behavior change is unrelated to interface inheritance here.
There is actually no inheritance, just an IDL implements statement, which is
equivalent to duplicating the IDL code in both classes (but avoids the
duplication).

As I mentioned earlier, the behavior change is because CanvasRenderingContext2D
and Path use to behave differently and now behave the same (as the IDL code is
now exactly the same but used to be slightly different due to the duplication).
The behavior change is:
1. A lot of arguments were optional for the methods on Path and now are
mandatory (as per spec). Could you please confirm that this matches the behavior
of other
browsers?
2. ellipse() method is now exposed on Path as well

Layout tests can be written for both changes. The CL already contains 1 layout
test covering 2).

[Justin Novosad, 2014-03-10 18:56:19]

On 2014/03/10 18:39:55, Chris Dumez wrote:

> The web-exposed behavior change is unrelated to interface inheritance here.
> There is actually no inheritance, just an IDL implements statement, which is
> equivalent to duplicating the IDL code in both classes (but avoids the
> duplication).
> 
Ah, got it. lgtm, but we still need API owner approval.

[eseidel, 2014-03-14 16:10:31]

What parts of this are observable from JS?

[eseidel, 2014-03-14 16:12:10]

https://codereview.chromium.org/191923002/diff/80001/Source/core/html/canvas/...
File Source/core/html/canvas/Path2D.idl (left):

https://codereview.chromium.org/191923002/diff/80001/Source/core/html/canvas/...
Source/core/html/canvas/Path2D.idl:40: void moveTo([Default=Undefined] optional
float x,
So these are no longer optional after your change, correct?   Does that mean
they will throw exceptions?

[Inactive, 2014-03-14 17:40:29]

On 2014/03/14 16:12:10, eseidel wrote:
>
https://codereview.chromium.org/191923002/diff/80001/Source/core/html/canvas/...
> File Source/core/html/canvas/Path2D.idl (left):
> 
>
https://codereview.chromium.org/191923002/diff/80001/Source/core/html/canvas/...
> Source/core/html/canvas/Path2D.idl:40: void moveTo([Default=Undefined]
optional
> float x,
> So these are no longer optional after your change, correct?   Does that mean
> they will throw exceptions?

Yes, it will now throw exceptions if the arguments are omitted as they are now
mandatory on both objects.

JS visible changes:
1. A lot of arguments were optional for the methods on Path and now are
mandatory (as per spec). Yes, it will now throw if args are omitted.
2. ellipse() method is now exposed on Path as well

[Justin Novosad, 2014-03-14 18:00:40]

https://codereview.chromium.org/191923002/diff/80001/Source/core/html/canvas/...
File Source/core/html/canvas/Path2D.idl (left):

https://codereview.chromium.org/191923002/diff/80001/Source/core/html/canvas/...
Source/core/html/canvas/Path2D.idl:40: void moveTo([Default=Undefined] optional
float x,
On 2014/03/14 16:12:11, eseidel wrote:
> So these are no longer optional after your change, correct?   Does that mean
> they will throw exceptions?

Just to make sure you noticed: this interface is not shipping yet so we can
still make this change without breaking the web.

[Inactive, 2014-03-14 18:05:33]

On 2014/03/14 18:00:40, junov wrote:
>
https://codereview.chromium.org/191923002/diff/80001/Source/core/html/canvas/...
> File Source/core/html/canvas/Path2D.idl (left):
> 
>
https://codereview.chromium.org/191923002/diff/80001/Source/core/html/canvas/...
> Source/core/html/canvas/Path2D.idl:40: void moveTo([Default=Undefined]
optional
> float x,
> On 2014/03/14 16:12:11, eseidel wrote:
> > So these are no longer optional after your change, correct?   Does that mean
> > they will throw exceptions?
> 
> Just to make sure you noticed: this interface is not shipping yet so we can
> still make this change without breaking the web.

Oh, I did not notice this was not shipping yet. Then API Owner approval isn't
strictly needed I believe, sorry I missed that.

[eseidel, 2014-03-14 21:15:20]

If path2d isn't shipping, lgtm.  Otherwise throwing exceptions where we didn't
before is concerning and may break content.

[zino, 2014-03-16 15:21:18]

On 2014/03/14 21:15:20, eseidel wrote:
> If path2d isn't shipping, lgtm.  Otherwise throwing exceptions where we didn't
> before is concerning and may break content.

Thanks to everyone :)

It is not shipping yet.
Would you mind if I commit this?

[dshwang, 2014-03-16 17:03:12]

On 2014/03/16 15:21:18, zino wrote:
> It is not shipping yet.
> Would you mind if I commit this?

additional non-owner lgtm.

I guess so, because this CL already got enough lgtm and everything under
ExperimentalCanvasFeatures guard.

[eseidel, 2014-03-16 20:40:39]

lgtm

[commit-bot: I haz the power, 2014-03-16 20:40:50]

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

[commit-bot: I haz the power, 2014-03-16 22:50:13]

Change committed as 169316