Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2203993002:
Add a Physical Web omnibox autocomplete provider (Closed)
|
Created:
4 years, 4 months ago by mattreynolds Modified:
4 years, 3 months ago CC:
chromium-reviews, Dirk Pranke, sdefresne+watchlist_chromium.org, droger+watchlist_chromium.org, asvitkine+watch_chromium.org, blundell+watchlist_chromium.org, mmocny Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd a Physical Web omnibox autocomplete provider
When nearby devices are broadcasting Physical Web URLs,
Chrome will display an autocomplete suggestion in the
omnibox. The suggestion item will link to a WebUI page
containing the full list of nearby URLs.
BUG=630769
Committed: https://crrev.com/5afc0169014d1f9560d58799615bc04429d67df0
Cr-Commit-Position: refs/heads/master@{#413262}
Patch Set 1 #Patch Set 2 : data source changes #
Total comments: 43
Patch Set 3 : changes for mpearson@ #Patch Set 4 : make overflow item localizable #
Total comments: 21
Patch Set 5 : improved comments, AppendOverflowItem #
Total comments: 15
Patch Set 6 : add unit tests #
Total comments: 29
Patch Set 7 : unit test fixes #
Total comments: 4
Patch Set 8 : almost done #
Total comments: 11
Patch Set 9 : remove GYP changes, set contents for overflow item #
Total comments: 2
Patch Set 10 : fix nit #Patch Set 11 : oops, wrong CL #
Messages
Total messages: 31 (5 generated)
mattreynolds@chromium.org changed reviewers: + mpearson@chromium.org
Hi Mark, I'm trying to put together a Physical Web autocomplete provider that creates a match item when there are nearby devices broadcasting URLs. Could you take a look at what I have so far?
On 2016/08/02 22:06:35, mattreynolds wrote: > Hi Mark, > > I'm trying to put together a Physical Web autocomplete provider that creates a > match item when there are nearby devices broadcasting URLs. Could you take a > look at what I have so far? Apologies for not getting to it this week. The beginning of next week for sure! --mark
Generally looks fine. I have some minor concerns about how you get the physical web information into the provider and what your choice of match types. Certainly no red flags anywhere in this patch. Lots of comments below. cheers, mark https://codereview.chromium.org/2203993002/diff/20001/components/metrics/prot... File components/metrics/proto/omnibox_event.proto (right): https://codereview.chromium.org/2203993002/diff/20001/components/metrics/prot... components/metrics/proto/omnibox_event.proto:171: PHYSICAL_WEB = 15; // URLs broadcast by nearby devices (iOS only). URLs -> suggestions (because this refers to the *provider* and I imagine the provider might want to suggest other things at some point, e.g., if a broadcast URL looks like a search query, it might display the search query as a suggestion) https://codereview.chromium.org/2203993002/diff/20001/components/metrics/prot... components/metrics/proto/omnibox_event.proto:171: PHYSICAL_WEB = 15; // URLs broadcast by nearby devices (iOS only). Also, before submitting this external changelist, please make the corresponding change to the proto in the google internal codebase. https://codereview.chromium.org/2203993002/diff/20001/components/omnibox/brow... File components/omnibox/browser/autocomplete_match.cc (right): https://codereview.chromium.org/2203993002/diff/20001/components/omnibox/brow... components/omnibox/browser/autocomplete_match.cc:194: }; I think you need to add the icon here too, otherwise the array size is wrong. (Probably hit the assert on like 221.) https://codereview.chromium.org/2203993002/diff/20001/components/omnibox/brow... File components/omnibox/browser/autocomplete_match_type.h (right): https://codereview.chromium.org/2203993002/diff/20001/components/omnibox/brow... components/omnibox/browser/autocomplete_match_type.h:54: PHYSICAL_WEB = 21, // A Physical Web neabry URL with nit: nearby -> nearby https://codereview.chromium.org/2203993002/diff/20001/components/omnibox/brow... components/omnibox/browser/autocomplete_match_type.h:55: // metadata. does "with metadata" add anything here? https://codereview.chromium.org/2203993002/diff/20001/components/omnibox/brow... components/omnibox/browser/autocomplete_match_type.h:56: NUM_TYPES, The overflow item is qualitatively different than a physical web URL. I think it should probably be a separate match type, not grouped into the same bucket. (comment also applies to omnibox_event.proto) https://codereview.chromium.org/2203993002/diff/20001/components/omnibox/brow... File components/omnibox/browser/autocomplete_provider_client.h (right): https://codereview.chromium.org/2203993002/diff/20001/components/omnibox/brow... components/omnibox/browser/autocomplete_provider_client.h:65: virtual PhysicalWebDataSource* GetPhysicalWebDataSource() = 0; I think there's a lot of stuff here already but I must ask: is there a way you can get the physical web data source to the provider without adding more here. For instance, can you simply pass it in as part of the constructor in autocomplete_controller, no need to muck with this generic AutocompleteProviderClient? https://codereview.chromium.org/2203993002/diff/20001/components/omnibox/brow... File components/omnibox/browser/physical_web_provider.cc (right): https://codereview.chromium.org/2203993002/diff/20001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.cc:18: static const int kPhysicalWebMaxMatches = 1; side comment: later you might want to put this under experimental control https://codereview.chromium.org/2203993002/diff/20001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.cc:18: static const int kPhysicalWebMaxMatches = 1; If this is set to 1, does that mean that if there's a physical web device nearby, no URL is every displayed, only the overflow item? https://codereview.chromium.org/2203993002/diff/20001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.cc:20: // Relevance score of a Physical Web URL autocomplete match. Please comment, justifying why this is intentionally higher than the clipboard suggestions. Or maybe it lower, justifying why it's lower. :-) https://codereview.chromium.org/2203993002/diff/20001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.cc:23: // Relevance score of the overflow item. Please explain what an "overflow" item is or point to an explanation. https://codereview.chromium.org/2203993002/diff/20001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.cc:31: //static nit: space between "//" and "static" https://codereview.chromium.org/2203993002/diff/20001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.cc:50: } Why don't you simply return early if you have no data? https://codereview.chromium.org/2203993002/diff/20001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.cc:72: int metadata_count = metadata_list->GetSize(); nit: const https://codereview.chromium.org/2203993002/diff/20001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.cc:72: int metadata_count = metadata_list->GetSize(); general comment: most of your ints throughout should probably be size_ts. https://codereview.chromium.org/2203993002/diff/20001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.cc:76: ? metadata_count : kPhysicalWebMaxMatches - 1; nit: operators go at end of lines, not beginning of following lines https://codereview.chromium.org/2203993002/diff/20001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.cc:78: for (int i = 0; i < metadata_count && (int)matches_.size() < max_rows; ++i) { Don't all these Value getters have return values (e.g., success)? https://codereview.chromium.org/2203993002/diff/20001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.cc:94: // Physical Web results should always omit protocols and never appear bold I am skeptical of this omitting protocols. This philosophy differs from the default omnibox where we always display secure protocols explicitly. I don't see why this situation is different. https://codereview.chromium.org/2203993002/diff/20001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.cc:119: int previous_match_count = matches_.size(); nit: const https://codereview.chromium.org/2203993002/diff/20001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.cc:122: GURL url(kChromeUIPhysicalWebURL); only used in one place; no need to declare a variable for it. https://codereview.chromium.org/2203993002/diff/20001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.cc:138: match.allowed_to_be_default_match = matches_.empty(); I think the overflow item should never be allowed to be the default, and it should even have a chance to be if the earlier code works right. https://codereview.chromium.org/2203993002/diff/20001/components/omnibox/brow... File components/omnibox/browser/physical_web_provider.h (right): https://codereview.chromium.org/2203993002/diff/20001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.h:30: void AppendOverflowItemIfNecessary(base::ListValue* metadata_list); Comment above two functions.
https://codereview.chromium.org/2203993002/diff/20001/components/metrics/prot... File components/metrics/proto/omnibox_event.proto (right): https://codereview.chromium.org/2203993002/diff/20001/components/metrics/prot... components/metrics/proto/omnibox_event.proto:171: PHYSICAL_WEB = 15; // URLs broadcast by nearby devices (iOS only). On 2016/08/08 20:41:36, Mark P wrote: > Also, before submitting this external changelist, please make the corresponding > change to the proto in the google internal codebase. Updated the comment, and thanks for the heads-up on the proto. Created cl/129789244 for the internal change. https://codereview.chromium.org/2203993002/diff/20001/components/omnibox/brow... File components/omnibox/browser/autocomplete_match.cc (right): https://codereview.chromium.org/2203993002/diff/20001/components/omnibox/brow... components/omnibox/browser/autocomplete_match.cc:194: }; On 2016/08/08 20:41:37, Mark P wrote: > I think you need to add the icon here too, otherwise the array size is wrong. > (Probably hit the assert on like 221.) Done. Also added to the list in TypeToVectorIcon. https://codereview.chromium.org/2203993002/diff/20001/components/omnibox/brow... File components/omnibox/browser/autocomplete_match_type.h (right): https://codereview.chromium.org/2203993002/diff/20001/components/omnibox/brow... components/omnibox/browser/autocomplete_match_type.h:54: PHYSICAL_WEB = 21, // A Physical Web neabry URL with On 2016/08/08 20:41:37, Mark P wrote: > nit: nearby -> nearby Done. https://codereview.chromium.org/2203993002/diff/20001/components/omnibox/brow... components/omnibox/browser/autocomplete_match_type.h:55: // metadata. On 2016/08/08 20:41:37, Mark P wrote: > does "with metadata" add anything here? Not really, removed https://codereview.chromium.org/2203993002/diff/20001/components/omnibox/brow... components/omnibox/browser/autocomplete_match_type.h:56: NUM_TYPES, On 2016/08/08 20:41:37, Mark P wrote: > The overflow item is qualitatively different than a physical web URL. I think > it should probably be a separate match type, not grouped into the same bucket. > (comment also applies to omnibox_event.proto) Makes sense, done. https://codereview.chromium.org/2203993002/diff/20001/components/omnibox/brow... File components/omnibox/browser/autocomplete_provider_client.h (right): https://codereview.chromium.org/2203993002/diff/20001/components/omnibox/brow... components/omnibox/browser/autocomplete_provider_client.h:65: virtual PhysicalWebDataSource* GetPhysicalWebDataSource() = 0; On 2016/08/08 20:41:37, Mark P wrote: > I think there's a lot of stuff here already but I must ask: is there a way you > can get the physical web data source to the provider without adding more here. > For instance, can you simply pass it in as part of the constructor in > autocomplete_controller, no need to muck with this generic > AutocompleteProviderClient? The issue is that the PhysicalWebDataSource instance is owned by ApplicationContext, which is iOS-specific and can't be accessed directly from //chrome/browser (or //components/omnibox). AutocompleteProviderClientImpl appears to be where we're embedding other iOS-specific implementations of components needed by autocomplete providers which is why I added it there. When we bring the feature to other platforms we'll attach the data source instance to BrowserProcess and make it accessible to the omnibox component through ChromeAutocompleteProviderClient::GetPhysicalWebDataSource(). Re: AutocompleteController, it is created by OmniboxController, which is in turn created by OmniboxEditModel, OmniboxView, and finally OmniboxViewIOS. OmniboxViewIOS appears to be the first point in the chain where we can add iOS-specific embeddings, but doesn't seem like a good fit for the data source method. WDYT? https://codereview.chromium.org/2203993002/diff/20001/components/omnibox/brow... File components/omnibox/browser/physical_web_provider.cc (right): https://codereview.chromium.org/2203993002/diff/20001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.cc:18: static const int kPhysicalWebMaxMatches = 1; On 2016/08/08 20:41:38, Mark P wrote: > If this is set to 1, does that mean that if there's a physical web device > nearby, no URL is every displayed, only the overflow item? With kPhysicalWebMaxMatches == 1, there should be at most one suggestion created by this provider. If there is a single nearby URL, then the suggestion will be for that URL. If there are multiple nearby URLs, the suggestion will be the overflow item which navigates to the WebUI when tapped. Re: side comment, we do plan to create an experiment for this, but currently we only have permission to add one item. https://codereview.chromium.org/2203993002/diff/20001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.cc:20: // Relevance score of a Physical Web URL autocomplete match. On 2016/08/08 20:41:37, Mark P wrote: > Please comment, justifying why this is intentionally higher than the clipboard > suggestions. > Or maybe it lower, justifying why it's lower. :-) The current value was chosen arbitrarily, I think we should lower it below the clipboard URL relevance. ClipboardURLProvider says: "The relevance is 800 to beat ZeroSuggest results." ZeroSuggestProvider sets its default relevance as 100 but the actual relevance scores come from the server. Do you know what their range is? https://codereview.chromium.org/2203993002/diff/20001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.cc:23: // Relevance score of the overflow item. On 2016/08/08 20:41:38, Mark P wrote: > Please explain what an "overflow" item is or point to an explanation. I added some explanation to the comment above kPhysicalWebMaxMatches https://codereview.chromium.org/2203993002/diff/20001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.cc:31: //static On 2016/08/08 20:41:37, Mark P wrote: > nit: space between "//" and "static" Done. https://codereview.chromium.org/2203993002/diff/20001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.cc:50: } On 2016/08/08 20:41:37, Mark P wrote: > Why don't you simply return early if you have no data? Done. https://codereview.chromium.org/2203993002/diff/20001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.cc:72: int metadata_count = metadata_list->GetSize(); On 2016/08/08 20:41:37, Mark P wrote: > general comment: most of your ints throughout should probably be size_ts. Done x2 https://codereview.chromium.org/2203993002/diff/20001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.cc:76: ? metadata_count : kPhysicalWebMaxMatches - 1; On 2016/08/08 20:41:37, Mark P wrote: > nit: operators go at end of lines, not beginning of following lines Done. https://codereview.chromium.org/2203993002/diff/20001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.cc:78: for (int i = 0; i < metadata_count && (int)matches_.size() < max_rows; ++i) { On 2016/08/08 20:41:37, Mark P wrote: > Don't all these Value getters have return values (e.g., success)? Added validation. I think in general it's fine to not validate here since the data comes from an internal data source that performs its own validation, but it couldn't hurt either. https://codereview.chromium.org/2203993002/diff/20001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.cc:94: // Physical Web results should always omit protocols and never appear bold On 2016/08/08 20:41:37, Mark P wrote: > I am skeptical of this omitting protocols. This philosophy differs from the > default omnibox where we always display secure protocols explicitly. I don't > see why this situation is different. Changed to kFormatUrlOmitHttp. https://codereview.chromium.org/2203993002/diff/20001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.cc:119: int previous_match_count = matches_.size(); On 2016/08/08 20:41:37, Mark P wrote: > nit: const Done. https://codereview.chromium.org/2203993002/diff/20001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.cc:122: GURL url(kChromeUIPhysicalWebURL); On 2016/08/08 20:41:37, Mark P wrote: > only used in one place; no need to declare a variable for it. Done. https://codereview.chromium.org/2203993002/diff/20001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.cc:138: match.allowed_to_be_default_match = matches_.empty(); On 2016/08/08 20:41:37, Mark P wrote: > I think the overflow item should never be allowed to be the default, and it > should even have a chance to be if the earlier code works right. In the case where we only have one "slot" to fill (kPhysicalWebMaxMatches == 1), the overflow item will often be the only match item from this provider. I think it's appropriate for it to be the default match in that case. https://codereview.chromium.org/2203993002/diff/20001/components/omnibox/brow... File components/omnibox/browser/physical_web_provider.h (right): https://codereview.chromium.org/2203993002/diff/20001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.h:30: void AppendOverflowItemIfNecessary(base::ListValue* metadata_list); On 2016/08/08 20:41:38, Mark P wrote: > Comment above two functions. Done.
Generally it's in a pretty good state now. --mark https://codereview.chromium.org/2203993002/diff/20001/components/omnibox/brow... File components/omnibox/browser/autocomplete_provider_client.h (right): https://codereview.chromium.org/2203993002/diff/20001/components/omnibox/brow... components/omnibox/browser/autocomplete_provider_client.h:65: virtual PhysicalWebDataSource* GetPhysicalWebDataSource() = 0; On 2016/08/09 21:42:02, mattreynolds wrote: > On 2016/08/08 20:41:37, Mark P wrote: > > I think there's a lot of stuff here already but I must ask: is there a way you > > can get the physical web data source to the provider without adding more here. > > > For instance, can you simply pass it in as part of the constructor in > > autocomplete_controller, no need to muck with this generic > > AutocompleteProviderClient? > > The issue is that the PhysicalWebDataSource instance is owned by > ApplicationContext, which is iOS-specific and can't be accessed directly from > //chrome/browser (or //components/omnibox). AutocompleteProviderClientImpl > appears to be where we're embedding other iOS-specific implementations of > components needed by autocomplete providers which is why I added it there. Makes sense to me. I hadn't rememberad all those iOS overrides in the impl file. > When > we bring the feature to other platforms we'll attach the data source instance to > BrowserProcess and make it accessible to the omnibox component through > ChromeAutocompleteProviderClient::GetPhysicalWebDataSource(). > > Re: AutocompleteController, it is created by OmniboxController, which is in turn > created by OmniboxEditModel, OmniboxView, and finally OmniboxViewIOS. > OmniboxViewIOS appears to be the first point in the chain where we can add > iOS-specific embeddings, but doesn't seem like a good fit for the data source > method. WDYT? I agree, passing something the physical web data source down from OmniboxView seems wrong. https://codereview.chromium.org/2203993002/diff/20001/components/omnibox/brow... File components/omnibox/browser/physical_web_provider.cc (right): https://codereview.chromium.org/2203993002/diff/20001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.cc:138: match.allowed_to_be_default_match = matches_.empty(); On 2016/08/09 21:42:03, mattreynolds wrote: > On 2016/08/08 20:41:37, Mark P wrote: > > I think the overflow item should never be allowed to be the default, and it > > should even have a chance to be if the earlier code works right. > > In the case where we only have one "slot" to fill (kPhysicalWebMaxMatches == 1), > the overflow item will often be the only match item from this provider. I think > it's appropriate for it to be the default match in that case. Acknowledged. https://codereview.chromium.org/2203993002/diff/60001/components/omnibox/brow... File components/omnibox/browser/physical_web_provider.cc (right): https://codereview.chromium.org/2203993002/diff/60001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.cc:20: // exceeds this limit, then an overflow item is created. Tapping the overflow nit: omit "then" https://codereview.chromium.org/2203993002/diff/60001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.cc:22: // is counted as a match result for the purposes of the match limit. Can you put this example of yours in the comment? With kPhysicalWebMaxMatches == 1, there should be at most one suggestion created by this provider. If there is a single nearby URL, then the suggestion will be for that URL. If there are multiple nearby URLs, the suggestion will be the overflow item which navigates to the WebUI when tapped. https://codereview.chromium.org/2203993002/diff/60001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.cc:23: static const int kPhysicalWebMaxMatches = 1; Can you put a DCHECK assertion somewhere that this is less than general provider limit of kMaxMatches? https://codereview.chromium.org/2203993002/diff/60001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.cc:26: static const int kPhysicalWebUrlRelevance = 700; On 2016/08/09 21:42:03, mattreynolds wrote: > On 2016/08/08 20:41:37, Mark P wrote: > > Please comment, justifying why this is intentionally higher than the clipboard > > suggestions. > > Or maybe it lower, justifying why it's lower. :-) > > The current value was chosen arbitrarily, I think we should lower it below the > clipboard URL relevance. > > ClipboardURLProvider says: "The relevance is 800 to beat ZeroSuggest results." > > ZeroSuggestProvider sets its default relevance as 100 but the actual relevance > scores come from the server. Do you know what their range is? I wrote: I think it's rarely above 600. I suggest using 700 and not worrying and more. And then I saw you chose 700. :-) Nonetheless, please comment that this is intentionally lower than clipboard suggestions. https://codereview.chromium.org/2203993002/diff/60001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.cc:99: // Physical Web results should never omit protocols and never appear bold On 2016/08/09 21:42:03, mattreynolds wrote: > On 2016/08/08 20:41:37, Mark P wrote: > > I am skeptical of this omitting protocols. This philosophy differs from the > > default omnibox where we always display secure protocols explicitly. I don't > > see why this situation is different. > > Changed to kFormatUrlOmitHttp. Please revise this comment; it's currently wrong. https://codereview.chromium.org/2203993002/diff/60001/components/omnibox/brow... File components/omnibox/browser/physical_web_provider.h (right): https://codereview.chromium.org/2203993002/diff/60001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.h:30: // matches allowed by this provider. If the total number of nearby URLs First sentence: this should mention where the matches are being added and where the URLs come from. Are matches being encoded and stored in |metadata_list|? Somewhere else? And where is the info on what matches exist coming from? Something injected? Something in |metadata_list|. https://codereview.chromium.org/2203993002/diff/60001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.h:30: // matches allowed by this provider. If the total number of nearby URLs nit: omit "by this provider"; it doesn't add anything useful in this context. https://codereview.chromium.org/2203993002/diff/60001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.h:31: // exceeds this limit, one match is reserved for an overflow item. nit: "one match" -> "one slot" (I assume you don't fill in this extra slot with a match.) https://codereview.chromium.org/2203993002/diff/60001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.h:35: // maximum number of matches allowed by this provider. Selecting the overflow ditto same concerns about unclear what variables are being changed. Also, in general would it make sense--and make the code simpler--to make this function unconditional (always add something) and simply be smart about when to call it? https://codereview.chromium.org/2203993002/diff/60001/components/omnibox_stri... File components/omnibox_strings.grdp (right): https://codereview.chromium.org/2203993002/diff/60001/components/omnibox_stri... components/omnibox_strings.grdp:22: {URL_COUNT, plural, =1 {1 web page found} other {# web pages found}} FYI, I've never seen a grdp line that looks anything like this. Are you sure it's correct?
Thanks Mark! https://codereview.chromium.org/2203993002/diff/60001/components/omnibox/brow... File components/omnibox/browser/physical_web_provider.cc (right): https://codereview.chromium.org/2203993002/diff/60001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.cc:20: // exceeds this limit, then an overflow item is created. Tapping the overflow On 2016/08/10 23:33:20, Mark P wrote: > nit: omit "then" Done. https://codereview.chromium.org/2203993002/diff/60001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.cc:22: // is counted as a match result for the purposes of the match limit. On 2016/08/10 23:33:20, Mark P wrote: > Can you put this example of yours in the comment? > With kPhysicalWebMaxMatches == 1, there should be at most one suggestion created > by this provider. If there is a single nearby URL, then the suggestion will be > for that URL. If there are multiple nearby URLs, the suggestion will be the > overflow item which navigates to the WebUI when tapped. Done. https://codereview.chromium.org/2203993002/diff/60001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.cc:23: static const int kPhysicalWebMaxMatches = 1; On 2016/08/10 23:33:20, Mark P wrote: > Can you put a DCHECK assertion somewhere that this is less than general provider > limit of kMaxMatches? Sure, added it to Start. https://codereview.chromium.org/2203993002/diff/60001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.cc:26: static const int kPhysicalWebUrlRelevance = 700; On 2016/08/10 23:33:21, Mark P wrote: > On 2016/08/09 21:42:03, mattreynolds wrote: > > On 2016/08/08 20:41:37, Mark P wrote: > > > Please comment, justifying why this is intentionally higher than the > clipboard > > > suggestions. > > > Or maybe it lower, justifying why it's lower. :-) > > > > The current value was chosen arbitrarily, I think we should lower it below the > > clipboard URL relevance. > > > > ClipboardURLProvider says: "The relevance is 800 to beat ZeroSuggest results." > > > > ZeroSuggestProvider sets its default relevance as 100 but the actual relevance > > scores come from the server. Do you know what their range is? > > I wrote: I think it's rarely above 600. I suggest using 700 and not worrying > and more. > And then I saw you chose 700. :-) > > Nonetheless, please comment that this is intentionally lower than clipboard > suggestions. Done. https://codereview.chromium.org/2203993002/diff/60001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.cc:99: // Physical Web results should never omit protocols and never appear bold On 2016/08/10 23:33:20, Mark P wrote: > On 2016/08/09 21:42:03, mattreynolds wrote: > > On 2016/08/08 20:41:37, Mark P wrote: > > > I am skeptical of this omitting protocols. This philosophy differs from the > > > default omnibox where we always display secure protocols explicitly. I > don't > > > see why this situation is different. > > > > Changed to kFormatUrlOmitHttp. > > Please revise this comment; it's currently wrong. Fixed https://codereview.chromium.org/2203993002/diff/60001/components/omnibox/brow... File components/omnibox/browser/physical_web_provider.h (right): https://codereview.chromium.org/2203993002/diff/60001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.h:30: // matches allowed by this provider. If the total number of nearby URLs On 2016/08/10 23:33:21, Mark P wrote: > First sentence: this should mention where the matches are being added and where > the URLs come from. Are matches being encoded and stored in |metadata_list|? > Somewhere else? And where is the info on what matches exist coming from? > Something injected? Something in |metadata_list|. Done. https://codereview.chromium.org/2203993002/diff/60001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.h:31: // exceeds this limit, one match is reserved for an overflow item. On 2016/08/10 23:33:21, Mark P wrote: > nit: "one match" -> "one slot" > (I assume you don't fill in this extra slot with a match.) The terminology is confusing since technically they are all AutocompleteMatches, but I agree "slot" is clearer here. https://codereview.chromium.org/2203993002/diff/60001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.h:35: // maximum number of matches allowed by this provider. Selecting the overflow On 2016/08/10 23:33:21, Mark P wrote: > ditto same concerns about unclear what variables are being changed. > > Also, in general would it make sense--and make the code simpler--to make this > function unconditional (always add something) and simply be smart about when to > call it? Makes sense, renamed to AppendOverflowItem and moved the conditional logic into ConstructMatches since it already needs to consider whether there should be an overflow item. https://codereview.chromium.org/2203993002/diff/60001/components/omnibox_stri... File components/omnibox_strings.grdp (right): https://codereview.chromium.org/2203993002/diff/60001/components/omnibox_stri... components/omnibox_strings.grdp:22: {URL_COUNT, plural, =1 {1 web page found} other {# web pages found}} On 2016/08/10 23:33:21, Mark P wrote: > FYI, I've never seen a grdp line that looks anything like this. Are you sure > it's correct? Yup, this is the preferred way to handle plurals (and gender) in localized strings, see go/plurals. There are some more examples in chromium_strings.grd.
Please consider adding a unit test. I know the code in its current form is simple, and I won't require adding one. But you may have some idea of the possible ways it can evolve in the future and in some of those cases it might be useful to have tests in place. Think about it. --mark https://codereview.chromium.org/2203993002/diff/60001/components/omnibox_stri... File components/omnibox_strings.grdp (right): https://codereview.chromium.org/2203993002/diff/60001/components/omnibox_stri... components/omnibox_strings.grdp:22: {URL_COUNT, plural, =1 {1 web page found} other {# web pages found}} On 2016/08/11 01:25:03, mattreynolds wrote: > On 2016/08/10 23:33:21, Mark P wrote: > > FYI, I've never seen a grdp line that looks anything like this. Are you sure > > it's correct? > > Yup, this is the preferred way to handle plurals (and gender) in localized > strings, see go/plurals. There are some more examples in chromium_strings.grd. Should this be "URL_count"? That seems like more natural capitalization. https://codereview.chromium.org/2203993002/diff/80001/components/omnibox/brow... File components/omnibox/browser/physical_web_provider.cc (right): https://codereview.chromium.org/2203993002/diff/80001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.cc:83: const bool needs_overflow = metadata_count > kPhysicalWebMaxMatches; This needs overflow is no longer correct, as you may not add something if there's a malformed entry in metadata. Just do a direct size comparison at the time you need. This also means max_rows could be calculated incorrectly. https://codereview.chromium.org/2203993002/diff/80001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.cc:87: for (size_t i = 0; i < metadata_count && matches_.size() < max_rows; ++i) { nit: parens around the comparison operators (but this comparison may be revised given the above comment) https://codereview.chromium.org/2203993002/diff/80001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.cc:112: match.fill_into_edit += Why +=? https://codereview.chromium.org/2203993002/diff/80001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.cc:116: AutocompleteMatch::ClassifyLocationInString(base::string16::npos, 0, This more naturally immediately follows the initialization of match.contents. https://codereview.chromium.org/2203993002/diff/80001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.cc:116: AutocompleteMatch::ClassifyLocationInString(base::string16::npos, 0, I think the first three parameters should actually be 0, match.contents.length(), match.contents.length() not what you have. What do you think? (and where did you get this from, if it came from somewhere else?) ditto analogous comment for the two other calls below https://codereview.chromium.org/2203993002/diff/80001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.cc:127: matches_.push_back(match); I notice you give all results the same relevance score. This means that can change their order in the final sort phase. Does the physical web data come in a particular order (e.g., quality of signal)? If so, you may want to use that order and give all the results decreasing relevance scores. Obviously, the overflow item should have a relevance score just below your lowest-scoring item. https://codereview.chromium.org/2203993002/diff/80001/components/omnibox/brow... File components/omnibox/browser/physical_web_provider.h (right): https://codereview.chromium.org/2203993002/diff/80001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.h:31: // number of nearby URLs exceeds this limit, one slot is reserved for an nit: change "slot" back to "match" and "reserved" to "used" (My comment made sense before you made ConstructMatches() call AppendOverflowItem(). Before, when you called them sequentially in Start(), ConstructMatches() really did leave one empty slot expecting AppendOverflowItem() to fill it in.
https://codereview.chromium.org/2203993002/diff/60001/components/omnibox_stri... File components/omnibox_strings.grdp (right): https://codereview.chromium.org/2203993002/diff/60001/components/omnibox_stri... components/omnibox_strings.grdp:22: {URL_COUNT, plural, =1 {1 web page found} other {# web pages found}} On 2016/08/11 23:16:03, Mark P wrote: > On 2016/08/11 01:25:03, mattreynolds wrote: > > On 2016/08/10 23:33:21, Mark P wrote: > > > FYI, I've never seen a grdp line that looks anything like this. Are you > sure > > > it's correct? > > > > Yup, this is the preferred way to handle plurals (and gender) in localized > > strings, see go/plurals. There are some more examples in chromium_strings.grd. > > Should this be "URL_count"? That seems like more natural capitalization. Done. https://codereview.chromium.org/2203993002/diff/80001/components/omnibox/brow... File components/omnibox/browser/physical_web_provider.cc (right): https://codereview.chromium.org/2203993002/diff/80001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.cc:83: const bool needs_overflow = metadata_count > kPhysicalWebMaxMatches; On 2016/08/11 23:16:03, Mark P wrote: > This needs overflow is no longer correct, as you may not add something if > there's a malformed entry in metadata. Just do a direct size comparison at the > time you need. This also means max_rows could be calculated incorrectly. Fixed so that we only count slots as used once we've successfully created an AutocompleteMatch and added it to matches_. There's a small chance we'll create the overflow item unnecessarily. (eg: suppose we have 1 slot and 2 metadata items, the second item could be invalid since we never validate it) https://codereview.chromium.org/2203993002/diff/80001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.cc:87: for (size_t i = 0; i < metadata_count && matches_.size() < max_rows; ++i) { On 2016/08/11 23:16:03, Mark P wrote: > nit: parens around the comparison operators > (but this comparison may be revised given the above comment) Acknowledged. https://codereview.chromium.org/2203993002/diff/80001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.cc:112: match.fill_into_edit += On 2016/08/11 23:16:03, Mark P wrote: > Why +=? Not sure, I borrowed this code from ZeroSuggestProvider::NavigationToMatch. It doesn't appear to have any purpose there either, so I switched to just = https://codereview.chromium.org/2203993002/diff/80001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.cc:116: AutocompleteMatch::ClassifyLocationInString(base::string16::npos, 0, On 2016/08/11 23:16:03, Mark P wrote: > I think the first three parameters should actually be > 0, match.contents.length(), match.contents.length() > not what you have. > > What do you think? (and where did you get this from, if it came from somewhere > else?) > > ditto analogous comment for the two other calls below This was also copied from ZeroSuggestProvider::NavigationToMatch. We're trying to mark the entire string as "no match". ClassifyLocationInString has an early-exit case with match_location=base::string16::npos that will avoid classifying any portion of the string with ACMatchClassification::MATCH. However, if we pass match_location=0 then it will create a classification with the MATCH bit set. https://codereview.chromium.org/2203993002/diff/80001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.cc:127: matches_.push_back(match); On 2016/08/11 23:16:03, Mark P wrote: > I notice you give all results the same relevance score. This means that can > change their order in the final sort phase. Does the physical web data come in > a particular order (e.g., quality of signal)? If so, you may want to use that > order and give all the results decreasing relevance scores. > > Obviously, the overflow item should have a relevance score just below your > lowest-scoring item. Yes, we would like to preserve the order from the data source. I've adjusted the relevance scores so they decrease from the base value. https://codereview.chromium.org/2203993002/diff/80001/components/omnibox/brow... File components/omnibox/browser/physical_web_provider.h (right): https://codereview.chromium.org/2203993002/diff/80001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.h:31: // number of nearby URLs exceeds this limit, one slot is reserved for an On 2016/08/11 23:16:03, Mark P wrote: > nit: change "slot" back to "match" and "reserved" to "used" > (My comment made sense before you made ConstructMatches() call > AppendOverflowItem(). Before, when you called them sequentially in Start(), > ConstructMatches() really did leave one empty slot expecting > AppendOverflowItem() to fill it in. Done.
thanks for the tests. --mark https://codereview.chromium.org/2203993002/diff/80001/components/omnibox/brow... File components/omnibox/browser/physical_web_provider.cc (right): https://codereview.chromium.org/2203993002/diff/80001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.cc:83: const bool needs_overflow = metadata_count > kPhysicalWebMaxMatches; On 2016/08/16 20:35:25, mattreynolds wrote: > On 2016/08/11 23:16:03, Mark P wrote: > > This needs overflow is no longer correct, as you may not add something if > > there's a malformed entry in metadata. Just do a direct size comparison at > the > > time you need. This also means max_rows could be calculated incorrectly. > > Fixed so that we only count slots as used once we've successfully created an > AutocompleteMatch and added it to matches_. There's a small chance we'll create > the overflow item unnecessarily. (eg: suppose we have 1 slot and 2 metadata > items, the second item could be invalid since we never validate it) Acknowledged. This seems like small enough of a deal that it isn't worth fixing. https://codereview.chromium.org/2203993002/diff/80001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.cc:116: AutocompleteMatch::ClassifyLocationInString(base::string16::npos, 0, On 2016/08/16 20:35:25, mattreynolds wrote: > On 2016/08/11 23:16:03, Mark P wrote: > > I think the first three parameters should actually be > > 0, match.contents.length(), match.contents.length() > > not what you have. > > > > What do you think? (and where did you get this from, if it came from somewhere > > else?) > > > > ditto analogous comment for the two other calls below > > This was also copied from ZeroSuggestProvider::NavigationToMatch. We're trying > to mark the entire string as "no match". ClassifyLocationInString has an > early-exit case with match_location=base::string16::npos that will avoid > classifying any portion of the string with ACMatchClassification::MATCH. > However, if we pass match_location=0 then it will create a classification with > the MATCH bit set. Well, that's a poorly commented behavior of ClassifyLocationInString(), but so be it. Leaving it as-is is fine, though if you want to do the more direct thing: match.contents_class.push_back(ACMatchClassification(0, ACMatchClassification::URL)); rather than the current long-winded call, that would be good too. https://codereview.chromium.org/2203993002/diff/100001/components/omnibox/bro... File components/omnibox/browser/physical_web_provider.cc (right): https://codereview.chromium.org/2203993002/diff/100001/components/omnibox/bro... components/omnibox/browser/physical_web_provider.cc:108: if (remaining_slots == 1 && remaining_metadata > remaining_slots) { nit: parens around the comparison operators https://codereview.chromium.org/2203993002/diff/100001/components/omnibox/bro... components/omnibox/browser/physical_web_provider.cc:144: void PhysicalWebProvider::AppendOverflowItem(int additional_url_count, nit: either align the parameters at the paren or align them both flush left. https://codereview.chromium.org/2203993002/diff/100001/components/omnibox/bro... components/omnibox/browser/physical_web_provider.cc:147: AutocompleteMatchType::PHYSICAL_WEB_OVERFLOW); Good catch. https://codereview.chromium.org/2203993002/diff/100001/components/omnibox/bro... File components/omnibox/browser/physical_web_provider_unittest.cc (right): https://codereview.chromium.org/2203993002/diff/100001/components/omnibox/bro... components/omnibox/browser/physical_web_provider_unittest.cc:29: {} nit: prefer { at end of previous line ditto on the other constructors (if everything fits on one line, you can put the {} on the same line; if the constructor and initializer terms do not fit on one line, please split the {} over two lines) https://codereview.chromium.org/2203993002/diff/100001/components/omnibox/bro... components/omnibox/browser/physical_web_provider_unittest.cc:33: // PhysicalWebDataSource implementation This comment makes it seem like it applies to only the block below, but it applies to more things in this class. Please revise for clarity or remove it. https://codereview.chromium.org/2203993002/diff/100001/components/omnibox/bro... components/omnibox/browser/physical_web_provider_unittest.cc:71: // Convenience method for accessing the mock data source. This seems unnecessary. https://codereview.chromium.org/2203993002/diff/100001/components/omnibox/bro... components/omnibox/browser/physical_web_provider_unittest.cc:95: static std::unique_ptr<base::ListValue> CreateMetadata( Comment. https://codereview.chromium.org/2203993002/diff/100001/components/omnibox/bro... components/omnibox/browser/physical_web_provider_unittest.cc:99: std::ostringstream url; cleanup https://codereview.chromium.org/2203993002/diff/100001/components/omnibox/bro... components/omnibox/browser/physical_web_provider_unittest.cc:105: metadata_item->SetString("title", "Example title"); consider putting "i" in the title and description as well https://codereview.chromium.org/2203993002/diff/100001/components/omnibox/bro... components/omnibox/browser/physical_web_provider_unittest.cc:131: std::string(), GURL(url), metrics::OmniboxEventProto::INVALID_SPEC, Prefer OTHER to be correspond with the http://www.cnn.com/ input. ditto below https://codereview.chromium.org/2203993002/diff/100001/components/omnibox/bro... components/omnibox/browser/physical_web_provider_unittest.cc:155: EXPECT_TRUE(provider_->matches().size() == 1); Prefer EXPECT_EQ(1, provider_->matches().size()); for better test failure messages. Ditto elsewhere: EXPECT_EQ(expected, actual) There's also things like EXPECT_LT, EXPECT_GT, EXPECT_GE, and so on. https://codereview.chromium.org/2203993002/diff/100001/components/omnibox/bro... components/omnibox/browser/physical_web_provider_unittest.cc:157: EXPECT_TRUE(metadata_match.type == AutocompleteMatchType::PHYSICAL_WEB); Consider checking some of the other fields (i.e., URL goes to the right place, title is set correctly, highlighting is correct, ...) https://codereview.chromium.org/2203993002/diff/100001/components/omnibox/bro... components/omnibox/browser/physical_web_provider_unittest.cc:165: // AutocompleteProvider's limit. Good comment. :-) https://codereview.chromium.org/2203993002/diff/100001/components/omnibox/bro... components/omnibox/browser/physical_web_provider_unittest.cc:203: std::string(), GURL(), metrics::OmniboxEventProto::INVALID_SPEC, consider using something like INSTANT_NTP_WITH_OMNIBOX_AS_STARTING_FOCUS instead of INVALID_SPEC
https://codereview.chromium.org/2203993002/diff/100001/components/omnibox/bro... File components/omnibox/browser/physical_web_provider.cc (right): https://codereview.chromium.org/2203993002/diff/100001/components/omnibox/bro... components/omnibox/browser/physical_web_provider.cc:108: if (remaining_slots == 1 && remaining_metadata > remaining_slots) { On 2016/08/16 22:49:08, Mark P wrote: > nit: parens around the comparison operators Done. https://codereview.chromium.org/2203993002/diff/100001/components/omnibox/bro... components/omnibox/browser/physical_web_provider.cc:144: void PhysicalWebProvider::AppendOverflowItem(int additional_url_count, On 2016/08/16 22:49:08, Mark P wrote: > nit: either align the parameters at the paren or align them both flush left. Done. https://codereview.chromium.org/2203993002/diff/100001/components/omnibox/bro... components/omnibox/browser/physical_web_provider.cc:147: AutocompleteMatchType::PHYSICAL_WEB_OVERFLOW); On 2016/08/16 22:49:08, Mark P wrote: > Good catch. Hooray for unit testing! https://codereview.chromium.org/2203993002/diff/100001/components/omnibox/bro... File components/omnibox/browser/physical_web_provider_unittest.cc (right): https://codereview.chromium.org/2203993002/diff/100001/components/omnibox/bro... components/omnibox/browser/physical_web_provider_unittest.cc:29: {} On 2016/08/16 22:49:08, Mark P wrote: > nit: prefer { at end of previous line > ditto on the other constructors > (if everything fits on one line, you can put the {} on the same line; > if the constructor and initializer terms do not fit on one line, please > split the {} over two lines) Done. https://codereview.chromium.org/2203993002/diff/100001/components/omnibox/bro... components/omnibox/browser/physical_web_provider_unittest.cc:33: // PhysicalWebDataSource implementation On 2016/08/16 22:49:09, Mark P wrote: > This comment makes it seem like it applies to only the block below, but it > applies to more things in this class. Please revise for clarity or remove it. Removed https://codereview.chromium.org/2203993002/diff/100001/components/omnibox/bro... components/omnibox/browser/physical_web_provider_unittest.cc:71: // Convenience method for accessing the mock data source. On 2016/08/16 22:49:08, Mark P wrote: > This seems unnecessary. Removed. https://codereview.chromium.org/2203993002/diff/100001/components/omnibox/bro... components/omnibox/browser/physical_web_provider_unittest.cc:95: static std::unique_ptr<base::ListValue> CreateMetadata( On 2016/08/16 22:49:08, Mark P wrote: > Comment. Done. https://codereview.chromium.org/2203993002/diff/100001/components/omnibox/bro... components/omnibox/browser/physical_web_provider_unittest.cc:99: std::ostringstream url; On 2016/08/16 22:49:08, Mark P wrote: > cleanup Not sure what this comment was referring to, the ostringstream is used below to compose the url. This block was modified to add i to the title and description, please take another look. https://codereview.chromium.org/2203993002/diff/100001/components/omnibox/bro... components/omnibox/browser/physical_web_provider_unittest.cc:105: metadata_item->SetString("title", "Example title"); On 2016/08/16 22:49:08, Mark P wrote: > consider putting "i" in the title and description as well Done. https://codereview.chromium.org/2203993002/diff/100001/components/omnibox/bro... components/omnibox/browser/physical_web_provider_unittest.cc:131: std::string(), GURL(url), metrics::OmniboxEventProto::INVALID_SPEC, On 2016/08/16 22:49:08, Mark P wrote: > Prefer OTHER to be correspond with the http://www.cnn.com/ input. > ditto below Done. https://codereview.chromium.org/2203993002/diff/100001/components/omnibox/bro... components/omnibox/browser/physical_web_provider_unittest.cc:155: EXPECT_TRUE(provider_->matches().size() == 1); On 2016/08/16 22:49:08, Mark P wrote: > Prefer > EXPECT_EQ(1, provider_->matches().size()); > for better test failure messages. > > Ditto elsewhere: > EXPECT_EQ(expected, actual) > > There's also things like > EXPECT_LT, EXPECT_GT, EXPECT_GE, and so on. Done. https://codereview.chromium.org/2203993002/diff/100001/components/omnibox/bro... components/omnibox/browser/physical_web_provider_unittest.cc:157: EXPECT_TRUE(metadata_match.type == AutocompleteMatchType::PHYSICAL_WEB); On 2016/08/16 22:49:09, Mark P wrote: > Consider checking some of the other fields (i.e., URL goes to the right place, > title is set correctly, highlighting is correct, ...) Added checks for destination_url, contents, and description https://codereview.chromium.org/2203993002/diff/100001/components/omnibox/bro... components/omnibox/browser/physical_web_provider_unittest.cc:203: std::string(), GURL(), metrics::OmniboxEventProto::INVALID_SPEC, On 2016/08/16 22:49:09, Mark P wrote: > consider using something like INSTANT_NTP_WITH_OMNIBOX_AS_STARTING_FOCUS instead > of INVALID_SPEC Done.
Almost done. :-) --mark https://codereview.chromium.org/2203993002/diff/100001/components/omnibox/bro... File components/omnibox/browser/physical_web_provider_unittest.cc (right): https://codereview.chromium.org/2203993002/diff/100001/components/omnibox/bro... components/omnibox/browser/physical_web_provider_unittest.cc:71: // Convenience method for accessing the mock data source. On 2016/08/17 01:07:22, mattreynolds wrote: > On 2016/08/16 22:49:08, Mark P wrote: > > This seems unnecessary. > > Removed. Oops, I see why you had this--so you don't have to cast below. Please put it back (sorry!) and add something to the comment such as "so we don't have to downcast in the code below." https://codereview.chromium.org/2203993002/diff/120001/components/omnibox/bro... File components/omnibox/browser/physical_web_provider_unittest.cc (right): https://codereview.chromium.org/2203993002/diff/120001/components/omnibox/bro... components/omnibox/browser/physical_web_provider_unittest.cc:95: std::string item_id = buf.str(); What's wrong with base::SizeTToString()? https://codereview.chromium.org/2203993002/diff/120001/components/omnibox/bro... components/omnibox/browser/physical_web_provider_unittest.cc:197: EXPECT_EQ(AutocompleteMatchType::PHYSICAL_WEB_OVERFLOW, overflow_match.type); Does it makes sense to check other fields here? Perhaps at least the destination URL? Maybe also the title and description (though no sure if that will just work with the generate strings i18n stuff).
Thanks Mark! https://codereview.chromium.org/2203993002/diff/100001/components/omnibox/bro... File components/omnibox/browser/physical_web_provider_unittest.cc (right): https://codereview.chromium.org/2203993002/diff/100001/components/omnibox/bro... components/omnibox/browser/physical_web_provider_unittest.cc:71: // Convenience method for accessing the mock data source. On 2016/08/18 19:52:26, Mark P wrote: > On 2016/08/17 01:07:22, mattreynolds wrote: > > On 2016/08/16 22:49:08, Mark P wrote: > > > This seems unnecessary. > > > > Removed. > > Oops, I see why you had this--so you don't have to cast below. Please put it > back (sorry!) and add something to the comment such as "so we don't have to > downcast in the code below." Done. https://codereview.chromium.org/2203993002/diff/120001/components/omnibox/bro... File components/omnibox/browser/physical_web_provider_unittest.cc (right): https://codereview.chromium.org/2203993002/diff/120001/components/omnibox/bro... components/omnibox/browser/physical_web_provider_unittest.cc:95: std::string item_id = buf.str(); On 2016/08/18 19:52:26, Mark P wrote: > What's wrong with base::SizeTToString()? Nothing, just never encountered it before. Thanks! https://codereview.chromium.org/2203993002/diff/120001/components/omnibox/bro... components/omnibox/browser/physical_web_provider_unittest.cc:197: EXPECT_EQ(AutocompleteMatchType::PHYSICAL_WEB_OVERFLOW, overflow_match.type); On 2016/08/18 19:52:26, Mark P wrote: > Does it makes sense to check other fields here? Perhaps at least the > destination URL? Maybe also the title and description (though no sure if that > will just work with the generate strings i18n stuff). Sure, I added checks for the destination URL, description, and contents (which should be empty). The i18n shouldn't be an issue as long as we create the string the same way.
lgtm once you resolve the final issue below --mark https://codereview.chromium.org/2203993002/diff/140001/components/omnibox/bro... File components/omnibox/browser/physical_web_provider_unittest.cc (right): https://codereview.chromium.org/2203993002/diff/140001/components/omnibox/bro... components/omnibox/browser/physical_web_provider_unittest.cc:206: EXPECT_TRUE(overflow_match.contents.empty()); Contents are supposed to be empty? Shouldn't it be the URL?
https://codereview.chromium.org/2203993002/diff/140001/components/omnibox/bro... File components/omnibox/browser/physical_web_provider_unittest.cc (right): https://codereview.chromium.org/2203993002/diff/140001/components/omnibox/bro... components/omnibox/browser/physical_web_provider_unittest.cc:206: EXPECT_TRUE(overflow_match.contents.empty()); On 2016/08/18 23:15:46, Mark P wrote: > Contents are supposed to be empty? Shouldn't it be the URL? We weren't planning on showing the URL in the match item since it's a chrome:// URL that wouldn't have much meaning to the user. Instead, we'll use the contents string to display the first N characters of the page title of the top result. See the mock on p2 of go/pw-bling-omnibox, it'll be something like "Example pa... and 2 more web pages".
https://codereview.chromium.org/2203993002/diff/140001/components/omnibox/bro... File components/omnibox/browser/physical_web_provider_unittest.cc (right): https://codereview.chromium.org/2203993002/diff/140001/components/omnibox/bro... components/omnibox/browser/physical_web_provider_unittest.cc:206: EXPECT_TRUE(overflow_match.contents.empty()); On 2016/08/18 23:28:39, mattreynolds wrote: > On 2016/08/18 23:15:46, Mark P wrote: > > Contents are supposed to be empty? Shouldn't it be the URL? > > We weren't planning on showing the URL in the match item since it's a chrome:// > URL that wouldn't have much meaning to the user. Instead, we'll use the contents > string to display the first N characters of the page title of the top result. > See the mock on p2 of go/pw-bling-omnibox, it'll be something like "Example > pa... and 2 more web pages". Shouldn't this setting of the contents string happen somewhere in this patchset?
mattreynolds@chromium.org changed reviewers: + blundell@chromium.org, rohitrao@chromium.org
blundell@chromium.org: Please review changes in //components rohitrao@chromium.org: Please review changes in //ios/chrome
https://codereview.chromium.org/2203993002/diff/140001/components/omnibox/bro... File components/omnibox/browser/physical_web_provider_unittest.cc (right): https://codereview.chromium.org/2203993002/diff/140001/components/omnibox/bro... components/omnibox/browser/physical_web_provider_unittest.cc:206: EXPECT_TRUE(overflow_match.contents.empty()); On 2016/08/18 23:33:43, Mark P wrote: > On 2016/08/18 23:28:39, mattreynolds wrote: > > On 2016/08/18 23:15:46, Mark P wrote: > > > Contents are supposed to be empty? Shouldn't it be the URL? > > > > We weren't planning on showing the URL in the match item since it's a > chrome:// > > URL that wouldn't have much meaning to the user. Instead, we'll use the > contents > > string to display the first N characters of the page title of the top result. > > See the mock on p2 of go/pw-bling-omnibox, it'll be something like "Example > > pa... and 2 more web pages". > > Shouldn't this setting of the contents string happen somewhere in this patchset? I decided to leave it out since the format of the string isn't nailed down yet, and this CL is already pretty large.
https://codereview.chromium.org/2203993002/diff/140001/components/omnibox/bro... File components/omnibox/browser/physical_web_provider_unittest.cc (right): https://codereview.chromium.org/2203993002/diff/140001/components/omnibox/bro... components/omnibox/browser/physical_web_provider_unittest.cc:206: EXPECT_TRUE(overflow_match.contents.empty()); On 2016/08/18 23:48:51, mattreynolds wrote: > On 2016/08/18 23:33:43, Mark P wrote: > > On 2016/08/18 23:28:39, mattreynolds wrote: > > > On 2016/08/18 23:15:46, Mark P wrote: > > > > Contents are supposed to be empty? Shouldn't it be the URL? > > > > > > We weren't planning on showing the URL in the match item since it's a > > chrome:// > > > URL that wouldn't have much meaning to the user. Instead, we'll use the > > contents > > > string to display the first N characters of the page title of the top > result. > > > See the mock on p2 of go/pw-bling-omnibox, it'll be something like "Example > > > pa... and 2 more web pages". > > > > Shouldn't this setting of the contents string happen somewhere in this > patchset? > > I decided to leave it out since the format of the string isn't nailed down yet, > and this CL is already pretty large. It looks like this changelist enables this provider by default. You don't want the contents blank by default. Whenever you submit this changelist, please file a releaseblock beta bug targetted to the current milestone to set contents correctly.
//components lgtm https://codereview.chromium.org/2203993002/diff/140001/components/components_... File components/components_tests.gyp (right): https://codereview.chromium.org/2203993002/diff/140001/components/components_... components/components_tests.gyp:476: 'omnibox/browser/physical_web_provider_unittest.cc', You can revert the changes to this file, as we're not required to keep the GYP build in sync anymore. https://codereview.chromium.org/2203993002/diff/140001/components/omnibox.gypi File components/omnibox.gypi (right): https://codereview.chromium.org/2203993002/diff/140001/components/omnibox.gyp... components/omnibox.gypi:33: 'physical_web_data_source', same comment as on other gypfile. https://codereview.chromium.org/2203993002/diff/140001/components/omnibox_str... File components/omnibox_strings.grdp (right): https://codereview.chromium.org/2203993002/diff/140001/components/omnibox_str... components/omnibox_strings.grdp:21: <message name="IDS_PHYSICAL_WEB_OVERFLOW" desc="The label in the omnibox dropdown indicating that multiple nearby devices are broadcasting URLs."> Can you add per-file owners of this file to //components/OWNERS, similar to the existing line there for omnibox.gypi?
ios LGTM
https://codereview.chromium.org/2203993002/diff/140001/components/components_... File components/components_tests.gyp (right): https://codereview.chromium.org/2203993002/diff/140001/components/components_... components/components_tests.gyp:476: 'omnibox/browser/physical_web_provider_unittest.cc', On 2016/08/19 07:18:34, blundell wrote: > You can revert the changes to this file, as we're not required to keep the GYP > build in sync anymore. Great news! Reverted. https://codereview.chromium.org/2203993002/diff/140001/components/omnibox.gypi File components/omnibox.gypi (right): https://codereview.chromium.org/2203993002/diff/140001/components/omnibox.gyp... components/omnibox.gypi:33: 'physical_web_data_source', On 2016/08/19 07:18:34, blundell wrote: > same comment as on other gypfile. Reverted https://codereview.chromium.org/2203993002/diff/140001/components/omnibox/bro... File components/omnibox/browser/physical_web_provider_unittest.cc (right): https://codereview.chromium.org/2203993002/diff/140001/components/omnibox/bro... components/omnibox/browser/physical_web_provider_unittest.cc:206: EXPECT_TRUE(overflow_match.contents.empty()); On 2016/08/19 03:55:03, Mark P wrote: > On 2016/08/18 23:48:51, mattreynolds wrote: > > On 2016/08/18 23:33:43, Mark P wrote: > > > On 2016/08/18 23:28:39, mattreynolds wrote: > > > > On 2016/08/18 23:15:46, Mark P wrote: > > > > > Contents are supposed to be empty? Shouldn't it be the URL? > > > > > > > > We weren't planning on showing the URL in the match item since it's a > > > chrome:// > > > > URL that wouldn't have much meaning to the user. Instead, we'll use the > > > contents > > > > string to display the first N characters of the page title of the top > > result. > > > > See the mock on p2 of go/pw-bling-omnibox, it'll be something like > "Example > > > > pa... and 2 more web pages". > > > > > > Shouldn't this setting of the contents string happen somewhere in this > > patchset? > > > > I decided to leave it out since the format of the string isn't nailed down > yet, > > and this CL is already pretty large. > > It looks like this changelist enables this provider by default. > You don't want the contents blank by default. > Whenever you submit this changelist, please file a releaseblock beta bug > targetted to the current milestone to set contents correctly. Ah, I didn't realize it would be an issue to leave contents empty. I went ahead and set it to the URL for now, please take another look. Although the provider is enabled by default, it shouldn't be possible for it to create any matches until Physical Web scanning is turned on. Currently, scanning is disabled under a flag.
still lgtm, one comment https://codereview.chromium.org/2203993002/diff/160001/components/omnibox/bro... File components/omnibox/browser/physical_web_provider.cc (right): https://codereview.chromium.org/2203993002/diff/160001/components/omnibox/bro... components/omnibox/browser/physical_web_provider.cc:149: match.destination_url = GURL(url_string); nit: use the variable url you just created.
https://codereview.chromium.org/2203993002/diff/160001/components/omnibox/bro... File components/omnibox/browser/physical_web_provider.cc (right): https://codereview.chromium.org/2203993002/diff/160001/components/omnibox/bro... components/omnibox/browser/physical_web_provider.cc:149: match.destination_url = GURL(url_string); On 2016/08/19 18:46:33, Mark P wrote: > nit: use the variable url you just created. Done.
The CQ bit was checked by mattreynolds@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from blundell@chromium.org, rohitrao@chromium.org, mpearson@chromium.org Link to the patchset: https://codereview.chromium.org/2203993002/#ps180001 (title: "fix nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Add a Physical Web omnibox autocomplete provider When nearby devices are broadcasting Physical Web URLs, Chrome will display an autocomplete suggestion in the omnibox. The suggestion item will link to a WebUI page containing the full list of nearby URLs. BUG=630769 ========== to ========== Add a Physical Web omnibox autocomplete provider When nearby devices are broadcasting Physical Web URLs, Chrome will display an autocomplete suggestion in the omnibox. The suggestion item will link to a WebUI page containing the full list of nearby URLs. BUG=630769 Committed: https://crrev.com/5afc0169014d1f9560d58799615bc04429d67df0 Cr-Commit-Position: refs/heads/master@{#413262} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/5afc0169014d1f9560d58799615bc04429d67df0 Cr-Commit-Position: refs/heads/master@{#413262} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
