Use temporal AutocompleteClassifier to get proper selection navigation url #5407
Conversation
simonhong
changed the title
bsclifton
force-pushed
the
simon_issue_9293
branch
from
75c2418 to
345d644
Compare
|
Changed to draft state because |
chromium_src/chrome/browser/renderer_context_menu/render_view_context_menu.cc
Outdated
Show resolved
Hide resolved
|
kindly ping to owners @bridiver :) |
simonhong
force-pushed
the
simon_issue_9293
branch
2 times, most recently
from
42d6846 to
e05546e
Compare
|
|
||
| #define BRAVE_APPEND_SEARCH_PROVIDER \ | ||
| if (GetProfile()->IsOffTheRecord()) { \ | ||
| selection_navigation_url_ = \ |
There was a problem hiding this comment.
I'm confused here, the selection_navigation_url_ is set from AutocompleteClassifierFactory::GetForProfile(GetProfile())->Classify( so why aren't we just setting the correct url from the AutocompleteClassifier?
There was a problem hiding this comment.
AutocompleteClassifierFactory always uses normal profile.
So we did https://github.com/brave/brave-core/pull/5407/files#diff-ffcd3bd1d7f1c7ee80c4637a3f919142L10. This PR reverts it and instead set correct url by patching here.
As you found previously, that service used normal & incognito profile mixed for incognito profile.
https://github.com/brave/brave-core/pull/5407/files#diff-2325211a427ac6f007ad338411e100aaL14
|
@bridiver Kindly ping. |
To get proper selection navigation url for profile, use tempral AutocompleteClassfier. We can't use AutocompleteClassifierFactory for this because AutocompleteClassifierFactory doesn't create service for otr profile. But we need different navigation url for OTR profile.
GuestWindowControllerTest was failed because OTR's TemplateURLService is not in loaded state while test case is running. If it's not in loaded state, it doesn't call observer. So, explicitly call OnTemplateURLServiceChanged().
simonhong
force-pushed
the
simon_issue_9293
branch
from
e05546e to
832a1c9
Compare
simonhong
force-pushed
the
simon_issue_9293
branch
from
832a1c9 to
49ea4ef
Compare
| @@ -1,44 +0,0 @@ | |||
| /* Copyright 2019 The Brave Authors. All rights reserved. | |||
There was a problem hiding this comment.
why are we deleting the test? We still need some kind of test for this
There was a problem hiding this comment.
This test was for BraveAutocompleteProviderClientForClassifier and it's not used anymore.
Instead, we use AutocompleteClassifier directly.
To get proper selection navigation url for profile, use tempral
AutocompleteClassfier. We can't use AutocompleteClassifierFactory
for this because AutocompleteClassifierFactory doesn't create service
for otr profile. But we need different navigation url for OTR profile.
This is code refactoring that fixes brave/brave-browser#1758
Resolves brave/brave-browser#9239
Submitter Checklist:
npm run lint)git rebase master(if needed).git rebase -ito squash commits (if needed).Test Plan:
After this change, brave/brave-browser#1758 should be handled properly.
Reviewer Checklist:
After-merge Checklist:
changes has landed on.