Associate a scan mode with Physical Web listeners

Associate a scan mode with Physical Web listeners

Instead of having a blanket scanning policy, it would be nice if client
code could specify the kind of scan required.

This change lets Physical Web listeners specify a scan mode when they
register.  This mode is recorded, but not currently used.  A followup
CL will surface this to the platform layer where Android/iOS can use it
appropriately.

BUG=703308
Review-Url: https://codereview.chromium.org/2765713002
Cr-Commit-Position: refs/heads/master@{#459507}
Committed: https://chromium.googlesource.com/chromium/src/+/1c16fca164db734cdafd0a428423392dbd3a93ab

[cco3, 2017-03-21 01:07:12]

cco3@chromium.org changed reviewers:
+ mattreynolds@chromium.org

[cco3, 2017-03-21 01:07:12]

Hi Matt.  I have a followup CL to use the data when this one goes it.

[mattreynolds, 2017-03-21 19:02:56]

https://codereview.chromium.org/2765713002/diff/1/components/physical_web/dat...
File components/physical_web/data_source/physical_web_data_source.h (right):

https://codereview.chromium.org/2765713002/diff/1/components/physical_web/dat...
components/physical_web/data_source/physical_web_data_source.h:26:
BACKGROUND_INTERMITTENT = 1 << 0,
What should we do if a client specifies an invalid scan mode? (eg,
BACKGROUND_INTERMITTENT on iOS). I think it would be reasonable to fall back to
OPPORTUNISTIC, but it might be better to emit an unsupported mode error.

https://codereview.chromium.org/2765713002/diff/1/components/physical_web/dat...
File components/physical_web/data_source/physical_web_data_source_impl.cc
(left):

https://codereview.chromium.org/2765713002/diff/1/components/physical_web/dat...
components/physical_web/data_source/physical_web_data_source_impl.cc:18:
observer_list_.AddObserver(physical_web_listener);
ObserverList says observers shouldn't be added more than once. Can you add a
check here to ensure we won't double-add when changing the scan mode?

[cco3, 2017-03-21 19:28:45]

https://codereview.chromium.org/2765713002/diff/1/components/physical_web/dat...
File components/physical_web/data_source/physical_web_data_source.h (right):

https://codereview.chromium.org/2765713002/diff/1/components/physical_web/dat...
components/physical_web/data_source/physical_web_data_source.h:26:
BACKGROUND_INTERMITTENT = 1 << 0,
On 2017/03/21 19:02:56, mattreynolds wrote:
> What should we do if a client specifies an invalid scan mode? (eg,
> BACKGROUND_INTERMITTENT on iOS). I think it would be reasonable to fall back
to
> OPPORTUNISTIC, but it might be better to emit an unsupported mode error.

I don't think a fall-back is necessary.  These are just policies that may be
requested, like hints.  If we get one we don't want to acknowledge, we just
ignore it.

https://codereview.chromium.org/2765713002/diff/1/components/physical_web/dat...
File components/physical_web/data_source/physical_web_data_source_impl.cc
(left):

https://codereview.chromium.org/2765713002/diff/1/components/physical_web/dat...
components/physical_web/data_source/physical_web_data_source_impl.cc:18:
observer_list_.AddObserver(physical_web_listener);
On 2017/03/21 19:02:56, mattreynolds wrote:
> ObserverList says observers shouldn't be added more than once. Can you add a
> check here to ensure we won't double-add when changing the scan mode?

O snap.

[mattreynolds, 2017-03-21 20:21:12]

lgtm

[cco3, 2017-03-21 20:21:34]

cco3@chromium.org changed reviewers:
+ dtrainor@chromium.org

[cco3, 2017-03-21 20:21:35]

Hi David, would you be able to take a look at this change?  Thanks!

[David Trainor- moved to gerrit, 2017-03-22 03:14:12]

lgtm

[cco3, 2017-03-23 18:03:21]

The CQ bit was checked by cco3@chromium.org

[commit-bot: I haz the power, 2017-03-23 18:04:38]

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-03-23 18:15:11]

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

[commit-bot: I haz the power, 2017-03-23 18:15:13]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[cco3, 2017-03-23 19:34:32]

cco3@chromium.org changed reviewers:
+ vitaliii@chromium.org

[cco3, 2017-03-23 19:34:33]

Hi Vitali, could I get our LGTM on the one-liner in components/ntp_snippets?

[cco3, 2017-03-23 19:34:53]

On 2017/03/23 19:34:33, cco3 wrote:
> Hi Vitali, could I get our LGTM on the one-liner in components/ntp_snippets?

your*

[vitaliii, 2017-03-24 09:41:49]

One nit in physical_web_data_source_impl.cc

LGTM for components/ntp_snippets.

https://codereview.chromium.org/2765713002/diff/60001/components/physical_web...
File components/physical_web/data_source/physical_web_data_source_impl.cc
(right):

https://codereview.chromium.org/2765713002/diff/60001/components/physical_web...
components/physical_web/data_source/physical_web_data_source_impl.cc:29:
scan_modes_.erase(scan_modes_.find(physical_web_listener));
Nit: unordered_map.erase seems to accept key type as a parameter (see (3) in
http://en.cppreference.com/w/cpp/container/unordered_map/erase), so find here
seems redundant.

[cco3, 2017-03-24 18:00:37]

The CQ bit was checked by cco3@chromium.org

[cco3, 2017-03-24 18:00:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtrainor@chromium.org, vitaliii@chromium.org, mattreynolds@chromium.org
Link to the patchset: https://codereview.chromium.org/2765713002/#ps80001
(title: "Use key as parame to erase()")

[commit-bot: I haz the power, 2017-03-24 18:01:19]

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-03-24 19:09:58]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1490378437038120,
"parent_rev": "adbce8ba658534687a82e731ef138678f116f3a2", "commit_rev":
"1c16fca164db734cdafd0a428423392dbd3a93ab"}

[commit-bot: I haz the power, 2017-03-24 19:10:47]

Description was changed from

==========
Associate a scan mode with Physical Web listeners

Instead of having a blanket scanning policy, it would be nice if client
code could specify the kind of scan required.

This change lets Physical Web listeners specify a scan mode when they
register.  This mode is recorded, but not currently used.  A followup
CL will surface this to the platform layer where Android/iOS can use it
appropriately.

BUG=703308
==========

to

==========
Associate a scan mode with Physical Web listeners

Instead of having a blanket scanning policy, it would be nice if client
code could specify the kind of scan required.

This change lets Physical Web listeners specify a scan mode when they
register.  This mode is recorded, but not currently used.  A followup
CL will surface this to the platform layer where Android/iOS can use it
appropriately.

BUG=703308

Review-Url: https://codereview.chromium.org/2765713002
Cr-Commit-Position: refs/heads/master@{#459507}
Committed:
https://chromium.googlesource.com/chromium/src/+/1c16fca164db734cdafd0a428423...
==========

[commit-bot: I haz the power, 2017-03-24 19:10:48]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/1c16fca164db734cdafd0a428423...