Add MostVistedSites to ContentSuggestionsMediator

Add MostVistedSites to ContentSuggestionsMediator

The ContentSuggestionsMediator now returns the MostVisited tiles in the
suggestions availables.

BUG=707754
Review-Url: https://codereview.chromium.org/2798563002
Cr-Commit-Position: refs/heads/master@{#462435}
Committed: https://chromium.googlesource.com/chromium/src/+/3147e853f63020dc085577281385e4aa898a95c6

[gambard, 2017-04-04 09:42:43]

gambard@chromium.org changed reviewers:
+ lpromero@chromium.org, marq@chromium.org

[gambard, 2017-04-04 09:42:44]

PTAL.

[lpromero, 2017-04-04 11:25:21]

lgtm

https://codereview.chromium.org/2798563002/diff/20001/ios/chrome/browser/cont...
File ios/chrome/browser/content_suggestions/content_suggestions_coordinator.mm
(right):

https://codereview.chromium.org/2798563002/diff/20001/ios/chrome/browser/cont...
ios/chrome/browser/content_suggestions/content_suggestions_coordinator.mm:5:
#import
"ios/chrome/browser/content_suggestions/content_suggestions_coordinator.h"
Technically, this coordinator (and the mediator) should belong to
ios/chrome/browser/ui/content_suggestions:content_suggestions, as it has
dependencies on ios/chrome/browser/ui.

Moreover, the content_suggestions_view_controller should move to
ios/chrome/browser/ui/content_suggestions:content_suggestions.

Could you fix in a follow-up?

https://codereview.chromium.org/2798563002/diff/20001/ios/chrome/browser/cont...
File ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm
(right):

https://codereview.chromium.org/2798563002/diff/20001/ios/chrome/browser/cont...
ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm:58:
@property(nonatomic, assign)
For completeness, can you add a quick comment to these two properties?

https://codereview.chromium.org/2798563002/diff/20001/ios/chrome/browser/cont...
ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm:140:
std::vector<ntp_snippets::Category> categories =
Should you create a new -addContentSuggestionsToArray: for this second part, to
be on equal footing? (Maybe just the inner of the loop?)

https://codereview.chromium.org/2798563002/diff/20001/ios/chrome/browser/cont...
ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm:395:
return sectionInfo != self.mostVisitedSectionInfo;
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.

https://codereview.chromium.org/2798563002/diff/20001/ios/chrome/browser/cont...
File ios/chrome/browser/content_suggestions/mediator_util.h (right):

https://codereview.chromium.org/2798563002/diff/20001/ios/chrome/browser/cont...
ios/chrome/browser/content_suggestions/mediator_util.h:63: // Creates and
returns an empty suggestions.
*suggestion

https://codereview.chromium.org/2798563002/diff/20001/ios/chrome/browser/ui/c...
File ios/chrome/browser/ui/content_suggestions/content_suggestions_data_sink.h
(right):

https://codereview.chromium.org/2798563002/diff/20001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_data_sink.h:39: -
(void)faviconAvailableForURL:(const GURL&)URL;
Is this used only for MostVisited?

[gambard, 2017-04-04 16:14:27]

Thanks!

https://codereview.chromium.org/2798563002/diff/20001/ios/chrome/browser/cont...
File ios/chrome/browser/content_suggestions/content_suggestions_coordinator.mm
(right):

https://codereview.chromium.org/2798563002/diff/20001/ios/chrome/browser/cont...
ios/chrome/browser/content_suggestions/content_suggestions_coordinator.mm:5:
#import
"ios/chrome/browser/content_suggestions/content_suggestions_coordinator.h"
On 2017/04/04 11:25:20, lpromero wrote:
> Technically, this coordinator (and the mediator) should belong to
> ios/chrome/browser/ui/content_suggestions:content_suggestions, as it has
> dependencies on ios/chrome/browser/ui.
> 
> Moreover, the content_suggestions_view_controller should move to
> ios/chrome/browser/ui/content_suggestions:content_suggestions.
> 
> Could you fix in a follow-up?

This needs to be discussed offline.

https://codereview.chromium.org/2798563002/diff/20001/ios/chrome/browser/cont...
File ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm
(right):

https://codereview.chromium.org/2798563002/diff/20001/ios/chrome/browser/cont...
ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm:58:
@property(nonatomic, assign)
On 2017/04/04 11:25:20, lpromero wrote:
> For completeness, can you add a quick comment to these two properties?

Done.

https://codereview.chromium.org/2798563002/diff/20001/ios/chrome/browser/cont...
ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm:140:
std::vector<ntp_snippets::Category> categories =
On 2017/04/04 11:25:20, lpromero wrote:
> Should you create a new -addContentSuggestionsToArray: for this second part,
to
> be on equal footing? (Maybe just the inner of the loop?)

Done.

https://codereview.chromium.org/2798563002/diff/20001/ios/chrome/browser/cont...
ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm:395:
return sectionInfo != self.mostVisitedSectionInfo;
On 2017/04/04 11:25:20, lpromero wrote:
> 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.

i have changed the name to isRelatedToContentSuggestionsService

https://codereview.chromium.org/2798563002/diff/20001/ios/chrome/browser/cont...
File ios/chrome/browser/content_suggestions/mediator_util.h (right):

https://codereview.chromium.org/2798563002/diff/20001/ios/chrome/browser/cont...
ios/chrome/browser/content_suggestions/mediator_util.h:63: // Creates and
returns an empty suggestions.
On 2017/04/04 11:25:20, lpromero wrote:
> *suggestion

Done.

https://codereview.chromium.org/2798563002/diff/20001/ios/chrome/browser/ui/c...
File ios/chrome/browser/ui/content_suggestions/content_suggestions_data_sink.h
(right):

https://codereview.chromium.org/2798563002/diff/20001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_data_sink.h:39: -
(void)faviconAvailableForURL:(const GURL&)URL;
On 2017/04/04 11:25:20, lpromero wrote:
> Is this used only for MostVisited?

I think so. This is to propagate the notification from the MostVisitedSites
Observer.

[gambard, 2017-04-04 16:14:36]

The CQ bit was checked by gambard@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-04 16:15:04]

Dry run: 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, 2017-04-04 18:13:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-04 18:13:32]

Dry run: This issue passed the CQ dry run.

[gambard, 2017-04-06 09:30:41]

The CQ bit was checked by gambard@chromium.org

[gambard, 2017-04-06 09:30:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
lpromero@chromium.org
Link to the patchset: https://codereview.chromium.org/2798563002/#ps40001
(title: "Address comments")

[commit-bot: I haz the power, 2017-04-06 09:31:03]

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, 2017-04-06 09:33:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-06 09:33:03]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[gambard, 2017-04-06 09:38:43]

The CQ bit was checked by gambard@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-06 09:39:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gambard, 2017-04-06 11:16:08]

The CQ bit was checked by gambard@chromium.org

[gambard, 2017-04-06 11:16:08]

The patchset sent to the CQ was uploaded after l-g-t-m from
lpromero@chromium.org
Link to the patchset: https://codereview.chromium.org/2798563002/#ps80001
(title: "Rebase (different HEAD)")

[commit-bot: I haz the power, 2017-04-06 11:16:20]

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, 2017-04-06 12:16:55]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1491477368139200,
"parent_rev": "23539089aaee4a68f814b0f42b2ec04885fe69ea", "commit_rev":
"3147e853f63020dc085577281385e4aa898a95c6"}

[commit-bot: I haz the power, 2017-04-06 12:17:49]

Description was changed from

==========
Add MostVistedSites to ContentSuggestionsMediator

The ContentSuggestionsMediator now returns the MostVisited tiles in the
suggestions availables.

BUG=707754
==========

to

==========
Add MostVistedSites to ContentSuggestionsMediator

The ContentSuggestionsMediator now returns the MostVisited tiles in the
suggestions availables.

BUG=707754

Review-Url: https://codereview.chromium.org/2798563002
Cr-Commit-Position: refs/heads/master@{#462435}
Committed:
https://chromium.googlesource.com/chromium/src/+/3147e853f63020dc085577281385...
==========

[commit-bot: I haz the power, 2017-04-06 12:17:50]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/3147e853f63020dc085577281385...