Supplement should have a Member to the corresponding Supplementable object

Supplement should have a Member to the corresponding Supplementable object

Currently a lot of Supplement objects are observing ContextLifecycleObservers
because they cannot access the corresponding Supplementable object, which is silly.
This CL adds a strong reference from a Supplement object to the corresponding
Supplementable object. That way we can remove a bunch of ContextLifecycleObservers
from Supplement objects in follow-up CLs.

MSVC requires to include X.h when inheriting from Supplement<X>.
This CL adds the necessary include statements.

BUG=610176
Committed: https://crrev.com/84f686de38202c5f5b9bb7863a1caea1f659ed59
Cr-Commit-Position: refs/heads/master@{#440733}

[haraken, 2016-12-26 07:21:54]

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

[commit-bot: I haz the power, 2016-12-26 07:22:01]

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-12-26 07:24:10]

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

[commit-bot: I haz the power, 2016-12-26 07:24:10]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  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 on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[haraken, 2016-12-26 07:27:17]

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

[commit-bot: I haz the power, 2016-12-26 07:27:29]

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-12-26 08:48:40]

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

[commit-bot: I haz the power, 2016-12-26 08:48:41]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[haraken, 2016-12-26 08:54:41]

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

[commit-bot: I haz the power, 2016-12-26 08:54:50]

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-12-26 10:12:00]

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

[commit-bot: I haz the power, 2016-12-26 10:12:01]

Dry run: This issue passed the CQ dry run.

[haraken, 2016-12-26 11:02:58]

Description was changed from

==========
Supplement should have a Member to the corresponding Supplementable object

Currently a lot of Supplement objects are observing ContextLifecycleObservers
because they cannot access the corresponding Supplementable object, which is
silly.
This CL adds a strong reference from a Supplement object to the corresponding
Supplementable object. That way we can remove a bunch of
ContextLifecycleObservers
from Supplement objects in follow-up CLs.

MSVC requires to include X.h when inheriting from Supplement<X>.
This CL adds the necessary include statements.

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

to

==========
Supplement should have a Member to the corresponding Supplementable object

Currently a lot of Supplement objects are observing ContextLifecycleObservers
because they cannot access the corresponding Supplementable object, which is
silly.
This CL adds a strong reference from a Supplement object to the corresponding
Supplementable object. That way we can remove a bunch of
ContextLifecycleObservers
from Supplement objects in follow-up CLs.

MSVC requires to include X.h when inheriting from Supplement<X>.
This CL adds the necessary include statements.

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

[haraken, 2016-12-26 11:03:22]

haraken@chromium.org changed reviewers:
+ dcheng@chromium.org

[haraken, 2016-12-26 11:03:23]

PTAL

(This is a revised version of https://codereview.chromium.org/2150443002/, which
was already reviewed by Sigbjorn, who is ooo now.)

[dcheng, 2016-12-27 05:21:23]

lgtm

[haraken, 2016-12-27 05:24:19]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2016-12-27 05:24: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-12-27 05:29:11]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1482816259493890,
"parent_rev": "03985a0781e3da1b8f57144eb2894a47d5f9958c", "commit_rev":
"3d0a1d2a99f4fbbc8b7108caba6df0a9a23435ef"}

[commit-bot: I haz the power, 2016-12-27 05:29:54]

Description was changed from

==========
Supplement should have a Member to the corresponding Supplementable object

Currently a lot of Supplement objects are observing ContextLifecycleObservers
because they cannot access the corresponding Supplementable object, which is
silly.
This CL adds a strong reference from a Supplement object to the corresponding
Supplementable object. That way we can remove a bunch of
ContextLifecycleObservers
from Supplement objects in follow-up CLs.

MSVC requires to include X.h when inheriting from Supplement<X>.
This CL adds the necessary include statements.

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

to

==========
Supplement should have a Member to the corresponding Supplementable object

Currently a lot of Supplement objects are observing ContextLifecycleObservers
because they cannot access the corresponding Supplementable object, which is
silly.
This CL adds a strong reference from a Supplement object to the corresponding
Supplementable object. That way we can remove a bunch of
ContextLifecycleObservers
from Supplement objects in follow-up CLs.

MSVC requires to include X.h when inheriting from Supplement<X>.
This CL adds the necessary include statements.

BUG=610176

Review-Url: https://codereview.chromium.org/2606643002
==========

[commit-bot: I haz the power, 2016-12-27 05:29:56]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2017-01-02 15:45:33]

Description was changed from

==========
Supplement should have a Member to the corresponding Supplementable object

Currently a lot of Supplement objects are observing ContextLifecycleObservers
because they cannot access the corresponding Supplementable object, which is
silly.
This CL adds a strong reference from a Supplement object to the corresponding
Supplementable object. That way we can remove a bunch of
ContextLifecycleObservers
from Supplement objects in follow-up CLs.

MSVC requires to include X.h when inheriting from Supplement<X>.
This CL adds the necessary include statements.

BUG=610176

Review-Url: https://codereview.chromium.org/2606643002
==========

to

==========
Supplement should have a Member to the corresponding Supplementable object

Currently a lot of Supplement objects are observing ContextLifecycleObservers
because they cannot access the corresponding Supplementable object, which is
silly.
This CL adds a strong reference from a Supplement object to the corresponding
Supplementable object. That way we can remove a bunch of
ContextLifecycleObservers
from Supplement objects in follow-up CLs.

MSVC requires to include X.h when inheriting from Supplement<X>.
This CL adds the necessary include statements.

BUG=610176

Committed: https://crrev.com/84f686de38202c5f5b9bb7863a1caea1f659ed59
Cr-Commit-Position: refs/heads/master@{#440733}
==========

[commit-bot: I haz the power, 2017-01-02 15:45:34]

Patchset 4 (id:??) landed as
https://crrev.com/84f686de38202c5f5b9bb7863a1caea1f659ed59
Cr-Commit-Position: refs/heads/master@{#440733}

[sof, 2017-01-19 07:38:38]

sigbjornf@opera.com changed reviewers:
+ sigbjornf@opera.com

[sof, 2017-01-19 07:38:39]

https://codereview.chromium.org/2606643002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/Supplementable.h (right):

https://codereview.chromium.org/2606643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/Supplementable.h:98: // TODO(haraken): Remove
the default constructor.
I'd argue that this isn't a natural goal for all supplements -- there are some
implementations where the extension/supplement effectively only wants to be
anonymously hosted somewhere once instantiated.

Which allows them to be a declared supplement of several potential hosts, but
only register with one (e.g., LocalFileSystem, ImageBitmapFactories). Recasting
their implementations to be single-hosted could be done, but the result feels
cumbersome.

(https://codereview.chromium.org/2645613004/ is a trial example how it comes out
for LocalFileSystem.)

[haraken, 2017-01-19 08:27:01]

On 2017/01/19 07:38:39, sof wrote:
>
https://codereview.chromium.org/2606643002/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/platform/Supplementable.h (right):
> 
>
https://codereview.chromium.org/2606643002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/platform/Supplementable.h:98: // TODO(haraken):
Remove
> the default constructor.
> I'd argue that this isn't a natural goal for all supplements -- there are some
> implementations where the extension/supplement effectively only wants to be
> anonymously hosted somewhere once instantiated.
> 
> Which allows them to be a declared supplement of several potential hosts, but
> only register with one (e.g., LocalFileSystem, ImageBitmapFactories).
Recasting
> their implementations to be single-hosted could be done, but the result feels
> cumbersome.
> 
> (https://codereview.chromium.org/2645613004/ is a trial example how it comes
out
> for LocalFileSystem.)

I think the underlying problem is that we don't have a good abstraction layer
for a "frame" on a worker.

These supplement objects are supplementing WorkerClient and LocalFrame (or
LocalDOMWindow etc) because the supplement objects are accessed by the main
thread and worker threads. If we have a notion like "FrameBase", the supplement
objects can just supplement the FrameBase and always provide a non-null pointer
to the constructor.

Nit: Another weird thing is that we have pretty different ways to install
supplements between the main thread (ChromeClient) and worker threads
(WorkerClient).

[sof, 2017-01-19 08:57:26]

On 2017/01/19 08:27:01, haraken wrote:
> On 2017/01/19 07:38:39, sof wrote:
> >
>
https://codereview.chromium.org/2606643002/diff/60001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/platform/Supplementable.h (right):
> > 
> >
>
https://codereview.chromium.org/2606643002/diff/60001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/platform/Supplementable.h:98: // TODO(haraken):
> Remove
> > the default constructor.
> > I'd argue that this isn't a natural goal for all supplements -- there are
some
> > implementations where the extension/supplement effectively only wants to be
> > anonymously hosted somewhere once instantiated.
> > 
> > Which allows them to be a declared supplement of several potential hosts,
but
> > only register with one (e.g., LocalFileSystem, ImageBitmapFactories).
> Recasting
> > their implementations to be single-hosted could be done, but the result
feels
> > cumbersome.
> > 
> > (https://codereview.chromium.org/2645613004/ is a trial example how it comes
> out
> > for LocalFileSystem.)
> 
> I think the underlying problem is that we don't have a good abstraction layer
> for a "frame" on a worker.
> 
> These supplement objects are supplementing WorkerClient and LocalFrame (or
> LocalDOMWindow etc) because the supplement objects are accessed by the main
> thread and worker threads. If we have a notion like "FrameBase", the
supplement
> objects can just supplement the FrameBase and always provide a non-null
pointer
> to the constructor.
> 
> Nit: Another weird thing is that we have pretty different ways to install
> supplements between the main thread (ChromeClient) and worker threads
> (WorkerClient).

That could be the way to unify and address the two.

An alternative is to support explicit supplementable registration over some
shared GC mixin interface, which these multi-supplement implementations would
derive from rather than Supplement<T>s.

[haraken, 2017-01-19 09:07:35]

On 2017/01/19 08:57:26, sof wrote:
> On 2017/01/19 08:27:01, haraken wrote:
> > On 2017/01/19 07:38:39, sof wrote:
> > >
> >
>
https://codereview.chromium.org/2606643002/diff/60001/third_party/WebKit/Sour...
> > > File third_party/WebKit/Source/platform/Supplementable.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2606643002/diff/60001/third_party/WebKit/Sour...
> > > third_party/WebKit/Source/platform/Supplementable.h:98: // TODO(haraken):
> > Remove
> > > the default constructor.
> > > I'd argue that this isn't a natural goal for all supplements -- there are
> some
> > > implementations where the extension/supplement effectively only wants to
be
> > > anonymously hosted somewhere once instantiated.
> > > 
> > > Which allows them to be a declared supplement of several potential hosts,
> but
> > > only register with one (e.g., LocalFileSystem, ImageBitmapFactories).
> > Recasting
> > > their implementations to be single-hosted could be done, but the result
> feels
> > > cumbersome.
> > > 
> > > (https://codereview.chromium.org/2645613004/ is a trial example how it
comes
> > out
> > > for LocalFileSystem.)
> > 
> > I think the underlying problem is that we don't have a good abstraction
layer
> > for a "frame" on a worker.
> > 
> > These supplement objects are supplementing WorkerClient and LocalFrame (or
> > LocalDOMWindow etc) because the supplement objects are accessed by the main
> > thread and worker threads. If we have a notion like "FrameBase", the
> supplement
> > objects can just supplement the FrameBase and always provide a non-null
> pointer
> > to the constructor.
> > 
> > Nit: Another weird thing is that we have pretty different ways to install
> > supplements between the main thread (ChromeClient) and worker threads
> > (WorkerClient).
> 
> That could be the way to unify and address the two.
> 
> An alternative is to support explicit supplementable registration over some
> shared GC mixin interface, which these multi-supplement implementations would
> derive from rather than Supplement<T>s.

There's some discussion in the worker team to introduce a FrameBase to share
more code between the main thread and workers, so that seems like a right way to
go (though they might not have a bandwidth to work on it).