Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2435103002:
Omnibox: Preserve display text and select all on a focus search (Closed)
|
Created:
4 years, 2 months ago by Tom (Use chromium acct) Modified:
4 years, 1 month ago Reviewers:
Peter Kasting CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionOmnibox: Preserve display text and select all on a focus search
BUG=657243
R=pkasting@chromium.org
Committed: https://crrev.com/795d4bec22b00f41f4a87e193494a728d73aa72d
Cr-Commit-Position: refs/heads/master@{#427535}
Patch Set 1 : Initial PS to see if any tests are failing #Patch Set 2 : Add browser tests #
Total comments: 4
Patch Set 3 : Remove printfs #
Total comments: 22
Patch Set 4 : Refactor #
Total comments: 6
Patch Set 5 : Refactor #Patch Set 6 : Use display_text.length() as caret pos for KEYBOARD_SHORTCUT #
Messages
Total messages: 30 (17 generated)
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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Initial PS to see if any tests are failing BUG=657243 R=pkasting@chromium.org ========== to ========== Omnibox: Preserve display text and select all on a focus search BUG=657243 R=pkasting@chromium.org ==========
https://chromiumcodereview.appspot.com/2435103002/diff/20001/components/omnib... File components/omnibox/browser/omnibox_edit_model.cc (right): https://chromiumcodereview.appspot.com/2435103002/diff/20001/components/omnib... components/omnibox/browser/omnibox_edit_model.cc:564: user_input_in_progress_ = true; Should this be SetInputInProgress(true)?
Only looked at omnibox_edit_model.cc. https://codereview.chromium.org/2435103002/diff/20001/components/omnibox/brow... File components/omnibox/browser/omnibox_edit_model.cc (right): https://codereview.chromium.org/2435103002/diff/20001/components/omnibox/brow... components/omnibox/browser/omnibox_edit_model.cc:418: // or temporary text. We need to touch this function. Because you're changing what the user text is, the "case 1" code will be triggered more often, when we don't actually want it to (e.g. all the case 2 cases). Basically, every time the user hits ctrl-K, we want to use the "case 2 or 3" code here, so we compute autocomplete suggestions based on the cursor being at the end of the new text, instead of possibly somewhere in the middle. I haven't thought about what the new code or comments need to be. https://codereview.chromium.org/2435103002/diff/20001/components/omnibox/brow... components/omnibox/browser/omnibox_edit_model.cc:564: user_input_in_progress_ = true; On 2016/10/20 20:34:54, Tom Anderson wrote: > Should this be SetInputInProgress(true)? Yes, I think so. You should consider using SetUserText() in place of these two lines. This would also allow deleting "is_keyword_hint_ = false;" below. I would double-check that that doesn't do anything you explicitly don't want.
The CQ bit was checked by thomasanderson@google.com to run a CQ dry run
https://codereview.chromium.org/2435103002/diff/20001/components/omnibox/brow... File components/omnibox/browser/omnibox_edit_model.cc (right): https://codereview.chromium.org/2435103002/diff/20001/components/omnibox/brow... components/omnibox/browser/omnibox_edit_model.cc:418: // or temporary text. On 2016/10/20 21:42:00, Peter Kasting wrote: > We need to touch this function. Because you're changing what the user text is, > the "case 1" code will be triggered more often, when we don't actually want it > to (e.g. all the case 2 cases). Basically, every time the user hits ctrl-K, we > want to use the "case 2 or 3" code here, so we compute autocomplete suggestions > based on the cursor being at the end of the new text, instead of possibly > somewhere in the middle. > > I haven't thought about what the new code or comments need to be. I couldn't really figure out a way to make this work, so I just decided to make the callers responsible for handling the caret position. I think this is better than StartAutocomplete trying to figure out the caret position based on various hints. https://codereview.chromium.org/2435103002/diff/40001/components/omnibox/brow... File components/omnibox/browser/omnibox_edit_model.cc (right): https://codereview.chromium.org/2435103002/diff/40001/components/omnibox/brow... components/omnibox/browser/omnibox_edit_model.cc:542: case KeywordModeEntryMethod::KEYBOARD_SHORTCUT: I don't know if it's better for EnterKeywordModeForDefaultSearchProvider to calculate the display text and selection, or if it's better to pass these along as arguments. It might make the "?" trimming below more clear if they're arguments, but I want to hear what you think https://codereview.chromium.org/2435103002/diff/40001/components/omnibox/brow... components/omnibox/browser/omnibox_edit_model.cc:550: view_->SetWindowTextAndCaretPos(view_->GetText(), 0, false, false); It would be useful if OmniboxView had a SetUserTextAndCaretPos, don't you think?
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2435103002/diff/40001/chrome/browser/autocomp... File chrome/browser/autocomplete/autocomplete_browsertest.cc (right): https://codereview.chromium.org/2435103002/diff/40001/chrome/browser/autocomp... chrome/browser/autocomplete/autocomplete_browsertest.cc:333: // Focus search when the permanent URL is showing should result in an empty Nit: Focus search -> Calling FocusSearch() https://codereview.chromium.org/2435103002/diff/40001/chrome/browser/ui/omnib... File chrome/browser/ui/omnibox/omnibox_view_browsertest.cc (right): https://codereview.chromium.org/2435103002/diff/40001/chrome/browser/ui/omnib... chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:887: // Make sure the display text is preserved on a focus search when the display Nit: on a focus search when -> when calling FocusSearch() while https://codereview.chromium.org/2435103002/diff/40001/chrome/browser/ui/omnib... chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:899: ASSERT_TRUE(popup_model->IsOpen()); Nit: Change ASSERT to EXPECT on any of these where failing the condition won't result in a segfault (or similar) if we keep running https://codereview.chromium.org/2435103002/diff/40001/chrome/browser/ui/omnib... chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:906: // Focus searching with autocomplete text should preserve the autocompleted Nit: Focus searching with autocomplete text -> Calling FocusSearch() with an inline autocompletion https://codereview.chromium.org/2435103002/diff/40001/chrome/browser/ui/omnib... chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:923: // Focus searching when we select a non-default autocomplete entry should Nit: Calling FocusSearch() with temporary text showing should https://codereview.chromium.org/2435103002/diff/40001/components/omnibox/brow... File components/omnibox/browser/omnibox_edit_model.cc (right): https://codereview.chromium.org/2435103002/diff/40001/components/omnibox/brow... components/omnibox/browser/omnibox_edit_model.cc:425: client_->CurrentPageExists() ? client_->GetURL() : GURL(), ClassifyPage(), Seems like this isn't the only place that does this... wondering if GetURL() itself should handle this case and simplify the callers. https://codereview.chromium.org/2435103002/diff/40001/components/omnibox/brow... components/omnibox/browser/omnibox_edit_model.cc:545: view_->SelectAll(false); Nit: Personally I'd write this as a call to the Model's SetUserText(), followed by an explicit SetWindowTextAndCaretPos() call. This is less redundant under the hood, and it's more parallel with the code below. https://codereview.chromium.org/2435103002/diff/40001/components/omnibox/brow... components/omnibox/browser/omnibox_edit_model.cc:549: view_->SetUserText(view_->GetText().substr(1), false); No reason for "view_->" on this, that's just a wrapper around a call back to the model + a call to SetWindowTextAndCaretPos() (that you'll be overriding). https://codereview.chromium.org/2435103002/diff/40001/components/omnibox/brow... components/omnibox/browser/omnibox_edit_model.cc:550: view_->SetWindowTextAndCaretPos(view_->GetText(), 0, false, false); On 2016/10/25 02:11:08, Tom Anderson wrote: > It would be useful if OmniboxView had a SetUserTextAndCaretPos, don't you think? I don't think so, but we can revisit after responding to my other ideas here, to see if you still think this makes sense. https://codereview.chromium.org/2435103002/diff/40001/components/omnibox/brow... components/omnibox/browser/omnibox_edit_model.cc:559: keyword_mode_entry_method_ = entry_method; Nit: I would move this block above the switch. See next comment for why. https://codereview.chromium.org/2435103002/diff/40001/components/omnibox/brow... components/omnibox/browser/omnibox_edit_model.cc:563: StartAutocomplete(sel_start != sel_end, false); The primary difference between this and just calling UdpatePopup() is that this always allows inline autocompletion, even if the user entered keyword mode by placing a question mark at the beginning of the text. Is that what we want? If I put my cursor at the start of my text and hit '?', should I not only toggle into keyword mode but potentially get an inline autocompletion at the end of my search query? (Practically speaking, this would be rare; it depends on the scores from the keyword suggestion service, I think?) I suspect we probably want the UpdatePopup() behavior. If so, and if you move the keyword-related block up and change to using SetWindowTextAndCaretPos() consistently above (as my previous comments suggest), then you can pass trus for the |update_popup| arguments of those SetWindowTextAndCaretPos() calls and eliminate this entirely. BTW, I don't know what should be passed for |notify_text_changed| -- I don't recall offhand what that does and I didn't bother to look it up. You should look into that too.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2435103002/diff/40001/chrome/browser/autocomp... File chrome/browser/autocomplete/autocomplete_browsertest.cc (right): https://codereview.chromium.org/2435103002/diff/40001/chrome/browser/autocomp... chrome/browser/autocomplete/autocomplete_browsertest.cc:333: // Focus search when the permanent URL is showing should result in an empty On 2016/10/25 02:46:33, Peter Kasting wrote: > Nit: Focus search -> Calling FocusSearch() Done. https://codereview.chromium.org/2435103002/diff/40001/chrome/browser/ui/omnib... File chrome/browser/ui/omnibox/omnibox_view_browsertest.cc (right): https://codereview.chromium.org/2435103002/diff/40001/chrome/browser/ui/omnib... chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:887: // Make sure the display text is preserved on a focus search when the display On 2016/10/25 02:46:33, Peter Kasting wrote: > Nit: on a focus search when -> when calling FocusSearch() while Done. https://codereview.chromium.org/2435103002/diff/40001/chrome/browser/ui/omnib... chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:899: ASSERT_TRUE(popup_model->IsOpen()); On 2016/10/25 02:46:33, Peter Kasting wrote: > Nit: Change ASSERT to EXPECT on any of these where failing the condition won't > result in a segfault (or similar) if we keep running Done. https://codereview.chromium.org/2435103002/diff/40001/chrome/browser/ui/omnib... chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:906: // Focus searching with autocomplete text should preserve the autocompleted On 2016/10/25 02:46:33, Peter Kasting wrote: > Nit: Focus searching with autocomplete text -> Calling FocusSearch() with an > inline autocompletion Done. https://codereview.chromium.org/2435103002/diff/40001/chrome/browser/ui/omnib... chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:923: // Focus searching when we select a non-default autocomplete entry should On 2016/10/25 02:46:33, Peter Kasting wrote: > Nit: Calling FocusSearch() with temporary text showing should Done. https://codereview.chromium.org/2435103002/diff/40001/components/omnibox/brow... File components/omnibox/browser/omnibox_edit_model.cc (right): https://codereview.chromium.org/2435103002/diff/40001/components/omnibox/brow... components/omnibox/browser/omnibox_edit_model.cc:425: client_->CurrentPageExists() ? client_->GetURL() : GURL(), ClassifyPage(), On 2016/10/25 02:46:33, Peter Kasting wrote: > Seems like this isn't the only place that does this... wondering if GetURL() > itself should handle this case and simplify the callers. Done. https://codereview.chromium.org/2435103002/diff/40001/components/omnibox/brow... components/omnibox/browser/omnibox_edit_model.cc:545: view_->SelectAll(false); On 2016/10/25 02:46:33, Peter Kasting wrote: > Nit: Personally I'd write this as a call to the Model's SetUserText(), followed > by an explicit SetWindowTextAndCaretPos() call. This is less redundant under > the hood, and it's more parallel with the code below. The doc on SetUserText says only the view should call it (is this out of date?) https://cs.chromium.org/chromium/src/components/omnibox/browser/omnibox_edit_... Also SetWindowTextAndCaretPos has no mechanism for selecing all (unless you meant SetUserText(); SetWindowTextAndCaretPos(); SelectAll(); ?) https://codereview.chromium.org/2435103002/diff/40001/components/omnibox/brow... components/omnibox/browser/omnibox_edit_model.cc:549: view_->SetUserText(view_->GetText().substr(1), false); On 2016/10/25 02:46:33, Peter Kasting wrote: > No reason for "view_->" on this, that's just a wrapper around a call back to the > model + a call to SetWindowTextAndCaretPos() (that you'll be overriding). Ok, done. I've also made the changes above. I used InternalSetUserText instead of SetUserText because the latter clears the keyword state. https://codereview.chromium.org/2435103002/diff/40001/components/omnibox/brow... components/omnibox/browser/omnibox_edit_model.cc:563: StartAutocomplete(sel_start != sel_end, false); On 2016/10/25 02:46:33, Peter Kasting wrote: > The primary difference between this and just calling UdpatePopup() is that this > always allows inline autocompletion, even if the user entered keyword mode by > placing a question mark at the beginning of the text. > > Is that what we want? If I put my cursor at the start of my text and hit '?', > should I not only toggle into keyword mode but potentially get an inline > autocompletion at the end of my search query? (Practically speaking, this would > be rare; it depends on the scores from the keyword suggestion service, I think?) > I'd argue we want the UpdatePopup() behavior because it wouldn't disturb the cursor position > I suspect we probably want the UpdatePopup() behavior. > > If so, and if you move the keyword-related block up and change to using > SetWindowTextAndCaretPos() consistently above (as my previous comments suggest), > then you can pass trus for the |update_popup| arguments of those > SetWindowTextAndCaretPos() calls and eliminate this entirely. > Done. I like this solution better. > BTW, I don't know what should be passed for |notify_text_changed| -- I don't > recall offhand what that does and I didn't bother to look it up. You should > look into that too.
LGTM https://codereview.chromium.org/2435103002/diff/60001/chrome/browser/ui/omnib... File chrome/browser/ui/omnibox/chrome_omnibox_client.cc (right): https://codereview.chromium.org/2435103002/diff/60001/chrome/browser/ui/omnib... chrome/browser/ui/omnibox/chrome_omnibox_client.cc:143: return controller_->GetWebContents(); This doesn't produce a "forcing to true/false (performance warning)" on MSVC? https://codereview.chromium.org/2435103002/diff/60001/components/omnibox/brow... File components/omnibox/browser/omnibox_edit_model.cc (right): https://codereview.chromium.org/2435103002/diff/60001/components/omnibox/brow... components/omnibox/browser/omnibox_edit_model.cc:551: InternalSetUserText(display_text); Nit: Could move these next lines (that the two cases have in common) down below the switch, and then guard the SelectAll() call with a conditional. Also, since in the QUESTION_MARK case |user_input_in_progress_| is set, we can sort-of share even more code. Here's a possible replacement for the whole switch: base::string16 display_text = user_input_in_progress_ ? view_->GetText() : base::string16(); if (entry_method == KeywordModeEntryMethod::QUESTION_MARK) display_text.erase(0, 1); InternalSetUserText(display_text); view_->SetWindowTextAndCaretPos(display_text, 0, true, false); if (entry_method == KeywordModeEntryMethod::KEYBOARD_SHORTCUT) view_->SelectAll(false);
https://codereview.chromium.org/2435103002/diff/60001/chrome/browser/ui/omnib... File chrome/browser/ui/omnibox/chrome_omnibox_client.cc (right): https://codereview.chromium.org/2435103002/diff/60001/chrome/browser/ui/omnib... chrome/browser/ui/omnibox/chrome_omnibox_client.cc:143: return controller_->GetWebContents(); On 2016/10/25 20:21:27, Peter Kasting wrote: > This doesn't produce a "forcing to true/false (performance warning)" on MSVC? idk, I didn't run a dry run on this PS. Reverted just in case https://codereview.chromium.org/2435103002/diff/60001/components/omnibox/brow... File components/omnibox/browser/omnibox_edit_model.cc (right): https://codereview.chromium.org/2435103002/diff/60001/components/omnibox/brow... components/omnibox/browser/omnibox_edit_model.cc:551: InternalSetUserText(display_text); On 2016/10/25 20:21:27, Peter Kasting wrote: > Nit: Could move these next lines (that the two cases have in common) down below > the switch, and then guard the SelectAll() call with a conditional. > > Also, since in the QUESTION_MARK case |user_input_in_progress_| is set, we can > sort-of share even more code. Here's a possible replacement for the whole > switch: > > base::string16 display_text = > user_input_in_progress_ ? view_->GetText() : base::string16(); > if (entry_method == KeywordModeEntryMethod::QUESTION_MARK) > display_text.erase(0, 1); > > InternalSetUserText(display_text); > view_->SetWindowTextAndCaretPos(display_text, 0, true, false); > if (entry_method == KeywordModeEntryMethod::KEYBOARD_SHORTCUT) > view_->SelectAll(false); Done. https://codereview.chromium.org/2435103002/diff/60001/components/omnibox/brow... components/omnibox/browser/omnibox_edit_model.cc:552: view_->SetWindowTextAndCaretPos(display_text, 0, true, false); I wanted to clear up one thing I noticed before I land. Now that UpdatePopup() is called before SelectAll(), we're using 0 as the cursor position to AutocompleteInput (before adjusting for keyword). Is this right, or should it be len(display_text)? Or does it not matter?
https://codereview.chromium.org/2435103002/diff/60001/components/omnibox/brow... File components/omnibox/browser/omnibox_edit_model.cc (right): https://codereview.chromium.org/2435103002/diff/60001/components/omnibox/brow... components/omnibox/browser/omnibox_edit_model.cc:552: view_->SetWindowTextAndCaretPos(display_text, 0, true, false); On 2016/10/25 21:55:09, Tom Anderson wrote: > I wanted to clear up one thing I noticed before I land. > > Now that UpdatePopup() is called before SelectAll(), we're using 0 as the cursor > position to AutocompleteInput (before adjusting for keyword). > > Is this right, or should it be len(display_text)? Or does it not matter? Good question. In the case of QUESTION_MARK it should definitely be zero (that's where the cursor really is), in the KEYBOARD_SHORTCUT case it should probably be display_text.length(). I doubt it will make a huge difference, but that seems more correct. :/
On 2016/10/25 22:06:11, Peter Kasting wrote: > https://codereview.chromium.org/2435103002/diff/60001/components/omnibox/brow... > File components/omnibox/browser/omnibox_edit_model.cc (right): > > https://codereview.chromium.org/2435103002/diff/60001/components/omnibox/brow... > components/omnibox/browser/omnibox_edit_model.cc:552: > view_->SetWindowTextAndCaretPos(display_text, 0, true, false); > On 2016/10/25 21:55:09, Tom Anderson wrote: > > I wanted to clear up one thing I noticed before I land. > > > > Now that UpdatePopup() is called before SelectAll(), we're using 0 as the > cursor > > position to AutocompleteInput (before adjusting for keyword). > > > > Is this right, or should it be len(display_text)? Or does it not matter? > > Good question. In the case of QUESTION_MARK it should definitely be zero > (that's where the cursor really is), in the KEYBOARD_SHORTCUT case it should > probably be display_text.length(). I doubt it will make a huge difference, but > that seems more correct. :/ Done. Will try to land this PS unless you have any objections
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/v2/patch-status/codereview.chromium.or...
Sure, looks fine
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 pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2435103002/#ps100001 (title: "Use display_text.length() as caret pos for KEYBOARD_SHORTCUT")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Omnibox: Preserve display text and select all on a focus search BUG=657243 R=pkasting@chromium.org ========== to ========== Omnibox: Preserve display text and select all on a focus search BUG=657243 R=pkasting@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Omnibox: Preserve display text and select all on a focus search BUG=657243 R=pkasting@chromium.org ========== to ========== Omnibox: Preserve display text and select all on a focus search BUG=657243 R=pkasting@chromium.org Committed: https://crrev.com/795d4bec22b00f41f4a87e193494a728d73aa72d Cr-Commit-Position: refs/heads/master@{#427535} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/795d4bec22b00f41f4a87e193494a728d73aa72d Cr-Commit-Position: refs/heads/master@{#427535} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
