New issue
Advanced search Search tips

Issue 637869 link

Starred by 1 user

Issue metadata

Status: WontFix
Owner: ----
Closed: Sep 24
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Bug



Sign in to add a comment

Geolocation classes cleanup

Project Member Reported by mcasas@chromium.org, Aug 15 2016

Issue description


wez@ had a few suggestions to (further) improve Geolocation classes (slightly redacted):

1. I think we should migrate it from Singleton to LazyInstance; they're pretty much equivalent but LazyInstance avoids polluting the interface with Singleton guff.

2. The implementation should be split out into the existing singleton, which callers interact with, and a "core" object which exists only while it is in-use, and holds all the code & state that is run & used on the Geolocation thread. When it goes out of use, you just DeleteSoon that object on the Geolocation thread.

3. It should ideally be initialised with a SequencedTaskRunner, or get one from the browser's SequencedWorkerPool, rather than creating its own dedicated thread.

4. If it must have its own Thread, it should own that as a member, not derive from Thread!

5. MockLocationProvider should be renamed to FakeLocationProvider, since it is a fake, not a mock.  The comments on the factory functions should be tidied up, since they're incorrect.  I'd also recommend that the factory functions be moved to static-member-functions CreateFoo() of the class itself, and updated to use unique_ptr<>.

 
Status: Started (was: Available)
When rewiring the threading, keep in mind that on Mac the wifi scan will block the thread for 2-4 seconds in about 50% of the cases. Would it be fine to do that on one from the pool? See the Net.Wifi.ScanLatency histogram.

Comment 3 by w...@chromium.org, Aug 18 2016

Re #2: I think that's acceptable so long as we pull a task-runner from the correct worker pool.
Project Member

Comment 4 by bugdroid1@chromium.org, Aug 26 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d04d18d7205ad8f2ea24bfa956a833193c6531f9

commit d04d18d7205ad8f2ea24bfa956a833193c6531f9
Author: lethalantidote <lethalantidote@chromium.org>
Date: Fri Aug 26 17:09:11 2016

Gets rid of the LocationArbitrator interface, in preference for LocationProvider.

This CL reorganizes classes that derived from LocationArbitrator to instead
derive from LocationProvider. There was a quite a bit of overlap between the
two interfaces, and merging the two in favor of LocationProvider makes later
work with Blimp much easier.

Other work included:
*Addresses TODO to remove CreateArbitrator in favor of SetArbitratorForTest

BUG=614486,  637869 

Review-Url: https://codereview.chromium.org/2226143002
Cr-Commit-Position: refs/heads/master@{#414735}

[modify] https://crrev.com/d04d18d7205ad8f2ea24bfa956a833193c6531f9/blimp/engine/feature/geolocation/blimp_location_provider.cc
[modify] https://crrev.com/d04d18d7205ad8f2ea24bfa956a833193c6531f9/blimp/engine/feature/geolocation/blimp_location_provider.h
[modify] https://crrev.com/d04d18d7205ad8f2ea24bfa956a833193c6531f9/blimp/engine/feature/geolocation/blimp_location_provider_unittest.cc
[modify] https://crrev.com/d04d18d7205ad8f2ea24bfa956a833193c6531f9/blimp/engine/feature/geolocation/engine_geolocation_feature_unittest.cc
[modify] https://crrev.com/d04d18d7205ad8f2ea24bfa956a833193c6531f9/device/geolocation/BUILD.gn
[modify] https://crrev.com/d04d18d7205ad8f2ea24bfa956a833193c6531f9/device/geolocation/geolocation_provider_impl.cc
[modify] https://crrev.com/d04d18d7205ad8f2ea24bfa956a833193c6531f9/device/geolocation/geolocation_provider_impl.h
[modify] https://crrev.com/d04d18d7205ad8f2ea24bfa956a833193c6531f9/device/geolocation/geolocation_provider_impl_unittest.cc
[delete] https://crrev.com/dca0b3af9cef28d4fdb953e3b83bbe94a7671df6/device/geolocation/location_arbitrator.h
[modify] https://crrev.com/d04d18d7205ad8f2ea24bfa956a833193c6531f9/device/geolocation/location_arbitrator_impl.cc
[modify] https://crrev.com/d04d18d7205ad8f2ea24bfa956a833193c6531f9/device/geolocation/location_arbitrator_impl.h
[modify] https://crrev.com/d04d18d7205ad8f2ea24bfa956a833193c6531f9/device/geolocation/location_arbitrator_impl_unittest.cc
[modify] https://crrev.com/d04d18d7205ad8f2ea24bfa956a833193c6531f9/device/geolocation/location_provider.h
[modify] https://crrev.com/d04d18d7205ad8f2ea24bfa956a833193c6531f9/device/geolocation/location_provider_android.cc
[modify] https://crrev.com/d04d18d7205ad8f2ea24bfa956a833193c6531f9/device/geolocation/location_provider_android.h
[modify] https://crrev.com/d04d18d7205ad8f2ea24bfa956a833193c6531f9/device/geolocation/location_provider_base.cc
[modify] https://crrev.com/d04d18d7205ad8f2ea24bfa956a833193c6531f9/device/geolocation/location_provider_base.h
[delete] https://crrev.com/dca0b3af9cef28d4fdb953e3b83bbe94a7671df6/device/geolocation/mock_location_arbitrator.cc
[delete] https://crrev.com/dca0b3af9cef28d4fdb953e3b83bbe94a7671df6/device/geolocation/mock_location_arbitrator.h
[modify] https://crrev.com/d04d18d7205ad8f2ea24bfa956a833193c6531f9/device/geolocation/mock_location_provider.cc
[modify] https://crrev.com/d04d18d7205ad8f2ea24bfa956a833193c6531f9/device/geolocation/mock_location_provider.h
[modify] https://crrev.com/d04d18d7205ad8f2ea24bfa956a833193c6531f9/device/geolocation/network_location_provider.cc
[modify] https://crrev.com/d04d18d7205ad8f2ea24bfa956a833193c6531f9/device/geolocation/network_location_provider.h
[modify] https://crrev.com/d04d18d7205ad8f2ea24bfa956a833193c6531f9/device/geolocation/network_location_provider_unittest.cc

Project Member

Comment 5 by bugdroid1@chromium.org, Sep 1 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/409ec48bdceb927a16bbc46f20ecc6a9edd24daa

commit 409ec48bdceb927a16bbc46f20ecc6a9edd24daa
Author: lethalantidote <lethalantidote@chromium.org>
Date: Thu Sep 01 20:53:00 2016

Gets rid of LocationProviderBase.

It provided very little functionality to just three classes, one of which will be disappearing soon (FakeLocationProvider). All the functionality was protected, meaning removing the interface has no impact outside these three classes.

BUG= 637869 

Review-Url: https://codereview.chromium.org/2298013002
Cr-Commit-Position: refs/heads/master@{#416047}

[modify] https://crrev.com/409ec48bdceb927a16bbc46f20ecc6a9edd24daa/device/geolocation/BUILD.gn
[modify] https://crrev.com/409ec48bdceb927a16bbc46f20ecc6a9edd24daa/device/geolocation/fake_location_provider.cc
[modify] https://crrev.com/409ec48bdceb927a16bbc46f20ecc6a9edd24daa/device/geolocation/fake_location_provider.h
[modify] https://crrev.com/409ec48bdceb927a16bbc46f20ecc6a9edd24daa/device/geolocation/location_provider_android.cc
[modify] https://crrev.com/409ec48bdceb927a16bbc46f20ecc6a9edd24daa/device/geolocation/location_provider_android.h
[delete] https://crrev.com/af2f7ed196372bcf7cbfcf673a3f3c05d146c78b/device/geolocation/location_provider_base.cc
[delete] https://crrev.com/af2f7ed196372bcf7cbfcf673a3f3c05d146c78b/device/geolocation/location_provider_base.h
[modify] https://crrev.com/409ec48bdceb927a16bbc46f20ecc6a9edd24daa/device/geolocation/network_location_provider.cc
[modify] https://crrev.com/409ec48bdceb927a16bbc46f20ecc6a9edd24daa/device/geolocation/network_location_provider.h

Cc: -mvanouwe...@chromium.org

Comment 7 by mcasas@chromium.org, Mar 13 2017

Status: Available (was: Started)
Components: Blink>Geolocation
Components: -Blink>Location
Project Member

Comment 10 by sheriffbot@chromium.org, Sep 24

Labels: Hotlist-Recharge-Cold
Status: Untriaged (was: Available)
This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue.

Sorry for the inconvenience if the bug really should have been left as Available.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Status: WontFix (was: Untriaged)

Sign in to add a comment