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..4a0fb9e496331df2fdefdd6890ed5f390e0e0c61 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;
 // 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,21 @@ 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_;
+  // Map from URLs to timestamps of the last time the URL was nearby.
+  base::scoped_nsobject<NSMutableDictionary> scanTimestamps_;
+  // 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 {
@@ -92,6 +113,7 @@ enum BeaconType {
         initWithDelegate:self
                    queue:dispatch_get_main_queue()]);
     unresolvedDevices_.reset([[NSMutableArray alloc] init]);
+    scanTimestamps_.reset([[NSMutableDictionary alloc] init]);
     [[NSHTTPCookieStorage sharedHTTPCookieStorage]
         setCookieAcceptPolicy:NSHTTPCookieAcceptPolicyNever];
   }
@@ -106,6 +128,10 @@ enum BeaconType {
 - (void)dealloc {
   [centralManager_ setDelegate:nil];
   centralManager_.reset();
+  if (updateTimer_.get()) {
+    [updateTimer_ invalidate];
+    updateTimer_.reset();
+  }
   [super dealloc];
 }
 
@@ -114,6 +140,7 @@ enum BeaconType {
   [finalUrls_ removeAllObjects];
   [devicesUrls_ removeAllObjects];
   [devices_ removeAllObjects];
+  [scanTimestamps_ removeAllObjects];
   started_ = YES;
   if ([self bluetoothEnabled])
     [self reallyStart];
@@ -129,7 +156,7 @@ enum BeaconType {
   }
   [pendingRequests_ removeAllObjects];
   if (!pendingStart_ && [self bluetoothEnabled]) {
-    [centralManager_ stopScan];
+    [self reallyStop];
   }
   pendingStart_ = NO;
   started_ = NO;
@@ -189,6 +216,14 @@ enum BeaconType {
   [unresolvedDevices_ removeAllObjects];
 }
 
+- (void)setOnLostDetectionEnabled:(BOOL)enabled {
+  BOOL oldOnLostDetectionEnabled = onLostDetectionEnabled_;
+  onLostDetectionEnabled_ = enabled;
+  if (started_ && oldOnLostDetectionEnabled != onLostDetectionEnabled_) {
+    [self start];

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

This will reset all the lists. Is that expected?
Should we document this?

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

I added a comment documenting the behavior

+  }
+}
+
 - (int)unresolvedBeaconsCount {
   return [unresolvedDevices_ count];
 }
@@ -205,11 +240,97 @@ 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];
+
+    // 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];
+  NSMutableArray* lostResolvedUrls = [NSMutableArray array];
+
+  for (PhysicalWebDevice* device in [self devices]) {
+    NSDate* scanTimestamp = [scanTimestamps_ objectForKey:[device requestURL]];
+    NSTimeInterval elapsedSeconds = [now timeIntervalSinceDate:scanTimestamp];
+    if (elapsedSeconds > kLostThresholdSeconds) {
+      [lostDevices addObject:device];
+      [lostScannedUrls addObject:[device requestURL]];
+      [lostResolvedUrls addObject:[device url]];
+      [devicesUrls_ removeObject:[device requestURL]];
+      [finalUrls_ removeObject:[device url]];
+    }
+  }
+
+  for (PhysicalWebDevice* device in unresolvedDevices_.get()) {
+    NSDate* scanTimestamp = [scanTimestamps_ objectForKey:[device requestURL]];
+    NSTimeInterval elapsedSeconds = [now timeIntervalSinceDate:scanTimestamp];
+    if (elapsedSeconds > kLostThresholdSeconds) {
+      [lostUnresolvedDevices addObject:device];
+      [lostScannedUrls addObject:[device requestURL]];
+      [devicesUrls_ removeObject:[device requestURL]];
+    }
+  }
+
+  [devices_ removeObjectsInArray:lostDevices];
+  [unresolvedDevices_ removeObjectsInArray:lostUnresolvedDevices];

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

should we cancel the resolve URL query?

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

Done.

+  [scanTimestamps_ removeObjectsForKeys:lostScannedUrls];
+
+  if ([lostDevices count]) {
+    [delegate_ scannerUpdatedDevices:self];
+  }
+
+  // For unknown reasons, when scanning for longer periods (on the order of

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

Can you file a radar on this, file a crbug that reference the radar?

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

Done.

+  // 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 +343,7 @@ enum BeaconType {
   } else {
     if (started_ && !pendingStart_) {
       pendingStart_ = YES;
-      [centralManager_ stopScan];
+      [self reallyStop];
     }
   }
   [delegate_ scannerBluetoothStatusUpdated:self];
@@ -274,10 +395,12 @@ enum BeaconType {
   if (!device.get())
     return;
 
+  [scanTimestamps_ setObject:[NSDate date] forKey:[device requestURL]];

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

Would it make sense to add this to the device object instead?

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

Done.

+
   // Skip if the URL has already been seen.
-  if ([devicesUrls_ containsObject:[device url]])
+  if ([devicesUrls_ containsObject:[device requestURL]])
     return;
-  [devicesUrls_ addObject:[device url]];
+  [devicesUrls_ addObject:[device requestURL]];
 
   if (networkRequestEnabled_) {
     [self requestMetadataForDevice:device];
@@ -326,7 +449,7 @@ enum BeaconType {
     return nil;
 
   return [[PhysicalWebDevice alloc] initWithURL:url
-                                     requestURL:nil
+                                     requestURL:url
                                            icon:nil
                                           title:nil
                                     description:nil