Remove destructors from ScriptWrappable objects in modules/

Remove destructors from ScriptWrappable objects in modules/

R=haraken@chromium.org
BUG=340522
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=179446

[sof, 2014-07-03 16:44:43]

Please take a look.

[sof, 2014-07-03 21:19:13]

Defining a destructor-less ScriptWrappable with Oilpan enabled is really an
approximation, I've now realized.

It is always fine to use a destructor-less ScriptWrappable in combination with
GarbageCollected, regardless of enable_oilpan. Not having the ability to express
that (via another class or GC plugin cooperation) lead to the addition of
another transition type here.

That's not particularly pretty - we need to agree on what to do for
ScriptWrappable when used with GarbageCollected (for modules where Oilpan is now
enabled by default.) I think I'm leaning towards having the GC plugin spot &
allow ScriptWrappable for these cases.

[haraken, 2014-07-04 01:25:29]

+zerny

On 2014/07/03 21:19:13, sof wrote:
> Defining a destructor-less ScriptWrappable with Oilpan enabled is really an
> approximation, I've now realized.
> 
> It is always fine to use a destructor-less ScriptWrappable in combination with
> GarbageCollected, regardless of enable_oilpan. Not having the ability to
express
> that (via another class or GC plugin cooperation) lead to the addition of
> another transition type here.
> 
> That's not particularly pretty - we need to agree on what to do for
> ScriptWrappable when used with GarbageCollected (for modules where Oilpan is
now
> enabled by default.) I think I'm leaning towards having the GC plugin spot &
> allow ScriptWrappable for these cases.

Yeah, GarbageCollectedFinalizedWillBeGarbageCollected looks ugly.

Since we don't want to confuse developers working on modules, shall we improve
the gc plugin first and land this CL then?

https://codereview.chromium.org/367113005/diff/20001/Source/modules/push_mess...
File Source/modules/push_messaging/PushManager.h (right):

https://codereview.chromium.org/367113005/diff/20001/Source/modules/push_mess...
Source/modules/push_messaging/PushManager.h:19:
DECLARE_EMPTY_DESTRUCTOR_WILL_BE_REMOVED(PushManager);

Since PushManager is FINAL, it looks OK to not use a virtual destructor.

[sof, 2014-07-04 05:09:05]

On 2014/07/04 01:25:29, haraken wrote:
> +zerny
> 
> On 2014/07/03 21:19:13, sof wrote:
> > Defining a destructor-less ScriptWrappable with Oilpan enabled is really an
> > approximation, I've now realized.
> > 
> > It is always fine to use a destructor-less ScriptWrappable in combination
with
> > GarbageCollected, regardless of enable_oilpan. Not having the ability to
> express
> > that (via another class or GC plugin cooperation) lead to the addition of
> > another transition type here.
> > 
> > That's not particularly pretty - we need to agree on what to do for
> > ScriptWrappable when used with GarbageCollected (for modules where Oilpan is
> now
> > enabled by default.) I think I'm leaning towards having the GC plugin spot &
> > allow ScriptWrappable for these cases.
> 
> Yeah, GarbageCollectedFinalizedWillBeGarbageCollected looks ugly.
> 
> Since we don't want to confuse developers working on modules, shall we improve
> the gc plugin first and land this CL then?
> 

Let's do that (or some other alternative we end up agreeing on.)

[zerny-chromium, 2014-07-07 10:57:18]

On 2014/07/04 05:09:05, sof wrote:
> On 2014/07/04 01:25:29, haraken wrote:
> > +zerny
> > 
> > On 2014/07/03 21:19:13, sof wrote:
> > > Defining a destructor-less ScriptWrappable with Oilpan enabled is really
an
> > > approximation, I've now realized.
> > > 
> > > It is always fine to use a destructor-less ScriptWrappable in combination
> with
> > > GarbageCollected, regardless of enable_oilpan. Not having the ability to
> > express
> > > that (via another class or GC plugin cooperation) lead to the addition of
> > > another transition type here.
> > > 
> > > That's not particularly pretty - we need to agree on what to do for
> > > ScriptWrappable when used with GarbageCollected (for modules where Oilpan
is
> > now
> > > enabled by default.) I think I'm leaning towards having the GC plugin spot
&
> > > allow ScriptWrappable for these cases.
> > 
> > Yeah, GarbageCollectedFinalizedWillBeGarbageCollected looks ugly.
> > 
> > Since we don't want to confuse developers working on modules, shall we
improve
> > the gc plugin first and land this CL then?
> > 
> 
> Let's do that (or some other alternative we end up agreeing on.)

Updating the plugin to exclude ScriptWrappable from the "needs finalization" is
non-trivial, since we will need to implement (some variant of) clangs "trivial
dtor" code. Right now we simply piggyback on the compiler which also means that
the plugin is actually correct.

As Sigbjørn has done here, we can use
GarbageCollectedFinalizedWillBeGarbageCollected which is correct according to
the rules we have outlined for proper use of oilpan. If it is deemed to ugly, we
can also simply use GarbageCollectedFinalized and at some later point, when
ScriptWrappable is shipped in full, remove the finalized.

If decided against, I can update the plugin with the exception, but it will not
give us the same safety as the current checks.

[sof, 2014-07-08 11:40:56]

Reverted the use of a transition type.

Depends on https://codereview.chromium.org/374593002/ landing first before it
can (potentially) go ahead.

[haraken, 2014-07-08 11:47:34]

LGTM!

[zerny-chromium, 2014-07-08 14:56:02]

On 2014/07/08 11:40:56, sof wrote:
> Reverted the use of a transition type.
> 
> Depends on https://codereview.chromium.org/374593002/ landing first before it
> can (potentially) go ahead.

The plugin CL needs to land and also we need to a new clang roll. I'd rather not
push for an independent plugin roll on this since it is not of immediate need.
Until clang rolls you can land the patch with a transition type or wait.

[sof, 2014-07-08 15:00:26]

On 2014/07/08 14:56:02, zerny-chromium wrote:
> On 2014/07/08 11:40:56, sof wrote:
> > Reverted the use of a transition type.
> > 
> > Depends on https://codereview.chromium.org/374593002/ landing first before
it
> > can (potentially) go ahead.
> 
> The plugin CL needs to land and also we need to a new clang roll. I'd rather
not
> push for an independent plugin roll on this since it is not of immediate need.
> Until clang rolls you can land the patch with a transition type or wait.

ok. Of course I'll wait for that roll.

[sof, 2014-08-02 08:33:48]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-08-02 08:34:13]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sigbjornf@opera.com/367113005/60001

[commit-bot: I haz the power, 2014-08-02 09:23:51]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/20301)

[commit-bot: I haz the power, 2014-08-02 09:59:35]

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

[commit-bot: I haz the power, 2014-08-02 09:59:36]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/20309)

[sof, 2014-08-02 10:16:50]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-08-02 10:17:16]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sigbjornf@opera.com/367113005/60001

[commit-bot: I haz the power, 2014-08-02 10:53:16]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/20315)

[commit-bot: I haz the power, 2014-08-02 11:30:12]

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

[commit-bot: I haz the power, 2014-08-02 11:30:13]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/20321)

[sof, 2014-08-02 11:37:19]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-08-02 11:37:52]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sigbjornf@opera.com/367113005/60001

[commit-bot: I haz the power, 2014-08-02 12:41:59]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/20328)

[commit-bot: I haz the power, 2014-08-02 13:17:42]

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

[commit-bot: I haz the power, 2014-08-02 13:17:43]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/20338)

[haraken, 2014-08-02 13:44:48]

oh, now the plugin is rolled and we can land this CL :)

[sof, 2014-08-02 17:11:55]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-08-02 17:12:30]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sigbjornf@opera.com/367113005/60001

[commit-bot: I haz the power, 2014-08-02 17:43:42]

Change committed as 179446

[sof, 2014-08-02 17:48:04]

On 2014/08/02 13:44:48, haraken wrote:
> oh, now the plugin is rolled and we can land this CL :)

Yes! Definitely worth landing this one finally :) I checked, but if I missed a
ScriptWrappable object that has been added while it has been sitting there (and
it doesn't needed *Finalized either), I apologize the oversight.

(Away for another week, at least.)