Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1855423003:
Interpret '?' and Ctrl-K or Ctrl-E as putting omnibox in keyword search mode for Default Search Pro… (Closed)
|
Created:
4 years, 8 months ago by Tom (Use chromium acct) Modified:
4 years, 6 months ago CC:
chromium-reviews, James Su, tfarina Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionInterpret '?' and Ctrl-K or Ctrl-E as putting omnibox in keyword search mode for Default Search Provider.
When the user presses Ctrl-K or Ctrl-E:
Transition the user into keyword mode using their default search provider. Pressing Ctrl-K will give the omnibox focus, and will display "Search Google:" on the left. If the user has already entered text into the omnibox and presses Ctrl-K, their text will be preserved, but they will be transitioned into keyword mode anyway. If the user is already in keyword mode (even if it's with a non-default search provider), the text they have entered will become selected. Upon pressing backspace, they will be left with an empty omnibox.
When the user types '?' into the omnibox:
If the user types or pastes a '?' at the beginning of their (possibly empty) input text, transition them into keyword mode with their default search provider. Upon pressing backspace, the omnibox will be left '?' (and their input).
Remove forced queries. We should be able to search for '?' directly.
BUG=267629, 592540
Committed: https://crrev.com/00687d006b3b838d1274f52a2dee455ce988d2fd
Cr-Commit-Position: refs/heads/master@{#398580}
Patch Set 1 #Patch Set 2 : Added method stubs in omnibox_edit_unittest.cc #Patch Set 3 : Changed MOCK_METHOD0 to MOCK_METHOD1 in autocomplete_text_field_unittest_helper.h #Patch Set 4 : Removed complex last_keystroke_was_question_mark logic, modified browsertest for focus search to re… #Patch Set 5 : Reverted autocomplete_text_field* #
Total comments: 49
Patch Set 6 : Removed forced queries using '?'. Removed Ctrl-K preserving the user's keyword if they're already … #
Total comments: 29
Patch Set 7 : Refactored #Patch Set 8 : Fixed a unit test compilation #Patch Set 9 : Fixed compilation on Mac and removed forced query unit tests #
Total comments: 13
Patch Set 10 : Refactored #Patch Set 11 : Remove calls to InternalSetUserText #Patch Set 12 : Fixed unit test compilation #
Total comments: 18
Patch Set 13 : Fix nits #Patch Set 14 : Rebase, make '?' enter keyword mode on nonempty inputs #
Total comments: 13
Patch Set 15 : Remove SetCaretPos #Patch Set 16 : Moved ? browser test to interactive ui test #
Total comments: 2
Patch Set 17 : Deprecated forced query in omnibox_input_type.proto #Patch Set 18 : Add case for deprecated forced query #Patch Set 19 : Removed call to StartAutocomplete and move caret to beginning when typing ? #Patch Set 20 : Really removed call to UpdatePopup #
Total comments: 13
Patch Set 21 : Factor out some code into OmniboxView and fix nits #
Total comments: 14
Patch Set 22 : Refactor State stuff #Patch Set 23 : Fix unit test compilation #Patch Set 24 : Fix bug in OmniboxView::GetStateChanges #
Total comments: 1
Patch Set 25 : Moved OmniboxViewStateChanges into OmniboxView::StateChanges #Patch Set 26 : Added includes for omnibox_edit_model.h #Patch Set 27 : Add SkColor include #Patch Set 28 : Add includes for mac tests #Messages
Total messages: 191 (79 generated)
thomasanderson@google.com changed reviewers: + pkasting@chromium.org
The CQ bit was checked by thomasanderson@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1855423003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1855423003/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
The CQ bit was checked by thomasanderson@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1855423003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1855423003/20001
The CQ bit was checked by thomasanderson@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1855423003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1855423003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...) linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by thomasanderson@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1855423003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1855423003/40001
The CQ bit was unchecked by thomasanderson@google.com
The CQ bit was checked by thomasanderson@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1855423003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1855423003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Can you write up on the CL description the various behaviors this implements, including e.g. backspacing out of keyword mode in the various cases? We discussed this some in person, but that will be useful both for future code spelunking looking at this CL, as well as giving me something to refer back to repeatedly when looking closely at the actual behavioral impacts.
Also, the test failure here is real, so you'll want to fix that.
Description was changed from ========== Interpret '?' and Ctrl-K or Ctrl-E as putting omnibox in keyword search mode for Default Search Provider. BUG=267629 ========== to ========== Interpret '?' and Ctrl-K or Ctrl-E as putting omnibox in keyword search mode for Default Search Provider. When the user presses Ctrl-K or Ctrl-E: Transition the user into keyword mode using their default search provider. Pressing Ctrl-K will give the omnibox focus, and will display "Search Google:" on the left. If the user has already entered text into the omnibox and presses Ctrl-K, their text will be preserved, but they will be transitioned into keyword mode anyway. If the user is already in keyword mode (even if it's with a non-default search provider), the text they have entered will become selected. Upon pressing backspace, they will be left with an empty omnibox. When the user types '?' into the omnibox: If the omnibox is currently filled with zero or more whitespaces and the user types '?', they will be transitioned into keyword mode with their default search provider. Pasting in a '?' will not enter keyword mode. Upon pressing backspace, the omnibox will be left with a single '?' with no whitespaces, even if there were whitespaces when the user typed the '?'. This may or may not be correct behavior. If it isn't, we could do one of the following: 1. Remember the '?' and whitespaces so we can restore it upon pressing backspace. 2. Only transition into keyword mode when the user types '?' on a blank omnibox. 3. Ignore this uncommon case BUG=267629 ==========
The CQ bit was checked by thomasanderson@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1855423003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1855423003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by thomasanderson@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1855423003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1855423003/60001
Description was changed from ========== Interpret '?' and Ctrl-K or Ctrl-E as putting omnibox in keyword search mode for Default Search Provider. When the user presses Ctrl-K or Ctrl-E: Transition the user into keyword mode using their default search provider. Pressing Ctrl-K will give the omnibox focus, and will display "Search Google:" on the left. If the user has already entered text into the omnibox and presses Ctrl-K, their text will be preserved, but they will be transitioned into keyword mode anyway. If the user is already in keyword mode (even if it's with a non-default search provider), the text they have entered will become selected. Upon pressing backspace, they will be left with an empty omnibox. When the user types '?' into the omnibox: If the omnibox is currently filled with zero or more whitespaces and the user types '?', they will be transitioned into keyword mode with their default search provider. Pasting in a '?' will not enter keyword mode. Upon pressing backspace, the omnibox will be left with a single '?' with no whitespaces, even if there were whitespaces when the user typed the '?'. This may or may not be correct behavior. If it isn't, we could do one of the following: 1. Remember the '?' and whitespaces so we can restore it upon pressing backspace. 2. Only transition into keyword mode when the user types '?' on a blank omnibox. 3. Ignore this uncommon case BUG=267629 ========== to ========== Interpret '?' and Ctrl-K or Ctrl-E as putting omnibox in keyword search mode for Default Search Provider. When the user presses Ctrl-K or Ctrl-E: Transition the user into keyword mode using their default search provider. Pressing Ctrl-K will give the omnibox focus, and will display "Search Google:" on the left. If the user has already entered text into the omnibox and presses Ctrl-K, their text will be preserved, but they will be transitioned into keyword mode anyway. If the user is already in keyword mode (even if it's with a non-default search provider), the text they have entered will become selected. Upon pressing backspace, they will be left with an empty omnibox. When the user types '?' into the omnibox: If the omnibox is currently empty and the user types '?' (or, the user has the entire text selected and overwrites it with '?', or they paste in '?'), they will be transitioned into keyword mode with their default search provider. Upon pressing backspace, the omnibox will be left with a single '?'. BUG=267629 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by thomasanderson@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1855423003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1855423003/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
- Cleaned up some complex logic - Changed issue description - Fixed test failure
"If the user is already in keyword mode (even if it's with a non-default search provider), the text they have entered will become selected." Does this mean we'll change over to the DSP and select the text, or we'll leave the user with their existing keyword and select the text? The latter seems wrong.
If I type '?', hit backspace to get the question mark back, and then type "foo.com", do I search for "foo.com" or "?foo.com"? It ought to be the latter, and we ought to have a test for this. I don't think your code does this because you didn't touch AutocompleteInput::Parse().
https://codereview.chromium.org/1855423003/diff/80001/chrome/browser/autocomp... File chrome/browser/autocomplete/autocomplete_browsertest.cc (right): https://codereview.chromium.org/1855423003/diff/80001/chrome/browser/autocomp... chrome/browser/autocomplete/autocomplete_browsertest.cc:202: Nit: Don't add these https://codereview.chromium.org/1855423003/diff/80001/chrome/browser/autocomp... chrome/browser/autocomplete/autocomplete_browsertest.cc:218: base::string16 dsp = Nit: Since this is really a keyword and not a provider itself, and since we generally avoid abbreviations, I would name this |default_search_keyword|. https://codereview.chromium.org/1855423003/diff/80001/chrome/browser/autocomp... chrome/browser/autocomplete/autocomplete_browsertest.cc:221: const char query_text[] = "foo"; Every place below that wants this wants it as a string16. So make it a const base::string16. Then you can eliminate the |query_text_len| temp and just call .length() on this. https://codereview.chromium.org/1855423003/diff/80001/chrome/browser/autocomp... chrome/browser/autocomplete/autocomplete_browsertest.cc:226: #define FOCUS_SEARCH_CHECK_PRECONDITIONS() \ We try to avoid macros if possible. Why not put this stuff in a helper function? https://codereview.chromium.org/1855423003/diff/80001/chrome/browser/autocomp... chrome/browser/autocomplete/autocomplete_browsertest.cc:336: selection_start : selection_end); Nit: Use std::min() https://codereview.chromium.org/1855423003/diff/80001/chrome/browser/autocomp... chrome/browser/autocomplete/autocomplete_browsertest.cc:339: selection_start : selection_end); Nit: Use std::max() https://codereview.chromium.org/1855423003/diff/80001/chrome/browser/autocomp... chrome/browser/autocomplete/autocomplete_browsertest.cc:344: // If the user got into keyword mode using a keyboard shortcut, and presses Nit: Tense agreement: got -> gets (here and below) https://codereview.chromium.org/1855423003/diff/80001/chrome/browser/autocomp... chrome/browser/autocomplete/autocomplete_browsertest.cc:351: // the user presses Ctrl-K Nit: Capitalization, trailing period (multiple places) https://codereview.chromium.org/1855423003/diff/80001/chrome/browser/autocomp... chrome/browser/autocomplete/autocomplete_browsertest.cc:354: omnibox_model->ClearKeyword(); Can we just send a backspace keypress? That ensures the keystroke is hooked to the right method. (2 places) https://codereview.chromium.org/1855423003/diff/80001/chrome/browser/autocomp... chrome/browser/autocomplete/autocomplete_browsertest.cc:366: // keyword mode with their dsp. If they press backspace, they should be left Nit: with their dsp -> for their default search provider https://codereview.chromium.org/1855423003/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm (right): https://codereview.chromium.org/1855423003/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:361: // using their default search platform. Nit: platform -> provider https://codereview.chromium.org/1855423003/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right): https://codereview.chromium.org/1855423003/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_view_views.cc:283: // using their default search platform. Nit: platform -> provider https://codereview.chromium.org/1855423003/diff/80001/components/omnibox/brow... File components/omnibox/browser/omnibox_edit_model.cc (right): https://codereview.chromium.org/1855423003/diff/80001/components/omnibox/brow... components/omnibox/browser/omnibox_edit_model.cc:48: Nit: Don't add this https://codereview.chromium.org/1855423003/diff/80001/components/omnibox/brow... components/omnibox/browser/omnibox_edit_model.cc:338: if (user_text_ == base::ASCIIToUTF16("?")) { Nit: Combine these ifs https://codereview.chromium.org/1855423003/diff/80001/components/omnibox/brow... components/omnibox/browser/omnibox_edit_model.cc:648: Nit: I'd remove this blank line https://codereview.chromium.org/1855423003/diff/80001/components/omnibox/brow... components/omnibox/browser/omnibox_edit_model.cc:879: view_->OnBeforePossibleChange(); This will result in calling this in the "state 4" case below, which is bad. https://codereview.chromium.org/1855423003/diff/80001/components/omnibox/brow... components/omnibox/browser/omnibox_edit_model.cc:893: const char* clear_text = nullptr; Use a std::string in preference to char*. Or better yet, avoid the switch and the temp by restructuring like this: if (keyword_mode_entry_method == KeywordModeEntryMethod::KEYBOARD_SHORTCUT || keyword_mode_entry_method == KeywordModeEntryMethod::QUESTION_MARK) { if (keyword_mode_entry_method == KeywordModeEntryMethod::QUESTION_MARK) view_->SetWindowTextAndCaretPos('?' + view_->GetText(), 1, false, false); else view_->SetWindowTextAndCaretPos(view_->GetText(), 0, false, false); ... } https://codereview.chromium.org/1855423003/diff/80001/components/omnibox/brow... components/omnibox/browser/omnibox_edit_model.cc:962: view_->OnBeforePossibleChange(); This will result in calling this twice, which is bad. https://codereview.chromium.org/1855423003/diff/80001/components/omnibox/brow... File components/omnibox/browser/omnibox_edit_model.h (right): https://codereview.chromium.org/1855423003/diff/80001/components/omnibox/brow... components/omnibox/browser/omnibox_edit_model.h:39: enum EnteredKeywordModeMethod { Nit: I suggest changing this to an enum class, renaming it KeywordModeEntryMethod, and eliminating the "ENTERED_KEYWORD_MODE_VIA_" on all the values. Then you can use |keyword_mode_entry_method| for variable/param names to match the enum name. https://codereview.chromium.org/1855423003/diff/80001/components/omnibox/brow... components/omnibox/browser/omnibox_edit_model.h:247: // Set the current keyword to that of the user's default search provider and Nit: Function comments should be declarative, not imperative. https://codereview.chromium.org/1855423003/diff/80001/components/omnibox/brow... components/omnibox/browser/omnibox_edit_model.h:248: // update the view so the user sees "Search Google:" in the omnibox. Nit: Don't say "Search Google" since that's not accurate. Instead say something like "...so the user sees the keyword chip in the omnibox." https://codereview.chromium.org/1855423003/diff/80001/components/omnibox/brow... components/omnibox/browser/omnibox_edit_model.h:249: void SetKeywordWithDefaultSearchProvider( Nit: Maybe "EnterKeywordModeForDefaultSearchProvider()"? https://codereview.chromium.org/1855423003/diff/80001/components/omnibox/brow... File components/omnibox/browser/omnibox_view.h (right): https://codereview.chromium.org/1855423003/diff/80001/components/omnibox/brow... components/omnibox/browser/omnibox_view.h:101: // Transition the user into keyword mode with their default search provider. Nit: Function comments should be declarative, not imperative.
On 2016/04/08 00:12:42, Peter Kasting wrote: > "If the user is already in keyword mode (even if it's with a non-default search > provider), the text they have entered will become selected." > > Does this mean we'll change over to the DSP and select the text, or we'll leave > the user with their existing keyword and select the text? The latter seems > wrong. Currently, we'll keep the existing keyword and select the text, but this can change. On 2016/04/08 00:22:31, Peter Kasting wrote: > If I type '?', hit backspace to get the question mark back, and then type > "foo.com", do I search for "foo.com" or "?foo.com"? It ought to be the latter, > and we ought to have a test for this. I don't think your code does this because > you didn't touch AutocompleteInput::Parse(). You would search for "foo.com", but this can change as well. https://codereview.chromium.org/1855423003/diff/80001/chrome/browser/autocomp... File chrome/browser/autocomplete/autocomplete_browsertest.cc (right): https://codereview.chromium.org/1855423003/diff/80001/chrome/browser/autocomp... chrome/browser/autocomplete/autocomplete_browsertest.cc:226: #define FOCUS_SEARCH_CHECK_PRECONDITIONS() \ On 2016/04/08 00:39:55, Peter Kasting wrote: > We try to avoid macros if possible. > > Why not put this stuff in a helper function? I originally had it as a helper function, but the line number that the EXPECT_* printed out was useless. Having it as a macro gives correct line numbers, but I agree that a function is probably the right way to go.
On 2016/04/08 01:13:55, Tom Anderson wrote: > On 2016/04/08 00:12:42, Peter Kasting wrote: > > "If the user is already in keyword mode (even if it's with a non-default > search > > provider), the text they have entered will become selected." > > > > Does this mean we'll change over to the DSP and select the text, or we'll > leave > > the user with their existing keyword and select the text? The latter seems > > wrong. > > Currently, we'll keep the existing keyword and select the text, > but this can change. > > On 2016/04/08 00:22:31, Peter Kasting wrote: > > If I type '?', hit backspace to get the question mark back, and then type > > "foo.com", do I search for "foo.com" or "?foo.com"? It ought to be the > latter, > > and we ought to have a test for this. I don't think your code does this > because > > you didn't touch AutocompleteInput::Parse(). > > You would search for "foo.com", but this can change as well. Let's fix both of these. The latter might be tricky depending on how we normally handle keyword mode (it's been a long time since I looked at how the edit specifies keyword mode for the controller), but it seems like without doing it there's no real benefit to letting users backspace out of '?' mode. > https://codereview.chromium.org/1855423003/diff/80001/chrome/browser/autocomp... > File chrome/browser/autocomplete/autocomplete_browsertest.cc (right): > > https://codereview.chromium.org/1855423003/diff/80001/chrome/browser/autocomp... > chrome/browser/autocomplete/autocomplete_browsertest.cc:226: #define > FOCUS_SEARCH_CHECK_PRECONDITIONS() \ > On 2016/04/08 00:39:55, Peter Kasting wrote: > > We try to avoid macros if possible. > > > > Why not put this stuff in a helper function? > > I originally had it as a helper function, but the line number that the EXPECT_* > printed out was useless. Having it as a macro gives correct line numbers, but I > agree that a function is probably the right way to go. Use a function, but if you need more information when things fail, add SCOPED_TRACEs to the callers describing anything necessary.
The CQ bit was checked by thomasanderson@google.com to run a CQ dry run
https://codereview.chromium.org/1855423003/diff/80001/chrome/browser/autocomp... File chrome/browser/autocomplete/autocomplete_browsertest.cc (right): https://codereview.chromium.org/1855423003/diff/80001/chrome/browser/autocomp... chrome/browser/autocomplete/autocomplete_browsertest.cc:202: On 2016/04/08 00:39:56, Peter Kasting wrote: > Nit: Don't add these Done. https://codereview.chromium.org/1855423003/diff/80001/chrome/browser/autocomp... chrome/browser/autocomplete/autocomplete_browsertest.cc:218: base::string16 dsp = On 2016/04/08 00:39:56, Peter Kasting wrote: > Nit: Since this is really a keyword and not a provider itself, and since we > generally avoid abbreviations, I would name this |default_search_keyword|. Done. https://codereview.chromium.org/1855423003/diff/80001/chrome/browser/autocomp... chrome/browser/autocomplete/autocomplete_browsertest.cc:221: const char query_text[] = "foo"; On 2016/04/08 00:39:56, Peter Kasting wrote: > Every place below that wants this wants it as a string16. So make it a const > base::string16. Then you can eliminate the |query_text_len| temp and just call > .length() on this. Done. https://codereview.chromium.org/1855423003/diff/80001/chrome/browser/autocomp... chrome/browser/autocomplete/autocomplete_browsertest.cc:226: #define FOCUS_SEARCH_CHECK_PRECONDITIONS() \ On 2016/04/08 00:39:55, Peter Kasting wrote: > We try to avoid macros if possible. > > Why not put this stuff in a helper function? Done. https://codereview.chromium.org/1855423003/diff/80001/chrome/browser/autocomp... chrome/browser/autocomplete/autocomplete_browsertest.cc:336: selection_start : selection_end); On 2016/04/08 00:39:56, Peter Kasting wrote: > Nit: Use std::min() Done. https://codereview.chromium.org/1855423003/diff/80001/chrome/browser/autocomp... chrome/browser/autocomplete/autocomplete_browsertest.cc:339: selection_start : selection_end); On 2016/04/08 00:39:56, Peter Kasting wrote: > Nit: Use std::max() Done. https://codereview.chromium.org/1855423003/diff/80001/chrome/browser/autocomp... chrome/browser/autocomplete/autocomplete_browsertest.cc:344: // If the user got into keyword mode using a keyboard shortcut, and presses On 2016/04/08 00:39:56, Peter Kasting wrote: > Nit: Tense agreement: got -> gets > (here and below) Done. https://codereview.chromium.org/1855423003/diff/80001/chrome/browser/autocomp... chrome/browser/autocomplete/autocomplete_browsertest.cc:351: // the user presses Ctrl-K On 2016/04/08 00:39:55, Peter Kasting wrote: > Nit: Capitalization, trailing period (multiple places) Done. https://codereview.chromium.org/1855423003/diff/80001/chrome/browser/autocomp... chrome/browser/autocomplete/autocomplete_browsertest.cc:354: omnibox_model->ClearKeyword(); On 2016/04/08 00:39:55, Peter Kasting wrote: > Can we just send a backspace keypress? That ensures the keystroke is hooked to > the right method. (2 places) Is there a testing API for pressing backspace or moving around using the arrow keys? At the moment, it seems I would have to write platform specific code for mac vs the others. https://codereview.chromium.org/1855423003/diff/80001/chrome/browser/autocomp... chrome/browser/autocomplete/autocomplete_browsertest.cc:366: // keyword mode with their dsp. If they press backspace, they should be left On 2016/04/08 00:39:55, Peter Kasting wrote: > Nit: with their dsp -> for their default search provider Done. https://codereview.chromium.org/1855423003/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm (right): https://codereview.chromium.org/1855423003/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:361: // using their default search platform. On 2016/04/08 00:39:56, Peter Kasting wrote: > Nit: platform -> provider Done. https://codereview.chromium.org/1855423003/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right): https://codereview.chromium.org/1855423003/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_view_views.cc:283: // using their default search platform. On 2016/04/08 00:39:56, Peter Kasting wrote: > Nit: platform -> provider Done. https://codereview.chromium.org/1855423003/diff/80001/components/omnibox/brow... File components/omnibox/browser/omnibox_edit_model.cc (right): https://codereview.chromium.org/1855423003/diff/80001/components/omnibox/brow... components/omnibox/browser/omnibox_edit_model.cc:48: On 2016/04/08 00:39:56, Peter Kasting wrote: > Nit: Don't add this Done. https://codereview.chromium.org/1855423003/diff/80001/components/omnibox/brow... components/omnibox/browser/omnibox_edit_model.cc:338: if (user_text_ == base::ASCIIToUTF16("?")) { On 2016/04/08 00:39:56, Peter Kasting wrote: > Nit: Combine these ifs Done. https://codereview.chromium.org/1855423003/diff/80001/components/omnibox/brow... components/omnibox/browser/omnibox_edit_model.cc:648: On 2016/04/08 00:39:56, Peter Kasting wrote: > Nit: I'd remove this blank line Done. https://codereview.chromium.org/1855423003/diff/80001/components/omnibox/brow... components/omnibox/browser/omnibox_edit_model.cc:879: view_->OnBeforePossibleChange(); On 2016/04/08 00:39:56, Peter Kasting wrote: > This will result in calling this in the "state 4" case below, which is bad. Done. I also moved view_->OnAfterPossibleChange(false) so it isn't called during state 4, can you confirm this is correct? https://codereview.chromium.org/1855423003/diff/80001/components/omnibox/brow... components/omnibox/browser/omnibox_edit_model.cc:893: const char* clear_text = nullptr; On 2016/04/08 00:39:56, Peter Kasting wrote: > Use a std::string in preference to char*. > > Or better yet, avoid the switch and the temp by restructuring like this: > > if (keyword_mode_entry_method == KeywordModeEntryMethod::KEYBOARD_SHORTCUT || > keyword_mode_entry_method == KeywordModeEntryMethod::QUESTION_MARK) { > if (keyword_mode_entry_method == KeywordModeEntryMethod::QUESTION_MARK) > view_->SetWindowTextAndCaretPos('?' + view_->GetText(), 1, false, false); > else > view_->SetWindowTextAndCaretPos(view_->GetText(), 0, false, false); > ... > } Done. https://codereview.chromium.org/1855423003/diff/80001/components/omnibox/brow... File components/omnibox/browser/omnibox_edit_model.h (right): https://codereview.chromium.org/1855423003/diff/80001/components/omnibox/brow... components/omnibox/browser/omnibox_edit_model.h:247: // Set the current keyword to that of the user's default search provider and On 2016/04/08 00:39:56, Peter Kasting wrote: > Nit: Function comments should be declarative, not imperative. Done. https://codereview.chromium.org/1855423003/diff/80001/components/omnibox/brow... components/omnibox/browser/omnibox_edit_model.h:248: // update the view so the user sees "Search Google:" in the omnibox. On 2016/04/08 00:39:56, Peter Kasting wrote: > Nit: Don't say "Search Google" since that's not accurate. Instead say something > like "...so the user sees the keyword chip in the omnibox." Done. https://codereview.chromium.org/1855423003/diff/80001/components/omnibox/brow... components/omnibox/browser/omnibox_edit_model.h:249: void SetKeywordWithDefaultSearchProvider( On 2016/04/08 00:39:56, Peter Kasting wrote: > Nit: Maybe "EnterKeywordModeForDefaultSearchProvider()"? Done. https://codereview.chromium.org/1855423003/diff/80001/components/omnibox/brow... File components/omnibox/browser/omnibox_view.h (right): https://codereview.chromium.org/1855423003/diff/80001/components/omnibox/brow... components/omnibox/browser/omnibox_view.h:101: // Transition the user into keyword mode with their default search provider. On 2016/04/08 00:39:56, Peter Kasting wrote: > Nit: Function comments should be declarative, not imperative. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1855423003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1855423003/100001
https://codereview.chromium.org/1855423003/diff/80001/chrome/browser/autocomp... File chrome/browser/autocomplete/autocomplete_browsertest.cc (right): https://codereview.chromium.org/1855423003/diff/80001/chrome/browser/autocomp... chrome/browser/autocomplete/autocomplete_browsertest.cc:354: omnibox_model->ClearKeyword(); On 2016/04/12 20:03:04, Tom Anderson wrote: > On 2016/04/08 00:39:55, Peter Kasting wrote: > > Can we just send a backspace keypress? That ensures the keystroke is hooked > to > > the right method. (2 places) > > Is there a testing API for pressing backspace or moving around using the arrow > keys? At the moment, it seems I would have to write platform specific code for > mac vs the others. I thought there was an API to send any key by key code?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
https://codereview.chromium.org/1855423003/diff/80001/components/omnibox/brow... File components/omnibox/browser/omnibox_edit_model.h (right): https://codereview.chromium.org/1855423003/diff/80001/components/omnibox/brow... components/omnibox/browser/omnibox_edit_model.h:39: enum EnteredKeywordModeMethod { On 2016/04/08 00:39:56, Peter Kasting wrote: > Nit: I suggest changing this to an enum class, renaming it > KeywordModeEntryMethod, and eliminating the "ENTERED_KEYWORD_MODE_VIA_" on all > the values. > > Then you can use |keyword_mode_entry_method| for variable/param names to match > the enum name. Still applicable. https://codereview.chromium.org/1855423003/diff/80001/components/omnibox/brow... components/omnibox/browser/omnibox_edit_model.h:522: // user would immediately be transitioned back into keyword mode. Nit: Maybe this. I renamed the variable to match how you seem to use it, but see comment in the .cc file also. // Set when the user is in the process of exiting keyword mode. This is // useful to ensure we don't toggle back into keyword mode in the process of // handling the current event. bool clearing_keyword_; https://codereview.chromium.org/1855423003/diff/100001/chrome/browser/autocom... File chrome/browser/autocomplete/autocomplete_browsertest.cc (right): https://codereview.chromium.org/1855423003/diff/100001/chrome/browser/autocom... chrome/browser/autocomplete/autocomplete_browsertest.cc:201: static void focusSearchCheckPreconditions() { Nit: First letter of function name should be capitalized. https://codereview.chromium.org/1855423003/diff/100001/chrome/browser/autocom... chrome/browser/autocomplete/autocomplete_browsertest.cc:202: LocationBar* location_bar = GetLocationBar(); You can't call GetLocationBar() from a non-member function. Make this a class member. https://codereview.chromium.org/1855423003/diff/100001/chrome/browser/autocom... chrome/browser/autocomplete/autocomplete_browsertest.cc:232: base::string16 query_text = ASCIIToUTF16("foo"); Need base:: qualifier https://codereview.chromium.org/1855423003/diff/100001/chrome/browser/autocom... chrome/browser/autocomplete/autocomplete_browsertest.cc:335: EXPECT_EQ(query_text.length(), std::min(selection_start, selection_end)); Shouldn't this be max()? https://codereview.chromium.org/1855423003/diff/100001/components/omnibox/bro... File components/omnibox/browser/autocomplete_input_unittest.cc (right): https://codereview.chromium.org/1855423003/diff/100001/components/omnibox/bro... components/omnibox/browser/autocomplete_input_unittest.cc:289: // regular case, no changes. Nit: While here: Capitalize these https://codereview.chromium.org/1855423003/diff/100001/components/omnibox/bro... components/omnibox/browser/autocomplete_input_unittest.cc:297: // forced query. Nit: Maybe "A leading '?' used to be a magic character indicating the following input should be treated as a "forced query", but now if such a string reaches the AutocompleteInput parser the '?' should just be treated like a normal character." https://codereview.chromium.org/1855423003/diff/100001/components/omnibox/bro... File components/omnibox/browser/autocomplete_result.cc (right): https://codereview.chromium.org/1855423003/diff/100001/components/omnibox/bro... components/omnibox/browser/autocomplete_result.cc:256: // for query inputs. Nit: Move this comment into the first conditional arm, and remove the comma and everything following. https://codereview.chromium.org/1855423003/diff/100001/components/omnibox/bro... File components/omnibox/browser/omnibox_edit_model.cc (right): https://codereview.chromium.org/1855423003/diff/100001/components/omnibox/bro... components/omnibox/browser/omnibox_edit_model.cc:337: user_text_ == base::ASCIIToUTF16("?")) { It doesn't seem like this handles the case of inserting a '?' before an existing string, which it probably should? https://codereview.chromium.org/1855423003/diff/100001/components/omnibox/bro... components/omnibox/browser/omnibox_edit_model.cc:879: just_cleared_keyword_ = true; Nit: Use base::AutoReset here to ensure you set this false on all exit paths, instead of doing it manually. Although... is clearing this on exit to this function safe? Maybe this should persist until the next time something actually changes? What I'm worried about is a case where you backspace out of keyword mode and then hit something that doesn't actually change the text -- like the home or end keys or up arrow or something. This shouldn't suddenly jump back into keyword mode. Make sure that isn't broken... https://codereview.chromium.org/1855423003/diff/100001/components/omnibox/bro... components/omnibox/browser/omnibox_edit_model.cc:891: // or typing a question mark in a blank omnibox, don't restore the keyword. Nit: "... Instead, restore the question mark iff the user originally typed one." https://codereview.chromium.org/1855423003/diff/100001/components/omnibox/bro... components/omnibox/browser/omnibox_edit_model.cc:894: view_->OnBeforePossibleChange(); Nit: This function body is basically identical to the "states 1-3" block below, and if we changed one place we'd probably want to change the other. I suspect given the comments below that these two entry methods can't result in us winding up in the state 4 block anyway. Therefore maybe instead of adding all this, we should just modify the states 1-3 block to set the right window text. At that point, we'd basically want to use conditionals to set a "prefix" variable to one of (keyword + space, question mark, nothing), then call SetWindowTextAndCaretPos(prefix + view_->GetText(), prefix.length(), false, false);. https://codereview.chromium.org/1855423003/diff/100001/components/omnibox/bro... components/omnibox/browser/omnibox_edit_model.cc:895: if (keyword_entered_method_ == ENTERED_KEYWORD_MODE_VIA_QUESTION_MARK) Nit: Needs {} https://codereview.chromium.org/1855423003/diff/100001/components/omnibox/bro... File components/omnibox/browser/omnibox_view.h (right): https://codereview.chromium.org/1855423003/diff/100001/components/omnibox/bro... components/omnibox/browser/omnibox_view.h:103: // user's text if they're already in keyword mode. Doesn't this select the text regardless? (Which is what it should do, I think.) https://codereview.chromium.org/1855423003/diff/100001/components/omnibox/bro... components/omnibox/browser/omnibox_view.h:104: virtual void SetForcedQuery() = 0; Nit: This function name should be changed as "forced query" isn't a meaningful concept anymore; maybe this should also be named EnterKeywordModeForDefaultSearchProvider()?
The CQ bit was checked by thomasanderson@google.com to run a CQ dry run
https://codereview.chromium.org/1855423003/diff/80001/components/omnibox/brow... File components/omnibox/browser/omnibox_edit_model.cc (right): https://codereview.chromium.org/1855423003/diff/80001/components/omnibox/brow... components/omnibox/browser/omnibox_edit_model.cc:962: view_->OnBeforePossibleChange(); On 2016/04/08 00:39:56, Peter Kasting wrote: > This will result in calling this twice, which is bad. Done. https://codereview.chromium.org/1855423003/diff/80001/components/omnibox/brow... File components/omnibox/browser/omnibox_edit_model.h (right): https://codereview.chromium.org/1855423003/diff/80001/components/omnibox/brow... components/omnibox/browser/omnibox_edit_model.h:522: // user would immediately be transitioned back into keyword mode. On 2016/04/13 02:52:16, Peter Kasting wrote: > Nit: Maybe this. I renamed the variable to match how you seem to use it, but > see comment in the .cc file also. > > // Set when the user is in the process of exiting keyword mode. This is > // useful to ensure we don't toggle back into keyword mode in the process of > // handling the current event. > bool clearing_keyword_; Done. https://codereview.chromium.org/1855423003/diff/100001/chrome/browser/autocom... File chrome/browser/autocomplete/autocomplete_browsertest.cc (right): https://codereview.chromium.org/1855423003/diff/100001/chrome/browser/autocom... chrome/browser/autocomplete/autocomplete_browsertest.cc:201: static void focusSearchCheckPreconditions() { On 2016/04/13 02:52:16, Peter Kasting wrote: > Nit: First letter of function name should be capitalized. Done. https://codereview.chromium.org/1855423003/diff/100001/chrome/browser/autocom... chrome/browser/autocomplete/autocomplete_browsertest.cc:202: LocationBar* location_bar = GetLocationBar(); On 2016/04/13 02:52:16, Peter Kasting wrote: > You can't call GetLocationBar() from a non-member function. Make this a class > member. Done. https://codereview.chromium.org/1855423003/diff/100001/chrome/browser/autocom... chrome/browser/autocomplete/autocomplete_browsertest.cc:232: base::string16 query_text = ASCIIToUTF16("foo"); On 2016/04/13 02:52:16, Peter Kasting wrote: > Need base:: qualifier Done. https://codereview.chromium.org/1855423003/diff/100001/chrome/browser/autocom... chrome/browser/autocomplete/autocomplete_browsertest.cc:335: EXPECT_EQ(query_text.length(), std::min(selection_start, selection_end)); On 2016/04/13 02:52:16, Peter Kasting wrote: > Shouldn't this be max()? Done. https://codereview.chromium.org/1855423003/diff/100001/components/omnibox/bro... File components/omnibox/browser/autocomplete_input_unittest.cc (right): https://codereview.chromium.org/1855423003/diff/100001/components/omnibox/bro... components/omnibox/browser/autocomplete_input_unittest.cc:289: // regular case, no changes. On 2016/04/13 02:52:16, Peter Kasting wrote: > Nit: While here: Capitalize these Done. https://codereview.chromium.org/1855423003/diff/100001/components/omnibox/bro... components/omnibox/browser/autocomplete_input_unittest.cc:297: // forced query. On 2016/04/13 02:52:16, Peter Kasting wrote: > Nit: Maybe "A leading '?' used to be a magic character indicating the following > input should be treated as a "forced query", but now if such a string reaches > the AutocompleteInput parser the '?' should just be treated like a normal > character." Done. https://codereview.chromium.org/1855423003/diff/100001/components/omnibox/bro... File components/omnibox/browser/autocomplete_result.cc (right): https://codereview.chromium.org/1855423003/diff/100001/components/omnibox/bro... components/omnibox/browser/autocomplete_result.cc:256: // for query inputs. On 2016/04/13 02:52:16, Peter Kasting wrote: > Nit: Move this comment into the first conditional arm, and remove the comma and > everything following. Done. https://codereview.chromium.org/1855423003/diff/100001/components/omnibox/bro... File components/omnibox/browser/omnibox_edit_model.cc (right): https://codereview.chromium.org/1855423003/diff/100001/components/omnibox/bro... components/omnibox/browser/omnibox_edit_model.cc:337: user_text_ == base::ASCIIToUTF16("?")) { On 2016/04/13 02:52:16, Peter Kasting wrote: > It doesn't seem like this handles the case of inserting a '?' before an existing > string, which it probably should? It does not. However, it would be a bit complex to add that logic, as we would have to keep the user_text_ before OnChanged was called in order to determine if a question mark was typed. Is it worth it to add this? https://codereview.chromium.org/1855423003/diff/100001/components/omnibox/bro... components/omnibox/browser/omnibox_edit_model.cc:879: just_cleared_keyword_ = true; On 2016/04/13 02:52:16, Peter Kasting wrote: > Nit: Use base::AutoReset here to ensure you set this false on all exit paths, > instead of doing it manually. > > Although... is clearing this on exit to this function safe? Maybe this should > persist until the next time something actually changes? What I'm worried about > is a case where you backspace out of keyword mode and then hit something that > doesn't actually change the text -- like the home or end keys or up arrow or > something. This shouldn't suddenly jump back into keyword mode. Make sure that > isn't broken... Not broken. Added base::AutoReset for now. On a side note, if I were to add the complex logic for OnChanged that I described in another reply, we would not need just_cleared_keyword_, so that could be a bonus. https://codereview.chromium.org/1855423003/diff/100001/components/omnibox/bro... components/omnibox/browser/omnibox_edit_model.cc:891: // or typing a question mark in a blank omnibox, don't restore the keyword. On 2016/04/13 02:52:16, Peter Kasting wrote: > Nit: "... Instead, restore the question mark iff the user originally typed one." Done. https://codereview.chromium.org/1855423003/diff/100001/components/omnibox/bro... components/omnibox/browser/omnibox_edit_model.cc:894: view_->OnBeforePossibleChange(); On 2016/04/13 02:52:16, Peter Kasting wrote: > Nit: This function body is basically identical to the "states 1-3" block below, > and if we changed one place we'd probably want to change the other. > > I suspect given the comments below that these two entry methods can't result in > us winding up in the state 4 block anyway. Therefore maybe instead of adding > all this, we should just modify the states 1-3 block to set the right window > text. At that point, we'd basically want to use conditionals to set a "prefix" > variable to one of (keyword + space, question mark, nothing), then call > SetWindowTextAndCaretPos(prefix + view_->GetText(), prefix.length(), false, > false);. Done. https://codereview.chromium.org/1855423003/diff/100001/components/omnibox/bro... components/omnibox/browser/omnibox_edit_model.cc:895: if (keyword_entered_method_ == ENTERED_KEYWORD_MODE_VIA_QUESTION_MARK) On 2016/04/13 02:52:16, Peter Kasting wrote: > Nit: Needs {} Done. https://codereview.chromium.org/1855423003/diff/100001/components/omnibox/bro... File components/omnibox/browser/omnibox_view.h (right): https://codereview.chromium.org/1855423003/diff/100001/components/omnibox/bro... components/omnibox/browser/omnibox_view.h:103: // user's text if they're already in keyword mode. On 2016/04/13 02:52:16, Peter Kasting wrote: > Doesn't this select the text regardless? (Which is what it should do, I think.) Done. https://codereview.chromium.org/1855423003/diff/100001/components/omnibox/bro... components/omnibox/browser/omnibox_view.h:104: virtual void SetForcedQuery() = 0; On 2016/04/13 02:52:16, Peter Kasting wrote: > Nit: This function name should be changed as "forced query" isn't a meaningful > concept anymore; maybe this should also be named > EnterKeywordModeForDefaultSearchProvider()? I Agree. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1855423003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1855423003/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
The CQ bit was checked by thomasanderson@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1855423003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1855423003/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by thomasanderson@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1855423003/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1855423003/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1855423003/diff/100001/components/omnibox/bro... File components/omnibox/browser/omnibox_edit_model.cc (right): https://codereview.chromium.org/1855423003/diff/100001/components/omnibox/bro... components/omnibox/browser/omnibox_edit_model.cc:337: user_text_ == base::ASCIIToUTF16("?")) { On 2016/04/13 23:37:40, Tom Anderson wrote: > On 2016/04/13 02:52:16, Peter Kasting wrote: > > It doesn't seem like this handles the case of inserting a '?' before an > existing > > string, which it probably should? > > It does not. However, it would be a bit complex to add that logic, as we would > have to keep the user_text_ before OnChanged was called in order to determine if > a question mark was typed. Is it worth it to add this? I think so -- if you prefix some existing text with a keyword and a space, the space will flip into keyword mode, so we should be parallel here. I am OK with that being a separate change, as long as you're up for doing it after this one. Or if you think it would be reasonably easy/not add lots of complications and bugs, you can do it in this CL. https://codereview.chromium.org/1855423003/diff/160001/chrome/browser/autocom... File chrome/browser/autocomplete/autocomplete_browsertest.cc (right): https://codereview.chromium.org/1855423003/diff/160001/chrome/browser/autocom... chrome/browser/autocomplete/autocomplete_browsertest.cc:75: void FocusSearchCheckPreconditions() { Nit: Technically, I think this could be const, since GetLocationBar() is const. But really that function violates a style rule that const functions not return non-const pointers. So honestly either that function shouldn't be const, or it should return a pointer-to-const -- and similarly for GetAutocompleteController(). Fixing these in turn will be annoying and isn't related to this CL at all, so either do it separately or just don't worry about it. https://codereview.chromium.org/1855423003/diff/160001/chrome/browser/autocom... chrome/browser/autocomplete/autocomplete_browsertest.cc:255: // Focus search when omnibox is _not_ alread in forced query mode. There is no such thing as forced query mode anymore... (multiple places) https://codereview.chromium.org/1855423003/diff/160001/components/omnibox/bro... File components/omnibox/browser/omnibox_edit_model.cc (right): https://codereview.chromium.org/1855423003/diff/160001/components/omnibox/bro... components/omnibox/browser/omnibox_edit_model.cc:957: if (keyword_mode_entry_method_ == METHOD_KEYBOARD_SHORTCUT) Nit: Simpler: if (keyword_mode_entry_method_ == METHOD_QUESTION_MARK) prefix = base::ASCIIToUTF16("?"); else if (keyword_mode_entry_method_ != METHOD_KEYBOARD_SHORTCUT) prefix = keyword_ + base::ASCIIToUTF16(" "); https://codereview.chromium.org/1855423003/diff/160001/components/omnibox/bro... components/omnibox/browser/omnibox_edit_model.cc:966: InternalSetUserText(prefix + view_->GetText()); Why does this need to be added? Shouldn't OnAfterPossibleChange() below already deal with this? https://codereview.chromium.org/1855423003/diff/160001/components/omnibox/bro... File components/omnibox/browser/omnibox_edit_model.h (right): https://codereview.chromium.org/1855423003/diff/160001/components/omnibox/bro... components/omnibox/browser/omnibox_edit_model.h:45: KEYWORD_MODE_ENTRY_METHOD_NUM_ITEMS Any particular reason to not make this an enum class? The names are better than before, but it still seems like KeywordModeEntryMethod::SPACE_AT_END or KeywordModeEntryMethod::NUM_ITEMS buy maximum safety and clarity.
The CQ bit was checked by thomasanderson@google.com to run a CQ dry run
https://codereview.chromium.org/1855423003/diff/160001/chrome/browser/autocom... File chrome/browser/autocomplete/autocomplete_browsertest.cc (right): https://codereview.chromium.org/1855423003/diff/160001/chrome/browser/autocom... chrome/browser/autocomplete/autocomplete_browsertest.cc:75: void FocusSearchCheckPreconditions() { On 2016/04/15 00:58:55, Peter Kasting wrote: > Nit: Technically, I think this could be const, since GetLocationBar() is const. > But really that function violates a style rule that const functions not return > non-const pointers. So honestly either that function shouldn't be const, or it > should return a pointer-to-const -- and similarly for > GetAutocompleteController(). Fixing these in turn will be annoying and isn't > related to this CL at all, so either do it separately or just don't worry about > it. Acknowledged. https://codereview.chromium.org/1855423003/diff/160001/chrome/browser/autocom... chrome/browser/autocomplete/autocomplete_browsertest.cc:255: // Focus search when omnibox is _not_ alread in forced query mode. On 2016/04/15 00:58:55, Peter Kasting wrote: > There is no such thing as forced query mode anymore... (multiple places) Done. https://codereview.chromium.org/1855423003/diff/160001/components/omnibox/bro... File components/omnibox/browser/omnibox_edit_model.cc (right): https://codereview.chromium.org/1855423003/diff/160001/components/omnibox/bro... components/omnibox/browser/omnibox_edit_model.cc:957: if (keyword_mode_entry_method_ == METHOD_KEYBOARD_SHORTCUT) On 2016/04/15 00:58:55, Peter Kasting wrote: > Nit: Simpler: > > if (keyword_mode_entry_method_ == METHOD_QUESTION_MARK) > prefix = base::ASCIIToUTF16("?"); > else if (keyword_mode_entry_method_ != METHOD_KEYBOARD_SHORTCUT) > prefix = keyword_ + base::ASCIIToUTF16(" "); Done. https://codereview.chromium.org/1855423003/diff/160001/components/omnibox/bro... components/omnibox/browser/omnibox_edit_model.cc:966: InternalSetUserText(prefix + view_->GetText()); On 2016/04/15 00:58:55, Peter Kasting wrote: > Why does this need to be added? Shouldn't OnAfterPossibleChange() below already > deal with this? Without it, if I press Ctrl-K and then backspace, "google.com " is left in the search bar, because that's what I set the text to on line 649. However, this seems suspicious to me also, like it's masking some other behavior. https://codereview.chromium.org/1855423003/diff/160001/components/omnibox/bro... File components/omnibox/browser/omnibox_edit_model.h (right): https://codereview.chromium.org/1855423003/diff/160001/components/omnibox/bro... components/omnibox/browser/omnibox_edit_model.h:45: KEYWORD_MODE_ENTRY_METHOD_NUM_ITEMS On 2016/04/15 00:58:55, Peter Kasting wrote: > Any particular reason to not make this an enum class? The names are better than > before, but it still seems like KeywordModeEntryMethod::SPACE_AT_END or > KeywordModeEntryMethod::NUM_ITEMS buy maximum safety and clarity. Done. The enum class was breaking UMA_HISTOGRAM_ENUMERATION due to its strong typing, but I added (int) casts to silence the errors
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1855423003/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1855423003/180001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1855423003/diff/160001/components/omnibox/bro... File components/omnibox/browser/omnibox_edit_model.cc (right): https://codereview.chromium.org/1855423003/diff/160001/components/omnibox/bro... components/omnibox/browser/omnibox_edit_model.cc:651: view_->OnTemporaryTextMaybeChanged(user_text_, false, true); Calling InternalSetUserText() + OnTemporaryTextMaybeChanged() + StartAutocomplete() isn't the right way to implement this. This isn't a temporary text change; that's for when you're arrowing around the dropdown and you want to preserve the underlying original user edits in case the user hits "escape" to revert to them. You want to just call view_->SetUserText() or something like it; in other words, you want to just blow away all previous user editing, preserved state, etc. This is probably why you had to add the InternalSetUserText() call down below that we're both suspicious of.
The CQ bit was checked by thomasanderson@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1855423003/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1855423003/200001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
The CQ bit was checked by thomasanderson@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1855423003/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1855423003/220001
https://codereview.chromium.org/1855423003/diff/160001/components/omnibox/bro... File components/omnibox/browser/omnibox_edit_model.cc (right): https://codereview.chromium.org/1855423003/diff/160001/components/omnibox/bro... components/omnibox/browser/omnibox_edit_model.cc:651: view_->OnTemporaryTextMaybeChanged(user_text_, false, true); On 2016/04/15 20:32:47, Peter Kasting wrote: > Calling InternalSetUserText() + OnTemporaryTextMaybeChanged() + > StartAutocomplete() isn't the right way to implement this. This isn't a > temporary text change; that's for when you're arrowing around the dropdown and > you want to preserve the underlying original user edits in case the user hits > "escape" to revert to them. > > You want to just call view_->SetUserText() or something like it; in other words, > you want to just blow away all previous user editing, preserved state, etc. > > This is probably why you had to add the InternalSetUserText() call down below > that we're both suspicious of. Done. https://codereview.chromium.org/1855423003/diff/160001/components/omnibox/bro... components/omnibox/browser/omnibox_edit_model.cc:966: InternalSetUserText(prefix + view_->GetText()); On 2016/04/15 00:58:55, Peter Kasting wrote: > Why does this need to be added? Shouldn't OnAfterPossibleChange() below already > deal with this? Removed call to InternalSetUserText
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1855423003/diff/220001/chrome/browser/autocom... File chrome/browser/autocomplete/autocomplete_browsertest.cc (right): https://codereview.chromium.org/1855423003/diff/220001/chrome/browser/autocom... chrome/browser/autocomplete/autocomplete_browsertest.cc:255: // Focus search when omnibox is _not_ alread in keyword mode. Nit: While here: alread -> already https://codereview.chromium.org/1855423003/diff/220001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm (right): https://codereview.chromium.org/1855423003/diff/220001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:374: [[field_ currentEditor] setSelectedRange:range]; Looks like these two lines are just trying to do what SelectAll() does? https://codereview.chromium.org/1855423003/diff/220001/components/omnibox/bro... File components/omnibox/browser/omnibox_edit_model.cc (right): https://codereview.chromium.org/1855423003/diff/220001/components/omnibox/bro... components/omnibox/browser/omnibox_edit_model.cc:646: base::string16 query = DisplayTextFromUserText(user_text_); Nit: Can be const https://codereview.chromium.org/1855423003/diff/220001/components/omnibox/bro... components/omnibox/browser/omnibox_edit_model.cc:656: (int)keyword_mode_entry_method, Nit: No C-style casts (several places) https://codereview.chromium.org/1855423003/diff/220001/components/omnibox/bro... components/omnibox/browser/omnibox_edit_model.cc:943: AcceptTemporaryTextAsUserText(); I don't think this is right. If I have an entry in mid-dropdown for amazon.com, and it has an associated keyword, then I think if I hit tab enough times to first get to that line and then enter keyword mode, I reach here. If we call AcceptTemporaryTextAsUserText() when I hit shift-tab, then I exit temporary text mode (i.e. tabbing-through-the-dropdown mode) entirely and am no longer tabbing through the dropdown. What "odd behavior" were you trying to fix here? In my testing things seem to work OK. https://codereview.chromium.org/1855423003/diff/220001/components/omnibox/bro... components/omnibox/browser/omnibox_edit_model.cc:974: view_->SetCaretPos(prefix.length()); Why call these and not SetWindowTextAndCaretPos()? https://codereview.chromium.org/1855423003/diff/220001/components/omnibox/bro... components/omnibox/browser/omnibox_edit_model.cc:977: StartAutocomplete(false, true, false); Why is this needed? SetUserText() above should have already updated the popup. https://codereview.chromium.org/1855423003/diff/220001/components/omnibox/bro... File components/omnibox/browser/omnibox_edit_model.h (right): https://codereview.chromium.org/1855423003/diff/220001/components/omnibox/bro... components/omnibox/browser/omnibox_edit_model.h:246: bool AcceptKeyword(KeywordModeEntryMethod keyword_mode_entry_method); Nit: For brevity I'd name the arg here and below |entry_method|
Sorry for the sloppiness in ClearKeyword(), I feel the new code is a bit too fragile. https://codereview.chromium.org/1855423003/diff/220001/chrome/browser/autocom... File chrome/browser/autocomplete/autocomplete_browsertest.cc (right): https://codereview.chromium.org/1855423003/diff/220001/chrome/browser/autocom... chrome/browser/autocomplete/autocomplete_browsertest.cc:255: // Focus search when omnibox is _not_ alread in keyword mode. On 2016/04/28 21:24:41, Peter Kasting wrote: > Nit: While here: alread -> already Done. https://codereview.chromium.org/1855423003/diff/220001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm (right): https://codereview.chromium.org/1855423003/diff/220001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:374: [[field_ currentEditor] setSelectedRange:range]; On 2016/04/28 21:24:41, Peter Kasting wrote: > Looks like these two lines are just trying to do what SelectAll() does? Done. https://codereview.chromium.org/1855423003/diff/220001/components/omnibox/bro... File components/omnibox/browser/omnibox_edit_model.cc (right): https://codereview.chromium.org/1855423003/diff/220001/components/omnibox/bro... components/omnibox/browser/omnibox_edit_model.cc:646: base::string16 query = DisplayTextFromUserText(user_text_); On 2016/04/28 21:24:41, Peter Kasting wrote: > Nit: Can be const Done. https://codereview.chromium.org/1855423003/diff/220001/components/omnibox/bro... components/omnibox/browser/omnibox_edit_model.cc:656: (int)keyword_mode_entry_method, On 2016/04/28 21:24:41, Peter Kasting wrote: > Nit: No C-style casts (several places) Done. https://codereview.chromium.org/1855423003/diff/220001/components/omnibox/bro... components/omnibox/browser/omnibox_edit_model.cc:943: AcceptTemporaryTextAsUserText(); On 2016/04/28 21:24:41, Peter Kasting wrote: > I don't think this is right. > > If I have an entry in mid-dropdown for http://amazon.com, and it has an associated > keyword, then I think if I hit tab enough times to first get to that line and > then enter keyword mode, I reach here. If we call > AcceptTemporaryTextAsUserText() when I hit shift-tab, then I exit temporary text > mode (i.e. tabbing-through-the-dropdown mode) entirely and am no longer tabbing > through the dropdown. > > What "odd behavior" were you trying to fix here? In my testing things seem to > work OK. To repro the odd behavior (without the call to AcceptTemporaryTextAsUserText()): type "goo<tab>" to go into keyword mode for google.com. Type backspace to clear keyword mode. Press ctrl-K. The text used to be google.com, but is now "goo". I was able to tab into and out of keyword mode with the patch applied. How exactly can I reproduce your issue? https://codereview.chromium.org/1855423003/diff/220001/components/omnibox/bro... components/omnibox/browser/omnibox_edit_model.cc:974: view_->SetCaretPos(prefix.length()); On 2016/04/28 21:24:41, Peter Kasting wrote: > Why call these and not SetWindowTextAndCaretPos()? If I use SetWidnowTextAndCaretPos, I get this behavior: Press ctrl-k to go into keyword mode. Press backspace to leave keyword mode. I should be left with an empty omnibox, but I'm left with "google.com". I think that because I use SetUserText in EnterKeywordModeforDefaultSearchProvider, I also need to use it here to update the model https://codereview.chromium.org/1855423003/diff/220001/components/omnibox/bro... components/omnibox/browser/omnibox_edit_model.cc:977: StartAutocomplete(false, true, false); On 2016/04/28 21:24:41, Peter Kasting wrote: > Why is this needed? SetUserText() above should have already updated the popup. Done. https://codereview.chromium.org/1855423003/diff/220001/components/omnibox/bro... File components/omnibox/browser/omnibox_edit_model.h (right): https://codereview.chromium.org/1855423003/diff/220001/components/omnibox/bro... components/omnibox/browser/omnibox_edit_model.h:246: bool AcceptKeyword(KeywordModeEntryMethod keyword_mode_entry_method); On 2016/04/28 21:24:41, Peter Kasting wrote: > Nit: For brevity I'd name the arg here and below |entry_method| Done.
https://codereview.chromium.org/1855423003/diff/220001/components/omnibox/bro... File components/omnibox/browser/omnibox_edit_model.cc (right): https://codereview.chromium.org/1855423003/diff/220001/components/omnibox/bro... components/omnibox/browser/omnibox_edit_model.cc:943: AcceptTemporaryTextAsUserText(); On 2016/04/29 01:10:21, Tom Anderson wrote: > On 2016/04/28 21:24:41, Peter Kasting wrote: > > I don't think this is right. > > > > If I have an entry in mid-dropdown for http://amazon.com, and it has an > associated > > keyword, then I think if I hit tab enough times to first get to that line and > > then enter keyword mode, I reach here. If we call > > AcceptTemporaryTextAsUserText() when I hit shift-tab, then I exit temporary > text > > mode (i.e. tabbing-through-the-dropdown mode) entirely and am no longer > tabbing > > through the dropdown. > > > > What "odd behavior" were you trying to fix here? In my testing things seem to > > work OK. > > To repro the odd behavior (without the call to AcceptTemporaryTextAsUserText()): > type "goo<tab>" to go into keyword mode for http://google.com. Type backspace to clear > keyword mode. Press ctrl-K. The text used to be http://google.com, but is now "goo". > > I was able to tab into and out of keyword mode with the patch applied. How > exactly can I reproduce your issue? Ensure the keyword mode you're tabbing into is not associated with the default match, but rather with a match several rows down. When you tab back out we shouldn't reset the rest of the dropdown and escape should do exactly what it would do if you had tabbed down to that row but not gone into keyword mode at all. https://codereview.chromium.org/1855423003/diff/220001/components/omnibox/bro... components/omnibox/browser/omnibox_edit_model.cc:974: view_->SetCaretPos(prefix.length()); On 2016/04/29 01:10:21, Tom Anderson wrote: > On 2016/04/28 21:24:41, Peter Kasting wrote: > > Why call these and not SetWindowTextAndCaretPos()? > > If I use SetWidnowTextAndCaretPos, I get this behavior: > Press ctrl-k to go into keyword mode. Press backspace to leave keyword mode. I > should be left with an empty omnibox, but I'm left with "google.com". I think > that because I use SetUserText in EnterKeywordModeforDefaultSearchProvider, I > also need to use it here to update the model That doesn't sound like the right cause; this code doesn't know what method you called when entering keyword mode. I'd like to get a clearer call stack trace through all the code in here to see precisely what's resulting in the text you describe. It's very possible that the solution to all this stuff is that we need to ditch the somewhat-insane under-the-hood representation of keyword mode using a modified user text string that gets its prefix snipped off for display to the user, and keep those two pieces of info entirely separate.
On 2016/04/29 01:36:47, Peter Kasting wrote: > https://codereview.chromium.org/1855423003/diff/220001/components/omnibox/bro... > File components/omnibox/browser/omnibox_edit_model.cc (right): > > https://codereview.chromium.org/1855423003/diff/220001/components/omnibox/bro... > components/omnibox/browser/omnibox_edit_model.cc:943: > AcceptTemporaryTextAsUserText(); > On 2016/04/29 01:10:21, Tom Anderson wrote: > > On 2016/04/28 21:24:41, Peter Kasting wrote: > > > I don't think this is right. > > > > > > If I have an entry in mid-dropdown for http://amazon.com, and it has an > > associated > > > keyword, then I think if I hit tab enough times to first get to that line > and > > > then enter keyword mode, I reach here. If we call > > > AcceptTemporaryTextAsUserText() when I hit shift-tab, then I exit temporary > > text > > > mode (i.e. tabbing-through-the-dropdown mode) entirely and am no longer > > tabbing > > > through the dropdown. > > > > > > What "odd behavior" were you trying to fix here? In my testing things seem > to > > > work OK. > > > > To repro the odd behavior (without the call to > AcceptTemporaryTextAsUserText()): > > type "goo<tab>" to go into keyword mode for http://google.com. Type backspace > to clear > > keyword mode. Press ctrl-K. The text used to be http://google.com, but is > now "goo". > > > > I was able to tab into and out of keyword mode with the patch applied. How > > exactly can I reproduce your issue? > > Ensure the keyword mode you're tabbing into is not associated with the default > match, but rather with a match several rows down. When you tab back out we > shouldn't reset the rest of the dropdown and escape should do exactly what it > would do if you had tabbed down to that row but not gone into keyword mode at > all. I'm still not able to repro. See the video here https://bugs.chromium.org/p/chromium/issues/detail?id=267629#c12 But, I think I'll just revert this chunk anyway since you can reproduce this. > > https://codereview.chromium.org/1855423003/diff/220001/components/omnibox/bro... > components/omnibox/browser/omnibox_edit_model.cc:974: > view_->SetCaretPos(prefix.length()); > On 2016/04/29 01:10:21, Tom Anderson wrote: > > On 2016/04/28 21:24:41, Peter Kasting wrote: > > > Why call these and not SetWindowTextAndCaretPos()? > > > > If I use SetWidnowTextAndCaretPos, I get this behavior: > > Press ctrl-k to go into keyword mode. Press backspace to leave keyword mode. > I > > should be left with an empty omnibox, but I'm left with "google.com". I think > > that because I use SetUserText in EnterKeywordModeforDefaultSearchProvider, I > > also need to use it here to update the model > > That doesn't sound like the right cause; this code doesn't know what method you > called when entering keyword mode. I'd like to get a clearer call stack trace > through all the code in here to see precisely what's resulting in the text you > describe. > > It's very possible that the solution to all this stuff is that we need to ditch > the somewhat-insane under-the-hood representation of keyword mode using a > modified user text string that gets its prefix snipped off for display to the > user, and keep those two pieces of info entirely separate. I made a wip cl that I can rebase these changes on here: https://codereview.chromium.org/1943683002 Should we continue with this option, or just keep the changes in this CL? I also began to split the keyword from the query in AutocompleteInput and some other classes, but there were far too many dependencies that needed to change.
On 2016/05/02 23:55:20, Tom Anderson wrote: > On 2016/04/29 01:36:47, Peter Kasting wrote: > > > https://codereview.chromium.org/1855423003/diff/220001/components/omnibox/bro... > > File components/omnibox/browser/omnibox_edit_model.cc (right): > > > > > https://codereview.chromium.org/1855423003/diff/220001/components/omnibox/bro... > > components/omnibox/browser/omnibox_edit_model.cc:943: > > AcceptTemporaryTextAsUserText(); > > On 2016/04/29 01:10:21, Tom Anderson wrote: > > > On 2016/04/28 21:24:41, Peter Kasting wrote: > > > > I don't think this is right. > > > > > > > > If I have an entry in mid-dropdown for http://amazon.com, and it has an > > > associated > > > > keyword, then I think if I hit tab enough times to first get to that line > > and > > > > then enter keyword mode, I reach here. If we call > > > > AcceptTemporaryTextAsUserText() when I hit shift-tab, then I exit > temporary > > > text > > > > mode (i.e. tabbing-through-the-dropdown mode) entirely and am no longer > > > tabbing > > > > through the dropdown. > > > > > > > > What "odd behavior" were you trying to fix here? In my testing things > seem > > to > > > > work OK. > > > > > > To repro the odd behavior (without the call to > > AcceptTemporaryTextAsUserText()): > > > type "goo<tab>" to go into keyword mode for http://google.com. Type > backspace > > to clear > > > keyword mode. Press ctrl-K. The text used to be http://google.com, but is > > now "goo". > > > > > > I was able to tab into and out of keyword mode with the patch applied. How > > > exactly can I reproduce your issue? > > > > Ensure the keyword mode you're tabbing into is not associated with the default > > match, but rather with a match several rows down. When you tab back out we > > shouldn't reset the rest of the dropdown and escape should do exactly what it > > would do if you had tabbed down to that row but not gone into keyword mode at > > all. > > I'm still not able to repro. See the video here > https://bugs.chromium.org/p/chromium/issues/detail?id=267629#c12 > But, I think I'll just revert this chunk anyway since you can reproduce this. No, I was stating a hypothetical bug, not one I was observing in a debugger. I was interested in understanding, if what I was describing didn't happen, why it didn't happen. > I made a wip cl that I can rebase these changes on here: > https://codereview.chromium.org/1943683002 > Should we continue with this option, or just keep the changes in this CL? > > I also began to split the keyword from the query in AutocompleteInput and some > other classes, but there were far too many dependencies that needed to change. Since this stuff is subtle and confusing, I'd say let's do everything as separate pieces. I suspect the right thing to do is to do both of the changes you describe above first, then return to the functionality of this patch, which will hopefully be easier to accomplish without risking any side effects in edge cases.
Description was changed from ========== Interpret '?' and Ctrl-K or Ctrl-E as putting omnibox in keyword search mode for Default Search Provider. When the user presses Ctrl-K or Ctrl-E: Transition the user into keyword mode using their default search provider. Pressing Ctrl-K will give the omnibox focus, and will display "Search Google:" on the left. If the user has already entered text into the omnibox and presses Ctrl-K, their text will be preserved, but they will be transitioned into keyword mode anyway. If the user is already in keyword mode (even if it's with a non-default search provider), the text they have entered will become selected. Upon pressing backspace, they will be left with an empty omnibox. When the user types '?' into the omnibox: If the omnibox is currently empty and the user types '?' (or, the user has the entire text selected and overwrites it with '?', or they paste in '?'), they will be transitioned into keyword mode with their default search provider. Upon pressing backspace, the omnibox will be left with a single '?'. BUG=267629 ========== to ========== Interpret '?' and Ctrl-K or Ctrl-E as putting omnibox in keyword search mode for Default Search Provider. When the user presses Ctrl-K or Ctrl-E: Transition the user into keyword mode using their default search provider. Pressing Ctrl-K will give the omnibox focus, and will display "Search Google:" on the left. If the user has already entered text into the omnibox and presses Ctrl-K, their text will be preserved, but they will be transitioned into keyword mode anyway. If the user is already in keyword mode (even if it's with a non-default search provider), the text they have entered will become selected. Upon pressing backspace, they will be left with an empty omnibox. When the user types '?' into the omnibox: If the user types or pastes a '?' at the beginning of their (possibly empty) input text, transition them into keyword mode with their default search provider. Upon pressing backspace, the omnibox will be left '?' (and their input). Remove forced queries. We should be able to search for '?' directly. BUG=267629,592540 ==========
The CQ bit was checked by thomasanderson@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1855423003/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1855423003/260001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #14 (id:260001) has been deleted
The CQ bit was checked by thomasanderson@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1855423003/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1855423003/280001
Patchset #14 (id:280001) has been deleted
The CQ bit was checked by thomasanderson@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1855423003/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1855423003/300001
Rebased on changes from https://codereview.chromium.org/1943683002/ https://codereview.chromium.org/1855423003/diff/300001/components/omnibox/bro... File components/omnibox/browser/omnibox_edit_model.cc (right): https://codereview.chromium.org/1855423003/diff/300001/components/omnibox/bro... components/omnibox/browser/omnibox_edit_model.cc:961: StartAutocomplete(false, false, false); If I remove StartAutocomplete here, I get bad behavior when doing the following: 1. press ctrl-K to enter keyword mode 2. press backspace 3. observe that the autocomplete popup is still present Maybe there's a check for the difference of user_text_ that's not taking the keyword_ difference into account? (OnAfterPossibleChange?) https://codereview.chromium.org/1855423003/diff/300001/components/omnibox/bro... components/omnibox/browser/omnibox_edit_model.cc:1290: KeywordModeEntryMethod::QUESTION_MARK); If I type "abc" in the omnibox, move the cursor to the beginning and press '?', we correctly enter keyword mode and the user_text_ is "abc", but the cursor is at the end of the text. Should it be at the beginning instead?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
The CQ bit was checked by thomasanderson@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1855423003/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1855423003/320001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
The CQ bit was checked by thomasanderson@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1855423003/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1855423003/340001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
mpearson@chromium.org changed reviewers: + mpearson@chromium.org
not lgtm (happened to be reading old snippets and saw a link to this changelist in progress) https://codereview.chromium.org/1855423003/diff/340001/components/metrics/pro... File components/metrics/proto/omnibox_input_type.proto (left): https://codereview.chromium.org/1855423003/diff/340001/components/metrics/pro... components/metrics/proto/omnibox_input_type.proto:38: FORCED_QUERY = 5; Do not remove this entry. Because this proto message is written to logs, we cannot get rid of =5 last we accidentally replace it with something else later and then not be able to understand what 5 means in logs. Instead, mark it as deprecated. You will also have to make the analogous change to the google3 definition of this proto.
The CQ bit was checked by thomasanderson@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1855423003/360001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1855423003/360001
The CQ bit was unchecked by thomasanderson@google.com
https://codereview.chromium.org/1855423003/diff/340001/components/metrics/pro... File components/metrics/proto/omnibox_input_type.proto (left): https://codereview.chromium.org/1855423003/diff/340001/components/metrics/pro... components/metrics/proto/omnibox_input_type.proto:38: FORCED_QUERY = 5; On 2016/05/19 23:27:07, Mark P wrote: > Do not remove this entry. Because this proto message is written to logs, we > cannot get rid of =5 last we accidentally replace it with something else later > and then not be able to understand what 5 means in logs. > Instead, mark it as deprecated. > > You will also have to make the analogous change to the google3 definition of > this proto. Done. google3 cl 122783890
The CQ bit was checked by thomasanderson@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1855423003/360001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1855423003/360001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
The CQ bit was checked by thomasanderson@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1855423003/380001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1855423003/380001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
components/metrics/proto/omnibox_input_type.proto lgtm
Can you please file a bug to get the support pages updated? I think because we're not displaying a question mark anymore, some of them are a bit misleading. I found these two: https://support.google.com/chromebook/answer/183101?hl=en https://support.google.com/chrome/answer/157179?hl=en (section on "Address Bar") There may be others. --mark
https://codereview.chromium.org/1855423003/diff/300001/components/omnibox/bro... File components/omnibox/browser/omnibox_edit_model.cc (right): https://codereview.chromium.org/1855423003/diff/300001/components/omnibox/bro... components/omnibox/browser/omnibox_edit_model.cc:961: StartAutocomplete(false, false, false); On 2016/05/18 23:58:36, Tom Anderson wrote: > If I remove StartAutocomplete here, I get bad behavior when doing the following: > > 1. press ctrl-K to enter keyword mode > 2. press backspace > 3. observe that the autocomplete popup is still present > > Maybe there's a check for the difference of user_text_ that's not taking the > keyword_ difference into account? (OnAfterPossibleChange?) Maybe the right fix here is that the popup should not appear after step 1. It didn't use to when typing a '?', but maybe that's because we ran autocomplete but the input was parsed as INVALID? Whereas now the input is effectively "google.com ". So maybe we should modify EnterKeywordModeForDefaultSearchProvider() so that it runs autocomplete only if the popup is already open (to update it)? Or is there an entry method that can happen when the popup is currently closed that should result in the popup opening? https://codereview.chromium.org/1855423003/diff/300001/components/omnibox/bro... components/omnibox/browser/omnibox_edit_model.cc:1290: KeywordModeEntryMethod::QUESTION_MARK); On 2016/05/18 23:58:36, Tom Anderson wrote: > If I type "abc" in the omnibox, move the cursor to the beginning and press '?', > we correctly enter keyword mode and the user_text_ is "abc", but the cursor is > at the end of the text. Should it be at the beginning instead? Yes.
The CQ bit was checked by thomasanderson@google.com to run a CQ dry run
https://codereview.chromium.org/1855423003/diff/300001/components/omnibox/bro... File components/omnibox/browser/omnibox_edit_model.cc (right): https://codereview.chromium.org/1855423003/diff/300001/components/omnibox/bro... components/omnibox/browser/omnibox_edit_model.cc:961: StartAutocomplete(false, false, false); On 2016/05/21 00:01:01, Peter Kasting wrote: > On 2016/05/18 23:58:36, Tom Anderson wrote: > > If I remove StartAutocomplete here, I get bad behavior when doing the > following: > > > > 1. press ctrl-K to enter keyword mode > > 2. press backspace > > 3. observe that the autocomplete popup is still present > > > > Maybe there's a check for the difference of user_text_ that's not taking the > > keyword_ difference into account? (OnAfterPossibleChange?) > > Maybe the right fix here is that the popup should not appear after step 1. > > It didn't use to when typing a '?', but maybe that's because we ran autocomplete > but the input was parsed as INVALID? Whereas now the input is effectively > "google.com ". > > So maybe we should modify EnterKeywordModeForDefaultSearchProvider() so that it > runs autocomplete only if the popup is already open (to update it)? Or is there > an entry method that can happen when the popup is currently closed that should > result in the popup opening? Well, we would want the popup to remain open if the user already has text and then types ctrl-k. Also, the popup can be open anyway if the user enters text and then deletes it. They will be left with a "<Type a search term>" popup. I changed this to UpdatePopup() instead. LMK if there's a better [way to do this]|[place to put this]. For your reference, the stack trace I get when clearing the popup by deleting all text in the omnibox is: #0 0x7f456cb5f83e base::debug::StackTrace::StackTrace() #1 0x7f4570156be5 OmniboxPopupContentsView::UpdatePopupAppearance() #2 0x7f45706907d6 OmniboxPopupModel::OnResultChanged() #3 0x7f457067a39e OmniboxController::OnResultChanged() #4 0x7f4570627286 AutocompleteController::NotifyChanged() #5 0x7f4570625951 AutocompleteController::UpdateResult() #6 0x7f4570624dfb AutocompleteController::Start() #7 0x7f457067a233 OmniboxController::StartAutocomplete() #8 0x7f457067d7f6 OmniboxEditModel::StartAutocomplete() #9 0x7f457015fb22 OmniboxViewViews::UpdatePopup() #10 0x7f4570680bd0 OmniboxEditModel::ClearKeyword() #11 0x7f4570161e14 OmniboxViewViews::HandleKeyEvent() #12 0x7f456209db08 views::Textfield::OnKeyPressed() https://codereview.chromium.org/1855423003/diff/300001/components/omnibox/bro... components/omnibox/browser/omnibox_edit_model.cc:1290: KeywordModeEntryMethod::QUESTION_MARK); On 2016/05/21 00:01:01, Peter Kasting wrote: > On 2016/05/18 23:58:36, Tom Anderson wrote: > > If I type "abc" in the omnibox, move the cursor to the beginning and press > '?', > > we correctly enter keyword mode and the user_text_ is "abc", but the cursor is > > at the end of the text. Should it be at the beginning instead? > > Yes. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1855423003/400001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1855423003/400001
https://codereview.chromium.org/1855423003/diff/300001/components/omnibox/bro... File components/omnibox/browser/omnibox_edit_model.cc (right): https://codereview.chromium.org/1855423003/diff/300001/components/omnibox/bro... components/omnibox/browser/omnibox_edit_model.cc:961: StartAutocomplete(false, false, false); On 2016/05/27 22:12:07, Tom Anderson wrote: > On 2016/05/21 00:01:01, Peter Kasting wrote: > > On 2016/05/18 23:58:36, Tom Anderson wrote: > > > If I remove StartAutocomplete here, I get bad behavior when doing the > > following: > > > > > > 1. press ctrl-K to enter keyword mode > > > 2. press backspace > > > 3. observe that the autocomplete popup is still present > > > > > > Maybe there's a check for the difference of user_text_ that's not taking the > > > keyword_ difference into account? (OnAfterPossibleChange?) > > > > Maybe the right fix here is that the popup should not appear after step 1. > > > > It didn't use to when typing a '?', but maybe that's because we ran > autocomplete > > but the input was parsed as INVALID? Whereas now the input is effectively > > "google.com ". > > > > So maybe we should modify EnterKeywordModeForDefaultSearchProvider() so that > it > > runs autocomplete only if the popup is already open (to update it)? Or is > there > > an entry method that can happen when the popup is currently closed that should > > result in the popup opening? > > Well, we would want the popup to remain open if the user already has text and > then types ctrl-k. Right, that's why I said "runs autocomplete only if the popup is already open". > I changed this to UpdatePopup() instead. Doesn't that have the exact same effects as your previous code? The only real difference is that it sets the input in progress bit, which should always already be set. Let's back up here. The issue you're having is that you hit ctrl-K, it opens a "type to search" popup, you hit delete, and the popup contents don't change at all? If so then OnAfterPossibleChange() is not catching the keyword mode change and should be. It seems to catch other exit-from-keyword-mode changes, so I'd want to understand why it doesn't catch this one.
https://codereview.chromium.org/1855423003/diff/300001/components/omnibox/bro... File components/omnibox/browser/omnibox_edit_model.cc (right): https://codereview.chromium.org/1855423003/diff/300001/components/omnibox/bro... components/omnibox/browser/omnibox_edit_model.cc:961: StartAutocomplete(false, false, false); On 2016/05/27 22:24:04, Peter Kasting wrote: > On 2016/05/27 22:12:07, Tom Anderson wrote: > > On 2016/05/21 00:01:01, Peter Kasting wrote: > > > On 2016/05/18 23:58:36, Tom Anderson wrote: > > > > If I remove StartAutocomplete here, I get bad behavior when doing the > > > following: > > > > > > > > 1. press ctrl-K to enter keyword mode > > > > 2. press backspace > > > > 3. observe that the autocomplete popup is still present > > > > > > > > Maybe there's a check for the difference of user_text_ that's not taking > the > > > > keyword_ difference into account? (OnAfterPossibleChange?) > > > > > > Maybe the right fix here is that the popup should not appear after step 1. > > > > > > It didn't use to when typing a '?', but maybe that's because we ran > > autocomplete > > > but the input was parsed as INVALID? Whereas now the input is effectively > > > "google.com ". > > > > > > So maybe we should modify EnterKeywordModeForDefaultSearchProvider() so that > > it > > > runs autocomplete only if the popup is already open (to update it)? Or is > > there > > > an entry method that can happen when the popup is currently closed that > should > > > result in the popup opening? > > > > Well, we would want the popup to remain open if the user already has text and > > then types ctrl-k. > > Right, that's why I said "runs autocomplete only if the popup is already open". > I tested this out already. Even if we do this, we can still type text, which will open the popup. If we then delete text, the popup will be open, but the omnibox empty. When we press backspace to clear the keyword, the popup will still be open. > > I changed this to UpdatePopup() instead. > > Doesn't that have the exact same effects as your previous code? The only real > difference is that it sets the input in progress bit, which should always > already be set. > > Let's back up here. The issue you're having is that you hit ctrl-K, it opens a > "type to search" popup, you hit delete, and the popup contents don't change at > all? If so then OnAfterPossibleChange() is not catching the keyword mode change > and should be. It seems to catch other exit-from-keyword-mode changes, so I'd > want to understand why it doesn't catch this one. That's correct. Ah, now I see it. Line 1240. The early return is skipping UpdatePopup
The CQ bit was checked by thomasanderson@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1855423003/420001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1855423003/420001
Made the change in OnAfterPossibleChange
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Nice. This is almost done. Congratulations on your patience and persistence on this change! https://codereview.chromium.org/1855423003/diff/300001/chrome/browser/autocom... File chrome/browser/autocomplete/autocomplete_browsertest.cc (right): https://codereview.chromium.org/1855423003/diff/300001/chrome/browser/autocom... chrome/browser/autocomplete/autocomplete_browsertest.cc:335: // they should be left with '?' in the omnibox. Just so I'm clear: this test needed to move elsewhere to fix flakiness, right? What about the other tests from this testcase, are they similarly in need of movement? I'm wondering if we should be moving them all at once, or if any overlap with other existing non-flaky tests and should just be deleted. https://codereview.chromium.org/1855423003/diff/420001/chrome/browser/ui/omni... File chrome/browser/ui/omnibox/omnibox_view_browsertest.cc (right): https://codereview.chromium.org/1855423003/diff/420001/chrome/browser/ui/omni... chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:880: // they should be left with '?' in the omnibox. Nit: The second sentence here really belongs as part/all of the comment on the code block below. https://codereview.chromium.org/1855423003/diff/420001/chrome/browser/ui/view... File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right): https://codereview.chromium.org/1855423003/diff/420001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views.cc:529: keyword_before_change_ = model()->keyword(); This may be worth pulling out to a separate CL, but: It seems like both here and in the Mac implementation, we have a whole bunch of member variables, which we initialize with state in OnBeforePossibleChange(), then we make the same state checks in OnAfterPossibleChange(), compute the differences, and pass them to the edit model's OnAfterPossibleChange(). For one, this is a lot of duplicated code between Views and Mac that I wonder if we can hoist to somewhere in the cross-platform parent class. For another (and this is rather orthogonal to the above), having a bunch of separate members, and a bunch of separate args to OmniboxEditModel::OnAfterPossibleChange(), is awkward. And the latter issue results in everywhere else that wants to call that function (e.g. tests) passing a bunch of bools, which is not very readable and prone to error. I wonder if we should consider creating a couple of bits to help out here, e.g.: * Some kind of helper function that can return either a struct or a tuple that represents all the variables we need to track across a possible state change. Then this struct/tuple can replace all the separate member variables on this class. * A struct to contain all the "XYZ changed" flags, with a null constructor so tests can easily just call OnAfterPossibleChange(..., StuffChangedStruct()), and a constructor that takes two of the above structs/tuples (the "before" and "after" states) and computes its flag values from those. This latter constructor can be used in the view's OnAfterPossibleChange(). https://codereview.chromium.org/1855423003/diff/420001/components/omnibox/bro... File components/omnibox/browser/omnibox_edit_model.cc (right): https://codereview.chromium.org/1855423003/diff/420001/components/omnibox/bro... components/omnibox/browser/omnibox_edit_model.cc:1248: return keyword_differs; What are the consequences of returning true versus false here? I assume what you're doing here is correct, but the comment in the header says something like "when a significant change occurs" which is pretty vague about what callers would expect in a case like this. This also informs how I react to where you return false below in the new code to handle pressing '?'; it would be nice to be able to say precisely which state callers think they need to update if this returns true, so that we can say that EnterKeywordModeForDefaultSearchProvider() has updated all that state and thus returning false is correct. https://codereview.chromium.org/1855423003/diff/420001/components/omnibox/bro... components/omnibox/browser/omnibox_edit_model.cc:1293: no_selection && !is_keyword_selected()) { Nit: Reverse this conditional and early-return true https://codereview.chromium.org/1855423003/diff/420001/components/omnibox/bro... components/omnibox/browser/omnibox_edit_model.cc:1296: if (selection_start == 1 && user_text_[0] == '?') { MaybeAcceptKeywordBySpace() checks that the paste state is NONE, i.e. if we paste in a space instead of typing it it won't trigger keyword mode. I think we should do the same here. Pasting a "?" in seems like it should result in a question mark showing, not being treated as the search accelerator. Nit: IIRC file style is to put parens around each boolean subexpr. https://codereview.chromium.org/1855423003/diff/420001/components/omnibox/bro... components/omnibox/browser/omnibox_edit_model.cc:1317: MaybeAcceptKeywordBySpace(user_text_)); Nit: Distribute the '!' through this expression
Will update with the mac changes once Chrome finishes compiling on my laptop... https://codereview.chromium.org/1855423003/diff/300001/chrome/browser/autocom... File chrome/browser/autocomplete/autocomplete_browsertest.cc (right): https://codereview.chromium.org/1855423003/diff/300001/chrome/browser/autocom... chrome/browser/autocomplete/autocomplete_browsertest.cc:335: // they should be left with '?' in the omnibox. On 2016/06/01 01:07:27, Peter Kasting wrote: > Just so I'm clear: this test needed to move elsewhere to fix flakiness, right? > What about the other tests from this testcase, are they similarly in need of > movement? I'm wondering if we should be moving them all at once, or if any > overlap with other existing non-flaky tests and should just be deleted. No, they're both flaky :P It was so that I could do ASSERT_NO_FATAL_FAILURE(SendKey(ui::VKEY_BACK, 0)); instead of omnibox_model->ClearKeyword(); https://codereview.chromium.org/1855423003/diff/420001/chrome/browser/ui/omni... File chrome/browser/ui/omnibox/omnibox_view_browsertest.cc (right): https://codereview.chromium.org/1855423003/diff/420001/chrome/browser/ui/omni... chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:880: // they should be left with '?' in the omnibox. On 2016/06/01 01:07:27, Peter Kasting wrote: > Nit: The second sentence here really belongs as part/all of the comment on the > code block below. Done. https://codereview.chromium.org/1855423003/diff/420001/chrome/browser/ui/view... File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right): https://codereview.chromium.org/1855423003/diff/420001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views.cc:529: keyword_before_change_ = model()->keyword(); On 2016/06/01 01:07:27, Peter Kasting wrote: > This may be worth pulling out to a separate CL, but: > > It seems like both here and in the Mac implementation, we have a whole bunch of > member variables, which we initialize with state in OnBeforePossibleChange(), > then we make the same state checks in OnAfterPossibleChange(), compute the > differences, and pass them to the edit model's OnAfterPossibleChange(). > > For one, this is a lot of duplicated code between Views and Mac that I wonder if > we can hoist to somewhere in the cross-platform parent class. > > For another (and this is rather orthogonal to the above), having a bunch of > separate members, and a bunch of separate args to > OmniboxEditModel::OnAfterPossibleChange(), is awkward. And the latter issue > results in everywhere else that wants to call that function (e.g. tests) passing > a bunch of bools, which is not very readable and prone to error. I wonder if we > should consider creating a couple of bits to help out here, e.g.: > * Some kind of helper function that can return either a struct or a tuple that > represents all the variables we need to track across a possible state change. > Then this struct/tuple can replace all the separate member variables on this > class. > * A struct to contain all the "XYZ changed" flags, with a null constructor so > tests can easily just call OnAfterPossibleChange(..., StuffChangedStruct()), and > a constructor that takes two of the above structs/tuples (the "before" and > "after" states) and computes its flag values from those. This latter > constructor can be used in the view's OnAfterPossibleChange(). Did both. Here's to DRY Unfortunately it's on this patch set. However, I have a commit on my machine that I can just cherry-pick and put in a different patch set if that's easier for you. https://codereview.chromium.org/1855423003/diff/420001/components/omnibox/bro... File components/omnibox/browser/omnibox_edit_model.cc (right): https://codereview.chromium.org/1855423003/diff/420001/components/omnibox/bro... components/omnibox/browser/omnibox_edit_model.cc:1248: return keyword_differs; On 2016/06/01 01:07:27, Peter Kasting wrote: > What are the consequences of returning true versus false here? I assume what > you're doing here is correct, but the comment in the header says something like > "when a significant change occurs" which is pretty vague about what callers > would expect in a case like this. > > This also informs how I react to where you return false below in the new code to > handle pressing '?'; it would be nice to be able to say precisely which state > callers think they need to update if this returns true, so that we can say that > EnterKeywordModeForDefaultSearchProvider() has updated all that state and thus > returning false is correct. My memory is foggy since I changed the "return false" to "return keyword_differs" like 2 months ago, but if I remember correctly, I think if we return false, the view does not get rerendered. So if we just returned false here like before, if the keyword changes without the text changing, the view will not change, and we will have to type an extra character for the view to update and see the keyword change. So, I guess it might be more correct to say "if a view-visible change occurs" instead of "when a significant change occurs"? https://codereview.chromium.org/1855423003/diff/420001/components/omnibox/bro... components/omnibox/browser/omnibox_edit_model.cc:1293: no_selection && !is_keyword_selected()) { On 2016/06/01 01:07:27, Peter Kasting wrote: > Nit: Reverse this conditional and early-return true Done. https://codereview.chromium.org/1855423003/diff/420001/components/omnibox/bro... components/omnibox/browser/omnibox_edit_model.cc:1296: if (selection_start == 1 && user_text_[0] == '?') { On 2016/06/01 01:07:27, Peter Kasting wrote: > MaybeAcceptKeywordBySpace() checks that the paste state is NONE, i.e. if we > paste in a space instead of typing it it won't trigger keyword mode. > > I think we should do the same here. Pasting a "?" in seems like it should > result in a question mark showing, not being treated as the search accelerator. > > Nit: IIRC file style is to put parens around each boolean subexpr. Done, also removed the paste state check from MaybeAcceptKeywordBySpace. LMK if you think that should be added back. https://codereview.chromium.org/1855423003/diff/420001/components/omnibox/bro... components/omnibox/browser/omnibox_edit_model.cc:1317: MaybeAcceptKeywordBySpace(user_text_)); On 2016/06/01 01:07:27, Peter Kasting wrote: > Nit: Distribute the '!' through this expression Done.
https://codereview.chromium.org/1855423003/diff/300001/chrome/browser/autocom... File chrome/browser/autocomplete/autocomplete_browsertest.cc (right): https://codereview.chromium.org/1855423003/diff/300001/chrome/browser/autocom... chrome/browser/autocomplete/autocomplete_browsertest.cc:335: // they should be left with '?' in the omnibox. On 2016/06/02 19:24:39, Tom Anderson wrote: > On 2016/06/01 01:07:27, Peter Kasting wrote: > > Just so I'm clear: this test needed to move elsewhere to fix flakiness, right? > > > What about the other tests from this testcase, are they similarly in need of > > movement? I'm wondering if we should be moving them all at once, or if any > > overlap with other existing non-flaky tests and should just be deleted. > > No, they're both flaky :P Meaning, the remaining tests are flaky, or meaning that both old and new versions of this test are flaky? If the latter, does that mean you never figured out the cause of flakiness? Are you going to keep trying on that front? https://codereview.chromium.org/1855423003/diff/420001/components/omnibox/bro... File components/omnibox/browser/omnibox_edit_model.cc (right): https://codereview.chromium.org/1855423003/diff/420001/components/omnibox/bro... components/omnibox/browser/omnibox_edit_model.cc:1248: return keyword_differs; On 2016/06/02 19:24:40, Tom Anderson wrote: > On 2016/06/01 01:07:27, Peter Kasting wrote: > > What are the consequences of returning true versus false here? I assume what > > you're doing here is correct, but the comment in the header says something > like > > "when a significant change occurs" which is pretty vague about what callers > > would expect in a case like this. > > > > This also informs how I react to where you return false below in the new code > to > > handle pressing '?'; it would be nice to be able to say precisely which state > > callers think they need to update if this returns true, so that we can say > that > > EnterKeywordModeForDefaultSearchProvider() has updated all that state and thus > > returning false is correct. > > My memory is foggy since I changed the "return false" to "return > keyword_differs" like 2 months ago, but if I remember correctly, I think if we > return false, the view does not get rerendered. So if we just returned false > here like before, if the keyword changes without the text changing, the view > will not change, and we will have to type an extra character for the view to > update and see the keyword change. > > So, I guess it might be more correct to say "if a view-visible change occurs" > instead of "when a significant change occurs"? "if the view needs to be repainted"? https://codereview.chromium.org/1855423003/diff/440001/chrome/browser/ui/view... File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right): https://codereview.chromium.org/1855423003/diff/440001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views.cc:528: GetTextState(text_state_before_change_); Seems like you want to make similar changes to OmniboxViewMac. https://codereview.chromium.org/1855423003/diff/440001/chrome/browser/ui/view... File chrome/browser/ui/views/omnibox/omnibox_view_views.h (right): https://codereview.chromium.org/1855423003/diff/440001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views.h:210: // different presentation (smaller font size). This is used for opups. Oops? https://codereview.chromium.org/1855423003/diff/440001/components/omnibox/bro... File components/omnibox/browser/omnibox_edit_model.h (right): https://codereview.chromium.org/1855423003/diff/440001/components/omnibox/bro... components/omnibox/browser/omnibox_edit_model.h:86: struct TextStateChange { Nit: I think this should be defined in OmniboxView instead of here. It seems like everything else related to this, including the method that actually computes it, is defined in that class already, and while its consumer is in this class, all its concepts relate to things in the view. Assuming you follow my name suggestion there, I would then name this StateChanges and document like "Represents the changes between two State objects. This is used by the model to determine how its internal state should be updated after the view state changes. See OmniboxEditModel::OnAfterPossibleChange()." https://codereview.chromium.org/1855423003/diff/440001/components/omnibox/bro... File components/omnibox/browser/omnibox_view.cc (right): https://codereview.chromium.org/1855423003/diff/440001/components/omnibox/bro... components/omnibox/browser/omnibox_view.cc:192: !((old_sel_empty && new_sel_empty) || sel_same_ignoring_direction); Nit: Distribute ! through https://codereview.chromium.org/1855423003/diff/440001/components/omnibox/bro... File components/omnibox/browser/omnibox_view.h (right): https://codereview.chromium.org/1855423003/diff/440001/components/omnibox/bro... components/omnibox/browser/omnibox_view.h:238: struct TextState { Nit: I would name and document this as follows: // Tracks important state that may change between OnBeforePossibleChange() and // OnAfterPossibleChange(). struct State { ("TextState" seems inaccurate because the keyword isn't really part of the "text".) https://codereview.chromium.org/1855423003/diff/440001/components/omnibox/bro... components/omnibox/browser/omnibox_view.h:250: void GetTextState(TextState& state); Nit: Arguments which the function modifies must be passed by pointer, not non-const ref. https://codereview.chromium.org/1855423003/diff/440001/components/omnibox/bro... components/omnibox/browser/omnibox_view.h:253: // |TextStateChange| is passed to |OmniboxEditModel::OnAfterPossibleChange|. Nit: No || on type names. Actually, assuming you follow my documentation suggestion for the state change class (see comments elsewhere), I'd just remove this second sentence, as those comments cover it better.
The CQ bit was checked by thomasanderson@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1855423003/460001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
Patchset #22 (id:460001) has been deleted
The CQ bit was checked by thomasanderson@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1855423003/480001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by thomasanderson@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1855423003/500001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by thomasanderson@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1855423003/520001
https://codereview.chromium.org/1855423003/diff/440001/chrome/browser/ui/view... File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right): https://codereview.chromium.org/1855423003/diff/440001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views.cc:528: GetTextState(text_state_before_change_); On 2016/06/04 02:17:18, Peter Kasting wrote: > Seems like you want to make similar changes to OmniboxViewMac. Done. https://codereview.chromium.org/1855423003/diff/440001/chrome/browser/ui/view... File chrome/browser/ui/views/omnibox/omnibox_view_views.h (right): https://codereview.chromium.org/1855423003/diff/440001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views.h:210: // different presentation (smaller font size). This is used for opups. On 2016/06/04 02:17:18, Peter Kasting wrote: > Oops? oops https://codereview.chromium.org/1855423003/diff/440001/components/omnibox/bro... File components/omnibox/browser/omnibox_edit_model.h (right): https://codereview.chromium.org/1855423003/diff/440001/components/omnibox/bro... components/omnibox/browser/omnibox_edit_model.h:86: struct TextStateChange { On 2016/06/04 02:17:18, Peter Kasting wrote: > Nit: I think this should be defined in OmniboxView instead of here. It seems > like everything else related to this, including the method that actually > computes it, is defined in that class already, and while its consumer is in this > class, all its concepts relate to things in the view. > > Assuming you follow my name suggestion there, I would then name this > StateChanges and document like "Represents the changes between two State > objects. This is used by the model to determine how its internal state should > be updated after the view state changes. See > OmniboxEditModel::OnAfterPossibleChange()." Done, but unfortunately you cannot forward-declare nested classes, so I instead moved this to OmniboxViewStateChanges https://codereview.chromium.org/1855423003/diff/440001/components/omnibox/bro... File components/omnibox/browser/omnibox_view.cc (right): https://codereview.chromium.org/1855423003/diff/440001/components/omnibox/bro... components/omnibox/browser/omnibox_view.cc:192: !((old_sel_empty && new_sel_empty) || sel_same_ignoring_direction); On 2016/06/04 02:17:18, Peter Kasting wrote: > Nit: Distribute ! through Done. https://codereview.chromium.org/1855423003/diff/440001/components/omnibox/bro... File components/omnibox/browser/omnibox_view.h (right): https://codereview.chromium.org/1855423003/diff/440001/components/omnibox/bro... components/omnibox/browser/omnibox_view.h:238: struct TextState { On 2016/06/04 02:17:18, Peter Kasting wrote: > Nit: I would name and document this as follows: > > // Tracks important state that may change between OnBeforePossibleChange() and > // OnAfterPossibleChange(). > struct State { > > ("TextState" seems inaccurate because the keyword isn't really part of the > "text".) Done. https://codereview.chromium.org/1855423003/diff/440001/components/omnibox/bro... components/omnibox/browser/omnibox_view.h:250: void GetTextState(TextState& state); On 2016/06/04 02:17:18, Peter Kasting wrote: > Nit: Arguments which the function modifies must be passed by pointer, not > non-const ref. Done. https://codereview.chromium.org/1855423003/diff/440001/components/omnibox/bro... components/omnibox/browser/omnibox_view.h:253: // |TextStateChange| is passed to |OmniboxEditModel::OnAfterPossibleChange|. On 2016/06/04 02:17:18, Peter Kasting wrote: > Nit: No || on type names. Actually, assuming you follow my documentation > suggestion for the state change class (see comments elsewhere), I'd just remove > this second sentence, as those comments cover it better. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1855423003/diff/300001/chrome/browser/autocom... File chrome/browser/autocomplete/autocomplete_browsertest.cc (right): https://codereview.chromium.org/1855423003/diff/300001/chrome/browser/autocom... chrome/browser/autocomplete/autocomplete_browsertest.cc:335: // they should be left with '?' in the omnibox. On 2016/06/04 02:17:18, Peter Kasting wrote: > On 2016/06/02 19:24:39, Tom Anderson wrote: > > On 2016/06/01 01:07:27, Peter Kasting wrote: > > > Just so I'm clear: this test needed to move elsewhere to fix flakiness, > right? > > > > > What about the other tests from this testcase, are they similarly in need of > > > movement? I'm wondering if we should be moving them all at once, or if any > > > overlap with other existing non-flaky tests and should just be deleted. > > > > No, they're both flaky :P > > Meaning, the remaining tests are flaky, or meaning that both old and new > versions of this test are flaky? > > If the latter, does that mean you never figured out the cause of flakiness? Are > you going to keep trying on that front? Both the new and old versions are flaky, and unfortunately I never found the cause of the flakiness. However, the bots are always reliable, and if they fail, they fail in the same way across the board. :S Also, I should point out that existing tests are flaky as well. For example, if I run the omnibox-related browser tests locally (without the changes), about 1/3 of them will fail. I'm probably not going to look into this flakiness further.
https://codereview.chromium.org/1855423003/diff/300001/chrome/browser/autocom... File chrome/browser/autocomplete/autocomplete_browsertest.cc (right): https://codereview.chromium.org/1855423003/diff/300001/chrome/browser/autocom... chrome/browser/autocomplete/autocomplete_browsertest.cc:335: // they should be left with '?' in the omnibox. On 2016/06/04 22:20:48, Tom Anderson wrote: > On 2016/06/04 02:17:18, Peter Kasting wrote: > > On 2016/06/02 19:24:39, Tom Anderson wrote: > > > On 2016/06/01 01:07:27, Peter Kasting wrote: > > > > Just so I'm clear: this test needed to move elsewhere to fix flakiness, > > right? > > > > > > > What about the other tests from this testcase, are they similarly in need > of > > > > movement? I'm wondering if we should be moving them all at once, or if > any > > > > overlap with other existing non-flaky tests and should just be deleted. > > > > > > No, they're both flaky :P > > > > Meaning, the remaining tests are flaky, or meaning that both old and new > > versions of this test are flaky? > > > > If the latter, does that mean you never figured out the cause of flakiness? > Are > > you going to keep trying on that front? > > Both the new and old versions are flaky, and unfortunately I never found the > cause of the flakiness. However, the bots are always reliable, and if they > fail, they fail in the same way across the board. :S > Also, I should point out that existing tests are flaky as well. For example, if > I run the omnibox-related browser tests locally (without the changes), about 1/3 > of them will fail. I'm probably not going to look into this flakiness further. What does "the bots are always reliable" mean? Does that mean "these tests don't seem to fail on the bots, they're only flaky locally"? If not, and these are also flaky on the bots, how come they're not already disabled? Do we just have flaky tests turning various runs red and no one has done anything about it? We don't want to be in a world where we have flaky tests running on the bots; it's better to have no tests than flaky ones. Especially if you've touched these tests this much, flakiness in them is going to get sent straight back to you, so you'll end up investigating this anyway... https://codereview.chromium.org/1855423003/diff/520001/components/omnibox/bro... File components/omnibox/browser/omnibox_view.h (right): https://codereview.chromium.org/1855423003/diff/520001/components/omnibox/bro... components/omnibox/browser/omnibox_view.h:41: struct OmniboxViewStateChanges { Rather than declare this outside OmniboxView, can we just flip the includes around and have OmniboxEditModel #include this header, and this header forward-declare that class (and then have the .cc file #include the relevant header)? It looks like the only uses of OmniboxEditModel in this file are ones that should work with forward-declarations.
The CQ bit was checked by thomasanderson@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1855423003/540001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
On 2016/06/05 01:20:27, Peter Kasting wrote: > https://codereview.chromium.org/1855423003/diff/300001/chrome/browser/autocom... > File chrome/browser/autocomplete/autocomplete_browsertest.cc (right): > > https://codereview.chromium.org/1855423003/diff/300001/chrome/browser/autocom... > chrome/browser/autocomplete/autocomplete_browsertest.cc:335: // they should be > left with '?' in the omnibox. > On 2016/06/04 22:20:48, Tom Anderson wrote: > > On 2016/06/04 02:17:18, Peter Kasting wrote: > > > On 2016/06/02 19:24:39, Tom Anderson wrote: > > > > On 2016/06/01 01:07:27, Peter Kasting wrote: > > > > > Just so I'm clear: this test needed to move elsewhere to fix flakiness, > > > right? > > > > > > > > > What about the other tests from this testcase, are they similarly in > need > > of > > > > > movement? I'm wondering if we should be moving them all at once, or if > > any > > > > > overlap with other existing non-flaky tests and should just be deleted. > > > > > > > > No, they're both flaky :P > > > > > > Meaning, the remaining tests are flaky, or meaning that both old and new > > > versions of this test are flaky? > > > > > > If the latter, does that mean you never figured out the cause of flakiness? > > Are > > > you going to keep trying on that front? > > > > Both the new and old versions are flaky, and unfortunately I never found the > > cause of the flakiness. However, the bots are always reliable, and if they > > fail, they fail in the same way across the board. :S > > Also, I should point out that existing tests are flaky as well. For example, > if > > I run the omnibox-related browser tests locally (without the changes), about > 1/3 > > of them will fail. I'm probably not going to look into this flakiness > further. > > What does "the bots are always reliable" mean? Does that mean "these tests > don't seem to fail on the bots, they're only flaky locally"? If not, and these > are also flaky on the bots, how come they're not already disabled? Do we just > have flaky tests turning various runs red and no one has done anything about it? > I mean that the PASS/FAIL given by the bots is accurate. There are neither false positives nor false negatives on the bots, so we shouldn't disable any tests. However, there are false negatives on my machine. It's due to a race condition between the test runner and the browser. For example, if we wait for, say, a transition into keyword mode, and I add a printf on the transition, sometimes the test will pass, and I see the print before the PASS indication; other times, the test will fail, but I still see the print after the FAIL while the test is being torn down. I've tried synchronizing with the browser by using the various testing APIs. The most extreme one waited until the message pump was empty before unblocking, and even that didn't work. I tried adding an artificial sleep(1), but I still get the same flaky behavior: immediately after the 1s is up, I get a print and a PASS, or a FAIL and a print. I also tried running with single-process and single_process. > We don't want to be in a world where we have flaky tests running on the bots; > it's better to have no tests than flaky ones. > > Especially if you've touched these tests this much, flakiness in them is going > to get sent straight back to you, so you'll end up investigating this anyway... > > https://codereview.chromium.org/1855423003/diff/520001/components/omnibox/bro... > File components/omnibox/browser/omnibox_view.h (right): > > https://codereview.chromium.org/1855423003/diff/520001/components/omnibox/bro... > components/omnibox/browser/omnibox_view.h:41: struct OmniboxViewStateChanges { > Rather than declare this outside OmniboxView, can we just flip the includes > around and have OmniboxEditModel #include this header, and this header > forward-declare that class (and then have the .cc file #include the relevant > header)? It looks like the only uses of OmniboxEditModel in this file are ones > that should work with forward-declarations. Done.
The CQ bit was checked by thomasanderson@google.com to run a CQ dry run
jannis.beyer22@gmail.com changed reviewers: + jannis.beyer22@gmail.com
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1855423003/560001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by thomasanderson@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1855423003/580001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
On 2016/06/05 02:07:37, Tom Anderson wrote: > On 2016/06/05 01:20:27, Peter Kasting wrote: > > What does "the bots are always reliable" mean? Does that mean "these tests > > don't seem to fail on the bots, they're only flaky locally"? If not, and > these > > are also flaky on the bots, how come they're not already disabled? Do we just > > have flaky tests turning various runs red and no one has done anything about > it? > > > > I mean that the PASS/FAIL given by the bots is accurate. There are neither > false positives nor false negatives on the bots, so we shouldn't disable any > tests. > > However, there are false negatives on my machine. It's due to a race condition > between the test runner and the browser. For example, if we wait for, say, a > transition into keyword mode, and I add a printf on the transition, sometimes > the test will pass, and I see the print before the PASS indication; other > times, the test will fail, but I still see the print after the FAIL while the > test is being torn down. I've tried synchronizing with the browser by using the > various testing APIs. The most extreme one waited until the message pump was > empty before unblocking, and even that didn't work. I tried adding an > artificial sleep(1), but I still get the same flaky behavior: immediately after > the 1s is up, I get a print and a PASS, or a FAIL and a print. I also tried > running with single-process and single_process. This really worries me. If the test is flaky on your machine, then it's potentially flaky on the bot as well, even if we aren't observing any flakiness. This feels like a timebomb. Did you consult with some of the testing experts, e.g. phajdan.jr, on possible ways to fix this? I guess I would be OK landing this change if it doesn't make anything worse, but I'd really like to mandate making the tests not flaky as a separate CL in that case. I don't think this should be indefinitely on you to bang your head against, I'm hoping someone more experienced with fixing test flakiness can bring more tools to bear. LGTM otherwise.
The CQ bit was checked by thomasanderson@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1855423003/600001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by thomasanderson@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mpearson@chromium.org, pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/1855423003/#ps600001 (title: "Add includes for mac tests")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1855423003/600001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
thomasanderson@google.com changed reviewers: + jochen@chromium.org
+ jochen@ for OWNERS approval for chrome/test/base/ui_test_utils.cc
lgtm
The CQ bit was checked by thomasanderson@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1855423003/600001
Message was sent while issue was closed.
Description was changed from ========== Interpret '?' and Ctrl-K or Ctrl-E as putting omnibox in keyword search mode for Default Search Provider. When the user presses Ctrl-K or Ctrl-E: Transition the user into keyword mode using their default search provider. Pressing Ctrl-K will give the omnibox focus, and will display "Search Google:" on the left. If the user has already entered text into the omnibox and presses Ctrl-K, their text will be preserved, but they will be transitioned into keyword mode anyway. If the user is already in keyword mode (even if it's with a non-default search provider), the text they have entered will become selected. Upon pressing backspace, they will be left with an empty omnibox. When the user types '?' into the omnibox: If the user types or pastes a '?' at the beginning of their (possibly empty) input text, transition them into keyword mode with their default search provider. Upon pressing backspace, the omnibox will be left '?' (and their input). Remove forced queries. We should be able to search for '?' directly. BUG=267629,592540 ========== to ========== Interpret '?' and Ctrl-K or Ctrl-E as putting omnibox in keyword search mode for Default Search Provider. When the user presses Ctrl-K or Ctrl-E: Transition the user into keyword mode using their default search provider. Pressing Ctrl-K will give the omnibox focus, and will display "Search Google:" on the left. If the user has already entered text into the omnibox and presses Ctrl-K, their text will be preserved, but they will be transitioned into keyword mode anyway. If the user is already in keyword mode (even if it's with a non-default search provider), the text they have entered will become selected. Upon pressing backspace, they will be left with an empty omnibox. When the user types '?' into the omnibox: If the user types or pastes a '?' at the beginning of their (possibly empty) input text, transition them into keyword mode with their default search provider. Upon pressing backspace, the omnibox will be left '?' (and their input). Remove forced queries. We should be able to search for '?' directly. BUG=267629,592540 ==========
Message was sent while issue was closed.
Committed patchset #28 (id:600001)
Message was sent while issue was closed.
Description was changed from ========== Interpret '?' and Ctrl-K or Ctrl-E as putting omnibox in keyword search mode for Default Search Provider. When the user presses Ctrl-K or Ctrl-E: Transition the user into keyword mode using their default search provider. Pressing Ctrl-K will give the omnibox focus, and will display "Search Google:" on the left. If the user has already entered text into the omnibox and presses Ctrl-K, their text will be preserved, but they will be transitioned into keyword mode anyway. If the user is already in keyword mode (even if it's with a non-default search provider), the text they have entered will become selected. Upon pressing backspace, they will be left with an empty omnibox. When the user types '?' into the omnibox: If the user types or pastes a '?' at the beginning of their (possibly empty) input text, transition them into keyword mode with their default search provider. Upon pressing backspace, the omnibox will be left '?' (and their input). Remove forced queries. We should be able to search for '?' directly. BUG=267629,592540 ========== to ========== Interpret '?' and Ctrl-K or Ctrl-E as putting omnibox in keyword search mode for Default Search Provider. When the user presses Ctrl-K or Ctrl-E: Transition the user into keyword mode using their default search provider. Pressing Ctrl-K will give the omnibox focus, and will display "Search Google:" on the left. If the user has already entered text into the omnibox and presses Ctrl-K, their text will be preserved, but they will be transitioned into keyword mode anyway. If the user is already in keyword mode (even if it's with a non-default search provider), the text they have entered will become selected. Upon pressing backspace, they will be left with an empty omnibox. When the user types '?' into the omnibox: If the user types or pastes a '?' at the beginning of their (possibly empty) input text, transition them into keyword mode with their default search provider. Upon pressing backspace, the omnibox will be left '?' (and their input). Remove forced queries. We should be able to search for '?' directly. BUG=267629,592540 Committed: https://crrev.com/00687d006b3b838d1274f52a2dee455ce988d2fd Cr-Commit-Position: refs/heads/master@{#398580} ==========
Message was sent while issue was closed.
Patchset 28 (id:??) landed as https://crrev.com/00687d006b3b838d1274f52a2dee455ce988d2fd Cr-Commit-Position: refs/heads/master@{#398580}
Message was sent while issue was closed.
Nice work on this involved changelist. I'm not sure if you noticed what I mentioned in message #120: Can you please file a bug to get the support pages updated? I think because we're not displaying a question mark anymore, some of them are a bit misleading. I found these two: https://support.google.com/chromebook/answer/183101?hl=en https://support.google.com/chrome/answer/157179?hl=en (section on "Address Bar") There may be others. --mark
Message was sent while issue was closed.
On Wed, Jun 8, 2016 at 12:56 PM, <mpearson@chromium.org> wrote: > Nice work on this involved changelist. > > I'm not sure if you noticed what I mentioned in message #120: > > Can you please file a bug to get the support pages updated? > I think this comment got ignored, so I filed a bug for you: b/29314703 <https://buganizer.corp.google.com/29314703> --mark > I think because > we're not displaying a question mark anymore, some of them are a bit > misleading. I found these two: > https://support.google.com/chromebook/answer/183101?hl=en > https://support.google.com/chrome/answer/157179?hl=en (section on "Address > Bar") > There may be others. > > --mark > > https://codereview.chromium.org/1855423003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
On 2016/06/13 18:09:13, Mark P wrote: > On Wed, Jun 8, 2016 at 12:56 PM, <mailto:mpearson@chromium.org> wrote: > > > Nice work on this involved changelist. > > > > I'm not sure if you noticed what I mentioned in message #120: > > > > Can you please file a bug to get the support pages updated? > > > > I think this comment got ignored, so I filed a bug for you: b/29314703 > <https://buganizer.corp.google.com/29314703> > > --mark > > > > > I think because > > we're not displaying a question mark anymore, some of them are a bit > > misleading. I found these two: > > https://support.google.com/chromebook/answer/183101?hl=en > > https://support.google.com/chrome/answer/157179?hl=en (section on "Address > > Bar") > > There may be others. > > > > --mark > > > > https://codereview.chromium.org/1855423003/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. Thanks Mark. Sorry for ignoring that, I meant to do that a while ago |
