Add MostVistedSites to ContentSuggestionsMediator

ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm

Index: ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm
diff --git a/ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm b/ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm
index 15c36ba9e8e5c4696a7ffc4a855132149a5f1a5e..cdc44b997eee63719ab6ca2d3bd32a2d3eb40ba6 100644
--- a/ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm
+++ b/ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm
@@ -11,9 +11,12 @@
 #include "components/ntp_snippets/category.h"
 #include "components/ntp_snippets/category_info.h"
 #include "components/ntp_snippets/content_suggestion.h"
+#include "components/ntp_tiles/most_visited_sites.h"
+#include "components/ntp_tiles/ntp_tile.h"
 #import "ios/chrome/browser/content_suggestions/content_suggestions_category_wrapper.h"
 #import "ios/chrome/browser/content_suggestions/content_suggestions_service_bridge_observer.h"
 #import "ios/chrome/browser/content_suggestions/mediator_util.h"
+#include "ios/chrome/browser/ntp_tiles/most_visited_sites_observer_bridge.h"
 #import "ios/chrome/browser/ui/content_suggestions/content_suggestion.h"
 #import "ios/chrome/browser/ui/content_suggestions/content_suggestions_data_sink.h"
 #import "ios/chrome/browser/ui/content_suggestions/content_suggestions_image_fetcher.h"
@@ -30,15 +33,28 @@ namespace {
 
 // Size of the favicon returned by the provider.
 const CGFloat kDefaultFaviconSize = 16;
+// Maximum number of most visited tiles fetched.
+const NSInteger kMaxNumMostVisitedTiles = 8;
 
 }  // namespace
 
 @interface ContentSuggestionsMediator ()<ContentSuggestionsImageFetcher,
-                                         ContentSuggestionsServiceObserver> {
+                                         ContentSuggestionsServiceObserver,
+                                         MostVisitedSitesObserving> {
   // Bridge for this class to become an observer of a ContentSuggestionsService.
   std::unique_ptr<ContentSuggestionsServiceBridge> _suggestionBridge;
+  std::unique_ptr<ntp_tiles::MostVisitedSites> _mostVisitedSites;
+  std::unique_ptr<ntp_tiles::MostVisitedSitesObserverBridge> _mostVisitedBridge;
 }
 
+// Most visited data from the MostVisitedSites service (copied upon receiving
+// the callback).
+@property(nonatomic, assign) std::vector<ntp_tiles::NTPTile> mostVisitedData;
+// Section Info for the Most Visited section.
+@property(nonatomic, strong)
+    ContentSuggestionsSectionInformation* mostVisitedSectionInfo;
+// Whether the page impression has been recorded.
+@property(nonatomic, assign) BOOL recordedPageImpression;
 @property(nonatomic, assign)

[lpromero, 2017/04/04 11:25:20]

For completeness, can you add a quick comment to these two properties?

[gambard, 2017/04/04 16:14:26]

Done.

     ntp_snippets::ContentSuggestionsService* contentService;
 @property(nonatomic, strong, nonnull)
@@ -68,6 +84,9 @@ const CGFloat kDefaultFaviconSize = 16;
 
 @implementation ContentSuggestionsMediator
 
+@synthesize mostVisitedData = _mostVisitedData;
+@synthesize mostVisitedSectionInfo = _mostVisitedSectionInfo;
+@synthesize recordedPageImpression = _recordedPageImpression;
 @synthesize contentService = _contentService;
 @synthesize dataSink = _dataSink;
 @synthesize sectionInformationByCategory = _sectionInformationByCategory;
@@ -77,7 +96,9 @@ const CGFloat kDefaultFaviconSize = 16;
 
 - (instancetype)
 initWithContentService:(ntp_snippets::ContentSuggestionsService*)contentService
-      largeIconService:(favicon::LargeIconService*)largeIconService {
+      largeIconService:(favicon::LargeIconService*)largeIconService
+       mostVisitedSite:
+           (std::unique_ptr<ntp_tiles::MostVisitedSites>)mostVisitedSites {
   self = [super init];
   if (self) {
     _suggestionBridge =
@@ -88,6 +109,13 @@ initWithContentService:(ntp_snippets::ContentSuggestionsService*)contentService
         initWithFaviconSize:kDefaultFaviconSize
              minFaviconSize:1
            largeIconService:largeIconService];
+
+    _mostVisitedSectionInfo = MostVisitedSectionInformation();
+    _mostVisitedSites = std::move(mostVisitedSites);
+    _mostVisitedBridge =
+        base::MakeUnique<ntp_tiles::MostVisitedSitesObserverBridge>(self);
+    _mostVisitedSites->SetMostVisitedURLsObserver(_mostVisitedBridge.get(),
+                                                  kMaxNumMostVisitedTiles);
   }
   return self;
 }
@@ -105,9 +133,12 @@ initWithContentService:(ntp_snippets::ContentSuggestionsService*)contentService
 #pragma mark - ContentSuggestionsDataSource
 
 - (NSArray<ContentSuggestion*>*)allSuggestions {
+  NSMutableArray<ContentSuggestion*>* dataHolders = [NSMutableArray array];
+
+  [self addMostVisitedToArray:dataHolders];
+
   std::vector<ntp_snippets::Category> categories =

[lpromero, 2017/04/04 11:25:20]

Should you create a new -addContentSuggestionsToArray: for this second part, to
be on equal footing? (Maybe just the inner of the loop?)

[gambard, 2017/04/04 16:14:26]

Done.

       self.contentService->GetCategories();
-  NSMutableArray<ContentSuggestion*>* dataHolders = [NSMutableArray array];
   for (auto& category : categories) {
     const std::vector<ntp_snippets::ContentSuggestion>& suggestions =
         self.contentService->GetSuggestionsForCategory(category);
@@ -118,15 +149,21 @@ initWithContentService:(ntp_snippets::ContentSuggestionsService*)contentService
 
 - (NSArray<ContentSuggestion*>*)suggestionsForSection:
     (ContentSuggestionsSectionInformation*)sectionInfo {
-  ntp_snippets::Category category =
-      [[self categoryWrapperForSectionInfo:sectionInfo] category];
-
   NSMutableArray* convertedSuggestions = [NSMutableArray array];
-  const std::vector<ntp_snippets::ContentSuggestion>& suggestions =
-      self.contentService->GetSuggestionsForCategory(category);
-  [self addSuggestions:suggestions
-          fromCategory:category
-               toArray:convertedSuggestions];
+
+  if (sectionInfo == self.mostVisitedSectionInfo) {
+    [self addMostVisitedToArray:convertedSuggestions];
+  } else {
+    ntp_snippets::Category category =
+        [[self categoryWrapperForSectionInfo:sectionInfo] category];
+
+    const std::vector<ntp_snippets::ContentSuggestion>& suggestions =
+        self.contentService->GetSuggestionsForCategory(category);
+    [self addSuggestions:suggestions
+            fromCategory:category
+                 toArray:convertedSuggestions];
+  }
+
   return convertedSuggestions;
 }
 
@@ -139,6 +176,9 @@ initWithContentService:(ntp_snippets::ContentSuggestionsService*)contentService
                     fromSectionInfo:
                         (ContentSuggestionsSectionInformation*)sectionInfo
                            callback:(MoreSuggestionsFetched)callback {
+  if (![self isAContentSuggestionsCategory:sectionInfo])
+    return;
+
   std::set<std::string> known_suggestion_ids;
   for (ContentSuggestionIdentifier* identifier in knownSuggestions) {
     if (identifier.sectionInfo != sectionInfo)
@@ -243,6 +283,23 @@ initWithContentService:(ntp_snippets::ContentSuggestionsService*)contentService
       }));
 }
 
+#pragma mark - MostVisitedSitesObserving
+
+- (void)onMostVisitedURLsAvailable:
+    (const ntp_tiles::NTPTilesVector&)mostVisited {
+  self.mostVisitedData = mostVisited;
+  [self.dataSink reloadSection:self.mostVisitedSectionInfo];
+
+  if (mostVisited.size() && !self.recordedPageImpression) {
+    self.recordedPageImpression = YES;
+    RecordPageImpression(mostVisited);
+  }
+}
+
+- (void)onIconMadeAvailable:(const GURL&)siteURL {
+  [self.dataSink faviconAvailableForURL:siteURL];
+}
+
 #pragma mark - Private
 
 - (void)addSuggestions:
@@ -272,10 +329,7 @@ initWithContentService:(ntp_snippets::ContentSuggestionsService*)contentService
   }
 
   if (suggestions.size() == 0) {
-    ContentSuggestion* suggestion = [[ContentSuggestion alloc] init];
-    suggestion.type = ContentSuggestionTypeEmpty;
-    suggestion.suggestionIdentifier =
-        [[ContentSuggestionIdentifier alloc] init];
+    ContentSuggestion* suggestion = EmptySuggestion();
     suggestion.suggestionIdentifier.sectionInfo =
         self.sectionInformationByCategory[categoryWrapper];
 
@@ -317,4 +371,28 @@ initWithContentService:(ntp_snippets::ContentSuggestionsService*)contentService
   }
 }
 
+// Adds all the suggestions for the |mostVisitedData| to |suggestions|.
+- (void)addMostVisitedToArray:(NSMutableArray<ContentSuggestion*>*)suggestions {
+  if (self.mostVisitedData.empty()) {
+    ContentSuggestion* suggestion = EmptySuggestion();
+    suggestion.suggestionIdentifier.sectionInfo = self.mostVisitedSectionInfo;
+    [suggestions addObject:suggestion];
+
+    return;
+  }
+
+  for (const ntp_tiles::NTPTile& tile : self.mostVisitedData) {
+    ContentSuggestion* suggestion = ConvertNTPTile(tile);
+    suggestion.suggestionIdentifier.sectionInfo = self.mostVisitedSectionInfo;
+    [suggestions addObject:suggestion];
+  }
+}
+
+// Returns whether the |sectionInfo| is associated with a category from the
+// content suggestions service.
+- (BOOL)isAContentSuggestionsCategory:
+    (ContentSuggestionsSectionInformation*)sectionInfo {
+  return sectionInfo != self.mostVisitedSectionInfo;

[lpromero, 2017/04/04 11:25:20]

Feels weird since mostVisitedSectionInfo is a
ContentSuggestionsSectionInformation.

For a quick fix:
Since you use this method by using the negation of the result, what about:
-isMostVisitedCategory: or -isMostVisitedSection:

If MostVisited can't be considered as a Content Suggestion with our definition,
then maybe we need a more generic type name to avoid confusion.

[gambard, 2017/04/04 16:14:26]

i have changed the name to isRelatedToContentSuggestionsService

+}
+
 @end