Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2598063002:
[Presentation API] Handle multiple Presentation URLs in PresentationRequest::getAvailability() (Closed)
Description[Presentation API] Handle multiple Presentation URLs in PresentationRequest::getAvailability()
- Handle muliple URLs in getAvailability(), startListening(), and stopListening()
- When screen availability for a particular url comes back, update all PresentationRequests related to this url
- Added PresentationDispatcherUnittest
BUG=627655
Review-Url: https://codereview.chromium.org/2598063002
Cr-Commit-Position: refs/heads/master@{#445278}
Committed: https://chromium.googlesource.com/chromium/src/+/9bc9d0de59b3d141441fae948fd09298fcfb5f75
Patch Set 1 #
Total comments: 14
Patch Set 2 : resolve code review comments from Mark #
Total comments: 50
Patch Set 3 : seperate AvailabilityStatus from ListeningStatus and simplify book keeping code #
Total comments: 35
Patch Set 4 : rebase (skip unittest) #Patch Set 5 : resolve code review comments from Mark #Patch Set 6 : merge presentation_dispatcher_unittest.cc #
Total comments: 34
Patch Set 7 : resolve code review comments from Mark #
Total comments: 27
Patch Set 8 : resolve code review comments from Derek #
Total comments: 6
Patch Set 9 : resolve code review comments from Derek #
Total comments: 2
Patch Set 10 : resolve code review comments from Derek #
Messages
Total messages: 42 (17 generated)
zhaobin@chromium.org changed reviewers: + imcheng@chromium.org, mfoltz@chromium.org, mlamouri@chromium.org
Overall looks good: wanted to discuss design alternatives. Thanks for adding a unit test. https://codereview.chromium.org/2598063002/diff/1/content/renderer/presentati... File content/renderer/presentation/presentation_dispatcher.cc (right): https://codereview.chromium.org/2598063002/diff/1/content/renderer/presentati... content/renderer/presentation/presentation_dispatcher.cc:80: "getAvailability() isn't supported at the moment. It can be due to " kNotSupportedError per style guide https://codereview.chromium.org/2598063002/diff/1/content/renderer/presentati... content/renderer/presentation/presentation_dispatcher.cc:82: "try to blindly start a session in that case."; I would keep this simple: Screen availability monitoring not supported https://codereview.chromium.org/2598063002/diff/1/content/renderer/presentati... content/renderer/presentation/presentation_dispatcher.cc:261: availability_status_[urls] = base::WrapUnique(status); Would it be simpler to continue tracking status for individual URLs and remember which callbacks/observers are interested in which URLs? The underlying PresentationService API to get availability information uses individual URLs, so it would make processing updates from it simpler. The downside is that you would need to map a status change to pending callbacks and observers. But I have a hunch that would be simpler overall (by having cleanly separate availability state and observer state). What do you think? https://codereview.chromium.org/2598063002/diff/1/content/renderer/presentati... File content/renderer/presentation/presentation_dispatcher.h (right): https://codereview.chromium.org/2598063002/diff/1/content/renderer/presentati... content/renderer/presentation/presentation_dispatcher.h:100: const blink::WebVector<blink::WebURL>& availabilityUrl, Nit: availabilityUrls https://codereview.chromium.org/2598063002/diff/1/content/renderer/presentati... content/renderer/presentation/presentation_dispatcher.h:170: UNSUPPORTED I don't recall exactly why we have an API in the dispatcher to report that a specific URL is not supported for screen availability monitoring. I would normally think this is a property of the entire browser (for example a setting or low battery mode) but this does give the embedder maximum flexibility. I'm not sure how we should report overall availability for a request when some of the URLs are unsupported... maybe handle this the same as UNKNOWN? https://codereview.chromium.org/2598063002/diff/1/content/renderer/presentati... content/renderer/presentation/presentation_dispatcher.h:189: std::map<std::vector<GURL>, std::unique_ptr<AvailabilityStatus>> Is it okay to use a container as a map key? I usually think of availability as applying to a specific URL. https://codereview.chromium.org/2598063002/diff/1/content/renderer/presentati... content/renderer/presentation/presentation_dispatcher.h:196: std::map<GURL, ListeningState> url_listening_state_; Can we combine these with AvailabilityStatus?
Patchset #2 (id:20001) has been deleted
Resolved code review comments from Mark and updated AvailabilityStatus to track individual url now. https://codereview.chromium.org/2598063002/diff/1/content/renderer/presentati... File content/renderer/presentation/presentation_dispatcher.cc (right): https://codereview.chromium.org/2598063002/diff/1/content/renderer/presentati... content/renderer/presentation/presentation_dispatcher.cc:80: "getAvailability() isn't supported at the moment. It can be due to " On 2016/12/23 01:04:22, mfoltz_ooo_until_jan_3 wrote: > kNotSupportedError per style guide Code removed. https://codereview.chromium.org/2598063002/diff/1/content/renderer/presentati... content/renderer/presentation/presentation_dispatcher.cc:82: "try to blindly start a session in that case."; On 2016/12/23 01:04:22, mfoltz_ooo_until_jan_3 wrote: > I would keep this simple: > > Screen availability monitoring not supported Code removed. https://codereview.chromium.org/2598063002/diff/1/content/renderer/presentati... content/renderer/presentation/presentation_dispatcher.cc:261: availability_status_[urls] = base::WrapUnique(status); On 2016/12/23 01:04:22, mfoltz_ooo_until_jan_3 wrote: > Would it be simpler to continue tracking status for individual URLs and remember > which callbacks/observers are interested in which URLs? > > The underlying PresentationService API to get availability information uses > individual URLs, so it would make processing updates from it simpler. > > The downside is that you would need to map a status change to pending callbacks > and observers. But I have a hunch that would be simpler overall (by having > cleanly separate availability state and observer state). > > What do you think? Done. https://codereview.chromium.org/2598063002/diff/1/content/renderer/presentati... File content/renderer/presentation/presentation_dispatcher.h (right): https://codereview.chromium.org/2598063002/diff/1/content/renderer/presentati... content/renderer/presentation/presentation_dispatcher.h:100: const blink::WebVector<blink::WebURL>& availabilityUrl, On 2016/12/23 01:04:22, mfoltz_ooo_until_jan_3 wrote: > Nit: availabilityUrls Done. https://codereview.chromium.org/2598063002/diff/1/content/renderer/presentati... content/renderer/presentation/presentation_dispatcher.h:170: UNSUPPORTED On 2016/12/23 01:04:22, mfoltz_ooo_until_jan_3 wrote: > I don't recall exactly why we have an API in the dispatcher to report that a > specific URL is not supported for screen availability monitoring. I would > normally think this is a property of the entire browser (for example a setting > or low battery mode) but this does give the embedder maximum flexibility. > > I'm not sure how we should report overall availability for a request when some > of the URLs are unsupported... maybe handle this the same as UNKNOWN? > Done. https://codereview.chromium.org/2598063002/diff/1/content/renderer/presentati... content/renderer/presentation/presentation_dispatcher.h:189: std::map<std::vector<GURL>, std::unique_ptr<AvailabilityStatus>> On 2016/12/23 01:04:22, mfoltz_ooo_until_jan_3 wrote: > Is it okay to use a container as a map key? > > I usually think of availability as applying to a specific URL. Code removed. https://codereview.chromium.org/2598063002/diff/1/content/renderer/presentati... content/renderer/presentation/presentation_dispatcher.h:196: std::map<GURL, ListeningState> url_listening_state_; On 2016/12/23 01:04:22, mfoltz_ooo_until_jan_3 wrote: > Can we combine these with AvailabilityStatus? Code removed.
Overall looks good. The bookkeeping to track callbacks and observers was a little more complicated than I hoped. A couple of high level comments: - We want to tell the page as soon as we have one URL availability that is not UNKNOWN. If an additional availability comes back to change status from unavailable to available, then we will fire an event to tell the page. - Each PresentationAvailability object should generate one callback (to resolve the Promise) and one observer (to drive the event handler). I wonder if refactoring the Blink side to expose a WebPresentationAvailability object we could implement in the dispatcher would simplify things? - Eventually we will migrate this logic into Blink (along with message queueing) for Onion Soup. So thinking about how to split up the dispatcher into PresentationConnection.cpp, PresentationAvailability.cpp would be good to keep in mind. https://codereview.chromium.org/2598063002/diff/40001/content/renderer/presen... File content/renderer/presentation/presentation_dispatcher.cc (right): https://codereview.chromium.org/2598063002/diff/40001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:271: UpdateAvailabilityObserversAndCallbacks(status); Can this loop be combined with the previous one? Or do you need all status objects updated first? https://codereview.chromium.org/2598063002/diff/40001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:353: UpdateAvailabilityObserversAndCallbacks(status); This only needs to be called if last_known_availability has changed, right? https://codereview.chromium.org/2598063002/diff/40001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:366: UpdateAvailabilityObserversAndCallbacks(status); This only needs to be called if last_known_availability has changed, right? https://codereview.chromium.org/2598063002/diff/40001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:370: void PresentationDispatcher::UpdateAvailabilityObserversAndCallbacks( Shorter as UpdateAvailabilityStatus ? https://codereview.chromium.org/2598063002/diff/40001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:371: AvailabilityStatus* status) { Could this be a method on AvailabilityStatus()? https://codereview.chromium.org/2598063002/diff/40001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:375: observer->availabilityChanged(availability == Where do you check that the availability has changed from the previous value that was passed to the observer? https://codereview.chromium.org/2598063002/diff/40001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:382: if (availability == ScreenAvailability::UNKNOWN) { Nit: This could be a switch statement with a NOTREACHED default case. https://codereview.chromium.org/2598063002/diff/40001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:576: return ScreenAvailability::UNKNOWN; IMO, we should only return UNKNOWN if all URLs are UNKNOWN (i.e. we have absolutely no information about the status). Otherwise we can return the best information we have now for some URLs (AVAILABLE/UNAVAILABLE/UNSUPPORTED). https://codereview.chromium.org/2598063002/diff/40001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:579: if (available_cnt > 0) This case should go first: if any URL is available, then the request is available. https://codereview.chromium.org/2598063002/diff/40001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:583: return ScreenAvailability::UNAVAILABLE; This case should go third, when some URLs are unavailable and some are unknown, then report UNAVAILABLE. https://codereview.chromium.org/2598063002/diff/40001/content/renderer/presen... File content/renderer/presentation/presentation_dispatcher.h (right): https://codereview.chromium.org/2598063002/diff/40001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.h:50: friend class PresentationDispatcherTest; Takumi is also landing a unit test, so please coordinate with him on the best way to do that; maybe land this unit test separately after rebasing on his patch? https://codereview.chromium.org/2598063002/diff/40001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.h:175: const blink::WebVector<blink::WebURL>& availability_urls, Can this use std::vector<GURL> to be consistent with AvailabilityStatus and the rest of the implementation in this file? https://codereview.chromium.org/2598063002/diff/40001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.h:192: explicit AvailabilityStatus(const GURL& availability_urls); availability_url https://codereview.chromium.org/2598063002/diff/40001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.h:198: AvailabilityCallbacksSet availability_callbacks; It's confusing in the code to have both availability_callbacks and availability_callbacks_. Can these be renamed status_callbacks and status_observers? https://codereview.chromium.org/2598063002/diff/40001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.h:207: AvailabilityCallbacksVector availability_callbacks_; Is this for ownership purposes (i.e. to make sure all callbacks are deleted)? Can you add a comment explaining why this is tracking callbacks both here and in AvailabilityStatus? https://codereview.chromium.org/2598063002/diff/40001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.h:225: const blink::WebVector<blink::WebURL>& urls); std::vector<GURL>
I left some comments on top of mfoltz@. Mostly superficial because I didn't want to comment on logic you were going to change or +1 mfoltz@ comments. Looks generally good with a few things to change! :) https://codereview.chromium.org/2598063002/diff/40001/content/renderer/presen... File content/renderer/presentation/presentation_dispatcher.cc (left): https://codereview.chromium.org/2598063002/diff/40001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:369: "try to blindly start a session in that case."); Why are you removing the entire error message? https://codereview.chromium.org/2598063002/diff/40001/content/renderer/presen... File content/renderer/presentation/presentation_dispatcher.cc (right): https://codereview.chromium.org/2598063002/diff/40001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:268: DCHECK(status_it != availability_status_.end()); I think you can use DCHECK_NE here. https://codereview.chromium.org/2598063002/diff/40001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:390: availability_callback->callback->onSuccess(is_available); nit: merge in one line https://codereview.chromium.org/2598063002/diff/40001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:402: continue; style: { } https://codereview.chromium.org/2598063002/diff/40001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:552: size_t unknown_cnt = 0; style: s/cnt/count/. the abreviation doesn't seem needed here. https://codereview.chromium.org/2598063002/diff/40001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:573: } FWIW, you can simplify this by having a map that associates the enum to an int so you don't need to write the whole switch but just `counters[url_availability_]++;` (or something similar) https://codereview.chromium.org/2598063002/diff/40001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:582: DCHECK(unavailable_cnt == urls.size()); DCHECK_EQ https://codereview.chromium.org/2598063002/diff/40001/content/renderer/presen... File content/renderer/presentation/presentation_dispatcher.h (right): https://codereview.chromium.org/2598063002/diff/40001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.h:166: enum class ScreenAvailability { It's pretty much a nit but why not using AvailabilityStatus instead of ScreenAvailability? It's a bit odd to see "Screen" mentioned as it's not really mentioned elsewhere. Feel free to ignore :) https://codereview.chromium.org/2598063002/diff/40001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.h:224: PresentationDispatcher::ScreenAvailability GetScreenAvailability( GetAvailabilityStatus()?
I looked through the code that controls availability monitoring from Blink. There are three cases that change the listening state for a PresentationAvailability object: 1. Page is suspended because a synchronous API was called (print(), alert(), etc.). This will call stopListening() when the page is suspended and startListening() when the API finishes. 2. Page visibilty changes (i.e. tab goes in the background). If the page is hidden, it calls stop and start when the page is visible again. 3. Availability object destroyed. This calls stop(). PresentationDispatcher implements this by repeatedly adding and removing the WebPresentationAvailabilityObserver. Instead we could add a state flag to the observer object (to handle cases 1&2) and a separate API to notify the dispatcher that the availability object is about to be destroyed. I'm not sure that would simplify this drastically; it would just change the logic in UpdateListeningState(), so I wouldn't pursue that change unless you see a benefit. --- The other source of complexity from my viewpoint is that each URL has a status object that can map onto multiple observers and callbacks (since a URL can be present in multiple PresentationAvailability objects). This is the design I suggested, since I felt it made tracking the status of each URL clearer, but does come at a cost to tracking all the callbacks. I don't expect there to be many live PresentationAvailability objects at once: a simpler approach would be to keep global lists of all observers and callbacks. When a status update comes in for a URL, iterate through them all and compute the new availability if the URL matches, and fire the observer/callback if necessary. It's not as efficient but means less bookkeeping. What do you think? https://codereview.chromium.org/2598063002/diff/40001/content/renderer/presen... File content/renderer/presentation/presentation_dispatcher.h (right): https://codereview.chromium.org/2598063002/diff/40001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.h:166: enum class ScreenAvailability { On 2017/01/04 at 16:15:00, mlamouri wrote: > It's pretty much a nit but why not using AvailabilityStatus instead of ScreenAvailability? It's a bit odd to see "Screen" mentioned as it's not really mentioned elsewhere. Feel free to ignore :) There's already an AvailabilityStatus struct declared in this class (@L191). I think ScreenAvailability is okay since there needs to be some thing that's available or not.
Patchset #3 (id:60001) has been deleted
Please review patch 3 against base. Implementation is different from patch 2. Seperate PresentationAvailability status (AvailabilityStatus) from URL listening status (ListeningStatus). There is one AvailabilityStatus per URL set, may corresponds to multiple PresentationRequests with the same urls; There is one ListeningStatus per URL; ListeningStatus does not take reference of AvailabilityStatus and removes bookkeeping code. When ListeningStatus for a single URL is changed (OnScreenAvailabilityUpdated), iterate through all AvailabilityStatus, find those contain URL, and update. Removed UpdateListeningState(status) from PresentationDispatcher::OnScreenAvailabilityUpdated(). We start/stop listening to URL based on PresentationAvailability object's lifecycle. We do not need to update listening status when URL reports its listening state. https://codereview.chromium.org/2598063002/diff/40001/content/renderer/presen... File content/renderer/presentation/presentation_dispatcher.cc (right): https://codereview.chromium.org/2598063002/diff/40001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:268: DCHECK(status_it != availability_status_.end()); On 2017/01/04 16:15:00, mlamouri wrote: > I think you can use DCHECK_NE here. Code removed. https://codereview.chromium.org/2598063002/diff/40001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:271: UpdateAvailabilityObserversAndCallbacks(status); On 2017/01/03 21:47:13, mfoltz_ooo_until_jan_3 wrote: > Can this loop be combined with the previous one? Or do you need all status > objects updated first? Code removed. https://codereview.chromium.org/2598063002/diff/40001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:353: UpdateAvailabilityObserversAndCallbacks(status); On 2017/01/03 21:47:13, mfoltz_ooo_until_jan_3 wrote: > This only needs to be called if last_known_availability has changed, right? Done. https://codereview.chromium.org/2598063002/diff/40001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:366: UpdateAvailabilityObserversAndCallbacks(status); On 2017/01/03 21:47:12, mfoltz_ooo_until_jan_3 wrote: > This only needs to be called if last_known_availability has changed, right? Done. https://codereview.chromium.org/2598063002/diff/40001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:370: void PresentationDispatcher::UpdateAvailabilityObserversAndCallbacks( On 2017/01/03 21:47:13, mfoltz_ooo_until_jan_3 wrote: > Shorter as UpdateAvailabilityStatus ? Done. https://codereview.chromium.org/2598063002/diff/40001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:371: AvailabilityStatus* status) { On 2017/01/03 21:47:13, mfoltz_ooo_until_jan_3 wrote: > Could this be a method on AvailabilityStatus()? Done. https://codereview.chromium.org/2598063002/diff/40001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:375: observer->availabilityChanged(availability == On 2017/01/03 21:47:13, mfoltz_ooo_until_jan_3 wrote: > Where do you check that the availability has changed from the previous value > that was passed to the observer? No check in PresentationDispatcher. PresentationRequest::availabilityChanged has the check logic, only fire event if availability value has changed. Do we need to add the check in PresentationDispatcher as well? https://codereview.chromium.org/2598063002/diff/40001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:382: if (availability == ScreenAvailability::UNKNOWN) { On 2017/01/03 21:47:13, mfoltz_ooo_until_jan_3 wrote: > Nit: This could be a switch statement with a NOTREACHED default case. Code removed. https://codereview.chromium.org/2598063002/diff/40001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:390: availability_callback->callback->onSuccess(is_available); On 2017/01/04 16:15:00, mlamouri wrote: > nit: merge in one line Done. https://codereview.chromium.org/2598063002/diff/40001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:402: continue; On 2017/01/04 16:15:00, mlamouri wrote: > style: { } Code removed. https://codereview.chromium.org/2598063002/diff/40001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:552: size_t unknown_cnt = 0; On 2017/01/04 16:15:00, mlamouri wrote: > style: s/cnt/count/. the abreviation doesn't seem needed here. Code removed. https://codereview.chromium.org/2598063002/diff/40001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:573: } On 2017/01/04 16:15:00, mlamouri wrote: > FWIW, you can simplify this by having a map that associates the enum to an int > so you don't need to write the whole switch but just > `counters[url_availability_]++;` (or something similar) Done. https://codereview.chromium.org/2598063002/diff/40001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:576: return ScreenAvailability::UNKNOWN; On 2017/01/03 21:47:13, mfoltz_ooo_until_jan_3 wrote: > IMO, we should only return UNKNOWN if all URLs are UNKNOWN (i.e. we have > absolutely no information about the status). Otherwise we can return the best > information we have now for some URLs (AVAILABLE/UNAVAILABLE/UNSUPPORTED). Done. https://codereview.chromium.org/2598063002/diff/40001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:579: if (available_cnt > 0) On 2017/01/03 21:47:13, mfoltz_ooo_until_jan_3 wrote: > This case should go first: if any URL is available, then the request is > available. Done. https://codereview.chromium.org/2598063002/diff/40001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:582: DCHECK(unavailable_cnt == urls.size()); On 2017/01/04 16:15:00, mlamouri wrote: > DCHECK_EQ Done. https://codereview.chromium.org/2598063002/diff/40001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:583: return ScreenAvailability::UNAVAILABLE; On 2017/01/03 21:47:13, mark a. foltz wrote: > This case should go third, when some URLs are unavailable and some are unknown, > then report UNAVAILABLE. > Done. https://codereview.chromium.org/2598063002/diff/40001/content/renderer/presen... File content/renderer/presentation/presentation_dispatcher.h (right): https://codereview.chromium.org/2598063002/diff/40001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.h:50: friend class PresentationDispatcherTest; On 2017/01/03 21:47:13, mfoltz_ooo_until_jan_3 wrote: > Takumi is also landing a unit test, so please coordinate with him on the best > way to do that; maybe land this unit test separately after rebasing on his > patch? Sure. https://codereview.chromium.org/2598063002/diff/40001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.h:166: enum class ScreenAvailability { On 2017/01/04 16:15:00, mlamouri wrote: > It's pretty much a nit but why not using AvailabilityStatus instead of > ScreenAvailability? It's a bit odd to see "Screen" mentioned as it's not really > mentioned elsewhere. Feel free to ignore :) ScreenAvailability represents if there is any screen (display) available for a given url. AvailabilityStatus is the status of PresentationAvailability, which has multiple urls. Naming is a little confusing, please let me know if you have better suggestions :) https://codereview.chromium.org/2598063002/diff/40001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.h:175: const blink::WebVector<blink::WebURL>& availability_urls, On 2017/01/03 21:47:13, mfoltz_ooo_until_jan_3 wrote: > Can this use std::vector<GURL> to be consistent with AvailabilityStatus and the > rest of the implementation in this file? Done. https://codereview.chromium.org/2598063002/diff/40001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.h:192: explicit AvailabilityStatus(const GURL& availability_urls); On 2017/01/03 21:47:13, mfoltz_ooo_until_jan_3 wrote: > availability_url Done. https://codereview.chromium.org/2598063002/diff/40001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.h:198: AvailabilityCallbacksSet availability_callbacks; On 2017/01/03 21:47:13, mfoltz_ooo_until_jan_3 wrote: > It's confusing in the code to have both availability_callbacks and > availability_callbacks_. > > Can these be renamed status_callbacks and status_observers? Code removed. https://codereview.chromium.org/2598063002/diff/40001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.h:207: AvailabilityCallbacksVector availability_callbacks_; On 2017/01/03 21:47:13, mfoltz_ooo_until_jan_3 wrote: > Is this for ownership purposes (i.e. to make sure all callbacks are deleted)? > > Can you add a comment explaining why this is tracking callbacks both here and in > AvailabilityStatus? Code removed. https://codereview.chromium.org/2598063002/diff/40001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.h:224: PresentationDispatcher::ScreenAvailability GetScreenAvailability( On 2017/01/04 16:15:00, mlamouri wrote: > GetAvailabilityStatus()? Cannot use GetAvailabilityStatus since AvailabilityStatus is representing sth else... https://codereview.chromium.org/2598063002/diff/40001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.h:225: const blink::WebVector<blink::WebURL>& urls); On 2017/01/03 21:47:13, mfoltz_ooo_until_jan_3 wrote: > std::vector<GURL> Done.
TBD: Review unit test Thank you! This patch is easier to understand. https://codereview.chromium.org/2598063002/diff/80001/content/renderer/presen... File content/renderer/presentation/presentation_dispatcher.cc (right): https://codereview.chromium.org/2598063002/diff/80001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:272: // Resolve promise if we know some URLs' status. // Reject Promise if screen availability is unsupported for all URLs. https://codereview.chromium.org/2598063002/diff/80001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:286: AvailabilityStatus* status = nullptr; Consider using std::find_if instead of looping. https://codereview.chromium.org/2598063002/diff/80001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:296: availability_set_.insert(base::WrapUnique(status)); When are entries removed from |availability_set_|? https://codereview.chromium.org/2598063002/diff/80001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:321: status->availability_observers.insert(observer); Do you need to break the loop after inserting the observer (there should only be one inserted)? https://codereview.chromium.org/2598063002/diff/80001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:322: } Can you add a DCHECK() that one observer was actually inserted at the end of the loop? https://codereview.chromium.org/2598063002/diff/80001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:336: status->availability_observers.erase(observer); Do you need to break after finding the matching status? https://codereview.chromium.org/2598063002/diff/80001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:337: } Can you add a DCHECK that exactly one observer was erased? https://codereview.chromium.org/2598063002/diff/80001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:397: // screen_availability cannot be UNKNOWN or UNSUPPORTED here DCHECK(availability == AVAILABLE || availability == UNAVAILABLE) https://codereview.chromium.org/2598063002/diff/80001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:436: DCHECK_EQ(screen_availability, ScreenAvailability::UNSUPPORTED); What if a different URL in |status->urls| was already AVAILABLE? https://codereview.chromium.org/2598063002/diff/80001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:566: void PresentationDispatcher::StartListening(const GURL& url) { It's potentially confusing to have startListening and StartListening methods. Since the ones above are determined by the WebPresentationClient API, can you rename these two? https://codereview.chromium.org/2598063002/diff/80001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:570: listening_status_[url] = base::WrapUnique(listening_status); Nit: Use .insert() to avoid an extra allocation. When are entries removed from listening_status_? https://codereview.chromium.org/2598063002/diff/80001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:594: DCHECK(listening_status); This means that StopListening was called on a URL that StartListening was never called on, or was called twice on the same URL, correct? https://codereview.chromium.org/2598063002/diff/80001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:615: std::map<ScreenAvailability, size_t> counters; Does this initialize the values to zero? https://codereview.chromium.org/2598063002/diff/80001/content/renderer/presen... File content/renderer/presentation/presentation_dispatcher.h (right): https://codereview.chromium.org/2598063002/diff/80001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.h:200: // Map of AvailabilityStatus for known PresentationRequest. s/Map/Set/ https://codereview.chromium.org/2598063002/diff/80001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.h:206: // Stops listening to |url| if no PresentationAvailability is observing |url|. Document that StartListening() must have been called first and StopListening must be called only once per URL. https://codereview.chromium.org/2598063002/diff/80001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.h:209: ListeningStatus* GetListeningStatus(const GURL& url); Document that this method returns nullptr if there is no status for |url|. https://codereview.chromium.org/2598063002/diff/80001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.h:218: PresentationDispatcher::ScreenAvailability GetScreenAvailability( I don't think you need PresentationDispatcher:: here.
zhaobin@, is there something specifically you want me to look at here or is mfoltz@ review enough?
On 2017/01/11 11:19:41, mlamouri wrote: > zhaobin@, is there something specifically you want me to look at here or is > mfoltz@ review enough? mfoltz@ review is enough for me. Feel free to take a look if you have time :)
Patchset #4 (id:100001) has been deleted
I will send out a new iteration to merge unit tests. https://codereview.chromium.org/2598063002/diff/80001/content/renderer/presen... File content/renderer/presentation/presentation_dispatcher.cc (right): https://codereview.chromium.org/2598063002/diff/80001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:272: // Resolve promise if we know some URLs' status. On 2017/01/11 00:06:02, mark a. foltz wrote: > // Reject Promise if screen availability is unsupported for all URLs. Done. https://codereview.chromium.org/2598063002/diff/80001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:286: AvailabilityStatus* status = nullptr; On 2017/01/11 00:06:02, mark a. foltz wrote: > Consider using std::find_if instead of looping. Done. https://codereview.chromium.org/2598063002/diff/80001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:296: availability_set_.insert(base::WrapUnique(status)); On 2017/01/11 00:06:02, mark a. foltz wrote: > When are entries removed from |availability_set_|? We do not. |availability_set_| will finally be cleared in dtor. Current single url implementation does not remove |AvailabilityStatus| from |availability_status_|. We may remove an entry when there is no observers or no callbacks for a set of urls and add it back when new observer or callback becomes available. https://codereview.chromium.org/2598063002/diff/80001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:321: status->availability_observers.insert(observer); On 2017/01/11 00:06:02, mark a. foltz wrote: > Do you need to break the loop after inserting the observer (there should only be > one inserted)? Done. https://codereview.chromium.org/2598063002/diff/80001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:322: } On 2017/01/11 00:06:02, mark a. foltz wrote: > Can you add a DCHECK() that one observer was actually inserted at the end of the > loop? Done. https://codereview.chromium.org/2598063002/diff/80001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:336: status->availability_observers.erase(observer); On 2017/01/11 00:06:02, mark a. foltz wrote: > Do you need to break after finding the matching status? Done. https://codereview.chromium.org/2598063002/diff/80001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:337: } On 2017/01/11 00:06:02, mark a. foltz wrote: > Can you add a DCHECK that exactly one observer was erased? Done. https://codereview.chromium.org/2598063002/diff/80001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:397: // screen_availability cannot be UNKNOWN or UNSUPPORTED here On 2017/01/11 00:06:02, mark a. foltz wrote: > DCHECK(availability == AVAILABLE || availability == UNAVAILABLE) Done. https://codereview.chromium.org/2598063002/diff/80001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:436: DCHECK_EQ(screen_availability, ScreenAvailability::UNSUPPORTED); On 2017/01/11 00:06:02, mark a. foltz wrote: > What if a different URL in |status->urls| was already AVAILABLE? Is it possible to have some URLs report AVAILABLE and some report UNSUPPORTED? If so, shall we wait for all status before rejecting promise as UNSUPPORTED? https://codereview.chromium.org/2598063002/diff/80001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:566: void PresentationDispatcher::StartListening(const GURL& url) { On 2017/01/11 00:06:02, mark a. foltz wrote: > It's potentially confusing to have startListening and StartListening methods. > Since the ones above are determined by the WebPresentationClient API, can you > rename these two? Done. https://codereview.chromium.org/2598063002/diff/80001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:570: listening_status_[url] = base::WrapUnique(listening_status); On 2017/01/11 00:06:02, mark a. foltz wrote: > Nit: Use .insert() to avoid an extra allocation. > > When are entries removed from listening_status_? Done. https://codereview.chromium.org/2598063002/diff/80001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:594: DCHECK(listening_status); On 2017/01/11 00:06:02, mark a. foltz wrote: > This means that StopListening was called on a URL that StartListening was never > called on, or was called twice on the same URL, correct? It means StartListening was never called on. Calling StopListening twice is fine, since we do not remove url listening status after StopListening. https://codereview.chromium.org/2598063002/diff/80001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:615: std::map<ScreenAvailability, size_t> counters; On 2017/01/11 00:06:02, mark a. foltz wrote: > Does this initialize the values to zero? Yes. https://codereview.chromium.org/2598063002/diff/80001/content/renderer/presen... File content/renderer/presentation/presentation_dispatcher.h (right): https://codereview.chromium.org/2598063002/diff/80001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.h:200: // Map of AvailabilityStatus for known PresentationRequest. On 2017/01/11 00:06:03, mark a. foltz wrote: > s/Map/Set/ Done. https://codereview.chromium.org/2598063002/diff/80001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.h:206: // Stops listening to |url| if no PresentationAvailability is observing |url|. On 2017/01/11 00:06:02, mark a. foltz wrote: > Document that StartListening() must have been called first and StopListening > must be called only once per URL. Done. https://codereview.chromium.org/2598063002/diff/80001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.h:209: ListeningStatus* GetListeningStatus(const GURL& url); On 2017/01/11 00:06:02, mark a. foltz wrote: > Document that this method returns nullptr if there is no status for |url|. Done. https://codereview.chromium.org/2598063002/diff/80001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.h:218: PresentationDispatcher::ScreenAvailability GetScreenAvailability( On 2017/01/11 00:06:03, mark a. foltz wrote: > I don't think you need PresentationDispatcher:: here. Done.
Went ahead and added comments on unit test (hopefully still apply after merge). https://codereview.chromium.org/2598063002/diff/80001/content/renderer/presen... File content/renderer/presentation/presentation_dispatcher_unittest.cc (right): https://codereview.chromium.org/2598063002/diff/80001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher_unittest.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. s/2014/2017/ https://codereview.chromium.org/2598063002/diff/160001/content/renderer/prese... File content/renderer/presentation/presentation_dispatcher_unittest.cc (right): https://codereview.chromium.org/2598063002/diff/160001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher_unittest.cc:52: explicit MockPresentationAvailabilityObserver(const std::vector<GURL>& gurls) Do we need both mock constructors? https://codereview.chromium.org/2598063002/diff/160001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher_unittest.cc:217: void ChangeURLState(const GURL& gurl, URLState state) { s|gurl|url| https://codereview.chromium.org/2598063002/diff/160001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher_unittest.cc:232: dispatcher_.startListening(observer); Since |dispatcher_| is protected, it should be possible to call this directly in the test case. https://codereview.chromium.org/2598063002/diff/160001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher_unittest.cc:236: dispatcher_.stopListening(observer); Similar comment as StartListening(). https://codereview.chromium.org/2598063002/diff/160001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher_unittest.cc:239: void TestGetAvailability( Can you document what this is testing? E.g., Tests that PresenationService is called for getAvailability(urls), after |urls| change state to |states|. https://codereview.chromium.org/2598063002/diff/160001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher_unittest.cc:249: client()->getAvailability(urls, base::WrapUnique(mock_callback)); Document that this function takes ownership of |mock_callback|. https://codereview.chromium.org/2598063002/diff/160001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher_unittest.cc:250: for (size_t i = 0; i < std::min(urls.size(), states.size()); i++) Should you require urls.size() == states.size()? https://codereview.chromium.org/2598063002/diff/160001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher_unittest.cc:251: ChangeURLState(urls[i], states[i]); Nit: blank line after loop statement https://codereview.chromium.org/2598063002/diff/160001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher_unittest.cc:252: run_loop.RunUntilIdle(); It seems like in most cases |mock_callback| will be invoked; do you need to set an expectation on it? Or is that done in the test cases below? https://codereview.chromium.org/2598063002/diff/160001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher_unittest.cc:268: const std::vector<blink::WebVector<blink::WebURL>> urls_set_; |urls_| ? https://codereview.chromium.org/2598063002/diff/160001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher_unittest.cc:483: auto* mock_callback = new MockPresentationAvailabilityCallbacks(); You can use a StrictMock<> to verify that mock_callback is not invoked in this test. https://github.com/google/googletest/blob/master/googlemock/docs/CookBook.md https://codereview.chromium.org/2598063002/diff/160001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher_unittest.cc:486: TestGetAvailability(urls, states, mock_callback); Could this just take a vector<GURL> and convert to WebURL internally, so you could write: TestGetAvailability({url1_}, {}, mock_callback) https://codereview.chromium.org/2598063002/diff/160001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher_unittest.cc:491: EXPECT_CALL(*mock_callback, onSuccess(true)); Nit: consistently add a blank line after setting expectations https://codereview.chromium.org/2598063002/diff/160001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher_unittest.cc:635: ChangeURLState(gurl2_, URLState::Unavailable); Set expectation that mock_observer{1,2}_ will have availabilityChanged(false) invoked https://codereview.chromium.org/2598063002/diff/160001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher_unittest.cc:661: ChangeURLState(gurl1_, URLState::Available); Can you change the state to Unavailable and test that availabiltyChanged(false) is invoked? https://codereview.chromium.org/2598063002/diff/160001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher_unittest.cc:666: OnScreenAvailabilityUpdatedInvokesMultipleAvailabilityChanged) { I didn't see any test of availabilityChanged() in this test case.
One minor comment on the .cc. Otherwise looks good. https://codereview.chromium.org/2598063002/diff/160001/content/renderer/prese... File content/renderer/presentation/presentation_dispatcher.cc (right): https://codereview.chromium.org/2598063002/diff/160001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher.cc:320: auto status_it = This is repeated a few times; add a private method to search for AvailabilityStatus to avoid duplication.
https://codereview.chromium.org/2598063002/diff/160001/content/renderer/prese... File content/renderer/presentation/presentation_dispatcher.cc (right): https://codereview.chromium.org/2598063002/diff/160001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher.cc:320: auto status_it = On 2017/01/13 19:06:32, mark a. foltz wrote: > This is repeated a few times; add a private method to search for > AvailabilityStatus to avoid duplication. Done. https://codereview.chromium.org/2598063002/diff/160001/content/renderer/prese... File content/renderer/presentation/presentation_dispatcher_unittest.cc (right): https://codereview.chromium.org/2598063002/diff/160001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher_unittest.cc:52: explicit MockPresentationAvailabilityObserver(const std::vector<GURL>& gurls) On 2017/01/13 19:01:16, mark a. foltz wrote: > Do we need both mock constructors? Done. https://codereview.chromium.org/2598063002/diff/160001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher_unittest.cc:217: void ChangeURLState(const GURL& gurl, URLState state) { On 2017/01/13 19:01:15, mark a. foltz wrote: > s|gurl|url| Done. https://codereview.chromium.org/2598063002/diff/160001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher_unittest.cc:232: dispatcher_.startListening(observer); On 2017/01/13 19:01:16, mark a. foltz wrote: > Since |dispatcher_| is protected, it should be possible to call this directly in > the test case. Done. https://codereview.chromium.org/2598063002/diff/160001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher_unittest.cc:236: dispatcher_.stopListening(observer); On 2017/01/13 19:01:16, mark a. foltz wrote: > Similar comment as StartListening(). Done. https://codereview.chromium.org/2598063002/diff/160001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher_unittest.cc:239: void TestGetAvailability( On 2017/01/13 19:01:16, mark a. foltz wrote: > Can you document what this is testing? E.g., > > Tests that PresenationService is called for getAvailability(urls), after |urls| > change state to |states|. Done. https://codereview.chromium.org/2598063002/diff/160001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher_unittest.cc:249: client()->getAvailability(urls, base::WrapUnique(mock_callback)); On 2017/01/13 19:01:17, mark a. foltz wrote: > Document that this function takes ownership of |mock_callback|. Done. https://codereview.chromium.org/2598063002/diff/160001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher_unittest.cc:250: for (size_t i = 0; i < std::min(urls.size(), states.size()); i++) On 2017/01/13 19:01:15, mark a. foltz wrote: > Should you require urls.size() == states.size()? Done. https://codereview.chromium.org/2598063002/diff/160001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher_unittest.cc:251: ChangeURLState(urls[i], states[i]); On 2017/01/13 19:01:16, mark a. foltz wrote: > Nit: blank line after loop statement Done. https://codereview.chromium.org/2598063002/diff/160001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher_unittest.cc:252: run_loop.RunUntilIdle(); On 2017/01/13 19:01:15, mark a. foltz wrote: > It seems like in most cases |mock_callback| will be invoked; do you need to set > an expectation on it? Or is that done in the test cases below? Set in test cases below. https://codereview.chromium.org/2598063002/diff/160001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher_unittest.cc:268: const std::vector<blink::WebVector<blink::WebURL>> urls_set_; On 2017/01/13 19:01:16, mark a. foltz wrote: > |urls_| ? removed :) https://codereview.chromium.org/2598063002/diff/160001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher_unittest.cc:483: auto* mock_callback = new MockPresentationAvailabilityCallbacks(); On 2017/01/13 19:01:16, mark a. foltz wrote: > You can use a StrictMock<> to verify that mock_callback is not invoked in this > test. > > https://github.com/google/googletest/blob/master/googlemock/docs/CookBook.md Done. https://codereview.chromium.org/2598063002/diff/160001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher_unittest.cc:486: TestGetAvailability(urls, states, mock_callback); On 2017/01/13 19:01:16, mark a. foltz wrote: > Could this just take a vector<GURL> and convert to WebURL internally, so you > could write: > > TestGetAvailability({url1_}, {}, mock_callback) Done. https://codereview.chromium.org/2598063002/diff/160001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher_unittest.cc:491: EXPECT_CALL(*mock_callback, onSuccess(true)); On 2017/01/13 19:01:16, mark a. foltz wrote: > Nit: consistently add a blank line after setting expectations Done. https://codereview.chromium.org/2598063002/diff/160001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher_unittest.cc:635: ChangeURLState(gurl2_, URLState::Unavailable); On 2017/01/13 19:01:16, mark a. foltz wrote: > Set expectation that mock_observer{1,2}_ will have availabilityChanged(false) > invoked Done. https://codereview.chromium.org/2598063002/diff/160001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher_unittest.cc:661: ChangeURLState(gurl1_, URLState::Available); On 2017/01/13 19:01:16, mark a. foltz wrote: > Can you change the state to Unavailable and test that availabiltyChanged(false) > is invoked? Done. https://codereview.chromium.org/2598063002/diff/160001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher_unittest.cc:666: OnScreenAvailabilityUpdatedInvokesMultipleAvailabilityChanged) { On 2017/01/13 19:01:15, mark a. foltz wrote: > I didn't see any test of availabilityChanged() in this test case. Done.
https://codereview.chromium.org/2598063002/diff/180001/content/renderer/prese... File content/renderer/presentation/presentation_dispatcher.cc (right): https://codereview.chromium.org/2598063002/diff/180001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher.cc:289: availability_set_.insert(base::WrapUnique(status)); Do we ever erase from / clear |availability_set_|? Looking again, it seems like we don't erase or clear |availability_status_| before the changes either. I believe the correct behavior should be to clear them on navigation, and maybe when the object no longer has any callbacks/observers. WDYT? https://codereview.chromium.org/2598063002/diff/180001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher.cc:393: auto screen_availability = GetScreenAvailability(status->urls); nit: blank line above this https://codereview.chromium.org/2598063002/diff/180001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher.cc:406: status->availability_callbacks.Clear(); The original code had some logic to stop listening after this when there are no more observers/callbacks. https://codereview.chromium.org/2598063002/diff/180001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher.cc:435: const blink::WebString& not_supported_error = blink::WebString::fromUTF8( nit: put this outside the for-loop. https://codereview.chromium.org/2598063002/diff/180001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher.cc:587: DCHECK(listening_status); This will still blow up in release builds. Suggest either turning this into a CHECK or handle nullptr gracefully. https://codereview.chromium.org/2598063002/diff/180001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher.cc:614: if (status_it == availability_set_.end()) nit: replace with: return status_it == availability_set_.end() ? nullptr : status_it->get(); https://codereview.chromium.org/2598063002/diff/180001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher.cc:640: DCHECK_EQ(counters[ScreenAvailability::UNKNOWN], urls.size()); nit: operator[] contains side effects so we avoid calling it inside a DCHECK. https://codereview.chromium.org/2598063002/diff/180001/content/renderer/prese... File content/renderer/presentation/presentation_dispatcher.h (right): https://codereview.chromium.org/2598063002/diff/180001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher.h:193: struct AvailabilityStatus { nit: This doesn't really contain the ScreenAvailability value so the struct name is a bit confusing. https://codereview.chromium.org/2598063002/diff/180001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher.h:194: AvailabilityStatus(const std::vector<GURL>& availability_urls); Still needs explicit? https://codereview.chromium.org/2598063002/diff/180001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher.h:197: std::vector<GURL> urls; Can this be const? https://codereview.chromium.org/2598063002/diff/180001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher.h:226: ListeningStatus* GetListeningStatus(const GURL& url); Can this method and the 2 below be const?
LGTM with imcheng@ comments addressed. https://codereview.chromium.org/2598063002/diff/180001/content/renderer/prese... File content/renderer/presentation/presentation_dispatcher.h (right): https://codereview.chromium.org/2598063002/diff/180001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher.h:193: struct AvailabilityStatus { On 2017/01/17 at 20:53:17, imcheng wrote: > nit: This doesn't really contain the ScreenAvailability value so the struct name is a bit confusing. Maybe AvailabilityCallbacks? https://codereview.chromium.org/2598063002/diff/180001/content/renderer/prese... File content/renderer/presentation/presentation_dispatcher_unittest.cc (right): https://codereview.chromium.org/2598063002/diff/180001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher_unittest.cc:50: explicit MockPresentationAvailabilityObserver(const std::vector<GURL>& gurls) Nit: s/gurls/urls/ for consistency https://codereview.chromium.org/2598063002/diff/180001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher_unittest.cc:245: // This function takes ownership of |mock_callback|. This comment probably belongs with the function-level comment above.
https://codereview.chromium.org/2598063002/diff/180001/content/renderer/prese... File content/renderer/presentation/presentation_dispatcher.cc (right): https://codereview.chromium.org/2598063002/diff/180001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher.cc:289: availability_set_.insert(base::WrapUnique(status)); On 2017/01/17 20:53:17, imcheng wrote: > Do we ever erase from / clear |availability_set_|? > > Looking again, it seems like we don't erase or clear |availability_status_| > before the changes either. I believe the correct behavior should be to clear > them on navigation, and maybe when the object no longer has any > callbacks/observers. WDYT? Done. https://codereview.chromium.org/2598063002/diff/180001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher.cc:393: auto screen_availability = GetScreenAvailability(status->urls); On 2017/01/17 20:53:17, imcheng wrote: > nit: blank line above this Done. https://codereview.chromium.org/2598063002/diff/180001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher.cc:406: status->availability_callbacks.Clear(); On 2017/01/17 20:53:17, imcheng wrote: > The original code had some logic to stop listening after this when there are no > more observers/callbacks. Done. https://codereview.chromium.org/2598063002/diff/180001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher.cc:435: const blink::WebString& not_supported_error = blink::WebString::fromUTF8( On 2017/01/17 20:53:17, imcheng wrote: > nit: put this outside the for-loop. Done. https://codereview.chromium.org/2598063002/diff/180001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher.cc:587: DCHECK(listening_status); On 2017/01/17 20:53:17, imcheng wrote: > This will still blow up in release builds. Suggest either turning this into a > CHECK or handle nullptr gracefully. Done. https://codereview.chromium.org/2598063002/diff/180001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher.cc:614: if (status_it == availability_set_.end()) On 2017/01/17 20:53:17, imcheng wrote: > nit: replace with: > > return status_it == availability_set_.end() ? nullptr : status_it->get(); Done. https://codereview.chromium.org/2598063002/diff/180001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher.cc:640: DCHECK_EQ(counters[ScreenAvailability::UNKNOWN], urls.size()); On 2017/01/17 20:53:17, imcheng wrote: > nit: operator[] contains side effects so we avoid calling it inside a DCHECK. Done. https://codereview.chromium.org/2598063002/diff/180001/content/renderer/prese... File content/renderer/presentation/presentation_dispatcher.h (right): https://codereview.chromium.org/2598063002/diff/180001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher.h:193: struct AvailabilityStatus { On 2017/01/17 21:04:24, mark a. foltz wrote: > On 2017/01/17 at 20:53:17, imcheng wrote: > > nit: This doesn't really contain the ScreenAvailability value so the struct > name is a bit confusing. > > Maybe AvailabilityCallbacks? AvailabilityListeners? https://codereview.chromium.org/2598063002/diff/180001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher.h:194: AvailabilityStatus(const std::vector<GURL>& availability_urls); On 2017/01/17 20:53:17, imcheng wrote: > Still needs explicit? Done. https://codereview.chromium.org/2598063002/diff/180001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher.h:197: std::vector<GURL> urls; On 2017/01/17 20:53:17, imcheng wrote: > Can this be const? Done. https://codereview.chromium.org/2598063002/diff/180001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher.h:226: ListeningStatus* GetListeningStatus(const GURL& url); On 2017/01/17 20:53:17, imcheng wrote: > Can this method and the 2 below be const? Done. https://codereview.chromium.org/2598063002/diff/180001/content/renderer/prese... File content/renderer/presentation/presentation_dispatcher_unittest.cc (right): https://codereview.chromium.org/2598063002/diff/180001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher_unittest.cc:50: explicit MockPresentationAvailabilityObserver(const std::vector<GURL>& gurls) On 2017/01/17 21:04:24, mark a. foltz wrote: > Nit: s/gurls/urls/ for consistency Done. https://codereview.chromium.org/2598063002/diff/180001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher_unittest.cc:245: // This function takes ownership of |mock_callback|. On 2017/01/17 21:04:24, mark a. foltz wrote: > This comment probably belongs with the function-level comment above. Done.
Removing self from reviewer list as it seems I'm not required here.
mlamouri@chromium.org changed reviewers: - mlamouri@chromium.org
https://codereview.chromium.org/2598063002/diff/200001/content/renderer/prese... File content/renderer/presentation/presentation_dispatcher.cc (right): https://codereview.chromium.org/2598063002/diff/200001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher.cc:597: void PresentationDispatcher::StopListeningToURL(const GURL& url) { nit: rename to MaybeStopListeningToURL since it doesn't always stop listening. https://codereview.chromium.org/2598063002/diff/200001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher.cc:652: availability_set_.erase(listener_it++); There should only be at most one listener entry in the set, so no point in continue iterating after it is removed? https://codereview.chromium.org/2598063002/diff/200001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher.cc:678: auto unknown_counter = counters[ScreenAvailability::UNKNOWN]; Sorry to be pedantic but this is introduces unneeded side effect. Maybe we don't need |counters| at all. Reading the logic in this function it seems like we can replace the map with 3 booleans (has_available, has_supported, has_unavailable). Even better may be to order the ScreenAvailability enums such that we can use max to determine the overall ScreenAvailability to return?
https://codereview.chromium.org/2598063002/diff/200001/content/renderer/prese... File content/renderer/presentation/presentation_dispatcher.cc (right): https://codereview.chromium.org/2598063002/diff/200001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher.cc:597: void PresentationDispatcher::StopListeningToURL(const GURL& url) { On 2017/01/18 22:40:32, imcheng wrote: > nit: rename to MaybeStopListeningToURL since it doesn't always stop listening. Done. https://codereview.chromium.org/2598063002/diff/200001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher.cc:652: availability_set_.erase(listener_it++); On 2017/01/18 22:40:32, imcheng wrote: > There should only be at most one listener entry in the set, so no point in > continue iterating after it is removed? Done. https://codereview.chromium.org/2598063002/diff/200001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher.cc:678: auto unknown_counter = counters[ScreenAvailability::UNKNOWN]; On 2017/01/18 22:40:32, imcheng wrote: > Sorry to be pedantic but this is introduces unneeded side effect. > > Maybe we don't need |counters| at all. Reading the logic in this function it > seems like we can replace the map with 3 booleans (has_available, has_supported, > has_unavailable). > > Even better may be to order the ScreenAvailability enums such that we can use > max to determine the overall ScreenAvailability to return? Done.
lgtm https://codereview.chromium.org/2598063002/diff/220001/content/renderer/prese... File content/renderer/presentation/presentation_dispatcher.cc (right): https://codereview.chromium.org/2598063002/diff/220001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher.cc:652: availability_set_.erase(listener_it++); The ++ here is redundant. Also just return; in the next statement?
The CQ bit was checked by zhaobin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by zhaobin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2598063002/diff/220001/content/renderer/prese... File content/renderer/presentation/presentation_dispatcher.cc (right): https://codereview.chromium.org/2598063002/diff/220001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher.cc:652: availability_set_.erase(listener_it++); On 2017/01/19 20:35:47, imcheng wrote: > The ++ here is redundant. Also just return; in the next statement? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by zhaobin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mfoltz@chromium.org, imcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2598063002/#ps240001 (title: "resolve code review comments from Derek")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 240001, "attempt_start_ts": 1484982619038250,
"parent_rev": "7ca861148c5375e15b85831ee2826a818d682c93", "commit_rev":
"9bc9d0de59b3d141441fae948fd09298fcfb5f75"}
Message was sent while issue was closed.
Description was changed from ========== [Presentation API] Handle multiple Presentation URLs in PresentationRequest::getAvailability() - Handle muliple URLs in getAvailability(), startListening(), and stopListening() - When screen availability for a particular url comes back, update all PresentationRequests related to this url - Added PresentationDispatcherUnittest BUG=627655 ========== to ========== [Presentation API] Handle multiple Presentation URLs in PresentationRequest::getAvailability() - Handle muliple URLs in getAvailability(), startListening(), and stopListening() - When screen availability for a particular url comes back, update all PresentationRequests related to this url - Added PresentationDispatcherUnittest BUG=627655 Review-Url: https://codereview.chromium.org/2598063002 Cr-Commit-Position: refs/heads/master@{#445278} Committed: https://chromium.googlesource.com/chromium/src/+/9bc9d0de59b3d141441fae948fd0... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:240001) as https://chromium.googlesource.com/chromium/src/+/9bc9d0de59b3d141441fae948fd0... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
