988409 - Turn on Path2D by default

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[Rik Cabanier]

See intent to ship thread: https://groups.google.com/forum/#!topic/mozilla.dev.platform/efoymZ_gk6I

Comment 1

[Rik Cabanier]

Attachment: Turn on Path2D by default

try run: https://tbpl.mozilla.org/?tree=Try&rev=32cf5a740909

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

What's up with the repeated changes with the review requestee?

Comment 3

[Rik Cabanier]

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-7) from comment #2)
> What's up with the repeated changes with the review requestee?

Nothing. I'm trying to find someone who will review and there was no response on IRC.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8397419 [details] [diff] [review]
Turn on Path2D by default

Review of attachment 8397419 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/tests/mochitest/general/test_interfaces.html
@@ +735,5 @@
>      "PaintRequestList",
>  // IMPORTANT: Do not change this list without review from a DOM peer!
>      "PannerNode",
>  // IMPORTANT: Do not change this list without review from a DOM peer!
> +    "Path2D",

Just add pref: "canvas.path.enabled" like we do for other interfaces in this list --- and make that its own patch.

Comment 5

[Rik Cabanier]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #4)
> Comment on attachment 8397419 [details] [diff] [review]
> Turn on Path2D by default
> 
> Review of attachment 8397419 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/tests/mochitest/general/test_interfaces.html
> @@ +735,5 @@
> >      "PaintRequestList",
> >  // IMPORTANT: Do not change this list without review from a DOM peer!
> >      "PannerNode",
> >  // IMPORTANT: Do not change this list without review from a DOM peer!
> > +    "Path2D",
> 
> Just add pref: "canvas.path.enabled" like we do for other interfaces in this
> list --- and make that its own patch.

This patch does that too. see modules/libpref/src/init/all.js
Are you saying as I should break this in 2 patches?

Comment 6

[(no longer active)]

(In reply to Rik Cabanier from comment #5)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #4)
> > Comment on attachment 8397419 [details] [diff] [review]
> > Turn on Path2D by default
> > 
> > Review of attachment 8397419 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/tests/mochitest/general/test_interfaces.html
> > @@ +735,5 @@
> > >      "PaintRequestList",
> > >  // IMPORTANT: Do not change this list without review from a DOM peer!
> > >      "PannerNode",
> > >  // IMPORTANT: Do not change this list without review from a DOM peer!
> > > +    "Path2D",

This change really needs an r+ from a DOM peer.  Those comments should make that *very* clear.

Comment 7

[Rik Cabanier]

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #6)
> (In reply to Rik Cabanier from comment #5)
> > (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #4)
> > > Comment on attachment 8397419 [details] [diff] [review]
> > > Turn on Path2D by default
> > > 
> > > Review of attachment 8397419 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > ::: dom/tests/mochitest/general/test_interfaces.html
> > > @@ +735,5 @@
> > > >      "PaintRequestList",
> > > >  // IMPORTANT: Do not change this list without review from a DOM peer!
> > > >      "PannerNode",
> > > >  // IMPORTANT: Do not change this list without review from a DOM peer!
> > > > +    "Path2D",
> 
> This change really needs an r+ from a DOM peer.  Those comments should make
> that *very* clear.
Ok. Who is a DOM peer?

Comment 8

[(no longer active)]

(In reply to comment #7)
> (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> #6)
> > (In reply to Rik Cabanier from comment #5)
> > > (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #4)
> > > > Comment on attachment 8397419 [details] [diff] [review]
> > > > Turn on Path2D by default
> > > > 
> > > > Review of attachment 8397419 [details] [diff] [review]:
> > > > -----------------------------------------------------------------
> > > > 
> > > > ::: dom/tests/mochitest/general/test_interfaces.html
> > > > @@ +735,5 @@
> > > > >      "PaintRequestList",
> > > > >  // IMPORTANT: Do not change this list without review from a DOM peer!
> > > > >      "PannerNode",
> > > > >  // IMPORTANT: Do not change this list without review from a DOM peer!
> > > > > +    "Path2D",
> > 
> > This change really needs an r+ from a DOM peer.  Those comments should make
> > that *very* clear.
> Ok. Who is a DOM peer?

Please see the full list of owners and peers here: <https://wiki.mozilla.org/Modules/Core#Document_Object_Model>

Comment 9

[Rik Cabanier]

Comment on attachment 8397419 [details] [diff] [review]
Turn on Path2D by default

Thanks.
Adding Boris as an additional reviewer since this adds a user facing object.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8397419 [details] [diff] [review]
Turn on Path2D by default

This seems fine assuming we've decided on the name for sure.

I think what roc meant was changing the test to this:

   { name: "Path2D", pref: "canvas.path.enabled" },

so that it would not need to be modified again if we have to disable for some reason: it would just know that the name should be there if and only if the pref is set.

Comment 11

[Rik Cabanier]

(In reply to Boris Zbarsky [:bz] from comment #10)
> Comment on attachment 8397419 [details] [diff] [review]
> Turn on Path2D by default
> 
> This seems fine assuming we've decided on the name for sure.

WebKit has the new name and Blink just approved the new name for shipping so I think we're good :-)
 
> I think what roc meant was changing the test to this:
> 
>    { name: "Path2D", pref: "canvas.path.enabled" },

ah. I didn't know that was possible. Thanks, I will update the patch.

Comment 12

[Rik Cabanier]

Attachment: Turn on Path2D by default

Comment 13

[Rik Cabanier]

try build: https://tbpl.mozilla.org/?tree=Try&rev=a26640ccfbad

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0f3ea452319c

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0f3ea452319c

Comment 16

[Rik Cabanier]

See http://blogs.adobe.com/webplatform/2014/04/01/new-canvas-features/ for a description of the feature.

Comment 17

[Florian Scholz (Open Web Docs)]

See bug 830734 comment 67 for MDN docs.