Add method to convert visible networks into Proto

Add method to convert visible networks into Proto
and corresponding tests.

BUG=718475
Review-Url: https://codereview.chromium.org/2884013002
Cr-Commit-Position: refs/heads/master@{#473920}
Committed: https://chromium.googlesource.com/chromium/src/+/e4d6205e5cf8eefe6ec03c630c8b7798aca0c8d8

[lbargu, 2017-05-15 16:45:45]

Description was changed from

==========
Add method to convert visible networks into JSON
and corresponding tests.

BUG=718475
==========

to

==========
Add method to convert visible networks into Proto
and corresponding tests.

BUG=718475
==========

[lbargu, 2017-05-15 16:49:25]

lbargu@google.com changed reviewers:
+ tedchoc@chromium.org

[lbargu, 2017-05-17 15:44:54]

The CQ bit was checked by lbargu@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-17 15:45:43]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-17 17:13:33]

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

[commit-bot: I haz the power, 2017-05-17 17:13:34]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[lbargu, 2017-05-17 17:23:06]

lbargu@google.com changed reviewers:
+ dougt@chromium.org

[Ted C, 2017-05-17 17:44:25]

https://codereview.chromium.org/2884013002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/VisibleNetworks.java
(right):

https://codereview.chromium.org/2884013002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/VisibleNetworks.java:218:
wifi.bssid = bssid();
what is the default value for wifi.bssid?  Is it not null?

Should we just do wifi.bssid = bssid(); ?

If we can't, we should reuse the local var above.

Same general comment as below

If clang format allows it, we should do these on a single line

if (bssid != null) wifi.bssid = bssid;

https://codereview.chromium.org/2884013002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/VisibleNetworks.java:430:
default:
should we just fall through these?

case VisibleCell.UNKNOWN_RADIO_TYPE:
case VisibleCell.UNKNOWN_MISSING_LOCATION_PERMISSION_RADIO_TYPE:
default:

https://codereview.chromium.org/2884013002/diff/20001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/omnibox/geo/VisibleNetworksTest.java
(right):

https://codereview.chromium.org/2884013002/diff/20001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/omnibox/geo/VisibleNetworksTest.java:201:
case VisibleCell.UNKNOWN_RADIO_TYPE:
I "think" this is a bit misleading.  There is only one radio type.  the switch
statement implies we are testing all of them, but we're only testing one.

For now, we should just use the one that is valid.

also, we should use assertEquals instead of assertTrue

[dougt, 2017-05-17 20:51:17]

one general favor - next time, factor out rote/mechnical changes such as
removing this.* into it's on CL that we can rubber stamp.

https://codereview.chromium.org/2884013002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/VisibleNetworks.java
(right):

https://codereview.chromium.org/2884013002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/VisibleNetworks.java:231:
return visibleNetwork;
two questions:

Can any of these accessors return null and is it bad to assign null to values in
the descriptor?  I ask because this code gets alot simpler if either the
accessors never return null or if it is okay to assign null to the descriptor. 
For example:


        public PartnerLocationDescriptor.VisibleNetwork toProto(boolean
connected) {
            PartnerLocationDescriptor.VisibleNetwork visibleNetwork =
                    new PartnerLocationDescriptor.VisibleNetwork();

            PartnerLocationDescriptor.VisibleNetwork.WiFi wifi =
                    new PartnerLocationDescriptor.VisibleNetwork.WiFi();

            wifi.bssid = bssid();
            wifi.levelDbm = level();
            visibleNetwork.wifi = wifi;

            visibleNetwork.timestampMs = timestampMs();
            visibleNetwork.connected = connected;
            return visibleNetwork;
        }

https://codereview.chromium.org/2884013002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/VisibleNetworks.java:437:
cell.cellId = cellId;
same nullable question here.

[lbargu, 2017-05-18 12:42:48]

https://codereview.chromium.org/2884013002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/VisibleNetworks.java
(right):

https://codereview.chromium.org/2884013002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/VisibleNetworks.java:218:
wifi.bssid = bssid();
On 2017/05/17 17:44:25, Ted C wrote:
> what is the default value for wifi.bssid?  Is it not null?
> 
> Should we just do wifi.bssid = bssid(); ?
> 
> If we can't, we should reuse the local var above.
> 
> Same general comment as below
> 
> If clang format allows it, we should do these on a single line
> 
> if (bssid != null) wifi.bssid = bssid;

Sorry, forgot this is nano protos. Should be ok to assign directly. Also added
tests to verify cases where data is missing (empty networks).

https://codereview.chromium.org/2884013002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/VisibleNetworks.java:231:
return visibleNetwork;
On 2017/05/17 20:51:17, dougt wrote:
> two questions:
> 
> Can any of these accessors return null and is it bad to assign null to values
in
> the descriptor?  I ask because this code gets alot simpler if either the
> accessors never return null or if it is okay to assign null to the descriptor.

> For example:
> 
> 
>         public PartnerLocationDescriptor.VisibleNetwork toProto(boolean
> connected) {
>             PartnerLocationDescriptor.VisibleNetwork visibleNetwork =
>                     new PartnerLocationDescriptor.VisibleNetwork();
> 
>             PartnerLocationDescriptor.VisibleNetwork.WiFi wifi =
>                     new PartnerLocationDescriptor.VisibleNetwork.WiFi();
> 
>             wifi.bssid = bssid();
>             wifi.levelDbm = level();
>             visibleNetwork.wifi = wifi;
> 
>             visibleNetwork.timestampMs = timestampMs();
>             visibleNetwork.connected = connected;
>             return visibleNetwork;
>         }

Done.

https://codereview.chromium.org/2884013002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/VisibleNetworks.java:430:
default:
On 2017/05/17 17:44:25, Ted C wrote:
> should we just fall through these?
> 
> case VisibleCell.UNKNOWN_RADIO_TYPE:
> case VisibleCell.UNKNOWN_MISSING_LOCATION_PERMISSION_RADIO_TYPE:
> default:

Done.

https://codereview.chromium.org/2884013002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/VisibleNetworks.java:437:
cell.cellId = cellId;
On 2017/05/17 20:51:17, dougt wrote:
> same nullable question here.

Done.

https://codereview.chromium.org/2884013002/diff/20001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/omnibox/geo/VisibleNetworksTest.java
(right):

https://codereview.chromium.org/2884013002/diff/20001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/omnibox/geo/VisibleNetworksTest.java:201:
case VisibleCell.UNKNOWN_RADIO_TYPE:
On 2017/05/17 17:44:25, Ted C wrote:
> I "think" this is a bit misleading.  There is only one radio type.  the switch
> statement implies we are testing all of them, but we're only testing one.
> 
> For now, we should just use the one that is valid.
> 
> also, we should use assertEquals instead of assertTrue

Done.

[lbargu, 2017-05-18 12:43:04]

The CQ bit was checked by lbargu@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-18 12:43:31]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-18 15:05:38]

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

[commit-bot: I haz the power, 2017-05-18 15:05:40]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[lbargu, 2017-05-19 15:24:26]

The CQ bit was checked by lbargu@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-19 15:25:03]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-19 16:55:10]

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

[commit-bot: I haz the power, 2017-05-19 16:55:11]

Dry run: This issue passed the CQ dry run.

[lbargu, 2017-05-22 15:10:32]

The CQ bit was checked by lbargu@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-22 15:10:55]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-22 16:51:05]

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

[commit-bot: I haz the power, 2017-05-22 16:51:06]

Dry run: This issue passed the CQ dry run.

[lbargu, 2017-05-23 10:07:31]

Friendly ping. All comments resolved.

[Ted C, 2017-05-23 14:18:38]

lgtm

https://codereview.chromium.org/2884013002/diff/80001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/omnibox/geo/VisibleNetworksTest.java
(right):

https://codereview.chromium.org/2884013002/diff/80001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/omnibox/geo/VisibleNetworksTest.java:238:
assertEquals(new Integer(PartnerLocationDescriptor.VisibleNetwork.Cell.GSM),
cell.type);
do you need this "new Integer("?  it should autobox them to capital I integers
for assertEquals to work

[lbargu, 2017-05-23 14:36:47]

Thanks for the review!

https://codereview.chromium.org/2884013002/diff/80001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/omnibox/geo/VisibleNetworksTest.java
(right):

https://codereview.chromium.org/2884013002/diff/80001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/omnibox/geo/VisibleNetworksTest.java:238:
assertEquals(new Integer(PartnerLocationDescriptor.VisibleNetwork.Cell.GSM),
cell.type);
On 2017/05/23 14:18:38, Ted C wrote:
> do you need this "new Integer("?  it should autobox them to capital I integers
> for assertEquals to work

Compiler complains about ambiguous use of assertEquals (with ints vs Objects).
Using intValue in proto instead.

[lbargu, 2017-05-23 14:36:57]

The CQ bit was checked by lbargu@google.com

[lbargu, 2017-05-23 14:36:58]

The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org
Link to the patchset: https://codereview.chromium.org/2884013002/#ps100001
(title: "Add method to convert visible networks into Proto and corresponding
tests.")

[commit-bot: I haz the power, 2017-05-23 14:37:16]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-23 16:03:33]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1495550217515720,
"parent_rev": "250918d769bb378b1152db5176a642499fbe4146", "commit_rev":
"e4d6205e5cf8eefe6ec03c630c8b7798aca0c8d8"}

[commit-bot: I haz the power, 2017-05-23 16:03:44]

Description was changed from

==========
Add method to convert visible networks into Proto
and corresponding tests.

BUG=718475
==========

to

==========
Add method to convert visible networks into Proto
and corresponding tests.

BUG=718475

Review-Url: https://codereview.chromium.org/2884013002
Cr-Commit-Position: refs/heads/master@{#473920}
Committed:
https://chromium.googlesource.com/chromium/src/+/e4d6205e5cf8eefe6ec03c630c8b...
==========

[commit-bot: I haz the power, 2017-05-23 16:03:45]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/e4d6205e5cf8eefe6ec03c630c8b...