Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2028823002:
Refactor to make BlimpLocationProvider accessible to content layer. (Closed)
|
Created:
4 years, 6 months ago by CJ Modified:
4 years, 6 months ago CC:
chromium-reviews, anandc+watch-blimp_chromium.org, maniscalco+watch-blimp_chromium.org, sriramsr+watch-blimp_chromium.org, mlamouri+watch-geolocation_chromium.org, jam, nyquist+watch-blimp_chromium.org, marcinjb+watch-blimp_chromium.org, jessicag+watch-blimp_chromium.org, kmarshall+watch-blimp_chromium.org, lethalantidote+watch-blimp_chromium.org, Michael van Ouwerkerk, darin-cc_chromium.org, dtrainor+watch-blimp_chromium.org, shaktisahu+watch-blimp_chromium.org, khushalsagar+watch-blimp_chromium.org, scheib, mcasas Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRefactor to make BlimpLocationProvider accessible to content layer.
This change adds to ContentBrowserClient the ability to indicate whether one should use NetworkLocationProvider objects when accessing location data. The LocationArbitratorImpl now checks this boolean to determine what LocationProviders to create and register, allowing for system/override location providers to be used without the network location providers.
BlimpContentBrowserClient is updated to use these changes to indicate that NetworkLocationProvider objects should not be used. It also provides a BlimpLocationProvider through OverrideSystemLocationProvider. Since UseNetworkLocationProviders returns false, and we supply the override, Blimp will only use the BlimpLocationProvider for querying location.
Other additions:
* Moved non-NetworkLocationProvider creation out into a separate function to allow for optional instantiation of NetworkLocationProvider objects.
* Moved call to UseNetworkLocationProviders() so that its result is now passed to LocaitonArbitratorImpl through to GeolocationProviderImpl. This was done to give a better way to test the codepath for the case that UseNetworkLocationProviders returns false.
BUG=614486
Committed: https://crrev.com/564907f6ecd1040e0074aeeadf7da5e2ec7fdde6
Cr-Commit-Position: refs/heads/master@{#401453}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Fixes in response to creis #
Total comments: 1
Patch Set 3 : Changes to address kmarshall's comments #
Total comments: 6
Patch Set 4 : Addresses kmarshall's comments #
Total comments: 4
Patch Set 5 : Response to mvanouwerkerk's #21-1, #26 (partial) comments #
Total comments: 15
Patch Set 6 : Addresses Wez's comments #
Total comments: 2
Patch Set 7 : Moved commemt re: mvanouwerkerk's suggestion #Patch Set 8 : Makes testing override location provider accessible #
Total comments: 6
Patch Set 9 : Addresses kmarshall's and mvanouwerkerk's comments + code clean up #
Total comments: 44
Patch Set 10 : In response to mvanouwerkerk's #61 comments #Patch Set 11 : Rebase update #Patch Set 12 : Resolving merge conflict #Patch Set 13 : Addresses kmarshall's #63 comments #
Total comments: 15
Patch Set 14 : Addresses mvanouwerkerk's #81 comments #
Total comments: 24
Patch Set 15 : In response to Wez's #86 comments #
Total comments: 12
Patch Set 16 : Addresses mvanouwerkerk's #90 comments #Patch Set 17 : Addresses wez's #96 and creis's #95 comments #
Total comments: 2
Patch Set 18 : Addresses Wez's #101 comments #Patch Set 19 : Merge with Master #Patch Set 20 : Fixes try issue #Messages
Total messages: 120 (39 generated)
lethalantidote@chromium.org changed reviewers: + haibinlu@chromium.org, kmarshall@chromium.org, wez@chromium.org
lethalantidote@chromium.org changed reviewers: + haibinlu@chromium.org, kmarshall@chromium.org, wez@chromium.org
lethalantidote@chromium.org changed reviewers: + creis@chromium.org
+creis@, +mvanouwerkerk@ OWNERS.
On 2016/05/31 21:06:50, lethalantidote wrote: > +creis@, +mvanouwerkerk@ OWNERS. Please clarify what you want people to review. I assume mvanouwerkerk@ should review content/browser/geolocation? I'm not familiar with that one. A few nits on content/public and otherwise looks good. (Note: it generally helps to get at least one approval before going for content/ OWNERs.)
Oops, wrong button. Here's the nits. https://codereview.chromium.org/2028823002/diff/1/content/public/browser/cont... File content/public/browser/content_browser_client.h (right): https://codereview.chromium.org/2028823002/diff/1/content/public/browser/cont... content/public/browser/content_browser_client.h:625: // its sole LocationProvider. Called when any other LocationProvider nit: s/Called when/Should return a non-null value if/ https://codereview.chromium.org/2028823002/diff/1/content/public/browser/cont... content/public/browser/content_browser_client.h:627: virtual LocationProvider* SoleLocationProvider(); nit: GetSoleLocationProvider
On 2016/05/31 21:44:01, Charlie Reis wrote: > On 2016/05/31 21:06:50, lethalantidote wrote: > > +creis@, +mvanouwerkerk@ OWNERS. > > Please clarify what you want people to review. I assume mvanouwerkerk@ should > review content/browser/geolocation? I'm not familiar with that one. > > A few nits on content/public and otherwise looks good. (Note: it generally > helps to get at least one approval before going for content/ OWNERs.) +creis@ -> content/public/browser/ +mvanouwerkerk@ -> content/browser/geolocation (Sorry, I'm still learning. Does that suffice?).
https://codereview.chromium.org/2028823002/diff/1/content/public/browser/cont... File content/public/browser/content_browser_client.h (right): https://codereview.chromium.org/2028823002/diff/1/content/public/browser/cont... content/public/browser/content_browser_client.h:625: // its sole LocationProvider. Called when any other LocationProvider On 2016/05/31 21:44:18, Charlie Reis wrote: > nit: s/Called when/Should return a non-null value if/ Done. https://codereview.chromium.org/2028823002/diff/1/content/public/browser/cont... content/public/browser/content_browser_client.h:627: virtual LocationProvider* SoleLocationProvider(); On 2016/05/31 21:44:18, Charlie Reis wrote: > nit: GetSoleLocationProvider Done.
nit: CL description has some typos. :P I'd recommend splitting this CL into two - the one that adds the necessary Content-layer API, and a separate one that updates the Blimp embedder to make use of that API. Besides making for more logically scoped CLs, it would allow the CL descriptions to be clearer - the current description, for example, describes a mixture of Blimp-specific and more general changes.
https://codereview.chromium.org/2028823002/diff/20001/blimp/engine/app/blimp_... File blimp/engine/app/blimp_content_browser_client.h (right): https://codereview.chromium.org/2028823002/diff/20001/blimp/engine/app/blimp_... blimp/engine/app/blimp_content_browser_client.h:37: content::LocationProvider* GetSoleLocationProvider() override; The implementation is the same as OverrideSystemLocationProvider, but with subtly different semantics that must be obeyed by the caller. What if we just expressed those semantics specifically via a getter interface? Like e.g. have a virtual bool "UseDefaultLocationProviders()" that would return true by default in the base class, but would be overridden to return false for the BlimpContentBrowserClient. We would use the same OverrideSystemLocationProvider() method in that case.
On 2016/05/31 22:05:30, lethalantidote wrote: > On 2016/05/31 21:44:01, Charlie Reis wrote: > > On 2016/05/31 21:06:50, lethalantidote wrote: > > > +creis@, +mvanouwerkerk@ OWNERS. > > > > Please clarify what you want people to review. I assume mvanouwerkerk@ should > > review content/browser/geolocation? I'm not familiar with that one. > > > > A few nits on content/public and otherwise looks good. (Note: it generally > > helps to get at least one approval before going for content/ OWNERs.) > > +creis@ -> content/public/browser/ > +mvanouwerkerk@ -> content/browser/geolocation > > (Sorry, I'm still learning. Does that suffice?). Yep, that's perfect. On 2016/05/31 22:20:16, Wez wrote: > nit: CL description has some typos. :P > > I'd recommend splitting this CL into two - the one that adds the necessary > Content-layer API, and a separate one that updates the Blimp embedder to make > use of that API. Besides making for more logically scoped CLs, it would allow > the CL descriptions to be clearer - the current description, for example, > describes a mixture of Blimp-specific and more general changes. I'm going to veto that. :) We don't add new public APIs in content/ unless there's a use for them outside content/ in the same CL (in most circumstances).
On 2016/05/31 22:42:55, Kevin M wrote: > https://codereview.chromium.org/2028823002/diff/20001/blimp/engine/app/blimp_... > File blimp/engine/app/blimp_content_browser_client.h (right): > > https://codereview.chromium.org/2028823002/diff/20001/blimp/engine/app/blimp_... > blimp/engine/app/blimp_content_browser_client.h:37: content::LocationProvider* > GetSoleLocationProvider() override; > The implementation is the same as OverrideSystemLocationProvider, but with > subtly different semantics that must be obeyed by the caller. What if we just > expressed those semantics specifically via a getter interface? Like e.g. have a > virtual bool "UseDefaultLocationProviders()" that would return true by default > in the base class, but would be overridden to return false for the > BlimpContentBrowserClient. > > We would use the same OverrideSystemLocationProvider() method in that case. Done.
Description was changed from ========== Refactor to make BlimpLocationProvider accessible to content layer. This change adds a function to BlmpContentBrowserClient and ContentBrowserClient that will return a specified content provider intended to be the sole provider. For BlimpContentBrowswerClient, this is BlimpLocationProvider. The LocationArbitratorImpl queries for a sole LocationProvider, and if one is returned, will not create any of the other default providers. BUG=614486 ========== to ========== Refactor to make BlimpLocationProvider accessible to content layer. This change adds to ContentBrowserClient the ability to indicate whether one should use default location providers when accessing location data. The LocationArbitratorImpl now checks this boolean to determine what LocationProviders to create and register. BlimpContentBrowswerClient is updated to use these changes to indicate that default location providers should not be used and that it will instead provide a BlimpLocationProvider through OverrideSystemLocationProvider. BUG=614486 ==========
lgtm https://codereview.chromium.org/2028823002/diff/40001/blimp/engine/app/blimp_... File blimp/engine/app/blimp_content_browser_client.h (right): https://codereview.chromium.org/2028823002/diff/40001/blimp/engine/app/blimp_... blimp/engine/app/blimp_content_browser_client.h:37: content::LocationProvider* OverrideSystemLocationProvider() override; Cluster these overrides with the others under "content::ContentBrowserClient implementation." https://codereview.chromium.org/2028823002/diff/40001/content/browser/geoloca... File content/browser/geolocation/location_arbitrator_impl.cc (right): https://codereview.chromium.org/2028823002/diff/40001/content/browser/geoloca... content/browser/geolocation/location_arbitrator_impl.cc:112: LocationProvider* provider = nit: provider_override https://codereview.chromium.org/2028823002/diff/40001/content/public/browser/... File content/public/browser/content_browser_client.h (right): https://codereview.chromium.org/2028823002/diff/40001/content/public/browser/... content/public/browser/content_browser_client.h:624: // If false, indicates that LocationProviders normally used by a process Can you phrase this in a positive sense?
https://codereview.chromium.org/2028823002/diff/40001/blimp/engine/app/blimp_... File blimp/engine/app/blimp_content_browser_client.h (right): https://codereview.chromium.org/2028823002/diff/40001/blimp/engine/app/blimp_... blimp/engine/app/blimp_content_browser_client.h:37: content::LocationProvider* OverrideSystemLocationProvider() override; On 2016/05/31 23:43:55, Kevin M wrote: > Cluster these overrides with the others under "content::ContentBrowserClient > implementation." Done. https://codereview.chromium.org/2028823002/diff/40001/content/browser/geoloca... File content/browser/geolocation/location_arbitrator_impl.cc (right): https://codereview.chromium.org/2028823002/diff/40001/content/browser/geoloca... content/browser/geolocation/location_arbitrator_impl.cc:112: LocationProvider* provider = On 2016/05/31 23:43:55, Kevin M wrote: > nit: provider_override Done. https://codereview.chromium.org/2028823002/diff/40001/content/public/browser/... File content/public/browser/content_browser_client.h (right): https://codereview.chromium.org/2028823002/diff/40001/content/public/browser/... content/public/browser/content_browser_client.h:624: // If false, indicates that LocationProviders normally used by a process On 2016/05/31 23:43:55, Kevin M wrote: > Can you phrase this in a positive sense? Done.
https://codereview.chromium.org/2028823002/diff/40001/blimp/engine/app/blimp_... File blimp/engine/app/blimp_content_browser_client.h (right): https://codereview.chromium.org/2028823002/diff/40001/blimp/engine/app/blimp_... blimp/engine/app/blimp_content_browser_client.h:37: content::LocationProvider* OverrideSystemLocationProvider() override; On 2016/05/31 23:43:55, Kevin M wrote: > Cluster these overrides with the others under "content::ContentBrowserClient > implementation." Done. https://codereview.chromium.org/2028823002/diff/40001/content/browser/geoloca... File content/browser/geolocation/location_arbitrator_impl.cc (right): https://codereview.chromium.org/2028823002/diff/40001/content/browser/geoloca... content/browser/geolocation/location_arbitrator_impl.cc:112: LocationProvider* provider = On 2016/05/31 23:43:55, Kevin M wrote: > nit: provider_override Done. https://codereview.chromium.org/2028823002/diff/40001/content/public/browser/... File content/public/browser/content_browser_client.h (right): https://codereview.chromium.org/2028823002/diff/40001/content/public/browser/... content/public/browser/content_browser_client.h:624: // If false, indicates that LocationProviders normally used by a process On 2016/05/31 23:43:55, Kevin M wrote: > Can you phrase this in a positive sense? Done.
mvanouwerkerk@chromium.org changed reviewers: + mvanouwerkerk@chromium.org
I'll take a look. It helps when you actually add me as a reviewer, in addition to mentioning me in a comment :-)
Whatever approach we settle on, it should also be covered by a test. https://codereview.chromium.org/2028823002/diff/60001/blimp/engine/app/blimp_... File blimp/engine/app/blimp_content_browser_client.h (right): https://codereview.chromium.org/2028823002/diff/60001/blimp/engine/app/blimp_... blimp/engine/app/blimp_content_browser_client.h:43: BlimpLocationProvider location_provider_; I'm not sure that this actually works correctly as a value member right now. It means the provider is always instantiated, even when not needed, and its destructor calls StopProvider which says NOTIMPLEMENTED. Isn't this crashing at every shutdown? I'd suggest keeping an std::unique_ptr<BlimpLocationProvider> and instantiating one lazily when needed. That would also make it easier to e.g. insert a fake in tests. https://codereview.chromium.org/2028823002/diff/60001/content/browser/geoloca... File content/browser/geolocation/location_arbitrator_impl.cc (right): https://codereview.chromium.org/2028823002/diff/60001/content/browser/geoloca... content/browser/geolocation/location_arbitrator_impl.cc:103: if (GetContentClient()->browser()->UseDefaultLocationProviders()) { This adds one more layer of indirection, as NewNetworkLocationProvider already has an ifdef to return null on Android. You would want the same for Blimp, but my understanding is that Blimp doesn't have code in the content layer. Still, maybe you could reverse the ifdef in NewNetworkLocationProvider to only provide an instance for the specific configurations of which we know we want one, like in NewSystemLocationProvider: #if defined(OS_WIN) || defined(OS_MACOSX) || defined(OS_LINUX) That way, you won't need UseDefaultLocationProviders().
The CQ bit was checked by mvanouwerkerk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2028823002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2028823002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The bots are very colorful :-) Better make them green.
https://codereview.chromium.org/2028823002/diff/60001/blimp/engine/app/blimp_... File blimp/engine/app/blimp_content_browser_client.h (right): https://codereview.chromium.org/2028823002/diff/60001/blimp/engine/app/blimp_... blimp/engine/app/blimp_content_browser_client.h:43: BlimpLocationProvider location_provider_; On 2016/06/01 15:57:35, Michael van Ouwerkerk wrote: > I'm not sure that this actually works correctly as a value member right now. It > means the provider is always instantiated, even when not needed, and its > destructor calls StopProvider which says NOTIMPLEMENTED. Isn't this crashing at > every shutdown? > > I'd suggest keeping an std::unique_ptr<BlimpLocationProvider> and instantiating > one lazily when needed. That would also make it easier to e.g. insert a fake in > tests. Done.
The CQ bit was checked by lethalantidote@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2028823002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2028823002/80001
https://codereview.chromium.org/2028823002/diff/60001/content/browser/geoloca... File content/browser/geolocation/location_arbitrator_impl.cc (right): https://codereview.chromium.org/2028823002/diff/60001/content/browser/geoloca... content/browser/geolocation/location_arbitrator_impl.cc:103: if (GetContentClient()->browser()->UseDefaultLocationProviders()) { On 2016/06/01 15:57:35, Michael van Ouwerkerk wrote: > This adds one more layer of indirection, as NewNetworkLocationProvider already > has an ifdef to return null on Android. You would want the same for Blimp, but > my understanding is that Blimp doesn't have code in the content layer. Still, > maybe you could reverse the ifdef in NewNetworkLocationProvider to only provide > an instance for the specific configurations of which we know we want one, like > in NewSystemLocationProvider: > > #if defined(OS_WIN) || defined(OS_MACOSX) || defined(OS_LINUX) > > That way, you won't need UseDefaultLocationProviders(). Blimp is built with OS_LINUX, so it wouldn't quite work that way. We are also trying to stay away from introducing any new OS_ build flags to keep Blimp complexity separated from the rest of chrome.
Very nice :) https://codereview.chromium.org/2028823002/diff/80001/blimp/engine/app/blimp_... File blimp/engine/app/blimp_content_browser_client.cc (right): https://codereview.chromium.org/2028823002/diff/80001/blimp/engine/app/blimp_... blimp/engine/app/blimp_content_browser_client.cc:44: if (location_provider_ == nullptr) { nit: if (!location_provider_) (Testing a pointer against nullptr is a special-case in which we test-as-bool) https://codereview.chromium.org/2028823002/diff/80001/blimp/engine/app/blimp_... File blimp/engine/app/blimp_content_browser_client.h (right): https://codereview.chromium.org/2028823002/diff/80001/blimp/engine/app/blimp_... blimp/engine/app/blimp_content_browser_client.h:35: bool UseDefaultLocationProviders() override; nit: Are there "default" location providers besides the network-based ones? If not then I'd suggest that this be UseNetworkLocationProviders, or FallbackToNetworkLocationProviders() or something. (Otherwise it's not clear whether the default Android "system" location provider counts as a "default" one or a "system" one) https://codereview.chromium.org/2028823002/diff/80001/blimp/engine/feature/ge... File blimp/engine/feature/geolocation/blimp_location_provider.cc (right): https://codereview.chromium.org/2028823002/diff/80001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/blimp_location_provider.cc:24: } nit: Have you run "git cl format" on this? https://codereview.chromium.org/2028823002/diff/80001/content/browser/geoloca... File content/browser/geolocation/location_arbitrator_impl.cc (right): https://codereview.chromium.org/2028823002/diff/80001/content/browser/geoloca... content/browser/geolocation/location_arbitrator_impl.cc:103: if (GetContentClient()->browser()->UseDefaultLocationProviders()) { Do you need access-token-stores to have been loaded if we're not using network location? If not then is there somewhere earlier at which we could/should do this test? e.g Should we do this in StartProviders(), under the if (providers_.empty()) case, and skip both GetAccessTokenStore() and LoadAccessTokens() in the case in which we don't need the network providers? Do any non-network providers use the access tokens? https://codereview.chromium.org/2028823002/diff/80001/content/public/browser/... File content/public/browser/content_browser_client.h (right): https://codereview.chromium.org/2028823002/diff/80001/content/public/browser/... content/public/browser/content_browser_client.h:621: // information. nit: Now that we're actually using this in Chromium we could consider removing this comment? https://codereview.chromium.org/2028823002/diff/80001/content/public/browser/... content/public/browser/content_browser_client.h:625: // used. Not sure what "used by a process" means here. I'd suggest something like "Returns true if the location API should fall-back to using network-based location approximation." or similar.
https://codereview.chromium.org/2028823002/diff/80001/blimp/engine/app/blimp_... File blimp/engine/app/blimp_content_browser_client.cc (right): https://codereview.chromium.org/2028823002/diff/80001/blimp/engine/app/blimp_... blimp/engine/app/blimp_content_browser_client.cc:44: if (location_provider_ == nullptr) { On 2016/06/01 21:45:19, Wez wrote: > nit: if (!location_provider_) > > (Testing a pointer against nullptr is a special-case in which we test-as-bool) Thanks, I was wondering what was the actual way we did this! Done. https://codereview.chromium.org/2028823002/diff/80001/blimp/engine/app/blimp_... File blimp/engine/app/blimp_content_browser_client.h (right): https://codereview.chromium.org/2028823002/diff/80001/blimp/engine/app/blimp_... blimp/engine/app/blimp_content_browser_client.h:35: bool UseDefaultLocationProviders() override; On 2016/06/01 21:45:19, Wez wrote: > nit: Are there "default" location providers besides the network-based ones? If > not then I'd suggest that this be UseNetworkLocationProviders, or > FallbackToNetworkLocationProviders() or something. > > (Otherwise it's not clear whether the default Android "system" location provider > counts as a "default" one or a "system" one) Done. https://codereview.chromium.org/2028823002/diff/80001/blimp/engine/feature/ge... File blimp/engine/feature/geolocation/blimp_location_provider.cc (right): https://codereview.chromium.org/2028823002/diff/80001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/blimp_location_provider.cc:24: } On 2016/06/01 21:45:19, Wez wrote: > nit: Have you run "git cl format" on this? Done. https://codereview.chromium.org/2028823002/diff/80001/content/browser/geoloca... File content/browser/geolocation/location_arbitrator_impl.cc (right): https://codereview.chromium.org/2028823002/diff/80001/content/browser/geoloca... content/browser/geolocation/location_arbitrator_impl.cc:103: if (GetContentClient()->browser()->UseDefaultLocationProviders()) { On 2016/06/01 21:45:19, Wez wrote: > Do you need access-token-stores to have been loaded if we're not using network > location? If not then is there somewhere earlier at which we could/should do > this test? > > e.g Should we do this in StartProviders(), under the if (providers_.empty()) > case, and skip both GetAccessTokenStore() and LoadAccessTokens() in the case in > which we don't need the network providers? > > Do any non-network providers use the access tokens? Don't think so. Done. https://codereview.chromium.org/2028823002/diff/80001/content/public/browser/... File content/public/browser/content_browser_client.h (right): https://codereview.chromium.org/2028823002/diff/80001/content/public/browser/... content/public/browser/content_browser_client.h:621: // information. On 2016/06/01 21:45:19, Wez wrote: > nit: Now that we're actually using this in Chromium we could consider removing > this comment? Maybe keep it in case a refactor removes our use case? https://codereview.chromium.org/2028823002/diff/80001/content/public/browser/... content/public/browser/content_browser_client.h:625: // used. On 2016/06/01 21:45:19, Wez wrote: > Not sure what "used by a process" means here. > > I'd suggest something like "Returns true if the location API should fall-back to > using network-based location approximation." or similar. It doesn't really fall back on it though? If true, we use it in addition to the SystemLocationProvider. I did update the comment though. Let me know if you think this suffices.
The CQ bit was checked by mvanouwerkerk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2028823002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2028823002/100001
Thanks CJ! This generally looks good, but what is your test plan for covering this functionality? There are lots of geolocation tests, so you might be able to insert a test client with different responses. Also, please update the CL description to match the updated code. https://codereview.chromium.org/2028823002/diff/100001/content/browser/geoloc... File content/browser/geolocation/location_arbitrator_impl.cc (right): https://codereview.chromium.org/2028823002/diff/100001/content/browser/geoloc... content/browser/geolocation/location_arbitrator_impl.cc:55: // GetAccessTokenStore() will return NULL for embedders not implementing Keep this comment with the code it documents.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/06/02 10:04:32, Michael van Ouwerkerk wrote: > Thanks CJ! This generally looks good, but what is your test plan for covering > this functionality? There are lots of geolocation tests, so you might be able to > insert a test client with different responses. > > Also, please update the CL description to match the updated code. > > https://codereview.chromium.org/2028823002/diff/100001/content/browser/geoloc... > File content/browser/geolocation/location_arbitrator_impl.cc (right): > > https://codereview.chromium.org/2028823002/diff/100001/content/browser/geoloc... > content/browser/geolocation/location_arbitrator_impl.cc:55: // > GetAccessTokenStore() will return NULL for embedders not implementing > Keep this comment with the code it documents. Test update forthcoming.
Description was changed from ========== Refactor to make BlimpLocationProvider accessible to content layer. This change adds to ContentBrowserClient the ability to indicate whether one should use default location providers when accessing location data. The LocationArbitratorImpl now checks this boolean to determine what LocationProviders to create and register. BlimpContentBrowswerClient is updated to use these changes to indicate that default location providers should not be used and that it will instead provide a BlimpLocationProvider through OverrideSystemLocationProvider. BUG=614486 ========== to ========== Refactor to make BlimpLocationProvider accessible to content layer. This change adds to ContentBrowserClient the ability to indicate whether one should use default location providers when accessing location data. The LocationArbitratorImpl now checks this boolean to determine what LocationProviders to create and register. BlimpContentBrowserClient is updated to use these changes to indicate that default location providers should not be used and that it will instead provide a BlimpLocationProvider through OverrideSystemLocationProvider. Other additions: * Moved non-NetworkLocationProvider creation out into a separate function to allow for optional instantiation of NetworkLocationProvider objects. BUG=614486 ==========
https://codereview.chromium.org/2028823002/diff/100001/content/browser/geoloc... File content/browser/geolocation/location_arbitrator_impl.cc (right): https://codereview.chromium.org/2028823002/diff/100001/content/browser/geoloc... content/browser/geolocation/location_arbitrator_impl.cc:55: // GetAccessTokenStore() will return NULL for embedders not implementing On 2016/06/02 10:04:32, Michael van Ouwerkerk wrote: > Keep this comment with the code it documents. Done.
How does this CL relate to crrev.com/2015403002? Does it just need rebasing onto the prior one?
The CL description still mentions default providers, while ContentBrowserClient now works with specific types of providers. For clarity, please update the CL description.
Description was changed from ========== Refactor to make BlimpLocationProvider accessible to content layer. This change adds to ContentBrowserClient the ability to indicate whether one should use default location providers when accessing location data. The LocationArbitratorImpl now checks this boolean to determine what LocationProviders to create and register. BlimpContentBrowserClient is updated to use these changes to indicate that default location providers should not be used and that it will instead provide a BlimpLocationProvider through OverrideSystemLocationProvider. Other additions: * Moved non-NetworkLocationProvider creation out into a separate function to allow for optional instantiation of NetworkLocationProvider objects. BUG=614486 ========== to ========== Refactor to make BlimpLocationProvider accessible to content layer. This change adds to ContentBrowserClient the ability to indicate whether one should use NetworkLocationProvider objects when accessing location data. The LocationArbitratorImpl now checks this boolean to determine what LocationProviders to create and register, allowing for system/override location providers to be used without the network location providers. BlimpContentBrowserClient is updated to use these changes to indicate that NetworkLocationProvider objects should not be used. It also provides a BlimpLocationProvider through OverrideSystemLocationProvider. Since UseNetworkLocationProviders returns false, and we supply the override, Blimp will only use the BlimpLocationProvider for querying location. Other additions: * Moved non-NetworkLocationProvider creation out into a separate function to allow for optional instantiation of NetworkLocationProvider objects. * Moved call to UseNetworkLocationProviders() so that its result is now passed to LocaitonArbitratorImpl through to GeolocationProviderImpl. This was done to give a better way to test the codepath for the case that UseNetworkLocationProviders returns false. BUG=614486 ==========
Updated CL description. Also added support for testing the override and use_network = false code paths. Please let me know if this approach to testing is ok. I haven't added any actual unit tests on top of it yet in case I need to scrap it. The basic idea is to make a MockLocationProvider that will take the place of the override one, and use a boolean in the test to indicate whether we wish to use it or not. This was done instead of mocking out the getContentProvider->browser->OverrideSystemLocationProvider(), since it was not as accessible, but can be done if it is thought more reasonable. I also pulled out the check for using the network provider out so that the boolean can be passed in, and therefore more accessible for testing.
https://codereview.chromium.org/2028823002/diff/140001/content/browser/geoloc... File content/browser/geolocation/location_arbitrator.h (right): https://codereview.chromium.org/2028823002/diff/140001/content/browser/geoloc... content/browser/geolocation/location_arbitrator.h:20: virtual void StartProviders(bool enable_high_accuracy, bool use_network) = 0; I'm not yet convinced that adding a bool here is the right way to go, it's a bit confusing and not a very scalable approach. Maybe you can avoid adding this argument by inserting a fake ContentBrowserClient instead? See SetBrowserClientForTesting and TestContentBrowserClient. That way, you could fake its methods like OverrideSystemLocationProvider and UseNetworkLocationProviders.
Adding scheib to CC.
The CQ bit was checked by mvanouwerkerk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2028823002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/2028823002/diff/140001/content/browser/geoloc... File content/browser/geolocation/location_arbitrator.h (right): https://codereview.chromium.org/2028823002/diff/140001/content/browser/geoloc... content/browser/geolocation/location_arbitrator.h:20: virtual void StartProviders(bool enable_high_accuracy, bool use_network) = 0; On 2016/06/06 17:17:28, Michael van Ouwerkerk wrote: > I'm not yet convinced that adding a bool here is the right way to go, it's a bit > confusing and not a very scalable approach. Maybe you can avoid adding this > argument by inserting a fake ContentBrowserClient instead? See > SetBrowserClientForTesting and TestContentBrowserClient. That way, you could > fake its methods like OverrideSystemLocationProvider and > UseNetworkLocationProviders. +1 https://codereview.chromium.org/2028823002/diff/140001/content/browser/geoloc... File content/browser/geolocation/location_arbitrator_impl_unittest.cc (right): https://codereview.chromium.org/2028823002/diff/140001/content/browser/geoloc... content/browser/geolocation/location_arbitrator_impl_unittest.cc:75: override_(NULL), nit: use nullptr instead of NULL now, and you can switch all three of these to use inline initialization (MockLocatinoProvider* override_ = nullptr) https://codereview.chromium.org/2028823002/diff/140001/content/browser/geoloc... content/browser/geolocation/location_arbitrator_impl_unittest.cc:157: MockLocationProvider* overrider() { "override"?
On 2016/06/06 17:17:28, Michael van Ouwerkerk wrote: > https://codereview.chromium.org/2028823002/diff/140001/content/browser/geoloc... > File content/browser/geolocation/location_arbitrator.h (right): > > https://codereview.chromium.org/2028823002/diff/140001/content/browser/geoloc... > content/browser/geolocation/location_arbitrator.h:20: virtual void > StartProviders(bool enable_high_accuracy, bool use_network) = 0; > I'm not yet convinced that adding a bool here is the right way to go, it's a bit > confusing and not a very scalable approach. Maybe you can avoid adding this > argument by inserting a fake ContentBrowserClient instead? See > SetBrowserClientForTesting and TestContentBrowserClient. That way, you could > fake its methods like OverrideSystemLocationProvider and > UseNetworkLocationProviders. Thanks! I didn't know about those! I'll send an update soon with those used.
On 2016/06/06 17:17:28, Michael van Ouwerkerk wrote: > https://codereview.chromium.org/2028823002/diff/140001/content/browser/geoloc... > File content/browser/geolocation/location_arbitrator.h (right): > > https://codereview.chromium.org/2028823002/diff/140001/content/browser/geoloc... > content/browser/geolocation/location_arbitrator.h:20: virtual void > StartProviders(bool enable_high_accuracy, bool use_network) = 0; > I'm not yet convinced that adding a bool here is the right way to go, it's a bit > confusing and not a very scalable approach. Maybe you can avoid adding this > argument by inserting a fake ContentBrowserClient instead? See > SetBrowserClientForTesting and TestContentBrowserClient. That way, you could > fake its methods like OverrideSystemLocationProvider and > UseNetworkLocationProviders. Thanks! I didn't know about those! I'll send an update soon with those used.
On 2016/06/06 17:17:28, Michael van Ouwerkerk wrote: > https://codereview.chromium.org/2028823002/diff/140001/content/browser/geoloc... > File content/browser/geolocation/location_arbitrator.h (right): > > https://codereview.chromium.org/2028823002/diff/140001/content/browser/geoloc... > content/browser/geolocation/location_arbitrator.h:20: virtual void > StartProviders(bool enable_high_accuracy, bool use_network) = 0; > I'm not yet convinced that adding a bool here is the right way to go, it's a bit > confusing and not a very scalable approach. Maybe you can avoid adding this > argument by inserting a fake ContentBrowserClient instead? See > SetBrowserClientForTesting and TestContentBrowserClient. That way, you could > fake its methods like OverrideSystemLocationProvider and > UseNetworkLocationProviders. Thanks! I didn't know about those! I'll send an update soon with those used.
On 2016/06/06 17:17:28, Michael van Ouwerkerk wrote: > https://codereview.chromium.org/2028823002/diff/140001/content/browser/geoloc... > File content/browser/geolocation/location_arbitrator.h (right): > > https://codereview.chromium.org/2028823002/diff/140001/content/browser/geoloc... > content/browser/geolocation/location_arbitrator.h:20: virtual void > StartProviders(bool enable_high_accuracy, bool use_network) = 0; > I'm not yet convinced that adding a bool here is the right way to go, it's a bit > confusing and not a very scalable approach. Maybe you can avoid adding this > argument by inserting a fake ContentBrowserClient instead? See > SetBrowserClientForTesting and TestContentBrowserClient. That way, you could > fake its methods like OverrideSystemLocationProvider and > UseNetworkLocationProviders. Thanks! I didn't know about those! I'll send an update soon with those used.
Updated to address comments as well as adds tests for override and override/no-network code paths. https://codereview.chromium.org/2028823002/diff/140001/content/browser/geoloc... File content/browser/geolocation/location_arbitrator_impl_unittest.cc (right): https://codereview.chromium.org/2028823002/diff/140001/content/browser/geoloc... content/browser/geolocation/location_arbitrator_impl_unittest.cc:75: override_(NULL), On 2016/06/08 20:47:25, Kevin M wrote: > nit: use nullptr instead of NULL now, and you can switch all three of these to > use inline initialization (MockLocatinoProvider* override_ = nullptr) Done. https://codereview.chromium.org/2028823002/diff/140001/content/browser/geoloc... content/browser/geolocation/location_arbitrator_impl_unittest.cc:157: MockLocationProvider* overrider() { On 2016/06/08 20:47:25, Kevin M wrote: > "override"? Not sure if I was allowed to use it since override is a keyword..
The CQ bit was checked by lethalantidote@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2028823002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Thanks CJ! I think we're getting there. https://codereview.chromium.org/2028823002/diff/160001/content/browser/geoloc... File content/browser/geolocation/geolocation_provider_impl.cc (right): https://codereview.chromium.org/2028823002/diff/160001/content/browser/geoloc... content/browser/geolocation/geolocation_provider_impl.cc:18: #include "content/public/common/content_client.h" What are these includes used for? I can't find any usage of these clients in this file. https://codereview.chromium.org/2028823002/diff/160001/content/browser/geoloc... File content/browser/geolocation/location_arbitrator_impl.cc (right): https://codereview.chromium.org/2028823002/diff/160001/content/browser/geoloc... content/browser/geolocation/location_arbitrator_impl.cc:99: void LocationArbitratorImpl::RegisterSystemProvider() { Please keep the same order as in the header, so this method goes below RegisterProvider. https://codereview.chromium.org/2028823002/diff/160001/content/browser/geoloc... content/browser/geolocation/location_arbitrator_impl.cc:105: DoStartProviders(); I don't think DoStartProviders should be called form a method called RegisterSystemProvider, it is unexpected and does not match the behavior of RegisterProvider. You should probably just call DoStartProviders when it is needed from StartProviders or OnAccessTokenStoresLoaded. https://codereview.chromium.org/2028823002/diff/160001/content/browser/geoloc... File content/browser/geolocation/location_arbitrator_impl_unittest.cc (right): https://codereview.chromium.org/2028823002/diff/160001/content/browser/geoloc... content/browser/geolocation/location_arbitrator_impl_unittest.cc:68: class OverrideContentBrowserClient : public TestContentBrowserClient { The name of this class trips me up a bit, as I read Override as verb when at the start of the symbol name... making it look like a method name. How about GeolocationTestContentBrowserClient or FakeContentBrowserClient? https://codereview.chromium.org/2028823002/diff/160001/content/browser/geoloc... content/browser/geolocation/location_arbitrator_impl_unittest.cc:84: private: nit: I would expect to see a blank line before "private:". Did you run "git cl format"? https://codereview.chromium.org/2028823002/diff/160001/content/browser/geoloc... content/browser/geolocation/location_arbitrator_impl_unittest.cc:95: cell_ = nullptr; Why move these inside the constructor body? Generally, prefer using the initialization list. +1 for using nullptr though! https://codereview.chromium.org/2028823002/diff/160001/content/browser/geoloc... content/browser/geolocation/location_arbitrator_impl_unittest.cc:119: // Three location providers, with nice short names to make the tests more Just two again. https://codereview.chromium.org/2028823002/diff/160001/content/browser/geoloc... content/browser/geolocation/location_arbitrator_impl_unittest.cc:122: // type). |override_| will be included only if the ContentBrowser specifies |override_| is gone again https://codereview.chromium.org/2028823002/diff/160001/content/browser/geoloc... content/browser/geolocation/location_arbitrator_impl_unittest.cc:172: MockLocationProvider* overrider() { Two things here: 1) This is not a simple member getter, so it should be named in the GetFoo style. 2) Something like GetSystemLocationProviderOverride would be more meaningful, though admittedly much more verbose. https://codereview.chromium.org/2028823002/diff/160001/content/browser/geoloc... content/browser/geolocation/location_arbitrator_impl_unittest.cc:241: content::SetBrowserClientForTesting(override_content_browser_client_.get()); nit: I think there's no need for "content::" as we are in that namespace. https://codereview.chromium.org/2028823002/diff/160001/content/browser/geoloc... content/browser/geolocation/location_arbitrator_impl_unittest.cc:242: ASSERT_TRUE(arbitrator_ != NULL); nit: use nullptr in new code, but even simpler to just write "ASSERT_TRUE(arbitrator_);" https://codereview.chromium.org/2028823002/diff/160001/content/browser/geoloc... content/browser/geolocation/location_arbitrator_impl_unittest.cc:274: content::SetBrowserClientForTesting(override_content_browser_client_.get()); nit: I think there's no need for "content::" as we are in that namespace. https://codereview.chromium.org/2028823002/diff/160001/content/browser/geoloc... content/browser/geolocation/location_arbitrator_impl_unittest.cc:275: ASSERT_TRUE(arbitrator_ != NULL); nit: use nullptr in new code, but even simpler to just write "ASSERT_TRUE(arbitrator_);" https://codereview.chromium.org/2028823002/diff/160001/content/browser/geoloc... content/browser/geolocation/location_arbitrator_impl_unittest.cc:283: EXPECT_TRUE(access_token_store_->access_token_map_.empty()); delete this duplicate line https://codereview.chromium.org/2028823002/diff/160001/content/browser/geoloc... content/browser/geolocation/location_arbitrator_impl_unittest.cc:378: // 14 minutes later AdvanceTimeNow(base::TimeDelta::FromMinutes(14)); Did you intend to comment out this line?
https://codereview.chromium.org/2028823002/diff/160001/content/browser/geoloc... File content/browser/geolocation/geolocation_provider_impl.cc (right): https://codereview.chromium.org/2028823002/diff/160001/content/browser/geoloc... content/browser/geolocation/geolocation_provider_impl.cc:18: #include "content/public/common/content_client.h" On 2016/06/09 13:03:10, Michael van Ouwerkerk wrote: > What are these includes used for? I can't find any usage of these clients in > this file. Forgot to remove them. My bad. https://codereview.chromium.org/2028823002/diff/160001/content/browser/geoloc... File content/browser/geolocation/location_arbitrator_impl.cc (right): https://codereview.chromium.org/2028823002/diff/160001/content/browser/geoloc... content/browser/geolocation/location_arbitrator_impl.cc:99: void LocationArbitratorImpl::RegisterSystemProvider() { On 2016/06/09 13:03:10, Michael van Ouwerkerk wrote: > Please keep the same order as in the header, so this method goes below > RegisterProvider. Done. https://codereview.chromium.org/2028823002/diff/160001/content/browser/geoloc... content/browser/geolocation/location_arbitrator_impl.cc:105: DoStartProviders(); On 2016/06/09 13:03:10, Michael van Ouwerkerk wrote: > I don't think DoStartProviders should be called form a method called > RegisterSystemProvider, it is unexpected and does not match the behavior of > RegisterProvider. You should probably just call DoStartProviders when it is > needed from StartProviders or OnAccessTokenStoresLoaded. Done. https://codereview.chromium.org/2028823002/diff/160001/content/browser/geoloc... File content/browser/geolocation/location_arbitrator_impl_unittest.cc (right): https://codereview.chromium.org/2028823002/diff/160001/content/browser/geoloc... content/browser/geolocation/location_arbitrator_impl_unittest.cc:68: class OverrideContentBrowserClient : public TestContentBrowserClient { On 2016/06/09 13:03:11, Michael van Ouwerkerk wrote: > The name of this class trips me up a bit, as I read Override as verb when at the > start of the symbol name... making it look like a method name. How about > GeolocationTestContentBrowserClient or FakeContentBrowserClient? Done. https://codereview.chromium.org/2028823002/diff/160001/content/browser/geoloc... content/browser/geolocation/location_arbitrator_impl_unittest.cc:84: private: On 2016/06/09 13:03:10, Michael van Ouwerkerk wrote: > nit: I would expect to see a blank line before "private:". Did you run "git cl > format"? Done. https://codereview.chromium.org/2028823002/diff/160001/content/browser/geoloc... content/browser/geolocation/location_arbitrator_impl_unittest.cc:95: cell_ = nullptr; On 2016/06/09 13:03:10, Michael van Ouwerkerk wrote: > Why move these inside the constructor body? Generally, prefer using the > initialization list. +1 for using nullptr though! Done. https://codereview.chromium.org/2028823002/diff/160001/content/browser/geoloc... content/browser/geolocation/location_arbitrator_impl_unittest.cc:119: // Three location providers, with nice short names to make the tests more On 2016/06/09 13:03:11, Michael van Ouwerkerk wrote: > Just two again. Done. https://codereview.chromium.org/2028823002/diff/160001/content/browser/geoloc... content/browser/geolocation/location_arbitrator_impl_unittest.cc:122: // type). |override_| will be included only if the ContentBrowser specifies On 2016/06/09 13:03:10, Michael van Ouwerkerk wrote: > |override_| is gone again Done. https://codereview.chromium.org/2028823002/diff/160001/content/browser/geoloc... content/browser/geolocation/location_arbitrator_impl_unittest.cc:172: MockLocationProvider* overrider() { On 2016/06/09 13:03:11, Michael van Ouwerkerk wrote: > Two things here: > 1) This is not a simple member getter, so it should be named in the GetFoo > style. > 2) Something like GetSystemLocationProviderOverride would be more meaningful, > though admittedly much more verbose. Done. https://codereview.chromium.org/2028823002/diff/160001/content/browser/geoloc... content/browser/geolocation/location_arbitrator_impl_unittest.cc:241: content::SetBrowserClientForTesting(override_content_browser_client_.get()); On 2016/06/09 13:03:11, Michael van Ouwerkerk wrote: > nit: I think there's no need for "content::" as we are in that namespace. Done. https://codereview.chromium.org/2028823002/diff/160001/content/browser/geoloc... content/browser/geolocation/location_arbitrator_impl_unittest.cc:242: ASSERT_TRUE(arbitrator_ != NULL); On 2016/06/09 13:03:11, Michael van Ouwerkerk wrote: > nit: use nullptr in new code, but even simpler to just write > "ASSERT_TRUE(arbitrator_);" Done. https://codereview.chromium.org/2028823002/diff/160001/content/browser/geoloc... content/browser/geolocation/location_arbitrator_impl_unittest.cc:274: content::SetBrowserClientForTesting(override_content_browser_client_.get()); On 2016/06/09 13:03:11, Michael van Ouwerkerk wrote: > nit: I think there's no need for "content::" as we are in that namespace. Done. https://codereview.chromium.org/2028823002/diff/160001/content/browser/geoloc... content/browser/geolocation/location_arbitrator_impl_unittest.cc:275: ASSERT_TRUE(arbitrator_ != NULL); On 2016/06/09 13:03:10, Michael van Ouwerkerk wrote: > nit: use nullptr in new code, but even simpler to just write > "ASSERT_TRUE(arbitrator_);" Done. https://codereview.chromium.org/2028823002/diff/160001/content/browser/geoloc... content/browser/geolocation/location_arbitrator_impl_unittest.cc:283: EXPECT_TRUE(access_token_store_->access_token_map_.empty()); On 2016/06/09 13:03:11, Michael van Ouwerkerk wrote: > delete this duplicate line Was wondering about it since it was in the original test. Removing there too. https://codereview.chromium.org/2028823002/diff/160001/content/browser/geoloc... content/browser/geolocation/location_arbitrator_impl_unittest.cc:378: // 14 minutes later AdvanceTimeNow(base::TimeDelta::FromMinutes(14)); On 2016/06/09 13:03:10, Michael van Ouwerkerk wrote: > Did you intend to comment out this line? Done.
https://codereview.chromium.org/2028823002/diff/160001/content/browser/geoloc... File content/browser/geolocation/location_arbitrator_impl.cc (right): https://codereview.chromium.org/2028823002/diff/160001/content/browser/geoloc... content/browser/geolocation/location_arbitrator_impl.cc:69: return; What about loading the system providers instead of aborting completely? https://codereview.chromium.org/2028823002/diff/160001/content/browser/geoloc... content/browser/geolocation/location_arbitrator_impl.cc:75: } else { Should this be an "else"? Aren't the network location providers loaded in addition to the system provider? https://codereview.chromium.org/2028823002/diff/160001/content/browser/geoloc... content/browser/geolocation/location_arbitrator_impl.cc:125: RegisterSystemProvider(); Do we need to register system providers in this call, or can we do it before we kick off the async token fetch? https://codereview.chromium.org/2028823002/diff/160001/content/browser/geoloc... File content/browser/geolocation/location_arbitrator_impl.h (right): https://codereview.chromium.org/2028823002/diff/160001/content/browser/geoloc... content/browser/geolocation/location_arbitrator_impl.h:71: void RegisterSystemProvider(); Separate functions w/leading comments with a newline. Can you add a comment for your new function? https://codereview.chromium.org/2028823002/diff/160001/content/browser/geoloc... File content/browser/geolocation/mock_location_provider.cc (right): https://codereview.chromium.org/2028823002/diff/160001/content/browser/geoloc... content/browser/geolocation/mock_location_provider.cc:101: make these private https://codereview.chromium.org/2028823002/diff/160001/content/browser/geoloc... content/browser/geolocation/mock_location_provider.cc:102: base::WeakPtrFactory<MockLocationProvider> weak_factory_; I know this isn't your module, but here are a few suggestions for making it a little spiffier while we're here. At the same time, there's so much that I want to comment on here :F nit: weak factory should always be the final field in the list nit: DISALLOW_COPY_AND_ASSIGN() nit: I don't know if weak factory is even useful here, because this is test code and destruction order is easier to control. you can try changing the "weak_factory_.GetWeakPtr()" call with "base::Unretained(this)" and see if the test code continues to work.
The CQ bit was checked by lethalantidote@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2028823002/200001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
The CQ bit was checked by lethalantidote@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2028823002/220001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by lethalantidote@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2028823002/240001
https://codereview.chromium.org/2028823002/diff/160001/content/browser/geoloc... File content/browser/geolocation/location_arbitrator_impl.cc (right): https://codereview.chromium.org/2028823002/diff/160001/content/browser/geoloc... content/browser/geolocation/location_arbitrator_impl.cc:69: return; On 2016/06/09 20:36:44, Kevin M wrote: > What about loading the system providers instead of aborting completely? Done. https://codereview.chromium.org/2028823002/diff/160001/content/browser/geoloc... content/browser/geolocation/location_arbitrator_impl.cc:75: } else { On 2016/06/09 20:36:44, Kevin M wrote: > Should this be an "else"? Aren't the network location providers loaded in > addition to the system provider? Done. https://codereview.chromium.org/2028823002/diff/160001/content/browser/geoloc... content/browser/geolocation/location_arbitrator_impl.cc:125: RegisterSystemProvider(); On 2016/06/09 20:36:44, Kevin M wrote: > Do we need to register system providers in this call, or can we do it before we > kick off the async token fetch? Done. https://codereview.chromium.org/2028823002/diff/160001/content/browser/geoloc... File content/browser/geolocation/location_arbitrator_impl.h (right): https://codereview.chromium.org/2028823002/diff/160001/content/browser/geoloc... content/browser/geolocation/location_arbitrator_impl.h:71: void RegisterSystemProvider(); On 2016/06/09 20:36:44, Kevin M wrote: > Separate functions w/leading comments with a newline. > > Can you add a comment for your new function? Done. https://codereview.chromium.org/2028823002/diff/160001/content/browser/geoloc... File content/browser/geolocation/mock_location_provider.cc (right): https://codereview.chromium.org/2028823002/diff/160001/content/browser/geoloc... content/browser/geolocation/mock_location_provider.cc:101: On 2016/06/09 20:36:44, Kevin M wrote: > make these private Done. https://codereview.chromium.org/2028823002/diff/160001/content/browser/geoloc... content/browser/geolocation/mock_location_provider.cc:102: base::WeakPtrFactory<MockLocationProvider> weak_factory_; On 2016/06/09 20:36:44, Kevin M wrote: > I know this isn't your module, but here are a few suggestions for making it a > little spiffier while we're here. At the same time, there's so much that I want > to comment on here :F > > nit: weak factory should always be the final field in the list > nit: DISALLOW_COPY_AND_ASSIGN() > nit: I don't know if weak factory is even useful here, because this is test code > and destruction order is easier to control. you can try changing the > "weak_factory_.GetWeakPtr()" call with "base::Unretained(this)" and see if the > test code continues to work. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by lethalantidote@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2028823002/240001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Exceeded global retry quota
https://codereview.chromium.org/2028823002/diff/240001/blimp/engine/app/blimp... File blimp/engine/app/blimp_content_browser_client.h (right): https://codereview.chromium.org/2028823002/diff/240001/blimp/engine/app/blimp... blimp/engine/app/blimp_content_browser_client.h:9: #include "blimp/engine/feature/geolocation/blimp_location_provider.h" nit: you could forward declare BlimpLocationProvider and include this in the cc file. https://codereview.chromium.org/2028823002/diff/240001/content/browser/geoloc... File content/browser/geolocation/location_arbitrator_impl.h (right): https://codereview.chromium.org/2028823002/diff/240001/content/browser/geoloc... content/browser/geolocation/location_arbitrator_impl.h:95: base::CancelableCallback<void(AccessTokenStore::AccessTokenMap, Why is this cancelable but never cancelled, and why is it stored in the arbitrator? It seems neither aspect is needed. https://codereview.chromium.org/2028823002/diff/240001/content/browser/geoloc... File content/browser/geolocation/location_arbitrator_impl_unittest.cc (left): https://codereview.chromium.org/2028823002/diff/240001/content/browser/geoloc... content/browser/geolocation/location_arbitrator_impl_unittest.cc:269: nit: please leave this blank line as is https://codereview.chromium.org/2028823002/diff/240001/content/browser/geoloc... File content/browser/geolocation/location_arbitrator_impl_unittest.cc (right): https://codereview.chromium.org/2028823002/diff/240001/content/browser/geoloc... content/browser/geolocation/location_arbitrator_impl_unittest.cc:119: MockLocationProvider* cell_; nit: While these names (cell_ and gps_) may have been good at some point in time, it looks like things have diverged. For example, there used to be plans for a cell tower based location provider, but what we have now as a network location provider is based on IP address and wifi access points. Would you mind adding a TODO in my name to rename |cell_| to |network_location_provider_| and |gps_| to |system_location_provider_|?
https://codereview.chromium.org/2028823002/diff/240001/blimp/engine/app/blimp... File blimp/engine/app/blimp_content_browser_client.h (right): https://codereview.chromium.org/2028823002/diff/240001/blimp/engine/app/blimp... blimp/engine/app/blimp_content_browser_client.h:9: #include "blimp/engine/feature/geolocation/blimp_location_provider.h" On 2016/06/10 12:46:52, Michael van Ouwerkerk wrote: > nit: you could forward declare BlimpLocationProvider and include this in the cc > file. I thought convention was to have the include in the same file in which its contents are referenced? https://codereview.chromium.org/2028823002/diff/240001/content/browser/geoloc... File content/browser/geolocation/location_arbitrator_impl.h (right): https://codereview.chromium.org/2028823002/diff/240001/content/browser/geoloc... content/browser/geolocation/location_arbitrator_impl.h:95: base::CancelableCallback<void(AccessTokenStore::AccessTokenMap, On 2016/06/10 12:46:52, Michael van Ouwerkerk wrote: > Why is this cancelable but never cancelled, and why is it stored in the > arbitrator? It seems neither aspect is needed. This was done so the reset method could be used. The arbitrator was doing checks in the OnAccessTokenStoresLoaded method that were basically doing the functionality of Reset(), but the checks caused the need for some weird control flow. https://codereview.chromium.org/2028823002/diff/240001/content/browser/geoloc... File content/browser/geolocation/location_arbitrator_impl_unittest.cc (left): https://codereview.chromium.org/2028823002/diff/240001/content/browser/geoloc... content/browser/geolocation/location_arbitrator_impl_unittest.cc:269: On 2016/06/10 12:46:52, Michael van Ouwerkerk wrote: > nit: please leave this blank line as is Done. https://codereview.chromium.org/2028823002/diff/240001/content/browser/geoloc... File content/browser/geolocation/location_arbitrator_impl_unittest.cc (right): https://codereview.chromium.org/2028823002/diff/240001/content/browser/geoloc... content/browser/geolocation/location_arbitrator_impl_unittest.cc:119: MockLocationProvider* cell_; On 2016/06/10 12:46:52, Michael van Ouwerkerk wrote: > nit: While these names (cell_ and gps_) may have been good at some point in > time, it looks like things have diverged. For example, there used to be plans > for a cell tower based location provider, but what we have now as a network > location provider is based on IP address and wifi access points. Would you mind > adding a TODO in my name to rename |cell_| to |network_location_provider_| and > |gps_| to |system_location_provider_|? Done.
https://codereview.chromium.org/2028823002/diff/240001/content/browser/geoloc... File content/browser/geolocation/location_arbitrator_impl.h (right): https://codereview.chromium.org/2028823002/diff/240001/content/browser/geoloc... content/browser/geolocation/location_arbitrator_impl.h:95: base::CancelableCallback<void(AccessTokenStore::AccessTokenMap, On 2016/06/10 19:51:34, CJ wrote: > On 2016/06/10 12:46:52, Michael van Ouwerkerk wrote: > > Why is this cancelable but never cancelled, and why is it stored in the > > arbitrator? It seems neither aspect is needed. > > This was done so the reset method could be used. The arbitrator was doing checks > in the OnAccessTokenStoresLoaded method that were basically doing the > functionality of Reset(), but the checks caused the need for some weird control > flow. To be a little more specific - it's cancelable so that OnAccessTokenStoresLoaded() only gets called once. The previous implementation of OnAccessTokenStoresLoaded did something similar in a more roundabout way by checking whether |providers_| is empty.
The CQ bit was checked by lethalantidote@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2028823002/260001
Looks good, w/ a few remaining nits. https://codereview.chromium.org/2028823002/diff/80001/content/public/browser/... File content/public/browser/content_browser_client.h (right): https://codereview.chromium.org/2028823002/diff/80001/content/public/browser/... content/public/browser/content_browser_client.h:621: // information. On 2016/06/01 22:37:10, CJ wrote: > On 2016/06/01 21:45:19, Wez wrote: > > nit: Now that we're actually using this in Chromium we could consider removing > > this comment? > > Maybe keep it in case a refactor removes our use case? Acknowledged. https://codereview.chromium.org/2028823002/diff/80001/content/public/browser/... content/public/browser/content_browser_client.h:625: // used. On 2016/06/01 22:37:10, CJ wrote: > On 2016/06/01 21:45:19, Wez wrote: > > Not sure what "used by a process" means here. > > > > I'd suggest something like "Returns true if the location API should fall-back > to > > using network-based location approximation." or similar. > > It doesn't really fall back on it though? If true, we use it in addition to the > SystemLocationProvider. I did update the comment though. Let me know if you > think this suffices. Looks good - you might say "... in addition to the system provider, if any." https://codereview.chromium.org/2028823002/diff/160001/content/browser/geoloc... File content/browser/geolocation/location_arbitrator_impl.h (right): https://codereview.chromium.org/2028823002/diff/160001/content/browser/geoloc... content/browser/geolocation/location_arbitrator_impl.h:70: void RegisterProvider(LocationProvider* provider); nit: Since we're already modifying this header, could you tweak this to take a unique_ptr - then you can trim the comment down to just the "added or deleted" aspect. https://codereview.chromium.org/2028823002/diff/260001/content/browser/geoloc... File content/browser/geolocation/location_arbitrator_impl.cc (right): https://codereview.chromium.org/2028823002/diff/260001/content/browser/geoloc... content/browser/geolocation/location_arbitrator_impl.cc:80: // requests of location to wait eternally for a reply. nit: Strange wording; I think you mean ".. to avoid JS callers waiting indefinitely for a reply."? This seems less to avoid JS waiting indefinitely, and more to be a simple case of that being the way the API is suppose to work - that it returns an error if it cannot succeed? https://codereview.chromium.org/2028823002/diff/260001/content/browser/geoloc... File content/browser/geolocation/location_arbitrator_impl.h (right): https://codereview.chromium.org/2028823002/diff/260001/content/browser/geoloc... content/browser/geolocation/location_arbitrator_impl.h:75: void RegisterSystemProvider(); nit: If you're going to have a per-function comment then put a blank line after the function, so it doesn't look like a comment on the whole block. https://codereview.chromium.org/2028823002/diff/260001/content/browser/geoloc... File content/browser/geolocation/location_arbitrator_impl_unittest.cc (right): https://codereview.chromium.org/2028823002/diff/260001/content/browser/geoloc... content/browser/geolocation/location_arbitrator_impl_unittest.cc:70: void SetUseNetwork(bool use_network) { use_network_ = use_network; } nit: As per Chromium-style guide, simple setters and getters should use unix_hacker_style() naming. https://codereview.chromium.org/2028823002/diff/260001/content/browser/geoloc... content/browser/geolocation/location_arbitrator_impl_unittest.cc:72: LocationProvider* OverrideSystemLocationProvider() override { Note that in general we do not allow virtual functions to be declared in-line in Chromium - the only reason we're allowing it here is that that is already the style this code file follows, and it's a test file. Please don't use inline virtuals in production code, or new test code. :) https://codereview.chromium.org/2028823002/diff/260001/content/browser/geoloc... content/browser/geolocation/location_arbitrator_impl_unittest.cc:79: MockLocationProvider* provider_ = nullptr; Please add a comment here to clarify that this pointer does not own the object - when it is returned by Override...() the caller takes ownership, so this is just a reference we can use for mocking purposes. (I was about to ask why we need this, since we can just set expectations on the mock and then pass ownership in to the location framework, but looking at the MockLocationProvider, it seems it's actually _not_ a mock (thing that looks like Foo that just lets you test whether Foo APIs were called) but a fake (thing that is designed to actually pretend to behave like a real Foo). :-/ ) https://codereview.chromium.org/2028823002/diff/260001/content/browser/geoloc... content/browser/geolocation/location_arbitrator_impl_unittest.cc:83: }; nit: DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/2028823002/diff/260001/content/browser/geoloc... content/browser/geolocation/location_arbitrator_impl_unittest.cc:237: TEST_F(GeolocationLocationArbitratorTest, OverrideNoNetworkUsage) { nit: Suggest calling this case CustomSystemProviderOnly https://codereview.chromium.org/2028823002/diff/260001/content/browser/geoloc... content/browser/geolocation/location_arbitrator_impl_unittest.cc:270: TEST_F(GeolocationLocationArbitratorTest, OverrideAndNetworkUsage) { nit: Suggest CustomSystemAndDefaultNetworkProviders https://codereview.chromium.org/2028823002/diff/260001/content/browser/geoloc... content/browser/geolocation/location_arbitrator_impl_unittest.cc:417: std::unique_ptr<MockLocationProvider> fakeMockProvider( nit: Follow unix_hacker_style naming for variables. Also, "fake mock provider" is a super confusing name (see comment re mocks and fakes, above) - since we're modifying this anyway, let's call it dummy_provider, say? https://codereview.chromium.org/2028823002/diff/260001/content/browser/geoloc... File content/browser/geolocation/mock_location_provider.cc (right): https://codereview.chromium.org/2028823002/diff/260001/content/browser/geoloc... content/browser/geolocation/mock_location_provider.cc:97: base::Unretained(this), position_)); What's the rationale for this change? Is the new requirement that the caller must ensure that the provider remains alive after calling StartProvider or OnPermissionGranted, until HandlePositionChanged, documented somewhere? https://codereview.chromium.org/2028823002/diff/260001/content/browser/geoloc... File content/browser/geolocation/mock_location_provider.h (right): https://codereview.chromium.org/2028823002/diff/260001/content/browser/geoloc... content/browser/geolocation/mock_location_provider.h:24: MockLocationProvider(); As noted elsewhere, this should be FakeLocationProvider, but we can fix that later. ;)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2028823002/diff/80001/content/public/browser/... File content/public/browser/content_browser_client.h (right): https://codereview.chromium.org/2028823002/diff/80001/content/public/browser/... content/public/browser/content_browser_client.h:625: // used. On 2016/06/10 23:40:34, Wez wrote: > On 2016/06/01 22:37:10, CJ wrote: > > On 2016/06/01 21:45:19, Wez wrote: > > > Not sure what "used by a process" means here. > > > > > > I'd suggest something like "Returns true if the location API should > fall-back > > to > > > using network-based location approximation." or similar. > > > > It doesn't really fall back on it though? If true, we use it in addition to > the > > SystemLocationProvider. I did update the comment though. Let me know if you > > think this suffices. > > Looks good - you might say "... in addition to the system provider, if any." Done. https://codereview.chromium.org/2028823002/diff/160001/content/browser/geoloc... File content/browser/geolocation/location_arbitrator_impl.h (right): https://codereview.chromium.org/2028823002/diff/160001/content/browser/geoloc... content/browser/geolocation/location_arbitrator_impl.h:70: void RegisterProvider(LocationProvider* provider); On 2016/06/10 23:40:34, Wez wrote: > nit: Since we're already modifying this header, could you tweak this to take a > unique_ptr - then you can trim the comment down to just the "added or deleted" > aspect. Done. https://codereview.chromium.org/2028823002/diff/260001/content/browser/geoloc... File content/browser/geolocation/location_arbitrator_impl.cc (right): https://codereview.chromium.org/2028823002/diff/260001/content/browser/geoloc... content/browser/geolocation/location_arbitrator_impl.cc:80: // requests of location to wait eternally for a reply. On 2016/06/10 23:40:34, Wez wrote: > nit: Strange wording; I think you mean ".. to avoid JS callers waiting > indefinitely for a reply."? > > This seems less to avoid JS waiting indefinitely, and more to be a simple case > of that being the way the API is suppose to work - that it returns an error if > it cannot succeed? I didn't write the comment, just have been trying to keep it with the code it belongs with, so my insight into why it is written in such a way is limited. Updating according to your feedback, making it more about the general error handling. https://codereview.chromium.org/2028823002/diff/260001/content/browser/geoloc... File content/browser/geolocation/location_arbitrator_impl.h (right): https://codereview.chromium.org/2028823002/diff/260001/content/browser/geoloc... content/browser/geolocation/location_arbitrator_impl.h:75: void RegisterSystemProvider(); On 2016/06/10 23:40:34, Wez wrote: > nit: If you're going to have a per-function comment then put a blank line after > the function, so it doesn't look like a comment on the whole block. Done. https://codereview.chromium.org/2028823002/diff/260001/content/browser/geoloc... File content/browser/geolocation/location_arbitrator_impl_unittest.cc (right): https://codereview.chromium.org/2028823002/diff/260001/content/browser/geoloc... content/browser/geolocation/location_arbitrator_impl_unittest.cc:70: void SetUseNetwork(bool use_network) { use_network_ = use_network; } On 2016/06/10 23:40:35, Wez wrote: > nit: As per Chromium-style guide, simple setters and getters should use > unix_hacker_style() naming. Done. https://codereview.chromium.org/2028823002/diff/260001/content/browser/geoloc... content/browser/geolocation/location_arbitrator_impl_unittest.cc:72: LocationProvider* OverrideSystemLocationProvider() override { On 2016/06/10 23:40:35, Wez wrote: > Note that in general we do not allow virtual functions to be declared in-line in > Chromium - the only reason we're allowing it here is that that is already the > style this code file follows, and it's a test file. Please don't use inline > virtuals in production code, or new test code. :) Acknowledged. https://codereview.chromium.org/2028823002/diff/260001/content/browser/geoloc... content/browser/geolocation/location_arbitrator_impl_unittest.cc:79: MockLocationProvider* provider_ = nullptr; On 2016/06/10 23:40:34, Wez wrote: > Please add a comment here to clarify that this pointer does not own the object - > when it is returned by Override...() the caller takes ownership, so this is just > a reference we can use for mocking purposes. > > (I was about to ask why we need this, since we can just set expectations on the > mock and then pass ownership in to the location framework, but looking at the > MockLocationProvider, it seems it's actually _not_ a mock (thing that looks like > Foo that just lets you test whether Foo APIs were called) but a fake (thing that > is designed to actually pretend to behave like a real Foo). :-/ ) Done. https://codereview.chromium.org/2028823002/diff/260001/content/browser/geoloc... content/browser/geolocation/location_arbitrator_impl_unittest.cc:83: }; On 2016/06/10 23:40:35, Wez wrote: > nit: DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/2028823002/diff/260001/content/browser/geoloc... content/browser/geolocation/location_arbitrator_impl_unittest.cc:237: TEST_F(GeolocationLocationArbitratorTest, OverrideNoNetworkUsage) { On 2016/06/10 23:40:35, Wez wrote: > nit: Suggest calling this case CustomSystemProviderOnly Done. https://codereview.chromium.org/2028823002/diff/260001/content/browser/geoloc... content/browser/geolocation/location_arbitrator_impl_unittest.cc:270: TEST_F(GeolocationLocationArbitratorTest, OverrideAndNetworkUsage) { On 2016/06/10 23:40:35, Wez wrote: > nit: Suggest CustomSystemAndDefaultNetworkProviders Done. https://codereview.chromium.org/2028823002/diff/260001/content/browser/geoloc... content/browser/geolocation/location_arbitrator_impl_unittest.cc:417: std::unique_ptr<MockLocationProvider> fakeMockProvider( On 2016/06/10 23:40:35, Wez wrote: > nit: Follow unix_hacker_style naming for variables. > > Also, "fake mock provider" is a super confusing name (see comment re mocks and > fakes, above) - since we're modifying this anyway, let's call it dummy_provider, > say? Done. https://codereview.chromium.org/2028823002/diff/260001/content/browser/geoloc... File content/browser/geolocation/mock_location_provider.cc (right): https://codereview.chromium.org/2028823002/diff/260001/content/browser/geoloc... content/browser/geolocation/mock_location_provider.cc:97: base::Unretained(this), position_)); On 2016/06/10 23:40:35, Wez wrote: > What's the rationale for this change? Is the new requirement that the caller > must ensure that the provider remains alive after calling StartProvider or > OnPermissionGranted, until HandlePositionChanged, documented somewhere? I am truthfully not sure. This is something that kmarshall@ pointed out to be changed. I will talk to him about it. https://codereview.chromium.org/2028823002/diff/260001/content/browser/geoloc... File content/browser/geolocation/mock_location_provider.h (right): https://codereview.chromium.org/2028823002/diff/260001/content/browser/geoloc... content/browser/geolocation/mock_location_provider.h:24: MockLocationProvider(); On 2016/06/10 23:40:35, Wez wrote: > As noted elsewhere, this should be FakeLocationProvider, but we can fix that > later. ;) Acknowledged.
lgtm with nits Thanks! https://codereview.chromium.org/2028823002/diff/240001/blimp/engine/app/blimp... File blimp/engine/app/blimp_content_browser_client.h (right): https://codereview.chromium.org/2028823002/diff/240001/blimp/engine/app/blimp... blimp/engine/app/blimp_content_browser_client.h:9: #include "blimp/engine/feature/geolocation/blimp_location_provider.h" On 2016/06/10 19:51:34, CJ wrote: > On 2016/06/10 12:46:52, Michael van Ouwerkerk wrote: > > nit: you could forward declare BlimpLocationProvider and include this in the > cc > > file. > > I thought convention was to have the include in the same file in which its > contents are referenced? You're right, it is. However, it's also convention to use forward declarations in headers, as is done here for BlimpBrowserMainParts and BlimpBrowserContext. I mean, BlimpLocationProvider is not really used in this file is it, just referred to? https://codereview.chromium.org/2028823002/diff/240001/content/browser/geoloc... File content/browser/geolocation/location_arbitrator_impl.h (right): https://codereview.chromium.org/2028823002/diff/240001/content/browser/geoloc... content/browser/geolocation/location_arbitrator_impl.h:95: base::CancelableCallback<void(AccessTokenStore::AccessTokenMap, On 2016/06/10 19:51:34, CJ wrote: > On 2016/06/10 12:46:52, Michael van Ouwerkerk wrote: > > Why is this cancelable but never cancelled, and why is it stored in the > > arbitrator? It seems neither aspect is needed. > > This was done so the reset method could be used. The arbitrator was doing checks > in the OnAccessTokenStoresLoaded method that were basically doing the > functionality of Reset(), but the checks caused the need for some weird control > flow. Ah, that's really neat actually. Thanks! https://codereview.chromium.org/2028823002/diff/240001/content/browser/geoloc... content/browser/geolocation/location_arbitrator_impl.h:95: base::CancelableCallback<void(AccessTokenStore::AccessTokenMap, On 2016/06/10 20:39:11, Kevin M wrote: > On 2016/06/10 19:51:34, CJ wrote: > > On 2016/06/10 12:46:52, Michael van Ouwerkerk wrote: > > > Why is this cancelable but never cancelled, and why is it stored in the > > > arbitrator? It seems neither aspect is needed. > > > > This was done so the reset method could be used. The arbitrator was doing > checks > > in the OnAccessTokenStoresLoaded method that were basically doing the > > functionality of Reset(), but the checks caused the need for some weird > control > > flow. > > To be a little more specific - it's cancelable so that > OnAccessTokenStoresLoaded() only gets called once. The previous implementation > of OnAccessTokenStoresLoaded did something similar in a more roundabout way by > checking whether |providers_| is empty. It would be good to add a little documentation at the usage site to explain why this is needed (multiple calls).
The CQ bit was checked by mvanouwerkerk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2028823002/280001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
content/public/browser LGTM. https://codereview.chromium.org/2028823002/diff/280001/content/browser/geoloc... File content/browser/geolocation/location_arbitrator_impl.cc (right): https://codereview.chromium.org/2028823002/diff/280001/content/browser/geoloc... content/browser/geolocation/location_arbitrator_impl.cc:79: // If no providers are available, we report an error to avoid nit: Only one space after comma. https://codereview.chromium.org/2028823002/diff/280001/content/browser/geoloc... File content/browser/geolocation/location_arbitrator_impl.h (right): https://codereview.chromium.org/2028823002/diff/280001/content/browser/geoloc... content/browser/geolocation/location_arbitrator_impl.h:96: token_store_callback_; On 2016/06/14 16:46:42, mvanouwerkerk - OOO to June 20 wrote: > It would be good to add a little documentation at the usage site to explain why > this is needed (multiple calls). Just a reminder for this request.
OK, just a couple of remaining nits to clean up. https://codereview.chromium.org/2028823002/diff/260001/content/browser/geoloc... File content/browser/geolocation/mock_location_provider.cc (right): https://codereview.chromium.org/2028823002/diff/260001/content/browser/geoloc... content/browser/geolocation/mock_location_provider.cc:97: base::Unretained(this), position_)); On 2016/06/13 23:45:20, CJ wrote: > On 2016/06/10 23:40:35, Wez wrote: > > What's the rationale for this change? Is the new requirement that the caller > > must ensure that the provider remains alive after calling StartProvider or > > OnPermissionGranted, until HandlePositionChanged, documented somewhere? > > I am truthfully not sure. This is something that kmarshall@ pointed out to be > changed. I will talk to him about it. Acknowledged. https://codereview.chromium.org/2028823002/diff/280001/content/browser/geoloc... File content/browser/geolocation/location_arbitrator_impl_unittest.cc (right): https://codereview.chromium.org/2028823002/diff/280001/content/browser/geoloc... content/browser/geolocation/location_arbitrator_impl_unittest.cc:83: // GetSystemLocationProviderOverride(). The caller takes ownership. What is GetSystemLocationProviderOverride()? https://codereview.chromium.org/2028823002/diff/280001/content/public/browser... File content/public/browser/content_browser_client.h (right): https://codereview.chromium.org/2028823002/diff/280001/content/public/browser... content/public/browser/content_browser_client.h:613: // Return nullptr to use the default one for the platform to be created. This comment should really document that the caller takes ownership of the returned LocationProvider, since that's not clear from the API name. https://codereview.chromium.org/2028823002/diff/280001/content/public/browser... content/public/browser/content_browser_client.h:620: // location approximation in addition to the sytem provider, if any. typo: sytem
https://codereview.chromium.org/2028823002/diff/240001/blimp/engine/app/blimp... File blimp/engine/app/blimp_content_browser_client.h (right): https://codereview.chromium.org/2028823002/diff/240001/blimp/engine/app/blimp... blimp/engine/app/blimp_content_browser_client.h:9: #include "blimp/engine/feature/geolocation/blimp_location_provider.h" On 2016/06/14 16:46:42, mvanouwerkerk - OOO to June 20 wrote: > On 2016/06/10 19:51:34, CJ wrote: > > On 2016/06/10 12:46:52, Michael van Ouwerkerk wrote: > > > nit: you could forward declare BlimpLocationProvider and include this in the > > cc > > > file. > > > > I thought convention was to have the include in the same file in which its > > contents are referenced? > > You're right, it is. However, it's also convention to use forward declarations > in headers, as is done here for BlimpBrowserMainParts and BlimpBrowserContext. I > mean, BlimpLocationProvider is not really used in this file is it, just referred > to? Ah, so if it is just referred to, as in with a member variable, then we just include it in the cc file? Done. https://codereview.chromium.org/2028823002/diff/240001/content/browser/geoloc... File content/browser/geolocation/location_arbitrator_impl.h (right): https://codereview.chromium.org/2028823002/diff/240001/content/browser/geoloc... content/browser/geolocation/location_arbitrator_impl.h:95: base::CancelableCallback<void(AccessTokenStore::AccessTokenMap, On 2016/06/14 16:46:42, Michael van Ouwerkerk wrote: > On 2016/06/10 20:39:11, Kevin M wrote: > > On 2016/06/10 19:51:34, CJ wrote: > > > On 2016/06/10 12:46:52, Michael van Ouwerkerk wrote: > > > > Why is this cancelable but never cancelled, and why is it stored in the > > > > arbitrator? It seems neither aspect is needed. > > > > > > This was done so the reset method could be used. The arbitrator was doing > > checks > > > in the OnAccessTokenStoresLoaded method that were basically doing the > > > functionality of Reset(), but the checks caused the need for some weird > > control > > > flow. > > > > To be a little more specific - it's cancelable so that > > OnAccessTokenStoresLoaded() only gets called once. The previous implementation > > of OnAccessTokenStoresLoaded did something similar in a more roundabout way by > > checking whether |providers_| is empty. > > It would be good to add a little documentation at the usage site to explain why > this is needed (multiple calls). Done.
kmarshall@chromium.org changed reviewers: - haibinlu@chromium.org
https://codereview.chromium.org/2028823002/diff/260001/content/browser/geoloc... File content/browser/geolocation/mock_location_provider.cc (right): https://codereview.chromium.org/2028823002/diff/260001/content/browser/geoloc... content/browser/geolocation/mock_location_provider.cc:97: base::Unretained(this), position_)); On 2016/06/17 05:50:09, Wez wrote: > On 2016/06/13 23:45:20, CJ wrote: > > On 2016/06/10 23:40:35, Wez wrote: > > > What's the rationale for this change? Is the new requirement that the caller > > > must ensure that the provider remains alive after calling StartProvider or > > > OnPermissionGranted, until HandlePositionChanged, documented somewhere? > > > > I am truthfully not sure. This is something that kmarshall@ pointed out to be > > changed. I will talk to him about it. > > Acknowledged. Talked to him. He said since it is test code and ownership/deletion order is known, there's no reason to have that level of protection. https://codereview.chromium.org/2028823002/diff/280001/content/browser/geoloc... File content/browser/geolocation/location_arbitrator_impl.cc (right): https://codereview.chromium.org/2028823002/diff/280001/content/browser/geoloc... content/browser/geolocation/location_arbitrator_impl.cc:79: // If no providers are available, we report an error to avoid On 2016/06/16 21:56:54, Charlie Reis wrote: > nit: Only one space after comma. Done. https://codereview.chromium.org/2028823002/diff/280001/content/browser/geoloc... File content/browser/geolocation/location_arbitrator_impl.h (right): https://codereview.chromium.org/2028823002/diff/280001/content/browser/geoloc... content/browser/geolocation/location_arbitrator_impl.h:96: token_store_callback_; On 2016/06/16 21:56:54, Charlie Reis wrote: > On 2016/06/14 16:46:42, mvanouwerkerk - OOO to June 20 wrote: > > It would be good to add a little documentation at the usage site to explain > why > > this is needed (multiple calls). > > Just a reminder for this request. Done. https://codereview.chromium.org/2028823002/diff/280001/content/browser/geoloc... File content/browser/geolocation/location_arbitrator_impl_unittest.cc (right): https://codereview.chromium.org/2028823002/diff/280001/content/browser/geoloc... content/browser/geolocation/location_arbitrator_impl_unittest.cc:83: // GetSystemLocationProviderOverride(). The caller takes ownership. On 2016/06/17 05:50:09, Wez wrote: > What is GetSystemLocationProviderOverride()? A function found in GeolocationLocationArbitratorTest. It follows the getter pattern for the rest of the fake providers (MockLocationProvider objects). Should I update this comment to include the class name, or is more information needed? https://codereview.chromium.org/2028823002/diff/280001/content/public/browser... File content/public/browser/content_browser_client.h (right): https://codereview.chromium.org/2028823002/diff/280001/content/public/browser... content/public/browser/content_browser_client.h:613: // Return nullptr to use the default one for the platform to be created. On 2016/06/17 05:50:09, Wez wrote: > This comment should really document that the caller takes ownership of the > returned LocationProvider, since that's not clear from the API name. Done. https://codereview.chromium.org/2028823002/diff/280001/content/public/browser... content/public/browser/content_browser_client.h:620: // location approximation in addition to the sytem provider, if any. On 2016/06/17 05:50:09, Wez wrote: > typo: sytem Done.
LGTM w/ nits. https://codereview.chromium.org/2028823002/diff/280001/content/browser/geoloc... File content/browser/geolocation/location_arbitrator_impl_unittest.cc (right): https://codereview.chromium.org/2028823002/diff/280001/content/browser/geoloc... content/browser/geolocation/location_arbitrator_impl_unittest.cc:83: // GetSystemLocationProviderOverride(). The caller takes ownership. On 2016/06/21 22:16:07, CJ wrote: > On 2016/06/17 05:50:09, Wez wrote: > > What is GetSystemLocationProviderOverride()? > > A function found in GeolocationLocationArbitratorTest. It follows the getter > pattern for the rest of the fake providers (MockLocationProvider objects). > Should I update this comment to include the class name, or is more information > needed? Yes, since this function doesn't exist in this file, I'd fully-qualify the name (i.e. prefix with ClassName::) https://codereview.chromium.org/2028823002/diff/320001/blimp/engine/app/blimp... File blimp/engine/app/blimp_content_browser_client.cc (right): https://codereview.chromium.org/2028823002/diff/320001/blimp/engine/app/blimp... blimp/engine/app/blimp_content_browser_client.cc:8: #include "blimp/engine/feature/geolocation/blimp_location_provider.h"#include "blimp/engine/feature/geolocation/blimp_location_provider.h"#include "blimp/engine/feature/geolocation/blimp_location_provider.h" This looks messed-up?
https://codereview.chromium.org/2028823002/diff/280001/content/browser/geoloc... File content/browser/geolocation/location_arbitrator_impl_unittest.cc (right): https://codereview.chromium.org/2028823002/diff/280001/content/browser/geoloc... content/browser/geolocation/location_arbitrator_impl_unittest.cc:83: // GetSystemLocationProviderOverride(). The caller takes ownership. On 2016/06/22 18:51:01, Wez wrote: > On 2016/06/21 22:16:07, CJ wrote: > > On 2016/06/17 05:50:09, Wez wrote: > > > What is GetSystemLocationProviderOverride()? > > > > A function found in GeolocationLocationArbitratorTest. It follows the getter > > pattern for the rest of the fake providers (MockLocationProvider objects). > > Should I update this comment to include the class name, or is more information > > needed? > > Yes, since this function doesn't exist in this file, I'd fully-qualify the name > (i.e. prefix with ClassName::) But it does? Line 178. Adding the class name to the comment. https://codereview.chromium.org/2028823002/diff/320001/blimp/engine/app/blimp... File blimp/engine/app/blimp_content_browser_client.cc (right): https://codereview.chromium.org/2028823002/diff/320001/blimp/engine/app/blimp... blimp/engine/app/blimp_content_browser_client.cc:8: #include "blimp/engine/feature/geolocation/blimp_location_provider.h"#include "blimp/engine/feature/geolocation/blimp_location_provider.h"#include "blimp/engine/feature/geolocation/blimp_location_provider.h" On 2016/06/22 18:51:02, Wez wrote: > This looks messed-up? Oh my. Not sure what happened there. Fixed.
The CQ bit was checked by lethalantidote@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kmarshall@chromium.org, creis@chromium.org, mvanouwerkerk@chromium.org, wez@chromium.org Link to the patchset: https://codereview.chromium.org/2028823002/#ps340001 (title: "Addresses Wez's #101 comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2028823002/340001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-device-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by lethalantidote@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org, kmarshall@chromium.org, wez@chromium.org, mvanouwerkerk@chromium.org Link to the patchset: https://codereview.chromium.org/2028823002/#ps360001 (title: "Merge with Master")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2028823002/360001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2028823002/diff/240001/blimp/engine/app/blimp... File blimp/engine/app/blimp_content_browser_client.h (right): https://codereview.chromium.org/2028823002/diff/240001/blimp/engine/app/blimp... blimp/engine/app/blimp_content_browser_client.h:9: #include "blimp/engine/feature/geolocation/blimp_location_provider.h" On 2016/06/21 21:27:41, CJ wrote: > On 2016/06/14 16:46:42, mvanouwerkerk - OOO to June 20 wrote: > > On 2016/06/10 19:51:34, CJ wrote: > > > On 2016/06/10 12:46:52, Michael van Ouwerkerk wrote: > > > > nit: you could forward declare BlimpLocationProvider and include this in > the > > > cc > > > > file. > > > > > > I thought convention was to have the include in the same file in which its > > > contents are referenced? > > > > You're right, it is. However, it's also convention to use forward declarations > > in headers, as is done here for BlimpBrowserMainParts and BlimpBrowserContext. > I > > mean, BlimpLocationProvider is not really used in this file is it, just > referred > > to? > > Ah, so if it is just referred to, as in with a member variable, then we just > include it in the cc file? > > Done. This change did not pass try. Reverting back to the original import in the header.
The CQ bit was checked by lethalantidote@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org, kmarshall@chromium.org, wez@chromium.org, mvanouwerkerk@chromium.org Link to the patchset: https://codereview.chromium.org/2028823002/#ps380001 (title: "Fixes try issue")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2028823002/380001
Message was sent while issue was closed.
Description was changed from ========== Refactor to make BlimpLocationProvider accessible to content layer. This change adds to ContentBrowserClient the ability to indicate whether one should use NetworkLocationProvider objects when accessing location data. The LocationArbitratorImpl now checks this boolean to determine what LocationProviders to create and register, allowing for system/override location providers to be used without the network location providers. BlimpContentBrowserClient is updated to use these changes to indicate that NetworkLocationProvider objects should not be used. It also provides a BlimpLocationProvider through OverrideSystemLocationProvider. Since UseNetworkLocationProviders returns false, and we supply the override, Blimp will only use the BlimpLocationProvider for querying location. Other additions: * Moved non-NetworkLocationProvider creation out into a separate function to allow for optional instantiation of NetworkLocationProvider objects. * Moved call to UseNetworkLocationProviders() so that its result is now passed to LocaitonArbitratorImpl through to GeolocationProviderImpl. This was done to give a better way to test the codepath for the case that UseNetworkLocationProviders returns false. BUG=614486 ========== to ========== Refactor to make BlimpLocationProvider accessible to content layer. This change adds to ContentBrowserClient the ability to indicate whether one should use NetworkLocationProvider objects when accessing location data. The LocationArbitratorImpl now checks this boolean to determine what LocationProviders to create and register, allowing for system/override location providers to be used without the network location providers. BlimpContentBrowserClient is updated to use these changes to indicate that NetworkLocationProvider objects should not be used. It also provides a BlimpLocationProvider through OverrideSystemLocationProvider. Since UseNetworkLocationProviders returns false, and we supply the override, Blimp will only use the BlimpLocationProvider for querying location. Other additions: * Moved non-NetworkLocationProvider creation out into a separate function to allow for optional instantiation of NetworkLocationProvider objects. * Moved call to UseNetworkLocationProviders() so that its result is now passed to LocaitonArbitratorImpl through to GeolocationProviderImpl. This was done to give a better way to test the codepath for the case that UseNetworkLocationProviders returns false. BUG=614486 ==========
Message was sent while issue was closed.
Committed patchset #20 (id:380001)
Message was sent while issue was closed.
Description was changed from ========== Refactor to make BlimpLocationProvider accessible to content layer. This change adds to ContentBrowserClient the ability to indicate whether one should use NetworkLocationProvider objects when accessing location data. The LocationArbitratorImpl now checks this boolean to determine what LocationProviders to create and register, allowing for system/override location providers to be used without the network location providers. BlimpContentBrowserClient is updated to use these changes to indicate that NetworkLocationProvider objects should not be used. It also provides a BlimpLocationProvider through OverrideSystemLocationProvider. Since UseNetworkLocationProviders returns false, and we supply the override, Blimp will only use the BlimpLocationProvider for querying location. Other additions: * Moved non-NetworkLocationProvider creation out into a separate function to allow for optional instantiation of NetworkLocationProvider objects. * Moved call to UseNetworkLocationProviders() so that its result is now passed to LocaitonArbitratorImpl through to GeolocationProviderImpl. This was done to give a better way to test the codepath for the case that UseNetworkLocationProviders returns false. BUG=614486 ========== to ========== Refactor to make BlimpLocationProvider accessible to content layer. This change adds to ContentBrowserClient the ability to indicate whether one should use NetworkLocationProvider objects when accessing location data. The LocationArbitratorImpl now checks this boolean to determine what LocationProviders to create and register, allowing for system/override location providers to be used without the network location providers. BlimpContentBrowserClient is updated to use these changes to indicate that NetworkLocationProvider objects should not be used. It also provides a BlimpLocationProvider through OverrideSystemLocationProvider. Since UseNetworkLocationProviders returns false, and we supply the override, Blimp will only use the BlimpLocationProvider for querying location. Other additions: * Moved non-NetworkLocationProvider creation out into a separate function to allow for optional instantiation of NetworkLocationProvider objects. * Moved call to UseNetworkLocationProviders() so that its result is now passed to LocaitonArbitratorImpl through to GeolocationProviderImpl. This was done to give a better way to test the codepath for the case that UseNetworkLocationProviders returns false. BUG=614486 Committed: https://crrev.com/564907f6ecd1040e0074aeeadf7da5e2ec7fdde6 Cr-Commit-Position: refs/heads/master@{#401453} ==========
Message was sent while issue was closed.
Patchset 20 (id:??) landed as https://crrev.com/564907f6ecd1040e0074aeeadf7da5e2ec7fdde6 Cr-Commit-Position: refs/heads/master@{#401453} |
