Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2696563002:
[NTP::SectionOrder] Add EM oriented order. (Closed)
Description[NTP::SectionOrder] Add EM oriented order.
Add new EM default order (articles, downloads, bookmarks) and allow
choosing the default order via variations.
BUG=690330
Review-Url: https://codereview.chromium.org/2696563002
Cr-Commit-Position: refs/heads/master@{#450365}
Committed: https://chromium.googlesource.com/chromium/src/+/90790ebcd0632f1860a89179643ef89fcf34c270
Patch Set 1 #
Total comments: 29
Patch Set 2 : clean rebase. #Patch Set 3 : treib@ comments. #
Total comments: 2
Patch Set 4 : treib@ comment. #
Messages
Total messages: 27 (18 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...
vitaliii@chromium.org changed reviewers: + treib@chromium.org
Hi treib@, Could you have a look please? https://codereview.chromium.org/2696563002/diff/1/chrome/browser/about_flags.cc File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2696563002/diff/1/chrome/browser/about_flags.... chrome/browser/about_flags.cc:521: kContentSuggestionsCategoryOrderFeatureVariationEmergingMarketsOriented), Should I replace EmergingMarkets with EM in order to make this line shorter?
https://codereview.chromium.org/2696563002/diff/1/chrome/app/generated_resour... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2696563002/diff/1/chrome/app/generated_resour... chrome/app/generated_resources.grd:14260: + Set default order of content suggestion sections (e.g. on the NTP). nit: s/sections/categories/, to be consistent https://codereview.chromium.org/2696563002/diff/1/chrome/browser/about_flags.cc File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2696563002/diff/1/chrome/browser/about_flags.... chrome/browser/about_flags.cc:508: { Is this "git cl format"ted? Looks weird... https://codereview.chromium.org/2696563002/diff/1/chrome/browser/about_flags.... chrome/browser/about_flags.cc:521: kContentSuggestionsCategoryOrderFeatureVariationEmergingMarketsOriented), On 2017/02/13 13:51:57, vitaliii wrote: > Should I replace EmergingMarkets with EM in order to make this line shorter? Sure, why not https://codereview.chromium.org/2696563002/diff/1/chrome/browser/about_flags.... chrome/browser/about_flags.cc:1847: // TODO(crbug.com/690450): Use ntp_snippets::kStudyName as a feature trial. FYI: The common format (everywhere except for ios/) is "TODO(ldap)", though arguably the bug number is more useful than a user name. So.. eh. nit: s/as a/as the/ https://codereview.chromium.org/2696563002/diff/1/chrome/browser/about_flags.... chrome/browser/about_flags.cc:1849: // about::flags if they reuse a feature trial. Currently, only a single FEATURE_WITH_VARIATIONS_VALUE_TYPE can use a given study name. ? (I'm slightly confused by your formulation) https://codereview.chromium.org/2696563002/diff/1/components/ntp_snippets/cat... File components/ntp_snippets/category_rankers/constant_category_ranker.cc (right): https://codereview.chromium.org/2696563002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/constant_category_ranker.cc:80: categories.push_back(KnownCategories::ARTICLES); ARTICLES wasn't listed here before. Are you sure this won't break anything? https://codereview.chromium.org/2696563002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/constant_category_ranker.cc:91: default: Don't list a "default" case. Then, when a new enum entry is added without also updating the switch, there'll be a compile error. https://codereview.chromium.org/2696563002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/constant_category_ranker.cc:98: static_cast<size_t>(KnownCategories::REMOTE_CATEGORIES_OFFSET), This isn't really correct - it works now, but it's not guaranteed that all known remote categories will always have contiguous IDs. (In fact, as soon as we add any other known remote category, they won't be contiguous anymore.) I'm not sure what's best to do here - we do want to make sure any local categories are listed here. Maybe a "static_assert(LOCAL_CATEGORIES_COUNT == <current known value>)" with a proper message is enough? WDYT? https://codereview.chromium.org/2696563002/diff/1/components/ntp_snippets/fea... File components/ntp_snippets/features.cc (right): https://codereview.chromium.org/2696563002/diff/1/components/ntp_snippets/fea... components/ntp_snippets/features.cc:96: extern const char kCategoryOrderEmergingMarketsOriented[] = No "extern" here (does it work like this?!) https://codereview.chromium.org/2696563002/diff/1/components/ntp_snippets/fea... components/ntp_snippets/features.cc:100: std::string category_order_value = nit: Move this after the check if the feature is enabled https://codereview.chromium.org/2696563002/diff/1/components/ntp_snippets/fea... components/ntp_snippets/features.cc:119: NOTREACHED() << "The " << kCategoryOrderParameter << " parameter value is '" This shouldn't be a NOTREACHED - getting an invalid input parameter is not a bug in Chrome. LOG(DFATAL) would be okay if you want. https://codereview.chromium.org/2696563002/diff/1/components/ntp_snippets/fea... File components/ntp_snippets/features.h (right): https://codereview.chromium.org/2696563002/diff/1/components/ntp_snippets/fea... components/ntp_snippets/features.h:70: extern const char kCategoryOrderParameter[]; Comment, similar to CategoryRanker above? https://codereview.chromium.org/2696563002/diff/1/components/ntp_snippets/fea... components/ntp_snippets/features.h:75: GENERAL, DEFAULT?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by vitaliii@chromium.org to run a CQ dry run
Hi treib@, I addressed your comments, could you have a look? https://codereview.chromium.org/2696563002/diff/1/chrome/app/generated_resour... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2696563002/diff/1/chrome/app/generated_resour... chrome/app/generated_resources.grd:14260: + Set default order of content suggestion sections (e.g. on the NTP). On 2017/02/13 14:25:22, Marc Treib wrote: > nit: s/sections/categories/, to be consistent Done. https://codereview.chromium.org/2696563002/diff/1/chrome/browser/about_flags.cc File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2696563002/diff/1/chrome/browser/about_flags.... chrome/browser/about_flags.cc:508: { On 2017/02/13 14:25:22, Marc Treib wrote: > Is this "git cl format"ted? Looks weird... Yes, it is. If you write this definition as one line, git cl format produces the code above. This should be fixed by s/EmergingMarkets/EM mentioned below. https://codereview.chromium.org/2696563002/diff/1/chrome/browser/about_flags.... chrome/browser/about_flags.cc:521: kContentSuggestionsCategoryOrderFeatureVariationEmergingMarketsOriented), On 2017/02/13 14:25:22, Marc Treib wrote: > On 2017/02/13 13:51:57, vitaliii wrote: > > Should I replace EmergingMarkets with EM in order to make this line shorter? > > Sure, why not Done. https://codereview.chromium.org/2696563002/diff/1/chrome/browser/about_flags.... chrome/browser/about_flags.cc:1847: // TODO(crbug.com/690450): Use ntp_snippets::kStudyName as a feature trial. On 2017/02/13 14:25:22, Marc Treib wrote: > FYI: The common format (everywhere except for ios/) is "TODO(ldap)", though > arguably the bug number is more useful than a user name. So.. eh. > > nit: s/as a/as the/ Done. Replaced. Also moved the bug ling inside the TODO. https://codereview.chromium.org/2696563002/diff/1/chrome/browser/about_flags.... chrome/browser/about_flags.cc:1849: // about::flags if they reuse a feature trial. On 2017/02/13 14:25:23, Marc Treib wrote: > Currently, only a single FEATURE_WITH_VARIATIONS_VALUE_TYPE can use a given > study name. > ? > (I'm slightly confused by your formulation) Done. https://codereview.chromium.org/2696563002/diff/1/components/ntp_snippets/cat... File components/ntp_snippets/category_rankers/constant_category_ranker.cc (right): https://codereview.chromium.org/2696563002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/constant_category_ranker.cc:80: categories.push_back(KnownCategories::ARTICLES); On 2017/02/13 14:25:23, Marc Treib wrote: > ARTICLES wasn't listed here before. Are you sure this won't break anything? It was (see line 87 in the old code). Under this case, |categories| is exactly the same as it was previously. https://codereview.chromium.org/2696563002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/constant_category_ranker.cc:91: default: On 2017/02/13 14:25:23, Marc Treib wrote: > Don't list a "default" case. Then, when a new enum entry is added without also > updating the switch, there'll be a compile error. Done. Cool, I did not know this. https://codereview.chromium.org/2696563002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/constant_category_ranker.cc:98: static_cast<size_t>(KnownCategories::REMOTE_CATEGORIES_OFFSET), On 2017/02/13 14:25:23, Marc Treib wrote: > This isn't really correct - it works now, but it's not guaranteed that all known > remote categories will always have contiguous IDs. (In fact, as soon as we add > any other known remote category, they won't be contiguous anymore.) > > I'm not sure what's best to do here - we do want to make sure any local > categories are listed here. Maybe a "static_assert(LOCAL_CATEGORIES_COUNT == > <current known value>)" with a proper message is enough? WDYT? Done. https://codereview.chromium.org/2696563002/diff/1/components/ntp_snippets/fea... File components/ntp_snippets/features.cc (right): https://codereview.chromium.org/2696563002/diff/1/components/ntp_snippets/fea... components/ntp_snippets/features.cc:96: extern const char kCategoryOrderEmergingMarketsOriented[] = On 2017/02/13 14:25:23, Marc Treib wrote: > No "extern" here (does it work like this?!) Done. It did work like that. https://codereview.chromium.org/2696563002/diff/1/components/ntp_snippets/fea... components/ntp_snippets/features.cc:100: std::string category_order_value = On 2017/02/13 14:25:23, Marc Treib wrote: > nit: Move this after the check if the feature is enabled Done. https://codereview.chromium.org/2696563002/diff/1/components/ntp_snippets/fea... components/ntp_snippets/features.cc:119: NOTREACHED() << "The " << kCategoryOrderParameter << " parameter value is '" On 2017/02/13 14:25:23, Marc Treib wrote: > This shouldn't be a NOTREACHED - getting an invalid input parameter is not a bug > in Chrome. LOG(DFATAL) would be okay if you want. Done. https://codereview.chromium.org/2696563002/diff/1/components/ntp_snippets/fea... File components/ntp_snippets/features.h (right): https://codereview.chromium.org/2696563002/diff/1/components/ntp_snippets/fea... components/ntp_snippets/features.h:70: extern const char kCategoryOrderParameter[]; On 2017/02/13 14:25:23, Marc Treib wrote: > Comment, similar to CategoryRanker above? Done. https://codereview.chromium.org/2696563002/diff/1/components/ntp_snippets/fea... components/ntp_snippets/features.h:75: GENERAL, On 2017/02/13 14:25:23, Marc Treib wrote: > DEFAULT? I thought about this. These orders may be used in ClickBasedRanker as a default (initial) order, so I was reluctant to name one of these orders "DEFAULT". Overall, neither default nor general are that informative.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm https://codereview.chromium.org/2696563002/diff/1/components/ntp_snippets/cat... File components/ntp_snippets/category_rankers/constant_category_ranker.cc (right): https://codereview.chromium.org/2696563002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/constant_category_ranker.cc:80: categories.push_back(KnownCategories::ARTICLES); On 2017/02/14 09:32:32, vitaliii wrote: > On 2017/02/13 14:25:23, Marc Treib wrote: > > ARTICLES wasn't listed here before. Are you sure this won't break anything? > > It was (see line 87 in the old code). > Under this case, |categories| is exactly the same as it was previously. Ah, indeed. I overlooked that, sorry. https://codereview.chromium.org/2696563002/diff/40001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2696563002/diff/40001/chrome/app/generated_re... chrome/app/generated_resources.grd:14265: + <message name="IDS_FLAGS_CONTENT_SUGGESTIONS_CATEGORY_ORDER_DESCRIPTION" desc="Description for the flag to choose a deafult category order for content suggestions, e.g. on the New Tab page." translateable="false"> s/deafult/default/
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
Hi treib@, I addressed your comment, no need to look. https://codereview.chromium.org/2696563002/diff/1/components/ntp_snippets/cat... File components/ntp_snippets/category_rankers/constant_category_ranker.cc (right): https://codereview.chromium.org/2696563002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/constant_category_ranker.cc:80: categories.push_back(KnownCategories::ARTICLES); On 2017/02/14 09:48:54, Marc Treib wrote: > On 2017/02/14 09:32:32, vitaliii wrote: > > On 2017/02/13 14:25:23, Marc Treib wrote: > > > ARTICLES wasn't listed here before. Are you sure this won't break anything? > > > > It was (see line 87 in the old code). > > Under this case, |categories| is exactly the same as it was previously. > > Ah, indeed. I overlooked that, sorry. Acknowledged. https://codereview.chromium.org/2696563002/diff/40001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2696563002/diff/40001/chrome/app/generated_re... chrome/app/generated_resources.grd:14265: + <message name="IDS_FLAGS_CONTENT_SUGGESTIONS_CATEGORY_ORDER_DESCRIPTION" desc="Description for the flag to choose a deafult category order for content suggestions, e.g. on the New Tab page." translateable="false"> On 2017/02/14 09:48:54, Marc Treib wrote: > s/deafult/default/ Done.
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: + asvitkine@chromium.org
Hi asvitkine@, Could you have a look at histograms.xml? Unrelated to this change, should we allow submitting changes to histograms.xml without OWNERS approval if they only contain new histogram values? Basically, one can add a flag to about_flags.cc without OWNERS approval, but needs histograms.xml approval anyway and there is no EMEA based people here.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM If you're only updating an enum in histograms.xml, I'm OK if you TBR me.
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/2696563002/#ps60001 (title: "treib@ comment.")
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": 1487086873612190,
"parent_rev": "f63ac52a3a74c9856d6fead1b1fb26a4948b35fe", "commit_rev":
"90790ebcd0632f1860a89179643ef89fcf34c270"}
Message was sent while issue was closed.
Description was changed from ========== [NTP::SectionOrder] Add EM oriented order. Add new EM default order (articles, downloads, bookmarks) and allow choosing the default order via variations. BUG=690330 ========== to ========== [NTP::SectionOrder] Add EM oriented order. Add new EM default order (articles, downloads, bookmarks) and allow choosing the default order via variations. BUG=690330 Review-Url: https://codereview.chromium.org/2696563002 Cr-Commit-Position: refs/heads/master@{#450365} Committed: https://chromium.googlesource.com/chromium/src/+/90790ebcd0632f1860a89179643e... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/90790ebcd0632f1860a89179643e... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
