[Presentation API] Plumbing the |defaultpresentationstart| event from Blink to browser/Mojo.

[Presentation API] Plumbing the |defaultpresentationstart| event from Blink to browser/Mojo.

BUG=459001
Blocked by the Blink change due to changes in WebPresentationController interface: https://codereview.chromium.org/974993002

Committed: https://crrev.com/686a3ffc232a880ccde17e5825d8ab1e046a052c
Cr-Commit-Position: refs/heads/master@{#320031}

[whywhat, 2015-03-03 20:44:04]

avayvod@chromium.org changed reviewers:
+ imcheng@chromium.org, mfoltz@chromium.org, peter@chromium.org

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

PTaL

[Peter Beverloo, 2015-03-03 21:40:41]

Generally we use the following pattern in CL descriptions to indicate
dependencies:

"""
This CL is part of a three-sided patch:
  [1] https://codereview.chromium.org/12345
  [2] This CL.
  [3] The cleanup patch you have yet to write.
"""

https://codereview.chromium.org/978543002/diff/1/content/common/presentation/...
File content/common/presentation/presentation_service.mojom (right):

https://codereview.chromium.org/978543002/diff/1/content/common/presentation/...
content/common/presentation/presentation_service.mojom:45:
WaitForDefaultPresentation()
ListenToDefaultPresentation perhaps? Wait* in naming usually implies that the
call will block until whatever it waits for has completed.

https://codereview.chromium.org/978543002/diff/1/content/renderer/presentatio...
File content/renderer/presentation/presentation_dispatcher.cc (right):

https://codereview.chromium.org/978543002/diff/1/content/renderer/presentatio...
content/renderer/presentation/presentation_dispatcher.cc:64:
base::Unretained(this)));
   nit: indentation

https://codereview.chromium.org/978543002/diff/1/content/renderer/presentatio...
content/renderer/presentation/presentation_dispatcher.cc:76:
base::Unretained(this)));
nit: indentation

[whywhat, 2015-03-05 19:07:46]

The cleanup patch is no longer needed so I won't change the change description
as you suggested.

[mark a. foltz, 2015-03-05 19:53:52]

The patch I reviewed still called it "Listen" - did you need to upload the
"Wait" patch?

https://codereview.chromium.org/978543002/diff/20001/content/common/presentat...
File content/common/presentation/presentation_service.mojom (right):

https://codereview.chromium.org/978543002/diff/20001/content/common/presentat...
content/common/presentation/presentation_service.mojom:44: // agent.
Presumably this places a requirement on the implementation to queue up any such
session before this is called?  Can this requirement be described explicitly?

[whywhat, 2015-03-05 20:26:14]

https://codereview.chromium.org/978543002/diff/1/content/common/presentation/...
File content/common/presentation/presentation_service.mojom (right):

https://codereview.chromium.org/978543002/diff/1/content/common/presentation/...
content/common/presentation/presentation_service.mojom:45:
WaitForDefaultPresentation()
On 2015/03/03 21:40:40, Peter Beverloo wrote:
> ListenToDefaultPresentation perhaps? Wait* in naming usually implies that the
> call will block until whatever it waits for has completed.

Done.

https://codereview.chromium.org/978543002/diff/1/content/renderer/presentatio...
File content/renderer/presentation/presentation_dispatcher.cc (right):

https://codereview.chromium.org/978543002/diff/1/content/renderer/presentatio...
content/renderer/presentation/presentation_dispatcher.cc:64:
base::Unretained(this)));
On 2015/03/03 21:40:40, Peter Beverloo wrote:
>    nit: indentation

Done.

https://codereview.chromium.org/978543002/diff/1/content/renderer/presentatio...
content/renderer/presentation/presentation_dispatcher.cc:76:
base::Unretained(this)));
On 2015/03/03 21:40:40, Peter Beverloo wrote:
> nit: indentation

Done.

[whywhat, 2015-03-05 20:50:35]

On 2015/03/05 19:53:52, mark a. foltz wrote:
> The patch I reviewed still called it "Listen" - did you need to upload the
> "Wait" patch?

No, we agreed that Wait is a confusing name as it implies blocking. Listen was
the final name.

> 
>
https://codereview.chromium.org/978543002/diff/20001/content/common/presentat...
> File content/common/presentation/presentation_service.mojom (right):
> 
>
https://codereview.chromium.org/978543002/diff/20001/content/common/presentat...
> content/common/presentation/presentation_service.mojom:44: // agent.
> Presumably this places a requirement on the implementation to queue up any
such
> session before this is called?  Can this requirement be described explicitly?

Done.

[mark a. foltz, 2015-03-06 22:00:49]

lgtm

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

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

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

lgtm

https://codereview.chromium.org/978543002/diff/40001/content/renderer/present...
File content/renderer/presentation/presentation_dispatcher.cc (right):

https://codereview.chromium.org/978543002/diff/40001/content/renderer/present...
content/renderer/presentation/presentation_dispatcher.cc:162:
base::Unretained(this)));
nit: style, I would do something like
presentation_service_->ListenForDefaultPresentationStart(
    base::Bind(&PresentationDispatcher::OnDefaultPresentationStarted,
               base::Unretained(this)));

(feel free to ignore)

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

lgtm

[whywhat, 2015-03-10 15:19:50]

https://codereview.chromium.org/978543002/diff/40001/content/renderer/present...
File content/renderer/presentation/presentation_dispatcher.cc (right):

https://codereview.chromium.org/978543002/diff/40001/content/renderer/present...
content/renderer/presentation/presentation_dispatcher.cc:162:
base::Unretained(this)));
On 2015/03/06 22:06:56, Mounir Lamouri wrote:
> nit: style, I would do something like
> presentation_service_->ListenForDefaultPresentationStart(
>     base::Bind(&PresentationDispatcher::OnDefaultPresentationStarted,
>                base::Unretained(this)));
> 
> (feel free to ignore)

I had that but then I liked how both params to base::Bind were vertically
aligned...

[whywhat, 2015-03-10 23:21:13]

The CQ bit was checked by avayvod@chromium.org

[whywhat, 2015-03-10 23:21:13]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org,
mfoltz@chromium.org, mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/978543002/#ps60001 (title:
"Renamed DefaultPresentation to DefaultSession in line with the Blink CL")

[commit-bot: I haz the power, 2015-03-10 23:22:26]

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

[commit-bot: I haz the power, 2015-03-11 02:28:22]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-03-11 02:29:09]

Patchset 4 (id:??) landed as
https://crrev.com/686a3ffc232a880ccde17e5825d8ab1e046a052c
Cr-Commit-Position: refs/heads/master@{#320031}