Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(683)

Issue 2831553003: List our features in ntp_snippets::kAllFeatures (Closed)

Created:
3 years, 8 months ago by sfiera
Modified:
3 years, 8 months ago
Reviewers:
jkrcal, dgn
CC:
chromium-reviews, noyau+watch_chromium.org, ntp-dev+reviews_chromium.org, asvitkine+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

List our features in ntp_snippets::kAllFeatures Move the notifications feature into the component for consistency, even though nothing in the component actually uses it. Having this list lets us answer questions list "what active experiment IDs correspond to our features?" Active experiments are important for debugging, but they're only loosely tied to features. BUG=713038 Review-Url: https://codereview.chromium.org/2831553003 Cr-Commit-Position: refs/heads/master@{#466640} Committed: https://chromium.googlesource.com/chromium/src/+/429f312b0855e94bd2facaa27842513fc2c362dd

Patch Set 1 #

Total comments: 4

Patch Set 2 : Lengthen comment #

Patch Set 3 : rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+89 lines, -98 lines) Patch
M chrome/browser/BUILD.gn View 1 2 1 chunk +0 lines, -2 lines 0 comments Download
M chrome/browser/about_flags.cc View 1 2 2 chunks +2 lines, -3 lines 0 comments Download
M chrome/browser/android/chrome_feature_list.cc View 2 chunks +1 line, -2 lines 0 comments Download
M chrome/browser/android/ntp/content_suggestions_notification_helper.cc View 2 chunks +4 lines, -4 lines 0 comments Download
M chrome/browser/android/ntp/content_suggestions_notifier_service.cc View 2 chunks +11 lines, -11 lines 0 comments Download
M chrome/browser/ntp_snippets/content_suggestions_notifier_service_factory.cc View 1 chunk +2 lines, -2 lines 0 comments Download
D chrome/browser/ntp_snippets/ntp_snippets_features.h View 1 chunk +0 lines, -49 lines 0 comments Download
D chrome/browser/ntp_snippets/ntp_snippets_features.cc View 1 chunk +0 lines, -25 lines 0 comments Download
M components/ntp_snippets/features.h View 1 2 chunks +41 lines, -0 lines 0 comments Download
M components/ntp_snippets/features.cc View 2 chunks +28 lines, -0 lines 0 comments Download

Dependent Patchsets:

Messages

Total messages: 26 (14 generated)
sfiera
Small prelude to https://crrev.com/2826013002
3 years, 8 months ago (2017-04-19 09:55:24 UTC) #4
jkrcal
lgtm https://codereview.chromium.org/2831553003/diff/1/components/ntp_snippets/features.cc File components/ntp_snippets/features.cc (right): https://codereview.chromium.org/2831553003/diff/1/components/ntp_snippets/features.cc#newcode17 components/ntp_snippets/features.cc:17: const base::Feature*(kAllFeatures[]) = {&kArticleSuggestionsFeature, I am a bit ...
3 years, 8 months ago (2017-04-19 11:23:40 UTC) #7
sfiera
https://codereview.chromium.org/2831553003/diff/1/components/ntp_snippets/features.cc File components/ntp_snippets/features.cc (right): https://codereview.chromium.org/2831553003/diff/1/components/ntp_snippets/features.cc#newcode17 components/ntp_snippets/features.cc:17: const base::Feature*(kAllFeatures[]) = {&kArticleSuggestionsFeature, On 2017/04/19 11:23:40, jkrcal wrote: ...
3 years, 8 months ago (2017-04-19 11:41:10 UTC) #8
dgn
Which features do you expect to have listed here? There are other missing, at least ...
3 years, 8 months ago (2017-04-19 12:53:26 UTC) #9
sfiera
On 2017/04/19 12:53:26, dgn (in PST timezone) wrote: > Which features do you expect to ...
3 years, 8 months ago (2017-04-19 13:04:54 UTC) #10
sfiera
Ping? I'm happy to move that feature as well, if you think we should. This ...
3 years, 8 months ago (2017-04-20 17:36:43 UTC) #11
dgn
sorry for the delay, lgtm
3 years, 8 months ago (2017-04-21 20:42:40 UTC) #12
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2831553003/20001
3 years, 8 months ago (2017-04-24 11:59:05 UTC) #16
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2831553003/40001
3 years, 8 months ago (2017-04-24 12:00:00 UTC) #19
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/437364)
3 years, 8 months ago (2017-04-24 14:44:38 UTC) #21
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2831553003/40001
3 years, 8 months ago (2017-04-24 14:48:00 UTC) #23
commit-bot: I haz the power
3 years, 8 months ago (2017-04-24 16:18:12 UTC) #26
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/429f312b0855e94bd2facaa27842...

Powered by Google App Engine
This is Rietveld 408576698