Enable lost URL detection in the Physical Web scanner

Enable lost URL detection in the Physical Web scanner

The Physical Web scanner was initially designed to run for a relatively
short duration (less than a minute) and would report any URLs seen during
this time as nearby. With this change, the scanner will keep track of how
recently a URL was seen so that URLs that are no longer nearby can be
removed from the list.

BUG=630769
Committed: https://crrev.com/7afb153e71eee978830f552c3002348b9c9501d5
Cr-Commit-Position: refs/heads/master@{#426859}

[mattreynolds, 2016-10-13 00:55:36]

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

[mattreynolds, 2016-10-13 00:55:37]

Hi Olivier, PTAL

[Olivier, 2016-10-13 13:00:19]

https://codereview.chromium.org/2413923002/diff/1/ios/chrome/common/physical_...
File ios/chrome/common/physical_web/physical_web_scanner.mm (right):

https://codereview.chromium.org/2413923002/diff/1/ios/chrome/common/physical_...
ios/chrome/common/physical_web/physical_web_scanner.mm:223: [self start];
This will reset all the lists. Is that expected?
Should we document this?

https://codereview.chromium.org/2413923002/diff/1/ios/chrome/common/physical_...
ios/chrome/common/physical_web/physical_web_scanner.mm:314: [unresolvedDevices_
removeObjectsInArray:lostUnresolvedDevices];
should we cancel the resolve URL query?

https://codereview.chromium.org/2413923002/diff/1/ios/chrome/common/physical_...
ios/chrome/common/physical_web/physical_web_scanner.mm:321: // For unknown
reasons, when scanning for longer periods (on the order of
Can you file a radar on this, file a crbug that reference the radar?

https://codereview.chromium.org/2413923002/diff/1/ios/chrome/common/physical_...
ios/chrome/common/physical_web/physical_web_scanner.mm:398: [scanTimestamps_
setObject:[NSDate date] forKey:[device requestURL]];
Would it make sense to add this to the device object instead?

[mattreynolds, 2016-10-18 18:56:47]

https://codereview.chromium.org/2413923002/diff/1/ios/chrome/common/physical_...
File ios/chrome/common/physical_web/physical_web_scanner.mm (right):

https://codereview.chromium.org/2413923002/diff/1/ios/chrome/common/physical_...
ios/chrome/common/physical_web/physical_web_scanner.mm:223: [self start];
On 2016/10/13 13:00:19, Olivier Robin wrote:
> This will reset all the lists. Is that expected?
> Should we document this?

I added a comment documenting the behavior

https://codereview.chromium.org/2413923002/diff/1/ios/chrome/common/physical_...
ios/chrome/common/physical_web/physical_web_scanner.mm:314: [unresolvedDevices_
removeObjectsInArray:lostUnresolvedDevices];
On 2016/10/13 13:00:19, Olivier Robin wrote:
> should we cancel the resolve URL query?

Done.

https://codereview.chromium.org/2413923002/diff/1/ios/chrome/common/physical_...
ios/chrome/common/physical_web/physical_web_scanner.mm:321: // For unknown
reasons, when scanning for longer periods (on the order of
On 2016/10/13 13:00:19, Olivier Robin wrote:
> Can you file a radar on this, file a crbug that reference the radar?

Done.

https://codereview.chromium.org/2413923002/diff/1/ios/chrome/common/physical_...
ios/chrome/common/physical_web/physical_web_scanner.mm:398: [scanTimestamps_
setObject:[NSDate date] forKey:[device requestURL]];
On 2016/10/13 13:00:19, Olivier Robin wrote:
> Would it make sense to add this to the device object instead?

Done.

[Olivier, 2016-10-19 10:22:54]

One question, nits, then looks good.

https://codereview.chromium.org/2413923002/diff/20001/ios/chrome/common/physi...
File ios/chrome/common/physical_web/physical_web_scanner.mm (right):

https://codereview.chromium.org/2413923002/diff/20001/ios/chrome/common/physi...
ios/chrome/common/physical_web/physical_web_scanner.mm:57: -
(void)onUpdate:(NSTimer*)timer;
NIT: rename. Update means there is an update.
Rename to 
onUpdateTimeElapsed:
or something equivalent.

https://codereview.chromium.org/2413923002/diff/20001/ios/chrome/common/physi...
ios/chrome/common/physical_web/physical_web_scanner.mm:216: BOOL
oldOnLostDetectionEnabled = onLostDetectionEnabled_;
nit:
if (enabled == onLostDetectionEnabled_)
  return;

then consider there is an update.

https://codereview.chromium.org/2413923002/diff/20001/ios/chrome/common/physi...
ios/chrome/common/physical_web/physical_web_scanner.mm:258:
CBCentralManagerScanOptionAllowDuplicatesKey, nil];
Question: 
Is this needed if we do the workaround?
Can we save battery by not allowing duplicates?

https://codereview.chromium.org/2413923002/diff/20001/ios/chrome/common/physi...
ios/chrome/common/physical_web/physical_web_scanner.mm:323: // Workaround for
crbug.com/657056
format
// TODO(crbug.com/657056): Remove this workaround when radar is fixed.

[mattreynolds, 2016-10-19 18:20:22]

https://codereview.chromium.org/2413923002/diff/20001/ios/chrome/common/physi...
File ios/chrome/common/physical_web/physical_web_scanner.mm (right):

https://codereview.chromium.org/2413923002/diff/20001/ios/chrome/common/physi...
ios/chrome/common/physical_web/physical_web_scanner.mm:57: -
(void)onUpdate:(NSTimer*)timer;
On 2016/10/19 10:22:54, Olivier Robin wrote:
> NIT: rename. Update means there is an update.
> Rename to 
> onUpdateTimeElapsed:
> or something equivalent.

Done.

https://codereview.chromium.org/2413923002/diff/20001/ios/chrome/common/physi...
ios/chrome/common/physical_web/physical_web_scanner.mm:216: BOOL
oldOnLostDetectionEnabled = onLostDetectionEnabled_;
On 2016/10/19 10:22:54, Olivier Robin wrote:
> nit:
> if (enabled == onLostDetectionEnabled_)
>   return;
> 
> then consider there is an update.

Done.

https://codereview.chromium.org/2413923002/diff/20001/ios/chrome/common/physi...
ios/chrome/common/physical_web/physical_web_scanner.mm:258:
CBCentralManagerScanOptionAllowDuplicatesKey, nil];
On 2016/10/19 10:22:54, Olivier Robin wrote:
> Question: 
> Is this needed if we do the workaround?
> Can we save battery by not allowing duplicates?

I ran some tests and it seems it's not needed if we're restarting the scanner
frequently.

https://codereview.chromium.org/2413923002/diff/20001/ios/chrome/common/physi...
ios/chrome/common/physical_web/physical_web_scanner.mm:323: // Workaround for
crbug.com/657056
On 2016/10/19 10:22:54, Olivier Robin wrote:
> format
> // TODO(crbug.com/657056): Remove this workaround when radar is fixed.

Done.

https://codereview.chromium.org/2413923002/diff/20001/ios/chrome/common/physi...
ios/chrome/common/physical_web/physical_web_scanner.mm:404: if ([devicesUrls_
containsObject:[device requestURL]])
found a bug: if the URL has already been seen, we won't update the timestamp.

[Olivier, 2016-10-20 07:05:21]

https://codereview.chromium.org/2413923002/diff/20001/ios/chrome/common/physi...
File ios/chrome/common/physical_web/physical_web_scanner.mm (right):

https://codereview.chromium.org/2413923002/diff/20001/ios/chrome/common/physi...
ios/chrome/common/physical_web/physical_web_scanner.mm:404: if ([devicesUrls_
containsObject:[device requestURL]])
On 2016/10/19 18:20:21, mattreynolds wrote:
> found a bug: if the URL has already been seen, we won't update the timestamp.

I don't get it. 
You update the timestamp on the line above.
By removing this, won't you issue a new network request?

[mattreynolds, 2016-10-20 22:03:28]

https://codereview.chromium.org/2413923002/diff/20001/ios/chrome/common/physi...
File ios/chrome/common/physical_web/physical_web_scanner.mm (right):

https://codereview.chromium.org/2413923002/diff/20001/ios/chrome/common/physi...
ios/chrome/common/physical_web/physical_web_scanner.mm:404: if ([devicesUrls_
containsObject:[device requestURL]])
On 2016/10/20 07:05:21, Olivier Robin wrote:
> On 2016/10/19 18:20:21, mattreynolds wrote:
> > found a bug: if the URL has already been seen, we won't update the
timestamp.
> 
> I don't get it. 
> You update the timestamp on the line above.
> By removing this, won't you issue a new network request?

If this is the first time we've seen the device's requestURL then it will work
fine.  But suppose we've already seen the requestURL, then it will be in
devicesUrls_.  We'll exit early and discard the device we set the timestamp on. 
Instead, we need to reach into devices_ (or unresolvedDevices_ if it's
unresolved) and set the timestamp on the pre-existing PhysicalWebDevice.

[Olivier, 2016-10-21 09:22:14]

lgtm

https://codereview.chromium.org/2413923002/diff/40001/ios/chrome/common/physi...
File ios/chrome/common/physical_web/physical_web_scanner.mm (right):

https://codereview.chromium.org/2413923002/diff/40001/ios/chrome/common/physi...
ios/chrome/common/physical_web/physical_web_scanner.mm:278: for
(PhysicalWebDevice* device in [self devices]) {
You don't need to sort here. May be use devices_.get()

https://codereview.chromium.org/2413923002/diff/40001/ios/chrome/common/physi...
ios/chrome/common/physical_web/physical_web_scanner.mm:393: [unresolvedDevice
setScanTimestamp:[NSDate date]];
break;
or return; ?

[Olivier, 2016-10-21 09:22:15]

lgtm

lgtm

[mattreynolds, 2016-10-21 18:16:05]

Thanks Olivier!

https://codereview.chromium.org/2413923002/diff/40001/ios/chrome/common/physi...
File ios/chrome/common/physical_web/physical_web_scanner.mm (right):

https://codereview.chromium.org/2413923002/diff/40001/ios/chrome/common/physi...
ios/chrome/common/physical_web/physical_web_scanner.mm:278: for
(PhysicalWebDevice* device in [self devices]) {
On 2016/10/21 09:22:13, Olivier Robin wrote:
> You don't need to sort here. May be use devices_.get()

Done.

https://codereview.chromium.org/2413923002/diff/40001/ios/chrome/common/physi...
ios/chrome/common/physical_web/physical_web_scanner.mm:393: [unresolvedDevice
setScanTimestamp:[NSDate date]];
On 2016/10/21 09:22:14, Olivier Robin wrote:
> break;
> or return; ?

Done.

[mattreynolds, 2016-10-21 18:16:23]

The CQ bit was checked by mattreynolds@chromium.org

[mattreynolds, 2016-10-21 18:16:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
olivierrobin@chromium.org
Link to the patchset: https://codereview.chromium.org/2413923002/#ps60001
(title: "fixes")

[commit-bot: I haz the power, 2016-10-21 18:16:51]

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, 2016-10-21 19:17:03]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-10-21 19:44:50]

Description was changed from

==========
Enable lost URL detection in the Physical Web scanner

The Physical Web scanner was initially designed to run for a relatively
short duration (less than a minute) and would report any URLs seen during
this time as nearby. With this change, the scanner will keep track of how
recently a URL was seen so that URLs that are no longer nearby can be
removed from the list.

BUG=630769
==========

to

==========
Enable lost URL detection in the Physical Web scanner

The Physical Web scanner was initially designed to run for a relatively
short duration (less than a minute) and would report any URLs seen during
this time as nearby. With this change, the scanner will keep track of how
recently a URL was seen so that URLs that are no longer nearby can be
removed from the list.

BUG=630769

Committed: https://crrev.com/7afb153e71eee978830f552c3002348b9c9501d5
Cr-Commit-Position: refs/heads/master@{#426859}
==========

[commit-bot: I haz the power, 2016-10-21 19:44:51]

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