Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2975683002:
Add GeolocationManager service (Closed)
|
Created:
3 years, 5 months ago by asimjour1 Modified:
3 years, 5 months ago CC:
Aaron Boodman, abarth-chromium, chromium-reviews, darin (slow to review), darin-cc_chromium.org, feature-vr-reviews_chromium.org, jam, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd GeolocationConfig interface
GeolocationConfig is designed to be used from the chrome/browser.
For now the only method is IsHighAccuracyLocationBeingCaptured, that
indicates if the location is captured with high_accuracy=enabled flag.
BUG=731758
Review-Url: https://codereview.chromium.org/2975683002
Cr-Commit-Position: refs/heads/master@{#486123}
Committed: https://chromium.googlesource.com/chromium/src/+/839ce1be4b4cdc2a0f92bd0da4fc1fe3d10f67e0
Patch Set 1 #Patch Set 2 : Clean up #Patch Set 3 : Fix Windows dll-interface problem #
Total comments: 8
Patch Set 4 : Add GeolocationManager service #
Total comments: 9
Patch Set 5 : Rename GeolocationManager to GeolocationConfig #Patch Set 6 : Rename GeolocationManager to GeolocationConfig #Patch Set 7 : Rebase #
Total comments: 10
Patch Set 8 : Address comments #
Total comments: 4
Patch Set 9 : typo #Messages
Total messages: 56 (33 generated)
The CQ bit was checked by asimjour@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by asimjour@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by asimjour@chromium.org
Description was changed from ========== Add GeolocationManager service GeolocationManager is designed to be used from the chrome/browser. For now the only method is IsHighAccuracyLocationBeingCaptured, that indicates if location is captured with high_accuracy=enabled flag. BUG=731758 ========== to ========== Add GeolocationManager service GeolocationManager is designed to be used from the chrome/browser. For now the only method is IsHighAccuracyLocationBeingCaptured, that indicates if location is captured with high_accuracy=enabled flag. BUG=731758 ==========
asimjour@chromium.org changed reviewers: + avi@chromium.org, blundell@chromium.org, vollick@chromium.org
asimjour@chromium.org changed reviewers: + rockot@chromium.org
rockot@chromium.org: Please review changes in
content lgtm
On 2017/07/10 19:18:35, Avi (ping after 24h) wrote: > content lgtm vr_shell lgtm
https://codereview.chromium.org/2975683002/diff/40001/content/public/app/mojo... File content/public/app/mojo/content_browser_manifest.json (right): https://codereview.chromium.org/2975683002/diff/40001/content/public/app/mojo... content/public/app/mojo/content_browser_manifest.json:49: "geolocation_manager": [ geolocation_config? https://codereview.chromium.org/2975683002/diff/40001/device/geolocation/publ... File device/geolocation/public/interfaces/geolocation_manager.mojom (right): https://codereview.chromium.org/2975683002/diff/40001/device/geolocation/publ... device/geolocation/public/interfaces/geolocation_manager.mojom:7: // The Geolocation manager provides a service for trusted consumers to get the nit: please don't use the word "service" in an IPC or mojom context unless referring to actual services. Please also do not refer to "frames" here. https://codereview.chromium.org/2975683002/diff/40001/device/geolocation/publ... device/geolocation/public/interfaces/geolocation_manager.mojom:9: interface GeolocationManager { Maybe GeolocationConfig is a more accurate name?
amp@chromium.org changed reviewers: + amp@chromium.org
Drive by comments/questions. Manager seems like the wrong name for this. There is already a geolocation provider, which it looks like it had to be modified for the new method anyway. Why not continue that naming format? Also per my inline comment about services below, it looks like there is already an existing geolocation service which would be better to augment rather then create a new one (but I'm not familiar with geolocation and it's uses at all so maybe I'm way off base). Lastly, perhaps split the geolocation and the vr changes into two changes, it feels like a lot is happening here that would be easier to review if it was split up. https://codereview.chromium.org/2975683002/diff/40001/device/geolocation/publ... File device/geolocation/public/interfaces/geolocation_manager.mojom (right): https://codereview.chromium.org/2975683002/diff/40001/device/geolocation/publ... device/geolocation/public/interfaces/geolocation_manager.mojom:12: IsHighAccuracyLocationBeingCaptured() => (bool high_accuracy); Why not add this to the existing geolocation.mojom that has the setter for high accuracy?
https://codereview.chromium.org/2975683002/diff/40001/content/public/app/mojo... File content/public/app/mojo/content_browser_manifest.json (right): https://codereview.chromium.org/2975683002/diff/40001/content/public/app/mojo... content/public/app/mojo/content_browser_manifest.json:49: "geolocation_manager": [ On 2017/07/10 19:38:55, Ken Rockot(use gerrit already) wrote: > geolocation_config? Done. https://codereview.chromium.org/2975683002/diff/40001/device/geolocation/publ... File device/geolocation/public/interfaces/geolocation_manager.mojom (right): https://codereview.chromium.org/2975683002/diff/40001/device/geolocation/publ... device/geolocation/public/interfaces/geolocation_manager.mojom:7: // The Geolocation manager provides a service for trusted consumers to get the On 2017/07/10 19:38:55, Ken Rockot(use gerrit already) wrote: > nit: please don't use the word "service" in an IPC or mojom context unless > referring to actual services. Please also do not refer to "frames" here. Done. https://codereview.chromium.org/2975683002/diff/40001/device/geolocation/publ... device/geolocation/public/interfaces/geolocation_manager.mojom:9: interface GeolocationManager { On 2017/07/10 19:38:55, Ken Rockot(use gerrit already) wrote: > Maybe GeolocationConfig is a more accurate name? The name came from https://bugs.chromium.org/p/chromium/issues/detail?id=725053 and offline discussion with blundell@. GeolocationConfig works for me, but I'll wait for blundell@ to confirm. https://codereview.chromium.org/2975683002/diff/40001/device/geolocation/publ... device/geolocation/public/interfaces/geolocation_manager.mojom:12: IsHighAccuracyLocationBeingCaptured() => (bool high_accuracy); On 2017/07/10 20:13:25, amp wrote: > Why not add this to the existing geolocation.mojom that has the setter for high > accuracy? Geolocation.mojom is per-frame interface and it is not designed to be used from chrome/browser.
https://codereview.chromium.org/2975683002/diff/40001/content/public/app/mojo... File content/public/app/mojo/content_browser_manifest.json (right): https://codereview.chromium.org/2975683002/diff/40001/content/public/app/mojo... content/public/app/mojo/content_browser_manifest.json:49: "geolocation_manager": [ On 2017/07/10 19:38:55, Ken Rockot(use gerrit already) wrote: > geolocation_config? Done. https://codereview.chromium.org/2975683002/diff/40001/device/geolocation/publ... File device/geolocation/public/interfaces/geolocation_manager.mojom (right): https://codereview.chromium.org/2975683002/diff/40001/device/geolocation/publ... device/geolocation/public/interfaces/geolocation_manager.mojom:7: // The Geolocation manager provides a service for trusted consumers to get the On 2017/07/10 19:38:55, Ken Rockot(use gerrit already) wrote: > nit: please don't use the word "service" in an IPC or mojom context unless > referring to actual services. Please also do not refer to "frames" here. Done. https://codereview.chromium.org/2975683002/diff/40001/device/geolocation/publ... device/geolocation/public/interfaces/geolocation_manager.mojom:9: interface GeolocationManager { On 2017/07/10 19:38:55, Ken Rockot(use gerrit already) wrote: > Maybe GeolocationConfig is a more accurate name? The name came from https://bugs.chromium.org/p/chromium/issues/detail?id=725053 and offline discussion with blundell@. GeolocationConfig works for me, but I'll wait for blundell@ to confirm. https://codereview.chromium.org/2975683002/diff/40001/device/geolocation/publ... device/geolocation/public/interfaces/geolocation_manager.mojom:12: IsHighAccuracyLocationBeingCaptured() => (bool high_accuracy); On 2017/07/10 20:13:25, amp wrote: > Why not add this to the existing geolocation.mojom that has the setter for high > accuracy? Geolocation.mojom is per-frame interface and it is not designed to be used from chrome/browser.
asimjour@chromium.org changed reviewers: + estark@chromium.org
On 2017/07/10 20:45:50, asimjour1 wrote: > https://codereview.chromium.org/2975683002/diff/40001/content/public/app/mojo... > File content/public/app/mojo/content_browser_manifest.json (right): > > https://codereview.chromium.org/2975683002/diff/40001/content/public/app/mojo... > content/public/app/mojo/content_browser_manifest.json:49: "geolocation_manager": > [ > On 2017/07/10 19:38:55, Ken Rockot(use gerrit already) wrote: > > geolocation_config? > > Done. > > https://codereview.chromium.org/2975683002/diff/40001/device/geolocation/publ... > File device/geolocation/public/interfaces/geolocation_manager.mojom (right): > > https://codereview.chromium.org/2975683002/diff/40001/device/geolocation/publ... > device/geolocation/public/interfaces/geolocation_manager.mojom:7: // The > Geolocation manager provides a service for trusted consumers to get the > On 2017/07/10 19:38:55, Ken Rockot(use gerrit already) wrote: > > nit: please don't use the word "service" in an IPC or mojom context unless > > referring to actual services. Please also do not refer to "frames" here. > > Done. > > https://codereview.chromium.org/2975683002/diff/40001/device/geolocation/publ... > device/geolocation/public/interfaces/geolocation_manager.mojom:9: interface > GeolocationManager { > On 2017/07/10 19:38:55, Ken Rockot(use gerrit already) wrote: > > Maybe GeolocationConfig is a more accurate name? > > The name came from > https://bugs.chromium.org/p/chromium/issues/detail?id=725053 > and offline discussion with blundell@. GeolocationConfig works for me, but I'll > wait for blundell@ to confirm. > > https://codereview.chromium.org/2975683002/diff/40001/device/geolocation/publ... > device/geolocation/public/interfaces/geolocation_manager.mojom:12: > IsHighAccuracyLocationBeingCaptured() => (bool high_accuracy); > On 2017/07/10 20:13:25, amp wrote: > > Why not add this to the existing geolocation.mojom that has the setter for > high > > accuracy? > > Geolocation.mojom is per-frame interface and it is not designed to be used from > chrome/browser. +estark@ PTAL content/public/app/mojo/content_browser_manifest.json and device/geolocation/public/interfaces/geolocation_manager.mojom
The CQ bit was checked by asimjour@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
amp@: Good questions. asimjour1@ and I had previously had a discussion about these points, but it didn't get captured anywhere. On 2017/07/10 20:13:25, amp wrote: > Drive by comments/questions. > > Manager seems like the wrong name for this. There is already a geolocation > provider, which it looks like it had to be modified for the new method anyway. > Why not continue that naming format? We're currently still working to come up with consistent naming patterns for different "types of things" in the services world. We're using "Provider" in some cases to mean "vender of another interface", so I don't want to overload that here (also I don't think "Provider" really conveys what this interface is doing). That said, I agree with you and I like Ken's suggestion of GeolocationConfig. The other method we'll eventually add to this interface is a method to set a global GeolocationClient that the service impl uses, so GeolocationConfig will be appropriate going forward as well. > > Also per my inline comment about services below, it looks like there is already > an existing geolocation service which would be better to augment rather then > create a new one (but I'm not familiar with geolocation and it's uses at all so > maybe I'm way off base). > asimjour1@ replied to this, but just to reiterate: yes, the use cases are very different. GeolocationService is tailored to the web platform use case: an untrusted client looking to get geolocation information within the context of a specific frame. This new interface is for use by trusted clients and gets (+ eventually sets) information that is global across the impl of geolocation. Note however that both these interfaces will eventually be hosted by the Device Service, so they will be in the context of "one service", just different interfaces exposed by that service.
https://codereview.chromium.org/2975683002/diff/60001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_shell.h (right): https://codereview.chromium.org/2975683002/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell.h:237: device::mojom::GeolocationManagerPtr geolocation_manager_ptr; naming nit: geolocation_manager_ https://codereview.chromium.org/2975683002/diff/60001/content/browser/service... File content/browser/service_manager/common_browser_interfaces.cc (right): https://codereview.chromium.org/2975683002/diff/60001/content/browser/service... content/browser/service_manager/common_browser_interfaces.cc:86: registry_.AddInterface(base::Bind(&device::GeolocationManager::Create)); I think what you want to do is add a *call* to RegisterMainThreadInterface() up by line 38 in this file (where there's the call for MemoryCoordinator). That said, this file seems like a weird place to put this as we're only exposing this interface to the browser process. Ken, is there a better place to put this? Note that wherever we put it is temporary anyway as it will be hosted by the Device Service once we servicify geolocation, so maybe it doesn't matter too much. https://codereview.chromium.org/2975683002/diff/60001/device/geolocation/publ... File device/geolocation/public/interfaces/geolocation_manager.mojom (right): https://codereview.chromium.org/2975683002/diff/60001/device/geolocation/publ... device/geolocation/public/interfaces/geolocation_manager.mojom:8: // information across all the frames. nit: // Provides an interface for trusted consumers to get privileged information. https://codereview.chromium.org/2975683002/diff/60001/device/geolocation/publ... device/geolocation/public/interfaces/geolocation_manager.mojom:9: interface GeolocationManager { I suggested the name GeolocationManager to you, but I do think that Ken's suggestion of GeolocationConfig is actually better. Apologies for the churn!
The CQ bit was checked by asimjour@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2975683002/diff/60001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_shell.h (right): https://codereview.chromium.org/2975683002/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell.h:237: device::mojom::GeolocationManagerPtr geolocation_manager_ptr; On 2017/07/11 09:08:33, blundell wrote: > naming nit: geolocation_manager_ Done. https://codereview.chromium.org/2975683002/diff/60001/content/browser/service... File content/browser/service_manager/common_browser_interfaces.cc (right): https://codereview.chromium.org/2975683002/diff/60001/content/browser/service... content/browser/service_manager/common_browser_interfaces.cc:86: registry_.AddInterface(base::Bind(&device::GeolocationManager::Create)); On 2017/07/11 09:08:34, blundell wrote: > I think what you want to do is add a *call* to RegisterMainThreadInterface() up > by line 38 in this file (where there's the call for MemoryCoordinator). That > said, this file seems like a weird place to put this as we're only exposing this > interface to the browser process. Ken, is there a better place to put this? Note > that wherever we put it is temporary anyway as it will be hosted by the Device > Service once we servicify geolocation, so maybe it doesn't matter too much. replaced with a call to RegisterMainThreadInterface https://codereview.chromium.org/2975683002/diff/60001/device/geolocation/publ... File device/geolocation/public/interfaces/geolocation_manager.mojom (right): https://codereview.chromium.org/2975683002/diff/60001/device/geolocation/publ... device/geolocation/public/interfaces/geolocation_manager.mojom:8: // information across all the frames. On 2017/07/11 09:08:34, blundell wrote: > nit: // Provides an interface for trusted consumers to get privileged > information. Done. https://codereview.chromium.org/2975683002/diff/60001/device/geolocation/publ... device/geolocation/public/interfaces/geolocation_manager.mojom:9: interface GeolocationManager { On 2017/07/11 09:08:34, blundell wrote: > I suggested the name GeolocationManager to you, but I do think that Ken's > suggestion of GeolocationConfig is actually better. Apologies for the churn! Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
lgtm https://codereview.chromium.org/2975683002/diff/60001/content/browser/service... File content/browser/service_manager/common_browser_interfaces.cc (right): https://codereview.chromium.org/2975683002/diff/60001/content/browser/service... content/browser/service_manager/common_browser_interfaces.cc:86: registry_.AddInterface(base::Bind(&device::GeolocationManager::Create)); On 2017/07/11 at 15:04:12, asimjour1 wrote: > On 2017/07/11 09:08:34, blundell wrote: > > I think what you want to do is add a *call* to RegisterMainThreadInterface() up > > by line 38 in this file (where there's the call for MemoryCoordinator). That > > said, this file seems like a weird place to put this as we're only exposing this > > interface to the browser process. Ken, is there a better place to put this? Note > > that wherever we put it is temporary anyway as it will be hosted by the Device > > Service once we servicify geolocation, so maybe it doesn't matter too much. > > replaced with a call to RegisterMainThreadInterface It's a bit weird but it seems OK to me given that it's temporary.
The CQ bit was checked by asimjour@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_headless_rel on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?))
lgtm, thanks! Could you please update the CL description to talk about GeolocationConfig instead of GeolocationManager and call it a Mojo interface rather than a service? Also add that this interface is being added as geolocation will shortly be moved to the Device Service, at which point //chrome wouldn't be able to access it via C++ anyway. https://codereview.chromium.org/2975683002/diff/120001/chrome/browser/android... File chrome/browser/android/vr_shell/vr_shell.cc (right): https://codereview.chromium.org/2975683002/diff/120001/chrome/browser/android... chrome/browser/android/vr_shell/vr_shell.cc:655: &VrShell::SetHighAccuracyLocation, weak_ptr_factory_.GetWeakPtr())); this can be base::Unretained because this object owns |geolocation_config_| and thus manages its lifetime. https://codereview.chromium.org/2975683002/diff/120001/chrome/browser/android... chrome/browser/android/vr_shell/vr_shell.cc:680: if (high_accuracy_location != high_accuracy_location_) nit: more clear as if (high_accuracy_location == high_accuracy_location_) return; // lines 681 and 682 https://codereview.chromium.org/2975683002/diff/120001/device/geolocation/geo... File device/geolocation/geolocation_config.cc (right): https://codereview.chromium.org/2975683002/diff/120001/device/geolocation/geo... device/geolocation/geolocation_config.cc:27: GeolocationProvider::GetInstance()->HighAccuracyLocationInUse(); nit: just inline this call into the invocation of the callback https://codereview.chromium.org/2975683002/diff/120001/device/geolocation/geo... device/geolocation/geolocation_config.cc:29: return; nit: eliminate the return
https://codereview.chromium.org/2975683002/diff/120001/device/geolocation/pub... File device/geolocation/public/interfaces/geolocation_config.mojom (right): https://codereview.chromium.org/2975683002/diff/120001/device/geolocation/pub... device/geolocation/public/interfaces/geolocation_config.mojom:7: // Provides an interface for trusted consumers to get privileged information. nit: this description seems a little vague. Does "trusted consumers" mean the browser process? Is "privileged information" geolocation data?
https://codereview.chromium.org/2975683002/diff/120001/chrome/browser/android... File chrome/browser/android/vr_shell/vr_shell.cc (right): https://codereview.chromium.org/2975683002/diff/120001/chrome/browser/android... chrome/browser/android/vr_shell/vr_shell.cc:655: &VrShell::SetHighAccuracyLocation, weak_ptr_factory_.GetWeakPtr())); On 2017/07/12 13:28:06, blundell wrote: > this can be base::Unretained because this object owns |geolocation_config_| and > thus manages its lifetime. Done. https://codereview.chromium.org/2975683002/diff/120001/chrome/browser/android... chrome/browser/android/vr_shell/vr_shell.cc:680: if (high_accuracy_location != high_accuracy_location_) On 2017/07/12 13:28:06, blundell wrote: > nit: > > more clear as > > if (high_accuracy_location == high_accuracy_location_) > return; > // lines 681 and 682 Done. https://codereview.chromium.org/2975683002/diff/120001/device/geolocation/geo... File device/geolocation/geolocation_config.cc (right): https://codereview.chromium.org/2975683002/diff/120001/device/geolocation/geo... device/geolocation/geolocation_config.cc:27: GeolocationProvider::GetInstance()->HighAccuracyLocationInUse(); On 2017/07/12 13:28:06, blundell wrote: > nit: just inline this call into the invocation of the callback Done. https://codereview.chromium.org/2975683002/diff/120001/device/geolocation/geo... device/geolocation/geolocation_config.cc:29: return; On 2017/07/12 13:28:06, blundell wrote: > nit: eliminate the return Done. https://codereview.chromium.org/2975683002/diff/120001/device/geolocation/pub... File device/geolocation/public/interfaces/geolocation_config.mojom (right): https://codereview.chromium.org/2975683002/diff/120001/device/geolocation/pub... device/geolocation/public/interfaces/geolocation_config.mojom:7: // Provides an interface for trusted consumers to get privileged information. On 2017/07/12 15:48:15, estark wrote: > nit: this description seems a little vague. Does "trusted consumers" mean the > browser process? Is "privileged information" geolocation data? Updated the comment to only contain the interface that this CL has introduced.
mojom lgtm https://codereview.chromium.org/2975683002/diff/140001/device/geolocation/pub... File device/geolocation/public/interfaces/geolocation_config.mojom (right): https://codereview.chromium.org/2975683002/diff/140001/device/geolocation/pub... device/geolocation/public/interfaces/geolocation_config.mojom:10: // Returns true if the location is being captured and high_accurary is typo: high_accuracy
https://codereview.chromium.org/2975683002/diff/140001/device/geolocation/pub... File device/geolocation/public/interfaces/geolocation_config.mojom (right): https://codereview.chromium.org/2975683002/diff/140001/device/geolocation/pub... device/geolocation/public/interfaces/geolocation_config.mojom:7: // Provides an interface for browser process to access geolocation capturing I think it's wrong to refer to "browser process" in a device service mojom, but perhaps this is just another symptom of the more general problem of security review being more difficult in a mojom world. Fine for now I guess.
Description was changed from ========== Add GeolocationManager service GeolocationManager is designed to be used from the chrome/browser. For now the only method is IsHighAccuracyLocationBeingCaptured, that indicates if location is captured with high_accuracy=enabled flag. BUG=731758 ========== to ========== Add GeolocationConfig interface GeolocationConfig is designed to be used from the chrome/browser. For now the only method is IsHighAccuracyLocationBeingCaptured, that indicates if the location is captured with high_accuracy=enabled flag. BUG=731758 ==========
The CQ bit was checked by asimjour@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2975683002/diff/140001/device/geolocation/pub... File device/geolocation/public/interfaces/geolocation_config.mojom (right): https://codereview.chromium.org/2975683002/diff/140001/device/geolocation/pub... device/geolocation/public/interfaces/geolocation_config.mojom:10: // Returns true if the location is being captured and high_accurary is On 2017/07/12 18:11:51, estark wrote: > typo: high_accuracy Done.
The CQ bit was checked by asimjour@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vollick@chromium.org, avi@chromium.org, rockot@chromium.org, blundell@chromium.org, estark@chromium.org Link to the patchset: https://codereview.chromium.org/2975683002/#ps160001 (title: "typo")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1499896724466660,
"parent_rev": "ce350d8fa68ece595cd676f492e89c40582fde7d", "commit_rev":
"839ce1be4b4cdc2a0f92bd0da4fc1fe3d10f67e0"}
Message was sent while issue was closed.
Description was changed from ========== Add GeolocationConfig interface GeolocationConfig is designed to be used from the chrome/browser. For now the only method is IsHighAccuracyLocationBeingCaptured, that indicates if the location is captured with high_accuracy=enabled flag. BUG=731758 ========== to ========== Add GeolocationConfig interface GeolocationConfig is designed to be used from the chrome/browser. For now the only method is IsHighAccuracyLocationBeingCaptured, that indicates if the location is captured with high_accuracy=enabled flag. BUG=731758 Review-Url: https://codereview.chromium.org/2975683002 Cr-Commit-Position: refs/heads/master@{#486123} Committed: https://chromium.googlesource.com/chromium/src/+/839ce1be4b4cdc2a0f92bd0da4fc... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/chromium/src/+/839ce1be4b4cdc2a0f92bd0da4fc...
Message was sent while issue was closed.
https://codereview.chromium.org/2975683002/diff/140001/device/geolocation/pub... File device/geolocation/public/interfaces/geolocation_config.mojom (right): https://codereview.chromium.org/2975683002/diff/140001/device/geolocation/pub... device/geolocation/public/interfaces/geolocation_config.mojom:7: // Provides an interface for browser process to access geolocation capturing On 2017/07/12 18:19:32, Ken Rockot(use gerrit already) wrote: > I think it's wrong to refer to "browser process" in a device service mojom, but > perhaps this is just another symptom of the more general problem of security > review being more difficult in a mojom world. Fine for now I guess. I agree with Ken. The Device Service is conceptually below a concept like the browser process. As a compromise, could we write something like "for trusted clients (e.g., the browser process in Chrome)" ?
Message was sent while issue was closed.
On 2017/07/13 11:30:15, blundell wrote: > https://codereview.chromium.org/2975683002/diff/140001/device/geolocation/pub... > File device/geolocation/public/interfaces/geolocation_config.mojom (right): > > https://codereview.chromium.org/2975683002/diff/140001/device/geolocation/pub... > device/geolocation/public/interfaces/geolocation_config.mojom:7: // Provides an > interface for browser process to access geolocation capturing > On 2017/07/12 18:19:32, Ken Rockot(use gerrit already) wrote: > > I think it's wrong to refer to "browser process" in a device service mojom, > but > > perhaps this is just another symptom of the more general problem of security > > review being more difficult in a mojom world. Fine for now I guess. > > I agree with Ken. The Device Service is conceptually below a concept like the > browser process. As a compromise, could we write something like > > "for trusted clients (e.g., the browser process in Chrome)" > > ? That seems fine to me. My main point of confusion was the "privileged information" part, for which it sounds like the fix is uncontroversial.
Message was sent while issue was closed.
On 2017/07/13 16:32:10, estark wrote: > On 2017/07/13 11:30:15, blundell wrote: > > > https://codereview.chromium.org/2975683002/diff/140001/device/geolocation/pub... > > File device/geolocation/public/interfaces/geolocation_config.mojom (right): > > > > > https://codereview.chromium.org/2975683002/diff/140001/device/geolocation/pub... > > device/geolocation/public/interfaces/geolocation_config.mojom:7: // Provides > an > > interface for browser process to access geolocation capturing > > On 2017/07/12 18:19:32, Ken Rockot(use gerrit already) wrote: > > > I think it's wrong to refer to "browser process" in a device service mojom, > > but > > > perhaps this is just another symptom of the more general problem of security > > > review being more difficult in a mojom world. Fine for now I guess. > > > > I agree with Ken. The Device Service is conceptually below a concept like the > > browser process. As a compromise, could we write something like > > > > "for trusted clients (e.g., the browser process in Chrome)" > > > > ? > > That seems fine to me. My main point of confusion was the "privileged > information" part, for which it sounds like the fix is uncontroversial. I uploaded another CL and fixed the comment https://chromium-review.googlesource.com/c/571031/ |
