Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1131053005:
[Presentation API] Convert screen availability API into Client style. (Closed)
|
Created:
5 years, 7 months ago by imcheng (use chromium acct) Modified:
5 years, 7 months ago CC:
Aaron Boodman, abarth-chromium, ben+mojo_chromium.org, chromium-reviews, darin (slow to review), darin-cc_chromium.org, jam, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-content_chromium.org, qsr+mojo_chromium.org, viettrungluu+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] Convert screen availability API into Client style.
Introduced a PresentationServiceClient mojo interface implemented by
PresentationDispatcher. On connection to PresentationService,
PDispatcher will pass a proxy of itself to PSImpl.
Please note that screen availability updates are already throttled
from the browser's perspective.
When a screen availability update is available, it will be transmitted
back to PSDispatcher via the PresentationServiceClient interface.
In the future, I plan to use the same interface for other types of
observer-style APIs (e.g. default session start, session state change,
on message).
Also, simplified the ScreenAvailability API so it only operates on
the DPU (or 1-UA mode if absent). The current semantics to support
listening on arbitrarily presentation URLs + handling behavior for
DPU is very confusing. We should revisit the implementation when we
decide to support it.
This will also hopefully fix the bug where ScreenAvailability mojo
callbacks accumulate over time.
Also fix mojom indent.
BUG=485337
Committed: https://crrev.com/9ce5394bf20f18b1413a214bef4548055c3bcb83
Cr-Commit-Position: refs/heads/master@{#329457}
Patch Set 1 : #
Total comments: 30
Patch Set 2 : Addressed 1st set comments #
Total comments: 2
Patch Set 3 : Addressed 2nd comments; change null checks #Patch Set 4 : Rebase #Patch Set 5 : Fix tests #Patch Set 6 : Fix dcheck #
Messages
Total messages: 18 (4 generated)
Patchset #1 (id:1) has been deleted
imcheng@google.com changed reviewers: + avayvod@chromium.org, mfoltz@chromium.org
PTAL
Overall looks good, just a few questions about the organization of various interfaces. https://codereview.chromium.org/1131053005/diff/20001/content/browser/present... File content/browser/presentation/presentation_service_impl.cc (right): https://codereview.chromium.org/1131053005/diff/20001/content/browser/present... content/browser/presentation/presentation_service_impl.cc:106: client_ = client.Pass(); DCHECK(!client_) and DCHECK(client) before doing this? https://codereview.chromium.org/1131053005/diff/20001/content/browser/present... content/browser/presentation/presentation_service_impl.cc:109: void PresentationServiceImpl::ListenForScreenAvailability() { Ah, so this is a side effect of setting the DPU. https://codereview.chromium.org/1131053005/diff/20001/content/browser/present... content/browser/presentation/presentation_service_impl.cc:111: if (!delegate_) If the delegate isn't supported by the embedder maybe we should disable the PresentationService. https://codereview.chromium.org/1131053005/diff/20001/content/browser/present... File content/browser/presentation/presentation_service_impl.h (right): https://codereview.chromium.org/1131053005/diff/20001/content/browser/present... content/browser/presentation/presentation_service_impl.h:95: // This class receive screen availability results from the embedder and s/This class receive/The instance receives/ https://codereview.chromium.org/1131053005/diff/20001/content/browser/present... content/browser/presentation/presentation_service_impl.h:170: void ListenForScreenAvailability() override; Does this need to be part of the public API, or can it be decided by the presence of at least one active ScreenAvailabilityListener? https://codereview.chromium.org/1131053005/diff/20001/content/browser/present... content/browser/presentation/presentation_service_impl.h:171: void StopListeningForScreenAvailability() override; Parallel comment to the one above https://codereview.chromium.org/1131053005/diff/20001/content/browser/present... content/browser/presentation/presentation_service_impl.h:218: // registers it with |delegate_|. Replaces the existing listener if any. Since the listener is not returned to the caller does it need to be replaced? Or can the existing one just be left in place? https://codereview.chromium.org/1131053005/diff/20001/content/renderer/presen... File content/renderer/presentation/presentation_dispatcher.h (right): https://codereview.chromium.org/1131053005/diff/20001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.h:46: private: Why does the WebPresentationClient implementation now need to be public?
lgtm re the simpler approach could you expand a little more on the throttling of the availability and the rest of the messages you plan to convert? mojo-dev did confirm that unless the browser is blocking the page, it's fine to use client vs. callbacks but I think I missed the conversation where we decided to refactor presentation service. https://codereview.chromium.org/1131053005/diff/20001/content/browser/present... File content/browser/presentation/presentation_service_impl.cc (right): https://codereview.chromium.org/1131053005/diff/20001/content/browser/present... content/browser/presentation/presentation_service_impl.cc:128: if (screen_availability_listener_.get()) { Couldn't you just call StopListeningForScreenAvailability() here? https://codereview.chromium.org/1131053005/diff/20001/content/browser/present... content/browser/presentation/presentation_service_impl.cc:463: } DCHECK(service_)? https://codereview.chromium.org/1131053005/diff/20001/content/browser/present... content/browser/presentation/presentation_service_impl.cc:476: service_->client_->OnScreenAvailabilityUpdated(available); DCHECK client_? https://codereview.chromium.org/1131053005/diff/20001/content/browser/present... File content/browser/presentation/presentation_service_impl.h (right): https://codereview.chromium.org/1131053005/diff/20001/content/browser/present... content/browser/presentation/presentation_service_impl.h:62: DidNavigateNotThisFrame); s/NotThis/Other? https://codereview.chromium.org/1131053005/diff/20001/content/browser/present... content/browser/presentation/presentation_service_impl.h:219: void CreateScreenAvailabilityListener(const std::string& presentation_url); s/Create/[Re]Set? s/Create/Register? I'd expect a create function to return the created object while this one resets a member. You could even pass the new listener instead of the url to remove the create aspect of it? https://codereview.chromium.org/1131053005/diff/20001/content/renderer/presen... File content/renderer/presentation/presentation_dispatcher.cc (right): https://codereview.chromium.org/1131053005/diff/20001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:86: } else { nit: don't need the {}
On 2015/05/11 17:58:19, whywhat wrote: > lgtm re the simpler approach > > could you expand a little more on the throttling of the availability and the > rest of the messages you plan to convert? mojo-dev did confirm that unless the > browser is blocking the page, it's fine to use client vs. callbacks but I think > I missed the conversation where we decided to refactor presentation service. > The provider manager will throttle screen availability results to maximum of 2 updates per sec. We may also convert ListenForSessionStateChange() and ListenForSessionMessages() to client style, provided the updates are throttled in a similar manner. > https://codereview.chromium.org/1131053005/diff/20001/content/browser/present... > File content/browser/presentation/presentation_service_impl.cc (right): > > https://codereview.chromium.org/1131053005/diff/20001/content/browser/present... > content/browser/presentation/presentation_service_impl.cc:128: if > (screen_availability_listener_.get()) { > Couldn't you just call StopListeningForScreenAvailability() here? > > https://codereview.chromium.org/1131053005/diff/20001/content/browser/present... > content/browser/presentation/presentation_service_impl.cc:463: } > DCHECK(service_)? > > https://codereview.chromium.org/1131053005/diff/20001/content/browser/present... > content/browser/presentation/presentation_service_impl.cc:476: > service_->client_->OnScreenAvailabilityUpdated(available); > DCHECK client_? > > https://codereview.chromium.org/1131053005/diff/20001/content/browser/present... > File content/browser/presentation/presentation_service_impl.h (right): > > https://codereview.chromium.org/1131053005/diff/20001/content/browser/present... > content/browser/presentation/presentation_service_impl.h:62: > DidNavigateNotThisFrame); > s/NotThis/Other? > > https://codereview.chromium.org/1131053005/diff/20001/content/browser/present... > content/browser/presentation/presentation_service_impl.h:219: void > CreateScreenAvailabilityListener(const std::string& presentation_url); > s/Create/[Re]Set? > s/Create/Register? > > I'd expect a create function to return the created object while this one resets > a member. > You could even pass the new listener instead of the url to remove the create > aspect of it? > > https://codereview.chromium.org/1131053005/diff/20001/content/renderer/presen... > File content/renderer/presentation/presentation_dispatcher.cc (right): > > https://codereview.chromium.org/1131053005/diff/20001/content/renderer/presen... > content/renderer/presentation/presentation_dispatcher.cc:86: } else { > nit: don't need the {}
https://codereview.chromium.org/1131053005/diff/20001/content/browser/present... File content/browser/presentation/presentation_service_impl.cc (right): https://codereview.chromium.org/1131053005/diff/20001/content/browser/present... content/browser/presentation/presentation_service_impl.cc:106: client_ = client.Pass(); On 2015/05/09 07:01:32, mark a. foltz wrote: > DCHECK(!client_) and DCHECK(client) before doing this? Added DCHECK(client_.is_null());. client should be guaranteed to be non-null by mojom. https://codereview.chromium.org/1131053005/diff/20001/content/browser/present... content/browser/presentation/presentation_service_impl.cc:109: void PresentationServiceImpl::ListenForScreenAvailability() { On 2015/05/09 07:01:32, mark a. foltz wrote: > Ah, so this is a side effect of setting the DPU. Yes. If we are already listening for screen availability when DPU changes, then we will start using new DPU. https://codereview.chromium.org/1131053005/diff/20001/content/browser/present... content/browser/presentation/presentation_service_impl.cc:111: if (!delegate_) On 2015/05/09 07:01:32, mark a. foltz wrote: > If the delegate isn't supported by the embedder maybe we should disable the > PresentationService. Ideally that would be the case. We could add a flag (or reuse an existing content flag?) to tell PresentationDispatcher to not connect to PSImpl. Otherwise we could call back to the client with an error after SetClient is called. But that would be for a later patch. https://codereview.chromium.org/1131053005/diff/20001/content/browser/present... content/browser/presentation/presentation_service_impl.cc:128: if (screen_availability_listener_.get()) { On 2015/05/11 17:58:18, whywhat wrote: > Couldn't you just call StopListeningForScreenAvailability() here? Done. https://codereview.chromium.org/1131053005/diff/20001/content/browser/present... content/browser/presentation/presentation_service_impl.cc:463: } On 2015/05/11 17:58:18, whywhat wrote: > DCHECK(service_)? Done. https://codereview.chromium.org/1131053005/diff/20001/content/browser/present... content/browser/presentation/presentation_service_impl.cc:476: service_->client_->OnScreenAvailabilityUpdated(available); On 2015/05/11 17:58:18, whywhat wrote: > DCHECK client_? Added !is_null() DCHECK to ctor. https://codereview.chromium.org/1131053005/diff/20001/content/browser/present... File content/browser/presentation/presentation_service_impl.h (right): https://codereview.chromium.org/1131053005/diff/20001/content/browser/present... content/browser/presentation/presentation_service_impl.h:62: DidNavigateNotThisFrame); On 2015/05/11 17:58:19, whywhat wrote: > s/NotThis/Other? Done. https://codereview.chromium.org/1131053005/diff/20001/content/browser/present... content/browser/presentation/presentation_service_impl.h:95: // This class receive screen availability results from the embedder and On 2015/05/09 07:01:32, mark a. foltz wrote: > s/This class receive/The instance receives/ Done. https://codereview.chromium.org/1131053005/diff/20001/content/browser/present... content/browser/presentation/presentation_service_impl.h:170: void ListenForScreenAvailability() override; On 2015/05/09 07:01:32, mark a. foltz wrote: > Does this need to be part of the public API, or can it be decided by the > presence of at least one active ScreenAvailabilityListener? Not sure if I understood your comment correctly. This function is what the renderer uses to inform PSImpl that a listener is to be added. https://codereview.chromium.org/1131053005/diff/20001/content/browser/present... content/browser/presentation/presentation_service_impl.h:171: void StopListeningForScreenAvailability() override; On 2015/05/09 07:01:32, mark a. foltz wrote: > Parallel comment to the one above Ditto https://codereview.chromium.org/1131053005/diff/20001/content/browser/present... content/browser/presentation/presentation_service_impl.h:218: // registers it with |delegate_|. Replaces the existing listener if any. On 2015/05/09 07:01:32, mark a. foltz wrote: > Since the listener is not returned to the caller does it need to be replaced? > Or can the existing one just be left in place? A listener is bound to a URL, so if we were to start listening to a different URL, the old listener will be replaced. Of course, we can change the listener to that it's possible to simply swap in a different URL as this is just an internal detail, but I find the current model easier to reason with. https://codereview.chromium.org/1131053005/diff/20001/content/browser/present... content/browser/presentation/presentation_service_impl.h:219: void CreateScreenAvailabilityListener(const std::string& presentation_url); On 2015/05/11 17:58:18, whywhat wrote: > s/Create/[Re]Set? > s/Create/Register? > > I'd expect a create function to return the created object while this one resets > a member. > You could even pass the new listener instead of the url to remove the create > aspect of it? Renamed to ResetScreenAvailabilityListener. https://codereview.chromium.org/1131053005/diff/20001/content/renderer/presen... File content/renderer/presentation/presentation_dispatcher.h (right): https://codereview.chromium.org/1131053005/diff/20001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.h:46: private: On 2015/05/09 07:01:32, mark a. foltz wrote: > Why does the WebPresentationClient implementation now need to be public? Reverted placement of private: label. My bad.
https://codereview.chromium.org/1131053005/diff/20001/content/renderer/presen... File content/renderer/presentation/presentation_dispatcher.cc (right): https://codereview.chromium.org/1131053005/diff/20001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:86: } else { On 2015/05/11 17:58:19, whywhat wrote: > nit: don't need the {} Done.
lgtm https://codereview.chromium.org/1131053005/diff/20001/content/browser/present... File content/browser/presentation/presentation_service_impl.h (right): https://codereview.chromium.org/1131053005/diff/20001/content/browser/present... content/browser/presentation/presentation_service_impl.h:170: void ListenForScreenAvailability() override; On 2015/05/11 22:01:54, imcheng wrote: > On 2015/05/09 07:01:32, mark a. foltz wrote: > > Does this need to be part of the public API, or can it be decided by the > > presence of at least one active ScreenAvailabilityListener? > > Not sure if I understood your comment correctly. This function is what the > renderer uses to inform PSImpl that a listener is to be added. Okay, I understand better now. This triggers the creation of a ScreenAvailabilityListenerImpl. https://codereview.chromium.org/1131053005/diff/20001/content/browser/present... content/browser/presentation/presentation_service_impl.h:218: // registers it with |delegate_|. Replaces the existing listener if any. On 2015/05/11 22:01:54, imcheng wrote: > On 2015/05/09 07:01:32, mark a. foltz wrote: > > Since the listener is not returned to the caller does it need to be replaced? > > Or can the existing one just be left in place? > > A listener is bound to a URL, so if we were to start listening to a different > URL, the old listener will be replaced. Of course, we can change the listener to > that it's possible to simply swap in a different URL as this is just an internal > detail, but I find the current model easier to reason with. SGTM https://codereview.chromium.org/1131053005/diff/40001/content/browser/present... File content/browser/presentation/presentation_service_impl.cc (right): https://codereview.chromium.org/1131053005/diff/40001/content/browser/present... content/browser/presentation/presentation_service_impl.cc:115: if (screen_availability_listener_.get() && ISTM this check should go into ResetScreenAvailabilityListener since you don't want to have any side effects if the URL is the same.
https://codereview.chromium.org/1131053005/diff/40001/content/browser/present... File content/browser/presentation/presentation_service_impl.cc (right): https://codereview.chromium.org/1131053005/diff/40001/content/browser/present... content/browser/presentation/presentation_service_impl.cc:115: if (screen_availability_listener_.get() && On 2015/05/11 23:48:38, mark a. foltz wrote: > ISTM this check should go into ResetScreenAvailabilityListener since you don't > want to have any side effects if the URL is the same. In the two cases where ResetScreenAvailabilityListener is called, we already know the URL is different. I will add a DCHECK there to ensure that.
On 2015/05/11 at 22:01:39, imcheng wrote: > On 2015/05/11 17:58:19, whywhat wrote: > > lgtm re the simpler approach > > > > could you expand a little more on the throttling of the availability and the > > rest of the messages you plan to convert? mojo-dev did confirm that unless the > > browser is blocking the page, it's fine to use client vs. callbacks but I think > > I missed the conversation where we decided to refactor presentation service. > > > > The provider manager will throttle screen availability results to maximum of 2 updates per sec. > > We may also convert ListenForSessionStateChange() and ListenForSessionMessages() to client style, provided the updates > are throttled in a similar manner. What would be the frequency limit for the session messages? I imagine a game that runs in 1-UA mode could have high demands for getting messages often. > > > > https://codereview.chromium.org/1131053005/diff/20001/content/browser/present... > > File content/browser/presentation/presentation_service_impl.cc (right): > > > > https://codereview.chromium.org/1131053005/diff/20001/content/browser/present... > > content/browser/presentation/presentation_service_impl.cc:128: if > > (screen_availability_listener_.get()) { > > Couldn't you just call StopListeningForScreenAvailability() here? > > > > https://codereview.chromium.org/1131053005/diff/20001/content/browser/present... > > content/browser/presentation/presentation_service_impl.cc:463: } > > DCHECK(service_)? > > > > https://codereview.chromium.org/1131053005/diff/20001/content/browser/present... > > content/browser/presentation/presentation_service_impl.cc:476: > > service_->client_->OnScreenAvailabilityUpdated(available); > > DCHECK client_? > > > > https://codereview.chromium.org/1131053005/diff/20001/content/browser/present... > > File content/browser/presentation/presentation_service_impl.h (right): > > > > https://codereview.chromium.org/1131053005/diff/20001/content/browser/present... > > content/browser/presentation/presentation_service_impl.h:62: > > DidNavigateNotThisFrame); > > s/NotThis/Other? > > > > https://codereview.chromium.org/1131053005/diff/20001/content/browser/present... > > content/browser/presentation/presentation_service_impl.h:219: void > > CreateScreenAvailabilityListener(const std::string& presentation_url); > > s/Create/[Re]Set? > > s/Create/Register? > > > > I'd expect a create function to return the created object while this one resets > > a member. > > You could even pass the new listener instead of the url to remove the create > > aspect of it? > > > > https://codereview.chromium.org/1131053005/diff/20001/content/renderer/presen... > > File content/renderer/presentation/presentation_dispatcher.cc (right): > > > > https://codereview.chromium.org/1131053005/diff/20001/content/renderer/presen... > > content/renderer/presentation/presentation_dispatcher.cc:86: } else { > > nit: don't need the {}
On 2015/05/12 10:48:14, whywhat wrote: > On 2015/05/11 at 22:01:39, imcheng wrote: > > On 2015/05/11 17:58:19, whywhat wrote: > > > lgtm re the simpler approach > > > > > > could you expand a little more on the throttling of the availability and the > > > rest of the messages you plan to convert? mojo-dev did confirm that unless > the > > > browser is blocking the page, it's fine to use client vs. callbacks but I > think > > > I missed the conversation where we decided to refactor presentation service. > > > > > > > The provider manager will throttle screen availability results to maximum of 2 > updates per sec. > > > > We may also convert ListenForSessionStateChange() and > ListenForSessionMessages() to client style, provided the updates > > are throttled in a similar manner. > > What would be the frequency limit for the session messages? I imagine a game > that runs in 1-UA mode could have high demands for getting messages often. > Chatted with various folks. Messaging APIs should be kept GetNext() style in order to have proper flow control. For others such as state change, which do not occur frequently, it's fine for them to be client style as long as there is throttling on sender side. Regardless of the style, each function should have consistent style throughout the stack (Blink <-> Browser and Browser <-> Provider Manager). So there's some work to convert messaging APIs to GetNext in the Browser <-> PM layer. > > > > > > > > https://codereview.chromium.org/1131053005/diff/20001/content/browser/present... > > > File content/browser/presentation/presentation_service_impl.cc (right): > > > > > > > https://codereview.chromium.org/1131053005/diff/20001/content/browser/present... > > > content/browser/presentation/presentation_service_impl.cc:128: if > > > (screen_availability_listener_.get()) { > > > Couldn't you just call StopListeningForScreenAvailability() here? > > > > > > > https://codereview.chromium.org/1131053005/diff/20001/content/browser/present... > > > content/browser/presentation/presentation_service_impl.cc:463: } > > > DCHECK(service_)? > > > > > > > https://codereview.chromium.org/1131053005/diff/20001/content/browser/present... > > > content/browser/presentation/presentation_service_impl.cc:476: > > > service_->client_->OnScreenAvailabilityUpdated(available); > > > DCHECK client_? > > > > > > > https://codereview.chromium.org/1131053005/diff/20001/content/browser/present... > > > File content/browser/presentation/presentation_service_impl.h (right): > > > > > > > https://codereview.chromium.org/1131053005/diff/20001/content/browser/present... > > > content/browser/presentation/presentation_service_impl.h:62: > > > DidNavigateNotThisFrame); > > > s/NotThis/Other? > > > > > > > https://codereview.chromium.org/1131053005/diff/20001/content/browser/present... > > > content/browser/presentation/presentation_service_impl.h:219: void > > > CreateScreenAvailabilityListener(const std::string& presentation_url); > > > s/Create/[Re]Set? > > > s/Create/Register? > > > > > > I'd expect a create function to return the created object while this one > resets > > > a member. > > > You could even pass the new listener instead of the url to remove the create > > > aspect of it? > > > > > > > https://codereview.chromium.org/1131053005/diff/20001/content/renderer/presen... > > > File content/renderer/presentation/presentation_dispatcher.cc (right): > > > > > > > https://codereview.chromium.org/1131053005/diff/20001/content/renderer/presen... > > > content/renderer/presentation/presentation_dispatcher.cc:86: } else { > > > nit: don't need the {}
The CQ bit was checked by imcheng@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from avayvod@chromium.org, mfoltz@chromium.org Link to the patchset: https://codereview.chromium.org/1131053005/#ps120001 (title: "Fix dcheck")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1131053005/120001
On 2015/05/12 at 17:59:27, imcheng wrote: > On 2015/05/12 10:48:14, whywhat wrote: > > On 2015/05/11 at 22:01:39, imcheng wrote: > > > On 2015/05/11 17:58:19, whywhat wrote: > > > > lgtm re the simpler approach > > > > > > > > could you expand a little more on the throttling of the availability and the > > > > rest of the messages you plan to convert? mojo-dev did confirm that unless > > the > > > > browser is blocking the page, it's fine to use client vs. callbacks but I > > think > > > > I missed the conversation where we decided to refactor presentation service. > > > > > > > > > > The provider manager will throttle screen availability results to maximum of 2 > > updates per sec. > > > > > > We may also convert ListenForSessionStateChange() and > > ListenForSessionMessages() to client style, provided the updates > > > are throttled in a similar manner. > > > > What would be the frequency limit for the session messages? I imagine a game > > that runs in 1-UA mode could have high demands for getting messages often. > > > > Chatted with various folks. Messaging APIs should be kept GetNext() style > in order to have proper flow control. For others such as state change, which do > not occur frequently, it's fine for them to be client style as long as there is > throttling on sender side. Regardless of the style, each function should have > consistent style throughout the stack (Blink <-> Browser and Browser <-> Provider > Manager). So there's some work to convert messaging APIs to GetNext in the > Browser <-> PM layer. > Sounds good. I'm glad that the interface becomes much simpler for the most part! > > > > > > > > > > > > > https://codereview.chromium.org/1131053005/diff/20001/content/browser/present... > > > > File content/browser/presentation/presentation_service_impl.cc (right): > > > > > > > > > > https://codereview.chromium.org/1131053005/diff/20001/content/browser/present... > > > > content/browser/presentation/presentation_service_impl.cc:128: if > > > > (screen_availability_listener_.get()) { > > > > Couldn't you just call StopListeningForScreenAvailability() here? > > > > > > > > > > https://codereview.chromium.org/1131053005/diff/20001/content/browser/present... > > > > content/browser/presentation/presentation_service_impl.cc:463: } > > > > DCHECK(service_)? > > > > > > > > > > https://codereview.chromium.org/1131053005/diff/20001/content/browser/present... > > > > content/browser/presentation/presentation_service_impl.cc:476: > > > > service_->client_->OnScreenAvailabilityUpdated(available); > > > > DCHECK client_? > > > > > > > > > > https://codereview.chromium.org/1131053005/diff/20001/content/browser/present... > > > > File content/browser/presentation/presentation_service_impl.h (right): > > > > > > > > > > https://codereview.chromium.org/1131053005/diff/20001/content/browser/present... > > > > content/browser/presentation/presentation_service_impl.h:62: > > > > DidNavigateNotThisFrame); > > > > s/NotThis/Other? > > > > > > > > > > https://codereview.chromium.org/1131053005/diff/20001/content/browser/present... > > > > content/browser/presentation/presentation_service_impl.h:219: void > > > > CreateScreenAvailabilityListener(const std::string& presentation_url); > > > > s/Create/[Re]Set? > > > > s/Create/Register? > > > > > > > > I'd expect a create function to return the created object while this one > > resets > > > > a member. > > > > You could even pass the new listener instead of the url to remove the create > > > > aspect of it? > > > > > > > > > > https://codereview.chromium.org/1131053005/diff/20001/content/renderer/presen... > > > > File content/renderer/presentation/presentation_dispatcher.cc (right): > > > > > > > > > > https://codereview.chromium.org/1131053005/diff/20001/content/renderer/presen... > > > > content/renderer/presentation/presentation_dispatcher.cc:86: } else { > > > > nit: don't need the {}
Message was sent while issue was closed.
Committed patchset #6 (id:120001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/9ce5394bf20f18b1413a214bef4548055c3bcb83 Cr-Commit-Position: refs/heads/master@{#329457} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
