Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
839773002:
Plumbing from WebPresentationClient to the Presentation Mojo service for (Closed)
|
Created:
5 years, 11 months ago by whywhat Modified:
5 years, 10 months ago Reviewers:
Avi (use Gerrit), mlamouri (slow - plz ping), Peter Beverloo, mark a. foltz, imcheng, qsr, blundell CC:
Aaron Boodman, abarth-chromium, ben+mojo_chromium.org, chromium-reviews, creis+watch_chromium.org, darin (slow to review), darin-cc_chromium.org, jam, mkwst+moarreviews-renderer_chromium.org, nasko+codewatch_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, imcheng Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPlumbing from WebPresentationClient to the Presentation Mojo service for
availablechange event. Blink CL: https://codereview.chromium.org/832263007
BUG=412331
Committed: https://crrev.com/bca35fadafda9c1d6494fb8f33cbd06500d063e5
Cr-Commit-Position: refs/heads/master@{#313772}
Patch Set 1 #
Total comments: 22
Patch Set 2 : Addressed Peter's comments #Patch Set 3 : Fixed the build #
Total comments: 22
Patch Set 4 : #
Total comments: 6
Patch Set 5 : Got rid of the PresentationServiceClient #
Total comments: 4
Patch Set 6 : Swapped handling the event and requesting a new one #
Total comments: 26
Patch Set 7 : #Patch Set 8 : Added OnChangeAvailabilityListenerRemoved #
Total comments: 12
Patch Set 9 : CHECK->DCHECK, corrected comments in mojom #Patch Set 10 : Rebase #Patch Set 11 : Fixed various trybot failures. #Patch Set 12 : Updated comment #Messages
Total messages: 48 (10 generated)
avayvod@chromium.org changed reviewers: + peter@chromium.org
Initial round of review(s), please.
https://codereview.chromium.org/839773002/diff/1/content/common/presentation_... File content/common/presentation_service.mojom (right): https://codereview.chromium.org/839773002/diff/1/content/common/presentation_... content/common/presentation_service.mojom:1: // Copyright 2014 The Chromium Authors. All rights reserved. nit: 2015. Elsewhere too. https://codereview.chromium.org/839773002/diff/1/content/common/presentation_... content/common/presentation_service.mojom:10: interface PresentationService { Nothing provides this service yet, right? I very much prefer adding both sides of {IPC, Mojo} communication in the same patch, even when it's very bare-bone. https://codereview.chromium.org/839773002/diff/1/content/common/presentation_... content/common/presentation_service.mojom:13: // when the default presentation url/id are parsed first and/or when the frame While I'm not the appropriate reviewer for Mojo stuff, I don't like how these comments consistently lack grammar and capitals. https://codereview.chromium.org/839773002/diff/1/content/common/presentation_... content/common/presentation_service.mojom:15: SetDefaultPresentation( This is not being used yet. The same goes for DefaultPresentationListenerChanged, StartSession and JoinSession. https://codereview.chromium.org/839773002/diff/1/content/common/presentation_... File content/common/presentation_session.mojom (right): https://codereview.chromium.org/839773002/diff/1/content/common/presentation_... content/common/presentation_session.mojom:11: interface PresentationSession { If you prune the unused methods in the other .mojom file, we can avoid adding this entire module right now. https://codereview.chromium.org/839773002/diff/1/content/renderer/presentatio... File content/renderer/presentation/presentation_dispatcher.cc (right): https://codereview.chromium.org/839773002/diff/1/content/renderer/presentatio... content/renderer/presentation/presentation_dispatcher.cc:25: ConnectToPresentationServiceIfNeeded(); nit: indent. In removeDeviceAvailabilityListener too. https://codereview.chromium.org/839773002/diff/1/content/renderer/presentatio... content/renderer/presentation/presentation_dispatcher.cc:41: } NOTIMPLEMENTED(). https://codereview.chromium.org/839773002/diff/1/content/renderer/presentatio... File content/renderer/presentation/presentation_dispatcher.h (right): https://codereview.chromium.org/839773002/diff/1/content/renderer/presentatio... content/renderer/presentation/presentation_dispatcher.h:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. nit: no (c). https://codereview.chromium.org/839773002/diff/1/content/renderer/presentatio... content/renderer/presentation/presentation_dispatcher.h:22: virtual ~PresentationDispatcher(); ~PresentationDispatcher() override; https://codereview.chromium.org/839773002/diff/1/content/renderer/presentatio... content/renderer/presentation/presentation_dispatcher.h:25: // WebPresentationClient nit: "WebPresentationClient implementation." or similar. https://codereview.chromium.org/839773002/diff/1/content/renderer/presentatio... content/renderer/presentation/presentation_dispatcher.h:27: blink::WebPresentationController* controller) override; We don't use "override" for Blink API methods (it makes three-sided patches a pain). Elsewhere, please only use one of {virtual, override} per Chromium style.
https://codereview.chromium.org/839773002/diff/1/content/common/presentation_... File content/common/presentation_service.mojom (right): https://codereview.chromium.org/839773002/diff/1/content/common/presentation_... content/common/presentation_service.mojom:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2015/01/07 15:13:00, Peter Beverloo wrote: > nit: 2015. Elsewhere too. Done. https://codereview.chromium.org/839773002/diff/1/content/common/presentation_... content/common/presentation_service.mojom:10: interface PresentationService { On 2015/01/07 15:13:00, Peter Beverloo wrote: > Nothing provides this service yet, right? I very much prefer adding both sides > of {IPC, Mojo} communication in the same patch, even when it's very bare-bone. Done. https://codereview.chromium.org/839773002/diff/1/content/common/presentation_... content/common/presentation_service.mojom:13: // when the default presentation url/id are parsed first and/or when the frame On 2015/01/07 15:13:00, Peter Beverloo wrote: > While I'm not the appropriate reviewer for Mojo stuff, I don't like how these > comments consistently lack grammar and capitals. Done. https://codereview.chromium.org/839773002/diff/1/content/common/presentation_... content/common/presentation_service.mojom:15: SetDefaultPresentation( On 2015/01/07 15:13:01, Peter Beverloo wrote: > This is not being used yet. > > The same goes for DefaultPresentationListenerChanged, StartSession and > JoinSession. Removed from this CL. https://codereview.chromium.org/839773002/diff/1/content/common/presentation_... File content/common/presentation_session.mojom (right): https://codereview.chromium.org/839773002/diff/1/content/common/presentation_... content/common/presentation_session.mojom:11: interface PresentationSession { On 2015/01/07 15:13:01, Peter Beverloo wrote: > If you prune the unused methods in the other .mojom file, we can avoid adding > this entire module right now. Done. https://codereview.chromium.org/839773002/diff/1/content/renderer/presentatio... File content/renderer/presentation/presentation_dispatcher.cc (right): https://codereview.chromium.org/839773002/diff/1/content/renderer/presentatio... content/renderer/presentation/presentation_dispatcher.cc:25: ConnectToPresentationServiceIfNeeded(); On 2015/01/07 15:13:01, Peter Beverloo wrote: > nit: indent. In removeDeviceAvailabilityListener too. Done. https://codereview.chromium.org/839773002/diff/1/content/renderer/presentatio... content/renderer/presentation/presentation_dispatcher.cc:41: } On 2015/01/07 15:13:01, Peter Beverloo wrote: > NOTIMPLEMENTED(). Removed. https://codereview.chromium.org/839773002/diff/1/content/renderer/presentatio... File content/renderer/presentation/presentation_dispatcher.h (right): https://codereview.chromium.org/839773002/diff/1/content/renderer/presentatio... content/renderer/presentation/presentation_dispatcher.h:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. On 2015/01/07 15:13:01, Peter Beverloo wrote: > nit: no (c). Done. https://codereview.chromium.org/839773002/diff/1/content/renderer/presentatio... content/renderer/presentation/presentation_dispatcher.h:22: virtual ~PresentationDispatcher(); On 2015/01/07 15:13:01, Peter Beverloo wrote: > ~PresentationDispatcher() override; Done. https://codereview.chromium.org/839773002/diff/1/content/renderer/presentatio... content/renderer/presentation/presentation_dispatcher.h:25: // WebPresentationClient On 2015/01/07 15:13:01, Peter Beverloo wrote: > nit: "WebPresentationClient implementation." or similar. Done. https://codereview.chromium.org/839773002/diff/1/content/renderer/presentatio... content/renderer/presentation/presentation_dispatcher.h:27: blink::WebPresentationController* controller) override; On 2015/01/07 15:13:01, Peter Beverloo wrote: > We don't use "override" for Blink API methods (it makes three-sided patches a > pain). Elsewhere, please only use one of {virtual, override} per Chromium style. Done.
peter@chromium.org changed reviewers: + mlamouri@chromium.org
Mostly lg, but please ask someone to take a look at the Mojo side of things. +mlamouri for a first look, perhaps? https://codereview.chromium.org/839773002/diff/40001/content/common/presentat... File content/common/presentation_service.mojom (right): https://codereview.chromium.org/839773002/diff/40001/content/common/presentat... content/common/presentation_service.mojom:23: // Fires the |availablechange| event. +"in JavaScript." https://codereview.chromium.org/839773002/diff/40001/content/renderer/present... File content/renderer/presentation/OWNERS (right): https://codereview.chromium.org/839773002/diff/40001/content/renderer/present... content/renderer/presentation/OWNERS:2: mfoltz@chromium.org Why isn't this aligned with content/browser/presentation/OWNERS? https://codereview.chromium.org/839773002/diff/40001/content/renderer/present... File content/renderer/presentation/presentation_dispatcher.cc (right): https://codereview.chromium.org/839773002/diff/40001/content/renderer/present... content/renderer/presentation/presentation_dispatcher.cc:39: if (!presentation_service_.get()) { nit: I'd prefer early returns over a block like this encapsulating the function's entire body. https://codereview.chromium.org/839773002/diff/40001/content/renderer/present... File content/renderer/presentation/presentation_dispatcher.h (right): https://codereview.chromium.org/839773002/diff/40001/content/renderer/present... content/renderer/presentation/presentation_dispatcher.h:36: blink::WebPresentationController* controller_; nit: maybe add a comment about controller_ being a weak reference?
lg but I think you will need to remove the Client= notation in your mojo interface. https://codereview.chromium.org/839773002/diff/40001/content/browser/presenta... File content/browser/presentation/presentation_service_impl.cc (right): https://codereview.chromium.org/839773002/diff/40001/content/browser/presenta... content/browser/presentation/presentation_service_impl.cc:33: } Remove this. https://codereview.chromium.org/839773002/diff/40001/content/browser/presenta... File content/browser/presentation/presentation_service_impl.h (right): https://codereview.chromium.org/839773002/diff/40001/content/browser/presenta... content/browser/presentation/presentation_service_impl.h:22: // probably be required for the actual implementation. I don't think you need that TODO. You will simply change |CreateService| if you need to. https://codereview.chromium.org/839773002/diff/40001/content/browser/presenta... content/browser/presentation/presentation_service_impl.h:35: void OnConnectionError() override; You don't use |OnConnectionError|, just remove it. https://codereview.chromium.org/839773002/diff/40001/content/common/presentat... File content/common/presentation_service.mojom (right): https://codereview.chromium.org/839773002/diff/40001/content/common/presentat... content/common/presentation_service.mojom:7: [Client=PresentationServiceClient] My understanding is that we want to get ride of Client... https://codereview.chromium.org/839773002/diff/40001/content/common/presentat... content/common/presentation_service.mojom:13: AddDeviceAvailabilityListener(); ... instead, you could pass an interface in this method. https://codereview.chromium.org/839773002/diff/40001/content/common/presentat... content/common/presentation_service.mojom:18: RemoveDeviceAvailabilityListener(); ... and that one. https://codereview.chromium.org/839773002/diff/40001/content/renderer/present... File content/renderer/presentation/presentation_dispatcher.h (right): https://codereview.chromium.org/839773002/diff/40001/content/renderer/present... content/renderer/presentation/presentation_dispatcher.h:36: blink::WebPresentationController* controller_; On 2015/01/07 at 19:41:21, Peter Beverloo wrote: > nit: maybe add a comment about controller_ being a weak reference? A comment pointing that it is a weak reference would simply state the obvious. You might want to point whether it can be null though and its lifetime.
https://codereview.chromium.org/839773002/diff/40001/content/browser/presenta... File content/browser/presentation/presentation_service_impl.cc (right): https://codereview.chromium.org/839773002/diff/40001/content/browser/presenta... content/browser/presentation/presentation_service_impl.cc:33: } On 2015/01/08 15:08:53, Mounir Lamouri wrote: > Remove this. Done. https://codereview.chromium.org/839773002/diff/40001/content/browser/presenta... File content/browser/presentation/presentation_service_impl.h (right): https://codereview.chromium.org/839773002/diff/40001/content/browser/presenta... content/browser/presentation/presentation_service_impl.h:22: // probably be required for the actual implementation. On 2015/01/08 15:08:54, Mounir Lamouri wrote: > I don't think you need that TODO. You will simply change |CreateService| if you > need to. Done. https://codereview.chromium.org/839773002/diff/40001/content/browser/presenta... content/browser/presentation/presentation_service_impl.h:35: void OnConnectionError() override; On 2015/01/08 15:08:54, Mounir Lamouri wrote: > You don't use |OnConnectionError|, just remove it. Done. https://codereview.chromium.org/839773002/diff/40001/content/common/presentat... File content/common/presentation_service.mojom (right): https://codereview.chromium.org/839773002/diff/40001/content/common/presentat... content/common/presentation_service.mojom:7: [Client=PresentationServiceClient] On 2015/01/08 15:08:54, Mounir Lamouri wrote: > My understanding is that we want to get ride of Client... Done. https://codereview.chromium.org/839773002/diff/40001/content/common/presentat... content/common/presentation_service.mojom:13: AddDeviceAvailabilityListener(); On 2015/01/08 15:08:54, Mounir Lamouri wrote: > ... instead, you could pass an interface in this method. Done. https://codereview.chromium.org/839773002/diff/40001/content/common/presentat... content/common/presentation_service.mojom:18: RemoveDeviceAvailabilityListener(); On 2015/01/08 15:08:54, Mounir Lamouri wrote: > ... and that one. Got it. Very convenient :( https://codereview.chromium.org/839773002/diff/40001/content/common/presentat... content/common/presentation_service.mojom:23: // Fires the |availablechange| event. On 2015/01/07 19:41:21, Peter Beverloo wrote: > +"in JavaScript." Done. https://codereview.chromium.org/839773002/diff/40001/content/renderer/present... File content/renderer/presentation/OWNERS (right): https://codereview.chromium.org/839773002/diff/40001/content/renderer/present... content/renderer/presentation/OWNERS:2: mfoltz@chromium.org On 2015/01/07 19:41:21, Peter Beverloo wrote: > Why isn't this aligned with content/browser/presentation/OWNERS? Done. https://codereview.chromium.org/839773002/diff/40001/content/renderer/present... File content/renderer/presentation/presentation_dispatcher.cc (right): https://codereview.chromium.org/839773002/diff/40001/content/renderer/present... content/renderer/presentation/presentation_dispatcher.cc:39: if (!presentation_service_.get()) { On 2015/01/07 19:41:21, Peter Beverloo wrote: > nit: I'd prefer early returns over a block like this encapsulating the > function's entire body. Done. https://codereview.chromium.org/839773002/diff/40001/content/renderer/present... File content/renderer/presentation/presentation_dispatcher.h (right): https://codereview.chromium.org/839773002/diff/40001/content/renderer/present... content/renderer/presentation/presentation_dispatcher.h:36: blink::WebPresentationController* controller_; On 2015/01/08 15:08:54, Mounir Lamouri wrote: > On 2015/01/07 at 19:41:21, Peter Beverloo wrote: > > nit: maybe add a comment about controller_ being a weak reference? > > A comment pointing that it is a weak reference would simply state the obvious. > You might want to point whether it can be null though and its lifetime. Done. https://codereview.chromium.org/839773002/diff/40001/content/renderer/present... content/renderer/presentation/presentation_dispatcher.h:36: blink::WebPresentationController* controller_; On 2015/01/07 19:41:21, Peter Beverloo wrote: > nit: maybe add a comment about controller_ being a weak reference? Done.
mlamouri@chromium.org changed reviewers: + blundell@chromium.org
lgtm For the Mojo interface, I'm not sure how you could prevent using the Client model and still have the guarantee of receiving the events you want. I've added Colin who could give some more insights. https://codereview.chromium.org/839773002/diff/60001/content/browser/presenta... File content/browser/presentation/presentation_service_impl.cc (right): https://codereview.chromium.org/839773002/diff/60001/content/browser/presenta... content/browser/presentation/presentation_service_impl.cc:24: presentation::PresentationServiceClientPtr) { nit: add a name https://codereview.chromium.org/839773002/diff/60001/content/browser/presenta... content/browser/presentation/presentation_service_impl.cc:29: void PresentationServiceImpl::UpdateAvailableChangeWatched(bool) { nit: add a name https://codereview.chromium.org/839773002/diff/60001/content/renderer/present... File content/renderer/presentation/presentation_dispatcher.h (right): https://codereview.chromium.org/839773002/diff/60001/content/renderer/present... content/renderer/presentation/presentation_dispatcher.h:19: public blink::WebPresentationClient, You might need NON_EXPORTED_BASE() around blink::WebPresentationClient. I think it will not compile on Windows otherwise, IIRC.
On 2015/01/15 11:40:13, Mounir Lamouri wrote: > lgtm > > For the Mojo interface, I'm not sure how you could prevent using the Client > model and still have the guarantee of receiving the events you want. I've added > Colin who could give some more insights. > > https://codereview.chromium.org/839773002/diff/60001/content/browser/presenta... > File content/browser/presentation/presentation_service_impl.cc (right): > > https://codereview.chromium.org/839773002/diff/60001/content/browser/presenta... > content/browser/presentation/presentation_service_impl.cc:24: > presentation::PresentationServiceClientPtr) { > nit: add a name > > https://codereview.chromium.org/839773002/diff/60001/content/browser/presenta... > content/browser/presentation/presentation_service_impl.cc:29: void > PresentationServiceImpl::UpdateAvailableChangeWatched(bool) { > nit: add a name > > https://codereview.chromium.org/839773002/diff/60001/content/renderer/present... > File content/renderer/presentation/presentation_dispatcher.h (right): > > https://codereview.chromium.org/839773002/diff/60001/content/renderer/present... > content/renderer/presentation/presentation_dispatcher.h:19: public > blink::WebPresentationClient, > You might need NON_EXPORTED_BASE() around blink::WebPresentationClient. I think > it will not compile on Windows otherwise, IIRC.
Is there a reason why the model used by Geolocation doesn't work here? https://codereview.chromium.org/841133002/ Basically, the client of the PresentationService asks it for the next available change. The PresentationServiceImpl queues the received callback and calls it when there is a next available change. When the client of the PresentationService has that callback invoked, in addition to processing the result it also calls the PresentationService again asking it for the next available change. The benefit of this model (as Darin explains in more detail) is that the PresentationServiceImpl only generates messages at the rate that the client is able to process them.
On 2015/01/15 17:02:39, blundell wrote: > Is there a reason why the model used by Geolocation doesn't work here? > > https://codereview.chromium.org/841133002/ > > Basically, the client of the PresentationService asks it for the next available > change. The PresentationServiceImpl queues the received callback and calls it > when there is a next available change. When the client of the > PresentationService has that callback invoked, in addition to processing the > result it also calls the PresentationService again asking it for the next > available change. The benefit of this model (as Darin explains in more detail) > is that the PresentationServiceImpl only generates messages at the rate that the > client is able to process them. not lgtm just pending resolution of this issue, since we've recently come to the conclusion that the model used here, while natural, is actually an anti-pattern in the Mojo world.
On 2015/01/15 at 17:02:39, blundell wrote: > Is there a reason why the model used by Geolocation doesn't work here? > > https://codereview.chromium.org/841133002/ > > Basically, the client of the PresentationService asks it for the next available change. The PresentationServiceImpl queues the received callback and calls it when there is a next available change. When the client of the PresentationService has that callback invoked, in addition to processing the result it also calls the PresentationService again asking it for the next available change. The benefit of this model (as Darin explains in more detail) is that the PresentationServiceImpl only generates messages at the rate that the client is able to process them. As far as I understand it, and it seems to be the conclusion of the mojo-dev thread is that with this solution, you might miss information. If a device becomes available then unavailable very quickly, the first message (available) might be received by the observer/client. Before it replies, the device will be unavailable with no observer to notify. Then, the observer/client will consider the device available while it is not. Am I understanding that correctly?
On 2015/01/15 17:10:41, Mounir Lamouri wrote: > On 2015/01/15 at 17:02:39, blundell wrote: > > Is there a reason why the model used by Geolocation doesn't work here? > > > > https://codereview.chromium.org/841133002/ > > > > Basically, the client of the PresentationService asks it for the next > available change. The PresentationServiceImpl queues the received callback and > calls it when there is a next available change. When the client of the > PresentationService has that callback invoked, in addition to processing the > result it also calls the PresentationService again asking it for the next > available change. The benefit of this model (as Darin explains in more detail) > is that the PresentationServiceImpl only generates messages at the rate that the > client is able to process them. > > As far as I understand it, and it seems to be the conclusion of the mojo-dev > thread is that with this solution, you might miss information. If a device > becomes available then unavailable very quickly, the first message (available) > might be received by the observer/client. Before it replies, the device will be > unavailable with no observer to notify. Then, the observer/client will consider > the device available while it is not. Am I understanding that correctly? You can cache the list of event, and send all of them when you are asked for the next events. Something like this: interface MyService { StartObserving() => MyServiceObserver } interface MyServiceObserver { GetNextStates() => array[StateChanges] } The array of state will return the list of StateChanges since the last time it has been called.
On 2015/01/15 17:10:41, Mounir Lamouri wrote: > On 2015/01/15 at 17:02:39, blundell wrote: > > Is there a reason why the model used by Geolocation doesn't work here? > > > > https://codereview.chromium.org/841133002/ > > > > Basically, the client of the PresentationService asks it for the next > available change. The PresentationServiceImpl queues the received callback and > calls it when there is a next available change. When the client of the > PresentationService has that callback invoked, in addition to processing the > result it also calls the PresentationService again asking it for the next > available change. The benefit of this model (as Darin explains in more detail) > is that the PresentationServiceImpl only generates messages at the rate that the > client is able to process them. > > As far as I understand it, and it seems to be the conclusion of the mojo-dev > thread is that with this solution, you might miss information. If a device > becomes available then unavailable very quickly, the first message (available) > might be received by the observer/client. Before it replies, the device will be > unavailable with no observer to notify. Then, the observer/client will consider > the device available while it is not. Am I understanding that correctly? It gets a little trickier if it's important that the client receive every single event, yes. Not impossible though. In that case PresentationServiceImpl can keep a queue of events. If there's no callback when an event occurs, it pushes it onto the queue. If a request comes in when the queue is non-empty, it pops from the front of the queue and sends the popped event back. In this case there's still a risk of backlog; that risk has now been shifted entirely into PresentationServiceImpl, which can decide what to do when it detects overflow of its queue of events; e.g., drop the oldest events or enter some kind of failure mode or do something more specific to the semantics of the events such as drop a chain of events if the net state after the entire chain is the same as from before the chain.
On 2015/01/15 17:16:41, blundell wrote: > On 2015/01/15 17:10:41, Mounir Lamouri wrote: > > On 2015/01/15 at 17:02:39, blundell wrote: > > > Is there a reason why the model used by Geolocation doesn't work here? > > > > > > https://codereview.chromium.org/841133002/ > > > > > > Basically, the client of the PresentationService asks it for the next > > available change. The PresentationServiceImpl queues the received callback and > > calls it when there is a next available change. When the client of the > > PresentationService has that callback invoked, in addition to processing the > > result it also calls the PresentationService again asking it for the next > > available change. The benefit of this model (as Darin explains in more detail) > > is that the PresentationServiceImpl only generates messages at the rate that > the > > client is able to process them. > > > > As far as I understand it, and it seems to be the conclusion of the mojo-dev > > thread is that with this solution, you might miss information. If a device > > becomes available then unavailable very quickly, the first message (available) > > might be received by the observer/client. Before it replies, the device will > be > > unavailable with no observer to notify. Then, the observer/client will > consider > > the device available while it is not. Am I understanding that correctly? > > It gets a little trickier if it's important that the client receive every single > event, yes. Not impossible though. In that case PresentationServiceImpl can keep > a queue of events. If there's no callback when an event occurs, it pushes it > onto the queue. If a request comes in when the queue is non-empty, it pops from > the front of the queue and sends the popped event back. In this case there's > still a risk of backlog; that risk has now been shifted entirely into > PresentationServiceImpl, which can decide what to do when it detects overflow of > its queue of events; e.g., drop the oldest events or enter some kind of failure > mode or do something more specific to the semantics of the events such as drop a > chain of events if the net state after the entire chain is the same as from > before the chain. Emails crossed :). qsr's suggestion of sending the entire queue of events when a request comes in and the queue is nonempty is better.
On 2015/01/15 17:19:51, blundell wrote: > On 2015/01/15 17:16:41, blundell wrote: > > On 2015/01/15 17:10:41, Mounir Lamouri wrote: > > > On 2015/01/15 at 17:02:39, blundell wrote: > > > > Is there a reason why the model used by Geolocation doesn't work here? > > > > > > > > https://codereview.chromium.org/841133002/ > > > > > > > > Basically, the client of the PresentationService asks it for the next > > > available change. The PresentationServiceImpl queues the received callback > and > > > calls it when there is a next available change. When the client of the > > > PresentationService has that callback invoked, in addition to processing the > > > result it also calls the PresentationService again asking it for the next > > > available change. The benefit of this model (as Darin explains in more > detail) > > > is that the PresentationServiceImpl only generates messages at the rate that > > the > > > client is able to process them. > > > > > > As far as I understand it, and it seems to be the conclusion of the mojo-dev > > > thread is that with this solution, you might miss information. If a device > > > becomes available then unavailable very quickly, the first message > (available) > > > might be received by the observer/client. Before it replies, the device will > > be > > > unavailable with no observer to notify. Then, the observer/client will > > consider > > > the device available while it is not. Am I understanding that correctly? > > > > It gets a little trickier if it's important that the client receive every > single > > event, yes. Not impossible though. In that case PresentationServiceImpl can > keep > > a queue of events. If there's no callback when an event occurs, it pushes it > > onto the queue. If a request comes in when the queue is non-empty, it pops > from > > the front of the queue and sends the popped event back. In this case there's > > still a risk of backlog; that risk has now been shifted entirely into > > PresentationServiceImpl, which can decide what to do when it detects overflow > of > > its queue of events; e.g., drop the oldest events or enter some kind of > failure > > mode or do something more specific to the semantics of the events such as drop > a > > chain of events if the net state after the entire chain is the same as from > > before the chain. > > Emails crossed :). qsr's suggestion of sending the entire queue of events when a > request comes in and the queue is nonempty is better. I think in case of the Presentation API we only care about the latest state of screen availability, so the service implementation can simply hold the last known state and return it to the frame on demand. I changed the code to pass the callback to the service. PTAL.
https://codereview.chromium.org/839773002/diff/60001/content/browser/presenta... File content/browser/presentation/presentation_service_impl.cc (right): https://codereview.chromium.org/839773002/diff/60001/content/browser/presenta... content/browser/presentation/presentation_service_impl.cc:24: presentation::PresentationServiceClientPtr) { On 2015/01/15 11:40:13, Mounir Lamouri wrote: > nit: add a name Done. https://codereview.chromium.org/839773002/diff/60001/content/browser/presenta... content/browser/presentation/presentation_service_impl.cc:29: void PresentationServiceImpl::UpdateAvailableChangeWatched(bool) { On 2015/01/15 11:40:13, Mounir Lamouri wrote: > nit: add a name Done. https://codereview.chromium.org/839773002/diff/60001/content/renderer/present... File content/renderer/presentation/presentation_dispatcher.h (right): https://codereview.chromium.org/839773002/diff/60001/content/renderer/present... content/renderer/presentation/presentation_dispatcher.h:19: public blink::WebPresentationClient, On 2015/01/15 11:40:13, Mounir Lamouri wrote: > You might need NON_EXPORTED_BASE() around blink::WebPresentationClient. I think > it will not compile on Windows otherwise, IIRC. From the description, I need it if the base class is not exported while this one is. Well, this one is not exported either AFAIK. I'll add it if actually needed.
Whose review are you looking for at this point?
On 2015/01/26 08:11:35, blundell wrote: > Whose review are you looking for at this point? Hey Colin, since one of your replies is highlighted red, I'd like to get your review (and green light hopefully) first :) Then I'll chase the relevant owner(s) as needed. Thanks, Anton.
https://codereview.chromium.org/839773002/diff/80001/content/common/presentat... File content/common/presentation/presentation_service.mojom (right): https://codereview.chromium.org/839773002/diff/80001/content/common/presentat... content/common/presentation/presentation_service.mojom:14: GetScreenAvailability() => (bool available); If my understanding is correct, you might miss results with this design. Is that not an issue?
this flow is fine to me from the mojo pov, but i leave it up to you and Mounir to confirm that it's ok wrt the design requirements in this specific case. https://codereview.chromium.org/839773002/diff/80001/content/renderer/present... File content/renderer/presentation/presentation_dispatcher.cc (right): https://codereview.chromium.org/839773002/diff/80001/content/renderer/present... content/renderer/presentation/presentation_dispatcher.cc:44: updateAvailableChangeWatched(controller_->isAvailableChangeWatched()); You should call this before line 41 as you don't know what side-effects line 41 will have (will it e.g. possibly delete |this| in some future world?).
https://codereview.chromium.org/839773002/diff/80001/content/common/presentat... File content/common/presentation/presentation_service.mojom (right): https://codereview.chromium.org/839773002/diff/80001/content/common/presentat... content/common/presentation/presentation_service.mojom:14: GetScreenAvailability() => (bool available); On 2015/01/26 10:26:39, Mounir Lamouri wrote: > If my understanding is correct, you might miss results with this design. Is that > not an issue? See my reply earlier: I don't think we care about the website getting all the states in order, just the final one. As I see it, the Mojo service doesn't have to queue the events but simply store the last known value and pass it to the callback if there was a change since the last callback was called. https://codereview.chromium.org/839773002/diff/80001/content/renderer/present... File content/renderer/presentation/presentation_dispatcher.cc (right): https://codereview.chromium.org/839773002/diff/80001/content/renderer/present... content/renderer/presentation/presentation_dispatcher.cc:44: updateAvailableChangeWatched(controller_->isAvailableChangeWatched()); On 2015/01/26 10:31:08, blundell wrote: > You should call this before line 41 as you don't know what side-effects line 41 > will have (will it e.g. possibly delete |this| in some future world?). Done.
avayvod@chromium.org changed reviewers: + mfoltz@chromium.org
Mark/Derek, could you take a look, esp. from the dropping the availability events on the service side point of view.
imcheng@chromium.org changed reviewers: + imcheng@chromium.org
lgtm
A few more comments around PresentationDispatcher https://codereview.chromium.org/839773002/diff/100001/content/browser/present... File content/browser/presentation/presentation_service_impl.h (right): https://codereview.chromium.org/839773002/diff/100001/content/browser/present... content/browser/presentation/presentation_service_impl.h:31: void GetScreenAvailability(const AvailabilityCallback& callback) override; Should this implementation method be private? Where is it going to be called? https://codereview.chromium.org/839773002/diff/100001/content/common/presenta... File content/common/presentation/presentation_service.mojom (right): https://codereview.chromium.org/839773002/diff/100001/content/common/presenta... content/common/presentation/presentation_service.mojom:9: // and the client is ready to process the result. To get the subsequent event I didn't quite follow the second sentence. Should the client always call this method twice and ignore the first result? Or should it just call the method repeatedly to get the latest availability status? https://codereview.chromium.org/839773002/diff/100001/content/common/presenta... content/common/presentation/presentation_service.mojom:11: // Might start the presentation screens discovery. The discovery might be May start discovery of presentation screens. The implementation may stop discovery once there are no active calls to GetScreenAvailability. https://codereview.chromium.org/839773002/diff/100001/content/renderer/presen... File content/renderer/presentation/presentation_dispatcher.cc (right): https://codereview.chromium.org/839773002/diff/100001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:17: PresentationDispatcher::~PresentationDispatcher() {} Is it okay to drop the ptr to presentation_service_ on the floor? If the service isn't counting how many clients are holding ptrs, how do we know when to destroy it safely? https://codereview.chromium.org/839773002/diff/100001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:21: // Provide the service with the callback to get the next |availablechange| Should we CHECK that controller_ == nullptr - assuming each time this is called is a frame attach/detach/re-attach? Or can controller_ be swapped directly? https://codereview.chromium.org/839773002/diff/100001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:31: presentation_service_->GetScreenAvailability( The lifetime of the PresentationDispatcher is tied to the lifetime of the frame, right. What if the frame is destroyed before the callback fires? Should this class have a WeakPtrFactory? https://codereview.chromium.org/839773002/diff/100001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:52: &presentation_service_); Can this ever fail? Do we need to CHECK the result or will that happen upstream? https://codereview.chromium.org/839773002/diff/100001/content/renderer/presen... File content/renderer/presentation/presentation_dispatcher.h (right): https://codereview.chromium.org/839773002/diff/100001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.h:26: blink::WebPresentationController* controller); override? https://codereview.chromium.org/839773002/diff/100001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.h:27: virtual void updateAvailableChangeWatched(bool watched); override? https://codereview.chromium.org/839773002/diff/100001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.h:33: // Used as a weak reference. Can be null since lifetime is bound to the frame. Should this be refcounted? Or is it simply set or |null| and the lifetime is controlled entirely by one other object? https://codereview.chromium.org/839773002/diff/100001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.h:34: blink::WebPresentationController* controller_; Does this need fwd declaration? https://codereview.chromium.org/839773002/diff/100001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.h:35: presentation::PresentationServicePtr presentation_service_; Ditto
Haven't uploaded the patch yet. https://codereview.chromium.org/839773002/diff/100001/content/browser/present... File content/browser/presentation/presentation_service_impl.h (right): https://codereview.chromium.org/839773002/diff/100001/content/browser/present... content/browser/presentation/presentation_service_impl.h:31: void GetScreenAvailability(const AvailabilityCallback& callback) override; On 2015/01/27 21:56:09, mark a. foltz wrote: > Should this implementation method be private? Where is it going to be called? It's an override of the base class method which is public and will be called by Mojo framework when the client calls the corresponding method through PresentationServicePtr https://codereview.chromium.org/839773002/diff/100001/content/common/presenta... File content/common/presentation/presentation_service.mojom (right): https://codereview.chromium.org/839773002/diff/100001/content/common/presenta... content/common/presentation/presentation_service.mojom:9: // and the client is ready to process the result. To get the subsequent event On 2015/01/27 21:56:09, mark a. foltz wrote: > I didn't quite follow the second sentence. Should the client always call this > method twice and ignore the first result? Or should it just call the method > repeatedly to get the latest availability status? Call the method repeatedly. Updated the wording. https://codereview.chromium.org/839773002/diff/100001/content/common/presenta... content/common/presentation/presentation_service.mojom:11: // Might start the presentation screens discovery. The discovery might be On 2015/01/27 21:56:09, mark a. foltz wrote: > May start discovery of presentation screens. The implementation may stop > discovery once there are no active calls to GetScreenAvailability. I actually introduced an explicit OnScreenAvailabilityListenerRemoved() method to help stop discovery faster. See our Mojo service document. https://codereview.chromium.org/839773002/diff/100001/content/renderer/presen... File content/renderer/presentation/presentation_dispatcher.cc (right): https://codereview.chromium.org/839773002/diff/100001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:17: PresentationDispatcher::~PresentationDispatcher() {} On 2015/01/27 21:56:09, mark a. foltz wrote: > Is it okay to drop the ptr to presentation_service_ on the floor? If the > service isn't counting how many clients are holding ptrs, how do we know when to > destroy it safely? presentation_service_ is a smart pointer (mojo::InterfacePtr). I assume it takes care of these things. I thought about updating the WebPresentationController that the client became null. https://codereview.chromium.org/839773002/diff/100001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:21: // Provide the service with the callback to get the next |availablechange| On 2015/01/27 21:56:09, mark a. foltz wrote: > Should we CHECK that controller_ == nullptr - assuming each time this is called > is a frame attach/detach/re-attach? Or can controller_ be swapped directly? You mean that either controller_ or controller is null? We probably should. https://codereview.chromium.org/839773002/diff/100001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:31: presentation_service_->GetScreenAvailability( On 2015/01/27 21:56:09, mark a. foltz wrote: > The lifetime of the PresentationDispatcher is tied to the lifetime of the frame, > right. What if the frame is destroyed before the callback fires? Should this > class have a WeakPtrFactory? I don't know for sure. But my guess is that the callback is stored on the renderer side of Mojo, so if the frame dies, Mojo part of it knows not to run the callback. GeolocationDispatcher passes Unretained(this) the same way. +blundell, what do you think? https://codereview.chromium.org/839773002/diff/100001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:52: &presentation_service_); On 2015/01/27 21:56:09, mark a. foltz wrote: > Can this ever fail? Do we need to CHECK the result or will that happen > upstream? This either calls the service's CreateService immediately or later as a pending connection request when the ServiceRegistry is bound in render_host_impl. Is it possible for the dispatcher to call this before the registry is bound and what happens in that case (does Mojo suspend all calls until the service is created?), I don't know. Colin? https://codereview.chromium.org/839773002/diff/100001/content/renderer/presen... File content/renderer/presentation/presentation_dispatcher.h (right): https://codereview.chromium.org/839773002/diff/100001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.h:26: blink::WebPresentationController* controller); On 2015/01/27 21:56:09, mark a. foltz wrote: > override? As suggested by another reviewer, implementations of Blink interfaces don't use override in case the interface has to be changed (so that the change to the interface doesn't prevent Blink from rolling into Chromium). https://codereview.chromium.org/839773002/diff/100001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.h:27: virtual void updateAvailableChangeWatched(bool watched); On 2015/01/27 21:56:09, mark a. foltz wrote: > override? Ditto. https://codereview.chromium.org/839773002/diff/100001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.h:33: // Used as a weak reference. Can be null since lifetime is bound to the frame. On 2015/01/27 21:56:09, mark a. foltz wrote: > Should this be refcounted? Or is it simply set or |null| and the lifetime is > controlled entirely by one other object? Yes, controller lives on the Blink heap (Oilpan) so can't be ref counted from here AFAIK. The dispatcher is updated with the correct pointer when needed. https://codereview.chromium.org/839773002/diff/100001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.h:34: blink::WebPresentationController* controller_; On 2015/01/27 21:56:09, mark a. foltz wrote: > Does this need fwd declaration? Not really since WebPresentationClient already forward declares it to be able to define its setController() method. https://codereview.chromium.org/839773002/diff/100001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.h:35: presentation::PresentationServicePtr presentation_service_; On 2015/01/27 21:56:09, mark a. foltz wrote: > Ditto This is defined in the presentation_service.mojom.h included in this file.
qsr@chromium.org changed reviewers: + qsr@chromium.org
https://codereview.chromium.org/839773002/diff/100001/content/renderer/presen... File content/renderer/presentation/presentation_dispatcher.cc (right): https://codereview.chromium.org/839773002/diff/100001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:31: presentation_service_->GetScreenAvailability( On 2015/01/28 01:52:38, whywhat wrote: > On 2015/01/27 21:56:09, mark a. foltz wrote: > > The lifetime of the PresentationDispatcher is tied to the lifetime of the > frame, > > right. What if the frame is destroyed before the callback fires? Should this > > class have a WeakPtrFactory? > > I don't know for sure. But my guess is that the callback is stored on the > renderer side of Mojo, so if the frame dies, Mojo part of it knows not to run > the callback. GeolocationDispatcher passes Unretained(this) the same way. > > +blundell, what do you think? The callback is stored on the persentation_service_ proxy, so as it will be deleted with this class, this is perfectly fine. https://codereview.chromium.org/839773002/diff/100001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:52: &presentation_service_); It depends a lot what you mean by fail. After this call, you are guaranteed that presentation_service_ is usable, and you can start calling method immediately. But if the other side doesn't bind it, your calls will just be lost. The way it works is that at this point, you create a pair of pipe, you bind one to your proxy, and you send the other away to be bound to an implementation. You have no guarantee that this will happen, but you can start writing message to your end of the pipe immediately.
avayvod@chromium.org changed reviewers: + avi@chromium.org
+Avi for ownership reviewal You're reviewing a relevant CL for imcheng@ already (crrev.com/883953002)
Just nits. https://codereview.chromium.org/839773002/diff/140001/content/renderer/presen... File content/renderer/presentation/presentation_dispatcher.cc (right): https://codereview.chromium.org/839773002/diff/140001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:29: CHECK(controller != controller_ && (!controller || !controller_)); Why CHECK? We usually do DCHECK for programming errors.
lgtm Copy editing suggestions on the mojom documentation. https://codereview.chromium.org/839773002/diff/140001/content/common/presenta... File content/common/presentation/presentation_service.mojom (right): https://codereview.chromium.org/839773002/diff/140001/content/common/presenta... content/common/presentation/presentation_service.mojom:8: // Returns the last session state if it’s changed since the last time the Returns the last screen availability state https://codereview.chromium.org/839773002/diff/140001/content/common/presenta... content/common/presentation/presentation_service.mojom:10: // the callback to get the next updates about the last availability status. Which callback? https://codereview.chromium.org/839773002/diff/140001/content/common/presenta... content/common/presentation/presentation_service.mojom:10: // the callback to get the next updates about the last availability status. s/updates/update/ s/last// https://codereview.chromium.org/839773002/diff/140001/content/common/presenta... content/common/presentation/presentation_service.mojom:13: // |presentation_url| might be specified to help the s/might/can/ https://codereview.chromium.org/839773002/diff/140001/content/common/presenta... content/common/presentation/presentation_service.mojom:14: // implementation to filter the incompatible screens. s/the/out/
https://codereview.chromium.org/839773002/diff/140001/content/common/presenta... File content/common/presentation/presentation_service.mojom (right): https://codereview.chromium.org/839773002/diff/140001/content/common/presenta... content/common/presentation/presentation_service.mojom:8: // Returns the last session state if it’s changed since the last time the On 2015/01/28 19:37:02, mark a. foltz wrote: > Returns the last screen availability state Done. https://codereview.chromium.org/839773002/diff/140001/content/common/presenta... content/common/presentation/presentation_service.mojom:10: // the callback to get the next updates about the last availability status. On 2015/01/28 19:37:02, mark a. foltz wrote: > Which callback? Done. https://codereview.chromium.org/839773002/diff/140001/content/common/presenta... content/common/presentation/presentation_service.mojom:10: // the callback to get the next updates about the last availability status. On 2015/01/28 19:37:02, mark a. foltz wrote: > s/updates/update/ > > s/last// Done. https://codereview.chromium.org/839773002/diff/140001/content/common/presenta... content/common/presentation/presentation_service.mojom:13: // |presentation_url| might be specified to help the On 2015/01/28 19:37:02, mark a. foltz wrote: > s/might/can/ Done. https://codereview.chromium.org/839773002/diff/140001/content/common/presenta... content/common/presentation/presentation_service.mojom:14: // implementation to filter the incompatible screens. On 2015/01/28 19:37:02, mark a. foltz wrote: > s/the/out/ Done. https://codereview.chromium.org/839773002/diff/140001/content/renderer/presen... File content/renderer/presentation/presentation_dispatcher.cc (right): https://codereview.chromium.org/839773002/diff/140001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:29: CHECK(controller != controller_ && (!controller || !controller_)); On 2015/01/28 19:04:05, Avi wrote: > Why CHECK? We usually do DCHECK for programming errors. Done.
PTAL I'd like to commit this once the Blink part rolls down to Chromium (tomorrow LON time, probably). Thanks!
On 2015/01/28 21:33:53, whywhat wrote: > PTAL > > I'd like to commit this once the Blink part rolls down to Chromium (tomorrow LON > time, probably). > > Thanks! LGTM
The CQ bit was checked by avayvod@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/839773002/220001
The CQ bit was unchecked by commit-bot@chromium.org
A disapproval has been posted by following reviewers: blundell@chromium.org. Please make sure to get positive review before another CQ attempt.
lgtm
The CQ bit was checked by mlamouri@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/839773002/220001
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/bca35fadafda9c1d6494fb8f33cbd06500d063e5 Cr-Commit-Position: refs/heads/master@{#313772} |
