Add the Presentation API module and a single event target

Added a module for the Presentation API implementation and a dummy navigator.presentation.onavailablechange IDL.

BUG=412331
Intent to implement on blink-dev: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/J0ToSdOQVP8

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

[whywhat, 2014-07-19 01:00:19]

This is just a WIP prototype patch uploaded to make sure it's shared and that I
get feedback early.

[Peter Beverloo, 2014-07-19 10:16:13]

Feedback you ask, feedback you get :-).

Some overall items about this patch:

  * You should guard it behind a flag in RuntimeEnabledFeatures, to be defined
as status=test until it does something useful.

  * You should include a layout test to verify that the API is indeed being
exposed, and the property returns the mock value you specified. Sounds useless,
but it allows you to verify that you break behavior later on :-).

  * Obviously we need an intent to implement before considering landing this.

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

https://codereview.chromium.org/408663002/diff/1/Source/modules/presentation/...
Source/modules/presentation/NavigatorPresentation.cpp:2: *  Copyright (C) 2014
Google Inc.
nit: Need to use the new copyright comment (as in the header file).

https://codereview.chromium.org/408663002/diff/1/Source/modules/presentation/...
Source/modules/presentation/NavigatorPresentation.cpp:38: // static
micro nit: The // static comments aren't part of the Blink styleguide. Not
harmful either, though.

https://codereview.chromium.org/408663002/diff/1/Source/modules/presentation/...
Source/modules/presentation/NavigatorPresentation.cpp:56: Presentation*
NavigatorPresentation::presentation(Navigator& navigator)
You should return a reference since this won't ever be NULL.

https://codereview.chromium.org/408663002/diff/1/Source/modules/presentation/...
Source/modules/presentation/NavigatorPresentation.cpp:63: if (!m_presentation &&
frame())
ASSERT() on frame() instead? When wouldn't there be a frame?

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

https://codereview.chromium.org/408663002/diff/1/Source/modules/presentation/...
Source/modules/presentation/NavigatorPresentation.h:12: namespace WebCore {
nit: s/WebCore/blink/

Elsewhere too, we switched namespaces :-).

https://codereview.chromium.org/408663002/diff/1/Source/modules/presentation/...
Source/modules/presentation/NavigatorPresentation.h:18: : public
NoBaseWillBeGarbageCollectedFinalized<NavigatorPresentation>
No need to use the Oilpan transitional types, you may as well declare this as
being GarbageCollectedFinalized<>.

https://codereview.chromium.org/408663002/diff/1/Source/modules/presentation/...
Source/modules/presentation/NavigatorPresentation.h:28: Presentation*
presentation() const;
nit: This shouldn't be |const| since it can modify |m_presentation|, which
shouldn't be declared as mutable (even though this still is somewhat common).

https://codereview.chromium.org/408663002/diff/1/Source/modules/presentation/...
Source/modules/presentation/NavigatorPresentation.h:28: Presentation*
presentation() const;
You could also make this private since there are no callers outside of the
static ::presentation method.

https://codereview.chromium.org/408663002/diff/1/Source/modules/presentation/...
Source/modules/presentation/NavigatorPresentation.h:30: void trace(Visitor*);
virtual void trace(Visitor*) OVERRIDE;

https://codereview.chromium.org/408663002/diff/1/Source/modules/presentation/...
Source/modules/presentation/NavigatorPresentation.h:42: #endif // Presentation_h
nit: NavigatorPresentation_h

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

https://codereview.chromium.org/408663002/diff/1/Source/modules/presentation/...
Source/modules/presentation/NavigatorPresentation.idl:2: * Copyright (C) 2014
Google Inc. All rights reserved.
nit: use the new copyright header.

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

https://codereview.chromium.org/408663002/diff/1/Source/modules/presentation/...
Source/modules/presentation/Presentation.h:16: , public ActiveDOMObject {
This doesn't have to be an ActiveDOMObject in this patch yet, you should add it
when it's necessary. When you do, be sure to override hasPendingActivity() to
tell the GC whether it's safe to collect it regardless of traceability.

https://codereview.chromium.org/408663002/diff/1/Source/modules/presentation/...
Source/modules/presentation/Presentation.h:20: void trace(Visitor*);
virtual void trace(Visitor*) OVERRIDE;

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

https://codereview.chromium.org/408663002/diff/1/Source/modules/presentation/...
Source/modules/presentation/Presentation.idl:38: readonly attribute boolean
displayAvailable;
Out of interest, is this really going to be a bool rather than a promise? Fine
in either case, I expect you to know your spec :-).

[mark a. foltz, 2014-07-21 18:24:59]

Not a Blink developer so not a lot of direct feedback :-)

I am curious how this spec will interface with Blink garbage collection when it
is more fully fleshed out.

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

https://codereview.chromium.org/408663002/diff/1/Source/modules/presentation/...
Source/modules/presentation/Presentation.idl:38: readonly attribute boolean
displayAvailable;
This isn't part of the CG final report spec.  Is this just a dummy property that
will be replaced later?

[whywhat, 2014-07-23 00:39:26]

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

https://codereview.chromium.org/408663002/diff/1/Source/modules/presentation/...
Source/modules/presentation/NavigatorPresentation.cpp:2: *  Copyright (C) 2014
Google Inc.
On 2014/07/19 10:16:12, Peter Beverloo wrote:
> nit: Need to use the new copyright comment (as in the header file).

Done.

https://codereview.chromium.org/408663002/diff/1/Source/modules/presentation/...
Source/modules/presentation/NavigatorPresentation.cpp:38: // static
On 2014/07/19 10:16:12, Peter Beverloo wrote:
> micro nit: The // static comments aren't part of the Blink styleguide. Not
> harmful either, though.

Acknowledged.

https://codereview.chromium.org/408663002/diff/1/Source/modules/presentation/...
Source/modules/presentation/NavigatorPresentation.cpp:56: Presentation*
NavigatorPresentation::presentation(Navigator& navigator)
On 2014/07/19 10:16:12, Peter Beverloo wrote:
> You should return a reference since this won't ever be NULL.

Done. I wonder if it should be NULL in cases when the device doesn't support
effective mirroring to a remote agent or second screen though (e.g. cheap phones
that can't share screen with Chromecasts or connect to anything via a microUSB
to HDMI cable).

https://codereview.chromium.org/408663002/diff/1/Source/modules/presentation/...
Source/modules/presentation/NavigatorPresentation.cpp:63: if (!m_presentation &&
frame())
On 2014/07/19 10:16:12, Peter Beverloo wrote:
> ASSERT() on frame() instead? When wouldn't there be a frame?

No idea. Seems everyone's checking for it :)

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

https://codereview.chromium.org/408663002/diff/1/Source/modules/presentation/...
Source/modules/presentation/NavigatorPresentation.h:12: namespace WebCore {
On 2014/07/19 10:16:12, Peter Beverloo wrote:
> nit: s/WebCore/blink/
> 
> Elsewhere too, we switched namespaces :-).

Done.

https://codereview.chromium.org/408663002/diff/1/Source/modules/presentation/...
Source/modules/presentation/NavigatorPresentation.h:18: : public
NoBaseWillBeGarbageCollectedFinalized<NavigatorPresentation>
On 2014/07/19 10:16:12, Peter Beverloo wrote:
> No need to use the Oilpan transitional types, you may as well declare this as
> being GarbageCollectedFinalized<>.

It seems like I have to use WillBeHeapSupplement with Navigator for some reason,
it doesn't compile if using plain HeapSupplement (maybe I need to add blink::
since that's what WillBeHeapSupplement is)?

Done for the other classes.

https://codereview.chromium.org/408663002/diff/1/Source/modules/presentation/...
Source/modules/presentation/NavigatorPresentation.h:28: Presentation*
presentation() const;
On 2014/07/19 10:16:12, Peter Beverloo wrote:
> You could also make this private since there are no callers outside of the
> static ::presentation method.

Done.

https://codereview.chromium.org/408663002/diff/1/Source/modules/presentation/...
Source/modules/presentation/NavigatorPresentation.h:28: Presentation*
presentation() const;
On 2014/07/19 10:16:12, Peter Beverloo wrote:
> nit: This shouldn't be |const| since it can modify |m_presentation|, which
> shouldn't be declared as mutable (even though this still is somewhat common).

Done.

https://codereview.chromium.org/408663002/diff/1/Source/modules/presentation/...
Source/modules/presentation/NavigatorPresentation.h:30: void trace(Visitor*);
On 2014/07/19 10:16:12, Peter Beverloo wrote:
> virtual void trace(Visitor*) OVERRIDE;

Done.

https://codereview.chromium.org/408663002/diff/1/Source/modules/presentation/...
Source/modules/presentation/NavigatorPresentation.h:42: #endif // Presentation_h
On 2014/07/19 10:16:12, Peter Beverloo wrote:
> nit: NavigatorPresentation_h

Done.

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

https://codereview.chromium.org/408663002/diff/1/Source/modules/presentation/...
Source/modules/presentation/NavigatorPresentation.idl:2: * Copyright (C) 2014
Google Inc. All rights reserved.
On 2014/07/19 10:16:12, Peter Beverloo wrote:
> nit: use the new copyright header.

Done.

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

https://codereview.chromium.org/408663002/diff/1/Source/modules/presentation/...
Source/modules/presentation/Presentation.h:16: , public ActiveDOMObject {
On 2014/07/19 10:16:13, Peter Beverloo wrote:
> This doesn't have to be an ActiveDOMObject in this patch yet, you should add
it
> when it's necessary. When you do, be sure to override hasPendingActivity() to
> tell the GC whether it's safe to collect it regardless of traceability.

Ok. Seems like it has to be something so I made it a DOMWindowProperty for now.

https://codereview.chromium.org/408663002/diff/1/Source/modules/presentation/...
Source/modules/presentation/Presentation.h:20: void trace(Visitor*);
On 2014/07/19 10:16:13, Peter Beverloo wrote:
> virtual void trace(Visitor*) OVERRIDE;

This is not declared in any of the base classes of this one :(

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

https://codereview.chromium.org/408663002/diff/1/Source/modules/presentation/...
Source/modules/presentation/Presentation.idl:38: readonly attribute boolean
displayAvailable;
On 2014/07/19 10:16:13, Peter Beverloo wrote:
> Out of interest, is this really going to be a bool rather than a promise? Fine
> in either case, I expect you to know your spec :-).

We must have an event handler since the device availability might change over
time without the website doing anything. Having a promise for the property then
seems like too much and as Mark mentions below, the property itself is absent
from the current spec, there's just an event handler.

https://codereview.chromium.org/408663002/diff/1/Source/modules/presentation/...
Source/modules/presentation/Presentation.idl:38: readonly attribute boolean
displayAvailable;
On 2014/07/21 18:24:58, mark a. foltz wrote:
> This isn't part of the CG final report spec.  Is this just a dummy property
that
> will be replaced later?

Yep.

[whywhat, 2014-07-23 00:41:13]

On 2014/07/19 10:16:13, Peter Beverloo wrote:
> Feedback you ask, feedback you get :-).
> 
> Some overall items about this patch:
> 
>   * You should guard it behind a flag in RuntimeEnabledFeatures, to be defined
> as status=test until it does something useful.

Can I use experimental? I like running a local build of Chrome with the flag and
see the feature for myself (even now).

> 
>   * You should include a layout test to verify that the API is indeed being
> exposed, and the property returns the mock value you specified. Sounds
useless,
> but it allows you to verify that you break behavior later on :-).

Done. The sooner you add a test the higher the likeliness you will have it going
further.

> 
>   * Obviously we need an intent to implement before considering landing this.

Agree. This is for a prototyping only.

> 
>
https://codereview.chromium.org/408663002/diff/1/Source/modules/presentation/...
> File Source/modules/presentation/NavigatorPresentation.cpp (right):
> 
>
https://codereview.chromium.org/408663002/diff/1/Source/modules/presentation/...
> Source/modules/presentation/NavigatorPresentation.cpp:2: *  Copyright (C) 2014
> Google Inc.
> nit: Need to use the new copyright comment (as in the header file).
> 
>
https://codereview.chromium.org/408663002/diff/1/Source/modules/presentation/...
> Source/modules/presentation/NavigatorPresentation.cpp:38: // static
> micro nit: The // static comments aren't part of the Blink styleguide. Not
> harmful either, though.
> 
>
https://codereview.chromium.org/408663002/diff/1/Source/modules/presentation/...
> Source/modules/presentation/NavigatorPresentation.cpp:56: Presentation*
> NavigatorPresentation::presentation(Navigator& navigator)
> You should return a reference since this won't ever be NULL.
> 
>
https://codereview.chromium.org/408663002/diff/1/Source/modules/presentation/...
> Source/modules/presentation/NavigatorPresentation.cpp:63: if (!m_presentation
&&
> frame())
> ASSERT() on frame() instead? When wouldn't there be a frame?
> 
>
https://codereview.chromium.org/408663002/diff/1/Source/modules/presentation/...
> File Source/modules/presentation/NavigatorPresentation.h (right):
> 
>
https://codereview.chromium.org/408663002/diff/1/Source/modules/presentation/...
> Source/modules/presentation/NavigatorPresentation.h:12: namespace WebCore {
> nit: s/WebCore/blink/
> 
> Elsewhere too, we switched namespaces :-).
> 
>
https://codereview.chromium.org/408663002/diff/1/Source/modules/presentation/...
> Source/modules/presentation/NavigatorPresentation.h:18: : public
> NoBaseWillBeGarbageCollectedFinalized<NavigatorPresentation>
> No need to use the Oilpan transitional types, you may as well declare this as
> being GarbageCollectedFinalized<>.
> 
>
https://codereview.chromium.org/408663002/diff/1/Source/modules/presentation/...
> Source/modules/presentation/NavigatorPresentation.h:28: Presentation*
> presentation() const;
> nit: This shouldn't be |const| since it can modify |m_presentation|, which
> shouldn't be declared as mutable (even though this still is somewhat common).
> 
>
https://codereview.chromium.org/408663002/diff/1/Source/modules/presentation/...
> Source/modules/presentation/NavigatorPresentation.h:28: Presentation*
> presentation() const;
> You could also make this private since there are no callers outside of the
> static ::presentation method.
> 
>
https://codereview.chromium.org/408663002/diff/1/Source/modules/presentation/...
> Source/modules/presentation/NavigatorPresentation.h:30: void trace(Visitor*);
> virtual void trace(Visitor*) OVERRIDE;
> 
>
https://codereview.chromium.org/408663002/diff/1/Source/modules/presentation/...
> Source/modules/presentation/NavigatorPresentation.h:42: #endif //
Presentation_h
> nit: NavigatorPresentation_h
> 
>
https://codereview.chromium.org/408663002/diff/1/Source/modules/presentation/...
> File Source/modules/presentation/NavigatorPresentation.idl (right):
> 
>
https://codereview.chromium.org/408663002/diff/1/Source/modules/presentation/...
> Source/modules/presentation/NavigatorPresentation.idl:2: * Copyright (C) 2014
> Google Inc. All rights reserved.
> nit: use the new copyright header.
> 
>
https://codereview.chromium.org/408663002/diff/1/Source/modules/presentation/...
> File Source/modules/presentation/Presentation.h (right):
> 
>
https://codereview.chromium.org/408663002/diff/1/Source/modules/presentation/...
> Source/modules/presentation/Presentation.h:16: , public ActiveDOMObject {
> This doesn't have to be an ActiveDOMObject in this patch yet, you should add
it
> when it's necessary. When you do, be sure to override hasPendingActivity() to
> tell the GC whether it's safe to collect it regardless of traceability.
> 
>
https://codereview.chromium.org/408663002/diff/1/Source/modules/presentation/...
> Source/modules/presentation/Presentation.h:20: void trace(Visitor*);
> virtual void trace(Visitor*) OVERRIDE;
> 
>
https://codereview.chromium.org/408663002/diff/1/Source/modules/presentation/...
> File Source/modules/presentation/Presentation.idl (right):
> 
>
https://codereview.chromium.org/408663002/diff/1/Source/modules/presentation/...
> Source/modules/presentation/Presentation.idl:38: readonly attribute boolean
> displayAvailable;
> Out of interest, is this really going to be a bool rather than a promise? Fine
> in either case, I expect you to know your spec :-).

[Peter Beverloo, 2014-07-25 18:01:18]

> Can I use experimental? I like running a local build of
> Chrome with the flag and see the feature for myself (even
> now).

It would be exposed to many developers, without doing something useful yet. I
personally prefer to kick things up to status=experimental when they have at
least very limited functionality, but others are generally fine with doing this
earlier.

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

https://codereview.chromium.org/408663002/diff/1/Source/modules/presentation/...
Source/modules/presentation/NavigatorPresentation.cpp:56: Presentation*
NavigatorPresentation::presentation(Navigator& navigator)
On 2014/07/23 00:39:25, whywhat wrote:
> On 2014/07/19 10:16:12, Peter Beverloo wrote:
> > You should return a reference since this won't ever be NULL.
> 
> Done. I wonder if it should be NULL in cases when the device doesn't support
> effective mirroring to a remote agent or second screen though (e.g. cheap
phones
> that can't share screen with Chromecasts or connect to anything via a microUSB
> to HDMI cable). 

Sounds like something you should have a design document for.

https://codereview.chromium.org/408663002/diff/40001/LayoutTests/presentation...
File LayoutTests/presentation/presentation-api.html (right):

https://codereview.chromium.org/408663002/diff/40001/LayoutTests/presentation...
LayoutTests/presentation/presentation-api.html:27: assert_true('dispatchEvent'
in navigator.presentation);
nit [25-27]: no need to test EventTarget or the instanceof operator :-).

https://codereview.chromium.org/408663002/diff/40001/Source/modules/presentat...
File Source/modules/presentation/NavigatorPresentation.cpp (right):

https://codereview.chromium.org/408663002/diff/40001/Source/modules/presentat...
Source/modules/presentation/NavigatorPresentation.cpp:8: #include
"core/dom/Document.h"
nit: This seems to be unused.

https://codereview.chromium.org/408663002/diff/40001/Source/modules/presentat...
Source/modules/presentation/NavigatorPresentation.cpp:11: #include
"modules/presentation/Presentation.h"
nit: You include this in the header.

https://codereview.chromium.org/408663002/diff/40001/Source/modules/presentat...
Source/modules/presentation/NavigatorPresentation.cpp:50: ASSERT(frame());
Actually, I think frame() can return NULL when a document creates an iframe in
the same origin, gets a reference to the |navigator| instance of that document,
and then accesses the |presentation| property on it..

https://codereview.chromium.org/408663002/diff/40001/Source/modules/presentat...
File Source/modules/presentation/NavigatorPresentation.h (right):

https://codereview.chromium.org/408663002/diff/40001/Source/modules/presentat...
Source/modules/presentation/NavigatorPresentation.h:15: class Presentation;
nit: You include Presentation.h, so no need to forward declare.

https://codereview.chromium.org/408663002/diff/40001/Source/modules/presentat...
Source/modules/presentation/NavigatorPresentation.h:18: : public
GarbageCollectedFinalized<NavigatorPresentation>
mmm. It looks like every other module defining a partial Navigator interface
still uses NoBaseWillBeGarbageCollectedFinalized<> here. Let's go for parity?

https://codereview.chromium.org/408663002/diff/40001/Source/modules/presentat...
Source/modules/presentation/NavigatorPresentation.h:20: , public
DOMWindowProperty {
WILL_BE_USING_GARBAGE_COLLECTED_MIXIN(NavigatorPresentation);

https://codereview.chromium.org/408663002/diff/40001/Source/modules/presentat...
Source/modules/presentation/NavigatorPresentation.h:27: void trace(Visitor*)
OVERRIDE;
This should be virtual. The method will in fact be provided in the ancestry,
namely through HeapSupplement<> (which extends GarbageCollectedMixin, which
defines trace() as pure virtual).

https://codereview.chromium.org/408663002/diff/40001/Source/modules/presentat...
Source/modules/presentation/NavigatorPresentation.h:32:
NavigatorPresentation(LocalFrame*);
Constructors with only one argument should be marked as explicit.

https://codereview.chromium.org/408663002/diff/40001/Source/modules/presentat...
Source/modules/presentation/NavigatorPresentation.h:36:
PersistentWillBeMember<Presentation> m_presentation;
s/PersistentWillBeMember/Member/?

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

https://codereview.chromium.org/408663002/diff/40001/Source/modules/presentat...
Source/modules/presentation/Presentation.h:17: , DOMWindowProperty {
This shouldn't be a DOMWindowProperty, definitely not one without a visibility
keyword :-).

In order to support the EventTarget implementation, you need access to the
execution context.

Looking to the near future, I guess you will need to tell the embedder when to
start and stop listening to changes in active devices. For that you would extend
from ContextLifecycleObserver, or from ActiveDOMObject if there's a pending
operation to keep the object alive.

Extending from CLO seems fine for now. If you don't want to do that, you could
keep the execution context as a weak member (RawPtrWillBeWeakMember).

https://codereview.chromium.org/408663002/diff/40001/Source/modules/presentat...
File Source/modules/presentation/Presentation.idl (right):

https://codereview.chromium.org/408663002/diff/40001/Source/modules/presentat...
Source/modules/presentation/Presentation.idl:8: GarbageCollected,
Should this be an interface object?

[whywhat, 2014-08-19 18:07:35]

PTAL

Eric, I added you instead of Peter who's on vacation. Could you take over the
review for now, please?

https://codereview.chromium.org/408663002/diff/40001/LayoutTests/presentation...
File LayoutTests/presentation/presentation-api.html (right):

https://codereview.chromium.org/408663002/diff/40001/LayoutTests/presentation...
LayoutTests/presentation/presentation-api.html:27: assert_true('dispatchEvent'
in navigator.presentation);
On 2014/07/25 18:01:17, Peter Beverloo (OOO - Sept 1) wrote:
> nit [25-27]: no need to test EventTarget or the instanceof operator :-).

I'm testing that navigator.presentation is EventTarget. I guess I could check
for only one of those conditions (with instanceof) in case EventTarget
properties ever change.

https://codereview.chromium.org/408663002/diff/40001/Source/modules/presentat...
File Source/modules/presentation/NavigatorPresentation.cpp (right):

https://codereview.chromium.org/408663002/diff/40001/Source/modules/presentat...
Source/modules/presentation/NavigatorPresentation.cpp:8: #include
"core/dom/Document.h"
On 2014/07/25 18:01:17, Peter Beverloo (OOO - Sept 1) wrote:
> nit: This seems to be unused.

It's needed for the compiler to know that Document* returned by document() can
be converted to ExecutionContext since it inherits from it.

https://codereview.chromium.org/408663002/diff/40001/Source/modules/presentat...
Source/modules/presentation/NavigatorPresentation.cpp:11: #include
"modules/presentation/Presentation.h"
On 2014/07/25 18:01:17, Peter Beverloo (OOO - Sept 1) wrote:
> nit: You include this in the header.

Done.

https://codereview.chromium.org/408663002/diff/40001/Source/modules/presentat...
Source/modules/presentation/NavigatorPresentation.cpp:50: ASSERT(frame());
On 2014/07/25 18:01:17, Peter Beverloo (OOO - Sept 1) wrote:
> Actually, I think frame() can return NULL when a document creates an iframe in
> the same origin, gets a reference to the |navigator| instance of that
document,
> and then accesses the |presentation| property on it..

So how do I create a presentation object then? Or should I revert presentation()
to return a pointer and return NULL in the case you describe?

https://codereview.chromium.org/408663002/diff/40001/Source/modules/presentat...
File Source/modules/presentation/NavigatorPresentation.h (right):

https://codereview.chromium.org/408663002/diff/40001/Source/modules/presentat...
Source/modules/presentation/NavigatorPresentation.h:15: class Presentation;
On 2014/07/25 18:01:18, Peter Beverloo (OOO - Sept 1) wrote:
> nit: You include Presentation.h, so no need to forward declare.

Removed the header. Prefer to forward declare.

https://codereview.chromium.org/408663002/diff/40001/Source/modules/presentat...
Source/modules/presentation/NavigatorPresentation.h:18: : public
GarbageCollectedFinalized<NavigatorPresentation>
On 2014/07/25 18:01:18, Peter Beverloo (OOO - Sept 1) wrote:
> mmm. It looks like every other module defining a partial Navigator interface
> still uses NoBaseWillBeGarbageCollectedFinalized<> here. Let's go for parity?

Done.

https://codereview.chromium.org/408663002/diff/40001/Source/modules/presentat...
Source/modules/presentation/NavigatorPresentation.h:20: , public
DOMWindowProperty {
On 2014/07/25 18:01:17, Peter Beverloo (OOO - Sept 1) wrote:
> WILL_BE_USING_GARBAGE_COLLECTED_MIXIN(NavigatorPresentation);

Done.

https://codereview.chromium.org/408663002/diff/40001/Source/modules/presentat...
Source/modules/presentation/NavigatorPresentation.h:27: void trace(Visitor*)
OVERRIDE;
On 2014/07/25 18:01:18, Peter Beverloo (OOO - Sept 1) wrote:
> This should be virtual. The method will in fact be provided in the ancestry,
> namely through HeapSupplement<> (which extends GarbageCollectedMixin, which
> defines trace() as pure virtual).

Done.

https://codereview.chromium.org/408663002/diff/40001/Source/modules/presentat...
Source/modules/presentation/NavigatorPresentation.h:32:
NavigatorPresentation(LocalFrame*);
On 2014/07/25 18:01:17, Peter Beverloo (OOO - Sept 1) wrote:
> Constructors with only one argument should be marked as explicit.

Done.

https://codereview.chromium.org/408663002/diff/40001/Source/modules/presentat...
Source/modules/presentation/NavigatorPresentation.h:36:
PersistentWillBeMember<Presentation> m_presentation;
On 2014/07/25 18:01:17, Peter Beverloo (OOO - Sept 1) wrote:
> s/PersistentWillBeMember/Member/?

Nah, let's go for consistency.

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

https://codereview.chromium.org/408663002/diff/40001/Source/modules/presentat...
Source/modules/presentation/Presentation.h:17: , DOMWindowProperty {
On 2014/07/25 18:01:18, Peter Beverloo (OOO - Sept 1) wrote:
> This shouldn't be a DOMWindowProperty, definitely not one without a visibility
> keyword :-).
> 
> In order to support the EventTarget implementation, you need access to the
> execution context.
> 
> Looking to the near future, I guess you will need to tell the embedder when to
> start and stop listening to changes in active devices. For that you would
extend
> from ContextLifecycleObserver, or from ActiveDOMObject if there's a pending
> operation to keep the object alive.
> 
> Extending from CLO seems fine for now. If you don't want to do that, you could
> keep the execution context as a weak member (RawPtrWillBeWeakMember).

Done.

https://codereview.chromium.org/408663002/diff/40001/Source/modules/presentat...
File Source/modules/presentation/Presentation.idl (right):

https://codereview.chromium.org/408663002/diff/40001/Source/modules/presentat...
Source/modules/presentation/Presentation.idl:8: GarbageCollected,
On 2014/07/25 18:01:18, Peter Beverloo (OOO - Sept 1) wrote:
> Should this be an interface object?

It is an interface? What do you mean?

[eseidel, 2014-08-19 18:20:58]

I'm not confident I understand the oilpan bits correctly.  But otherwise this
lgtm.

https://codereview.chromium.org/408663002/diff/60001/Source/modules/presentat...
File Source/modules/presentation/NavigatorPresentation.cpp (right):

https://codereview.chromium.org/408663002/diff/60001/Source/modules/presentat...
Source/modules/presentation/NavigatorPresentation.cpp:52: return
*m_presentation.get();
Won't this fail if frame is null?

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

https://codereview.chromium.org/408663002/diff/60001/Source/modules/presentat...
Source/modules/presentation/NavigatorPresentation.h:16: class
NavigatorPresentation FINAL
Few classes in Blink have ever achieved this level of multi-inheritance.  You
deserve an award.

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

https://codereview.chromium.org/408663002/diff/60001/Source/modules/presentat...
Source/modules/presentation/Presentation.cpp:25: return new
Presentation(executionContext);
I guess we can kill all these create() functions in the oilpan world.

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

https://codereview.chromium.org/408663002/diff/60001/Source/modules/presentat...
Source/modules/presentation/Presentation.h:24: virtual const WTF::AtomicString&
interfaceName() const OVERRIDE;
No WTF is needed.

[Mads Ager (chromium), 2014-08-20 07:14:00]

Oilpan bits LGTM

[Mads Ager (chromium), 2014-08-20 07:27:36]

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

https://codereview.chromium.org/408663002/diff/60001/Source/modules/presentat...
Source/modules/presentation/Presentation.h:18:
DEFINE_EVENT_TARGET_REFCOUNTING(EventTargetWithInlineData);
Looking at this again, this doesn't look right. You need Presentation to support
ref counting when compiling with enable_oilpan=0. You should use the base class
in the DEFINE_EVENT_TARGET_REFCOUNTING macro.

So this should be:

class Presentation FINAL
    : public
RefCountedGarbageCollectedWillBeGarbageCollectedFinalized<Presentation>
    , public EventTargetWithInlineData,
    , ContextLifecycleObserver {
   
DEFINE_EVENT_TARGET_REFCOUNTING_WILL_BE_REMOVED(RefCountedGarbageCollected<Presentation>);

[whywhat, 2014-08-20 16:40:42]

https://codereview.chromium.org/408663002/diff/60001/Source/modules/presentat...
File Source/modules/presentation/NavigatorPresentation.cpp (right):

https://codereview.chromium.org/408663002/diff/60001/Source/modules/presentat...
Source/modules/presentation/NavigatorPresentation.cpp:52: return
*m_presentation.get();
On 2014/08/19 18:20:58, eseidel wrote:
> Won't this fail if frame is null?

Should I then change the return type to a pointer and return null?

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

https://codereview.chromium.org/408663002/diff/60001/Source/modules/presentat...
Source/modules/presentation/NavigatorPresentation.h:16: class
NavigatorPresentation FINAL
On 2014/08/19 18:20:58, eseidel wrote:
> Few classes in Blink have ever achieved this level of multi-inheritance.  You
> deserve an award.

Acknowledged.

Not sure if it's a compliment or what? :)

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

https://codereview.chromium.org/408663002/diff/60001/Source/modules/presentat...
Source/modules/presentation/Presentation.cpp:25: return new
Presentation(executionContext);
On 2014/08/19 18:20:58, eseidel wrote:
> I guess we can kill all these create() functions in the oilpan world.

Acknowledged.

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

https://codereview.chromium.org/408663002/diff/60001/Source/modules/presentat...
Source/modules/presentation/Presentation.h:18:
DEFINE_EVENT_TARGET_REFCOUNTING(EventTargetWithInlineData);
On 2014/08/20 07:27:36, Mads Ager (chromium) wrote:
> Looking at this again, this doesn't look right. You need Presentation to
support
> ref counting when compiling with enable_oilpan=0. You should use the base
class
> in the DEFINE_EVENT_TARGET_REFCOUNTING macro.
> 
> So this should be:
> 
> class Presentation FINAL
>     : public
> RefCountedGarbageCollectedWillBeGarbageCollectedFinalized<Presentation>
>     , public EventTargetWithInlineData,
>     , ContextLifecycleObserver {
>    
>
DEFINE_EVENT_TARGET_REFCOUNTING_WILL_BE_REMOVED(RefCountedGarbageCollected<Presentation>);

Done.

https://codereview.chromium.org/408663002/diff/60001/Source/modules/presentat...
Source/modules/presentation/Presentation.h:24: virtual const WTF::AtomicString&
interfaceName() const OVERRIDE;
On 2014/08/19 18:20:58, eseidel wrote:
> No WTF is needed.

Done.

[whywhat, 2014-09-10 19:45:43]

PTAL

[Peter Beverloo, 2014-09-12 12:22:09]

https://codereview.chromium.org/408663002/diff/120001/LayoutTests/presentatio...
File LayoutTests/presentation/presentation-api.html (right):

https://codereview.chromium.org/408663002/diff/120001/LayoutTests/presentatio...
LayoutTests/presentation/presentation-api.html:22:
assert_true(navigator.presentation instanceof EventTarget)
I still think we only have to keep the assert from line 22. The rest are
redundant. But not a huge issue.

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

https://codereview.chromium.org/408663002/diff/120001/Source/modules/presenta...
Source/modules/presentation/NavigatorPresentation.cpp:61:
WillBeHeapSupplement<Navigator>::trace(visitor);
+DOMWindowProperty::trace(visitor)

https://codereview.chromium.org/408663002/diff/120001/Source/modules/presenta...
File Source/modules/presentation/NavigatorPresentation.idl (right):

https://codereview.chromium.org/408663002/diff/120001/Source/modules/presenta...
Source/modules/presentation/NavigatorPresentation.idl:9: ]
nit: we usually don't have a blank line between "]" and the interface
definition.

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

https://codereview.chromium.org/408663002/diff/120001/Source/modules/presenta...
Source/modules/presentation/Presentation.h:16: , ContextLifecycleObserver {
Please indicate visibility of this parent class.

https://codereview.chromium.org/408663002/diff/120001/Source/modules/presenta...
Source/modules/presentation/Presentation.h:19: public:
Do you need WILL_BE_USING_GARBAGE_COLLECTED_MIXIN() here?

https://codereview.chromium.org/408663002/diff/120001/Source/modules/presenta...
File Source/modules/presentation/Presentation.idl (right):

https://codereview.chromium.org/408663002/diff/120001/Source/modules/presenta...
Source/modules/presentation/Presentation.idl:11: attribute EventHandler
onavailablechange;
nit: four space indent.

[whywhat, 2014-09-12 15:01:15]

https://codereview.chromium.org/408663002/diff/120001/LayoutTests/presentatio...
File LayoutTests/presentation/presentation-api.html (right):

https://codereview.chromium.org/408663002/diff/120001/LayoutTests/presentatio...
LayoutTests/presentation/presentation-api.html:22:
assert_true(navigator.presentation instanceof EventTarget)
On 2014/09/12 12:22:09, Peter Beverloo wrote:
> I still think we only have to keep the assert from line 22. The rest are
> redundant. But not a huge issue.

Done.

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

https://codereview.chromium.org/408663002/diff/120001/Source/modules/presenta...
Source/modules/presentation/NavigatorPresentation.cpp:61:
WillBeHeapSupplement<Navigator>::trace(visitor);
On 2014/09/12 12:22:09, Peter Beverloo wrote:
> +DOMWindowProperty::trace(visitor)

Done.

https://codereview.chromium.org/408663002/diff/120001/Source/modules/presenta...
File Source/modules/presentation/NavigatorPresentation.idl (right):

https://codereview.chromium.org/408663002/diff/120001/Source/modules/presenta...
Source/modules/presentation/NavigatorPresentation.idl:9: ]
On 2014/09/12 12:22:09, Peter Beverloo wrote:
> nit: we usually don't have a blank line between "]" and the interface
> definition.

Done.

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

https://codereview.chromium.org/408663002/diff/120001/Source/modules/presenta...
Source/modules/presentation/Presentation.h:16: , ContextLifecycleObserver {
On 2014/09/12 12:22:09, Peter Beverloo wrote:
> Please indicate visibility of this parent class.

Done.

https://codereview.chromium.org/408663002/diff/120001/Source/modules/presenta...
Source/modules/presentation/Presentation.h:19: public:
On 2014/09/12 12:22:09, Peter Beverloo wrote:
> Do you need WILL_BE_USING_GARBAGE_COLLECTED_MIXIN() here?

I believe I do. At least to be consistent with other Navigator* objects like
Battery/Geolocation/etc.

https://codereview.chromium.org/408663002/diff/120001/Source/modules/presenta...
File Source/modules/presentation/Presentation.idl (right):

https://codereview.chromium.org/408663002/diff/120001/Source/modules/presenta...
Source/modules/presentation/Presentation.idl:11: attribute EventHandler
onavailablechange;
On 2014/09/12 12:22:09, Peter Beverloo wrote:
> nit: four space indent.

Done.

[Peter Beverloo, 2014-09-12 15:07:23]

lgtm

https://codereview.chromium.org/408663002/diff/140001/LayoutTests/presentatio...
File LayoutTests/presentation/presentation-api.html (right):

https://codereview.chromium.org/408663002/diff/140001/LayoutTests/presentatio...
LayoutTests/presentation/presentation-api.html:19:
assert_true(navigator.presentation instanceof EventTarget)
microscopic nit: missing ;

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

https://codereview.chromium.org/408663002/diff/140001/Source/modules/presenta...
Source/modules/presentation/NavigatorPresentation.cpp:52: return 0;
Out of interest, when would this be a valid case? Should we ASSERT instead?

https://codereview.chromium.org/408663002/diff/140001/Source/modules/presenta...
File Source/modules/presentation/Presentation.idl (right):

https://codereview.chromium.org/408663002/diff/140001/Source/modules/presenta...
Source/modules/presentation/Presentation.idl:5: //
http://webscreens.github.io/presentation-api/#presentation-interface
You do know that this doesn't match the current draft?

[Peter Beverloo, 2014-09-12 15:08:26]

Finally, please add a link to the Intent to Implement in your description.

[whywhat, 2014-09-12 15:17:43]

https://codereview.chromium.org/408663002/diff/140001/LayoutTests/presentatio...
File LayoutTests/presentation/presentation-api.html (right):

https://codereview.chromium.org/408663002/diff/140001/LayoutTests/presentatio...
LayoutTests/presentation/presentation-api.html:19:
assert_true(navigator.presentation instanceof EventTarget)
On 2014/09/12 15:07:23, Peter Beverloo wrote:
> microscopic nit: missing ;

Done.

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

https://codereview.chromium.org/408663002/diff/140001/Source/modules/presenta...
Source/modules/presentation/NavigatorPresentation.cpp:52: return 0;
On 2014/09/12 15:07:23, Peter Beverloo wrote:
> Out of interest, when would this be a valid case? Should we ASSERT instead?

Hm, in comment #8 Eric asked "won't it fail it frame() is null" so I believed it
is possible :)

https://codereview.chromium.org/408663002/diff/140001/Source/modules/presenta...
File Source/modules/presentation/Presentation.idl (right):

https://codereview.chromium.org/408663002/diff/140001/Source/modules/presenta...
Source/modules/presentation/Presentation.idl:5: //
http://webscreens.github.io/presentation-api/#presentation-interface
On 2014/09/12 15:07:23, Peter Beverloo wrote:
> You do know that this doesn't match the current draft?

I believe the draft is incorrect in the naming of interfaces: Navigator should
be NavigatorPresentation and then Presentation is just Presentation (at least if
it should match Blink IDL naming which I'm not 100% sure about; should it?).

[whywhat, 2014-09-12 15:19:53]

On 2014/09/12 15:08:26, Peter Beverloo wrote:
> Finally, please add a link to the Intent to Implement in your description.

Do I need to add it to every implementation CL? I've added it to this one and to
the bug for the future.

[Peter Beverloo, 2014-09-12 15:24:36]

On 2014/09/12 15:19:53, whywhat wrote:
> On 2014/09/12 15:08:26, Peter Beverloo wrote:
> > Finally, please add a link to the Intent to Implement in your description.
> 
> Do I need to add it to every implementation CL? I've added it to this one and
to
> the bug for the future.

No, just this one is fine, given that it introduces your module.

[whywhat, 2014-09-15 12:41:33]

The CQ bit was checked by avayvod@chromium.org

[commit-bot: I haz the power, 2014-09-15 12:42:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/408663002/200001

[commit-bot: I haz the power, 2014-09-15 12:48:17]

Committed patchset #11 (id:200001) as 181975