Add device location reporting

Add device location reporting

This CL implements device location reporting for Enterprise-enrolled Chrome OS devices. The position is determined using the existing Chrome geolocation stack.

This CL depends on https://chromiumcodereview.appspot.com/10344016/

ISSUE=chromium-os:18710
TEST=unit_tests including new DeviceStatusCollectorTest.Location

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=135348

[bartfab (slow), 2012-04-19 19:38:14]

Hi Joao, John, Nikita,

could you review the parts of this CL that touch upon your respective areas of
expertise? Thanks.

[Joao da Silva, 2012-04-20 09:49:57]

https://chromiumcodereview.appspot.com/10103029/diff/2001/chrome/browser/poli...
File chrome/browser/policy/device_status_collector.cc (right):

https://chromiumcodereview.appspot.com/10103029/diff/2001/chrome/browser/poli...
chrome/browser/policy/device_status_collector.cc:59: class
DeviceStatusLocationHelper : public GeolocationObserver {
I'd move this one to its own file.

https://chromiumcodereview.appspot.com/10103029/diff/2001/chrome/browser/poli...
chrome/browser/policy/device_status_collector.cc:69: virtual void
OnLocationUpdate(const Geoposition& position);
OVERRIDE

https://chromiumcodereview.appspot.com/10103029/diff/2001/chrome/browser/poli...
chrome/browser/policy/device_status_collector.cc:120: position.timestamp =
base::Time::FromInternalValue(timestamp);
Nit: base::Time -> Time

https://chromiumcodereview.appspot.com/10103029/diff/2001/chrome/browser/poli...
chrome/browser/policy/device_status_collector.cc:133: // The destructor is only
public so that the DeleteSoon task can call it.
Make the dtor private, and add

friend class base::DeleteHelper<DeviceStatusLocationHelper>;

https://chromiumcodereview.appspot.com/10103029/diff/2001/chrome/browser/poli...
chrome/browser/policy/device_status_collector.cc:142: // posted to it are
processed before destrucion.
*destruction

https://chromiumcodereview.appspot.com/10103029/diff/2001/chrome/browser/poli...
chrome/browser/policy/device_status_collector.cc:148: // 2. DestructInner()
reaches the IO thread after any updates queued up for it
In these 2 lines: DestructInner -> DestructInternal

https://chromiumcodereview.appspot.com/10103029/diff/2001/chrome/browser/poli...
chrome/browser/policy/device_status_collector.cc:201:
provider_->OnPermissionGranted();
Doesn't this need a GURL?

https://chromiumcodereview.appspot.com/10103029/diff/2001/chrome/browser/poli...
chrome/browser/policy/device_status_collector.cc:223:
&DeviceStatusLocationHelper::StartObserving);
Shouldn't this be in an else block here, so that interval > elapsed for sure?

https://chromiumcodereview.appspot.com/10103029/diff/2001/chrome/browser/poli...
chrome/browser/policy/device_status_collector.cc:356: if
(geolocation_provider_for_test_)
Braces around the block when it spans more than 1 line

https://chromiumcodereview.appspot.com/10103029/diff/2001/chrome/browser/poli...
File chrome/browser/policy/proto/device_management_backend.proto (right):

https://chromiumcodereview.appspot.com/10103029/diff/2001/chrome/browser/poli...
chrome/browser/policy/proto/device_management_backend.proto:280: // Altitude in
(above WGS84 datum).
Altitude in <what>?

https://chromiumcodereview.appspot.com/10103029/diff/2001/chrome/browser/poli...
chrome/browser/policy/proto/device_management_backend.proto:323: // Device
location
Nit: make this a sentence, and close with '.'.

https://chromiumcodereview.appspot.com/10103029/diff/2001/chrome/common/pref_...
File chrome/common/pref_names.cc (left):

https://chromiumcodereview.appspot.com/10103029/diff/2001/chrome/common/pref_...
chrome/common/pref_names.cc:1653: const char kReportDeviceActivityTimes[] =
"device_status.report_activity_times";
Nice cleanup

https://chromiumcodereview.appspot.com/10103029/diff/2001/content/browser/geo...
File content/browser/geolocation/geolocation_provider.h (right):

https://chromiumcodereview.appspot.com/10103029/diff/2001/content/browser/geo...
content/browser/geolocation/geolocation_provider.h:60: friend struct
DefaultSingletonTraits<GeolocationProvider>;
I'm not familiar with this code, but a singleton with a GetInstace() method
should also have a protected/private ctor.

[Nikita (slow), 2012-04-20 11:44:42]

chromeos/login lgtm

You'll need owners approval for chromeos/* code.

[John Knottenbelt, 2012-04-20 12:50:42]

content/browser/geolocation lgtm  
(one inline comment)

https://chromiumcodereview.appspot.com/10103029/diff/2001/content/browser/geo...
File content/browser/geolocation/geolocation_provider.h (right):

https://chromiumcodereview.appspot.com/10103029/diff/2001/content/browser/geo...
content/browser/geolocation/geolocation_provider.h:60: friend struct
DefaultSingletonTraits<GeolocationProvider>;
Agree.

On 2012/04/20 09:49:57, Joao da Silva wrote:
> I'm not familiar with this code, but a singleton with a GetInstace() method
> should also have a protected/private ctor.

[bartfab (slow), 2012-04-20 14:10:15]

https://chromiumcodereview.appspot.com/10103029/diff/2001/chrome/browser/poli...
File chrome/browser/policy/device_status_collector.cc (right):

https://chromiumcodereview.appspot.com/10103029/diff/2001/chrome/browser/poli...
chrome/browser/policy/device_status_collector.cc:59: class
DeviceStatusLocationHelper : public GeolocationObserver {
On 2012/04/20 09:49:57, Joao da Silva wrote:
> I'd move this one to its own file.

Done.

https://chromiumcodereview.appspot.com/10103029/diff/2001/chrome/browser/poli...
chrome/browser/policy/device_status_collector.cc:69: virtual void
OnLocationUpdate(const Geoposition& position);
On 2012/04/20 09:49:57, Joao da Silva wrote:
> OVERRIDE

Done.

https://chromiumcodereview.appspot.com/10103029/diff/2001/chrome/browser/poli...
chrome/browser/policy/device_status_collector.cc:120: position.timestamp =
base::Time::FromInternalValue(timestamp);
It moved to its own .cc file without "using base::Time;" now.

https://chromiumcodereview.appspot.com/10103029/diff/2001/chrome/browser/poli...
chrome/browser/policy/device_status_collector.cc:133: // The destructor is only
public so that the DeleteSoon task can call it.
On 2012/04/20 09:49:57, Joao da Silva wrote:
> Make the dtor private, and add
> 
> friend class base::DeleteHelper<DeviceStatusLocationHelper>;

Done.

https://chromiumcodereview.appspot.com/10103029/diff/2001/chrome/browser/poli...
chrome/browser/policy/device_status_collector.cc:142: // posted to it are
processed before destrucion.
On 2012/04/20 09:49:57, Joao da Silva wrote:
> *destruction

Done.

https://chromiumcodereview.appspot.com/10103029/diff/2001/chrome/browser/poli...
chrome/browser/policy/device_status_collector.cc:148: // 2. DestructInner()
reaches the IO thread after any updates queued up for it
On 2012/04/20 09:49:57, Joao da Silva wrote:
> In these 2 lines: DestructInner -> DestructInternal

Done.

https://chromiumcodereview.appspot.com/10103029/diff/2001/chrome/browser/poli...
chrome/browser/policy/device_status_collector.cc:201:
provider_->OnPermissionGranted();
The GURL parameter was not actually being used for anything anymore. It has just
been removed in CL 10107017.

https://chromiumcodereview.appspot.com/10103029/diff/2001/chrome/browser/poli...
chrome/browser/policy/device_status_collector.cc:223:
&DeviceStatusLocationHelper::StartObserving);
On 2012/04/20 09:49:57, Joao da Silva wrote:
> Shouldn't this be in an else block here, so that interval > elapsed for sure?

Done.

https://chromiumcodereview.appspot.com/10103029/diff/2001/chrome/browser/poli...
chrome/browser/policy/device_status_collector.cc:356: if
(geolocation_provider_for_test_)
On 2012/04/20 09:49:57, Joao da Silva wrote:
> Braces around the block when it spans more than 1 line

Done.

https://chromiumcodereview.appspot.com/10103029/diff/2001/chrome/browser/poli...
File chrome/browser/policy/proto/device_management_backend.proto (right):

https://chromiumcodereview.appspot.com/10103029/diff/2001/chrome/browser/poli...
chrome/browser/policy/proto/device_management_backend.proto:280: // Altitude in
(above WGS84 datum).
In 0.546806649 fathoms :).

Done.

https://chromiumcodereview.appspot.com/10103029/diff/2001/chrome/browser/poli...
chrome/browser/policy/proto/device_management_backend.proto:323: // Device
location
On 2012/04/20 09:49:57, Joao da Silva wrote:
> Nit: make this a sentence, and close with '.'.

Done.

https://chromiumcodereview.appspot.com/10103029/diff/2001/content/browser/geo...
File content/browser/geolocation/geolocation_provider.h (right):

https://chromiumcodereview.appspot.com/10103029/diff/2001/content/browser/geo...
content/browser/geolocation/geolocation_provider.h:60: friend struct
DefaultSingletonTraits<GeolocationProvider>;
On 2012/04/20 09:49:57, Joao da Silva wrote:
> I'm not familiar with this code, but a singleton with a GetInstace() method
> should also have a protected/private ctor.

Done.

[bartfab (slow), 2012-04-20 14:16:48]

All comments addressed.

I also modified the GeolocationProvider so that it is aware that the client
thread is always, always the Browser IO thread. At John's request, I am looping
in Jonathan to review that new part of the CL.

[Joao da Silva, 2012-04-20 14:21:09]

lgtm. The chromeos/ stuff was created by Julian and we're quite familiar with
it, so go ahead and land this :-)

http://codereview.chromium.org/10103029/diff/2001/chrome/browser/policy/devic...
File chrome/browser/policy/device_status_collector.cc (right):

http://codereview.chromium.org/10103029/diff/2001/chrome/browser/policy/devic...
chrome/browser/policy/device_status_collector.cc:201:
provider_->OnPermissionGranted();
On 2012/04/20 14:10:15, bartfab wrote:
> The GURL parameter was not actually being used for anything anymore. It has
just
> been removed in CL 10107017.

I see, thanks for the clarification.

http://codereview.chromium.org/10103029/diff/5057/chrome/browser/policy/devic...
File chrome/browser/policy/device_status_collector.cc (right):

http://codereview.chromium.org/10103029/diff/5057/chrome/browser/policy/devic...
chrome/browser/policy/device_status_collector.cc:11: #include
"chrome/browser/policy/device_status_location_helper.h"
Nit: order

http://codereview.chromium.org/10103029/diff/5057/chrome/browser/policy/devic...
File chrome/browser/policy/device_status_location_helper.cc (right):

http://codereview.chromium.org/10103029/diff/5057/chrome/browser/policy/devic...
chrome/browser/policy/device_status_location_helper.cc:24: std::string
timestamp_str;
#include <string>

http://codereview.chromium.org/10103029/diff/5057/chrome/browser/policy/devic...
chrome/browser/policy/device_status_location_helper.cc:26: const
base::DictionaryValue* location =
#include "base/values.h"

http://codereview.chromium.org/10103029/diff/5057/chrome/browser/policy/devic...
chrome/browser/policy/device_status_location_helper.cc:58: // 2.
DestructInteral() reaches the IO thread after any updates queued up for
nit: DestructInteral -> DestructInternal

http://codereview.chromium.org/10103029/diff/5057/chrome/browser/policy/devic...
File chrome/browser/policy/device_status_location_helper.h (right):

http://codereview.chromium.org/10103029/diff/5057/chrome/browser/policy/devic...
chrome/browser/policy/device_status_location_helper.h:33: friend class
base::DeleteHelper<DeviceStatusLocationHelper>;
#include "base/sequenced_task_runner_helpers.h" for the declaration of this one

[bartfab (slow), 2012-04-23 09:48:54]

http://codereview.chromium.org/10103029/diff/5057/chrome/browser/policy/devic...
File chrome/browser/policy/device_status_collector.cc (right):

http://codereview.chromium.org/10103029/diff/5057/chrome/browser/policy/devic...
chrome/browser/policy/device_status_collector.cc:11: #include
"chrome/browser/policy/device_status_location_helper.h"
On 2012/04/20 14:21:09, Joao da Silva wrote:
> Nit: order

Done.

http://codereview.chromium.org/10103029/diff/5057/chrome/browser/policy/devic...
File chrome/browser/policy/device_status_location_helper.cc (right):

http://codereview.chromium.org/10103029/diff/5057/chrome/browser/policy/devic...
chrome/browser/policy/device_status_location_helper.cc:24: std::string
timestamp_str;
On 2012/04/20 14:21:09, Joao da Silva wrote:
> #include <string>

Done.

http://codereview.chromium.org/10103029/diff/5057/chrome/browser/policy/devic...
chrome/browser/policy/device_status_location_helper.cc:26: const
base::DictionaryValue* location =
On 2012/04/20 14:21:09, Joao da Silva wrote:
> #include "base/values.h"

Done.

http://codereview.chromium.org/10103029/diff/5057/chrome/browser/policy/devic...
chrome/browser/policy/device_status_location_helper.cc:58: // 2.
DestructInteral() reaches the IO thread after any updates queued up for
On 2012/04/20 14:21:09, Joao da Silva wrote:
> nit: DestructInteral -> DestructInternal

Done.

http://codereview.chromium.org/10103029/diff/5057/chrome/browser/policy/devic...
File chrome/browser/policy/device_status_location_helper.h (right):

http://codereview.chromium.org/10103029/diff/5057/chrome/browser/policy/devic...
chrome/browser/policy/device_status_location_helper.h:33: friend class
base::DeleteHelper<DeviceStatusLocationHelper>;
A forward declaration should actually be sufficient. I added that.

[bartfab (slow), 2012-04-23 09:52:30]

All comments so far are addressed. Jonathan, when you get a chance, could you
have a look? If my changes to the GeolocationProvider are OK, the CL is ready to
go.

[joth, 2012-04-23 10:05:14]

I only read the geolocation provider side of it, but lgtm for those files. small
suggestions..

http://codereview.chromium.org/10103029/diff/5057/chrome/browser/policy/devic...
File chrome/browser/policy/device_status_location_helper.cc (right):

http://codereview.chromium.org/10103029/diff/5057/chrome/browser/policy/devic...
chrome/browser/policy/device_status_location_helper.cc:28: if
(location->GetDouble("latitude", &position.latitude) &&
a comment would be useful - explaining why the location is persisted across
sessions, and that this is a one-time restore of the last-known location.
(timestamp indicates freshness of that fix)

http://codereview.chromium.org/10103029/diff/5057/chrome/browser/policy/devic...
chrome/browser/policy/device_status_location_helper.cc:132: void
DeviceStatusLocationHelper::ScheduleUpdate(const Geoposition& position) {
comment explaining why the timer is needed here would be useful. (AIUI: want to
save battery and only request location updates every kPollIntervalSeconds, which
is assumed to be several orders of magnitude longer than the geolocation
provider's internal refresh period)

http://codereview.chromium.org/10103029/diff/5057/chrome/browser/policy/devic...
File chrome/browser/policy/device_status_location_helper.h (right):

http://codereview.chromium.org/10103029/diff/5057/chrome/browser/policy/devic...
chrome/browser/policy/device_status_location_helper.h:30: static const unsigned
int kPollIntervalSeconds = 1800;
nit: could write this as = 30 * kSecondsPerMinute

http://codereview.chromium.org/10103029/diff/5057/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

http://codereview.chromium.org/10103029/diff/5057/chrome/common/pref_names.cc...
chrome/common/pref_names.cc:1651: // A local state pref holding the last known
location when device location
nit: "local state pref" is spurious as this is in local state section (line 948)

http://codereview.chromium.org/10103029/diff/5057/content/browser/geolocation...
File content/browser/geolocation/geolocation_provider.h (right):

http://codereview.chromium.org/10103029/diff/5057/content/browser/geolocation...
content/browser/geolocation/geolocation_provider.h:60: protected:
comment why protected. (derivation required for test code only)

http://codereview.chromium.org/10103029/diff/13007/content/browser/geolocatio...
File content/browser/geolocation/geolocation_provider.h (right):

http://codereview.chromium.org/10103029/diff/13007/content/browser/geolocatio...
content/browser/geolocation/geolocation_provider.h:53: // called on the IO
thread so that the GeolocationProvider is always
add a DCHECK for this?

[Joao da Silva, 2012-04-23 10:19:57]

Still looks good to me, just a couple nits left.

http://codereview.chromium.org/10103029/diff/13007/chrome/browser/policy/devi...
File chrome/browser/policy/device_status_collector.cc (right):

http://codereview.chromium.org/10103029/diff/13007/chrome/browser/policy/devi...
chrome/browser/policy/device_status_collector.cc:308:
em::DeviceStatusReportRequest* request) {
Nit: indent (4 spaces)

http://codereview.chromium.org/10103029/diff/13007/chrome/browser/policy/devi...
File chrome/browser/policy/device_status_collector_unittest.cc (right):

http://codereview.chromium.org/10103029/diff/13007/chrome/browser/policy/devi...
chrome/browser/policy/device_status_collector_unittest.cc:37: #include
"testing/gmock/include/gmock/gmock-spec-builders.h"
This isn't common, usually just "gmock.h" is included (it includes all the
others)

http://codereview.chromium.org/10103029/diff/13007/chrome/browser/policy/devi...
chrome/browser/policy/device_status_collector_unittest.cc:187: using
::testing::SetArgPointee;
Move these to the top

http://codereview.chromium.org/10103029/diff/13007/chrome/browser/policy/devi...
chrome/browser/policy/device_status_collector_unittest.cc:282:
base::TimeDelta::FromMinutes(10));
Nit: no need for the base:: qualifiers in these 2 lines

http://codereview.chromium.org/10103029/diff/13007/chrome/browser/policy/devi...
chrome/browser/policy/device_status_collector_unittest.cc:574: 
Nitty nit: only 1 newline :-)

http://codereview.chromium.org/10103029/diff/13007/chrome/browser/policy/devi...
chrome/browser/policy/device_status_collector_unittest.cc:605:
EXPECT_TRUE(prefs_.GetDictionary(prefs::kDeviceLocation)->empty());
Also check that this is not empty after getting a valid fix?

http://codereview.chromium.org/10103029/diff/13007/content/browser/geolocatio...
File content/browser/geolocation/geolocation_provider.cc (right):

http://codereview.chromium.org/10103029/diff/13007/content/browser/geolocatio...
content/browser/geolocation/geolocation_provider.cc:12: #include
"base/message_loop.h"
Nit: order

http://codereview.chromium.org/10103029/diff/13007/content/browser/geolocatio...
content/browser/geolocation/geolocation_provider.cc:149: return
content::BrowserThread::CurrentlyOn(content::BrowserThread::IO);
It's common to just 

DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::IO));

It's also common to declare

using content::BrowserThread;

I'd remove OnIOThread() and spare this unnecessary symbol in release builds.

http://codereview.chromium.org/10103029/diff/13007/content/browser/geolocatio...
File content/browser/geolocation/mock_location_provider.h (right):

http://codereview.chromium.org/10103029/diff/13007/content/browser/geolocatio...
content/browser/geolocation/mock_location_provider.h:12: #include
"base/threading/thread.h"
Since you're doing a little IWYU here: these 2 aren't necessary :-)

And message_loop_proxy.h should be moved from the .cc to the .h here

[bartfab (slow), 2012-04-23 12:21:27]

http://codereview.chromium.org/10103029/diff/5057/chrome/browser/policy/devic...
File chrome/browser/policy/device_status_location_helper.cc (right):

http://codereview.chromium.org/10103029/diff/5057/chrome/browser/policy/devic...
chrome/browser/policy/device_status_location_helper.cc:28: if
(location->GetDouble("latitude", &position.latitude) &&
On 2012/04/23 10:05:14, joth wrote:
> a comment would be useful - explaining why the location is persisted across
> sessions, and that this is a one-time restore of the last-known location.
> (timestamp indicates freshness of that fix)

Done.

http://codereview.chromium.org/10103029/diff/5057/chrome/browser/policy/devic...
chrome/browser/policy/device_status_location_helper.cc:132: void
DeviceStatusLocationHelper::ScheduleUpdate(const Geoposition& position) {
On 2012/04/23 10:05:14, joth wrote:
> comment explaining why the timer is needed here would be useful. (AIUI: want
to
> save battery and only request location updates every kPollIntervalSeconds,
which
> is assumed to be several orders of magnitude longer than the geolocation
> provider's internal refresh period)

Done.

http://codereview.chromium.org/10103029/diff/5057/chrome/browser/policy/devic...
File chrome/browser/policy/device_status_location_helper.h (right):

http://codereview.chromium.org/10103029/diff/5057/chrome/browser/policy/devic...
chrome/browser/policy/device_status_location_helper.h:30: static const unsigned
int kPollIntervalSeconds = 1800;
How about = 30 * 60 instead? time.h has various conversion constants but they
seem to deal with less frequently used units and tricky cases. The number of
seconds per minute seems a bit too trivial to warrant its own constant.

http://codereview.chromium.org/10103029/diff/5057/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

http://codereview.chromium.org/10103029/diff/5057/chrome/common/pref_names.cc...
chrome/common/pref_names.cc:1651: // A local state pref holding the last known
location when device location
On 2012/04/23 10:05:14, joth wrote:
> nit: "local state pref" is spurious as this is in local state section (line
948)
> 

Done.

http://codereview.chromium.org/10103029/diff/5057/content/browser/geolocation...
File content/browser/geolocation/geolocation_provider.h (right):

http://codereview.chromium.org/10103029/diff/5057/content/browser/geolocation...
content/browser/geolocation/geolocation_provider.h:60: protected:
On 2012/04/23 10:05:14, joth wrote:
> comment why protected. (derivation required for test code only)

Done.

http://codereview.chromium.org/10103029/diff/13007/chrome/browser/policy/devi...
File chrome/browser/policy/device_status_collector.cc (right):

http://codereview.chromium.org/10103029/diff/13007/chrome/browser/policy/devi...
chrome/browser/policy/device_status_collector.cc:308:
em::DeviceStatusReportRequest* request) {
On 2012/04/23 10:19:57, Joao da Silva wrote:
> Nit: indent (4 spaces)

Done.

http://codereview.chromium.org/10103029/diff/13007/chrome/browser/policy/devi...
File chrome/browser/policy/device_status_collector_unittest.cc (right):

http://codereview.chromium.org/10103029/diff/13007/chrome/browser/policy/devi...
chrome/browser/policy/device_status_collector_unittest.cc:37: #include
"testing/gmock/include/gmock/gmock-spec-builders.h"
On 2012/04/23 10:19:57, Joao da Silva wrote:
> This isn't common, usually just "gmock.h" is included (it includes all the
> others)

Done.

http://codereview.chromium.org/10103029/diff/13007/chrome/browser/policy/devi...
chrome/browser/policy/device_status_collector_unittest.cc:187: using
::testing::SetArgPointee;
On 2012/04/23 10:19:57, Joao da Silva wrote:
> Move these to the top

Done.

http://codereview.chromium.org/10103029/diff/13007/chrome/browser/policy/devi...
chrome/browser/policy/device_status_collector_unittest.cc:282:
base::TimeDelta::FromMinutes(10));
On 2012/04/23 10:19:57, Joao da Silva wrote:
> Nit: no need for the base:: qualifiers in these 2 lines

Done.

http://codereview.chromium.org/10103029/diff/13007/chrome/browser/policy/devi...
chrome/browser/policy/device_status_collector_unittest.cc:574: 
On 2012/04/23 10:19:57, Joao da Silva wrote:
> Nitty nit: only 1 newline :-)

Done.

http://codereview.chromium.org/10103029/diff/13007/chrome/browser/policy/devi...
chrome/browser/policy/device_status_collector_unittest.cc:605:
EXPECT_TRUE(prefs_.GetDictionary(prefs::kDeviceLocation)->empty());
On 2012/04/23 10:19:57, Joao da Silva wrote:
> Also check that this is not empty after getting a valid fix?

Done.

http://codereview.chromium.org/10103029/diff/13007/content/browser/geolocatio...
File content/browser/geolocation/geolocation_provider.cc (right):

http://codereview.chromium.org/10103029/diff/13007/content/browser/geolocatio...
content/browser/geolocation/geolocation_provider.cc:12: #include
"base/message_loop.h"
On 2012/04/23 10:19:57, Joao da Silva wrote:
> Nit: order

Done.

http://codereview.chromium.org/10103029/diff/13007/content/browser/geolocatio...
content/browser/geolocation/geolocation_provider.cc:149: return
content::BrowserThread::CurrentlyOn(content::BrowserThread::IO);
Done. Since you say it is common to add a using line for content::BrowserThread,
I did the same in device_status_location_helper.cc.

http://codereview.chromium.org/10103029/diff/13007/content/browser/geolocatio...
File content/browser/geolocation/geolocation_provider.h (right):

http://codereview.chromium.org/10103029/diff/13007/content/browser/geolocatio...
content/browser/geolocation/geolocation_provider.h:53: // called on the IO
thread so that the GeolocationProvider is always
There is a DCHECK in GetInstance() already. I put in the comment to explain why
it is there.

http://codereview.chromium.org/10103029/diff/13007/content/browser/geolocatio...
File content/browser/geolocation/mock_location_provider.h (right):

http://codereview.chromium.org/10103029/diff/13007/content/browser/geolocatio...
content/browser/geolocation/mock_location_provider.h:12: #include
"base/threading/thread.h"
I was limiting myself to code that I really touched. But you are right - once
you are doing something, you might as well do it right. Done.

[commit-bot: I haz the power, 2012-04-23 15:01:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bartfab@chromium.org/10103029/24001

[commit-bot: I haz the power, 2012-04-23 15:19:55]

Try job failure for 10103029-24001 (retry) on mac_rel for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-04-23 15:26:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bartfab@chromium.org/10103029/34002

[commit-bot: I haz the power, 2012-04-23 16:05:27]

Try job failure for 10103029-34002 (retry) on mac_rel for step "check_deps".
It's a second try, previously, step "check_deps" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[joth, 2012-04-23 16:10:33]

http://codereview.chromium.org/10103029/diff/34002/chrome/browser/policy/devi...
File chrome/browser/policy/device_status_collector.cc (right):

http://codereview.chromium.org/10103029/diff/34002/chrome/browser/policy/devi...
chrome/browser/policy/device_status_collector.cc:27: #include
"content/browser/geolocation/geolocation_provider.h"
I think this is causing the check-deps error: looks like chrome/browser may only
access content layer via content/public/....

exposing geolocaiton in the content public API would be somewhat unfortunate. I
suggest you ping jam@ to discuss options on how to achieve this.

[bartfab (slow), 2012-04-23 16:17:48]

http://codereview.chromium.org/10103029/diff/34002/chrome/browser/policy/devi...
File chrome/browser/policy/device_status_collector.cc (right):

http://codereview.chromium.org/10103029/diff/34002/chrome/browser/policy/devi...
chrome/browser/policy/device_status_collector.cc:27: #include
"content/browser/geolocation/geolocation_provider.h"
Yes, I spotted that too. Will ping jam@.

[bartfab (slow), 2012-05-03 14:15:37]

Hi Joao, Nikita,

Could you please review this new implementation of the CL? The previous version
was LGTM'd by you but failed on the trybots as it straddled the content/chrome
boundary. The new implementation uses a freshly introduced content geolocation
API to work around this issue.

[Joao da Silva, 2012-05-03 19:04:08]

https://chromiumcodereview.appspot.com/10103029/diff/38002/chrome/browser/pol...
File chrome/browser/policy/device_status_collector.cc (right):

https://chromiumcodereview.appspot.com/10103029/diff/38002/chrome/browser/pol...
chrome/browser/policy/device_status_collector.cc:105: if
(location->GetDouble("latitude", &position.latitude) &&
It'd be nicer to use constants for these strings:

namespace {
const char kLatitude[] = "latitude";
...
}

https://chromiumcodereview.appspot.com/10103029/diff/38002/chrome/browser/pol...
chrome/browser/policy/device_status_collector.cc:115: position.timestamp =
base::Time::FromInternalValue(timestamp);
nit: drop the base::

https://chromiumcodereview.appspot.com/10103029/diff/38002/chrome/browser/pol...
chrome/browser/policy/device_status_collector.cc:381:
base::TimeDelta::FromSeconds(kGeolocationPollIntervalSeconds);
nit: drop the base:: in these 3 lines

https://chromiumcodereview.appspot.com/10103029/diff/38002/chrome/browser/pol...
chrome/browser/policy/device_status_collector.cc:413: DictionaryValue location;
nit: base::*Value (here and elsewhere)

https://chromiumcodereview.appspot.com/10103029/diff/38002/chrome/browser/pol...
File chrome/browser/policy/device_status_collector.h (right):

https://chromiumcodereview.appspot.com/10103029/diff/38002/chrome/browser/pol...
chrome/browser/policy/device_status_collector.h:145:
base::WeakPtrFactory<DeviceStatusCollector> weak_ptr_factory_;
nitty nit: I've been encouraged to name these 'weak_factory_'

https://chromiumcodereview.appspot.com/10103029/diff/38002/chrome/browser/pol...
File chrome/browser/policy/device_status_collector_unittest.cc (right):

https://chromiumcodereview.appspot.com/10103029/diff/38002/chrome/browser/pol...
chrome/browser/policy/device_status_collector_unittest.cc:177: // by the
AutoMockLocationProvider.
AutoMockLocationProvider -> MockPositionUpdateRequester?

https://chromiumcodereview.appspot.com/10103029/diff/38002/chrome/browser/pol...
chrome/browser/policy/device_status_collector_unittest.cc:182: EXPECT_EQ(0,
location.error_code());
nit: use the constant instead of 0?

https://chromiumcodereview.appspot.com/10103029/diff/38002/chrome/browser/pol...
chrome/browser/policy/device_status_collector_unittest.cc:194: EXPECT_EQ(3.,
location.accuracy());
Use EXPECT_DOUBLE_EQ for these 3

https://chromiumcodereview.appspot.com/10103029/diff/38002/chrome/browser/pol...
chrome/browser/policy/device_status_collector_unittest.cc:205: EXPECT_EQ(1,
location.error_code());
nit: use the constant instead of 1?

https://chromiumcodereview.appspot.com/10103029/diff/38002/chrome/common/pref...
File chrome/common/pref_names.cc (right):

https://chromiumcodereview.appspot.com/10103029/diff/38002/chrome/common/pref...
chrome/common/pref_names.cc:1657: extern const char kDeviceLocation[] =
"device_status.location";
Remove the extern here and in the line above, it only makes sense in
declarations.

[bartfab (slow), 2012-05-04 09:14:57]

https://chromiumcodereview.appspot.com/10103029/diff/38002/chrome/browser/pol...
File chrome/browser/policy/device_status_collector.cc (right):

https://chromiumcodereview.appspot.com/10103029/diff/38002/chrome/browser/pol...
chrome/browser/policy/device_status_collector.cc:105: if
(location->GetDouble("latitude", &position.latitude) &&
On 2012/05/03 19:04:08, Joao da Silva wrote:
> It'd be nicer to use constants for these strings:
> 
> namespace {
> const char kLatitude[] = "latitude";
> ...
> }

Done.

https://chromiumcodereview.appspot.com/10103029/diff/38002/chrome/browser/pol...
chrome/browser/policy/device_status_collector.cc:115: position.timestamp =
base::Time::FromInternalValue(timestamp);
On 2012/05/03 19:04:08, Joao da Silva wrote:
> nit: drop the base::

Done.

https://chromiumcodereview.appspot.com/10103029/diff/38002/chrome/browser/pol...
chrome/browser/policy/device_status_collector.cc:381:
base::TimeDelta::FromSeconds(kGeolocationPollIntervalSeconds);
On 2012/05/03 19:04:08, Joao da Silva wrote:
> nit: drop the base:: in these 3 lines

Done.

https://chromiumcodereview.appspot.com/10103029/diff/38002/chrome/browser/pol...
chrome/browser/policy/device_status_collector.cc:413: DictionaryValue location;
On 2012/05/03 19:04:08, Joao da Silva wrote:
> nit: base::*Value (here and elsewhere)

Done.

https://chromiumcodereview.appspot.com/10103029/diff/38002/chrome/browser/pol...
File chrome/browser/policy/device_status_collector.h (right):

https://chromiumcodereview.appspot.com/10103029/diff/38002/chrome/browser/pol...
chrome/browser/policy/device_status_collector.h:145:
base::WeakPtrFactory<DeviceStatusCollector> weak_ptr_factory_;
On 2012/05/03 19:04:08, Joao da Silva wrote:
> nitty nit: I've been encouraged to name these 'weak_factory_'

Done.

https://chromiumcodereview.appspot.com/10103029/diff/38002/chrome/browser/pol...
File chrome/browser/policy/device_status_collector_unittest.cc (right):

https://chromiumcodereview.appspot.com/10103029/diff/38002/chrome/browser/pol...
chrome/browser/policy/device_status_collector_unittest.cc:177: // by the
AutoMockLocationProvider.
Actually, SetMockPositionToReturnNext(). This comment was a leftover from the
previous implementation.

https://chromiumcodereview.appspot.com/10103029/diff/38002/chrome/browser/pol...
chrome/browser/policy/device_status_collector_unittest.cc:182: EXPECT_EQ(0,
location.error_code());
On 2012/05/03 19:04:08, Joao da Silva wrote:
> nit: use the constant instead of 0?

Done.

https://chromiumcodereview.appspot.com/10103029/diff/38002/chrome/browser/pol...
chrome/browser/policy/device_status_collector_unittest.cc:194: EXPECT_EQ(3.,
location.accuracy());
On 2012/05/03 19:04:08, Joao da Silva wrote:
> Use EXPECT_DOUBLE_EQ for these 3

Done.

https://chromiumcodereview.appspot.com/10103029/diff/38002/chrome/browser/pol...
chrome/browser/policy/device_status_collector_unittest.cc:205: EXPECT_EQ(1,
location.error_code());
On 2012/05/03 19:04:08, Joao da Silva wrote:
> nit: use the constant instead of 1?

Done.

https://chromiumcodereview.appspot.com/10103029/diff/38002/chrome/common/pref...
File chrome/common/pref_names.cc (right):

https://chromiumcodereview.appspot.com/10103029/diff/38002/chrome/common/pref...
chrome/common/pref_names.cc:1657: extern const char kDeviceLocation[] =
"device_status.location";
On 2012/05/03 19:04:08, Joao da Silva wrote:
> Remove the extern here and in the line above, it only makes sense in
> declarations.

Done.

[Joao da Silva, 2012-05-04 09:23:17]

lgtm

[commit-bot: I haz the power, 2012-05-04 13:57:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bartfab@chromium.org/10103029/47002

[commit-bot: I haz the power, 2012-05-04 15:34:06]

Change committed as 135348