[Presentation API] 1-UA: notify receiver page when receiver connection becomes available (blink sid…

[Presentation API] 1-UA: notify receiver page when receiver connection becomes available (blink side)

- Add WebPresentationReceiver class
- Add OnReceiverConnectionAvailable() to PresentationDispatch class (to be invoked by PresentationServiceImpl in following CL)
- Rename PresentationReceiver::onConnectionReceived() to onReceiverConnectionAvailable()

This CL is splited from Issue 2324233002. It only contains blink side changes.

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

[zhaobin, 2016-09-14 02:44:57]

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

[zhaobin, 2016-09-14 18:00:28]

Description was changed from

==========
[Presentation API] 1-UA: notify receiver page when receiver connection becomes
available (blink side)

- Add WebPresentationReceiver class
- Add OnReceiverConnectionAvailable() to PresentationDispatch class (to be
invoked by PresentationServiceImpl in following CL)
- Rename PresentationReceiver::onConnectionReceived() to
onReceiverConnectionAvailable()

This CL is splited from Issue 2324233002. It only contains blink side changes.

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

to

==========
[Presentation API] 1-UA: notify receiver page when receiver connection becomes
available (blink side)

- Add WebPresentationReceiver class
- Add OnReceiverConnectionAvailable() to PresentationDispatch class (to be
invoked by PresentationServiceImpl in following CL)
- Rename PresentationReceiver::onConnectionReceived() to
onReceiverConnectionAvailable()

This CL is splited from Issue 2324233002. It only contains blink side changes.

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

[zhaobin, 2016-09-14 18:00:28]

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

[mlamouri (slow - plz ping), 2016-09-15 10:49:39]

lgtm

https://codereview.chromium.org/2340433003/diff/20001/content/renderer/presen...
File content/renderer/presentation/presentation_dispatcher.cc (right):

https://codereview.chromium.org/2340433003/diff/20001/content/renderer/presen...
content/renderer/presentation/presentation_dispatcher.cc:409: new
PresentationConnectionClient(std::move(session_info)));
style: add {} when the statement is wrapped in more than one line.

https://codereview.chromium.org/2340433003/diff/20001/content/renderer/presen...
File content/renderer/presentation/presentation_dispatcher.h (right):

https://codereview.chromium.org/2340433003/diff/20001/content/renderer/presen...
content/renderer/presentation/presentation_dispatcher.h:140:
blink::WebPresentationReceiver* m_receiver;
style: `receiver_`

[mark a. foltz, 2016-09-15 16:53:19]

https://codereview.chromium.org/2340433003/diff/20001/content/renderer/presen...
File content/renderer/presentation/presentation_dispatcher.cc (right):

https://codereview.chromium.org/2340433003/diff/20001/content/renderer/presen...
content/renderer/presentation/presentation_dispatcher.cc:407: if (m_receiver)
What if this is called before m_receiver is set?  I believe you'll need to queue
up connections in that case.

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

https://codereview.chromium.org/2340433003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/presentation/Presentation.cpp:60: auto client
= controller ? controller->client() : nullptr;
It looks like this can only be nullptr if Presentation API is disabled as a
runtime feature.

It would clean up a lot of these extra checks if we could assume it was always
enabled.   Can we assume that now Mounir?

https://codereview.chromium.org/2340433003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/presentation/Presentation.cpp:63: // for that.
We need to actually do this to match spec; navigator.presentation.receiver must
be null in a non-receiving browsing context.

http://w3c.github.io/presentation-api/#receiving-user-agent

Can you replace TODO(mlamouri) here with TODO(crbug.com/647296) here?

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

https://codereview.chromium.org/2340433003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/presentation/PresentationReceiverTest.cpp:77:
auto receiver = new PresentationReceiver(&scope.frame(), nullptr);
Please add a test case with a mock WebPresentationClient that checks that
receiver->setClient is called on the first call to receiver().

https://codereview.chromium.org/2340433003/diff/20001/third_party/WebKit/publ...
File
third_party/WebKit/public/platform/modules/presentation/WebPresentationReceiver.h
(right):

https://codereview.chromium.org/2340433003/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/presentation/WebPresentationReceiver.h:19:
// Called when receiver page gets an incoming connection
Nit: terminate with period.

https://codereview.chromium.org/2340433003/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/modules/presentation/presentation.mojom
(right):

https://codereview.chromium.org/2340433003/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/presentation/presentation.mojom:144:
OnReceiverConnectionAvailable(PresentationSessionInfo sessionInfo);
s/sessionInfo/connection/

We should rename the rest of this API to use "Connection" instead of "Session"
consistently (not in this patch though).

At the top of this file, can you add:

TODO(crbug.com/647290): Rename "Session" to "Connection"

[zhaobin, 2016-09-15 18:56:03]

Thanks a lot for the comments!

Mounir, could you please comment on Mark's question: shall we always resume that
Presentation API is enabled (presentation controller is not null)?

Thanks,
Bin

https://codereview.chromium.org/2340433003/diff/20001/content/renderer/presen...
File content/renderer/presentation/presentation_dispatcher.cc (right):

https://codereview.chromium.org/2340433003/diff/20001/content/renderer/presen...
content/renderer/presentation/presentation_dispatcher.cc:407: if (m_receiver)
On 2016/09/15 16:53:18, mark a. foltz wrote:
> What if this is called before m_receiver is set?  I believe you'll need to
queue
> up connections in that case.

Yes. Going to send out another CL with queuing logic.

https://codereview.chromium.org/2340433003/diff/20001/content/renderer/presen...
content/renderer/presentation/presentation_dispatcher.cc:409: new
PresentationConnectionClient(std::move(session_info)));
On 2016/09/15 10:49:39, mlamouri wrote:
> style: add {} when the statement is wrapped in more than one line.

Done.

https://codereview.chromium.org/2340433003/diff/20001/content/renderer/presen...
File content/renderer/presentation/presentation_dispatcher.h (right):

https://codereview.chromium.org/2340433003/diff/20001/content/renderer/presen...
content/renderer/presentation/presentation_dispatcher.h:140:
blink::WebPresentationReceiver* m_receiver;
On 2016/09/15 10:49:39, mlamouri wrote:
> style: `receiver_`

Done.

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

https://codereview.chromium.org/2340433003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/presentation/Presentation.cpp:60: auto client
= controller ? controller->client() : nullptr;
On 2016/09/15 16:53:18, mark a. foltz wrote:
> It looks like this can only be nullptr if Presentation API is disabled as a
> runtime feature.
> 
> It would clean up a lot of these extra checks if we could assume it was always
> enabled.   Can we assume that now Mounir?
> 

Acknowledged.

https://codereview.chromium.org/2340433003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/presentation/Presentation.cpp:63: // for that.
On 2016/09/15 16:53:18, mark a. foltz wrote:
> We need to actually do this to match spec; navigator.presentation.receiver
must
> be null in a non-receiving browsing context.
> 
> http://w3c.github.io/presentation-api/#receiving-user-agent
> 
> Can you replace TODO(mlamouri) here with TODO(crbug.com/647296) here?

Done.

https://codereview.chromium.org/2340433003/diff/20001/third_party/WebKit/publ...
File
third_party/WebKit/public/platform/modules/presentation/WebPresentationReceiver.h
(right):

https://codereview.chromium.org/2340433003/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/presentation/WebPresentationReceiver.h:19:
// Called when receiver page gets an incoming connection
On 2016/09/15 16:53:18, mark a. foltz wrote:
> Nit: terminate with period.

Done.

https://codereview.chromium.org/2340433003/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/modules/presentation/presentation.mojom
(right):

https://codereview.chromium.org/2340433003/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/presentation/presentation.mojom:144:
OnReceiverConnectionAvailable(PresentationSessionInfo sessionInfo);
On 2016/09/15 16:53:19, mark a. foltz wrote:
> s/sessionInfo/connection/
> 
> We should rename the rest of this API to use "Connection" instead of "Session"
> consistently (not in this patch though).
> 
> At the top of this file, can you add:
> 
> TODO(crbug.com/647290): Rename "Session" to "Connection"

Done.

[mark a. foltz, 2016-09-21 12:53:36]

lgtm

[zhaobin, 2016-09-21 18:29:51]

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

[commit-bot: I haz the power, 2016-09-21 18:30:19]

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-09-21 18:34:15]

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

[commit-bot: I haz the power, 2016-09-21 18:34:16]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  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-09-21 19:26:43]

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

[commit-bot: I haz the power, 2016-09-21 19:27:23]

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-09-21 19:40:24]

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

[commit-bot: I haz the power, 2016-09-21 19:40:24]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[zhaobin, 2016-09-21 20:22:24]

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

[commit-bot: I haz the power, 2016-09-21 20:22:53]

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-09-21 21:32:41]

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

[commit-bot: I haz the power, 2016-09-21 21:32:42]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[zhaobin, 2016-09-22 00:28:08]

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

[commit-bot: I haz the power, 2016-09-22 00:28:40]

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-09-22 02:24:43]

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

[commit-bot: I haz the power, 2016-09-22 02:24:44]

Dry run: This issue passed the CQ dry run.

[zhaobin, 2016-09-22 17:02:56]

The CQ bit was checked by zhaobin@chromium.org

[zhaobin, 2016-09-22 17:02:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
mlamouri@chromium.org, mfoltz@chromium.org
Link to the patchset: https://codereview.chromium.org/2340433003/#ps100001
(title: "fix windows build errors")

[commit-bot: I haz the power, 2016-09-22 17:03:20]

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-22 17:12:26]

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

[commit-bot: I haz the power, 2016-09-22 17:12:27]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[zhaobin, 2016-09-22 17:51:28]

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

[zhaobin, 2016-09-23 20:01:00]

Adding reviewers for following files:

third_party/WebKit/Source/platform/BUILD.gn
third_party/WebKit/Source/platform/exported/WebPresentationReceiver.cpp
third_party/WebKit/public/platform/modules/presentation/presentation.mojom

[zhaobin, 2016-09-28 17:33:11]

Hi Daniel,

  Could you please take a look at the presentation.mojom change when you have
time? Thank you very much!

Bin

[dcheng, 2016-09-28 23:55:13]

The mojo change itself LGTM, but I hope we can better rationalize when things
are null and when they aren't. There's an awful lot of null checks here, and it
would be nice to simplify them.

https://codereview.chromium.org/2340433003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/presentation/Presentation.cpp (right):

https://codereview.chromium.org/2340433003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/presentation/Presentation.cpp:60: auto client
= controller ? controller->client() : nullptr;
Nit: auto*

[zhaobin, 2016-09-29 18:38:53]

Created crbug/651524 to track cleanup task for null checks.

https://codereview.chromium.org/2340433003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/presentation/Presentation.cpp (right):

https://codereview.chromium.org/2340433003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/presentation/Presentation.cpp:60: auto client
= controller ? controller->client() : nullptr;
On 2016/09/28 23:55:13, dcheng wrote:
> Nit: auto*

Done.

[zhaobin, 2016-09-29 18:54:36]

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

[commit-bot: I haz the power, 2016-09-29 18:55:10]

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-09-29 20:32:41]

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

[commit-bot: I haz the power, 2016-09-29 20:32:42]

Dry run: This issue passed the CQ dry run.

[haraken, 2016-09-29 23:59:23]

platform/ LGTM

[zhaobin, 2016-09-30 00:00:43]

The CQ bit was checked by zhaobin@chromium.org

[zhaobin, 2016-09-30 00:00:44]

The patchset sent to the CQ was uploaded after l-g-t-m from mfoltz@chromium.org,
mlamouri@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2340433003/#ps140001
(title: "resolve code review comments from dcheng")

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

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-30 00:11:14]

Description was changed from

==========
[Presentation API] 1-UA: notify receiver page when receiver connection becomes
available (blink side)

- Add WebPresentationReceiver class
- Add OnReceiverConnectionAvailable() to PresentationDispatch class (to be
invoked by PresentationServiceImpl in following CL)
- Rename PresentationReceiver::onConnectionReceived() to
onReceiverConnectionAvailable()

This CL is splited from Issue 2324233002. It only contains blink side changes.

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

to

==========
[Presentation API] 1-UA: notify receiver page when receiver connection becomes
available (blink side)

- Add WebPresentationReceiver class
- Add OnReceiverConnectionAvailable() to PresentationDispatch class (to be
invoked by PresentationServiceImpl in following CL)
- Rename PresentationReceiver::onConnectionReceived() to
onReceiverConnectionAvailable()

This CL is splited from Issue 2324233002. It only contains blink side changes.

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

[commit-bot: I haz the power, 2016-09-30 00:11:15]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-09-30 00:15:37]

Description was changed from

==========
[Presentation API] 1-UA: notify receiver page when receiver connection becomes
available (blink side)

- Add WebPresentationReceiver class
- Add OnReceiverConnectionAvailable() to PresentationDispatch class (to be
invoked by PresentationServiceImpl in following CL)
- Rename PresentationReceiver::onConnectionReceived() to
onReceiverConnectionAvailable()

This CL is splited from Issue 2324233002. It only contains blink side changes.

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

to

==========
[Presentation API] 1-UA: notify receiver page when receiver connection becomes
available (blink side)

- Add WebPresentationReceiver class
- Add OnReceiverConnectionAvailable() to PresentationDispatch class (to be
invoked by PresentationServiceImpl in following CL)
- Rename PresentationReceiver::onConnectionReceived() to
onReceiverConnectionAvailable()

This CL is splited from Issue 2324233002. It only contains blink side changes.

BUG=525660

Committed: https://crrev.com/31637e5d150105f89429c88a651ba1cc4c4453c4
Cr-Commit-Position: refs/heads/master@{#421985}
==========

[commit-bot: I haz the power, 2016-09-30 00:15:39]

Patchset 8 (id:??) landed as
https://crrev.com/31637e5d150105f89429c88a651ba1cc4c4453c4
Cr-Commit-Position: refs/heads/master@{#421985}