Added plumbing for the availablechange event from Blink to WebPresentationClient.

Added plumbing for the availablechange event from Blink to WebPresentationClient.
Chromium CL: https://codereview.chromium.org/839773002

BUG=412331
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=189136

[whywhat, 2015-01-07 12:27:37]

avayvod@chromium.org changed reviewers:
+ peter@chromium.org

[whywhat, 2015-01-07 12:27:38]

Asking for the initial round of review(s)

[Peter Beverloo, 2015-01-07 14:29:09]

Thanks for the patch! I find it to be very low on information about what you're
actually trying to do, and how you envision it to work. Do you have a document
or explanation describing the plan?

https://codereview.chromium.org/832263007/diff/1/Source/modules/presentation/...
File Source/modules/presentation/AvailableChangeEvent.cpp (right):

https://codereview.chromium.org/832263007/diff/1/Source/modules/presentation/...
Source/modules/presentation/AvailableChangeEvent.cpp:27: ,
m_available(initializer.hasAvailable() ? initializer.available() : false)
Please set the default value in AvailableChangeEventInit.idl rather than forcing
it here.

https://codereview.chromium.org/832263007/diff/1/Source/modules/presentation/...
Source/modules/presentation/AvailableChangeEvent.cpp:38: 
nit: excess newlines.

https://codereview.chromium.org/832263007/diff/1/Source/modules/presentation/...
File Source/modules/presentation/AvailableChangeEvent.h (right):

https://codereview.chromium.org/832263007/diff/1/Source/modules/presentation/...
Source/modules/presentation/AvailableChangeEvent.h:17: virtual
~AvailableChangeEvent();
Muh. We should update the style guide to match Chromium in regards to
virtual/override. Kicked the blink-dev@ thread, but it's likely to be out of
scope for this CL.

https://codereview.chromium.org/832263007/diff/1/Source/modules/presentation/...
File Source/modules/presentation/AvailableChangeEvent.idl (right):

https://codereview.chromium.org/832263007/diff/1/Source/modules/presentation/...
Source/modules/presentation/AvailableChangeEvent.idl:10: ]
nit: "] interface" on the same line.

https://codereview.chromium.org/832263007/diff/1/Source/modules/presentation/...
Source/modules/presentation/AvailableChangeEvent.idl:12: readonly attribute
boolean available;
nit: indent.

https://codereview.chromium.org/832263007/diff/1/Source/modules/presentation/...
File Source/modules/presentation/AvailableChangeEventInit.idl (right):

https://codereview.chromium.org/832263007/diff/1/Source/modules/presentation/...
Source/modules/presentation/AvailableChangeEventInit.idl:1: // Copyright 2014
The Chromium Authors. All rights reserved.
nit: 2015

https://codereview.chromium.org/832263007/diff/1/Source/modules/presentation/...
Source/modules/presentation/AvailableChangeEventInit.idl:8: boolean available;
nit: indent. Also include the default value here, please.

https://codereview.chromium.org/832263007/diff/1/Source/modules/presentation/...
File Source/modules/presentation/Presentation.cpp (right):

https://codereview.chromium.org/832263007/diff/1/Source/modules/presentation/...
Source/modules/presentation/Presentation.cpp:39:
controller->setPresentation(presentation);
When is it valid for the runtime Presentation flag to be enabled, but not having
had a WebPresentationClient provided by the embedder? It seems to me like
presentationController() can ASSERT and return a reference instead.

https://codereview.chromium.org/832263007/diff/1/Source/modules/presentation/...
Source/modules/presentation/Presentation.cpp:101: void
Presentation::deviceAvailabilityChanged(bool available)
Why is this called "deviceAvailabilityChanged" while the event is
"availablechanged"? Usually methods like this are called something like
"didChangeAvailability".

https://codereview.chromium.org/832263007/diff/1/Source/modules/presentation/...
Source/modules/presentation/Presentation.cpp:108: if (!frame())
Rather than having a separate frame() method, could you just inline it and have
a local LocalFrame* variable?

https://codereview.chromium.org/832263007/diff/1/Source/modules/presentation/...
File Source/modules/presentation/Presentation.h (right):

https://codereview.chromium.org/832263007/diff/1/Source/modules/presentation/...
Source/modules/presentation/Presentation.h:41: virtual bool
addEventListener(const AtomicString&, PassRefPtr<EventListener>, bool) override;
Please give the "AtomicString" and "bool" arguments a name, as it's not obvious
what they mean.

https://codereview.chromium.org/832263007/diff/1/Source/modules/presentation/...
File Source/modules/presentation/PresentationController.cpp (right):

https://codereview.chromium.org/832263007/diff/1/Source/modules/presentation/...
Source/modules/presentation/PresentationController.cpp:59: if (m_presentation)
Should we get this event if there is no m_presentation anymore?

Right now, when the Presentation object gets destroyed, it calls into
PresentationController::setPresentation(), but only in the destructor. Since
m_presentation is a traced Persistent/Member, the lifetimes of Presentation will
therefore be coupled to PresentationController, so that call isn't actually
required.

Furthermore, since PresentationController assumes there is only a single
Presentation instance, it would be good to ASSERT() in setPresentation() that we
don't accidentally create a second one per LocalFrame.

https://codereview.chromium.org/832263007/diff/1/Source/modules/presentation/...
Source/modules/presentation/PresentationController.cpp:66:
m_client->addDeviceAvailabilityListener();
Hm, so the embedder is responsible for doing some kind of reference counting for
active listeners per WebPresentationController*? Does the embedder care about
the exact number of active listeners, or only about the boolean "is there a
listener" question?

https://codereview.chromium.org/832263007/diff/1/Source/modules/presentation/...
Source/modules/presentation/PresentationController.cpp:75: void
PresentationController::setPresentation(Presentation* presentation)
Can we limit the scope of what PresentationController knows about |presentation|
by using the WebPresentationController type instead?

https://codereview.chromium.org/832263007/diff/1/Source/modules/presentation/...
File Source/modules/presentation/PresentationController.h (right):

https://codereview.chromium.org/832263007/diff/1/Source/modules/presentation/...
Source/modules/presentation/PresentationController.h:31: // Inherited from
Supplement.
If you override it, then you're not inheriting it. There's also no parent class
called "Supplement".

https://codereview.chromium.org/832263007/diff/1/public/platform/WebPresentat...
File public/platform/WebPresentationClient.h (right):

https://codereview.chromium.org/832263007/diff/1/public/platform/WebPresentat...
public/platform/WebPresentationClient.h:1: // Copyright (c) 2015 The Chromium
Authors. All rights reserved.
nit: no (c).

https://codereview.chromium.org/832263007/diff/1/public/platform/WebPresentat...
public/platform/WebPresentationClient.h:12: class WebPresentationClient {
This is an API class, so please add documentation comment blocks to what the
expected behavior is.

https://codereview.chromium.org/832263007/diff/1/public/platform/WebPresentat...
File public/platform/WebPresentationController.h (right):

https://codereview.chromium.org/832263007/diff/1/public/platform/WebPresentat...
public/platform/WebPresentationController.h:1: // Copyright (c) 2015 The
Chromium Authors. All rights reserved.
nit: no (c).

https://codereview.chromium.org/832263007/diff/1/public/platform/WebPresentat...
public/platform/WebPresentationController.h:10: class WebPresentationController
{
dito re: documentation.

https://codereview.chromium.org/832263007/diff/1/public/platform/WebPresentat...
public/platform/WebPresentationController.h:14: virtual void
deviceAvailabilityChanged(bool) = 0;
Please give the argument a name.

https://codereview.chromium.org/832263007/diff/1/public/web/WebFrameClient.h
File public/web/WebFrameClient.h (right):

https://codereview.chromium.org/832263007/diff/1/public/web/WebFrameClient.h#...
public/web/WebFrameClient.h:340: virtual WebPresentationClient*
presentationClient() { return 0; }
nit: two blank lines under here for consistency. End the comment sentence with a
period.

[whywhat, 2015-01-07 16:22:16]

Re: what I'm trying to do.
I'm simply plumbing the API calls to the Mojo interface (see the Chromium CL)
following the other modules around. I envision other methods of Presentation
being implemented in a similar manner. Presentation would also own
PresentationSession objects that in turn would use the same plumbing structure
to content to talk to PresentationSessionPtr-s provided by the Mojo service.

https://codereview.chromium.org/832263007/diff/1/Source/modules/presentation/...
File Source/modules/presentation/AvailableChangeEvent.cpp (right):

https://codereview.chromium.org/832263007/diff/1/Source/modules/presentation/...
Source/modules/presentation/AvailableChangeEvent.cpp:27: ,
m_available(initializer.hasAvailable() ? initializer.available() : false)
On 2015/01/07 14:29:08, Peter Beverloo wrote:
> Please set the default value in AvailableChangeEventInit.idl rather than
forcing
> it here.

Done.

https://codereview.chromium.org/832263007/diff/1/Source/modules/presentation/...
Source/modules/presentation/AvailableChangeEvent.cpp:38: 
On 2015/01/07 14:29:08, Peter Beverloo wrote:
> nit: excess newlines.

Done.

https://codereview.chromium.org/832263007/diff/1/Source/modules/presentation/...
File Source/modules/presentation/AvailableChangeEvent.idl (right):

https://codereview.chromium.org/832263007/diff/1/Source/modules/presentation/...
Source/modules/presentation/AvailableChangeEvent.idl:10: ]
On 2015/01/07 14:29:08, Peter Beverloo wrote:
> nit: "] interface" on the same line.

Done.

https://codereview.chromium.org/832263007/diff/1/Source/modules/presentation/...
Source/modules/presentation/AvailableChangeEvent.idl:12: readonly attribute
boolean available;
On 2015/01/07 14:29:08, Peter Beverloo wrote:
> nit: indent.

Done.

https://codereview.chromium.org/832263007/diff/1/Source/modules/presentation/...
File Source/modules/presentation/AvailableChangeEventInit.idl (right):

https://codereview.chromium.org/832263007/diff/1/Source/modules/presentation/...
Source/modules/presentation/AvailableChangeEventInit.idl:1: // Copyright 2014
The Chromium Authors. All rights reserved.
On 2015/01/07 14:29:08, Peter Beverloo wrote:
> nit: 2015

Done.

https://codereview.chromium.org/832263007/diff/1/Source/modules/presentation/...
Source/modules/presentation/AvailableChangeEventInit.idl:8: boolean available;
On 2015/01/07 14:29:08, Peter Beverloo wrote:
> nit: indent. Also include the default value here, please.

Done.

https://codereview.chromium.org/832263007/diff/1/Source/modules/presentation/...
File Source/modules/presentation/Presentation.cpp (right):

https://codereview.chromium.org/832263007/diff/1/Source/modules/presentation/...
Source/modules/presentation/Presentation.cpp:39:
controller->setPresentation(presentation);
On 2015/01/07 14:29:08, Peter Beverloo wrote:
> When is it valid for the runtime Presentation flag to be enabled, but not
having
> had a WebPresentationClient provided by the embedder? It seems to me like
> presentationController() can ASSERT and return a reference instead.

I imagined a hypothetical situation where the UA might run on hardware that
doesn't support screen discovery/presenting and decided to be on the safe side.

https://codereview.chromium.org/832263007/diff/1/Source/modules/presentation/...
Source/modules/presentation/Presentation.cpp:101: void
Presentation::deviceAvailabilityChanged(bool available)
On 2015/01/07 14:29:08, Peter Beverloo wrote:
> Why is this called "deviceAvailabilityChanged" while the event is
> "availablechanged"? Usually methods like this are called something like
> "didChangeAvailability".

Done.

https://codereview.chromium.org/832263007/diff/1/Source/modules/presentation/...
Source/modules/presentation/Presentation.cpp:108: if (!frame())
On 2015/01/07 14:29:08, Peter Beverloo wrote:
> Rather than having a separate frame() method, could you just inline it and
have
> a local LocalFrame* variable?

Done.

https://codereview.chromium.org/832263007/diff/1/Source/modules/presentation/...
File Source/modules/presentation/Presentation.h (right):

https://codereview.chromium.org/832263007/diff/1/Source/modules/presentation/...
Source/modules/presentation/Presentation.h:41: virtual bool
addEventListener(const AtomicString&, PassRefPtr<EventListener>, bool) override;
On 2015/01/07 14:29:09, Peter Beverloo wrote:
> Please give the "AtomicString" and "bool" arguments a name, as it's not
obvious
> what they mean.

Done.

https://codereview.chromium.org/832263007/diff/1/Source/modules/presentation/...
File Source/modules/presentation/PresentationController.cpp (right):

https://codereview.chromium.org/832263007/diff/1/Source/modules/presentation/...
Source/modules/presentation/PresentationController.cpp:59: if (m_presentation)
On 2015/01/07 14:29:09, Peter Beverloo wrote:
> Should we get this event if there is no m_presentation anymore?
> 
> Right now, when the Presentation object gets destroyed, it calls into
> PresentationController::setPresentation(), but only in the destructor. Since
> m_presentation is a traced Persistent/Member, the lifetimes of Presentation
will
> therefore be coupled to PresentationController, so that call isn't actually
> required.
> 
> Furthermore, since PresentationController assumes there is only a single
> Presentation instance, it would be good to ASSERT() in setPresentation() that
we
> don't accidentally create a second one per LocalFrame.

Done.

https://codereview.chromium.org/832263007/diff/1/Source/modules/presentation/...
Source/modules/presentation/PresentationController.cpp:66:
m_client->addDeviceAvailabilityListener();
On 2015/01/07 14:29:09, Peter Beverloo wrote:
> Hm, so the embedder is responsible for doing some kind of reference counting
for
> active listeners per WebPresentationController*? Does the embedder care about
> the exact number of active listeners, or only about the boolean "is there a
> listener" question?

In general, the embedder should care if there's any number listeners or none.
However, I'm not sure it makes sense to do the accounting for listeners number
here rather than delegating to the embedder implementing the Mojo interface.
Depending on the discovery methods supported, the UA might ignore the boolean
completely or on the contrary actually care about the number.

https://codereview.chromium.org/832263007/diff/1/Source/modules/presentation/...
Source/modules/presentation/PresentationController.cpp:75: void
PresentationController::setPresentation(Presentation* presentation)
On 2015/01/07 14:29:09, Peter Beverloo wrote:
> Can we limit the scope of what PresentationController knows about
|presentation|
> by using the WebPresentationController type instead?

Not sure I want to introduce another interface and I didn't see such dependency
injection in other modules.
PresentationController already implements WebPresentationController to abstract
PresentationDispatcher (WebPresentationClient implementation) from knowing
anything about PresentationController.

https://codereview.chromium.org/832263007/diff/1/Source/modules/presentation/...
File Source/modules/presentation/PresentationController.h (right):

https://codereview.chromium.org/832263007/diff/1/Source/modules/presentation/...
Source/modules/presentation/PresentationController.h:31: // Inherited from
Supplement.
On 2015/01/07 14:29:09, Peter Beverloo wrote:
> If you override it, then you're not inheriting it. There's also no parent
class
> called "Supplement".

Done.

https://codereview.chromium.org/832263007/diff/1/public/platform/WebPresentat...
File public/platform/WebPresentationClient.h (right):

https://codereview.chromium.org/832263007/diff/1/public/platform/WebPresentat...
public/platform/WebPresentationClient.h:1: // Copyright (c) 2015 The Chromium
Authors. All rights reserved.
On 2015/01/07 14:29:09, Peter Beverloo wrote:
> nit: no (c).

Done.

https://codereview.chromium.org/832263007/diff/1/public/platform/WebPresentat...
public/platform/WebPresentationClient.h:12: class WebPresentationClient {
On 2015/01/07 14:29:09, Peter Beverloo wrote:
> This is an API class, so please add documentation comment blocks to what the
> expected behavior is.

Done.

https://codereview.chromium.org/832263007/diff/1/public/platform/WebPresentat...
File public/platform/WebPresentationController.h (right):

https://codereview.chromium.org/832263007/diff/1/public/platform/WebPresentat...
public/platform/WebPresentationController.h:1: // Copyright (c) 2015 The
Chromium Authors. All rights reserved.
On 2015/01/07 14:29:09, Peter Beverloo wrote:
> nit: no (c).

Done.

https://codereview.chromium.org/832263007/diff/1/public/platform/WebPresentat...
public/platform/WebPresentationController.h:10: class WebPresentationController
{
On 2015/01/07 14:29:09, Peter Beverloo wrote:
> dito re: documentation.

Done.

https://codereview.chromium.org/832263007/diff/1/public/platform/WebPresentat...
public/platform/WebPresentationController.h:14: virtual void
deviceAvailabilityChanged(bool) = 0;
On 2015/01/07 14:29:09, Peter Beverloo wrote:
> Please give the argument a name.

Done.

https://codereview.chromium.org/832263007/diff/1/public/web/WebFrameClient.h
File public/web/WebFrameClient.h (right):

https://codereview.chromium.org/832263007/diff/1/public/web/WebFrameClient.h#...
public/web/WebFrameClient.h:340: virtual WebPresentationClient*
presentationClient() { return 0; }
On 2015/01/07 14:29:09, Peter Beverloo wrote:
> nit: two blank lines under here for consistency. End the comment sentence with
a
> period.

Done.

[whywhat, 2015-01-07 19:45:36]

+Mark and Derek FYI
Note the link to the Chromium CL in the description.

[Peter Beverloo, 2015-01-07 19:53:12]

Thanks for the updates!!

https://codereview.chromium.org/832263007/diff/1/Source/modules/presentation/...
File Source/modules/presentation/Presentation.cpp (right):

https://codereview.chromium.org/832263007/diff/1/Source/modules/presentation/...
Source/modules/presentation/Presentation.cpp:39:
controller->setPresentation(presentation);
On 2015/01/07 16:22:16, whywhat wrote:
> On 2015/01/07 14:29:08, Peter Beverloo wrote:
> > When is it valid for the runtime Presentation flag to be enabled, but not
> having
> > had a WebPresentationClient provided by the embedder? It seems to me like
> > presentationController() can ASSERT and return a reference instead.
> 
> I imagined a hypothetical situation where the UA might run on hardware that
> doesn't support screen discovery/presenting and decided to be on the safe
side.

In that case we wouldn't enable the run-time flag, causing the entire feature to
not be exposed. If the run-time flag is enabled, it's fair to ASSERT existence
of the PresentationController.

https://codereview.chromium.org/832263007/diff/1/Source/modules/presentation/...
File Source/modules/presentation/PresentationController.cpp (right):

https://codereview.chromium.org/832263007/diff/1/Source/modules/presentation/...
Source/modules/presentation/PresentationController.cpp:66:
m_client->addDeviceAvailabilityListener();
On 2015/01/07 16:22:16, whywhat wrote:
> On 2015/01/07 14:29:09, Peter Beverloo wrote:
> > Hm, so the embedder is responsible for doing some kind of reference counting
> for
> > active listeners per WebPresentationController*? Does the embedder care
about
> > the exact number of active listeners, or only about the boolean "is there a
> > listener" question?
> 
> In general, the embedder should care if there's any number listeners or none.
> However, I'm not sure it makes sense to do the accounting for listeners number
> here rather than delegating to the embedder implementing the Mojo interface.
> Depending on the discovery methods supported, the UA might ignore the boolean
> completely or on the contrary actually care about the number.

In order to keep information local, we should only tell the embedder if there
are *any* listeners or *no* listeners - the embedder will not behave differently
if there are two event listeners rather than one.

https://codereview.chromium.org/832263007/diff/1/Source/modules/presentation/...
Source/modules/presentation/PresentationController.cpp:75: void
PresentationController::setPresentation(Presentation* presentation)
On 2015/01/07 16:22:16, whywhat wrote:
> On 2015/01/07 14:29:09, Peter Beverloo wrote:
> > Can we limit the scope of what PresentationController knows about
> |presentation|
> > by using the WebPresentationController type instead?
> 
> Not sure I want to introduce another interface and I didn't see such
dependency
> injection in other modules.
> PresentationController already implements WebPresentationController to
abstract
> PresentationDispatcher (WebPresentationClient implementation) from knowing
> anything about PresentationController.

Oh right, that's OK. Just make sure the names of the method on Presentation and
PresentationController stay in sync then :-).

https://codereview.chromium.org/832263007/diff/60001/Source/modules/presentat...
File Source/modules/presentation/AvailableChangeEvent.h (right):

https://codereview.chromium.org/832263007/diff/60001/Source/modules/presentat...
Source/modules/presentation/AvailableChangeEvent.h:14: // availablechange event
implementation used with the Presentation API,
nit: This is the AvailableChangeEvent object definition, not implementation of
the event. It doesn't contain any logic.

https://codereview.chromium.org/832263007/diff/60001/Source/modules/presentat...
File Source/modules/presentation/Presentation.h (right):

https://codereview.chromium.org/832263007/diff/60001/Source/modules/presentat...
Source/modules/presentation/Presentation.h:41: // The UA needs to keep track if
anything is listening to the event so it could stop the might
s/UA/embedder/

https://codereview.chromium.org/832263007/diff/60001/public/platform/WebPrese...
File public/platform/WebPresentationClient.h (right):

https://codereview.chromium.org/832263007/diff/60001/public/platform/WebPrese...
public/platform/WebPresentationClient.h:12: // The implementation the embedder
has to provide for navigator.presentation to work.
nit: s/navigator.presentation/the Presentation API/. The property it's exposed
on doesn't really matter (any may change if you ever decide to expose part of it
in workers).

[whywhat, 2015-01-08 16:05:28]

https://codereview.chromium.org/832263007/diff/1/Source/modules/presentation/...
File Source/modules/presentation/Presentation.cpp (right):

https://codereview.chromium.org/832263007/diff/1/Source/modules/presentation/...
Source/modules/presentation/Presentation.cpp:39:
controller->setPresentation(presentation);
On 2015/01/07 19:53:11, Peter Beverloo wrote:
> On 2015/01/07 16:22:16, whywhat wrote:
> > On 2015/01/07 14:29:08, Peter Beverloo wrote:
> > > When is it valid for the runtime Presentation flag to be enabled, but not
> > having
> > > had a WebPresentationClient provided by the embedder? It seems to me like
> > > presentationController() can ASSERT and return a reference instead.
> > 
> > I imagined a hypothetical situation where the UA might run on hardware that
> > doesn't support screen discovery/presenting and decided to be on the safe
> side.
> 
> In that case we wouldn't enable the run-time flag, causing the entire feature
to
> not be exposed. If the run-time flag is enabled, it's fair to ASSERT existence
> of the PresentationController.

Done.

https://codereview.chromium.org/832263007/diff/1/Source/modules/presentation/...
File Source/modules/presentation/PresentationController.cpp (right):

https://codereview.chromium.org/832263007/diff/1/Source/modules/presentation/...
Source/modules/presentation/PresentationController.cpp:66:
m_client->addDeviceAvailabilityListener();
On 2015/01/07 19:53:11, Peter Beverloo wrote:
> On 2015/01/07 16:22:16, whywhat wrote:
> > On 2015/01/07 14:29:09, Peter Beverloo wrote:
> > > Hm, so the embedder is responsible for doing some kind of reference
counting
> > for
> > > active listeners per WebPresentationController*? Does the embedder care
> about
> > > the exact number of active listeners, or only about the boolean "is there
a
> > > listener" question?
> > 
> > In general, the embedder should care if there's any number listeners or
none.
> > However, I'm not sure it makes sense to do the accounting for listeners
number
> > here rather than delegating to the embedder implementing the Mojo interface.
> > Depending on the discovery methods supported, the UA might ignore the
boolean
> > completely or on the contrary actually care about the number.
> 
> In order to keep information local, we should only tell the embedder if there
> are *any* listeners or *no* listeners - the embedder will not behave
differently
> if there are two event listeners rather than one.

Done.

https://codereview.chromium.org/832263007/diff/1/Source/modules/presentation/...
Source/modules/presentation/PresentationController.cpp:75: void
PresentationController::setPresentation(Presentation* presentation)
On 2015/01/07 19:53:11, Peter Beverloo wrote:
> On 2015/01/07 16:22:16, whywhat wrote:
> > On 2015/01/07 14:29:09, Peter Beverloo wrote:
> > > Can we limit the scope of what PresentationController knows about
> > |presentation|
> > > by using the WebPresentationController type instead?
> > 
> > Not sure I want to introduce another interface and I didn't see such
> dependency
> > injection in other modules.
> > PresentationController already implements WebPresentationController to
> abstract
> > PresentationDispatcher (WebPresentationClient implementation) from knowing
> > anything about PresentationController.
> 
> Oh right, that's OK. Just make sure the names of the method on Presentation
and
> PresentationController stay in sync then :-).

I assume that most of the time the code won't compile then :)

https://codereview.chromium.org/832263007/diff/60001/Source/modules/presentat...
File Source/modules/presentation/AvailableChangeEvent.h (right):

https://codereview.chromium.org/832263007/diff/60001/Source/modules/presentat...
Source/modules/presentation/AvailableChangeEvent.h:14: // availablechange event
implementation used with the Presentation API,
On 2015/01/07 19:53:11, Peter Beverloo wrote:
> nit: This is the AvailableChangeEvent object definition, not implementation of
> the event. It doesn't contain any logic.

Done.

https://codereview.chromium.org/832263007/diff/60001/Source/modules/presentat...
File Source/modules/presentation/Presentation.h (right):

https://codereview.chromium.org/832263007/diff/60001/Source/modules/presentat...
Source/modules/presentation/Presentation.h:41: // The UA needs to keep track if
anything is listening to the event so it could stop the might
On 2015/01/07 19:53:11, Peter Beverloo wrote:
> s/UA/embedder/

Done.

https://codereview.chromium.org/832263007/diff/60001/public/platform/WebPrese...
File public/platform/WebPresentationClient.h (right):

https://codereview.chromium.org/832263007/diff/60001/public/platform/WebPrese...
public/platform/WebPresentationClient.h:12: // The implementation the embedder
has to provide for navigator.presentation to work.
On 2015/01/07 19:53:11, Peter Beverloo wrote:
> nit: s/navigator.presentation/the Presentation API/. The property it's exposed
> on doesn't really matter (any may change if you ever decide to expose part of
it
> in workers).

Done.

[mark a. foltz, 2015-01-09 00:35:09]

mfoltz@chromium.org changed reviewers:
+ mfoltz@chromium.org

[mark a. foltz, 2015-01-09 00:35:10]

A few drive-by comments; I'm not familiar with Blink style so they may be
superfluous.  Otherwise looks good.

I want to understand the object relationships.  Are they:

Document => LocalFrame =0..1=> PresentationController
PresentationController => Presentation

How are multiple presentations controlled by one frame?

https://codereview.chromium.org/832263007/diff/40002/Source/modules/presentat...
File Source/modules/presentation/AvailableChangeEvent.h (right):

https://codereview.chromium.org/832263007/diff/40002/Source/modules/presentat...
Source/modules/presentation/AvailableChangeEvent.h:25: static
PassRefPtrWillBeRawPtr<AvailableChangeEvent> create(const AtomicString&
eventType, bool available)
Won't the event type always be "availablechange"?

https://codereview.chromium.org/832263007/diff/40002/Source/modules/presentat...
File Source/modules/presentation/PresentationController.h (right):

https://codereview.chromium.org/832263007/diff/40002/Source/modules/presentat...
Source/modules/presentation/PresentationController.h:20: class
PresentationController final : public
NoBaseWillBeGarbageCollectedFinalized<PresentationController>, public
WillBeHeapSupplement<LocalFrame>, public WebPresentationController {
Is there a line length limit in the Blink style guide? This will read poorly
unless you have a 200-character terminal window.

https://codereview.chromium.org/832263007/diff/40002/Source/modules/presentat...
Source/modules/presentation/PresentationController.h:34: virtual void
trace(Visitor*) override;
Named parameter?

https://codereview.chromium.org/832263007/diff/40002/Source/modules/presentat...
Source/modules/presentation/PresentationController.h:44: void
setPresentation(Presentation*);
Does this need a named parameter?

https://codereview.chromium.org/832263007/diff/40002/public/web/WebFrameClient.h
File public/web/WebFrameClient.h (right):

https://codereview.chromium.org/832263007/diff/40002/public/web/WebFrameClien...
public/web/WebFrameClient.h:340: virtual WebPresentationClient*
presentationClient() { return 0; }
return nullptr?

[whywhat, 2015-01-22 21:18:08]

On 2015/01/09 00:35:10, mark a. foltz wrote:
> A few drive-by comments; I'm not familiar with Blink style so they may be
> superfluous.  Otherwise looks good.
> 
> I want to understand the object relationships.  Are they:
> 
> Document => LocalFrame =0..1=> PresentationController
> PresentationController => Presentation
> 
> How are multiple presentations controlled by one frame?

So the PresentationController is "provided" to the LocalFrame (so yes,
essentially, it's owned by the frame). LocalFrame is either refcounted or lives
on OilPan heap (garbage collected) depending on the platform. As I understand,
there's one local frame per frame on the page (with the exception for remote
iframes which are also represented by RemoteFrame objects but as I understand
have a LocalFrame associated object as well so I don't need to modify
RemoteFrame) so there's exactly one PresentationController per frame.

Now Presentation object is created by NavigatorPresentation lazily and is
referenced (refcounted or pointed to in OilPan) from both NavigatorPresentation
and PresentationController.
Navigator owns NavigatorPresentation and Navigator is referenced by
LocalDOMWindow which is lazily provided/set to the LocalFrame. So there's
exactly one Presentation object per PresentationController.

> 
>
https://codereview.chromium.org/832263007/diff/40002/Source/modules/presentat...
> File Source/modules/presentation/AvailableChangeEvent.h (right):
> 
>
https://codereview.chromium.org/832263007/diff/40002/Source/modules/presentat...
> Source/modules/presentation/AvailableChangeEvent.h:25: static
> PassRefPtrWillBeRawPtr<AvailableChangeEvent> create(const AtomicString&
> eventType, bool available)
> Won't the event type always be "availablechange"?
> 
>
https://codereview.chromium.org/832263007/diff/40002/Source/modules/presentat...
> File Source/modules/presentation/PresentationController.h (right):
> 
>
https://codereview.chromium.org/832263007/diff/40002/Source/modules/presentat...
> Source/modules/presentation/PresentationController.h:20: class
> PresentationController final : public
> NoBaseWillBeGarbageCollectedFinalized<PresentationController>, public
> WillBeHeapSupplement<LocalFrame>, public WebPresentationController {
> Is there a line length limit in the Blink style guide? This will read poorly
> unless you have a 200-character terminal window.
> 
>
https://codereview.chromium.org/832263007/diff/40002/Source/modules/presentat...
> Source/modules/presentation/PresentationController.h:34: virtual void
> trace(Visitor*) override;
> Named parameter?
> 
>
https://codereview.chromium.org/832263007/diff/40002/Source/modules/presentat...
> Source/modules/presentation/PresentationController.h:44: void
> setPresentation(Presentation*);
> Does this need a named parameter?
> 
>
https://codereview.chromium.org/832263007/diff/40002/public/web/WebFrameClient.h
> File public/web/WebFrameClient.h (right):
> 
>
https://codereview.chromium.org/832263007/diff/40002/public/web/WebFrameClien...
> public/web/WebFrameClient.h:340: virtual WebPresentationClient*
> presentationClient() { return 0; }
> return nullptr?

[whywhat, 2015-01-22 21:18:18]

https://codereview.chromium.org/832263007/diff/40002/Source/modules/presentat...
File Source/modules/presentation/AvailableChangeEvent.h (right):

https://codereview.chromium.org/832263007/diff/40002/Source/modules/presentat...
Source/modules/presentation/AvailableChangeEvent.h:25: static
PassRefPtrWillBeRawPtr<AvailableChangeEvent> create(const AtomicString&
eventType, bool available)
On 2015/01/09 00:35:10, mark a. foltz wrote:
> Won't the event type always be "availablechange"?

Yes, but V8 bindings code calls these static create() methods with the event
type parameter. No idea why :(

https://codereview.chromium.org/832263007/diff/40002/Source/modules/presentat...
File Source/modules/presentation/PresentationController.h (right):

https://codereview.chromium.org/832263007/diff/40002/Source/modules/presentat...
Source/modules/presentation/PresentationController.h:20: class
PresentationController final : public
NoBaseWillBeGarbageCollectedFinalized<PresentationController>, public
WillBeHeapSupplement<LocalFrame>, public WebPresentationController {
On 2015/01/09 00:35:10, mark a. foltz wrote:
> Is there a line length limit in the Blink style guide? This will read poorly
> unless you have a 200-character terminal window.

Nope, there's no limit. :( As I understand it, the style guide doesn't actually
encourage to break the lines either.

https://codereview.chromium.org/832263007/diff/40002/Source/modules/presentat...
Source/modules/presentation/PresentationController.h:34: virtual void
trace(Visitor*) override;
On 2015/01/09 00:35:10, mark a. foltz wrote:
> Named parameter?

Per Blink/WebKit style guide the name that's obvious from the type (e.g.
visitor) is redundant and should be omitted.

https://codereview.chromium.org/832263007/diff/40002/Source/modules/presentat...
Source/modules/presentation/PresentationController.h:44: void
setPresentation(Presentation*);
On 2015/01/09 00:35:10, mark a. foltz wrote:
> Does this need a named parameter?

Ditto.

https://codereview.chromium.org/832263007/diff/40002/public/web/WebFrameClient.h
File public/web/WebFrameClient.h (right):

https://codereview.chromium.org/832263007/diff/40002/public/web/WebFrameClien...
public/web/WebFrameClient.h:340: virtual WebPresentationClient*
presentationClient() { return 0; }
On 2015/01/09 00:35:10, mark a. foltz wrote:
> return nullptr?

I did "return 0;" for consistency. There're many "return 0;" and no "return
nullptr;" statements in this file.

[mlamouri (slow - plz ping), 2015-01-22 23:18:03]

mlamouri@chromium.org changed reviewers:
+ mlamouri@chromium.org

[mlamouri (slow - plz ping), 2015-01-22 23:18:04]

https://codereview.chromium.org/832263007/diff/40002/Source/modules/presentat...
File Source/modules/presentation/Presentation.h (right):

https://codereview.chromium.org/832263007/diff/40002/Source/modules/presentat...
Source/modules/presentation/Presentation.h:56: PresentationController*
presentationController();
Are you sure that this must _never_ return nullptr? It seems that it can when
the frame is detached. Could you check whether it is null before using the
PresentationController when you are not already sure to have a frame? I think
this might fix your tests failures.

[mlamouri (slow - plz ping), 2015-01-23 18:40:09]

Also, I think that NavigatorPresentation doesn't need to be a DOMWindowProperty,
it's already supplementating Navigator that is a DOMWindowProperty.

In addition, you could probably be fine with Presentation being a
DOMWindowProperty instead of a ContextLifeCycleObserver.

[whywhat, 2015-01-23 19:20:19]

Thanks Mounir. I also removed some unnecessary inheritance from the Presentation
and NavigatorPresentation classes as you suggested.

https://codereview.chromium.org/832263007/diff/40002/Source/modules/presentat...
File Source/modules/presentation/Presentation.h (right):

https://codereview.chromium.org/832263007/diff/40002/Source/modules/presentat...
Source/modules/presentation/Presentation.h:56: PresentationController*
presentationController();
On 2015/01/22 23:18:04, Mounir Lamouri wrote:
> Are you sure that this must _never_ return nullptr? It seems that it can when
> the frame is detached. Could you check whether it is null before using the
> PresentationController when you are not already sure to have a frame? I think
> this might fix your tests failures.

Done.

[whywhat, 2015-01-26 10:17:44]

avayvod@chromium.org changed reviewers:
+ mkwst@chromium.org

[whywhat, 2015-01-26 10:17:44]

+Mike for OWNERS review

[mlamouri (slow - plz ping), 2015-01-26 10:24:43]

mlamouri@chromium.org changed reviewers:
- mkwst@chromium.org

[mlamouri (slow - plz ping), 2015-01-26 10:24:44]

lgtm with the FrameObserver/DOMWindowProperty question resolved.

https://codereview.chromium.org/832263007/diff/150001/Source/modules/presenta...
File Source/modules/presentation/AvailableChangeEvent.cpp (right):

https://codereview.chromium.org/832263007/diff/150001/Source/modules/presenta...
Source/modules/presentation/AvailableChangeEvent.cpp:21: ,
m_available(available)
nit: you could call the ctor below if creating a AvailableChangeEventInit on the
fly isn't too much work.

https://codereview.chromium.org/832263007/diff/150001/Source/modules/presenta...
File Source/modules/presentation/AvailableChangeEvent.h (right):

https://codereview.chromium.org/832263007/diff/150001/Source/modules/presenta...
Source/modules/presentation/AvailableChangeEvent.h:15: // see
http://w3c.github.io/presentation-api/#availablechangeevent for details.
nit: can you use HTTPS? :)

https://codereview.chromium.org/832263007/diff/150001/Source/modules/presenta...
File Source/modules/presentation/Presentation.h (right):

https://codereview.chromium.org/832263007/diff/150001/Source/modules/presenta...
Source/modules/presentation/Presentation.h:23: , public FrameDestructionObserver
{
It seems that most things are using DOMWindowProperty. The only difference seems
to be the LocalDOMWindow::registerProperty() call. I wonder if that keeps the
object alive if there is no JS reference to it. Maybe mkwst@ will have some
insights?

[whywhat, 2015-01-26 11:18:05]

https://codereview.chromium.org/832263007/diff/150001/Source/modules/presenta...
File Source/modules/presentation/AvailableChangeEvent.cpp (right):

https://codereview.chromium.org/832263007/diff/150001/Source/modules/presenta...
Source/modules/presentation/AvailableChangeEvent.cpp:21: ,
m_available(available)
On 2015/01/26 10:24:44, Mounir Lamouri wrote:
> nit: you could call the ctor below if creating a AvailableChangeEventInit on
the
> fly isn't too much work.

AvailableChangeEventInit doesn't have a constructor that takes |available| and
the code is generated from the IDL. I wonder if I can add a constructor through
the IDL but I didn't find any existing example in the code base.

TL;DR It is too much work.

https://codereview.chromium.org/832263007/diff/150001/Source/modules/presenta...
File Source/modules/presentation/AvailableChangeEvent.h (right):

https://codereview.chromium.org/832263007/diff/150001/Source/modules/presenta...
Source/modules/presentation/AvailableChangeEvent.h:15: // see
http://w3c.github.io/presentation-api/#availablechangeevent for details.
On 2015/01/26 10:24:44, Mounir Lamouri wrote:
> nit: can you use HTTPS? :)

Done.

https://codereview.chromium.org/832263007/diff/150001/Source/modules/presenta...
File Source/modules/presentation/Presentation.h (right):

https://codereview.chromium.org/832263007/diff/150001/Source/modules/presenta...
Source/modules/presentation/Presentation.h:23: , public FrameDestructionObserver
{
On 2015/01/26 10:24:44, Mounir Lamouri wrote:
> It seems that most things are using DOMWindowProperty. The only difference
seems
> to be the LocalDOMWindow::registerProperty() call. I wonder if that keeps the
> object alive if there is no JS reference to it. Maybe mkwst@ will have some
> insights?

Acknowledged.

[mark a. foltz, 2015-01-26 19:53:39]

lgtm

Minor comments mostly around includes/declarations and a couple of requests for
clarification.

https://codereview.chromium.org/832263007/diff/170001/Source/modules/presenta...
File Source/modules/presentation/NavigatorPresentation.cpp (right):

https://codereview.chromium.org/832263007/diff/170001/Source/modules/presenta...
Source/modules/presentation/NavigatorPresentation.cpp:8: #include
"core/dom/Document.h"
Don't see a reference to Document in this file.

https://codereview.chromium.org/832263007/diff/170001/Source/modules/presenta...
Source/modules/presentation/NavigatorPresentation.cpp:9: #include
"core/frame/LocalFrame.h"
Is this still required?

https://codereview.chromium.org/832263007/diff/170001/Source/modules/presenta...
File Source/modules/presentation/NavigatorPresentation.h (right):

https://codereview.chromium.org/832263007/diff/170001/Source/modules/presenta...
Source/modules/presentation/NavigatorPresentation.h:12: class LocalFrame;
Forward declaration is not used

https://codereview.chromium.org/832263007/diff/170001/Source/modules/presenta...
File Source/modules/presentation/Presentation.cpp (right):

https://codereview.chromium.org/832263007/diff/170001/Source/modules/presenta...
Source/modules/presentation/Presentation.cpp:12: #include "core/dom/Document.h"
Is this required (don't see references to Document)?

https://codereview.chromium.org/832263007/diff/170001/Source/modules/presenta...
File Source/modules/presentation/Presentation.h (right):

https://codereview.chromium.org/832263007/diff/170001/Source/modules/presenta...
Source/modules/presentation/Presentation.h:32: virtual const AtomicString&
interfaceName() const override;
Do AtomicString, String, ExecutionContext, Visitor, etc. need to be forward
declared?

https://codereview.chromium.org/832263007/diff/170001/Source/modules/presenta...
Source/modules/presentation/Presentation.h:39: ScriptPromise
startSession(ScriptState*, const String& senderId, const String&
presentationId);
Which parameter conveys the presented URL?  Should senderId be named
presentationUrl?

https://codereview.chromium.org/832263007/diff/170001/Source/modules/presenta...
Source/modules/presentation/Presentation.h:40: ScriptPromise
joinSession(ScriptState*, const String& senderId, const String& presentationId);
Ditto

https://codereview.chromium.org/832263007/diff/170001/Source/modules/presenta...
File Source/modules/presentation/PresentationController.cpp (right):

https://codereview.chromium.org/832263007/diff/170001/Source/modules/presenta...
Source/modules/presentation/PresentationController.cpp:83: void
PresentationController::willDetachFrameHost()
It what circumstances would a PresentationController be detached from its frame
host? Is this the circumstance where a frame is navigating, in which case, do we
need to reset the availablechanged watched status?

[whywhat, 2015-01-26 20:16:17]

avayvod@chromium.org changed reviewers:
+ dglazkov@chromium.org

[whywhat, 2015-01-26 20:16:18]

Dmitry, since you were already CCed on the email and since I forgot to add
timezone friendlier mkwst (although I thought I did and waited for the review
for the whole day), may you do the honor and review the cl as a suggested owner
of the files I need to change?

Modulo nits Mark just pointed out which I'm going to fix right away.

[whywhat, 2015-01-27 13:51:24]

https://codereview.chromium.org/832263007/diff/170001/Source/modules/presenta...
File Source/modules/presentation/NavigatorPresentation.cpp (right):

https://codereview.chromium.org/832263007/diff/170001/Source/modules/presenta...
Source/modules/presentation/NavigatorPresentation.cpp:8: #include
"core/dom/Document.h"
On 2015/01/26 19:53:38, mark a. foltz wrote:
> Don't see a reference to Document in this file.

Done.

https://codereview.chromium.org/832263007/diff/170001/Source/modules/presenta...
Source/modules/presentation/NavigatorPresentation.cpp:9: #include
"core/frame/LocalFrame.h"
On 2015/01/26 19:53:38, mark a. foltz wrote:
> Is this still required?

Done.

https://codereview.chromium.org/832263007/diff/170001/Source/modules/presenta...
File Source/modules/presentation/Presentation.cpp (right):

https://codereview.chromium.org/832263007/diff/170001/Source/modules/presenta...
Source/modules/presentation/Presentation.cpp:12: #include "core/dom/Document.h"
On 2015/01/26 19:53:38, mark a. foltz wrote:
> Is this required (don't see references to Document)?

There's an implicit reference when converting the document() pointer to the
ExecutionContext base class pointer.

https://codereview.chromium.org/832263007/diff/170001/Source/modules/presenta...
File Source/modules/presentation/Presentation.h (right):

https://codereview.chromium.org/832263007/diff/170001/Source/modules/presenta...
Source/modules/presentation/Presentation.h:32: virtual const AtomicString&
interfaceName() const override;
On 2015/01/26 19:53:38, mark a. foltz wrote:
> Do AtomicString, String, ExecutionContext, Visitor, etc. need to be forward
> declared?

I don't usually forward declare types required in overridden methods - it must
be done in the base class header.
So from those listed, String is the only one that needs a forward declaration
from my point of view (though all of them are included through others somehow it
seems).

https://codereview.chromium.org/832263007/diff/170001/Source/modules/presenta...
Source/modules/presentation/Presentation.h:39: ScriptPromise
startSession(ScriptState*, const String& senderId, const String&
presentationId);
On 2015/01/26 19:53:38, mark a. foltz wrote:
> Which parameter conveys the presented URL?  Should senderId be named
> presentationUrl?

Yes, a copy/paste error from some old API reference page probably...

https://codereview.chromium.org/832263007/diff/170001/Source/modules/presenta...
Source/modules/presentation/Presentation.h:40: ScriptPromise
joinSession(ScriptState*, const String& senderId, const String& presentationId);
On 2015/01/26 19:53:38, mark a. foltz wrote:
> Ditto

Done.

[whywhat, 2015-01-27 13:53:37]

avayvod@chromium.org changed reviewers:
+ mkwst@chromium.org
- dglazkov@chromium.org

[whywhat, 2015-01-27 14:26:55]

https://codereview.chromium.org/832263007/diff/170001/Source/modules/presenta...
File Source/modules/presentation/PresentationController.cpp (right):

https://codereview.chromium.org/832263007/diff/170001/Source/modules/presenta...
Source/modules/presentation/PresentationController.cpp:83: void
PresentationController::willDetachFrameHost()
On 2015/01/26 19:53:39, mark a. foltz wrote:
> It what circumstances would a PresentationController be detached from its
frame
> host? Is this the circumstance where a frame is navigating, in which case, do
we
> need to reset the availablechanged watched status?

I would hope the event would be deleted but my quick testing with logging didn't
reveal that. Added a reset for |availablechange| watched status.

I hope mkwst@ can shed some light here as well :)

[Mike West, 2015-01-27 17:46:09]

(Sorry. I'm way behind the 30m I promised you, Anton)

https://codereview.chromium.org/832263007/diff/150001/Source/modules/presenta...
File Source/modules/presentation/Presentation.h (right):

https://codereview.chromium.org/832263007/diff/150001/Source/modules/presenta...
Source/modules/presentation/Presentation.h:23: , public FrameDestructionObserver
{
On 2015/01/26 11:18:05, whywhat wrote:
> On 2015/01/26 10:24:44, Mounir Lamouri wrote:
> > It seems that most things are using DOMWindowProperty. The only difference
> seems
> > to be the LocalDOMWindow::registerProperty() call. I wonder if that keeps
the
> > object alive if there is no JS reference to it. Maybe mkwst@ will have some
> > insights?
> 
> Acknowledged.

DOMWindowProperty is the right thing to use here; we're exposing it to
JavaScript, and that class handles unregistration properly at the right time.

https://codereview.chromium.org/832263007/diff/170001/Source/modules/presenta...
File Source/modules/presentation/PresentationController.cpp (right):

https://codereview.chromium.org/832263007/diff/170001/Source/modules/presenta...
Source/modules/presentation/PresentationController.cpp:83: void
PresentationController::willDetachFrameHost()
On 2015/01/27 14:26:55, whywhat wrote:
> On 2015/01/26 19:53:39, mark a. foltz wrote:
> > It what circumstances would a PresentationController be detached from its
> frame
> > host? Is this the circumstance where a frame is navigating, in which case,
do
> we
> > need to reset the availablechanged watched status?
> 
> I would hope the event would be deleted but my quick testing with logging
didn't
> reveal that. Added a reset for |availablechange| watched status.
> 
> I hope mkwst@ can shed some light here as well :)

Moving to DOMWindowProperty makes this clearer. It's not a question of detaching
the FrameHost, it's a question of the global object associated with an object.

[whywhat, 2015-01-27 22:06:08]

https://codereview.chromium.org/832263007/diff/150001/Source/modules/presenta...
File Source/modules/presentation/Presentation.h (right):

https://codereview.chromium.org/832263007/diff/150001/Source/modules/presenta...
Source/modules/presentation/Presentation.h:23: , public FrameDestructionObserver
{
On 2015/01/27 17:46:08, Mike West wrote:
> On 2015/01/26 11:18:05, whywhat wrote:
> > On 2015/01/26 10:24:44, Mounir Lamouri wrote:
> > > It seems that most things are using DOMWindowProperty. The only difference
> > seems
> > > to be the LocalDOMWindow::registerProperty() call. I wonder if that keeps
> the
> > > object alive if there is no JS reference to it. Maybe mkwst@ will have
some
> > > insights?
> > 
> > Acknowledged.
> 
> DOMWindowProperty is the right thing to use here; we're exposing it to
> JavaScript, and that class handles unregistration properly at the right time.

Done.

https://codereview.chromium.org/832263007/diff/170001/Source/modules/presenta...
File Source/modules/presentation/PresentationController.cpp (right):

https://codereview.chromium.org/832263007/diff/170001/Source/modules/presenta...
Source/modules/presentation/PresentationController.cpp:83: void
PresentationController::willDetachFrameHost()
On 2015/01/27 17:46:08, Mike West wrote:
> On 2015/01/27 14:26:55, whywhat wrote:
> > On 2015/01/26 19:53:39, mark a. foltz wrote:
> > > It what circumstances would a PresentationController be detached from its
> > frame
> > > host? Is this the circumstance where a frame is navigating, in which case,
> do
> > we
> > > need to reset the availablechanged watched status?
> > 
> > I would hope the event would be deleted but my quick testing with logging
> didn't
> > reveal that. Added a reset for |availablechange| watched status.
> > 
> > I hope mkwst@ can shed some light here as well :)
> 
> Moving to DOMWindowProperty makes this clearer. It's not a question of
detaching
> the FrameHost, it's a question of the global object associated with an object.

Done.

[Mike West, 2015-01-28 15:29:00]

LGTM, thanks.

[whywhat, 2015-01-28 20:11:33]

The CQ bit was checked by avayvod@chromium.org

[commit-bot: I haz the power, 2015-01-28 20:12:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/832263007/230001

[commit-bot: I haz the power, 2015-01-28 20:16:01]

Committed patchset #13 (id:230001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=189136