AutocompleteClassifier::Classify getting called a lot on certain events |
|
Issue description
Chrome Version: 65.0.3322.3
OS: Linux
What steps will reproduce the problem?
(1) Enter text into the Omnibox in 2 tabs.
(2) Switch between the tabs.
(3)
What is the expected result?
Classify() to be called on the each input once per switch.
What happens instead?
Classify() is called 3 times with empty input, and 5 times with
the entered input.
Also, backspacing the text to nothing seems to trigger a similar set of calls, although with fewer calls, I presume because it is only trying to update a single tab.
Here are some stack traces which highlight the problem. They were captured at a point of printing the input to Classify().
OV::TextChanged()
OVV::EmphasizedURLComponents() // also calls CurrentTextIsURL directly
UpdateTextStyle()
OEM::CurrentTextIsURL()
CurrentMatch()
GetInfoForCurrentText()
AutocompleteClassifier::Classify()
TextChanged() also calls further down the stack:
OV::TextChanged()
OEM::OnChanged()
CurrentMatch()
...
Another grouping:
OV::TextChanged()
OEM::OnChanged()
LocationBarView::OnChanged()
RefreshLocationIcon
OV::GetVectorIcon()
OEM::CurrentTextType()
CurrentMatch()
...
and
Browser::ActiveTabChanged()
UpdateToolbar()
ToolbarView::Update()
LocationBarView::Update()
OnChanged()
...
Another, I believe called on the tab we're leaving, because the text is "":
OVV::RevertAll()
OV::RevertAll() // also calls TextChanged directly
OEM::Revert()
OVV::SetWindowTextAndCaretPos()
OV::TextChanged()
Also classifies empty text:
OEM::SetUserText()
SetInputInProgress()
NotifyObserversInputInProgress()
ChromeOmniboxEditController::OnInputInProgress()
LocationBarView::UpdateWithoutTabRestore()
Update()
OnChanged()
...
If most of this is unavoidable (or 'practically' unavoidable), then perhaps we can avoid the work done on each call. Presumably, Classify() calls into HistoryItemsForTerms() which is non-trivial. Just preventing Classify() from re-classifying text that it just finished would be a win.
,
Jan 26 2018
Do all these reproduce after your fix for bug 669727 ?
,
Jan 26 2018
I'm concerned about the proposed change (remembering the match returned CurrenMatch(), in the case there is no current match for the current text) for several reasons. For example, there's a notification for OnCurrentMatchChanged(). In these cases, the current match will change. Do we want to send notifications? If not, isn't that weird? Also, sometimes we intentionally InvalidateCurrentMatch(). I'm worried with the proposed change we'll be causing current match to get set at times when it previously wouldn't, and this mean we should be invalidating it at different places. Or conditionally invalidating it. (If it was set as a side effect through the new code, it should be invalidated under some conditions. If it was set through the normal mechanism, it shouldn't be.) Let's see if the problem remains after bug 669727 is fixed? In the meantime, a non-worrisome thing to do is put a few if !text.empty() calls around. In particular, short-circuit CurrentTextIsURL() if the text is empty. No need to check on the current match under those conditions. That may help some (all?) of the cases you see where text is empty.
,
Jan 29 2018
> Do all these reproduce after your fix for bug 669727 ? Of course. The other fix only addressed the diamond in SetInputInProgress(). > I'm concerned about the proposed change Ok, I'll look into whether this can still be done safely some other way. I did address the diamond surrounding CurrentTextIsUrl(), so my issue is almost cut in half. > In the meantime, a non-worrisome thing to do is put a few > if !text.empty() There's a whole bunch of text fields in that class. Given your concern about the current match (which is what CurrentTextIsURL() uses) being sync'd with them, do you have any advice on which one we can trust? In any case, this only helps the blank inputs, whereas my 5 calls to Classify() contain text.
,
Jan 29 2018
> Given your concern about the current match (which is what CurrentTextIsURL() uses) > being sync'd with them, do you have any advice on which one we can trust? I looks like the CurrentTextIsURL() bubbles down to CurrentMatch() to GetInfoForCurrentText(), which then uses view_->GetText(). https://cs.chromium.org/chromium/src/components/omnibox/browser/omnibox_edit_model.cc?type=cs&sq=package:chromium&l=1347 That sounds safe to me. (It's naturally current, i.e., what the user is seeing.) |
|
►
Sign in to add a comment |
|
Comment 1 by k...@chromium.org
, Jan 26 2018Owner: k...@chromium.org
Status: Assigned (was: Untriaged)