Add versions of stroke, fill, and clip that take a Path parameter.

Add versions of stroke, fill, and clip to CanvasRenderingContext2D that take a Path parameter.

The spec reference is:
http://www.whatwg.org/specs/web-apps/current-work/multipage/the-canvas-element.html#dom-context-2d-fill

This partially addresses bug 330506. The rest of these functions need to be added to completely close that bug, which I plan to do in a following CL:
 - drawSystemFocusRing(path, element);
 - drawCustomFocusRing(path, element);
 - isPointInPath(path, x, y);
 - isPointInStroke(path, x, y);
 - scrollPathIntoView(path);

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

[jcgregorio, 2014-01-23 18:17:13]

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

https://codereview.chromium.org/137353004/diff/110001/Source/core/html/canvas...
Source/core/html/canvas/CanvasRenderingContext2D.idl:83: void fill(Path? path,
optional CanvasWindingRule winding);
The part that I'm unsure about is that if I need to add
[RuntimeEnabled=ExperimentalCanvasFeatures] here, since fill, stroke and clip
already exist in CanvasRenderingContext2D.

It appears that putting [RuntimeEnabled=ExperimentalCanvasFeatures] on the first
definition of a method, such as 'fill', will hide *all* definitions of 'fill'
behind the features flag, while not putting it first definition, but putting on
on the second or later definition of a method will cause all definitions to
*not* be hidden behind the features flag.

[Justin Novosad, 2014-01-23 18:49:24]

canvas-path-context-clip.js appears to be missing

[jcgregorio, 2014-01-23 18:56:51]

On 2014/01/23 18:49:24, junov wrote:
> canvas-path-context-clip.js appears to be missing

It shows up for me, can you see it here?

  
https://codereview.chromium.org/137353004/diff/110001/LayoutTests/fast/canvas...

[Rik, 2014-01-23 19:04:56]

Were the lgtm's to implement this on the blink mailing list?

I did not receive any public feedback on my proposal for Shapes so maybe we
should just go ahead with this API and have duplicate functionality in the
future.

[Stephen White, 2014-01-23 19:17:05]

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

https://codereview.chromium.org/137353004/diff/110001/Source/core/html/canvas...
Source/core/html/canvas/CanvasRenderingContext2D.idl:83: void fill(Path? path,
optional CanvasWindingRule winding);
On 2014/01/23 18:17:14, jcgregorio wrote:
> The part that I'm unsure about is that if I need to add
> [RuntimeEnabled=ExperimentalCanvasFeatures] here, since fill, stroke and clip
> already exist in CanvasRenderingContext2D.
> 
> It appears that putting [RuntimeEnabled=ExperimentalCanvasFeatures] on the
first
> definition of a method, such as 'fill', will hide *all* definitions of 'fill'
> behind the features flag, while not putting it first definition, but putting
on
> on the second or later definition of a method will cause all definitions to
> *not* be hidden behind the features flag.

If so, that sounds like a bug in the RuntimeEnabled feature that should be fixed
before this is landed. Perhaps eseidel knows what's up here.

We definitely need to keep new API behind a flag, if it hasn't passed through
the Blink new feature launch process.

[jcgregorio, 2014-01-23 19:18:01]

On 2014/01/23 19:04:56, Rik wrote:
> Were the lgtm's to implement this on the blink mailing list?
> 
> I did not receive any public feedback on my proposal for Shapes so maybe we
> should just go ahead with this API and have duplicate functionality in the
> future.

This will still be hidden behind the experimental canvas flag with the intent
to update it to whatever the spec settles down to.

[Justin Novosad, 2014-01-23 19:20:20]

I agree. There needs to be at least an "intent to implement" thread on blink-dev
before implementing any web facing APIs.  It is even more important for stuff
that is still in flux in the spec.

[jcgregorio, 2014-01-23 20:08:31]

On 2014/01/23 19:20:20, junov wrote:
> I agree. There needs to be at least an "intent to implement" thread on
blink-dev
> before implementing any web facing APIs.  It is even more important for stuff
> that is still in flux in the spec.

My mistake, I had looked at the bug,
https://code.google.com/p/chromium/issues/detail?id=159839, but hadn't looked at
the Intent to Implement:
https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/MGxEfzBPw68/Mw....
I will close up this CL for now and respond to the Shape2D proposal on the
whatwg mailing list.

  Thanks,
  -joe

[eseidel, 2014-01-23 20:24:01]

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

https://codereview.chromium.org/137353004/diff/110001/Source/core/html/canvas...
Source/core/html/canvas/CanvasRenderingContext2D.idl:83: void fill(Path? path,
optional CanvasWindingRule winding);
On 2014/01/23 19:17:06, Stephen White wrote:
> On 2014/01/23 18:17:14, jcgregorio wrote:
> > The part that I'm unsure about is that if I need to add
> > [RuntimeEnabled=ExperimentalCanvasFeatures] here, since fill, stroke and
clip
> > already exist in CanvasRenderingContext2D.
> > 
> > It appears that putting [RuntimeEnabled=ExperimentalCanvasFeatures] on the
> first
> > definition of a method, such as 'fill', will hide *all* definitions of
'fill'
> > behind the features flag, while not putting it first definition, but putting
> on
> > on the second or later definition of a method will cause all definitions to
> > *not* be hidden behind the features flag.
> 
> If so, that sounds like a bug in the RuntimeEnabled feature that should be
fixed
> before this is landed. Perhaps eseidel knows what's up here.
> 
> We definitely need to keep new API behind a flag, if it hasn't passed through
> the Blink new feature launch process.

RuntimeEnabled= is a bindings generation feature.  I'd bring it up with haraken
or nbarth.

[dshwang, 2014-01-23 21:17:16]

afaik jinho.bang@ works in progress on canvas path also.
https://codereview.chromium.org/118103003/

https://codereview.chromium.org/137353004/diff/110001/LayoutTests/fast/canvas...
File LayoutTests/fast/canvas/script-tests/canvas-path-context-stroke.js (right):

https://codereview.chromium.org/137353004/diff/110001/LayoutTests/fast/canvas...
LayoutTests/fast/canvas/script-tests/canvas-path-context-stroke.js:46:
testStrokeWith(path);
I'm curious how result is changed if following code added.

ctx.scale(2);
testStrokeWith();
testStrokeWith(path);

I guess testStrokeWith() draws x1 rect because ctx.scale(2) deals with ctx.path,
but testStrokeWith(path) draws x2 rect because ctx.scale(2) does not do anything
about path.

The spec is vague about this case.

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

https://codereview.chromium.org/137353004/diff/110001/Source/core/html/canvas...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:1002: return;
Could v8 binder fire dom exception if domPath is null?
I ask because the first argument is not optional.

The spec:
  void clip(Path path, optional CanvasFillRule fillRule = "nonzero");

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

https://codereview.chromium.org/137353004/diff/110001/Source/core/html/canvas...
Source/core/html/canvas/CanvasRenderingContext2D.idl:87: void clip(Path? path,
optional CanvasWindingRule winding);
why "Path? path", instead of "Path path"?

The spec:
  void clip(Path path, optional CanvasFillRule fillRule = "nonzero");

[Nils Barth (inactive), 2014-01-29 06:59:16]

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

https://codereview.chromium.org/137353004/diff/110001/Source/core/html/canvas...
Source/core/html/canvas/CanvasRenderingContext2D.idl:83: void fill(Path? path,
optional CanvasWindingRule winding);
Thanks for raising this Eric, and thanks to Joe and Stephen for analysis!
This is a limitation of the current bindings generator,
and has actually come up another time recently.

I've opened a bug for this:
[RuntimeEnabled] on overloaded methods
https://code.google.com/p/chromium/issues/detail?id=339000

Likely won't get to this soon (compiler rewrite), but maybe end of Feb or in
March.

[jcgregorio, 2014-01-29 17:51:36]

I believe 

 
https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/MGxEfzBPw68/Lj...

means that this CL is now unblocked on the Intent to Implement front?

[jcgregorio, 2014-01-29 17:51:49]

https://codereview.chromium.org/137353004/diff/110001/LayoutTests/fast/canvas...
File LayoutTests/fast/canvas/script-tests/canvas-path-context-stroke.js (right):

https://codereview.chromium.org/137353004/diff/110001/LayoutTests/fast/canvas...
LayoutTests/fast/canvas/script-tests/canvas-path-context-stroke.js:46:
testStrokeWith(path);
On 2014/01/23 21:17:17, dshwang wrote:
> I'm curious how result is changed if following code added.
> 
> ctx.scale(2);
> testStrokeWith();
> testStrokeWith(path);
> 
> I guess testStrokeWith() draws x1 rect because ctx.scale(2) deals with
ctx.path,
> but testStrokeWith(path) draws x2 rect because ctx.scale(2) does not do
anything
> about path.

Actually, my reading of the spec is that they should both do the same thing,
i.e. both be
scaled by 2, that the current transformation matrix applies to both.

 
http://www.whatwg.org/specs/web-apps/current-work/multipage/the-canvas-elemen...

> 
> The spec is vague about this case.

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

https://codereview.chromium.org/137353004/diff/110001/Source/core/html/canvas...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:1002: return;
Updated IDL to match the spec, Path is no longer optional.

On 2014/01/23 21:17:17, dshwang wrote:
> Could v8 binder fire dom exception if domPath is null?
> I ask because the first argument is not optional.
> 
> The spec:
>   void clip(Path path, optional CanvasFillRule fillRule = "nonzero");

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

https://codereview.chromium.org/137353004/diff/110001/Source/core/html/canvas...
Source/core/html/canvas/CanvasRenderingContext2D.idl:83: void fill(Path? path,
optional CanvasWindingRule winding);
Thanks for the update and good news about the rewrite, I wasn't looking forward
to doing any Perl :-)

Is this really a blocker for this CL, Path is already hidden behind a
RuntimeEnabled flag, even if fill(path, winding) isn't hidden, you could never
call it because you would never be able to create the needed Path object.

On 2014/01/29 06:59:17, Nils Barth wrote:
> Thanks for raising this Eric, and thanks to Joe and Stephen for analysis!
> This is a limitation of the current bindings generator,
> and has actually come up another time recently.
> 
> I've opened a bug for this:
> [RuntimeEnabled] on overloaded methods
> https://code.google.com/p/chromium/issues/detail?id=339000
> 
> Likely won't get to this soon (compiler rewrite), but maybe end of Feb or in
> March.

https://codereview.chromium.org/137353004/diff/110001/Source/core/html/canvas...
Source/core/html/canvas/CanvasRenderingContext2D.idl:87: void clip(Path? path,
optional CanvasWindingRule winding);
On 2014/01/23 21:17:17, dshwang wrote:
> why "Path? path", instead of "Path path"?
> 
> The spec:
>   void clip(Path path, optional CanvasFillRule fillRule = "nonzero");

Done.

[Stephen White, 2014-01-29 18:08:56]

On 2014/01/29 17:51:49, jcgregorio wrote:
>
https://codereview.chromium.org/137353004/diff/110001/LayoutTests/fast/canvas...
> File LayoutTests/fast/canvas/script-tests/canvas-path-context-stroke.js
(right):
> 
>
https://codereview.chromium.org/137353004/diff/110001/LayoutTests/fast/canvas...
> LayoutTests/fast/canvas/script-tests/canvas-path-context-stroke.js:46:
> testStrokeWith(path);
> On 2014/01/23 21:17:17, dshwang wrote:
> > I'm curious how result is changed if following code added.
> > 
> > ctx.scale(2);
> > testStrokeWith();
> > testStrokeWith(path);
> > 
> > I guess testStrokeWith() draws x1 rect because ctx.scale(2) deals with
> ctx.path,
> > but testStrokeWith(path) draws x2 rect because ctx.scale(2) does not do
> anything
> > about path.
> 
> Actually, my reading of the spec is that they should both do the same thing,
> i.e. both be
> scaled by 2, that the current transformation matrix applies to both.
> 
>  
>
http://www.whatwg.org/specs/web-apps/current-work/multipage/the-canvas-elemen...
> 
> > 
> > The spec is vague about this case.
> 
>
https://codereview.chromium.org/137353004/diff/110001/Source/core/html/canvas...
> File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):
> 
>
https://codereview.chromium.org/137353004/diff/110001/Source/core/html/canvas...
> Source/core/html/canvas/CanvasRenderingContext2D.cpp:1002: return;
> Updated IDL to match the spec, Path is no longer optional.
> 
> On 2014/01/23 21:17:17, dshwang wrote:
> > Could v8 binder fire dom exception if domPath is null?
> > I ask because the first argument is not optional.
> > 
> > The spec:
> >   void clip(Path path, optional CanvasFillRule fillRule = "nonzero");
> 
>
https://codereview.chromium.org/137353004/diff/110001/Source/core/html/canvas...
> File Source/core/html/canvas/CanvasRenderingContext2D.idl (right):
> 
>
https://codereview.chromium.org/137353004/diff/110001/Source/core/html/canvas...
> Source/core/html/canvas/CanvasRenderingContext2D.idl:83: void fill(Path? path,
> optional CanvasWindingRule winding);
> Thanks for the update and good news about the rewrite, I wasn't looking
forward
> to doing any Perl :-)
> 
> Is this really a blocker for this CL, Path is already hidden behind a
> RuntimeEnabled flag, even if fill(path, winding) isn't hidden, you could never
> call it because you would never be able to create the needed Path object.

I'm pretty sure we don't want to make the APIs discoverable via inspection if
they're behind a flag, since code could still inadvertently depend on their
presence and names, even if non-functional. You probably should verify that with
one of the Blink API owners if I'm right about this, though.

On a pragmatic level, there are some autogenerated Inspector tests which will
probably start to fail if you introduce new API without adding them to that
test. Running the layout test bots should tell you if that's the case.

> On 2014/01/29 06:59:17, Nils Barth wrote:
> > Thanks for raising this Eric, and thanks to Joe and Stephen for analysis!
> > This is a limitation of the current bindings generator,
> > and has actually come up another time recently.
> > 
> > I've opened a bug for this:
> > [RuntimeEnabled] on overloaded methods
> > https://code.google.com/p/chromium/issues/detail?id=339000
> > 
> > Likely won't get to this soon (compiler rewrite), but maybe end of Feb or in
> > March.
> 
>
https://codereview.chromium.org/137353004/diff/110001/Source/core/html/canvas...
> Source/core/html/canvas/CanvasRenderingContext2D.idl:87: void clip(Path? path,
> optional CanvasWindingRule winding);
> On 2014/01/23 21:17:17, dshwang wrote:
> > why "Path? path", instead of "Path path"?
> > 
> > The spec:
> >   void clip(Path path, optional CanvasFillRule fillRule = "nonzero");
> 
> Done.

[jcgregorio, 2014-01-29 19:19:17]

On Wed, Jan 29, 2014 at 1:08 PM, <senorblanco@chromium.org> wrote
>
>
> I'm pretty sure we don't want to make the APIs discoverable via inspection
> if
> they're behind a flag, since code could still inadvertently depend on their
> presence and names, even if non-functional.


Do you mean inspection of the idl or of the interface presented in JS? At
the
JS/V8 level nothing really changes because there is already a 'fill' method
on the CanvasRenderingContext2D, and it merely shows up as a 'native code'
implementation. Whether or not the experimental canvas flag is turned on or
off
that remains the same.



> You probably should verify that with
> one of the Blink API owners if I'm right about this, though.
>

Will do.


>
> On a pragmatic level, there are some autogenerated Inspector tests which
> will
> probably start to fail if you introduce new API without adding them to that
> test. Running the layout test bots should tell you if that's the case.


I've kicked off a few layout test bots, let me know if there are specific
ones I should try.


>
>
>  On 2014/01/29 06:59:17, Nils Barth wrote:
>> > Thanks for raising this Eric, and thanks to Joe and Stephen for
>> analysis!
>> > This is a limitation of the current bindings generator,
>> > and has actually come up another time recently.
>> >
>> > I've opened a bug for this:
>> > [RuntimeEnabled] on overloaded methods
>> > https://code.google.com/p/chromium/issues/detail?id=339000
>> >
>> > Likely won't get to this soon (compiler rewrite), but maybe end of Feb
>> or in
>> > March.
>>
>
>
> https://codereview.chromium.org/137353004/diff/110001/
> Source/core/html/canvas/CanvasRenderingContext2D.idl#newcode87
>
>> Source/core/html/canvas/CanvasRenderingContext2D.idl:87: void clip(Path?
>> path,
>> optional CanvasWindingRule winding);
>> On 2014/01/23 21:17:17, dshwang wrote:
>> > why "Path? path", instead of "Path path"?
>> >
>> > The spec:
>> >   void clip(Path path, optional CanvasFillRule fillRule = "nonzero");
>>
>
>  Done.
>>
>
>
> https://codereview.chromium.org/137353004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[jcgregorio, 2014-01-30 20:41:57]

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

https://codereview.chromium.org/137353004/diff/110001/Source/core/html/canvas...
Source/core/html/canvas/CanvasRenderingContext2D.idl:83: void fill(Path? path,
optional CanvasWindingRule winding);
Updated the .idl to have RuntimeEnabled=ExperimentalCanvasFeatures where
possible, and comments with a link to the crbug where it isn't possible.

On 2014/01/29 17:51:49, jcgregorio wrote:
> Thanks for the update and good news about the rewrite, I wasn't looking
forward
> to doing any Perl :-)
> 
> Is this really a blocker for this CL, Path is already hidden behind a
> RuntimeEnabled flag, even if fill(path, winding) isn't hidden, you could never
> call it because you would never be able to create the needed Path object.
> 
> On 2014/01/29 06:59:17, Nils Barth wrote:
> > Thanks for raising this Eric, and thanks to Joe and Stephen for analysis!
> > This is a limitation of the current bindings generator,
> > and has actually come up another time recently.
> > 
> > I've opened a bug for this:
> > [RuntimeEnabled] on overloaded methods
> > https://code.google.com/p/chromium/issues/detail?id=339000
> > 
> > Likely won't get to this soon (compiler rewrite), but maybe end of Feb or in
> > March.
>

[Nils Barth (inactive), 2014-01-31 00:42:37]

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

https://codereview.chromium.org/137353004/diff/110001/Source/core/html/canvas...
Source/core/html/canvas/CanvasRenderingContext2D.idl:83: void fill(Path? path,
optional CanvasWindingRule winding);
On 2014/01/30 20:41:57, jcgregorio wrote:
> Updated the .idl to have RuntimeEnabled=ExperimentalCanvasFeatures where
> possible, and comments with a link to the crbug where it isn't possible.

Thanks Joe - LGTM for IDL!

[dshwang, 2014-01-31 12:29:00]

https://codereview.chromium.org/137353004/diff/110001/LayoutTests/fast/canvas...
File LayoutTests/fast/canvas/script-tests/canvas-path-context-stroke.js (right):

https://codereview.chromium.org/137353004/diff/110001/LayoutTests/fast/canvas...
LayoutTests/fast/canvas/script-tests/canvas-path-context-stroke.js:46:
testStrokeWith(path);
On 2014/01/29 17:51:49, jcgregorio wrote:
> On 2014/01/23 21:17:17, dshwang wrote:
> > I'm curious how result is changed if following code added.
> > 
> > ctx.scale(2);
> > testStrokeWith();
> > testStrokeWith(path);
> > 
> > I guess testStrokeWith() draws x1 rect because ctx.scale(2) deals with
> ctx.path,
> > but testStrokeWith(path) draws x2 rect because ctx.scale(2) does not do
> anything
> > about path.
> 
> Actually, my reading of the spec is that they should both do the same thing,
> i.e. both be
> scaled by 2, that the current transformation matrix applies to both.
> 
>  
>
http://www.whatwg.org/specs/web-apps/current-work/multipage/the-canvas-elemen...
> 
> > 
> > The spec is vague about this case.
> 

Your impl scales by 2 into only path object.

See CanvasRenderingContext2D::scale(). All ctx transforms apply inverse matrix
to m_path. So the drawing results of m_path and new path object are different
after applying any transform.

void CanvasRenderingContext2D::scale(float sx, float sy)
{
    ...
    c->scale(FloatSize(sx, sy));
    m_path.transform(AffineTransform().scaleNonUniform(1.0 / sx, 1.0 / sy));
}

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

https://codereview.chromium.org/137353004/diff/110001/Source/core/html/canvas...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:1002: return;
On 2014/01/29 17:51:49, jcgregorio wrote:
> Updated IDL to match the spec, Path is no longer optional.
> 
> On 2014/01/23 21:17:17, dshwang wrote:
> > Could v8 binder fire dom exception if domPath is null?
> > I ask because the first argument is not optional.
> > 
> > The spec:
> >   void clip(Path path, optional CanvasFillRule fillRule = "nonzero");
> 

I mean this 'if statement' is not necessary because path is not optional.
V8 binding code should emit exception if domPath is null.

[jcgregorio, 2014-01-31 18:08:48]

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

https://codereview.chromium.org/137353004/diff/110001/Source/core/html/canvas...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:1002: return;
On 2014/01/31 12:29:00, dshwang wrote:
> On 2014/01/29 17:51:49, jcgregorio wrote:
> > Updated IDL to match the spec, Path is no longer optional.
> > 
> > On 2014/01/23 21:17:17, dshwang wrote:
> > > Could v8 binder fire dom exception if domPath is null?
> > > I ask because the first argument is not optional.
> > > 
> > > The spec:
> > >   void clip(Path path, optional CanvasFillRule fillRule = "nonzero");
> > 
> 
> I mean this 'if statement' is not necessary because path is not optional.
> V8 binding code should emit exception if domPath is null. 

Done.

[jcgregorio, 2014-01-31 18:14:17]

On Fri, Jan 31, 2014 at 7:29 AM, <dongseong.hwang@intel.com> wrote:

>
> https://codereview.chromium.org/137353004/diff/110001/
> LayoutTests/fast/canvas/script-tests/canvas-path-context-stroke.js
> File LayoutTests/fast/canvas/script-tests/canvas-path-context-stroke.js
> (right):
>
> https://codereview.chromium.org/137353004/diff/110001/
> LayoutTests/fast/canvas/script-tests/canvas-path-
> context-stroke.js#newcode46
> LayoutTests/fast/canvas/script-tests/canvas-path-context-stroke.js:46:
> testStrokeWith(path);
> On 2014/01/29 17:51:49, jcgregorio wrote:
>
>> On 2014/01/23 21:17:17, dshwang wrote:
>> > I'm curious how result is changed if following code added.
>> >
>> > ctx.scale(2);
>> > testStrokeWith();
>> > testStrokeWith(path);
>> >
>> > I guess testStrokeWith() draws x1 rect because ctx.scale(2) deals
>>
> with
>
>> ctx.path,
>> > but testStrokeWith(path) draws x2 rect because ctx.scale(2) does not
>>
> do
>
>> anything
>> > about path.
>>
>
>  Actually, my reading of the spec is that they should both do the same
>>
> thing,
>
>> i.e. both be
>> scaled by 2, that the current transformation matrix applies to both.
>>
>
>
>
> http://www.whatwg.org/specs/web-apps/current-work/
> multipage/the-canvas-element.html#transformations
>
>  >
>> > The spec is vague about this case.
>>
>
>
> Your impl scales by 2 into only path object.
>
> See CanvasRenderingContext2D::scale(). All ctx transforms apply inverse
> matrix to m_path. So the drawing results of m_path and new path object
> are different after applying any transform.
>
> void CanvasRenderingContext2D::scale(float sx, float sy)
> {
>     ...
>     c->scale(FloatSize(sx, sy));
>     m_path.transform(AffineTransform().scaleNonUniform(1.0 / sx, 1.0 /
> sy));
>
> }
>

Sorry, I'm still not following, both fill(winding) and fill(path, winding)
end up calling
fillImpl(path, winding), which does the same thing for either the path
passed in
or m_path.

For example, if I load the attached 2x.html file with this patch applied
I get the results in 2x.png, i.e. the scale(2, 2) applies the same to both
the direct fill on canvas and to the fill that takes a path argument. Is
this
what you would expect to happen?

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[dshwang, 2014-02-03 12:59:54]

On 2014/01/31 18:08:48, jcgregorio wrote:
>
https://codereview.chromium.org/137353004/diff/110001/Source/core/html/canvas...
> File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):
> 
>
https://codereview.chromium.org/137353004/diff/110001/Source/core/html/canvas...
> Source/core/html/canvas/CanvasRenderingContext2D.cpp:1002: return;
> On 2014/01/31 12:29:00, dshwang wrote:
> > On 2014/01/29 17:51:49, jcgregorio wrote:
> > > Updated IDL to match the spec, Path is no longer optional.
> > > 
> > > On 2014/01/23 21:17:17, dshwang wrote:
> > > > Could v8 binder fire dom exception if domPath is null?
> > > > I ask because the first argument is not optional.
> > > > 
> > > > The spec:
> > > >   void clip(Path path, optional CanvasFillRule fillRule = "nonzero");
> > > 
> > 
> > I mean this 'if statement' is not necessary because path is not optional.
> > V8 binding code should emit exception if domPath is null. 
> 
> Done.

Could you cover it by layout test?

On 2014/01/31 18:14:17, jcgregorio wrote:
> On Fri, Jan 31, 2014 at 7:29 AM, <mailto:dongseong.hwang@intel.com> wrote:
> 
> >
> > https://codereview.chromium.org/137353004/diff/110001/
> > LayoutTests/fast/canvas/script-tests/canvas-path-context-stroke.js
> > File LayoutTests/fast/canvas/script-tests/canvas-path-context-stroke.js
> > (right):
> >
> > https://codereview.chromium.org/137353004/diff/110001/
> > LayoutTests/fast/canvas/script-tests/canvas-path-
> > context-stroke.js#newcode46
> > LayoutTests/fast/canvas/script-tests/canvas-path-context-stroke.js:46:
> > testStrokeWith(path);
> > On 2014/01/29 17:51:49, jcgregorio wrote:
> >
> >> On 2014/01/23 21:17:17, dshwang wrote:
> >> > I'm curious how result is changed if following code added.
> >> >
> >> > ctx.scale(2);
> >> > testStrokeWith();
> >> > testStrokeWith(path);
> >> >
> >> > I guess testStrokeWith() draws x1 rect because ctx.scale(2) deals
> >>
> > with
> >
> >> ctx.path,
> >> > but testStrokeWith(path) draws x2 rect because ctx.scale(2) does not
> >>
> > do
> >
> >> anything
> >> > about path.
> >>
> >
> >  Actually, my reading of the spec is that they should both do the same
> >>
> > thing,
> >
> >> i.e. both be
> >> scaled by 2, that the current transformation matrix applies to both.
> >>
> >
> >
> >
> > http://www.whatwg.org/specs/web-apps/current-work/
> > multipage/the-canvas-element.html#transformations
> >
> >  >
> >> > The spec is vague about this case.
> >>
> >
> >
> > Your impl scales by 2 into only path object.
> >
> > See CanvasRenderingContext2D::scale(). All ctx transforms apply inverse
> > matrix to m_path. So the drawing results of m_path and new path object
> > are different after applying any transform.
> >
> > void CanvasRenderingContext2D::scale(float sx, float sy)
> > {
> >     ...
> >     c->scale(FloatSize(sx, sy));
> >     m_path.transform(AffineTransform().scaleNonUniform(1.0 / sx, 1.0 /
> > sy));
> >
> > }
> >
> 
> Sorry, I'm still not following, both fill(winding) and fill(path, winding)
> end up calling
> fillImpl(path, winding), which does the same thing for either the path
> passed in
> or m_path.

After scaling, when you call both fill(winding) and fill(path, winding), path
and m_path are different.

ctx.scale(2); <--- Because this op only affects m_path.
testStrokeWith();
testStrokeWith(path);

> For example, if I load the attached 2x.html file with this patch applied
> I get the results in 2x.png, i.e. the scale(2, 2) applies the same to both
> the direct fill on canvas and to the fill that takes a path argument. Is
> this
> what you would expect to happen?

The result is different as where you apply scale.

Case 1)
var path = new Path();
drawRectangleOn(path);
ctx.scale(2);
testStrokeWith();
testStrokeWith(path);

Expected results: both are scaled by 2

Case 2)
var path = new Path();
drawRectangleOn(path);
testStrokeWith();
testStrokeWith(path);
ctx.scale(2);

Expected results: m_path (== ctx.path) is scaled by 1 while path is scaled by 2.

I think this non-intuitive behavior is not intended from spec author. I wish
this is resolved in spec level before implementation.

@Rik wdyt?

[dshwang, 2014-02-03 13:37:32]

I correct above pseudo code to not make any confusion.

Case 1)
var ctx = document.getElementById('canvas').getContext('2d');
var path = new Path();
ctx.scale(2, 2);
ctx.rect(0, 0, 100, 100);
path.rect(0, 0, 100, 100);
ctx.fill();
ctx.fill(path);

Expected results: both are scaled by 2

Case 2)
var ctx = document.getElementById('canvas').getContext('2d');
var path = new Path();
ctx.rect(0, 0, 100, 100);
path.rect(0, 0, 100, 100);
ctx.scale(2, 2); <--- it applies inverse matrix to m_path
ctx.fill();
ctx.fill(path);

Expected results: m_path (== ctx.path) is scaled by 1 while path is scaled by 2.

[jcgregorio, 2014-02-03 13:55:44]

On Mon, Feb 3, 2014 at 8:37 AM, <dongseong.hwang@intel.com> wrote:

> I correct above pseudo code to not make any confusion.
>
> Case 1)
> var ctx = document.getElementById('canvas').getContext('2d');
>
> var path = new Path();
> ctx.scale(2, 2);
> ctx.rect(0, 0, 100, 100);
> path.rect(0, 0, 100, 100);
> ctx.fill();
> ctx.fill(path);
>
>
> Expected results: both are scaled by 2
>
> Case 2)
> var ctx = document.getElementById('canvas').getContext('2d');
>
> var path = new Path();
> ctx.rect(0, 0, 100, 100);
> path.rect(0, 0, 100, 100);
> ctx.scale(2, 2); <--- it applies inverse matrix to m_path
> ctx.fill();
> ctx.fill(path);
>
>
> Expected results: m_path (== ctx.path) is scaled by 1 while path is scaled
> by 2.
>

That's exactly what I would expect to happen according to the current
wording of the canvas spec:

When the intended path is a Path object, the coordinates and lines of its
subpaths must be transformed according to theCanvasRenderingContext2D
object's current transformation matrix when used by these methods (without
affecting the Path object itself). When the intended path is the current
default path, it is not affected by the transform. (This is because
transformations already affect the current default path when it is
constructed, so applying it when it is painted as well would result in a
double transformation.)


>
> https://codereview.chromium.org/137353004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[jcgregorio, 2014-02-03 14:24:24]

On 2014/02/03 12:59:54, dshwang wrote:
> > > On 2014/01/29 17:51:49, jcgregorio wrote:
> > 
> > Done.
> 
> Could you cover it by layout test?

Done.

[Rik, 2014-02-07 05:17:10]

On 2014/02/03 12:59:54, dshwang wrote:
> 
> Case 1)
> var path = new Path();
> drawRectangleOn(path);
> ctx.scale(2);
> testStrokeWith();
> testStrokeWith(path);
> 
> Expected results: both are scaled by 2
> 
> Case 2)
> var path = new Path();
> drawRectangleOn(path);
> testStrokeWith();
> testStrokeWith(path);
> ctx.scale(2);
> 
> Expected results: m_path (== ctx.path) is scaled by 1 while path is scaled by
2.
> 
> I think this non-intuitive behavior is not intended from spec author. I wish
> this is resolved in spec level before implementation.
> 
> @Rik wdyt?

I'm unsure what the question is. Is m_path ever visible by the authors?
Actually, m_path must be scaled by .5 to compensate for ctx.scale(2);

Let's take the following code:
var path = new Path;
Path.rect(0,0,100,100);
ctx.rect(0,0,100,100);
ctx.scale(2);
ctx.stroke()
ctx.stroke(path);

expected results: a rectangle 100x100 + 200x200

[jcgregorio, 2014-02-19 13:45:12]

Ping.

I believe all the concerns raised to date have been addressed.

[Justin Novosad, 2014-02-19 15:36:41]

Since the Path constructor is hidden behind the experimental flag, I think we
are not exposing much by not having some of the new path methods hidden.  

LGTM with nit.

https://codereview.chromium.org/137353004/diff/420001/Source/core/html/canvas...
File Source/core/html/canvas/CanvasRenderingContext2D.h (right):

https://codereview.chromium.org/137353004/diff/420001/Source/core/html/canvas...
Source/core/html/canvas/CanvasRenderingContext2D.h:314: void fillImpl(const
Path&, const String& windingRuleString);
Blinkism nit: Impl->Internal

[eseidel, 2014-02-19 15:44:24]

Since we're not guarding these, due to a bug, that means we are actually
shipping these live to all users.  Once we ship, it's very difficult to unship.

We really should just fix the generator bug so we can actually follow our
process.  The process itself isn't important, so much as the ability to change
the APIs w/o accidentally breaking developers when the spec changes tomorrow
before other browsers ship.

[eseidel, 2014-02-19 15:45:03]

I'm really not trying to make your life difficult.  We just have no way to
unship things once we've shipped them.  Or at least our ways are long and
difficult. :(

[Justin Novosad, 2014-02-19 15:57:07]

Eric, just to be clear, why is it hard to unship a method that cannot be called?
new Path() -> not exposed
CanvasRenderingContext2D.fill(Path) -> technically exposed, but unusable because
there is no way of obtaining a Path object

Joe: have you tried different ways of working around the bug?  Have you tried
getting rid of 'optional' and doing this instead in the IDL:

void fill();
void fill(CanvasWindingRule winding);
[RuntimeEnabled=ExperimentalCanvasFeatures] void fill(Path path);
[RuntimeEnabled=ExperimentalCanvasFeatures] void fill(Path path,
CanvasWindingRule winding);

[Justin Novosad, 2014-02-19 16:03:01]

On 2014/02/19 15:57:07, junov wrote:
> Joe: have you tried different ways of working around the bug?  Have you tried
> getting rid of 'optional' and doing this instead in the IDL:
> 
> void fill();
> void fill(CanvasWindingRule winding);
> [RuntimeEnabled=ExperimentalCanvasFeatures] void fill(Path path);
> [RuntimeEnabled=ExperimentalCanvasFeatures] void fill(Path path,
> CanvasWindingRule winding);

FWIW, this worked with the flavors of drawImage that take an ImageBitmap
argument. IIRC you have to put the runtime enabled overloads last, and specify
all overloads explicitly (no 'optional').

[Inactive, 2014-02-19 16:06:23]

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

https://codereview.chromium.org/137353004/diff/420001/Source/core/html/canvas...
Source/core/html/canvas/CanvasRenderingContext2D.idl:84: void fill(Path path,
optional CanvasWindingRule winding);
Until the generator is fixed, you could mark the method as [Custom] and handle
the overloading manually in the custom bindings, with a proper runtime enabled
flag check to handle 2 args.

[jcgregorio, 2014-02-19 16:12:25]

On Wed, Feb 19, 2014 at 10:57 AM, <junov@chromium.org> wrote:

> Eric, just to be clear, why is it hard to unship a method that cannot be
> called?
> new Path() -> not exposed
> CanvasRenderingContext2D.fill(Path) -> technically exposed, but unusable
> because
> there is no way of obtaining a Path object
>
> Joe: have you tried different ways of working around the bug?  Have you
> tried
> getting rid of 'optional' and doing this instead in the IDL:
>
> void fill();
> void fill(CanvasWindingRule winding);
> [RuntimeEnabled=ExperimentalCanvasFeatures] void fill(Path path);
> [RuntimeEnabled=ExperimentalCanvasFeatures] void fill(Path path,
> CanvasWindingRule winding);
>

I hadn't thought of doing that, will give it a try, if that doesn't work it
looks like I have [Custom]
to fall back on.

  Thanks,
  -joe


>
>
> https://codereview.chromium.org/137353004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[eseidel, 2014-02-19 16:21:54]

Sorry, maybe I'm missing context.  I was not aware that these methods can't be
called (that the guard already happens at a class level?)

Let me just give you some context, since you all are much closer to the
knowledge necessary to make these decisions:

- We have no deprecation strategy for the web.  We're trying hard to build one,
but right our best efforts can remove something up to about 0.06% usage, and
only after a long drawn-out process.  We even have features with *lower* usage
than this which we've struggled to remove (XSLT comes to mind).
- This is in part due to JS's fail-fast design (if you remove something it turns
into a "can't call undefined" exception and breaks someone page).
- Everything we do is live. Trunk is always shippable.  Branches can happen at
any time.
- Someone uses most everything we ship.  We have individuals who scour Chrome
for new features.  If someone blogs about draw(path, style), removing it will
either break users or take us a long time.
- Shipping also imposes a constraint on other engines.  We take a part of the
namespace, influence spec authors to commit to designs other engines don't like,
etc.

As a result of all this we try to be extremely cautious/conscious about what we
ship.

(Our process is also public and involves the whole community as the community
commits to maintaining an API, rather than an individual.)

I'm fully supportive of this feature.  Happy for Blink to ship it whenever you
believe it's ready.  Happy to l-g-t-m this change if needed, I just want to be
upfront about the possible risks.  Reading this before, it seemed silly to
unintentionally ship a feature because of a bug in our code generator.

If that's not the case, and this isn't actually accessible from the web due to
other runtime guards, than great, commit away!

[jcgregorio, 2014-02-19 16:30:59]

On Wed, Feb 19, 2014 at 11:21 AM, <eseidel@chromium.org> wrote:

> Sorry, maybe I'm missing context.  I was not aware that these methods
> can't be
> called (that the guard already happens at a class level?)
>
> Let me just give you some context, since you all are much closer to the
> knowledge necessary to make these decisions:
>
> - We have no deprecation strategy for the web.  We're trying hard to build
> one,
> but right our best efforts can remove something up to about 0.06% usage,
> and
> only after a long drawn-out process.  We even have features with *lower*
> usage
> than this which we've struggled to remove (XSLT comes to mind).
> - This is in part due to JS's fail-fast design (if you remove something it
> turns
> into a "can't call undefined" exception and breaks someone page).
> - Everything we do is live. Trunk is always shippable.  Branches can
> happen at
> any time.
> - Someone uses most everything we ship.  We have individuals who scour
> Chrome
> for new features.  If someone blogs about draw(path, style), removing it
> will
> either break users or take us a long time.
> - Shipping also imposes a constraint on other engines.  We take a part of
> the
> namespace, influence spec authors to commit to designs other engines don't
> like,
> etc.
>
> As a result of all this we try to be extremely cautious/conscious about
> what we
> ship.
>
> (Our process is also public and involves the whole community as the
> community
> commits to maintaining an API, rather than an individual.)
>
> I'm fully supportive of this feature.  Happy for Blink to ship it whenever
> you
> believe it's ready.  Happy to l-g-t-m this change if needed, I just want
> to be
> upfront about the possible risks.  Reading this before, it seemed silly to
> unintentionally ship a feature because of a bug in our code generator.
>
> If that's not the case, and this isn't actually accessible from the web
> due to
> other runtime guards, than great, commit away!
>

I believe that this is the case with the CL right now, that it isn't
actually accessible from the web due to
other runtime guards, but if I can make that even more explicit by
rearranging the IDL like junov
suggested, then I'd like to make that change before proceeding.


>
> https://codereview.chromium.org/137353004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[jcgregorio, 2014-02-20 18:48:52]

Updated the .idl file so that all the methods that take Path parameters are
behind [RuntimeEnabled=ExperimentalCanvasFeatures].

https://codereview.chromium.org/137353004/diff/420001/Source/core/html/canvas...
File Source/core/html/canvas/CanvasRenderingContext2D.h (right):

https://codereview.chromium.org/137353004/diff/420001/Source/core/html/canvas...
Source/core/html/canvas/CanvasRenderingContext2D.h:314: void fillImpl(const
Path&, const String& windingRuleString);
On 2014/02/19 15:36:41, junov wrote:
> Blinkism nit: Impl->Internal

Done.

[eseidel, 2014-02-20 18:59:51]

lgtm

Other than the this->, this seems fine.

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

https://codereview.chromium.org/137353004/diff/930001/Source/core/html/canvas...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:918:
this->fillInternal(m_path, windingRuleString);
Why is this-> needed?

https://codereview.chromium.org/137353004/diff/930001/Source/core/html/canvas...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:929: return;
This CL inconsistently uses { } around oneline ifs.  Historically WebKit didn't
use these, but I Think that's been slowly changing in Blink.  I don't feel
strongly, but it seems a bit odd for some lines to have it and some not.

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

https://codereview.chromium.org/137353004/diff/930001/Source/core/html/canvas...
Source/core/html/canvas/CanvasRenderingContext2D.idl:86: void
clip(CanvasWindingRule winding);
Some of these are redundant, presumably to work around a generator bug?  Should
we add a // FIXME: here for when that bug is resolved?

[jcgregorio, 2014-02-20 19:28:29]

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

https://codereview.chromium.org/137353004/diff/930001/Source/core/html/canvas...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:918:
this->fillInternal(m_path, windingRuleString);
On 2014/02/20 18:59:52, eseidel wrote:
> Why is this-> needed?

It's not, fixed. (I'm confusing blink style with skia style.)

https://codereview.chromium.org/137353004/diff/930001/Source/core/html/canvas...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:929: return;
Fixed all the functions I touched in this CL to use {} around one-line ifs.

On 2014/02/20 18:59:52, eseidel wrote:
> This CL inconsistently uses { } around oneline ifs.  Historically WebKit
didn't
> use these, but I Think that's been slowly changing in Blink.  I don't feel
> strongly, but it seems a bit odd for some lines to have it and some not.

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

https://codereview.chromium.org/137353004/diff/930001/Source/core/html/canvas...
Source/core/html/canvas/CanvasRenderingContext2D.idl:86: void
clip(CanvasWindingRule winding);
On 2014/02/20 18:59:52, eseidel wrote:
> Some of these are redundant, presumably to work around a generator bug? 
Should
> we add a // FIXME: here for when that bug is resolved?

Done.

[jcgregorio, 2014-02-24 17:39:27]

The CQ bit was checked by jcgregorio@google.com

[commit-bot: I haz the power, 2014-02-24 17:39:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jcgregorio@google.com/137353004/1150001

[commit-bot: I haz the power, 2014-02-24 22:12:43]

Change committed as 167705

[Nils Barth (inactive), 2014-06-13 02:33:00]

BTW, the cruft in the use of [RuntimeEnabled] can now be fixed up
(thanks to bindings CG changes by Jens), which I've done in:
Cleanup [RuntimeEnabled] use in CanvasRenderingContext2D
https://codereview.chromium.org/334803002/

...and we can avoid these in future!