Enable lost URL detection in the Physical Web scanner

ios/chrome/common/physical_web/physical_web_scanner.mm

Index: ios/chrome/common/physical_web/physical_web_scanner.mm
diff --git a/ios/chrome/common/physical_web/physical_web_scanner.mm b/ios/chrome/common/physical_web/physical_web_scanner.mm
index 02c5d0c0c03d6f1fb5c979a23eb6c11217c30bd8..a7c7184db4def9cd2af8ff4ac22b766fa48b7334 100644
--- a/ios/chrome/common/physical_web/physical_web_scanner.mm
+++ b/ios/chrome/common/physical_web/physical_web_scanner.mm
@@ -25,6 +25,12 @@ namespace {
 NSString* const kUriBeaconServiceUUID = @"FED8";
 NSString* const kEddystoneBeaconServiceUUID = @"FEAA";
 
+// The length of time in seconds since a URL was last seen before it should be
+// considered lost (ie, no longer nearby).
+const NSTimeInterval kLostThresholdSeconds = 15.0;
+// The time interval in seconds between checks for lost URLs.
+const NSTimeInterval kUpdateIntervalSeconds = 6.0;
+
 enum BeaconType {
   BEACON_TYPE_NONE,
   BEACON_TYPE_URIBEACON,
@@ -41,8 +47,14 @@ enum BeaconType {
 + (PhysicalWebDevice*)newDeviceFromData:(NSData*)data
                                    rssi:(int)rssi
                                    type:(BeaconType)type;
-// Starts the CoreBluetooth scanner when the bluetooth is powered on.
+// Starts the CoreBluetooth scanner when the bluetooth is powered on and starts
+// the update timer.
 - (void)reallyStart;
+// Stops the CoreBluetooth scanner and update timer.
+- (void)reallyStop;
+// Timer callback to check for lost URLs based on the elapsed time since they
+// were last seen.
+- (void)onUpdate:(NSTimer*)timer;

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

NIT: rename. Update means there is an update.
Rename to 
onUpdateTimeElapsed:
or something equivalent.

[mattreynolds, 2016/10/19 18:20:21]

Done.

 // Requests metadata of a device if the same URL has not been requested before.
 - (void)requestMetadataForDevice:(PhysicalWebDevice*)device;
 // Returns the beacon type given the advertisement data.
@@ -72,12 +84,19 @@ enum BeaconType {
   base::scoped_nsobject<NSMutableSet> finalUrls_;
   // CoreBluetooth scanner.
   base::scoped_nsobject<CBCentralManager> centralManager_;
+  // When YES, we will notify the delegate if a previously nearby URL is lost
+  // and remove it from the list of nearby devices.
+  BOOL onLostDetectionEnabled_;
   // The value is YES if network requests can be sent.
   BOOL networkRequestEnabled_;
   // List of unresolved PhysicalWebDevice when network requests are not enabled.
   base::scoped_nsobject<NSMutableArray> unresolvedDevices_;
+  // A repeating timer to check for lost URLs. If the elapsed time since an URL
+  // was last seen exceeds a threshold, the URL is considered lost.
+  base::scoped_nsobject<NSTimer> updateTimer_;
 }
 
+@synthesize onLostDetectionEnabled = onLostDetectionEnabled_;
 @synthesize networkRequestEnabled = networkRequestEnabled_;
 
 - (instancetype)initWithDelegate:(id<PhysicalWebScannerDelegate>)delegate {
@@ -106,6 +125,10 @@ enum BeaconType {
 - (void)dealloc {
   [centralManager_ setDelegate:nil];
   centralManager_.reset();
+  if (updateTimer_.get()) {
+    [updateTimer_ invalidate];
+    updateTimer_.reset();
+  }
   [super dealloc];
 }
 
@@ -129,7 +152,7 @@ enum BeaconType {
   }
   [pendingRequests_ removeAllObjects];
   if (!pendingStart_ && [self bluetoothEnabled]) {
-    [centralManager_ stopScan];
+    [self reallyStop];
   }
   pendingStart_ = NO;
   started_ = NO;
@@ -189,6 +212,14 @@ enum BeaconType {
   [unresolvedDevices_ removeAllObjects];
 }
 
+- (void)setOnLostDetectionEnabled:(BOOL)enabled {
+  BOOL oldOnLostDetectionEnabled = onLostDetectionEnabled_;

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

nit:
if (enabled == onLostDetectionEnabled_)
  return;

then consider there is an update.

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

Done.

+  onLostDetectionEnabled_ = enabled;
+  if (started_ && oldOnLostDetectionEnabled != onLostDetectionEnabled_) {
+    [self start];
+  }
+}
+
 - (int)unresolvedBeaconsCount {
   return [unresolvedDevices_ count];
 }
@@ -205,11 +236,104 @@ enum BeaconType {
 
 - (void)reallyStart {
   pendingStart_ = NO;
+
+  if (updateTimer_.get()) {
+    [updateTimer_ invalidate];
+    updateTimer_.reset();
+  }
+
   NSArray* serviceUUIDs = @[
     [CBUUID UUIDWithString:kUriBeaconServiceUUID],
     [CBUUID UUIDWithString:kEddystoneBeaconServiceUUID]
   ];
-  [centralManager_ scanForPeripheralsWithServices:serviceUUIDs options:nil];
+  NSDictionary* options = nil;
+  if (onLostDetectionEnabled_) {
+    // To detect lost URLs, we configure the scanner with the AllowDuplicates
+    // option enabled, which will notify us each time an advertising packet is
+    // received rather than only the first time. A URL is considered lost if the
+    // most recent sighting was more than |kLostThresholdSeconds| ago.
+    options =
+        [NSDictionary dictionaryWithObjectsAndKeys:
+                          [NSNumber numberWithBool:YES],
+                          CBCentralManagerScanOptionAllowDuplicatesKey, nil];

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

Question: 
Is this needed if we do the workaround?
Can we save battery by not allowing duplicates?

[mattreynolds, 2016/10/19 18:20:21]

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

+
+    // Register a repeating timer to periodically check for lost URLs.
+    updateTimer_.reset([NSTimer
+        scheduledTimerWithTimeInterval:kUpdateIntervalSeconds
+                                target:self
+                              selector:@selector(onUpdate:)
+                              userInfo:nil
+                               repeats:YES]);
+  }
+  [centralManager_ scanForPeripheralsWithServices:serviceUUIDs options:options];
+}
+
+- (void)reallyStop {
+  if (updateTimer_.get()) {
+    [updateTimer_ invalidate];
+    updateTimer_.reset();
+  }
+
+  [centralManager_ stopScan];
+}
+
+- (void)onUpdate:(NSTimer*)timer {
+  NSDate* now = [NSDate date];
+  NSMutableArray* lostDevices = [NSMutableArray array];
+  NSMutableArray* lostUnresolvedDevices = [NSMutableArray array];
+  NSMutableArray* lostScannedUrls = [NSMutableArray array];
+
+  for (PhysicalWebDevice* device in [self devices]) {
+    NSDate* scanTimestamp = [device scanTimestamp];
+    NSTimeInterval elapsedSeconds = [now timeIntervalSinceDate:scanTimestamp];
+    if (elapsedSeconds > kLostThresholdSeconds) {
+      [lostDevices addObject:device];
+      [lostScannedUrls addObject:[device requestURL]];
+      [devicesUrls_ removeObject:[device requestURL]];
+      [finalUrls_ removeObject:[device url]];
+    }
+  }
+
+  for (PhysicalWebDevice* device in unresolvedDevices_.get()) {
+    NSDate* scanTimestamp = [device scanTimestamp];
+    NSTimeInterval elapsedSeconds = [now timeIntervalSinceDate:scanTimestamp];
+    if (elapsedSeconds > kLostThresholdSeconds) {
+      [lostUnresolvedDevices addObject:device];
+      [lostScannedUrls addObject:[device requestURL]];
+      [devicesUrls_ removeObject:[device requestURL]];
+    }
+  }
+
+  NSMutableArray* requestsToRemove = [NSMutableArray array];
+  for (PhysicalWebRequest* request in pendingRequests_.get()) {
+    if ([lostScannedUrls containsObject:[request requestURL]]) {
+      [request cancel];
+      [requestsToRemove addObject:request];
+    }
+  }
+
+  [devices_ removeObjectsInArray:lostDevices];
+  [unresolvedDevices_ removeObjectsInArray:lostUnresolvedDevices];
+  [pendingRequests_ removeObjectsInArray:requestsToRemove];
+
+  if ([lostDevices count]) {
+    [delegate_ scannerUpdatedDevices:self];
+  }
+
+  // Workaround for crbug.com/657056

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

format
// TODO(crbug.com/657056): Remove this workaround when radar is fixed.

[mattreynolds, 2016/10/19 18:20:21]

Done.

+  // For unknown reasons, when scanning for longer periods (on the order of
+  // minutes), the scanner is less reliable at detecting all nearby URLs. As a
+  // workaround, we restart the scanner each time we check for lost URLs.
+  NSArray* serviceUUIDs = @[
+    [CBUUID UUIDWithString:kUriBeaconServiceUUID],
+    [CBUUID UUIDWithString:kEddystoneBeaconServiceUUID]
+  ];
+  NSDictionary* options = [NSDictionary
+      dictionaryWithObjectsAndKeys:[NSNumber numberWithBool:YES],
+                                   CBCentralManagerScanOptionAllowDuplicatesKey,
+                                   nil];
+  [centralManager_ stopScan];
+  [centralManager_ scanForPeripheralsWithServices:serviceUUIDs options:options];
 }
 
 #pragma mark -
@@ -222,7 +346,7 @@ enum BeaconType {
   } else {
     if (started_ && !pendingStart_) {
       pendingStart_ = YES;
-      [centralManager_ stopScan];
+      [self reallyStop];
     }
   }
   [delegate_ scannerBluetoothStatusUpdated:self];
@@ -274,10 +398,12 @@ enum BeaconType {
   if (!device.get())
     return;
 
+  [device setScanTimestamp:[NSDate date]];
+
   // Skip if the URL has already been seen.
-  if ([devicesUrls_ containsObject:[device url]])
+  if ([devicesUrls_ containsObject:[device requestURL]])

[mattreynolds, 2016/10/19 18:20:21]

found a bug: if the URL has already been seen, we won't update the timestamp.

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

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]

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.

     return;
-  [devicesUrls_ addObject:[device url]];
+  [devicesUrls_ addObject:[device requestURL]];
 
   if (networkRequestEnabled_) {
     [self requestMetadataForDevice:device];
@@ -326,13 +452,14 @@ enum BeaconType {
     return nil;
 
   return [[PhysicalWebDevice alloc] initWithURL:url
-                                     requestURL:nil
+                                     requestURL:url
                                            icon:nil
                                           title:nil
                                     description:nil
                                   transmitPower:transmitPower
                                            rssi:rssi
-                                           rank:physical_web::kMaxRank];
+                                           rank:physical_web::kMaxRank
+                                  scanTimestamp:[NSDate date]];
 }
 
 - (void)requestMetadataForDevice:(PhysicalWebDevice*)device {