Disambiguate LifecycleObserver::contextDestroyed

Disambiguate LifecycleObserver::contextDestroyed

Some objects inherit from multiple lifecycle observers
(e.g., ContextLifecycleObserver and PageLifecycleObserver).
Then the same method contextDestroyed() is called multiple times
when ExecutionContext gets detached *and* when Page gets detached.
This is really confusing.

To disambiguate the situation, this CL introduces a Context* parameter
to contextDestroyed(); e.g., contextDestroyed(ExecutionContext*),
contextDestroyed(Page*), contextDestroyed(WorkerThreadLifecycleContext*) etc.

(In this CL, the Context* parameter is used only to disambiguate contextDestroyed(),
but the parameter will be acutally used in a follow-up CL, where I'm planning to
clear out LifecycleObserver::m_lifecycleContext before calling contextDestroyed().)

BUG=610176
Review-Url: https://codereview.chromium.org/2629593004
Cr-Commit-Position: refs/heads/master@{#443607}
Committed: https://chromium.googlesource.com/chromium/src/+/a6201a8f53c7b297c93a8ea381263c5d52b2b687

[haraken, 2017-01-13 12:26:36]

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

[haraken, 2017-01-13 12:26:38]

PTAL

[yhirano, 2017-01-13 13:20:27]

lgtm

[sof, 2017-01-13 14:14:54]

lgtm

It begs the question though if "context destruction" could only be supported by
ExecutionContext (and the new SynchronousMutation notifier), but perhaps that's
over-fitting the design for current use.

[haraken, 2017-01-13 14:45:46]

On 2017/01/13 14:14:54, sof wrote:
> lgtm
> 
> It begs the question though if "context destruction" could only be supported
by
> ExecutionContext (and the new SynchronousMutation notifier), but perhaps
that's
> over-fitting the design for current use.

contextDestroyed is also supported by WorkerThreadLifecycleContext as well.

[haraken, 2017-01-13 14:45:55]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2017-01-13 14:46:14]

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, 2017-01-13 14:49:52]

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

[commit-bot: I haz the power, 2017-01-13 14:49:55]

Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[haraken, 2017-01-13 15:44:37]

The CQ bit was checked by haraken@chromium.org

[haraken, 2017-01-13 15:44:38]

The patchset sent to the CQ was uploaded after l-g-t-m from sigbjornf@opera.com,
yhirano@chromium.org
Link to the patchset: https://codereview.chromium.org/2629593004/#ps20001
(title: "temp")

[commit-bot: I haz the power, 2017-01-13 15:45:03]

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, 2017-01-13 18:06:26]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1484322277756920,
"parent_rev": "9205e65c7dbfa96352e305ed7210946fc9c9e93c", "commit_rev":
"a6201a8f53c7b297c93a8ea381263c5d52b2b687"}

[commit-bot: I haz the power, 2017-01-13 18:06:57]

Description was changed from

==========
Disambiguate LifecycleObserver::contextDestroyed

Some objects inherit from multiple lifecycle observers
(e.g., ContextLifecycleObserver and PageLifecycleObserver).
Then the same method contextDestroyed() is called multiple times
when ExecutionContext gets detached *and* when Page gets detached.
This is really confusing.

To disambiguate the situation, this CL introduces a Context* parameter
to contextDestroyed(); e.g., contextDestroyed(ExecutionContext*),
contextDestroyed(Page*), contextDestroyed(WorkerThreadLifecycleContext*) etc.

(In this CL, the Context* parameter is used only to disambiguate
contextDestroyed(),
but the parameter will be acutally used in a follow-up CL, where I'm planning to
clear out LifecycleObserver::m_lifecycleContext before calling
contextDestroyed().)

BUG=610176
==========

to

==========
Disambiguate LifecycleObserver::contextDestroyed

Some objects inherit from multiple lifecycle observers
(e.g., ContextLifecycleObserver and PageLifecycleObserver).
Then the same method contextDestroyed() is called multiple times
when ExecutionContext gets detached *and* when Page gets detached.
This is really confusing.

To disambiguate the situation, this CL introduces a Context* parameter
to contextDestroyed(); e.g., contextDestroyed(ExecutionContext*),
contextDestroyed(Page*), contextDestroyed(WorkerThreadLifecycleContext*) etc.

(In this CL, the Context* parameter is used only to disambiguate
contextDestroyed(),
but the parameter will be acutally used in a follow-up CL, where I'm planning to
clear out LifecycleObserver::m_lifecycleContext before calling
contextDestroyed().)

BUG=610176

Review-Url: https://codereview.chromium.org/2629593004
Cr-Commit-Position: refs/heads/master@{#443607}
Committed:
https://chromium.googlesource.com/chromium/src/+/a6201a8f53c7b297c93a8ea38126...
==========

[commit-bot: I haz the power, 2017-01-13 18:06:59]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/a6201a8f53c7b297c93a8ea38126...

[sof, 2017-01-15 12:52:45]

On 2017/01/13 14:45:46, haraken wrote:
> On 2017/01/13 14:14:54, sof wrote:
> > lgtm
> > 
> > It begs the question though if "context destruction" could only be supported
> by
> > ExecutionContext (and the new SynchronousMutation notifier), but perhaps
> that's
> > over-fitting the design for current use.
> 
> contextDestroyed is also supported by WorkerThreadLifecycleContext as well.

Yes, missed those.

It is unfortunate to now have to define all these empty contextDestroyed(Page*)
overrides. At the cost of doing a bit more work at compile time,
https://codereview.chromium.org/2634713002/ avoids that (and effectively
implements what the question above tried to get at.)

[haraken, 2017-01-15 13:03:42]

On 2017/01/15 12:52:45, sof wrote:
> On 2017/01/13 14:45:46, haraken wrote:
> > On 2017/01/13 14:14:54, sof wrote:
> > > lgtm
> > > 
> > > It begs the question though if "context destruction" could only be
supported
> > by
> > > ExecutionContext (and the new SynchronousMutation notifier), but perhaps
> > that's
> > > over-fitting the design for current use.
> > 
> > contextDestroyed is also supported by WorkerThreadLifecycleContext as well.
> 
> Yes, missed those.
> 
> It is unfortunate to now have to define all these empty
contextDestroyed(Page*)
> overrides. At the cost of doing a bit more work at compile time,
> https://codereview.chromium.org/2634713002/ avoids that (and effectively
> implements what the question above tried to get at.)

ah, that looks tidier!

[sof, 2017-01-15 15:28:19]

On 2017/01/15 13:03:42, haraken wrote:
> On 2017/01/15 12:52:45, sof wrote:
> > On 2017/01/13 14:45:46, haraken wrote:
> > > On 2017/01/13 14:14:54, sof wrote:
> > > > lgtm
> > > > 
> > > > It begs the question though if "context destruction" could only be
> supported
> > > by
> > > > ExecutionContext (and the new SynchronousMutation notifier), but perhaps
> > > that's
> > > > over-fitting the design for current use.
> > > 
> > > contextDestroyed is also supported by WorkerThreadLifecycleContext as
well.
> > 
> > Yes, missed those.
> > 
> > It is unfortunate to now have to define all these empty
> contextDestroyed(Page*)
> > overrides. At the cost of doing a bit more work at compile time,
> > https://codereview.chromium.org/2634713002/ avoids that (and effectively
> > implements what the question above tried to get at.)
> 
> ah, that looks tidier!

It might not be obvious why the outright removal of the empty
"contextDestroyed(Page*)" overrides wouldn't be the simplest thing to do, but
doing so runs into (e.g.) clang's hidden overloaded virtual warning for classes
that are both ExecutionContext and PageVisibility observers. i.e., having just a
single override

  virtual void contextDestroyed(ExecutionContext*) override;

for such observers makes the compiler warn that you're hiding the overloaded
|contextDestroyed()| virtual over Page*.