Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2947643002:
[ios] Enables the BuiltinProvider. (Closed)
|
Created:
3 years, 6 months ago by rohitrao (ping after 24h) Modified:
3 years, 6 months ago CC:
chromium-reviews, marq+watch_chromium.org, jdonnelly+watch_chromium.org, ios-reviews+chrome_chromium.org, noyau+watch_chromium.org, ios-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[ios] Enables the BuiltinProvider.
Implements two methods in the iOS AutocompleteProviderClientImpl that provide
the list of builtin URLs to the provider.
BUG=725120
TEST=The omnibox should suggest builtin URLs when typing "chrome://"
TEST=The omnibox should inline-autocomplete "chrome://version" and "chrome://chrome-urls"
Review-Url: https://codereview.chromium.org/2947643002
Cr-Commit-Position: refs/heads/master@{#481238}
Committed: https://chromium.googlesource.com/chromium/src/+/4115f4c206ddd94f5ee0674432263e42727de4e5
Patch Set 1 #
Total comments: 9
Patch Set 2 : Review #
Messages
Total messages: 28 (17 generated)
rohitrao@chromium.org changed reviewers: + pkasting@chromium.org, sdefresne@chromium.org
The CQ bit was checked by rohitrao@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...
jdonnelly@chromium.org changed reviewers: + jdonnelly@chromium.org
Drive-by since I'm now in components/omnibox/OWNERS and am going to try to take some code reviews off of pkasting's plate. This code looks good but can you also #if !defined(OS_IOS) whichever URLs don't work in iOS in chrome/common/url_constants.cc?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM if the answer to my first question below is "no" -- and thanks to jdonnelly for starting to step up here :) https://codereview.chromium.org/2947643002/diff/1/ios/chrome/browser/autocomp... File ios/chrome/browser/autocomplete/autocomplete_provider_client_impl.cc (right): https://codereview.chromium.org/2947643002/diff/1/ios/chrome/browser/autocomp... ios/chrome/browser/autocomplete/autocomplete_provider_client_impl.cc:121: std::vector<base::string16> AutocompleteProviderClientImpl::GetBuiltinURLs() { It looks like these two functions are basically copies of the ones in ChromeAutocompleteProviderClient with the "if !ANDROID" stuff removed. Is there any place we can put this code that will be shared so we don't need to duplicate it? https://codereview.chromium.org/2947643002/diff/1/ios/chrome/browser/autocomp... ios/chrome/browser/autocomplete/autocomplete_provider_client_impl.cc:128: for (std::vector<std::string>::iterator i(chrome_builtins.begin()); Nit: Range-based for can help here https://codereview.chromium.org/2947643002/diff/1/ios/chrome/browser/autocomp... ios/chrome/browser/autocomplete/autocomplete_provider_client_impl.cc:139: return builtins_to_provide; Nit: I bet you could just do: return {base::ASCIIToUTF16(kChromeUIChromeURLsURL), base::ASCIIToUTF16(kChromeUIVersionURL)};
The CQ bit was checked by rohitrao@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...
On 2017/06/19 14:24:45, Justin Donnelly wrote: > This code looks good but can you also #if !defined(OS_IOS) whichever URLs don't > work in iOS in chrome/common/url_constants.cc? Let me push back on this a bit. The iOS port is completely disallowed from compiling any code from the chrome/ directory. I think the presence of OS_IOS in chrome/ muddles this, and I'd actually like to move in the direction of removing the #ifdefs. If I added a comment pointing to the iOS version of these constants, would that suffice to make it clear that iOS is using different values? https://codereview.chromium.org/2947643002/diff/1/ios/chrome/browser/autocomp... File ios/chrome/browser/autocomplete/autocomplete_provider_client_impl.cc (right): https://codereview.chromium.org/2947643002/diff/1/ios/chrome/browser/autocomp... ios/chrome/browser/autocomplete/autocomplete_provider_client_impl.cc:121: std::vector<base::string16> AutocompleteProviderClientImpl::GetBuiltinURLs() { On 2017/06/19 21:00:43, Peter Kasting wrote: > It looks like these two functions are basically copies of the ones in > ChromeAutocompleteProviderClient with the "if !ANDROID" stuff removed. > > Is there any place we can put this code that will be shared so we don't need to > duplicate it? The Client interfaces exist to allow for embedder-specific logic, so almost by definition there shouldn't be much shared code in these functions. We could rework the interface to move more of the duplicated code into the callers. In this specific case, the std::sort() could move into the BuiltinProvider constructor. The rest of the duplicated code is just converting from std::string to string16. My take is that this code isn't really interesting enough (one sort, one string conversion) to worry about deduplicating. Let me know if you feel differently. https://codereview.chromium.org/2947643002/diff/1/ios/chrome/browser/autocomp... ios/chrome/browser/autocomplete/autocomplete_provider_client_impl.cc:128: for (std::vector<std::string>::iterator i(chrome_builtins.begin()); On 2017/06/19 21:00:43, Peter Kasting wrote: > Nit: Range-based for can help here Done. https://codereview.chromium.org/2947643002/diff/1/ios/chrome/browser/autocomp... ios/chrome/browser/autocomplete/autocomplete_provider_client_impl.cc:139: return builtins_to_provide; On 2017/06/19 21:00:43, Peter Kasting wrote: > Nit: I bet you could just do: > > return {base::ASCIIToUTF16(kChromeUIChromeURLsURL), > base::ASCIIToUTF16(kChromeUIVersionURL)}; Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/06/20 12:08:20, rohitrao (ping after 24h) wrote: > On 2017/06/19 14:24:45, Justin Donnelly wrote: > If I added a comment pointing to the iOS version of these constants, would that > suffice to make it clear that iOS is using different values? Ah, I should have realized these were different values. I don't think a comment is necessary.
lgtm https://codereview.chromium.org/2947643002/diff/1/ios/chrome/browser/autocomp... File ios/chrome/browser/autocomplete/autocomplete_provider_client_impl.cc (right): https://codereview.chromium.org/2947643002/diff/1/ios/chrome/browser/autocomp... ios/chrome/browser/autocomplete/autocomplete_provider_client_impl.cc:121: std::vector<base::string16> AutocompleteProviderClientImpl::GetBuiltinURLs() { On 2017/06/20 12:08:20, rohitrao (ping after 24h) wrote: > My take is that this code isn't really interesting enough (one sort, one string > conversion) to worry about deduplicating. Let me know if you feel differently. I had the same reaction as Peter (ugh, duplication) but came to the same conclusion as Rohit (there's really very little here that's being duplicated).
https://codereview.chromium.org/2947643002/diff/1/ios/chrome/browser/autocomp... File ios/chrome/browser/autocomplete/autocomplete_provider_client_impl.cc (right): https://codereview.chromium.org/2947643002/diff/1/ios/chrome/browser/autocomp... ios/chrome/browser/autocomplete/autocomplete_provider_client_impl.cc:121: std::vector<base::string16> AutocompleteProviderClientImpl::GetBuiltinURLs() { On 2017/06/20 15:16:39, Justin Donnelly wrote: > On 2017/06/20 12:08:20, rohitrao (ping after 24h) wrote: > > My take is that this code isn't really interesting enough (one sort, one > string > > conversion) to worry about deduplicating. Let me know if you feel > differently. > > I had the same reaction as Peter (ugh, duplication) but came to the same > conclusion as Rohit (there's really very little here that's being duplicated). Is there any reason not to just change the API to fetch an unsorted list of std::strings? It seems like that's just as reasonable of an embedder API and would allow for deduping these bits. I agree that they're minor, but I don't see much win in trying to keep them separate. You're fine to land either way, but that would be my suggestion.
Thanks for the reviews! https://codereview.chromium.org/2947643002/diff/1/ios/chrome/browser/autocomp... File ios/chrome/browser/autocomplete/autocomplete_provider_client_impl.cc (right): https://codereview.chromium.org/2947643002/diff/1/ios/chrome/browser/autocomp... ios/chrome/browser/autocomplete/autocomplete_provider_client_impl.cc:121: std::vector<base::string16> AutocompleteProviderClientImpl::GetBuiltinURLs() { On 2017/06/21 01:10:15, Peter Kasting wrote: > On 2017/06/20 15:16:39, Justin Donnelly wrote: > > On 2017/06/20 12:08:20, rohitrao (ping after 24h) wrote: > > > My take is that this code isn't really interesting enough (one sort, one > > string > > > conversion) to worry about deduplicating. Let me know if you feel > > differently. > > > > I had the same reaction as Peter (ugh, duplication) but came to the same > > conclusion as Rohit (there's really very little here that's being duplicated). > > Is there any reason not to just change the API to fetch an unsorted list of > std::strings? It seems like that's just as reasonable of an embedder API and > would allow for deduping these bits. I agree that they're minor, but I don't > see much win in trying to keep them separate. > > You're fine to land either way, but that would be my suggestion. I'll send this out as a followup; we can look at the diff and see how we feel.
The CQ bit was checked by rohitrao@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2947643002/#ps20001 (title: "Review")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by rohitrao@chromium.org
Description was changed from ========== [ios] Enables the BuiltinProvider. Implements two methods in the iOS AutocompleteProviderClientImpl that provide the list of builtin URLs to the provider. BUG=725120 ========== to ========== [ios] Enables the BuiltinProvider. Implements two methods in the iOS AutocompleteProviderClientImpl that provide the list of builtin URLs to the provider. BUG=725120 TEST=The omnibox should suggest builtin URLs when typing "chrome://" TEST=The omnibox should inline-autocomplete "chrome://version" and "chrome://chrome-urls" ==========
The CQ bit was checked by rohitrao@chromium.org
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": 20001, "attempt_start_ts": 1498058362749360,
"parent_rev": "dd04f963fac90b8d3502d8e96340b07be463b760", "commit_rev":
"4115f4c206ddd94f5ee0674432263e42727de4e5"}
Message was sent while issue was closed.
Description was changed from ========== [ios] Enables the BuiltinProvider. Implements two methods in the iOS AutocompleteProviderClientImpl that provide the list of builtin URLs to the provider. BUG=725120 TEST=The omnibox should suggest builtin URLs when typing "chrome://" TEST=The omnibox should inline-autocomplete "chrome://version" and "chrome://chrome-urls" ========== to ========== [ios] Enables the BuiltinProvider. Implements two methods in the iOS AutocompleteProviderClientImpl that provide the list of builtin URLs to the provider. BUG=725120 TEST=The omnibox should suggest builtin URLs when typing "chrome://" TEST=The omnibox should inline-autocomplete "chrome://version" and "chrome://chrome-urls" Review-Url: https://codereview.chromium.org/2947643002 Cr-Commit-Position: refs/heads/master@{#481238} Committed: https://chromium.googlesource.com/chromium/src/+/4115f4c206ddd94f5ee067443226... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/4115f4c206ddd94f5ee067443226... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
