Issue 940503002: [PresentationAPI] Added plumbing for start/joinSession from JS to platform

Issue 940503002: [PresentationAPI] Added plumbing for start/joinSession from JS to platform (Closed)

Created:
5 years, 10 months ago by whywhat

Modified:
5 years, 9 months ago

Reviewers:
mark a. foltz, imcheng, mlamouri (slow - plz ping)

CC:
blink-reviews, dglazkov+blink

Base URL:
https://chromium.googlesource.com/chromium/blink.git@master

Target Ref:
refs/heads/master

Project:
blink

Visibility:
Public.

More Reviews

Description

Added plumbing from navigator.presentation.start/joinSession to the WebPresentationClient and back. Added WebPresentationSessionClient and WebPresentationSessionCreateCallback with an implementation. public/platform change: https://codereview.chromium.org/935103002 Chromium change: https://codereview.chromium.org/935083002/ BUG=459005 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=191038

Patch Set 2 : Moved public/platform changes out to a separate issue #

Patch Set 3 : Moved the change to the existing public interface to this patch #

Patch Set 4 : Changed the Callbacks class in response to comments in other CLs #

Total comments: 16

Patch Set 5 : Addressed comment #

Total comments: 16

Patch Set 6 : Addressed various lifetime issues with callbacks #

Total comments: 4

Patch Set 7 : Moved session initialization to its create #

Patch Set 8 : Aligned PresentationSessionClientCallbacks with CallbackPromiseAdapter #

Patch Set 9 : Full change #

Created: 5 years, 10 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+265 lines, -14 lines)			Patch
M	Source/modules/modules.gypi	View	1 2 3 4 5 6 7 8	1 chunk	+4 lines, -0 lines	0 comments	Download
M	Source/modules/presentation/Presentation.h	View	1 2 3 4 5 6 8	3 chunks	+8 lines, -0 lines	0 comments	Download
M	Source/modules/presentation/Presentation.cpp	View	1 2 3 4 5 6 8	5 chunks	+23 lines, -2 lines	0 comments	Download
M	Source/modules/presentation/PresentationController.h	View	1 2 3 8	2 chunks	+7 lines, -1 line	0 comments	Download
M	Source/modules/presentation/PresentationController.cpp	View	1 2 3 4 5 8	1 chunk	+18 lines, -0 lines	0 comments	Download
A	Source/modules/presentation/PresentationError.h	View	1 2 3 4 5 6 7	1 chunk	+27 lines, -0 lines	0 comments	Download
A	Source/modules/presentation/PresentationError.cpp	View	1 2 3 4 5 6 7	1 chunk	+32 lines, -0 lines	0 comments	Download
M	Source/modules/presentation/PresentationSession.h	View	1 2 3 4 5 6 7 8	2 chunks	+9 lines, -4 lines	0 comments	Download
M	Source/modules/presentation/PresentationSession.cpp	View	1 2 3 4 5 6 7 8	3 chunks	+28 lines, -7 lines	0 comments	Download
A	Source/modules/presentation/PresentationSessionClientCallbacks.h	View	1 2 3 4 6 7 8	1 chunk	+44 lines, -0 lines	0 comments	Download
A	Source/modules/presentation/PresentationSessionClientCallbacks.cpp	View	1 2 3 4 5 6 7 8	1 chunk	+56 lines, -0 lines	0 comments	Download
M	public/platform/modules/presentation/WebPresentationClient.h	View	1 2 3 4 5 6 7 8	2 chunks	+9 lines, -0 lines	0 comments	Download

Messages

Total messages: 18 (4 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

mlamouri (slow - plz ping)

Regarding the WebCallbacks implementation, you might want to have a look at CallbackPromiseAdapter which is ...

5 years, 10 months ago (2015-02-23 13:46:11 UTC) #3

whywhat

https://codereview.chromium.org/940503002/diff/60001/Source/modules/presentation/PresentationController.cpp File Source/modules/presentation/PresentationController.cpp (right): https://codereview.chromium.org/940503002/diff/60001/Source/modules/presentation/PresentationController.cpp#newcode79 Source/modules/presentation/PresentationController.cpp:79: if (m_client) On 2015/02/23 13:46:10, Mounir Lamouri wrote: > ...

5 years, 10 months ago (2015-02-25 14:11:06 UTC) #4

whywhat

On 2015/02/23 13:46:11, Mounir Lamouri wrote: > Regarding the WebCallbacks implementation, you might want to ...

5 years, 10 months ago (2015-02-25 14:12:32 UTC) #5

mlamouri (slow - plz ping)

https://codereview.chromium.org/940503002/diff/80001/Source/modules/presentation/PresentationController.cpp File Source/modules/presentation/PresentationController.cpp (right): https://codereview.chromium.org/940503002/diff/80001/Source/modules/presentation/PresentationController.cpp#newcode80 Source/modules/presentation/PresentationController.cpp:80: m_client->startSession(presentationUrl, presentationId, callbacks); Hmm, sorry for realizing that now, ...

5 years, 10 months ago (2015-02-25 20:51:07 UTC) #8

whywhat

5 years, 10 months ago (2015-02-25 21:04:14 UTC) #9

https://codereview.chromium.org/940503002/diff/80001/Source/modules/presentat...
File Source/modules/presentation/PresentationController.cpp (right):

https://codereview.chromium.org/940503002/diff/80001/Source/modules/presentat...
Source/modules/presentation/PresentationController.cpp:80:
m_client->startSession(presentationUrl, presentationId, callbacks);
On 2015/02/25 20:51:06, Mounir Lamouri wrote:
> Hmm, sorry for realizing that now, but you have to |delete callbacks;| if
there
> is no |m_client|, otherwise, you are leaking that.
> 
> Maybe something like:
> if (!m_client) {
>   delete callbacks;
>   return;
> }
> m_client->...

I start thinking I need a ref-counted pointer here...

https://codereview.chromium.org/940503002/diff/80001/Source/modules/presentat...
File Source/modules/presentation/PresentationController.h (right):

https://codereview.chromium.org/940503002/diff/80001/Source/modules/presentat...
Source/modules/presentation/PresentationController.h:12: #include
"public/platform/modules/presentation/WebPresentationClient.h"
On 2015/02/25 20:51:06, Mounir Lamouri wrote:
> Why do you now need to #include instead of fwd declare?

That's where WebPresentationSessionClientCallbacks are defined.

https://codereview.chromium.org/940503002/diff/80001/Source/modules/presentat...
File Source/modules/presentation/PresentationSession.cpp (right):

https://codereview.chromium.org/940503002/diff/80001/Source/modules/presentat...
Source/modules/presentation/PresentationSession.cpp:17: , m_id(client->getId())
On 2015/02/25 20:51:06, Mounir Lamouri wrote:
> So if you only need the id, maybe you could only pass the ID here and in the
> callback. WebCallbacks usually own the pointers they are given. By passing
> |client|, it is expected that the PresentationSession will own it and it's a
bit
> confusing.

The client needs to outlive the callback since it is used to communicate with
the session object on the browser process side via Mojo.

mlamouri (slow - plz ping)

5 years, 10 months ago (2015-02-25 21:08:54 UTC) #10

whywhat

5 years, 10 months ago (2015-02-25 22:46:52 UTC) #11

https://codereview.chromium.org/940503002/diff/80001/Source/modules/presentat...
File Source/modules/presentation/PresentationController.cpp (right):

https://codereview.chromium.org/940503002/diff/80001/Source/modules/presentat...
Source/modules/presentation/PresentationController.cpp:80:
m_client->startSession(presentationUrl, presentationId, callbacks);
On 2015/02/25 21:08:54, Mounir Lamouri wrote:
> On 2015/02/25 at 21:04:14, whywhat wrote:
> > On 2015/02/25 20:51:06, Mounir Lamouri wrote:
> > > Hmm, sorry for realizing that now, but you have to |delete callbacks;| if
> there
> > > is no |m_client|, otherwise, you are leaking that.
> > > 
> > > Maybe something like:
> > > if (!m_client) {
> > >   delete callbacks;
> > >   return;
> > > }
> > > m_client->...
> > 
> > I start thinking I need a ref-counted pointer here...
> 
> There is no ref pointer that crosses Blink/Chromium boundary.

Something simple like std::unique_ptr would work in this case. It seems Blink
lacks such a pointer and RefPtr is too much :(

https://codereview.chromium.org/940503002/diff/80001/Source/modules/presentat...
Source/modules/presentation/PresentationController.cpp:86:
m_client->joinSession(presentationUrl, presentationId, callbacks);
On 2015/02/25 20:51:06, Mounir Lamouri wrote:
> ditto

Done.

https://codereview.chromium.org/940503002/diff/80001/Source/modules/presentat...
File Source/modules/presentation/PresentationSession.cpp (right):

https://codereview.chromium.org/940503002/diff/80001/Source/modules/presentat...
Source/modules/presentation/PresentationSession.cpp:17: , m_id(client->getId())
On 2015/02/25 21:08:54, Mounir Lamouri wrote:
> On 2015/02/25 at 21:04:14, whywhat wrote:
> > On 2015/02/25 20:51:06, Mounir Lamouri wrote:
> > > So if you only need the id, maybe you could only pass the ID here and in
the
> > > callback. WebCallbacks usually own the pointers they are given. By passing
> > > |client|, it is expected that the PresentationSession will own it and it's
a
> bit
> > > confusing.
> > 
> > The client needs to outlive the callback since it is used to communicate
with
> the session object on the browser process side via Mojo.
> 
> Understood. That's why you should pass the id only, not the whole client.
Unless
> you will need the client later? (which would be problematic)

Ok, I'll pass the presentation id and will use it to look the
WebPresentationSessionClient up later on.

https://codereview.chromium.org/940503002/diff/80001/Source/modules/presentat...
File Source/modules/presentation/PresentationSessionClientCallbacks.cpp (right):

https://codereview.chromium.org/940503002/diff/80001/Source/modules/presentat...
Source/modules/presentation/PresentationSessionClientCallbacks.cpp:36:
PresentationSession* session =
PresentationSession::create(m_presentation->frame(), client);
On 2015/02/25 20:51:06, Mounir Lamouri wrote:
> Could you rewrite that so it follows the same pattern as
CallbackPromiseAdapter.
> It would imply having a ::take() method instead of ::create().
> 
> There are a few checks on m_resolver that you are not doing and should be
doing.

I added the checks and fixed ownership of the parameters passed. I think my code
is simpler and clearer that what you suggested about following
CallbackPromiseAdapter though equivalent.

https://codereview.chromium.org/940503002/diff/80001/Source/modules/presentat...
Source/modules/presentation/PresentationSessionClientCallbacks.cpp:46:
m_resolver->reject(DOMException::create(InvalidAccessError, error->message));
On 2015/02/25 20:51:06, Mounir Lamouri wrote:
> ditto.

Done.

mlamouri (slow - plz ping)

I think it would be better if you could follow the same pattern as CallbackPromiseResolver. ...

5 years, 10 months ago (2015-02-26 10:57:03 UTC) #12

whywhat

https://codereview.chromium.org/940503002/diff/100001/Source/modules/presentation/PresentationSessionClientCallbacks.cpp File Source/modules/presentation/PresentationSessionClientCallbacks.cpp (right): https://codereview.chromium.org/940503002/diff/100001/Source/modules/presentation/PresentationSessionClientCallbacks.cpp#newcode36 Source/modules/presentation/PresentationSessionClientCallbacks.cpp:36: PresentationSession* session = PresentationSession::create(m_presentation->frame(), *id); On 2015/02/26 10:57:03, Mounir ...

5 years, 10 months ago (2015-02-26 15:50:54 UTC) #13

commit-bot: I haz the power

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

5 years, 9 months ago (2015-02-28 01:58:13 UTC) #17

commit-bot: I haz the power

5 years, 9 months ago (2015-02-28 02:03:24 UTC) #18

Message was sent while issue was closed.

Committed patchset #9 (id:160001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=191038

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages