Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2755503002:
Add a new entry to omnibox_event.proto to log specific type of contextual suggestions (Closed)
DescriptionAdd a new entry to omnibox_event.proto to log specific types for the suggestions provided by omnibox. Moreover, in this CL, we implement the mechanics of populating this new entry with specific types of contextual zero-suggest suggestions.
BUG=682420
Review-Url: https://codereview.chromium.org/2755503002
Cr-Commit-Position: refs/heads/master@{#461327}
Committed: https://chromium.googlesource.com/chromium/src/+/67d53ace23978af7d28d83fade1a09993079085a
Patch Set 1 #Patch Set 2 : #
Total comments: 14
Patch Set 3 : Use integer identifiers instead of human-readable types. #Patch Set 4 : Moved the logging of the specific type in the omnibox_event proto. #
Total comments: 2
Patch Set 5 : specific_type_identifier can be used outside of contextual suggestions. #
Total comments: 19
Patch Set 6 : Add unit test for parsing and use type specifier for search suggestions. #
Total comments: 12
Patch Set 7 : Small fixes. #
Total comments: 16
Patch Set 8 : Rename subtype field and fix test file #
Total comments: 6
Patch Set 9 : Fix consistency between comments and field names. #
Total comments: 9
Patch Set 10 : Small fixes and sync. #Messages
Total messages: 47 (19 generated)
The CQ bit was checked by gcomanici@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...
Description was changed from ========== Add histogram that records the type of contextual suggestions provided to users through zero suggest. Moreover, an additional histogram records the type of contextual suggestion that is selected by a user. BUG=682420 ========== to ========== Add histogram that records the type of contextual suggestions provided to users through zero suggest. Moreover, an additional histogram records the type of contextual suggestion that is selected by a user. BUG=682420 ==========
gcomanici@chromium.org changed reviewers: + mpearson@chromium.org
The CQ bit was checked by gcomanici@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...
I only glanced at this to look at the general design and have some questions. Once the design settles down and have a concrete proposal, we'll need to double-check with chrome-privacy@ about logging additional server-provided information (this should likely be a rubberstamp I think) and also the search leads (as mentioned in a comment). --mark https://codereview.chromium.org/2755503002/diff/20001/components/omnibox/brow... File components/omnibox/browser/autocomplete_match.h (right): https://codereview.chromium.org/2755503002/diff/20001/components/omnibox/brow... components/omnibox/browser/autocomplete_match.h:374: std::string specific_type; I'd curious why you've chosen to add a specific_type / sub_type here as opposed to expanding the list of valid types. For example, we have a variety of search query types that are based on the server suggestion: SEARCH_SUGGEST (general), SEARCH_SUGGEST_TAIL, SEARCH_SUGGEST_ENTITY, SEARCH_SUGGEST_PERSONALIZED, etc. I'm not sure whether that's a better approach than adding another field. I'd like us to know your reasoning for the choice you made. https://codereview.chromium.org/2755503002/diff/20001/components/omnibox/brow... File components/omnibox/browser/omnibox_field_trial.cc (right): https://codereview.chromium.org/2755503002/diff/20001/components/omnibox/brow... components/omnibox/browser/omnibox_field_trial.cc:805: "cooccurrence_pages", If we keep the human-readable names (and maybe if not), we will need to check with the search leads to confirm that they're comfortable with revealing the components of the suggestion system. (This is probably okay. They have pushed back on specific logging--more detailed than this--in the past, so it can't hurt to check.) https://codereview.chromium.org/2755503002/diff/20001/components/omnibox/brow... File components/omnibox/browser/search_suggestion_parser.cc (right): https://codereview.chromium.org/2755503002/diff/20001/components/omnibox/brow... components/omnibox/browser/search_suggestion_parser.cc:456: const base::ListValue* zero_suggest_subtypes = NULL; If we're getting stuff from the server, it should be as part of // 5th element: Optional key-value pairs from the Suggest server. not at yet another element. https://codereview.chromium.org/2755503002/diff/20001/components/omnibox/brow... components/omnibox/browser/search_suggestion_parser.cc:504: zero_suggest_subtypes->GetString(index, &zero_suggest_specific_type); I'm curious: why do you have the server send down human-readable strings as opposed simply an int / enum value? The strings will make it harder to change the logging in the future. I suppose there's something to be said for readability (both in the response and if you're displaying them within Chrome somewhere as a debug page), but the lack of extensibility worries me.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thank you, Mark! Please let me know if there is an easy way to start the discussion with chrome-privacy@. Is a design doc the starting point? https://codereview.chromium.org/2755503002/diff/20001/components/omnibox/brow... File components/omnibox/browser/autocomplete_match.h (right): https://codereview.chromium.org/2755503002/diff/20001/components/omnibox/brow... components/omnibox/browser/autocomplete_match.h:374: std::string specific_type; On 2017/03/14 20:17:49, Mark P wrote: > I'd curious why you've chosen to add a specific_type / sub_type here as opposed > to expanding the list of valid types. For example, we have a variety of search > query types that are based on the server suggestion: SEARCH_SUGGEST (general), > SEARCH_SUGGEST_TAIL, SEARCH_SUGGEST_ENTITY, SEARCH_SUGGEST_PERSONALIZED, etc. > > I'm not sure whether that's a better approach than adding another field. I'd > like us to know your reasoning for the choice you made. I was mostly concerned about adding unnecessary complexity to the enum of AutoComplete types. Since this change introduces 12 additional types which are of interest only to the team working on contextual suggestions, it made more sense to have a separate independent field. I'd rather not pollute histograms which are concerned with the high-level AC types. https://codereview.chromium.org/2755503002/diff/20001/components/omnibox/brow... File components/omnibox/browser/omnibox_field_trial.cc (right): https://codereview.chromium.org/2755503002/diff/20001/components/omnibox/brow... components/omnibox/browser/omnibox_field_trial.cc:805: "cooccurrence_pages", On 2017/03/14 20:17:49, Mark P wrote: > If we keep the human-readable names (and maybe if not), we will need to check > with the search leads to confirm that they're comfortable with revealing the > components of the suggestion system. (This is probably okay. They have pushed > back on specific logging--more detailed than this--in the past, so it can't hurt > to check.) Can you CC somebody that can do this? Or, should I instead do this in a separate thread? Note I could alternatively make a change on the server side to provide numerical ids instead of these names. https://codereview.chromium.org/2755503002/diff/20001/components/omnibox/brow... File components/omnibox/browser/search_suggestion_parser.cc (right): https://codereview.chromium.org/2755503002/diff/20001/components/omnibox/brow... components/omnibox/browser/search_suggestion_parser.cc:456: const base::ListValue* zero_suggest_subtypes = NULL; On 2017/03/14 20:17:50, Mark P wrote: > If we're getting stuff from the server, it should be as part of > // 5th element: Optional key-value pairs from the Suggest server. > not at yet another element. I agree. Although the server side puts this values in the 6th elements for now, this will change in the near future to your proposal. I will create a server side CL to fix this and change this code accordingly. https://codereview.chromium.org/2755503002/diff/20001/components/omnibox/brow... components/omnibox/browser/search_suggestion_parser.cc:504: zero_suggest_subtypes->GetString(index, &zero_suggest_specific_type); On 2017/03/14 20:17:49, Mark P wrote: > I'm curious: why do you have the server send down human-readable strings as > opposed simply an int / enum value? The strings will make it harder to change > the logging in the future. I suppose there's something to be said for > readability (both in the response and if you're displaying them within Chrome > somewhere as a debug page), but the lack of extensibility worries me. The short answer is that it makes it easy to use the exact same output for other analyses that are independent of Chrome code and where the human-readable strings are useful. But I do agree with you. I will take your advice to make a server side CL that addresses this issue.
https://codereview.chromium.org/2755503002/diff/20001/components/omnibox/brow... File components/omnibox/browser/autocomplete_match.h (right): https://codereview.chromium.org/2755503002/diff/20001/components/omnibox/brow... components/omnibox/browser/autocomplete_match.h:374: std::string specific_type; On 2017/03/14 20:42:55, gcomanici wrote: > On 2017/03/14 20:17:49, Mark P wrote: > > I'd curious why you've chosen to add a specific_type / sub_type here as > opposed > > to expanding the list of valid types. For example, we have a variety of > search > > query types that are based on the server suggestion: SEARCH_SUGGEST (general), > > SEARCH_SUGGEST_TAIL, SEARCH_SUGGEST_ENTITY, SEARCH_SUGGEST_PERSONALIZED, etc. > > > > I'm not sure whether that's a better approach than adding another field. I'd > > like us to know your reasoning for the choice you made. > > I was mostly concerned about adding unnecessary complexity to the enum of > AutoComplete types. Since this change introduces 12 additional types which are > of interest only to the team working on contextual suggestions, it made more > sense to have a separate independent field. I'd rather not pollute histograms > which are concerned with the high-level AC types. Makes sense to me. What about the decision not to store this in omnibox_event.proto? Do you think you'll never want to do analysis like looking at the list of types of zero suggest suggestions shown at the time a suggestion was used? Or look at interactions between, say, suggestion position and zero suggest specific type? https://codereview.chromium.org/2755503002/diff/20001/components/omnibox/brow... File components/omnibox/browser/omnibox_field_trial.cc (right): https://codereview.chromium.org/2755503002/diff/20001/components/omnibox/brow... components/omnibox/browser/omnibox_field_trial.cc:805: "cooccurrence_pages", On 2017/03/14 20:42:55, gcomanici wrote: > On 2017/03/14 20:17:49, Mark P wrote: > > If we keep the human-readable names (and maybe if not), we will need to check > > with the search leads to confirm that they're comfortable with revealing the > > components of the suggestion system. (This is probably okay. They have > pushed > > back on specific logging--more detailed than this--in the past, so it can't > hurt > > to check.) > > Can you CC somebody that can do this? Or, should I instead do this in a separate > thread? Note I could alternatively make a change on the server side to provide > numerical ids instead of these names. I can help do this once we have a concrete proposal for what you want to log and where. https://codereview.chromium.org/2755503002/diff/20001/components/omnibox/brow... File components/omnibox/browser/search_suggestion_parser.cc (right): https://codereview.chromium.org/2755503002/diff/20001/components/omnibox/brow... components/omnibox/browser/search_suggestion_parser.cc:504: zero_suggest_subtypes->GetString(index, &zero_suggest_specific_type); On 2017/03/14 20:42:55, gcomanici wrote: > On 2017/03/14 20:17:49, Mark P wrote: > > I'm curious: why do you have the server send down human-readable strings as > > opposed simply an int / enum value? The strings will make it harder to change > > the logging in the future. I suppose there's something to be said for > > readability (both in the response and if you're displaying them within Chrome > > somewhere as a debug page), but the lack of extensibility worries me. > > The short answer is that it makes it easy to use the exact same output for other > analyses that are independent of Chrome code and where the human-readable > strings are useful. But I do agree with you. I will take your advice to make a > server side CL that addresses this issue. The elephant in the room here is that regardless of int or string, once this is set, it cannot be easily changed because the UMA values are written to logs. Having the server send strings means that you cannot add new types without a chrome change. Having the server send ints means that the server needs to keep these ints constant for all eternity. For example, if the news int = 5, it must always be = 5, and if you remove the news type from the server, you cannot reuse that enum value. https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histo...
Also, let's wait on talking to chrome-privacy until this design is more established. --mark
https://codereview.chromium.org/2755503002/diff/20001/components/omnibox/brow... File components/omnibox/browser/autocomplete_match.h (right): https://codereview.chromium.org/2755503002/diff/20001/components/omnibox/brow... components/omnibox/browser/autocomplete_match.h:374: std::string specific_type; On 2017/03/14 21:04:09, Mark P wrote: > On 2017/03/14 20:42:55, gcomanici wrote: > > On 2017/03/14 20:17:49, Mark P wrote: > > > I'd curious why you've chosen to add a specific_type / sub_type here as > > opposed > > > to expanding the list of valid types. For example, we have a variety of > > search > > > query types that are based on the server suggestion: SEARCH_SUGGEST > (general), > > > SEARCH_SUGGEST_TAIL, SEARCH_SUGGEST_ENTITY, SEARCH_SUGGEST_PERSONALIZED, > etc. > > > > > > I'm not sure whether that's a better approach than adding another field. > I'd > > > like us to know your reasoning for the choice you made. > > > > I was mostly concerned about adding unnecessary complexity to the enum of > > AutoComplete types. Since this change introduces 12 additional types which are > > of interest only to the team working on contextual suggestions, it made more > > sense to have a separate independent field. I'd rather not pollute histograms > > which are concerned with the high-level AC types. > > Makes sense to me. > > What about the decision not to store this in omnibox_event.proto? Do you think > you'll never want to do analysis like looking at the list of types of zero > suggest suggestions shown at the time a suggestion was used? Or look at > interactions between, say, suggestion position and zero suggest specific type? Short answer: I'm scared of omnibox_event.proto. I looked a bit into it, but I don't have a good grasp of where the data saved in this proto is used for updating histograms. Indeed, further analyses will be of interest, but I believe that will happen when we get more traffic. With this CL, we are trying to go for quick gains in quality, which will allow hopefully allow us to get more usage and more in-depth analyses. I chose not to go for the proto in the interest of getting this specific histograms ready earlier, but if you believe using the proto is not as complex as I think it is, I could look into incorporating these signals in the proto. https://codereview.chromium.org/2755503002/diff/20001/components/omnibox/brow... File components/omnibox/browser/omnibox_field_trial.cc (right): https://codereview.chromium.org/2755503002/diff/20001/components/omnibox/brow... components/omnibox/browser/omnibox_field_trial.cc:805: "cooccurrence_pages", On 2017/03/14 21:04:09, Mark P wrote: > On 2017/03/14 20:42:55, gcomanici wrote: > > On 2017/03/14 20:17:49, Mark P wrote: > > > If we keep the human-readable names (and maybe if not), we will need to > check > > > with the search leads to confirm that they're comfortable with revealing the > > > components of the suggestion system. (This is probably okay. They have > > pushed > > > back on specific logging--more detailed than this--in the past, so it can't > > hurt > > > to check.) > > > > Can you CC somebody that can do this? Or, should I instead do this in a > separate > > thread? Note I could alternatively make a change on the server side to provide > > numerical ids instead of these names. > > I can help do this once we have a concrete proposal for what you want to log and > where. SG https://codereview.chromium.org/2755503002/diff/20001/components/omnibox/brow... File components/omnibox/browser/search_suggestion_parser.cc (right): https://codereview.chromium.org/2755503002/diff/20001/components/omnibox/brow... components/omnibox/browser/search_suggestion_parser.cc:504: zero_suggest_subtypes->GetString(index, &zero_suggest_specific_type); On 2017/03/14 21:04:09, Mark P wrote: > On 2017/03/14 20:42:55, gcomanici wrote: > > On 2017/03/14 20:17:49, Mark P wrote: > > > I'm curious: why do you have the server send down human-readable strings as > > > opposed simply an int / enum value? The strings will make it harder to > change > > > the logging in the future. I suppose there's something to be said for > > > readability (both in the response and if you're displaying them within > Chrome > > > somewhere as a debug page), but the lack of extensibility worries me. > > > > The short answer is that it makes it easy to use the exact same output for > other > > analyses that are independent of Chrome code and where the human-readable > > strings are useful. But I do agree with you. I will take your advice to make a > > server side CL that addresses this issue. > > The elephant in the room here is that regardless of int or string, once this is > set, it cannot be easily changed because the UMA values are written to logs. > > Having the server send strings means that you cannot add new types without a > chrome change. > > Having the server send ints means that the server needs to keep these ints > constant for all eternity. For example, if the news int = 5, it must always be > = 5, and if you remove the news type from the server, you cannot reuse that enum > value. > https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histo... I would go with the latter proposal (i.e. use ints). I think it will be easy to comply to these restrictions on the server side.
Sorry I got caught up with other things today. I'll review this again tomorrow, and also send two e-mails to check whether this is all okay. --mark
On 2017/03/15 23:27:25, Mark P wrote: > Sorry I got caught up with other things today. > > I'll review this again tomorrow, and also send two e-mails to check whether this > is all okay. > > --mark Hi Mark, I followed up on your suggestion to use the omnibox_event.proto to log the specific types of contextual suggestion. My intuition is that I would have to prepare another CL to generate historgram data from the logged events, but if you think some of it can be added in this CL, let me know. Thanks!
Haven't reviewed yet, but need to publish this comment so it's public. --mark https://codereview.chromium.org/2755503002/diff/60001/components/metrics/prot... File components/metrics/proto/omnibox_event.proto (right): https://codereview.chromium.org/2755503002/diff/60001/components/metrics/prot... components/metrics/proto/omnibox_event.proto:258: // suggestion provided through zero-suggest. Please use more generic language, because we want want to use this outside the context of contextual suggestions / zero-suggest. Perhaps, something like // Used to identify the specific source / type of suggestion for suggestions provided by the suggest server. For meaning of individual values, see the server-side enum.
On 2017/03/20 21:51:43, Mark P wrote: > Haven't reviewed yet, but need to publish this comment so it's public. > > --mark > > https://codereview.chromium.org/2755503002/diff/60001/components/metrics/prot... > File components/metrics/proto/omnibox_event.proto (right): > > https://codereview.chromium.org/2755503002/diff/60001/components/metrics/prot... > components/metrics/proto/omnibox_event.proto:258: // suggestion provided through > zero-suggest. > Please use more generic language, because we want want to use this outside the > context of contextual suggestions / zero-suggest. Perhaps, something like > // Used to identify the specific source / type of suggestion for suggestions > provided by the suggest server. For meaning of individual values, see the > server-side enum. Based on your comment about the the use of the type identifier outside of the context of contextual suggestions, I added a patch to make sure that this is logged for all requests, not just contextual zero suggest.
https://codereview.chromium.org/2755503002/diff/60001/components/metrics/prot... File components/metrics/proto/omnibox_event.proto (right): https://codereview.chromium.org/2755503002/diff/60001/components/metrics/prot... components/metrics/proto/omnibox_event.proto:258: // suggestion provided through zero-suggest. On 2017/03/20 21:51:43, Mark P wrote: > Please use more generic language, because we want want to use this outside the > context of contextual suggestions / zero-suggest. Perhaps, something like > // Used to identify the specific source / type of suggestion for suggestions > provided by the suggest server. For meaning of individual values, see the > server-side enum. Done.
Generally fine. :-) See comments for revisions below. As you saw I checked with chrome privacy, and they're okay with this change. I also checked with the suggest lead about revealing these ids. The lead said that was okay too. So we're good to go! Can you please take the change you made to omnibox_event.proto here and make the identical change in the internal google version of this file? Please send it to me for review. Generally we want the internal proto buffer to be updated before submitting the chrome code that could upload entries with that data. thanks, mark https://codereview.chromium.org/2755503002/diff/80001/chrome/browser/autocomp... File chrome/browser/autocomplete/search_provider_unittest.cc (right): https://codereview.chromium.org/2755503002/diff/80001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider_unittest.cc:1: // Copyright 2012 The Chromium Authors. All rights reserved. Please add a test here that we're parsing / extracting the specific type parameter correctly from the replies. (There might already be a test that this fits well into.) https://codereview.chromium.org/2755503002/diff/80001/components/omnibox/brow... File components/omnibox/browser/autocomplete_match.cc (right): https://codereview.chromium.org/2755503002/diff/80001/components/omnibox/brow... components/omnibox/browser/autocomplete_match.cc:166: specific_type_identifier = match.specific_type_identifier; Nice work. Most times I review changes to AutocompleteMatch, this block of code gets overlooked. :-) https://codereview.chromium.org/2755503002/diff/80001/components/omnibox/brow... File components/omnibox/browser/autocomplete_match.h (right): https://codereview.chromium.org/2755503002/diff/80001/components/omnibox/brow... components/omnibox/browser/autocomplete_match.h:372: // the server-side enum. nit: add Only used for logging. Please also explicit identify a value as being "unset". 0 is fine. Whatever you pick, you need to comment it. https://codereview.chromium.org/2755503002/diff/80001/components/omnibox/brow... File components/omnibox/browser/omnibox_metrics_provider.cc (right): https://codereview.chromium.org/2755503002/diff/80001/components/omnibox/brow... components/omnibox/browser/omnibox_metrics_provider.cc:163: suggestion->set_specific_type_identifier(i->specific_type_identifier); Please only set the specific type field in the proto buffer if the field is set in the AutocompleteMatch. https://codereview.chromium.org/2755503002/diff/80001/components/omnibox/brow... File components/omnibox/browser/search_suggestion_parser.cc (right): https://codereview.chromium.org/2755503002/diff/80001/components/omnibox/brow... components/omnibox/browser/search_suggestion_parser.cc:23: #include "components/omnibox/browser/omnibox_field_trial.h" Why do you need this? https://codereview.chromium.org/2755503002/diff/80001/components/omnibox/brow... components/omnibox/browser/search_suggestion_parser.cc:451: if (extras->GetList("specifictypeid", &specific_type_identifiers) && Let's use the "google:" as with everything else here. https://codereview.chromium.org/2755503002/diff/80001/components/omnibox/brow... components/omnibox/browser/search_suggestion_parser.cc:501: // add zero suggest subtype Correct comment (not just zero suggest; we're not calling them subtypes in the code; plus make into a sentence.) Also, per my request to make this more general, it shouldn't be in this block limited to navsuggestions. https://codereview.chromium.org/2755503002/diff/80001/components/omnibox/brow... File components/omnibox/browser/search_suggestion_parser.h (right): https://codereview.chromium.org/2755503002/diff/80001/components/omnibox/brow... components/omnibox/browser/search_suggestion_parser.h:205: int specific_type_identifier); Please hoist this logic up to SuggestResult. There's no reason it has to be particular to URL suggestions. https://codereview.chromium.org/2755503002/diff/80001/components/omnibox/brow... components/omnibox/browser/search_suggestion_parser.h:206: NavigationResult(const NavigationResult& nav); Curious: why do you need to add a copy constructor. https://codereview.chromium.org/2755503002/diff/80001/components/omnibox/brow... File components/omnibox/browser/zero_suggest_provider.cc (right): https://codereview.chromium.org/2755503002/diff/80001/components/omnibox/brow... components/omnibox/browser/zero_suggest_provider.cc:343: AutocompleteMatch ZeroSuggestProvider::NavigationToMatch( Per my request to make this more general, you'll also need to put analogous code in SearchProvider::NavigationToMatch() (so it works for as-you-type suggestions) and also BaseSearchProvider::CreateSearchSuggestion() (so it works for query suggestions).
Patchset #6 (id:100001) has been deleted
Thanks, Mark! Added a unit test for response parsing and I made the usage of the type identifier more general. https://codereview.chromium.org/2755503002/diff/80001/chrome/browser/autocomp... File chrome/browser/autocomplete/search_provider_unittest.cc (right): https://codereview.chromium.org/2755503002/diff/80001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider_unittest.cc:1: // Copyright 2012 The Chromium Authors. All rights reserved. On 2017/03/21 19:54:24, Mark P wrote: > Please add a test here that we're parsing / extracting the specific type > parameter correctly from the replies. (There might already be a test that this > fits well into.) Done. https://codereview.chromium.org/2755503002/diff/80001/components/omnibox/brow... File components/omnibox/browser/autocomplete_match.h (right): https://codereview.chromium.org/2755503002/diff/80001/components/omnibox/brow... components/omnibox/browser/autocomplete_match.h:372: // the server-side enum. On 2017/03/21 19:54:25, Mark P wrote: > nit: add > Only used for logging. > Please also explicit identify a value as being "unset". 0 is fine. Whatever > you pick, you need to comment it. Done. https://codereview.chromium.org/2755503002/diff/80001/components/omnibox/brow... File components/omnibox/browser/omnibox_metrics_provider.cc (right): https://codereview.chromium.org/2755503002/diff/80001/components/omnibox/brow... components/omnibox/browser/omnibox_metrics_provider.cc:163: suggestion->set_specific_type_identifier(i->specific_type_identifier); On 2017/03/21 19:54:25, Mark P wrote: > Please only set the specific type field in the proto buffer if the field is set > in the AutocompleteMatch. Done. https://codereview.chromium.org/2755503002/diff/80001/components/omnibox/brow... File components/omnibox/browser/search_suggestion_parser.cc (right): https://codereview.chromium.org/2755503002/diff/80001/components/omnibox/brow... components/omnibox/browser/search_suggestion_parser.cc:23: #include "components/omnibox/browser/omnibox_field_trial.h" On 2017/03/21 19:54:25, Mark P wrote: > Why do you need this? Removed. https://codereview.chromium.org/2755503002/diff/80001/components/omnibox/brow... components/omnibox/browser/search_suggestion_parser.cc:451: if (extras->GetList("specifictypeid", &specific_type_identifiers) && On 2017/03/21 19:54:25, Mark P wrote: > Let's use the "google:" as with everything else here. Done. https://codereview.chromium.org/2755503002/diff/80001/components/omnibox/brow... components/omnibox/browser/search_suggestion_parser.cc:501: // add zero suggest subtype On 2017/03/21 19:54:25, Mark P wrote: > Correct comment (not just zero suggest; we're not calling them subtypes in the > code; plus make into a sentence.) > > Also, per my request to make this more general, it shouldn't be in this block > limited to navsuggestions. I removed it, as the code is self explanatory. https://codereview.chromium.org/2755503002/diff/80001/components/omnibox/brow... File components/omnibox/browser/search_suggestion_parser.h (right): https://codereview.chromium.org/2755503002/diff/80001/components/omnibox/brow... components/omnibox/browser/search_suggestion_parser.h:205: int specific_type_identifier); On 2017/03/21 19:54:25, Mark P wrote: > Please hoist this logic up to SuggestResult. There's no reason it has to be > particular to URL suggestions. Done. https://codereview.chromium.org/2755503002/diff/80001/components/omnibox/brow... components/omnibox/browser/search_suggestion_parser.h:206: NavigationResult(const NavigationResult& nav); On 2017/03/21 19:54:25, Mark P wrote: > Curious: why do you need to add a copy constructor. I removed it. It was there because the compiler was complaining for some reason. Now that specific_type_identifier is in Result, I don't get that compilation warning anymore. https://codereview.chromium.org/2755503002/diff/80001/components/omnibox/brow... File components/omnibox/browser/zero_suggest_provider.cc (right): https://codereview.chromium.org/2755503002/diff/80001/components/omnibox/brow... components/omnibox/browser/zero_suggest_provider.cc:343: AutocompleteMatch ZeroSuggestProvider::NavigationToMatch( On 2017/03/21 19:54:25, Mark P wrote: > Per my request to make this more general, you'll also need to put analogous code > in > SearchProvider::NavigationToMatch() (so it works for as-you-type suggestions) > and also > BaseSearchProvider::CreateSearchSuggestion() (so it works for query > suggestions). > Done.
Only one substantial request in these comments. Thanks for your patience. :-) --mark https://codereview.chromium.org/2755503002/diff/120001/chrome/browser/autocom... File chrome/browser/autocomplete/search_provider_unittest.cc (right): https://codereview.chromium.org/2755503002/diff/120001/chrome/browser/autocom... chrome/browser/autocomplete/search_provider_unittest.cc:2520: const Match matches[2]; nit: expected_matches https://codereview.chromium.org/2755503002/diff/120001/chrome/browser/autocom... chrome/browser/autocomplete/search_provider_unittest.cc:2522: {// Check that the specific type is set to 0 when these values are not Note to self: I'm not reviewing the rest of this test given my comment on the parser. https://codereview.chromium.org/2755503002/diff/120001/components/omnibox/bro... File components/omnibox/browser/autocomplete_match.h (right): https://codereview.chromium.org/2755503002/diff/120001/components/omnibox/bro... components/omnibox/browser/autocomplete_match.h:372: // the server-side enum. add Only used for logging. https://codereview.chromium.org/2755503002/diff/120001/components/omnibox/bro... components/omnibox/browser/autocomplete_match.h:374: int specific_type_identifier = 0; nit: omit "=0" (chromium style I think) https://codereview.chromium.org/2755503002/diff/120001/components/omnibox/bro... File components/omnibox/browser/omnibox_metrics_provider.cc (right): https://codereview.chromium.org/2755503002/diff/120001/components/omnibox/bro... components/omnibox/browser/omnibox_metrics_provider.cc:163: if (i->specific_type_identifier > 0) { nit: chromium style is no { } necessary for single-line clauses after an "if". https://codereview.chromium.org/2755503002/diff/120001/components/omnibox/bro... File components/omnibox/browser/search_suggestion_parser.cc (right): https://codereview.chromium.org/2755503002/diff/120001/components/omnibox/bro... components/omnibox/browser/search_suggestion_parser.cc:452: if (extras->GetList("google:specifictypeid", &specific_type_identifiers) && I think it'd be more appropriate to have these identified in the "google:suggestdetail" array, not as a separate parallel array.
(I definitely want to get these details right because, with client code that will keep running indefinitely in the wild, it's really important to get logging and get messages passed over the wire correct. That's why I'm being extra careful.) --mark
Oh, and please revise the changelist description. It's entirely wrong now. :-P --mark
Description was changed from ========== Add histogram that records the type of contextual suggestions provided to users through zero suggest. Moreover, an additional histogram records the type of contextual suggestion that is selected by a user. BUG=682420 ========== to ========== Add a new entry to omnibox_event.proto to log specific types for the suggestions provided by omnibox. Moreover, in this CL, we implement the mechanics of populating this new entry with specific types of contextual zero-suggest suggestions. BUG=682420 ==========
Thanks Mark! Title and description are now changed. https://codereview.chromium.org/2755503002/diff/120001/chrome/browser/autocom... File chrome/browser/autocomplete/search_provider_unittest.cc (right): https://codereview.chromium.org/2755503002/diff/120001/chrome/browser/autocom... chrome/browser/autocomplete/search_provider_unittest.cc:2520: const Match matches[2]; On 2017/03/23 22:49:53, Mark P wrote: > nit: expected_matches Done. https://codereview.chromium.org/2755503002/diff/120001/components/omnibox/bro... File components/omnibox/browser/autocomplete_match.h (right): https://codereview.chromium.org/2755503002/diff/120001/components/omnibox/bro... components/omnibox/browser/autocomplete_match.h:372: // the server-side enum. On 2017/03/23 22:49:53, Mark P wrote: > add > Only used for logging. Done. https://codereview.chromium.org/2755503002/diff/120001/components/omnibox/bro... components/omnibox/browser/autocomplete_match.h:374: int specific_type_identifier = 0; On 2017/03/23 22:49:53, Mark P wrote: > nit: omit "=0" (chromium style I think) Done. https://codereview.chromium.org/2755503002/diff/120001/components/omnibox/bro... File components/omnibox/browser/omnibox_metrics_provider.cc (right): https://codereview.chromium.org/2755503002/diff/120001/components/omnibox/bro... components/omnibox/browser/omnibox_metrics_provider.cc:163: if (i->specific_type_identifier > 0) { On 2017/03/23 22:49:53, Mark P wrote: > nit: chromium style is no { } necessary for single-line clauses after an "if". Done. https://codereview.chromium.org/2755503002/diff/120001/components/omnibox/bro... File components/omnibox/browser/search_suggestion_parser.cc (right): https://codereview.chromium.org/2755503002/diff/120001/components/omnibox/bro... components/omnibox/browser/search_suggestion_parser.cc:452: if (extras->GetList("google:specifictypeid", &specific_type_identifiers) && On 2017/03/23 22:49:54, Mark P wrote: > I think it'd be more appropriate to have these identified in the > "google:suggestdetail" array, not as a separate parallel array. Are you sure? google:suggestdetail is a dictionary, so really all I would change is add the separate array inside google:suggestdetail and add another layer. IMO we don't really gain much.
Apologies that I didn't get to this on Friday. It looks good! Normally I'd approve this right now, but I'm not supposed to approve until the internal .proto change has been submitted. In any case, this changelist is really close. comments below. --mark https://codereview.chromium.org/2755503002/diff/120001/components/omnibox/bro... File components/omnibox/browser/search_suggestion_parser.cc (right): https://codereview.chromium.org/2755503002/diff/120001/components/omnibox/bro... components/omnibox/browser/search_suggestion_parser.cc:452: if (extras->GetList("google:specifictypeid", &specific_type_identifiers) && On 2017/03/24 16:24:59, gcomanici wrote: > On 2017/03/23 22:49:54, Mark P wrote: > > I think it'd be more appropriate to have these identified in the > > "google:suggestdetail" array, not as a separate parallel array. > > Are you sure? google:suggestdetail is a dictionary, so really all I would change > is add the separate array inside google:suggestdetail and add another layer. > IMO we don't really gain much. You're right; I was wrong. I misread the sample strings in your test file and thought it was structured differently. The way you chose to structure it is good. https://codereview.chromium.org/2755503002/diff/140001/chrome/browser/autocom... File chrome/browser/autocomplete/search_provider_unittest.cc (right): https://codereview.chromium.org/2755503002/diff/140001/chrome/browser/autocom... chrome/browser/autocomplete/search_provider_unittest.cc:2520: const Match expected_matches[2]; This needs a comment. Until I read the test code in detail, I thought this represented the order the expects should be returned in. But you're not testing that; you're just using this array more like a dictionary. https://codereview.chromium.org/2755503002/diff/140001/chrome/browser/autocom... chrome/browser/autocomplete/search_provider_unittest.cc:2522: {// Check that the specific type is set to 0 when these values are not Please follow the formatting of other tests in this file: // comment { blob } <- blank line (optional, depends on how dense the code looks) // comment { blob , { may other blob that tests similar thing that the comment applies to } <- blank line (optional) etc. (no { // comment continuation of blob https://codereview.chromium.org/2755503002/diff/140001/chrome/browser/autocom... chrome/browser/autocomplete/search_provider_unittest.cc:2523: // proviede in the response. nit: provided https://codereview.chromium.org/2755503002/diff/140001/chrome/browser/autocom... chrome/browser/autocomplete/search_provider_unittest.cc:2532: "\"NAVIGATION\"],\"google:specifictypeid\":[1, 3]}]", please make sure that the dictionary keys (google:specifictypeid:) are always the beginning of the line so they're easy to find. For example, {a: {b, c}, d: {e, f}} is hard to read. https://codereview.chromium.org/2755503002/diff/140001/chrome/browser/autocom... chrome/browser/autocomplete/search_provider_unittest.cc:2535: // suggestions is smaller than the number of typeid's provided. typeid's -> ids (to avoid the inappropriate apostrophe, and typeids looks odd) ditto below https://codereview.chromium.org/2755503002/diff/140001/chrome/browser/autocom... chrome/browser/autocomplete/search_provider_unittest.cc:2538: "{\"google:suggesttype\":[\"QUERY\"],\"google:specifictypeid\":[1, 2, " google:suggesttype should have two entries here, not one. https://codereview.chromium.org/2755503002/diff/140001/chrome/browser/autocom... chrome/browser/autocomplete/search_provider_unittest.cc:2542: // suggestions is largerthan the number of typeid's provided. larger than https://codereview.chromium.org/2755503002/diff/140001/chrome/browser/autocom... chrome/browser/autocomplete/search_provider_unittest.cc:2546: {{"bar foo", 0}, {"bar foz", 0}}}}; Please add a test for normal flow that also includes google:suggestrelevance in such as way as to reorder the suggestions from their default order. (i.e., the ids should stick with their suggestions)
Thanks Mark. I addressed your comments on the last patch and I changed the name of the field in the proto to match the server side naming. PTAL. https://codereview.chromium.org/2755503002/diff/140001/chrome/browser/autocom... File chrome/browser/autocomplete/search_provider_unittest.cc (right): https://codereview.chromium.org/2755503002/diff/140001/chrome/browser/autocom... chrome/browser/autocomplete/search_provider_unittest.cc:2520: const Match expected_matches[2]; On 2017/03/25 22:16:48, Mark P wrote: > This needs a comment. Until I read the test code in detail, I thought this > represented the order the expects should be returned in. But you're not testing > that; you're just using this array more like a dictionary. Done. https://codereview.chromium.org/2755503002/diff/140001/chrome/browser/autocom... chrome/browser/autocomplete/search_provider_unittest.cc:2522: {// Check that the specific type is set to 0 when these values are not On 2017/03/25 22:16:48, Mark P wrote: > Please follow the formatting of other tests in this file: > > // comment > { blob } > <- blank line (optional, depends on how dense the code looks) > // comment > { blob , > { may other blob that tests similar thing that the comment applies to } > <- blank line (optional) > > etc. > > (no > { // comment > continuation of blob Done. https://codereview.chromium.org/2755503002/diff/140001/chrome/browser/autocom... chrome/browser/autocomplete/search_provider_unittest.cc:2523: // proviede in the response. On 2017/03/25 22:16:48, Mark P wrote: > nit: provided Fixed. https://codereview.chromium.org/2755503002/diff/140001/chrome/browser/autocom... chrome/browser/autocomplete/search_provider_unittest.cc:2532: "\"NAVIGATION\"],\"google:specifictypeid\":[1, 3]}]", On 2017/03/25 22:16:48, Mark P wrote: > please make sure that the dictionary keys (google:specifictypeid:) are always > the beginning of the line so they're easy to find. For example, > {a: {b, > c}, d: {e, f}} > is hard to read. Done. https://codereview.chromium.org/2755503002/diff/140001/chrome/browser/autocom... chrome/browser/autocomplete/search_provider_unittest.cc:2535: // suggestions is smaller than the number of typeid's provided. On 2017/03/25 22:16:48, Mark P wrote: > typeid's > -> > ids > > (to avoid the inappropriate apostrophe, and typeids looks odd) > > ditto below Done. https://codereview.chromium.org/2755503002/diff/140001/chrome/browser/autocom... chrome/browser/autocomplete/search_provider_unittest.cc:2538: "{\"google:suggesttype\":[\"QUERY\"],\"google:specifictypeid\":[1, 2, " On 2017/03/25 22:16:48, Mark P wrote: > google:suggesttype should have two entries here, not one. Done. https://codereview.chromium.org/2755503002/diff/140001/chrome/browser/autocom... chrome/browser/autocomplete/search_provider_unittest.cc:2542: // suggestions is largerthan the number of typeid's provided. On 2017/03/25 22:16:48, Mark P wrote: > larger than Fixed. https://codereview.chromium.org/2755503002/diff/140001/chrome/browser/autocom... chrome/browser/autocomplete/search_provider_unittest.cc:2546: {{"bar foo", 0}, {"bar foz", 0}}}}; On 2017/03/25 22:16:48, Mark P wrote: > Please add a test for normal flow that also includes google:suggestrelevance in > such as way as to reorder the suggestions from their default order. (i.e., the > ids should stick with their suggestions) Done.
https://codereview.chromium.org/2755503002/diff/160001/components/metrics/pro... File components/metrics/proto/omnibox_event.proto (right): https://codereview.chromium.org/2755503002/diff/160001/components/metrics/pro... components/metrics/proto/omnibox_event.proto:1: // Copyright 2014 The Chromium Authors. All rights reserved. Can you please make the omnibox_event.proto changes be as close as possible to the internal copy, sans confidential information (if any)? https://codereview.chromium.org/2755503002/diff/160001/components/omnibox/bro... File components/omnibox/browser/autocomplete_match.h (right): https://codereview.chromium.org/2755503002/diff/160001/components/omnibox/bro... components/omnibox/browser/autocomplete_match.h:370: // Used to identify the specific source / type of suggestion for suggestions This comment should be updated to reflect the wording (in the multiple places changed) in omnibox_event.proto. https://codereview.chromium.org/2755503002/diff/160001/components/omnibox/bro... File components/omnibox/browser/search_suggestion_parser.cc (right): https://codereview.chromium.org/2755503002/diff/160001/components/omnibox/bro... components/omnibox/browser/search_suggestion_parser.cc:452: if (extras->GetList("google:specifictypeid", &result_subtype_identifiers) && Please choose a new string name that corresponds with the code.
By the way, I'm glad you were busy this week, because so was I. :-) Hence, I don't feel so bad about not getting to this review sooner this week. cheers, mark
On 2017/03/31 17:37:33, Mark P wrote: > By the way, I'm glad you were busy this week, because so was I. :-) Hence, I > don't feel so bad about not getting to this review sooner this week. > > cheers, > mark :-) It was busy but exciting week on my end, indeed. Based on your comments, I noticed that I was not too careful in maintaining consistency between variable names and comments. Fixed in this CL. PTAL. Thanks! Gheorghe
https://codereview.chromium.org/2755503002/diff/160001/components/metrics/pro... File components/metrics/proto/omnibox_event.proto (right): https://codereview.chromium.org/2755503002/diff/160001/components/metrics/pro... components/metrics/proto/omnibox_event.proto:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2017/03/31 17:36:42, Mark P wrote: > Can you please make the omnibox_event.proto changes be as close as possible to > the internal copy, sans confidential information (if any)? Done. https://codereview.chromium.org/2755503002/diff/160001/components/omnibox/bro... File components/omnibox/browser/autocomplete_match.h (right): https://codereview.chromium.org/2755503002/diff/160001/components/omnibox/bro... components/omnibox/browser/autocomplete_match.h:370: // Used to identify the specific source / type of suggestion for suggestions On 2017/03/31 17:36:42, Mark P wrote: > This comment should be updated to reflect the wording (in the multiple places > changed) in omnibox_event.proto. Done. https://codereview.chromium.org/2755503002/diff/160001/components/omnibox/bro... File components/omnibox/browser/search_suggestion_parser.cc (right): https://codereview.chromium.org/2755503002/diff/160001/components/omnibox/bro... components/omnibox/browser/search_suggestion_parser.cc:452: if (extras->GetList("google:specifictypeid", &result_subtype_identifiers) && On 2017/03/31 17:36:43, Mark P wrote: > Please choose a new string name that corresponds with the code. Done.
lgtm modulo nits Thanks for your attention to detail. --mark https://codereview.chromium.org/2755503002/diff/180001/components/metrics/pro... File components/metrics/proto/omnibox_event.proto (right): https://codereview.chromium.org/2755503002/diff/180001/components/metrics/pro... components/metrics/proto/omnibox_event.proto:202: // the experimental servicei (note that not all two separate typos in this line https://codereview.chromium.org/2755503002/diff/180001/components/metrics/pro... components/metrics/proto/omnibox_event.proto:204: // zero-suggest suggestions). replace: are contextual zero-suggest suggestions -> come from the experimental service https://codereview.chromium.org/2755503002/diff/180001/components/omnibox/bro... File components/omnibox/browser/autocomplete_match.h (right): https://codereview.chromium.org/2755503002/diff/180001/components/omnibox/bro... components/omnibox/browser/autocomplete_match.h:373: // type. Contact service providers for more details. To correct my earlier suggestion, perhaps instead of the last two sentences, say, See |result_subtype_identifier| in omnibox.event.proto for more details. https://codereview.chromium.org/2755503002/diff/180001/components/omnibox/bro... File components/omnibox/browser/search_suggestion_parser.h (right): https://codereview.chromium.org/2755503002/diff/180001/components/omnibox/bro... components/omnibox/browser/search_suggestion_parser.h:94: // suggest server. The meaning of individual values is determined by the ditto comment in other file
The CQ bit was checked by gcomanici@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...
Thanks, Mark! https://codereview.chromium.org/2755503002/diff/180001/components/metrics/pro... File components/metrics/proto/omnibox_event.proto (right): https://codereview.chromium.org/2755503002/diff/180001/components/metrics/pro... components/metrics/proto/omnibox_event.proto:202: // the experimental servicei (note that not all On 2017/03/31 20:50:25, Mark P wrote: > two separate typos in this line Fixed. https://codereview.chromium.org/2755503002/diff/180001/components/metrics/pro... components/metrics/proto/omnibox_event.proto:204: // zero-suggest suggestions). On 2017/03/31 20:50:25, Mark P wrote: > replace: > are contextual zero-suggest suggestions > -> > come from the experimental service Done. https://codereview.chromium.org/2755503002/diff/180001/components/omnibox/bro... File components/omnibox/browser/autocomplete_match.h (right): https://codereview.chromium.org/2755503002/diff/180001/components/omnibox/bro... components/omnibox/browser/autocomplete_match.h:373: // type. Contact service providers for more details. On 2017/03/31 20:50:25, Mark P wrote: > To correct my earlier suggestion, perhaps instead of the last two sentences, > say, > See |result_subtype_identifier| in omnibox.event.proto for more details. Done. https://codereview.chromium.org/2755503002/diff/180001/components/omnibox/bro... File components/omnibox/browser/search_suggestion_parser.h (right): https://codereview.chromium.org/2755503002/diff/180001/components/omnibox/bro... components/omnibox/browser/search_suggestion_parser.h:94: // suggest server. The meaning of individual values is determined by the On 2017/03/31 20:50:25, Mark P wrote: > ditto comment in other file Done.
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 gcomanici@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/2755503002/#ps200001 (title: "Small fixes and sync.")
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": 200001, "attempt_start_ts": 1491066161241710,
"parent_rev": "761ab5e1b8a8e5d24200753b340250416c9b6a61", "commit_rev":
"67d53ace23978af7d28d83fade1a09993079085a"}
Message was sent while issue was closed.
Description was changed from ========== Add a new entry to omnibox_event.proto to log specific types for the suggestions provided by omnibox. Moreover, in this CL, we implement the mechanics of populating this new entry with specific types of contextual zero-suggest suggestions. BUG=682420 ========== to ========== Add a new entry to omnibox_event.proto to log specific types for the suggestions provided by omnibox. Moreover, in this CL, we implement the mechanics of populating this new entry with specific types of contextual zero-suggest suggestions. BUG=682420 Review-Url: https://codereview.chromium.org/2755503002 Cr-Commit-Position: refs/heads/master@{#461327} Committed: https://chromium.googlesource.com/chromium/src/+/67d53ace23978af7d28d83fade1a... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:200001) as https://chromium.googlesource.com/chromium/src/+/67d53ace23978af7d28d83fade1a...
Message was sent while issue was closed.
https://codereview.chromium.org/2755503002/diff/180001/components/omnibox/bro... File components/omnibox/browser/autocomplete_match.h (right): https://codereview.chromium.org/2755503002/diff/180001/components/omnibox/bro... components/omnibox/browser/autocomplete_match.h:373: // type. Contact service providers for more details. On 2017/04/01 16:01:52, gcomanici wrote: > On 2017/03/31 20:50:25, Mark P wrote: > > To correct my earlier suggestion, perhaps instead of the last two sentences, > > say, > > See |result_subtype_identifier| in omnibox.event.proto for more details. > > Done. In a trivial follow-up changelist, can you please revise this to say result_subtype_identifier as I suggested? You mentioned result_type_identifier, which is not the name of the field in omnibox_event.proto. ditto other file
Message was sent while issue was closed.
Patchset #11 (id:220001) has been deleted |
