SimpleGeolocation should support sending WiFi AP data.

SimpleGeolocation should support sending WiFi AP data.

BUG=596690
TEST=unittest

Committed: https://crrev.com/9af1adea25a1a643a7f849616d3f51b33a9657aa
Cr-Commit-Position: refs/heads/master@{#382968}

[Alexander Alekseev, 2016-03-22 06:19:15]

Description was changed from

==========
SimpleGeolocation should support sending WiFi AP data.

BUG=596690
TEST=manual
==========

to

==========
SimpleGeolocation should support sending WiFi AP data.

BUG=596690
TEST=unittest
==========

[Alexander Alekseev, 2016-03-22 06:19:23]

The CQ bit was checked by alemate@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-22 06:19:32]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1819173002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1819173002/20001

[Alexander Alekseev, 2016-03-22 06:56:59]

The CQ bit was checked by alemate@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-22 06:57:08]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1819173002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1819173002/40001

[Alexander Alekseev, 2016-03-22 07:00:09]

The CQ bit was checked by alemate@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-22 07:00:20]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1819173002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1819173002/60001

[Alexander Alekseev, 2016-03-22 07:22:48]

alemate@chromium.org changed reviewers:
+ holte@chromium.org, stevenjb@chromium.org

[Alexander Alekseev, 2016-03-22 07:22:48]

Please review:

holte@:
  tools/metrics/histograms.xml

stevenjb@:
  all

[Alexander Alekseev, 2016-03-22 07:30:19]

The CQ bit was checked by alemate@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-22 07:30:26]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1819173002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1819173002/80001

[commit-bot: I haz the power, 2016-03-22 08:56:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-22 08:56:41]

Dry run: This issue passed the CQ dry run.

[stevenjb, 2016-03-22 16:39:49]

https://codereview.chromium.org/1819173002/diff/80001/chromeos/geolocation/si...
File chromeos/geolocation/simple_geolocation_provider.cc (right):

https://codereview.chromium.org/1819173002/diff/80001/chromeos/geolocation/si...
chromeos/geolocation/simple_geolocation_provider.cc:28:
chromeos::WifiAccessPointVector access_points;
Rather than allocating this locally and copying the result below, use
scoped_ptr<WAPV>(new WAPV) here and pass the pointer to |result| on success.

https://codereview.chromium.org/1819173002/diff/80001/chromeos/geolocation/si...
chromeos/geolocation/simple_geolocation_provider.cc:59: :
scoped_ptr<WifiAccessPointVector>()));
: nullptr

https://codereview.chromium.org/1819173002/diff/80001/chromeos/geolocation/si...
File chromeos/geolocation/simple_geolocation_request.cc (right):

https://codereview.chromium.org/1819173002/diff/80001/chromeos/geolocation/si...
chromeos/geolocation/simple_geolocation_request.cc:49: // const char kCarrier[] 
             = "carrier";
Remove?

https://codereview.chromium.org/1819173002/diff/80001/chromeos/geolocation/si...
chromeos/geolocation/simple_geolocation_request.cc:347: (base::Time::Now() -
access_point.timestamp).InMilliseconds()));
{}

https://codereview.chromium.org/1819173002/diff/80001/chromeos/geolocation/si...
chromeos/geolocation/simple_geolocation_request.cc:385: // Call test hook before
s/before/first./ ?

https://codereview.chromium.org/1819173002/diff/80001/chromeos/geolocation/si...
File chromeos/geolocation/simple_geolocation_request.h (right):

https://codereview.chromium.org/1819173002/diff/80001/chromeos/geolocation/si...
chromeos/geolocation/simple_geolocation_request.h:54: // If wifi_data is not
NULL, it will be sent to the geolocation server.
s/NULL/null/

https://codereview.chromium.org/1819173002/diff/80001/chromeos/geolocation/si...
File chromeos/geolocation/simple_geolocation_unittest.cc (right):

https://codereview.chromium.org/1819173002/diff/80001/chromeos/geolocation/si...
chromeos/geolocation/simple_geolocation_unittest.cc:289:
SimpleGeolocationProvider provider(NULL, GURL(kTestGeolocationProviderUrl));
s/NULL/nullptr/ throughout.

https://codereview.chromium.org/1819173002/diff/80001/chromeos/geolocation/si...
chromeos/geolocation/simple_geolocation_unittest.cc:428: "}",
requests_monitor.last_request_body());
Declare a constant for human readable strings.

https://codereview.chromium.org/1819173002/diff/80001/chromeos/network/geoloc...
File chromeos/network/geolocation_handler.h (right):

https://codereview.chromium.org/1819173002/diff/80001/chromeos/network/geoloc...
chromeos/network/geolocation_handler.h:55: friend class
SimpleGeolocationWiFiTest;
blank line

https://codereview.chromium.org/1819173002/diff/80001/chromeos/timezone/timez...
File chromeos/timezone/timezone_resolver.h (right):

https://codereview.chromium.org/1819173002/diff/80001/chromeos/timezone/timez...
chromeos/timezone/timezone_resolver.h:82: bool
send_wifi_data_to_geolocation_api_ = false;
Initialize all members inline or none.

[Alexander Alekseev, 2016-03-23 00:04:46]

https://codereview.chromium.org/1819173002/diff/80001/chromeos/geolocation/si...
File chromeos/geolocation/simple_geolocation_provider.cc (right):

https://codereview.chromium.org/1819173002/diff/80001/chromeos/geolocation/si...
chromeos/geolocation/simple_geolocation_provider.cc:28:
chromeos::WifiAccessPointVector access_points;
On 2016/03/22 16:39:49, stevenjb wrote:
> Rather than allocating this locally and copying the result below, use
> scoped_ptr<WAPV>(new WAPV) here and pass the pointer to |result| on success.

Done.

https://codereview.chromium.org/1819173002/diff/80001/chromeos/geolocation/si...
chromeos/geolocation/simple_geolocation_provider.cc:59: :
scoped_ptr<WifiAccessPointVector>()));
On 2016/03/22 16:39:49, stevenjb wrote:
> : nullptr

Done.

https://codereview.chromium.org/1819173002/diff/80001/chromeos/geolocation/si...
File chromeos/geolocation/simple_geolocation_request.cc (right):

https://codereview.chromium.org/1819173002/diff/80001/chromeos/geolocation/si...
chromeos/geolocation/simple_geolocation_request.cc:49: // const char kCarrier[] 
             = "carrier";
On 2016/03/22 16:39:49, stevenjb wrote:
> Remove?

Done.

https://codereview.chromium.org/1819173002/diff/80001/chromeos/geolocation/si...
chromeos/geolocation/simple_geolocation_request.cc:347: (base::Time::Now() -
access_point.timestamp).InMilliseconds()));
On 2016/03/22 16:39:49, stevenjb wrote:
> {}

Done.

https://codereview.chromium.org/1819173002/diff/80001/chromeos/geolocation/si...
chromeos/geolocation/simple_geolocation_request.cc:385: // Call test hook before
On 2016/03/22 16:39:49, stevenjb wrote:
> s/before/first./ ?

This is only half of the comment I was about to write ;)

Done.

https://codereview.chromium.org/1819173002/diff/80001/chromeos/geolocation/si...
File chromeos/geolocation/simple_geolocation_request.h (right):

https://codereview.chromium.org/1819173002/diff/80001/chromeos/geolocation/si...
chromeos/geolocation/simple_geolocation_request.h:54: // If wifi_data is not
NULL, it will be sent to the geolocation server.
On 2016/03/22 16:39:49, stevenjb wrote:
> s/NULL/null/

Done.

https://codereview.chromium.org/1819173002/diff/80001/chromeos/geolocation/si...
File chromeos/geolocation/simple_geolocation_unittest.cc (right):

https://codereview.chromium.org/1819173002/diff/80001/chromeos/geolocation/si...
chromeos/geolocation/simple_geolocation_unittest.cc:289:
SimpleGeolocationProvider provider(NULL, GURL(kTestGeolocationProviderUrl));
On 2016/03/22 16:39:49, stevenjb wrote:
> s/NULL/nullptr/ throughout.

Done.

https://codereview.chromium.org/1819173002/diff/80001/chromeos/geolocation/si...
chromeos/geolocation/simple_geolocation_unittest.cc:428: "}",
requests_monitor.last_request_body());
On 2016/03/22 16:39:49, stevenjb wrote:
> Declare a constant for human readable strings.

Done.

https://codereview.chromium.org/1819173002/diff/80001/chromeos/network/geoloc...
File chromeos/network/geolocation_handler.h (right):

https://codereview.chromium.org/1819173002/diff/80001/chromeos/network/geoloc...
chromeos/network/geolocation_handler.h:55: friend class
SimpleGeolocationWiFiTest;
On 2016/03/22 16:39:49, stevenjb wrote:
> blank line

Done.

https://codereview.chromium.org/1819173002/diff/80001/chromeos/timezone/timez...
File chromeos/timezone/timezone_resolver.h (right):

https://codereview.chromium.org/1819173002/diff/80001/chromeos/timezone/timez...
chromeos/timezone/timezone_resolver.h:82: bool
send_wifi_data_to_geolocation_api_ = false;
On 2016/03/22 16:39:49, stevenjb wrote:
> Initialize all members inline or none.

Done.

[Alexander Alekseev, 2016-03-23 00:04:59]

The CQ bit was checked by alemate@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-23 00:05:20]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1819173002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1819173002/100001

[stevenjb, 2016-03-23 00:31:08]

lgtm

[commit-bot: I haz the power, 2016-03-23 01:45:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-23 01:45:55]

Dry run: This issue passed the CQ dry run.

[Alexander Alekseev, 2016-03-23 02:26:39]

holte@ , Friendly ping.

[Steven Holte, 2016-03-23 18:41:46]

https://codereview.chromium.org/1819173002/diff/100001/chromeos/geolocation/s...
File chromeos/geolocation/simple_geolocation_request.cc (right):

https://codereview.chromium.org/1819173002/diff/100001/chromeos/geolocation/s...
chromeos/geolocation/simple_geolocation_request.cc:319:
UMA_HISTOGRAM_BOOLEAN("SimpleGeolocation.Request.HasWiFiAccessPoints",
It would be better to wrap a function call around this macro invocation, since
it will create a static variable pointing at the Histogram object.

[Steven Holte, 2016-03-23 18:42:35]

lgtm % wrapping a function around the macro

[Alexander Alekseev, 2016-03-23 22:08:23]

Thank you!

https://codereview.chromium.org/1819173002/diff/100001/chromeos/geolocation/s...
File chromeos/geolocation/simple_geolocation_request.cc (right):

https://codereview.chromium.org/1819173002/diff/100001/chromeos/geolocation/s...
chromeos/geolocation/simple_geolocation_request.cc:319:
UMA_HISTOGRAM_BOOLEAN("SimpleGeolocation.Request.HasWiFiAccessPoints",
On 2016/03/23 18:41:46, Steven Holte wrote:
> It would be better to wrap a function call around this macro invocation, since
> it will create a static variable pointing at the Histogram object.

Done.

[Alexander Alekseev, 2016-03-23 22:08:32]

The CQ bit was checked by alemate@chromium.org

[Alexander Alekseev, 2016-03-23 22:08:33]

The patchset sent to the CQ was uploaded after l-g-t-m from
stevenjb@chromium.org, holte@chromium.org
Link to the patchset: https://codereview.chromium.org/1819173002/#ps120001
(title: "Update after review.")

[commit-bot: I haz the power, 2016-03-23 22:09:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1819173002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1819173002/120001

[commit-bot: I haz the power, 2016-03-23 23:21:52]

Description was changed from

==========
SimpleGeolocation should support sending WiFi AP data.

BUG=596690
TEST=unittest
==========

to

==========
SimpleGeolocation should support sending WiFi AP data.

BUG=596690
TEST=unittest
==========

[commit-bot: I haz the power, 2016-03-23 23:21:53]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-03-23 23:23:17]

Description was changed from

==========
SimpleGeolocation should support sending WiFi AP data.

BUG=596690
TEST=unittest
==========

to

==========
SimpleGeolocation should support sending WiFi AP data.

BUG=596690
TEST=unittest

Committed: https://crrev.com/9af1adea25a1a643a7f849616d3f51b33a9657aa
Cr-Commit-Position: refs/heads/master@{#382968}
==========

[commit-bot: I haz the power, 2016-03-23 23:23:18]

Patchset 7 (id:??) landed as
https://crrev.com/9af1adea25a1a643a7f849616d3f51b33a9657aa
Cr-Commit-Position: refs/heads/master@{#382968}