Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2903293002:
[Android] Omnibox Placeholder Experiment (Closed)
|
Created:
3 years, 7 months ago by huayinz Modified:
3 years, 6 months ago CC:
chromium-reviews, jdonnelly+watch_chromium.org, ntp-dev+reviews_chromium.org, noyau+watch_chromium.org, asvitkine+watch_chromium.org, agrieve+watch_chromium.org, srahim+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[Android] Omnibox Placeholder Experiment
Added:
+ Variations of hint text in omnibox placeholder for the experiment
+ Chrome flags to change hint text for testing purpose
+ Field trial testing config for testing purpose
+ The hint text change will show up on the second launch after
flag is set.
BUG=723908
Review-Url: https://codereview.chromium.org/2903293002
Cr-Commit-Position: refs/heads/master@{#476027}
Committed: https://chromium.googlesource.com/chromium/src/+/ea3fb69a9bb9ce86d98fa6f9cabe6084a739fdf5
Patch Set 1 #
Total comments: 24
Patch Set 2 : Modify as suggested #
Total comments: 5
Patch Set 3 : change class and method names #Patch Set 4 : cache experiment group instead of hint text #
Total comments: 6
Patch Set 5 : Omnibox Placeholder Experiment #Patch Set 6 : fix some naming issues #
Total comments: 5
Patch Set 7 : remove chrome flag #
Total comments: 2
Patch Set 8 : remove redundant initialization #Messages
Total messages: 30 (10 generated)
Description was changed from ========== Clank Omnibox Placeholder Experiment Added: + Variations of hint text in omnibox placeholder for the experiment + Chrome flags to change hint text for testing purpose + Field trial testing config for testing purpose BUG=723908 ========== to ========== Clank Omnibox Placeholder Experiment Added: + Variations of hint text in omnibox placeholder for the experiment + Chrome flags to change hint text for testing purpose + Field trial testing config for testing purpose + The hint text change will show up on the second launch after flag is set. BUG=723908 ==========
huayinz@chromium.org changed reviewers: + mdjones@chromium.org
Thank you Matt!
https://codereview.chromium.org/2903293002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ChromeSwitches.java (right): https://codereview.chromium.org/2903293002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ChromeSwitches.java:202: public static final String OMNIBOX_PLACEHOLDER_EXPERIMENT_SWITCH = nit: Maybe these should be OMNIBOX_HINT_EXPERIMENT. "Switch" isn't really necessary since we are in a file called *Switches.java. https://codereview.chromium.org/2903293002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ChromeSwitches.java:203: "omnibox-placeholder-experiment-switch"; Same thing here; "omnibox-hint-experiment" should be enough. https://codereview.chromium.org/2903293002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/UrlBarFieldTrial.java (right): https://codereview.chromium.org/2903293002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/omnibox/UrlBarFieldTrial.java:17: * Provides Field Trial support for Clank Omnibox Placeholder Experiment nit: punctuation. https://codereview.chromium.org/2903293002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/omnibox/UrlBarFieldTrial.java:19: nit: remove extra new-line. https://codereview.chromium.org/2903293002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/omnibox/UrlBarFieldTrial.java:34: private UrlBarFieldTrial() {} nit: /** Prevent initialization of this class. */ https://codereview.chromium.org/2903293002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/omnibox/UrlBarFieldTrial.java:39: public static void cacheUrlBarHint() { We should name everything consistently; UrlBar -> Omnibox, Placeholder -> Hint, etc. https://codereview.chromium.org/2903293002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/omnibox/UrlBarFieldTrial.java:43: CommandLine instance = CommandLine.getInstance(); It might be worth checking CommandLine.isInitialized() before attempting to access this (since flags are native). https://codereview.chromium.org/2903293002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/omnibox/UrlBarFieldTrial.java:81: private static void setUrlBarHint() { Based on the name of this function, it's unclear to me if the experiment flag is cached or the hint text itself is. https://codereview.chromium.org/2903293002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/omnibox/UrlBarFieldTrial.java:81: private static void setUrlBarHint() { nit: javadoc https://codereview.chromium.org/2903293002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/preferences/ChromePreferenceManager.java (right): https://codereview.chromium.org/2903293002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/preferences/ChromePreferenceManager.java:425: public void setUrlBarHint(String hint) { Same as above; is this caching the hint or the experiment switch? Also UrlBar -> Omnibox. https://codereview.chromium.org/2903293002/diff/1/chrome/browser/flag_descrip... File chrome/browser/flag_descriptions.cc (right): https://codereview.chromium.org/2903293002/diff/1/chrome/browser/flag_descrip... chrome/browser/flag_descriptions.cc:2258: "Switching this option changes the hint text in omnibox" nit: "...hint text in the omnibox..."
https://codereview.chromium.org/2903293002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ChromeSwitches.java (right): https://codereview.chromium.org/2903293002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ChromeSwitches.java:202: public static final String OMNIBOX_PLACEHOLDER_EXPERIMENT_SWITCH = On 2017/05/26 16:14:34, mdjones wrote: > nit: Maybe these should be OMNIBOX_HINT_EXPERIMENT. "Switch" isn't really > necessary since we are in a file called *Switches.java. Done. https://codereview.chromium.org/2903293002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ChromeSwitches.java:203: "omnibox-placeholder-experiment-switch"; On 2017/05/26 16:14:34, mdjones wrote: > Same thing here; "omnibox-hint-experiment" should be enough. Done. https://codereview.chromium.org/2903293002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/UrlBarFieldTrial.java (right): https://codereview.chromium.org/2903293002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/omnibox/UrlBarFieldTrial.java:17: * Provides Field Trial support for Clank Omnibox Placeholder Experiment On 2017/05/26 16:14:34, mdjones wrote: > nit: punctuation. Done. https://codereview.chromium.org/2903293002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/omnibox/UrlBarFieldTrial.java:19: On 2017/05/26 16:14:34, mdjones wrote: > nit: remove extra new-line. Done. https://codereview.chromium.org/2903293002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/omnibox/UrlBarFieldTrial.java:34: private UrlBarFieldTrial() {} On 2017/05/26 16:14:34, mdjones wrote: > nit: /** Prevent initialization of this class. */ Done. https://codereview.chromium.org/2903293002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/omnibox/UrlBarFieldTrial.java:39: public static void cacheUrlBarHint() { On 2017/05/26 16:14:34, mdjones wrote: > We should name everything consistently; UrlBar -> Omnibox, Placeholder -> Hint, > etc. Done. https://codereview.chromium.org/2903293002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/omnibox/UrlBarFieldTrial.java:43: CommandLine instance = CommandLine.getInstance(); On 2017/05/26 16:14:34, mdjones wrote: > It might be worth checking CommandLine.isInitialized() before attempting to > access this (since flags are native). Done. https://codereview.chromium.org/2903293002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/omnibox/UrlBarFieldTrial.java:81: private static void setUrlBarHint() { On 2017/05/26 16:14:34, mdjones wrote: > Based on the name of this function, it's unclear to me if the experiment flag is > cached or the hint text itself is. The hint text itself is cached in both shared preference and the static variable in this class. https://codereview.chromium.org/2903293002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/omnibox/UrlBarFieldTrial.java:81: private static void setUrlBarHint() { On 2017/05/26 16:14:34, mdjones wrote: > nit: javadoc Done. https://codereview.chromium.org/2903293002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/preferences/ChromePreferenceManager.java (right): https://codereview.chromium.org/2903293002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/preferences/ChromePreferenceManager.java:425: public void setUrlBarHint(String hint) { On 2017/05/26 16:14:34, mdjones wrote: > Same as above; is this caching the hint or the experiment switch? Also UrlBar -> > Omnibox. See above^ https://codereview.chromium.org/2903293002/diff/1/chrome/browser/flag_descrip... File chrome/browser/flag_descriptions.cc (right): https://codereview.chromium.org/2903293002/diff/1/chrome/browser/flag_descrip... chrome/browser/flag_descriptions.cc:2258: "Switching this option changes the hint text in omnibox" On 2017/05/26 16:14:35, mdjones wrote: > nit: "...hint text in the omnibox..." Done.
https://codereview.chromium.org/2903293002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/UrlBarFieldTrial.java (right): https://codereview.chromium.org/2903293002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/omnibox/UrlBarFieldTrial.java:81: private static void setUrlBarHint() { On 2017/05/26 17:21:36, huayinz wrote: > On 2017/05/26 16:14:34, mdjones wrote: > > Based on the name of this function, it's unclear to me if the experiment flag > is > > cached or the hint text itself is. > > The hint text itself is cached in both shared preference and the static variable > in this class. In most cases it's best practice to cache the flag value rather than the thing it is enabling. For example, imagine the language changes, you would have the incorrect text cached. It also helps if you need to make more than one decision based on the flag. You can store the current text in a static variable in this class, but the shared preferences should definitely contain the flag value. https://codereview.chromium.org/2903293002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeSwitches.java (right): https://codereview.chromium.org/2903293002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeSwitches.java:192: public static final String OMNIBOX_PLACEHOLDER_EXPERIMENT = "omnibox-placeholder-experiment"; Please update the other code in this patch to match the naming convention here. https://codereview.chromium.org/2903293002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/UrlBarFieldTrial.java (right): https://codereview.chromium.org/2903293002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omnibox/UrlBarFieldTrial.java:19: public class UrlBarFieldTrial { OmniboxPlaceholderFieldTrial https://codereview.chromium.org/2903293002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/ChromePreferenceManager.java (right): https://codereview.chromium.org/2903293002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/ChromePreferenceManager.java:61: private static final String URL_BAR_HINT_KEY = "url_bar_hint"; OMNIBOX_PLACEHOLDER_TEXT?
https://codereview.chromium.org/2903293002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/UrlBarFieldTrial.java (right): https://codereview.chromium.org/2903293002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omnibox/UrlBarFieldTrial.java:19: public class UrlBarFieldTrial { On 2017/05/26 22:08:51, mdjones wrote: > OmniboxPlaceholderFieldTrial Done. https://codereview.chromium.org/2903293002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/ChromePreferenceManager.java (right): https://codereview.chromium.org/2903293002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/ChromePreferenceManager.java:61: private static final String URL_BAR_HINT_KEY = "url_bar_hint"; On 2017/05/26 22:08:51, mdjones wrote: > OMNIBOX_PLACEHOLDER_TEXT? Done.
On 2017/05/26 22:55:04, huayinz wrote: > https://codereview.chromium.org/2903293002/diff/20001/chrome/android/java/src... > File > chrome/android/java/src/org/chromium/chrome/browser/omnibox/UrlBarFieldTrial.java > (right): > > https://codereview.chromium.org/2903293002/diff/20001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/omnibox/UrlBarFieldTrial.java:19: > public class UrlBarFieldTrial { > On 2017/05/26 22:08:51, mdjones wrote: > > OmniboxPlaceholderFieldTrial > > Done. > > https://codereview.chromium.org/2903293002/diff/20001/chrome/android/java/src... > File > chrome/android/java/src/org/chromium/chrome/browser/preferences/ChromePreferenceManager.java > (right): > > https://codereview.chromium.org/2903293002/diff/20001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/preferences/ChromePreferenceManager.java:61: > private static final String URL_BAR_HINT_KEY = "url_bar_hint"; > On 2017/05/26 22:08:51, mdjones wrote: > > OMNIBOX_PLACEHOLDER_TEXT? > > Done. Don't forget about my last comment in https://codereview.chromium.org/2903293002/diff/1/chrome/android/java/src/org...
Patchset #3 (id:40001) has been deleted
Patchset #4 (id:80001) has been deleted
https://codereview.chromium.org/2903293002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/UrlBarFieldTrial.java (right): https://codereview.chromium.org/2903293002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/omnibox/UrlBarFieldTrial.java:81: private static void setUrlBarHint() { On 2017/05/26 22:08:51, mdjones wrote: > On 2017/05/26 17:21:36, huayinz wrote: > > On 2017/05/26 16:14:34, mdjones wrote: > > > Based on the name of this function, it's unclear to me if the experiment > flag > > is > > > cached or the hint text itself is. > > > > The hint text itself is cached in both shared preference and the static > variable > > in this class. > > In most cases it's best practice to cache the flag value rather than the thing > it is enabling. For example, imagine the language changes, you would have the > incorrect text cached. It also helps if you need to make more than one decision > based on the flag. You can store the current text in a static variable in this > class, but the shared preferences should definitely contain the flag value. Done.
lgtm % naming nits. https://codereview.chromium.org/2903293002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/OmniboxPlaceholderFieldTrial.java (right): https://codereview.chromium.org/2903293002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/omnibox/OmniboxPlaceholderFieldTrial.java:47: ChromePreferenceManager.getInstance().setOmniboxPlaceholder(groupName); nit: setOmniboxPlaceholderGroup or similar https://codereview.chromium.org/2903293002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/ChromePreferenceManager.java (right): https://codereview.chromium.org/2903293002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/ChromePreferenceManager.java:61: private static final String OMNIBOX_PLACEHOLDER_KEY = "omnibox-placeholder"; nit: omnibox-placeholder-group https://codereview.chromium.org/2903293002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/util/FeatureUtilities.java (right): https://codereview.chromium.org/2903293002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/util/FeatureUtilities.java:192: OmniboxPlaceholderFieldTrial.cacheOmniboxPlaceholder(); nit: cacheOmniboxPlaceholderGroup
huayinz@chromium.org changed reviewers: + jwd@chromium.org, tedchoc@chromium.org
jwd@chromium.org: Please review changes in testing/variations/fieldtrial_testing_config.json tedchoc@chromium.org: Please review changes in all files Thanks so much, Becky https://codereview.chromium.org/2903293002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/OmniboxPlaceholderFieldTrial.java (right): https://codereview.chromium.org/2903293002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/omnibox/OmniboxPlaceholderFieldTrial.java:47: ChromePreferenceManager.getInstance().setOmniboxPlaceholder(groupName); On 2017/05/30 17:34:08, mdjones wrote: > nit: setOmniboxPlaceholderGroup or similar Done. https://codereview.chromium.org/2903293002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/ChromePreferenceManager.java (right): https://codereview.chromium.org/2903293002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/ChromePreferenceManager.java:61: private static final String OMNIBOX_PLACEHOLDER_KEY = "omnibox-placeholder"; On 2017/05/30 17:34:08, mdjones wrote: > nit: omnibox-placeholder-group Done. https://codereview.chromium.org/2903293002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/util/FeatureUtilities.java (right): https://codereview.chromium.org/2903293002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/util/FeatureUtilities.java:192: OmniboxPlaceholderFieldTrial.cacheOmniboxPlaceholder(); On 2017/05/30 17:34:08, mdjones wrote: > nit: cacheOmniboxPlaceholderGroup Done.
Description was changed from ========== Clank Omnibox Placeholder Experiment Added: + Variations of hint text in omnibox placeholder for the experiment + Chrome flags to change hint text for testing purpose + Field trial testing config for testing purpose + The hint text change will show up on the second launch after flag is set. BUG=723908 ========== to ========== [Android] Omnibox Placeholder Experiment Added: + Variations of hint text in omnibox placeholder for the experiment + Chrome flags to change hint text for testing purpose + Field trial testing config for testing purpose + The hint text change will show up on the second launch after flag is set. BUG=723908 ==========
fixed some naming issues
https://codereview.chromium.org/2903293002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/OmniboxPlaceholderFieldTrial.java (right): https://codereview.chromium.org/2903293002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/omnibox/OmniboxPlaceholderFieldTrial.java:39: String groupName = FieldTrialList.findFullName(FIELD_TRIAL_NAME); I "think" they are suggesting most experiments to use the FeatureList (see ChromeFeatureList.java) path instead of FieldTrialList. I guess I'll defer to the metrics reviewer, but I was under the impression they would want you to do something like: if (ChromeFeatureList.isEnabled(FIELD_TRIAL_NAME) { groupName = ChromeFeatureList.getFieldTrialParamByFeature( FIELD_TRIAL_NAME, STRING_OVERRIDE_TYPE); } The idea is that would give you more flexibility if you wanted to run multiple experiment arms with the same param so you could ensure the groups behave similarly. https://codereview.chromium.org/2903293002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/omnibox/OmniboxPlaceholderFieldTrial.java:53: private static void setOmniboxPlaceholder() { nit, but I would call this initOmniboxPlaceholder (set to me implies taking a param) https://codereview.chromium.org/2903293002/diff/140001/chrome/browser/about_f... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2903293002/diff/140001/chrome/browser/about_f... chrome/browser/about_flags.cc:679: const FeatureEntry::Choice kOmniboxPlaceholderExperimentChoices[] = { From the bug, it mentions that we don't need something in about:flags for this but we could set a command line instead. I think this is a bit overkill IMO for this experiment, so I wouldn't add this unless it is really needed. I don't see this as something a user would need to fiddle with and the testers can just set command lines that override the finch configs (which I can help with if needed)
https://codereview.chromium.org/2903293002/diff/140001/chrome/browser/about_f... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2903293002/diff/140001/chrome/browser/about_f... chrome/browser/about_flags.cc:679: const FeatureEntry::Choice kOmniboxPlaceholderExperimentChoices[] = { On 2017/05/30 22:14:28, Ted C wrote: > From the bug, it mentions that we don't need something in about:flags for this > but we could set a command line instead. I think this is a bit overkill IMO for > this experiment, so I wouldn't add this unless it is really needed. > > I don't see this as something a user would need to fiddle with and the testers > can just set command lines that override the finch configs (which I can help > with if needed) AFAIK I can only override finch config on user debug mode. I kept the flags b/c I'm not sure if tester is testing under user debug mode or user mode.
On 2017/05/30 23:10:30, huayinz wrote: > https://codereview.chromium.org/2903293002/diff/140001/chrome/browser/about_f... > File chrome/browser/about_flags.cc (right): > > https://codereview.chromium.org/2903293002/diff/140001/chrome/browser/about_f... > chrome/browser/about_flags.cc:679: const FeatureEntry::Choice > kOmniboxPlaceholderExperimentChoices[] = { > On 2017/05/30 22:14:28, Ted C wrote: > > From the bug, it mentions that we don't need something in about:flags for this > > but we could set a command line instead. I think this is a bit overkill IMO > for > > this experiment, so I wouldn't add this unless it is really needed. > > > > I don't see this as something a user would need to fiddle with and the testers > > can just set command lines that override the finch configs (which I can help > > with if needed) > > AFAIK I can only override finch config on user debug mode. I kept the flags b/c > I'm not sure if tester is testing under user debug mode or user mode. You are indeed correct about the debug mode. In my opinion, it is a balance we have to strike between the complexity we are adding to our UI/code vs the likelihood of the feature breaking on devices that are not userdebug. In this case, I think we are setting hint text very much like we always have but just with a different potential value. To me, it seems like overkill.
On 2017/05/30 23:13:26, Ted C wrote: > On 2017/05/30 23:10:30, huayinz wrote: > > > https://codereview.chromium.org/2903293002/diff/140001/chrome/browser/about_f... > > File chrome/browser/about_flags.cc (right): > > > > > https://codereview.chromium.org/2903293002/diff/140001/chrome/browser/about_f... > > chrome/browser/about_flags.cc:679: const FeatureEntry::Choice > > kOmniboxPlaceholderExperimentChoices[] = { > > On 2017/05/30 22:14:28, Ted C wrote: > > > From the bug, it mentions that we don't need something in about:flags for > this > > > but we could set a command line instead. I think this is a bit overkill IMO > > for > > > this experiment, so I wouldn't add this unless it is really needed. > > > > > > I don't see this as something a user would need to fiddle with and the > testers > > > can just set command lines that override the finch configs (which I can help > > > with if needed) > > > > AFAIK I can only override finch config on user debug mode. I kept the flags > b/c > > I'm not sure if tester is testing under user debug mode or user mode. > > You are indeed correct about the debug mode. In my opinion, it is a balance > we have to strike between the complexity we are adding to our UI/code vs > the likelihood of the feature breaking on devices that are not userdebug. > > In this case, I think we are setting hint text very much like we always > have but just with a different potential value. To me, it seems like > overkill. I agree that the flag might be too much for this experiment. I removed it:)
removed chrome flag https://codereview.chromium.org/2903293002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/OmniboxPlaceholderFieldTrial.java (right): https://codereview.chromium.org/2903293002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/omnibox/OmniboxPlaceholderFieldTrial.java:53: private static void setOmniboxPlaceholder() { On 2017/05/30 22:14:28, Ted C wrote: > nit, but I would call this initOmniboxPlaceholder (set to me implies taking a > param) Done.
lgtm https://codereview.chromium.org/2903293002/diff/160001/testing/variations/fie... File testing/variations/fieldtrial_testing_config.json (right): https://codereview.chromium.org/2903293002/diff/160001/testing/variations/fie... testing/variations/fieldtrial_testing_config.json:1906: "name": "SearchOrTypeUrl" Note: It's only the first experiment that's applied for dev builds and bots.
lgtm https://codereview.chromium.org/2903293002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/OmniboxPlaceholderFieldTrial.java (right): https://codereview.chromium.org/2903293002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/omnibox/OmniboxPlaceholderFieldTrial.java:28: private static String sCachedHint = null; null is the default, so need for this (and it actually slightly increases apk size .. context: https://groups.google.com/a/chromium.org/d/msg/chromium-dev/ylbLOvLs0bs/uVGEn...)
The CQ bit was checked by huayinz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mdjones@chromium.org, tedchoc@chromium.org, jwd@chromium.org Link to the patchset: https://codereview.chromium.org/2903293002/#ps180001 (title: "remove redundant initialization")
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": 180001, "attempt_start_ts": 1496255988643270,
"parent_rev": "c59068590b02e4c25544b62a5a9a5cf6863252b3", "commit_rev":
"ea3fb69a9bb9ce86d98fa6f9cabe6084a739fdf5"}
Message was sent while issue was closed.
Description was changed from ========== [Android] Omnibox Placeholder Experiment Added: + Variations of hint text in omnibox placeholder for the experiment + Chrome flags to change hint text for testing purpose + Field trial testing config for testing purpose + The hint text change will show up on the second launch after flag is set. BUG=723908 ========== to ========== [Android] Omnibox Placeholder Experiment Added: + Variations of hint text in omnibox placeholder for the experiment + Chrome flags to change hint text for testing purpose + Field trial testing config for testing purpose + The hint text change will show up on the second launch after flag is set. BUG=723908 Review-Url: https://codereview.chromium.org/2903293002 Cr-Commit-Position: refs/heads/master@{#476027} Committed: https://chromium.googlesource.com/chromium/src/+/ea3fb69a9bb9ce86d98fa6f9cabe... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:180001) as https://chromium.googlesource.com/chromium/src/+/ea3fb69a9bb9ce86d98fa6f9cabe... |
