[PresentationAPI] Added/changed public/platform interfaces for start/joinSession

[PresentationAPI] Added/changed public/platform interfaces for start/joinSession

See https://codereview.chromium.org/940503002/ and https://codereview.chromium.org/935083002/
for uses and implementation.

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

[whywhat, 2015-02-18 16:39:15]

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

[whywhat, 2015-02-18 16:39:15]

PTAL.

Note that due to the Blink/Chromium being separate repositories, one has to
commit an interface patch like this first, then the Chromium change implementing
the interfaces and mocks and then the Blink change using the new
interfaces/methods.

So this has to be reviewed looking back at the Chromium and Blink patches
(referenced in the description).

[whywhat, 2015-02-18 16:45:12]

On 2015/02/18 16:39:15, whywhat wrote:
> PTAL.
> 
> Note that due to the Blink/Chromium being separate repositories, one has to
> commit an interface patch like this first, then the Chromium change
implementing
> the interfaces and mocks and then the Blink change using the new
> interfaces/methods.
> 
> So this has to be reviewed looking back at the Chromium and Blink patches
> (referenced in the description).

Ok, it seems to be a bit different:
1. Submit the new interfaces only (no new methods in existing interfaces;
otherwise the existing implementations will fail)
2. Submit the Chromium change that implements new methods before these are
declared in the interface.
3. Submit the Blink change that adds new methods and usage of the interfaces.
Still three changes only (sic!) but reshuffled a bit.

[mark a. foltz, 2015-02-18 17:55:30]

LGTM with some suggested docstring edits.

https://codereview.chromium.org/935103002/diff/20001/public/platform/modules/...
File public/platform/modules/presentation/WebPresentationSessionCreateCallback.h
(right):

https://codereview.chromium.org/935103002/diff/20001/public/platform/modules/...
public/platform/modules/presentation/WebPresentationSessionCreateCallback.h:14:
// When creating the session succeeds, returns the implementation of the
..., onSuccess() is invoked with the implementation...

https://codereview.chromium.org/935103002/diff/20001/public/platform/modules/...
public/platform/modules/presentation/WebPresentationSessionCreateCallback.h:16:
// an appropriate error code.
..., onError() is invoked with an appropriate error message.

[mlamouri (slow - plz ping), 2015-02-18 21:08:55]

https://codereview.chromium.org/935103002/diff/20001/public/platform/modules/...
File public/platform/modules/presentation/WebPresentationSessionCreateCallback.h
(right):

https://codereview.chromium.org/935103002/diff/20001/public/platform/modules/...
public/platform/modules/presentation/WebPresentationSessionCreateCallback.h:22:
virtual void onError(const WebString& message) = 0;
Instead of passing a simple string, could you pass a struct that contains an
enum (ErrorType) and a string (the message). The Web API will have to expose
DOMException and the DOMException type is actually more important that the
message.

You can take example on WebPushError.

[mlamouri (slow - plz ping), 2015-02-18 21:29:16]

https://codereview.chromium.org/935103002/diff/20001/public/platform/modules/...
File public/platform/modules/presentation/WebPresentationSessionCreateCallback.h
(right):

https://codereview.chromium.org/935103002/diff/20001/public/platform/modules/...
public/platform/modules/presentation/WebPresentationSessionCreateCallback.h:17:
class WebPresentationSessionCreateCallback {
Could you inherit from WebCallbacks?
Also, that "WebPresentationSessionCreateCallback" name is odd. What about
"WebPresentationSessionClientCallbacks"?

[whywhat, 2015-02-19 14:20:48]

New patchsets have been uploaded after l-g-t-m from mfoltz@chromium.org

[whywhat, 2015-02-19 14:23:25]

https://codereview.chromium.org/935103002/diff/20001/public/platform/modules/...
File public/platform/modules/presentation/WebPresentationSessionCreateCallback.h
(right):

https://codereview.chromium.org/935103002/diff/20001/public/platform/modules/...
public/platform/modules/presentation/WebPresentationSessionCreateCallback.h:14:
// When creating the session succeeds, returns the implementation of the
On 2015/02/18 17:55:29, mark a. foltz wrote:
> ..., onSuccess() is invoked with the implementation...

Interface removed, but I'll apply this to the implementation of the WebCallbacks
interface.

https://codereview.chromium.org/935103002/diff/20001/public/platform/modules/...
public/platform/modules/presentation/WebPresentationSessionCreateCallback.h:16:
// an appropriate error code.
On 2015/02/18 17:55:29, mark a. foltz wrote:
> ..., onError() is invoked with an appropriate error message.

Interface removed, but I'll apply this to the implementation of the WebCallbacks
interface.

https://codereview.chromium.org/935103002/diff/20001/public/platform/modules/...
public/platform/modules/presentation/WebPresentationSessionCreateCallback.h:17:
class WebPresentationSessionCreateCallback {
On 2015/02/18 21:29:16, Mounir Lamouri wrote:
> Could you inherit from WebCallbacks?
> Also, that "WebPresentationSessionCreateCallback" name is odd. What about
> "WebPresentationSessionClientCallbacks"?

If I use WebCallbacks, I don't really need a separate interface, do I? I'll
directly inherit from WebCallbacks<WebPresentationSessionClient,
WebPresentationError> in PresentationSessionClientCallbacks in Blink.

https://codereview.chromium.org/935103002/diff/20001/public/platform/modules/...
public/platform/modules/presentation/WebPresentationSessionCreateCallback.h:22:
virtual void onError(const WebString& message) = 0;
On 2015/02/18 21:08:55, Mounir Lamouri wrote:
> Instead of passing a simple string, could you pass a struct that contains an
> enum (ErrorType) and a string (the message). The Web API will have to expose
> DOMException and the DOMException type is actually more important that the
> message.
> 
> You can take example on WebPushError.

Added WebPresentationError.

[whywhat, 2015-02-19 16:27:20]

PTAL

[mark a. foltz, 2015-02-19 17:52:26]

Still LGTM

[mlamouri (slow - plz ping), 2015-02-23 13:58:53]

lgtm with the comment applied

https://codereview.chromium.org/935103002/diff/60001/public/platform/modules/...
File public/platform/modules/presentation/WebPresentationError.h (right):

https://codereview.chromium.org/935103002/diff/60001/public/platform/modules/...
public/platform/modules/presentation/WebPresentationError.h:27:
WebPresentationError(int errorType, const WebString& message)
I think you should get ride of that constructor. Consumers of the public Blink
API should not pass random integers.

[whywhat, 2015-02-23 14:25:04]

New patchsets have been uploaded after l-g-t-m from
mfoltz@chromium.org,mlamouri@chromium.org

[whywhat, 2015-02-25 11:39:49]

https://codereview.chromium.org/935103002/diff/60001/public/platform/modules/...
File public/platform/modules/presentation/WebPresentationError.h (right):

https://codereview.chromium.org/935103002/diff/60001/public/platform/modules/...
public/platform/modules/presentation/WebPresentationError.h:27:
WebPresentationError(int errorType, const WebString& message)
On 2015/02/23 13:58:53, Mounir Lamouri wrote:
> I think you should get ride of that constructor. Consumers of the public Blink
> API should not pass random integers.

Done.

[whywhat, 2015-02-25 15:02:22]

The CQ bit was checked by avayvod@chromium.org

[commit-bot: I haz the power, 2015-02-25 15:03:06]

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

[commit-bot: I haz the power, 2015-02-25 17:27:26]

Committed patchset #5 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=190843