Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1943683002:
Refactor OmniboxEditModel::user_text_ to not include keyword (Closed)
|
Created:
4 years, 7 months ago by Tom (Use chromium acct) Modified:
4 years, 7 months ago Reviewers:
Peter Kasting CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRefactor OmniboxEditModel::user_text_ to not include keyword
Committed: https://crrev.com/2f005e51521f7e9f9695758b3400e0acd4d7bfc0
Cr-Commit-Position: refs/heads/master@{#394553}
Patch Set 1 #Patch Set 2 : Cleaned up a bit #
Total comments: 8
Patch Set 3 : Refactor, remove unnecessary calls to MaybeStripKeyword #
Total comments: 12
Patch Set 4 : Bugfixes #Patch Set 5 : Fix unit test compilation #Patch Set 6 : Fix Windows test #Patch Set 7 : Bugfixes #Patch Set 8 : Restore keyword state on tab switch #
Total comments: 16
Patch Set 9 : Add test coverage & refactor #Messages
Total messages: 43 (15 generated)
thomasanderson@google.com changed reviewers: + pkasting@chromium.org
https://codereview.chromium.org/1943683002/diff/20001/components/omnibox/brow... File components/omnibox/browser/omnibox_edit_model.cc (right): https://codereview.chromium.org/1943683002/diff/20001/components/omnibox/brow... components/omnibox/browser/omnibox_edit_model.cc:248: state->user_text, false); Nit: Rewrap https://codereview.chromium.org/1943683002/diff/20001/components/omnibox/brow... components/omnibox/browser/omnibox_edit_model.cc:343: user_text_ = MaybeStripKeyword(temp); (1) Why is this line necessary? If the call above sets |user_text_| in an undesirable way, at least explain that here (if not fix the call) (2) If this is necessary, why not just save off the |user_text_| instead of possibly-stripping a possibly-added keyword? https://codereview.chromium.org/1943683002/diff/20001/components/omnibox/brow... components/omnibox/browser/omnibox_edit_model.cc:497: user_text_.length() - user_text_.length(); Nit: Rewrap https://codereview.chromium.org/1943683002/diff/20001/components/omnibox/brow... components/omnibox/browser/omnibox_edit_model.cc:1077: user_text_ = MaybeStripKeyword(user_text_); When will this be needed? I'd think AcceptKeyword() would do this if we had changed into keyword mode.
https://codereview.chromium.org/1943683002/diff/20001/components/omnibox/brow... File components/omnibox/browser/omnibox_edit_model.cc (right): https://codereview.chromium.org/1943683002/diff/20001/components/omnibox/brow... components/omnibox/browser/omnibox_edit_model.cc:248: state->user_text, false); On 2016/05/07 01:42:39, Peter Kasting wrote: > Nit: Rewrap Done. https://codereview.chromium.org/1943683002/diff/20001/components/omnibox/brow... components/omnibox/browser/omnibox_edit_model.cc:343: user_text_ = MaybeStripKeyword(temp); On 2016/05/07 01:42:39, Peter Kasting wrote: > (1) Why is this line necessary? If the call above sets |user_text_| in an > undesirable way, at least explain that here (if not fix the call) > > (2) If this is necessary, why not just save off the |user_text_| instead of > possibly-stripping a possibly-added keyword? I wasn't 100% sure if the behavior of OmniboxClient::OnTextChanged relied on the user_text having an encoded keyword. Reverted this; not seeing any bad behavior so far. https://codereview.chromium.org/1943683002/diff/20001/components/omnibox/brow... components/omnibox/browser/omnibox_edit_model.cc:497: user_text_.length() - user_text_.length(); On 2016/05/07 01:42:39, Peter Kasting wrote: > Nit: Rewrap Done. https://codereview.chromium.org/1943683002/diff/20001/components/omnibox/brow... components/omnibox/browser/omnibox_edit_model.cc:1077: user_text_ = MaybeStripKeyword(user_text_); On 2016/05/07 01:42:39, Peter Kasting wrote: > When will this be needed? I'd think AcceptKeyword() would do this if we had > changed into keyword mode. CreatedKeywordSearchByInsertingSpaceInMiddle. I noticed OnAfterPossibleChange doesn't update is_keyword_hint_ so I didn't want to put the MaybeStripKeyword there. I've moved this and added setting the keyword state to OnAfterPossibleChange and all seems to work well.
https://codereview.chromium.org/1943683002/diff/40001/components/omnibox/brow... File components/omnibox/browser/omnibox_edit_model.cc (right): https://codereview.chromium.org/1943683002/diff/40001/components/omnibox/brow... components/omnibox/browser/omnibox_edit_model.cc:206: const base::string16 user_text(user_text_); I think this change means we'll now take the first arm of the conditional when someone switches tabs when in keyword mode with no visible text, when we didn't before. That doesn't seem right. I think you probably want something like: const base::string16 display_text = view_->GetText(); if (MaybePrependKeyword(display_text).empty()) { ... } else { InternalSetUserText(display_text); } Though if we're in some sort of temporary text keyword case, I think maybe the second arm is supposed to "accept" that keyword as a side effect. You probably want to test tabbing to keyword mode on a non-default dropdown match and then changing tabs away and back and seeing if the temporary keyword was "accepted" as the current state before and after your change. If you do find problems there, the change in AcceptTemporaryTextAsUserText() is probably worrisome as well. https://codereview.chromium.org/1943683002/diff/40001/components/omnibox/brow... components/omnibox/browser/omnibox_edit_model.cc:247: view_->SetUserText(state->user_text, state->user_text, false); I think this was the only place that passed different values for the first two args. This means we should simplify SetUserText() to just take one arg. https://codereview.chromium.org/1943683002/diff/40001/components/omnibox/brow... components/omnibox/browser/omnibox_edit_model.cc:484: // If we're in keyword mode, we're not displaying the full |user_text_|, so This comment is no longer correct, as we no longer have undisplayed text in |user_text_|. https://codereview.chromium.org/1943683002/diff/40001/components/omnibox/brow... components/omnibox/browser/omnibox_edit_model.cc:492: cursor_position += user_text_.length() - user_text_.length(); This statement now does nothing. In general, you won't notice obvious bad things if the cursor position here is set wrongly, so you'll need to debug/test carefully to ensure you're making the right adjustments. I would think you'd need something like: cursor_position += MaybePrependKeyword(user_text_).length() - user_text_.length(); ...And update the comment accordingly. https://codereview.chromium.org/1943683002/diff/40001/components/omnibox/brow... components/omnibox/browser/omnibox_edit_model.cc:575: has_temporary_text_ ? MaybePrependKeyword(user_text_) : input_.text(), We know is_keyword_selected() is false, so MaybePrependKeyword() here does nothing. However, I don't think using the |user_text_| is right. I don't believe that gets updated as the user tabs through the dropdown. I would think view_->GetText() (as we did before) would be correct instead. https://codereview.chromium.org/1943683002/diff/40001/components/omnibox/brow... components/omnibox/browser/omnibox_edit_model.cc:1231: } This seems really questionable. Before your patch, what actually cleared is_keyword_hint_, updated user_text_, etc. when we created a keyword search by inserting a space in the middle? Why is that code not taking effect without this?
https://codereview.chromium.org/1943683002/diff/40001/components/omnibox/brow... File components/omnibox/browser/omnibox_edit_model.cc (right): https://codereview.chromium.org/1943683002/diff/40001/components/omnibox/brow... components/omnibox/browser/omnibox_edit_model.cc:206: const base::string16 user_text(user_text_); On 2016/05/09 23:15:53, Peter Kasting wrote: > I think this change means we'll now take the first arm of the conditional when > someone switches tabs when in keyword mode with no visible text, when we didn't > before. That doesn't seem right. > > I think you probably want something like: > > const base::string16 display_text = view_->GetText(); > if (MaybePrependKeyword(display_text).empty()) { > ... > } else { > InternalSetUserText(display_text); > } > > Though if we're in some sort of temporary text keyword case, I think maybe the > second arm is supposed to "accept" that keyword as a side effect. You probably > want to test tabbing to keyword mode on a non-default dropdown match and then > changing tabs away and back and seeing if the temporary keyword was "accepted" > as the current state before and after your change. If you do find problems > there, the change in AcceptTemporaryTextAsUserText() is probably worrisome as > well. Good catch. Done. https://codereview.chromium.org/1943683002/diff/40001/components/omnibox/brow... components/omnibox/browser/omnibox_edit_model.cc:247: view_->SetUserText(state->user_text, state->user_text, false); On 2016/05/09 23:15:53, Peter Kasting wrote: > I think this was the only place that passed different values for the first two > args. > > This means we should simplify SetUserText() to just take one arg. Done. https://codereview.chromium.org/1943683002/diff/40001/components/omnibox/brow... components/omnibox/browser/omnibox_edit_model.cc:484: // If we're in keyword mode, we're not displaying the full |user_text_|, so On 2016/05/09 23:15:53, Peter Kasting wrote: > This comment is no longer correct, as we no longer have undisplayed text in > |user_text_|. Done. https://codereview.chromium.org/1943683002/diff/40001/components/omnibox/brow... components/omnibox/browser/omnibox_edit_model.cc:492: cursor_position += user_text_.length() - user_text_.length(); On 2016/05/09 23:15:53, Peter Kasting wrote: > This statement now does nothing. In general, you won't notice obvious bad > things if the cursor position here is set wrongly, so you'll need to debug/test > carefully to ensure you're making the right adjustments. > > I would think you'd need something like: > > cursor_position += > MaybePrependKeyword(user_text_).length() - user_text_.length(); > > ...And update the comment accordingly. Done. https://codereview.chromium.org/1943683002/diff/40001/components/omnibox/brow... components/omnibox/browser/omnibox_edit_model.cc:575: has_temporary_text_ ? MaybePrependKeyword(user_text_) : input_.text(), On 2016/05/09 23:15:53, Peter Kasting wrote: > We know is_keyword_selected() is false, so MaybePrependKeyword() here does > nothing. > > However, I don't think using the |user_text_| is right. I don't believe that > gets updated as the user tabs through the dropdown. I would think > view_->GetText() (as we did before) would be correct instead. Done. https://codereview.chromium.org/1943683002/diff/40001/components/omnibox/brow... components/omnibox/browser/omnibox_edit_model.cc:1231: } On 2016/05/09 23:15:53, Peter Kasting wrote: > This seems really questionable. Before your patch, what actually cleared > is_keyword_hint_, updated user_text_, etc. when we created a keyword search by > inserting a space in the middle? Why is that code not taking effect without > this? Here's the stack trace I get from setting a watchpoint on is_keyword_hint_. Moved MaybeStripKeyword back where it was before. #0 0x0000555558b0d6a8 in OmniboxEditModel::OnPopupDataChanged ( this=0x20b8584c78e0, text="", destination_for_temporary_text_change=0x0, keyword="g\000o\000o\000g\000l\000e\000.\000c\000o\000m\000", is_keyword_hint=false) at ../../components/omnibox/browser/omnibox_edit_model.cc:1074 #1 0x0000555558b0e665 in OmniboxEditModel::OnCurrentMatchChanged ( this=0x20b8584c78e0) at ../../components/omnibox/browser/omnibox_edit_model.cc:1275 #2 0x0000555558b065ad in OmniboxController::OnResultChanged ( this=0x20b8584c7ba0, default_match_changed=true) at ../../components/omnibox/browser/omnibox_controller.cc:50 #3 0x0000555558ab3406 in AutocompleteController::NotifyChanged ( this=0x20b857ee2020, notify_default_match=true) at ../../components/omnibox/browser/autocomplete_controller.cc:611 #4 0x0000555558ab1ad1 in AutocompleteController::UpdateResult ( this=0x20b857ee2020, regenerate_result=false, force_notify_default_match_changed=true) at ../../components/omnibox/browser/autocomplete_controller.cc:473 #5 0x0000555558ab0f7b in AutocompleteController::Start (this=0x20b857ee2020, input=...) at ../../components/omnibox/browser/autocomplete_controller.cc:299 #6 0x0000555558b06463 in OmniboxController::StartAutocomplete ( this=0x20b8584c7ba0, input=...) ---Type <return> to continue, or q <return> to quit--- at ../../components/omnibox/browser/omnibox_controller.cc:39 #7 0x0000555558b099f4 in OmniboxEditModel::StartAutocomplete ( this=0x20b8584c78e0, has_selected_text=false, prevent_inline_autocomplete=true, entering_keyword_mode=false) at ../../components/omnibox/browser/omnibox_edit_model.cc:524 #8 0x000055555856c3a2 in OmniboxViewViews::UpdatePopup (this=0x20b858791c20) at ../../chrome/browser/ui/views/omnibox/omnibox_view_views.cc:475 #9 0x0000555558b0dee4 in OmniboxEditModel::OnAfterPossibleChange ( this=0x20b8584c78e0, old_text="g\000o\000o\000g\000l\000e\000.\000c\000o\000m\000t\000e\000s\000t\000", new_text="g\000o\000o\000g\000l\000e\000.\000c\000o\000m\000 \000t\000e\000s\000t\000", selection_start=11, selection_end=11, selection_differs=false, text_differs=true, just_deleted_text=false, allow_keyword_ui_change=true) at ../../components/omnibox/browser/omnibox_edit_model.cc:1230 #10 0x000055555856c90e in OmniboxViewViews::OnAfterPossibleChange ( this=0x20b858791c20, allow_keyword_ui_change=true) at ../../chrome/browser/ui/views/omnibox/omnibox_view_views.cc:551 #11 0x000055555856e6ff in OmniboxViewViews::OnAfterUserAction ( this=0x20b858791c20, sender=0x20b858791c38) at ../../chrome/browser/ui/views/omnibox/omnibox_view_views.cc:975 #12 0x00007fffed160785 in views::Textfield::OnAfterUserAction ( this=0x20b858791c38) at ../../ui/views/controls/textfield/textfield.cc:1795 ---Type <return> to continue, or q <return> to quit--- #13 0x00007fffed1671bb in views::Textfield::DoInsertChar (this=0x20b858791c38, ch=32) at ../../ui/views/controls/textfield/textfield.cc:1650 #14 0x000055555856e3c6 in OmniboxViewViews::DoInsertChar (this=0x20b858791c20, ch=32) at ../../chrome/browser/ui/views/omnibox/omnibox_view_views.cc:917 #15 0x00007fffed165daa in views::Textfield::InsertChar (this=0x20b858791c38, event=...) at ../../ui/views/controls/textfield/textfield.cc:1476 #16 0x00007fffed38c723 in ui::InputMethodAuraLinux::ProcessKeyEventDone ( this=0x20b858e173e0, event=0x7fffffffc128, filtered=true, is_handled=false) at ../../ui/base/ime/input_method_auralinux.cc:175
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/1943683002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1943683002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/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/1943683002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1943683002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
I'm debugging the new test failures, specifically OmniboxViewTest::AcceptKeywordBySpace, and it looks like there's a race condition (maybe??). If I throw in some sleep(1)'s in this test, I can get it to pass, but looking at the test, it's just straight-line code with no delays. How should I get these tests passing again?
On 2016/05/10 19:59:44, Tom Anderson wrote: > I'm debugging the new test failures, specifically > OmniboxViewTest::AcceptKeywordBySpace, and it looks like there's a race > condition (maybe??). If I throw in some sleep(1)'s in this test, I can get it > to pass, but looking at the test, it's just straight-line code with no delays. > How should I get these tests passing again? Depending on where the failures are, generally the issue is that the test will send keystrokes to the omnibox but not actually wait for the update to have taken place. There's some kind of utility that basically lets you wait for the keystroke to have been handled before proceeding. I don't remember what this has called because it's been so long since I wrote an omnibox test like this, but hopefully if you peer at enough different omnibox tests something will jump out.
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/1943683002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1943683002/100001
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_...)
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/1943683002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1943683002/120001
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/1943683002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1943683002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Fixed tests
On 2016/05/14 21:26:37, Tom Anderson wrote: > Fixed tests Ping
https://codereview.chromium.org/1943683002/diff/140001/components/omnibox/bro... File components/omnibox/browser/omnibox_edit_model.cc (right): https://codereview.chromium.org/1943683002/diff/140001/components/omnibox/bro... components/omnibox/browser/omnibox_edit_model.cc:311: is_keyword_hint_ = false; What caller(s) of this function require that we clear this state, versus which don't? I just want to make sure this is the right place in the call chain to put this code. https://codereview.chromium.org/1943683002/diff/140001/components/omnibox/bro... components/omnibox/browser/omnibox_edit_model.cc:483: size_t cursor_position; Nit: Perhaps if up here you added a temp: const base::string16 input_text = MaybePrependKeyword(user_text_); ...then you could refer to this in the three separate places below that do this, saving both time and verbosity. https://codereview.chromium.org/1943683002/diff/140001/components/omnibox/bro... components/omnibox/browser/omnibox_edit_model.cc:488: // The text that AutocompleteInput expects is of the form Nit: Maybe start this whole comment with "For keyword searches," or similar, since this is only about what happens when we're doing a keyword search? https://codereview.chromium.org/1943683002/diff/140001/components/omnibox/bro... components/omnibox/browser/omnibox_edit_model.cc:489: // "<keyword><SpaceChar><query>", where our query is |user_text_|. So if Nit: I'd just use an actual space in place of "<SpaceChar>" in this comment, it seems clearer. https://codereview.chromium.org/1943683002/diff/140001/components/omnibox/bro... components/omnibox/browser/omnibox_edit_model.cc:494: // |is_keyword_hint_|, so MaybeStripKeyword() will believe we are already in Nit: Did you mean to say MaybePrependKeyword() here? https://codereview.chromium.org/1943683002/diff/140001/components/omnibox/bro... components/omnibox/browser/omnibox_edit_model.cc:841: InternalSetUserText(view_->GetText()); Don't we need to call MaybePrependKeyword() here? Otherwise, if I tab through the dropdown into keyword mode on a non-default row, then hit ctrl-tab + ctrl-shift-tab to change tabs away and back, I won't be in keyword mode anymore. https://codereview.chromium.org/1943683002/diff/140001/components/omnibox/bro... components/omnibox/browser/omnibox_edit_model.cc:1079: if(!keyword_was_selected && is_keyword_selected()) { Nit: Space after if https://codereview.chromium.org/1943683002/diff/140001/components/omnibox/bro... components/omnibox/browser/omnibox_edit_model.cc:1080: // We just entered keyword mode, so remove the keyword from the query. Nit: query -> input
Also, is it possible to add tests for any of the problems I've pointed out in previous patch sets? I'm worried that our existing test coverage here is insufficient, and most of these sorts of things should be caught by tests without me even having to think of them.
The CQ bit was checked by thomasanderson@google.com to run a CQ dry run
https://codereview.chromium.org/1943683002/diff/140001/components/omnibox/bro... File components/omnibox/browser/omnibox_edit_model.cc (right): https://codereview.chromium.org/1943683002/diff/140001/components/omnibox/bro... components/omnibox/browser/omnibox_edit_model.cc:311: is_keyword_hint_ = false; On 2016/05/18 03:32:22, Peter Kasting wrote: > What caller(s) of this function require that we clear this state, versus which > don't? I just want to make sure this is the right place in the call chain to > put this code. There were some browser tests that called SetUserText and then had a check that the keyword was empty afterwards. I think the only one that doesn't is on line 247, but there's a comment to clarify. https://codereview.chromium.org/1943683002/diff/140001/components/omnibox/bro... components/omnibox/browser/omnibox_edit_model.cc:483: size_t cursor_position; On 2016/05/18 03:32:22, Peter Kasting wrote: > Nit: Perhaps if up here you added a temp: > > const base::string16 input_text = MaybePrependKeyword(user_text_); > > ...then you could refer to this in the three separate places below that do this, > saving both time and verbosity. Done. https://codereview.chromium.org/1943683002/diff/140001/components/omnibox/bro... components/omnibox/browser/omnibox_edit_model.cc:488: // The text that AutocompleteInput expects is of the form On 2016/05/18 03:32:22, Peter Kasting wrote: > Nit: Maybe start this whole comment with "For keyword searches," or similar, > since this is only about what happens when we're doing a keyword search? Done. https://codereview.chromium.org/1943683002/diff/140001/components/omnibox/bro... components/omnibox/browser/omnibox_edit_model.cc:489: // "<keyword><SpaceChar><query>", where our query is |user_text_|. So if On 2016/05/18 03:32:22, Peter Kasting wrote: > Nit: I'd just use an actual space in place of "<SpaceChar>" in this comment, it > seems clearer. Done. https://codereview.chromium.org/1943683002/diff/140001/components/omnibox/bro... components/omnibox/browser/omnibox_edit_model.cc:494: // |is_keyword_hint_|, so MaybeStripKeyword() will believe we are already in On 2016/05/18 03:32:22, Peter Kasting wrote: > Nit: Did you mean to say MaybePrependKeyword() here? Yes & done https://codereview.chromium.org/1943683002/diff/140001/components/omnibox/bro... components/omnibox/browser/omnibox_edit_model.cc:841: InternalSetUserText(view_->GetText()); On 2016/05/18 03:32:22, Peter Kasting wrote: > Don't we need to call MaybePrependKeyword() here? > > Otherwise, if I tab through the dropdown into keyword mode on a non-default row, > then hit ctrl-tab + ctrl-shift-tab to change tabs away and back, I won't be in > keyword mode anymore. InternalSetUserText will not change the keyword (that's in SetUserText). In a quick test, ctrl-tab + ctrl-shift-tab from keyword mode remains in keyword mode, even on a non-default row. https://codereview.chromium.org/1943683002/diff/140001/components/omnibox/bro... components/omnibox/browser/omnibox_edit_model.cc:1079: if(!keyword_was_selected && is_keyword_selected()) { On 2016/05/18 03:32:22, Peter Kasting wrote: > Nit: Space after if Done. https://codereview.chromium.org/1943683002/diff/140001/components/omnibox/bro... components/omnibox/browser/omnibox_edit_model.cc:1080: // We just entered keyword mode, so remove the keyword from the query. On 2016/05/18 03:32:22, Peter Kasting wrote: > Nit: query -> input Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1943683002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1943683002/160001
LGTM
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
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1943683002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1943683002/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Refactor OmniboxEditModel::user_text_ to not include keyword ========== to ========== Refactor OmniboxEditModel::user_text_ to not include keyword Committed: https://crrev.com/2f005e51521f7e9f9695758b3400e0acd4d7bfc0 Cr-Commit-Position: refs/heads/master@{#394553} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/2f005e51521f7e9f9695758b3400e0acd4d7bfc0 Cr-Commit-Position: refs/heads/master@{#394553} |
