[Presentation API] Added the DefaultPresentationStart event and parsing of the default presentation…

[Presentation API] Added the DefaultPresentationStart event and necessary plumbing for the embedder to implement it.

BUG=459001
Blocks Chromium change: https://codereview.chromium.org/978543002

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

[whywhat, 2015-03-03 20:42:39]

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

[whywhat, 2015-03-03 20:42:40]

PTaL

[Peter Beverloo, 2015-03-03 21:19:25]

Can you please add more information to the CL description? This feature is not
described in the specification, and having some reference about why that's OK
(outside of the bug) would be very useful.

When people scrum recent commits in their git checkout, they often don't go to
the bugs associated with the changes.

https://codereview.chromium.org/974993002/diff/40001/Source/modules/modules.gypi
File Source/modules/modules.gypi (right):

https://codereview.chromium.org/974993002/diff/40001/Source/modules/modules.g...
Source/modules/modules.gypi:883:
'presentation/DefaultPresentationStartEvent.cpp',
.cpp before the .h please (alphabetized ordering)

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

https://codereview.chromium.org/974993002/diff/40001/Source/modules/presentat...
Source/modules/presentation/DefaultPresentationStartEvent.h:17: // see
https://code.google.com/p/chromium/issues/detail?id=459001 for details.
I don't think this comment is useful at all. Instead, could you have the comment
say that the event is used for? For example:

// Presentation API event to be fired when a presentation has been triggered
// by the embedder for the default presentation URL.

https://codereview.chromium.org/974993002/diff/40001/Source/modules/presentat...
Source/modules/presentation/DefaultPresentationStartEvent.h:36:
PresentationSession& session() { return *m_session; }
You probably want to gracefully handle m_session being NULL, given that not all
the constructors actually create one. (Return a pointer instead.)

Consider this JavaScript snippet:

var event = document.createEvent('defaultpresentationstart', { .. });
event.session; // boom

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

https://codereview.chromium.org/974993002/diff/40001/Source/modules/presentat...
Source/modules/presentation/DefaultPresentationStartEvent.idl:5: //
https://code.google.com/p/chromium/issues/detail?id=459001
I don't like a link to a bug rather than a spec link :-(.

Do you have an ETA for getting this added to the spec? Linking to the GitHub
issue would already be much better than this. And please be upfront about the
fact that this is a non-standard addition - this might become a concern in your
Intent to Ship.

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

https://codereview.chromium.org/974993002/diff/40001/Source/modules/presentat...
Source/modules/presentation/DefaultPresentationStartEventInit.idl:5: //
https://code.google.com/p/chromium/issues/detail?id=459001
dito

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

https://codereview.chromium.org/974993002/diff/40001/Source/modules/presentat...
Source/modules/presentation/Presentation.cpp:31: static PresentationEventInfo*
presentationEventInfos(unsigned& length)
nit: unsigned int? Not strictly required (yet), but closer to Chromium style.

https://codereview.chromium.org/974993002/diff/40001/Source/modules/presentat...
Source/modules/presentation/Presentation.cpp:34: {
EventTypeNames::availablechange,
&PresentationController::updateAvailableChangeWatched },
You're introducing a static initializer here.

PresentationEventInfo.eventType should be a const&.

https://codereview.chromium.org/974993002/diff/40001/Source/modules/presentat...
Source/modules/presentation/Presentation.cpp:35: {
EventTypeNames::defaultpresentationstart,
&PresentationController::updateDefaultPresentationStartWatched },
How many events do you expect to have in the end? It's hard to make a judgement
call about whether to prefer some duplicated code or this array structure
without that data. Perhaps there'd be an opportunity to generalize code in some
other way?

https://codereview.chromium.org/974993002/diff/40001/Source/modules/presentat...
Source/modules/presentation/Presentation.cpp:42: 
nit: double blank line

https://codereview.chromium.org/974993002/diff/40001/Source/modules/presentat...
Source/modules/presentation/Presentation.cpp:120: PresentationEventInfo*
eventInfos = presentationEventInfos(length);
nit: may as well pass a pointer to presentationEventInfo for readability. Not
strictly required by the styleguide.

https://codereview.chromium.org/974993002/diff/40001/public/platform/modules/...
File public/platform/modules/presentation/WebPresentationClient.h (right):

https://codereview.chromium.org/974993002/diff/40001/public/platform/modules/...
public/platform/modules/presentation/WebPresentationClient.h:37: virtual void
updateDefaultPresentationStartWatched(bool watched) {
BLINK_ASSERT_NOT_REACHED(); }
nit: add a FIXME to change this to be pure virtual?

[Peter Beverloo, 2015-03-03 21:33:02]

One further thought, what do you expect the following code to do?

=========================
presentation.addEventListener('defaultpresentationstart', function(event) {
  event.preventDefault();
});
=========================

I would expect the default presentation to be cancelled, in which case the
embedder might fall back on tab mirroring. This would be quite idiomatic for
JavaScript code.

Right now you define the event to not be cancelable, but that may not make
sense.

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

https://codereview.chromium.org/974993002/diff/40001/Source/modules/presentat...
Source/modules/presentation/DefaultPresentationStartEvent.cpp:19: :
Event(eventType, false, false)
Perhaps you want to clarify these argument values;

    : Event(eventType, false /* canBubble */, false /* cancelable */)

[Peter Beverloo, 2015-03-03 22:34:24]

Taking another step back -- given that this event does not require any form of
polling, the embedder shouldn't have to know about whether this particular event
exists at all. That means you can drop a large part of your plumbing.

Instead, just make it possible for the embedder to dispatch the
"defaultpresentationstart" event, and learn whether the event was canceled by
the developer or not. That's really all you need. If there's nothing listening
to the event, then nothing canceled it either :-).

[whywhat, 2015-03-05 18:41:47]

PTAL

Mark,Derek, are you fine with the embedder not knowing if the frame is listening
to the event or not?
Worst case the embedder starts the default presentation and the page doesn't
handle/cancel the event - it's the web developers fault.

We could notify the embedder if the event has been cancelled and fall back to
tab mirroring I guess although if the presentation is controlled entirely via
cloud for example, the website wouldn't want the embedder to do that.

https://codereview.chromium.org/974993002/diff/40001/Source/modules/modules.gypi
File Source/modules/modules.gypi (right):

https://codereview.chromium.org/974993002/diff/40001/Source/modules/modules.g...
Source/modules/modules.gypi:883:
'presentation/DefaultPresentationStartEvent.cpp',
On 2015/03/03 21:19:24, Peter Beverloo wrote:
> .cpp before the .h please (alphabetized ordering)

Done.

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

https://codereview.chromium.org/974993002/diff/40001/Source/modules/presentat...
Source/modules/presentation/DefaultPresentationStartEvent.cpp:19: :
Event(eventType, false, false)
On 2015/03/03 21:33:02, Peter Beverloo wrote:
> Perhaps you want to clarify these argument values;
> 
>     : Event(eventType, false /* canBubble */, false /* cancelable */)

Done.

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

https://codereview.chromium.org/974993002/diff/40001/Source/modules/presentat...
Source/modules/presentation/DefaultPresentationStartEvent.h:17: // see
https://code.google.com/p/chromium/issues/detail?id=459001 for details.
On 2015/03/03 21:19:24, Peter Beverloo wrote:
> I don't think this comment is useful at all. Instead, could you have the
comment
> say that the event is used for? For example:
> 
> // Presentation API event to be fired when a presentation has been triggered
> // by the embedder for the default presentation URL.

Done.

https://codereview.chromium.org/974993002/diff/40001/Source/modules/presentat...
Source/modules/presentation/DefaultPresentationStartEvent.h:36:
PresentationSession& session() { return *m_session; }
On 2015/03/03 21:19:24, Peter Beverloo wrote:
> You probably want to gracefully handle m_session being NULL, given that not
all
> the constructors actually create one. (Return a pointer instead.)
> 
> Consider this JavaScript snippet:
> 
> var event = document.createEvent('defaultpresentationstart', { .. });
> event.session; // boom

Done.

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

https://codereview.chromium.org/974993002/diff/40001/Source/modules/presentat...
Source/modules/presentation/DefaultPresentationStartEvent.idl:5: //
https://code.google.com/p/chromium/issues/detail?id=459001
On 2015/03/03 21:19:24, Peter Beverloo wrote:
> I don't like a link to a bug rather than a spec link :-(.
> 
> Do you have an ETA for getting this added to the spec? Linking to the GitHub
> issue would already be much better than this. And please be upfront about the
> fact that this is a non-standard addition - this might become a concern in
your
> Intent to Ship.

Done.

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

https://codereview.chromium.org/974993002/diff/40001/Source/modules/presentat...
Source/modules/presentation/DefaultPresentationStartEventInit.idl:5: //
https://code.google.com/p/chromium/issues/detail?id=459001
On 2015/03/03 21:19:24, Peter Beverloo wrote:
> dito

Done.

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

https://codereview.chromium.org/974993002/diff/40001/Source/modules/presentat...
Source/modules/presentation/Presentation.cpp:31: static PresentationEventInfo*
presentationEventInfos(unsigned& length)
On 2015/03/03 21:19:25, Peter Beverloo wrote:
> nit: unsigned int? Not strictly required (yet), but closer to Chromium style.

Removed

https://codereview.chromium.org/974993002/diff/40001/Source/modules/presentat...
Source/modules/presentation/Presentation.cpp:34: {
EventTypeNames::availablechange,
&PresentationController::updateAvailableChangeWatched },
On 2015/03/03 21:19:25, Peter Beverloo wrote:
> You're introducing a static initializer here.
> 
> PresentationEventInfo.eventType should be a const&.

Acknowledged.

https://codereview.chromium.org/974993002/diff/40001/Source/modules/presentat...
Source/modules/presentation/Presentation.cpp:35: {
EventTypeNames::defaultpresentationstart,
&PresentationController::updateDefaultPresentationStartWatched },
On 2015/03/03 21:19:25, Peter Beverloo wrote:
> How many events do you expect to have in the end? It's hard to make a
judgement
> call about whether to prefer some duplicated code or this array structure
> without that data. Perhaps there'd be an opportunity to generalize code in
some
> other way?

Acknowledged.

https://codereview.chromium.org/974993002/diff/40001/Source/modules/presentat...
Source/modules/presentation/Presentation.cpp:42: 
On 2015/03/03 21:19:25, Peter Beverloo wrote:
> nit: double blank line

Done.

https://codereview.chromium.org/974993002/diff/40001/Source/modules/presentat...
Source/modules/presentation/Presentation.cpp:120: PresentationEventInfo*
eventInfos = presentationEventInfos(length);
On 2015/03/03 21:19:24, Peter Beverloo wrote:
> nit: may as well pass a pointer to presentationEventInfo for readability. Not
> strictly required by the styleguide.

Acknowledged.

https://codereview.chromium.org/974993002/diff/40001/public/platform/modules/...
File public/platform/modules/presentation/WebPresentationClient.h (right):

https://codereview.chromium.org/974993002/diff/40001/public/platform/modules/...
public/platform/modules/presentation/WebPresentationClient.h:37: virtual void
updateDefaultPresentationStartWatched(bool watched) {
BLINK_ASSERT_NOT_REACHED(); }
On 2015/03/03 21:19:25, Peter Beverloo wrote:
> nit: add a FIXME to change this to be pure virtual?

Done.

[imcheng, 2015-03-05 19:26:49]

On 2015/03/05 18:41:47, whywhat wrote:
> PTAL
> 
> Mark,Derek, are you fine with the embedder not knowing if the frame is
listening
> to the event or not?
> Worst case the embedder starts the default presentation and the page doesn't
> handle/cancel the event - it's the web developers fault.
> 
> We could notify the embedder if the event has been cancelled and fall back to
> tab mirroring I guess although if the presentation is controlled entirely via
> cloud for example, the website wouldn't want the embedder to do that.
> 

Chatted offline. I think the embedder doesn't need to know about the event at
all.
Embedder can just always notify Blink when a default presentation starts.
Once we plumb setDefaultPresentationUrl through, embedder should know the
default
presentation URL for a given frame. When a session starts, the embedder should
then know which frame(s) to notify based on matching presentation URLs.

>
https://codereview.chromium.org/974993002/diff/40001/Source/modules/modules.gypi
> File Source/modules/modules.gypi (right):
> 
>
https://codereview.chromium.org/974993002/diff/40001/Source/modules/modules.g...
> Source/modules/modules.gypi:883:
> 'presentation/DefaultPresentationStartEvent.cpp',
> On 2015/03/03 21:19:24, Peter Beverloo wrote:
> > .cpp before the .h please (alphabetized ordering)
> 
> Done.
> 
>
https://codereview.chromium.org/974993002/diff/40001/Source/modules/presentat...
> File Source/modules/presentation/DefaultPresentationStartEvent.cpp (right):
> 
>
https://codereview.chromium.org/974993002/diff/40001/Source/modules/presentat...
> Source/modules/presentation/DefaultPresentationStartEvent.cpp:19: :
> Event(eventType, false, false)
> On 2015/03/03 21:33:02, Peter Beverloo wrote:
> > Perhaps you want to clarify these argument values;
> > 
> >     : Event(eventType, false /* canBubble */, false /* cancelable */)
> 
> Done.
> 
>
https://codereview.chromium.org/974993002/diff/40001/Source/modules/presentat...
> File Source/modules/presentation/DefaultPresentationStartEvent.h (right):
> 
>
https://codereview.chromium.org/974993002/diff/40001/Source/modules/presentat...
> Source/modules/presentation/DefaultPresentationStartEvent.h:17: // see
> https://code.google.com/p/chromium/issues/detail?id=459001 for details.
> On 2015/03/03 21:19:24, Peter Beverloo wrote:
> > I don't think this comment is useful at all. Instead, could you have the
> comment
> > say that the event is used for? For example:
> > 
> > // Presentation API event to be fired when a presentation has been triggered
> > // by the embedder for the default presentation URL.
> 
> Done.
> 
>
https://codereview.chromium.org/974993002/diff/40001/Source/modules/presentat...
> Source/modules/presentation/DefaultPresentationStartEvent.h:36:
> PresentationSession& session() { return *m_session; }
> On 2015/03/03 21:19:24, Peter Beverloo wrote:
> > You probably want to gracefully handle m_session being NULL, given that not
> all
> > the constructors actually create one. (Return a pointer instead.)
> > 
> > Consider this JavaScript snippet:
> > 
> > var event = document.createEvent('defaultpresentationstart', { .. });
> > event.session; // boom
> 
> Done.
> 
>
https://codereview.chromium.org/974993002/diff/40001/Source/modules/presentat...
> File Source/modules/presentation/DefaultPresentationStartEvent.idl (right):
> 
>
https://codereview.chromium.org/974993002/diff/40001/Source/modules/presentat...
> Source/modules/presentation/DefaultPresentationStartEvent.idl:5: //
> https://code.google.com/p/chromium/issues/detail?id=459001
> On 2015/03/03 21:19:24, Peter Beverloo wrote:
> > I don't like a link to a bug rather than a spec link :-(.
> > 
> > Do you have an ETA for getting this added to the spec? Linking to the GitHub
> > issue would already be much better than this. And please be upfront about
the
> > fact that this is a non-standard addition - this might become a concern in
> your
> > Intent to Ship.
> 
> Done.
> 
>
https://codereview.chromium.org/974993002/diff/40001/Source/modules/presentat...
> File Source/modules/presentation/DefaultPresentationStartEventInit.idl
(right):
> 
>
https://codereview.chromium.org/974993002/diff/40001/Source/modules/presentat...
> Source/modules/presentation/DefaultPresentationStartEventInit.idl:5: //
> https://code.google.com/p/chromium/issues/detail?id=459001
> On 2015/03/03 21:19:24, Peter Beverloo wrote:
> > dito
> 
> Done.
> 
>
https://codereview.chromium.org/974993002/diff/40001/Source/modules/presentat...
> File Source/modules/presentation/Presentation.cpp (right):
> 
>
https://codereview.chromium.org/974993002/diff/40001/Source/modules/presentat...
> Source/modules/presentation/Presentation.cpp:31: static PresentationEventInfo*
> presentationEventInfos(unsigned& length)
> On 2015/03/03 21:19:25, Peter Beverloo wrote:
> > nit: unsigned int? Not strictly required (yet), but closer to Chromium
style.
> 
> Removed
> 
>
https://codereview.chromium.org/974993002/diff/40001/Source/modules/presentat...
> Source/modules/presentation/Presentation.cpp:34: {
> EventTypeNames::availablechange,
> &PresentationController::updateAvailableChangeWatched },
> On 2015/03/03 21:19:25, Peter Beverloo wrote:
> > You're introducing a static initializer here.
> > 
> > PresentationEventInfo.eventType should be a const&.
> 
> Acknowledged.
> 
>
https://codereview.chromium.org/974993002/diff/40001/Source/modules/presentat...
> Source/modules/presentation/Presentation.cpp:35: {
> EventTypeNames::defaultpresentationstart,
> &PresentationController::updateDefaultPresentationStartWatched },
> On 2015/03/03 21:19:25, Peter Beverloo wrote:
> > How many events do you expect to have in the end? It's hard to make a
> judgement
> > call about whether to prefer some duplicated code or this array structure
> > without that data. Perhaps there'd be an opportunity to generalize code in
> some
> > other way?
> 
> Acknowledged.
> 
>
https://codereview.chromium.org/974993002/diff/40001/Source/modules/presentat...
> Source/modules/presentation/Presentation.cpp:42: 
> On 2015/03/03 21:19:25, Peter Beverloo wrote:
> > nit: double blank line
> 
> Done.
> 
>
https://codereview.chromium.org/974993002/diff/40001/Source/modules/presentat...
> Source/modules/presentation/Presentation.cpp:120: PresentationEventInfo*
> eventInfos = presentationEventInfos(length);
> On 2015/03/03 21:19:24, Peter Beverloo wrote:
> > nit: may as well pass a pointer to presentationEventInfo for readability.
Not
> > strictly required by the styleguide.
> 
> Acknowledged.
> 
>
https://codereview.chromium.org/974993002/diff/40001/public/platform/modules/...
> File public/platform/modules/presentation/WebPresentationClient.h (right):
> 
>
https://codereview.chromium.org/974993002/diff/40001/public/platform/modules/...
> public/platform/modules/presentation/WebPresentationClient.h:37: virtual void
> updateDefaultPresentationStartWatched(bool watched) {
> BLINK_ASSERT_NOT_REACHED(); }
> On 2015/03/03 21:19:25, Peter Beverloo wrote:
> > nit: add a FIXME to change this to be pure virtual?
> 
> Done.

[mark a. foltz, 2015-03-05 19:44:10]

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

[mark a. foltz, 2015-03-05 19:44:11]

FYI to discuss event naming for consistency with the rest of the spec.  You
drove this discussion in the WG so you get the final say.

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

https://codereview.chromium.org/974993002/diff/40001/Source/modules/presentat...
Source/modules/presentation/DefaultPresentationStartEvent.h:18: class
DefaultPresentationStartEvent final : public Event {
Why "DefaultPresentationStart?"  The purpose of the event is to convey to the
page a specific PresentationSession, and we say "startSession" instead of
"startPresentation."  So I would suggest "DefaultSessionStart" instead.

[whywhat, 2015-03-05 20:46:03]

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

https://codereview.chromium.org/974993002/diff/40001/Source/modules/presentat...
Source/modules/presentation/DefaultPresentationStartEvent.h:18: class
DefaultPresentationStartEvent final : public Event {
On 2015/03/05 19:44:11, mark a. foltz wrote:
> Why "DefaultPresentationStart?"  The purpose of the event is to convey to the
> page a specific PresentationSession, and we say "startSession" instead of
> "startPresentation."  So I would suggest "DefaultSessionStart" instead.
> 

Yes, I think using session is more consistent. However, neither default session
nor default presentation seem to be valid terms: in "Default Presentation URL"
the url is default, not the presentation/session. Since it designates an
automatic session launch by the browser, Maybe |autosessionstart|?

Probably late but I also realize that having sessions is more of an
implementation detail and a bit redundant in the spec so we could have chosen
simpler names like navigator.presentation.start/join/onautostart/etc

[whywhat, 2015-03-05 21:01:27]

On 2015/03/05 20:46:03, whywhat wrote:
>
https://codereview.chromium.org/974993002/diff/40001/Source/modules/presentat...
> File Source/modules/presentation/DefaultPresentationStartEvent.h (right):
> 
>
https://codereview.chromium.org/974993002/diff/40001/Source/modules/presentat...
> Source/modules/presentation/DefaultPresentationStartEvent.h:18: class
> DefaultPresentationStartEvent final : public Event {
> On 2015/03/05 19:44:11, mark a. foltz wrote:
> > Why "DefaultPresentationStart?"  The purpose of the event is to convey to
the
> > page a specific PresentationSession, and we say "startSession" instead of
> > "startPresentation."  So I would suggest "DefaultSessionStart" instead.
> > 
> 
> Yes, I think using session is more consistent. However, neither default
session
> nor default presentation seem to be valid terms: in "Default Presentation URL"
> the url is default, not the presentation/session. Since it designates an
> automatic session launch by the browser, Maybe |autosessionstart|?

I probably meant |sessionautostart|

> 
> Probably late but I also realize that having sessions is more of an
> implementation detail and a bit redundant in the spec so we could have chosen
> simpler names like navigator.presentation.start/join/onautostart/etc

[mark a. foltz, 2015-03-05 21:01:50]

On 2015/03/05 20:46:03, whywhat wrote:
>
https://codereview.chromium.org/974993002/diff/40001/Source/modules/presentat...
> File Source/modules/presentation/DefaultPresentationStartEvent.h (right):
> 
>
https://codereview.chromium.org/974993002/diff/40001/Source/modules/presentat...
> Source/modules/presentation/DefaultPresentationStartEvent.h:18: class
> DefaultPresentationStartEvent final : public Event {
> On 2015/03/05 19:44:11, mark a. foltz wrote:
> > Why "DefaultPresentationStart?"  The purpose of the event is to convey to
the
> > page a specific PresentationSession, and we say "startSession" instead of
> > "startPresentation."  So I would suggest "DefaultSessionStart" instead.
> > 
> 
> Yes, I think using session is more consistent. However, neither default
session
> nor default presentation seem to be valid terms: in "Default Presentation URL"
> the url is default, not the presentation/session. Since it designates an
> automatic session launch by the browser, Maybe |autosessionstart|?

But most circumstances will still require some user action to initiate, so not
sure "auto" conveys the right meaning.

> 
> Probably late but I also realize that having sessions is more of an
> implementation detail and a bit redundant in the spec so we could have chosen
> simpler names like navigator.presentation.start/join/onautostart/etc

I like that too, but renaming the entire spec is probably a conversation to have
in the WG or F2F.

m.

[mlamouri (slow - plz ping), 2015-03-06 22:43:41]

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

[mlamouri (slow - plz ping), 2015-03-06 22:43:42]

lgtm

https://codereview.chromium.org/974993002/diff/80001/Source/modules/presentat...
File Source/modules/presentation/DefaultPresentationStartEvent.h (right):

https://codereview.chromium.org/974993002/diff/80001/Source/modules/presentat...
Source/modules/presentation/DefaultPresentationStartEvent.h:9: #include
"modules/presentation/DefaultPresentationStartEventInit.h"
This can probably be a forward declaration.

https://codereview.chromium.org/974993002/diff/80001/Source/modules/presentat...
Source/modules/presentation/DefaultPresentationStartEvent.h:17: // See
https://code.google.com/p/chromium/issues/detail?id=459001 for details.
I don't think you should point to bugs unless it's an open issue. If someone
care, they will look at git blame to find out the bug associated to the change.

https://codereview.chromium.org/974993002/diff/80001/Source/modules/presentat...
File Source/modules/presentation/DefaultPresentationStartEvent.idl (right):

https://codereview.chromium.org/974993002/diff/80001/Source/modules/presentat...
Source/modules/presentation/DefaultPresentationStartEvent.idl:8: // Chromium
specific discussion might be found at
https://code.google.com/p/chromium/issues/detail?id=459001
See comment below.

https://codereview.chromium.org/974993002/diff/80001/Source/modules/presentat...
File Source/modules/presentation/DefaultPresentationStartEventInit.idl (right):

https://codereview.chromium.org/974993002/diff/80001/Source/modules/presentat...
Source/modules/presentation/DefaultPresentationStartEventInit.idl:8: // Chromium
specific discussion might be found at
https://code.google.com/p/chromium/issues/detail?id=459001
Hmm, I don't think I would add that. There are two outcomes:
- it's rejected, never added to the spec so you will remove it from Chromium
implementation;
- it's accepted, added to the spec, you will forget to update that comment.

https://codereview.chromium.org/974993002/diff/80001/Source/modules/presentat...
File Source/modules/presentation/PresentationController.cpp (right):

https://codereview.chromium.org/974993002/diff/80001/Source/modules/presentat...
Source/modules/presentation/PresentationController.cpp:85: PresentationSession*
session = PresentationSession::take(sessionClient, m_presentation);
I like how you are re-using the ::take() and ::dispose() methods that you
initially did not want to use ;)

https://codereview.chromium.org/974993002/diff/80001/public/platform/modules/...
File public/platform/modules/presentation/WebPresentationClient.h (right):

https://codereview.chromium.org/974993002/diff/80001/public/platform/modules/...
public/platform/modules/presentation/WebPresentationClient.h:9: #include
"public/platform/WebCommon.h"
You probably don't need to add that.

[Peter Beverloo, 2015-03-09 19:05:28]

lgtm

https://codereview.chromium.org/974993002/diff/80001/Source/modules/presentat...
File Source/modules/presentation/DefaultPresentationStartEvent.cpp (right):

https://codereview.chromium.org/974993002/diff/80001/Source/modules/presentat...
Source/modules/presentation/DefaultPresentationStartEvent.cpp:20: false, //
canBubble
nit: it's more common in Blink to have something like the following:

Event(eventType, false /* canBubble */, false /* cancelable */)

If you decide to keep this, please fix indentation.

https://codereview.chromium.org/974993002/diff/80001/Source/modules/presentat...
File Source/modules/presentation/DefaultPresentationStartEvent.h (right):

https://codereview.chromium.org/974993002/diff/80001/Source/modules/presentat...
Source/modules/presentation/DefaultPresentationStartEvent.h:17: // See
https://code.google.com/p/chromium/issues/detail?id=459001 for details.
On 2015/03/06 22:43:41, Mounir Lamouri wrote:
> I don't think you should point to bugs unless it's an open issue. If someone
> care, they will look at git blame to find out the bug associated to the
change.

+1, we should not point to bugs.

[whywhat, 2015-03-10 13:00:45]

https://codereview.chromium.org/974993002/diff/80001/Source/modules/presentat...
File Source/modules/presentation/DefaultPresentationStartEvent.cpp (right):

https://codereview.chromium.org/974993002/diff/80001/Source/modules/presentat...
Source/modules/presentation/DefaultPresentationStartEvent.cpp:20: false, //
canBubble
On 2015/03/09 19:05:28, Peter Beverloo wrote:
> nit: it's more common in Blink to have something like the following:
> 
> Event(eventType, false /* canBubble */, false /* cancelable */)

Yes, but  I believe this is harder to parse and stronger violates Chromium's two
spaces between code and comments law.

> 
> If you decide to keep this, please fix indentation.

By fixing do you mean indenting 4 more spaces?

https://codereview.chromium.org/974993002/diff/80001/Source/modules/presentat...
File Source/modules/presentation/DefaultPresentationStartEvent.h (right):

https://codereview.chromium.org/974993002/diff/80001/Source/modules/presentat...
Source/modules/presentation/DefaultPresentationStartEvent.h:9: #include
"modules/presentation/DefaultPresentationStartEventInit.h"
On 2015/03/06 22:43:41, Mounir Lamouri wrote:
> This can probably be a forward declaration.

Done.

https://codereview.chromium.org/974993002/diff/80001/Source/modules/presentat...
Source/modules/presentation/DefaultPresentationStartEvent.h:17: // See
https://code.google.com/p/chromium/issues/detail?id=459001 for details.
On 2015/03/06 22:43:41, Mounir Lamouri wrote:
> I don't think you should point to bugs unless it's an open issue. If someone
> care, they will look at git blame to find out the bug associated to the
change.

I believe there should be at least some pointer on where this event comes from.
The bug would be easier to update than the code.

https://codereview.chromium.org/974993002/diff/80001/Source/modules/presentat...
File Source/modules/presentation/DefaultPresentationStartEvent.idl (right):

https://codereview.chromium.org/974993002/diff/80001/Source/modules/presentat...
Source/modules/presentation/DefaultPresentationStartEvent.idl:8: // Chromium
specific discussion might be found at
https://code.google.com/p/chromium/issues/detail?id=459001
On 2015/03/06 22:43:41, Mounir Lamouri wrote:
> See comment below.

Hm? There's no below comment in this file :)

https://codereview.chromium.org/974993002/diff/80001/Source/modules/presentat...
File Source/modules/presentation/DefaultPresentationStartEventInit.idl (right):

https://codereview.chromium.org/974993002/diff/80001/Source/modules/presentat...
Source/modules/presentation/DefaultPresentationStartEventInit.idl:8: // Chromium
specific discussion might be found at
https://code.google.com/p/chromium/issues/detail?id=459001
On 2015/03/06 22:43:42, Mounir Lamouri wrote:
> Hmm, I don't think I would add that. There are two outcomes:
> - it's rejected, never added to the spec so you will remove it from Chromium
> implementation;
> - it's accepted, added to the spec, you will forget to update that comment.

Done.

https://codereview.chromium.org/974993002/diff/80001/Source/modules/presentat...
File Source/modules/presentation/PresentationController.cpp (right):

https://codereview.chromium.org/974993002/diff/80001/Source/modules/presentat...
Source/modules/presentation/PresentationController.cpp:85: PresentationSession*
session = PresentationSession::take(sessionClient, m_presentation);
On 2015/03/06 22:43:42, Mounir Lamouri wrote:
> I like how you are re-using the ::take() and ::dispose() methods that you
> initially did not want to use ;)

Well, once you've gotten used to the ownership transfer scheme, it becomes
handy.

https://codereview.chromium.org/974993002/diff/80001/public/platform/modules/...
File public/platform/modules/presentation/WebPresentationClient.h (right):

https://codereview.chromium.org/974993002/diff/80001/public/platform/modules/...
public/platform/modules/presentation/WebPresentationClient.h:9: #include
"public/platform/WebCommon.h"
On 2015/03/06 22:43:42, Mounir Lamouri wrote:
> You probably don't need to add that.

Done.

[Peter Beverloo, 2015-03-10 13:06:53]

https://codereview.chromium.org/974993002/diff/80001/Source/modules/presentat...
File Source/modules/presentation/DefaultPresentationStartEvent.cpp (right):

https://codereview.chromium.org/974993002/diff/80001/Source/modules/presentat...
Source/modules/presentation/DefaultPresentationStartEvent.cpp:20: false, //
canBubble
On 2015/03/10 13:00:44, whywhat wrote:
> On 2015/03/09 19:05:28, Peter Beverloo wrote:
> > nit: it's more common in Blink to have something like the following:
> > 
> > Event(eventType, false /* canBubble */, false /* cancelable */)
> 
> Yes, but  I believe this is harder to parse and stronger violates Chromium's
two
> spaces between code and comments law.
> 
> > 
> > If you decide to keep this, please fix indentation.
> 
> By fixing do you mean indenting 4 more spaces?
> 

https://code.google.com/p/chromium/codesearch#search/&q=%22*/,%22%20-f:usr&sq...

It's rather common actually, and I would prefer you to use that. Blink uses this
as well as Chromium does.

Indentation is off by any style guide. Either use the inline style mentioned
above, align with "(" or indent with four spaces as a continuation.

[whywhat, 2015-03-10 15:18:51]

https://codereview.chromium.org/974993002/diff/80001/Source/modules/presentat...
File Source/modules/presentation/DefaultPresentationStartEvent.cpp (right):

https://codereview.chromium.org/974993002/diff/80001/Source/modules/presentat...
Source/modules/presentation/DefaultPresentationStartEvent.cpp:20: false, //
canBubble
On 2015/03/10 13:06:53, Peter Beverloo wrote:
> On 2015/03/10 13:00:44, whywhat wrote:
> > On 2015/03/09 19:05:28, Peter Beverloo wrote:
> > > nit: it's more common in Blink to have something like the following:
> > > 
> > > Event(eventType, false /* canBubble */, false /* cancelable */)
> > 
> > Yes, but  I believe this is harder to parse and stronger violates Chromium's
> two
> > spaces between code and comments law.
> > 
> > > 
> > > If you decide to keep this, please fix indentation.
> > 
> > By fixing do you mean indenting 4 more spaces?
> > 
> 
>
https://code.google.com/p/chromium/codesearch#search/&q=%22*/,%22%20-f:usr&sq...
> 
> It's rather common actually, and I would prefer you to use that. Blink uses
this
> as well as Chromium does.
> 
> Indentation is off by any style guide. Either use the inline style mentioned
> above, align with "(" or indent with four spaces as a continuation.

Done.

[whywhat, 2015-03-10 15:20:10]

The CQ bit was checked by avayvod@chromium.org

[whywhat, 2015-03-10 15:20:10]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org,
mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/974993002/#ps120001
(title: "Fixed last reviewers wishes")

[commit-bot: I haz the power, 2015-03-10 15:22:06]

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

[commit-bot: I haz the power, 2015-03-10 15:39:55]

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

[commit-bot: I haz the power, 2015-03-10 15:39:56]

Try jobs failed on following builders:
  blink_presubmit on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/2...)

[jochen (gone - plz use gerrit), 2015-03-10 15:58:21]

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

[jochen (gone - plz use gerrit), 2015-03-10 15:58:21]

EventNames.in lgtm

[whywhat, 2015-03-10 15:59:43]

The CQ bit was checked by avayvod@chromium.org

[commit-bot: I haz the power, 2015-03-10 16:01:04]

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

[commit-bot: I haz the power, 2015-03-10 17:38:39]

Committed patchset #7 (id:120001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=191643