Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2736653002:
Suggested Articles can be dismissed (Closed)
|
Created:
3 years, 9 months ago by gambard Modified:
3 years, 9 months ago Reviewers:
lpromero CC:
chromium-reviews, marq+watch_chromium.org, pkl (ping after 24h if needed), noyau+watch_chromium.org, sdefresne+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSuggested Articles can be dismissed
Add the logic allowing the suggested articles to be dismissed in the UI and in
the backend.
BUG=693031, 698247, 698685
Review-Url: https://codereview.chromium.org/2736653002
Cr-Commit-Position: refs/heads/master@{#454882}
Committed: https://chromium.googlesource.com/chromium/src/+/4c48f1a300005c92a453e7864979b4780c2a9ac7
Patch Set 1 #Patch Set 2 : Cleanup #
Total comments: 21
Patch Set 3 : Address comments #
Total comments: 6
Patch Set 4 : Address comments #
Messages
Total messages: 13 (6 generated)
Description was changed from ========== Suggested Articles can be dismissed Add the logic allowing the suggested articles to be dismissed in the UI and in the backend. BUG=693031 ========== to ========== Suggested Articles can be dismissed Add the logic allowing the suggested articles to be dismissed in the UI and in the backend. BUG=693031, 698247, 698685 ==========
gambard@chromium.org changed reviewers: + lpromero@chromium.org
PTAL.
https://codereview.chromium.org/2736653002/diff/20001/ios/chrome/browser/cont... File ios/chrome/browser/content_suggestions/content_suggestions_coordinator.mm (right): https://codereview.chromium.org/2736653002/diff/20001/ios/chrome/browser/cont... ios/chrome/browser/content_suggestions/content_suggestions_coordinator.mm:98: _visible = NO; Also discard your mediator. https://codereview.chromium.org/2736653002/diff/20001/ios/chrome/browser/cont... ios/chrome/browser/content_suggestions/content_suggestions_coordinator.mm:137: __weak ContentSuggestionsArticleItem* weakArticle = articleItem; What is the issue you are avoiding by using a weak pointer? If the coordinator is still alive but the article has been destroyed? For example by the backend? In that case, what happens when dismissArticle:atIndexPath: is called with a nil item and an index path? https://codereview.chromium.org/2736653002/diff/20001/ios/chrome/browser/cont... File ios/chrome/browser/content_suggestions/content_suggestions_mediator.h (right): https://codereview.chromium.org/2736653002/diff/20001/ios/chrome/browser/cont... ios/chrome/browser/content_suggestions/content_suggestions_mediator.h:30: - (void)dismissSuggestion:(ContentSuggestionIdentifier*)suggestionIdentifier; Add a comment that UI code calls this to remove the suggestion from the content service. Otherwise it could be ambiguous. https://codereview.chromium.org/2736653002/diff/20001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/content_suggestions/content_suggestions_article_item.mm (right): https://codereview.chromium.org/2736653002/diff/20001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/content_suggestions_article_item.mm:74: return image; No action needed: This seems orthogonal to the initial purpose of the CL, no? Try to make smaller and atomic CLs if possible going forward, so it's easier to review and track. https://codereview.chromium.org/2736653002/diff/20001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/content_suggestions_article_item.mm:174: parentWidth - kImageSize - 3 * kStandardSpacing; No action needed: Idem, was this related to deletion? To empty image background? https://codereview.chromium.org/2736653002/diff/20001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/content_suggestions/content_suggestions_view_controller.h (right): https://codereview.chromium.org/2736653002/diff/20001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/content_suggestions_view_controller.h:33: - (void)dismissEntryAtIndexPath:(NSIndexPath*)indexPath; Add a comment, so we know what is the contract: remove from collection view only, or from view and CVModel? https://codereview.chromium.org/2736653002/diff/20001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/content_suggestions/content_suggestions_view_controller.mm (right): https://codereview.chromium.org/2736653002/diff/20001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/content_suggestions_view_controller.mm:62: willDeleteItemsAtIndexPaths:@[ indexPath ]]; Why do you call this? So that the item is removed from the model? Should willDelete be outside performBatchUpdates and the deleteItems call be inside? Actually, why do you need the performBatchUpdates?
Thanks, PTAL. https://codereview.chromium.org/2736653002/diff/20001/ios/chrome/browser/cont... File ios/chrome/browser/content_suggestions/content_suggestions_coordinator.mm (right): https://codereview.chromium.org/2736653002/diff/20001/ios/chrome/browser/cont... ios/chrome/browser/content_suggestions/content_suggestions_coordinator.mm:98: _visible = NO; On 2017/03/06 10:29:00, lpromero wrote: > Also discard your mediator. Done. https://codereview.chromium.org/2736653002/diff/20001/ios/chrome/browser/cont... ios/chrome/browser/content_suggestions/content_suggestions_coordinator.mm:137: __weak ContentSuggestionsArticleItem* weakArticle = articleItem; On 2017/03/06 10:29:00, lpromero wrote: > What is the issue you are avoiding by using a weak pointer? If the coordinator > is still alive but the article has been destroyed? For example by the backend? > In that case, what happens when dismissArticle:atIndexPath: is called with a nil > item and an index path? The idea is to prevent deleting an item twice: if it has been destroyed by the backend and already removed from the collection view, we would remove twice the item at its index path. I forgot the guard in |dismissArticle:atIndexPath| in this patch. https://codereview.chromium.org/2736653002/diff/20001/ios/chrome/browser/cont... File ios/chrome/browser/content_suggestions/content_suggestions_mediator.h (right): https://codereview.chromium.org/2736653002/diff/20001/ios/chrome/browser/cont... ios/chrome/browser/content_suggestions/content_suggestions_mediator.h:30: - (void)dismissSuggestion:(ContentSuggestionIdentifier*)suggestionIdentifier; On 2017/03/06 10:29:00, lpromero wrote: > Add a comment that UI code calls this to remove the suggestion from the content > service. Otherwise it could > be ambiguous. It should not be ambiguous as the mediator does not have a reference to the UI (by design) :) I have added a comment. https://codereview.chromium.org/2736653002/diff/20001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/content_suggestions/content_suggestions_article_item.mm (right): https://codereview.chromium.org/2736653002/diff/20001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/content_suggestions_article_item.mm:74: return image; On 2017/03/06 10:29:00, lpromero wrote: > No action needed: This seems orthogonal to the initial purpose of the CL, no? > Try to make smaller and atomic CLs if possible going forward, so it's easier to > review and track. It should have been removed before sending for review. Sorry. https://codereview.chromium.org/2736653002/diff/20001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/content_suggestions_article_item.mm:174: parentWidth - kImageSize - 3 * kStandardSpacing; On 2017/03/06 10:29:00, lpromero wrote: > No action needed: Idem, was this related to deletion? To empty image background? Same. https://codereview.chromium.org/2736653002/diff/20001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/content_suggestions/content_suggestions_view_controller.h (right): https://codereview.chromium.org/2736653002/diff/20001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/content_suggestions_view_controller.h:33: - (void)dismissEntryAtIndexPath:(NSIndexPath*)indexPath; On 2017/03/06 10:29:00, lpromero wrote: > Add a comment, so we know what is the contract: remove from collection view > only, or from view and CVModel? Well, for me the fact that it has a CVModel is an implementation detail. But I have added it in comment. https://codereview.chromium.org/2736653002/diff/20001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/content_suggestions/content_suggestions_view_controller.mm (right): https://codereview.chromium.org/2736653002/diff/20001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/content_suggestions_view_controller.mm:62: willDeleteItemsAtIndexPaths:@[ indexPath ]]; On 2017/03/06 10:29:00, lpromero wrote: > Why do you call this? So that the item is removed from the model? > Should willDelete be outside performBatchUpdates and the deleteItems call be > inside? > Actually, why do you need the performBatchUpdates? I use performBatchUpdates defensively because the UIColllectionView does not support updating multiple cells in the same runloop. I don't understand why willDelete (which delete items from the model) should be outside? I think you were the one asking me to put it inside the performBatchUpdate (in Reading List).
lgtm https://codereview.chromium.org/2736653002/diff/20001/ios/chrome/browser/cont... File ios/chrome/browser/content_suggestions/content_suggestions_coordinator.mm (right): https://codereview.chromium.org/2736653002/diff/20001/ios/chrome/browser/cont... ios/chrome/browser/content_suggestions/content_suggestions_coordinator.mm:137: __weak ContentSuggestionsArticleItem* weakArticle = articleItem; On 2017/03/06 12:16:00, gambard wrote: > On 2017/03/06 10:29:00, lpromero wrote: > > What is the issue you are avoiding by using a weak pointer? If the coordinator > > is still alive but the article has been destroyed? For example by the backend? > > In that case, what happens when dismissArticle:atIndexPath: is called with a > nil > > item and an index path? > > The idea is to prevent deleting an item twice: if it has been destroyed by the > backend and already removed from the collection view, we would remove twice the > item at its index path. > I forgot the guard in |dismissArticle:atIndexPath| in this patch. One issue with this: in a ref counted environment, you can't guarantee when an item is destroyed. I think you should not rely on this. Since articles have an identifier, couldn't you use this instead to prevent attempt to double delete? Other possibility: how come double deletes can happen at this stage? https://codereview.chromium.org/2736653002/diff/20001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/content_suggestions/content_suggestions_article_item.mm (right): https://codereview.chromium.org/2736653002/diff/20001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/content_suggestions_article_item.mm:74: return image; On 2017/03/06 12:16:00, gambard wrote: > On 2017/03/06 10:29:00, lpromero wrote: > > No action needed: This seems orthogonal to the initial purpose of the CL, no? > > Try to make smaller and atomic CLs if possible going forward, so it's easier > to > > review and track. > > It should have been removed before sending for review. Sorry. No problem :) https://codereview.chromium.org/2736653002/diff/20001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/content_suggestions/content_suggestions_view_controller.h (right): https://codereview.chromium.org/2736653002/diff/20001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/content_suggestions_view_controller.h:33: - (void)dismissEntryAtIndexPath:(NSIndexPath*)indexPath; On 2017/03/06 12:16:00, gambard wrote: > On 2017/03/06 10:29:00, lpromero wrote: > > Add a comment, so we know what is the contract: remove from collection view > > only, or from view and CVModel? > > Well, for me the fact that it has a CVModel is an implementation detail. But I > have added it in comment. True, too much details. As a rule of thumb, we should try to describe the "high-level" flow instead of what it does underneath. Maybe the comment should say something like: updates the view when the user wants to delete an entry. Anyway, looks good as is. https://codereview.chromium.org/2736653002/diff/20001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/content_suggestions/content_suggestions_view_controller.mm (right): https://codereview.chromium.org/2736653002/diff/20001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/content_suggestions_view_controller.mm:62: willDeleteItemsAtIndexPaths:@[ indexPath ]]; On 2017/03/06 12:16:00, gambard wrote: > On 2017/03/06 10:29:00, lpromero wrote: > > Why do you call this? So that the item is removed from the model? > > Should willDelete be outside performBatchUpdates and the deleteItems call be > > inside? > > Actually, why do you need the performBatchUpdates? > > I use performBatchUpdates defensively because the UIColllectionView does not > support updating multiple cells in the same runloop. Can -dismissEntryAtIndexPath: be called several times in a same turn of run loop? Will calls be queued and run in their own turn? > I don't understand why willDelete (which delete items from the model) should be > outside? I think you were the one asking me to put it inside the > performBatchUpdate (in Reading List). Ah I remember: https://github.com/material-components/material-components-ios/blob/620b5be11... Thanks for the refresher! https://codereview.chromium.org/2736653002/diff/40001/ios/chrome/browser/cont... File ios/chrome/browser/content_suggestions/content_suggestions_coordinator.mm (right): https://codereview.chromium.org/2736653002/diff/40001/ios/chrome/browser/cont... ios/chrome/browser/content_suggestions/content_suggestions_coordinator.mm:165: [weakSelf dismissArticle:weakArticle Did you want to add the guard in this CL? https://codereview.chromium.org/2736653002/diff/40001/ios/chrome/browser/cont... File ios/chrome/browser/content_suggestions/content_suggestions_mediator.h (right): https://codereview.chromium.org/2736653002/diff/40001/ios/chrome/browser/cont... ios/chrome/browser/content_suggestions/content_suggestions_mediator.h:30: // Dismiss the suggestion from the content suggestions service. It does change Dismisses https://codereview.chromium.org/2736653002/diff/40001/ios/chrome/browser/cont... ios/chrome/browser/content_suggestions/content_suggestions_mediator.h:30: // Dismiss the suggestion from the content suggestions service. It does change Did you mean: it doesn't change the UI? What I meant in my earlier comment was: the mediator can be a two way passthrough between the user and the data. This method removes from the data (model) after a user event. (IIUC)
Thanks! https://codereview.chromium.org/2736653002/diff/20001/ios/chrome/browser/cont... File ios/chrome/browser/content_suggestions/content_suggestions_coordinator.mm (right): https://codereview.chromium.org/2736653002/diff/20001/ios/chrome/browser/cont... ios/chrome/browser/content_suggestions/content_suggestions_coordinator.mm:137: __weak ContentSuggestionsArticleItem* weakArticle = articleItem; On 2017/03/06 13:02:30, lpromero wrote: > On 2017/03/06 12:16:00, gambard wrote: > > On 2017/03/06 10:29:00, lpromero wrote: > > > What is the issue you are avoiding by using a weak pointer? If the > coordinator > > > is still alive but the article has been destroyed? For example by the > backend? > > > In that case, what happens when dismissArticle:atIndexPath: is called with a > > nil > > > item and an index path? > > > > The idea is to prevent deleting an item twice: if it has been destroyed by the > > backend and already removed from the collection view, we would remove twice > the > > item at its index path. > > I forgot the guard in |dismissArticle:atIndexPath| in this patch. > > One issue with this: in a ref counted environment, you can't guarantee when an > item is destroyed. I think you should not rely on this. Since articles have an > identifier, couldn't you use this instead to prevent attempt to double delete? > Other possibility: how come double deletes can happen at this stage? I know but some guards is better than none. With the interactions as they are today, the item cannot be dismissed twice. But once the content suggestions service is updated in the background, it might change. https://codereview.chromium.org/2736653002/diff/20001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/content_suggestions/content_suggestions_view_controller.h (right): https://codereview.chromium.org/2736653002/diff/20001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/content_suggestions_view_controller.h:33: - (void)dismissEntryAtIndexPath:(NSIndexPath*)indexPath; On 2017/03/06 13:02:30, lpromero wrote: > On 2017/03/06 12:16:00, gambard wrote: > > On 2017/03/06 10:29:00, lpromero wrote: > > > Add a comment, so we know what is the contract: remove from collection view > > > only, or from view and CVModel? > > > > Well, for me the fact that it has a CVModel is an implementation detail. But I > > have added it in comment. > > True, too much details. As a rule of thumb, we should try to describe the > "high-level" flow instead of what it does underneath. Maybe the comment should > say something like: updates the view when the user wants to delete an entry. > Anyway, looks good as is. Acknowledged. https://codereview.chromium.org/2736653002/diff/20001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/content_suggestions/content_suggestions_view_controller.mm (right): https://codereview.chromium.org/2736653002/diff/20001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/content_suggestions_view_controller.mm:62: willDeleteItemsAtIndexPaths:@[ indexPath ]]; On 2017/03/06 13:02:30, lpromero wrote: > On 2017/03/06 12:16:00, gambard wrote: > > On 2017/03/06 10:29:00, lpromero wrote: > > > Why do you call this? So that the item is removed from the model? > > > Should willDelete be outside performBatchUpdates and the deleteItems call be > > > inside? > > > Actually, why do you need the performBatchUpdates? > > > > I use performBatchUpdates defensively because the UIColllectionView does not > > support updating multiple cells in the same runloop. > > Can -dismissEntryAtIndexPath: be called several times in a same turn of run > loop? Will calls be queued and run in their own turn? > > > I don't understand why willDelete (which delete items from the model) should > be > > outside? I think you were the one asking me to put it inside the > > performBatchUpdate (in Reading List). > > Ah I remember: > https://github.com/material-components/material-components-ios/blob/620b5be11... > Thanks for the refresher! For now it should not be called in the same turn of runloop. But in the future maybe (and this API is public so I don't want to enforce something like "spin the runloop between two call of this method"). By having performBatchUpdates, the updates will queue nicely if called during the same turn of runloop. https://codereview.chromium.org/2736653002/diff/40001/ios/chrome/browser/cont... File ios/chrome/browser/content_suggestions/content_suggestions_coordinator.mm (right): https://codereview.chromium.org/2736653002/diff/40001/ios/chrome/browser/cont... ios/chrome/browser/content_suggestions/content_suggestions_coordinator.mm:165: [weakSelf dismissArticle:weakArticle On 2017/03/06 13:02:31, lpromero wrote: > Did you want to add the guard in this CL? I have added it to the function. https://codereview.chromium.org/2736653002/diff/40001/ios/chrome/browser/cont... File ios/chrome/browser/content_suggestions/content_suggestions_mediator.h (right): https://codereview.chromium.org/2736653002/diff/40001/ios/chrome/browser/cont... ios/chrome/browser/content_suggestions/content_suggestions_mediator.h:30: // Dismiss the suggestion from the content suggestions service. It does change On 2017/03/06 13:02:31, lpromero wrote: > Did you mean: it doesn't change the UI? > > What I meant in my earlier comment was: the mediator can be a two way > passthrough between the user and the data. This method removes from the data > (model) after a user event. (IIUC) Yes for now, but it won't necessarily be called from the UI. So I prefer this comment instead of specifying from where this should be called. https://codereview.chromium.org/2736653002/diff/40001/ios/chrome/browser/cont... ios/chrome/browser/content_suggestions/content_suggestions_mediator.h:30: // Dismiss the suggestion from the content suggestions service. It does change On 2017/03/06 13:02:31, lpromero wrote: > Dismisses Done.
The CQ bit was checked by gambard@chromium.org
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/2736653002/#ps60001 (title: "Address comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1488821486609970,
"parent_rev": "c3cd10e07d20e7498c94a3fbe3735f0bb68c3296", "commit_rev":
"4c48f1a300005c92a453e7864979b4780c2a9ac7"}
Message was sent while issue was closed.
Description was changed from ========== Suggested Articles can be dismissed Add the logic allowing the suggested articles to be dismissed in the UI and in the backend. BUG=693031, 698247, 698685 ========== to ========== Suggested Articles can be dismissed Add the logic allowing the suggested articles to be dismissed in the UI and in the backend. BUG=693031, 698247, 698685 Review-Url: https://codereview.chromium.org/2736653002 Cr-Commit-Position: refs/heads/master@{#454882} Committed: https://chromium.googlesource.com/chromium/src/+/4c48f1a300005c92a453e7864979... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/4c48f1a300005c92a453e7864979... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
