Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1956393002:
Add a feature flag for enabling transparent doodle request. (Closed)
DescriptionAdd a feature flag for enabling transparent doodle request.
When the flag is enabled, use the transparent doodle request. This is disabled
by default until the GWS changes are live.
BUG=610319
Committed: https://crrev.com/57a51d6f7b4fcaa63d303f465fdb6b360495531c
Cr-Commit-Position: refs/heads/master@{#392665}
Patch Set 1 #
Total comments: 10
Patch Set 2 : Addressing comments #
Total comments: 6
Patch Set 3 : #Patch Set 4 : Merge with master #
Messages
Total messages: 26 (11 generated)
The CQ bit was checked by atanasova@chromium.org to run a CQ dry run
Description was changed from ========== Add a feature flag for enabling transparent doodle request. When the flag is enabled, use the transparent doodle request. This is disabled by default until the GWS changes are live. BUG=610319 ========== to ========== Add a feature flag for enabling transparent doodle request. When the flag is enabled, use the transparent doodle request. This is disabled by default until the GWS changes are live. BUG=610319 ==========
atanasova@chromium.org changed reviewers: + treib@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1956393002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1956393002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1956393002/diff/1/chrome/app/generated_resour... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1956393002/diff/1/chrome/app/generated_resour... chrome/app/generated_resources.grd:14824: + <message name="IDS_FLAGS_ENABLE_NTP_TRANSPARENT_DOODLE_NAME" desc="Name for the flag to enable transparent doodle request." translateable="false"> Interesting - some flags specify translateable=false, but it looks like most don't. Do you know if there's any rule for that? (I agree that there's no reason for translating this, I'm just curious.) https://codereview.chromium.org/1956393002/diff/1/chrome/browser/android/chro... File chrome/browser/android/chrome_feature_list.cc (right): https://codereview.chromium.org/1956393002/diff/1/chrome/browser/android/chro... chrome/browser/android/chrome_feature_list.cc:39: &kNTPTransparentDoodle, This one doesn't need to be exposed to Java.
https://codereview.chromium.org/1956393002/diff/1/chrome/app/generated_resour... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1956393002/diff/1/chrome/app/generated_resour... chrome/app/generated_resources.grd:14824: + <message name="IDS_FLAGS_ENABLE_NTP_TRANSPARENT_DOODLE_NAME" desc="Name for the flag to enable transparent doodle request." translateable="false"> On 2016/05/10 07:58:39, Marc Treib wrote: > Interesting - some flags specify translateable=false, but it looks like most > don't. Do you know if there's any rule for that? > (I agree that there's no reason for translating this, I'm just curious.) I am not sure what the rule for this is, but since this will be a temporary flag until we have the server side feature done, I thought it made sense. https://codereview.chromium.org/1956393002/diff/1/chrome/browser/android/chro... File chrome/browser/android/chrome_feature_list.cc (right): https://codereview.chromium.org/1956393002/diff/1/chrome/browser/android/chro... chrome/browser/android/chrome_feature_list.cc:39: &kNTPTransparentDoodle, On 2016/05/10 07:58:39, Marc Treib wrote: > This one doesn't need to be exposed to Java. Done.
tschumann@chromium.org changed reviewers: + tschumann@chromium.org
thanks for taking care of this. One nit ;-) https://codereview.chromium.org/1956393002/diff/1/chrome/browser/android/logo... File chrome/browser/android/logo_service.cc (right): https://codereview.chromium.org/1956393002/diff/1/chrome/browser/android/logo... chrome/browser/android/logo_service.cc:130: true, a comment explaining the semantics of the bools would be nice. Not sure what the chrome way of doing this is; something like: /*wants_cta=*/true, or true /* wants_cta */ would be nice.
LGTM (once you upload a new patchset with the one change) https://codereview.chromium.org/1956393002/diff/1/chrome/app/generated_resour... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1956393002/diff/1/chrome/app/generated_resour... chrome/app/generated_resources.grd:14824: + <message name="IDS_FLAGS_ENABLE_NTP_TRANSPARENT_DOODLE_NAME" desc="Name for the flag to enable transparent doodle request." translateable="false"> On 2016/05/10 08:38:41, atanasova wrote: > On 2016/05/10 07:58:39, Marc Treib wrote: > > Interesting - some flags specify translateable=false, but it looks like most > > don't. Do you know if there's any rule for that? > > (I agree that there's no reason for translating this, I'm just curious.) > > I am not sure what the rule for this is, but since this will be a temporary flag > until we have the server side feature done, I thought it made sense. Actually now that I think of it, I'm not sure if we even need an about:flags entry for this - controlling it via Finch, or via the command line for testing, might be good enough. But I guess there's no harm in adding the about:flags entry too. https://codereview.chromium.org/1956393002/diff/1/chrome/browser/android/logo... File chrome/browser/android/logo_service.cc (right): https://codereview.chromium.org/1956393002/diff/1/chrome/browser/android/logo... chrome/browser/android/logo_service.cc:130: true, On 2016/05/10 08:44:34, tschumann wrote: > a comment explaining the semantics of the bools would be nice. Not sure what the > chrome way of doing this is; something like: > /*wants_cta=*/true, > or > true /* wants_cta */ > > would be nice. Haha, I just made the exact same comment on the other CL :D I think the second way is more common in Chromiuem, but I've seen both. Either's fine IMO.
atanasova@chromium.org changed reviewers: + bauerb@chromium.org, cpu@chromium.org
@cpu For the generated_resources changes https://codereview.chromium.org/1956393002/diff/1/chrome/app/generated_resour... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1956393002/diff/1/chrome/app/generated_resour... chrome/app/generated_resources.grd:14824: + <message name="IDS_FLAGS_ENABLE_NTP_TRANSPARENT_DOODLE_NAME" desc="Name for the flag to enable transparent doodle request." translateable="false"> On 2016/05/10 08:54:59, Marc Treib wrote: > On 2016/05/10 08:38:41, atanasova wrote: > > On 2016/05/10 07:58:39, Marc Treib wrote: > > > Interesting - some flags specify translateable=false, but it looks like most > > > don't. Do you know if there's any rule for that? > > > (I agree that there's no reason for translating this, I'm just curious.) > > > > I am not sure what the rule for this is, but since this will be a temporary > flag > > until we have the server side feature done, I thought it made sense. > > Actually now that I think of it, I'm not sure if we even need an about:flags > entry for this - controlling it via Finch, or via the command line for testing, > might be good enough. But I guess there's no harm in adding the about:flags > entry too. Acknowledged. https://codereview.chromium.org/1956393002/diff/1/chrome/browser/android/logo... File chrome/browser/android/logo_service.cc (right): https://codereview.chromium.org/1956393002/diff/1/chrome/browser/android/logo... chrome/browser/android/logo_service.cc:130: true, On 2016/05/10 08:54:59, Marc Treib wrote: > On 2016/05/10 08:44:34, tschumann wrote: > > a comment explaining the semantics of the bools would be nice. Not sure what > the > > chrome way of doing this is; something like: > > /*wants_cta=*/true, > > or > > true /* wants_cta */ > > > > would be nice. > > Haha, I just made the exact same comment on the other CL :D > I think the second way is more common in Chromiuem, but I've seen both. Either's > fine IMO. Done.
lgtm https://codereview.chromium.org/1956393002/diff/20001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1956393002/diff/20001/chrome/app/generated_re... chrome/app/generated_resources.grd:14824: + <message name="IDS_FLAGS_ENABLE_NTP_TRANSPARENT_DOODLE_NAME" desc="Name for the flag to enable transparent doodle request." translateable="false"> +1 for not translating this, as it's only meant for developers. https://codereview.chromium.org/1956393002/diff/20001/chrome/app/generated_re... chrome/app/generated_resources.grd:14825: + Request transparent doodle Nit: "Doodle" is capitalized :) https://codereview.chromium.org/1956393002/diff/20001/chrome/app/generated_re... chrome/app/generated_resources.grd:14828: + If enabled, the request to get a doodle will include the transparent flag. "Request a transparent Doodle from Google servers."
lgtm https://codereview.chromium.org/1956393002/diff/1/chrome/browser/android/logo... File chrome/browser/android/logo_service.cc (right): https://codereview.chromium.org/1956393002/diff/1/chrome/browser/android/logo... chrome/browser/android/logo_service.cc:130: true, On 2016/05/10 09:12:54, atanasova wrote: > On 2016/05/10 08:54:59, Marc Treib wrote: > > On 2016/05/10 08:44:34, tschumann wrote: > > > a comment explaining the semantics of the bools would be nice. Not sure what > > the > > > chrome way of doing this is; something like: > > > /*wants_cta=*/true, > > > or > > > true /* wants_cta */ > > > > > > would be nice. > > > > Haha, I just made the exact same comment on the other CL :D > > I think the second way is more common in Chromiuem, but I've seen both. > Either's > > fine IMO. > > Done. To follow up: In google3 there's a recent trend to /*foo=*/bar, as we have a supporting critique finding which will tell you if the comment is wrong -- i.e. if the argument name is different.
atanasova@chromium.org changed reviewers: - cpu@chromium.org
Removing @cpu as a reviewer, I misunderstood the owners file.
https://codereview.chromium.org/1956393002/diff/20001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1956393002/diff/20001/chrome/app/generated_re... chrome/app/generated_resources.grd:14824: + <message name="IDS_FLAGS_ENABLE_NTP_TRANSPARENT_DOODLE_NAME" desc="Name for the flag to enable transparent doodle request." translateable="false"> On 2016/05/10 09:18:49, Bernhard Bauer wrote: > +1 for not translating this, as it's only meant for developers. Acknowledged. https://codereview.chromium.org/1956393002/diff/20001/chrome/app/generated_re... chrome/app/generated_resources.grd:14825: + Request transparent doodle On 2016/05/10 09:18:49, Bernhard Bauer wrote: > Nit: "Doodle" is capitalized :) Done. https://codereview.chromium.org/1956393002/diff/20001/chrome/app/generated_re... chrome/app/generated_resources.grd:14828: + If enabled, the request to get a doodle will include the transparent flag. On 2016/05/10 09:18:49, Bernhard Bauer wrote: > "Request a transparent Doodle from Google servers." Done.
The CQ bit was checked by atanasova@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org, bauerb@chromium.org, tschumann@chromium.org Link to the patchset: https://codereview.chromium.org/1956393002/#ps60001 (title: "Merge with master")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1956393002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1956393002/60001
Message was sent while issue was closed.
Description was changed from ========== Add a feature flag for enabling transparent doodle request. When the flag is enabled, use the transparent doodle request. This is disabled by default until the GWS changes are live. BUG=610319 ========== to ========== Add a feature flag for enabling transparent doodle request. When the flag is enabled, use the transparent doodle request. This is disabled by default until the GWS changes are live. BUG=610319 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Add a feature flag for enabling transparent doodle request. When the flag is enabled, use the transparent doodle request. This is disabled by default until the GWS changes are live. BUG=610319 ========== to ========== Add a feature flag for enabling transparent doodle request. When the flag is enabled, use the transparent doodle request. This is disabled by default until the GWS changes are live. BUG=610319 Committed: https://crrev.com/57a51d6f7b4fcaa63d303f465fdb6b360495531c Cr-Commit-Position: refs/heads/master@{#392665} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/57a51d6f7b4fcaa63d303f465fdb6b360495531c Cr-Commit-Position: refs/heads/master@{#392665} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
