Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2770893003:
Add logic for fetching the Reading List entries (Closed)
|
Created:
3 years, 9 months ago by gambard Modified:
3 years, 8 months ago CC:
chromium-reviews, ios-reviews_chromium.org, droger+watchlist_chromium.org, blundell+watchlist_chromium.org, sdefresne+watchlist_chromium.org, noyau+watch_chromium.org, ntp-dev+reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd logic for fetching the Reading List entries
This CL adds the logic in the ReadingListProvider, allowing it to fetch the
entries to be displayed.
BUG=702241
Review-Url: https://codereview.chromium.org/2770893003
Cr-Commit-Position: refs/heads/master@{#460697}
Committed: https://chromium.googlesource.com/chromium/src/+/250e9409aec179c1a8427adf454309e1d2f6482f
Patch Set 1 #Patch Set 2 : Add tests #Patch Set 3 : Rebase #Patch Set 4 : Rebase #
Total comments: 4
Patch Set 5 : Address comments #
Total comments: 28
Patch Set 6 : Address comments #Patch Set 7 : Cleanup #
Total comments: 8
Patch Set 8 : Address comments #Patch Set 9 : Change publisher #Patch Set 10 : Add title logic #
Dependent Patchsets: Messages
Total messages: 34 (14 generated)
gambard@chromium.org changed reviewers: + olivierrobin@chromium.org
PTAL.
https://codereview.chromium.org/2770893003/diff/60001/components/ntp_snippets... File components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc (right): https://codereview.chromium.org/2770893003/diff/60001/components/ntp_snippets... components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc:112: ReadingListModelBeingDeleted() { reading_list_model_->RemoveObserver(this); reading_list_model_ = nullptr; } https://codereview.chromium.org/2770893003/diff/60001/components/ntp_snippets... components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc:140: suggestion.set_title(base::UTF8ToUTF16(entry->Title())); What if title is empty ? Should we adopt the same logic as in ReadingListView?
gambard@chromium.org changed reviewers: + brettw@chromium.org, treib@chromium.org
Thanks, PTAL. +treib@: PTAL +brettw@ for DEPS include of components/url_formatter. https://codereview.chromium.org/2770893003/diff/60001/components/ntp_snippets... File components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc (right): https://codereview.chromium.org/2770893003/diff/60001/components/ntp_snippets... components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc:112: On 2017/03/28 12:33:51, Olivier Robin wrote: > ReadingListModelBeingDeleted() { > reading_list_model_->RemoveObserver(this); > reading_list_model_ = nullptr; > } Done. https://codereview.chromium.org/2770893003/diff/60001/components/ntp_snippets... components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc:140: suggestion.set_title(base::UTF8ToUTF16(entry->Title())); On 2017/03/28 12:33:51, Olivier Robin wrote: > What if title is empty ? Should we adopt the same logic as in ReadingListView? Done.
lgtm
https://codereview.chromium.org/2770893003/diff/80001/components/ntp_snippets... File components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc (left): https://codereview.chromium.org/2770893003/diff/80001/components/ntp_snippets... components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc:29: if (reading_list_model_->loaded()) { Is this not needed anymore? https://codereview.chromium.org/2770893003/diff/80001/components/ntp_snippets... File components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc (right): https://codereview.chromium.org/2770893003/diff/80001/components/ntp_snippets... components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc:19: namespace { nit: Move into the ntp_snippets namespace? https://codereview.chromium.org/2770893003/diff/80001/components/ntp_snippets... components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc:38: if (reading_list_model_) nit: braces please optional: ScopedObserver might make this simpler https://codereview.chromium.org/2770893003/diff/80001/components/ntp_snippets... components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc:127: for (const auto& url : reading_list_model_->Keys()) { s/auto/actual type/ ? IMO it's not clear what the type is otherwise. https://codereview.chromium.org/2770893003/diff/80001/components/ntp_snippets... components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc:129: if (!entry->IsRead()) { optional: "if (entry->IsRead()) { continue; }" ? https://codereview.chromium.org/2770893003/diff/80001/components/ntp_snippets... components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc:143: } Hm, IMO it's not really obvious what this is supposed to do, and that it actually does it ;) Some comments might help, or alternatively, maybe a simpler solution: Add all (unread) entries, std::partial_sort them, and resize as necessary? https://codereview.chromium.org/2770893003/diff/80001/components/ntp_snippets... components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc:152: if (entry->Title().size() > 0) { nit: !entry->Title().empty() https://codereview.chromium.org/2770893003/diff/80001/components/ntp_snippets... components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc:158: url_formatter::FormatUrl(entry->URL().GetOrigin())); If the URL is already used as the title, then this seems redundant. Maybe just leave it empty in that case? https://codereview.chromium.org/2770893003/diff/80001/components/ntp_snippets... File components/ntp_snippets/reading_list/reading_list_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2770893003/diff/80001/components/ntp_snippets... components/ntp_snippets/reading_list/reading_list_suggestions_provider_unittest.cc:56: testing::IsEmpty())) Add using declarations for the testing:: things? https://codereview.chromium.org/2770893003/diff/80001/components/ntp_snippets... components/ntp_snippets/reading_list/reading_list_suggestions_provider_unittest.cc:65: TEST_F(ReadingListSuggestionsProviderTest, ReturnedSuggestions) { nit: What does this test actually test? "ReturnsThreeLatestUnreadSuggestions"? https://codereview.chromium.org/2770893003/diff/80001/components/ntp_snippets... components/ntp_snippets/reading_list/reading_list_suggestions_provider_unittest.cc:66: GURL urlUnread1 = GURL("http://www.foo1.bar"); url_unread1 etc https://codereview.chromium.org/2770893003/diff/80001/components/ntp_snippets... components/ntp_snippets/reading_list/reading_list_suggestions_provider_unittest.cc:102: TEST_F(ReadingListSuggestionsProviderTest, OneReturnedSuggestions) { Also here: The test name should say what the test tests :)
Thanks, PTAL. https://codereview.chromium.org/2770893003/diff/80001/components/ntp_snippets... File components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc (left): https://codereview.chromium.org/2770893003/diff/80001/components/ntp_snippets... components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc:29: if (reading_list_model_->loaded()) { On 2017/03/28 13:25:15, Marc Treib wrote: > Is this not needed anymore? No, if the model is loaded when an observer is added, ReadingListModelLoaded() is called on the observer. I have added a comment to specify it. https://codereview.chromium.org/2770893003/diff/80001/components/ntp_snippets... File components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc (right): https://codereview.chromium.org/2770893003/diff/80001/components/ntp_snippets... components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc:19: namespace { On 2017/03/28 13:25:15, Marc Treib wrote: > nit: Move into the ntp_snippets namespace? Done. https://codereview.chromium.org/2770893003/diff/80001/components/ntp_snippets... components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc:38: if (reading_list_model_) On 2017/03/28 13:25:15, Marc Treib wrote: > nit: braces please > > optional: ScopedObserver might make this simpler Thanks, I did not know about ScopedObserver! https://codereview.chromium.org/2770893003/diff/80001/components/ntp_snippets... components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc:127: for (const auto& url : reading_list_model_->Keys()) { On 2017/03/28 13:25:15, Marc Treib wrote: > s/auto/actual type/ ? IMO it's not clear what the type is otherwise. Done. https://codereview.chromium.org/2770893003/diff/80001/components/ntp_snippets... components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc:129: if (!entry->IsRead()) { On 2017/03/28 13:25:16, Marc Treib wrote: > optional: "if (entry->IsRead()) { continue; }" ? Done. https://codereview.chromium.org/2770893003/diff/80001/components/ntp_snippets... components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc:143: } On 2017/03/28 13:25:16, Marc Treib wrote: > Hm, IMO it's not really obvious what this is supposed to do, and that it > actually does it ;) > Some comments might help, or alternatively, maybe a simpler solution: Add all > (unread) entries, std::partial_sort them, and resize as necessary? Done. https://codereview.chromium.org/2770893003/diff/80001/components/ntp_snippets... components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc:152: if (entry->Title().size() > 0) { On 2017/03/28 13:25:16, Marc Treib wrote: > nit: !entry->Title().empty() Done. https://codereview.chromium.org/2770893003/diff/80001/components/ntp_snippets... components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc:158: url_formatter::FormatUrl(entry->URL().GetOrigin())); On 2017/03/28 13:25:16, Marc Treib wrote: > If the URL is already used as the title, then this seems redundant. Maybe just > leave it empty in that case? The question was to do this test in the provider or a the view level. Maybe it makes more sense to have it a the view level. https://codereview.chromium.org/2770893003/diff/80001/components/ntp_snippets... File components/ntp_snippets/reading_list/reading_list_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2770893003/diff/80001/components/ntp_snippets... components/ntp_snippets/reading_list/reading_list_suggestions_provider_unittest.cc:56: testing::IsEmpty())) On 2017/03/28 13:25:16, Marc Treib wrote: > Add using declarations for the testing:: things? Done. https://codereview.chromium.org/2770893003/diff/80001/components/ntp_snippets... components/ntp_snippets/reading_list/reading_list_suggestions_provider_unittest.cc:65: TEST_F(ReadingListSuggestionsProviderTest, ReturnedSuggestions) { On 2017/03/28 13:25:16, Marc Treib wrote: > nit: What does this test actually test? "ReturnsThreeLatestUnreadSuggestions"? Done. https://codereview.chromium.org/2770893003/diff/80001/components/ntp_snippets... components/ntp_snippets/reading_list/reading_list_suggestions_provider_unittest.cc:66: GURL urlUnread1 = GURL("http://www.foo1.bar"); On 2017/03/28 13:25:16, Marc Treib wrote: > url_unread1 etc Done. https://codereview.chromium.org/2770893003/diff/80001/components/ntp_snippets... components/ntp_snippets/reading_list/reading_list_suggestions_provider_unittest.cc:102: TEST_F(ReadingListSuggestionsProviderTest, OneReturnedSuggestions) { On 2017/03/28 13:25:16, Marc Treib wrote: > Also here: The test name should say what the test tests :) Done.
Thanks! LGTM with some more nits/suggestions. https://codereview.chromium.org/2770893003/diff/80001/components/ntp_snippets... File components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc (right): https://codereview.chromium.org/2770893003/diff/80001/components/ntp_snippets... components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc:158: url_formatter::FormatUrl(entry->URL().GetOrigin())); On 2017/03/28 15:10:07, gambard wrote: > On 2017/03/28 13:25:16, Marc Treib wrote: > > If the URL is already used as the title, then this seems redundant. Maybe just > > leave it empty in that case? > > The question was to do this test in the provider or a the view level. Maybe it > makes more sense to have it a the view level. How would you check it there? After formatting, I don't know if a substring check or something like that would be sufficient. https://codereview.chromium.org/2770893003/diff/120001/components/ntp_snippet... File components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc (right): https://codereview.chromium.org/2770893003/diff/120001/components/ntp_snippet... components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc:141: if (entries.size() > kMaxEntries) { // Get the |kMaxEntries| most recent entries. ? https://codereview.chromium.org/2770893003/diff/120001/components/ntp_snippet... File components/ntp_snippets/reading_list/reading_list_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2770893003/diff/120001/components/ntp_snippet... components/ntp_snippets/reading_list/reading_list_suggestions_provider_unittest.cc:71: GURL urlRead1 = GURL("http://www.bar.foor"); url_read also, nitty nit: url_unread vs unread_title https://codereview.chromium.org/2770893003/diff/120001/components/ntp_snippet... components/ntp_snippets/reading_list/reading_list_suggestions_provider_unittest.cc:102: TEST_F(ReadingListSuggestionsProviderTest, ReturnsOneUnreadSuggestion) { It's still not clear to me what exactly this tests. "DoesNotReturnReadSuggestions"? Also it seems to be mostly a subset of the test above (which maybe does too many things?). https://codereview.chromium.org/2770893003/diff/120001/components/ntp_snippet... components/ntp_snippets/reading_list/reading_list_suggestions_provider_unittest.cc:104: GURL urlRead1 = GURL("http://www.bar.foor"); url_read
Thanks! https://codereview.chromium.org/2770893003/diff/120001/components/ntp_snippet... File components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc (right): https://codereview.chromium.org/2770893003/diff/120001/components/ntp_snippet... components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc:141: if (entries.size() > kMaxEntries) { On 2017/03/28 15:31:29, Marc Treib wrote: > // Get the |kMaxEntries| most recent entries. > ? Done. https://codereview.chromium.org/2770893003/diff/120001/components/ntp_snippet... File components/ntp_snippets/reading_list/reading_list_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2770893003/diff/120001/components/ntp_snippet... components/ntp_snippets/reading_list/reading_list_suggestions_provider_unittest.cc:71: GURL urlRead1 = GURL("http://www.bar.foor"); On 2017/03/28 15:31:29, Marc Treib wrote: > url_read > > also, nitty nit: url_unread vs unread_title I missed this one :) Done and done. https://codereview.chromium.org/2770893003/diff/120001/components/ntp_snippet... components/ntp_snippets/reading_list/reading_list_suggestions_provider_unittest.cc:102: TEST_F(ReadingListSuggestionsProviderTest, ReturnsOneUnreadSuggestion) { On 2017/03/28 15:31:29, Marc Treib wrote: > It's still not clear to me what exactly this tests. > "DoesNotReturnReadSuggestions"? > Also it seems to be mostly a subset of the test above (which maybe does too many > things?). It tests that if you have less than kMaxEntries unread entries it does not return the read entry (which was a behavior we were considering at some point). I have added a comment and updated the test name. https://codereview.chromium.org/2770893003/diff/120001/components/ntp_snippet... components/ntp_snippets/reading_list/reading_list_suggestions_provider_unittest.cc:104: GURL urlRead1 = GURL("http://www.bar.foor"); On 2017/03/28 15:31:29, Marc Treib wrote: > url_read Done.
https://codereview.chromium.org/2770893003/diff/80001/components/ntp_snippets... File components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc (right): https://codereview.chromium.org/2770893003/diff/80001/components/ntp_snippets... components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc:158: url_formatter::FormatUrl(entry->URL().GetOrigin())); On 2017/03/28 15:31:29, Marc Treib wrote: > On 2017/03/28 15:10:07, gambard wrote: > > On 2017/03/28 13:25:16, Marc Treib wrote: > > > If the URL is already used as the title, then this seems redundant. Maybe > just > > > leave it empty in that case? > > > > The question was to do this test in the provider or a the view level. Maybe it > > makes more sense to have it a the view level. > > How would you check it there? After formatting, I don't know if a substring > check or something like that would be sufficient. Sorry, I missread your comment. The URL is used in the title only if the title is empty. If the page as a title, it is displayed as title. In every case the domain name of the url is displayed as secondary text. So you have: [Title or full url] [domain]
https://codereview.chromium.org/2770893003/diff/80001/components/ntp_snippets... File components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc (right): https://codereview.chromium.org/2770893003/diff/80001/components/ntp_snippets... components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc:158: url_formatter::FormatUrl(entry->URL().GetOrigin())); On 2017/03/28 15:42:49, gambard wrote: > On 2017/03/28 15:31:29, Marc Treib wrote: > > On 2017/03/28 15:10:07, gambard wrote: > > > On 2017/03/28 13:25:16, Marc Treib wrote: > > > > If the URL is already used as the title, then this seems redundant. Maybe > > just > > > > leave it empty in that case? > > > > > > The question was to do this test in the provider or a the view level. Maybe > it > > > makes more sense to have it a the view level. > > > > How would you check it there? After formatting, I don't know if a substring > > check or something like that would be sufficient. > > Sorry, I missread your comment. The URL is used in the title only if the title > is empty. If the page as a title, it is displayed as title. > In every case the domain name of the url is displayed as secondary text. So you > have: > [Title or full url] > [domain] So if the page doesn't have a title, you'd see: www.publisher.com/article www.publisher.com which seems redundant to me. But not my decision I guess :) But now that I look at this, snippet_text is almost certainly the wrong field to set here. I think publisher_name would fit better.
https://codereview.chromium.org/2770893003/diff/80001/components/ntp_snippets... File components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc (right): https://codereview.chromium.org/2770893003/diff/80001/components/ntp_snippets... components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc:158: url_formatter::FormatUrl(entry->URL().GetOrigin())); On 2017/03/28 15:51:45, Marc Treib wrote: > On 2017/03/28 15:42:49, gambard wrote: > > On 2017/03/28 15:31:29, Marc Treib wrote: > > > On 2017/03/28 15:10:07, gambard wrote: > > > > On 2017/03/28 13:25:16, Marc Treib wrote: > > > > > If the URL is already used as the title, then this seems redundant. > Maybe > > > just > > > > > leave it empty in that case? > > > > > > > > The question was to do this test in the provider or a the view level. > Maybe > > it > > > > makes more sense to have it a the view level. > > > > > > How would you check it there? After formatting, I don't know if a substring > > > check or something like that would be sufficient. > > > > Sorry, I missread your comment. The URL is used in the title only if the title > > is empty. If the page as a title, it is displayed as title. > > In every case the domain name of the url is displayed as secondary text. So > you > > have: > > [Title or full url] > > [domain] > > So if the page doesn't have a title, you'd see: > http://www.publisher.com/article > http://www.publisher.com > which seems redundant to me. But not my decision I guess :) > > But now that I look at this, snippet_text is almost certainly the wrong field to > set here. I think publisher_name would fit better. Yes, you would see both. I looks better in the UI than in plain text here :) Done.
Thanks! Unqualified lgtm now :)
gambard@chromium.org changed reviewers: + pkasting@chromium.org
+pkasting@ for DEPS include
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...
DEPS lgtm
The CQ bit was unchecked by gambard@chromium.org
The CQ bit was checked by gambard@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from olivierrobin@chromium.org, treib@chromium.org Link to the patchset: https://codereview.chromium.org/2770893003/#ps180001 (title: "Add title logic")
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
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by gambard@chromium.org
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
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...)
The CQ bit was checked by gambard@chromium.org
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": 180001, "attempt_start_ts": 1490856882239740,
"parent_rev": "679a8ee1b416d892b465fc505a8e695f6788cf13", "commit_rev":
"250e9409aec179c1a8427adf454309e1d2f6482f"}
Message was sent while issue was closed.
Description was changed from ========== Add logic for fetching the Reading List entries This CL adds the logic in the ReadingListProvider, allowing it to fetch the entries to be displayed. BUG=702241 ========== to ========== Add logic for fetching the Reading List entries This CL adds the logic in the ReadingListProvider, allowing it to fetch the entries to be displayed. BUG=702241 Review-Url: https://codereview.chromium.org/2770893003 Cr-Commit-Position: refs/heads/master@{#460697} Committed: https://chromium.googlesource.com/chromium/src/+/250e9409aec179c1a8427adf4543... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://chromium.googlesource.com/chromium/src/+/250e9409aec179c1a8427adf4543... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
