Refresh the Most Visited tiles only on user interaction

ios/chrome/browser/ui/ntp/google_landing_mediator.mm

 // Copyright 2017 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #import "ios/chrome/browser/ui/ntp/google_landing_mediator.h"
 
 #import "base/ios/weak_nsobject.h"
 #include "base/mac/scoped_nsobject.h"
 #include "base/metrics/user_metrics.h"
 #include "base/metrics/user_metrics_action.h"
@@ skipping 88 matching lines @@
   std::unique_ptr<google_landing::SearchEngineObserver> _observer;
   TemplateURLService* _templateURLService;  // weak
 
   // A MostVisitedSites::Observer bridge object to get notified of most visited
   // sites changes.
   std::unique_ptr<ntp_tiles::MostVisitedSitesObserverBridge>
       _mostVisitedObserverBridge;
 
   std::unique_ptr<ntp_tiles::MostVisitedSites> _mostVisitedSites;
 
-  // Most visited data from the MostVisitedSites service (copied upon receiving
-  // the callback).
+  // Most visited data from the MostVisitedSites service currently in use.
   ntp_tiles::NTPTilesVector _mostVisitedData;
 
   // Observes the WebStateList so that this mediator can update the UI when the
   // active WebState changes.
   std::unique_ptr<WebStateListObserverBridge> _webStateListObserver;
 
   // What's new promo.
   std::unique_ptr<NotificationPromoWhatsNew> _notification_promo;
 }
 
 // Consumer to handle google landing update notifications.
 @property(nonatomic) id<GoogleLandingConsumer> consumer;
 
 // The WebStateList that is being observed by this mediator.
 @property(nonatomic, assign) WebStateList* webStateList;
 
 // The dispatcher for this mediator.
 @property(nonatomic, assign) id<ChromeExecuteCommand, UrlLoader> dispatcher;
 
+// Most visited data from the MostVisitedSites service (copied upon receiving
+// the callback), not yet used.
+@property(nonatomic, assign) ntp_tiles::NTPTilesVector freshMostVisitedData;
+
 // Perform initial setup.
 - (void)setUp;
 
+// If there is some fresh most visited tiles, they become the current tiles and
+// the consumer gets notified.
+- (void)useFreshData;
+
 @end
 
 @implementation GoogleLandingMediator
 
 @synthesize consumer = _consumer;
 @synthesize dispatcher = _dispatcher;
 @synthesize webStateList = _webStateList;
+@synthesize freshMostVisitedData = _freshMostVisitedData;
 
 - (instancetype)initWithConsumer:(id<GoogleLandingConsumer>)consumer
                     browserState:(ios::ChromeBrowserState*)browserState
                       dispatcher:(id<ChromeExecuteCommand, UrlLoader>)dispatcher
                     webStateList:(WebStateList*)webStateList {
   self = [super init];
   if (self) {
     _consumer = consumer;
     _browserState = browserState;
     _dispatcher = dispatcher;
@@ skipping 83 matching lines @@
   [self.consumer setLogoIsShowing:showLogo];
 }
 
 + (NSUInteger)maxSitesShown {
   return kMaxNumMostVisitedFavicons;
 }
 
 #pragma mark - MostVisitedSitesObserving
 
 - (void)onMostVisitedURLsAvailable:(const ntp_tiles::NTPTilesVector&)data {
+  if (_mostVisitedData.size() > 0) {
+    // If some content is already displayed to the user, do not update it to
+    // prevent updating the all the tiles without any action from the user.
+    self.freshMostVisitedData = data;
+    return;
+  }
+
   _mostVisitedData = data;
   [self.consumer mostVisitedDataUpdated];
 
   if (data.size() && !_recordedPageImpression) {
     _recordedPageImpression = YES;
     std::vector<ntp_tiles::metrics::TileImpression> tiles;
     for (const ntp_tiles::NTPTile& ntpTile : data) {
       tiles.emplace_back(ntpTile.source, ntp_tiles::UNKNOWN_TILE_TYPE,
                          ntpTile.url);
     }
@@ skipping 39 matching lines @@
     web::NavigationManager* nav = newWebState->GetNavigationManager();
     [self.consumer setCanGoForward:nav->CanGoForward()];
     [self.consumer setCanGoBack:nav->CanGoBack()];
   }
 }
 
 #pragma mark - GoogleLandingDataSource
 
 - (void)addBlacklistedURL:(const GURL&)url {
   _mostVisitedSites->AddOrRemoveBlacklistedUrl(url, true);
+  [self useFreshData];
 }
 
 - (void)removeBlacklistedURL:(const GURL&)url {
   _mostVisitedSites->AddOrRemoveBlacklistedUrl(url, false);
+  [self useFreshData];
 }
 
 - (ntp_tiles::NTPTile)mostVisitedAtIndex:(NSUInteger)index {
   return _mostVisitedData[index];
 }
 
 - (NSUInteger)mostVisitedSize {
   return _mostVisitedData.size();
 }
 
@@ skipping 40 matching lines @@
   if (_notification_promo->IsChromeCommand()) {
     base::scoped_nsobject<GenericChromeCommand> command(
         [[GenericChromeCommand alloc]
             initWithTag:_notification_promo->command_id()]);
     [self.dispatcher chromeExecuteCommand:command];
     return;
   }
   NOTREACHED();
 }
 
+#pragma mark - Private
+
+- (void)useFreshData {
+  if (self.freshMostVisitedData.size() == 0) {
+    return;

[justincohen, 2017/05/24 15:58:21]

should this NOTREACHED?

[gambard, 2017/06/02 12:47:04]

No. If you remove a tile, all the tiles are not necessarily updated. Why do you
think not REACHED would be good?

[justincohen, 2017/06/05 15:18:59]

When would removing a tile not call -onMostVisitedURLsAvailable?

It seems -useFreshData is only called after changing the blacklist, which should
always fire 
-onMostVisitedURLsAvailable: and set freshMostVisitedData.  I was suggesting
that if useFreshData is called without freshMostVisitedData being set, something
is wrong.

If you only have 1 tile, and you add it to the blacklist, freshMostVisitedData
won't get updated (if (_mostVisitedData.size() > 0))... so maybe it's not worth
it.

[gambard, 2017/06/06 15:34:54]

Actually this code do not remove the last tile. I will modify it, adding the
NOTREACHED and giving the possibility to remove the last tile.

+  }
+  _mostVisitedData = self.freshMostVisitedData;
+  self.freshMostVisitedData.clear();
+  [self.consumer mostVisitedDataUpdated];
+}
+
 @end