Limit the number of Most Visited lines to two

Limit the number of Most Visited lines to two

The Most Visited tiles are not longer displayed on more than two lines.

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

[gambard, 2017-05-18 13:53:57]

gambard@chromium.org changed reviewers:
+ eugenebut@chromium.org

[gambard, 2017-05-18 13:53:58]

PTAL.

[Eugene But (OOO till 7-30), 2017-05-18 15:02:01]

lgtm Ideally you want to ask someone who understands UI code to do this review.

https://codereview.chromium.org/2888403002/diff/1/ios/chrome/browser/ui/conte...
File
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_updater.mm
(right):

https://codereview.chromium.org/2888403002/diff/1/ios/chrome/browser/ui/conte...
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_updater.mm:105:
const CGFloat kNumberOfMostVisitedLines = 2;
Would it be useful to explain how we came up with "2"?

https://codereview.chromium.org/2888403002/diff/1/ios/chrome/browser/ui/conte...
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_updater.mm:116:
//
Please finish the comment :)

https://codereview.chromium.org/2888403002/diff/1/ios/chrome/browser/ui/conte...
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_updater.mm:378:
NSInteger futureNumber = MIN([self maxNumberOfMostVisited],
nit: s/futureNumber/newCount

https://codereview.chromium.org/2888403002/diff/1/ios/chrome/browser/ui/conte...
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_updater.mm:380:
NSInteger currentNumber = [model numberOfItemsInSection:mostVisitedSection];
nit: s/currentNumber/currentCount

https://codereview.chromium.org/2888403002/diff/1/ios/chrome/browser/ui/conte...
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_updater.mm:391:
dismissEntryAtIndexPath:[NSIndexPath
Do you want to use a local variable for NSIndexPath to improve the formatting?

https://codereview.chromium.org/2888403002/diff/1/ios/chrome/browser/ui/conte...
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_updater.mm:591:
// Returns the maximum number of Most Visited tiles to be displayed in the
Is this also SuggestedContentDelegate method? Should it be pragma marked?

https://codereview.chromium.org/2888403002/diff/1/ios/chrome/browser/ui/conte...
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_updater.mm:593:
- (NSInteger)maxNumberOfMostVisited {
How about |mostVisitedTileCount|?

[gambard, 2017-05-18 15:16:08]

Thanks!

https://codereview.chromium.org/2888403002/diff/1/ios/chrome/browser/ui/conte...
File
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_updater.mm
(right):

https://codereview.chromium.org/2888403002/diff/1/ios/chrome/browser/ui/conte...
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_updater.mm:105:
const CGFloat kNumberOfMostVisitedLines = 2;
On 2017/05/18 15:02:00, Eugene But wrote:
> Would it be useful to explain how we came up with "2"?

I have no idea why we picked 2 :)

https://codereview.chromium.org/2888403002/diff/1/ios/chrome/browser/ui/conte...
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_updater.mm:116:
//
On 2017/05/18 15:02:00, Eugene But wrote:
> Please finish the comment :)

Done.

https://codereview.chromium.org/2888403002/diff/1/ios/chrome/browser/ui/conte...
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_updater.mm:378:
NSInteger futureNumber = MIN([self maxNumberOfMostVisited],
On 2017/05/18 15:02:01, Eugene But wrote:
> nit: s/futureNumber/newCount 

Done.

https://codereview.chromium.org/2888403002/diff/1/ios/chrome/browser/ui/conte...
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_updater.mm:380:
NSInteger currentNumber = [model numberOfItemsInSection:mostVisitedSection];
On 2017/05/18 15:02:00, Eugene But wrote:
> nit: s/currentNumber/currentCount 

Done.

https://codereview.chromium.org/2888403002/diff/1/ios/chrome/browser/ui/conte...
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_updater.mm:391:
dismissEntryAtIndexPath:[NSIndexPath
On 2017/05/18 15:02:00, Eugene But wrote:
> Do you want to use a local variable for NSIndexPath to improve the formatting?

Done.

https://codereview.chromium.org/2888403002/diff/1/ios/chrome/browser/ui/conte...
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_updater.mm:591:
// Returns the maximum number of Most Visited tiles to be displayed in the
On 2017/05/18 15:02:00, Eugene But wrote:
> Is this also SuggestedContentDelegate method? Should it be pragma marked?

There is a |#pragma mark - Private methods| in between

https://codereview.chromium.org/2888403002/diff/1/ios/chrome/browser/ui/conte...
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_updater.mm:593:
- (NSInteger)maxNumberOfMostVisited {
On 2017/05/18 15:02:01, Eugene But wrote:
> How about |mostVisitedTileCount|?

It is the number of places where a Most Visited tile can be displayed. I have
changed it to -mostVisitedPlaceCount.

[gambard, 2017-05-18 15:16:16]

The CQ bit was checked by gambard@chromium.org

[gambard, 2017-05-18 15:16:16]

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

[commit-bot: I haz the power, 2017-05-18 15:16:48]

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-05-18 15:28:36]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1495120576449070,
"parent_rev": "5f68899466fab2852e217a6355e9fdff6268a267", "commit_rev":
"f344e0cdffec00b918859ccc01419cd1791b35dc"}

[commit-bot: I haz the power, 2017-05-18 15:28:54]

Description was changed from

==========
Limit the number of Most Visited lines to two

The Most Visited tiles are not longer displayed on more than two lines.

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

to

==========
Limit the number of Most Visited lines to two

The Most Visited tiles are not longer displayed on more than two lines.

BUG=707754

Review-Url: https://codereview.chromium.org/2888403002
Cr-Commit-Position: refs/heads/master@{#472820}
Committed:
https://chromium.googlesource.com/chromium/src/+/f344e0cdffec00b918859ccc0141...
==========

[commit-bot: I haz the power, 2017-05-18 15:28:56]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/f344e0cdffec00b918859ccc0141...