Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2581163004:
[NTP::SectionOrder] Propagate new order through ContentSuggestionsService. (Closed)
Description[NTP::SectionOrder] Propagate new order through ContentSuggestionsService.
Previously, |GetCategories()| did not update the order and, therefore,
order changes could not be propagated to UI.
After this CL, |GetCategories()| refreshes the order before returning
+1 test.
BUG=673743
Committed: https://crrev.com/8b5ab2844d4cb92e7abfefceea86e83e18f7da28
Cr-Commit-Position: refs/heads/master@{#439766}
Patch Set 1 #
Total comments: 6
Patch Set 2 : tschumann@ comments. #
Total comments: 4
Patch Set 3 : treib@ comments. #
Total comments: 8
Patch Set 4 : rebase. #Patch Set 5 : treib@ nits. #
Dependent Patchsets: Messages
Total messages: 52 (39 generated)
The CQ bit was checked by vitaliii@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: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by vitaliii@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: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by vitaliii@chromium.org to run a CQ dry run
Patchset #2 (id:20001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:1) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
Patchset #1 (id:40001) has been deleted
The CQ bit was checked by vitaliii@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...
Hi treib@, could you have a look?
Description was changed from ========== [NTP::SectionOrder] Propage new order through ContentSuggestionsService. Previously, |GetCategories()| did not update the order and, therefore, order changes could not be propagated to UI. After this CL, |GetCategories()| refreshes the order before returning +1 test. BUG=673743 ========== to ========== [NTP::SectionOrder] Propagate new order through ContentSuggestionsService. Previously, |GetCategories()| did not update the order and, therefore, order changes could not be propagated to UI. After this CL, |GetCategories()| refreshes the order before returning +1 test. BUG=673743 ==========
vitaliii@chromium.org changed reviewers: + treib@chromium.org
Hi treib@, Could you have a look?
nice! just some naming and comment nits. https://codereview.chromium.org/2581163004/diff/60001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service.h (right): https://codereview.chromium.org/2581163004/diff/60001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.h:105: // from |category_ranker_| immediately. IMO, this comment is exposing too many implementation details. What's important is that the order in which the categories are returned is the order in which they should be displayed. It doesn't matter how or with which members the service is computing this order. https://codereview.chromium.org/2581163004/diff/60001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.h:289: // |category_ranker_|. The order here may be stale, i.e. changes in the ranker nit: let's phrase this a bit clearer: The order in categories_ is not specified. Also, please don't add too many implementation details -- it'll get out of sync with the implementation (no need to mention SortCategories() or the ranker). The last sentence is worth keeping though. https://codereview.chromium.org/2581163004/diff/60001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service_unittest.cc (right): https://codereview.chromium.org/2581163004/diff/60001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service_unittest.cc:717: TEST_F(ContentSuggestionsServiceTest, ShouldRefreshCategoryOrder) { nit: "refresh" is a bit out of context here. What about ShouldGiveCategoriesInRankingOrder?
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 vitaliii@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...
vitaliii@chromium.org changed reviewers: + tschumann@chromium.org
I addressed tschumann@ comments, PTAL. https://codereview.chromium.org/2581163004/diff/60001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service.h (right): https://codereview.chromium.org/2581163004/diff/60001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.h:105: // from |category_ranker_| immediately. On 2016/12/19 09:06:38, tschumann wrote: > IMO, this comment is exposing too many implementation details. What's important > is that the order in which the categories are returned is the order in which > they should be displayed. > It doesn't matter how or with which members the service is computing this order. Done. https://codereview.chromium.org/2581163004/diff/60001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.h:289: // |category_ranker_|. The order here may be stale, i.e. changes in the ranker On 2016/12/19 09:06:38, tschumann wrote: > nit: let's phrase this a bit clearer: > The order in categories_ is not specified. > > Also, please don't add too many implementation details -- it'll get out of sync > with the implementation (no need to mention SortCategories() or the ranker). > The last sentence is worth keeping though. Done. https://codereview.chromium.org/2581163004/diff/60001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service_unittest.cc (right): https://codereview.chromium.org/2581163004/diff/60001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service_unittest.cc:717: TEST_F(ContentSuggestionsServiceTest, ShouldRefreshCategoryOrder) { On 2016/12/19 09:06:38, tschumann wrote: > nit: "refresh" is a bit out of context here. What about > ShouldGiveCategoriesInRankingOrder? Done. Now it is "ShouldReturnCategoriesInOrderToDisplay".
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2581163004/diff/80001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2581163004/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:397: SortCategories(); I think this call isn't required anymore. https://codereview.chromium.org/2581163004/diff/80001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service.h (right): https://codereview.chromium.org/2581163004/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.h:106: const std::vector<Category>& GetCategories(); Hm, it's a bit unfortunate that this can't be const anymore. Since |categories_| has an undefined order now anyway, maybe just return a sorted copy?
The CQ bit was checked by vitaliii@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...
Hi, PTAL. https://codereview.chromium.org/2581163004/diff/80001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2581163004/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:397: SortCategories(); On 2016/12/19 13:22:46, Marc Treib (OOO until Jan 12) wrote: > I think this call isn't required anymore. Done. https://codereview.chromium.org/2581163004/diff/80001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service.h (right): https://codereview.chromium.org/2581163004/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.h:106: const std::vector<Category>& GetCategories(); On 2016/12/19 13:22:46, Marc Treib (OOO until Jan 12) wrote: > Hm, it's a bit unfortunate that this can't be const anymore. > Since |categories_| has an undefined order now anyway, maybe just return a > sorted copy? Done.
lgtm LGTM with some nits, and one testing suggestion. https://codereview.chromium.org/2581163004/diff/100001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_service.h (right): https://codereview.chromium.org/2581163004/diff/100001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.h:286: // All current suggestion categories in undefined order. This vector contains nit: s/undefined/arbitrary/ ("undefined" has a very specific meaning in C++) https://codereview.chromium.org/2581163004/diff/100001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_service_unittest.cc (right): https://codereview.chromium.org/2581163004/diff/100001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service_unittest.cc:44: class MockProvider : public ContentSuggestionsProvider { Not your doing, but this is a weird combination of a mock and a fake. Mind adding a TODO to fix that? https://codereview.chromium.org/2581163004/diff/100001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service_unittest.cc:721: // The service is recreated to pick up a new ranker. nit: s/a new ranker/the new ranker/ https://codereview.chromium.org/2581163004/diff/100001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service_unittest.cc:734: .WillRepeatedly(Return(false)); I think here a fake would be better suited than a mock: We don't want to verify and calls to the ranker; we just want one that returns a pre-defined ordering.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Hi treib@, I addressed your nits, no need to look. https://codereview.chromium.org/2581163004/diff/100001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_service.h (right): https://codereview.chromium.org/2581163004/diff/100001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.h:286: // All current suggestion categories in undefined order. This vector contains On 2016/12/19 15:44:41, Marc Treib (OOO until Jan 12) wrote: > nit: s/undefined/arbitrary/ ("undefined" has a very specific meaning in C++) Done. https://codereview.chromium.org/2581163004/diff/100001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_service_unittest.cc (right): https://codereview.chromium.org/2581163004/diff/100001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service_unittest.cc:44: class MockProvider : public ContentSuggestionsProvider { On 2016/12/19 15:44:41, Marc Treib (OOO until Jan 12) wrote: > Not your doing, but this is a weird combination of a mock and a fake. Mind > adding a TODO to fix that? Done. https://codereview.chromium.org/2581163004/diff/100001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service_unittest.cc:721: // The service is recreated to pick up a new ranker. On 2016/12/19 15:44:41, Marc Treib (OOO until Jan 12) wrote: > nit: s/a new ranker/the new ranker/ Done. https://codereview.chromium.org/2581163004/diff/100001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service_unittest.cc:734: .WillRepeatedly(Return(false)); On 2016/12/19 15:44:41, Marc Treib (OOO until Jan 12) wrote: > I think here a fake would be better suited than a mock: We don't want to verify > and calls to the ranker; we just want one that returns a pre-defined ordering. Done.
The CQ bit was checked by vitaliii@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/2581163004/#ps140001 (title: "treib@ comments.")
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: android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #5 (id:140001) has been deleted
The CQ bit was checked by vitaliii@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/2581163004/#ps160001 (title: "treib@ nits.")
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": 160001, "attempt_start_ts": 1482224226376970,
"parent_rev": "4f1d83e46d409bfae1eea3cae4e1e4c27f978e7f", "commit_rev":
"2e14bf2cc6fc95eb7166ef6e0eb703a8dc3265d7"}
Message was sent while issue was closed.
Description was changed from ========== [NTP::SectionOrder] Propagate new order through ContentSuggestionsService. Previously, |GetCategories()| did not update the order and, therefore, order changes could not be propagated to UI. After this CL, |GetCategories()| refreshes the order before returning +1 test. BUG=673743 ========== to ========== [NTP::SectionOrder] Propagate new order through ContentSuggestionsService. Previously, |GetCategories()| did not update the order and, therefore, order changes could not be propagated to UI. After this CL, |GetCategories()| refreshes the order before returning +1 test. BUG=673743 Review-Url: https://codereview.chromium.org/2581163004 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== [NTP::SectionOrder] Propagate new order through ContentSuggestionsService. Previously, |GetCategories()| did not update the order and, therefore, order changes could not be propagated to UI. After this CL, |GetCategories()| refreshes the order before returning +1 test. BUG=673743 Review-Url: https://codereview.chromium.org/2581163004 ========== to ========== [NTP::SectionOrder] Propagate new order through ContentSuggestionsService. Previously, |GetCategories()| did not update the order and, therefore, order changes could not be propagated to UI. After this CL, |GetCategories()| refreshes the order before returning +1 test. BUG=673743 Committed: https://crrev.com/8b5ab2844d4cb92e7abfefceea86e83e18f7da28 Cr-Commit-Position: refs/heads/master@{#439766} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/8b5ab2844d4cb92e7abfefceea86e83e18f7da28 Cr-Commit-Position: refs/heads/master@{#439766} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
