Add a message to empty ContentSuggestions sections

Add a message to empty ContentSuggestions sections

This CL adds a message to be displayed in the empty ContentSuggestions sections.

BUG=686728
Review-Url: https://codereview.chromium.org/2775593002
Cr-Commit-Position: refs/heads/master@{#460052}
Committed: https://chromium.googlesource.com/chromium/src/+/08dded86021b3231e8db6ea77163b88c036bba76

[gambard, 2017-03-23 09:50:45]

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

[gambard, 2017-03-23 09:50:45]

PTAL.

[lpromero, 2017-03-23 10:54:38]

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

https://codereview.chromium.org/2775593002/diff/20001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_text_item.h:14:
@interface ContentSuggestionsTextItem
Not a fan of that old friend…
If what you need is just an item that takes a text, detail text and
suggestionIdentifier, we might as well create it. It can define
CollectionViewTextCell as its cellClass, that way you won't have to create a
cell class.

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

https://codereview.chromium.org/2775593002/diff/20001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestion.h:20: // Use this
type pass information about an empty section. Suggestion of this
"to" pass?

https://codereview.chromium.org/2775593002/diff/20001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestion.h:21: // type are
empty and should not be displayed.
Why "should not be displayed"? Isn't there a message to display?

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

https://codereview.chromium.org/2775593002/diff/20001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_updater.h:44:
// model.
Maybe mention that if a section has no items, the empty item is added?

https://codereview.chromium.org/2775593002/diff/20001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_updater.h:48:
// Adds the empty item for this section.
s/for/to? If you change, change also the method name.

https://codereview.chromium.org/2775593002/diff/20001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_updater.h:49:
- (void)addEmptyItemForSection:(NSInteger)section;
Please add unittests.

https://codereview.chromium.org/2775593002/diff/20001/ios/chrome/browser/ui/c...
File
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_updater.mm
(right):

https://codereview.chromium.org/2775593002/diff/20001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_updater.mm:236:
[self addItem:item toSectionWithIdentifier:sectionIdentifier];
Is there a code path that would have two empty items added to the same section?

https://codereview.chromium.org/2775593002/diff/20001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_updater.mm:237:
[indexPaths addObject:addedIndexPath];
You could reuse -addEmptyItemForSection: once it returns the added index path.

https://codereview.chromium.org/2775593002/diff/20001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_updater.mm:293:
- (void)addEmptyItemForSection:(NSInteger)section {
Return the index path of the newly added item.

https://codereview.chromium.org/2775593002/diff/20001/ios/chrome/browser/ui/c...
File
ios/chrome/browser/ui/content_suggestions/content_suggestions_view_controller.mm
(right):

https://codereview.chromium.org/2775593002/diff/20001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_view_controller.mm:70:
[self checkEmptySection:indexPath.section];
Can you update the name of the method, as it does more than checking. For
example -addEmptySectionPlaceholderIfNeeded: or something similar.

https://codereview.chromium.org/2775593002/diff/20001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_view_controller.mm:281:
NSIndexPath* emptyItem = [NSIndexPath indexPathForItem:0 inSection:section];
Use the return value of –addEmptyItemForSection: once it returns the indexpath.

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

https://codereview.chromium.org/2775593002/diff/20001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/identifier/content_suggestions_section_information.h:49:
// because the user dismissed suggestions, it should be shown in either cases.
Regarding the last sentence, does that means that this property will be updated
to reflect that, or that the property will not be honored in that case?

[gambard, 2017-03-23 15:25:50]

Thanks, PTAL.

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

https://codereview.chromium.org/2775593002/diff/20001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_text_item.h:14:
@interface ContentSuggestionsTextItem
On 2017/03/23 10:54:37, lpromero wrote:
> Not a fan of that old friend…
> If what you need is just an item that takes a text, detail text and
> suggestionIdentifier, we might as well create it. It can define
> CollectionViewTextCell as its cellClass, that way you won't have to create a
> cell class.

Done.

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

https://codereview.chromium.org/2775593002/diff/20001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestion.h:20: // Use this
type pass information about an empty section. Suggestion of this
On 2017/03/23 10:54:37, lpromero wrote:
> "to" pass?

Done.

https://codereview.chromium.org/2775593002/diff/20001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestion.h:21: // type are
empty and should not be displayed.
On 2017/03/23 10:54:37, lpromero wrote:
> Why "should not be displayed"? Isn't there a message to display?

Comment updated.

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

https://codereview.chromium.org/2775593002/diff/20001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_updater.h:44:
// model.
On 2017/03/23 10:54:37, lpromero wrote:
> Maybe mention that if a section has no items, the empty item is added?

The empty suggestion is a type of suggestion, with a corresponding item
determined by the updater.
So if you have an empty suggestion in your array, the updater will add the
corresponding item in the same way it will do it for the other type of
suggestions.
If you don't have an empty item but a section is empty, the updater will do
nothing. So for me this comment reflect what will happen.

https://codereview.chromium.org/2775593002/diff/20001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_updater.h:48:
// Adds the empty item for this section.
On 2017/03/23 10:54:37, lpromero wrote:
> s/for/to? If you change, change also the method name.

Done.

https://codereview.chromium.org/2775593002/diff/20001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_updater.h:49:
- (void)addEmptyItemForSection:(NSInteger)section;
On 2017/03/23 10:54:37, lpromero wrote:
> Please add unittests.

Done.

https://codereview.chromium.org/2775593002/diff/20001/ios/chrome/browser/ui/c...
File
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_updater.mm
(right):

https://codereview.chromium.org/2775593002/diff/20001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_updater.mm:236:
[self addItem:item toSectionWithIdentifier:sectionIdentifier];
On 2017/03/23 10:54:37, lpromero wrote:
> Is there a code path that would have two empty items added to the same
section?

Yes. This is why I am testing to know if the section contains 0 item (see lines
231-232).

https://codereview.chromium.org/2775593002/diff/20001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_updater.mm:237:
[indexPaths addObject:addedIndexPath];
On 2017/03/23 10:54:37, lpromero wrote:
> You could reuse -addEmptyItemForSection: once it returns the added index path.

I am not sure it makes sense here addEmptyItemForSection is a wrapper for the
two lines above, but it takes a section instead of a section identifier.

https://codereview.chromium.org/2775593002/diff/20001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_updater.mm:293:
- (void)addEmptyItemForSection:(NSInteger)section {
On 2017/03/23 10:54:37, lpromero wrote:
> Return the index path of the newly added item.

Done.

https://codereview.chromium.org/2775593002/diff/20001/ios/chrome/browser/ui/c...
File
ios/chrome/browser/ui/content_suggestions/content_suggestions_view_controller.mm
(right):

https://codereview.chromium.org/2775593002/diff/20001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_view_controller.mm:70:
[self checkEmptySection:indexPath.section];
On 2017/03/23 10:54:37, lpromero wrote:
> Can you update the name of the method, as it does more than checking. For
> example -addEmptySectionPlaceholderIfNeeded: or something similar.

Done.

https://codereview.chromium.org/2775593002/diff/20001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_view_controller.mm:281:
NSIndexPath* emptyItem = [NSIndexPath indexPathForItem:0 inSection:section];
On 2017/03/23 10:54:37, lpromero wrote:
> Use the return value of –addEmptyItemForSection: once it returns the
indexpath.

Done.

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

https://codereview.chromium.org/2775593002/diff/20001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/identifier/content_suggestions_section_information.h:49:
// because the user dismissed suggestions, it should be shown in either cases.
On 2017/03/23 10:54:37, lpromero wrote:
> Regarding the last sentence, does that means that this property will be
updated
> to reflect that, or that the property will not be honored in that case?

The property will not be honored.

[lpromero, 2017-03-23 19:08:42]

The CQ bit was checked by lpromero@chromium.org

[lpromero, 2017-03-23 19:08:43]

lgtm

[commit-bot: I haz the power, 2017-03-23 19:09:23]

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

[commit-bot: I haz the power, 2017-03-23 19:09:26]

This CL has an open dependency (Issue 2764783006 Patch 40001). Please resolve
the dependency and try again.
If you are sure that there is no real dependency, please use one of the options
listed in https://goo.gl/9Es4OR to land the CL.

[gambard, 2017-03-24 12:00:54]

The CQ bit was checked by gambard@chromium.org

[commit-bot: I haz the power, 2017-03-24 12:01:01]

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-03-24 12:07:23]

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

[commit-bot: I haz the power, 2017-03-24 12:07:24]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[gambard, 2017-03-24 12:12:17]

gambard@chromium.org changed reviewers:
+ baxley@chromium.org, treib@chromium.org

[gambard, 2017-03-24 12:12:18]

+treib@ for components/ntp_snippets_strings
+baxley@ for ios/test/BUILD

[baxley, 2017-03-24 14:15:02]

> +baxley@ for ios/test/BUILD
LGTM

[Marc Treib, 2017-03-27 08:08:04]

lgtm

[gambard, 2017-03-27 15:32:11]

The CQ bit was checked by gambard@chromium.org

[gambard, 2017-03-27 15:32:12]

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

[commit-bot: I haz the power, 2017-03-27 15:33:01]

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-03-27 19:23:32]

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

[commit-bot: I haz the power, 2017-03-27 19:23:34]

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

[gambard, 2017-03-28 07:13:17]

The CQ bit was checked by gambard@chromium.org

[gambard, 2017-03-28 07:13:18]

The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org,
baxley@chromium.org, lpromero@chromium.org
Link to the patchset: https://codereview.chromium.org/2775593002/#ps100001
(title: "Change tests")

[commit-bot: I haz the power, 2017-03-28 07:13:32]

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-03-28 09:32:59]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1490685197532190,
"parent_rev": "00a6d2f940c4f667933a016aa04e3835fad9e940", "commit_rev":
"08dded86021b3231e8db6ea77163b88c036bba76"}

[commit-bot: I haz the power, 2017-03-28 09:35:09]

Description was changed from

==========
Add a message to empty ContentSuggestions sections

This CL adds a message to be displayed in the empty ContentSuggestions sections.

BUG=686728
==========

to

==========
Add a message to empty ContentSuggestions sections

This CL adds a message to be displayed in the empty ContentSuggestions sections.

BUG=686728

Review-Url: https://codereview.chromium.org/2775593002
Cr-Commit-Position: refs/heads/master@{#460052}
Committed:
https://chromium.googlesource.com/chromium/src/+/08dded86021b3231e8db6ea77163...
==========

[commit-bot: I haz the power, 2017-03-28 09:35:10]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/08dded86021b3231e8db6ea77163...