Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
290333015:
Suggest about:blank autocompletion from BuiltinProvider. (Closed)
|
Created:
6 years, 7 months ago by msw Modified:
6 years, 6 months ago CC:
chromium-reviews, James Su, Mark P Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionSuggest about:blank autocompletion from BuiltinProvider.
Add support for [inline] autocomplete of about:blank.
The support handles fixup of the scheme separator.
Add BuiltinProviderTest.AboutBlank and do related cleanup.
Update OmniboxViewTest.UndoRedo and do related cleanup.
BUG=92577
TEST=about:blank is suggested in the omnibox; unit tests.
R=mpearson@chromium.org,pkasting@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=274083
Patch Set 1 #Patch Set 2 : Refine about:blank suggestion logic. #Patch Set 3 : Add an BuiltinProviderTest.AboutBlank unit test; cleanup. #Patch Set 4 : Additional cleanup. #Patch Set 5 : Sync and rebase. #Patch Set 6 : Fix OmniboxViewTest.UndoRedo, minor cleanup. #
Total comments: 17
Patch Set 7 : Address comments. #
Total comments: 2
Patch Set 8 : Update comment. #
Total comments: 4
Patch Set 9 : Sync and rebase. #Patch Set 10 : Address comment. #Patch Set 11 : Remove a case that fails on Android. #
Messages
Total messages: 31 (0 generated)
Hey Peter, please take a look; thanks!
Why don't you add about:blank to the list of URLs BuiltinProvider can suggest? That seems much more straightforward than special-casing a single suggestion. --mark
On 2014/05/24 14:12:09, Mark P wrote:
> Why don't you add about:blank to the list of URLs BuiltinProvider can suggest?
> That seems much more straightforward than special-casing a single suggestion.
>
> --mark
That's what I attempted to do in the first place, but it's not really that
straightforward.
The |url| for "about:bla" that's been through FixupURL will be corrected to
"chrome://bla", and we actually don't support redirecting chrome://blank to
about:blank (or showing an equivalent blank page). So, I need to check that the
original |text| actually used the about: scheme, still support various scheme
separators (":", ";", ":/", ";/", "://", ";//", ":///", ";///", etc.), and get
the matching input length for the match highlighting and inline autcompletion of
"about:blank". I'll gladly consider any simpler solution you can think of.
I'm going to try to fix the OmniboxViewTest.UndoRedo now:
OmniboxViewTest.UndoRedo (run #1):
[ RUN ] OmniboxViewTest.UndoRedo
...
../../chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:1426: Failure
Value of: omnibox_view->GetText()
Actual: about:blank
Expected: old_text.substr(0, old_text.size() - 2)
Which is: about:bla
../../chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:1430: Failure
Value of: omnibox_view->GetText()
Actual: about:bla
Expected: old_text
Which is: about:blank
[ FAILED ] OmniboxViewTest.UndoRedo, where TypeParam = and GetParam() = (396
ms)
Try adding chrome://blank to the list of the URLs that can be suggested. I still fundamentally don't see why this is different than any other URL, say, chrome://omnibox/. It gets suggested using about:om and chrome://om and so on with the various fixup things such as chrome:om. On Thu, May 29, 2014 at 2:56 PM, <msw@chromium.org> wrote: > On 2014/05/24 14:12:09, Mark P wrote: > >> Why don't you add about:blank to the list of URLs BuiltinProvider can >> suggest? >> > > That seems much more straightforward than special-casing a single >> suggestion. >> > > --mark >> > > That's what I attempted to do in the first place, but it's not really that > straightforward. > The |url| for "about:bla" that's been through FixupURL will be corrected to > "chrome://bla", and we actually don't support redirecting chrome://blank to > about:blank (or showing an equivalent blank page). So, I need to check > that the > original |text| actually used the about: scheme, still support various > scheme > separators (":", ";", ":/", ";/", "://", ";//", ":///", ";///", etc.), > and get > the matching input length for the match highlighting and inline > autcompletion of > "about:blank". I'll gladly consider any simpler solution you can think of. > > I'm going to try to fix the OmniboxViewTest.UndoRedo now: > > OmniboxViewTest.UndoRedo (run #1): > [ RUN ] OmniboxViewTest.UndoRedo > ... > ../../chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:1426: Failure > Value of: omnibox_view->GetText() > Actual: about:blank > Expected: old_text.substr(0, old_text.size() - 2) > Which is: about:bla > ../../chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:1430: Failure > Value of: omnibox_view->GetText() > Actual: about:bla > Expected: old_text > Which is: about:blank > [ FAILED ] OmniboxViewTest.UndoRedo, where TypeParam = and GetParam() = > (396 > ms) > > https://codereview.chromium.org/290333015/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Thu, May 29, 2014 at 3:04 PM, Mark Pearson <mpearson@chromium.org> wrote: > > Try adding chrome://blank to the list of the URLs that can be suggested. > > I still fundamentally don't see why this is different than any other URL, > say, chrome://omnibox/. It gets suggested using about:om and chrome://om > and so on with the various fixup things such as chrome:om. > Ignore my stupid comment. I didn't read your message fully. Here's a possibly better/simpler answer than what you're doing: make a chrome://blank. --mark > > > > On Thu, May 29, 2014 at 2:56 PM, <msw@chromium.org> wrote: > >> On 2014/05/24 14:12:09, Mark P wrote: >> >>> Why don't you add about:blank to the list of URLs BuiltinProvider can >>> suggest? >>> >> >> That seems much more straightforward than special-casing a single >>> suggestion. >>> >> >> --mark >>> >> >> That's what I attempted to do in the first place, but it's not really that >> straightforward. >> The |url| for "about:bla" that's been through FixupURL will be corrected >> to >> "chrome://bla", and we actually don't support redirecting chrome://blank >> to >> about:blank (or showing an equivalent blank page). So, I need to check >> that the >> original |text| actually used the about: scheme, still support various >> scheme >> separators (":", ";", ":/", ";/", "://", ";//", ":///", ";///", etc.), >> and get >> the matching input length for the match highlighting and inline >> autcompletion of >> "about:blank". I'll gladly consider any simpler solution you can think of. >> >> I'm going to try to fix the OmniboxViewTest.UndoRedo now: >> >> OmniboxViewTest.UndoRedo (run #1): >> [ RUN ] OmniboxViewTest.UndoRedo >> ... >> ../../chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:1426: Failure >> Value of: omnibox_view->GetText() >> Actual: about:blank >> Expected: old_text.substr(0, old_text.size() - 2) >> Which is: about:bla >> ../../chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:1430: Failure >> Value of: omnibox_view->GetText() >> Actual: about:bla >> Expected: old_text >> Which is: about:blank >> [ FAILED ] OmniboxViewTest.UndoRedo, where TypeParam = and GetParam() >> = (396 >> ms) >> >> https://codereview.chromium.org/290333015/ >> > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/05/29 22:04:42, Mark P wrote: > Try adding chrome://blank to the list of the URLs that can be suggested. > > I still fundamentally don't see why this is different than any other URL, > say, chrome://omnibox/. It gets suggested using about:om and chrome://om > and so on with the various fixup things such as chrome:om. But, as I mentioned, we don't support chrome://blank, and this doesn't intend to add support for typing chrome:blank. So, to implement your suggestion, I'd (1) add chrome://blank to the list of supported hosts even though it's not really supported (2) when the general match construction block produces chrome://blank, I'll need to (2a) check that the text input used the about scheme, not the chrome scheme and bail if chrome was used (2b) check that there is not path or trailing slash, since these aren't supported either for this specific case (2c) manually un-fixup the match's scheme from chrome to about and (2d) manually fixup the length for match highlighting and in-lining. This would complicate the general match construction block, by essentially moving all the logic I'm implementing there. That doesn't seem cleaner or simpler at all, but feel free to patch this in yourself and correct me or improve on my pattern.
On Thu, May 29, 2014 at 3:18 PM, <msw@chromium.org> wrote: > On 2014/05/29 22:04:42, Mark P wrote: > >> Try adding chrome://blank to the list of the URLs that can be suggested. >> > > I still fundamentally don't see why this is different than any other URL, >> say, chrome://omnibox/. It gets suggested using about:om and chrome://om >> and so on with the various fixup things such as chrome:om. >> > > But, as I mentioned, we don't support chrome://blank and this doesn't intend to > add support for typing chrome:blank. Why is this a bad option? Why can't we change how about:blank is implemented to make it identical to all the other local chrome scheme + host destinations? > So, to implement your suggestion, I'd (1) > add chrome://blank to the list of supported hosts even though it's not > really > supported (2) when the general match construction block produces > chrome://blank, > I'll need to (2a) check that the text input used the about scheme, not the > chrome scheme and bail if chrome was used (2b) check that there is not > path or > trailing slash, since these aren't supported either for this specific case > (2c) > manually un-fixup the match's scheme from chrome to about and (2d) manually > fixup the length for match highlighting and in-lining. This would > complicate the > general match construction block, by essentially moving all the logic I'm > implementing there. That doesn't seem cleaner or simpler at all, but feel > free > to patch this in yourself and correct me or improve on my pattern. > > https://codereview.chromium.org/290333015/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/05/29 22:10:40, Mark P wrote: > Ignore my stupid comment. I didn't read your message fully. > > Here's a possibly better/simpler answer than what you're doing: make a > chrome://blank. Chrome has many exceptions (some security-related, iirc) for the about:blank URL, and supporting chrome:blank in the same way isn't necessarily feasible or desirable. I can't find more specific information without digging around, but Perhaps Peter can elaborate, or maybe you'll see something in the existing uses: https://code.google.com/p/chromium/codesearch#search/&q=kAboutBlankURL%20-fil...
Yeah, in general about:blank is a very special URL for security and UI. I don't really know the details either but I'm pretty sure we can't change how it's handled to match other chrome:// pages. I think special-casing about:blank here is the only feasible option, unfortunately.
Test updated, please take a look; thanks!
https://codereview.chromium.org/290333015/diff/160001/chrome/browser/autocomp... File chrome/browser/autocomplete/builtin_provider.cc (right): https://codereview.chromium.org/290333015/diff/160001/chrome/browser/autocomp... chrome/browser/autocomplete/builtin_provider.cc:108: // Suggest about:blank for substrings, taking URL fixup into account. Note that this was of doing it will put about:blank before any other BuiltinProvider inputs that match the input. i.e., if there were an about:bee, the matches would no longer be alphabetic as they were before. I don't think this is an issue now. Regardless, consider whether it's worth a comment or a minor code change to leave things alphabetical. https://codereview.chromium.org/290333015/diff/160001/chrome/browser/autocomp... chrome/browser/autocomplete/builtin_provider.cc:112: StartsWith(blank_host, host, false) && url.path().length() <= 1 && nit: parens around binary operator <= https://codereview.chromium.org/290333015/diff/160001/chrome/browser/autocomp... chrome/browser/autocomplete/builtin_provider.cc:113: !EndsWith(text, base::ASCIIToUTF16("/"), false)) { please comment on the need for this final EndsWith clause. https://codereview.chromium.org/290333015/diff/160001/chrome/browser/autocomp... chrome/browser/autocomplete/builtin_provider.cc:117: const size_t corrected_input_length = 6 + host.length(); comment on the "6" (and if it's 6 because this is the length of "about:" then why don't you actually do length of kAboutScheme + ":"?) https://codereview.chromium.org/290333015/diff/160001/chrome/browser/autocomp... File chrome/browser/autocomplete/builtin_provider_unittest.cc (right): https://codereview.chromium.org/290333015/diff/160001/chrome/browser/autocomp... chrome/browser/autocomplete/builtin_provider_unittest.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. Thank you for simplifying this file. https://codereview.chromium.org/290333015/diff/160001/chrome/browser/autocomp... chrome/browser/autocomplete/builtin_provider_unittest.cc:39: base::string16(), GURL(), AutocompleteInput::INVALID_SPEC, true, nit: indentation https://codereview.chromium.org/290333015/diff/160001/chrome/browser/autocomp... chrome/browser/autocomplete/builtin_provider_unittest.cc:240: const GURL kURLB2 = GURL(kChrome + kSeparator1 + kHostB2); why don't you swap b1 and b2 so you don't have everything out of order below? https://codereview.chromium.org/290333015/diff/160001/chrome/browser/autocomp... chrome/browser/autocomplete/builtin_provider_unittest.cc:245: {kAboutBlank, 0, {}}, There's nothing wrong with BuiltinProvider suggesting the exact match. I don't see why this is worth testing/enforcing.
Comments addressed, please take another look; thanks! https://codereview.chromium.org/290333015/diff/160001/chrome/browser/autocomp... File chrome/browser/autocomplete/builtin_provider.cc (right): https://codereview.chromium.org/290333015/diff/160001/chrome/browser/autocomp... chrome/browser/autocomplete/builtin_provider.cc:108: // Suggest about:blank for substrings, taking URL fixup into account. On 2014/05/29 23:48:05, Mark P wrote: > Note that this was of doing it will put about:blank before any other > BuiltinProvider inputs that match the input. i.e., if there were an about:bee, > the matches would no longer be alphabetic as they were before. > I don't think this is an issue now. Regardless, consider whether it's worth a > comment or a minor code change to leave things alphabetical. It might be considered alphabetical if you're comparing "about:blank" to "chrome://blob-internals" and "chrome://bookmarks". I don't think it's really worth a code comment. https://codereview.chromium.org/290333015/diff/160001/chrome/browser/autocomp... chrome/browser/autocomplete/builtin_provider.cc:112: StartsWith(blank_host, host, false) && url.path().length() <= 1 && On 2014/05/29 23:48:05, Mark P wrote: > nit: parens around binary operator <= Done. https://codereview.chromium.org/290333015/diff/160001/chrome/browser/autocomp... chrome/browser/autocomplete/builtin_provider.cc:113: !EndsWith(text, base::ASCIIToUTF16("/"), false)) { On 2014/05/29 23:48:05, Mark P wrote: > please comment on the need for this final EndsWith clause. Done. https://codereview.chromium.org/290333015/diff/160001/chrome/browser/autocomp... chrome/browser/autocomplete/builtin_provider.cc:117: const size_t corrected_input_length = 6 + host.length(); On 2014/05/29 23:48:05, Mark P wrote: > comment on the "6" > (and if it's 6 because this is the length of "about:" then why don't you > actually do length of kAboutScheme + ":"?) Done (used kAboutSchemeLength + 1) and added a comment. https://codereview.chromium.org/290333015/diff/160001/chrome/browser/autocomp... File chrome/browser/autocomplete/builtin_provider_unittest.cc (right): https://codereview.chromium.org/290333015/diff/160001/chrome/browser/autocomp... chrome/browser/autocomplete/builtin_provider_unittest.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2014/05/29 23:48:05, Mark P wrote: > Thank you for simplifying this file. :) https://codereview.chromium.org/290333015/diff/160001/chrome/browser/autocomp... chrome/browser/autocomplete/builtin_provider_unittest.cc:39: base::string16(), GURL(), AutocompleteInput::INVALID_SPEC, true, On 2014/05/29 23:48:05, Mark P wrote: > nit: indentation Done. https://codereview.chromium.org/290333015/diff/160001/chrome/browser/autocomp... chrome/browser/autocomplete/builtin_provider_unittest.cc:240: const GURL kURLB2 = GURL(kChrome + kSeparator1 + kHostB2); On 2014/05/29 23:48:05, Mark P wrote: > why don't you swap b1 and b2 so you don't have everything out of order below? Done. https://codereview.chromium.org/290333015/diff/160001/chrome/browser/autocomp... chrome/browser/autocomplete/builtin_provider_unittest.cc:245: {kAboutBlank, 0, {}}, On 2014/05/29 23:48:05, Mark P wrote: > There's nothing wrong with BuiltinProvider suggesting the exact match. I don't > see why this is worth testing/enforcing. Agreed, I removed this test case and adjusted the similar ones below and added a new comment there.
https://codereview.chromium.org/290333015/diff/160001/chrome/browser/autocomp... File chrome/browser/autocomplete/builtin_provider.cc (right): https://codereview.chromium.org/290333015/diff/160001/chrome/browser/autocomp... chrome/browser/autocomplete/builtin_provider.cc:108: // Suggest about:blank for substrings, taking URL fixup into account. On 2014/05/30 00:29:49, msw wrote: > On 2014/05/29 23:48:05, Mark P wrote: > > Note that this was of doing it will put about:blank before any other > > BuiltinProvider inputs that match the input. i.e., if there were an > about:bee, > > the matches would no longer be alphabetic as they were before. > > I don't think this is an issue now. Regardless, consider whether it's worth a > > comment or a minor code change to leave things alphabetical. > > It might be considered alphabetical if you're comparing "about:blank" to > "chrome://blob-internals" and "chrome://bookmarks". I don't think it's really > worth a code comment. Okay. https://codereview.chromium.org/290333015/diff/170001/chrome/browser/autocomp... File chrome/browser/autocomplete/builtin_provider.cc (right): https://codereview.chromium.org/290333015/diff/170001/chrome/browser/autocomp... chrome/browser/autocomplete/builtin_provider.cc:109: // This match does not allow any trailing path or slash in the input. On 2014/05/30 00:29:49, msw wrote: > On 2014/05/29 23:48:05, Mark P wrote: > > please comment on the need for this final EndsWith clause. > > Done. This comment restates the test. I want to know why this test exists, because the other builtin provider suggestions allow a trailing slash.
Please take another look; thanks. https://codereview.chromium.org/290333015/diff/170001/chrome/browser/autocomp... File chrome/browser/autocomplete/builtin_provider.cc (right): https://codereview.chromium.org/290333015/diff/170001/chrome/browser/autocomp... chrome/browser/autocomplete/builtin_provider.cc:109: // This match does not allow any trailing path or slash in the input. On 2014/05/30 18:16:41, Mark P wrote: > On 2014/05/30 00:29:49, msw wrote: > > On 2014/05/29 23:48:05, Mark P wrote: > > > please comment on the need for this final EndsWith clause. > > > > Done. > > This comment restates the test. I want to know why this test exists, because > the other builtin provider suggestions allow a trailing slash. Done.
lgtm
TBR'ing peter for omnibox_view_browsertest.cc and landing.
The CQ bit was checked by msw@chromium.org
LGTM https://codereview.chromium.org/290333015/diff/190001/chrome/browser/ui/omnib... File chrome/browser/ui/omnibox/omnibox_view_browsertest.cc (right): https://codereview.chromium.org/290333015/diff/190001/chrome/browser/ui/omnib... chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:1405: // Delete three characters; "about:bl" should not trigger inline autocomplete. Hmm... I wonder if it would be more future-proof to delete all the way back to "abou" or something. https://codereview.chromium.org/290333015/diff/190001/chrome/browser/ui/omnib... chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:1443: // Backspace should delete one character; see http://crbug.com/192743 Nit: I'd omit the bug reference.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/msw@chromium.org/290333015/190001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_chromium_gn_compile_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...) android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...) chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...) linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...) linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...) chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...) linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...) linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
Comment addressed and synced, landing. https://codereview.chromium.org/290333015/diff/190001/chrome/browser/ui/omnib... File chrome/browser/ui/omnibox/omnibox_view_browsertest.cc (right): https://codereview.chromium.org/290333015/diff/190001/chrome/browser/ui/omnib... chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:1405: // Delete three characters; "about:bl" should not trigger inline autocomplete. On 2014/05/30 21:06:16, Peter Kasting wrote: > Hmm... I wonder if it would be more future-proof to delete all the way back to > "abou" or something. This works for now, and it shouldn't be hard to fix as needed later. https://codereview.chromium.org/290333015/diff/190001/chrome/browser/ui/omnib... chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:1443: // Backspace should delete one character; see http://crbug.com/192743 On 2014/05/30 21:06:16, Peter Kasting wrote: > Nit: I'd omit the bug reference. Done.
The CQ bit was checked by msw@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/msw@chromium.org/290333015/230001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
The CQ bit was checked by msw@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/msw@chromium.org/290333015/230002
Message was sent while issue was closed.
Change committed as 274083 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
