[Presentation API] Adds close method to Presentation session.

Adding plumbing from the PresentationSession IDL implementation to the platform interface.

This is part of a three-side change:
1. This (Blink) CL
2. Chromium CL: https://codereview.chromium.org/999003004/
3. Cleanup: Making the new WebPresentationClient method pure virtual.

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

[haibinlu, 2015-03-17 22:34:07]

haibinlu@chromium.org changed reviewers:
+ avayvod@chromium.org, imcheng@chromium.org

[whywhat, 2015-03-17 23:35:19]

Thanks for the change!

Could you please add more information to the issue description?

E.g. put [Presentation API] in front of the title and expand that you're just
"adding plumbing from the PresentationSession IDL implementation to the platform
interface" (since the method is already there from the webpage's point of view
without this CL and there's no working implementation still after this CL
lands).

I've had reviewers ask me to have a full list of CLs there, i.e.

"This is part of a three-side change:
1. This (Blink) CL
2. Chromium CL: <link>
3. Cleanup: Making the new WebPresentationClient method pure virtual <link>"

Although I won't insist on it as soon as all of the three are submitted
promptly.

https://codereview.chromium.org/1015903002/diff/1/Source/modules/presentation...
File Source/modules/presentation/PresentationController.cpp (right):

https://codereview.chromium.org/1015903002/diff/1/Source/modules/presentation...
Source/modules/presentation/PresentationController.cpp:109: if (!m_client) {
nit: remove the curly braces to be consistent with other one line if's in the
file

https://codereview.chromium.org/1015903002/diff/1/Source/modules/presentation...
Source/modules/presentation/PresentationController.cpp:112:
m_client->closeSession(presentationId);
high level question: do we want to get any result back, btw?

https://codereview.chromium.org/1015903002/diff/1/Source/modules/presentation...
File Source/modules/presentation/PresentationController.h (right):

https://codereview.chromium.org/1015903002/diff/1/Source/modules/presentation...
Source/modules/presentation/PresentationController.h:58: void closeSession(const
String& presentationId);
Shouldn't you pass the url too? I don't think we rely on just the ids to be
unique, only <url, id> pairs.
At least that's what i got from mfoltz@ and imcheng@ comments on
https://codereview.chromium.org/979413002/ We could discuss in more detail
tomorrow.

https://codereview.chromium.org/1015903002/diff/1/Source/modules/presentation...
File Source/modules/presentation/PresentationSession.cpp (right):

https://codereview.chromium.org/1015903002/diff/1/Source/modules/presentation...
Source/modules/presentation/PresentationSession.cpp:72: if (controller) {
nit: remove the braces

https://codereview.chromium.org/1015903002/diff/1/Source/modules/presentation...
File Source/modules/presentation/PresentationSession.h (right):

https://codereview.chromium.org/1015903002/diff/1/Source/modules/presentation...
Source/modules/presentation/PresentationSession.h:49: // Returns the
|PresentationController| object associated with the frame |Presentation|
corresponds to.
nit: Although Blink doesn't yet have line length limit, it doesn't forbid to
limit comments to 80 chars (unlike method prototypes, for example). I think
keeping this comment in 80 chars would make the reviews/reading the code
relatively easier.

https://codereview.chromium.org/1015903002/diff/1/public/platform/modules/pre...
File public/platform/modules/presentation/WebPresentationClient.h (right):

https://codereview.chromium.org/1015903002/diff/1/public/platform/modules/pre...
public/platform/modules/presentation/WebPresentationClient.h:43: // Called when
the frame request to close an existing session.
nit: I think I missed s at the end of the request in the comments above. Could
you s/request/requests, please?

https://codereview.chromium.org/1015903002/diff/1/public/platform/modules/pre...
public/platform/modules/presentation/WebPresentationClient.h:46: //
TODO(haibinlu): make it pure virtual once Chromium has the implementation.
nit: ditto re 80 chars

[haibinlu, 2015-03-18 00:58:23]

https://codereview.chromium.org/1015903002/diff/1/Source/modules/presentation...
File Source/modules/presentation/PresentationController.cpp (right):

https://codereview.chromium.org/1015903002/diff/1/Source/modules/presentation...
Source/modules/presentation/PresentationController.cpp:109: if (!m_client) {
On 2015/03/17 23:35:19, whywhat wrote:
> nit: remove the curly braces to be consistent with other one line if's in the
> file

Done.

https://codereview.chromium.org/1015903002/diff/1/Source/modules/presentation...
Source/modules/presentation/PresentationController.cpp:112:
m_client->closeSession(presentationId);
On 2015/03/17 23:35:19, whywhat wrote:
> high level question: do we want to get any result back, btw?


The result of session.close is onstatechange event. Two approaches:
1. Have PresentationController.closeSession return a promise. Then fire
onstatechagne when promise resolves to true.
2. Have MR send onstatechange event if session is stopped.
PresentationController.closeSession return void. 

From developer point of view, the two approaches have no difference. I prefer
the 2nd approach because MR has to send onstatechange anyway in case the session
was stopped by other senders etc. For example, 

1. web app A in browser 1 invokes session.close; 
2. web app B in browser 2 invokes session.close;
3. right after browser 1 sends out stop command, the session stopped message
arrives as a broadcast message. browser 1 knows the session was stopped, but can
not relate the message to its previous command because it is a broadcast
message.
4. browser 1 updates web app A via onstatechange. The promise returned is still
unresolved.

https://codereview.chromium.org/1015903002/diff/1/Source/modules/presentation...
File Source/modules/presentation/PresentationController.h (right):

https://codereview.chromium.org/1015903002/diff/1/Source/modules/presentation...
Source/modules/presentation/PresentationController.h:58: void closeSession(const
String& presentationId);
On 2015/03/17 23:35:19, whywhat wrote:
> Shouldn't you pass the url too? I don't think we rely on just the ids to be
> unique, only <url, id> pairs.
> At least that's what i got from mfoltz@ and imcheng@ comments on
> https://codereview.chromium.org/979413002/ We could discuss in more detail
> tomorrow.

Done. But I do not think URL+ID is unique in a browser instance. URL is just
extra piece of information MR can use.

https://codereview.chromium.org/1015903002/diff/1/Source/modules/presentation...
File Source/modules/presentation/PresentationSession.cpp (right):

https://codereview.chromium.org/1015903002/diff/1/Source/modules/presentation...
Source/modules/presentation/PresentationSession.cpp:72: if (controller) {
On 2015/03/17 23:35:19, whywhat wrote:
> nit: remove the braces

Done.

https://codereview.chromium.org/1015903002/diff/1/Source/modules/presentation...
File Source/modules/presentation/PresentationSession.h (right):

https://codereview.chromium.org/1015903002/diff/1/Source/modules/presentation...
Source/modules/presentation/PresentationSession.h:49: // Returns the
|PresentationController| object associated with the frame |Presentation|
corresponds to.
On 2015/03/17 23:35:19, whywhat wrote:
> nit: Although Blink doesn't yet have line length limit, it doesn't forbid to
> limit comments to 80 chars (unlike method prototypes, for example). I think
> keeping this comment in 80 chars would make the reviews/reading the code
> relatively easier.

Done.

https://codereview.chromium.org/1015903002/diff/1/public/platform/modules/pre...
File public/platform/modules/presentation/WebPresentationClient.h (right):

https://codereview.chromium.org/1015903002/diff/1/public/platform/modules/pre...
public/platform/modules/presentation/WebPresentationClient.h:43: // Called when
the frame request to close an existing session.
On 2015/03/17 23:35:19, whywhat wrote:
> nit: I think I missed s at the end of the request in the comments above. Could
> you s/request/requests, please?

Done.

https://codereview.chromium.org/1015903002/diff/1/public/platform/modules/pre...
public/platform/modules/presentation/WebPresentationClient.h:46: //
TODO(haibinlu): make it pure virtual once Chromium has the implementation.
On 2015/03/17 23:35:19, whywhat wrote:
> nit: ditto re 80 chars

Done.

[whywhat, 2015-03-18 12:51:19]

lgtm

https://codereview.chromium.org/1015903002/diff/1/Source/modules/presentation...
File Source/modules/presentation/PresentationController.cpp (right):

https://codereview.chromium.org/1015903002/diff/1/Source/modules/presentation...
Source/modules/presentation/PresentationController.cpp:112:
m_client->closeSession(presentationId);
On 2015/03/18 00:58:22, haibinlu wrote:
> On 2015/03/17 23:35:19, whywhat wrote:
> > high level question: do we want to get any result back, btw?
> 
> 
> The result of session.close is onstatechange event. Two approaches:
> 1. Have PresentationController.closeSession return a promise. Then fire
> onstatechagne when promise resolves to true.
> 2. Have MR send onstatechange event if session is stopped.
> PresentationController.closeSession return void. 
> 
> From developer point of view, the two approaches have no difference. I prefer
> the 2nd approach because MR has to send onstatechange anyway in case the
session
> was stopped by other senders etc. For example, 
> 
> 1. web app A in browser 1 invokes session.close; 
> 2. web app B in browser 2 invokes session.close;
> 3. right after browser 1 sends out stop command, the session stopped message
> arrives as a broadcast message. browser 1 knows the session was stopped, but
can
> not relate the message to its previous command because it is a broadcast
> message.
> 4. browser 1 updates web app A via onstatechange. The promise returned is
still
> unresolved.

Ok, I think this makes sense and aligns with the spec.

[haibinlu, 2015-03-18 21:14:33]

The CQ bit was checked by haibinlu@chromium.org

[commit-bot: I haz the power, 2015-03-18 21:15:23]

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

[commit-bot: I haz the power, 2015-03-18 22:51:04]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=192122