Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2898153004:
[NTP::Push] Add feature (Closed)
Description[NTP::Push] Add feature
Add Content Suggestion Push feature flag
BUG=728083
Review-Url: https://codereview.chromium.org/2898153004
Cr-Commit-Position: refs/heads/master@{#476368}
Committed: https://chromium.googlesource.com/chromium/src/+/e15e8c228ed7d48f5979537d54967326e37ba421
Patch Set 1 #Patch Set 2 : Remvoing unnecessary "NTP" from the feature name. #
Total comments: 4
Patch Set 3 : Response to code review #Patch Set 4 : minor #Patch Set 5 : Removing unused constants #
Total comments: 7
Patch Set 6 : vitaliii@ comments. #
Dependent Patchsets: Messages
Total messages: 24 (11 generated)
mamir@chromium.org changed reviewers: + sfiera@chromium.org
mamir@chromium.org changed reviewers: + markusheintz@chromium.org
You don't actually need the chrome://flags entry yet, just the Feature. I find it easier to set the features with adb_chrome_public_command_line for development, so I can get a clean state on each rebuild without needing to go into chrome://flags. https://codereview.chromium.org/2898153004/diff/20001/chrome/browser/flag_des... File chrome/browser/flag_descriptions.cc (right): https://codereview.chromium.org/2898153004/diff/20001/chrome/browser/flag_des... chrome/browser/flag_descriptions.cc:2447: "Receive GCM push updates for timely content at the New Tab Page"; It's not really tied to the New Tab Page. You could just say: "Receive timely content suggestions via GCM push" (applies to all places NTP appears in this CL) https://codereview.chromium.org/2898153004/diff/20001/chrome/browser/flag_des... chrome/browser/flag_descriptions.cc:2451: "server about timely content such as breaking news."; Describing this flag in terms of "client" and "server" makes sense to the implementer but not to others. Better to describe the observable behavior and not the implementation. "If enabled, Chrome will register with Google services to receive content suggestions via push (in addition to normal background polling). Push will be used for timely content, such as breaking news."
Thank you for the thorough review and suggestions provided. https://codereview.chromium.org/2898153004/diff/20001/chrome/browser/flag_des... File chrome/browser/flag_descriptions.cc (right): https://codereview.chromium.org/2898153004/diff/20001/chrome/browser/flag_des... chrome/browser/flag_descriptions.cc:2447: "Receive GCM push updates for timely content at the New Tab Page"; On 2017/05/24 14:11:46, sfiera wrote: > It's not really tied to the New Tab Page. You could just say: > "Receive timely content suggestions via GCM push" > > (applies to all places NTP appears in this CL) Done. https://codereview.chromium.org/2898153004/diff/20001/chrome/browser/flag_des... chrome/browser/flag_descriptions.cc:2451: "server about timely content such as breaking news."; On 2017/05/24 14:11:46, sfiera wrote: > Describing this flag in terms of "client" and "server" makes sense to the > implementer but not to others. Better to describe the observable behavior and > not the implementation. > "If enabled, Chrome will register with Google services to receive content > suggestions via push (in addition to normal background polling). Push will be > used for timely content, such as breaking news." Done.
LGTM But without the chrome://flags entry, those constants are unused.
Description was changed from ========== Adding NTP push updates feature flag BUG=719998 ========== to ========== Adding Content Suggestion Push feature flag BUG=719998 ==========
Done! Not sure if it's worth a separate CL now :-)
mamir@chromium.org changed reviewers: + vitaliii@chromium.org - markusheintz@chromium.org
subject nits: you could use some [prefix] for push updates CLs. E.g. I would go for [NTP::Push] and then the subject becomes "[NTP::Push] Add Content Suggestion Push feature". This makes it easier for sheriffs to understand the context of your CL. Also then the remaining subject can rely on this context and be shorter. E.g. here "[NTP::Push] Add feature". Also please do not use "ing" in the subject, instead it should be like an order, e.g. "Add Content Suggestion Push feature". Also you are not adding a flag anymore, so please remove it from the subject as well. Bug nit: you are using your launch bug here and this is uncommon. Usually, people create a second bug (so called "Tracking") for implementation details and block the launch bug on it (example crbug.com/659205). The launch bug will be used for launch discussions and having a lot of CLs there may be inconvenient. Also I would create a separate bug for this issue and use it here and block the tracking bug on this new bug :) (continuing the example above, crbug.com/666645 does this). https://codereview.chromium.org/2898153004/diff/80001/components/ntp_snippets... File components/ntp_snippets/features.cc (right): https://codereview.chromium.org/2898153004/diff/80001/components/ntp_snippets... components/ntp_snippets/features.cc:22: &kContentSuggestionsPushFeature, Please keep sorted (as the comment above suggests). https://codereview.chromium.org/2898153004/diff/80001/components/ntp_snippets... File components/ntp_snippets/features.h (right): https://codereview.chromium.org/2898153004/diff/80001/components/ntp_snippets... components/ntp_snippets/features.h:54: // Feature to listen for GCM push updates from the server s/server/server. to make it consistent with the rest of the file. https://codereview.chromium.org/2898153004/diff/80001/components/ntp_snippets... components/ntp_snippets/features.h:55: extern const base::Feature kContentSuggestionsPushFeature; Is there a reason for this location? It is right in the middle of Category Ranker things. Should we move it in the end after all notification things?
Description was changed from ========== Adding Content Suggestion Push feature flag BUG=719998 ========== to ========== Adding Content Suggestion Push feature flag BUG=728083 ==========
Description was changed from ========== Adding Content Suggestion Push feature flag BUG=728083 ========== to ========== Add Content Suggestion Push feature flag BUG=728083 ==========
https://codereview.chromium.org/2898153004/diff/80001/components/ntp_snippets... File components/ntp_snippets/features.cc (right): https://codereview.chromium.org/2898153004/diff/80001/components/ntp_snippets... components/ntp_snippets/features.cc:22: &kContentSuggestionsPushFeature, On 2017/05/31 10:23:55, vitaliii wrote: > Please keep sorted (as the comment above suggests). They were sorted but lost it during a rebase. Sorry. https://codereview.chromium.org/2898153004/diff/80001/components/ntp_snippets... components/ntp_snippets/features.cc:22: &kContentSuggestionsPushFeature, On 2017/05/31 10:23:55, vitaliii wrote: > Please keep sorted (as the comment above suggests). Done. https://codereview.chromium.org/2898153004/diff/80001/components/ntp_snippets... File components/ntp_snippets/features.h (right): https://codereview.chromium.org/2898153004/diff/80001/components/ntp_snippets... components/ntp_snippets/features.h:54: // Feature to listen for GCM push updates from the server On 2017/05/31 10:23:55, vitaliii wrote: > s/server/server. > > to make it consistent with the rest of the file. Done. https://codereview.chromium.org/2898153004/diff/80001/components/ntp_snippets... components/ntp_snippets/features.h:55: extern const base::Feature kContentSuggestionsPushFeature; On 2017/05/31 10:23:55, vitaliii wrote: > Is there a reason for this location? It is right in the middle of Category > Ranker things. Should we move it in the end after all notification things? Done.
LGTM % nit First line of the description must be the same as the subject, because the subject is not shown in many places, e.g. https://chromium.googlesource.com/chromium/src/+/2f11f6e2360566af1ad768a62168... (it may look like the first line is a subject, but it is just a description actually :) ).
Description was changed from ========== Add Content Suggestion Push feature flag BUG=728083 ========== to ========== [NTP::Push] Add feature Add Content Suggestion Push feature flag BUG=728083 ==========
On 2017/05/31 10:58:04, vitaliii wrote: > LGTM % nit > > First line of the description must be the same as the subject, because the > subject is not shown in many places, e.g. > https://chromium.googlesource.com/chromium/src/+/2f11f6e2360566af1ad768a62168... > (it may look like the first line is a subject, but it is just a description > actually :) ). Done!
The CQ bit was checked by mamir@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sfiera@chromium.org Link to the patchset: https://codereview.chromium.org/2898153004/#ps100001 (title: "vitaliii@ 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": 100001, "attempt_start_ts": 1496337733813460,
"parent_rev": "3391e936295e694b35688b2152103b8d1432153d", "commit_rev":
"e15e8c228ed7d48f5979537d54967326e37ba421"}
Message was sent while issue was closed.
Description was changed from ========== [NTP::Push] Add feature Add Content Suggestion Push feature flag BUG=728083 ========== to ========== [NTP::Push] Add feature Add Content Suggestion Push feature flag BUG=728083 Review-Url: https://codereview.chromium.org/2898153004 Cr-Commit-Position: refs/heads/master@{#476368} Committed: https://chromium.googlesource.com/chromium/src/+/e15e8c228ed7d48f5979537d5496... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/e15e8c228ed7d48f5979537d5496... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
