Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2724303002:
Omnibox - Zero Suggest - Log When/Whether Contextual Search is Possible (Closed)
DescriptionOmnibox - Zero Suggest - Log When/Whether Contextual Search is Possible
This will allow us to estimate the "headroom", i.e., how many people and
how many focus events could we actually help with contextual suggestions.
No functional changes (intended).
TEST=using about:histograms
BUG=696077
Review-Url: https://codereview.chromium.org/2724303002
Cr-Commit-Position: refs/heads/master@{#454483}
Committed: https://chromium.googlesource.com/chromium/src/+/7e53e414ae9db0bcb229b4b63053f6748fa93681
Patch Set 1 #Patch Set 2 : polish #Patch Set 3 : guard constructor for testing #Patch Set 4 : correct part of histograms.xml description #
Total comments: 2
Patch Set 5 : correct typo #
Total comments: 28
Patch Set 6 : pkasting's comments, plus improve metrics description #
Messages
Total messages: 38 (28 generated)
Description was changed from ========== Omnibox - Zero Suggest - Log When/Whether Contextual Search is Possible This will allow us to estimate the "headroom", i.e., how many people and how many focus events could we actually help with contextual suggestions. BUG=696077 ========== to ========== Omnibox - Zero Suggest - Log When/Whether Contextual Search is Possible This will allow us to estimate the "headroom", i.e., how many people and how many focus events could we actually help with contextual suggestions. BUG=696077 ==========
mpearson@chromium.org changed reviewers: + pkasting@chromium.org
Description was changed from ========== Omnibox - Zero Suggest - Log When/Whether Contextual Search is Possible This will allow us to estimate the "headroom", i.e., how many people and how many focus events could we actually help with contextual suggestions. BUG=696077 ========== to ========== Omnibox - Zero Suggest - Log When/Whether Contextual Search is Possible This will allow us to estimate the "headroom", i.e., how many people and how many focus events could we actually help with contextual suggestions. No functional changes (intended). BUG=696077 ==========
Description was changed from ========== Omnibox - Zero Suggest - Log When/Whether Contextual Search is Possible This will allow us to estimate the "headroom", i.e., how many people and how many focus events could we actually help with contextual suggestions. No functional changes (intended). BUG=696077 ========== to ========== Omnibox - Zero Suggest - Log When/Whether Contextual Search is Possible This will allow us to estimate the "headroom", i.e., how many people and how many focus events could we actually help with contextual suggestions. No functional changes (intended). TEST=use about:histograms (HAVE NOT YET TESTED) BUG=696077 ==========
[for folks on the CC link, this is changelist is just an FYI] Peter, Can you please review this sometime? It'd be nice to get it in by EOD Friday but it's not a big deal if you're busy or feeling overwhelmed. It'd be an easy thing to request approval to merge later, probably auto-granted. BTW, I have not yet had a chance to test this using about:histograms. (I can't do it at the computer I'm sitting at right now for complicated reasons.) I'll do that soon. thanks, mark
On 2017/03/02 05:30:19, Mark P wrote: > [for folks on the CC link, this is changelist is just an FYI] > > Peter, > > Can you please review this sometime? It'd be nice to get it in by EOD Friday oops, apparently I meant Thursday > but it's not a big deal if you're busy or feeling overwhelmed. It'd be an easy > thing to request approval to merge later, probably auto-granted. > > BTW, I have not yet had a chance to test this using about:histograms. (I can't > do it at the computer I'm sitting at right now for complicated reasons.) I'll > do that soon. > > thanks, > mark
mpearson@chromium.org changed reviewers: + isherman@chromium.org
+isherman for metrics review and approval Thanks Ilya! --mark
The CQ bit was checked by mpearson@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: 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_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== Omnibox - Zero Suggest - Log When/Whether Contextual Search is Possible This will allow us to estimate the "headroom", i.e., how many people and how many focus events could we actually help with contextual suggestions. No functional changes (intended). TEST=use about:histograms (HAVE NOT YET TESTED) BUG=696077 ========== to ========== Omnibox - Zero Suggest - Log When/Whether Contextual Search is Possible This will allow us to estimate the "headroom", i.e., how many people and how many focus events could we actually help with contextual suggestions. No functional changes (intended). TEST=use about:histograms (HAVE NOT YET TESTED USING A ELIGIBLE ACCONT) BUG=696077 ==========
Description was changed from ========== Omnibox - Zero Suggest - Log When/Whether Contextual Search is Possible This will allow us to estimate the "headroom", i.e., how many people and how many focus events could we actually help with contextual suggestions. No functional changes (intended). TEST=use about:histograms (HAVE NOT YET TESTED USING A ELIGIBLE ACCONT) BUG=696077 ========== to ========== Omnibox - Zero Suggest - Log When/Whether Contextual Search is Possible This will allow us to estimate the "headroom", i.e., how many people and how many focus events could we actually help with contextual suggestions. No functional changes (intended). TEST=use about:histograms (HAVE NOT YET TESTED USING A ELIGIBLE ACCOUNT) BUG=696077 ==========
The CQ bit was checked by mpearson@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 checked by mpearson@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.
Metrics LGTM, thanks Mark. https://codereview.chromium.org/2724303002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2724303002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:113287: +<enum name="ZeroSuggestEligibleOnFocus" type="int"> Wow, you've reached the very tail end of the file! Good job! =P
https://codereview.chromium.org/2724303002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2724303002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:113287: +<enum name="ZeroSuggestEligibleOnFocus" type="int"> On 2017/03/02 17:42:03, Ilya Sherman wrote: > Wow, you've reached the very tail end of the file! Good job! =P Acknowledged. I was thinking the same thing too! :-)
The CQ bit was checked by mpearson@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...
LGTM https://codereview.chromium.org/2724303002/diff/80001/components/omnibox/brow... File components/omnibox/browser/zero_suggest_provider.cc (right): https://codereview.chromium.org/2724303002/diff/80001/components/omnibox/brow... components/omnibox/browser/zero_suggest_provider.cc:47: // Represents if, on focus, ZeroSuggestProvider is allowed to display Nit: "Represents whether ZeroSuggestProvider is allowed to display contextual suggestions on focus, and if not, why not." https://codereview.chromium.org/2724303002/diff/80001/components/omnibox/brow... components/omnibox/browser/zero_suggest_provider.cc:51: enum ZeroSuggestEligibleOnFocus { Nit: Maybe you should use an enum class, and then remove ZERO_SUGGEST_ from the values below. I might also call this ZeroSuggestEligibility (is "On focus" necessary in the name to distinguish from another kind of zerosuggest eligibility?). https://codereview.chromium.org/2724303002/diff/80001/components/omnibox/brow... components/omnibox/browser/zero_suggest_provider.cc:54: ZERO_SUGGEST_GENERALLY_INELIGIBLE = 2, Nit: Maybe _URL_INELIGIBLE versus _GLOBALLY_INELIGIBLE? I'm not a fan of "generally" because it's vague (are there exceptions to the general rule?), and _BUT_NOT_THIS_URL is so verbose. https://codereview.chromium.org/2724303002/diff/80001/components/omnibox/brow... components/omnibox/browser/zero_suggest_provider.cc:82: std::string kArbitraryInsecureUrlString = "http://www.google.com/"; Nit: Would an intranet hostname like "test/" work? It would be nice to use a URL that's obviously not something we rely on having real meaning or being accessible. And bonus points for saving a couple bytes in the binary by being shorter :) https://codereview.chromium.org/2724303002/diff/80001/components/omnibox/brow... components/omnibox/browser/zero_suggest_provider.cc:118: std::string url_string(GetContextualSuggestionsUrl()); Nit: Prefer = to () for simple inits like this; see https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Variab... https://codereview.chromium.org/2724303002/diff/80001/components/omnibox/brow... components/omnibox/browser/zero_suggest_provider.cc:126: client()->GetTemplateURLService(); I don't have specific recommendations here, but I notice this code and GetContextualSuggestionsUrl() both have to fetch the TemplateURLService/default search provider like this, and both this code and the constructor get the contextual suggestions URL and call CanSendURL(). Think about whether there's any way to refactor into more helpers to avoid code duplication or something. There may not be. https://codereview.chromium.org/2724303002/diff/80001/components/omnibox/brow... components/omnibox/browser/zero_suggest_provider.cc:143: : ZERO_SUGGEST_GENERALLY_INELIGIBLE); Nit: Not a fan of nesting ?:. How about something like this to avoid it: ZeroSuggestEligibleOnFocus eligibility = ZERO_SUGGEST_ELIGIBLE; if (!can_send_current_url) { eligibility = CanSendURL(arbitrary_insecure_url, ...) ? ZERO_SUGGEST_GENERALLY_ELIGIBLE_BUT_NOT_THIS_URL : ZERO_SUGGEST_GENERALLY_INELIGIBLE; } This also avoids the second CanSendURL() call unless it's actually needed. https://codereview.chromium.org/2724303002/diff/80001/components/omnibox/brow... components/omnibox/browser/zero_suggest_provider.cc:156: base::string16 prefix; Nit: Can just pass string16() in the call below, no need for this temp https://codereview.chromium.org/2724303002/diff/80001/components/omnibox/brow... components/omnibox/browser/zero_suggest_provider.cc:232: // To check whether this is allowed, use an arbitrary insecure (http) URL Nit: I'd move these last 3/4ths of the comment to just above the actual histogram call below, since they're specific explanations of technical details in that call, rather than a general description of what the block is doing like the first line is. https://codereview.chromium.org/2724303002/diff/80001/components/omnibox/brow... components/omnibox/browser/zero_suggest_provider.cc:238: if (template_url_service != nullptr) { Nit: Optional, but I find it slightly more common to just do "if (x)" or "if (!x)" than "if (x != nullptr)" or "if (x == nullptr)". https://codereview.chromium.org/2724303002/diff/80001/components/omnibox/brow... components/omnibox/browser/zero_suggest_provider.cc:520: // in the redirect-to-chrome field trial). Nit: in -> is in https://codereview.chromium.org/2724303002/diff/80001/components/omnibox/brow... components/omnibox/browser/zero_suggest_provider.cc:526: return ""; Nit: "" -> std::string() https://codereview.chromium.org/2724303002/diff/80001/components/omnibox/brow... components/omnibox/browser/zero_suggest_provider.cc:528: if (OmniboxFieldTrial::InZeroSuggestRedirectToChromeFieldTrial()) { Nit: No {} https://codereview.chromium.org/2724303002/diff/80001/components/omnibox/brow... components/omnibox/browser/zero_suggest_provider.cc:531: base::string16 prefix; Nit: Can just pass string16() in the call below, no need for this temp
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I will add follow-up comments in a later changelist. Submitting now. --mark https://codereview.chromium.org/2724303002/diff/80001/components/omnibox/brow... File components/omnibox/browser/zero_suggest_provider.cc (right): https://codereview.chromium.org/2724303002/diff/80001/components/omnibox/brow... components/omnibox/browser/zero_suggest_provider.cc:47: // Represents if, on focus, ZeroSuggestProvider is allowed to display On 2017/03/02 21:31:34, Peter Kasting wrote: > Nit: "Represents whether ZeroSuggestProvider is allowed to display contextual > suggestions on focus, and if not, why not." Done. https://codereview.chromium.org/2724303002/diff/80001/components/omnibox/brow... components/omnibox/browser/zero_suggest_provider.cc:51: enum ZeroSuggestEligibleOnFocus { On 2017/03/02 21:31:34, Peter Kasting wrote: > Nit: Maybe you should use an enum class, and then remove ZERO_SUGGEST_ from the > values below. Sure. Done. > I might also call this ZeroSuggestEligibility (is "On focus" necessary in the > name to distinguish from another kind of zerosuggest eligibility?). I thought so originally because I had the zero suggest on profile open histogram. Then I realized that was also a zero suggest on focus, just recorded on profile open. Took your shorter name. Zero suggest only does this involving focus. https://codereview.chromium.org/2724303002/diff/80001/components/omnibox/brow... components/omnibox/browser/zero_suggest_provider.cc:54: ZERO_SUGGEST_GENERALLY_INELIGIBLE = 2, On 2017/03/02 21:31:34, Peter Kasting wrote: > Nit: Maybe _URL_INELIGIBLE versus _GLOBALLY_INELIGIBLE? Revised the middle enum name and added a comment. > I'm not a fan of > "generally" because it's vague (are there exceptions to the general rule?), Yes, there are exceptions here. For example, if the user has an incognito window open, they're generally ineligible as long as it's open, but they become eligible again when it's closed. This is why I intentionally am using a term like that as opposed to plain old ineligible. Left the last enum name. > and > _BUT_NOT_THIS_URL is so verbose. https://codereview.chromium.org/2724303002/diff/80001/components/omnibox/brow... components/omnibox/browser/zero_suggest_provider.cc:82: std::string kArbitraryInsecureUrlString = "http://www.google.com/"; On 2017/03/02 21:31:34, Peter Kasting wrote: > Nit: Would an intranet hostname like "test/" work? No. I can easily imagine having a test (not sure if we do) that prevents sending intranet URLs to a suggest server. > It would be nice to use a > URL that's obviously not something we rely on having real meaning or being > accessible. And bonus points for saving a couple bytes in the binary by being > shorter :) I'm open to another full domain name URL. I think Google is appropriate at this point, as the only zero suggest provider suggest server is Google (so it's even more likely that a URL-level restriction test will say, yes, it's okay to send a Google URL to a Google suggest server). https://codereview.chromium.org/2724303002/diff/80001/components/omnibox/brow... components/omnibox/browser/zero_suggest_provider.cc:118: std::string url_string(GetContextualSuggestionsUrl()); On 2017/03/02 21:31:34, Peter Kasting wrote: > Nit: Prefer = to () for simple inits like this; see > https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Variab... Done. https://codereview.chromium.org/2724303002/diff/80001/components/omnibox/brow... components/omnibox/browser/zero_suggest_provider.cc:126: client()->GetTemplateURLService(); On 2017/03/02 21:31:34, Peter Kasting wrote: > I don't have specific recommendations here, but I notice this code and > GetContextualSuggestionsUrl() both have to fetch the TemplateURLService/default > search provider like this, and both this code and the constructor get the > contextual suggestions URL and call CanSendURL(). > > Think about whether there's any way to refactor into more helpers to avoid code > duplication or something. There may not be. There isn't, because most of the parameters between the two CanSendURL calls differ, partially intentionally (different page classification for example in the constructor call), partially as a side effect of logging in the constructor (cannot call overloaded functions). I was a little bothered by the extra calls to GetTemplateURLService() and GetDefaultSearchProvider() in the current flow in Start(). (They get called both here and in GetContextualSuggestionsUrl().) Yet, I tried to pass in those variables to GetContextualSuggestionsUrl() just so we don't have to make those calls again. It looks ugly. And I doubt it's any more efficient; these calls are fast, and the compiler might be smart enough to optimize them down to one. https://codereview.chromium.org/2724303002/diff/80001/components/omnibox/brow... components/omnibox/browser/zero_suggest_provider.cc:143: : ZERO_SUGGEST_GENERALLY_INELIGIBLE); On 2017/03/02 21:31:34, Peter Kasting wrote: > Nit: Not a fan of nesting ?:. How about something like this to avoid it: > > ZeroSuggestEligibleOnFocus eligibility = ZERO_SUGGEST_ELIGIBLE; > if (!can_send_current_url) { > eligibility = CanSendURL(arbitrary_insecure_url, ...) > ? ZERO_SUGGEST_GENERALLY_ELIGIBLE_BUT_NOT_THIS_URL > : ZERO_SUGGEST_GENERALLY_INELIGIBLE; > } > > This also avoids the second CanSendURL() call unless it's actually needed. Good idea. Done. https://codereview.chromium.org/2724303002/diff/80001/components/omnibox/brow... components/omnibox/browser/zero_suggest_provider.cc:156: base::string16 prefix; On 2017/03/02 21:31:34, Peter Kasting wrote: > Nit: Can just pass string16() in the call below, no need for this temp I tried getting rid of it when I wrote the code, then looked at the code and thought, oh, why am I passing an empty string into SearchTermsArgs's constructor. Having this extra variable makes it clear. I thought there was a tip-of-the-week episode about this recently, but I can't find it. https://codereview.chromium.org/2724303002/diff/80001/components/omnibox/brow... components/omnibox/browser/zero_suggest_provider.cc:232: // To check whether this is allowed, use an arbitrary insecure (http) URL On 2017/03/02 21:31:34, Peter Kasting wrote: > Nit: I'd move these last 3/4ths of the comment to just above the actual > histogram call below, since they're specific explanations of technical details > in that call, rather than a general description of what the block is doing like > the first line is. Good diagnosis of the situation. Done. https://codereview.chromium.org/2724303002/diff/80001/components/omnibox/brow... components/omnibox/browser/zero_suggest_provider.cc:238: if (template_url_service != nullptr) { On 2017/03/02 21:31:34, Peter Kasting wrote: > Nit: Optional, but I find it slightly more common to just do "if (x)" or "if > (!x)" than "if (x != nullptr)" or "if (x == nullptr)". Left as is. (That may be true in general in the codebase, but the code in these search provider files tends to explicitly check for nulls.) https://codereview.chromium.org/2724303002/diff/80001/components/omnibox/brow... components/omnibox/browser/zero_suggest_provider.cc:520: // in the redirect-to-chrome field trial). On 2017/03/02 21:31:34, Peter Kasting wrote: > Nit: in -> is in Done. https://codereview.chromium.org/2724303002/diff/80001/components/omnibox/brow... components/omnibox/browser/zero_suggest_provider.cc:526: return ""; On 2017/03/02 21:31:34, Peter Kasting wrote: > Nit: "" -> std::string() Done. https://codereview.chromium.org/2724303002/diff/80001/components/omnibox/brow... components/omnibox/browser/zero_suggest_provider.cc:528: if (OmniboxFieldTrial::InZeroSuggestRedirectToChromeFieldTrial()) { On 2017/03/02 21:31:34, Peter Kasting wrote: > Nit: No {} Done. https://codereview.chromium.org/2724303002/diff/80001/components/omnibox/brow... components/omnibox/browser/zero_suggest_provider.cc:531: base::string16 prefix; On 2017/03/02 21:31:34, Peter Kasting wrote: > Nit: Can just pass string16() in the call below, no need for this temp See earlier comment.
Description was changed from ========== Omnibox - Zero Suggest - Log When/Whether Contextual Search is Possible This will allow us to estimate the "headroom", i.e., how many people and how many focus events could we actually help with contextual suggestions. No functional changes (intended). TEST=use about:histograms (HAVE NOT YET TESTED USING A ELIGIBLE ACCOUNT) BUG=696077 ========== to ========== Omnibox - Zero Suggest - Log When/Whether Contextual Search is Possible This will allow us to estimate the "headroom", i.e., how many people and how many focus events could we actually help with contextual suggestions. No functional changes (intended). TEST=use about:histograms BUG=696077 ==========
Description was changed from ========== Omnibox - Zero Suggest - Log When/Whether Contextual Search is Possible This will allow us to estimate the "headroom", i.e., how many people and how many focus events could we actually help with contextual suggestions. No functional changes (intended). TEST=use about:histograms BUG=696077 ========== to ========== Omnibox - Zero Suggest - Log When/Whether Contextual Search is Possible This will allow us to estimate the "headroom", i.e., how many people and how many focus events could we actually help with contextual suggestions. No functional changes (intended). TEST=using about:histograms BUG=696077 ==========
The CQ bit was checked by mpearson@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org, pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2724303002/#ps100001 (title: "pkasting's comments, plus improve metrics description")
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": 1488496843087480,
"parent_rev": "24b50378a13f0099e231bcbfefaea67eec005ab3", "commit_rev":
"7e53e414ae9db0bcb229b4b63053f6748fa93681"}
Message was sent while issue was closed.
Description was changed from ========== Omnibox - Zero Suggest - Log When/Whether Contextual Search is Possible This will allow us to estimate the "headroom", i.e., how many people and how many focus events could we actually help with contextual suggestions. No functional changes (intended). TEST=using about:histograms BUG=696077 ========== to ========== Omnibox - Zero Suggest - Log When/Whether Contextual Search is Possible This will allow us to estimate the "headroom", i.e., how many people and how many focus events could we actually help with contextual suggestions. No functional changes (intended). TEST=using about:histograms BUG=696077 Review-Url: https://codereview.chromium.org/2724303002 Cr-Commit-Position: refs/heads/master@{#454483} Committed: https://chromium.googlesource.com/chromium/src/+/7e53e414ae9db0bcb229b4b63053... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/7e53e414ae9db0bcb229b4b63053...
Message was sent while issue was closed.
alph@chromium.org changed reviewers: + alph@chromium.org
Message was sent while issue was closed.
Reverting the patch for breaking Linux bot. https://codereview.chromium.org/2726283004 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
