Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2815623002:
ReadingListProvider handles dismissal (Closed)
|
Created:
3 years, 8 months ago by gambard Modified:
3 years, 8 months ago Reviewers:
Marc Treib CC:
chromium-reviews, stkhapugin, noyau+watch_chromium.org, ntp-dev+reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionReadingListProvider handles dismissal
Implements the logic of the dismissal of Reading List entries in their provider.
BUG=707730, 702241
Review-Url: https://codereview.chromium.org/2815623002
Cr-Commit-Position: refs/heads/master@{#463989}
Committed: https://chromium.googlesource.com/chromium/src/+/1f0337d7e2d5fc26c25f264e83a726d56ba4ee0d
Patch Set 1 #Patch Set 2 : Rebase #
Total comments: 19
Patch Set 3 : Address comments #
Total comments: 10
Patch Set 4 : Address comments #Patch Set 5 : Address comments #
Messages
Total messages: 22 (13 generated)
gambard@chromium.org changed reviewers: + treib@chromium.org
PTAL. I am not completely sure about my understanding of the different method (clearHistory/GetDismissedSuggestionsForDebugging in particular).
https://codereview.chromium.org/2815623002/diff/20001/components/ntp_snippets... File components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc (right): https://codereview.chromium.org/2815623002/diff/20001/components/ntp_snippets... components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc:103: // Ignored. nit: Mention why it's okay to ignore? (I'd assume the ReadingListModel already handles this correctly.) Background: This is for the case when the provider itself stores any history-related state, which should get cleared during the "clear browsing data" process. https://codereview.chromium.org/2815623002/diff/20001/components/ntp_snippets... components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc:115: // vector. So essentially "not implemented"... hm. Couldn't you just go over reading_list_model_ and find all entries that have the dismissed flag set? This is used for about:snippets-internals (which I'm not sure exists on iOS, but if it doesn't, we should implement it). https://codereview.chromium.org/2815623002/diff/20001/components/ntp_snippets... components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc:118: void ReadingListSuggestionsProvider::ClearDismissedSuggestionsForDebugging( nit: empty line before https://codereview.chromium.org/2815623002/diff/20001/components/ntp_snippets... components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc:221: reading_list_model_->SetContentSuggestionsExtra(url, extra); Is the ContentSuggestionsExtra stuff actually implemented anywhere? Code search doesn't find it..? https://codereview.chromium.org/2815623002/diff/20001/components/ntp_snippets... File components/ntp_snippets/reading_list/reading_list_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2815623002/diff/20001/components/ntp_snippets... components/ntp_snippets/reading_list/reading_list_suggestions_provider_unittest.cc:73: GURL url_unread1_ = GURL("http://www.foo1.bar"); All these should probably be const, and can be initialized like this: const GURL url_unread1_("http://www.foo1.bar"); Alternatively, it might actually be better to just duplicate those in the tests that need them. IMO that'd make the tests easier to read, since the "input" and the expected result are closer together. Up to you though.
Thanks, PTAL. https://codereview.chromium.org/2815623002/diff/20001/components/ntp_snippets... File components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc (right): https://codereview.chromium.org/2815623002/diff/20001/components/ntp_snippets... components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc:103: // Ignored. On 2017/04/11 12:35:15, Marc Treib wrote: > nit: Mention why it's okay to ignore? (I'd assume the ReadingListModel already > handles this correctly.) > > Background: This is for the case when the provider itself stores any > history-related state, which should get cleared during the "clear browsing data" > process. Reading List has nothing to do with history :) I have added a comment https://codereview.chromium.org/2815623002/diff/20001/components/ntp_snippets... components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc:115: // vector. On 2017/04/11 12:35:15, Marc Treib wrote: > So essentially "not implemented"... hm. > Couldn't you just go over reading_list_model_ and find all entries that have the > dismissed flag set? > > This is used for about:snippets-internals (which I'm not sure exists on iOS, but > if it doesn't, we should implement it). Ok, but I guess the comment in the content_suggestions_provider.h should be updated :) We are not using about:snippets-internals on iOS. Should we? https://codereview.chromium.org/2815623002/diff/20001/components/ntp_snippets... components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc:118: void ReadingListSuggestionsProvider::ClearDismissedSuggestionsForDebugging( On 2017/04/11 12:35:15, Marc Treib wrote: > nit: empty line before Done. https://codereview.chromium.org/2815623002/diff/20001/components/ntp_snippets... components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc:221: reading_list_model_->SetContentSuggestionsExtra(url, extra); On 2017/04/11 12:35:15, Marc Treib wrote: > Is the ContentSuggestionsExtra stuff actually implemented anywhere? Code search > doesn't find it..? It is the depend CL :) https://codereview.chromium.org/2815623002/diff/20001/components/ntp_snippets... File components/ntp_snippets/reading_list/reading_list_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2815623002/diff/20001/components/ntp_snippets... components/ntp_snippets/reading_list/reading_list_suggestions_provider_unittest.cc:73: GURL url_unread1_ = GURL("http://www.foo1.bar"); On 2017/04/11 12:35:15, Marc Treib wrote: > All these should probably be const, and can be initialized like this: > const GURL url_unread1_("http://www.foo1.bar"); > > Alternatively, it might actually be better to just duplicate those in the tests > that need them. IMO that'd make the tests easier to read, since the "input" and > the expected result are closer together. Up to you though. If I move it to the test I need to move the AddEntries also and it create a lot a duplication. I created a constant.
LGTM with some more nits https://codereview.chromium.org/2815623002/diff/20001/components/ntp_snippets... File components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc (right): https://codereview.chromium.org/2815623002/diff/20001/components/ntp_snippets... components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc:115: // vector. On 2017/04/11 13:29:53, gambard wrote: > On 2017/04/11 12:35:15, Marc Treib wrote: > > So essentially "not implemented"... hm. > > Couldn't you just go over reading_list_model_ and find all entries that have > the > > dismissed flag set? > > > > This is used for about:snippets-internals (which I'm not sure exists on iOS, > but > > if it doesn't, we should implement it). > > Ok, but I guess the comment in the content_suggestions_provider.h should be > updated :) You mean the "may return an empty vector" part? I think that still applies; it's just that here it's not very difficult to implement it properly, so why not do it :) > We are not using about:snippets-internals on iOS. Should we? Yes, we should definitely get that working before launch. It's proven very useful in debugging problems in the wild. https://codereview.chromium.org/2815623002/diff/20001/components/ntp_snippets... components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc:221: reading_list_model_->SetContentSuggestionsExtra(url, extra); On 2017/04/11 13:29:53, gambard wrote: > On 2017/04/11 12:35:15, Marc Treib wrote: > > Is the ContentSuggestionsExtra stuff actually implemented anywhere? Code > search > > doesn't find it..? > > It is the depend CL :) Aah, missed that. Thanks! https://codereview.chromium.org/2815623002/diff/20001/components/ntp_snippets... File components/ntp_snippets/reading_list/reading_list_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2815623002/diff/20001/components/ntp_snippets... components/ntp_snippets/reading_list/reading_list_suggestions_provider_unittest.cc:73: GURL url_unread1_ = GURL("http://www.foo1.bar"); On 2017/04/11 13:29:53, gambard wrote: > On 2017/04/11 12:35:15, Marc Treib wrote: > > All these should probably be const, and can be initialized like this: > > const GURL url_unread1_("http://www.foo1.bar"); > > > > Alternatively, it might actually be better to just duplicate those in the > tests > > that need them. IMO that'd make the tests easier to read, since the "input" > and > > the expected result are closer together. Up to you though. > > If I move it to the test I need to move the AddEntries also and it create a lot > a duplication. > I created a constant. In tests, IMO duplication is sometimes better than making the reader jump around between the actual test and the test class. But again, up to you. https://codereview.chromium.org/2815623002/diff/40001/components/ntp_snippets... File components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc (right): https://codereview.chromium.org/2815623002/diff/40001/components/ntp_snippets... components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc:114: if (!reading_list_model_ || reading_list_model_->IsPerformingBatchUpdates()) { Can reading_list_model_ be null? https://codereview.chromium.org/2815623002/diff/40001/components/ntp_snippets... components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc:115: return; Should probably still call the callback with an empty vector in that case? https://codereview.chromium.org/2815623002/diff/40001/components/ntp_snippets... File components/ntp_snippets/reading_list/reading_list_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2815623002/diff/40001/components/ntp_snippets... components/ntp_snippets/reading_list/reading_list_suggestions_provider_unittest.cc:28: const std::string kTitleRead1 = "title_read1"; I think this will introduce static initializers, which are banned. If you have the constants here, you probably have to make them const char[]. https://codereview.chromium.org/2815623002/diff/40001/components/ntp_snippets... components/ntp_snippets/reading_list/reading_list_suggestions_provider_unittest.cc:150: Category::FromKnownCategory(KnownCategories::READING_LIST), ReadingListCategory()
Thanks! https://codereview.chromium.org/2815623002/diff/20001/components/ntp_snippets... File components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc (right): https://codereview.chromium.org/2815623002/diff/20001/components/ntp_snippets... components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc:115: // vector. On 2017/04/11 13:41:08, Marc Treib wrote: > On 2017/04/11 13:29:53, gambard wrote: > > On 2017/04/11 12:35:15, Marc Treib wrote: > > > So essentially "not implemented"... hm. > > > Couldn't you just go over reading_list_model_ and find all entries that have > > the > > > dismissed flag set? > > > > > > This is used for about:snippets-internals (which I'm not sure exists on iOS, > > but > > > if it doesn't, we should implement it). > > > > Ok, but I guess the comment in the content_suggestions_provider.h should be > > updated :) > > You mean the "may return an empty vector" part? I think that still applies; it's > just that here it's not very difficult to implement it properly, so why not do > it :) > > > We are not using about:snippets-internals on iOS. Should we? > > Yes, we should definitely get that working before launch. It's proven very > useful in debugging problems in the wild. Isn't it the page implemented in chrome/browser/ui/webui? We don't pull that code in iOS. https://codereview.chromium.org/2815623002/diff/20001/components/ntp_snippets... File components/ntp_snippets/reading_list/reading_list_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2815623002/diff/20001/components/ntp_snippets... components/ntp_snippets/reading_list/reading_list_suggestions_provider_unittest.cc:73: GURL url_unread1_ = GURL("http://www.foo1.bar"); On 2017/04/11 13:41:08, Marc Treib wrote: > On 2017/04/11 13:29:53, gambard wrote: > > On 2017/04/11 12:35:15, Marc Treib wrote: > > > All these should probably be const, and can be initialized like this: > > > const GURL url_unread1_("http://www.foo1.bar"); > > > > > > Alternatively, it might actually be better to just duplicate those in the > > tests > > > that need them. IMO that'd make the tests easier to read, since the "input" > > and > > > the expected result are closer together. Up to you though. > > > > If I move it to the test I need to move the AddEntries also and it create a > lot > > a duplication. > > I created a constant. > > In tests, IMO duplication is sometimes better than making the reader jump around > between the actual test and the test class. But again, up to you. I prefer it that way, as the values does not hold any meaning. https://codereview.chromium.org/2815623002/diff/40001/components/ntp_snippets... File components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc (right): https://codereview.chromium.org/2815623002/diff/40001/components/ntp_snippets... components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc:114: if (!reading_list_model_ || reading_list_model_->IsPerformingBatchUpdates()) { On 2017/04/11 13:41:09, Marc Treib wrote: > Can reading_list_model_ be null? Yes. Looks at ReadingListSuggestionsProvider::ReadingListModelBeingDeleted https://codereview.chromium.org/2815623002/diff/40001/components/ntp_snippets... components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc:115: return; On 2017/04/11 13:41:09, Marc Treib wrote: > Should probably still call the callback with an empty vector in that case? Done. https://codereview.chromium.org/2815623002/diff/40001/components/ntp_snippets... File components/ntp_snippets/reading_list/reading_list_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2815623002/diff/40001/components/ntp_snippets... components/ntp_snippets/reading_list/reading_list_suggestions_provider_unittest.cc:28: const std::string kTitleRead1 = "title_read1"; On 2017/04/11 13:41:09, Marc Treib wrote: > I think this will introduce static initializers, which are banned. If you have > the constants here, you probably have to make them const char[]. Done. https://codereview.chromium.org/2815623002/diff/40001/components/ntp_snippets... components/ntp_snippets/reading_list/reading_list_suggestions_provider_unittest.cc:150: Category::FromKnownCategory(KnownCategories::READING_LIST), On 2017/04/11 13:41:09, Marc Treib wrote: > ReadingListCategory() Done.
The CQ bit was checked by gambard@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2815623002/diff/20001/components/ntp_snippets... File components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc (right): https://codereview.chromium.org/2815623002/diff/20001/components/ntp_snippets... components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc:115: // vector. On 2017/04/11 15:01:18, gambard wrote: > On 2017/04/11 13:41:08, Marc Treib wrote: > > On 2017/04/11 13:29:53, gambard wrote: > > > On 2017/04/11 12:35:15, Marc Treib wrote: > > > > So essentially "not implemented"... hm. > > > > Couldn't you just go over reading_list_model_ and find all entries that > have > > > the > > > > dismissed flag set? > > > > > > > > This is used for about:snippets-internals (which I'm not sure exists on > iOS, > > > but > > > > if it doesn't, we should implement it). > > > > > > Ok, but I guess the comment in the content_suggestions_provider.h should be > > > updated :) > > > > You mean the "may return an empty vector" part? I think that still applies; > it's > > just that here it's not very difficult to implement it properly, so why not do > > it :) > > > > > We are not using about:snippets-internals on iOS. Should we? > > > > Yes, we should definitely get that working before launch. It's proven very > > useful in debugging problems in the wild. > > Isn't it the page implemented in chrome/browser/ui/webui? We don't pull that > code in iOS. Currently it is, yes. See e.g. components/ntp_tiles/webui/ for an example of what needs to be done to make things work on iOS. https://codereview.chromium.org/2815623002/diff/20001/components/ntp_snippets... File components/ntp_snippets/reading_list/reading_list_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2815623002/diff/20001/components/ntp_snippets... components/ntp_snippets/reading_list/reading_list_suggestions_provider_unittest.cc:73: GURL url_unread1_ = GURL("http://www.foo1.bar"); On 2017/04/11 15:01:18, gambard wrote: > On 2017/04/11 13:41:08, Marc Treib wrote: > > On 2017/04/11 13:29:53, gambard wrote: > > > On 2017/04/11 12:35:15, Marc Treib wrote: > > > > All these should probably be const, and can be initialized like this: > > > > const GURL url_unread1_("http://www.foo1.bar"); > > > > > > > > Alternatively, it might actually be better to just duplicate those in the > > > tests > > > > that need them. IMO that'd make the tests easier to read, since the > "input" > > > and > > > > the expected result are closer together. Up to you though. > > > > > > If I move it to the test I need to move the AddEntries also and it create a > > lot > > > a duplication. > > > I created a constant. > > > > In tests, IMO duplication is sometimes better than making the reader jump > around > > between the actual test and the test class. But again, up to you. > > I prefer it that way, as the values does not hold any meaning. What does hold a meaning is the times that are attached to the URLs, because they affect the sorting. https://codereview.chromium.org/2815623002/diff/40001/components/ntp_snippets... File components/ntp_snippets/reading_list/reading_list_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2815623002/diff/40001/components/ntp_snippets... components/ntp_snippets/reading_list/reading_list_suggestions_provider_unittest.cc:28: const std::string kTitleRead1 = "title_read1"; On 2017/04/11 15:01:18, gambard wrote: > On 2017/04/11 13:41:09, Marc Treib wrote: > > I think this will introduce static initializers, which are banned. If you have > > the constants here, you probably have to make them const char[]. > > Done. The URLs too :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks :) https://codereview.chromium.org/2815623002/diff/20001/components/ntp_snippets... File components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc (right): https://codereview.chromium.org/2815623002/diff/20001/components/ntp_snippets... components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc:115: // vector. On 2017/04/11 15:28:14, Marc Treib wrote: > On 2017/04/11 15:01:18, gambard wrote: > > On 2017/04/11 13:41:08, Marc Treib wrote: > > > On 2017/04/11 13:29:53, gambard wrote: > > > > On 2017/04/11 12:35:15, Marc Treib wrote: > > > > > So essentially "not implemented"... hm. > > > > > Couldn't you just go over reading_list_model_ and find all entries that > > have > > > > the > > > > > dismissed flag set? > > > > > > > > > > This is used for about:snippets-internals (which I'm not sure exists on > > iOS, > > > > but > > > > > if it doesn't, we should implement it). > > > > > > > > Ok, but I guess the comment in the content_suggestions_provider.h should > be > > > > updated :) > > > > > > You mean the "may return an empty vector" part? I think that still applies; > > it's > > > just that here it's not very difficult to implement it properly, so why not > do > > > it :) > > > > > > > We are not using about:snippets-internals on iOS. Should we? > > > > > > Yes, we should definitely get that working before launch. It's proven very > > > useful in debugging problems in the wild. > > > > Isn't it the page implemented in chrome/browser/ui/webui? We don't pull that > > code in iOS. > > Currently it is, yes. See e.g. components/ntp_tiles/webui/ for an example of > what needs to be done to make things work on iOS. I have created crbug.com/710769 https://codereview.chromium.org/2815623002/diff/20001/components/ntp_snippets... File components/ntp_snippets/reading_list/reading_list_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2815623002/diff/20001/components/ntp_snippets... components/ntp_snippets/reading_list/reading_list_suggestions_provider_unittest.cc:73: GURL url_unread1_ = GURL("http://www.foo1.bar"); On 2017/04/11 15:28:14, Marc Treib wrote: > On 2017/04/11 15:01:18, gambard wrote: > > On 2017/04/11 13:41:08, Marc Treib wrote: > > > On 2017/04/11 13:29:53, gambard wrote: > > > > On 2017/04/11 12:35:15, Marc Treib wrote: > > > > > All these should probably be const, and can be initialized like this: > > > > > const GURL url_unread1_("http://www.foo1.bar"); > > > > > > > > > > Alternatively, it might actually be better to just duplicate those in > the > > > > tests > > > > > that need them. IMO that'd make the tests easier to read, since the > > "input" > > > > and > > > > > the expected result are closer together. Up to you though. > > > > > > > > If I move it to the test I need to move the AddEntries also and it create > a > > > lot > > > > a duplication. > > > > I created a constant. > > > > > > In tests, IMO duplication is sometimes better than making the reader jump > > around > > > between the actual test and the test class. But again, up to you. > > > > I prefer it that way, as the values does not hold any meaning. > > What does hold a meaning is the times that are attached to the URLs, because > they affect the sorting. Yes. https://codereview.chromium.org/2815623002/diff/40001/components/ntp_snippets... File components/ntp_snippets/reading_list/reading_list_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2815623002/diff/40001/components/ntp_snippets... components/ntp_snippets/reading_list/reading_list_suggestions_provider_unittest.cc:28: const std::string kTitleRead1 = "title_read1"; On 2017/04/11 15:28:15, Marc Treib wrote: > On 2017/04/11 15:01:18, gambard wrote: > > On 2017/04/11 13:41:09, Marc Treib wrote: > > > I think this will introduce static initializers, which are banned. If you > have > > > the constants here, you probably have to make them const char[]. > > > > Done. > > The URLs too :) Moved to the class :)
The CQ bit was checked by gambard@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by gambard@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org Link to the patchset: https://codereview.chromium.org/2815623002/#ps80001 (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": 80001, "attempt_start_ts": 1491994597039060,
"parent_rev": "21ed71af85150ae613b7cb6ec62d01b393e1ee59", "commit_rev":
"1f0337d7e2d5fc26c25f264e83a726d56ba4ee0d"}
Message was sent while issue was closed.
Description was changed from ========== ReadingListProvider handles dismissal Implements the logic of the dismissal of Reading List entries in their provider. BUG=707730, 702241 ========== to ========== ReadingListProvider handles dismissal Implements the logic of the dismissal of Reading List entries in their provider. BUG=707730, 702241 Review-Url: https://codereview.chromium.org/2815623002 Cr-Commit-Position: refs/heads/master@{#463989} Committed: https://chromium.googlesource.com/chromium/src/+/1f0337d7e2d5fc26c25f264e83a7... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/1f0337d7e2d5fc26c25f264e83a7... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
