Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2639153002:
Make extensions DSE persistent in browser prefs (Reland) (Closed)
|
Created:
3 years, 11 months ago by Alexander Yashkin Modified:
3 years, 9 months ago CC:
chromium-apps-reviews_chromium.org, chromium-reviews, extensions-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake extensions DSE persistent in browser prefs (Reland)
This fix stores extension installed DSE in browser prefs using extension
overriden preferences API. This is needed so extension installed default
search would be available from browser before extensions subsystem load.
This will affect url for first loaded NTP which is taken from current
default search settings.
BUG=450534
R=pkasting@chromium.org, vasilii@chromium.org
Review-Url: https://codereview.chromium.org/2479113002
Cr-Original-Commit-Position: refs/heads/master@{#442235}
Review-Url: https://codereview.chromium.org/2639153002
Cr-Commit-Position: refs/heads/master@{#453949}
Committed: https://chromium.googlesource.com/chromium/src/+/858416e8845ab1289879133ca455fe86473e6ec2
Patch Set 1 #Patch Set 2 : Fixed default extension keywords conflicts problem #Patch Set 3 : Fixed flakiness caused by extension reload while TemplateURLService is not loaded #
Total comments: 10
Patch Set 4 : Fixed after review #Patch Set 5 : Rebased and updated after review, use new keyword conflicts resolution #
Total comments: 13
Patch Set 6 : Fixed after review, round 4 #
Total comments: 12
Patch Set 7 : Fixed after review, round 5 #Patch Set 8 : Fixed after review, round 6 #
Total comments: 3
Patch Set 9 : Minor fix after review, round 7 #Messages
Total messages: 64 (13 generated)
@vasillii, pkasting, PTAL again I found problem with keywords conflict, added minor change to solution and tests TemplateURLServiceTest.DefaultExtensionEngineAndPrepopulated TemplateURLServiceTest.TwoExtensionsWithSameEngine Now I store extension id in prefs with default search preferences dictionary. This is needed because TemplateUrlService can store extension TemplateURL with updated keyword due to its conflict with other engines. Yet i had to find and set it correctly on TemplateUrlService load when initializing from initial_default_search_provider_.
The CQ bit was checked by a-v-y@yandex-team.ru 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: No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
Can you please run tryjobs several times on win_chromium_rel_ng, win_chromium_x64_rel_ng configuration so that I was sure I fixed flakiness of tests.
Can you clarify step by step what happened when the test failed? I don't see how a conflict is possible as the testing extension is totally different from the prepopulated search providers.
On 2017/01/18 12:17:04, vasilii (slow) wrote: > Can you clarify step by step what happened when the test failed? > > I don't see how a conflict is possible as the testing extension is totally > different from the prepopulated search providers. Keyword conflict is not the source of test flakiness I think. Its separate issue that I have encountered, while trying to reproduce flaky test problem. About flaky tests - flaky tests are ExtensionBrowserTest.OverrideHomePageSettings and ExtensionBrowserTest.OverrideStartupPagesSettings All flaky tests failed on line [6024:4000:0109/084142.069:FATAL:template_url_service.cc(492)] Check failed: !FindTemplateURLForExtension(info->extension_id, template_url->type()). See https://bugs.chromium.org/p/chromium/issues/detail?id=679470 for details I think that trigger of this DCHECK indicates that test extension is already loaded and extension search engine is already added at start of ExtensionBrowserTest.OverrideHomePageSettings. I was not able to reproduce this problem locally. My current hypothesis is that cause of problem are newly added tests - PRE_OverridenDSEPersists and OverridenDSEPersists tests. PRE_OverridenDSEPersists test installs test extension into test profile, but OverridenDSEPersists test does not uninstall it. So its left in test profile and sometimes same directory is used by next test from same batch - i have no proofs, thats hypothesis. So the next tests OverrideHomePageSettings and OverrideStartupPagesSettings that try to load test extension trigger this DCHECK. To fix it I added extension uninstall at the end of OverridenDSEPersists test. If i am right this solution should fix flakiness, to check it I had asked you to run it on on trybots several times.
> To fix it I added extension uninstall at the end of OverridenDSEPersists test. > If i am right this solution should fix flakiness, to check it I had asked you to > run it on on trybots several times. Or can you explain how to do it. Thanks.
On 2017/01/18 12:50:35, Alexander Yashkin wrote: > On 2017/01/18 12:17:04, vasilii (slow) wrote: > > Can you clarify step by step what happened when the test failed? > > > > I don't see how a conflict is possible as the testing extension is totally > > different from the prepopulated search providers. > > Keyword conflict is not the source of test flakiness I think. > Its separate issue that I have encountered, while trying to reproduce flaky test > problem. > > About flaky tests - flaky tests are > ExtensionBrowserTest.OverrideHomePageSettings and > ExtensionBrowserTest.OverrideStartupPagesSettings > > All flaky tests failed on line > [6024:4000:0109/084142.069:FATAL:template_url_service.cc(492)] Check failed: > !FindTemplateURLForExtension(info->extension_id, template_url->type()). > See https://bugs.chromium.org/p/chromium/issues/detail?id=679470 for details > > I think that trigger of this DCHECK indicates that test extension is already > loaded and extension search engine is already added at start of > ExtensionBrowserTest.OverrideHomePageSettings. > I was not able to reproduce this problem locally. > > My current hypothesis is that cause of problem are newly added tests - > PRE_OverridenDSEPersists and OverridenDSEPersists tests. > PRE_OverridenDSEPersists test installs test extension into test profile, but > OverridenDSEPersists test does not uninstall it. > So its left in test profile and sometimes same directory is used by next test > from same batch - i have no proofs, thats hypothesis. > So the next tests OverrideHomePageSettings and OverrideStartupPagesSettings that > try to load test extension trigger this DCHECK. > > To fix it I added extension uninstall at the end of OverridenDSEPersists test. > If i am right this solution should fix flakiness, to check it I had asked you to > run it on on trybots several times. I was able to reproduce the test failure on my Win machine with the original CL. I was running only one test (anyway, every test gets a fresh directory). The debugger showed that SettingsOverridesAPI::OnExtensionLoaded is called twice.
On 2017/01/19 08:27:35, vasilii (slow) wrote: > On 2017/01/18 12:50:35, Alexander Yashkin wrote: > > On 2017/01/18 12:17:04, vasilii (slow) wrote: > > > Can you clarify step by step what happened when the test failed? > > > > > > I don't see how a conflict is possible as the testing extension is totally > > > different from the prepopulated search providers. > > > > Keyword conflict is not the source of test flakiness I think. > > Its separate issue that I have encountered, while trying to reproduce flaky > test > > problem. > > > > About flaky tests - flaky tests are > > ExtensionBrowserTest.OverrideHomePageSettings and > > ExtensionBrowserTest.OverrideStartupPagesSettings > > > > All flaky tests failed on line > > [6024:4000:0109/084142.069:FATAL:template_url_service.cc(492)] Check failed: > > !FindTemplateURLForExtension(info->extension_id, template_url->type()). > > See https://bugs.chromium.org/p/chromium/issues/detail?id=679470 for details > > > > I think that trigger of this DCHECK indicates that test extension is already > > loaded and extension search engine is already added at start of > > ExtensionBrowserTest.OverrideHomePageSettings. > > I was not able to reproduce this problem locally. > > > > My current hypothesis is that cause of problem are newly added tests - > > PRE_OverridenDSEPersists and OverridenDSEPersists tests. > > PRE_OverridenDSEPersists test installs test extension into test profile, but > > OverridenDSEPersists test does not uninstall it. > > So its left in test profile and sometimes same directory is used by next test > > from same batch - i have no proofs, thats hypothesis. > > So the next tests OverrideHomePageSettings and OverrideStartupPagesSettings > that > > try to load test extension trigger this DCHECK. > > > > To fix it I added extension uninstall at the end of OverridenDSEPersists test. > > If i am right this solution should fix flakiness, to check it I had asked you > to > > run it on on trybots several times. > > I was able to reproduce the test failure on my Win machine with the original CL. > I was running only one test (anyway, every test gets a fresh directory). The > debugger showed that SettingsOverridesAPI::OnExtensionLoaded is called twice. Whats your gn/args build configuration?
is_component_build = true enable_nacl = false generated with gn gen out\Default --ide=vs
On 2017/01/19 11:58:43, vasilii (slow) wrote: > is_component_build = true > enable_nacl = false > > generated with gn gen out\Default --ide=vs I am still not able to reproduce with your config on win 7. Can you show from what callstack both OnExtensionLoaded were called? If I could reproduce it locally I could try to make a fix for it.
On 2017/01/19 15:11:47, Alexander Yashkin wrote: > On 2017/01/19 11:58:43, vasilii (slow) wrote: > > is_component_build = true > > enable_nacl = false > > > > generated with gn gen out\Default --ide=vs > > I am still not able to reproduce with your config on win 7. > > Can you show from what callstack both OnExtensionLoaded were called? > If I could reproduce it locally I could try to make a fix for it. 1 call > browser_tests.exe!extensions::SettingsOverridesAPI::OnExtensionLoaded() Line 159 C++ browser_tests.exe!extensions::ExtensionRegistry::TriggerOnLoaded() Line 55 C++ browser_tests.exe!ExtensionService::NotifyExtensionLoaded() Line 1081 C++ browser_tests.exe!ExtensionService::AddExtension() Line 1548 C++ browser_tests.exe!ExtensionService::FinishInstallation() Line 1966 C++ browser_tests.exe!ExtensionService::AddNewOrUpdatedExtension() Line 1900 C++ browser_tests.exe!ExtensionService::OnExtensionInstalled() Line 1831 C++ browser_tests.exe!extensions::UnpackedInstaller::InstallExtension() Line 381 C++ browser_tests.exe!extensions::UnpackedInstaller::OnInstallChecksComplete() Line 266 C++ browser_tests.exe!base::internal::FunctorTraits<void (__cdecl extensions::UnpackedInstaller::*)(int) __ptr64,void>::Invoke<scoped_refptr<extensions::UnpackedInstaller> const & __ptr64,int>() Line 215 C++ browser_tests.exe!base::internal::InvokeHelper<0,void>::MakeItSo<void (__cdecl extensions::UnpackedInstaller::*const & __ptr64)(int) __ptr64,scoped_refptr<extensions::UnpackedInstaller> const & __ptr64,int>() Line 287 C++ browser_tests.exe!base::internal::Invoker<base::internal::BindState<void (__cdecl extensions::UnpackedInstaller::*)(int) __ptr64,scoped_refptr<extensions::UnpackedInstaller> >,void __cdecl(int)>::RunImpl<void (__cdecl extensions::UnpackedInstaller::*const & __ptr64)(int) __ptr64,std::tuple<scoped_refptr<extensions::UnpackedInstaller> > const & __ptr64,0>() Line 365 C++ browser_tests.exe!base::internal::Invoker<base::internal::BindState<void (__cdecl extensions::UnpackedInstaller::*)(int) __ptr64,scoped_refptr<extensions::UnpackedInstaller> >,void __cdecl(int)>::Run() Line 343 C++ browser_tests.exe!base::internal::RunMixin<base::Callback<void __cdecl(int),1,1> >::Run() Line 86 C++ browser_tests.exe!extensions::ExtensionInstallChecker::MaybeInvokeCallback() Line 171 C++ browser_tests.exe!extensions::ExtensionInstallChecker::OnRequirementsCheckDone() Line 112 C++ browser_tests.exe!base::internal::FunctorTraits<void (__cdecl extensions::ExtensionInstallChecker::*)(int,std::vector<std::basic_string<char,std::char_traits<char>,std::allocator<char> >,std::allocator<std::basic_string<char,std::char_traits<char>,std::allocator<char> > > > const & __ptr64) __ptr64,void>::Invoke<base::WeakPtr<extensions::ExtensionInstallChecker> const & __ptr64,int const & __ptr64,std::vector<std::basic_string<char,std::char_traits<char>,std::allocator<char> >,std::allocator<std::basic_string<char,std::char_traits<char>,std::allocator<char> > > > const & __ptr64>() Line 215 C++ browser_tests.exe!base::internal::InvokeHelper<1,void>::MakeItSo<void (__cdecl extensions::ExtensionInstallChecker::*const & __ptr64)(int,std::vector<std::basic_string<char,std::char_traits<char>,std::allocator<char> >,std::allocator<std::basic_string<char,std::char_traits<char>,std::allocator<char> > > > const & __ptr64) __ptr64,base::WeakPtr<extensions::ExtensionInstallChecker> const & __ptr64,int const & __ptr64,std::vector<std::basic_string<char,std::char_traits<char>,std::allocator<char> >,std::allocator<std::basic_string<char,std::char_traits<char>,std::allocator<char> > > > const & __ptr64>() Line 308 C++ browser_tests.exe!base::internal::Invoker<base::internal::BindState<void (__cdecl extensions::ExtensionInstallChecker::*)(int,std::vector<std::basic_string<char,std::char_traits<char>,std::allocator<char> >,std::allocator<std::basic_string<char,std::char_traits<char>,std::allocator<char> > > > const & __ptr64) __ptr64,base::WeakPtr<extensions::ExtensionInstallChecker>,int>,void __cdecl(std::vector<std::basic_string<char,std::char_traits<char>,std::allocator<char> >,std::allocator<std::basic_string<char,std::char_traits<char>,std::allocator<char> > > > const & __ptr64)>::RunImpl<void (__cdecl extensions::ExtensionInstallChecker::*const & __ptr64)(int,std::vector<std::basic_string<char,std::char_traits<char>,std::allocator<char> >,std::allocator<std::basic_string<char,std::char_traits<char>,std::allocator<char> > > > const & __ptr64) __ptr64,std::tuple<base::WeakPtr<extensions::ExtensionInstallChecker>,int> const & __ptr64,0,1>() Line 365 C++ browser_tests.exe!base::internal::Invoker<base::internal::BindState<void (__cdecl extensions::ExtensionInstallChecker::*)(int,std::vector<std::basic_string<char,std::char_traits<char>,std::allocator<char> >,std::allocator<std::basic_string<char,std::char_traits<char>,std::allocator<char> > > > const & __ptr64) __ptr64,base::WeakPtr<extensions::ExtensionInstallChecker>,int>,void __cdecl(std::vector<std::basic_string<char,std::char_traits<char>,std::allocator<char> >,std::allocator<std::basic_string<char,std::char_traits<char>,std::allocator<char> > > > const & __ptr64)>::Run() Line 343 C++ browser_tests.exe!base::internal::RunMixin<base::Callback<void __cdecl(std::vector<std::basic_string<char,std::char_traits<char>,std::allocator<char> >,std::allocator<std::basic_string<char,std::char_traits<char>,std::allocator<char> > > > const & __ptr64),1,1> >::Run() Line 86 C++ browser_tests.exe!base::internal::FunctorTraits<base::Callback<void __cdecl(std::vector<std::basic_string<char,std::char_traits<char>,std::allocator<char> >,std::allocator<std::basic_string<char,std::char_traits<char>,std::allocator<char> > > > const & __ptr64),1,1>,void>::Invoke<base::Callback<void __cdecl(std::vector<std::basic_string<char,std::char_traits<char>,std::allocator<char> >,std::allocator<std::basic_string<char,std::char_traits<char>,std::allocator<char> > > > const & __ptr64),1,1> const & __ptr64,std::vector<std::basic_string<char,std::char_traits<char>,std::allocator<char> >,std::allocator<std::basic_string<char,std::char_traits<char>,std::allocator<char> > > > const & __ptr64>() Line 266 C++ browser_tests.exe!base::internal::InvokeHelper<0,void>::MakeItSo<base::Callback<void __cdecl(std::vector<std::basic_string<char,std::char_traits<char>,std::allocator<char> >,std::allocator<std::basic_string<char,std::char_traits<char>,std::allocator<char> > > > const & __ptr64),1,1> const & __ptr64,std::vector<std::basic_string<char,std::char_traits<char>,std::allocator<char> >,std::allocator<std::basic_string<char,std::char_traits<char>,std::allocator<char> > > > const & __ptr64>() Line 287 C++ browser_tests.exe!base::internal::Invoker<base::internal::BindState<base::Callback<void __cdecl(std::vector<std::basic_string<char,std::char_traits<char>,std::allocator<char> >,std::allocator<std::basic_string<char,std::char_traits<char>,std::allocator<char> > > > const & __ptr64),1,1>,std::vector<std::basic_string<char,std::char_traits<char>,std::allocator<char> >,std::allocator<std::basic_string<char,std::char_traits<char>,std::allocator<char> > > > >,void __cdecl(void)>::RunImpl<base::Callback<void __cdecl(std::vector<std::basic_string<char,std::char_traits<char>,std::allocator<char> >,std::allocator<std::basic_string<char,std::char_traits<char>,std::allocator<char> > > > const & __ptr64),1,1> const & __ptr64,std::tuple<std::vector<std::basic_string<char,std::char_traits<char>,std::allocator<char> >,std::allocator<std::basic_string<char,std::char_traits<char>,std::allocator<char> > > > > const & __ptr64,0>() Line 365 C++ browser_tests.exe!base::internal::Invoker<base::internal::BindState<base::Callback<void __cdecl(std::vector<std::basic_string<char,std::char_traits<char>,std::allocator<char> >,std::allocator<std::basic_string<char,std::char_traits<char>,std::allocator<char> > > > const & __ptr64),1,1>,std::vector<std::basic_string<char,std::char_traits<char>,std::allocator<char> >,std::allocator<std::basic_string<char,std::char_traits<char>,std::allocator<char> > > > >,void __cdecl(void)>::Run() Line 343 C++ base.dll!base::internal::RunMixin<base::Callback<void __cdecl(void),0,0> >::Run() Line 68 C++ base.dll!base::debug::TaskAnnotator::RunTask(const char * queue_function, base::PendingTask * pending_task) Line 54 C++ base.dll!base::MessageLoop::RunTask(base::PendingTask * pending_task) Line 422 C++ base.dll!base::MessageLoop::DeferOrRunPendingTask(base::PendingTask pending_task) Line 433 C++ base.dll!base::MessageLoop::DoWork() Line 523 C++ base.dll!base::MessagePumpForUI::DoRunLoop() Line 173 C++ base.dll!base::MessagePumpWin::Run(base::MessagePump::Delegate * delegate) Line 58 C++ base.dll!base::MessageLoop::RunHandler() Line 387 C++ base.dll!base::RunLoop::Run() Line 38 C++ browser_tests.exe!extensions::TestExtensionRegistryObserver::Waiter::Wait() Line 20 C++ browser_tests.exe!extensions::TestExtensionRegistryObserver::Wait() Line 110 C++ browser_tests.exe!extensions::TestExtensionRegistryObserver::WaitForExtensionLoaded() Line 69 C++ browser_tests.exe!extensions::ChromeTestExtensionLoader::LoadUnpacked() Line 209 C++ browser_tests.exe!extensions::ChromeTestExtensionLoader::LoadExtension() Line 48 C++ browser_tests.exe!ExtensionBrowserTest::LoadExtensionWithInstallParam() Line 239 C++ browser_tests.exe!`anonymous namespace'::ExtensionBrowserTest_OverrideHomePageSettings_Test::RunTestOnMainThread() Line 77 C++
2nd call > browser_tests.exe!extensions::SettingsOverridesAPI::OnExtensionLoaded() Line 159 C++ browser_tests.exe!extensions::ExtensionRegistry::TriggerOnLoaded() Line 55 C++ browser_tests.exe!ExtensionService::NotifyExtensionLoaded() Line 1081 C++ browser_tests.exe!ExtensionService::EnableExtension() Line 888 C++ browser_tests.exe!ExtensionService::AddExtension() Line 1532 C++ browser_tests.exe!ExtensionService::FinishInstallation() Line 1966 C++ browser_tests.exe!ExtensionService::AddNewOrUpdatedExtension() Line 1900 C++ browser_tests.exe!ExtensionService::OnExtensionInstalled() Line 1831 C++ browser_tests.exe!extensions::UnpackedInstaller::InstallExtension() Line 381 C++ browser_tests.exe!extensions::UnpackedInstaller::OnInstallChecksComplete() Line 266 C++ browser_tests.exe!base::internal::FunctorTraits<void (__cdecl extensions::UnpackedInstaller::*)(int) __ptr64,void>::Invoke<scoped_refptr<extensions::UnpackedInstaller> const & __ptr64,int>() Line 215 C++ browser_tests.exe!base::internal::InvokeHelper<0,void>::MakeItSo<void (__cdecl extensions::UnpackedInstaller::*const & __ptr64)(int) __ptr64,scoped_refptr<extensions::UnpackedInstaller> const & __ptr64,int>() Line 287 C++ browser_tests.exe!base::internal::Invoker<base::internal::BindState<void (__cdecl extensions::UnpackedInstaller::*)(int) __ptr64,scoped_refptr<extensions::UnpackedInstaller> >,void __cdecl(int)>::RunImpl<void (__cdecl extensions::UnpackedInstaller::*const & __ptr64)(int) __ptr64,std::tuple<scoped_refptr<extensions::UnpackedInstaller> > const & __ptr64,0>() Line 365 C++ browser_tests.exe!base::internal::Invoker<base::internal::BindState<void (__cdecl extensions::UnpackedInstaller::*)(int) __ptr64,scoped_refptr<extensions::UnpackedInstaller> >,void __cdecl(int)>::Run() Line 343 C++ browser_tests.exe!base::internal::RunMixin<base::Callback<void __cdecl(int),1,1> >::Run() Line 86 C++ browser_tests.exe!extensions::ExtensionInstallChecker::MaybeInvokeCallback() Line 171 C++ browser_tests.exe!extensions::ExtensionInstallChecker::OnRequirementsCheckDone() Line 112 C++ browser_tests.exe!base::internal::FunctorTraits<void (__cdecl extensions::ExtensionInstallChecker::*)(int,std::vector<std::basic_string<char,std::char_traits<char>,std::allocator<char> >,std::allocator<std::basic_string<char,std::char_traits<char>,std::allocator<char> > > > const & __ptr64) __ptr64,void>::Invoke<base::WeakPtr<extensions::ExtensionInstallChecker> const & __ptr64,int const & __ptr64,std::vector<std::basic_string<char,std::char_traits<char>,std::allocator<char> >,std::allocator<std::basic_string<char,std::char_traits<char>,std::allocator<char> > > > const & __ptr64>() Line 215 C++ browser_tests.exe!base::internal::InvokeHelper<1,void>::MakeItSo<void (__cdecl extensions::ExtensionInstallChecker::*const & __ptr64)(int,std::vector<std::basic_string<char,std::char_traits<char>,std::allocator<char> >,std::allocator<std::basic_string<char,std::char_traits<char>,std::allocator<char> > > > const & __ptr64) __ptr64,base::WeakPtr<extensions::ExtensionInstallChecker> const & __ptr64,int const & __ptr64,std::vector<std::basic_string<char,std::char_traits<char>,std::allocator<char> >,std::allocator<std::basic_string<char,std::char_traits<char>,std::allocator<char> > > > const & __ptr64>() Line 308 C++ browser_tests.exe!base::internal::Invoker<base::internal::BindState<void (__cdecl extensions::ExtensionInstallChecker::*)(int,std::vector<std::basic_string<char,std::char_traits<char>,std::allocator<char> >,std::allocator<std::basic_string<char,std::char_traits<char>,std::allocator<char> > > > const & __ptr64) __ptr64,base::WeakPtr<extensions::ExtensionInstallChecker>,int>,void __cdecl(std::vector<std::basic_string<char,std::char_traits<char>,std::allocator<char> >,std::allocator<std::basic_string<char,std::char_traits<char>,std::allocator<char> > > > const & __ptr64)>::RunImpl<void (__cdecl extensions::ExtensionInstallChecker::*const & __ptr64)(int,std::vector<std::basic_string<char,std::char_traits<char>,std::allocator<char> >,std::allocator<std::basic_string<char,std::char_traits<char>,std::allocator<char> > > > const & __ptr64) __ptr64,std::tuple<base::WeakPtr<extensions::ExtensionInstallChecker>,int> const & __ptr64,0,1>() Line 365 C++ browser_tests.exe!base::internal::Invoker<base::internal::BindState<void (__cdecl extensions::ExtensionInstallChecker::*)(int,std::vector<std::basic_string<char,std::char_traits<char>,std::allocator<char> >,std::allocator<std::basic_string<char,std::char_traits<char>,std::allocator<char> > > > const & __ptr64) __ptr64,base::WeakPtr<extensions::ExtensionInstallChecker>,int>,void __cdecl(std::vector<std::basic_string<char,std::char_traits<char>,std::allocator<char> >,std::allocator<std::basic_string<char,std::char_traits<char>,std::allocator<char> > > > const & __ptr64)>::Run() Line 343 C++ browser_tests.exe!base::internal::RunMixin<base::Callback<void __cdecl(std::vector<std::basic_string<char,std::char_traits<char>,std::allocator<char> >,std::allocator<std::basic_string<char,std::char_traits<char>,std::allocator<char> > > > const & __ptr64),1,1> >::Run() Line 86 C++ browser_tests.exe!base::internal::FunctorTraits<base::Callback<void __cdecl(std::vector<std::basic_string<char,std::char_traits<char>,std::allocator<char> >,std::allocator<std::basic_string<char,std::char_traits<char>,std::allocator<char> > > > const & __ptr64),1,1>,void>::Invoke<base::Callback<void __cdecl(std::vector<std::basic_string<char,std::char_traits<char>,std::allocator<char> >,std::allocator<std::basic_string<char,std::char_traits<char>,std::allocator<char> > > > const & __ptr64),1,1> const & __ptr64,std::vector<std::basic_string<char,std::char_traits<char>,std::allocator<char> >,std::allocator<std::basic_string<char,std::char_traits<char>,std::allocator<char> > > > const & __ptr64>() Line 266 C++ browser_tests.exe!base::internal::InvokeHelper<0,void>::MakeItSo<base::Callback<void __cdecl(std::vector<std::basic_string<char,std::char_traits<char>,std::allocator<char> >,std::allocator<std::basic_string<char,std::char_traits<char>,std::allocator<char> > > > const & __ptr64),1,1> const & __ptr64,std::vector<std::basic_string<char,std::char_traits<char>,std::allocator<char> >,std::allocator<std::basic_string<char,std::char_traits<char>,std::allocator<char> > > > const & __ptr64>() Line 287 C++ browser_tests.exe!base::internal::Invoker<base::internal::BindState<base::Callback<void __cdecl(std::vector<std::basic_string<char,std::char_traits<char>,std::allocator<char> >,std::allocator<std::basic_string<char,std::char_traits<char>,std::allocator<char> > > > const & __ptr64),1,1>,std::vector<std::basic_string<char,std::char_traits<char>,std::allocator<char> >,std::allocator<std::basic_string<char,std::char_traits<char>,std::allocator<char> > > > >,void __cdecl(void)>::RunImpl<base::Callback<void __cdecl(std::vector<std::basic_string<char,std::char_traits<char>,std::allocator<char> >,std::allocator<std::basic_string<char,std::char_traits<char>,std::allocator<char> > > > const & __ptr64),1,1> const & __ptr64,std::tuple<std::vector<std::basic_string<char,std::char_traits<char>,std::allocator<char> >,std::allocator<std::basic_string<char,std::char_traits<char>,std::allocator<char> > > > > const & __ptr64,0>() Line 365 C++ browser_tests.exe!base::internal::Invoker<base::internal::BindState<base::Callback<void __cdecl(std::vector<std::basic_string<char,std::char_traits<char>,std::allocator<char> >,std::allocator<std::basic_string<char,std::char_traits<char>,std::allocator<char> > > > const & __ptr64),1,1>,std::vector<std::basic_string<char,std::char_traits<char>,std::allocator<char> >,std::allocator<std::basic_string<char,std::char_traits<char>,std::allocator<char> > > > >,void __cdecl(void)>::Run() Line 343 C++ base.dll!base::internal::RunMixin<base::Callback<void __cdecl(void),0,0> >::Run() Line 68 C++ base.dll!base::debug::TaskAnnotator::RunTask(const char * queue_function, base::PendingTask * pending_task) Line 54 C++ base.dll!base::MessageLoop::RunTask(base::PendingTask * pending_task) Line 422 C++ base.dll!base::MessageLoop::DeferOrRunPendingTask(base::PendingTask pending_task) Line 433 C++ base.dll!base::MessageLoop::DoWork() Line 523 C++ base.dll!base::MessagePumpForUI::DoRunLoop() Line 173 C++ base.dll!base::MessagePumpWin::Run(base::MessagePump::Delegate * delegate) Line 58 C++ base.dll!base::MessageLoop::RunHandler() Line 387 C++ base.dll!base::RunLoop::Run() Line 38 C++ browser_tests.exe!extensions::TestExtensionRegistryObserver::Waiter::Wait() Line 20 C++ browser_tests.exe!extensions::TestExtensionRegistryObserver::Wait() Line 110 C++ browser_tests.exe!extensions::TestExtensionRegistryObserver::WaitForExtensionLoaded() Line 69 C++ browser_tests.exe!extensions::ChromeTestExtensionLoader::LoadExtension() Line 78 C++ browser_tests.exe!ExtensionBrowserTest::LoadExtensionWithInstallParam() Line 239 C++ browser_tests.exe!`anonymous namespace'::ExtensionBrowserTest_OverrideHomePageSettings_Test::RunTestOnMainThread() Line 77 C++ Basically the difference is in ExtensionService::AddExtension() and ExtensionService::EnableExtension()
@vasilii I have analyzed the code of extension service and browser tests. Yes it looks like one call to LoadExtensionWithInstallParam can lead to 2 extension load attempts and two calls to OnExtensionLoaded. First load occures in line 48 of ChromeTestExtensionLoader::LoadExtension https://cs.chromium.org/chromium/src/chrome/browser/extensions/chrome_test_ex... and the second call on https://cs.chromium.org/chromium/src/chrome/browser/extensions/chrome_test_ex... of same function. At first attempt extension is loaded, and if install_param_ is not empty its reloaded in the same function. The problem is that extension reload does not lead to extension unload notification. After first call to SettingsOverridesAPI::OnExtensionLoaded we did not get corresponding SettingsOverridesAPI::OnExtensionUnloaded. And the second notification of SettingsOverridesAPI::OnExtensionLoaded leads to another attempt to add extension search engine, which already exists. Locally I always get correct sequence of events: SettingsOverridesAPI::OnExtensionLoaded SettingsOverridesAPI::OnExtensionUnloaded SettingsOverridesAPI::OnExtensionLoaded SettingsOverridesAPI::OnExtensionUnloaded First SettingsOverridesAPI::OnExtensionUnloaded on my system comes from callstack: > browser_tests.exe!extensions::SettingsOverridesAPI::OnExtensionUnloaded(content::BrowserContext * browser_context, const extensions::Extension * extension, extensions::UnloadedExtensionInfo::Reason reason) Line 205 C++ browser_tests.exe!extensions::ExtensionRegistry::TriggerOnUnloaded(const extensions::Extension * extension, extensions::UnloadedExtensionInfo::Reason reason) Line 71 C++ browser_tests.exe!ExtensionService::NotifyExtensionUnloaded(const extensions::Extension * extension, extensions::UnloadedExtensionInfo::Reason reason) Line 1136 C++ browser_tests.exe!ExtensionService::DisableExtension(const std::basic_string<char,std::char_traits<char>,std::allocator<char> > & extension_id, int disable_reasons) Line 946 C++ browser_tests.exe!ExtensionService::ReloadExtensionImpl(const std::basic_string<char,std::char_traits<char>,std::allocator<char> > & transient_extension_id, bool be_noisy) Line 681 C++ browser_tests.exe!ExtensionService::ReloadExtension(const std::basic_string<char,std::char_traits<char>,std::allocator<char> > & extension_id) Line 726 C++ browser_tests.exe!extensions::ChromeTestExtensionLoader::LoadExtension(const base::FilePath & path) Line 77 C++ browser_tests.exe!ExtensionBrowserTest::LoadExtensionWithInstallParam(const base::FilePath & path, int flags, const std::basic_string<char,std::char_traits<char>,std::allocator<char> > & install_param) Line 239 C++ browser_tests.exe!`anonymous namespace'::ExtensionBrowserTest_OverrideHomePageSettings_Test::RunTestOnMainThread() Line 77 C++ browser_tests.exe!InProcessBrowserTest::RunTestOnMainThreadLoop() Line 565 C++ browser_tests.exe!content::BrowserTestBase::ProxyRunTestOnMainThreadLoop() Line 346 C++ browser_tests.exe!base::internal::FunctorTraits<void (__cdecl content::BrowserTestBase::*)(void) __ptr64,void>::Invoke<content::BrowserTestBase * __ptr64>(void(content::BrowserTestBase::*)() method, content::BrowserTestBase * && receiver_ptr) Line 215 C++ browser_tests.exe!base::internal::InvokeHelper<0,void>::MakeItSo<void (__cdecl content::BrowserTestBase::*const & __ptr64)(void) __ptr64,content::BrowserTestBase * __ptr64>(void(content::BrowserTestBase::*)() & functor, content::BrowserTestBase * && <args_0>) Line 287 C++ browser_tests.exe!base::internal::Invoker<base::internal::BindState<void (__cdecl content::BrowserTestBase::*)(void) __ptr64,base::internal::UnretainedWrapper<content::BrowserTestBase> >,void __cdecl(void)>::RunImpl<void (__cdecl content::BrowserTestBase::*const & __ptr64)(void) __ptr64,std::tuple<base::internal::UnretainedWrapper<content::BrowserTestBase> > const & __ptr64,0>(void(content::BrowserTestBase::*)() & functor, const std::tuple<base::internal::UnretainedWrapper<content::BrowserTestBase> > & bound, base::IndexSequence<0> __formal) Line 365 C++ browser_tests.exe!base::internal::Invoker<base::internal::BindState<void (__cdecl content::BrowserTestBase::*)(void) __ptr64,base::internal::UnretainedWrapper<content::BrowserTestBase> >,void __cdecl(void)>::Run(base::internal::BindStateBase * base) Line 343 C++ browser_tests.exe!base::internal::RunMixin<base::Callback<void __cdecl(void),1,1> >::Run() Line 86 C++ browser_tests.exe!ChromeBrowserMainParts::PreMainMessageLoopRunImpl() Line 1934 C++ browser_tests.exe!ChromeBrowserMainParts::PreMainMessageLoopRun() Line 1238 C++ content.dll!content::BrowserMainLoop::PreMainMessageLoopRun() Line 1155 C++ content.dll!base::internal::FunctorTraits<int (__cdecl content::BrowserMainLoop::*)(void) __ptr64,void>::Invoke<content::BrowserMainLoop * __ptr64>(int(content::BrowserMainLoop::*)() method, content::BrowserMainLoop * && receiver_ptr) Line 215 C++ content.dll!base::internal::InvokeHelper<0,int>::MakeItSo<int (__cdecl content::BrowserMainLoop::*const & __ptr64)(void) __ptr64,content::BrowserMainLoop * __ptr64>(int(content::BrowserMainLoop::*)() & functor, content::BrowserMainLoop * && <args_0>) Line 287 C++ content.dll!base::internal::Invoker<base::internal::BindState<int (__cdecl content::BrowserMainLoop::*)(void) __ptr64,base::internal::UnretainedWrapper<content::BrowserMainLoop> >,int __cdecl(void)>::RunImpl<int (__cdecl content::BrowserMainLoop::*const & __ptr64)(void) __ptr64,std::tuple<base::internal::UnretainedWrapper<content::BrowserMainLoop> > const & __ptr64,0>(int(content::BrowserMainLoop::*)() & functor, const std::tuple<base::internal::UnretainedWrapper<content::BrowserMainLoop> > & bound, base::IndexSequence<0> __formal) Line 365 C++ content.dll!base::internal::Invoker<base::internal::BindState<int (__cdecl content::BrowserMainLoop::*)(void) __ptr64,base::internal::UnretainedWrapper<content::BrowserMainLoop> >,int __cdecl(void)>::Run(base::internal::BindStateBase * base) Line 343 C++ content.dll!base::internal::RunMixin<base::Callback<int __cdecl(void),1,1> >::Run() Line 86 C++ content.dll!content::StartupTaskRunner::RunAllTasksNow() Line 45 C++ content.dll!content::BrowserMainLoop::CreateStartupTasks() Line 965 C++ content.dll!content::BrowserMainRunnerImpl::Initialize(const content::MainFunctionParams & parameters) Line 127 C++ content.dll!content::BrowserMain(const content::MainFunctionParams & parameters) Line 42 C++ content.dll!content::RunNamedProcessTypeMain(const std::basic_string<char,std::char_traits<char>,std::allocator<char> > & process_type, const content::MainFunctionParams & main_function_params, content::ContentMainDelegate * delegate) Line 416 C++ content.dll!content::ContentMainRunnerImpl::Run() Line 793 C++ content.dll!content::ContentMain(const content::ContentMainParams & params) Line 20 C++ browser_tests.exe!content::BrowserTestBase::SetUp() Line 316 C++ browser_tests.exe!InProcessBrowserTest::SetUp() Line 252 C++ browser_tests.exe!ExtensionBrowserTest::SetUp() Line 161 C++ browser_tests.exe!testing::internal::HandleExceptionsInMethodIfSupported<testing::Test,void>(testing::Test * object, void(testing::Test::*)() method, const char * location) Line 2460 C++ browser_tests.exe!testing::Test::Run() Line 2472 C++ browser_tests.exe!testing::TestInfo::Run() Line 2660 C++ browser_tests.exe!testing::TestCase::Run() Line 2775 C++ browser_tests.exe!testing::internal::UnitTestImpl::RunAllTests() Line 4648 C++ browser_tests.exe!testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl,bool>(testing::internal::UnitTestImpl * object, bool(testing::internal::UnitTestImpl::*)() method, const char * location) Line 2460 C++ browser_tests.exe!testing::UnitTest::Run() Line 4255 C++ browser_tests.exe!RUN_ALL_TESTS() Line 2238 C++ browser_tests.exe!base::TestSuite::Run() Line 271 C++ browser_tests.exe!ChromeTestSuiteRunner::RunTestSuite(int argc, char * * argv) Line 63 C++ browser_tests.exe!ChromeTestLauncherDelegate::RunTestSuite(int argc, char * * argv) Line 73 C++ browser_tests.exe!content::LaunchTests(content::TestLauncherDelegate * launcher_delegate, int default_jobs, int argc, char * * argv) Line 520 C++ browser_tests.exe!LaunchChromeTests(int default_jobs, content::TestLauncherDelegate * delegate, int argc, char * * argv) Line 129 C++ browser_tests.exe!main(int argc, char * * argv) Line 15 C++ As I see in code of ExtensionService::DisableExtension it does not always call NotifyExtensionUnloaded. There are several early exits from function. Can you please check what branch it takes on your system and why it does not call NotifyExtensionUnloaded in your case?
I added log statements and that's what I get just before the failure: [4456:1520:0123/170409.216:INFO:settings_overrides_api.cc(158)] !!!!! Loaded ebm hgofbepebakgpkckamogblaimpogp [4456:1520:0123/170409.241:INFO:settings_overrides_api.cc(204)] !!!!! Unloaded e bmhgofbepebakgpkckamogblaimpogp [4456:1520:0123/170409.431:INFO:settings_overrides_api.cc(158)] !!!!! Loaded ebm hgofbepebakgpkckamogblaimpogp As you can see the extension is unloaded. However, TemplateURLService knows nothing about it as RemoveExtensionControlledTURL() is executed only if url_service_->loaded(). That's a real bug.
On 2017/01/23 16:09:26, vasilii wrote: > I added log statements and that's what I get just before the failure: > > [4456:1520:0123/170409.216:INFO:settings_overrides_api.cc(158)] !!!!! Loaded ebm > hgofbepebakgpkckamogblaimpogp > [4456:1520:0123/170409.241:INFO:settings_overrides_api.cc(204)] !!!!! Unloaded e > bmhgofbepebakgpkckamogblaimpogp > [4456:1520:0123/170409.431:INFO:settings_overrides_api.cc(158)] !!!!! Loaded ebm > hgofbepebakgpkckamogblaimpogp > > As you can see the extension is unloaded. However, TemplateURLService knows > nothing about it as RemoveExtensionControlledTURL() is executed only if > url_service_->loaded(). That's a real bug. Many thanks for help in remote debugging of this bug :) I have added locally delay in keyword table load and was able to reproduce this problem. I have created fix and test. PTAL again.
The CQ bit was checked by a-v-y@yandex-team.ru
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
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
I don't think I really understand the passing-the-extension-ID-in-prefs thing. It seems like this data ought to be available elsewhere in the TemplateURLService already, or should be passed in as a simple function argument, or something. It sounded as if maybe that change was about addressing another incident you noticed while trying to reproduce the original test flakiness. If that issue is already a problem today (i.e. without landing the original patch here), can we land the two changes separately, so we can get the original change + flakiness fix in and then take a look at this other issue in isolation? https://codereview.chromium.org/2639153002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/settings_overrides/settings_overrides_api.cc (right): https://codereview.chromium.org/2639153002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/settings_overrides/settings_overrides_api.cc:247: // Set extension id to prefs, so it can be used by TemplateURLService to Nit: Set -> Save https://codereview.chromium.org/2639153002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/settings_overrides/settings_overrides_browsertest.cc (right): https://codereview.chromium.org/2639153002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/settings_overrides/settings_overrides_browsertest.cc:204: IN_PROC_BROWSER_TEST_F(ExtensionBrowserTest, BeforeTemplateUrlServiceLoad) { Nit: Add a comment about what this is testing. https://codereview.chromium.org/2639153002/diff/40001/chrome/browser/search_e... File chrome/browser/search_engines/template_url_service_unittest.cc (right): https://codereview.chromium.org/2639153002/diff/40001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_unittest.cc:130: if (!ignore_keyword) { Nit: No {} https://codereview.chromium.org/2639153002/diff/40001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_unittest.cc:1550: true); Should we be checking as to which engine's keyword got changed? https://codereview.chromium.org/2639153002/diff/40001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_unittest.cc:1553: // Check that two extensions with same engine handled corrected. What parts of the engines being "the same" are relevant here? The keyword? The search URL? The fact that literally every piece of data from the two engines is the same?
https://codereview.chromium.org/2639153002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/settings_overrides/settings_overrides_api.cc (right): https://codereview.chromium.org/2639153002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/settings_overrides/settings_overrides_api.cc:247: // Set extension id to prefs, so it can be used by TemplateURLService to On 2017/01/25 at 22:31:23, Peter Kasting wrote: > Nit: Set -> Save Done https://codereview.chromium.org/2639153002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/settings_overrides/settings_overrides_browsertest.cc (right): https://codereview.chromium.org/2639153002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/settings_overrides/settings_overrides_browsertest.cc:204: IN_PROC_BROWSER_TEST_F(ExtensionBrowserTest, BeforeTemplateUrlServiceLoad) { On 2017/01/25 at 22:31:23, Peter Kasting wrote: > Nit: Add a comment about what this is testing. Done https://codereview.chromium.org/2639153002/diff/40001/chrome/browser/search_e... File chrome/browser/search_engines/template_url_service_unittest.cc (right): https://codereview.chromium.org/2639153002/diff/40001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_unittest.cc:130: if (!ignore_keyword) { On 2017/01/25 at 22:31:23, Peter Kasting wrote: > Nit: No {} Done https://codereview.chromium.org/2639153002/diff/40001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_unittest.cc:1550: true); On 2017/01/25 at 22:31:23, Peter Kasting wrote: > Should we be checking as to which engine's keyword got changed? Done https://codereview.chromium.org/2639153002/diff/40001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_unittest.cc:1553: // Check that two extensions with same engine handled corrected. On 2017/01/25 at 22:31:23, Peter Kasting wrote: > What parts of the engines being "the same" are relevant here? The keyword? The search URL? The fact that literally every piece of data from the two engines is the same? The keyword conflict is relevant. Changed comments and code to highlight this.
On 2017/01/25 at 22:31:24, pkasting wrote: > I don't think I really understand the passing-the-extension-ID-in-prefs thing. It seems like this data ought to be available elsewhere in the TemplateURLService already, or should be passed in as a simple function argument, or something. Why do I need to store extension_id in prefs: With new solution TemplateURLService can have initial_default_search_provider_ value controlled by extension at the very start of TemplateURLService initialization, before TemplateURLService or extensions are even loaded. This extension engine is created from TemplateURLData that is read by DefaultSearchManager on creation from extension controlled default_search_provider_data.template_url_data preference. Currently DefaultSearchManager and TemplateURLService do not know what extension controls this preference. They only know that its extension controlled one. Before TemplateURLService turns to loaded state all extensions are already loaded and have added their TemplateURLs to TemplateURLService calling AddExtensionControlledTURL from SettingsOverridesAPI::OnExtensionLoaded function. So, when TemplateURLService turns to loaded state it calls ApplyDefaultSearchChangeNoMetrics(initial_default_search_provider_). At this moment we need to find TemplateURL added by extension that corresponds to initial_default_search_provider_ and set it as default_search_provider_. The problem is that previously this was done using FindMatchingExtensionTemplateURL function and it worked because default extension engine could only be added on extension load. Now where is problem - TemplateURLData written by extension to default_search_provider_data.template_url_data pref can be different from TemplateURLData that is stored in TemplateURL created by extension. Reasons for this are keyword conflict resolution between search engines with same keyword. For example, first extension loads and creates TemplateURL with "keyword" keyword and writes its TemplateURLData to default_search_provider_data.template_url_data pref. After that second extension (with non default engine) loads and creates TemplateURL with "keyword" keyword. After keyword conflict resolution, keyword for first extension is changed to "keyword_" but default_search_provider_data.template_url_data pref still hold original "keyword" value. After browser is restarted, TemplateURLService first reads TemplateURLData from default_search_provider_data.template_url_data pref and after turning to loaded state it could not correctly find first extension TemplateURL because its keyword is changed to "keyword_". I think the straightest way to fix this problem is for TemplateURLService to know what extension currently controls default_search_provider_data.template_url_data pref. This could be done either by storing extension_id additionally in default_search_provider_data.template_url_data dictionary or by using ExtensionPrefValueMap::GetExtensionControllingPref function. To add ability to call GetExtensionControllingPref we need to add this function to TemplateURLServiceClient interface. I did so at first attempts of previous CL and there vasilii recommended to simply store extension id in prefs if its needed.
Why not just say in conflict resolution that, if two extensions try to register the same keyword, the current default always wins? This actually seems reasonable to me; if you installed an extension and made it your DSE (exceedingly unlikely), then installing another extension that uses the same keyword shouldn't auto-boot the first extension. I think this also parallels non-extension keywords; if we try to auto-add an engine that generates the same keyword as the current DSE, we won't evict it.
On 2017/01/26 at 08:52:21, pkasting wrote: > Why not just say in conflict resolution that, if two extensions try to register the same keyword, the current default always wins? > > This actually seems reasonable to me; if you installed an extension and made it your DSE (exceedingly unlikely), then installing another extension that uses the same keyword shouldn't auto-boot the first extension. > > I think this also parallels non-extension keywords; if we try to auto-add an engine that generates the same keyword as the current DSE, we won't evict it. To know what extension is controlling default search we need to get current owner of default_search_provider_data.template_url_data pref :) And solutions for this I described in last lines. To change how extension conflicting keywords resolve is another idea worth separate CL. In current CL I propose to leave keyword resolutions unchanged.
On 2017/01/26 13:58:38, Alexander Yashkin wrote: > On 2017/01/26 at 08:52:21, pkasting wrote: > > Why not just say in conflict resolution that, if two extensions try to > register the same keyword, the current default always wins? > > > > This actually seems reasonable to me; if you installed an extension and made > it your DSE (exceedingly unlikely), then installing another extension that uses > the same keyword shouldn't auto-boot the first extension. > > > > I think this also parallels non-extension keywords; if we try to auto-add an > engine that generates the same keyword as the current DSE, we won't evict it. > > To know what extension is controlling default search we need to get current > owner of default_search_provider_data.template_url_data pref :) > And solutions for this I described in last lines. > > To change how extension conflicting keywords resolve is another idea worth > separate CL. > In current CL I propose to leave keyword resolutions unchanged. The goal of my proposal was to avoid the need to ever know the owner of this pref. In general, the TemplateURLService knows what extension is controlling every TemplateURL through the AssociatedExtensionInfo struct. So you'd only need this information for time windows when you somehow don't have that struct. I assume the keyword conflict resolution you're describing is such a time, and in that case I'd say that the conflict resolution algorithm should explicitly check whether one of the involved engines currently matches the contents of the DSE pref, and if so, avoids changing anything related to that. Then you'll never need to worry that the deserialized pref will no longer match something somewhere.
On 2017/01/26 at 18:05:04, pkasting wrote: > On 2017/01/26 13:58:38, Alexander Yashkin wrote: > > On 2017/01/26 at 08:52:21, pkasting wrote: > > > Why not just say in conflict resolution that, if two extensions try to > > register the same keyword, the current default always wins? > > > > > > This actually seems reasonable to me; if you installed an extension and made > > it your DSE (exceedingly unlikely), then installing another extension that uses > > the same keyword shouldn't auto-boot the first extension. > > > > > > I think this also parallels non-extension keywords; if we try to auto-add an > > engine that generates the same keyword as the current DSE, we won't evict it. > > > > To know what extension is controlling default search we need to get current > > owner of default_search_provider_data.template_url_data pref :) > > And solutions for this I described in last lines. > > > > To change how extension conflicting keywords resolve is another idea worth > > separate CL. > > In current CL I propose to leave keyword resolutions unchanged. > > The goal of my proposal was to avoid the need to ever know the owner of this pref. In general, the TemplateURLService knows what extension is controlling every TemplateURL through the AssociatedExtensionInfo struct. So you'd only need this information for time windows when you somehow don't have that struct. I assume the keyword conflict resolution you're describing is such a time, and in that case I'd say that the conflict resolution algorithm should explicitly check whether one of the involved engines currently matches the contents of the DSE pref, and if so, avoids changing anything related to that. Then you'll never need to worry that the deserialized pref will no longer match something somewhere. IMHO, we could not avoid keyword conflicts and keyword modifications because of them. So the problem of matching extension TemplateURLData stored in pref and real TemplateURLData that is stored in TemplateURL will remain. Example that came to my mind: 1. Extension 1 is installed with default search, its keyword is "keyword". It writes its TemplateURLData to extension controlled DSE pref. 2. Extension 2 is installed with default search, its keyword is "keyword". It writes its TemplateURLData to extension controlled DSE pref. Extension 1 keyword in TemplateURLService is legally changed to "keyword_" to avoid conflict. 3. User disable extension 2 in chrome://extensions page. In SettingsOverridesAPI::OnExtensionUnloaded DSE pref controlled by extension 2 is unset. TemplateURL controlled by extension 2 is removed from TemplateURLService. 4. Pref observer in DefaultSearchManager is notified of DSE pref changes. It loads TemplateURLData from pref controlled by extension 1 that contains "keyword" value yet TemplateURLService contains TemplateURL for extension 1 with "keyword_" as a result of conflict. So we could not match pref value with values stored in TemplateURLService. Simplest solution is to store or get from extension prefs extension_id of extension that is currently controlling DSE pref.
On 2017/01/27 05:26:38, Alexander Yashkin wrote: > On 2017/01/26 at 18:05:04, pkasting wrote: > > On 2017/01/26 13:58:38, Alexander Yashkin wrote: > > > On 2017/01/26 at 08:52:21, pkasting wrote: > > > > Why not just say in conflict resolution that, if two extensions try to > > > register the same keyword, the current default always wins? > > > > > > > > This actually seems reasonable to me; if you installed an extension and > made > > > it your DSE (exceedingly unlikely), then installing another extension that > uses > > > the same keyword shouldn't auto-boot the first extension. > > > > > > > > I think this also parallels non-extension keywords; if we try to auto-add > an > > > engine that generates the same keyword as the current DSE, we won't evict > it. > > > > > > To know what extension is controlling default search we need to get current > > > owner of default_search_provider_data.template_url_data pref :) > > > And solutions for this I described in last lines. > > > > > > To change how extension conflicting keywords resolve is another idea worth > > > separate CL. > > > In current CL I propose to leave keyword resolutions unchanged. > > > > The goal of my proposal was to avoid the need to ever know the owner of this > pref. In general, the TemplateURLService knows what extension is controlling > every TemplateURL through the AssociatedExtensionInfo struct. So you'd only > need this information for time windows when you somehow don't have that struct. > I assume the keyword conflict resolution you're describing is such a time, and > in that case I'd say that the conflict resolution algorithm should explicitly > check whether one of the involved engines currently matches the contents of the > DSE pref, and if so, avoids changing anything related to that. Then you'll > never need to worry that the deserialized pref will no longer match something > somewhere. > > IMHO, we could not avoid keyword conflicts and keyword modifications because of > them. > So the problem of matching extension TemplateURLData stored in pref and real > TemplateURLData that is stored in TemplateURL will remain. > > Example that came to my mind: > 1. Extension 1 is installed with default search, its keyword is "keyword". It > writes its TemplateURLData to extension controlled DSE pref. > 2. Extension 2 is installed with default search, its keyword is "keyword". It > writes its TemplateURLData to extension controlled DSE pref. Extension 1 keyword > in TemplateURLService is legally changed to "keyword_" to avoid conflict. > 3. User disable extension 2 in chrome://extensions page. In > SettingsOverridesAPI::OnExtensionUnloaded DSE pref controlled by extension 2 is > unset. TemplateURL controlled by extension 2 is removed from TemplateURLService. > > 4. Pref observer in DefaultSearchManager is notified of DSE pref changes. It > loads TemplateURLData from pref controlled by extension 1 that contains > "keyword" value yet TemplateURLService contains TemplateURL for extension 1 with > "keyword_" as a result of conflict. So we could not match pref value with values > stored in TemplateURLService. > > Simplest solution is to store or get from extension prefs extension_id of > extension that is currently controlling DSE pref. At the point the user uninstalls the second extension, they're going to want the first extension to be triggered by "keyword", not "keyword_". So if we really care about this case, we can't address it by changing keywords to begin with; we need to shadow things instead. But this case is exceptionally rare (installing an extension and then setting its engine as DSE is rare, and installing two different extensions that provide DSEs on the same keyword is unheard of). This is why I asked if we couldn't separate solving this problem into a distinct followup CL where we could argue this out without complicating the main change.
On 2017/01/27 at 06:15:25, pkasting wrote: > On 2017/01/27 05:26:38, Alexander Yashkin wrote: > > On 2017/01/26 at 18:05:04, pkasting wrote: > > > On 2017/01/26 13:58:38, Alexander Yashkin wrote: > > > > On 2017/01/26 at 08:52:21, pkasting wrote: > > > > > Why not just say in conflict resolution that, if two extensions try to > > > > register the same keyword, the current default always wins? > > > > > > > > > > This actually seems reasonable to me; if you installed an extension and > > made > > > > it your DSE (exceedingly unlikely), then installing another extension that > > uses > > > > the same keyword shouldn't auto-boot the first extension. > > > > > > > > > > I think this also parallels non-extension keywords; if we try to auto-add > > an > > > > engine that generates the same keyword as the current DSE, we won't evict > > it. > > > > > > > > To know what extension is controlling default search we need to get current > > > > owner of default_search_provider_data.template_url_data pref :) > > > > And solutions for this I described in last lines. > > > > > > > > To change how extension conflicting keywords resolve is another idea worth > > > > separate CL. > > > > In current CL I propose to leave keyword resolutions unchanged. > > > > > > The goal of my proposal was to avoid the need to ever know the owner of this > > pref. In general, the TemplateURLService knows what extension is controlling > > every TemplateURL through the AssociatedExtensionInfo struct. So you'd only > > need this information for time windows when you somehow don't have that struct. > > I assume the keyword conflict resolution you're describing is such a time, and > > in that case I'd say that the conflict resolution algorithm should explicitly > > check whether one of the involved engines currently matches the contents of the > > DSE pref, and if so, avoids changing anything related to that. Then you'll > > never need to worry that the deserialized pref will no longer match something > > somewhere. > > > > IMHO, we could not avoid keyword conflicts and keyword modifications because of > > them. > > So the problem of matching extension TemplateURLData stored in pref and real > > TemplateURLData that is stored in TemplateURL will remain. > > > > Example that came to my mind: > > 1. Extension 1 is installed with default search, its keyword is "keyword". It > > writes its TemplateURLData to extension controlled DSE pref. > > 2. Extension 2 is installed with default search, its keyword is "keyword". It > > writes its TemplateURLData to extension controlled DSE pref. Extension 1 keyword > > in TemplateURLService is legally changed to "keyword_" to avoid conflict. > > 3. User disable extension 2 in chrome://extensions page. In > > SettingsOverridesAPI::OnExtensionUnloaded DSE pref controlled by extension 2 is > > unset. TemplateURL controlled by extension 2 is removed from TemplateURLService. > > > > 4. Pref observer in DefaultSearchManager is notified of DSE pref changes. It > > loads TemplateURLData from pref controlled by extension 1 that contains > > "keyword" value yet TemplateURLService contains TemplateURL for extension 1 with > > "keyword_" as a result of conflict. So we could not match pref value with values > > stored in TemplateURLService. > > > > Simplest solution is to store or get from extension prefs extension_id of > > extension that is currently controlling DSE pref. > > At the point the user uninstalls the second extension, they're going to want the first extension to be triggered by "keyword", not "keyword_". So if we really care about this case, we can't address it by changing keywords to begin with; we need to shadow things instead. > > But this case is exceptionally rare (installing an extension and then setting its engine as DSE is rare, and installing two different extensions that provide DSEs on the same keyword is unheard of). This is why I asked if we couldn't separate solving this problem into a distinct followup CL where we could argue this out without complicating the main change. Ok, real case I have stumbled on and after which I decided to add this complications. 1. I have extension with yandex search engine installed in my debug chrome. Its keyword is "yandex". 2. Prepopulated set for my country contains yandex search engine with "yandex" keyword. 3. After changes in this CL extensions engines are loaded before prepopulated engines. 4. On load yandex extension writes TemplateURLData to DSE pref and registers itself into TemplateURLService, its keyword in TemplateURLService is "yandex". 5. When keyword DB is loaded prepopulated engines are added, one of them prepopulated yandex engine with keyword "yandex". Extension keyword in TemplateURLService is changed to "yandex_" because of keyword conflict resolution. In current alghoritm last added keyword replaces previous. 6. When TemplateURLService is turned to loaded state it could not match its initial TemplateURLData for extension with current extension TemplateURL because of changed "keyword". So its not clear what engine must be set as default. Extension engine exists, yet we could not match it. To solve this problem I have added extension_id to DSE pref. I think its simplest solution for above situation. You propose changing keyword resolution so current default extension will always keep its keyword. After thinking for a while I agree that keyword resolution must be changed, because now extensions are loaded earlier than prepopulated engines and extension keywords are overriden instead of prepopulated keywords. For me it seems not right that installed extension looses to prepopulated engine. What my concerns are - I think that changing keyword resolution potentially will create another wave of problems, solving which can lead to overgrowing current CL even more. IMHO, changing keyword resolution is worth separate CL with its own tests for all kind of engines combinations (several extensions, extension and omnibox api, prepopulated and extension, extension and managed). Also storing extension_id, IMHO, anyway is better and more clear way of choosing extension default search, because choosing engine by matching TemplateURLData can in exotic cases (2 extensions with same engines) match to engine from another extension or fail to match at all as I accidentally encountered in the case above. I propose to leave current solution and change keyword resolution in another request. If you feel that changing keyword resolution is more appropriate way of solving this problem, I will try to implement it here. "installing an extension and then setting its engine as DSE is rare" - I think its rare on a large scale of things yet even yandex extension has 223,635 users. And its search engine is set as default just after user installed extension and agreed that their search will be changed. User dont need to change anything manually. Thanks for quick responses.
On 2017/01/27 08:13:25, Alexander Yashkin wrote: > Ok, real case I have stumbled on and after which I decided to add this > complications. > 1. I have extension with yandex search engine installed in my debug chrome. Its > keyword is "yandex". > 2. Prepopulated set for my country contains yandex search engine with "yandex" > keyword. The prepopulated set has the keyword "yandex.com", not "yandex". Does the extension use "yandex.com" instead of "yandex" as you have said? The system was not designed to handle extensions trying to use keywords that look like the prepopulated ones. For a bunch of reasons, that feels like Doing It Wrong to me. Why can't such users use the prepopulated entries? > 3. After changes in this CL extensions engines are loaded before prepopulated > engines. > 4. On load yandex extension writes TemplateURLData to DSE pref and registers > itself into TemplateURLService, its keyword in TemplateURLService is "yandex". > 5. When keyword DB is loaded prepopulated engines are added, one of them > prepopulated yandex engine with keyword "yandex". Extension keyword in > TemplateURLService is changed to "yandex_" because of keyword conflict > resolution. In current alghoritm last added keyword replaces previous. Right, the conflict resolution algorithm here is clearly wrong. No extension keyword should be ever changed, and that goes double for an extension keyword set as default. > Also storing extension_id, IMHO, anyway is better and more clear way of choosing > extension default search, > because choosing engine by matching TemplateURLData can in exotic cases (2 > extensions with same engines) match to engine from another extension > or fail to match at all as I accidentally encountered in the case above. I feel uncomfortable about this. The extension ID for a TemplateURL is already available in the system. If the problem is that we can't look it up because we've mangled the TemplateURL, then we should fix the mangling and see if we still have problems. The existing solution here has a bunch of small but annoying costs. Writing additional prefs isn't totally free -- we load way too many on startup and it causes a measurable startup hit -- so ideally we wouldn't add them if we can avoid it, though I concede that one string pref is not going to make much difference. Duplicating this data from one data store into another adds ways things can get out of sync; we've suffered this a lot over the years with mismatches between pref and database data for TemplateURLService, and I'm leery of having more. And the plumbing here feels awkward. It sounds like you have found cases where the current keyword resolution algorithm doesn't do the right thing, so IMO, that should be fixed separately from, and in advance of, landing this change, at which point this could be landed without the changes to plumb the extension ID. I realize that would take some time, but it seems like the original bug here is comparatively minor and taking a while to fix it would be OK. Maybe I'm misunderstanding the original bug.
On 2017/01/27 at 19:51:47, pkasting wrote:
> On 2017/01/27 08:13:25, Alexander Yashkin wrote:
> > Ok, real case I have stumbled on and after which I decided to add this
> > complications.
> > 1. I have extension with yandex search engine installed in my debug chrome.
Its
> > keyword is "yandex".
> > 2. Prepopulated set for my country contains yandex search engine with
"yandex"
> > keyword.
>
> The prepopulated set has the keyword "yandex.com", not "yandex". Does the
extension use "yandex.com" instead of "yandex" as you have said?
Keyword for extension and prepopulated engine is "yandex.ru".
I think developers of yandex extension mostly copied settings from
prepopulated_engines.json in chrome.
"chrome_settings_overrides": {
"search_provider": {
"encoding": "UTF-8",
"favicon_url":
"https://yastatic.net/islands/_/aKnllxm-gQhidpzbZqub7qe641g.ico",
"image_url":
"https://yandex.ru/images/search?rpt=imageview&cbird=5&clid=2220366",
"image_url_post_params":
"upfile={google:imageThumbnail},original_width={google:imageOriginalWidth},original_height={google:imageOriginalHeight},prg=1",
"is_default": true,
"keyword": "yandex.ru",
"name": "__MSG_searchName__",
"prepopulated_id": 15,
"search_url":
"https://yandex.ru/search/?from=chromesearch&clid=2196598&text={searchTerms}",
"suggest_url":
"https://suggest.yandex.net/suggest-ff.cgi?uil=ru&part={searchTerms}"
}
},
>
> The system was not designed to handle extensions trying to use keywords that
look like the prepopulated ones. For a bunch of reasons, that feels like Doing
It Wrong to me. Why can't such users use the prepopulated entries?
>
> > 3. After changes in this CL extensions engines are loaded before
prepopulated
> > engines.
> > 4. On load yandex extension writes TemplateURLData to DSE pref and registers
> > itself into TemplateURLService, its keyword in TemplateURLService is
"yandex".
> > 5. When keyword DB is loaded prepopulated engines are added, one of them
> > prepopulated yandex engine with keyword "yandex". Extension keyword in
> > TemplateURLService is changed to "yandex_" because of keyword conflict
> > resolution. In current alghoritm last added keyword replaces previous.
>
> Right, the conflict resolution algorithm here is clearly wrong. No extension
keyword should be ever changed, and that goes double for an extension keyword
set as default.
>
> > Also storing extension_id, IMHO, anyway is better and more clear way of
choosing
> > extension default search,
> > because choosing engine by matching TemplateURLData can in exotic cases (2
> > extensions with same engines) match to engine from another extension
> > or fail to match at all as I accidentally encountered in the case above.
>
> I feel uncomfortable about this. The extension ID for a TemplateURL is
already available in the system. If the problem is that we can't look it up
because we've mangled the TemplateURL, then we should fix the mangling and see
if we still have problems. The existing solution here has a bunch of small but
annoying costs. Writing additional prefs isn't totally free -- we load way too
many on startup and it causes a measurable startup hit -- so ideally we wouldn't
add them if we can avoid it, though I concede that one string pref is not going
to make much difference. Duplicating this data from one data store into another
adds ways things can get out of sync; we've suffered this a lot over the years
with mismatches between pref and database data for TemplateURLService, and I'm
leery of having more. And the plumbing here feels awkward.
>
> It sounds like you have found cases where the current keyword resolution
algorithm doesn't do the right thing, so IMO, that should be fixed separately
from, and in advance of, landing this change, at which point this could be
landed without the changes to plumb the extension ID.
>
> I realize that would take some time, but it seems like the original bug here
is comparatively minor and taking a while to fix it would be OK. Maybe I'm
misunderstanding the original bug.
Ok, I will submit new changes as they will be ready.
On 2017/01/27 20:14:49, Alexander Yashkin wrote:
> On 2017/01/27 at 19:51:47, pkasting wrote:
> > On 2017/01/27 08:13:25, Alexander Yashkin wrote:
> > > Ok, real case I have stumbled on and after which I decided to add this
> > > complications.
> > > 1. I have extension with yandex search engine installed in my debug
chrome.
> Its
> > > keyword is "yandex".
> > > 2. Prepopulated set for my country contains yandex search engine with
> "yandex"
> > > keyword.
> >
> > The prepopulated set has the keyword "yandex.com", not "yandex". Does the
> extension use "yandex.com" instead of "yandex" as you have said?
>
> Keyword for extension and prepopulated engine is "yandex.ru".
Ah, mea culpa. Always good to be wrong when you correct someone else's mistake
:)
> I think developers of yandex extension mostly copied settings from
> prepopulated_engines.json in chrome.
>
> "chrome_settings_overrides": {
> "search_provider": {
> "encoding": "UTF-8",
> "favicon_url":
> "https://yastatic.net/islands/_/aKnllxm-gQhidpzbZqub7qe641g.ico",
> "image_url":
> "https://yandex.ru/images/search?rpt=imageview&cbird=5&clid=2220366",
> "image_url_post_params":
>
"upfile={google:imageThumbnail},original_width={google:imageOriginalWidth},original_height={google:imageOriginalHeight},prg=1",
> "is_default": true,
> "keyword": "yandex.ru",
> "name": "__MSG_searchName__",
> "prepopulated_id": 15,
> "search_url":
> "https://yandex.ru/search/?from=chromesearch&clid=2196598&text={searchTerms}",
> "suggest_url":
> "https://suggest.yandex.net/suggest-ff.cgi?uil=ru&part={searchTerms}"
> }
> },
Workaround: the Yandex extension should not use "yandex.ru" as a keyword.
Longer-term change: can extensions just ask to set an existing prepopulated
engine as default? If so, the Yandex extension should do that, not add a new
duplicate entry. If not, maybe the feature request is that we change the
extension system to do this.
On 2017/01/27 at 20:25:13, pkasting wrote:
> On 2017/01/27 20:14:49, Alexander Yashkin wrote:
> > On 2017/01/27 at 19:51:47, pkasting wrote:
> > > On 2017/01/27 08:13:25, Alexander Yashkin wrote:
> > > > Ok, real case I have stumbled on and after which I decided to add this
> > > > complications.
> > > > 1. I have extension with yandex search engine installed in my debug
chrome.
> > Its
> > > > keyword is "yandex".
> > > > 2. Prepopulated set for my country contains yandex search engine with
> > "yandex"
> > > > keyword.
> > >
> > > The prepopulated set has the keyword "yandex.com", not "yandex". Does the
> > extension use "yandex.com" instead of "yandex" as you have said?
> >
> > Keyword for extension and prepopulated engine is "yandex.ru".
>
> Ah, mea culpa. Always good to be wrong when you correct someone else's
mistake :)
>
> > I think developers of yandex extension mostly copied settings from
> > prepopulated_engines.json in chrome.
> >
> > "chrome_settings_overrides": {
> > "search_provider": {
> > "encoding": "UTF-8",
> > "favicon_url":
> > "https://yastatic.net/islands/_/aKnllxm-gQhidpzbZqub7qe641g.ico",
> > "image_url":
> > "https://yandex.ru/images/search?rpt=imageview&cbird=5&clid=2220366",
> > "image_url_post_params":
> >
"upfile={google:imageThumbnail},original_width={google:imageOriginalWidth},original_height={google:imageOriginalHeight},prg=1",
> > "is_default": true,
> > "keyword": "yandex.ru",
> > "name": "__MSG_searchName__",
> > "prepopulated_id": 15,
> > "search_url":
> >
"https://yandex.ru/search/?from=chromesearch&clid=2196598&text={searchTerms}",
> > "suggest_url":
> > "https://suggest.yandex.net/suggest-ff.cgi?uil=ru&part={searchTerms}"
> > }
> > },
>
> Workaround: the Yandex extension should not use "yandex.ru" as a keyword.
>
> Longer-term change: can extensions just ask to set an existing prepopulated
engine as default? If so, the Yandex extension should do that, not add a new
duplicate entry. If not, maybe the feature request is that we change the
extension system to do this.
chrome_settings_overrides already have "prepopulated_id" field, it is used to
load TemplateURLData used to initialize fields of extension engine.
After that its fields are overriden by those values that exists in extension
manifest.
I think extension developers want to override some urls to add additional
information, for example indicating that
search requests are coming from extension engine not prepopulated one.
So I am not sure if they simply want to switch search to prepopulated one.
I agree with Peter that the discussion about the ID in prefs is irrelevant to both https://crbug.com/450534 and https://crbug.com/679470. If this is a reland of https://codereview.chromium.org/2479113002/ then it shouldn't contain fixes for a different bug. Regarding the discussion: - I believe that TemplateURLService should never try to modify the keywords and other data explicitly written in the extension's manifest. That is, TemplateURLService::FindMatchingExtensionTemplateURL() should always work. - I think that an extension search engine may hide the prepopulated search engine with the same prepopulated_id. This is definitely a feature and not a bug fix. There are interesting side effects to contemplate. E.g. there is a prepopulated default search engine. An extension tries to overwrite it but has a flag "is_default": false.
On 2017/01/30 at 15:39:17, vasilii wrote: > I agree with Peter that the discussion about the ID in prefs is irrelevant to both https://crbug.com/450534 and https://crbug.com/679470. If this is a reland of https://codereview.chromium.org/2479113002/ then it shouldn't contain fixes for a different bug. Not so clear, if this is a fix for a different bug - while fixing https://crbug.com/450534 I have changed sequence of engines added to TemplateURLService. Previously prepopulated engines were added first and after that extension engines overriding keywords from prepopulated engines. Now extension engines are added first and prepopulated later overriding keywords for extension engine. So without changes in https://crbug.com/450534 everything was working as intended. Now with this fix we have problem matching extension TemplateURLData from extension DSE pref with TemplateURL in TemplateURLService and sometimes extension engine could not be set in TemplateURLService even if extension is overriding DSE. I would say that correctly matching extension TemplateURLData to extension TemplateURL in all scenarios is part of whole solution. But its a sort of philosophical question now. We agreed with Peter that I submit changes in keyword conflicts resolution algorithm before and fix for extensions new_tab later. And after that hopefully we will not need extension_id stored in prefs. > > Regarding the discussion: > - I believe that TemplateURLService should never try to modify the keywords and other data explicitly written in the extension's manifest. That is, TemplateURLService::FindMatchingExtensionTemplateURL() should always work. Good to hear that, I'll try to implement it. > - I think that an extension search engine may hide the prepopulated search engine with the same prepopulated_id. This is definitely a feature and not a bug fix. There are interesting side effects to contemplate. E.g. there is a prepopulated default search engine. An extension tries to overwrite it but has a flag "is_default": false. I don't fully understand this last two sentences. Whats the expected outcome? Prepopulated engine will remain as default, but its keyword will be changed because of conflict with extension?
> > - I think that an extension search engine may hide the prepopulated search > engine with the same prepopulated_id. This is definitely a feature and not a bug > fix. There are interesting side effects to contemplate. E.g. there is a > prepopulated default search engine. An extension tries to overwrite it but has a > flag "is_default": false. > I don't fully understand this last two sentences. Whats the expected outcome? > Prepopulated engine will remain as default, but its keyword will be changed > because of conflict with extension? The expected outcome is not clear. It's obvious that the prepopulated engine will remain as default. But what happens to the extension search engine.
Vasilii, Peter PTAL at this CL again. No major changes, IMHO. I have rebased on master and updated after https://codereview.chromium.org/2682453002 has landed.
https://codereview.chromium.org/2639153002/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/api/settings_overrides/settings_overrides_browsertest.cc (right): https://codereview.chromium.org/2639153002/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/settings_overrides/settings_overrides_browsertest.cc:212: EXPECT_TRUE(VerifyTemplateURLServiceLoad(url_service)); What's going on here? Why you load the service in this test? https://codereview.chromium.org/2639153002/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/settings_overrides/settings_overrides_browsertest.cc:214: url_service->set_loaded(false); I don't think it's applicable here in the browser test as all the keywords were already loaded. https://codereview.chromium.org/2639153002/diff/80001/chrome/browser/search_e... File chrome/browser/search_engines/template_url_service_unittest.cc (right): https://codereview.chromium.org/2639153002/diff/80001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_unittest.cc:130: EXPECT_EQ(expected->url(), actual->url()); Why is the keyword check gone? https://codereview.chromium.org/2639153002/diff/80001/components/search_engin... File components/search_engines/template_url_service.cc (right): https://codereview.chromium.org/2639153002/diff/80001/components/search_engin... components/search_engines/template_url_service.cc:1937: : TemplateURL::NORMAL; Can here be more options like policy?
https://codereview.chromium.org/2639153002/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/api/settings_overrides/settings_overrides_browsertest.cc (right): https://codereview.chromium.org/2639153002/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/settings_overrides/settings_overrides_browsertest.cc:212: EXPECT_TRUE(VerifyTemplateURLServiceLoad(url_service)); On 2017/02/27 at 15:20:35, vasilii wrote: > What's going on here? Why you load the service in this test? I decided to ensure that TemplateURLService is loaded first before setting it explicitly to unloaded state. This is browser test so TemplateURLService for profile could be loaded or not loaded at the moment this test start to execute. It depends on the other tasks performed before current test, if they call TemplateURLServiceFactory::GetForProfile and DB task will execute before current task, TemplateURLService could be already loaded at the moment BeforeTemplateUrlServiceLoad is executed. I could be wrong, so please correct me if so. https://codereview.chromium.org/2639153002/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/settings_overrides/settings_overrides_browsertest.cc:214: url_service->set_loaded(false); On 2017/02/27 at 15:20:35, vasilii wrote: > I don't think it's applicable here in the browser test as all the keywords were already loaded. Why not applicable? I think set_loaded() function is very useful in this class, no matter if keywords were loaded or not. Just nearly every function in TemplateURLService has check for loaded_ flag and acts differently depending on its value. This test could catch the source of flakiness that we spend a lot of time tracing in current CL earlier. It checks that if extension is loaded/unloaded while TemplateURLService is in not-loaded state, extension controlled default search engine is set and removed correctly. https://codereview.chromium.org/2639153002/diff/80001/chrome/browser/search_e... File chrome/browser/search_engines/template_url_service_unittest.cc (right): https://codereview.chromium.org/2639153002/diff/80001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_unittest.cc:130: EXPECT_EQ(expected->url(), actual->url()); On 2017/02/27 at 15:20:35, vasilii wrote: > Why is the keyword check gone? My fault, did not restored previous version completely. Fixed. https://codereview.chromium.org/2639153002/diff/80001/components/search_engin... File components/search_engines/template_url_service.cc (right): https://codereview.chromium.org/2639153002/diff/80001/components/search_engin... components/search_engines/template_url_service.cc:1937: : TemplateURL::NORMAL; On 2017/02/27 at 15:20:35, vasilii wrote: > Can here be more options like policy? I think |source| can have DefaultSearchManager::FROM_POLICY or DefaultSearchManager::FROM_FALLBACK values, yet it still corresponds to TemplateURL::NORMAL engine type. Also where is minor bug here with bool changed = TemplateURL::MatchesData(initial_default_search_provider_.get(), data, search_terms_data()); which must be opposite, IMHO, bool changed = !TemplateURL::MatchesData() yet I leave its fix to separate CL.
LGTM with one substantive concern. https://codereview.chromium.org/2639153002/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/api/settings_overrides/settings_overrides_browsertest.cc (right): https://codereview.chromium.org/2639153002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/settings_overrides/settings_overrides_browsertest.cc:204: // This tests checks that extension overriding default search can be correctly Nit: "...that an extension overriding the default search engine can be correctly loaded before the TemplateURLService..." https://codereview.chromium.org/2639153002/diff/100001/components/search_engi... File components/search_engines/default_search_manager.cc (right): https://codereview.chromium.org/2639153002/diff/100001/components/search_engi... components/search_engines/default_search_manager.cc:263: MergePrefsDataWithPrepopulated(); Do we need to make this call regardless? It seems like not doing it means that if an extension is overriding the DSE, then no prepopulate data updates will be applied to any of the prepopulated engines. That sounds bad? What are the consequences of making this call even when an extension is controlling the DSE? https://codereview.chromium.org/2639153002/diff/100001/components/search_engi... File components/search_engines/template_url.h (right): https://codereview.chromium.org/2639153002/diff/100001/components/search_engi... components/search_engines/template_url.h:597: void set_extension_info( Nit: While here: I think this method is dead. Remove. https://codereview.chromium.org/2639153002/diff/100001/components/search_engi... File components/search_engines/template_url_service.cc (right): https://codereview.chromium.org/2639153002/diff/100001/components/search_engi... components/search_engines/template_url_service.cc:468: return template_url_ptr; Collapse these four lines into "return Add(...);". I wonder if this method shouldn't entirely disappear and be replaced by some kind of TemplateURL constructor that takes an AssociatedExtensionInfo pointer or something. https://codereview.chromium.org/2639153002/diff/100001/components/search_engi... components/search_engines/template_url_service.cc:486: NotifyObservers(); Nit: Collapse these two lines to a call to Remove().
https://codereview.chromium.org/2639153002/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/api/settings_overrides/settings_overrides_browsertest.cc (right): https://codereview.chromium.org/2639153002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/settings_overrides/settings_overrides_browsertest.cc:204: // This tests checks that extension overriding default search can be correctly On 2017/02/28 at 00:56:37, Peter Kasting wrote: > Nit: "...that an extension overriding the default search engine can be correctly loaded before the TemplateURLService..." Fixed, thanks. https://codereview.chromium.org/2639153002/diff/100001/components/search_engi... File components/search_engines/default_search_manager.cc (right): https://codereview.chromium.org/2639153002/diff/100001/components/search_engi... components/search_engines/default_search_manager.cc:263: MergePrefsDataWithPrepopulated(); On 2017/02/28 at 00:56:37, Peter Kasting wrote: > Do we need to make this call regardless? > > It seems like not doing it means that if an extension is overriding the DSE, then no prepopulate data updates will be applied to any of the prepopulated engines. That sounds bad? What are the consequences of making this call even when an extension is controlling the DSE? MergePrefsDataWithPrepopulated is updating values of default search engine restored from prefs with values from prepopulated engine if engine from prefs has valid prepopulate_id. No updates are applied to the prepopulated engines. Currently extension installed search engine can have non zero prepopulated_id field in extension manifest. This field is used to initialize extension engine fields on extension load to values from prepopulated engine. This is done in ConvertSearchProvider function. https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/settings_o... After initialization prepopulate_id field of newly created TemplateURLData is assigned zero at https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/settings_o... It seems logical since extension engine is not prepopulated and must not be returned by functions like FindPrepopulatedTemplateURL. So in current implementation MergePrefsDataWithPrepopulated will not affect default search engine installed by extension because extension engine has prepopulate_id = 0. I see no sense performing MergePrefsDataWithPrepopulated for extension engine. What I have thought of - where is possibility that rarely, after browser update, default search engine installed by extension will be reset to nullptr. This could happen if prepopulated engines change with browser update but extension engine fields were initialized and written to prefs while previous prepopulated engines version was active. In this case on TemplateURLService load we could fail to match TemplateURLData from prefs to TemplateURL created by extension on load and default_search_provider_ will be assigned nullptr in line https://cs.chromium.org/chromium/src/components/search_engines/template_url_s.... After browser restart default engine will be returned to extension controlled, but still. What solutions I could think of: 1. Again I think about storing extension_id of extension engine in prefs to use it for matching. 2. Another idea is keeping original prepopulate_id of extension search engines and updating their fields by MergePrefsDataWithPrepopulated in DefaultSearchManager. In this case we should ignore extension engines in all places there prepopulate_id is checked in TemplateURLService. This seems to me more error prone, but possible. 3. Or we can just ignore this situation due to its rare possibility. WDYT? https://codereview.chromium.org/2639153002/diff/100001/components/search_engi... File components/search_engines/template_url.h (right): https://codereview.chromium.org/2639153002/diff/100001/components/search_engi... components/search_engines/template_url.h:597: void set_extension_info( On 2017/02/28 00:56:37, Peter Kasting wrote: > Nit: While here: I think this method is dead. Remove. Done. https://codereview.chromium.org/2639153002/diff/100001/components/search_engi... File components/search_engines/template_url_service.cc (right): https://codereview.chromium.org/2639153002/diff/100001/components/search_engi... components/search_engines/template_url_service.cc:468: return template_url_ptr; On 2017/02/28 00:56:37, Peter Kasting wrote: > Collapse these four lines into "return Add(...);". > > I wonder if this method shouldn't entirely disappear and be replaced by some > kind of TemplateURL constructor that takes an AssociatedExtensionInfo pointer or > something. Collapse done. Good idea for refactoring, will do in separate CL. https://codereview.chromium.org/2639153002/diff/100001/components/search_engi... components/search_engines/template_url_service.cc:486: NotifyObservers(); On 2017/02/28 00:56:37, Peter Kasting wrote: > Nit: Collapse these two lines to a call to Remove(). Done.
https://codereview.chromium.org/2639153002/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/api/settings_overrides/settings_overrides_browsertest.cc (right): https://codereview.chromium.org/2639153002/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/settings_overrides/settings_overrides_browsertest.cc:212: EXPECT_TRUE(VerifyTemplateURLServiceLoad(url_service)); On 2017/02/27 19:36:03, Alexander Yashkin wrote: > On 2017/02/27 at 15:20:35, vasilii wrote: > > What's going on here? Why you load the service in this test? > > I decided to ensure that TemplateURLService is loaded first before setting it > explicitly to unloaded state. > This is browser test so TemplateURLService for profile could be loaded or not > loaded at the moment this test start to execute. > It depends on the other tasks performed before current test, if they call > TemplateURLServiceFactory::GetForProfile and DB task will execute > before current task, TemplateURLService could be already loaded at the moment > BeforeTemplateUrlServiceLoad is executed. > > I could be wrong, so please correct me if so. It's a browser test and what happens here should match the real behavior. There is no way TemplateUrlService may handle the results from the database and switch the flag back. https://codereview.chromium.org/2639153002/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/settings_overrides/settings_overrides_browsertest.cc:214: url_service->set_loaded(false); On 2017/02/27 19:36:03, Alexander Yashkin wrote: > On 2017/02/27 at 15:20:35, vasilii wrote: > > I don't think it's applicable here in the browser test as all the keywords > were already loaded. > > Why not applicable? I think set_loaded() function is very useful in this class, > no matter if keywords were loaded or not. > Just nearly every function in TemplateURLService has check for loaded_ flag and > acts differently depending on its value. > > This test could catch the source of flakiness that we spend a lot of time > tracing in current CL earlier. > It checks that if extension is loaded/unloaded while TemplateURLService is in > not-loaded state, > extension controlled default search engine is set and removed correctly. TemplateURLService::Load is called explicitly. If you say that it can be either loaded or not here then which component is calling the method? If it's critical for you that TemplateURLServiceLoad isn't loaded then it should be done either by suppressing Load() or delaying the result delivery. For me it's not that critical as the test should pass either way. https://codereview.chromium.org/2639153002/diff/80001/components/search_engin... File components/search_engines/template_url_service.cc (right): https://codereview.chromium.org/2639153002/diff/80001/components/search_engin... components/search_engines/template_url_service.cc:1937: : TemplateURL::NORMAL; On 2017/02/27 19:36:03, Alexander Yashkin wrote: > On 2017/02/27 at 15:20:35, vasilii wrote: > > Can here be more options like policy? > > I think |source| can have DefaultSearchManager::FROM_POLICY or > DefaultSearchManager::FROM_FALLBACK values, > yet it still corresponds to TemplateURL::NORMAL engine type. > > Also where is minor bug here with > bool changed = TemplateURL::MatchesData(initial_default_search_provider_.get(), > data, search_terms_data()); > > which must be opposite, IMHO, bool changed = !TemplateURL::MatchesData() > yet I leave its fix to separate CL. Acknowledged.
https://codereview.chromium.org/2639153002/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/api/settings_overrides/settings_overrides_browsertest.cc (right): https://codereview.chromium.org/2639153002/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/settings_overrides/settings_overrides_browsertest.cc:212: EXPECT_TRUE(VerifyTemplateURLServiceLoad(url_service)); On 2017/02/28 14:59:41, vasilii wrote: > On 2017/02/27 19:36:03, Alexander Yashkin wrote: > > On 2017/02/27 at 15:20:35, vasilii wrote: > > > What's going on here? Why you load the service in this test? > > > > I decided to ensure that TemplateURLService is loaded first before setting it > > explicitly to unloaded state. > > This is browser test so TemplateURLService for profile could be loaded or not > > loaded at the moment this test start to execute. > > It depends on the other tasks performed before current test, if they call > > TemplateURLServiceFactory::GetForProfile and DB task will execute > > before current task, TemplateURLService could be already loaded at the moment > > BeforeTemplateUrlServiceLoad is executed. > > > > I could be wrong, so please correct me if so. > > It's a browser test and what happens here should match the real behavior. There > is no way TemplateUrlService may handle the results from the database and switch > the flag back. Removed explicit TemplateURLService load and set_loaded(false). https://codereview.chromium.org/2639153002/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/settings_overrides/settings_overrides_browsertest.cc:214: url_service->set_loaded(false); On 2017/02/28 14:59:42, vasilii wrote: > On 2017/02/27 19:36:03, Alexander Yashkin wrote: > > On 2017/02/27 at 15:20:35, vasilii wrote: > > > I don't think it's applicable here in the browser test as all the keywords > > were already loaded. > > > > Why not applicable? I think set_loaded() function is very useful in this > class, > > no matter if keywords were loaded or not. > > Just nearly every function in TemplateURLService has check for loaded_ flag > and > > acts differently depending on its value. > > > > This test could catch the source of flakiness that we spend a lot of time > > tracing in current CL earlier. > > It checks that if extension is loaded/unloaded while TemplateURLService is in > > not-loaded state, > > extension controlled default search engine is set and removed correctly. > > TemplateURLService::Load is called explicitly. If you say that it can be either > loaded or not here then which component is calling the method? > If it's critical for you that TemplateURLServiceLoad isn't loaded then it should > be done either by suppressing Load() or delaying the result delivery. > For me it's not that critical as the test should pass either way. Removed TemplateURLService load.
https://codereview.chromium.org/2639153002/diff/100001/components/search_engi... File components/search_engines/default_search_manager.cc (right): https://codereview.chromium.org/2639153002/diff/100001/components/search_engi... components/search_engines/default_search_manager.cc:263: MergePrefsDataWithPrepopulated(); On 2017/02/28 11:59:56, Alexander Yashkin wrote: > What I have thought of - where is possibility that rarely, after browser update, > default search engine installed by extension will be reset to nullptr. > This could happen if prepopulated engines change with browser update but > extension engine fields were initialized and written to prefs while previous > prepopulated engines version was active. Could you spell this out more? The scenario is that the user loads an extension which specifies an engine with a prepopulate_id, and we populate its fields from that data (and set this engine as default? Not sure). Then on the next startup, the browser has been updated with new prepopulated data, and... something about loading with |initial_default_search_provider_| not set to values that match the ones in the extension. But I'm not really sure precisely how that happens? Can you walk me through the call chains in more detail here?
https://codereview.chromium.org/2639153002/diff/100001/components/search_engi... File components/search_engines/default_search_manager.cc (right): https://codereview.chromium.org/2639153002/diff/100001/components/search_engi... components/search_engines/default_search_manager.cc:263: MergePrefsDataWithPrepopulated(); On 2017/02/28 23:30:30, Peter Kasting wrote: > On 2017/02/28 11:59:56, Alexander Yashkin wrote: > > What I have thought of - where is possibility that rarely, after browser > update, > > default search engine installed by extension will be reset to nullptr. > > This could happen if prepopulated engines change with browser update but > > extension engine fields were initialized and written to prefs while previous > > prepopulated engines version was active. > > Could you spell this out more? > > The scenario is that the user loads an extension which specifies an engine with > a prepopulate_id, and we populate its fields from that data (and set this engine > as default? Not sure). Then on the next startup, the browser has been updated > with new prepopulated data, and... something about loading with > |initial_default_search_provider_| not set to values that match the ones in the > extension. But I'm not really sure precisely how that happens? Can you walk me > through the call chains in more detail here? I think I was wrong. This was my a theory: 1. Extension with default search is installed with prepopulated_id in manifest = 1. From SettingsOverridesAPI::RegisterSearchProvider it will write to extensions preferences TemplateURLData with some of fields initialized with values from prepopulated engine with id =1. For example search_url = "www.google.com/1". 2. Browser is updated and new version contains changed fields for prepopulated engine with id = 1. search_url = "www.google.com/2" 3. On updated browser start DefaultSearchManager::LoadDefaultSearchEngineFromPrefs loads from prefs TemplateURLData with previous values of extension engine, especially search_url, and set it to extension_default_search_ in DefaultSearchManager and initial_default_search_provider_ in TemplateURLService. 4. Extension is loaded and performs initialization in SettingsOverridesAPI::RegisterSearchProvider. It will load new fields for prepopulated engine with id =1. It will add AddExtensionControlledTURL with new value of search_url to TemplateURLService and write new TemplateURLData to prefs. 5. When turning to loaded state TemplateURLService will fail in ApplyDefaultSearchChangeNoMetrics when FindMatchingDefaultExtensionTemplateURL will fail to match initial_default_search_provider_ TemplateURLData with old value of search_url to extension TemplateURL with new search_url. Now I think that in step 4 when extension will write to prefs its new TemplateURLData, pref observer in DefaultSearchManager will reread new value and update extension_default_search_ and initial_default_search_provider_ in TemplateURLService to valid values. So no worry, false alarm :).
OK. I'm satisfied on my other concern, BTW; the function didn't do what I thought it did. Still LGTM.
Description was changed from ========== Make extensions DSE persistent in browser prefs (Reland) This fix stores extension installed DSE in browser prefs using extension overriden preferences API. This is needed so extension installed default search would be available from browser before extensions subsystem load. This will affect url for first loaded NTP which is taken from current default search settings. BUG=450534 R=pkasting@chromium.org, vasilii@chromium.org Review-Url: https://codereview.chromium.org/2479113002 Cr-Commit-Position: refs/heads/master@{#442235} ========== to ========== Make extensions DSE persistent in browser prefs (Reland) This fix stores extension installed DSE in browser prefs using extension overriden preferences API. This is needed so extension installed default search would be available from browser before extensions subsystem load. This will affect url for first loaded NTP which is taken from current default search settings. BUG=450534 R=pkasting@chromium.org, vasilii@chromium.org Review-Url: https://codereview.chromium.org/2479113002 Cr-Commit-Position: refs/heads/master@{#442235} ==========
a-v-y@yandex-team.ru changed reviewers: + gab@chromium.org
@gab, PTAL at session_startup_pref.h
lgtm https://codereview.chromium.org/2639153002/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/api/settings_overrides/settings_overrides_browsertest.cc (right): https://codereview.chromium.org/2639153002/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/settings_overrides/settings_overrides_browsertest.cc:200: UninstallExtension( I think it's useless
https://codereview.chromium.org/2639153002/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/api/settings_overrides/settings_overrides_browsertest.cc (right): https://codereview.chromium.org/2639153002/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/settings_overrides/settings_overrides_browsertest.cc:200: UninstallExtension( On 2017/03/01 09:22:25, vasilii wrote: > I think it's useless I would rather leave it to ensure no DCHECKs will be fired in TemplateURLSerivce on extension unload.
On 2017/03/01 10:59:32, Alexander Yashkin wrote: > https://codereview.chromium.org/2639153002/diff/140001/chrome/browser/extensi... > File > chrome/browser/extensions/api/settings_overrides/settings_overrides_browsertest.cc > (right): > > https://codereview.chromium.org/2639153002/diff/140001/chrome/browser/extensi... > chrome/browser/extensions/api/settings_overrides/settings_overrides_browsertest.cc:200: > UninstallExtension( > On 2017/03/01 09:22:25, vasilii wrote: > > I think it's useless > > I would rather leave it to ensure no DCHECKs will be fired in TemplateURLSerivce > on extension unload. OK, then put a comment.
https://codereview.chromium.org/2639153002/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/api/settings_overrides/settings_overrides_browsertest.cc (right): https://codereview.chromium.org/2639153002/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/settings_overrides/settings_overrides_browsertest.cc:200: UninstallExtension( On 2017/03/01 10:59:32, Alexander Yashkin wrote: > On 2017/03/01 09:22:25, vasilii wrote: > > I think it's useless > > I would rather leave it to ensure no DCHECKs will be fired in TemplateURLSerivce > on extension unload. Decided to remove it. Removed.
a-v-y@yandex-team.ru changed reviewers: + bauerb@chromium.org
@bauerb, PTAL at session_startup_pref.h, gab is busy.
prefs/ LGTM
The CQ bit was checked by a-v-y@yandex-team.ru
The patchset sent to the CQ was uploaded after l-g-t-m from vasilii@chromium.org, pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2639153002/#ps160001 (title: "Minor fix after review, round 7")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1488383369400800,
"parent_rev": "c7172cd2851a304d7c831cb23b3a7b3e3134e9d1", "commit_rev":
"858416e8845ab1289879133ca455fe86473e6ec2"}
Message was sent while issue was closed.
Description was changed from ========== Make extensions DSE persistent in browser prefs (Reland) This fix stores extension installed DSE in browser prefs using extension overriden preferences API. This is needed so extension installed default search would be available from browser before extensions subsystem load. This will affect url for first loaded NTP which is taken from current default search settings. BUG=450534 R=pkasting@chromium.org, vasilii@chromium.org Review-Url: https://codereview.chromium.org/2479113002 Cr-Commit-Position: refs/heads/master@{#442235} ========== to ========== Make extensions DSE persistent in browser prefs (Reland) This fix stores extension installed DSE in browser prefs using extension overriden preferences API. This is needed so extension installed default search would be available from browser before extensions subsystem load. This will affect url for first loaded NTP which is taken from current default search settings. BUG=450534 R=pkasting@chromium.org, vasilii@chromium.org Review-Url: https://codereview.chromium.org/2479113002 Cr-Original-Commit-Position: refs/heads/master@{#442235} Review-Url: https://codereview.chromium.org/2639153002 Cr-Commit-Position: refs/heads/master@{#453949} Committed: https://chromium.googlesource.com/chromium/src/+/858416e8845ab1289879133ca455... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/chromium/src/+/858416e8845ab1289879133ca455... |
