Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1314413005:
[Presentation API] 1-UA presentation support + presenter APIs. (Closed)
|
Created:
5 years, 3 months ago by imcheng Modified:
4 years ago CC:
Aaron Boodman, abarth-chromium, ben+mojo_chromium.org, chromium-apps-reviews_chromium.org, chromium-reviews, darin (slow to review), darin-cc_chromium.org, extensions-reviews_chromium.org, feature-media-reviews_chromium.org, jam, mcasas+watch_chromium.org, media-router+watch_chromium.org, miu+watch_chromium.org, posciak+watch_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, wjia+watch_chromium.org, yzshen+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Presentation API] 1-UA presentation support + presenter APIs.
This patch implements the 1-UA mode support according to the doc I sent
out a while ago.
Contains the following major components:
- OffscreenPresentationManager - browser context keyed service to
connect the controller and receiver sides of an 1-ua presentation.
Instance is shared with incognito browser contexts to allow
communication with the offscreen tab.
- ReceiverPresentationServiceDelegateImpl - receiver-side implementation
of PresentationServiceDelegate, only available to offscreen tabs.
Talks to OffscreenPresentationManager.
- PresentationServiceDelegateImpl - modified logic to maintain
controller side of offscreen presentations and redirect incoming
messages, as well as listen for messages / state changes from the receiver
side.
TODO:
- Need additional identifier for PresentationSessionInfo objects.
Current proposal is to use RFH ID and limit 1 PresentationSession
per presentation ID per frame. Until then we can't really have
multiple controllers / getSessions().
BUG=513859
Patch Set 1 #
Total comments: 50
Patch Set 2 : Addressed comments #Patch Set 3 : #
Total comments: 32
Patch Set 4 : Addressed Yuri's comments; rework of the OffscreenPresentationManager interface #
Total comments: 66
Patch Set 5 : Addressed mfoltz's comments rebase (forgot to upload as separate patchsets, sorry) #
Total comments: 2
Patch Set 6 : Rebase again to pick up Yuri's cl #
Total comments: 48
Patch Set 7 : addressed yuri's comments #16 #
Total comments: 80
Patch Set 8 : Addressed comments #18-21 #
Total comments: 12
Patch Set 9 : Compiles, but still need to address todos #Patch Set 10 : #Patch Set 11 : #Patch Set 12 : Rename + api change to align with spec #Patch Set 13 : Added tests #Patch Set 14 : rebase #Messages
Total messages: 32 (5 generated)
imcheng@chromium.org changed reviewers: + avayvod@chromium.org, mfoltz@chromium.org, miu@chromium.org
Reviewers: PTAL. High level feedbacks are appreciated. Note that between the regular and incognito implementations of PresentationServiceDelegate, we may be able to merge much of the logic since Kevin discovered a way to re-enable MR in incognito mode. If we go ahead and do that then I will modify this patch accordingly. If the design is ok on the high level, then I will split this patch into multiple smaller ones and send out reviews individually. Thanks!
Several comments inline, high level: 1. It would be cleaner to identify the route as an offscreen presentation so we don't have to pass a boolean flag everywhere 2. The 1-UA router could be more object oriented and encapsulate details of the presentation and its controllers, I feel this would help also with the id passing everywhere 3. The definitions of the objects in #2 could be in content to facilitate the glue between the PS and the PSD 4. Some bikeshed naming https://codereview.chromium.org/1314413005/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/tab_capture/offscreen_presentation.cc (right): https://codereview.chromium.org/1314413005/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/tab_capture/offscreen_presentation.cc:248: one_ua_presentation_router_->RegisterPresenter(presentation_id(), rfh_id); Can RegisterPresenter() just take the (id, WebContents) and do the lookups internally? https://codereview.chromium.org/1314413005/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/tab_capture/offscreen_presentation.cc:505: one_ua_presentation_router_->UnregisterPresenter(rfh_id); UnregisterPresentation(WebContents*) https://codereview.chromium.org/1314413005/diff/1/chrome/browser/media/router... File chrome/browser/media/router/create_presentation_session_request.h (right): https://codereview.chromium.org/1314413005/diff/1/chrome/browser/media/router... chrome/browser/media/router/create_presentation_session_request.h:57: bool is_one_ua_presentation); ISTM the "is_one_ua_presentation" is a flag that could be attached to the MediaRoute for the presentation, instead of plumbing it through here. Naming: is_offscreen_presentation https://codereview.chromium.org/1314413005/diff/1/chrome/browser/media/router... File chrome/browser/media/router/one_ua_presentation_router.h (right): https://codereview.chromium.org/1314413005/diff/1/chrome/browser/media/router... chrome/browser/media/router/one_ua_presentation_router.h:25: class OneUAPresentationRouter : public KeyedService { Naming nitpicks: The term "One-UA" comes from the spec and while it is helpful to distinguish the two cases, I feel like casual readers of the code won't know what it means. Also, this class is routing messages - but by calling it "Router" there might be some confusion between it and the MediaRouter. I would propose naming this "OffscreenPresentationManager." Open to other ideas as well. https://codereview.chromium.org/1314413005/diff/1/chrome/browser/media/router... chrome/browser/media/router/one_ua_presentation_router.h:45: // Returns true if the frame given by |frame_id| is a presenter frame. ISTM that registering a controller or presenter should return an object that acts as handle, say OffscreenPresenter and OffscreenController. Then you don't need to keep checking ids, you can just ask that object. https://codereview.chromium.org/1314413005/diff/1/chrome/browser/media/router... chrome/browser/media/router/one_ua_presentation_router.h:59: void GetSinglePresenterSession( Can the Presenter object (I mentioned above) get a callback when a controller is connected? https://codereview.chromium.org/1314413005/diff/1/chrome/browser/media/router... chrome/browser/media/router/one_ua_presentation_router.h:69: std::vector<content::PresentationSessionInfo> GetPresenterSessions( The Presenter would know which sessions are connected? https://codereview.chromium.org/1314413005/diff/1/chrome/browser/media/router... chrome/browser/media/router/one_ua_presentation_router.h:108: struct OneUAPresentationRoute { Perhaps the OffscreenPresenter owns a list of OffscreenControllers? https://codereview.chromium.org/1314413005/diff/1/chrome/browser/media/router... File chrome/browser/media/router/presentation_service_delegate_impl.h (right): https://codereview.chromium.org/1314413005/diff/1/chrome/browser/media/router... chrome/browser/media/router/presentation_service_delegate_impl.h:108: void GetPresenterSession( FWIW, the terminology in the spec group is moving towards PresentationReceiver to designate the browsing context that hosts the presentation. https://github.com/w3c/presentation-api/pull/186 So this could be GetPresentationReceiverSession... https://codereview.chromium.org/1314413005/diff/1/chrome/browser/media/router... chrome/browser/media/router/presentation_service_delegate_impl.h:187: const MediaRoute::Id& route_id, Can we pass the MediaRoute* here and ask if it's an offscreen presentation?
Will take another look once Mark's comments are addressed. https://codereview.chromium.org/1314413005/diff/1/chrome/browser/chrome_conte... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/1314413005/diff/1/chrome/browser/chrome_conte... chrome/browser/chrome_content_browser_client.cc:2560: return media_router::IncognitoPresentationServiceDelegate:: nit: is this an Impl of the PresentationServiceDelegate? could be reflected in the name to be consistent. https://codereview.chromium.org/1314413005/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/tab_capture/offscreen_presentation.h (right): https://codereview.chromium.org/1314413005/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/tab_capture/offscreen_presentation.h:186: void Close(); nit: the name is very out of sync from the comment - either the comment reveals implementation details or the name has to be changed to something along the lines of it https://codereview.chromium.org/1314413005/diff/1/chrome/browser/media/router... File chrome/browser/media/router/create_presentation_session_request.h (right): https://codereview.chromium.org/1314413005/diff/1/chrome/browser/media/router... chrome/browser/media/router/create_presentation_session_request.h:57: bool is_one_ua_presentation); On 2015/09/11 at 23:24:20, mark a. foltz wrote: > ISTM the "is_one_ua_presentation" is a flag that could be attached to the MediaRoute for the presentation, instead of plumbing it through here. > > Naming: is_offscreen_presentation +1 https://codereview.chromium.org/1314413005/diff/1/chrome/browser/media/router... File chrome/browser/media/router/incognito_presentation_service_delegate.h (right): https://codereview.chromium.org/1314413005/diff/1/chrome/browser/media/router... chrome/browser/media/router/incognito_presentation_service_delegate.h:41: class IncognitoPresentationServiceDelegate Maybe this shoud be something like ReceiverPresentationServiceDelegateImpl, Incognito/Offscreen are implementation details. https://codereview.chromium.org/1314413005/diff/1/chrome/browser/media/router... chrome/browser/media/router/incognito_presentation_service_delegate.h:118: content::WebContents* const web_contents_; Do you think there might be a BasePresentationServiceDelegateImpl that would hold the web_contents_ and any other common logic for example?
On the whole, I wouldn't use the "1UA" phrase everywhere as it isn't very meaningful to anyone not immediately familiar with the MR/Presentation API 1UA use case. Naming suggestions below. Pretty much in agreement with others' comments. Here are a few other things I noticed (or expanded upon): https://codereview.chromium.org/1314413005/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/tab_capture/offscreen_presentation.cc (right): https://codereview.chromium.org/1314413005/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/tab_capture/offscreen_presentation.cc:193: one_ua_presentation_router_( IMO, you should not store a pointer to the router in this class. It is only used once at start-up, and once at destruction time to register/deregister. Incorporating mfoltz's comment below, the register call would look like: media_router::OneUAPresentationRouterFactory:: GetOrCreateForBrowserContext(owner_->extension_web_contents() ->GetBrowserContext()) ->RegisterPresenter(presentation_id(), presentation_web_contents_.get()); https://codereview.chromium.org/1314413005/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/tab_capture/offscreen_presentation.cc:248: one_ua_presentation_router_->RegisterPresenter(presentation_id(), rfh_id); +1 to mfoltz's comment. Also, is registering the presentation here the correct order-of-operations? Meaning, it seems like the presentation should be registered *before* the navigation is started (on L231-235 above). https://codereview.chromium.org/1314413005/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/tab_capture/offscreen_presentation.h (right): https://codereview.chromium.org/1314413005/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/tab_capture/offscreen_presentation.h:186: void Close(); On 2015/09/14 15:20:39, whywhat wrote: > nit: the name is very out of sync from the comment - either the comment reveals > implementation details or the name has to be changed to something along the > lines of it I think it would be reasonable to name this method Destroy(), which handles de-registering and deletes |this| before returning. This is the approach classes like content::RenderWidgetHostImpl take. Ex: https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re... https://codereview.chromium.org/1314413005/diff/1/chrome/browser/media/router... File chrome/browser/media/router/create_presentation_session_request.h (right): https://codereview.chromium.org/1314413005/diff/1/chrome/browser/media/router... chrome/browser/media/router/create_presentation_session_request.h:57: bool is_one_ua_presentation); +1 too, due to the fact that this bool argument is viral across dozens of files. ;-) https://codereview.chromium.org/1314413005/diff/1/chrome/browser/media/router... File chrome/browser/media/router/incognito_presentation_service_delegate.h (right): https://codereview.chromium.org/1314413005/diff/1/chrome/browser/media/router... chrome/browser/media/router/incognito_presentation_service_delegate.h:41: class IncognitoPresentationServiceDelegate On 2015/09/14 15:20:39, whywhat wrote: > Maybe this shoud be something like ReceiverPresentationServiceDelegateImpl, > Incognito/Offscreen are implementation details. FYI--Ben Kalman in recent meeting proposed we used the wording "sandboxed," so maybe "SandboxedPresentationServiceDelegate" would be a good name? Or, alternatively, the incognito profile impl class is called "OffTheRecordProfile," so maybe "OffTheRecordPresentationServiceDelegate?" https://codereview.chromium.org/1314413005/diff/1/chrome/browser/media/router... File chrome/browser/media/router/one_ua_presentation_router.h (right): https://codereview.chromium.org/1314413005/diff/1/chrome/browser/media/router... chrome/browser/media/router/one_ua_presentation_router.h:25: class OneUAPresentationRouter : public KeyedService { On 2015/09/11 23:24:20, mark a. foltz wrote: > Naming nitpicks: > > The term "One-UA" comes from the spec and while it is helpful to distinguish the > two cases, I feel like casual readers of the code won't know what it means. > > Also, this class is routing messages - but by calling it "Router" there might be > some confusion between it and the MediaRouter. > > I would propose naming this "OffscreenPresentationManager." Open to other ideas > as well. Random OTOH replacements for the term "One UA Presentation": * CapturedPresentation (or MirroredPresentation, but IMO "Captured" is better) * SimulatedPresentation (because we're simulating a presentation page being "logically" rendered on a remote screen) * LocalInstancePresentation (or just LocalPresentation) * LocallyRenderedPresentation https://codereview.chromium.org/1314413005/diff/1/chrome/browser/media/router... File chrome/browser/media/router/render_frame_host_helper.h (right): https://codereview.chromium.org/1314413005/diff/1/chrome/browser/media/router... chrome/browser/media/router/render_frame_host_helper.h:12: using RenderFrameHostId = std::pair<int, int>; nit: Although they're likely always to be int's, it is documentative to define this as: using RenderFrameHostId = std::pair<decltype(content::RenderProcessHost::GetID()), decltype(content::RenderFrameHost::GetRoutingID())>; https://codereview.chromium.org/1314413005/diff/1/content/public/browser/pres... File content/public/browser/presentation_service_delegate.h (right): https://codereview.chromium.org/1314413005/diff/1/content/public/browser/pres... content/public/browser/presentation_service_delegate.h:189: int render_process_id, BTW--Most of the methods in this interface have these two render_process_id and render_frame_id arguments. Is it ever possible to use this API on non-main render frames? If not, you may want to clarify by naming the 2nd arg main_render_frame_id and also DCHECK()ing in the method impls.
Oh, and BTW, I will be resuming work on https://codereview.chromium.org/1221483002/ next week so we'll very soon be able to get something up-and-running! :)
Addressed comments, PTAL. There are some large changes in the new patchset, so
here's a summary:
Add OffscreenPresenterContext and OffscreenController classes.
These are handles for the PresentationServiceDelegateImpl classes to operate on
offscreen presentation session
objects. They serve as a thin layer to isolate the broader set of APIs of
OffscreenPresentationManager from the
PSDImpl classes, so that they will have access to a narrower set of APIs that
are appropriate in
presenter / controller contexts.
This means the offscreen persentations data is all kept in
OffscreenPresentationManager. It serves as an
intermediary since presenter/controller registration happens in various places.
I think it is simplest to keep
bookkeeping there.
This lets me privatize most of OffscreenPresentationManager APIs and only expose
some registration APIs. The
rest of APIs that operates on sessions will be done via the
Offscreen{Presenter,Controller} classes.
Internally OffscreenPresentationManager uses the following data objects for
representation:
- OffscreenPresentationSession corresponds to a PresentationSession object.
- OffscreenPresentationRoute represents a connection between a presenter
PresentationSession object and a controller PresentationSession object. Thus
there are multiple OffscreenPresentationRoute objects for a presentation when
there
are multiple controllers.
- OffscreenPresentationInfo contains all connections for that presentation, i.e.
each one corresponds to a presentation id.
Since we can't support multiple controller currently, there is only a single
OffscreenPresentationRoute in OffscreenPresentationInfo. (crbug.com/529911)
I still need to fill in comments for some new classes / methods. There's also
some opportunities for refactoring
out some common logic. But since I changed a lot of things here I want to make
sure the design is headed in the
right direction before making further changes. Thanks.
https://codereview.chromium.org/1314413005/diff/1/chrome/browser/chrome_conte...
File chrome/browser/chrome_content_browser_client.cc (right):
https://codereview.chromium.org/1314413005/diff/1/chrome/browser/chrome_conte...
chrome/browser/chrome_content_browser_client.cc:2560: return
media_router::IncognitoPresentationServiceDelegate::
On 2015/09/14 15:20:39, whywhat wrote:
> nit: is this an Impl of the PresentationServiceDelegate? could be reflected in
> the name to be consistent.
Renamed to ReceiverPresentationServiceDelegateImpl - see other comment in
incognito_presentation_service_delegate.h
https://codereview.chromium.org/1314413005/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/tab_capture/offscreen_presentation.cc
(right):
https://codereview.chromium.org/1314413005/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/tab_capture/offscreen_presentation.cc:193:
one_ua_presentation_router_(
On 2015/09/17 00:09:20, miu wrote:
> IMO, you should not store a pointer to the router in this class. It is only
> used once at start-up, and once at destruction time to register/deregister.
>
> Incorporating mfoltz's comment below, the register call would look like:
>
> media_router::OneUAPresentationRouterFactory::
> GetOrCreateForBrowserContext(owner_->extension_web_contents()
> ->GetBrowserContext())
> ->RegisterPresenter(presentation_id(),
> presentation_web_contents_.get());
Done.
https://codereview.chromium.org/1314413005/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/tab_capture/offscreen_presentation.cc:248:
one_ua_presentation_router_->RegisterPresenter(presentation_id(), rfh_id);
On 2015/09/11 23:24:20, mark a. foltz wrote:
> Can RegisterPresenter() just take the (id, WebContents) and do the lookups
> internally?
Done.
https://codereview.chromium.org/1314413005/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/tab_capture/offscreen_presentation.cc:248:
one_ua_presentation_router_->RegisterPresenter(presentation_id(), rfh_id);
On 2015/09/17 00:09:20, miu wrote:
> +1 to mfoltz's comment.
>
> Also, is registering the presentation here the correct order-of-operations?
> Meaning, it seems like the presentation should be registered *before* the
> navigation is started (on L231-235 above).
Ok, looks like main frame should be initialized when WebContents is created, so
I don't have to wait for LoadURLWithParams.
Is LoadURLWithParams async though? i.e., any presentation api requests from the
offscreen tab won't reach the browser process until after this extension
function returns.
Anyhow, I changed it to call RegisterPresenter before LoadURLWithParams is
called.
https://codereview.chromium.org/1314413005/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/tab_capture/offscreen_presentation.cc:505:
one_ua_presentation_router_->UnregisterPresenter(rfh_id);
On 2015/09/11 23:24:20, mark a. foltz wrote:
> UnregisterPresentation(WebContents*)
Done.
https://codereview.chromium.org/1314413005/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/tab_capture/offscreen_presentation.h (right):
https://codereview.chromium.org/1314413005/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/tab_capture/offscreen_presentation.h:186: void
Close();
On 2015/09/17 00:09:20, miu wrote:
> On 2015/09/14 15:20:39, whywhat wrote:
> > nit: the name is very out of sync from the comment - either the comment
> reveals
> > implementation details or the name has to be changed to something along the
> > lines of it
>
> I think it would be reasonable to name this method Destroy(), which handles
> de-registering and deletes |this| before returning. This is the approach
> classes like content::RenderWidgetHostImpl take. Ex:
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...
Done.
https://codereview.chromium.org/1314413005/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/tab_capture/offscreen_presentation.h:186: void
Close();
On 2015/09/14 15:20:39, whywhat wrote:
> nit: the name is very out of sync from the comment - either the comment
reveals
> implementation details or the name has to be changed to something along the
> lines of it
Done.
https://codereview.chromium.org/1314413005/diff/1/chrome/browser/media/router...
File chrome/browser/media/router/create_presentation_session_request.h (right):
https://codereview.chromium.org/1314413005/diff/1/chrome/browser/media/router...
chrome/browser/media/router/create_presentation_session_request.h:57: bool
is_one_ua_presentation);
On 2015/09/11 23:24:20, mark a. foltz wrote:
> ISTM the "is_one_ua_presentation" is a flag that could be attached to the
> MediaRoute for the presentation, instead of plumbing it through here.
>
> Naming: is_offscreen_presentation
Done.
https://codereview.chromium.org/1314413005/diff/1/chrome/browser/media/router...
chrome/browser/media/router/create_presentation_session_request.h:57: bool
is_one_ua_presentation);
On 2015/09/14 15:20:39, whywhat wrote:
> On 2015/09/11 at 23:24:20, mark a. foltz wrote:
> > ISTM the "is_one_ua_presentation" is a flag that could be attached to the
> MediaRoute for the presentation, instead of plumbing it through here.
> >
> > Naming: is_offscreen_presentation
>
> +1
Done.
https://codereview.chromium.org/1314413005/diff/1/chrome/browser/media/router...
chrome/browser/media/router/create_presentation_session_request.h:57: bool
is_one_ua_presentation);
On 2015/09/17 00:09:20, miu wrote:
> +1 too, due to the fact that this bool argument is viral across dozens of
files.
> ;-)
Done.
https://codereview.chromium.org/1314413005/diff/1/chrome/browser/media/router...
File chrome/browser/media/router/incognito_presentation_service_delegate.h
(right):
https://codereview.chromium.org/1314413005/diff/1/chrome/browser/media/router...
chrome/browser/media/router/incognito_presentation_service_delegate.h:41: class
IncognitoPresentationServiceDelegate
On 2015/09/14 15:20:39, whywhat wrote:
> Maybe this shoud be something like ReceiverPresentationServiceDelegateImpl,
> Incognito/Offscreen are implementation details.
Ok, I will go with ReceiverPresentationServiceDelegateImpl for now. It looks
like we made some progress towards re-enabling MR in incognito mode, so if it
ends up working then we can probably merge the logic in this class into PSDImpl.
https://codereview.chromium.org/1314413005/diff/1/chrome/browser/media/router...
chrome/browser/media/router/incognito_presentation_service_delegate.h:41: class
IncognitoPresentationServiceDelegate
On 2015/09/17 00:09:20, miu wrote:
> On 2015/09/14 15:20:39, whywhat wrote:
> > Maybe this shoud be something like ReceiverPresentationServiceDelegateImpl,
> > Incognito/Offscreen are implementation details.
>
> FYI--Ben Kalman in recent meeting proposed we used the wording "sandboxed," so
> maybe "SandboxedPresentationServiceDelegate" would be a good name? Or,
> alternatively, the incognito profile impl class is called
"OffTheRecordProfile,"
> so maybe "OffTheRecordPresentationServiceDelegate?"
Will go with ReceiverPresentationServiceDelegateImpl.
https://codereview.chromium.org/1314413005/diff/1/chrome/browser/media/router...
chrome/browser/media/router/incognito_presentation_service_delegate.h:118:
content::WebContents* const web_contents_;
On 2015/09/14 15:20:39, whywhat wrote:
> Do you think there might be a BasePresentationServiceDelegateImpl that would
> hold the web_contents_ and any other common logic for example?
It looks like the only common logic is web_contents_. So I don't think it's
worth it.
https://codereview.chromium.org/1314413005/diff/1/chrome/browser/media/router...
File chrome/browser/media/router/one_ua_presentation_router.h (right):
https://codereview.chromium.org/1314413005/diff/1/chrome/browser/media/router...
chrome/browser/media/router/one_ua_presentation_router.h:25: class
OneUAPresentationRouter : public KeyedService {
On 2015/09/11 23:24:20, mark a. foltz wrote:
> Naming nitpicks:
>
> The term "One-UA" comes from the spec and while it is helpful to distinguish
the
> two cases, I feel like casual readers of the code won't know what it means.
>
> Also, this class is routing messages - but by calling it "Router" there might
be
> some confusion between it and the MediaRouter.
>
> I would propose naming this "OffscreenPresentationManager." Open to other
ideas
> as well.
https://codereview.chromium.org/1314413005/diff/1/chrome/browser/media/router...
chrome/browser/media/router/one_ua_presentation_router.h:25: class
OneUAPresentationRouter : public KeyedService {
On 2015/09/17 00:09:20, miu wrote:
> On 2015/09/11 23:24:20, mark a. foltz wrote:
> > Naming nitpicks:
> >
> > The term "One-UA" comes from the spec and while it is helpful to distinguish
> the
> > two cases, I feel like casual readers of the code won't know what it means.
> >
> > Also, this class is routing messages - but by calling it "Router" there
might
> be
> > some confusion between it and the MediaRouter.
> >
> > I would propose naming this "OffscreenPresentationManager." Open to other
> ideas
> > as well.
>
> Random OTOH replacements for the term "One UA Presentation":
>
> * CapturedPresentation (or MirroredPresentation, but IMO "Captured" is better)
> * SimulatedPresentation (because we're simulating a presentation page being
> "logically" rendered on a remote screen)
> * LocalInstancePresentation (or just LocalPresentation)
> * LocallyRenderedPresentation
Went with OffscreenPresentationManager so it's consistent with the existing
definitions. Only concern I have is that we already have a
OffscreenPresentationsOwner, but they're under different namespaces so it might
be ok?
https://codereview.chromium.org/1314413005/diff/1/chrome/browser/media/router...
chrome/browser/media/router/one_ua_presentation_router.h:45: // Returns true if
the frame given by |frame_id| is a presenter frame.
On 2015/09/11 23:24:20, mark a. foltz wrote:
> ISTM that registering a controller or presenter should return an object that
> acts as handle, say OffscreenPresenter and OffscreenController. Then you don't
> need to keep checking ids, you can just ask that object.
I think we can do that for controllers. but we might still require at least one
lookup for presenter, since it is registered in a different place than where it
is used. See top level comment.
https://codereview.chromium.org/1314413005/diff/1/chrome/browser/media/router...
chrome/browser/media/router/one_ua_presentation_router.h:59: void
GetSinglePresenterSession(
On 2015/09/11 23:24:20, mark a. foltz wrote:
> Can the Presenter object (I mentioned above) get a callback when a controller
is
> connected?
What I have is the that the Presenter object will expose presentation receiver
APIs such as this and getSessions(). Presenter will then delegate the call to
OffscreenPresentationManager, who does the bookkeeping. I have decided to do
this since OffscreenPresentationManager is probably the best place to handle
bookkeeping both sender/receiver data and coordination of callbacks.
https://codereview.chromium.org/1314413005/diff/1/chrome/browser/media/router...
chrome/browser/media/router/one_ua_presentation_router.h:69:
std::vector<content::PresentationSessionInfo> GetPresenterSessions(
On 2015/09/11 23:24:20, mark a. foltz wrote:
> The Presenter would know which sessions are connected?
Created Presenter object and exposed this API. But, it still asks the manager
for the list of sessions (see above comment)
https://codereview.chromium.org/1314413005/diff/1/chrome/browser/media/router...
chrome/browser/media/router/one_ua_presentation_router.h:108: struct
OneUAPresentationRoute {
On 2015/09/11 23:24:20, mark a. foltz wrote:
> Perhaps the OffscreenPresenter owns a list of OffscreenControllers?
So my model here is (name changed after this patchset):
OneUAPresentationSession corresponds to a PresentationSession object.
OneUAPresentationRoute represents a connection between a presenter
PresentationSession object and a controller PresentationSession object. Thus
there are multiple OneUAPresentationRoute objects for a presentation when there
are multiple controllers.
OneUAPresentationInfo contains all connections for that presentation.
I don't think we can support multiple controllers currently. See
crbug.com/529911. But once we do we can change L130 to hold multiple
OneUAPresentationRoutes.
https://codereview.chromium.org/1314413005/diff/1/chrome/browser/media/router...
File chrome/browser/media/router/presentation_service_delegate_impl.h (right):
https://codereview.chromium.org/1314413005/diff/1/chrome/browser/media/router...
chrome/browser/media/router/presentation_service_delegate_impl.h:108: void
GetPresenterSession(
On 2015/09/11 23:24:20, mark a. foltz wrote:
> FWIW, the terminology in the spec group is moving towards PresentationReceiver
> to designate the browsing context that hosts the presentation.
>
> https://github.com/w3c/presentation-api/pull/186
>
> So this could be GetPresentationReceiverSession...
Done.
https://codereview.chromium.org/1314413005/diff/1/chrome/browser/media/router...
chrome/browser/media/router/presentation_service_delegate_impl.h:187: const
MediaRoute::Id& route_id,
On 2015/09/11 23:24:20, mark a. foltz wrote:
> Can we pass the MediaRoute* here and ask if it's an offscreen presentation?
Done.
https://codereview.chromium.org/1314413005/diff/1/chrome/browser/media/router...
File chrome/browser/media/router/render_frame_host_helper.h (right):
https://codereview.chromium.org/1314413005/diff/1/chrome/browser/media/router...
chrome/browser/media/router/render_frame_host_helper.h:12: using
RenderFrameHostId = std::pair<int, int>;
On 2015/09/17 00:09:20, miu wrote:
> nit: Although they're likely always to be int's, it is documentative to define
> this as:
>
> using RenderFrameHostId =
> std::pair<decltype(content::RenderProcessHost::GetID()),
> decltype(content::RenderFrameHost::GetRoutingID())>;
Done. Since these are member functions it ends up being slightly more verbose
but I think it's ok.
https://codereview.chromium.org/1314413005/diff/1/content/public/browser/pres...
File content/public/browser/presentation_service_delegate.h (right):
https://codereview.chromium.org/1314413005/diff/1/content/public/browser/pres...
content/public/browser/presentation_service_delegate.h:189: int
render_process_id,
On 2015/09/17 00:09:20, miu wrote:
> BTW--Most of the methods in this interface have these two render_process_id
and
> render_frame_id arguments. Is it ever possible to use this API on non-main
> render frames? If not, you may want to clarify by naming the 2nd arg
> main_render_frame_id and also DCHECK()ing in the method impls.
It's definitely possible to use it on non main frames. Any frame can invoke the
presentation API.
IMO, the refactoring for PS3 is over-engineered. Three main general points: 1. Adding layers of indirection and lots of lines code that add no functional value just makes things confusing. It also adds a heavy burden on future maintainers of the code. For example, a simple change, such as adding a data argument to a method call, now has to be made in several places instead of one. 2. The naming of these various classes is very confusing. At first, I approached the PS3 code trying to think up more-accurate naming to aid readability, and then realized this was futile because the classes themselves provided little-to-no functional value. 3. The added complexity isn't worth it, just to narrow-down the list of public methods. These classes aren't interfaces that 20+ code modules will depend on (e.g., content/public/... interfaces). Usually, there are only one or two classes that depend on them, all in the same dir or even the same file. https://codereview.chromium.org/1314413005/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/tab_capture/offscreen_presentation.cc (right): https://codereview.chromium.org/1314413005/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/tab_capture/offscreen_presentation.cc:248: one_ua_presentation_router_->RegisterPresenter(presentation_id(), rfh_id); On 2015/09/26 01:21:56, imcheng1 wrote: > On 2015/09/17 00:09:20, miu wrote: > > Also, is registering the presentation here the correct order-of-operations? > > Is LoadURLWithParams async though? i.e., any presentation api requests from the > offscreen tab won't reach the browser process until after this extension > function returns. Probably not, but there are multiple processes and threads involved. Might as well pretend the worst-case raciness is possible (even if it totally is not) and just move the lines of code up a bit. It'll also help make this code more robust against future changes that could introduce raciness. https://codereview.chromium.org/1314413005/diff/1/chrome/browser/media/router... File chrome/browser/media/router/one_ua_presentation_router.h (right): https://codereview.chromium.org/1314413005/diff/1/chrome/browser/media/router... chrome/browser/media/router/one_ua_presentation_router.h:25: class OneUAPresentationRouter : public KeyedService { On 2015/09/26 01:21:57, imcheng1 wrote: > On 2015/09/17 00:09:20, miu wrote: > > On 2015/09/11 23:24:20, mark a. foltz wrote: > > > Naming nitpicks: > > > > > > The term "One-UA" comes from the spec and while it is helpful to distinguish > > the > > > two cases, I feel like casual readers of the code won't know what it means. > > > > > > Also, this class is routing messages - but by calling it "Router" there > might > > be > > > some confusion between it and the MediaRouter. > > > > > > I would propose naming this "OffscreenPresentationManager." Open to other > > ideas > > > as well. > > > > Random OTOH replacements for the term "One UA Presentation": > > > > * CapturedPresentation (or MirroredPresentation, but IMO "Captured" is better) > > * SimulatedPresentation (because we're simulating a presentation page being > > "logically" rendered on a remote screen) > > * LocalInstancePresentation (or just LocalPresentation) > > * LocallyRenderedPresentation > > Went with OffscreenPresentationManager so it's consistent with the existing > definitions. Only concern I have is that we already have a > OffscreenPresentationsOwner, but they're under different namespaces so it might > be ok? Yes, because I am going to rename the OffscreenPresentation and OffscreenPresentationsOwner classes to reflect the fact that the extension API is going public for a general off-screen tab case. The classes will likely be renamed to something like SandboxedOffscreenTab. See TODO comment I added in my CL (first change in this file): https://codereview.chromium.org/1314413005/diff/40001/chrome/browser/extensio... https://codereview.chromium.org/1314413005/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/tab_capture/offscreen_presentation.h (right): https://codereview.chromium.org/1314413005/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/tab_capture/offscreen_presentation.h:189: // Gets the OffscreenPresentationManager instance associated with the To be consistent with the naming and comments in the rest of this module, could you make this comment read: // Gets the OPM instance associated with the extension's BrowserContext. https://codereview.chromium.org/1314413005/diff/40001/chrome/browser/media/ro... File chrome/browser/media/router/media_route.h (right): https://codereview.chromium.org/1314413005/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/media_route.h:41: MediaRoute(const MediaRoute::Id& media_route_id, 8 arguments?!? IMO, you badly need to make this 3 arguments or less and initialize the other data members to common default values. Then, introduce setter methods and pass const MediaRoutes around if you're worried about other code trying to change these objects. https://codereview.chromium.org/1314413005/diff/40001/chrome/browser/media/ro... File chrome/browser/media/router/offscreen_presentation_manager.cc (right): https://codereview.chromium.org/1314413005/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/offscreen_presentation_manager.cc:89: const RenderFrameHostId& frame_id, 1. |frame_id| argument is not used, so can you remove it? 2. Is there a reason the caller of this method can't just call the manager's ListenForMessages() method directly? https://codereview.chromium.org/1314413005/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/offscreen_presentation_manager.cc:105: const content::PresentationSessionInfo& session = listener->GetSessionInfo(); These three lines need to be wrapped in a #ifndef NDEBUG...#endif since this statement should not execute in release builds either. https://codereview.chromium.org/1314413005/diff/40001/chrome/browser/media/ro... File chrome/browser/media/router/offscreen_presentation_manager.h (right): https://codereview.chromium.org/1314413005/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/offscreen_presentation_manager.h:28: // Exposes APIs to get presentation receiver sessions and operate on receiver IMO, this class should be deleted. It adds too little functional value to justify the extra indirection (and extra 100+ LOC). FWICT, ReceiverPresentationServiceDelegateImpl should just call methods of the OffscreenPresentationManager directly. https://codereview.chromium.org/1314413005/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/offscreen_presentation_manager.h:40: void GetPresentationReceiverSession( These two methods have almost the exact same name, and also seem to differ a lot in behavior (an async callback versus a sync return value). Why is that? Should their names be more different to reflect their major differences? How about some comments explaining what they do and how they are meant to be used? https://codereview.chromium.org/1314413005/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/offscreen_presentation_manager.h:78: // Exposes APIs to operate on a single controller session for an offscreen This class doesn't expose APIs. It declares public methods, and it's redundant to say that. ;-) https://codereview.chromium.org/1314413005/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/offscreen_presentation_manager.h:85: class OffscreenController { This class doesn't include any control logic. It's a facade class, providing messaging capability to/from an off-screen tab. Consider re-naming it to something like OffscreenTabPresentation, and adjusting comments and naming around that. (The PresenterFrame that uses this class would have the role of the "controller" here.) https://codereview.chromium.org/1314413005/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/offscreen_presentation_manager.h:112: class OffscreenPresentationManager : public KeyedService { Naming: "Manager" is so abstract. How about "OffscreenTabMessageExchanger" or something to that effect? https://codereview.chromium.org/1314413005/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/offscreen_presentation_manager.h:153: struct OffscreenPresentationSessionInfo { Consider merging this into OffscreenController. This will require you to change the ownership semantics around OffscreenController (e.g., RegisterController might return a WeakPtr<OffscreenController> instead), but will result in fewer moving parts and simpler code. https://codereview.chromium.org/1314413005/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/offscreen_presentation_manager.h:168: struct OffscreenPresentationRoute { Suggestion: Delete this class, and move |presenter| and |controller| into the struct below. https://codereview.chromium.org/1314413005/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/offscreen_presentation_manager.h:179: struct OffscreenPresentationInfo { naming: "Info" isn't accurate. https://codereview.chromium.org/1314413005/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/offscreen_presentation_manager.h:182: bool IsEmpty() const; naming: How about HasRoutedPeers()? (This suggestion inverts the boolean.) https://codereview.chromium.org/1314413005/diff/40001/chrome/browser/media/ro... File chrome/browser/media/router/presentation_service_delegate_impl.cc (right): https://codereview.chromium.org/1314413005/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:75: const RenderFrameHostId& render_frame_host_id, I assume this argument refers to a specific RenderFrame within the WebContents and is not necessarily the main frame? You should document this. nit: Consider swapping the 1st and 2nd argument since the RFH is contained by the WC. https://codereview.chromium.org/1314413005/diff/40001/chrome/browser/media/ro... File chrome/browser/media/router/receiver_presentation_service_delegate_impl.cc (right): https://codereview.chromium.org/1314413005/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/receiver_presentation_service_delegate_impl.cc:53: bool ReceiverPresentationServiceDelegateImpl::AddScreenAvailabilityListener( There are a ton of "NOTIMPLEMENTED()" methods. Consider moving them into the superclass as default implementations of the interface. https://codereview.chromium.org/1314413005/diff/40001/chrome/browser/media/ro... File chrome/browser/media/router/receiver_presentation_service_delegate_impl.h (right): https://codereview.chromium.org/1314413005/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/receiver_presentation_service_delegate_impl.h:42: class ReceiverPresentationServiceDelegateImpl naming: Personally, I disagree with the other reviewer saying the "Impl" was needed here. Also, "Receiver" doesn't feel very accurate. How about: OffscreenTabPresentationServiceDelegate?
Thanks for the feedback Yuri. Reading the code again I do agree the many of changes added in ps3 are unnecessary. I will be working in a new patchset incorporating your feedback. I have some ideas on how to simplify the ownership / data model as well.
Patchset #4 (id:60001) has been deleted
PTAL. Sorry it took so long. Based on the feedback (which was very useful), I took some time to think of a new design, which I think it a lot cleaner now. I have also added comments which explains how it works. https://codereview.chromium.org/1314413005/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/tab_capture/offscreen_presentation.cc (right): https://codereview.chromium.org/1314413005/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/tab_capture/offscreen_presentation.cc:248: one_ua_presentation_router_->RegisterPresenter(presentation_id(), rfh_id); On 2015/09/27 00:22:08, miu wrote: > On 2015/09/26 01:21:56, imcheng1 wrote: > > On 2015/09/17 00:09:20, miu wrote: > > > Also, is registering the presentation here the correct order-of-operations? > > > > Is LoadURLWithParams async though? i.e., any presentation api requests from > the > > offscreen tab won't reach the browser process until after this extension > > function returns. > > Probably not, but there are multiple processes and threads involved. Might as > well pretend the worst-case raciness is possible (even if it totally is not) and > just move the lines of code up a bit. It'll also help make this code more > robust against future changes that could introduce raciness. Acknowledged. Now I call ReceiverPresentationServiceDelegate::CreateForWebContents (which calls RegisterReceiverTab) before navigation. https://codereview.chromium.org/1314413005/diff/1/chrome/browser/media/router... File chrome/browser/media/router/one_ua_presentation_router.h (right): https://codereview.chromium.org/1314413005/diff/1/chrome/browser/media/router... chrome/browser/media/router/one_ua_presentation_router.h:25: class OneUAPresentationRouter : public KeyedService { On 2015/09/27 00:22:08, miu wrote: > On 2015/09/26 01:21:57, imcheng1 wrote: > > On 2015/09/17 00:09:20, miu wrote: > > > On 2015/09/11 23:24:20, mark a. foltz wrote: > > > > Naming nitpicks: > > > > > > > > The term "One-UA" comes from the spec and while it is helpful to > distinguish > > > the > > > > two cases, I feel like casual readers of the code won't know what it > means. > > > > > > > > Also, this class is routing messages - but by calling it "Router" there > > might > > > be > > > > some confusion between it and the MediaRouter. > > > > > > > > I would propose naming this "OffscreenPresentationManager." Open to other > > > ideas > > > > as well. > > > > > > Random OTOH replacements for the term "One UA Presentation": > > > > > > * CapturedPresentation (or MirroredPresentation, but IMO "Captured" is > better) > > > * SimulatedPresentation (because we're simulating a presentation page being > > > "logically" rendered on a remote screen) > > > * LocalInstancePresentation (or just LocalPresentation) > > > * LocallyRenderedPresentation > > > > Went with OffscreenPresentationManager so it's consistent with the existing > > definitions. Only concern I have is that we already have a > > OffscreenPresentationsOwner, but they're under different namespaces so it > might > > be ok? > > Yes, because I am going to rename the OffscreenPresentation and > OffscreenPresentationsOwner classes to reflect the fact that the extension API > is going public for a general off-screen tab case. The classes will likely be > renamed to something like SandboxedOffscreenTab. > > See TODO comment I added in my CL (first change in this file): > https://codereview.chromium.org/1314413005/diff/40001/chrome/browser/extensio... Acknowledged. https://codereview.chromium.org/1314413005/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/tab_capture/offscreen_presentation.h (right): https://codereview.chromium.org/1314413005/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/tab_capture/offscreen_presentation.h:189: // Gets the OffscreenPresentationManager instance associated with the On 2015/09/27 00:22:08, miu wrote: > To be consistent with the naming and comments in the rest of this module, could > you make this comment read: > > // Gets the OPM instance associated with the extension's BrowserContext. Code removed. https://codereview.chromium.org/1314413005/diff/40001/chrome/browser/media/ro... File chrome/browser/media/router/media_route.h (right): https://codereview.chromium.org/1314413005/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/media_route.h:41: MediaRoute(const MediaRoute::Id& media_route_id, On 2015/09/27 00:22:08, miu wrote: > 8 arguments?!? IMO, you badly need to make this 3 arguments or less and > initialize the other data members to common default values. Then, introduce > setter methods and pass const MediaRoutes around if you're worried about other > code trying to change these objects. I have removed the arg from the ctor and added a setter. For longer term I want to see if we can do a builder pattern so tests don't have to specify the other common args. The only place in production code that uses it actually already have all the args. https://codereview.chromium.org/1314413005/diff/40001/chrome/browser/media/ro... File chrome/browser/media/router/offscreen_presentation_manager.cc (right): https://codereview.chromium.org/1314413005/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/offscreen_presentation_manager.cc:89: const RenderFrameHostId& frame_id, On 2015/09/27 00:22:08, miu wrote: > 1. |frame_id| argument is not used, so can you remove it? > > 2. Is there a reason the caller of this method can't just call the manager's > ListenForMessages() method directly? 1. Removed. Also removed some other unused fields to keep it clean. 2. no longer applicable since message routing no longer happens in the manager but in OffscreenPresentationRoute (which isn't exposed publicly) https://codereview.chromium.org/1314413005/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/offscreen_presentation_manager.cc:105: const content::PresentationSessionInfo& session = listener->GetSessionInfo(); On 2015/09/27 00:22:08, miu wrote: > These three lines need to be wrapped in a #ifndef NDEBUG...#endif since this > statement should not execute in release builds either. Removed https://codereview.chromium.org/1314413005/diff/40001/chrome/browser/media/ro... File chrome/browser/media/router/offscreen_presentation_manager.h (right): https://codereview.chromium.org/1314413005/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/offscreen_presentation_manager.h:28: // Exposes APIs to get presentation receiver sessions and operate on receiver On 2015/09/27 00:22:08, miu wrote: > IMO, this class should be deleted. It adds too little functional value to > justify the extra indirection (and extra 100+ LOC). FWICT, > ReceiverPresentationServiceDelegateImpl should just call methods of the > OffscreenPresentationManager directly. Done. https://codereview.chromium.org/1314413005/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/offscreen_presentation_manager.h:40: void GetPresentationReceiverSession( On 2015/09/27 00:22:08, miu wrote: > These two methods have almost the exact same name, and also seem to differ a lot > in behavior (an async callback versus a sync return value). Why is that? > Should their names be more different to reflect their major differences? How > about some comments explaining what they do and how they are meant to be used? I have removed these functions from here. I will document their behaviors in PresentationServiceDelegate. https://codereview.chromium.org/1314413005/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/offscreen_presentation_manager.h:78: // Exposes APIs to operate on a single controller session for an offscreen On 2015/09/27 00:22:08, miu wrote: > This class doesn't expose APIs. It declares public methods, and it's redundant > to say that. ;-) Updated comment. https://codereview.chromium.org/1314413005/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/offscreen_presentation_manager.h:85: class OffscreenController { On 2015/09/27 00:22:08, miu wrote: > This class doesn't include any control logic. It's a facade class, providing > messaging capability to/from an off-screen tab. Consider re-naming it to > something like OffscreenTabPresentation, and adjusting comments and naming > around that. > > (The PresenterFrame that uses this class would have the role of the "controller" > here.) As discussed offline yesterday, I have reworked this class to give it more role of handling the callbacks by shifting the logic from OffscreenPresentationManager to this class. https://codereview.chromium.org/1314413005/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/offscreen_presentation_manager.h:112: class OffscreenPresentationManager : public KeyedService { On 2015/09/27 00:22:08, miu wrote: > Naming: "Manager" is so abstract. How about "OffscreenTabMessageExchanger" or > something to that effect? Not sure if it's a better name, since message exchange don't happen directly in this class anymore. Now that this class is mostly bookkeeping and coordination between controllers and receiver, how about OffscreenPresentationRegistry? https://codereview.chromium.org/1314413005/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/offscreen_presentation_manager.h:153: struct OffscreenPresentationSessionInfo { On 2015/09/27 00:22:08, miu wrote: > Consider merging this into OffscreenController. This will require you to change > the ownership semantics around OffscreenController (e.g., RegisterController > might return a WeakPtr<OffscreenController> instead), but will result in fewer > moving parts and simpler code. I have moved the callback and listener fields what used to be known as OffscreenController, which is now a RAII-ish class created by OffscreenPresentationManager and owned by the PresentationServiceDelegate implementations. https://codereview.chromium.org/1314413005/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/offscreen_presentation_manager.h:168: struct OffscreenPresentationRoute { On 2015/09/27 00:22:08, miu wrote: > Suggestion: Delete this class, and move |presenter| and |controller| into the > struct below. I have added support for multiple controllers for offscreen presentations in this class. We will have multiple OffscreenPresentationRoutes in what used to be known as OffscreenPresentationInfo. https://codereview.chromium.org/1314413005/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/offscreen_presentation_manager.h:179: struct OffscreenPresentationInfo { On 2015/09/27 00:22:08, miu wrote: > naming: "Info" isn't accurate. Renamed to OffscreenPresentation. https://codereview.chromium.org/1314413005/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/offscreen_presentation_manager.h:182: bool IsEmpty() const; On 2015/09/27 00:22:08, miu wrote: > naming: How about HasRoutedPeers()? (This suggestion inverts the boolean.) Removed function. https://codereview.chromium.org/1314413005/diff/40001/chrome/browser/media/ro... File chrome/browser/media/router/presentation_service_delegate_impl.cc (right): https://codereview.chromium.org/1314413005/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:75: const RenderFrameHostId& render_frame_host_id, On 2015/09/27 00:22:08, miu wrote: > I assume this argument refers to a specific RenderFrame within the WebContents > and is not necessarily the main frame? You should document this. > > nit: Consider swapping the 1st and 2nd argument since the RFH is contained by > the WC. Done. https://codereview.chromium.org/1314413005/diff/40001/chrome/browser/media/ro... File chrome/browser/media/router/receiver_presentation_service_delegate_impl.cc (right): https://codereview.chromium.org/1314413005/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/receiver_presentation_service_delegate_impl.cc:53: bool ReceiverPresentationServiceDelegateImpl::AddScreenAvailabilityListener( On 2015/09/27 00:22:08, miu wrote: > There are a ton of "NOTIMPLEMENTED()" methods. Consider moving them into the > superclass as default implementations of the interface. Ack. I think it's not worth the trouble since all of them are pure virtual methods and it's only unimplemented in this version. https://codereview.chromium.org/1314413005/diff/40001/chrome/browser/media/ro... File chrome/browser/media/router/receiver_presentation_service_delegate_impl.h (right): https://codereview.chromium.org/1314413005/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/receiver_presentation_service_delegate_impl.h:42: class ReceiverPresentationServiceDelegateImpl On 2015/09/27 00:22:09, miu wrote: > naming: Personally, I disagree with the other reviewer saying the "Impl" was > needed here. Also, "Receiver" doesn't feel very accurate. How about: > OffscreenTabPresentationServiceDelegate? I don't feel strongly either way regarding Impl. I think "Receiver" is fine, since this version only implements the receiver APIs. I think it can be argued either way.
One round of review comments focused on the modified, not the new files. Mostly minor requests. I will make another pass and try to assess how it all fits together. https://codereview.chromium.org/1314413005/diff/80001/chrome/browser/chrome_c... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/1314413005/diff/80001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:2559: if (web_contents->GetBrowserContext()->IsOffTheRecord()) { Does this assume that all incognito webcontents are presentations? Maybe we could add a webcontents->IsPresentation() method to make this clearer. https://codereview.chromium.org/1314413005/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/presentation_service_delegate_impl.cc (right): https://codereview.chromium.org/1314413005/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:120: OffscreenPresentationSession* FindOffscreenSession( Can you document this method? Which session is returned, and what if the presentation_id is not an offscreen presentation? https://codereview.chromium.org/1314413005/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:150: // the associated BrowserContext. I'm assuming that the BC outlives this object; can you make sure that is clear in comments for this class? https://codereview.chromium.org/1314413005/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:165: OffscreenPresentationManagerFactory::GetOrCreateForBrowserContext( Do we need to look this up for every presentation frame, or can we pass it in? https://codereview.chromium.org/1314413005/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/presentation_service_delegate_impl.h (right): https://codereview.chromium.org/1314413005/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.h:107: void GetPresentationReceiverSession( Do we need both this and the GetSessions method below? If so, please add some comments explaining the difference. https://codereview.chromium.org/1314413005/diff/80001/content/browser/present... File content/browser/presentation/presentation_service_impl.cc (right): https://codereview.chromium.org/1314413005/diff/80001/content/browser/present... content/browser/presentation/presentation_service_impl.cc:470: new GetPresentationReceiverSessionMojoCallback(callback)); Do we need to wrap the passed-in callback in another callback? Can we just pass it as-is to OnGetPresentationReceiverSession using base::bind? Or is it not refcounted? https://codereview.chromium.org/1314413005/diff/80001/content/browser/present... File content/browser/presentation/presentation_service_impl.h (right): https://codereview.chromium.org/1314413005/diff/80001/content/browser/present... content/browser/presentation/presentation_service_impl.h:109: using GetPresentationReceiverSessionMojoCallback = Can we combine this with DefaultSessionMojoCallback (perhaps as PresentationSessionMojoCallback)?
I was able to take a look at everything except the receiver_presentation_service_delgate files before I had to go. Hopefully these will give you something to do before I get back :-) https://codereview.chromium.org/1314413005/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/offscreen_presentation_manager.cc (right): https://codereview.chromium.org/1314413005/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/offscreen_presentation_manager.cc:54: OnMessageReceived(scoped_ptr<content::PresentationSessionMessage> message) { IIUC the PresentationService gets batches of messages. Can we propagate them in batches here? https://code.google.com/p/chromium/codesearch#chromium/src/content/common/pre... https://codereview.chromium.org/1314413005/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/offscreen_presentation_manager.cc:67: DCHECK(!ContainsKey(offscreen_presentations_, presentation_id)); Well this will be a no-op in release builds. Maybe DLOG(WARNING) and return early instead? https://codereview.chromium.org/1314413005/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/offscreen_presentation_manager.cc:78: DCHECK(it != offscreen_presentations_.end()); Similar comment here - it would be better to DLOG and return rather than dereference the invalid iterator below. https://codereview.chromium.org/1314413005/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/offscreen_presentation_manager.cc:96: void OffscreenPresentationManager::RemovePresentation( This must only be called on presentations that are completely disconnected and about to be torn down, right? https://codereview.chromium.org/1314413005/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/offscreen_presentation_manager.cc:107: controller_(nullptr), This class would be simpler by accepting a non-null controller_ and receiver_ in the ctor. https://codereview.chromium.org/1314413005/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/offscreen_presentation_manager.cc:133: if (is_controller) { This would seem simpler as: if (is_controller && controller_) { controller_->OnSessionStateChanged() return; } etc. https://codereview.chromium.org/1314413005/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/offscreen_presentation_manager.cc:156: OffscreenPresentationManager::OffscreenPresentationSession* other_session = Help me understand - the connection will always have ptrs to the controller and receiver but the is_controller flag controls which one will be used? IMO adding methods like GetSession() and GetRemoteSession() would be more readable, e.g. this becomes GetRemoteSession()->OnMessageReceived() https://codereview.chromium.org/1314413005/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/offscreen_presentation_manager.cc:198: controller_frame_id, this)); ISTM the connection should create and own the receiver_session and controller_session. This would seem to avoid a lot of the null checking above as well as the need for the Init() call below. https://codereview.chromium.org/1314413005/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/offscreen_presentation_manager.cc:214: void OffscreenPresentationManager::OffscreenPresentation::RemoveConnection( What guarantees that the two sessions are in the disconnected state before they are removed from the controller/presentation? https://codereview.chromium.org/1314413005/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/offscreen_presentation_manager.h (right): https://codereview.chromium.org/1314413005/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/offscreen_presentation_manager.h:20: // Acts as an intermediary between the tab that hosts an offscreen presentation Nit: there's no "tab" per se just a WebContents container. Also: There's no guarantee that the presentation has to outlive navigation. (The spec is vague on this.) https://codereview.chromium.org/1314413005/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/offscreen_presentation_manager.h:29: // Receiver tab is created to host the offscreen presentation and registers It's not clear how the receiver WebContents is used in the example below. The example is just registering a presentation_id. https://codereview.chromium.org/1314413005/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/offscreen_presentation_manager.h:38: // void OnSessionAvailable( Can this be called more than once as additional controllers are connected? https://codereview.chromium.org/1314413005/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/offscreen_presentation_manager.h:48: // scoped_ptr<OffscreenPresentationSession> controller_session = The comments should make it clear that the OPS is used by the receiver WebContents; the controller WebContents already has created a PresentationSession for the connection (represented by |session_info| I assume) https://codereview.chromium.org/1314413005/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/offscreen_presentation_manager.h:61: // manager->UnregisterOffscreenPresentationReceiver(presentation_id); This would be called in response to PresentationSession.terminate(), or a request by the user or the MRP to kill the associated route. I would imagine that the manager would also observe the WebContents and unregister the presentation on window.close() or navigation. https://codereview.chromium.org/1314413005/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/offscreen_presentation_manager.h:62: // Threading constraints need to be discussed somewhere in here. https://codereview.chromium.org/1314413005/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/offscreen_presentation_manager.h:80: // Instances must not outlive the OffscreenPresentationManager instance that Does the manager keep track of the live sessions/presentations? Will the manager live forever once created, or can it be destroyed and re-created on demand? https://codereview.chromium.org/1314413005/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/offscreen_presentation_manager.h:132: // to the presentation. What happens to pending callbacks passed into Register*Receiver()? https://codereview.chromium.org/1314413005/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/offscreen_presentation_manager.h:146: CreateOffscreenPresentationConnection( Nit: Might name this ConnectToOffscreenPresentation to lead with the verb. https://codereview.chromium.org/1314413005/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/offscreen_presentation_manager.h:148: const RenderFrameHostId& controller_frame_id); It seems a little asymmetrical for the controller to be able to connect synchronously, while the receiver must pass a callback. What state is the session in when it is returned? https://codereview.chromium.org/1314413005/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/offscreen_presentation_manager.h:160: void RemoveSession(bool is_controller); What does is_controller mean? Does this mean the caller is a controller or it's trying to remove a session created by a controller? In general I prefer APIs that are explicit instead of using boolean flags, so instead: RemoveControllerSession() RemoveReceiverSession() https://codereview.chromium.org/1314413005/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/offscreen_presentation_manager.h:161: void SendMessage(bool is_controller, SendMessageToController() SendMessageToReceiver() What would happen if a controller called this with is_controller = false? https://codereview.chromium.org/1314413005/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/offscreen_presentation_manager.h:176: // Not owned by this class. Destruction of either would seem to require that this is also destroyed, otherwise, one end of the connection is dangling and that doesn't seem useful. What enforces that? https://codereview.chromium.org/1314413005/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/offscreen_presentation_manager.h:186: // Contains callback to the receiver to inform it of new connections Called only once, or once per connected controller? https://codereview.chromium.org/1314413005/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/offscreen_presentation_manager.h:196: scoped_ptr<OffscreenPresentationSession> AddConnectionAndNotifyReceiver( Is this called when a new controller connects? Maybe OnControllerConnected() https://codereview.chromium.org/1314413005/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/offscreen_presentation_manager.h:198: void OnReceiverDetached(); What is a "detached" receiver? Do you mean controller? Maybe OnControllerDisconnected()? https://codereview.chromium.org/1314413005/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/offscreen_presentation_manager_factory.cc (right): https://codereview.chromium.org/1314413005/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/offscreen_presentation_manager_factory.cc:41: OffscreenPresentationManagerFactory::GetBrowserContextToUse( Will this return the same OPM for a profile and any incognito context spawned from it? Since we are creating a new browser context for each offscreen tab, will each get its own OPM?
PTAL https://codereview.chromium.org/1314413005/diff/80001/chrome/browser/chrome_c... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/1314413005/diff/80001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:2559: if (web_contents->GetBrowserContext()->IsOffTheRecord()) { On 2015/10/01 06:25:28, mark a. foltz wrote: > Does this assume that all incognito webcontents are presentations? Maybe we > could add a webcontents->IsPresentation() method to make this clearer. Not all incognito WebContents are presentations. We only create ReceiverPresentationServiceDelegateImpl for a WebContents in offscreen_presentation.cc. So this function will still return nullptr for non-receiver incognito WebContents . https://codereview.chromium.org/1314413005/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/offscreen_presentation_manager.cc (right): https://codereview.chromium.org/1314413005/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/offscreen_presentation_manager.cc:54: OnMessageReceived(scoped_ptr<content::PresentationSessionMessage> message) { On 2015/10/01 18:39:12, mark a. foltz wrote: > IIUC the PresentationService gets batches of messages. Can we propagate them in > batches here? > > https://code.google.com/p/chromium/codesearch#chromium/src/content/common/pre... > > Yes. I added a TODO here. I want to make sure it works on the high level before adding more logic. https://codereview.chromium.org/1314413005/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/offscreen_presentation_manager.cc:67: DCHECK(!ContainsKey(offscreen_presentations_, presentation_id)); On 2015/10/01 18:39:12, mark a. foltz wrote: > Well this will be a no-op in release builds. Maybe DLOG(WARNING) and return > early instead? insert() will also be no-op if there is already an existing entry. Or, do you mean we should log in release build? https://codereview.chromium.org/1314413005/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/offscreen_presentation_manager.cc:78: DCHECK(it != offscreen_presentations_.end()); On 2015/10/01 18:39:12, mark a. foltz wrote: > Similar comment here - it would be better to DLOG and return rather than > dereference the invalid iterator below. We are always deleting the OffscreenPresentation here by calling erase(presentation_id) so it's not an issue anymore. https://codereview.chromium.org/1314413005/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/offscreen_presentation_manager.cc:96: void OffscreenPresentationManager::RemovePresentation( On 2015/10/01 18:39:12, mark a. foltz wrote: > This must only be called on presentations that are completely disconnected and > about to be torn down, right? This function is removed. But that is correct - I added a DCHECK in ~OffscreenPresentation to check offscreen_presentations_ is empty. Also added contract for UnregisterOffscreenPresentationReceiver that all connections must be destroyed before calling it. https://codereview.chromium.org/1314413005/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/offscreen_presentation_manager.cc:107: controller_(nullptr), On 2015/10/01 18:39:12, mark a. foltz wrote: > This class would be simpler by accepting a non-null controller_ and receiver_ in > the ctor. The issue is that OffscreenPresentationConnection and OffscreenPresentationSession reference each other. So to do that I would have to set the Connection* separately on the sessions. Another way is to construct the Sessions here, then call a method to get the Session*'s, then wrap them in scoped_ptr<>s and return them to the controller and receiver. After weighing all 3 I went with what I have now. https://codereview.chromium.org/1314413005/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/offscreen_presentation_manager.cc:133: if (is_controller) { On 2015/10/01 18:39:12, mark a. foltz wrote: > This would seem simpler as: > > if (is_controller && controller_) { > controller_->OnSessionStateChanged() > return; > } > > etc. Ack https://codereview.chromium.org/1314413005/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/offscreen_presentation_manager.cc:156: OffscreenPresentationManager::OffscreenPresentationSession* other_session = On 2015/10/01 18:39:12, mark a. foltz wrote: > Help me understand - the connection will always have ptrs to the controller and > receiver but the is_controller flag controls which one will be used? > > IMO adding methods like GetSession() and GetRemoteSession() would be more > readable, e.g. this becomes GetRemoteSession()->OnMessageReceived() Went with your other comment in the .h - SendMessageTo{Controller,Receiver}, etc. https://codereview.chromium.org/1314413005/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/offscreen_presentation_manager.cc:198: controller_frame_id, this)); On 2015/10/01 18:39:12, mark a. foltz wrote: > ISTM the connection should create and own the receiver_session and > controller_session. > > This would seem to avoid a lot of the null checking above as well as the need > for the Init() call below. Per comment above, I could new these Session objects in the Connection's ctor (cleanest way get around the circular reference), but then I would have to get the Sessions back as raw ptrs, wrap them in scoped_ptr<>s to return them to controller/receiver. It just feels a bit weird to me. https://codereview.chromium.org/1314413005/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/offscreen_presentation_manager.cc:214: void OffscreenPresentationManager::OffscreenPresentation::RemoveConnection( On 2015/10/01 18:39:12, mark a. foltz wrote: > What guarantees that the two sessions are in the disconnected state before they > are removed from the controller/presentation? In the new patchset, this is only called from OffscreenPresentationConnection when one side has been removed and other has been notified of disconnection. https://codereview.chromium.org/1314413005/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/offscreen_presentation_manager.h (right): https://codereview.chromium.org/1314413005/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/offscreen_presentation_manager.h:20: // Acts as an intermediary between the tab that hosts an offscreen presentation On 2015/10/01 18:39:12, mark a. foltz wrote: > Nit: there's no "tab" per se just a WebContents container. > > Also: There's no guarantee that the presentation has to outlive navigation. > (The spec is vague on this.) Ok, I will replace "tab" with "WebContents". Regarding navigation, that is my understanding as well. The current implementation is that controllers will be disconnected if receiver navigates with a chance to reconnect. https://codereview.chromium.org/1314413005/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/offscreen_presentation_manager.h:29: // Receiver tab is created to host the offscreen presentation and registers On 2015/10/01 18:39:13, mark a. foltz wrote: > It's not clear how the receiver WebContents is used in the example below. > The example is just registering a presentation_id. Actually, the receiver side of OPM is not necessarily tied to WebContents. It just so happens for offscreen tabs that it is. I will just refer to them as "receivers". https://codereview.chromium.org/1314413005/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/offscreen_presentation_manager.h:38: // void OnSessionAvailable( On 2015/10/01 18:39:12, mark a. foltz wrote: > Can this be called more than once as additional controllers are connected? Yes. In fact this provides a pretty straightforward implementation of onsessionavailable. https://codereview.chromium.org/1314413005/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/offscreen_presentation_manager.h:48: // scoped_ptr<OffscreenPresentationSession> controller_session = On 2015/10/01 18:39:12, mark a. foltz wrote: > The comments should make it clear that the OPS is used by the receiver > WebContents; the controller WebContents already has created a > PresentationSession for the connection (represented by |session_info| I assume) For symmetry, Controllers also use OffscrenPresentationSession to communicate with receiver and they are created by calling this function. I've updated comments here so the function only takes a presentation id and frame id. PresentationSessionInfo is just a POD. It only contains presentation url and id. https://codereview.chromium.org/1314413005/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/offscreen_presentation_manager.h:61: // manager->UnregisterOffscreenPresentationReceiver(presentation_id); On 2015/10/01 18:39:13, mark a. foltz wrote: > This would be called in response to PresentationSession.terminate(), or a > request by the user or the MRP to kill the associated route. > > I would imagine that the manager would also observe the WebContents and > unregister the presentation on window.close() or navigation. Correct. Right now this is called on ~ReceiverPresentationServiceDelegate, which is called when receiver WebContents is being destroyed. https://codereview.chromium.org/1314413005/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/offscreen_presentation_manager.h:62: // On 2015/10/01 18:39:12, mark a. foltz wrote: > Threading constraints need to be discussed somewhere in here. Done. https://codereview.chromium.org/1314413005/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/offscreen_presentation_manager.h:80: // Instances must not outlive the OffscreenPresentationManager instance that On 2015/10/01 18:39:12, mark a. foltz wrote: > Does the manager keep track of the live sessions/presentations? > Will the manager live forever once created, or can it be destroyed and > re-created on demand? Since it is a KeyedService, it will live for as long as the BrowserContext once created. https://codereview.chromium.org/1314413005/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/offscreen_presentation_manager.h:132: // to the presentation. On 2015/10/01 18:39:12, mark a. foltz wrote: > What happens to pending callbacks passed into Register*Receiver()? The callback will be removed from this class. I will add comments stating that. https://codereview.chromium.org/1314413005/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/offscreen_presentation_manager.h:146: CreateOffscreenPresentationConnection( On 2015/10/01 18:39:12, mark a. foltz wrote: > Nit: Might name this ConnectToOffscreenPresentation to lead with the verb. Done. https://codereview.chromium.org/1314413005/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/offscreen_presentation_manager.h:148: const RenderFrameHostId& controller_frame_id); On 2015/10/01 18:39:13, mark a. foltz wrote: > It seems a little asymmetrical for the controller to be able to connect > synchronously, while the receiver must pass a callback. > > What state is the session in when it is returned? Yes, this is also how presentation.receiver API works (i.e., it has to wait for sessions to be available)? Nevertheless, I tried to address this by making sure controllers and receiver use the same OffscreenPresentationSession API to talk to each other. The state will be connected. https://codereview.chromium.org/1314413005/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/offscreen_presentation_manager.h:160: void RemoveSession(bool is_controller); On 2015/10/01 18:39:12, mark a. foltz wrote: > What does is_controller mean? Does this mean the caller is a controller or it's > trying to remove a session created by a controller? > > > In general I prefer APIs that are explicit instead of using boolean flags, so > instead: > > RemoveControllerSession() > RemoveReceiverSession() Done. https://codereview.chromium.org/1314413005/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/offscreen_presentation_manager.h:161: void SendMessage(bool is_controller, On 2015/10/01 18:39:13, mark a. foltz wrote: > SendMessageToController() > SendMessageToReceiver() > Done. > What would happen if a controller called this with is_controller = false? Then it will send a message to itself. We could add DCHECK to make sure it doesn't happen. But IMO it's not a major issue since this is only used internally. https://codereview.chromium.org/1314413005/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/offscreen_presentation_manager.h:176: // Not owned by this class. On 2015/10/01 18:39:13, mark a. foltz wrote: > Destruction of either would seem to require that this is also destroyed, > otherwise, one end of the connection is dangling and that doesn't seem useful. > > What enforces that? Currently, this will be destroyed once both sessions become nullptr. To change it so it's destroyed when either one becomes nullptr, we just have to invalidate the other session's pointer to this class, and make the SendMessage failure logic to that class. https://codereview.chromium.org/1314413005/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/offscreen_presentation_manager.h:186: // Contains callback to the receiver to inform it of new connections On 2015/10/01 18:39:13, mark a. foltz wrote: > Called only once, or once per connected controller? Once per connected controller. https://codereview.chromium.org/1314413005/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/offscreen_presentation_manager.h:196: scoped_ptr<OffscreenPresentationSession> AddConnectionAndNotifyReceiver( On 2015/10/01 18:39:12, mark a. foltz wrote: > Is this called when a new controller connects? > Maybe OnControllerConnected() I want to keep name as AddConnection() so there's a symmetry with RemoveConnection(). https://codereview.chromium.org/1314413005/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/offscreen_presentation_manager.h:198: void OnReceiverDetached(); On 2015/10/01 18:39:12, mark a. foltz wrote: > What is a "detached" receiver? Do you mean controller? > > Maybe OnControllerDisconnected()? No, this is called when the receiver unregisters. Actually now that we destroy the connection when either controller or receiver is gone, we don't need this method anymore as long as the receiver destroys all of its session objects before calling unregister, which is the case now. https://codereview.chromium.org/1314413005/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/offscreen_presentation_manager_factory.cc (right): https://codereview.chromium.org/1314413005/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/offscreen_presentation_manager_factory.cc:41: OffscreenPresentationManagerFactory::GetBrowserContextToUse( On 2015/10/01 18:39:13, mark a. foltz wrote: > Will this return the same OPM for a profile and any incognito context spawned > from it? > > Since we are creating a new browser context for each offscreen tab, will each > get its own OPM? So in ReceiverPresentationServiceDelegate we explicitly get a OffscreenPresentationManager* using the original browser context as key. So the offscreen tab will talk to the same OPM as the controllers. But here, this is configured to return a separate OffscreenPresentationManager for each context, even for incognito ones. I think we can remove this function and just use the default behavior, which is to return nullptr for incognito contexts. https://codereview.chromium.org/1314413005/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/presentation_service_delegate_impl.cc (right): https://codereview.chromium.org/1314413005/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:120: OffscreenPresentationSession* FindOffscreenSession( On 2015/10/01 06:25:28, mark a. foltz wrote: > Can you document this method? Which session is returned, and what if the > presentation_id is not an offscreen presentation? Done. https://codereview.chromium.org/1314413005/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:150: // the associated BrowserContext. On 2015/10/01 06:25:28, mark a. foltz wrote: > I'm assuming that the BC outlives this object; can you make sure that is clear > in comments for this class? Done. https://codereview.chromium.org/1314413005/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:165: OffscreenPresentationManagerFactory::GetOrCreateForBrowserContext( On 2015/10/01 06:25:28, mark a. foltz wrote: > Do we need to look this up for every presentation frame, or can we pass it in? We don't need to look it up for every frame. However since PresentationFrameManager does not use it at all, I feel it is simpler if we just look it up here. https://codereview.chromium.org/1314413005/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/presentation_service_delegate_impl.h (right): https://codereview.chromium.org/1314413005/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.h:107: void GetPresentationReceiverSession( On 2015/10/01 06:25:28, mark a. foltz wrote: > Do we need both this and the GetSessions method below? > If so, please add some comments explaining the difference. I added comments in presentation_service_delegate.h interface. https://codereview.chromium.org/1314413005/diff/80001/content/browser/present... File content/browser/presentation/presentation_service_impl.cc (right): https://codereview.chromium.org/1314413005/diff/80001/content/browser/present... content/browser/presentation/presentation_service_impl.cc:470: new GetPresentationReceiverSessionMojoCallback(callback)); On 2015/10/01 06:25:28, mark a. foltz wrote: > Do we need to wrap the passed-in callback in another callback? Can we just pass > it as-is to OnGetPresentationReceiverSession using base::bind? Or is it not > refcounted? 1) It used to be that we can't store mojo callbacks by value, so we store scoped_ptrs to them instead. I just tried again and it appears that it is no longer true, so I will change receiver_session_callback_ to hold the callback by value. 2) We also need to invoke receiver_session_callback_ on Reset(), that is why it is not bound to OnGetPresentationReceiverSession. https://codereview.chromium.org/1314413005/diff/80001/content/browser/present... File content/browser/presentation/presentation_service_impl.h (right): https://codereview.chromium.org/1314413005/diff/80001/content/browser/present... content/browser/presentation/presentation_service_impl.h:109: using GetPresentationReceiverSessionMojoCallback = On 2015/10/01 06:25:29, mark a. foltz wrote: > Can we combine this with DefaultSessionMojoCallback (perhaps as > PresentationSessionMojoCallback)? Done.
https://codereview.chromium.org/1314413005/diff/100001/content/browser/presen... File content/browser/presentation/presentation_service_impl.h (right): https://codereview.chromium.org/1314413005/diff/100001/content/browser/presen... content/browser/presentation/presentation_service_impl.h:101: using PresentationSessionMojoCallback = nit: Could this rename be done separately maybe? It seems to affect some files exclusively so distracts from the main change?
The refactoring made things much cleaner. Most of my comments this round are about impl details within the new factoring: https://codereview.chromium.org/1314413005/diff/120001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/1314413005/diff/120001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:2600: return media_router::ReceiverPresentationServiceDelegateImpl:: This is a little confusing. How is ReceiverPresentationServiceDelegateImpl appropriate for all off-the-record profiles? Doesn't it only pertain to off-screen tabs? Perhaps this code should be: if (auto* impl = ReceiverPresServiceDelegateImpl::FromWebContents(...)) return impl; // WebContents is an off-screen tab. return PresentationServiceDelegateImpl::GetOrCreateForWebContents(...); Also, OOC, should we be worried about extension background pages attempting to use the navigator.presentation API directly? Do we want to allow or forbid that? https://codereview.chromium.org/1314413005/diff/120001/chrome/browser/extensi... File chrome/browser/extensions/api/tab_capture/offscreen_presentation.cc (right): https://codereview.chromium.org/1314413005/diff/120001/chrome/browser/extensi... chrome/browser/extensions/api/tab_capture/offscreen_presentation.cc:207: media_router::ReceiverPresentationServiceDelegateImpl::CreateForWebContents( sanity-check: Do we need to make sure there isn't already another off-screen tab WebContents associated with the same presentation_id? Seems like things would break if there were two of them with the same ID. https://codereview.chromium.org/1314413005/diff/120001/chrome/browser/media/r... File chrome/browser/media/router/media_router_type_converters.cc (right): https://codereview.chromium.org/1314413005/diff/120001/chrome/browser/media/r... chrome/browser/media/router/media_router_type_converters.cc:64: route.set_is_offscreen_presentation(input->is_offscreen_presentation); Unless I'm missing something, consider just defining a copy constructor in the MediaRoute class. Then, you wouldn't even need these two Convert() functions. It would be fine if this were a clean-up TODO for a later CL. https://codereview.chromium.org/1314413005/diff/120001/chrome/browser/media/r... File chrome/browser/media/router/offscreen_presentation_manager.cc (right): https://codereview.chromium.org/1314413005/diff/120001/chrome/browser/media/r... chrome/browser/media/router/offscreen_presentation_manager.cc:28: if (is_controller_) Consider getting rid of the |is_controller_| bool and the separate OffPresConnection::RemoveXXX() methods and just make this entire dtor body: if (connection_) connection_->RemoveSession(this); https://codereview.chromium.org/1314413005/diff/120001/chrome/browser/media/r... chrome/browser/media/router/offscreen_presentation_manager.cc:42: if (is_controller_) Without the |is_controller_| bool, you might change this to: connection_->SendMessageFrom(this, message.Pass(), callback); SendMessageFrom() would call SendMessage() to send the message to the other endpoint (i.e., controller_ if from the receiver_, and vice versa). https://codereview.chromium.org/1314413005/diff/120001/chrome/browser/media/r... chrome/browser/media/router/offscreen_presentation_manager.cc:51: messages_callback_ = callback; Since there can only be one listener, should you DCHECK(messages_callback_.is_null())? This would also apply to the other "listener" and "state change" methods below... https://codereview.chromium.org/1314413005/diff/120001/chrome/browser/media/r... chrome/browser/media/router/offscreen_presentation_manager.cc:103: OffscreenPresentationManager::OffscreenPresentationSession>(); nit: OffscreenPresentationSession is an inner class of the manager, so you could change this line to just: return scoped_ptr<OffscreenPresentationSession>(); https://codereview.chromium.org/1314413005/diff/120001/chrome/browser/media/r... chrome/browser/media/router/offscreen_presentation_manager.cc:114: : presentation_(presentation), Please be consistent everywhere with ordering of values (i.e., presentation before controller_frame_id, or vice versa). Probably presentation should be first. https://codereview.chromium.org/1314413005/diff/120001/chrome/browser/media/r... File chrome/browser/media/router/offscreen_presentation_manager.h (right): https://codereview.chromium.org/1314413005/diff/120001/chrome/browser/media/r... chrome/browser/media/router/offscreen_presentation_manager.h:21: // presentation (AKA the receiver) and the frames that start/connect and nit: s/frames/render frames/ for clarity https://codereview.chromium.org/1314413005/diff/120001/chrome/browser/media/r... chrome/browser/media/router/offscreen_presentation_manager.h:36: // &base::Bind(&OnSessionAvailable)); Delete the first ampersand. https://codereview.chromium.org/1314413005/diff/120001/chrome/browser/media/r... chrome/browser/media/router/offscreen_presentation_manager.h:59: // sessions must be destroyed before unregistration. formatting: More text fits on this line. Also, many of the comment blocks throughout this file should be fixed. https://codereview.chromium.org/1314413005/diff/120001/chrome/browser/media/r... chrome/browser/media/router/offscreen_presentation_manager.h:140: // NOTE: before this call can be made, all OffscreenPresentationSession This "note" comment makes me wonder: Could you enforce this by changing the method so that OffPresManager takes back its ownership of the OffPressSession object? Meaning: void UnregisterOffscreenPresentationReceiver( scoped_ptr<OffscreenPresentationSession> receiver_session); https://codereview.chromium.org/1314413005/diff/120001/chrome/browser/media/r... chrome/browser/media/router/offscreen_presentation_manager.h:151: // |receiver_available_callback_| will be invoked with a nit: To clarify, might I suggest this last sentence be replaced with: In addition, the entity that has registered (or will register) the receiver will have its ReceiverSessionAvailableCallback run to notify it that the connection is now complete. https://codereview.chromium.org/1314413005/diff/120001/chrome/browser/media/r... File chrome/browser/media/router/presentation_service_delegate_impl.cc (right): https://codereview.chromium.org/1314413005/diff/120001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:116: // Gets the OffscreenPresentationSession associated with |presentation_id| nit: add a line of whitespace above this line https://codereview.chromium.org/1314413005/diff/120001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:201: LOG(ERROR) << "CreateOffscreenPresentationConnection returned nullptr."; nit: DLOG(ERROR) since this message does not help end-users in official builds. https://codereview.chromium.org/1314413005/diff/120001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:316: if (ContainsKey(session_state_observers_, presentation_id)) { OOC, why can there only be one observer? Should |session_state_observers_| be a multimap (or vector)? https://codereview.chromium.org/1314413005/diff/120001/chrome/browser/media/r... File chrome/browser/media/router/receiver_presentation_service_delegate_impl.cc (right): https://codereview.chromium.org/1314413005/diff/120001/chrome/browser/media/r... chrome/browser/media/router/receiver_presentation_service_delegate_impl.cc:224: for (const auto& session : receiver_sessions_) { Hmm...Weird to be creating a vector of receiver_sessions_.size() copies of the same struct. Is that your intention? If so, this would be simpler and more efficient: return std::vector<content::PresentationSessionInfo>( receiver_sessions_.size(), content::PresentationSessionInfo(std::string(), presentation_id_)); https://codereview.chromium.org/1314413005/diff/120001/chrome/browser/media/r... File chrome/browser/media/router/receiver_presentation_service_delegate_impl.h (right): https://codereview.chromium.org/1314413005/diff/120001/chrome/browser/media/r... chrome/browser/media/router/receiver_presentation_service_delegate_impl.h:35: // Only the main frame of the offscreen tab is allowed to make receiver OOC, why is only the main frame allowed? I could see a web dev wanting to design some static content for the "main" frame and then nest dynamic content and/or all the control logic within a nested IFRAME. https://codereview.chromium.org/1314413005/diff/120001/chrome/browser/media/r... chrome/browser/media/router/receiver_presentation_service_delegate_impl.h:46: // TODO: update comments Please do. ;-) https://codereview.chromium.org/1314413005/diff/120001/content/browser/presen... File content/browser/presentation/presentation_service_impl.cc (right): https://codereview.chromium.org/1314413005/diff/120001/content/browser/presen... content/browser/presentation/presentation_service_impl.cc:470: receiver_session_callback_ = callback; Can there only be one call to GetPresentationReceiverSession() at a time? Whether yes or no, I don't think you should store the callback as a member of this class. Just add an argument to pass |callback| to OnGetPresentationReceiverSession() and include it in the base::Bind(...) below. Exception: Does |callback| need to be run if it is has not been run by the time PresentationServiceImpl is destroyed (i.e., this implies the receiver never became available)? https://codereview.chromium.org/1314413005/diff/120001/content/common/present... File content/common/presentation/presentation_service.mojom (right): https://codereview.chromium.org/1314413005/diff/120001/content/common/present... content/common/presentation/presentation_service.mojom:108: // Gets a single receiver PresentationSessionInfo object for the See my comment in presentation_service_delegate.h, re: what if a receiver never becomes available? https://codereview.chromium.org/1314413005/diff/120001/content/public/browser... File content/public/browser/presentation_service_delegate.h (right): https://codereview.chromium.org/1314413005/diff/120001/content/public/browser... content/public/browser/presentation_service_delegate.h:42: virtual void OnDelegateDestroyed() = 0; nit: The methods in this interface should probably be defined as no-ops, instead of forcing subclasses to override all pure virtual methods. This relates to my comment below... https://codereview.chromium.org/1314413005/diff/120001/content/public/browser... content/public/browser/presentation_service_delegate.h:187: // Gets a receiver presentation session for the offscreen presentation hosted The wording in this comment was a bit confusing to me. How about: Runs |callback| once the receiver becomes available. |callback| may be run during this method call (if the receiver is already available), or at some later time. |render_process_id| and |render_frame_id| must refer to the main render frame of an off-screen tab. BTW--What should happen if a receiver never becomes available? Is the callback guaranteed to be run with a nullptr, or never run at all? https://codereview.chromium.org/1314413005/diff/120001/content/public/browser... content/public/browser/presentation_service_delegate.h:196: virtual void GetPresentationReceiverSession( Consider naming this method something like NotifyWhenReceiverSessionIsAvailable() to make it clear that it's different from GetPresentationReceiverSessions(). Even better, consider eliminating this method (and ListenForSessionMessages() and ListenForSessionStateChange()) by adding extra methods to the Observer interface. Then, you also won't need all these extra callback typedefs.
https://codereview.chromium.org/1314413005/diff/120001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/1314413005/diff/120001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:2600: return media_router::ReceiverPresentationServiceDelegateImpl:: On 2015/10/07 21:51:46, miu wrote: > This is a little confusing. How is ReceiverPresentationServiceDelegateImpl > appropriate for all off-the-record profiles? Doesn't it only pertain to > off-screen tabs? > > Perhaps this code should be: > > if (auto* impl = ReceiverPresServiceDelegateImpl::FromWebContents(...)) > return impl; // WebContents is an off-screen tab. > return PresentationServiceDelegateImpl::GetOrCreateForWebContents(...); > > Also, OOC, should we be worried about extension background pages attempting to > use the navigator.presentation API directly? Do we want to allow or forbid > that? We've disabled presentation API in incognito mode, so this method should return null for non-offscreen incognito WebContents. I will restructure this and add some comments so it's clearer. https://codereview.chromium.org/1314413005/diff/120001/chrome/browser/extensi... File chrome/browser/extensions/api/tab_capture/offscreen_presentation.cc (right): https://codereview.chromium.org/1314413005/diff/120001/chrome/browser/extensi... chrome/browser/extensions/api/tab_capture/offscreen_presentation.cc:207: media_router::ReceiverPresentationServiceDelegateImpl::CreateForWebContents( On 2015/10/07 21:51:46, miu wrote: > sanity-check: Do we need to make sure there isn't already another off-screen tab > WebContents associated with the same presentation_id? Seems like things would > break if there were two of them with the same ID. We have a DCHECK for that in OffscreenPresentationManager::RegisterOffscreenPresentationReceiver, which is called in ReceiverPresentationServiceDelegateImpl's constructor. https://codereview.chromium.org/1314413005/diff/120001/chrome/browser/media/r... File chrome/browser/media/router/media_router_type_converters.cc (right): https://codereview.chromium.org/1314413005/diff/120001/chrome/browser/media/r... chrome/browser/media/router/media_router_type_converters.cc:64: route.set_is_offscreen_presentation(input->is_offscreen_presentation); On 2015/10/07 21:51:46, miu wrote: > Unless I'm missing something, consider just defining a copy constructor in the > MediaRoute class. Then, you wouldn't even need these two Convert() functions. > It would be fine if this were a clean-up TODO for a later CL. These are template specializations for mojo type converters which is usually how we convert from mojo types to "native" types, I think. I was also thinking that by putting the converters here, we won't introduce mojo types into media_route.{h,cc} so it's cleaner. https://codereview.chromium.org/1314413005/diff/120001/chrome/browser/media/r... File chrome/browser/media/router/offscreen_presentation_manager.cc (right): https://codereview.chromium.org/1314413005/diff/120001/chrome/browser/media/r... chrome/browser/media/router/offscreen_presentation_manager.cc:28: if (is_controller_) On 2015/10/07 21:51:46, miu wrote: > Consider getting rid of the |is_controller_| bool and the separate > OffPresConnection::RemoveXXX() methods and just make this entire dtor body: > > if (connection_) > connection_->RemoveSession(this); Done. https://codereview.chromium.org/1314413005/diff/120001/chrome/browser/media/r... chrome/browser/media/router/offscreen_presentation_manager.cc:42: if (is_controller_) On 2015/10/07 21:51:46, miu wrote: > Without the |is_controller_| bool, you might change this to: > > connection_->SendMessageFrom(this, message.Pass(), callback); > > SendMessageFrom() would call SendMessage() to send the message to the other > endpoint (i.e., controller_ if from the receiver_, and vice versa). Done. https://codereview.chromium.org/1314413005/diff/120001/chrome/browser/media/r... chrome/browser/media/router/offscreen_presentation_manager.cc:51: messages_callback_ = callback; On 2015/10/07 21:51:46, miu wrote: > Since there can only be one listener, should you > DCHECK(messages_callback_.is_null())? This would also apply to the other > "listener" and "state change" methods below... Done. https://codereview.chromium.org/1314413005/diff/120001/chrome/browser/media/r... chrome/browser/media/router/offscreen_presentation_manager.cc:103: OffscreenPresentationManager::OffscreenPresentationSession>(); On 2015/10/07 21:51:46, miu wrote: > nit: OffscreenPresentationSession is an inner class of the manager, so you could > change this line to just: > > return scoped_ptr<OffscreenPresentationSession>(); Done. https://codereview.chromium.org/1314413005/diff/120001/chrome/browser/media/r... chrome/browser/media/router/offscreen_presentation_manager.cc:114: : presentation_(presentation), On 2015/10/07 21:51:46, miu wrote: > Please be consistent everywhere with ordering of values (i.e., presentation > before controller_frame_id, or vice versa). Probably presentation should be > first. Done. https://codereview.chromium.org/1314413005/diff/120001/chrome/browser/media/r... File chrome/browser/media/router/offscreen_presentation_manager.h (right): https://codereview.chromium.org/1314413005/diff/120001/chrome/browser/media/r... chrome/browser/media/router/offscreen_presentation_manager.h:21: // presentation (AKA the receiver) and the frames that start/connect and On 2015/10/07 21:51:46, miu wrote: > nit: s/frames/render frames/ for clarity Done. https://codereview.chromium.org/1314413005/diff/120001/chrome/browser/media/r... chrome/browser/media/router/offscreen_presentation_manager.h:36: // &base::Bind(&OnSessionAvailable)); On 2015/10/07 21:51:46, miu wrote: > Delete the first ampersand. Done. https://codereview.chromium.org/1314413005/diff/120001/chrome/browser/media/r... chrome/browser/media/router/offscreen_presentation_manager.h:59: // sessions must be destroyed before unregistration. On 2015/10/07 21:51:46, miu wrote: > formatting: More text fits on this line. Also, many of the comment blocks > throughout this file should be fixed. Done. https://codereview.chromium.org/1314413005/diff/120001/chrome/browser/media/r... chrome/browser/media/router/offscreen_presentation_manager.h:140: // NOTE: before this call can be made, all OffscreenPresentationSession On 2015/10/07 21:51:46, miu wrote: > This "note" comment makes me wonder: Could you enforce this by changing the > method so that OffPresManager takes back its ownership of the OffPressSession > object? Meaning: > > void UnregisterOffscreenPresentationReceiver( > scoped_ptr<OffscreenPresentationSession> receiver_session); That could work. I separate the two calls because there can be multiple OffscreenPresentationSessions for the receiver at any given time, and OffscreenPresentationSessions do not have the same lifetime as the receiver's registration/unregistration (but it does lie strictly within it, i.e. I can destroy OffscreenPresentationSessions any time before calling UnregisterOffscreenPresentationReceiver) I think having only one way to manage OffscreenPresentationSession objects from receiver's perspective is cleaner. https://codereview.chromium.org/1314413005/diff/120001/chrome/browser/media/r... chrome/browser/media/router/offscreen_presentation_manager.h:151: // |receiver_available_callback_| will be invoked with a On 2015/10/07 21:51:46, miu wrote: > nit: To clarify, might I suggest this last sentence be replaced with: > > In addition, the entity that has registered (or will register) the receiver > will have its ReceiverSessionAvailableCallback run to notify it that the > connection is now complete. Thanks. I made some modification to your suggestion. https://codereview.chromium.org/1314413005/diff/120001/chrome/browser/media/r... File chrome/browser/media/router/presentation_service_delegate_impl.cc (right): https://codereview.chromium.org/1314413005/diff/120001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:116: // Gets the OffscreenPresentationSession associated with |presentation_id| On 2015/10/07 21:51:46, miu wrote: > nit: add a line of whitespace above this line Done. https://codereview.chromium.org/1314413005/diff/120001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:201: LOG(ERROR) << "CreateOffscreenPresentationConnection returned nullptr."; On 2015/10/07 21:51:46, miu wrote: > nit: DLOG(ERROR) since this message does not help end-users in official builds. Done. https://codereview.chromium.org/1314413005/diff/120001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:316: if (ContainsKey(session_state_observers_, presentation_id)) { On 2015/10/07 21:51:46, miu wrote: > OOC, why can there only be one observer? Should |session_state_observers_| be a > multimap (or vector)? The only caller is the renderer where PresentationDispatcher will add a listener as soon as it receives a newly created session. If this changes then we can make this hold multiple observers per presentation. https://codereview.chromium.org/1314413005/diff/120001/chrome/browser/media/r... File chrome/browser/media/router/receiver_presentation_service_delegate_impl.cc (right): https://codereview.chromium.org/1314413005/diff/120001/chrome/browser/media/r... chrome/browser/media/router/receiver_presentation_service_delegate_impl.cc:224: for (const auto& session : receiver_sessions_) { On 2015/10/07 21:51:46, miu wrote: > Hmm...Weird to be creating a vector of receiver_sessions_.size() copies of the > same struct. Is that your intention? > > If so, this would be simpler and more efficient: > > return std::vector<content::PresentationSessionInfo>( > receiver_sessions_.size(), > content::PresentationSessionInfo(std::string(), presentation_id_)); That is my intention, for now. The TODO and bug explains that we need an additional id to distinguish these sessions, and we would just add that id in the content::PresentationSessionInfo constructor below. Since we have TODO here I guess there's no point to intentionally write the code this way. https://codereview.chromium.org/1314413005/diff/120001/chrome/browser/media/r... File chrome/browser/media/router/receiver_presentation_service_delegate_impl.h (right): https://codereview.chromium.org/1314413005/diff/120001/chrome/browser/media/r... chrome/browser/media/router/receiver_presentation_service_delegate_impl.h:35: // Only the main frame of the offscreen tab is allowed to make receiver On 2015/10/07 21:51:46, miu wrote: > OOC, why is only the main frame allowed? I could see a web dev wanting to > design some static content for the "main" frame and then nest dynamic content > and/or all the control logic within a nested IFRAME. It's much trickier to define behavior when multiple render frames can access the receiver API. For instance it's not clear what it means to have two frames both call getSession() -- should they both get the same session that can talk to the same controller? If not, which frame should get the session? https://codereview.chromium.org/1314413005/diff/120001/chrome/browser/media/r... chrome/browser/media/router/receiver_presentation_service_delegate_impl.h:46: // TODO: update comments On 2015/10/07 21:51:46, miu wrote: > Please do. ;-) Done. https://codereview.chromium.org/1314413005/diff/120001/content/browser/presen... File content/browser/presentation/presentation_service_impl.cc (right): https://codereview.chromium.org/1314413005/diff/120001/content/browser/presen... content/browser/presentation/presentation_service_impl.cc:470: receiver_session_callback_ = callback; On 2015/10/07 21:51:46, miu wrote: > Can there only be one call to GetPresentationReceiverSession() at a time? > > Whether yes or no, I don't think you should store the callback as a member of > this class. Just add an argument to pass |callback| to > OnGetPresentationReceiverSession() and include it in the base::Bind(...) below. > > Exception: Does |callback| need to be run if it is has not been run by the time > PresentationServiceImpl is destroyed (i.e., this implies the receiver never > became available)? It does need to be run before it is destroyed (see L569), which is why we keep a reference to it in this class. It's one of the quirks with Mojo. https://codereview.chromium.org/1314413005/diff/120001/content/common/present... File content/common/presentation/presentation_service.mojom (right): https://codereview.chromium.org/1314413005/diff/120001/content/common/present... content/common/presentation/presentation_service.mojom:108: // Gets a single receiver PresentationSessionInfo object for the On 2015/10/07 21:51:47, miu wrote: > See my comment in presentation_service_delegate.h, re: what if a receiver never > becomes available? since all mojo callbacks have to be invoked before they are destroyed, null will be returned just before this request becomes invalid. Added comments. https://codereview.chromium.org/1314413005/diff/120001/content/public/browser... File content/public/browser/presentation_service_delegate.h (right): https://codereview.chromium.org/1314413005/diff/120001/content/public/browser... content/public/browser/presentation_service_delegate.h:42: virtual void OnDelegateDestroyed() = 0; On 2015/10/07 21:51:47, miu wrote: > nit: The methods in this interface should probably be defined as no-ops, instead > of forcing subclasses to override all pure virtual methods. This relates to my > comment below... Ack. we only have 2 implementations of this and they are different. So I will keep this for now. See comment below as well. https://codereview.chromium.org/1314413005/diff/120001/content/public/browser... content/public/browser/presentation_service_delegate.h:187: // Gets a receiver presentation session for the offscreen presentation hosted On 2015/10/07 21:51:47, miu wrote: > The wording in this comment was a bit confusing to me. How about: > > Runs |callback| once the receiver becomes available. |callback| may be run > during this method call (if the receiver is already available), or at some later > time. |render_process_id| and |render_frame_id| must refer to the main render > frame of an off-screen tab. > > BTW--What should happen if a receiver never becomes available? Is the callback > guaranteed to be run with a nullptr, or never run at all? Done. If a receiver session never becomes available, the callback will never be run. https://codereview.chromium.org/1314413005/diff/120001/content/public/browser... content/public/browser/presentation_service_delegate.h:196: virtual void GetPresentationReceiverSession( On 2015/10/07 21:51:47, miu wrote: > Consider naming this method something like > NotifyWhenReceiverSessionIsAvailable() to make it clear that it's different from > GetPresentationReceiverSessions(). > > Even better, consider eliminating this method (and ListenForSessionMessages() > and ListenForSessionStateChange()) by adding extra methods to the Observer > interface. Then, you also won't need all these extra callback typedefs. Renamed per your suggestion. I am not a fan of the PresentationServiceDelegate::Observer interface. I think it's clearer to define the expected behavior yet rather than having an all purpose observer class.
LGTM with comments applied. This seems pretty solid and thanks for the detailed comments in the key header files. Most of my comments are minor, but there are a few things I noticed that should be addressed before committing. (1) In several places there is code that updates maps but first DCHECKs() that the key exists/doesn't exist. What do you want this code to do in release builds? If you can't trust the caller of the API to pass you a valid key then you should probably log an error and return early instead. If you *do* control the callers and this is a true "can't happen" situation then leaving as a DCHECK is okay - or the DCHECK isn't needed at all. (2) This patch implements ReceiverPresentationServiceDelegateImpl as a subclass but refuses to implement many of the methods of the base class. This usually means that inheritance is not the correct relationship. I would suggest making it independent of the base class (possibly factoring out a shared base class). Another approach is to merge the receiver functionality into the existing PresentationServiceDelegateImpl and have the controller-specific methods return errors when invoked from a receiver context. Since Chrome implements both the controller and receiver APIs, there's logically one service backing them (of course we can split up that implementation if desired). (3) Some basic unit test coverage for the new code should be implemented. I can take another look at the tests. (4) There's some implicit contract that all sessions/connections must be disposed before a presentation is torn down. This can happen for several reasons so I'd rather this be enforced in one place (i.e. the OPM) versus having higher layers deal with this. Maybe it's there but I missed it? https://codereview.chromium.org/1314413005/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/media_route.h (right): https://codereview.chromium.org/1314413005/diff/140001/chrome/browser/media/r... chrome/browser/media/router/media_route.h:74: bool is_offscreen_presentation() const { return is_offscreen_presentation_; } Please document the meaning of is_offscreen_presentation. https://codereview.chromium.org/1314413005/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/offscreen_presentation_manager.cc (right): https://codereview.chromium.org/1314413005/diff/140001/chrome/browser/media/r... chrome/browser/media/router/offscreen_presentation_manager.cc:64: ScopedVector<content::PresentationSessionMessage> messages; Please add a TODO to handle multiple messages at once. https://codereview.chromium.org/1314413005/diff/140001/chrome/browser/media/r... chrome/browser/media/router/offscreen_presentation_manager.cc:75: DCHECK(!ContainsKey(offscreen_presentations_, presentation_id)); How do you intend to handle this case in release builds? https://codereview.chromium.org/1314413005/diff/140001/chrome/browser/media/r... chrome/browser/media/router/offscreen_presentation_manager.cc:85: DCHECK(ContainsKey(offscreen_presentations_, presentation_id)); Similar comments https://codereview.chromium.org/1314413005/diff/140001/chrome/browser/media/r... chrome/browser/media/router/offscreen_presentation_manager.cc:175: // The receiver must have destroyed all connections before unregistration. Can unregistration do this as a side effect? It would seem cleaner for the OPM to watch the WebContents and the OPM to coordinate the teardown of all the presentation sessions in response to window.close(), session.terminate(), failed streaming or navigation, instead of having each of these code paths manage the lifetimes of the sessions. https://codereview.chromium.org/1314413005/diff/140001/chrome/browser/media/r... chrome/browser/media/router/offscreen_presentation_manager.cc:182: DCHECK(!ContainsKey(connections_, controller_frame_id)); Behavior in release builds? https://codereview.chromium.org/1314413005/diff/140001/chrome/browser/media/r... chrome/browser/media/router/offscreen_presentation_manager.cc:199: DCHECK(ContainsKey(connections_, controller_frame_id)); Ditto https://codereview.chromium.org/1314413005/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/offscreen_presentation_manager.h (right): https://codereview.chromium.org/1314413005/diff/140001/chrome/browser/media/r... chrome/browser/media/router/offscreen_presentation_manager.h:40: // [Calls methods in |receiver_session| to send/receive messages, etc.] Nitpick: I wouldn't expect this to happen in this callback, rather, it would happen in different callbacks based on send() invocations. Maybe instead: [Retains |receiver_session| to send/receive messages in response to send()] https://codereview.chromium.org/1314413005/diff/140001/chrome/browser/media/r... chrome/browser/media/router/offscreen_presentation_manager.h:45: // to controller frame) and the receiver session (given to receiver). Note To be clear, each controller <--> receiver relationship corresponds to exactly one pair of OffscreenPresentationSessions (one for the controller, one for the receiver)? https://codereview.chromium.org/1314413005/diff/140001/chrome/browser/media/r... chrome/browser/media/router/offscreen_presentation_manager.h:57: // When the receiver is no longer associated with an offscreen presentation, it Which object is "the receiver" in this context? It seems like this could be the job of the OffscreenPresentationManager (by observing the offscreen tab/WebContents)? https://codereview.chromium.org/1314413005/diff/140001/chrome/browser/media/r... chrome/browser/media/router/offscreen_presentation_manager.h:62: // receiver_sessions.clear(); Where does the receiver get |receiver_sessions|? From multiple invocations of OnReceiverSession()? https://codereview.chromium.org/1314413005/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/presentation_service_delegate_impl.cc (right): https://codereview.chromium.org/1314413005/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:130: // Maps from presentation ID to the corresponding OffscreenController within Can this paragraph be wrapped? https://codereview.chromium.org/1314413005/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:190: DCHECK(!ContainsKey(offscreen_presentation_sessions_, presentation_id)); Behavior in release builds? https://codereview.chromium.org/1314413005/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:272: auto it = presentation_id_to_route_id_.find(session.presentation_id); This lookup only really makes sense for 2-UA presentations (no |offscreen_session|) since there the page is messaging the route directly. Move to line 284? https://codereview.chromium.org/1314413005/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:814: // See ReceiverPresentationServiceDelegateImpl for details. We need to decide how to handle the receiver APIs in Blink. If we plumb through a bit saying a frame belongs to an offscreen presentation, we could switch the APIs between being no-ops and calling through to content. Also see https://github.com/w3c/presentation-api/issues/205 https://codereview.chromium.org/1314413005/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/receiver_presentation_service_delegate_impl.cc (right): https://codereview.chromium.org/1314413005/diff/140001/chrome/browser/media/r... chrome/browser/media/router/receiver_presentation_service_delegate_impl.cc:27: DCHECK(web_contents); Who is responsible for checking that the WebContents is appropriate for presentations (i.e. independent incognito profile)? For example, is the offscreen tab API calling this method with the WebContents it has created? https://codereview.chromium.org/1314413005/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/render_frame_host_helper.h (right): https://codereview.chromium.org/1314413005/diff/140001/chrome/browser/media/r... chrome/browser/media/router/render_frame_host_helper.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. Maybe this should be named render_frame_host_id.h? https://codereview.chromium.org/1314413005/diff/140001/chrome/browser/media/r... chrome/browser/media/router/render_frame_host_helper.h:16: std::pair<decltype(((content::RenderProcessHost*)nullptr)->GetID()), Is decltype necessary here? It seems to only be recommended for rare situations like variadic templates. https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/_zoNvZd_dSo If the types of the ids get widened to long (which seems unlikely) you should get a compile error when trying to assign them to pair<int, int>. https://codereview.chromium.org/1314413005/diff/140001/content/browser/presen... File content/browser/presentation/presentation_type_converters.h (right): https://codereview.chromium.org/1314413005/diff/140001/content/browser/presen... content/browser/presentation/presentation_type_converters.h:31: if (!input.presentation_url.empty()) What is creating a presentation with an empty URL? https://codereview.chromium.org/1314413005/diff/140001/content/common/present... File content/common/presentation/presentation_service.mojom (right): https://codereview.chromium.org/1314413005/diff/140001/content/common/present... content/common/presentation/presentation_service.mojom:111: // null will be returned. This might be simpler as, // Returns a PresentationSessionInfo object for this frame, if it is the // main frame of an offscreen presentation. Otherwise returns null. https://codereview.chromium.org/1314413005/diff/140001/content/common/present... content/common/presentation/presentation_service.mojom:113: // with url set to empty, and id set to the ID of the offscreen presentation. Can we look up the URL from the PresentationRequest that generated the offscreen presentation? https://codereview.chromium.org/1314413005/diff/140001/content/public/browser... File content/public/browser/presentation_service_delegate.h (right): https://codereview.chromium.org/1314413005/diff/140001/content/public/browser... content/public/browser/presentation_service_delegate.h:189: // available. Registers a callback to be notified when the offscreen presentation corresponding to the frame is available. https://codereview.chromium.org/1314413005/diff/140001/content/public/browser... content/public/browser/presentation_service_delegate.h:204: // hosted on the offscreen tab containing this frame. Returns all presentation sessions for the offscreen presentation corresponding to the given frame (if any).
Also the previous comment about the rename of Callback typeded. https://codereview.chromium.org/1314413005/diff/140001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/1314413005/diff/140001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:2610: } else { nit: I believe if (a) return b; else return c; is usually written as if (a) return b; return c; https://codereview.chromium.org/1314413005/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/media_router_type_converters.cc (right): https://codereview.chromium.org/1314413005/diff/140001/chrome/browser/media/r... chrome/browser/media/router/media_router_type_converters.cc:64: route.set_is_offscreen_presentation(input->is_offscreen_presentation); nit: it feels like for consistency is_offscreen_presentation should just be passed in the constructor like the rest of the parameters. https://codereview.chromium.org/1314413005/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/offscreen_presentation_manager.cc (right): https://codereview.chromium.org/1314413005/diff/140001/chrome/browser/media/r... chrome/browser/media/router/offscreen_presentation_manager.cc:49: state_change_listener_ = listener; nit: DCHECK(listener); https://codereview.chromium.org/1314413005/diff/140001/chrome/browser/media/r... chrome/browser/media/router/offscreen_presentation_manager.cc:54: if (state_change_listener_) Shouldn't be a DCHECK rather so we always have a listener for state changes before they happen? Or should we queue them? https://codereview.chromium.org/1314413005/diff/140001/chrome/browser/media/r... chrome/browser/media/router/offscreen_presentation_manager.cc:63: if (!messages_callback_.is_null()) { nit: Seems like the inverse condition would be more readable: if (messages_callback_.is_null()) return; Should we queue messages until the callback is given or assert it's there by the time when the messages come? https://codereview.chromium.org/1314413005/diff/140001/chrome/browser/media/r... chrome/browser/media/router/offscreen_presentation_manager.cc:159: session->OnMessageReceived(message.Pass()); Did you mean other_session? https://codereview.chromium.org/1314413005/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/offscreen_presentation_manager.h (right): https://codereview.chromium.org/1314413005/diff/140001/chrome/browser/media/r... chrome/browser/media/router/offscreen_presentation_manager.h:70: // Forward declarations for OffscreenPresentationSession. nit: I think this comment is redundant. https://codereview.chromium.org/1314413005/diff/140001/chrome/browser/media/r... chrome/browser/media/router/offscreen_presentation_manager.h:99: void ListenForStateChanges( nit: missed a comment for this method? https://codereview.chromium.org/1314413005/diff/140001/chrome/browser/media/r... chrome/browser/media/router/offscreen_presentation_manager.h:113: content::PresentationSessionStateListener* state_change_listener_; nit: make it const pointer? https://codereview.chromium.org/1314413005/diff/140001/chrome/browser/media/r... chrome/browser/media/router/offscreen_presentation_manager.h:182: // Not owned by this class. nit: Should this comment relate to the pointer above as well? https://codereview.chromium.org/1314413005/diff/140001/chrome/browser/media/r... chrome/browser/media/router/offscreen_presentation_manager.h:182: // Not owned by this class. nit: Should this comment relate to the pointer above as well? https://codereview.chromium.org/1314413005/diff/140001/chrome/browser/media/r... chrome/browser/media/router/offscreen_presentation_manager.h:183: OffscreenPresentationSession* controller_; nit: const pointers? https://codereview.chromium.org/1314413005/diff/140001/chrome/browser/media/r... chrome/browser/media/router/offscreen_presentation_manager.h:230: } // namespace media_router nit: I know this is kind of the current style of the media/router codebase but I'd prefer better separation between all the classes (there're four in one header and worse in .cc). Could you at least separate the implementations in the .cc file better (with a 80 char comment line and more line breaks?) https://codereview.chromium.org/1314413005/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/presentation_service_delegate_impl.cc (right): https://codereview.chromium.org/1314413005/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:316: } else { nit: remove else https://codereview.chromium.org/1314413005/diff/140001/content/browser/presen... File content/browser/presentation/presentation_type_converters.h (right): https://codereview.chromium.org/1314413005/diff/140001/content/browser/presen... content/browser/presentation/presentation_type_converters.h:31: if (!input.presentation_url.empty()) the default value of output->url is empty anyway, what's wrong with assigning an empty url to it?
Also the previous comment about the rename of Callback typeded.
A few little things left (other reviewer's comments seemed to cover the rest of my concerns): https://codereview.chromium.org/1314413005/diff/140001/content/public/browser... File content/public/browser/presentation_service_delegate.h (right): https://codereview.chromium.org/1314413005/diff/140001/content/public/browser... content/public/browser/presentation_service_delegate.h:189: // available. On 2015/10/12 21:56:10, mark a. foltz wrote: > Registers a callback to be notified when the offscreen presentation > corresponding to the frame is available. I'm not sure "register" is the right verb here, though. This is a one-time callback, not a repeat callback. https://codereview.chromium.org/1314413005/diff/140001/content/public/browser... content/public/browser/presentation_service_delegate.h:205: // Unlike |GetPresentationReceiverSession|, this method immediately returns This part of the comment is stale now (GetPresentationReceiverSession was renamed). Suggest you just delete it since the new naming makes things clear. https://codereview.chromium.org/1314413005/diff/140001/content/public/browser... content/public/browser/presentation_service_delegate.h:209: virtual std::vector<content::PresentationSessionInfo> style: Per style guide, consider having the caller provide the vector via an output argument instead: virtual bool GetPresentationReceiverSessions(int, int, std::vector<PresSessInfo>* sessions); (Then you can return false if |render_process_id| and |render_frame_id| are bad references.)
Regarding mfoltz's comment 2, I looked into merging the two PresentationServiceDelegate implementations. The idea is to have PSDImpl hold a controller and a receiver component, and moving the code from ReceiverPSDImpl into PSDImpl's receiver component. Doing that requires me to first clean up the logic for default presentation URL, which is non trivial work in and of itself. I have a patch out (https://codereview.chromium.org/1406013003/) which will probably become a prerequisite of this patch. In the mean time I have addressed the comments in the new patch set. I have also removed GetPresentationReceiverSessions() since we are moving away from navigator.presentation.receiver.getConnections(). https://codereview.chromium.org/1314413005/diff/100001/content/browser/presen... File content/browser/presentation/presentation_service_impl.h (right): https://codereview.chromium.org/1314413005/diff/100001/content/browser/presen... content/browser/presentation/presentation_service_impl.h:101: using PresentationSessionMojoCallback = On 2015/10/07 13:25:53, whywhat wrote: > nit: Could this rename be done separately maybe? It seems to affect some files > exclusively so distracts from the main change? I am changing this in a separate CL I am currently working on. See top level comment. https://codereview.chromium.org/1314413005/diff/140001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/1314413005/diff/140001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:2610: } else { On 2015/10/13 15:21:30, whywhat wrote: > nit: I believe > > if (a) > return b; > else > return c; > > is usually written as > > if (a) > return b; > > return c; Done. https://codereview.chromium.org/1314413005/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/media_route.h (right): https://codereview.chromium.org/1314413005/diff/140001/chrome/browser/media/r... chrome/browser/media/router/media_route.h:74: bool is_offscreen_presentation() const { return is_offscreen_presentation_; } On 2015/10/12 21:56:10, mark a. foltz wrote: > Please document the meaning of is_offscreen_presentation. Done. https://codereview.chromium.org/1314413005/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/media_router_type_converters.cc (right): https://codereview.chromium.org/1314413005/diff/140001/chrome/browser/media/r... chrome/browser/media/router/media_router_type_converters.cc:64: route.set_is_offscreen_presentation(input->is_offscreen_presentation); On 2015/10/13 15:21:30, whywhat wrote: > nit: it feels like for consistency is_offscreen_presentation should just be > passed in the constructor like the rest of the parameters. The problem is the MediaRoute class is starting to have too many constructor arguments. The constructor should take fewer arguments with fields set to default values initially, which can then be modified with setters. This will need to be fixed in a separate patch. See Yuri's comment in an earlier PS. https://codereview.chromium.org/1314413005/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/offscreen_presentation_manager.cc (right): https://codereview.chromium.org/1314413005/diff/140001/chrome/browser/media/r... chrome/browser/media/router/offscreen_presentation_manager.cc:49: state_change_listener_ = listener; On 2015/10/13 15:21:30, whywhat wrote: > nit: DCHECK(listener); Done. https://codereview.chromium.org/1314413005/diff/140001/chrome/browser/media/r... chrome/browser/media/router/offscreen_presentation_manager.cc:54: if (state_change_listener_) On 2015/10/13 15:21:30, whywhat wrote: > Shouldn't be a DCHECK rather so we always have a listener for state changes > before they happen? Or should we queue them? I wouldn't expect a listener to always exist when a state change occurs. I don't think it's important to queue the state changes. The page can get the current state of the session already. https://codereview.chromium.org/1314413005/diff/140001/chrome/browser/media/r... chrome/browser/media/router/offscreen_presentation_manager.cc:63: if (!messages_callback_.is_null()) { On 2015/10/13 15:21:30, whywhat wrote: > nit: Seems like the inverse condition would be more readable: > > if (messages_callback_.is_null()) > return; > > Should we queue messages until the callback is given or assert it's there by the > time when the messages come? Done. For queueing, it can be done along with batching. We can have a bounded fifo queue for it. https://codereview.chromium.org/1314413005/diff/140001/chrome/browser/media/r... chrome/browser/media/router/offscreen_presentation_manager.cc:64: ScopedVector<content::PresentationSessionMessage> messages; On 2015/10/12 21:56:10, mark a. foltz wrote: > Please add a TODO to handle multiple messages at once. Moved comment from OffscreenPresentationConnection https://codereview.chromium.org/1314413005/diff/140001/chrome/browser/media/r... chrome/browser/media/router/offscreen_presentation_manager.cc:75: DCHECK(!ContainsKey(offscreen_presentations_, presentation_id)); On 2015/10/12 21:56:10, mark a. foltz wrote: > How do you intend to handle this case in release builds? This is a "can't happen" situation since the presentation id comes from MR. But adding it for sanity check. https://codereview.chromium.org/1314413005/diff/140001/chrome/browser/media/r... chrome/browser/media/router/offscreen_presentation_manager.cc:85: DCHECK(ContainsKey(offscreen_presentations_, presentation_id)); On 2015/10/12 21:56:10, mark a. foltz wrote: > Similar comments Ditto. https://codereview.chromium.org/1314413005/diff/140001/chrome/browser/media/r... chrome/browser/media/router/offscreen_presentation_manager.cc:159: session->OnMessageReceived(message.Pass()); On 2015/10/13 15:21:30, whywhat wrote: > Did you mean other_session? Done. Good catch, thanks! https://codereview.chromium.org/1314413005/diff/140001/chrome/browser/media/r... chrome/browser/media/router/offscreen_presentation_manager.cc:175: // The receiver must have destroyed all connections before unregistration. On 2015/10/12 21:56:10, mark a. foltz wrote: > Can unregistration do this as a side effect? > > It would seem cleaner for the OPM to watch the WebContents and the OPM to > coordinate the teardown of all the presentation sessions in response to > window.close(), session.terminate(), failed streaming or navigation, instead of > having each of these code paths manage the lifetimes of the sessions. All of the scenarios should already be covered by the offscreen WebContents being destroyed. RPSDImpl's lifetime is already tied to it, so it's pretty straightforward to destroy all sessions and unregister in RPSDImpl's dtor. https://codereview.chromium.org/1314413005/diff/140001/chrome/browser/media/r... chrome/browser/media/router/offscreen_presentation_manager.cc:182: DCHECK(!ContainsKey(connections_, controller_frame_id)); On 2015/10/12 21:56:10, mark a. foltz wrote: > Behavior in release builds? Changed this to LOG(ERROR) and return nullptr. In general we should prevent having multiple presentations with same id in a controlling context. https://codereview.chromium.org/1314413005/diff/140001/chrome/browser/media/r... chrome/browser/media/router/offscreen_presentation_manager.cc:199: DCHECK(ContainsKey(connections_, controller_frame_id)); On 2015/10/12 21:56:10, mark a. foltz wrote: > Ditto erase() is no-op is key is not found so we should be fine here. https://codereview.chromium.org/1314413005/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/offscreen_presentation_manager.h (right): https://codereview.chromium.org/1314413005/diff/140001/chrome/browser/media/r... chrome/browser/media/router/offscreen_presentation_manager.h:40: // [Calls methods in |receiver_session| to send/receive messages, etc.] On 2015/10/12 21:56:10, mark a. foltz wrote: > Nitpick: I wouldn't expect this to happen in this callback, rather, it would > happen in different callbacks based on send() invocations. > > Maybe instead: > > [Retains |receiver_session| to send/receive messages in response to send()] > > > Done. https://codereview.chromium.org/1314413005/diff/140001/chrome/browser/media/r... chrome/browser/media/router/offscreen_presentation_manager.h:45: // to controller frame) and the receiver session (given to receiver). Note On 2015/10/12 21:56:10, mark a. foltz wrote: > To be clear, each controller <--> receiver relationship corresponds to exactly > one pair of OffscreenPresentationSessions (one for the controller, one for the > receiver)? Yes that's correct. https://codereview.chromium.org/1314413005/diff/140001/chrome/browser/media/r... chrome/browser/media/router/offscreen_presentation_manager.h:57: // When the receiver is no longer associated with an offscreen presentation, it On 2015/10/12 21:56:10, mark a. foltz wrote: > Which object is "the receiver" in this context? It seems like this could be the > job of the OffscreenPresentationManager (by observing the offscreen > tab/WebContents)? The receiver is the party that called RegisterOffscreenPresentationReceiver which in our case is ReceiverPresentationServiceDelegateImpl. As discussed in person, since RPSDImpl's lifetime is tied to that of the offscreen tab's, it's fairly easy for it to destroy all OffscreenPresentationSession objects and unregister itself with OPM when the offscreen presentation is about to end. https://codereview.chromium.org/1314413005/diff/140001/chrome/browser/media/r... chrome/browser/media/router/offscreen_presentation_manager.h:62: // receiver_sessions.clear(); On 2015/10/12 21:56:10, mark a. foltz wrote: > Where does the receiver get |receiver_sessions|? From multiple invocations of > OnReceiverSession()? Yes. I added an example above using |receiver_sessions|. https://codereview.chromium.org/1314413005/diff/140001/chrome/browser/media/r... chrome/browser/media/router/offscreen_presentation_manager.h:70: // Forward declarations for OffscreenPresentationSession. On 2015/10/13 15:21:30, whywhat wrote: > nit: I think this comment is redundant. I added it to justify the extra 'private:' portion here which usually does not preceed 'public:'. https://codereview.chromium.org/1314413005/diff/140001/chrome/browser/media/r... chrome/browser/media/router/offscreen_presentation_manager.h:99: void ListenForStateChanges( On 2015/10/13 15:21:30, whywhat wrote: > nit: missed a comment for this method? Done. https://codereview.chromium.org/1314413005/diff/140001/chrome/browser/media/r... chrome/browser/media/router/offscreen_presentation_manager.h:113: content::PresentationSessionStateListener* state_change_listener_; On 2015/10/13 15:21:30, whywhat wrote: > nit: make it const pointer? It cannot be const since state_change_listener_ is set when ListenForstateChanges is called. Also non-const methods are called on state_change_listener_. https://codereview.chromium.org/1314413005/diff/140001/chrome/browser/media/r... chrome/browser/media/router/offscreen_presentation_manager.h:182: // Not owned by this class. On 2015/10/13 15:21:30, whywhat wrote: > nit: Should this comment relate to the pointer above as well? Done. https://codereview.chromium.org/1314413005/diff/140001/chrome/browser/media/r... chrome/browser/media/router/offscreen_presentation_manager.h:182: // Not owned by this class. On 2015/10/13 15:21:30, whywhat wrote: > nit: Should this comment relate to the pointer above as well? Done. https://codereview.chromium.org/1314413005/diff/140001/chrome/browser/media/r... chrome/browser/media/router/offscreen_presentation_manager.h:183: OffscreenPresentationSession* controller_; On 2015/10/13 15:21:31, whywhat wrote: > nit: const pointers? They are non-const because they are set during Init(). This is due to a circular reference between this class and OffscreenPresentationSession. See comments in earlier PS. https://codereview.chromium.org/1314413005/diff/140001/chrome/browser/media/r... chrome/browser/media/router/offscreen_presentation_manager.h:230: } // namespace media_router On 2015/10/13 15:21:31, whywhat wrote: > nit: I know this is kind of the current style of the media/router codebase but > I'd prefer better separation between all the classes (there're four in one > header and worse in .cc). Could you at least separate the implementations in the > .cc file better (with a 80 char comment line and more line breaks?) Done. LGTY? https://codereview.chromium.org/1314413005/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/presentation_service_delegate_impl.cc (right): https://codereview.chromium.org/1314413005/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:130: // Maps from presentation ID to the corresponding OffscreenController within On 2015/10/12 21:56:10, mark a. foltz wrote: > Can this paragraph be wrapped? Done. https://codereview.chromium.org/1314413005/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:190: DCHECK(!ContainsKey(offscreen_presentation_sessions_, presentation_id)); On 2015/10/12 21:56:10, mark a. foltz wrote: > Behavior in release builds? Changed ConnectToOffscreenPresentation to return nullptr in that case. There are actually some legitimate corner cases that could cause this to happen, so I removed the DCHECK too and added documentation. https://codereview.chromium.org/1314413005/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:272: auto it = presentation_id_to_route_id_.find(session.presentation_id); On 2015/10/12 21:56:10, mark a. foltz wrote: > This lookup only really makes sense for 2-UA presentations (no > |offscreen_session|) since there the page is messaging the route directly. Move > to line 284? Done. Also done similarly for below. https://codereview.chromium.org/1314413005/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:316: } else { On 2015/10/13 15:21:31, whywhat wrote: > nit: remove else Done. https://codereview.chromium.org/1314413005/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:814: // See ReceiverPresentationServiceDelegateImpl for details. On 2015/10/12 21:56:10, mark a. foltz wrote: > We need to decide how to handle the receiver APIs in Blink. If we plumb through > a bit saying a frame belongs to an offscreen presentation, we could switch the > APIs between being no-ops and calling through to content. > > Also see https://github.com/w3c/presentation-api/issues/205 Ok, I think we decided on PresentationServiceDelegateImpl having both controller and receiver logic. For the time being we could distinguish controller vs. receiver context at the WebContents level (i.e., in OffscreenPresentation, register offscreen tab WebContents as receiver context. All other Webcontents are considered controller). Agreed that we will need additional mechanism if it becomes a frame level property and/or allow a frame to be both controller and receiver. https://codereview.chromium.org/1314413005/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/receiver_presentation_service_delegate_impl.cc (right): https://codereview.chromium.org/1314413005/diff/140001/chrome/browser/media/r... chrome/browser/media/router/receiver_presentation_service_delegate_impl.cc:27: DCHECK(web_contents); On 2015/10/12 21:56:10, mark a. foltz wrote: > Who is responsible for checking that the WebContents is appropriate for > presentations (i.e. independent incognito profile)? > > For example, is the offscreen tab API calling this method with the WebContents > it has created? Right, offscreen tab API calls this with the offscreen tab only. We can add certainly add DCHECKs since code here does depend on the WebContents being from incognito context. https://codereview.chromium.org/1314413005/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/render_frame_host_helper.h (right): https://codereview.chromium.org/1314413005/diff/140001/chrome/browser/media/r... chrome/browser/media/router/render_frame_host_helper.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2015/10/12 21:56:10, mark a. foltz wrote: > Maybe this should be named render_frame_host_id.h? Done. https://codereview.chromium.org/1314413005/diff/140001/chrome/browser/media/r... chrome/browser/media/router/render_frame_host_helper.h:16: std::pair<decltype(((content::RenderProcessHost*)nullptr)->GetID()), On 2015/10/12 21:56:10, mark a. foltz wrote: > Is decltype necessary here? It seems to only be recommended for rare situations > like variadic templates. > > https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/_zoNvZd_dSo > > If the types of the ids get widened to long (which seems unlikely) you should > get a compile error when trying to assign them to pair<int, int>. > Done. https://codereview.chromium.org/1314413005/diff/140001/content/browser/presen... File content/browser/presentation/presentation_type_converters.h (right): https://codereview.chromium.org/1314413005/diff/140001/content/browser/presen... content/browser/presentation/presentation_type_converters.h:31: if (!input.presentation_url.empty()) On 2015/10/12 21:56:10, mark a. foltz wrote: > What is creating a presentation with an empty URL? PresentationConnection objects in receiving contexts have undefined presentation URL according to the spec. https://codereview.chromium.org/1314413005/diff/140001/content/browser/presen... content/browser/presentation/presentation_type_converters.h:31: if (!input.presentation_url.empty()) On 2015/10/13 15:21:31, whywhat wrote: > the default value of output->url is empty anyway, what's wrong with assigning an > empty url to it? I believe it makes a difference in mojo. mojo::String are initially considered "null". When you assign to it with a std::string, it becomes non-null. So for mojo::String, there is a difference between a "null" empty and a non-null empty. I was trying to use nulls for the url field in content/ code but it turns out to be confusing anyways, so I will just check for empty instead. https://codereview.chromium.org/1314413005/diff/140001/content/common/present... File content/common/presentation/presentation_service.mojom (right): https://codereview.chromium.org/1314413005/diff/140001/content/common/present... content/common/presentation/presentation_service.mojom:111: // null will be returned. On 2015/10/12 21:56:10, mark a. foltz wrote: > This might be simpler as, > > // Returns a PresentationSessionInfo object for this frame, if it is the > // main frame of an offscreen presentation. Otherwise returns null. Done. https://codereview.chromium.org/1314413005/diff/140001/content/common/present... content/common/presentation/presentation_service.mojom:113: // with url set to empty, and id set to the ID of the offscreen presentation. On 2015/10/12 21:56:10, mark a. foltz wrote: > Can we look up the URL from the PresentationRequest that generated the offscreen > presentation? We can certainly. But I believe spec says URL is undefined for receiver PresentationConnection objects so there is no use in including it here. https://codereview.chromium.org/1314413005/diff/140001/content/public/browser... File content/public/browser/presentation_service_delegate.h (right): https://codereview.chromium.org/1314413005/diff/140001/content/public/browser... content/public/browser/presentation_service_delegate.h:204: // hosted on the offscreen tab containing this frame. On 2015/10/12 21:56:10, mark a. foltz wrote: > Returns all presentation sessions for the offscreen presentation corresponding > to the given frame (if any). Done. https://codereview.chromium.org/1314413005/diff/140001/content/public/browser... content/public/browser/presentation_service_delegate.h:205: // Unlike |GetPresentationReceiverSession|, this method immediately returns On 2015/10/14 01:54:57, miu_NO_REVIEWING_UNTIL_OCT_19 wrote: > This part of the comment is stale now (GetPresentationReceiverSession was > renamed). Suggest you just delete it since the new naming makes things clear. Done. https://codereview.chromium.org/1314413005/diff/140001/content/public/browser... content/public/browser/presentation_service_delegate.h:209: virtual std::vector<content::PresentationSessionInfo> On 2015/10/14 01:54:57, miu_NO_REVIEWING_UNTIL_OCT_19 wrote: > style: Per style guide, consider having the caller provide the vector via an > output argument instead: > > virtual bool GetPresentationReceiverSessions(int, int, > std::vector<PresSessInfo>* sessions); > > (Then you can return false if |render_process_id| and |render_frame_id| are bad > references.) Done.
lgtm
More minor things in Patch Set 8: https://codereview.chromium.org/1314413005/diff/160001/chrome/browser/media/r... File chrome/browser/media/router/offscreen_presentation_manager.cc (right): https://codereview.chromium.org/1314413005/diff/160001/chrome/browser/media/r... chrome/browser/media/router/offscreen_presentation_manager.cc:196: LOG(ERROR) << "Frame " << controller_frame_id.first << ", " nit: LOG(DFATAL) instead? https://codereview.chromium.org/1314413005/diff/160001/chrome/browser/media/r... File chrome/browser/media/router/presentation_service_delegate_impl.h (right): https://codereview.chromium.org/1314413005/diff/160001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.h:215: scoped_ptr<PresentationFrameManager> frame_manager_; This should not be a scoped_ptr<> type. Avoid the extra indirection by just using: PresentationFrameManager frame_manager_; https://codereview.chromium.org/1314413005/diff/160001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.h:216: scoped_ptr<PresentationController> controller_; This new data member appears to be unused. https://codereview.chromium.org/1314413005/diff/160001/content/browser/presen... File content/browser/presentation/presentation_service_impl.cc (right): https://codereview.chromium.org/1314413005/diff/160001/content/browser/presen... content/browser/presentation/presentation_service_impl.cc:363: if (default_presentation_url_ == new_default_url) Why is this check here? It feels like a waste of memory to store a copy of this string just to de-dupe calls to SetDefaultPresentationURL() with the same url. Perhaps the delegate should do this (and only if it really needs to for correctness)? https://codereview.chromium.org/1314413005/diff/160001/content/browser/presen... content/browser/presentation/presentation_service_impl.cc:460: DCHECK(receiver_session_callback_.is_null()); and DCHECK(!callback.is_null()) https://codereview.chromium.org/1314413005/diff/160001/content/browser/presen... content/browser/presentation/presentation_service_impl.cc:475: if (!receiver_session_callback_.is_null()) { nit: Reduce indent on majority of the code by negating this return statement. Or, perhaps it should be a DCHECK(!receiver_session_callback_.is_null())?
Was this change abandoned? If so, please close it.
I have just started rebasing this patch (tons of merge conflicts). I will upload a new PS as soon as that is done.
I have rebased and brought the API support to align with the latest spec. Next up is to write some tests for the new code.
Description was changed from ========== [Presentation API] 1-UA presentation support + presenter APIs. This patch implements the 1-UA mode support according to the doc I sent out a while ago. Added getSession() to presentation_service.mojom. getSessions() is not yet implemented; see TODO below. Implemented getSession() of Presentation API for offscreen presentations in PresentationServiceImpl. TODO: - Need additional identifier for PresentationSessionInfo objects. Current proposal is to use RFH ID and limit 1 PresentationSession per presentation ID per frame. Until then we can't really have multiple controllers / getSessions(). BUG=513859 ========== to ========== [Presentation API] 1-UA presentation support + presenter APIs. This patch implements the 1-UA mode support according to the doc I sent out a while ago. Contains the following major components: - OffscreenPresentationManager - browser context keyed service to connect the controller and receiver sides of an 1-ua presentation. Instance is shared with incognito browser contexts to allow communication with the offscreen tab. - ReceiverPresentationServiceDelegateImpl - receiver-side implementation of PresentationServiceDelegate, only available to offscreen tabs. talks to OffscreenPresentationManager. - PresentationServiceDelegateImpl - modified logic to maintain controller side of offscreen presentations and redirect incoming messages, as well as listen for messages / state changes from the receiver side. TODO: - Need additional identifier for PresentationSessionInfo objects. Current proposal is to use RFH ID and limit 1 PresentationSession per presentation ID per frame. Until then we can't really have multiple controllers / getSessions(). BUG=513859 ==========
Patchset #13 (id:260001) has been deleted
Description was changed from ========== [Presentation API] 1-UA presentation support + presenter APIs. This patch implements the 1-UA mode support according to the doc I sent out a while ago. Contains the following major components: - OffscreenPresentationManager - browser context keyed service to connect the controller and receiver sides of an 1-ua presentation. Instance is shared with incognito browser contexts to allow communication with the offscreen tab. - ReceiverPresentationServiceDelegateImpl - receiver-side implementation of PresentationServiceDelegate, only available to offscreen tabs. talks to OffscreenPresentationManager. - PresentationServiceDelegateImpl - modified logic to maintain controller side of offscreen presentations and redirect incoming messages, as well as listen for messages / state changes from the receiver side. TODO: - Need additional identifier for PresentationSessionInfo objects. Current proposal is to use RFH ID and limit 1 PresentationSession per presentation ID per frame. Until then we can't really have multiple controllers / getSessions(). BUG=513859 ========== to ========== [Presentation API] 1-UA presentation support + presenter APIs. This patch implements the 1-UA mode support according to the doc I sent out a while ago. Contains the following major components: - OffscreenPresentationManager - browser context keyed service to connect the controller and receiver sides of an 1-ua presentation. Instance is shared with incognito browser contexts to allow communication with the offscreen tab. - ReceiverPresentationServiceDelegateImpl - receiver-side implementation of PresentationServiceDelegate, only available to offscreen tabs. Talks to OffscreenPresentationManager. - PresentationServiceDelegateImpl - modified logic to maintain controller side of offscreen presentations and redirect incoming messages, as well as listen for messages / state changes from the receiver side. TODO: - Need additional identifier for PresentationSessionInfo objects. Current proposal is to use RFH ID and limit 1 PresentationSession per presentation ID per frame. Until then we can't really have multiple controllers / getSessions(). BUG=513859 ==========
Rebased, updated the patch according to current specs, and added tests. You can view the changes by diffing against ps8. Two follow-up changes after this CL: - make it work for multiple controllers. - extension-initiated state change on the controller should be propagated to the receiver; this requires changes to PSDImpl to intercept state changes. https://codereview.chromium.org/1314413005/diff/160001/chrome/browser/media/r... File chrome/browser/media/router/offscreen_presentation_manager.cc (right): https://codereview.chromium.org/1314413005/diff/160001/chrome/browser/media/r... chrome/browser/media/router/offscreen_presentation_manager.cc:196: LOG(ERROR) << "Frame " << controller_frame_id.first << ", " On 2015/10/20 00:56:50, miu (OOO June 11-19) wrote: > nit: LOG(DFATAL) instead? Changed to DLOG(ERROR). We probably do not need this log to show up in release builds. https://codereview.chromium.org/1314413005/diff/160001/chrome/browser/media/r... File chrome/browser/media/router/presentation_service_delegate_impl.h (right): https://codereview.chromium.org/1314413005/diff/160001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.h:215: scoped_ptr<PresentationFrameManager> frame_manager_; On 2015/10/20 00:56:50, miu (OOO June 11-19) wrote: > This should not be a scoped_ptr<> type. Avoid the extra indirection by just > using: > > PresentationFrameManager frame_manager_; PresentationFrameManager is defined in the .cc file in an anonymous namespace, thus it is forward declared here and has to be the form of a pointer. Though I will say this has made things harder to test so we should look into whether it's worth it to move it into the header. https://codereview.chromium.org/1314413005/diff/160001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.h:216: scoped_ptr<PresentationController> controller_; On 2015/10/20 00:56:50, miu (OOO June 11-19) wrote: > This new data member appears to be unused. Removed. https://codereview.chromium.org/1314413005/diff/160001/content/browser/presen... File content/browser/presentation/presentation_service_impl.cc (right): https://codereview.chromium.org/1314413005/diff/160001/content/browser/presen... content/browser/presentation/presentation_service_impl.cc:363: if (default_presentation_url_ == new_default_url) On 2015/10/20 00:56:50, miu (OOO June 11-19) wrote: > Why is this check here? It feels like a waste of memory to store a copy of this > string just to de-dupe calls to SetDefaultPresentationURL() with the same url. > Perhaps the delegate should do this (and only if it really needs to for > correctness)? It looks like PSDImpl already does some form of deduplication. We can remove the field and the duplicate checking logic here in a separate patch. https://codereview.chromium.org/1314413005/diff/160001/content/browser/presen... content/browser/presentation/presentation_service_impl.cc:460: DCHECK(receiver_session_callback_.is_null()); On 2015/10/20 00:56:50, miu (OOO June 11-19) wrote: > and DCHECK(!callback.is_null()) Done. https://codereview.chromium.org/1314413005/diff/160001/content/browser/presen... content/browser/presentation/presentation_service_impl.cc:475: if (!receiver_session_callback_.is_null()) { On 2015/10/20 00:56:50, miu (OOO June 11-19) wrote: > nit: Reduce indent on majority of the code by negating this return statement. > Or, perhaps it should be a DCHECK(!receiver_session_callback_.is_null())? Removed.
[end of year cleanup] I think we're not planning on landing this, correct? Can this issue be closed now? |
