Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2479113002:
Make extensions DSE persistent in browser prefs (Closed)
|
Created:
4 years, 1 month ago by Alexander Yashkin Modified:
3 years, 11 months ago CC:
Bernhard Bauer, 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
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}
Committed: https://chromium.googlesource.com/chromium/src/+/f5f1407adee2b18b59b33fb8d74739dcc27d8c0b
Patch Set 1 #
Total comments: 4
Patch Set 2 : Move extension overriden DSE logic to setting_override_api.cc #
Total comments: 14
Patch Set 3 : Added DSE tests #
Total comments: 43
Patch Set 4 : Tests updated(rewritten) after review #
Total comments: 61
Patch Set 5 : Updated after review, round 2 #
Total comments: 2
Patch Set 6 : Updated after review, round 3 #Patch Set 7 : Fixed after rebase on master #
Total comments: 6
Patch Set 8 : Updated after review, round 4 #
Total comments: 6
Patch Set 9 : Updated after review, round 5 #
Total comments: 2
Patch Set 10 : Updated after review, round 6 #
Total comments: 56
Patch Set 11 : Fixed after review, round 7 #
Total comments: 60
Patch Set 12 : Fixed after review, round 8 #Patch Set 13 : Fixed linux compilation #Patch Set 14 : Fixed tests compilation #Patch Set 15 : Fixed tests compilation #Patch Set 16 : Fixed default extension keywords conflicts problem #Messages
Total messages: 99 (36 generated)
Peter and Vasilii, in this fix I am trying to fix problem with extension installed DSE being ignored for first browser NTP tab. I am using SetExtensionControlledPref to override kDefaultSearchProviderDataPrefName preference. I am currently working on unittests for this solution. I wanted you to look and approve or disapprove such approach as a whole. Also your advice for testing would be helpful. Thanks.
Does it really work? You set the extension pref exactly the same time TemplateURLService::AddExtensionControlledTURL was called. That is, after TemplateURLService is loaded. The problem occurs before TemplateURLService is loaded because it reads the normal prefs. - the code in components isn't supposed to know about extensions. That is a layering violation. You may still see "extensions" all over TemplateURLService. But that is because it was implemented before componentization effort. May be you could overwrite the prefs from settings_overrides_api.cc using the utility methods? - One thing to check. The extension prefs should not be synced to another machine. Rather the underlying value hidden by the extensions. Did you check it? https://codereview.chromium.org/2479113002/diff/1/chrome/browser/search_engin... File chrome/browser/search_engines/chrome_template_url_service_client.h (right): https://codereview.chromium.org/2479113002/diff/1/chrome/browser/search_engin... chrome/browser/search_engines/chrome_template_url_service_client.h:30: explicit ChromeTemplateURLServiceClient( no explicit. https://codereview.chromium.org/2479113002/diff/1/chrome/browser/search_engin... chrome/browser/search_engines/chrome_template_url_service_client.h:53: const std::string extension_id, const std::string pref_name, const std::string& https://codereview.chromium.org/2479113002/diff/1/components/search_engines/d... File components/search_engines/default_search_manager.cc (right): https://codereview.chromium.org/2479113002/diff/1/components/search_engines/d... components/search_engines/default_search_manager.cc:183: if (source != FROM_FALLBACK) Why did the conditions here change?
On 2016/11/07 at 15:00:05, vasilii wrote: > Does it really work? You set the extension pref exactly the same time TemplateURLService::AddExtensionControlledTURL was called. That is, after TemplateURLService is loaded. The problem occurs before TemplateURLService is loaded because it reads the normal prefs. I checked on mac and it worked. Tomorrow I will check on win machine too. My current understanding of how SetExtensionControlledPref works is following - it stores extension overriden prefs in special section for each extension that overrides preferences. In my computer, after I call SetExtensionControlledPref I see section 'preferences' under key "laddjijkcfpakbbnnedbhnnciecidncp" that coresponds to Yandex extension in Preferences file. This settings are taken in account by preference system until I call RemoveExtensionControlledPref. In my CL I call RemoveExtensionControlledPref in SettingsOverridesAPI::OnExtensionUnloaded which is called when extension is disabled or removed. So after the first call to SetExtensionControlledPref - default search preference is overriden and is used until extension is disabled/removed. After browser restart - extension overriden value for active extensions is loaded from preferences even before extension subsystem is loaded. I could be wrong in my understanding, so please correct me if its so. > - the code in components isn't supposed to know about extensions. That is a layering violation. You may still see "extensions" all over TemplateURLService. But that is because it was implemented before componentization effort. May be you could overwrite the prefs from settings_overrides_api.cc using the utility methods? I just found and read more info here https://www.chromium.org/developers/design-documents/preferences#TOC-Extensio... and I see that extension pref system already takes in consideration install_time of extension, so I can simply call SetExtensionControlledPref for every extension that overrides DSE. In next CL iteration, I'll try to override DSE pref them from settings_overrides_api.cc not from TemplateUrlService. > - One thing to check. The extension prefs should not be synced to another machine. Rather the underlying value hidden by the extensions. Did you check it? I thought about interaction with sync subsystem, but not checked it yet. I'll see if I can write unittest for such scenario. I will update and submit next iteration tomorrow. Thanks for review. > > https://codereview.chromium.org/2479113002/diff/1/chrome/browser/search_engin... > File chrome/browser/search_engines/chrome_template_url_service_client.h (right): > > https://codereview.chromium.org/2479113002/diff/1/chrome/browser/search_engin... > chrome/browser/search_engines/chrome_template_url_service_client.h:30: explicit ChromeTemplateURLServiceClient( > no explicit. > > https://codereview.chromium.org/2479113002/diff/1/chrome/browser/search_engin... > chrome/browser/search_engines/chrome_template_url_service_client.h:53: const std::string extension_id, const std::string pref_name, > const std::string& > > https://codereview.chromium.org/2479113002/diff/1/components/search_engines/d... > File components/search_engines/default_search_manager.cc (right): > > https://codereview.chromium.org/2479113002/diff/1/components/search_engines/d... > components/search_engines/default_search_manager.cc:183: if (source != FROM_FALLBACK) > Why did the conditions here change?
https://codereview.chromium.org/2479113002/diff/1/components/search_engines/d... File components/search_engines/default_search_manager.cc (right): https://codereview.chromium.org/2479113002/diff/1/components/search_engines/d... components/search_engines/default_search_manager.cc:183: if (source != FROM_FALLBACK) On 2016/11/07 at 15:00:05, vasilii wrote: > Why did the conditions here change? Previous check was to notify DefaultSearchManager observers on pref change if old or new DSE source depended from pref value. This were user selected and policy managed DSEs. Now with extension DSE also becoming dependent from this pref value, the only case where observers must NOT be called is when source is fallback DSE.
I committed second patch. In it I removed selection of extension DSE from template_url_service.cc and moved SetExtensionControlledPref to settings_override_api.cc where it is called on extension load. After preference value changes DefaultSearchManager will reload and update default search in OnDefaultSearchPrefChanged observer. It will also notify TemplateUrlService on DSE change. I checked on win machine and this solution works where too. I move to unittests.
I'll do an OWNER review for components/search_engines once you two have something you're sure you're happy with. Note that if you're moving a significant block of code from one file to another and making any changes to it, it might be good to do the move (with no changes) as a separate CL that this one can depend on, so the diff here is just the functional changes.
The approach looks good to me. There is a pending question about sync. I'd also like to see a browser test (see settings_overrides_browsertest.cc ) for the change. I think it's doable. https://codereview.chromium.org/2479113002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/settings_overrides/settings_overrides_api.h (right): https://codereview.chromium.org/2479113002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/settings_overrides/settings_overrides_api.h:41: base::Value* value) const; It would be nice to accept a unique_ptr here. https://codereview.chromium.org/2479113002/diff/20001/components/search_engin... File components/search_engines/default_search_manager_unittest.cc (right): https://codereview.chromium.org/2479113002/diff/20001/components/search_engin... components/search_engines/default_search_manager_unittest.cc:109: } Why are they empty? https://codereview.chromium.org/2479113002/diff/20001/components/search_engin... components/search_engines/default_search_manager_unittest.cc:305: // manager.SetExtensionControlledDefaultSearchEngine(*extension_data_1); Looks like you wanted to change something in this file but forgot. https://codereview.chromium.org/2479113002/diff/20001/components/search_engin... File components/search_engines/template_url_service_client.h (right): https://codereview.chromium.org/2479113002/diff/20001/components/search_engin... components/search_engines/template_url_service_client.h:17: } Not needed. https://codereview.chromium.org/2479113002/diff/20001/components/search_engin... File components/search_engines/util.cc (right): https://codereview.chromium.org/2479113002/diff/20001/components/search_engin... components/search_engines/util.cc:420: &result->suggestions_url_post_params); align parameters. https://codereview.chromium.org/2479113002/diff/20001/components/search_engin... components/search_engines/util.cc:450: result->alternate_urls.push_back(alternate_url); std::move(alternate_url) https://codereview.chromium.org/2479113002/diff/20001/components/search_engin... components/search_engines/util.cc:459: result->input_encodings.push_back(encoding); std::move(encoding) https://codereview.chromium.org/2479113002/diff/20001/components/search_engin... components/search_engines/util.cc:468: result->show_in_default_list = true; Why true here? https://codereview.chromium.org/2479113002/diff/20001/components/search_engin... components/search_engines/util.cc:516: it != data.alternate_urls.end(); ++it) { for (const auto& ...) seems shorter. Same below. https://codereview.chromium.org/2479113002/diff/20001/components/search_engin... components/search_engines/util.cc:519: url_dict->Set(DefaultSearchManager::kAlternateURLs, alternate_urls.release()); It's a deprecated method. Use std::move(alternate_urls); Same below.
On 2016/11/08 at 19:51:02, pkasting wrote: > I'll do an OWNER review for components/search_engines once you two have something you're sure you're happy with. > > Note that if you're moving a significant block of code from one file to another and making any changes to it, it might be good to do the move (with no changes) as a separate CL that this one can depend on, so the diff here is just the functional changes. I moved refactoring to separate CL https://codereview.chromium.org/2497853002. When it will be commited, I will return to current CL.
I have written some tests for changed behavior, added browsertest
default_search_manager_browsertest.cc
and also modified existing settings_overrides_browsertest.cc
I am not sure that api overrides directory is best place for new test, yet the
new test is very dependent on extension settings api.
I took a look at sync code in template_url_service.cc and see that currently
extension controlled search engines are not synced between devices.
This check is performed in ProcessTemplateURLChange:
// Avoid syncing extension-controlled search engines.
if (turl->type() == TemplateURL::NORMAL_CONTROLLED_BY_EXTENSION)
return;
Even if new default search would arrive with sync it will only override user
level pref, not extension controlled one.
This is done in OnSyncedDefaultSearchProviderGUIDChanged
if (turl)
default_search_manager_.SetUserSelectedDefaultSearchEngine(turl->data());
Yet, I would feel more safe if someone more experienced in sync field could take
a look at changes.
PTAL at changes, feel free to comment.
https://codereview.chromium.org/2479113002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/settings_overrides/settings_overrides_api.h
(right):
https://codereview.chromium.org/2479113002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/settings_overrides/settings_overrides_api.h:41:
base::Value* value) const;
On 2016/11/09 at 14:29:56, vasilii wrote:
> It would be nice to accept a unique_ptr here.
Done
https://codereview.chromium.org/2479113002/diff/20001/components/search_engin...
File components/search_engines/default_search_manager_unittest.cc (right):
https://codereview.chromium.org/2479113002/diff/20001/components/search_engin...
components/search_engines/default_search_manager_unittest.cc:109: }
On 2016/11/09 at 14:29:56, vasilii wrote:
> Why are they empty?
I was thinking at first to modify this unittest to check how
DefaultSearchManager works with extension overriden prefs.
But after looking at code and existing extensions tests I decided to add browser
test that installs/remove extension and
checks DefaultSearchManager interaction with extension DSE.
https://codereview.chromium.org/2479113002/diff/20001/components/search_engin...
components/search_engines/default_search_manager_unittest.cc:305: //
manager.SetExtensionControlledDefaultSearchEngine(*extension_data_1);
On 2016/11/09 at 14:29:56, vasilii wrote:
> Looks like you wanted to change something in this file but forgot.
Deleted, see comment above.
https://codereview.chromium.org/2479113002/diff/20001/components/search_engin...
File components/search_engines/template_url_service_client.h (right):
https://codereview.chromium.org/2479113002/diff/20001/components/search_engin...
components/search_engines/template_url_service_client.h:17: }
On 2016/11/09 at 14:29:56, vasilii wrote:
> Not needed.
Deleted.
On 2016/11/28 19:16:17, Alexander Yashkin wrote: > I took a look at sync code in template_url_service.cc and see that currently > extension controlled search engines are not synced between devices. > This check is performed in ProcessTemplateURLChange: > // Avoid syncing extension-controlled search engines. > if (turl->type() == TemplateURL::NORMAL_CONTROLLED_BY_EXTENSION) > return; This is because if the user has extensions synced, these will come in via the synced extensions, which will then locally add/override search engines as necessary. And if the user doesn't have extensions synced, then we shouldn't sync these elsewhere, as they're really an implementation detail of the extensions in question. > Yet, I would feel more safe if someone more experienced in sync field could take > a look at changes. I'm not terribly experienced here. If you want expertise you could try asking want of the sync/ OWNERS?
a-v-y@yandex-team.ru changed reviewers: + maxbogue@chromium.org
On 2016/11/28 at 19:43:00, pkasting wrote: > On 2016/11/28 19:16:17, Alexander Yashkin wrote: > > I took a look at sync code in template_url_service.cc and see that currently > > extension controlled search engines are not synced between devices. > > This check is performed in ProcessTemplateURLChange: > > // Avoid syncing extension-controlled search engines. > > if (turl->type() == TemplateURL::NORMAL_CONTROLLED_BY_EXTENSION) > > return; > > This is because if the user has extensions synced, these will come in via the synced extensions, which will then locally add/override search engines as necessary. > > And if the user doesn't have extensions synced, then we shouldn't sync these elsewhere, as they're really an implementation detail of the extensions in question. > > > Yet, I would feel more safe if someone more experienced in sync field could take > > a look at changes. > > I'm not terribly experienced here. If you want expertise you could try asking want of the sync/ OWNERS? Added maxbogue, PTAL to check that sync of search_engines does not break.
Regarding sync. There is template_url_service_sync_unittest.cc. You can add a test there which would: - overwrite DSE via the extension prefs. - start syncing. - check that those values didn't propagate to the server. https://codereview.chromium.org/2479113002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/settings_overrides/default_search_manager_browsertest.cc (right): https://codereview.chromium.org/2479113002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/settings_overrides/default_search_manager_browsertest.cc:85: base::RunLoop().RunUntilIdle(); RunUntilIdle() is dangerous in the browser tests because it can never be idle. You should wait for some specific event instead. https://codereview.chromium.org/2479113002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/settings_overrides/default_search_manager_browsertest.cc:92: IN_PROC_BROWSER_TEST_F(DefaultSearchManagerBrowserTest, ExtensionsOverrides) { I don't see why you need a separate file for this. Move it to the other browser tests or unit tests (preferred option). https://codereview.chromium.org/2479113002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/settings_overrides/settings_overrides_api.cc (right): https://codereview.chromium.org/2479113002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/settings_overrides/settings_overrides_api.cc:169: // symbol SessionStartupPref::kPrefValueURLs link error on mac. I don't understand it. Now everything compiles fine. What is the error and exact code? https://codereview.chromium.org/2479113002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/settings_overrides/settings_overrides_browsertest.cc (right): https://codereview.chromium.org/2479113002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/settings_overrides/settings_overrides_browsertest.cc:44: TemplateURLPrepopulateData::GetPrepopulatedEngine(prefs, 1)); prepopulated_id should come from the manifest instead of hardcoding '1'. https://codereview.chromium.org/2479113002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/settings_overrides/settings_overrides_browsertest.cc:49: result->contextual_search_url = google_engine->contextual_search_url; contextual_search_url and search_terms_replacement_key should be rather overwritten in the manifest itself. https://codereview.chromium.org/2479113002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/settings_overrides/settings_overrides_browsertest.cc:63: } // namespace Why do you move the namespace? https://codereview.chromium.org/2479113002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/settings_overrides/settings_overrides_browsertest.cc:65: IN_PROC_BROWSER_TEST_F(ExtensionBrowserTest, OverrideHomePageSettings) { If you decided to split the tests then one should overwrite only home page and the other one only DSE. Presumably, testing only the start-up pages is useful too. https://codereview.chromium.org/2479113002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/settings_overrides/settings_overrides_browsertest.cc:76: // On Mac, this API is limited to trunk. I think it's not true anymore. https://codereview.chromium.org/2479113002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/settings_overrides/settings_overrides_browsertest.cc:158: IN_PROC_BROWSER_TEST_F(ExtensionBrowserTest, PRE_OverridenDSEPersists) { The test should load an extension that overwrites only the DSE. Then you have to check the new tab page. That's what the bug is about. https://codereview.chromium.org/2479113002/diff/40001/components/search_engin... File components/search_engines/default_search_manager_unittest.cc (left): https://codereview.chromium.org/2479113002/diff/40001/components/search_engin... components/search_engines/default_search_manager_unittest.cc:260: TEST_F(DefaultSearchManagerTest, DefaultSearchSetByExtension) { I think the test is still useful. Why do you remove it? https://codereview.chromium.org/2479113002/diff/40001/components/search_engin... File components/search_engines/search_engines_test_util.cc (right): https://codereview.chromium.org/2479113002/diff/40001/components/search_engin... components/search_engines/search_engines_test_util.cc:18: // autogenerated from type param. No need to copy/paste the comment. https://codereview.chromium.org/2479113002/diff/40001/components/search_engin... components/search_engines/search_engines_test_util.cc:22: data->SetShortName(base::UTF8ToUTF16(std::string(type).append("name"))); type + "name"? Here and below. https://codereview.chromium.org/2479113002/diff/40001/components/search_engin... components/search_engines/search_engines_test_util.cc:42: ASSERT_TRUE(actual != NULL); Would it be easier to take const TemplateURLData& ? https://codereview.chromium.org/2479113002/diff/40001/components/search_engin... components/search_engines/search_engines_test_util.cc:68: using namespace policy; The style guide doesn't allow "using namespace". "using policy::PolicyLevel" is OK. https://codereview.chromium.org/2479113002/diff/40001/components/search_engin... File components/search_engines/search_engines_test_util.h (right): https://codereview.chromium.org/2479113002/diff/40001/components/search_engin... components/search_engines/search_engines_test_util.h:19: const std::string& type); Not clear what |type| is. From the implementation it should be something like "google.com/" I think you need to tweak the implementation so that every decent string "mydomain" would result in a valid return TemplateURLData. https://codereview.chromium.org/2479113002/diff/40001/components/search_engin... components/search_engines/search_engines_test_util.h:26: // Build default dearch policy for TemplateUrlData object. s/dearch/search https://codereview.chromium.org/2479113002/diff/40001/components/search_engin... components/search_engines/search_engines_test_util.h:28: policy::PolicyMap* policy); In what files do you use those functions? If it's just one then I'd move the implementation there. https://codereview.chromium.org/2479113002/diff/40001/components/search_engin... File components/search_engines/template_url.cc (right): https://codereview.chromium.org/2479113002/diff/40001/components/search_engin... components/search_engines/template_url.cc:1265: // its special initialization in TemplateUrl constructor. Move the comment to the header. https://codereview.chromium.org/2479113002/diff/40001/components/search_engin... File components/search_engines/template_url.h (right): https://codereview.chromium.org/2479113002/diff/40001/components/search_engin... components/search_engines/template_url.h:538: static bool SearchTermsReplacementKeysMatch( A comment?
I've looked at this a little but I'm also not familiar with the search engines code. I'll look again once the current comments are addressed.
PTAL at another round of changes, addressed most of comments. I had to add extension prefs storage support to TestingPrefService. After several attempts to use real extension prefs in unittests I found that most simple and convenient way is to add support for them to TestingPrefService like for managed one. https://codereview.chromium.org/2479113002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/settings_overrides/default_search_manager_browsertest.cc (right): https://codereview.chromium.org/2479113002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/settings_overrides/default_search_manager_browsertest.cc:85: base::RunLoop().RunUntilIdle(); On 2016/11/30 at 14:00:02, vasilii wrote: > RunUntilIdle() is dangerous in the browser tests because it can never be idle. You should wait for some specific event instead. Removed browser test, returned unittest. https://codereview.chromium.org/2479113002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/settings_overrides/default_search_manager_browsertest.cc:92: IN_PROC_BROWSER_TEST_F(DefaultSearchManagerBrowserTest, ExtensionsOverrides) { On 2016/11/30 at 14:00:02, vasilii wrote: > I don't see why you need a separate file for this. Move it to the other browser tests or unit tests (preferred option). Removed this browser test. Returned to default_search_manager_unittest.cc, although I had to modify TestingPrefService and TestingPrefServiceSyncable to add primitive extension pref storage support in tests. https://codereview.chromium.org/2479113002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/settings_overrides/settings_overrides_api.cc (right): https://codereview.chromium.org/2479113002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/settings_overrides/settings_overrides_api.cc:169: // symbol SessionStartupPref::kPrefValueURLs link error on mac. On 2016/11/30 at 14:00:02, vasilii wrote: > I don't understand it. Now everything compiles fine. What is the error and exact code? It was a problem with ODR rule applying for SessionStartupPref::kPrefValueURLs with usage in base::MakeUnique<base::FundamentalValue>(SessionStartupPref::kPrefValueURLs) SessionStartupPref::kPrefValueURLs variable need to be defined and its currently is only declared in chrome/browser/prefs/session_startup_pref.h. I added definition to session_startup_pref.cc https://codereview.chromium.org/2479113002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/settings_overrides/settings_overrides_browsertest.cc (right): https://codereview.chromium.org/2479113002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/settings_overrides/settings_overrides_browsertest.cc:44: TemplateURLPrepopulateData::GetPrepopulatedEngine(prefs, 1)); On 2016/11/30 at 14:00:02, vasilii wrote: > prepopulated_id should come from the manifest instead of hardcoding '1'. Added extraction of prepopulated_id from manifest. https://codereview.chromium.org/2479113002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/settings_overrides/settings_overrides_browsertest.cc:49: result->contextual_search_url = google_engine->contextual_search_url; On 2016/11/30 at 14:00:02, vasilii wrote: > contextual_search_url and search_terms_replacement_key should be rather overwritten in the manifest itself. I don't think that someone would want to override contextual_search_url and search_terms_replacement_key. They are rather exotic, IMHO, and are used only by Google engine AFAIK. I can add new_tab_url extraction to manifest parsing. new_tab_url is currently used by Google, Yandex and Bing search engines. Do you think it should be done in this CL? https://codereview.chromium.org/2479113002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/settings_overrides/settings_overrides_browsertest.cc:63: } // namespace On 2016/11/30 at 14:00:02, vasilii wrote: > Why do you move the namespace? Reverted, did not even noticed in hacking rage :) https://codereview.chromium.org/2479113002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/settings_overrides/settings_overrides_browsertest.cc:65: IN_PROC_BROWSER_TEST_F(ExtensionBrowserTest, OverrideHomePageSettings) { On 2016/11/30 at 14:00:02, vasilii wrote: > If you decided to split the tests then one should overwrite only home page and the other one only DSE. Presumably, testing only the start-up pages is useful too. Ok, splitted home page and startup pages tests. https://codereview.chromium.org/2479113002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/settings_overrides/settings_overrides_browsertest.cc:76: // On Mac, this API is limited to trunk. On 2016/11/30 at 14:00:02, vasilii wrote: > I think it's not true anymore. Deleted. https://codereview.chromium.org/2479113002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/settings_overrides/settings_overrides_browsertest.cc:158: IN_PROC_BROWSER_TEST_F(ExtensionBrowserTest, PRE_OverridenDSEPersists) { On 2016/11/30 at 14:00:02, vasilii wrote: > The test should load an extension that overwrites only the DSE. Then you have to check the new tab page. That's what the bug is about. new_tab page from extension overriden engine not applying to first tab is only one of the consequences of bigger problem, IMHO. The bigger problem is that extension overriden default search engine is not applied at all, until extension subsystem is loaded. Added new test extension that overrides DSE only and changed test to use it. Not sure that new extension is needed. Whats wrong with one extension used by both tests? https://codereview.chromium.org/2479113002/diff/40001/components/search_engin... File components/search_engines/default_search_manager_unittest.cc (left): https://codereview.chromium.org/2479113002/diff/40001/components/search_engin... components/search_engines/default_search_manager_unittest.cc:260: TEST_F(DefaultSearchManagerTest, DefaultSearchSetByExtension) { On 2016/11/30 at 14:00:03, vasilii wrote: > I think the test is still useful. Why do you remove it? Restored and made it work. https://codereview.chromium.org/2479113002/diff/40001/components/search_engin... File components/search_engines/search_engines_test_util.cc (right): https://codereview.chromium.org/2479113002/diff/40001/components/search_engin... components/search_engines/search_engines_test_util.cc:18: // autogenerated from type param. On 2016/11/30 at 14:00:03, vasilii wrote: > No need to copy/paste the comment. Deleted https://codereview.chromium.org/2479113002/diff/40001/components/search_engin... components/search_engines/search_engines_test_util.cc:22: data->SetShortName(base::UTF8ToUTF16(std::string(type).append("name"))); On 2016/11/30 at 14:00:03, vasilii wrote: > type + "name"? > > Here and below. I was sure append was preferrable in code style. Could not find the prooflink, though. Maybe Peter, pkasting, could help? https://codereview.chromium.org/2479113002/diff/40001/components/search_engin... components/search_engines/search_engines_test_util.cc:42: ASSERT_TRUE(actual != NULL); On 2016/11/30 at 14:00:03, vasilii wrote: > Would it be easier to take const TemplateURLData& ? That would mean to delete ASSERT_TRUE on input pointers and many test places will simply fail on nullptr access. https://codereview.chromium.org/2479113002/diff/40001/components/search_engin... components/search_engines/search_engines_test_util.cc:68: using namespace policy; On 2016/11/30 at 14:00:03, vasilii wrote: > The style guide doesn't allow "using namespace". "using policy::PolicyLevel" is OK. Removed this function, its not used after I switched from browsertests to unittests. https://codereview.chromium.org/2479113002/diff/40001/components/search_engin... File components/search_engines/search_engines_test_util.h (right): https://codereview.chromium.org/2479113002/diff/40001/components/search_engin... components/search_engines/search_engines_test_util.h:19: const std::string& type); On 2016/11/30 at 14:00:03, vasilii wrote: > Not clear what |type| is. From the implementation it should be something like "google.com/" > I think you need to tweak the implementation so that every decent string "mydomain" would result in a valid return TemplateURLData. Changed type to provider_name. I think parameter is used more like name than domain. What do you propose to tweak? I don't see errors in its implementation. IMHO, if this function returned non valid TemplateURLData, tests using it would fail. https://codereview.chromium.org/2479113002/diff/40001/components/search_engin... components/search_engines/search_engines_test_util.h:26: // Build default dearch policy for TemplateUrlData object. On 2016/11/30 at 14:00:03, vasilii wrote: > s/dearch/search Fixed https://codereview.chromium.org/2479113002/diff/40001/components/search_engin... components/search_engines/search_engines_test_util.h:28: policy::PolicyMap* policy); On 2016/11/30 at 14:00:03, vasilii wrote: > In what files do you use those functions? If it's just one then I'd move the implementation there. Deleted this function. https://codereview.chromium.org/2479113002/diff/40001/components/search_engin... File components/search_engines/template_url.cc (right): https://codereview.chromium.org/2479113002/diff/40001/components/search_engin... components/search_engines/template_url.cc:1265: // its special initialization in TemplateUrl constructor. On 2016/11/30 at 14:00:03, vasilii wrote: > Move the comment to the header. Done https://codereview.chromium.org/2479113002/diff/40001/components/search_engin... File components/search_engines/template_url.h (right): https://codereview.chromium.org/2479113002/diff/40001/components/search_engin... components/search_engines/template_url.h:538: static bool SearchTermsReplacementKeysMatch( On 2016/11/30 at 14:00:03, vasilii wrote: > A comment? Done
a-v-y@yandex-team.ru changed reviewers: + gab@chromium.org
https://codereview.chromium.org/2479113002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/settings_overrides/settings_overrides_browsertest.cc (right): https://codereview.chromium.org/2479113002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/settings_overrides/settings_overrides_browsertest.cc:49: result->contextual_search_url = google_engine->contextual_search_url; On 2016/12/05 18:16:48, Alexander Yashkin wrote: > On 2016/11/30 at 14:00:02, vasilii wrote: > > contextual_search_url and search_terms_replacement_key should be rather > overwritten in the manifest itself. > > I don't think that someone would want to override contextual_search_url and > search_terms_replacement_key. > They are rather exotic, IMHO, and are used only by Google engine AFAIK. > > I can add new_tab_url extraction to manifest parsing. > new_tab_url is currently used by Google, Yandex and Bing search engines. > Do you think it should be done in this CL? No, that's fine. https://codereview.chromium.org/2479113002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/settings_overrides/settings_overrides_browsertest.cc:158: IN_PROC_BROWSER_TEST_F(ExtensionBrowserTest, PRE_OverridenDSEPersists) { On 2016/12/05 18:16:48, Alexander Yashkin wrote: > On 2016/11/30 at 14:00:02, vasilii wrote: > > The test should load an extension that overwrites only the DSE. Then you have > to check the new tab page. That's what the bug is about. > > new_tab page from extension overriden engine not applying to first tab is only > one of the consequences of bigger problem, IMHO. > The bigger problem is that extension overriden default search engine is not > applied at all, until extension subsystem is loaded. > > Added new test extension that overrides DSE only and changed test to use it. > Not sure that new extension is needed. Whats wrong with one extension used by > both tests? I think you can reuse the extension. You can make the test stronger by using NewTabURLDetails and verifying that it reads the same new tab page that was set by the extension. https://codereview.chromium.org/2479113002/diff/40001/components/search_engin... File components/search_engines/search_engines_test_util.cc (right): https://codereview.chromium.org/2479113002/diff/40001/components/search_engin... components/search_engines/search_engines_test_util.cc:22: data->SetShortName(base::UTF8ToUTF16(std::string(type).append("name"))); On 2016/12/05 18:16:48, Alexander Yashkin wrote: > On 2016/11/30 at 14:00:03, vasilii wrote: > > type + "name"? > > > > Here and below. > > I was sure append was preferrable in code style. Could not find the prooflink, > though. > Maybe Peter, pkasting, could help? It's the same performance but better readabilty. https://codereview.chromium.org/2479113002/diff/40001/components/search_engin... components/search_engines/search_engines_test_util.cc:42: ASSERT_TRUE(actual != NULL); On 2016/12/05 18:16:48, Alexander Yashkin wrote: > On 2016/11/30 at 14:00:03, vasilii wrote: > > Would it be easier to take const TemplateURLData& ? > > That would mean to delete ASSERT_TRUE on input pointers and many test places > will simply fail on nullptr access. I see that you copied an existing function. Then it's fine. https://codereview.chromium.org/2479113002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/settings_overrides/settings_overrides_browsertest.cc (right): https://codereview.chromium.org/2479113002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/settings_overrides/settings_overrides_browsertest.cc:27: // manifest. Mention that it's the particular extension from chrome/test/data/extensions/settings_override/. https://codereview.chromium.org/2479113002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/settings_overrides/settings_overrides_browsertest.cc:152: auto extension_dse = TestExtensionSearchEngine(prefs, prep_id); std::unique_ptr<TemplateURLData> would help readabilty here. Same below. https://codereview.chromium.org/2479113002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/settings_overrides/settings_overrides_browsertest.cc:153: ExpectSimilar(¤t_dse->data(), extension_dse.get()); extension_dse is the expected one. Thus, it's the first parameter. Same below. https://codereview.chromium.org/2479113002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/settings_overrides/settings_overrides_browsertest.cc:208: // test extension. I guess you obtain |prep_id| the same way as above. If it's too complicated then hardcode it in both places like it was before. https://codereview.chromium.org/2479113002/diff/60001/chrome/browser/search_e... File chrome/browser/search_engines/template_url_service_sync_unittest.cc (right): https://codereview.chromium.org/2479113002/diff/60001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_sync_unittest.cc:1712: TEST_F(TemplateURLServiceSyncTest, SyncWithExtensionDefaultSearch) { I meant a different scenario in mind when I asked to write a test. - Sync is active. - An extension overrides the DSE. - Test that the sync model doesn't get this update because we should sync the extensions and not its settings. https://codereview.chromium.org/2479113002/diff/60001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_sync_unittest.cc:1735: extension.search_terms_replacement_key = "espv"; Do you wanna use your helper function here? https://codereview.chromium.org/2479113002/diff/60001/chrome/browser/search_e... File chrome/browser/search_engines/template_url_service_test_util.cc (right): https://codereview.chromium.org/2479113002/diff/60001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_test_util.cc:166: auto dict = TemplateURLDataToDictionary(ext_data); Readabilty would improve with an explicit type.. https://codereview.chromium.org/2479113002/diff/60001/chrome/browser/search_e... File chrome/browser/search_engines/template_url_service_test_util.h (right): https://codereview.chromium.org/2479113002/diff/60001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_test_util.h:70: // Write DSE into extension controlled preference in test profile. Please expand the DSE. https://codereview.chromium.org/2479113002/diff/60001/chrome/browser/search_e... File chrome/browser/search_engines/template_url_service_unittest.cc (right): https://codereview.chromium.org/2479113002/diff/60001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_unittest.cc:1453: // url service, still same extension engine is default. Can you explain why this is true? https://codereview.chromium.org/2479113002/diff/60001/components/pref_registr... File components/pref_registry/testing_pref_service_syncable.h (right): https://codereview.chromium.org/2479113002/diff/60001/components/pref_registr... components/pref_registry/testing_pref_service_syncable.h:20: TestingPrefServiceSyncable(TestingPrefStore* managed_prefs, How is this class different from components/sync_preferences/testing_pref_service_syncable.h? Do you use both? https://codereview.chromium.org/2479113002/diff/60001/components/search_engin... File components/search_engines/search_engines_test_util.cc (right): https://codereview.chromium.org/2479113002/diff/60001/components/search_engin... components/search_engines/search_engines_test_util.cc:9: #include "base/strings/string_split.h" unused? https://codereview.chromium.org/2479113002/diff/60001/components/search_engin... components/search_engines/search_engines_test_util.cc:12: #include "components/policy/policy_constants.h" Obsolete includes? https://codereview.chromium.org/2479113002/diff/60001/components/search_engin... File components/search_engines/template_url.h (right): https://codereview.chromium.org/2479113002/diff/60001/components/search_engin... components/search_engines/template_url.h:621: const AssociatedExtensionInfo* extension_info_for_tests() { Use either ForTesting suffix or #if defined(UNIT_TEST) https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md https://codereview.chromium.org/2479113002/diff/60001/components/search_engin... File components/search_engines/template_url_service.cc (right): https://codereview.chromium.org/2479113002/diff/60001/components/search_engin... components/search_engines/template_url_service.cc:500: ext_dse) { Shouldn't it be if (ext_dse) { DCHECK_EQ(TemplateURL::NORMAL_CONTROLLED_BY_EXTENSION, template_url->type()); } ? https://codereview.chromium.org/2479113002/diff/60001/components/search_engin... components/search_engines/template_url_service.cc:503: DCHECK(ext_dse->data().keyword() == template_url->data().keyword()); Use DCHECK_EQ https://codereview.chromium.org/2479113002/diff/60001/components/search_engin... components/search_engines/template_url_service.cc:1965: std::string ext_id = client_->GetExtensionControllingDSEPref(); I'm not sure about the whole flow. That call makes me suspicions. There are two cases. 1. TemplateURLService isn't loaded yet. This situation is fully handled in template_url_service.cc:1923. 2. TemplateURLService is loaded. TemplateURLService::AddExtensionControlledTURL should add a TemplateURL first. Then you overwrite the preferences, the code finds the correct search provider and makes it default. It shouldn't be possible that we need to add a search provider here. https://codereview.chromium.org/2479113002/diff/60001/components/search_engin... File components/search_engines/template_url_service.h (right): https://codereview.chromium.org/2479113002/diff/60001/components/search_engin... components/search_engines/template_url_service.h:427: friend class TemplateURLServiceTest; What method is the reason for this? https://codereview.chromium.org/2479113002/diff/60001/components/search_engin... File components/search_engines/template_url_service_client.h (right): https://codereview.chromium.org/2479113002/diff/60001/components/search_engin... components/search_engines/template_url_service_client.h:46: virtual std::string GetExtensionControllingDSEPref() = 0; Hopefully, you won't need it. The component should know better the current state. https://codereview.chromium.org/2479113002/diff/60001/components/sync_prefere... File components/sync_preferences/testing_pref_service_syncable.h (right): https://codereview.chromium.org/2479113002/diff/60001/components/sync_prefere... components/sync_preferences/testing_pref_service_syncable.h:25: TestingPrefStore* extension_prefs, I have mixed feelings about this. ExtensionPrefValueMap and ExtensionPrefs are defined in extensions/. The component shouldn't depend on the extensions/ On the other hand, PrefValueStore already knows the extensions perfectly. I'll defer to the owners of the directory.
https://codereview.chromium.org/2479113002/diff/40001/components/search_engin... File components/search_engines/search_engines_test_util.cc (right): https://codereview.chromium.org/2479113002/diff/40001/components/search_engin... components/search_engines/search_engines_test_util.cc:22: data->SetShortName(base::UTF8ToUTF16(std::string(type).append("name"))); On 2016/12/06 19:16:42, vasilii wrote: > On 2016/12/05 18:16:48, Alexander Yashkin wrote: > > On 2016/11/30 at 14:00:03, vasilii wrote: > > > type + "name"? > > > > > > Here and below. > > > > I was sure append was preferrable in code style. Could not find the prooflink, > > though. > > Maybe Peter, pkasting, could help? > > It's the same performance but better readabilty. There's no official preference. Unofficially, I normally prefer "+" for brevity.
gab@chromium.org changed reviewers: + bauerb@chromium.org, sdefresne@chromium.org
+bauerb/sdefrense see below. https://codereview.chromium.org/2479113002/diff/60001/chrome/browser/prefs/se... File chrome/browser/prefs/session_startup_pref.cc (right): https://codereview.chromium.org/2479113002/diff/60001/chrome/browser/prefs/se... chrome/browser/prefs/session_startup_pref.cc:47: const int SessionStartupPref::kPrefValueMax; Why do you need this? They're in the public header already? https://codereview.chromium.org/2479113002/diff/60001/components/pref_registr... File components/pref_registry/testing_pref_service_syncable.h (right): https://codereview.chromium.org/2479113002/diff/60001/components/pref_registr... components/pref_registry/testing_pref_service_syncable.h:20: TestingPrefServiceSyncable(TestingPrefStore* managed_prefs, On 2016/12/06 19:16:42, vasilii wrote: > How is this class different from > components/sync_preferences/testing_pref_service_syncable.h? > Do you use both? I'm also confused, maybe it wasn't properly removed in https://codereview.chromium.org/1305213009? +bauerb/sdefresne? https://codereview.chromium.org/2479113002/diff/60001/components/sync_prefere... File components/sync_preferences/testing_pref_service_syncable.h (right): https://codereview.chromium.org/2479113002/diff/60001/components/sync_prefere... components/sync_preferences/testing_pref_service_syncable.h:25: TestingPrefStore* extension_prefs, On 2016/12/06 19:16:42, vasilii wrote: > I have mixed feelings about this. ExtensionPrefValueMap and ExtensionPrefs are > defined in extensions/. The component shouldn't depend on the extensions/ > On the other hand, PrefValueStore already knows the extensions perfectly. I'll > defer to the owners of the directory. That's a good point. FWIW, a dependency on policies (i.e. managed_prefs and recommended_prefs) is also wrong here. This was moved from chrome/ where it was okay to speak on those terms. Here we'd ideally be using a generic naming for the same mappings.
https://codereview.chromium.org/2479113002/diff/60001/chrome/browser/prefs/se... File chrome/browser/prefs/session_startup_pref.cc (right): https://codereview.chromium.org/2479113002/diff/60001/chrome/browser/prefs/se... chrome/browser/prefs/session_startup_pref.cc:47: const int SessionStartupPref::kPrefValueMax; On 2016/12/06 19:35:04, gab wrote: > Why do you need this? They're in the public header already? The context is here https://groups.google.com/a/chromium.org/forum/?utm_source=digest&utm_medium=...
https://codereview.chromium.org/2479113002/diff/60001/chrome/browser/prefs/se... File chrome/browser/prefs/session_startup_pref.cc (right): https://codereview.chromium.org/2479113002/diff/60001/chrome/browser/prefs/se... chrome/browser/prefs/session_startup_pref.cc:47: const int SessionStartupPref::kPrefValueMax; On 2016/12/06 19:42:11, vasilii wrote: > On 2016/12/06 19:35:04, gab wrote: > > Why do you need this? They're in the public header already? > > The context is here > https://groups.google.com/a/chromium.org/forum/?utm_source=digest&utm_medium=... This thread is huge, any specific entry? The OP is about declaring constants static (which is already the case in the header) so I still don't see why you need to do this fwd-declaration here after the definition in the header.
On 2016/12/06 19:51:44, gab wrote: > https://codereview.chromium.org/2479113002/diff/60001/chrome/browser/prefs/se... > File chrome/browser/prefs/session_startup_pref.cc (right): > > https://codereview.chromium.org/2479113002/diff/60001/chrome/browser/prefs/se... > chrome/browser/prefs/session_startup_pref.cc:47: const int > SessionStartupPref::kPrefValueMax; > On 2016/12/06 19:42:11, vasilii wrote: > > On 2016/12/06 19:35:04, gab wrote: > > > Why do you need this? They're in the public header already? > > > > The context is here > > > https://groups.google.com/a/chromium.org/forum/?utm_source=digest&utm_medium=... > > This thread is huge, any specific entry? The OP is about declaring constants > static (which is already the case in the header) so I still don't see why you > need to do this fwd-declaration here after the definition in the header. Just read the last few posts from November this year.
bauerb@chromium.org changed reviewers: + hashimoto@chromium.org
+hashimoto https://codereview.chromium.org/2479113002/diff/60001/components/pref_registr... File components/pref_registry/testing_pref_service_syncable.h (right): https://codereview.chromium.org/2479113002/diff/60001/components/pref_registr... components/pref_registry/testing_pref_service_syncable.h:5: #ifndef COMPONENTS_PERF_REGISTRY_TESTING_PREF_SERVICE_SYNCABLE_H_ Also: lol, perf registry :) https://codereview.chromium.org/2479113002/diff/60001/components/pref_registr... components/pref_registry/testing_pref_service_syncable.h:20: TestingPrefServiceSyncable(TestingPrefStore* managed_prefs, On 2016/12/06 19:35:04, gab wrote: > On 2016/12/06 19:16:42, vasilii wrote: > > How is this class different from > > components/sync_preferences/testing_pref_service_syncable.h? > > Do you use both? > > I'm also confused, maybe it wasn't properly removed in > https://codereview.chromium.org/1305213009 > > +bauerb/sdefresne? No idea. It actually looks like it was added as a duplicate in https://codereview.chromium.org/340703002.
https://codereview.chromium.org/2479113002/diff/60001/chrome/browser/prefs/se... File chrome/browser/prefs/session_startup_pref.cc (right): https://codereview.chromium.org/2479113002/diff/60001/chrome/browser/prefs/se... chrome/browser/prefs/session_startup_pref.cc:47: const int SessionStartupPref::kPrefValueMax; On 2016/12/06 19:51:44, gab wrote: > On 2016/12/06 19:42:11, vasilii wrote: > > On 2016/12/06 19:35:04, gab wrote: > > > Why do you need this? They're in the public header already? > > > > The context is here > > > https://groups.google.com/a/chromium.org/forum/?utm_source=digest&utm_medium=... > > This thread is huge, any specific entry? The OP is about declaring constants > static (which is already the case in the header) so I still don't see why you > need to do this fwd-declaration here after the definition in the header. Ok I see [1] and in particular [2], but why wasn't this required before? [1] https://groups.google.com/a/chromium.org/d/msg/chromium-dev/pF7nAmUQ2uw/_hhBK... [2] http://www.open-std.org/jtc1/sc22/wg21/docs/cwg_defects.html#454
On 2016/12/07 16:26:28, gab wrote: > https://codereview.chromium.org/2479113002/diff/60001/chrome/browser/prefs/se... > File chrome/browser/prefs/session_startup_pref.cc (right): > > https://codereview.chromium.org/2479113002/diff/60001/chrome/browser/prefs/se... > chrome/browser/prefs/session_startup_pref.cc:47: const int > SessionStartupPref::kPrefValueMax; > On 2016/12/06 19:51:44, gab wrote: > > On 2016/12/06 19:42:11, vasilii wrote: > > > On 2016/12/06 19:35:04, gab wrote: > > > > Why do you need this? They're in the public header already? > > > > > > The context is here > > > > > > https://groups.google.com/a/chromium.org/forum/?utm_source=digest&utm_medium=... > > > > This thread is huge, any specific entry? The OP is about declaring constants > > static (which is already the case in the header) so I still don't see why you > > need to do this fwd-declaration here after the definition in the header. > > Ok I see [1] and in particular [2], but why wasn't this required before? > > [1] > https://groups.google.com/a/chromium.org/d/msg/chromium-dev/pF7nAmUQ2uw/_hhBK... > [2] http://www.open-std.org/jtc1/sc22/wg21/docs/cwg_defects.html#454 Before base::FundamentalValue() took an int parameter by value. Now the code uses base::MakeUnique which does perfect forwarding and, therefore, captures the reference to the constant. Probably, the workaround would be to use WrapUnique.
On 2016/12/07 16:39:10, vasilii wrote: > On 2016/12/07 16:26:28, gab wrote: > > > https://codereview.chromium.org/2479113002/diff/60001/chrome/browser/prefs/se... > > File chrome/browser/prefs/session_startup_pref.cc (right): > > > > > https://codereview.chromium.org/2479113002/diff/60001/chrome/browser/prefs/se... > > chrome/browser/prefs/session_startup_pref.cc:47: const int > > SessionStartupPref::kPrefValueMax; > > On 2016/12/06 19:51:44, gab wrote: > > > On 2016/12/06 19:42:11, vasilii wrote: > > > > On 2016/12/06 19:35:04, gab wrote: > > > > > Why do you need this? They're in the public header already? > > > > > > > > The context is here > > > > > > > > > > https://groups.google.com/a/chromium.org/forum/?utm_source=digest&utm_medium=... > > > > > > This thread is huge, any specific entry? The OP is about declaring constants > > > static (which is already the case in the header) so I still don't see why > you > > > need to do this fwd-declaration here after the definition in the header. > > > > Ok I see [1] and in particular [2], but why wasn't this required before? > > > > [1] > > > https://groups.google.com/a/chromium.org/d/msg/chromium-dev/pF7nAmUQ2uw/_hhBK... > > [2] http://www.open-std.org/jtc1/sc22/wg21/docs/cwg_defects.html#454 > > Before base::FundamentalValue() took an int parameter by value. Now the code > uses base::MakeUnique which does perfect forwarding and, therefore, captures the > reference to the constant. Probably, the workaround would be to use WrapUnique. I see, feels like these constants should just be using an enum anyways (with explicitly set values that is given relation with existing on-disk prefs), that way they don't need extra storage, their names merely represents an inline integral value.
https://codereview.chromium.org/2479113002/diff/60001/components/pref_registr... File components/pref_registry/testing_pref_service_syncable.h (right): https://codereview.chromium.org/2479113002/diff/60001/components/pref_registr... components/pref_registry/testing_pref_service_syncable.h:20: TestingPrefServiceSyncable(TestingPrefStore* managed_prefs, On 2016/12/07 15:36:40, Bernhard Bauer wrote: > On 2016/12/06 19:35:04, gab wrote: > > On 2016/12/06 19:16:42, vasilii wrote: > > > How is this class different from > > > components/sync_preferences/testing_pref_service_syncable.h? > > > Do you use both? > > > > I'm also confused, maybe it wasn't properly removed in > > https://codereview.chromium.org/1305213009 > > > > +bauerb/sdefresne? > > No idea. It actually looks like it was added as a duplicate in > https://codereview.chromium.org/340703002. I added user_prefs::TestingPrefServiceSyncable (= PrefService in components/prefs + PrefRegistrySyncable) for tests which cannot depend on chrome, but requires PrefRegistrySyncable. OTOH Chrome's ::TestingPrefServiceSyncable is PrefServiceSyncable in chrome + PrefRegistrySyncable. I couldn't remove ::TestingPrefServiceSyncable when adding user_prefs::TestingPrefServiceSyncable because tests under chrome expects PrefServiceSyncable, not PrefService. Now PrefServiceSyncable is placed under components/syncable_prefs, so perhaps it's OK to remove user_prefs::TestingPrefServiceSyncable and use ::TestingPrefServiceSyncable everywhere, if all existing users are OK with depending on components/syncable_prefs.
On 2016/12/08 07:45:28, hashimoto wrote: > https://codereview.chromium.org/2479113002/diff/60001/components/pref_registr... > File components/pref_registry/testing_pref_service_syncable.h (right): > > https://codereview.chromium.org/2479113002/diff/60001/components/pref_registr... > components/pref_registry/testing_pref_service_syncable.h:20: > TestingPrefServiceSyncable(TestingPrefStore* managed_prefs, > On 2016/12/07 15:36:40, Bernhard Bauer wrote: > > On 2016/12/06 19:35:04, gab wrote: > > > On 2016/12/06 19:16:42, vasilii wrote: > > > > How is this class different from > > > > components/sync_preferences/testing_pref_service_syncable.h? > > > > Do you use both? > > > > > > I'm also confused, maybe it wasn't properly removed in > > > https://codereview.chromium.org/1305213009 > > > > > > +bauerb/sdefresne? > > > > No idea. It actually looks like it was added as a duplicate in > > https://codereview.chromium.org/340703002. > > I added user_prefs::TestingPrefServiceSyncable (= PrefService in > components/prefs + PrefRegistrySyncable) for tests which cannot depend on > chrome, but requires PrefRegistrySyncable. > OTOH Chrome's ::TestingPrefServiceSyncable is PrefServiceSyncable in chrome + > PrefRegistrySyncable. > I couldn't remove ::TestingPrefServiceSyncable when adding > user_prefs::TestingPrefServiceSyncable because tests under chrome expects > PrefServiceSyncable, not PrefService. > > Now PrefServiceSyncable is placed under components/syncable_prefs, so perhaps > it's OK to remove user_prefs::TestingPrefServiceSyncable and use > ::TestingPrefServiceSyncable everywhere, if all existing users are OK with > depending on components/syncable_prefs. That dependency should be fine for everyone yes, let's do this.
Answered your questions and updated code. https://codereview.chromium.org/2479113002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/settings_overrides/settings_overrides_browsertest.cc (right): https://codereview.chromium.org/2479113002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/settings_overrides/settings_overrides_browsertest.cc:27: // manifest. On 2016/12/06 at 19:16:42, vasilii wrote: > Mention that it's the particular extension from chrome/test/data/extensions/settings_override/. Done https://codereview.chromium.org/2479113002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/settings_overrides/settings_overrides_browsertest.cc:152: auto extension_dse = TestExtensionSearchEngine(prefs, prep_id); On 2016/12/06 at 19:16:42, vasilii wrote: > std::unique_ptr<TemplateURLData> would help readabilty here. Same below. Done https://codereview.chromium.org/2479113002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/settings_overrides/settings_overrides_browsertest.cc:153: ExpectSimilar(¤t_dse->data(), extension_dse.get()); On 2016/12/06 at 19:16:42, vasilii wrote: > extension_dse is the expected one. Thus, it's the first parameter. Same below. Done https://codereview.chromium.org/2479113002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/settings_overrides/settings_overrides_browsertest.cc:208: // test extension. On 2016/12/06 at 19:16:42, vasilii wrote: > I guess you obtain |prep_id| the same way as above. If it's too complicated then hardcode it in both places like it was before. I can get prep_id by loading extension and checking its SettingsOverrides, like I did in OverrideDSE test But I don't want to do it here because test checks specially that extension search engine is active before extension load. Alternative would be parsing manifest json and extracting prepopulated_id from it. Easiest way is to use constant. Hardcoded constant in TestExtensionSearchEngine. https://codereview.chromium.org/2479113002/diff/60001/chrome/browser/prefs/se... File chrome/browser/prefs/session_startup_pref.cc (right): https://codereview.chromium.org/2479113002/diff/60001/chrome/browser/prefs/se... chrome/browser/prefs/session_startup_pref.cc:47: const int SessionStartupPref::kPrefValueMax; I beleive vasilii answered this question below. https://codereview.chromium.org/2479113002/diff/60001/chrome/browser/search_e... File chrome/browser/search_engines/template_url_service_sync_unittest.cc (right): https://codereview.chromium.org/2479113002/diff/60001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_sync_unittest.cc:1712: TEST_F(TemplateURLServiceSyncTest, SyncWithExtensionDefaultSearch) { On 2016/12/06 at 19:16:42, vasilii wrote: > I meant a different scenario in mind when I asked to write a test. > > - Sync is active. > - An extension overrides the DSE. > - Test that the sync model doesn't get this update because we should sync the extensions and not its settings. Ok, got it, added GetAllSyncDataWithSearchOverrideExtension. https://codereview.chromium.org/2479113002/diff/60001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_sync_unittest.cc:1735: extension.search_terms_replacement_key = "espv"; On 2016/12/06 at 19:16:42, vasilii wrote: > Do you wanna use your helper function here? Done https://codereview.chromium.org/2479113002/diff/60001/chrome/browser/search_e... File chrome/browser/search_engines/template_url_service_test_util.cc (right): https://codereview.chromium.org/2479113002/diff/60001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_test_util.cc:166: auto dict = TemplateURLDataToDictionary(ext_data); On 2016/12/06 at 19:16:42, vasilii wrote: > Readabilty would improve with an explicit type.. Expanded. https://codereview.chromium.org/2479113002/diff/60001/chrome/browser/search_e... File chrome/browser/search_engines/template_url_service_test_util.h (right): https://codereview.chromium.org/2479113002/diff/60001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_test_util.h:70: // Write DSE into extension controlled preference in test profile. On 2016/12/06 at 19:16:42, vasilii wrote: > Please expand the DSE. Done https://codereview.chromium.org/2479113002/diff/60001/chrome/browser/search_e... File chrome/browser/search_engines/template_url_service_unittest.cc (right): https://codereview.chromium.org/2479113002/diff/60001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_unittest.cc:1453: // url service, still same extension engine is default. On 2016/12/06 at 19:16:42, vasilii wrote: > Can you explain why this is true? Thats how I think it should work. How TemplateURLService initializes(Here DSE stands for default search engine): 1. After creation, but before TemplateURLService load is completed it reads initial DSE from preferences. And after previous call to test_util()->AddExtensionControlledTURL initial DSE is provided by extension overriden DSE pref. 2. After TemplateURLService is loaded but before extension subsystem is loaded, TemplateURLService creates current default TemplateUrl to use from initial DSE. This is done by ApplyDefaultSearchChangeNoMetrics called from ChangeToLoadedState function. Default TemplateUrl is created from initial and is still extension controlled one. 3. After extensions are loaded, those that override default search override DSE in prefs and registers themselves in TemplateUrlService. In browser its done from SettingsOverridesAPI::OnExtensionLoaded calling RegisterSearchProvider. Here in test I just call test_util()->AddExtensionControlledTURL manually to emulate such behaviour. Or is the question is about something else? https://codereview.chromium.org/2479113002/diff/60001/components/pref_registr... File components/pref_registry/testing_pref_service_syncable.h (right): https://codereview.chromium.org/2479113002/diff/60001/components/pref_registr... components/pref_registry/testing_pref_service_syncable.h:20: TestingPrefServiceSyncable(TestingPrefStore* managed_prefs, On 2016/12/08 at 07:45:28, hashimoto wrote: > On 2016/12/07 15:36:40, Bernhard Bauer wrote: > > On 2016/12/06 19:35:04, gab wrote: > > > On 2016/12/06 19:16:42, vasilii wrote: > > > > How is this class different from > > > > components/sync_preferences/testing_pref_service_syncable.h? > > > > Do you use both? > > > > > > I'm also confused, maybe it wasn't properly removed in > > > https://codereview.chromium.org/1305213009 > > > > > > +bauerb/sdefresne? > > > > No idea. It actually looks like it was added as a duplicate in > > https://codereview.chromium.org/340703002. > > I added user_prefs::TestingPrefServiceSyncable (= PrefService in components/prefs + PrefRegistrySyncable) for tests which cannot depend on chrome, but requires PrefRegistrySyncable. > OTOH Chrome's ::TestingPrefServiceSyncable is PrefServiceSyncable in chrome + PrefRegistrySyncable. > I couldn't remove ::TestingPrefServiceSyncable when adding user_prefs::TestingPrefServiceSyncable because tests under chrome expects PrefServiceSyncable, not PrefService. > > Now PrefServiceSyncable is placed under components/syncable_prefs, so perhaps it's OK to remove user_prefs::TestingPrefServiceSyncable and use ::TestingPrefServiceSyncable everywhere, if all existing users are OK with depending on components/syncable_prefs. I think this should be fixed, but not in the scope of current CL. https://codereview.chromium.org/2479113002/diff/60001/components/search_engin... File components/search_engines/search_engines_test_util.cc (right): https://codereview.chromium.org/2479113002/diff/60001/components/search_engin... components/search_engines/search_engines_test_util.cc:9: #include "base/strings/string_split.h" On 2016/12/06 at 19:16:42, vasilii wrote: > unused? You are right, deleted https://codereview.chromium.org/2479113002/diff/60001/components/search_engin... components/search_engines/search_engines_test_util.cc:12: #include "components/policy/policy_constants.h" On 2016/12/06 at 19:16:42, vasilii wrote: > Obsolete includes? Deleted https://codereview.chromium.org/2479113002/diff/60001/components/search_engin... File components/search_engines/template_url.h (right): https://codereview.chromium.org/2479113002/diff/60001/components/search_engin... components/search_engines/template_url.h:621: const AssociatedExtensionInfo* extension_info_for_tests() { On 2016/12/06 at 19:16:42, vasilii wrote: > Use either ForTesting suffix or #if defined(UNIT_TEST) https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md Fixed https://codereview.chromium.org/2479113002/diff/60001/components/search_engin... File components/search_engines/template_url_service.cc (right): https://codereview.chromium.org/2479113002/diff/60001/components/search_engin... components/search_engines/template_url_service.cc:500: ext_dse) { On 2016/12/06 at 19:16:42, vasilii wrote: > Shouldn't it be > if (ext_dse) { > DCHECK_EQ(TemplateURL::NORMAL_CONTROLLED_BY_EXTENSION, template_url->type()); > } > ? Agree, fixed. https://codereview.chromium.org/2479113002/diff/60001/components/search_engin... components/search_engines/template_url_service.cc:503: DCHECK(ext_dse->data().keyword() == template_url->data().keyword()); On 2016/12/06 at 19:16:42, vasilii wrote: > Use DCHECK_EQ Fixed https://codereview.chromium.org/2479113002/diff/60001/components/search_engin... components/search_engines/template_url_service.cc:1965: std::string ext_id = client_->GetExtensionControllingDSEPref(); On 2016/12/06 at 19:16:42, vasilii wrote: > I'm not sure about the whole flow. That call makes me suspicions. > There are two cases. > 1. TemplateURLService isn't loaded yet. This situation is fully handled in template_url_service.cc:1923. > 2. TemplateURLService is loaded. TemplateURLService::AddExtensionControlledTURL should add a TemplateURL first. Then you overwrite the preferences, the code finds the correct search provider and makes it default. It shouldn't be possible that we need to add a search provider here. I described earlier the flow in comments to template_url_service_unittest.cc The problem this code solves is that when TemplateURLService is turned to loaded state it is possible that extensions are not loaded yet and extension controlled TURLs are not added yet to TemplateURLService. I beleive that currently where is no guaranty that SettingsOverridesAPI::OnExtensionLoaded is called for every extension at the moment when TemplateURLService::ChangeToLoadedState function called. Even if all extensions were loaded before TemplateURLService, they are waiting for TemplateURLService load registered by RegisterOnLoadedCallback - settings_overrides_api.cc:197. And TemplateURLService calls its onload callbacks only after creating its default_search_provider from intial_search_provider. So at the moment this function called at TemplateURLService start, FindMatchingExtensionTemplateURL will certainly return nullptr and default_search_provider_ will be intialized to nullptr as well. So nullptr will be returned by TemplateURLService::GetDefaultSearchProvider() for some period until extension that override default search is loaded and registered in TemplateURLService. I beleive such situation is unacceptable. https://codereview.chromium.org/2479113002/diff/60001/components/search_engin... File components/search_engines/template_url_service.h (right): https://codereview.chromium.org/2479113002/diff/60001/components/search_engin... components/search_engines/template_url_service.h:427: friend class TemplateURLServiceTest; On 2016/12/06 at 19:16:42, vasilii wrote: > What method is the reason for this? Removed, it seems its left from previous tests attempts. https://codereview.chromium.org/2479113002/diff/60001/components/search_engin... File components/search_engines/template_url_service_client.h (right): https://codereview.chromium.org/2479113002/diff/60001/components/search_engin... components/search_engines/template_url_service_client.h:46: virtual std::string GetExtensionControllingDSEPref() = 0; On 2016/12/06 at 19:16:42, vasilii wrote: > Hopefully, you won't need it. The component should know better the current state. I have written my thoughts above, not sure we can get without it. https://codereview.chromium.org/2479113002/diff/60001/components/sync_prefere... File components/sync_preferences/testing_pref_service_syncable.h (right): https://codereview.chromium.org/2479113002/diff/60001/components/sync_prefere... components/sync_preferences/testing_pref_service_syncable.h:25: TestingPrefStore* extension_prefs, On 2016/12/06 at 19:16:42, vasilii wrote: > I have mixed feelings about this. ExtensionPrefValueMap and ExtensionPrefs are defined in extensions/. The component shouldn't depend on the extensions/ > On the other hand, PrefValueStore already knows the extensions perfectly. I'll defer to the owners of the directory. Whats wrong with it? Preference class declared in src/components/prefs/pref_service.h has IsManaged, IsExtensionControlled methods so there should be a convenient way to set/get such prefs in unittests without creating complex reallife ExtensionPrefValueMap and ExtensionPrefs objects. I thought that was the idea behind TestingPrefServiceSyncable and thats why its so widely used in tests.
On 2016/12/08 at 13:37:32, gab wrote: > On 2016/12/08 07:45:28, hashimoto wrote: > > https://codereview.chromium.org/2479113002/diff/60001/components/pref_registr... > > File components/pref_registry/testing_pref_service_syncable.h (right): > > > > https://codereview.chromium.org/2479113002/diff/60001/components/pref_registr... > > components/pref_registry/testing_pref_service_syncable.h:20: > > TestingPrefServiceSyncable(TestingPrefStore* managed_prefs, > > On 2016/12/07 15:36:40, Bernhard Bauer wrote: > > > On 2016/12/06 19:35:04, gab wrote: > > > > On 2016/12/06 19:16:42, vasilii wrote: > > > > > How is this class different from > > > > > components/sync_preferences/testing_pref_service_syncable.h? > > > > > Do you use both? > > > > > > > > I'm also confused, maybe it wasn't properly removed in > > > > https://codereview.chromium.org/1305213009 > > > > > > > > +bauerb/sdefresne? > > > > > > No idea. It actually looks like it was added as a duplicate in > > > https://codereview.chromium.org/340703002. > > > > I added user_prefs::TestingPrefServiceSyncable (= PrefService in > > components/prefs + PrefRegistrySyncable) for tests which cannot depend on > > chrome, but requires PrefRegistrySyncable. > > OTOH Chrome's ::TestingPrefServiceSyncable is PrefServiceSyncable in chrome + > > PrefRegistrySyncable. > > I couldn't remove ::TestingPrefServiceSyncable when adding > > user_prefs::TestingPrefServiceSyncable because tests under chrome expects > > PrefServiceSyncable, not PrefService. > > > > Now PrefServiceSyncable is placed under components/syncable_prefs, so perhaps > > it's OK to remove user_prefs::TestingPrefServiceSyncable and use > > ::TestingPrefServiceSyncable everywhere, if all existing users are OK with > > depending on components/syncable_prefs. > > That dependency should be fine for everyone yes, let's do this. Created https://codereview.chromium.org/2562733003 with removal of user_prefs::TestingPrefServiceSyncable
https://codereview.chromium.org/2479113002/diff/60001/chrome/browser/prefs/se... File chrome/browser/prefs/session_startup_pref.cc (right): https://codereview.chromium.org/2479113002/diff/60001/chrome/browser/prefs/se... chrome/browser/prefs/session_startup_pref.cc:47: const int SessionStartupPref::kPrefValueMax; On 2016/12/09 08:19:52, Alexander Yashkin wrote: > I beleive vasilii answered this question below. Right he replied: """Before base::FundamentalValue() took an int parameter by value. Now the code uses base::MakeUnique which does perfect forwarding and, therefore, captures the reference to the constant. Probably, the workaround would be to use WrapUnique.""" to which I replied: """feels like these constants should just be using an enum anyways (with explicitly set values that is given relation with existing on-disk prefs), that way they don't need extra storage, their names merely represents an inline integral value."""
Other than I'm okay with adding "extensions" to testing_pref_service.h as the terminology is unfortunately already present in components/prefs [1]. The component could become agnostic to this and just ask for PrefStores to be registered in order of importance but this isn't the case today and not something this CL should have to address. [1] https://cs.chromium.org/chromium/src/components/prefs/pref_value_store.h?rcl=... https://codereview.chromium.org/2479113002/diff/80001/components/prefs/testin... File components/prefs/testing_pref_service.h (right): https://codereview.chromium.org/2479113002/diff/80001/components/prefs/testin... components/prefs/testing_pref_service.h:112: template <> nit: rm extra space
https://codereview.chromium.org/2479113002/diff/60001/chrome/browser/prefs/se... File chrome/browser/prefs/session_startup_pref.cc (right): https://codereview.chromium.org/2479113002/diff/60001/chrome/browser/prefs/se... chrome/browser/prefs/session_startup_pref.cc:47: const int SessionStartupPref::kPrefValueMax; On 2016/12/09 at 18:18:27, gab wrote: > On 2016/12/09 08:19:52, Alexander Yashkin wrote: > > I beleive vasilii answered this question below. > > Right he replied: > """Before base::FundamentalValue() took an int parameter by value. Now the code > uses base::MakeUnique which does perfect forwarding and, therefore, captures the > reference to the constant. Probably, the workaround would be to use WrapUnique.""" > > to which I replied: > """feels like these constants should just be using an enum anyways (with > explicitly set values that is given relation with existing on-disk prefs), that > way they don't need extra storage, their names merely represents an inline > integral value.""" Ok, changed to enum. https://codereview.chromium.org/2479113002/diff/80001/components/prefs/testin... File components/prefs/testing_pref_service.h (right): https://codereview.chromium.org/2479113002/diff/80001/components/prefs/testin... components/prefs/testing_pref_service.h:112: template <> On 2016/12/09 at 18:32:57, gab wrote: > nit: rm extra space Removed
Description was changed from ========== Make extensions DSE persistent in browser prefs 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 ========== to ========== Make extensions DSE persistent in browser prefs 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 ==========
bauerb@chromium.org changed reviewers: - bauerb@chromium.org
https://codereview.chromium.org/2479113002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/settings_overrides/settings_overrides_browsertest.cc (right): https://codereview.chromium.org/2479113002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/settings_overrides/settings_overrides_browsertest.cc:208: // test extension. On 2016/12/09 08:19:51, Alexander Yashkin wrote: > On 2016/12/06 at 19:16:42, vasilii wrote: > > I guess you obtain |prep_id| the same way as above. If it's too complicated > then hardcode it in both places like it was before. > > I can get prep_id by loading extension and checking its SettingsOverrides, like > I did in OverrideDSE test > But I don't want to do it here because test checks specially that extension > search engine is active before extension load. > Alternative would be parsing manifest json and extracting prepopulated_id from > it. Easiest way is to use constant. > Hardcoded constant in TestExtensionSearchEngine. Acknowledged. https://codereview.chromium.org/2479113002/diff/60001/chrome/browser/search_e... File chrome/browser/search_engines/template_url_service_unittest.cc (right): https://codereview.chromium.org/2479113002/diff/60001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_unittest.cc:1453: // url service, still same extension engine is default. On 2016/12/09 08:19:52, Alexander Yashkin wrote: > On 2016/12/06 at 19:16:42, vasilii wrote: > > Can you explain why this is true? > > Thats how I think it should work. > > How TemplateURLService initializes(Here DSE stands for default search engine): > 1. After creation, but before TemplateURLService load is completed it reads > initial DSE from preferences. And after previous call to > test_util()->AddExtensionControlledTURL initial DSE is provided by extension > overriden DSE pref. > 2. After TemplateURLService is loaded but before extension subsystem is loaded, > TemplateURLService creates current default TemplateUrl to use from initial DSE. > This is done by ApplyDefaultSearchChangeNoMetrics called from > ChangeToLoadedState function. Default TemplateUrl is created from initial and is > still extension controlled one. > 3. After extensions are loaded, those that override default search override DSE > in prefs and registers themselves in TemplateUrlService. > In browser its done from SettingsOverridesAPI::OnExtensionLoaded calling > RegisterSearchProvider. Here in test I just call > test_util()->AddExtensionControlledTURL manually to emulate such behaviour. > > Or is the question is about something else? Acknowledged. https://codereview.chromium.org/2479113002/diff/60001/components/search_engin... File components/search_engines/template_url_service.cc (right): https://codereview.chromium.org/2479113002/diff/60001/components/search_engin... components/search_engines/template_url_service.cc:1965: std::string ext_id = client_->GetExtensionControllingDSEPref(); On 2016/12/09 08:19:52, Alexander Yashkin wrote: > On 2016/12/06 at 19:16:42, vasilii wrote: > > I'm not sure about the whole flow. That call makes me suspicions. > > There are two cases. > > 1. TemplateURLService isn't loaded yet. This situation is fully handled in > template_url_service.cc:1923. > > 2. TemplateURLService is loaded. > TemplateURLService::AddExtensionControlledTURL should add a TemplateURL first. > Then you overwrite the preferences, the code finds the correct search provider > and makes it default. It shouldn't be possible that we need to add a search > provider here. > > I described earlier the flow in comments to template_url_service_unittest.cc > > The problem this code solves is that when TemplateURLService is turned to loaded > state it is possible that extensions are not loaded yet and extension controlled > TURLs are not added yet to TemplateURLService. I beleive that currently where is > no guaranty that SettingsOverridesAPI::OnExtensionLoaded is called for every > extension at the moment when TemplateURLService::ChangeToLoadedState function > called. > > Even if all extensions were loaded before TemplateURLService, they are waiting > for TemplateURLService load registered by RegisterOnLoadedCallback - > settings_overrides_api.cc:197. And TemplateURLService calls its onload callbacks > only after creating its default_search_provider from intial_search_provider. > So at the moment this function called at TemplateURLService start, > FindMatchingExtensionTemplateURL will certainly return nullptr and > default_search_provider_ will be intialized to nullptr as well. So nullptr will > be returned by TemplateURLService::GetDefaultSearchProvider() for some period > until extension that override default search is loaded and registered in > TemplateURLService. > I beleive such situation is unacceptable. ExtensionService::NotifyExtensionLoaded is called from ProfileManager::CreateAndInitializeProfile indirectly in profile_manager.cc:1307. TemplateURLService is created from profile_manager.cc:1305. However, it's loaded lazily and asynchronously. Thus, you can take for granted that TemplateURLService::ChangeToLoadedState happens much later than OnExtensionLoaded(). The second point indeed means that adding a provider is necessary like in UpdateProvidersCreatedByPolicy(). However, does somebody care which ext_id it has? Before loading it's also unknown. If the answer is yes than I think we should pass it via the overwritten preference like all other meaningful search provider properties. If the answer is no, then setting it to something here and then overwriting in AddExtensionControlledTURL. https://codereview.chromium.org/2479113002/diff/120001/chrome/browser/extensi... File chrome/browser/extensions/api/settings_overrides/settings_overrides_browsertest.cc (right): https://codereview.chromium.org/2479113002/diff/120001/chrome/browser/extensi... chrome/browser/extensions/api/settings_overrides/settings_overrides_browsertest.cc:204: TemplateURLData* dse = default_manager.GetDefaultSearchEngine(&source); current_dse? https://codereview.chromium.org/2479113002/diff/120001/chrome/browser/extensi... chrome/browser/extensions/api/settings_overrides/settings_overrides_browsertest.cc:214: TemplateURL ext_turl(*TestExtensionSearchEngine(profile->GetPrefs()), *extension_dse? https://codereview.chromium.org/2479113002/diff/120001/chrome/browser/search_... File chrome/browser/search_engines/template_url_service_sync_unittest.cc (right): https://codereview.chromium.org/2479113002/diff/120001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_sync_unittest.cc:1750: TEST_F(TemplateURLServiceSyncTest, SyncWithExtensionDefaultSearch) { You have one test above that checks GetAllSyncData() with an extension DSE. Here you need to check that adding a new DSE doesn't trigger an update - you call MergeDataAndStartSyncing with PassProcessor(). - change the DSE. - check that processor() didn't get a command "add a search engine DSE".
https://codereview.chromium.org/2479113002/diff/60001/components/search_engin... File components/search_engines/template_url_service.cc (right): https://codereview.chromium.org/2479113002/diff/60001/components/search_engin... components/search_engines/template_url_service.cc:1965: std::string ext_id = client_->GetExtensionControllingDSEPref(); On 2016/12/13 at 17:16:39, vasilii wrote: > On 2016/12/09 08:19:52, Alexander Yashkin wrote: > > On 2016/12/06 at 19:16:42, vasilii wrote: > > > I'm not sure about the whole flow. That call makes me suspicions. > > > There are two cases. > > > 1. TemplateURLService isn't loaded yet. This situation is fully handled in > > template_url_service.cc:1923. > > > 2. TemplateURLService is loaded. > > TemplateURLService::AddExtensionControlledTURL should add a TemplateURL first. > > Then you overwrite the preferences, the code finds the correct search provider > > and makes it default. It shouldn't be possible that we need to add a search > > provider here. > > > > I described earlier the flow in comments to template_url_service_unittest.cc > > > > The problem this code solves is that when TemplateURLService is turned to loaded > > state it is possible that extensions are not loaded yet and extension controlled > > TURLs are not added yet to TemplateURLService. I beleive that currently where is > > no guaranty that SettingsOverridesAPI::OnExtensionLoaded is called for every > > extension at the moment when TemplateURLService::ChangeToLoadedState function > > called. > > > > Even if all extensions were loaded before TemplateURLService, they are waiting > > for TemplateURLService load registered by RegisterOnLoadedCallback - > > settings_overrides_api.cc:197. And TemplateURLService calls its onload callbacks > > only after creating its default_search_provider from intial_search_provider. > > So at the moment this function called at TemplateURLService start, > > FindMatchingExtensionTemplateURL will certainly return nullptr and > > default_search_provider_ will be intialized to nullptr as well. So nullptr will > > be returned by TemplateURLService::GetDefaultSearchProvider() for some period > > until extension that override default search is loaded and registered in > > TemplateURLService. > > I beleive such situation is unacceptable. > > ExtensionService::NotifyExtensionLoaded is called from ProfileManager::CreateAndInitializeProfile indirectly in profile_manager.cc:1307. TemplateURLService is created from profile_manager.cc:1305. However, it's loaded lazily and asynchronously. Thus, you can take for granted that TemplateURLService::ChangeToLoadedState happens much later than OnExtensionLoaded(). > > The second point indeed means that adding a provider is necessary like in UpdateProvidersCreatedByPolicy(). However, does somebody care which ext_id it has? Before loading it's also unknown. > If the answer is yes than I think we should pass it via the overwritten preference like all other meaningful search provider properties. > If the answer is no, then setting it to something here and then overwriting in AddExtensionControlledTURL. Regarding the point that TemplateURLService is created earlier but loads later than NotifyExtensionLoaded is called - is not it a relying to much on current implementation? So we know that TemplateURLService loads search engines from DB and turns to loaded state somewhere later, and we know that extension are loaded synchronously from CreateAndInitializeProfile. What if this order of events will change in future? To the second remark - I think that ext_id must be correct for every extension engine because it can be accessed by GetExtensionId() method of TemplateUrl and can be accessed by every listener of TemplateURL service change notification. So I beleive it must be valid and correct for every engine. To the question of passing it through search engine properties - this creates duplication of entities - preferences already know what extension controls (or does not control) every preference key. So I prefer to get ext_id directly from preference subsystem than duplicating it in search provider properties. With duplication of values where is always risk that values goes out of sync. https://codereview.chromium.org/2479113002/diff/120001/chrome/browser/extensi... File chrome/browser/extensions/api/settings_overrides/settings_overrides_browsertest.cc (right): https://codereview.chromium.org/2479113002/diff/120001/chrome/browser/extensi... chrome/browser/extensions/api/settings_overrides/settings_overrides_browsertest.cc:204: TemplateURLData* dse = default_manager.GetDefaultSearchEngine(&source); On 2016/12/13 at 17:16:39, vasilii wrote: > current_dse? Done https://codereview.chromium.org/2479113002/diff/120001/chrome/browser/extensi... chrome/browser/extensions/api/settings_overrides/settings_overrides_browsertest.cc:214: TemplateURL ext_turl(*TestExtensionSearchEngine(profile->GetPrefs()), On 2016/12/13 at 17:16:39, vasilii wrote: > *extension_dse? Done, thanks https://codereview.chromium.org/2479113002/diff/120001/chrome/browser/search_... File chrome/browser/search_engines/template_url_service_sync_unittest.cc (right): https://codereview.chromium.org/2479113002/diff/120001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_sync_unittest.cc:1750: TEST_F(TemplateURLServiceSyncTest, SyncWithExtensionDefaultSearch) { On 2016/12/13 at 17:16:39, vasilii wrote: > You have one test above that checks GetAllSyncData() with an extension DSE. Here you need to check that adding a new DSE doesn't trigger an update > - you call MergeDataAndStartSyncing with PassProcessor(). > - change the DSE. > - check that processor() didn't get a command "add a search engine DSE". Added checks after AddExtensionControlledTURL.
https://codereview.chromium.org/2479113002/diff/60001/components/search_engin... File components/search_engines/template_url_service.cc (right): https://codereview.chromium.org/2479113002/diff/60001/components/search_engin... components/search_engines/template_url_service.cc:1965: std::string ext_id = client_->GetExtensionControllingDSEPref(); On 2016/12/14 19:25:21, Alexander Yashkin wrote: > On 2016/12/13 at 17:16:39, vasilii wrote: > > On 2016/12/09 08:19:52, Alexander Yashkin wrote: > > > On 2016/12/06 at 19:16:42, vasilii wrote: > > > > I'm not sure about the whole flow. That call makes me suspicions. > > > > There are two cases. > > > > 1. TemplateURLService isn't loaded yet. This situation is fully handled in > > > template_url_service.cc:1923. > > > > 2. TemplateURLService is loaded. > > > TemplateURLService::AddExtensionControlledTURL should add a TemplateURL > first. > > > Then you overwrite the preferences, the code finds the correct search > provider > > > and makes it default. It shouldn't be possible that we need to add a search > > > provider here. > > > > > > I described earlier the flow in comments to template_url_service_unittest.cc > > > > > > The problem this code solves is that when TemplateURLService is turned to > loaded > > > state it is possible that extensions are not loaded yet and extension > controlled > > > TURLs are not added yet to TemplateURLService. I beleive that currently > where is > > > no guaranty that SettingsOverridesAPI::OnExtensionLoaded is called for every > > > extension at the moment when TemplateURLService::ChangeToLoadedState > function > > > called. > > > > > > Even if all extensions were loaded before TemplateURLService, they are > waiting > > > for TemplateURLService load registered by RegisterOnLoadedCallback - > > > settings_overrides_api.cc:197. And TemplateURLService calls its onload > callbacks > > > only after creating its default_search_provider from intial_search_provider. > > > > So at the moment this function called at TemplateURLService start, > > > FindMatchingExtensionTemplateURL will certainly return nullptr and > > > default_search_provider_ will be intialized to nullptr as well. So nullptr > will > > > be returned by TemplateURLService::GetDefaultSearchProvider() for some > period > > > until extension that override default search is loaded and registered in > > > TemplateURLService. > > > I beleive such situation is unacceptable. > > > > ExtensionService::NotifyExtensionLoaded is called from > ProfileManager::CreateAndInitializeProfile indirectly in > profile_manager.cc:1307. TemplateURLService is created from > profile_manager.cc:1305. However, it's loaded lazily and asynchronously. Thus, > you can take for granted that TemplateURLService::ChangeToLoadedState happens > much later than OnExtensionLoaded(). > > > > The second point indeed means that adding a provider is necessary like in > UpdateProvidersCreatedByPolicy(). However, does somebody care which ext_id it > has? Before loading it's also unknown. > > If the answer is yes than I think we should pass it via the overwritten > preference like all other meaningful search provider properties. > > If the answer is no, then setting it to something here and then overwriting in > AddExtensionControlledTURL. > > Regarding the point that TemplateURLService is created earlier but loads later > than NotifyExtensionLoaded is called - is not it a relying to much on current > implementation? So we know that TemplateURLService loads search engines from DB > and turns to loaded state somewhere later, and we know that extension are loaded > synchronously from CreateAndInitializeProfile. What if this order of events will > change in future? > > To the second remark - I think that ext_id must be correct for every extension > engine because it can be accessed by GetExtensionId() method of TemplateUrl and > can be accessed by every listener of TemplateURL service change notification. So > I beleive it must be valid and correct for every engine. > To the question of passing it through search engine properties - this creates > duplication of entities - preferences already know what extension controls (or > does not control) every preference key. So I prefer to get ext_id directly from > preference subsystem than duplicating it in search provider properties. > With duplication of values where is always risk that values goes out of sync. It's hardly possible that this will change in the future. Let's assume that somebody starts loading TemplateURLService from the constructor. Even in this case it will take some time on another thread and then the task will be posted back to the UI thread asynchronously. OnExtensionLoaded() is called synchronously, thus, it will be executed first. If somebody moves extension loading to n seconds after profile creation then there will be a plenty of bugs in Chrome like the one you are fixing. An extension can override the new tab page or inject a script to every page. Thus, they should be available immediately. What about calling AddExtensionControlledTURL() before the service loads? As the extension search engines aren't stored anywhere you can add them to template_urls_ earlier. https://codereview.chromium.org/2479113002/diff/140001/chrome/browser/search_... File chrome/browser/search_engines/template_url_service_sync_unittest.cc (right): https://codereview.chromium.org/2479113002/diff/140001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_sync_unittest.cc:1764: ASSERT_FALSE(model()->is_default_search_managed()); Seems irrelevant. https://codereview.chromium.org/2479113002/diff/140001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_sync_unittest.cc:1784: processor()->change_for_guid("key2").change_type()); Those checks should go up to the place where you added "key2" https://codereview.chromium.org/2479113002/diff/140001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_sync_unittest.cc:1800: // Change kSyncedDefaultSearchProviderGUID to point to the new entry and The code below has nothing to do with Sync. I'd remove it.
I will upload latest minor change later. Wanted to hear about making AddExtensionControlledTURL work before TemplateURLService is loaded. https://codereview.chromium.org/2479113002/diff/60001/components/search_engin... File components/search_engines/template_url_service.cc (right): https://codereview.chromium.org/2479113002/diff/60001/components/search_engin... components/search_engines/template_url_service.cc:1965: std::string ext_id = client_->GetExtensionControllingDSEPref(); On 2016/12/15 at 13:41:57, vasilii wrote: > On 2016/12/14 19:25:21, Alexander Yashkin wrote: > > On 2016/12/13 at 17:16:39, vasilii wrote: > > > On 2016/12/09 08:19:52, Alexander Yashkin wrote: > > > > On 2016/12/06 at 19:16:42, vasilii wrote: > > > > > I'm not sure about the whole flow. That call makes me suspicions. > > > > > There are two cases. > > > > > 1. TemplateURLService isn't loaded yet. This situation is fully handled in > > > > template_url_service.cc:1923. > > > > > 2. TemplateURLService is loaded. > > > > TemplateURLService::AddExtensionControlledTURL should add a TemplateURL > > first. > > > > Then you overwrite the preferences, the code finds the correct search > > provider > > > > and makes it default. It shouldn't be possible that we need to add a search > > > > provider here. > > > > > > > > I described earlier the flow in comments to template_url_service_unittest.cc > > > > > > > > The problem this code solves is that when TemplateURLService is turned to > > loaded > > > > state it is possible that extensions are not loaded yet and extension > > controlled > > > > TURLs are not added yet to TemplateURLService. I beleive that currently > > where is > > > > no guaranty that SettingsOverridesAPI::OnExtensionLoaded is called for every > > > > extension at the moment when TemplateURLService::ChangeToLoadedState > > function > > > > called. > > > > > > > > Even if all extensions were loaded before TemplateURLService, they are > > waiting > > > > for TemplateURLService load registered by RegisterOnLoadedCallback - > > > > settings_overrides_api.cc:197. And TemplateURLService calls its onload > > callbacks > > > > only after creating its default_search_provider from intial_search_provider. > > > > > > So at the moment this function called at TemplateURLService start, > > > > FindMatchingExtensionTemplateURL will certainly return nullptr and > > > > default_search_provider_ will be intialized to nullptr as well. So nullptr > > will > > > > be returned by TemplateURLService::GetDefaultSearchProvider() for some > > period > > > > until extension that override default search is loaded and registered in > > > > TemplateURLService. > > > > I beleive such situation is unacceptable. > > > > > > ExtensionService::NotifyExtensionLoaded is called from > > ProfileManager::CreateAndInitializeProfile indirectly in > > profile_manager.cc:1307. TemplateURLService is created from > > profile_manager.cc:1305. However, it's loaded lazily and asynchronously. Thus, > > you can take for granted that TemplateURLService::ChangeToLoadedState happens > > much later than OnExtensionLoaded(). > > > > > > The second point indeed means that adding a provider is necessary like in > > UpdateProvidersCreatedByPolicy(). However, does somebody care which ext_id it > > has? Before loading it's also unknown. > > > If the answer is yes than I think we should pass it via the overwritten > > preference like all other meaningful search provider properties. > > > If the answer is no, then setting it to something here and then overwriting in > > AddExtensionControlledTURL. > > > > Regarding the point that TemplateURLService is created earlier but loads later > > than NotifyExtensionLoaded is called - is not it a relying to much on current > > implementation? So we know that TemplateURLService loads search engines from DB > > and turns to loaded state somewhere later, and we know that extension are loaded > > synchronously from CreateAndInitializeProfile. What if this order of events will > > change in future? > > > > To the second remark - I think that ext_id must be correct for every extension > > engine because it can be accessed by GetExtensionId() method of TemplateUrl and > > can be accessed by every listener of TemplateURL service change notification. So > > I beleive it must be valid and correct for every engine. > > To the question of passing it through search engine properties - this creates > > duplication of entities - preferences already know what extension controls (or > > does not control) every preference key. So I prefer to get ext_id directly from > > preference subsystem than duplicating it in search provider properties. > > With duplication of values where is always risk that values goes out of sync. > > It's hardly possible that this will change in the future. Let's assume that somebody starts loading TemplateURLService from the constructor. Even in this case it will take some time on another thread and then the task will be posted back to the UI thread asynchronously. OnExtensionLoaded() is called synchronously, thus, it will be executed first. If somebody moves extension loading to n seconds after profile creation then there will be a plenty of bugs in Chrome like the one you are fixing. An extension can override the new tab page or inject a script to every page. Thus, they should be available immediately. > > What about calling AddExtensionControlledTURL() before the service loads? As the extension search engines aren't stored anywhere you can add them to template_urls_ earlier. Calling AddExtensionControlledTURL even before TemplateURLService is loaded is rather radical change. I think it could work, but why it was not done so in a first place? Currently SettingsOverridesAPI is explicitly waiting for TemplateURLService load completion before adding extension engine. I can implement such solution, but what owners of TemplateURLService say? @pkasting or @stevet? Can you help? https://codereview.chromium.org/2479113002/diff/140001/chrome/browser/search_... File chrome/browser/search_engines/template_url_service_sync_unittest.cc (right): https://codereview.chromium.org/2479113002/diff/140001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_sync_unittest.cc:1764: ASSERT_FALSE(model()->is_default_search_managed()); On 2016/12/15 at 13:41:57, vasilii wrote: > Seems irrelevant. Copy paste remains. Deleted. https://codereview.chromium.org/2479113002/diff/140001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_sync_unittest.cc:1784: processor()->change_for_guid("key2").change_type()); On 2016/12/15 at 13:41:57, vasilii wrote: > Those checks should go up to the place where you added "key2" Done https://codereview.chromium.org/2479113002/diff/140001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_sync_unittest.cc:1800: // Change kSyncedDefaultSearchProviderGUID to point to the new entry and On 2016/12/15 at 13:41:57, vasilii wrote: > The code below has nothing to do with Sync. I'd remove it. I think that this code checks that while extension controlled engine is active, user synced DSE will not change current search but will apply just after extension controlled search is removed.
https://codereview.chromium.org/2479113002/diff/60001/components/search_engin... File components/search_engines/template_url_service.cc (right): https://codereview.chromium.org/2479113002/diff/60001/components/search_engin... components/search_engines/template_url_service.cc:1965: std::string ext_id = client_->GetExtensionControllingDSEPref(); On 2016/12/15 15:47:10, Alexander Yashkin wrote: > Calling AddExtensionControlledTURL even before TemplateURLService is loaded is > rather radical change. > I think it could work, but why it was not done so in a first place? Maybe no one thought of it? If you're asking if it will break something, the answer is I don't know. I did less with the extension-related code here, and stevet is gone. I don't think anyone can answer you other than git blame, careful pondering, and maybe vasilii's guidance, which I trust much more than my own instincts on this issue.
https://codereview.chromium.org/2479113002/diff/60001/components/search_engin... File components/search_engines/template_url_service.cc (right): https://codereview.chromium.org/2479113002/diff/60001/components/search_engin... components/search_engines/template_url_service.cc:1965: std::string ext_id = client_->GetExtensionControllingDSEPref(); On 2016/12/16 at 08:41:06, Peter Kasting wrote: > On 2016/12/15 15:47:10, Alexander Yashkin wrote: > > Calling AddExtensionControlledTURL even before TemplateURLService is loaded is > > rather radical change. > > I think it could work, but why it was not done so in a first place? > > Maybe no one thought of it? If you're asking if it will break something, the answer is I don't know. I did less with the extension-related code here, and stevet is gone. I don't think anyone can answer you other than git blame, careful pondering, and maybe vasilii's guidance, which I trust much more than my own instincts on this issue. Ok, I'll try to implement it.
https://codereview.chromium.org/2479113002/diff/60001/components/search_engin... File components/search_engines/template_url_service.cc (right): https://codereview.chromium.org/2479113002/diff/60001/components/search_engin... components/search_engines/template_url_service.cc:1965: std::string ext_id = client_->GetExtensionControllingDSEPref(); On 2016/12/16 08:45:50, Alexander Yashkin wrote: > On 2016/12/16 at 08:41:06, Peter Kasting wrote: > > On 2016/12/15 15:47:10, Alexander Yashkin wrote: > > > Calling AddExtensionControlledTURL even before TemplateURLService is loaded > is > > > rather radical change. > > > I think it could work, but why it was not done so in a first place? > > > > Maybe no one thought of it? If you're asking if it will break something, the > answer is I don't know. I did less with the extension-related code here, and > stevet is gone. I don't think anyone can answer you other than git blame, > careful pondering, and maybe vasilii's guidance, which I trust much more than my > own instincts on this issue. > > Ok, I'll try to implement it. I didn't implement it this way in the beginning because I though that TemplateURLService isn't usable anyway before it's loaded and Chrome code waits before changing TemplateURLService. My assumption was almost correct. My immediate response to the bug was exactly that fix (https://bugs.chromium.org/p/chromium/issues/detail?id=450534#c2). At the same time overriding the prefs seems also a necessary step.
https://codereview.chromium.org/2479113002/diff/60001/components/search_engin... File components/search_engines/template_url_service.cc (right): https://codereview.chromium.org/2479113002/diff/60001/components/search_engin... components/search_engines/template_url_service.cc:1965: std::string ext_id = client_->GetExtensionControllingDSEPref(); On 2016/12/16 at 10:22:20, vasilii wrote: > On 2016/12/16 08:45:50, Alexander Yashkin wrote: > > On 2016/12/16 at 08:41:06, Peter Kasting wrote: > > > On 2016/12/15 15:47:10, Alexander Yashkin wrote: > > > > Calling AddExtensionControlledTURL even before TemplateURLService is loaded > > is > > > > rather radical change. > > > > I think it could work, but why it was not done so in a first place? > > > > > > Maybe no one thought of it? If you're asking if it will break something, the > > answer is I don't know. I did less with the extension-related code here, and > > stevet is gone. I don't think anyone can answer you other than git blame, > > careful pondering, and maybe vasilii's guidance, which I trust much more than my > > own instincts on this issue. > > > > Ok, I'll try to implement it. > > I didn't implement it this way in the beginning because I though that TemplateURLService isn't usable anyway before it's loaded and Chrome code waits before changing TemplateURLService. My assumption was almost correct. > > My immediate response to the bug was exactly that fix (https://bugs.chromium.org/p/chromium/issues/detail?id=450534#c2). At the same time overriding the prefs seems also a necessary step. Updated, enable extensions search engines registration in TemplateURLService before its loaded.
The approach looks good. https://codereview.chromium.org/2479113002/diff/160001/chrome/browser/search_... File chrome/browser/search_engines/template_url_service_unittest.cc (right): https://codereview.chromium.org/2479113002/diff/160001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_unittest.cc:1463: // Emulate extension system loaded and extension registers itself to template Wouldn't it be better to split the test into two here? The second test doesn't call VerifyLoad() in the beginning but adds an extension search engine. Then it loads and checks that the engine is still here.
https://codereview.chromium.org/2479113002/diff/160001/chrome/browser/search_... File chrome/browser/search_engines/template_url_service_unittest.cc (right): https://codereview.chromium.org/2479113002/diff/160001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_unittest.cc:1463: // Emulate extension system loaded and extension registers itself to template On 2016/12/20 at 16:50:26, vasilii wrote: > Wouldn't it be better to split the test into two here? > > The second test doesn't call VerifyLoad() in the beginning but adds an extension search engine. Then it loads and checks that the engine is still here. Done
On 2016/12/20 at 18:44:54, Alexander Yashkin wrote: > https://codereview.chromium.org/2479113002/diff/160001/chrome/browser/search_... > File chrome/browser/search_engines/template_url_service_unittest.cc (right): > > https://codereview.chromium.org/2479113002/diff/160001/chrome/browser/search_... > chrome/browser/search_engines/template_url_service_unittest.cc:1463: // Emulate extension system loaded and extension registers itself to template > On 2016/12/20 at 16:50:26, vasilii wrote: > > Wouldn't it be better to split the test into two here? > > > > The second test doesn't call VerifyLoad() in the beginning but adds an extension search engine. Then it loads and checks that the engine is still here. > > Done Current approach looks good to vasilii. @pkasting, please take a look at search_engines. @gab, please take a look at prefs changes.
https://codereview.chromium.org/2479113002/diff/60001/components/pref_registr... File components/pref_registry/testing_pref_service_syncable.h (right): https://codereview.chromium.org/2479113002/diff/60001/components/pref_registr... components/pref_registry/testing_pref_service_syncable.h:20: TestingPrefServiceSyncable(TestingPrefStore* managed_prefs, On 2016/12/06 19:35:04, gab wrote: > On 2016/12/06 19:16:42, vasilii wrote: > > How is this class different from > > components/sync_preferences/testing_pref_service_syncable.h? > > Do you use both? > > I'm also confused, maybe it wasn't properly removed in > https://codereview.chromium.org/1305213009 > > +bauerb/sdefresne? I don't know why they were not merged. We probably didn't notice the two really similar classes then.
sdefresne@chromium.org changed reviewers: - sdefresne@chromium.org
lgtm
prefs lgtm
Seems generally fine, the below comments are mostly minor. In a few places the functional changes were obscured by what seemed like cleanup changes. I love cleanup changes, and on small CLs I am happy to see them mixed with functional ones, but this is big enough that it might have been nice to try to stage it as refactorings/cleanups first, then functional changes, or something. https://codereview.chromium.org/2479113002/diff/180001/chrome/browser/search_... File chrome/browser/search_engines/template_url_service_sync_unittest.cc (right): https://codereview.chromium.org/2479113002/diff/180001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_sync_unittest.cc:464: std::unique_ptr<TemplateURL> deserialized(Deserialize(*iter)); Nit: Use = instead of (), for consistency with line 442 https://codereview.chromium.org/2479113002/diff/180001/chrome/browser/search_... File chrome/browser/search_engines/template_url_service_test_util.cc (right): https://codereview.chromium.org/2479113002/diff/180001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_test_util.cc:183: // Set extension overriden DSE value to prefs. Nit: This comment seems to just restate the function name below. Is it really adding anything? https://codereview.chromium.org/2479113002/diff/180001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_test_util.cc:195: if (ext_turl->GetExtensionInfoForTesting()->wants_to_be_default_engine) { Nit: No {} https://codereview.chromium.org/2479113002/diff/180001/chrome/browser/search_... File chrome/browser/search_engines/template_url_service_test_util.h (right): https://codereview.chromium.org/2479113002/diff/180001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_test_util.h:69: // Write default search engine into extension controlled preference in test Nit: Function comments should be declarative ("writes") rather than imperative ("write"); see http://google.github.io/styleguide/cppguide.html#Function_Comments . (4 places) https://codereview.chromium.org/2479113002/diff/180001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_test_util.h:71: void SetExtensionDefaultSearchInPrefs(const std::string& ext_id, This first arg is not actually used in the implementation. Remove unused parameters. (2 places) https://codereview.chromium.org/2479113002/diff/180001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_test_util.h:72: const TemplateURLData& ext_data); Nit: Avoid abbreviations like "ext" (could read as "external" or something); see http://google.github.io/styleguide/cppguide.html#General_Naming_Rules . If it's clear that this is the extension ID and data, just say |id| and |data|. If not, say |extension_id| or the like. (Multiple places) https://codereview.chromium.org/2479113002/diff/180001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_test_util.h:73: // Remove extension controlled default search engine preference from test Nit: I'd still put a blink line above this comment even though these two functions are loosely-related; it's enough to order them next to each other. (2 places) https://codereview.chromium.org/2479113002/diff/180001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_test_util.h:82: // Remove extension turl from preferences and model. Nit: Expand this (and comments like it) to cover the function contract and give more detail where relevant. For example: Removes a TemplateURL controlled by |ext_id| from the model, and, if necessary, from the extension-controlled default search preference. This TemplateURL must exist. https://codereview.chromium.org/2479113002/diff/180001/chrome/browser/search_... File chrome/browser/search_engines/template_url_service_unittest.cc (right): https://codereview.chromium.org/2479113002/diff/180001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_unittest.cc:1437: // Create default extension search engine. Nit: Blank line above this https://codereview.chromium.org/2479113002/diff/180001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_unittest.cc:1446: std::unique_ptr<TemplateURL> cloned_ext_dse( Nit: Prefer = to () for this "copy-like" initialization; see https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Variab... . You're welcome to use "auto" here to avoid restarting the type, since MakeUnique<TemplateURL> is pretty clear. https://codereview.chromium.org/2479113002/diff/180001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_unittest.cc:1457: // restarts, until extension is unloaded/disabled. Check it persists. Nit: Last sentence unnecessary (restates the code) https://codereview.chromium.org/2479113002/diff/180001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_unittest.cc:1464: // url service, this is always done before TemplateURLService is loaded. Nit: The meaning of this is unclear. Do you mean: "Chrome will load the extension system before the TemplateURLService, so extensions controlling the default search engine may be registered before the service has loaded." https://codereview.chromium.org/2479113002/diff/180001/components/search_engi... File components/search_engines/default_search_manager.cc (right): https://codereview.chromium.org/2479113002/diff/180001/components/search_engi... components/search_engines/default_search_manager.cc:178: Source source = GetDefaultSearchEngineSource(); Nit: I found this function unclear for a while until I realized that the whole purpose of |source| is now just to track whether we were in any other case than "was fallback, and still are". How about something like this: bool source_was_fallback = GetDefaultSearchEngineSource() == FROM_FALLBACK; LoadDefaultSearchEngineFromPrefs(); // The effective DSE may have changed unless we were using the fallback source // both before and after the above load. if (!source_was_fallback || (GetDefaultSearchEngineSource() != FROM_FALLBACK)) NotifyObserver(); https://codereview.chromium.org/2479113002/diff/180001/components/search_engi... File components/search_engines/default_search_manager_unittest.cc (left): https://codereview.chromium.org/2479113002/diff/180001/components/search_engi... components/search_engines/default_search_manager_unittest.cc:268: Nit: I wouldn't remove the blank lines in this test, I think they improved readability. https://codereview.chromium.org/2479113002/diff/180001/components/search_engi... File components/search_engines/default_search_manager_unittest.cc (right): https://codereview.chromium.org/2479113002/diff/180001/components/search_engi... components/search_engines/default_search_manager_unittest.cc:31: "default_search_provider_data.template_url_data"; Seems like this could go away and be replaced by DefaultSearchManager::kDefaultSearchProviderDataPrefName, which has the same value and is in this same component. https://codereview.chromium.org/2479113002/diff/180001/components/search_engi... components/search_engines/default_search_manager_unittest.cc:84: const TemplateURLData& data) { Once the pref change I suggested above is made, this function will be very similar to TemplateURLServiceTestUtil::SetExtensionDefaultSearchInPrefs(), which you're adding. Maybe it makes sense to just eliminate that function and its partner (or all four functions there?) in favor of similar ones here in components/search_engines/. https://codereview.chromium.org/2479113002/diff/180001/components/search_engi... components/search_engines/default_search_manager_unittest.cc:85: std::unique_ptr<base::DictionaryValue> entry( Nit: Use = https://codereview.chromium.org/2479113002/diff/180001/components/search_engi... File components/search_engines/search_engines_test_util.cc (right): https://codereview.chromium.org/2479113002/diff/180001/components/search_engi... components/search_engines/search_engines_test_util.cc:16: std::unique_ptr<TemplateURLData> data(new TemplateURLData()); Nit: Prefer MakeUnique() to bare new https://codereview.chromium.org/2479113002/diff/180001/components/search_engi... components/search_engines/search_engines_test_util.cc:34: ASSERT_TRUE(actual != NULL); Nit: Use nullptr, or just write ASSERT_TRUE(expected) and similar https://codereview.chromium.org/2479113002/diff/180001/components/search_engi... File components/search_engines/search_engines_test_util.h (right): https://codereview.chromium.org/2479113002/diff/180001/components/search_engi... components/search_engines/search_engines_test_util.h:12: // Generate TemplateUrlData structure useful for tests filled with values Nit: Url -> URL https://codereview.chromium.org/2479113002/diff/180001/components/search_engi... components/search_engines/search_engines_test_util.h:14: std::unique_ptr<TemplateURLData> GenerateDummyTemplateURLData( I'm kinda surprised we don't have functions like these two already somewhere. I'm also wondering if we could refactor more of our tests code (not in this CL!) to use these. https://codereview.chromium.org/2479113002/diff/180001/components/search_engi... components/search_engines/search_engines_test_util.h:18: // date_created or the last_modified time. Neither pointer should be nullptr. Nit: In comments, just say "null". https://codereview.chromium.org/2479113002/diff/180001/components/search_engi... File components/search_engines/template_url_prepopulate_data.cc (right): https://codereview.chromium.org/2479113002/diff/180001/components/search_engi... components/search_engines/template_url_prepopulate_data.cc:1047: size_t default_index; Nit: Probably should be done in a separate CL, but it seems like this isn't the only caller that doesn't really care about this value. Probably GetPrepopulatedEngines() should change to allow its second arg to be null for callers that don't care. https://codereview.chromium.org/2479113002/diff/180001/components/search_engi... components/search_engines/template_url_prepopulate_data.cc:1048: std::vector<std::unique_ptr<TemplateURLData>> engines = Nit: Use auto https://codereview.chromium.org/2479113002/diff/180001/components/search_engi... components/search_engines/template_url_prepopulate_data.cc:1053: } I tried writing this out with find_if(), but I don't know whether it's better in the end. It would probably be better with generic lambdas, but we don't support C++14. Up to you if you think this is worth using. auto Pred = [prepopulate_id](const std::unique_ptr<TemplateURLData>& data) { return data->prepopulate_id == prepopulate_id; }; auto it = std::find_if(engines.begin(), engines.end(), Pred); return it == engines.end() ? nullptr : std::move(*it); https://codereview.chromium.org/2479113002/diff/180001/components/search_engi... File components/search_engines/template_url_prepopulate_data.h (right): https://codereview.chromium.org/2479113002/diff/180001/components/search_engi... components/search_engines/template_url_prepopulate_data.h:45: // Find the prepopulated search engine with the given id. Nit: Find -> Returns, id -> |prepopulate_id| https://codereview.chromium.org/2479113002/diff/180001/components/search_engi... File components/search_engines/template_url_service.cc (right): https://codereview.chromium.org/2479113002/diff/180001/components/search_engi... components/search_engines/template_url_service.cc:497: if (template_url_ptr) { Nit: No {} https://codereview.chromium.org/2479113002/diff/180001/components/search_engi... components/search_engines/template_url_service.cc:519: KeywordWebDataService::BatchModeScoper scoper(web_data_service_.get()); I don't think we want this scoper anymore?
On 2016/12/22 at 20:49:04, pkasting wrote: > Seems generally fine, the below comments are mostly minor. > > In a few places the functional changes were obscured by what seemed like cleanup changes. I love cleanup changes, and on small CLs I am happy to see them mixed with functional ones, but this is big enough that it might have been nice to try to stage it as refactorings/cleanups first, then functional changes, or something. Agree, yet sometimes I do refactoring/cleanup because reviewer asks for it, and sometimes its not clear at the start what final variant of code will be and what refactoring/cleanup will be necessary. I already extracted https://codereview.chromium.org/2497853002 and cleaned up https://codereview.chromium.org/2562733003 from current CL.
https://codereview.chromium.org/2479113002/diff/180001/chrome/browser/search_... File chrome/browser/search_engines/template_url_service_sync_unittest.cc (right): https://codereview.chromium.org/2479113002/diff/180001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_sync_unittest.cc:464: std::unique_ptr<TemplateURL> deserialized(Deserialize(*iter)); On 2016/12/22 at 20:49:03, Peter Kasting wrote: > Nit: Use = instead of (), for consistency with line 442 Done https://codereview.chromium.org/2479113002/diff/180001/chrome/browser/search_... File chrome/browser/search_engines/template_url_service_test_util.cc (right): https://codereview.chromium.org/2479113002/diff/180001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_test_util.cc:183: // Set extension overriden DSE value to prefs. On 2016/12/22 at 20:49:03, Peter Kasting wrote: > Nit: This comment seems to just restate the function name below. Is it really adding anything? Removed https://codereview.chromium.org/2479113002/diff/180001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_test_util.cc:195: if (ext_turl->GetExtensionInfoForTesting()->wants_to_be_default_engine) { On 2016/12/22 at 20:49:03, Peter Kasting wrote: > Nit: No {} Removed https://codereview.chromium.org/2479113002/diff/180001/chrome/browser/search_... File chrome/browser/search_engines/template_url_service_test_util.h (right): https://codereview.chromium.org/2479113002/diff/180001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_test_util.h:69: // Write default search engine into extension controlled preference in test On 2016/12/22 at 20:49:03, Peter Kasting wrote: > Nit: Function comments should be declarative ("writes") rather than imperative ("write"); see http://google.github.io/styleguide/cppguide.html#Function_Comments . (4 places) Done https://codereview.chromium.org/2479113002/diff/180001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_test_util.h:71: void SetExtensionDefaultSearchInPrefs(const std::string& ext_id, On 2016/12/22 at 20:49:03, Peter Kasting wrote: > This first arg is not actually used in the implementation. Remove unused parameters. (2 places) This remains from previous approach, removed. https://codereview.chromium.org/2479113002/diff/180001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_test_util.h:72: const TemplateURLData& ext_data); On 2016/12/22 at 20:49:03, Peter Kasting wrote: > Nit: Avoid abbreviations like "ext" (could read as "external" or something); see http://google.github.io/styleguide/cppguide.html#General_Naming_Rules . If it's clear that this is the extension ID and data, just say |id| and |data|. If not, say |extension_id| or the like. (Multiple places) Done, replaced with "extension_" or removed. https://codereview.chromium.org/2479113002/diff/180001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_test_util.h:73: // Remove extension controlled default search engine preference from test On 2016/12/22 at 20:49:03, Peter Kasting wrote: > Nit: I'd still put a blink line above this comment even though these two functions are loosely-related; it's enough to order them next to each other. (2 places) Done https://codereview.chromium.org/2479113002/diff/180001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_test_util.h:82: // Remove extension turl from preferences and model. On 2016/12/22 at 20:49:03, Peter Kasting wrote: > Nit: Expand this (and comments like it) to cover the function contract and give more detail where relevant. For example: > > Removes a TemplateURL controlled by |ext_id| from the model, and, if necessary, from the extension-controlled default search preference. This TemplateURL must exist. Done, thanks. https://codereview.chromium.org/2479113002/diff/180001/chrome/browser/search_... File chrome/browser/search_engines/template_url_service_unittest.cc (right): https://codereview.chromium.org/2479113002/diff/180001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_unittest.cc:1437: // Create default extension search engine. On 2016/12/22 at 20:49:03, Peter Kasting wrote: > Nit: Blank line above this Done https://codereview.chromium.org/2479113002/diff/180001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_unittest.cc:1446: std::unique_ptr<TemplateURL> cloned_ext_dse( On 2016/12/22 at 20:49:03, Peter Kasting wrote: > Nit: Prefer = to () for this "copy-like" initialization; see https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Variab... . > > You're welcome to use "auto" here to avoid restarting the type, since MakeUnique<TemplateURL> is pretty clear. Done https://codereview.chromium.org/2479113002/diff/180001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_unittest.cc:1457: // restarts, until extension is unloaded/disabled. Check it persists. On 2016/12/22 at 20:49:03, Peter Kasting wrote: > Nit: Last sentence unnecessary (restates the code) Done https://codereview.chromium.org/2479113002/diff/180001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_unittest.cc:1464: // url service, this is always done before TemplateURLService is loaded. On 2016/12/22 at 20:49:03, Peter Kasting wrote: > Nit: The meaning of this is unclear. Do you mean: "Chrome will load the extension system before the TemplateURLService, so extensions controlling the default search engine may be registered before the service has loaded." Done, thanks. https://codereview.chromium.org/2479113002/diff/180001/components/search_engi... File components/search_engines/default_search_manager.cc (right): https://codereview.chromium.org/2479113002/diff/180001/components/search_engi... components/search_engines/default_search_manager.cc:178: Source source = GetDefaultSearchEngineSource(); On 2016/12/22 at 20:49:03, Peter Kasting wrote: > Nit: I found this function unclear for a while until I realized that the whole purpose of |source| is now just to track whether we were in any other case than "was fallback, and still are". > > How about something like this: > > bool source_was_fallback = GetDefaultSearchEngineSource() == FROM_FALLBACK; > > LoadDefaultSearchEngineFromPrefs(); > > // The effective DSE may have changed unless we were using the fallback source > // both before and after the above load. > if (!source_was_fallback || (GetDefaultSearchEngineSource() != FROM_FALLBACK)) > NotifyObserver(); Done, much clearer. https://codereview.chromium.org/2479113002/diff/180001/components/search_engi... File components/search_engines/default_search_manager_unittest.cc (left): https://codereview.chromium.org/2479113002/diff/180001/components/search_engi... components/search_engines/default_search_manager_unittest.cc:268: On 2016/12/22 at 20:49:04, Peter Kasting wrote: > Nit: I wouldn't remove the blank lines in this test, I think they improved readability. I dont remember removing them intentionally, maybe it was git cl format. Reverted https://codereview.chromium.org/2479113002/diff/180001/components/search_engi... File components/search_engines/default_search_manager_unittest.cc (right): https://codereview.chromium.org/2479113002/diff/180001/components/search_engi... components/search_engines/default_search_manager_unittest.cc:31: "default_search_provider_data.template_url_data"; On 2016/12/22 at 20:49:04, Peter Kasting wrote: > Seems like this could go away and be replaced by DefaultSearchManager::kDefaultSearchProviderDataPrefName, which has the same value and is in this same component. Done. I thought about it, yet decided to leave until later refactoring, to keep scope of current CL under control :) https://codereview.chromium.org/2479113002/diff/180001/components/search_engi... components/search_engines/default_search_manager_unittest.cc:84: const TemplateURLData& data) { On 2016/12/22 at 20:49:04, Peter Kasting wrote: > Once the pref change I suggested above is made, this function will be very similar to TemplateURLServiceTestUtil::SetExtensionDefaultSearchInPrefs(), which you're adding. > > Maybe it makes sense to just eliminate that function and its partner (or all four functions there?) in favor of similar ones here in components/search_engines/. I have extracted SetExtensionDefaultSearchInPrefs and RemoveExtensionDefaultSearchFromPrefs to search_engines/search_engines_test_util and reused in both places. Yet I am not sure about other functions, do you mean AddExtensionControlledTURL/RemoveExtensionControlledTURL in TemplateURLServiceTestUtil? They must have access to model() and profile() in TemplateURLServiceTestUtil. And they look more like part of interface in TemplateURLServiceTestUtil. https://codereview.chromium.org/2479113002/diff/180001/components/search_engi... components/search_engines/default_search_manager_unittest.cc:85: std::unique_ptr<base::DictionaryValue> entry( On 2016/12/22 at 20:49:03, Peter Kasting wrote: > Nit: Use = Done https://codereview.chromium.org/2479113002/diff/180001/components/search_engi... File components/search_engines/search_engines_test_util.cc (right): https://codereview.chromium.org/2479113002/diff/180001/components/search_engi... components/search_engines/search_engines_test_util.cc:16: std::unique_ptr<TemplateURLData> data(new TemplateURLData()); On 2016/12/22 at 20:49:04, Peter Kasting wrote: > Nit: Prefer MakeUnique() to bare new Done https://codereview.chromium.org/2479113002/diff/180001/components/search_engi... components/search_engines/search_engines_test_util.cc:34: ASSERT_TRUE(actual != NULL); On 2016/12/22 at 20:49:04, Peter Kasting wrote: > Nit: Use nullptr, or just write ASSERT_TRUE(expected) and similar Done https://codereview.chromium.org/2479113002/diff/180001/components/search_engi... File components/search_engines/search_engines_test_util.h (right): https://codereview.chromium.org/2479113002/diff/180001/components/search_engi... components/search_engines/search_engines_test_util.h:12: // Generate TemplateUrlData structure useful for tests filled with values On 2016/12/22 at 20:49:04, Peter Kasting wrote: > Nit: Url -> URL Done https://codereview.chromium.org/2479113002/diff/180001/components/search_engi... components/search_engines/search_engines_test_util.h:14: std::unique_ptr<TemplateURLData> GenerateDummyTemplateURLData( On 2016/12/22 at 20:49:04, Peter Kasting wrote: > I'm kinda surprised we don't have functions like these two already somewhere. > > I'm also wondering if we could refactor more of our tests code (not in this CL!) to use these. I think I saw several similar functions across code, for example in template_url_service_unittest.cc. Refactor is possible. https://codereview.chromium.org/2479113002/diff/180001/components/search_engi... components/search_engines/search_engines_test_util.h:18: // date_created or the last_modified time. Neither pointer should be nullptr. On 2016/12/22 at 20:49:04, Peter Kasting wrote: > Nit: In comments, just say "null". Done https://codereview.chromium.org/2479113002/diff/180001/components/search_engi... File components/search_engines/template_url_prepopulate_data.cc (right): https://codereview.chromium.org/2479113002/diff/180001/components/search_engi... components/search_engines/template_url_prepopulate_data.cc:1047: size_t default_index; On 2016/12/22 at 20:49:04, Peter Kasting wrote: > Nit: Probably should be done in a separate CL, but it seems like this isn't the only caller that doesn't really care about this value. Probably GetPrepopulatedEngines() should change to allow its second arg to be null for callers that don't care. Agree, it should be done, but in separate CL. https://codereview.chromium.org/2479113002/diff/180001/components/search_engi... components/search_engines/template_url_prepopulate_data.cc:1048: std::vector<std::unique_ptr<TemplateURLData>> engines = On 2016/12/22 at 20:49:04, Peter Kasting wrote: > Nit: Use auto Done https://codereview.chromium.org/2479113002/diff/180001/components/search_engi... components/search_engines/template_url_prepopulate_data.cc:1053: } On 2016/12/22 at 20:49:04, Peter Kasting wrote: > I tried writing this out with find_if(), but I don't know whether it's better in the end. It would probably be better with generic lambdas, but we don't support C++14. > > Up to you if you think this is worth using. > > auto Pred = [prepopulate_id](const std::unique_ptr<TemplateURLData>& data) { > return data->prepopulate_id == prepopulate_id; > }; > auto it = std::find_if(engines.begin(), engines.end(), Pred); > return it == engines.end() ? nullptr : std::move(*it); Old style is more clear here, IMHO. https://codereview.chromium.org/2479113002/diff/180001/components/search_engi... File components/search_engines/template_url_prepopulate_data.h (right): https://codereview.chromium.org/2479113002/diff/180001/components/search_engi... components/search_engines/template_url_prepopulate_data.h:45: // Find the prepopulated search engine with the given id. On 2016/12/22 at 20:49:04, Peter Kasting wrote: > Nit: Find -> Returns, id -> |prepopulate_id| Done https://codereview.chromium.org/2479113002/diff/180001/components/search_engi... File components/search_engines/template_url_service.cc (right): https://codereview.chromium.org/2479113002/diff/180001/components/search_engi... components/search_engines/template_url_service.cc:497: if (template_url_ptr) { On 2016/12/22 at 20:49:04, Peter Kasting wrote: > Nit: No {} Done https://codereview.chromium.org/2479113002/diff/180001/components/search_engi... components/search_engines/template_url_service.cc:519: KeywordWebDataService::BatchModeScoper scoper(web_data_service_.get()); On 2016/12/22 at 20:49:04, Peter Kasting wrote: > I don't think we want this scoper anymore? Removed
LGTM https://codereview.chromium.org/2479113002/diff/200001/chrome/browser/extensi... File chrome/browser/extensions/api/settings_overrides/settings_overrides_browsertest.cc (right): https://codereview.chromium.org/2479113002/diff/200001/chrome/browser/extensi... chrome/browser/extensions/api/settings_overrides/settings_overrides_browsertest.cc:48: std::unique_ptr<TemplateURLData> prep_engine( Nit: Prefer = to () for "simple" initialization like this; see https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Variab... . I might use "prepopulated" in place of "prep" in the name here to avoid confusion with e.g. "prepared" or "preparatory". https://codereview.chromium.org/2479113002/diff/200001/chrome/browser/extensi... chrome/browser/extensions/api/settings_overrides/settings_overrides_browsertest.cc:75: #if defined(OS_WIN) || defined(OS_MACOSX) Nit: It seems like at least three tests have this ifdef, all with the same #else block. Maybe instead we should do something like: #if defined(OS_WIN) || defined(OS_MACOSX) IN_PROC_BROWSER_TEST_F(ExtensionBrowserTest, OverrideHomePageSettings) { ... test body } ... more tests #else IN_PROC_BROWSER_TEST_F(ExtensionBrowserTest, SettingsOverridesDisallowed) { ... test that settings overrides don't work } #endif https://codereview.chromium.org/2479113002/diff/200001/chrome/browser/extensi... chrome/browser/extensions/api/settings_overrides/settings_overrides_browsertest.cc:214: // https://bugs.chromium.org/p/chromium/issues/detail?id=450534 Nit: Not everyone agrees with me, but I tend to omit bug references unless the reader really needs some additional context from the bug (and it can't be stated inline). In this case, I glanced at the bug, and it wasn't immediately obvious what specific information from there was relevant here. https://codereview.chromium.org/2479113002/diff/200001/chrome/browser/search_... File chrome/browser/search_engines/template_url_service_sync_unittest.cc (right): https://codereview.chromium.org/2479113002/diff/200001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_sync_unittest.cc:441: // Change default search provider to a extension one. Nit: a -> an https://codereview.chromium.org/2479113002/diff/200001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_sync_unittest.cc:457: Nit: Blank line not necessary https://codereview.chromium.org/2479113002/diff/200001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_sync_unittest.cc:461: iter != all_sync_data.end(); ++iter) { Nit: Range-based for? https://codereview.chromium.org/2479113002/diff/200001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_sync_unittest.cc:1758: EXPECT_EQ(1U, processor()->change_list_size()); Nit: Consider doing: const size_t pending_changes = processor()->change_list_size(); EXPECT_EQ(1U, pending_changes); This way, down below, you can compare to |pending_changes| instead of 1U to make it clearer the extension change should have left the existing number unaffected. The same comment applies to the "3U" for the number of affected engines. https://codereview.chromium.org/2479113002/diff/200001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_sync_unittest.cc:1766: // Change the default search provider to a extension one. Nit: a -> an https://codereview.chromium.org/2479113002/diff/200001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_sync_unittest.cc:1779: // Expect still one change because of user default engine change. Nit: Maybe just "Extension-related changes to the DSE should not be synced as search engine changes." plus the nit above about using temps instead of hardcoded values here would be clearer + eliminate the need for the comment just below. https://codereview.chromium.org/2479113002/diff/200001/chrome/browser/search_... File chrome/browser/search_engines/template_url_service_test_util.cc (right): https://codereview.chromium.org/2479113002/diff/200001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_test_util.cc:131: Nit: Not sure why this blank line here https://codereview.chromium.org/2479113002/diff/200001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_test_util.cc:164: TemplateURLData extension_data = extension_turl->data(); Nit: Why copy this off instead of using result->data() below? That would be more efficient. If for safety reasons, then see next comment. https://codereview.chromium.org/2479113002/diff/200001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_test_util.cc:167: if (wants_to_be_default) { Should this check |result| too? Seems like yes, unless for some reason we want to set the pref anyway if the model change fails. https://codereview.chromium.org/2479113002/diff/200001/chrome/browser/search_... File chrome/browser/search_engines/template_url_service_test_util.h (right): https://codereview.chromium.org/2479113002/diff/200001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_test_util.h:70: // pref in extension controlled preferences if extension wants to be default. Nit: This comment is missing articles ("an", "the", etc.) on a lot of things https://codereview.chromium.org/2479113002/diff/200001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_test_util.h:74: // Removes a TemplateURL controlled by |extension_id| from the model, and, Nit: Blank line above this https://codereview.chromium.org/2479113002/diff/200001/chrome/browser/search_... File chrome/browser/search_engines/template_url_service_unittest.cc (right): https://codereview.chromium.org/2479113002/diff/200001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_unittest.cc:1454: // Create non default extension search engine. Nit: non-default https://codereview.chromium.org/2479113002/diff/200001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_unittest.cc:1481: test_util()->ResetModel(false); Nit: Given the longer comment below about DSE persistence that gives this more context, I'd reorder this way: // A default search engine... [see comment rewrite suggestion below] test_util()->ResetModel(false); ... check DSE // Non-default extension engines are not persisted across restarts. EXPECT_FALSE(model()->GetTemplateURLForKeyword(ASCIIToUTF16("ext1"))); This puts the ResetModel() call under the comment that first talks about restarting, and puts the comment with more context first. https://codereview.chromium.org/2479113002/diff/200001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_unittest.cc:1486: // restarts, until extension is unloaded/disabled. Nit: How about "A default search engine set by an extension must be persisted across browser restarts, until the extension is unloaded/disabled." Also applies to the similar comment in the next test. https://codereview.chromium.org/2479113002/diff/200001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_unittest.cc:1509: // restarts, and is available before TemplateURLService load. Nit: is available before TemplateURLService load -> should be available before the TemplateURLService is loaded. https://codereview.chromium.org/2479113002/diff/200001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_unittest.cc:1513: // Load template url service. Nit: This comment restates the code. I'd replace it with the one just below, which really describes both these next statements. https://codereview.chromium.org/2479113002/diff/200001/components/prefs/testi... File components/prefs/testing_pref_service.h (right): https://codereview.chromium.org/2479113002/diff/200001/components/prefs/testi... components/prefs/testing_pref_service.h:45: // Useful in tests that only checks that preference is overriden by extension. Nit: "...only check that a preference is overridden by an extension". https://codereview.chromium.org/2479113002/diff/200001/components/search_engi... File components/search_engines/default_search_manager.cc (right): https://codereview.chromium.org/2479113002/diff/200001/components/search_engi... components/search_engines/default_search_manager.cc:257: extension_default_search_ = TemplateURLDataFromDictionary(*url_dict); Nit: Use |turl_data| here and below https://codereview.chromium.org/2479113002/diff/200001/components/search_engi... File components/search_engines/search_engines_test_util.h (right): https://codereview.chromium.org/2479113002/diff/200001/components/search_engi... components/search_engines/search_engines_test_util.h:16: // Generate TemplateURLData structure useful for tests filled with values Nit: Generate -> Generates a https://codereview.chromium.org/2479113002/diff/200001/components/search_engi... components/search_engines/search_engines_test_util.h:17: // autogenerated from provider_name param. Nit: provider_name param -> |provider_name| https://codereview.chromium.org/2479113002/diff/200001/components/search_engi... components/search_engines/search_engines_test_util.h:26: // Writes default search engine |extension_data| into extension controlled Nit: extension controlled -> the extension-controlled default search https://codereview.chromium.org/2479113002/diff/200001/components/search_engi... components/search_engines/search_engines_test_util.h:27: // preference in test pref service. Nit: test pref service -> |prefs| (2 places) https://codereview.chromium.org/2479113002/diff/200001/components/search_engi... components/search_engines/search_engines_test_util.h:32: // Removes extension controlled default search engine preference from test Nit: Removes extension controlled -> Removes the extension-controlled https://codereview.chromium.org/2479113002/diff/200001/components/search_engi... File components/search_engines/template_url.cc (right): https://codereview.chromium.org/2479113002/diff/200001/components/search_engi... components/search_engines/template_url.cc:1272: if (search_terms_replacement_key1 == search_terms_replacement_key2) Nit: If you move this to the end of the conditions, you can rewrite as "return search_terms_replacement_key1 == search_terms_replacement_key2;". https://codereview.chromium.org/2479113002/diff/200001/components/search_engi... File components/search_engines/template_url.h (right): https://codereview.chromium.org/2479113002/diff/200001/components/search_engi... components/search_engines/template_url.h:535: // its special initialization in TemplateUrl constructor. Nit: TemplateUrl -> TemplateURL https://codereview.chromium.org/2479113002/diff/200001/components/search_engi... components/search_engines/template_url.h:622: const AssociatedExtensionInfo* GetExtensionInfoForTesting() { Nit: Can be const https://codereview.chromium.org/2479113002/diff/200001/components/search_engi... File components/search_engines/template_url_prepopulate_data.cc (right): https://codereview.chromium.org/2479113002/diff/200001/components/search_engi... components/search_engines/template_url_prepopulate_data.cc:1044: // Find the prepopulated search engine with the given id. Nit: Omit this comment, since it's already stated in the header.
https://codereview.chromium.org/2479113002/diff/200001/chrome/browser/extensi... File chrome/browser/extensions/api/settings_overrides/settings_overrides_browsertest.cc (right): https://codereview.chromium.org/2479113002/diff/200001/chrome/browser/extensi... chrome/browser/extensions/api/settings_overrides/settings_overrides_browsertest.cc:48: std::unique_ptr<TemplateURLData> prep_engine( On 2017/01/06 at 01:44:57, Peter Kasting (backlogged) wrote: > Nit: Prefer = to () for "simple" initialization like this; see https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Variab... . > > I might use "prepopulated" in place of "prep" in the name here to avoid confusion with e.g. "prepared" or "preparatory". Done https://codereview.chromium.org/2479113002/diff/200001/chrome/browser/extensi... chrome/browser/extensions/api/settings_overrides/settings_overrides_browsertest.cc:75: #if defined(OS_WIN) || defined(OS_MACOSX) On 2017/01/06 at 01:44:57, Peter Kasting (backlogged) wrote: > Nit: It seems like at least three tests have this ifdef, all with the same #else block. > > Maybe instead we should do something like: > > #if defined(OS_WIN) || defined(OS_MACOSX) > IN_PROC_BROWSER_TEST_F(ExtensionBrowserTest, OverrideHomePageSettings) { > ... test body > } > > ... more tests > #else > IN_PROC_BROWSER_TEST_F(ExtensionBrowserTest, SettingsOverridesDisallowed) { > ... test that settings overrides don't work > } > #endif Good idea, done. https://codereview.chromium.org/2479113002/diff/200001/chrome/browser/extensi... chrome/browser/extensions/api/settings_overrides/settings_overrides_browsertest.cc:214: // https://bugs.chromium.org/p/chromium/issues/detail?id=450534 On 2017/01/06 at 01:44:57, Peter Kasting (backlogged) wrote: > Nit: Not everyone agrees with me, but I tend to omit bug references unless the reader really needs some additional context from the bug (and it can't be stated inline). In this case, I glanced at the bug, and it wasn't immediately obvious what specific information from there was relevant here. Removed bug link. That bug description is not really informative. https://codereview.chromium.org/2479113002/diff/200001/chrome/browser/search_... File chrome/browser/search_engines/template_url_service_sync_unittest.cc (right): https://codereview.chromium.org/2479113002/diff/200001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_sync_unittest.cc:441: // Change default search provider to a extension one. On 2017/01/06 at 01:44:57, Peter Kasting (backlogged) wrote: > Nit: a -> an My mistake, done. https://codereview.chromium.org/2479113002/diff/200001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_sync_unittest.cc:457: On 2017/01/06 at 01:44:57, Peter Kasting (backlogged) wrote: > Nit: Blank line not necessary Removed https://codereview.chromium.org/2479113002/diff/200001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_sync_unittest.cc:461: iter != all_sync_data.end(); ++iter) { On 2017/01/06 at 01:44:57, Peter Kasting (backlogged) wrote: > Nit: Range-based for? Done https://codereview.chromium.org/2479113002/diff/200001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_sync_unittest.cc:1758: EXPECT_EQ(1U, processor()->change_list_size()); On 2017/01/06 at 01:44:57, Peter Kasting (backlogged) wrote: > Nit: Consider doing: > > const size_t pending_changes = processor()->change_list_size(); > EXPECT_EQ(1U, pending_changes); > > This way, down below, you can compare to |pending_changes| instead of 1U to make it clearer the extension change should have left the existing number unaffected. The same comment applies to the "3U" for the number of affected engines. Done. https://codereview.chromium.org/2479113002/diff/200001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_sync_unittest.cc:1766: // Change the default search provider to a extension one. On 2017/01/06 at 01:44:57, Peter Kasting (backlogged) wrote: > Nit: a -> an Done. https://codereview.chromium.org/2479113002/diff/200001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_sync_unittest.cc:1779: // Expect still one change because of user default engine change. On 2017/01/06 at 01:44:57, Peter Kasting (backlogged) wrote: > Nit: Maybe just "Extension-related changes to the DSE should not be synced as search engine changes." plus the nit above about using temps instead of hardcoded values here would be clearer + eliminate the need for the comment just below. Done. https://codereview.chromium.org/2479113002/diff/200001/chrome/browser/search_... File chrome/browser/search_engines/template_url_service_test_util.cc (right): https://codereview.chromium.org/2479113002/diff/200001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_test_util.cc:131: On 2017/01/06 at 01:44:57, Peter Kasting (backlogged) wrote: > Nit: Not sure why this blank line here Removed. https://codereview.chromium.org/2479113002/diff/200001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_test_util.cc:164: TemplateURLData extension_data = extension_turl->data(); On 2017/01/06 at 01:44:57, Peter Kasting (backlogged) wrote: > Nit: Why copy this off instead of using result->data() below? That would be more efficient. If for safety reasons, then see next comment. Thanks, fixed. https://codereview.chromium.org/2479113002/diff/200001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_test_util.cc:167: if (wants_to_be_default) { On 2017/01/06 at 01:44:57, Peter Kasting (backlogged) wrote: > Should this check |result| too? Seems like yes, unless for some reason we want to set the pref anyway if the model change fails. Added check. https://codereview.chromium.org/2479113002/diff/200001/chrome/browser/search_... File chrome/browser/search_engines/template_url_service_test_util.h (right): https://codereview.chromium.org/2479113002/diff/200001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_test_util.h:70: // pref in extension controlled preferences if extension wants to be default. On 2017/01/06 at 01:44:57, Peter Kasting (backlogged) wrote: > Nit: This comment is missing articles ("an", "the", etc.) on a lot of things Tried to fix, hope I get it right. https://codereview.chromium.org/2479113002/diff/200001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_test_util.h:74: // Removes a TemplateURL controlled by |extension_id| from the model, and, On 2017/01/06 at 01:44:57, Peter Kasting (backlogged) wrote: > Nit: Blank line above this Done. https://codereview.chromium.org/2479113002/diff/200001/chrome/browser/search_... File chrome/browser/search_engines/template_url_service_unittest.cc (right): https://codereview.chromium.org/2479113002/diff/200001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_unittest.cc:1454: // Create non default extension search engine. On 2017/01/06 at 01:44:57, Peter Kasting (backlogged) wrote: > Nit: non-default Fixed. Thanks. https://codereview.chromium.org/2479113002/diff/200001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_unittest.cc:1481: test_util()->ResetModel(false); On 2017/01/06 at 01:44:57, Peter Kasting (backlogged) wrote: > Nit: Given the longer comment below about DSE persistence that gives this more context, I'd reorder this way: > > // A default search engine... [see comment rewrite suggestion below] > test_util()->ResetModel(false); > ... check DSE > > // Non-default extension engines are not persisted across restarts. > EXPECT_FALSE(model()->GetTemplateURLForKeyword(ASCIIToUTF16("ext1"))); > > This puts the ResetModel() call under the comment that first talks about restarting, and puts the comment with more context first. Fixed, as understood. https://codereview.chromium.org/2479113002/diff/200001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_unittest.cc:1486: // restarts, until extension is unloaded/disabled. On 2017/01/06 at 01:44:57, Peter Kasting (backlogged) wrote: > Nit: How about "A default search engine set by an extension must be persisted across browser restarts, until the extension is unloaded/disabled." > > Also applies to the similar comment in the next test. Fixed, thanks. https://codereview.chromium.org/2479113002/diff/200001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_unittest.cc:1509: // restarts, and is available before TemplateURLService load. On 2017/01/06 at 01:44:57, Peter Kasting (backlogged) wrote: > Nit: is available before TemplateURLService load -> should be available before the TemplateURLService is loaded. Done. https://codereview.chromium.org/2479113002/diff/200001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_unittest.cc:1513: // Load template url service. On 2017/01/06 at 01:44:57, Peter Kasting (backlogged) wrote: > Nit: This comment restates the code. I'd replace it with the one just below, which really describes both these next statements. Done. https://codereview.chromium.org/2479113002/diff/200001/components/prefs/testi... File components/prefs/testing_pref_service.h (right): https://codereview.chromium.org/2479113002/diff/200001/components/prefs/testi... components/prefs/testing_pref_service.h:45: // Useful in tests that only checks that preference is overriden by extension. On 2017/01/06 at 01:44:57, Peter Kasting (backlogged) wrote: > Nit: "...only check that a preference is overridden by an extension". Done, thanks. https://codereview.chromium.org/2479113002/diff/200001/components/search_engi... File components/search_engines/default_search_manager.cc (right): https://codereview.chromium.org/2479113002/diff/200001/components/search_engi... components/search_engines/default_search_manager.cc:257: extension_default_search_ = TemplateURLDataFromDictionary(*url_dict); On 2017/01/06 at 01:44:57, Peter Kasting (backlogged) wrote: > Nit: Use |turl_data| here and below Fixed, good shot. https://codereview.chromium.org/2479113002/diff/200001/components/search_engi... File components/search_engines/search_engines_test_util.h (right): https://codereview.chromium.org/2479113002/diff/200001/components/search_engi... components/search_engines/search_engines_test_util.h:16: // Generate TemplateURLData structure useful for tests filled with values On 2017/01/06 at 01:44:57, Peter Kasting (backlogged) wrote: > Nit: Generate -> Generates a Fixed. https://codereview.chromium.org/2479113002/diff/200001/components/search_engi... components/search_engines/search_engines_test_util.h:17: // autogenerated from provider_name param. On 2017/01/06 at 01:44:58, Peter Kasting (backlogged) wrote: > Nit: provider_name param -> |provider_name| Done. https://codereview.chromium.org/2479113002/diff/200001/components/search_engi... components/search_engines/search_engines_test_util.h:26: // Writes default search engine |extension_data| into extension controlled On 2017/01/06 at 01:44:58, Peter Kasting (backlogged) wrote: > Nit: extension controlled -> the extension-controlled default search Fixed. https://codereview.chromium.org/2479113002/diff/200001/components/search_engi... components/search_engines/search_engines_test_util.h:27: // preference in test pref service. On 2017/01/06 at 01:44:58, Peter Kasting (backlogged) wrote: > Nit: test pref service -> |prefs| (2 places) Fixed. https://codereview.chromium.org/2479113002/diff/200001/components/search_engi... components/search_engines/search_engines_test_util.h:32: // Removes extension controlled default search engine preference from test On 2017/01/06 at 01:44:57, Peter Kasting (backlogged) wrote: > Nit: Removes extension controlled -> Removes the extension-controlled Fixed. https://codereview.chromium.org/2479113002/diff/200001/components/search_engi... File components/search_engines/template_url.cc (right): https://codereview.chromium.org/2479113002/diff/200001/components/search_engi... components/search_engines/template_url.cc:1272: if (search_terms_replacement_key1 == search_terms_replacement_key2) On 2017/01/06 at 01:44:58, Peter Kasting (backlogged) wrote: > Nit: If you move this to the end of the conditions, you can rewrite as "return search_terms_replacement_key1 == search_terms_replacement_key2;". Done, although I think, performance-wise, its the most common case and should be checked before exotic cases. https://codereview.chromium.org/2479113002/diff/200001/components/search_engi... File components/search_engines/template_url.h (right): https://codereview.chromium.org/2479113002/diff/200001/components/search_engi... components/search_engines/template_url.h:535: // its special initialization in TemplateUrl constructor. On 2017/01/06 at 01:44:58, Peter Kasting (backlogged) wrote: > Nit: TemplateUrl -> TemplateURL Done. https://codereview.chromium.org/2479113002/diff/200001/components/search_engi... components/search_engines/template_url.h:622: const AssociatedExtensionInfo* GetExtensionInfoForTesting() { On 2017/01/06 at 01:44:58, Peter Kasting (backlogged) wrote: > Nit: Can be const Done. https://codereview.chromium.org/2479113002/diff/200001/components/search_engi... File components/search_engines/template_url_prepopulate_data.cc (right): https://codereview.chromium.org/2479113002/diff/200001/components/search_engi... components/search_engines/template_url_prepopulate_data.cc:1044: // Find the prepopulated search engine with the given id. On 2017/01/06 at 01:44:58, Peter Kasting (backlogged) wrote: > Nit: Omit this comment, since it's already stated in the header. Fixed.
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 gab@chromium.org, vasilii@chromium.org, pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2479113002/#ps220001 (title: "Fixed after review, round 8")
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
Try jobs failed on following builders: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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 gab@chromium.org, pkasting@chromium.org, vasilii@chromium.org Link to the patchset: https://codereview.chromium.org/2479113002/#ps240001 (title: "Fixed linux compilation")
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
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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 gab@chromium.org, pkasting@chromium.org, vasilii@chromium.org Link to the patchset: https://codereview.chromium.org/2479113002/#ps260001 (title: "Fixed tests compilation")
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
On 2017/01/07 at 17:40:32, commit-bot wrote: > Try jobs failed on following builders: > linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) I have separated changes in TestingPrefService to single CL https://codereview.chromium.org/2615403002/ to keep current CL more manageable.
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: This issue passed the CQ dry run.
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 gab@chromium.org, pkasting@chromium.org, vasilii@chromium.org Link to the patchset: https://codereview.chromium.org/2479113002/#ps280001 (title: "Fixed tests compilation")
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": 280001, "attempt_start_ts": 1483963281444570,
"parent_rev": "d0da75e5f451ce330c3cc3c1448c624a88e5af1b", "commit_rev":
"f5f1407adee2b18b59b33fb8d74739dcc27d8c0b"}
Message was sent while issue was closed.
Description was changed from ========== Make extensions DSE persistent in browser prefs 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 ========== to ========== Make extensions DSE persistent in browser prefs 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} Committed: https://chromium.googlesource.com/chromium/src/+/f5f1407adee2b18b59b33fb8d747... ==========
Message was sent while issue was closed.
Committed patchset #15 (id:280001) as https://chromium.googlesource.com/chromium/src/+/f5f1407adee2b18b59b33fb8d747...
Message was sent while issue was closed.
A revert of this CL (patchset #15 id:280001) has been created in https://codereview.chromium.org/2623833005/ by foolip@chromium.org. The reason for reverting is: ExtensionBrowserTest.OverrideHomePageSettings and ExtensionBrowserTest.OverrideStartupPagesSettings are flaky. BUG=679470,679569 .
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: This issue passed the CQ dry run.
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: This issue passed the CQ dry run.
@vasillii, 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: This issue passed the CQ dry run.
On 2017/01/17 13:56:14, Alexander Yashkin wrote: > @vasillii, PTAL again Please try to avoid uploading new patchsets on CLs that landed once. It makes it extremely confusing later to figure out what actually landed when trying to trace through revert chains. Instead, create a new CL whose first patchset is the patch that originally landed. Then upload the new changes as a second patchset, so reviewers can easily diff against the original to review just the changes. It's OK to TBR reviewers that don't need to re-review in order to minimize the friction of relanding.
Message was sent while issue was closed.
On 2017/01/17 at 18:17:30, pkasting wrote: > On 2017/01/17 13:56:14, Alexander Yashkin wrote: > > @vasillii, PTAL again > > Please try to avoid uploading new patchsets on CLs that landed once. It makes it extremely confusing later to figure out what actually landed when trying to trace through revert chains. > > Instead, create a new CL whose first patchset is the patch that originally landed. Then upload the new changes as a second patchset, so reviewers can easily diff against the original to review just the changes. It's OK to TBR reviewers that don't need to re-review in order to minimize the friction of relanding. Created https://codereview.chromium.org/2639153002/, thanks for advice. |
