Use visitDOMWrapper to preserve WebGL JS object wrappers

Delete old preserveObjectWrapper stuff, use visitDOMWrapper instead to preserve WebGL JS object wrappers. This covers expando-loss, expando-loss-2, new tests added in expando-loss-2, and expando tests in extension tests.

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

Committed: https://crrev.com/b7615cfadeb59ac220bd5994d4ec6f99341452f0
Cr-Commit-Position: refs/heads/master@{#416686}

[Kai Ninomiya, 2016-08-24 01:30:21]

Description was changed from

==========
[WIP, crashes] use visitDOMWrapper to preserve WebGL JS object wrappers

add extended attribute [Custom=VisitDOMWrapper]

delete old preserveObjectWrapper stuff

BUG=581997
==========

to

==========
[WIP, crashes] use visitDOMWrapper to preserve WebGL JS object wrappers

add extended attribute [Custom=VisitDOMWrapper]

delete old preserveObjectWrapper stuff

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

[Kai Ninomiya, 2016-08-24 19:30:05]

The CQ bit was checked by kainino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-24 19:31:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-24 19:37:53]

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

[commit-bot: I haz the power, 2016-08-24 19:37:54]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Kai Ninomiya, 2016-08-24 21:57:17]

The CQ bit was checked by kainino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-24 21:58:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-24 22:41:37]

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

[commit-bot: I haz the power, 2016-08-24 22:41:38]

Dry run: Try jobs failed on following builders:
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[Kai Ninomiya, 2016-08-25 00:19:12]

The CQ bit was checked by kainino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-25 00:19:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-25 02:23:13]

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

[commit-bot: I haz the power, 2016-08-25 02:23:14]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Kai Ninomiya, 2016-08-25 18:36:10]

The CQ bit was checked by kainino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-25 18:37:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-25 20:30:28]

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

[commit-bot: I haz the power, 2016-08-25 20:30:29]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Kai Ninomiya, 2016-08-25 22:26:39]

The CQ bit was checked by kainino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-25 22:27:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-26 00:22:26]

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

[commit-bot: I haz the power, 2016-08-26 00:22:27]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Kai Ninomiya, 2016-08-29 19:11:18]

The CQ bit was checked by kainino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-29 19:45:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-30 03:32:01]

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

[commit-bot: I haz the power, 2016-08-30 03:32:02]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Kai Ninomiya, 2016-08-31 17:56:22]

The CQ bit was checked by kainino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-31 17:57:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Kai Ninomiya, 2016-08-31 18:37:09]

Description was changed from

==========
[WIP, crashes] use visitDOMWrapper to preserve WebGL JS object wrappers

add extended attribute [Custom=VisitDOMWrapper]

delete old preserveObjectWrapper stuff

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

to

==========
Delete everything related to the old preserveObjectWrapper solution. Switch to
using visitDOMWrapper to preserve WebGL JS object wrappers (with a new extended
attribute [Custom=VisitDOMWrapperExtra]).

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

[commit-bot: I haz the power, 2016-08-31 19:33:25]

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

[commit-bot: I haz the power, 2016-08-31 19:33:26]

Dry run: Try jobs failed on following builders:
  mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)

[Kai Ninomiya, 2016-08-31 21:14:26]

The CQ bit was checked by kainino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-31 21:15:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Kai Ninomiya, 2016-08-31 21:23:10]

kainino@chromium.org changed reviewers:
+ haraken@chromium.org, kbr@chromium.org, yukishiino@chromium.org

[Kai Ninomiya, 2016-08-31 21:23:11]

PTAL, thanks.

Performance results:
https://crbug.com/581997#c23

[haraken, 2016-08-31 23:14:50]

https://codereview.chromium.org/2273683003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/IDLExtendedAttributes.md (right):

https://codereview.chromium.org/2273683003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/IDLExtendedAttributes.md:869: ####
[Custom=VisitDOMWrapperExtra] _(i)_

I do want to avoid introducing [Custom=VisitDOMWrapperExtra]. Would you
elaborate on why [Custom=VisitDOMWrapper] is not enough?

https://codereview.chromium.org/2273683003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webgl/WebGLObject.h (right):

https://codereview.chromium.org/2273683003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGLObject.h:83: if (scriptWrappable) {

I'd prefer moving the null check to DOMWrapperWorld.

[Ken Russell (switch to Gerrit), 2016-09-01 00:08:49]

https://codereview.chromium.org/2273683003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/IDLExtendedAttributes.md (right):

https://codereview.chromium.org/2273683003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/IDLExtendedAttributes.md:869: ####
[Custom=VisitDOMWrapperExtra] _(i)_
On 2016/08/31 23:14:50, haraken wrote:
> 
> I do want to avoid introducing [Custom=VisitDOMWrapperExtra]. Would you
> elaborate on why [Custom=VisitDOMWrapper] is not enough?
> 

As mentioned in the summary below, these classes already use the extended IDL
attribute SetWrapperReferenceFrom, which generates a visitDOMWrapper. We don't
want to copy the autogenerated code into a custom visitDOMWrapper, in case the
bindings generator changes.

[haraken, 2016-09-01 00:29:23]

On 2016/09/01 00:08:49, Ken Russell wrote:
>
https://codereview.chromium.org/2273683003/diff/140001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/bindings/IDLExtendedAttributes.md (right):
> 
>
https://codereview.chromium.org/2273683003/diff/140001/third_party/WebKit/Sou...
> third_party/WebKit/Source/bindings/IDLExtendedAttributes.md:869: ####
> [Custom=VisitDOMWrapperExtra] _(i)_
> On 2016/08/31 23:14:50, haraken wrote:
> > 
> > I do want to avoid introducing [Custom=VisitDOMWrapperExtra]. Would you
> > elaborate on why [Custom=VisitDOMWrapper] is not enough?
> > 
> 
> As mentioned in the summary below, these classes already use the extended IDL
> attribute SetWrapperReferenceFrom, which generates a visitDOMWrapper. We don't
> want to copy the autogenerated code into a custom visitDOMWrapper, in case the
> bindings generator changes.

Ah, thanks. It's only 4 lines of code, so I'd prefer just copying the generated
code to the custom bindings instead of introducing another extended IDL
attribute.

[commit-bot: I haz the power, 2016-09-01 01:55:37]

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

[commit-bot: I haz the power, 2016-09-01 01:55:38]

Dry run: This issue passed the CQ dry run.

[Ken Russell (switch to Gerrit), 2016-09-01 05:23:37]

On 2016/09/01 00:29:23, haraken wrote:
> On 2016/09/01 00:08:49, Ken Russell wrote:
> >
>
https://codereview.chromium.org/2273683003/diff/140001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/bindings/IDLExtendedAttributes.md (right):
> > 
> >
>
https://codereview.chromium.org/2273683003/diff/140001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/bindings/IDLExtendedAttributes.md:869: ####
> > [Custom=VisitDOMWrapperExtra] _(i)_
> > On 2016/08/31 23:14:50, haraken wrote:
> > > 
> > > I do want to avoid introducing [Custom=VisitDOMWrapperExtra]. Would you
> > > elaborate on why [Custom=VisitDOMWrapper] is not enough?
> > > 
> > 
> > As mentioned in the summary below, these classes already use the extended
IDL
> > attribute SetWrapperReferenceFrom, which generates a visitDOMWrapper. We
don't
> > want to copy the autogenerated code into a custom visitDOMWrapper, in case
the
> > bindings generator changes.
> 
> Ah, thanks. It's only 4 lines of code, so I'd prefer just copying the
generated
> code to the custom bindings instead of introducing another extended IDL
> attribute.

What if the style of those four lines of code changes in the code generator? We
strongly do not want to have a stale copy of this code elsewhere. I strongly
prefer to upgrade the code generator to support both a customized
VisitDOMWrapper as well as SetWrapperReferenceFrom.

[haraken, 2016-09-01 07:41:47]

On 2016/09/01 05:23:37, Ken Russell wrote:
> On 2016/09/01 00:29:23, haraken wrote:
> > On 2016/09/01 00:08:49, Ken Russell wrote:
> > >
> >
>
https://codereview.chromium.org/2273683003/diff/140001/third_party/WebKit/Sou...
> > > File third_party/WebKit/Source/bindings/IDLExtendedAttributes.md (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2273683003/diff/140001/third_party/WebKit/Sou...
> > > third_party/WebKit/Source/bindings/IDLExtendedAttributes.md:869: ####
> > > [Custom=VisitDOMWrapperExtra] _(i)_
> > > On 2016/08/31 23:14:50, haraken wrote:
> > > > 
> > > > I do want to avoid introducing [Custom=VisitDOMWrapperExtra]. Would you
> > > > elaborate on why [Custom=VisitDOMWrapper] is not enough?
> > > > 
> > > 
> > > As mentioned in the summary below, these classes already use the extended
> IDL
> > > attribute SetWrapperReferenceFrom, which generates a visitDOMWrapper. We
> don't
> > > want to copy the autogenerated code into a custom visitDOMWrapper, in case
> the
> > > bindings generator changes.
> > 
> > Ah, thanks. It's only 4 lines of code, so I'd prefer just copying the
> generated
> > code to the custom bindings instead of introducing another extended IDL
> > attribute.
> 
> What if the style of those four lines of code changes in the code generator?
We
> strongly do not want to have a stale copy of this code elsewhere. I strongly
> prefer to upgrade the code generator to support both a customized
> VisitDOMWrapper as well as SetWrapperReferenceFrom.

If you want to go with that route, you can just make the IDL compiler
unconditionally generate visitDOMWrapperCustom(). The IDL compiler can generate
an empty visitDOMWrapperCustom() for an IDL interface that doesn't have
[Custom=VisitDOMWrapper].

[Yuki, 2016-09-01 08:34:32]

Is it possible to make visitDOMWrapper call visitDOMWrapperCustom?
I'm thinking something like follows.

// code-generated
void V8T::visitDOMWrapper(...)
{
    // code for SetWrapperReferenceTo
    ...;

    // code for SetWrapperReferenceFrom
    ...;

    // only if [Custom=VisitDOMWrapper]
    V8T::visitDOMWrapperCustom(..., impl);
}

// hand-written
void V8T::visitDOMWrapperCustom(..., impl) { ...; }

We need a minor refactoring to make this happen, but I think you can inject your
custom visitDOMWrapperCustom() into generated code.  Does this make sense?

[haraken, 2016-09-01 08:35:50]

On 2016/09/01 08:34:32, Yuki wrote:
> Is it possible to make visitDOMWrapper call visitDOMWrapperCustom?
> I'm thinking something like follows.
> 
> // code-generated
> void V8T::visitDOMWrapper(...)
> {
>     // code for SetWrapperReferenceTo
>     ...;
> 
>     // code for SetWrapperReferenceFrom
>     ...;
> 
>     // only if [Custom=VisitDOMWrapper]
>     V8T::visitDOMWrapperCustom(..., impl);
> }
> 
> // hand-written
> void V8T::visitDOMWrapperCustom(..., impl) { ...; }
> 
> We need a minor refactoring to make this happen, but I think you can inject
your
> custom visitDOMWrapperCustom() into generated code.  Does this make sense?

Yes, I'm thinking along the line.

[Ken Russell (switch to Gerrit), 2016-09-01 21:03:54]

This is an excellent cleanup. It LGTM overall once haraken's and yukishiino's
comments are addressed (always just using Custom=VisitDOMWrapper and refactoring
how that works; moving the null check).

One comment in a few places in the code. Feel free to either address it or keep
things as they are, your choice.

https://codereview.chromium.org/2273683003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webgl/WebGLFramebuffer.cpp (right):

https://codereview.chromium.org/2273683003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGLFramebuffer.cpp:427:
trySetWrapperReferencesInAllWorlds(wrapper, this, isolate);
I'd suggest leaving this responsibility to the caller. It seems non-orthogonal
that visitChildDOMWrappers should also mark "this" as being used, rather than
just the objects "this" refers to.

https://codereview.chromium.org/2273683003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webgl/WebGLProgram.cpp (right):

https://codereview.chromium.org/2273683003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGLProgram.cpp:152:
trySetWrapperReferencesInAllWorlds(wrapper, this, isolate);
Same here.

https://codereview.chromium.org/2273683003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webgl/WebGLVertexArrayObjectBase.cpp
(right):

https://codereview.chromium.org/2273683003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGLVertexArrayObjectBase.cpp:145:
trySetWrapperReferencesInAllWorlds(wrapper, this, isolate);
Same comment here.

[haraken, 2016-09-01 21:10:30]

On 2016/09/01 08:35:50, haraken wrote:
> On 2016/09/01 08:34:32, Yuki wrote:
> > Is it possible to make visitDOMWrapper call visitDOMWrapperCustom?
> > I'm thinking something like follows.
> > 
> > // code-generated
> > void V8T::visitDOMWrapper(...)
> > {
> >     // code for SetWrapperReferenceTo
> >     ...;
> > 
> >     // code for SetWrapperReferenceFrom
> >     ...;
> > 
> >     // only if [Custom=VisitDOMWrapper]
> >     V8T::visitDOMWrapperCustom(..., impl);
> > }
> > 
> > // hand-written
> > void V8T::visitDOMWrapperCustom(..., impl) { ...; }
> > 
> > We need a minor refactoring to make this happen, but I think you can inject
> your
> > custom visitDOMWrapperCustom() into generated code.  Does this make sense?
> 
> Yes, I'm thinking along the line.

BTW, I'd suggest making this change in a separate CL, hopefully before landing
this CL.

[Kai Ninomiya, 2016-09-02 18:13:45]

Patchset #9 (id:160001) has been deleted

[Kai Ninomiya, 2016-09-02 18:15:15]

The CQ bit was checked by kainino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-02 18:16:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Kai Ninomiya, 2016-09-02 18:29:22]

https://codereview.chromium.org/2273683003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/IDLExtendedAttributes.md (right):

https://codereview.chromium.org/2273683003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/IDLExtendedAttributes.md:869: ####
[Custom=VisitDOMWrapperExtra] _(i)_
On 2016/09/01 00:08:49, Ken Russell wrote:
> On 2016/08/31 23:14:50, haraken wrote:
> > 
> > I do want to avoid introducing [Custom=VisitDOMWrapperExtra]. Would you
> > elaborate on why [Custom=VisitDOMWrapper] is not enough?
> > 
> 
> As mentioned in the summary below, these classes already use the extended IDL
> attribute SetWrapperReferenceFrom, which generates a visitDOMWrapper. We don't
> want to copy the autogenerated code into a custom visitDOMWrapper, in case the
> bindings generator changes.

Done.

https://codereview.chromium.org/2273683003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webgl/WebGLFramebuffer.cpp (right):

https://codereview.chromium.org/2273683003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGLFramebuffer.cpp:427:
trySetWrapperReferencesInAllWorlds(wrapper, this, isolate);
On 2016/09/01 21:03:53, Ken Russell wrote:
> I'd suggest leaving this responsibility to the caller. It seems non-orthogonal
> that visitChildDOMWrappers should also mark "this" as being used, rather than
> just the objects "this" refers to.

Done.

https://codereview.chromium.org/2273683003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webgl/WebGLObject.h (right):

https://codereview.chromium.org/2273683003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGLObject.h:83: if (scriptWrappable) {
On 2016/08/31 23:14:50, haraken wrote:
> 
> I'd prefer moving the null check to DOMWrapperWorld.

Done.

https://codereview.chromium.org/2273683003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webgl/WebGLProgram.cpp (right):

https://codereview.chromium.org/2273683003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGLProgram.cpp:152:
trySetWrapperReferencesInAllWorlds(wrapper, this, isolate);
On 2016/09/01 21:03:53, Ken Russell wrote:
> Same here.

Done.

https://codereview.chromium.org/2273683003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webgl/WebGLVertexArrayObjectBase.cpp
(right):

https://codereview.chromium.org/2273683003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGLVertexArrayObjectBase.cpp:145:
trySetWrapperReferencesInAllWorlds(wrapper, this, isolate);
On 2016/09/01 21:03:53, Ken Russell wrote:
> Same comment here.

Done.

[haraken, 2016-09-02 19:08:40]

haraken@chromium.org changed reviewers:
+ hlopko@chromium.org, hpayer@chromium.org, mlippautz@chromium.org

[haraken, 2016-09-02 19:08:41]

LGTM!

Just to confirm: These wrappers are not used by worker threads, right? Currently
there is a bug that DOMWrapperWorld::setWrapperReferencesInAllWorlds doesn't
visit wrappers on workers (which I need to fix).

+hlopko +mlippautz +hpayer: FYI. We need to support the visitDOMWrappers added
in this CL in our traceWrapper.

https://codereview.chromium.org/2273683003/diff/180001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/bindings/modules/v8/custom/V8WebGL2RenderingContextCustom.cpp
(right):

https://codereview.chromium.org/2273683003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/modules/v8/custom/V8WebGL2RenderingContextCustom.cpp:2:
* Copyright (C) 2016 Google Inc. All rights reserved.

Use the 3-line copyright.

https://codereview.chromium.org/2273683003/diff/180001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/bindings/modules/v8/custom/V8WebGLRenderingContextCustom.cpp
(right):

https://codereview.chromium.org/2273683003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/modules/v8/custom/V8WebGLRenderingContextCustom.cpp:2:
* Copyright (C) 2016 Google Inc. All rights reserved.

Use the 3-line copyright.

https://codereview.chromium.org/2273683003/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webgl/WebGLFramebuffer.cpp (right):

https://codereview.chromium.org/2273683003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGLFramebuffer.cpp:427: for (const
auto& kv : m_attachments) {

kv => attachement

Blink prefers a fully qualified name.

[Ken Russell (switch to Gerrit), 2016-09-02 19:13:57]

Excellent. LGTM again.

https://codereview.chromium.org/2273683003/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
(right):

https://codereview.chromium.org/2273683003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:5253: if
(extension) {
This null-check isn't needed.

[commit-bot: I haz the power, 2016-09-02 22:06:11]

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

[commit-bot: I haz the power, 2016-09-02 22:06:12]

Dry run: This issue passed the CQ dry run.

[Kai Ninomiya, 2016-09-03 00:22:57]

Description was changed from

==========
Delete everything related to the old preserveObjectWrapper solution. Switch to
using visitDOMWrapper to preserve WebGL JS object wrappers (with a new extended
attribute [Custom=VisitDOMWrapperExtra]).

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

to

==========
Delete old preserveObjectWrapper stuff, use visitDOMWrapper instead to preserve
WebGL JS object wrappers. This covers expando-loss, expando-loss-2, new tests
added in expando-loss-2, and expando tests in extension tests.

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

[Kai Ninomiya, 2016-09-03 01:02:06]

https://codereview.chromium.org/2273683003/diff/180001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/bindings/modules/v8/custom/V8WebGL2RenderingContextCustom.cpp
(right):

https://codereview.chromium.org/2273683003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/modules/v8/custom/V8WebGL2RenderingContextCustom.cpp:2:
* Copyright (C) 2016 Google Inc. All rights reserved.
On 2016/09/02 19:08:41, haraken wrote:
> 
> Use the 3-line copyright.

Is this right?

https://codereview.chromium.org/2273683003/diff/180001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/bindings/modules/v8/custom/V8WebGLRenderingContextCustom.cpp
(right):

https://codereview.chromium.org/2273683003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/modules/v8/custom/V8WebGLRenderingContextCustom.cpp:2:
* Copyright (C) 2016 Google Inc. All rights reserved.
On 2016/09/02 19:08:41, haraken wrote:
> 
> Use the 3-line copyright.

Done.

https://codereview.chromium.org/2273683003/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webgl/WebGLFramebuffer.cpp (right):

https://codereview.chromium.org/2273683003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGLFramebuffer.cpp:427: for (const
auto& kv : m_attachments) {
On 2016/09/02 19:08:41, haraken wrote:
> 
> kv => attachement
> 
> Blink prefers a fully qualified name.

Done.

https://codereview.chromium.org/2273683003/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
(right):

https://codereview.chromium.org/2273683003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:5253: if
(extension) {
On 2016/09/02 19:13:57, Ken Russell wrote:
> This null-check isn't needed.

Done.

[haraken, 2016-09-03 12:01:04]

Still LGTM

[Yuki, 2016-09-05 06:07:51]

LGTM.

[Kai Ninomiya, 2016-09-06 16:43:15]

The CQ bit was checked by kainino@chromium.org

[Kai Ninomiya, 2016-09-06 16:43:15]

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/2273683003/#ps200001
(title: "style")

[commit-bot: I haz the power, 2016-09-06 16:43:32]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-06 18:53:20]

Description was changed from

==========
Delete old preserveObjectWrapper stuff, use visitDOMWrapper instead to preserve
WebGL JS object wrappers. This covers expando-loss, expando-loss-2, new tests
added in expando-loss-2, and expando tests in extension tests.

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

to

==========
Delete old preserveObjectWrapper stuff, use visitDOMWrapper instead to preserve
WebGL JS object wrappers. This covers expando-loss, expando-loss-2, new tests
added in expando-loss-2, and expando tests in extension tests.

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

[commit-bot: I haz the power, 2016-09-06 18:53:21]

Committed patchset #10 (id:200001)

[commit-bot: I haz the power, 2016-09-06 18:56:53]

Description was changed from

==========
Delete old preserveObjectWrapper stuff, use visitDOMWrapper instead to preserve
WebGL JS object wrappers. This covers expando-loss, expando-loss-2, new tests
added in expando-loss-2, and expando tests in extension tests.

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

to

==========
Delete old preserveObjectWrapper stuff, use visitDOMWrapper instead to preserve
WebGL JS object wrappers. This covers expando-loss, expando-loss-2, new tests
added in expando-loss-2, and expando tests in extension tests.

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

Committed: https://crrev.com/b7615cfadeb59ac220bd5994d4ec6f99341452f0
Cr-Commit-Position: refs/heads/master@{#416686}
==========

[commit-bot: I haz the power, 2016-09-06 18:56:54]

Patchset 10 (id:??) landed as
https://crrev.com/b7615cfadeb59ac220bd5994d4ec6f99341452f0
Cr-Commit-Position: refs/heads/master@{#416686}