[Presentation API] make PresentationReceiver API consistent with latest spec

make PresentationReceiver API consistent with latest spec

https://w3c.github.io/presentation-api/#idl-def-presentationreceiver

Note: this API was never implemented and has no known
functional usages, so this is a no-op as far as Web compatibility is concerned;
we should send an intent to ship when the implementation is further along.

BUG=525660
Committed: https://crrev.com/df37acd456aec9ed78d809c5adeab0f8874a9c9d
Cr-Commit-Position: refs/heads/master@{#415841}

[zhaobin, 2016-08-22 21:51:34]

zhaobin@chromium.org changed reviewers:
+ imcheng@chromium.org, mfoltz@chromium.org, mlamouri@chromium.org

[zhaobin, 2016-08-22 21:51:34]

mfoltz@chromium.org: Please review changes in 

imcheng@chromium.org: Please review changes in 

mlamouri@chromium.org: Please review changes in

[haraken, 2016-08-23 00:03:47]

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

[haraken, 2016-08-23 00:03:48]

https://codereview.chromium.org/2265363002/diff/1/third_party/WebKit/Source/m...
File
third_party/WebKit/Source/modules/presentation/PresentationConnectionList.cpp
(right):

https://codereview.chromium.org/2265363002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/presentation/PresentationConnectionList.cpp:1:
// Copyright 2015 The Chromium Authors. All rights reserved.

2016

https://codereview.chromium.org/2265363002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/presentation/PresentationConnectionList.cpp:29:
return nullptr;

We shouldn't return nullptr from getExecutionContext().

https://codereview.chromium.org/2265363002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/presentation/PresentationConnectionList.h
(right):

https://codereview.chromium.org/2265363002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/presentation/PresentationConnectionList.h:1:
// Copyright 2015 The Chromium Authors. All rights reserved.

2016

https://codereview.chromium.org/2265363002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/presentation/PresentationConnectionList.h:8:
#include "bindings/core/v8/ScriptPromise.h"

It's unused.

https://codereview.chromium.org/2265363002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/presentation/PresentationConnectionList.h:22:
, public DOMWindowProperty {

Why do you need to inherit from DOMWindowProperty?

https://codereview.chromium.org/2265363002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/presentation/PresentationConnectionList.h:26:
PresentationConnectionList(LocalFrame*);

Add explicit.

https://codereview.chromium.org/2265363002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/presentation/PresentationReceiver.h
(right):

https://codereview.chromium.org/2265363002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/presentation/PresentationReceiver.h:25:
PresentationReceiver(LocalFrame*);

Add explicit.

[zhaobin, 2016-08-23 00:45:52]

https://codereview.chromium.org/2265363002/diff/1/third_party/WebKit/Source/m...
File
third_party/WebKit/Source/modules/presentation/PresentationConnectionList.cpp
(right):

https://codereview.chromium.org/2265363002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/presentation/PresentationConnectionList.cpp:1:
// Copyright 2015 The Chromium Authors. All rights reserved.
On 2016/08/23 00:03:48, haraken wrote:
> 
> 2016

Done.

https://codereview.chromium.org/2265363002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/presentation/PresentationConnectionList.cpp:1:
// Copyright 2015 The Chromium Authors. All rights reserved.
On 2016/08/23 00:03:48, haraken wrote:
> 
> 2016

Done.

https://codereview.chromium.org/2265363002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/presentation/PresentationConnectionList.cpp:29:
return nullptr;
On 2016/08/23 00:03:48, haraken wrote:
> 
> We shouldn't return nullptr from getExecutionContext().

Done.

https://codereview.chromium.org/2265363002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/presentation/PresentationConnectionList.h
(right):

https://codereview.chromium.org/2265363002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/presentation/PresentationConnectionList.h:1:
// Copyright 2015 The Chromium Authors. All rights reserved.
On 2016/08/23 00:03:48, haraken wrote:
> 
> 2016

Done.

https://codereview.chromium.org/2265363002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/presentation/PresentationConnectionList.h:8:
#include "bindings/core/v8/ScriptPromise.h"
On 2016/08/23 00:03:48, haraken wrote:
> 
> It's unused.

Done.

https://codereview.chromium.org/2265363002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/presentation/PresentationConnectionList.h:22:
, public DOMWindowProperty {
On 2016/08/23 00:03:48, haraken wrote:
> 
> Why do you need to inherit from DOMWindowProperty?

Done.

https://codereview.chromium.org/2265363002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/presentation/PresentationConnectionList.h:26:
PresentationConnectionList(LocalFrame*);
On 2016/08/23 00:03:48, haraken wrote:
> 
> Add explicit.

Done.

https://codereview.chromium.org/2265363002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/modules.gypi (right):

https://codereview.chromium.org/2265363002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/modules.gypi:1999:
'webusb/USBOutTransferResult.h',
Did not change this file. The diff comes from a git pull & merge. Sorry for the
inconvenience.

[haraken, 2016-08-23 02:19:33]

https://codereview.chromium.org/2265363002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/presentation/PresentationConnectionList.h
(right):

https://codereview.chromium.org/2265363002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/presentation/PresentationConnectionList.h:9:
#include "core/frame/DOMWindowProperty.h"

It's unused.

https://codereview.chromium.org/2265363002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/presentation/PresentationConnectionList.h:37:
Member<LocalFrame> m_frame;

There's not reason you need to keep a pointer to LocalFrame. It would be better
to make PresentationConnectionList inherit from ContextLifecycleObserver.

[mlamouri (slow - plz ping), 2016-08-23 13:04:50]

https://codereview.chromium.org/2265363002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/presentation/presentation-receiver.html
(right):

https://codereview.chromium.org/2265363002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/presentation/presentation-receiver.html:15:
assert_equals(typeof(navigator.presentation.receiver.connectionList), "object");
Shouldn't you test the connectionList object?

https://codereview.chromium.org/2265363002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/presentation/PresentationConnectionList.h
(right):

https://codereview.chromium.org/2265363002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/presentation/PresentationConnectionList.h:20:
: public EventTargetWithInlineData {
style: keep in the same line?

https://codereview.chromium.org/2265363002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/presentation/PresentationConnectionList.idl
(right):

https://codereview.chromium.org/2265363002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/presentation/PresentationConnectionList.idl:5:
// https://w3c.github.io/presentation-api/#interface-presentation
https://w3c.github.io/presentation-api/#interface-presentationconnectionlist

[mark a. foltz, 2016-08-23 19:08:49]

Nits:

- Please update the issue description with [Presentation API] for consistency
with other patches.

- You might add a link to the spec in the issue description for ease of
reference.

- I would add a note that this API was never implemented and has no known
functional usages, so this is a no-op as far as Web compatibility is concerned;
we should send an intent to ship when the implementation is further along.

mlamouri@: A couple of comments you may want to weigh in on relating to the
design patterns in this module.

https://codereview.chromium.org/2265363002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/presentation/PresentationConnectionList.cpp
(right):

https://codereview.chromium.org/2265363002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/presentation/PresentationConnectionList.cpp:29:
return m_frame ? m_frame->document() : nullptr;
I note that PresentationConnection also allows a nullptr ExecutionContext to be
returned, but that would seem to trigger crashes when the connection is
manipulated.

https://codereview.chromium.org/2265363002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/presentation/PresentationConnectionList.h
(right):

https://codereview.chromium.org/2265363002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/presentation/PresentationConnectionList.h:19:
class PresentationConnectionList final
mlamouri@: PresentationConnection, PresentationReceiver and
PresentationController inherit from DOMWindowProperty.  Should this object do
the same?

[zhaobin, 2016-08-23 20:40:49]

Description was changed from

==========
make PresentationReceiver API consistent with latest spec

BUG=525660
==========

to

==========
make PresentationReceiver API consistent with latest spec

https://w3c.github.io/presentation-api/#idl-def-presentationreceiver

Note: this API was never implemented and has no known
functional usages, so this is a no-op as far as Web compatibility is concerned;
we should send an intent to ship when the implementation is further along.

BUG=525660
==========

[zhaobin, 2016-08-23 20:43:54]

https://codereview.chromium.org/2265363002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/presentation/presentation-receiver.html
(right):

https://codereview.chromium.org/2265363002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/presentation/presentation-receiver.html:15:
assert_equals(typeof(navigator.presentation.receiver.connectionList), "object");
On 2016/08/23 13:04:50, mlamouri (slow) wrote:
> Shouldn't you test the connectionList object?

Yes. connectionList is a promise and hasn't been implemented yet. Plan to add
tests for PresentationConnectionList when we have an implementation. (Not sure
how to add test for connectionList in current state, as we cannot new
PresentationConnectionList object)

https://codereview.chromium.org/2265363002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/presentation/PresentationConnectionList.h
(right):

https://codereview.chromium.org/2265363002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/presentation/PresentationConnectionList.h:9:
#include "core/frame/DOMWindowProperty.h"
On 2016/08/23 02:19:33, haraken wrote:
> 
> It's unused.

Done.

https://codereview.chromium.org/2265363002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/presentation/PresentationConnectionList.h:20:
: public EventTargetWithInlineData {
On 2016/08/23 13:04:50, mlamouri (slow) wrote:
> style: keep in the same line?

Added a new base class, keep them in seperate lines?

https://codereview.chromium.org/2265363002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/presentation/PresentationConnectionList.h:37:
Member<LocalFrame> m_frame;
On 2016/08/23 02:19:33, haraken wrote:
> 
> There's not reason you need to keep a pointer to LocalFrame. It would be
better
> to make PresentationConnectionList inherit from ContextLifecycleObserver.

Done.

https://codereview.chromium.org/2265363002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/presentation/PresentationConnectionList.idl
(right):

https://codereview.chromium.org/2265363002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/presentation/PresentationConnectionList.idl:5:
// https://w3c.github.io/presentation-api/#interface-presentation
On 2016/08/23 13:04:50, mlamouri (slow) wrote:
> https://w3c.github.io/presentation-api/#interface-presentationconnectionlist

Done.

[haraken, 2016-08-24 01:09:33]

LGTM

https://codereview.chromium.org/2265363002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/presentation/PresentationConnectionList.cpp
(right):

https://codereview.chromium.org/2265363002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/presentation/PresentationConnectionList.cpp:13:
#include "modules/EventTargetModules.h"

Most of these #includes are unused.

https://codereview.chromium.org/2265363002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/presentation/PresentationConnectionList.h
(right):

https://codereview.chromium.org/2265363002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/presentation/PresentationConnectionList.h:25:
explicit PresentationConnectionList(LocalFrame*);

Would you pass in ExecutionContext* instead of LocalFrame*? It looks strange
that an object that inherits from ContextLifecycleObserver takes a LocalFrame*
parameter.

[mlamouri (slow - plz ping), 2016-08-24 16:48:59]

https://codereview.chromium.org/2265363002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/presentation/PresentationConnectionList.h
(right):

https://codereview.chromium.org/2265363002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/presentation/PresentationConnectionList.h:21:
, public ContextLifecycleObserver {
I think DOMWindowProperty would make sense.

https://codereview.chromium.org/2265363002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/presentation/PresentationReceiver.cpp
(right):

https://codereview.chromium.org/2265363002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/presentation/PresentationReceiver.cpp:13:
#include "modules/EventTargetModules.h"
You might want to clean up the list of includes.

https://codereview.chromium.org/2265363002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/presentation/PresentationReceiver.idl
(right):

https://codereview.chromium.org/2265363002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/presentation/PresentationReceiver.idl:7: // we
should send an intent to ship when the implementation is further along.
I don't think you need this comment.

[zhaobin, 2016-08-24 17:34:12]

https://codereview.chromium.org/2265363002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/presentation/PresentationConnectionList.cpp
(right):

https://codereview.chromium.org/2265363002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/presentation/PresentationConnectionList.cpp:13:
#include "modules/EventTargetModules.h"
On 2016/08/24 01:09:33, haraken wrote:
> 
> Most of these #includes are unused.

Done.

https://codereview.chromium.org/2265363002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/presentation/PresentationConnectionList.h
(right):

https://codereview.chromium.org/2265363002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/presentation/PresentationConnectionList.h:21:
, public ContextLifecycleObserver {
On 2016/08/24 16:48:59, mlamouri (slow) wrote:
> I think DOMWindowProperty would make sense.

haraken suggests that it is not necessary to inherit from DOMWindowProperty
during iteration 1.

I am fine with either. Leave it as it is and change it later during
implementation if necessary?

https://codereview.chromium.org/2265363002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/presentation/PresentationConnectionList.h:25:
explicit PresentationConnectionList(LocalFrame*);
On 2016/08/24 01:09:33, haraken wrote:
> 
> Would you pass in ExecutionContext* instead of LocalFrame*? It looks strange
> that an object that inherits from ContextLifecycleObserver takes a LocalFrame*
> parameter.

Done.

https://codereview.chromium.org/2265363002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/presentation/PresentationReceiver.cpp
(right):

https://codereview.chromium.org/2265363002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/presentation/PresentationReceiver.cpp:13:
#include "modules/EventTargetModules.h"
On 2016/08/24 16:48:59, mlamouri (slow) wrote:
> You might want to clean up the list of includes.

Done.

https://codereview.chromium.org/2265363002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/presentation/PresentationReceiver.idl
(right):

https://codereview.chromium.org/2265363002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/presentation/PresentationReceiver.idl:7: // we
should send an intent to ship when the implementation is further along.
On 2016/08/24 16:48:59, mlamouri (slow) wrote:
> I don't think you need this comment.

Done.

[mark a. foltz, 2016-08-25 18:06:34]

lgtm

[mlamouri (slow - plz ping), 2016-08-26 17:05:32]

lgtm

[zhaobin, 2016-08-26 17:53:51]

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

[commit-bot: I haz the power, 2016-08-26 17:54:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-26 19:45:55]

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

[commit-bot: I haz the power, 2016-08-26 19:45:56]

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

[zhaobin, 2016-08-29 22:41:14]

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

[commit-bot: I haz the power, 2016-08-29 23:19:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-30 03:25:01]

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

[commit-bot: I haz the power, 2016-08-30 03:25:02]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[zhaobin, 2016-08-30 17:55:28]

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

[commit-bot: I haz the power, 2016-08-30 17:56:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-30 21:37:51]

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

[commit-bot: I haz the power, 2016-08-30 21:37:52]

Dry run: This issue passed the CQ dry run.

[zhaobin, 2016-08-30 21:59:05]

Fix layout test failures

[zhaobin, 2016-08-31 22:31:54]

The CQ bit was checked by zhaobin@chromium.org

[zhaobin, 2016-08-31 22:31:55]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, mfoltz@chromium.org, mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/2265363002/#ps100001
(title: "fix layout test global-interface-listing.html")

[commit-bot: I haz the power, 2016-08-31 22:33:25]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-01 01:45:31]

Description was changed from

==========
make PresentationReceiver API consistent with latest spec

https://w3c.github.io/presentation-api/#idl-def-presentationreceiver

Note: this API was never implemented and has no known
functional usages, so this is a no-op as far as Web compatibility is concerned;
we should send an intent to ship when the implementation is further along.

BUG=525660
==========

to

==========
make PresentationReceiver API consistent with latest spec

https://w3c.github.io/presentation-api/#idl-def-presentationreceiver

Note: this API was never implemented and has no known
functional usages, so this is a no-op as far as Web compatibility is concerned;
we should send an intent to ship when the implementation is further along.

BUG=525660
==========

[commit-bot: I haz the power, 2016-09-01 01:45:32]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-09-01 01:48:20]

Description was changed from

==========
make PresentationReceiver API consistent with latest spec

https://w3c.github.io/presentation-api/#idl-def-presentationreceiver

Note: this API was never implemented and has no known
functional usages, so this is a no-op as far as Web compatibility is concerned;
we should send an intent to ship when the implementation is further along.

BUG=525660
==========

to

==========
make PresentationReceiver API consistent with latest spec

https://w3c.github.io/presentation-api/#idl-def-presentationreceiver

Note: this API was never implemented and has no known
functional usages, so this is a no-op as far as Web compatibility is concerned;
we should send an intent to ship when the implementation is further along.

BUG=525660

Committed: https://crrev.com/df37acd456aec9ed78d809c5adeab0f8874a9c9d
Cr-Commit-Position: refs/heads/master@{#415841}
==========

[commit-bot: I haz the power, 2016-09-01 01:48:22]

Patchset 6 (id:??) landed as
https://crrev.com/df37acd456aec9ed78d809c5adeab0f8874a9c9d
Cr-Commit-Position: refs/heads/master@{#415841}