Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2889823002:
[omnibox] Added 'OmniboxTailSuggestions' flag (Closed)
|
Created:
3 years, 7 months ago by Kevin Bailey Modified:
3 years, 6 months ago CC:
chromium-reviews, jdonnelly+watch_chromium.org, asvitkine+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[omnibox] Added 'OmniboxTailSuggestions' flag
Solely for experimenting with new query "tail" suggestions.
BUG=726769
Review-Url: https://codereview.chromium.org/2889823002
Cr-Commit-Position: refs/heads/master@{#476391}
Committed: https://chromium.googlesource.com/chromium/src/+/cc6214142ae45020926c78c3b1dff4eea7c7eedb
Patch Set 1 #Patch Set 2 : Rebase and formatting #Patch Set 3 : Added histogram enumeration #
Total comments: 11
Patch Set 4 : Comment and string changes #
Total comments: 4
Patch Set 5 : Changed string descriptions #
Total comments: 3
Patch Set 6 : Rebase #
Messages
Total messages: 27 (15 generated)
Description was changed from ========== [omnibox] Added 'OmniboxTailSuggestions' flag Solely for experimenting with new query suggestions. BUG= ========== to ========== [omnibox] Added 'OmniboxTailSuggestions' flag Solely for experimenting with new query "tail" suggestions. BUG=726769 ==========
krb@chromium.org changed reviewers: + mpearson@chromium.org
Bots are turning green. Could you take a look, Mark?
ping? Maybe got lost among the holiday email avalanche.
On 2017/05/31 18:42:45, Kevin Bailey wrote: > ping? Maybe got lost among the holiday email avalanche. Indeed, somehow I did miss this. Doing it now. --mark
lgtm modulo numerous minor comments --mark https://codereview.chromium.org/2889823002/diff/40001/chrome/browser/about_fl... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2889823002/diff/40001/chrome/browser/about_fl... chrome/browser/about_flags.cc:2817: {"omnibox-tail-suggestions", flag_descriptions::kOmniboxTailSuggestionsName, side comment: putting this here is fine with me, though I always feel compelled to point out that at the top of this list is a comment // To add a new entry, add to the end of kFeatureEntries. https://codereview.chromium.org/2889823002/diff/40001/chrome/browser/flag_des... File chrome/browser/flag_descriptions.cc (right): https://codereview.chromium.org/2889823002/diff/40001/chrome/browser/flag_des... chrome/browser/flag_descriptions.cc:2847: "Enable receiving tail suggestions in Omnibox."; The meaning of "tail suggestions" is not obvious. Please make this description understandable by someone who doesn't work on the omnibox. https://codereview.chromium.org/2889823002/diff/40001/chrome/browser/flag_des... File chrome/browser/flag_descriptions.h (right): https://codereview.chromium.org/2889823002/diff/40001/chrome/browser/flag_des... chrome/browser/flag_descriptions.h:1388: extern const char kOmniboxTailSuggestionsName[]; Uh, how does this compile? This is only defined for !OS_IOS blocks, yet the code in about flags is enabled for IOS. (I think IOS passes OS_MACOSX test in the about flags file. Yet the entity code compiles. /confused) https://codereview.chromium.org/2889823002/diff/40001/components/omnibox/brow... File components/omnibox/browser/omnibox_field_trial.cc (right): https://codereview.chromium.org/2889823002/diff/40001/components/omnibox/brow... components/omnibox/browser/omnibox_field_trial.cc:48: "OmniboxTailSuggestions", base::FEATURE_DISABLED_BY_DEFAULT}; Do you want an OS_ANDROID: enabled by default statement here? Otherwise this implies that it's disabled by default everywhere, which isn't the case. (It works on ANDROID already.) Or revise the comment.
A couple of these seem like more than nits, so I'll wait until EOD before submitting, in case you wanted to tweak them. https://codereview.chromium.org/2889823002/diff/40001/chrome/browser/about_fl... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2889823002/diff/40001/chrome/browser/about_fl... chrome/browser/about_flags.cc:2817: {"omnibox-tail-suggestions", flag_descriptions::kOmniboxTailSuggestionsName, On 2017/05/31 23:17:13, Mark P wrote: > side comment: putting this here is fine with me, though I always feel compelled > to point out that at the top of this list is a comment > // To add a new entry, add to the end of kFeatureEntries. I would prefer putting it here, both for locality of Omnibox features, as well as reduction in merge conflicts (which are always at their worst when someone is trying to fix a broken build.) If you don't mind, great. https://codereview.chromium.org/2889823002/diff/40001/chrome/browser/flag_des... File chrome/browser/flag_descriptions.cc (right): https://codereview.chromium.org/2889823002/diff/40001/chrome/browser/flag_des... chrome/browser/flag_descriptions.cc:2847: "Enable receiving tail suggestions in Omnibox."; On 2017/05/31 23:17:13, Mark P wrote: > The meaning of "tail suggestions" is not obvious. Please make this description > understandable by someone who doesn't work on the omnibox. I was duplicating 'entity suggestions'. I assume it should be fixed too. https://codereview.chromium.org/2889823002/diff/40001/chrome/browser/flag_des... File chrome/browser/flag_descriptions.h (right): https://codereview.chromium.org/2889823002/diff/40001/chrome/browser/flag_des... chrome/browser/flag_descriptions.h:1388: extern const char kOmniboxTailSuggestionsName[]; On 2017/05/31 23:17:13, Mark P wrote: > Uh, how does this compile? This is only defined for !OS_IOS blocks, yet the > code in about flags is enabled for IOS. (I think IOS passes OS_MACOSX test in > the about flags file. Yet the entity code compiles. /confused) I believe IOS is not a subset of MACOS. I had assumed CHROMEOS was a subset of LINUX but was wrong. I suspect it would become too verbose to have to put MACOS && !IOS all over the place. 2 IOS bots compiled successfully. https://codereview.chromium.org/2889823002/diff/40001/components/omnibox/brow... File components/omnibox/browser/omnibox_field_trial.cc (right): https://codereview.chromium.org/2889823002/diff/40001/components/omnibox/brow... components/omnibox/browser/omnibox_field_trial.cc:48: "OmniboxTailSuggestions", base::FEATURE_DISABLED_BY_DEFAULT}; On 2017/05/31 23:17:13, Mark P wrote: > Do you want an OS_ANDROID: enabled by default statement here? Otherwise this > implies that it's disabled by default everywhere, which isn't the case. (It > works on ANDROID already.) > > Or revise the comment. I revised the comment, since all the flag does is enable a particular experiment.
lgtm, with one comment below --mark https://codereview.chromium.org/2889823002/diff/40001/chrome/browser/about_fl... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2889823002/diff/40001/chrome/browser/about_fl... chrome/browser/about_flags.cc:2817: {"omnibox-tail-suggestions", flag_descriptions::kOmniboxTailSuggestionsName, On 2017/06/01 14:32:42, Kevin Bailey wrote: > On 2017/05/31 23:17:13, Mark P wrote: > > side comment: putting this here is fine with me, though I always feel > compelled > > to point out that at the top of this list is a comment > > // To add a new entry, add to the end of kFeatureEntries. > > I would prefer putting it here, both for locality of Omnibox features, as well > as reduction in merge conflicts (which are always at their worst when someone is > trying to fix a broken build.) If you don't mind, great. Acknowledged. https://codereview.chromium.org/2889823002/diff/40001/chrome/browser/flag_des... File chrome/browser/flag_descriptions.cc (right): https://codereview.chromium.org/2889823002/diff/40001/chrome/browser/flag_des... chrome/browser/flag_descriptions.cc:2847: "Enable receiving tail suggestions in Omnibox."; On 2017/06/01 14:32:42, Kevin Bailey wrote: > On 2017/05/31 23:17:13, Mark P wrote: > > The meaning of "tail suggestions" is not obvious. Please make this > description > > understandable by someone who doesn't work on the omnibox. > > I was duplicating 'entity suggestions'. I assume it should be fixed too. Acknowledged. https://codereview.chromium.org/2889823002/diff/40001/components/omnibox/brow... File components/omnibox/browser/omnibox_field_trial.cc (right): https://codereview.chromium.org/2889823002/diff/40001/components/omnibox/brow... components/omnibox/browser/omnibox_field_trial.cc:48: "OmniboxTailSuggestions", base::FEATURE_DISABLED_BY_DEFAULT}; On 2017/06/01 14:32:42, Kevin Bailey wrote: > On 2017/05/31 23:17:13, Mark P wrote: > > Do you want an OS_ANDROID: enabled by default statement here? Otherwise this > > implies that it's disabled by default everywhere, which isn't the case. (It > > works on ANDROID already.) > > > > Or revise the comment. > > I revised the comment, since all the flag does is enable a particular > experiment. Acknowledged. https://codereview.chromium.org/2889823002/diff/60001/chrome/browser/flag_des... File chrome/browser/flag_descriptions.cc (right): https://codereview.chromium.org/2889823002/diff/60001/chrome/browser/flag_des... chrome/browser/flag_descriptions.cc:2843: "Enable receiving entity suggestions - unambiguation descriptions - for " nit: I think the word you are looking for is "disambiguation" https://codereview.chromium.org/2889823002/diff/60001/chrome/browser/flag_des... chrome/browser/flag_descriptions.cc:2849: "suggestion, for Omnibox queries."; "an additional type of search suggestion" is pretty vague. How about something like "a type of search suggestion based on the last few words in the query"?
The CQ bit was checked by krb@chromium.org to run a CQ dry run
https://codereview.chromium.org/2889823002/diff/60001/chrome/browser/flag_des... File chrome/browser/flag_descriptions.cc (right): https://codereview.chromium.org/2889823002/diff/60001/chrome/browser/flag_des... chrome/browser/flag_descriptions.cc:2843: "Enable receiving entity suggestions - unambiguation descriptions - for " On 2017/06/01 16:23:58, Mark P wrote: > nit: I think the word you are looking for is "disambiguation" Yes, thanks. https://codereview.chromium.org/2889823002/diff/60001/chrome/browser/flag_des... chrome/browser/flag_descriptions.cc:2849: "suggestion, for Omnibox queries."; On 2017/06/01 16:23:58, Mark P wrote: > "an additional type of search suggestion" is pretty vague. > How about something like "a type of search suggestion based on the last few > words in the query"? Ok, but I think these descriptions are more for ruling _out_ flags, as the user scans the list, so shorter might have been better.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
https://codereview.chromium.org/2889823002/diff/80001/chrome/browser/about_fl... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2889823002/diff/80001/chrome/browser/about_fl... chrome/browser/about_flags.cc:2818: flag_descriptions::kOmniboxTailSuggestionsDescription, kOsDesktop, drive-by: the tail suggest deck proposes enabling tail suggest for desktop and iOS, yet this flag only does desktop. Which one is wrong?
https://codereview.chromium.org/2889823002/diff/80001/chrome/browser/about_fl... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2889823002/diff/80001/chrome/browser/about_fl... chrome/browser/about_flags.cc:2818: flag_descriptions::kOmniboxTailSuggestionsDescription, kOsDesktop, On 2017/06/01 17:29:50, Mark P wrote: > drive-by: the tail suggest deck proposes enabling tail suggest for desktop and > iOS, yet this flag only does desktop. Which one is wrong? While the desktop is capable of receiving and handling tail suggestions, my understanding is that iOS is not. However we would like to add this support. So as far as this CL is concerned, the answer is more like "not yet" than "never and the doc is wrong."
The CQ bit was checked by krb@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...
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 krb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mpearson@chromium.org Link to the patchset: https://codereview.chromium.org/2889823002/#ps100001 (title: "Rebase")
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": 1496345632067130,
"parent_rev": "b80a8570e2b0a3fe8ac5e45b84614c08f4584ee1", "commit_rev":
"cc6214142ae45020926c78c3b1dff4eea7c7eedb"}
Message was sent while issue was closed.
Description was changed from ========== [omnibox] Added 'OmniboxTailSuggestions' flag Solely for experimenting with new query "tail" suggestions. BUG=726769 ========== to ========== [omnibox] Added 'OmniboxTailSuggestions' flag Solely for experimenting with new query "tail" suggestions. BUG=726769 Review-Url: https://codereview.chromium.org/2889823002 Cr-Commit-Position: refs/heads/master@{#476391} Committed: https://chromium.googlesource.com/chromium/src/+/cc6214142ae45020926c78c3b1df... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/cc6214142ae45020926c78c3b1df...
Message was sent while issue was closed.
jdonnelly@chromium.org changed reviewers: + jdonnelly@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2889823002/diff/80001/chrome/browser/about_fl... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2889823002/diff/80001/chrome/browser/about_fl... chrome/browser/about_flags.cc:2818: flag_descriptions::kOmniboxTailSuggestionsDescription, kOsDesktop, On 2017/06/01 17:45:54, Kevin Bailey wrote: > On 2017/06/01 17:29:50, Mark P wrote: > > drive-by: the tail suggest deck proposes enabling tail suggest for desktop and > > iOS, yet this flag only does desktop. Which one is wrong? > > While the desktop is capable of receiving and handling tail suggestions, my > understanding is that iOS is not. However we would like to add this support. So > as far as this CL is concerned, the answer is more like "not yet" than "never > and the doc is wrong." I've added a comment to the doc mentioning that the iOS implementation will follow in the subsequent milestone. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
