Use a union typdef for the return type of canvas.getContext

Use a union typdef for the return type of canvas.getContext

This CL changes the IDL of getContext to match the specification

BUG=592091
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel

Committed: https://crrev.com/11260a2fbc93d4ac6d6bb328b9c66c5fe1432fe8
Cr-Commit-Position: refs/heads/master@{#380482}

[Justin Novosad, 2016-03-04 04:49:40]

Description was changed from

==========
Use a union typdef for the return type of canvas.getContext

This CL changes the IDL of getContext to match the specification

BUG=
==========

to

==========
Use a union typdef for the return type of canvas.getContext

This CL changes the IDL of getContext to match the specification

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

[Justin Novosad, 2016-03-04 05:59:17]

junov@chromium.org changed reviewers:
+ kbr@chromium.org

[Justin Novosad, 2016-03-04 05:59:17]

@kbr: This undoes part of this change you submitted over a year ago:
https://codereview.chromium.org/795833004
We can now go back to using a typedef return type instead of 'any', and we no
longer need to call with ScriptState. This does not break any of the inspector
tests.

Because the contexts are all in modules now, I had to jump through a few hoops
to put the typedef in modules and move getContext into a partial interface that
lives in a module. In the end, there are no layering violations.

[Justin Novosad, 2016-03-04 06:16:31]

FWIW, in case anyone would like to suggest better names, I am not particularly
proud of the names setCanvasGetContextResult and HTMLCanvasElementModule.

[Ken Russell (switch to Gerrit), 2016-03-04 18:36:30]

Please file a bug, block it on Issue 436217, and reference it from the CL. I
don't support committing stray CLs that make anything except trivial changes,
and this is not one of them. Thanks.

[Justin Novosad, 2016-03-04 20:41:41]

On 2016/03/04 18:36:30, Ken Russell wrote:
> Please file a bug, block it on Issue 436217, and reference it from the CL. I
> don't support committing stray CLs that make anything except trivial changes,
> and this is not one of them. Thanks.

Of course, I did not mean to make this a stray patch. I just uploaded this CL as
an experiment, expecting some things might explode, which they did not. Yay!
Looks like we could actually land this. I am still wondering if matching the IDL
from the spec justifies the complexity of tearing-out getContext into a module.
Ideally I would have liked to be able to solve this using a "partial typedef".
But is too wild a dream, I think.

[Justin Novosad, 2016-03-04 20:46:32]

Description was changed from

==========
Use a union typdef for the return type of canvas.getContext

This CL changes the IDL of getContext to match the specification

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

to

==========
Use a union typdef for the return type of canvas.getContext

This CL changes the IDL of getContext to match the specification

BUG=592091
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

[Justin Novosad, 2016-03-07 22:08:02]

Forgot to mention: new patch!

[Ken Russell (switch to Gerrit), 2016-03-09 19:59:38]

kbr@chromium.org changed reviewers:
+ haraken@chromium.org, sigbjornf@opera.com

[Ken Russell (switch to Gerrit), 2016-03-09 19:59:38]

Thanks for filing the bug. LGTM overall; would like to ask the Oilpan folks to
take a quick look too.

https://codereview.chromium.org/1761003003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp
(right):

https://codereview.chromium.org/1761003003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp:132:
result.setCanvasRenderingContext2D(RefPtrWillBeRawPtr<CanvasRenderingContext2D>(this));
Here and throughout: is this the correct type to use at this point in Oilpan's
development? (+haraken, sof)

[Justin Novosad, 2016-03-09 20:31:36]

On 2016/03/09 19:59:38, Ken Russell wrote:

>
https://codereview.chromium.org/1761003003/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp:132:
>
result.setCanvasRenderingContext2D(RefPtrWillBeRawPtr<CanvasRenderingContext2D>(this));
> Here and throughout: is this the correct type to use at this point in Oilpan's
> development? (+haraken, sof)

CanvasRenderingContext does not inherit RefCounted: it has its custom ref() and
unref() methods that tie it to the parent canvas element. Ripping that out is
orthogonal to this change IMHO.

[sof, 2016-03-09 20:46:30]

https://codereview.chromium.org/1761003003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp
(right):

https://codereview.chromium.org/1761003003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp:132:
result.setCanvasRenderingContext2D(RefPtrWillBeRawPtr<CanvasRenderingContext2D>(this));
On 2016/03/09 19:59:38, Ken Russell wrote:
> Here and throughout: is this the correct type to use at this point in Oilpan's
> development? (+haraken, sof)

A transition type is still in order & neeeded; when/if Oilpan is fully switched
to the generated code for unions will use CanvasRenderingContext2D* for this
setter.

But shouldn't it be PassRefPtrWillBeRawPtr<>?

[Justin Novosad, 2016-03-09 20:53:50]

On 2016/03/09 20:46:30, sof wrote:
> But shouldn't it be PassRefPtrWillBeRawPtr<>?
Indeed. Good catch.

[Justin Novosad, 2016-03-09 21:25:30]

The CQ bit was checked by junov@chromium.org

[Justin Novosad, 2016-03-09 21:25:30]

The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org
Link to the patchset: https://codereview.chromium.org/1761003003/#ps100001
(title: "*Pass*RefPtrWillBeRawPtr")

[commit-bot: I haz the power, 2016-03-09 21:26:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1761003003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1761003003/100001

[Justin Novosad, 2016-03-09 21:31:23]

The CQ bit was unchecked by junov@chromium.org

[sof, 2016-03-09 22:05:26]

https://codereview.chromium.org/1761003003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.h
(right):

https://codereview.chromium.org/1761003003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.h:86: void
setCanvasGetContextResult(RenderingContext&) final;
"final" on a "final" class - i'm (honestly) unfamiliar with that idiom - what
are trying to state?

https://codereview.chromium.org/1761003003/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/imagebitmap/ImageBitmapRenderingContext.cpp
(right):

https://codereview.chromium.org/1761003003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/imagebitmap/ImageBitmapRenderingContext.cpp:25:
result.setImageBitmapRenderingContext(RefPtrWillBeRawPtr<ImageBitmapRenderingContext>(this));
Another PassRefPtr<> needed here.

https://codereview.chromium.org/1761003003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContext.cpp (right):

https://codereview.chromium.org/1761003003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContext.cpp:81:
result.setWebGL2RenderingContext(RefPtrWillBeRawPtr<WebGL2RenderingContext>(this));
PassRefPtr<>

https://codereview.chromium.org/1761003003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webgl/WebGLRenderingContext.cpp (right):

https://codereview.chromium.org/1761003003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContext.cpp:110:
result.setWebGLRenderingContext(RefPtrWillBeRawPtr<WebGLRenderingContext>(this));
PassRefPtr<>

[Justin Novosad, 2016-03-09 22:31:00]

https://codereview.chromium.org/1761003003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.h
(right):

https://codereview.chromium.org/1761003003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.h:86: void
setCanvasGetContextResult(RenderingContext&) final;
On 2016/03/09 22:05:26, sof wrote:
> "final" on a "final" class - i'm (honestly) unfamiliar with that idiom - what
> are trying to state?

Since the class is "final" there is no technical difference between using
"final" or "override" here. I looked at the style guide section on C++11
features used in Chromium and there is no specific guidance on which is
preferred on a class that is "final".

[Justin Novosad, 2016-03-09 22:37:32]

new patch

[haraken, 2016-03-10 01:24:59]

LGTM

https://codereview.chromium.org/1761003003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/canvas/HTMLCanvasElementModule.h (right):

https://codereview.chromium.org/1761003003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/canvas/HTMLCanvasElementModule.h:17: class
HTMLCanvasElementModule {

Add STATIC_ONLY.

https://codereview.chromium.org/1761003003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/canvas/HTMLCanvasElementModule.idl
(right):

https://codereview.chromium.org/1761003003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/canvas/HTMLCanvasElementModule.idl:17: // (and
is not undefined or null). The binding must ignore this.

Would you file a bug for this?

[Justin Novosad, 2016-03-10 17:35:44]

On 2016/03/10 01:24:59, haraken wrote:
>
https://codereview.chromium.org/1761003003/diff/120001/third_party/WebKit/Sou...
> third_party/WebKit/Source/modules/canvas/HTMLCanvasElementModule.idl:17: //
(and
> is not undefined or null). The binding must ignore this.
> 
> Would you file a bug for this?

For now, Chrome is conforming to the weird state of things in the current spec. 
I don't think we should do anything about this special case until the spec is
fixed.
There is a bug filed against the spec for this:
https://github.com/whatwg/html/issues/595
I will add a comment to out IDL that references the spec issue.

[Justin Novosad, 2016-03-10 17:52:18]

The CQ bit was checked by junov@chromium.org

[Justin Novosad, 2016-03-10 17:52:19]

The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org,
haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/1761003003/#ps140001
(title: "final touches")

[commit-bot: I haz the power, 2016-03-10 17:52:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1761003003/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1761003003/140001

[commit-bot: I haz the power, 2016-03-10 22:56:57]

Description was changed from

==========
Use a union typdef for the return type of canvas.getContext

This CL changes the IDL of getContext to match the specification

BUG=592091
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

to

==========
Use a union typdef for the return type of canvas.getContext

This CL changes the IDL of getContext to match the specification

BUG=592091
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

[commit-bot: I haz the power, 2016-03-10 22:56:59]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-03-10 22:58:25]

Description was changed from

==========
Use a union typdef for the return type of canvas.getContext

This CL changes the IDL of getContext to match the specification

BUG=592091
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

to

==========
Use a union typdef for the return type of canvas.getContext

This CL changes the IDL of getContext to match the specification

BUG=592091
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel

Committed: https://crrev.com/11260a2fbc93d4ac6d6bb328b9c66c5fe1432fe8
Cr-Commit-Position: refs/heads/master@{#380482}
==========

[commit-bot: I haz the power, 2016-03-10 22:58:26]

Patchset 8 (id:??) landed as
https://crrev.com/11260a2fbc93d4ac6d6bb328b9c66c5fe1432fe8
Cr-Commit-Position: refs/heads/master@{#380482}