Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
229763005:
Store default search provider data in dictionary pref. (Closed)
|
Created:
6 years, 8 months ago by Cait (Slow) Modified:
6 years, 7 months ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionStore default search provider data in dictionary pref, and add DefaultSearchManager class to handle the reading and writing of this pref (DefaultSearchManager will eventually all default search related concerns).
This pref will be used to persist data about user-selected
default search providers. In future CLs, support will be added for default search providers set by policy and sync.
BUG=365762
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=266479
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=266615
R=gab@chromium.org, pkasting@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=266891
Patch Set 1 #Patch Set 2 : just pref changes #
Total comments: 8
Patch Set 3 : Clean up and Gab's comments #Patch Set 4 : Fix some dumb bugs #Patch Set 5 : Move constants to separate file #Patch Set 6 : put constants in a namespace #
Total comments: 9
Patch Set 7 : Make DSE Manager use TemplateURLData #
Total comments: 23
Patch Set 8 : Eriks comments and clean up #
Total comments: 67
Patch Set 9 : Address comments #
Total comments: 21
Patch Set 10 : More comments #
Total comments: 4
Patch Set 11 : Rebase on ToT #Patch Set 12 : Fix pref name #Patch Set 13 : Fix mem leak #
Total comments: 3
Patch Set 14 : Fix another mem leak #Patch Set 15 : ...and some nits #Messages
Total messages: 82 (0 generated)
Broad overview, don't want to jump into details at this point, but a few comments on how we probably want to approach various things in here. https://codereview.chromium.org/229763005/diff/20001/chrome/browser/search_en... File chrome/browser/search_engines/template_url_service.cc (right): https://codereview.chromium.org/229763005/diff/20001/chrome/browser/search_en... chrome/browser/search_engines/template_url_service.cc:65: const char kSyncGUID[] = "sync_guid"; Shouldn't this be "synced_guid"? https://codereview.chromium.org/229763005/diff/20001/chrome/browser/search_en... chrome/browser/search_engines/template_url_service.cc:2788: if (!prefs || !prefs->HasPrefPath(prefs::kDefaultSearchProviderData)) I don't think we want to check HasPrefPath() here. It will return false if there is only a default value available. Instead we want to let GetDictionary() below handle getting the default value if there is no user value (or whatever else there may be in the hierarchy of stores being checked). https://codereview.chromium.org/229763005/diff/20001/chrome/browser/search_en... File chrome/browser/search_engines/template_url_service_factory.cc (right): https://codereview.chromium.org/229763005/diff/20001/chrome/browser/search_en... chrome/browser/search_engines/template_url_service_factory.cc:127: registry->RegisterDictionaryPref( Add a TODO that we should register a default here rather than leave the empty dictionary as default.
https://codereview.chromium.org/229763005/diff/20001/chrome/browser/search_en... File chrome/browser/search_engines/template_url_service.cc (right): https://codereview.chromium.org/229763005/diff/20001/chrome/browser/search_en... chrome/browser/search_engines/template_url_service.cc:65: const char kSyncGUID[] = "sync_guid"; On 2014/04/11 20:27:46, gab wrote: > Shouldn't this be "synced_guid"? Done. https://codereview.chromium.org/229763005/diff/20001/chrome/browser/search_en... chrome/browser/search_engines/template_url_service.cc:2788: if (!prefs || !prefs->HasPrefPath(prefs::kDefaultSearchProviderData)) On 2014/04/11 20:27:46, gab wrote: > I don't think we want to check HasPrefPath() here. > > It will return false if there is only a default value available. > > Instead we want to let GetDictionary() below handle getting the default value if > there is no user value (or whatever else there may be in the hierarchy of stores > being checked). Done. https://codereview.chromium.org/229763005/diff/20001/chrome/browser/search_en... File chrome/browser/search_engines/template_url_service_factory.cc (right): https://codereview.chromium.org/229763005/diff/20001/chrome/browser/search_en... chrome/browser/search_engines/template_url_service_factory.cc:127: registry->RegisterDictionaryPref( On 2014/04/11 20:27:46, gab wrote: > Add a TODO that we should register a default here rather than leave the empty > dictionary as default. Done.
https://codereview.chromium.org/229763005/diff/20001/chrome/browser/search_en... File chrome/browser/search_engines/template_url_service.cc (right): https://codereview.chromium.org/229763005/diff/20001/chrome/browser/search_en... chrome/browser/search_engines/template_url_service.cc:65: const char kSyncGUID[] = "sync_guid"; Actually, looking closer at this, TemplateURLData calls this field sync_guid and I think for the time being it makes sense to keep our dictionary keys as close to TemplateURLData's member variable names as possible, for consistency reasons -- how about a TODO to update all of them at a later date? On 2014/04/14 15:20:35, Cait Phillips wrote: > On 2014/04/11 20:27:46, gab wrote: > > Shouldn't this be "synced_guid"? > > Done.
https://codereview.chromium.org/229763005/diff/20001/chrome/browser/search_en... File chrome/browser/search_engines/template_url_service.cc (right): https://codereview.chromium.org/229763005/diff/20001/chrome/browser/search_en... chrome/browser/search_engines/template_url_service.cc:65: const char kSyncGUID[] = "sync_guid"; On 2014/04/14 15:28:01, Cait Phillips wrote: > Actually, looking closer at this, TemplateURLData calls this field sync_guid and > I think for the time being it makes sense to keep our dictionary keys as close > to TemplateURLData's member variable names as possible, for consistency reasons > -- how about a TODO to update all of them at a later date? > > On 2014/04/14 15:20:35, Cait Phillips wrote: > > On 2014/04/11 20:27:46, gab wrote: > > > Shouldn't this be "synced_guid"? > > > > Done. > Ah ok, then let's keep it as "sync_guid". No need for a TODO; synced_guid will be a legacy pref in M36 and removed a few milestones later.
https://codereview.chromium.org/229763005/diff/90001/chrome/browser/search_en... File chrome/browser/search_engines/template_url_service.cc (right): https://codereview.chromium.org/229763005/diff/90001/chrome/browser/search_en... chrome/browser/search_engines/template_url_service.cc:2857: url_dict->SetString(default_search::kID, base::Int64ToString(url->id())); Can you push this latest update to argon18? I can't compile without it :(.
https://codereview.chromium.org/229763005/diff/90001/chrome/browser/search_en... File chrome/browser/search_engines/template_url_service.cc (left): https://codereview.chromium.org/229763005/diff/90001/chrome/browser/search_en... chrome/browser/search_engines/template_url_service.cc:59: nit: restore blank line. https://codereview.chromium.org/229763005/diff/90001/chrome/browser/search_en... File chrome/browser/search_engines/template_url_service.cc (right): https://codereview.chromium.org/229763005/diff/90001/chrome/browser/search_en... chrome/browser/search_engines/template_url_service.cc:2894: url_dict->Set(default_search::kAlternateURLs, &alternate_urls); Doesn't Set() take ownership of alternate_urls? https://codereview.chromium.org/229763005/diff/90001/chrome/browser/search_en... File chrome/browser/search_engines/template_url_service_factory.cc (right): https://codereview.chromium.org/229763005/diff/90001/chrome/browser/search_en... chrome/browser/search_engines/template_url_service_factory.cc:127: // TODO(caitkp): Set a default value here. In fact, the default should simply be NULL based on our subsequent discussions. https://codereview.chromium.org/229763005/diff/90001/chrome/browser/search_en... File chrome/browser/search_engines/template_url_service_unittest.cc (right): https://codereview.chromium.org/229763005/diff/90001/chrome/browser/search_en... chrome/browser/search_engines/template_url_service_unittest.cc:1699: } Are there not some "is equivalent TemplateURL" methods that could be used here, too?
https://codereview.chromium.org/229763005/diff/90001/chrome/browser/search_en... File chrome/browser/search_engines/template_url_service.cc (left): https://codereview.chromium.org/229763005/diff/90001/chrome/browser/search_en... chrome/browser/search_engines/template_url_service.cc:59: On 2014/04/22 15:47:24, erikwright wrote: > nit: restore blank line. Done. https://codereview.chromium.org/229763005/diff/90001/chrome/browser/search_en... File chrome/browser/search_engines/template_url_service.cc (right): https://codereview.chromium.org/229763005/diff/90001/chrome/browser/search_en... chrome/browser/search_engines/template_url_service.cc:2894: url_dict->Set(default_search::kAlternateURLs, &alternate_urls); On 2014/04/22 15:47:24, erikwright wrote: > Doesn't Set() take ownership of alternate_urls? Done. https://codereview.chromium.org/229763005/diff/90001/chrome/browser/search_en... File chrome/browser/search_engines/template_url_service_factory.cc (right): https://codereview.chromium.org/229763005/diff/90001/chrome/browser/search_en... chrome/browser/search_engines/template_url_service_factory.cc:127: // TODO(caitkp): Set a default value here. On 2014/04/22 15:47:24, erikwright wrote: > In fact, the default should simply be NULL based on our subsequent discussions. Done. https://codereview.chromium.org/229763005/diff/90001/chrome/browser/search_en... File chrome/browser/search_engines/template_url_service_unittest.cc (right): https://codereview.chromium.org/229763005/diff/90001/chrome/browser/search_en... chrome/browser/search_engines/template_url_service_unittest.cc:1699: } On 2014/04/22 15:47:24, erikwright wrote: > Are there not some "is equivalent TemplateURL" methods that could be used here, > too? ExpectSimilar is what we want here I think.
https://codereview.chromium.org/229763005/diff/130001/chrome/browser/search_e... File chrome/browser/search_engines/default_search_manager.h (right): https://codereview.chromium.org/229763005/diff/130001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager.h:16: DefaultSearchManager(); I think this constructor should take a PrefService*. It doesn't make sense to maintain state if we are not tied to a single PrefService (what would it mean to receive different PS pointers to subsequent calls to the various methods?). https://codereview.chromium.org/229763005/diff/130001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager.h:20: TemplateURLData* url); const ref TUData? https://codereview.chromium.org/229763005/diff/130001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager.h:23: TemplateURLData* GetDefaultSearchProvider(); return const-ref? https://codereview.chromium.org/229763005/diff/130001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager.h:26: void SetDefaultSearchProvider(TemplateURLData* url); const-ref? https://codereview.chromium.org/229763005/diff/130001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager.h:29: bool ReadFromPrefService(PrefService* prefs, TemplateURLData* url); With the existing implementation there is a concept of an "explicitly NULL" DSE. i.e., the user has said there is none, so the default should not be used. To support that, presumably this method could return true while leaving url NULL, right? If so, please document. Also note in comments that |url| will (presumably) remain owned by this class and may be invalidated by subsequent changes in the pref service. https://codereview.chromium.org/229763005/diff/130001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager.h:29: bool ReadFromPrefService(PrefService* prefs, TemplateURLData* url); I don't think there's a good reason to have separate methods for doing this. Just take the PS in the constructor, and every Get/Set will be from the PS. It's no big deal to construct one of these and then use it just to do a single read. One can even do: bool result = DefaultSearchManager(pref_service).ReadFromPrefService(&url); https://codereview.chromium.org/229763005/diff/130001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager.h:32: void SaveToPrefService(PrefService* prefs, const TemplateURLData* data); const-ref? https://codereview.chromium.org/229763005/diff/130001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager.h:35: TemplateURLData* default_search_provider_; scoped_ptr? https://codereview.chromium.org/229763005/diff/130001/chrome/browser/search_e... File chrome/browser/search_engines/default_search_manager_unittest.cc (right): https://codereview.chromium.org/229763005/diff/130001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager_unittest.cc:21: ASSERT_TRUE(expected != NULL); Don't use ASSERT in a helper function. Many developers do not immediately understand that it returns on failure, so the fact that it _must_ be an ASSERT (not an EXPECT) here is not self-evident. Use two EXPECTs and an if (...) return; instead. https://codereview.chromium.org/229763005/diff/130001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager_unittest.cc:45: profile_.reset(new TestingProfile(temp_dir_.path())); I believe there is also a TestingPrefService which would serve your needs at a lower level. https://codereview.chromium.org/229763005/diff/130001/chrome/browser/search_e... File chrome/browser/search_engines/template_url_service_factory.cc (right): https://codereview.chromium.org/229763005/diff/130001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_factory.cc:128: prefs::kDefaultSearchProviderData, DefaultSearchManager::RegisterProfilePrefs(registry); https://codereview.chromium.org/229763005/diff/130001/chrome/common/pref_names.h File chrome/common/pref_names.h (right): https://codereview.chromium.org/229763005/diff/130001/chrome/common/pref_name... chrome/common/pref_names.h:144: extern const char kDefaultSearchProviderData[]; I would declare and use this exclusively in default_search_manager.cc . That's one way to prevent others from abusing it as an API.
Erik, a couple questions about handling NULL DSEs (see inline). https://codereview.chromium.org/229763005/diff/130001/chrome/browser/search_e... File chrome/browser/search_engines/default_search_manager.h (right): https://codereview.chromium.org/229763005/diff/130001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager.h:29: bool ReadFromPrefService(PrefService* prefs, TemplateURLData* url); On 2014/04/23 14:18:01, erikwright wrote: > With the existing implementation there is a concept of an "explicitly NULL" DSE. > i.e., the user has said there is none, so the default should not be used. > > To support that, presumably this method could return true while leaving url > NULL, right? If so, please document. > > Also note in comments that |url| will (presumably) remain owned by this class > and may be invalidated by subsequent changes in the pref service Hmm.. I'm not sure what the best way to handle the "DSE was explicitly set to NULL" case is if we're reading from prefs every time (can Prefs differentiate between "this value was never set" and "this value was explicitly set to null"?) Also, if we're using a const ref to pass the TUData to this class to save the data to prefs, we *can't* pass a null ref. Do we really think there is a need to support setting the DSE to NULL? Certainly in the UI at least, there is no way to explicitly do this. In the existing code, it seems to be used primarily as a signal that we need to find a new DSE from somewhere. WDYT?
On 2014/04/23 17:26:29, Cait Phillips wrote: > Erik, a couple questions about handling NULL DSEs (see inline). > > https://codereview.chromium.org/229763005/diff/130001/chrome/browser/search_e... > File chrome/browser/search_engines/default_search_manager.h (right): > > https://codereview.chromium.org/229763005/diff/130001/chrome/browser/search_e... > chrome/browser/search_engines/default_search_manager.h:29: bool > ReadFromPrefService(PrefService* prefs, TemplateURLData* url); > On 2014/04/23 14:18:01, erikwright wrote: > > With the existing implementation there is a concept of an "explicitly NULL" > DSE. > > i.e., the user has said there is none, so the default should not be used. > > > > To support that, presumably this method could return true while leaving url > > NULL, right? If so, please document. > > > > Also note in comments that |url| will (presumably) remain owned by this class > > and may be invalidated by subsequent changes in the pref service > > Hmm.. I'm not sure what the best way to handle the "DSE was explicitly set to > NULL" case is if we're reading from prefs every time (can Prefs differentiate > between "this value was never set" and "this value was explicitly set to null"?) > > > Also, if we're using a const ref to pass the TUData to this class to save the > data to prefs, we *can't* pass a null ref. > > Do we really think there is a need to support setting the DSE to NULL? Certainly > in the UI at least, there is no way to explicitly do this. In the existing code, > it seems to be used primarily as a signal that we need to find a new DSE from > somewhere. WDYT? If it were to be supported, you would need to actually store some property that said "the user explicitly set this to NULL." Since it's a dictionary, I suppose it would be sufficient to just store an empty dict. You're right, this would either require keeping the pointer-based API or having a different method (Disable DSE?) for explicitly saying "no DSE." I'm fine not handling this for now - as we are refactoring, presumably we will discover the need for it when we try to rewrite some code that was previously setting the DSE to "explicitly NULL."
https://codereview.chromium.org/229763005/diff/130001/chrome/browser/search_e... File chrome/browser/search_engines/default_search_manager.h (right): https://codereview.chromium.org/229763005/diff/130001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager.h:16: DefaultSearchManager(); On 2014/04/23 14:18:01, erikwright wrote: > I think this constructor should take a PrefService*. It doesn't make sense to > maintain state if we are not tied to a single PrefService (what would it mean to > receive different PS pointers to subsequent calls to the various methods?). Done. https://codereview.chromium.org/229763005/diff/130001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager.h:20: TemplateURLData* url); On 2014/04/23 14:18:01, erikwright wrote: > const ref TUData? Done. https://codereview.chromium.org/229763005/diff/130001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager.h:23: TemplateURLData* GetDefaultSearchProvider(); On 2014/04/23 14:18:01, erikwright wrote: > return const-ref? Done. https://codereview.chromium.org/229763005/diff/130001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager.h:26: void SetDefaultSearchProvider(TemplateURLData* url); On 2014/04/23 14:18:01, erikwright wrote: > const-ref? Done. https://codereview.chromium.org/229763005/diff/130001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager.h:32: void SaveToPrefService(PrefService* prefs, const TemplateURLData* data); On 2014/04/23 14:18:01, erikwright wrote: > const-ref? Done. https://codereview.chromium.org/229763005/diff/130001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager.h:35: TemplateURLData* default_search_provider_; On 2014/04/23 14:18:01, erikwright wrote: > scoped_ptr? Removing this altogether until there is an explicit need for it. https://codereview.chromium.org/229763005/diff/130001/chrome/browser/search_e... File chrome/browser/search_engines/default_search_manager_unittest.cc (right): https://codereview.chromium.org/229763005/diff/130001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager_unittest.cc:21: ASSERT_TRUE(expected != NULL); On 2014/04/23 14:18:01, erikwright wrote: > Don't use ASSERT in a helper function. Many developers do not immediately > understand that it returns on failure, so the fact that it _must_ be an ASSERT > (not an EXPECT) here is not self-evident. > > Use two EXPECTs and an if (...) return; instead. Done. https://codereview.chromium.org/229763005/diff/130001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager_unittest.cc:45: profile_.reset(new TestingProfile(temp_dir_.path())); On 2014/04/23 14:18:01, erikwright wrote: > I believe there is also a TestingPrefService which would serve your needs at a > lower level. Done. https://codereview.chromium.org/229763005/diff/130001/chrome/browser/search_e... File chrome/browser/search_engines/template_url_service_factory.cc (right): https://codereview.chromium.org/229763005/diff/130001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_factory.cc:128: prefs::kDefaultSearchProviderData, On 2014/04/23 14:18:01, erikwright wrote: > DefaultSearchManager::RegisterProfilePrefs(registry); Done. https://codereview.chromium.org/229763005/diff/130001/chrome/common/pref_names.h File chrome/common/pref_names.h (right): https://codereview.chromium.org/229763005/diff/130001/chrome/common/pref_name... chrome/common/pref_names.h:144: extern const char kDefaultSearchProviderData[]; On 2014/04/23 14:18:01, erikwright wrote: > I would declare and use this exclusively in default_search_manager.cc . That's > one way to prevent others from abusing it as an API. Done.
Peter: PTAL -- Thanks!
Please update CL description to mention the new separation of concerns introduced by DefaultSearchManager.
lg, comments below, mostly about prefs. Cheers! Gab https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... File chrome/browser/search_engines/default_search_manager.cc (right): https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager.cc:51: return false; Should this even be allowed by the constructor? https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager.cc:52: } rm {} https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager.cc:56: return false; This is not possible; PrefService always returns a dictionary (the default one at registration -- an empty dict in this case -- if no other is available). https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager.cc:73: base::StringToInt64(id, &data->id); see PrefService::GetInt64 https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager.cc:114: for (size_t i = 0; i < alternate_urls->GetSize(); ++i) { Use ListValue::const_iterator here. https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager.cc:123: for (size_t i = 0; i < encodings->GetSize(); ++i) { ListValue::const_iterator here as well. https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager.cc:145: url_dict->SetString(default_search::kID, base::Int64ToString(data.id)); USe PrefService::SetInt64() https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager.cc:179: for (size_t i = 0; i < data.alternate_urls.size(); ++i) std::vector<std::string>::const_iterator https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager.cc:184: for (size_t i = 0; i < data.input_encodings.size(); ++i) std::vector<std::string>::const_iterator https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... File chrome/browser/search_engines/default_search_manager.h (right): https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager.h:26: bool ReadFromPrefService(TemplateURLData* url); I think the fact that it reads/saves to PrefService is an implementation detail. I would just call these methods. GetDefaultSearchEngineData() and SaveUserSelectedDefaultSearchEngineData(). https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... File chrome/browser/search_engines/default_search_manager_unittest.cc (right): https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager_unittest.cc:26: return; optional: You can just use ASSERT_TRUE for the NULL checks above and not need this extra logic here. https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager_unittest.cc:57: scoped_ptr<TestingPrefServiceSyncable> pref_service_; I think it's fine to leave this member in the protected section of your test fixture and remove the Prefs() accessor method. https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager_unittest.cc:80: } Also test reading with no prior writes (i.e. reading default values). https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... File chrome/browser/search_engines/template_url_service.cc (right): https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service.cc:315: default_search_manager_.reset(new DefaultSearchManager(GetPrefs())); Initialize in initializer lists rather than in the body (here and in other constructors). https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... File chrome/browser/search_engines/template_url_service_factory.cc (right): https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_factory.cc:128: DefaultSearchManager::RegisterProfilePrefs(registry); I would prefer having this call at the top before the individual prefs for this service are registered (i.e. delegate some work first and do remaining work after).
https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... File chrome/browser/search_engines/default_search_manager.cc (right): https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager.cc:51: return false; On 2014/04/23 20:00:18, gab wrote: > Should this even be allowed by the constructor? I agree; the constructor should DCHECK this and the rest of the code should just rely on it being set. Combined with your comment below, this means this function can return a TemplateURLData object directly instead of via outparam, since the bool will no longer be needed. https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager.cc:60: base::string16 keyword; Nit: Reverse declaration order to match usage order below https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager.cc:61: Nit: Eliminate this blank line and the next two https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager.cc:66: DCHECK(!search_url.empty()); Couldn't these DCHECKs fail if the user has mucked with their preferences file? https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... File chrome/browser/search_engines/default_search_manager_unittest.cc (right): https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager_unittest.cc:26: return; On 2014/04/23 20:00:18, gab wrote: > optional: You can just use ASSERT_TRUE for the NULL checks above and not need > this extra logic here. I would echo this except without the "optional" prefix :) https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager_unittest.cc:54: PrefService* Prefs() const { return pref_service_.get(); } This must be non-const or else return a const pointer. Nit: Name this getter "pref_service()". https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager_unittest.cc:57: scoped_ptr<TestingPrefServiceSyncable> pref_service_; On 2014/04/23 20:00:18, gab wrote: > I think it's fine to leave this member in the protected section of your test > fixture and remove the Prefs() accessor method. That would violate the Google style guide, which bans non-private class data members and doesn't make an exception for tests. https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... File chrome/browser/search_engines/template_url_keys.h (right): https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... chrome/browser/search_engines/template_url_keys.h:10: namespace default_search { Using a namespace for this, especially one that doesn't match anything in the file/path name of the containing file, is unusual. I'd prefer to do something more in keeping with typical Chrome practice. What this would be depends on where you intend to use these. Right now, they're only used in default_search_manager.cc, and could thus be in an anonymous namespace atop that file. Going forward, if you intend to use them elsewhere, they could perhaps be public members of DefaultSearchManager. https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... File chrome/browser/search_engines/template_url_service.h (right): https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service.h:780: scoped_ptr<DefaultSearchManager> default_search_manager_; Why is this a scoped_ptr<> instead of a straight data member?
https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... File chrome/browser/search_engines/default_search_manager.cc (right): https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. fix date, remove (c) https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager.cc:27: // A dictionary to hold all data related to the Default Search Engine. blank line before https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager.cc:51: return false; On 2014/04/23 20:00:18, gab wrote: > Should this even be allowed by the constructor? Agreed. DCHECK in ctor. https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager.cc:56: return false; On 2014/04/23 20:00:18, gab wrote: > This is not possible; PrefService always returns a dictionary (the default one > at registration -- an empty dict in this case -- if no other is available). Right. I guess you will have to treat an empty dictionary as undefined. https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager.cc:73: base::StringToInt64(id, &data->id); On 2014/04/23 20:00:18, gab wrote: > see PrefService::GetInt64 She's working with a dict, not a PrefService, here. https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager.cc:145: url_dict->SetString(default_search::kID, base::Int64ToString(data.id)); On 2014/04/23 20:00:18, gab wrote: > USe PrefService::SetInt64() not a PrefService.. https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... File chrome/browser/search_engines/default_search_manager.h (right): https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager.h:8: #include "base/basictypes.h" replace with base/macros.h https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager.h:17: class DefaultSearchManager { Some comments are required here. https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager.h:26: bool ReadFromPrefService(TemplateURLData* url); On 2014/04/23 20:00:18, gab wrote: > I think the fact that it reads/saves to PrefService is an implementation detail. > I would just call these methods. > > GetDefaultSearchEngineData() and SaveUserSelectedDefaultSearchEngineData(). Agreed, but Save -> Set, and I don't think Data is required. https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... File chrome/browser/search_engines/default_search_manager_unittest.cc (right): https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager_unittest.cc:26: return; On 2014/04/23 20:00:18, gab wrote: > optional: You can just use ASSERT_TRUE for the NULL checks above and not need > this extra logic here. I asked her not to. Many developers are not aware that ASSERT does a return on failure. As such, there's a risk of this later being replaced with an assert. I believe ASSERT should only be used directly in test cases. https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager_unittest.cc:52: virtual void TearDown() { pref_service_.reset(); } Not necessary. There is one test instance per test case execution.
https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... File chrome/browser/search_engines/default_search_manager.cc (right): https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager.cc:73: base::StringToInt64(id, &data->id); On 2014/04/23 20:48:53, erikwright wrote: > On 2014/04/23 20:00:18, gab wrote: > > see PrefService::GetInt64 > > She's working with a dict, not a PrefService, here. Duh, of course, this is annoying. DictionaryValue should be augmented to support this IMO, but that's another story... (this is not the first piece of code doing this custom logic for int64s).
https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... File chrome/browser/search_engines/default_search_manager_unittest.cc (right): https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager_unittest.cc:26: return; On 2014/04/23 20:48:53, erikwright wrote: > On 2014/04/23 20:00:18, gab wrote: > > optional: You can just use ASSERT_TRUE for the NULL checks above and not need > > this extra logic here. > > I asked her not to. Many developers are not aware that ASSERT does a return on > failure. As such, there's a risk of this later being replaced with an assert. I'm confused by what this last sentence is saying. Could you restate? ASSERT_TRUE seems clearly better to me here. > I believe ASSERT should only be used directly in test cases. That is very much not what tons of existing test code does. We use ASSERT_ liberally in helper functions in test files. And I approve of that usage.
https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... File chrome/browser/search_engines/default_search_manager.cc (right): https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager.cc:73: base::StringToInt64(id, &data->id); On 2014/04/23 20:56:21, gab wrote: > On 2014/04/23 20:48:53, erikwright wrote: > > On 2014/04/23 20:00:18, gab wrote: > > > see PrefService::GetInt64 > > > > She's working with a dict, not a PrefService, here. > > Duh, of course, this is annoying. DictionaryValue should be augmented to support > this IMO, but that's another story... (this is not the first piece of code doing > this custom logic for int64s). Then how about someone writes that change now, and checks it in, and we use it here?
https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... File chrome/browser/search_engines/default_search_manager.cc (right): https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager.cc:73: base::StringToInt64(id, &data->id); On 2014/04/23 20:57:04, Peter Kasting wrote: > On 2014/04/23 20:56:21, gab wrote: > > On 2014/04/23 20:48:53, erikwright wrote: > > > On 2014/04/23 20:00:18, gab wrote: > > > > see PrefService::GetInt64 > > > > > > She's working with a dict, not a PrefService, here. > > > > Duh, of course, this is annoying. DictionaryValue should be augmented to > support > > this IMO, but that's another story... (this is not the first piece of code > doing > > this custom logic for int64s). > > Then how about someone writes that change now, and checks it in, and we use it > here? Sounds good. I'm on it.
https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... File chrome/browser/search_engines/default_search_manager_unittest.cc (right): https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager_unittest.cc:26: return; On 2014/04/23 20:56:25, Peter Kasting wrote: > On 2014/04/23 20:48:53, erikwright wrote: > > On 2014/04/23 20:00:18, gab wrote: > > > optional: You can just use ASSERT_TRUE for the NULL checks above and not > need > > > this extra logic here. > > > > I asked her not to. Many developers are not aware that ASSERT does a return on > > failure. As such, there's a risk of this later being replaced with an assert. > > I'm confused by what this last sentence is saying. Could you restate? It's not obvious that this is a return statement. So a developer could easily look at this and say "Why are these ASSERTs and not EXPECTs? I'll fix that for you." > ASSERT_TRUE seems clearly better to me here. > > > I believe ASSERT should only be used directly in test cases. > > That is very much not what tons of existing test code does. We use ASSERT_ > liberally in helper functions in test files. And I approve of that usage. The other issue with it is that many developers assume that ASSERT terminates a test, without understanding how. So they put ASSERT in a helper function in order to terminate when the test should no longer continue. And then the test crashes because the caller keeps going with invalid state. Clearly I'm speaking very anecdotally here, both of these have definitely come up in codereviews for me.
On 2014/04/23 21:01:17, gab wrote: > https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... > File chrome/browser/search_engines/default_search_manager.cc (right): > > https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... > chrome/browser/search_engines/default_search_manager.cc:73: > base::StringToInt64(id, &data->id); > On 2014/04/23 20:57:04, Peter Kasting wrote: > > On 2014/04/23 20:56:21, gab wrote: > > > On 2014/04/23 20:48:53, erikwright wrote: > > > > On 2014/04/23 20:00:18, gab wrote: > > > > > see PrefService::GetInt64 > > > > > > > > She's working with a dict, not a PrefService, here. > > > > > > Duh, of course, this is annoying. DictionaryValue should be augmented to > > support > > > this IMO, but that's another story... (this is not the first piece of code > > doing > > > this custom logic for int64s). > > > > Then how about someone writes that change now, and checks it in, and we use it > > here? > > Sounds good. I'm on it. Is it ever used for anything other than base::Time() values?
https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... File chrome/browser/search_engines/default_search_manager_unittest.cc (right): https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager_unittest.cc:26: return; On 2014/04/23 21:11:49, erikwright wrote: > On 2014/04/23 20:56:25, Peter Kasting wrote: > > On 2014/04/23 20:48:53, erikwright wrote: > > > On 2014/04/23 20:00:18, gab wrote: > > > > optional: You can just use ASSERT_TRUE for the NULL checks above and not > > need > > > > this extra logic here. > > > > > > I asked her not to. Many developers are not aware that ASSERT does a return > on > > > failure. As such, there's a risk of this later being replaced with an > assert. > > > > I'm confused by what this last sentence is saying. Could you restate? > > It's not obvious that this is a return statement. So a developer could easily > look at this and say "Why are these ASSERTs and not EXPECTs? I'll fix that for > you." This means the developer fundamentally does not know there is any difference between EXPECT and ASSERT. I don't think we should code defensively against such developers. There may be a few (though I have never seen this confusion happen), but that doesn't justify this fundamentally worse code. > The other issue with it is that many developers assume that ASSERT terminates a > test, without understanding how. > > So they put ASSERT in a helper function in order to terminate when the test > should no longer continue. And then the test crashes because the caller keeps > going with invalid state. This is a more reasonable concern, but I still don't think it is sufficient justification to write the code here, especially when the test as written is not subject to this problem, nor likely to become subject to it. I'm going to remain with my original opinion: this needs to be changed to use ASSERT before this code is landed. If you still find this objectionable, I would solve by inlining the code from ExpectSimilar() into its lone caller.
https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... File chrome/browser/search_engines/default_search_manager_unittest.cc (right): https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager_unittest.cc:26: return; On 2014/04/23 21:15:47, Peter Kasting wrote: > On 2014/04/23 21:11:49, erikwright wrote: > > On 2014/04/23 20:56:25, Peter Kasting wrote: > > > On 2014/04/23 20:48:53, erikwright wrote: > > > > On 2014/04/23 20:00:18, gab wrote: > > > > > optional: You can just use ASSERT_TRUE for the NULL checks above and not > > > need > > > > > this extra logic here. > > > > > > > > I asked her not to. Many developers are not aware that ASSERT does a > return > > on > > > > failure. As such, there's a risk of this later being replaced with an > > assert. > > > > > > I'm confused by what this last sentence is saying. Could you restate? > > > > It's not obvious that this is a return statement. So a developer could easily > > look at this and say "Why are these ASSERTs and not EXPECTs? I'll fix that for > > you." > > This means the developer fundamentally does not know there is any difference > between EXPECT and ASSERT. I don't think we should code defensively against > such developers. There may be a few (though I have never seen this confusion > happen), but that doesn't justify this fundamentally worse code. You're right that I'm conflating this with the second issue. The problem is thinking that it terminates the test, not with thinking that the method will continue. > > The other issue with it is that many developers assume that ASSERT terminates > a > > test, without understanding how. > > > > So they put ASSERT in a helper function in order to terminate when the test > > should no longer continue. And then the test crashes because the caller keeps > > going with invalid state. > > This is a more reasonable concern, but I still don't think it is sufficient > justification to write the code here, especially when the test as written is not > subject to this problem, nor likely to become subject to it. > > I'm going to remain with my original opinion: this needs to be changed to use > ASSERT before this code is landed. If you still find this objectionable, I > would solve by inlining the code from ExpectSimilar() into its lone caller. My personal opinion remains that ASSERT should only be used directly in test cases, but I definitely don't feel strong enough about it to ask for changes here. As you say, it's common enough that one CL doesn't make a difference.
On 2014/04/23 21:13:11, erikwright wrote: > On 2014/04/23 21:01:17, gab wrote: > > > https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... > > File chrome/browser/search_engines/default_search_manager.cc (right): > > > > > https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... > > chrome/browser/search_engines/default_search_manager.cc:73: > > base::StringToInt64(id, &data->id); > > On 2014/04/23 20:57:04, Peter Kasting wrote: > > > On 2014/04/23 20:56:21, gab wrote: > > > > On 2014/04/23 20:48:53, erikwright wrote: > > > > > On 2014/04/23 20:00:18, gab wrote: > > > > > > see PrefService::GetInt64 > > > > > > > > > > She's working with a dict, not a PrefService, here. > > > > > > > > Duh, of course, this is annoying. DictionaryValue should be augmented to > > > support > > > > this IMO, but that's another story... (this is not the first piece of code > > > doing > > > > this custom logic for int64s). > > > > > > Then how about someone writes that change now, and checks it in, and we use > it > > > here? > > > > Sounds good. I'm on it. > > Is it ever used for anything other than base::Time() values? Yes, e.g., byte counts: https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/do...
https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... File chrome/browser/search_engines/default_search_manager.cc (right): https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager.cc:73: base::StringToInt64(id, &data->id); On 2014/04/23 21:01:17, gab wrote: > On 2014/04/23 20:57:04, Peter Kasting wrote: > > On 2014/04/23 20:56:21, gab wrote: > > > On 2014/04/23 20:48:53, erikwright wrote: > > > > On 2014/04/23 20:00:18, gab wrote: > > > > > see PrefService::GetInt64 > > > > > > > > She's working with a dict, not a PrefService, here. > > > > > > Duh, of course, this is annoying. DictionaryValue should be augmented to > > support > > > this IMO, but that's another story... (this is not the first piece of code > > doing > > > this custom logic for int64s). > > > > Then how about someone writes that change now, and checks it in, and we use it > > here? > > Sounds good. I'm on it. CL up @ https://codereview.chromium.org/254473002/
https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... File chrome/browser/search_engines/default_search_manager.cc (right): https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2014/04/23 20:48:53, erikwright wrote: > fix date, remove (c) Done. https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager.cc:27: // A dictionary to hold all data related to the Default Search Engine. On 2014/04/23 20:48:53, erikwright wrote: > blank line before Done. https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager.cc:51: return false; On 2014/04/23 20:41:06, Peter Kasting wrote: > On 2014/04/23 20:00:18, gab wrote: > > Should this even be allowed by the constructor? > > I agree; the constructor should DCHECK this and the rest of the code should just > rely on it being set. > > Combined with your comment below, this means this function can return a > TemplateURLData object directly instead of via outparam, since the bool will no > longer be needed. Done. https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager.cc:52: } On 2014/04/23 20:00:18, gab wrote: > rm {} Done. https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager.cc:56: return false; On 2014/04/23 20:00:18, gab wrote: > This is not possible; PrefService always returns a dictionary (the default one > at registration -- an empty dict in this case -- if no other is available). Done. https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager.cc:60: base::string16 keyword; On 2014/04/23 20:41:06, Peter Kasting wrote: > Nit: Reverse declaration order to match usage order below Done. https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager.cc:61: On 2014/04/23 20:41:06, Peter Kasting wrote: > Nit: Eliminate this blank line and the next two Done. https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager.cc:66: DCHECK(!search_url.empty()); On 2014/04/23 20:41:06, Peter Kasting wrote: > Couldn't these DCHECKs fail if the user has mucked with their preferences file? Settings hardening should catch this, and reset the entire DSE pref if any part of it gets mucked with, but w/o settings hardening the scenario you described could occur and result in weird behavior. I'll add code to handle it (but keep the DCHECKs, as this scenario should be an outlier). https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager.cc:114: for (size_t i = 0; i < alternate_urls->GetSize(); ++i) { On 2014/04/23 20:00:18, gab wrote: > Use ListValue::const_iterator here. Done. https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager.cc:123: for (size_t i = 0; i < encodings->GetSize(); ++i) { On 2014/04/23 20:00:18, gab wrote: > ListValue::const_iterator here as well. Done. https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager.cc:179: for (size_t i = 0; i < data.alternate_urls.size(); ++i) On 2014/04/23 20:00:18, gab wrote: > std::vector<std::string>::const_iterator Done. https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager.cc:184: for (size_t i = 0; i < data.input_encodings.size(); ++i) On 2014/04/23 20:00:18, gab wrote: > std::vector<std::string>::const_iterator Done. https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... File chrome/browser/search_engines/default_search_manager.h (right): https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager.h:8: #include "base/basictypes.h" On 2014/04/23 20:48:53, erikwright wrote: > replace with base/macros.h Done. https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager.h:17: class DefaultSearchManager { On 2014/04/23 20:48:53, erikwright wrote: > Some comments are required here. Done. https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager.h:26: bool ReadFromPrefService(TemplateURLData* url); On 2014/04/23 20:00:18, gab wrote: > I think the fact that it reads/saves to PrefService is an implementation detail. > I would just call these methods. > > GetDefaultSearchEngineData() and SaveUserSelectedDefaultSearchEngineData(). Done. https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... File chrome/browser/search_engines/default_search_manager_unittest.cc (right): https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager_unittest.cc:26: return; On 2014/04/23 20:00:18, gab wrote: > optional: You can just use ASSERT_TRUE for the NULL checks above and not need > this extra logic here. Erik had recommended against using ASSERTs in helper methods, for clarity: "Don't use ASSERT in a helper function. Many developers do not immediately understand that it returns on failure, so the fact that it _must_ be an ASSERT (not an EXPECT) here is not self-evident. Use two EXPECTs and an if (...) return; instead." https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager_unittest.cc:52: virtual void TearDown() { pref_service_.reset(); } On 2014/04/23 20:48:53, erikwright wrote: > Not necessary. There is one test instance per test case execution. Done. https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager_unittest.cc:54: PrefService* Prefs() const { return pref_service_.get(); } On 2014/04/23 20:41:06, Peter Kasting wrote: > This must be non-const or else return a const pointer. > > Nit: Name this getter "pref_service()". Done. https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager_unittest.cc:80: } On 2014/04/23 20:00:18, gab wrote: > Also test reading with no prior writes (i.e. reading default values). Done. For now this just amounts to an empty dictionary. Default value handling is coming in a future CL. https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... File chrome/browser/search_engines/template_url_keys.h (right): https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... chrome/browser/search_engines/template_url_keys.h:10: namespace default_search { On 2014/04/23 20:41:06, Peter Kasting wrote: > Using a namespace for this, especially one that doesn't match anything in the > file/path name of the containing file, is unusual. I'd prefer to do something > more in keeping with typical Chrome practice. > > What this would be depends on where you intend to use these. Right now, they're > only used in default_search_manager.cc, and could thus be in an anonymous > namespace atop that file. Going forward, if you intend to use them elsewhere, > they could perhaps be public members of DefaultSearchManager. They will be needed to populate the dictionary from policy values as well, so I think public members of DefaultSearchManager is the best option. https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... File chrome/browser/search_engines/template_url_service.cc (right): https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service.cc:315: default_search_manager_.reset(new DefaultSearchManager(GetPrefs())); On 2014/04/23 20:00:18, gab wrote: > Initialize in initializer lists rather than in the body (here and in other > constructors). DefaultSearchManager takes a PrefService as input, so doing this would require a check if profile is NULL in the initializer list -- seemed cleaner to do it here (since GetPrefs() already handles this check). https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... File chrome/browser/search_engines/template_url_service.h (right): https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service.h:780: scoped_ptr<DefaultSearchManager> default_search_manager_; On 2014/04/23 20:41:06, Peter Kasting wrote: > Why is this a scoped_ptr<> instead of a straight data member? TemplateURLService may be initialized with a NULL profile (and thus a NULL PrefService), so the options are either to have DefaultSearchManager handle null PrefServices, or use a scoped_ptr for default_search_manager_, which gets set to NULL when we have no PrefService. Of the 2, I think I prefer the latter, since at this point in time, DefaultSearchManager is useless w/o a PrefService. https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... File chrome/browser/search_engines/template_url_service_factory.cc (right): https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_factory.cc:128: DefaultSearchManager::RegisterProfilePrefs(registry); On 2014/04/23 20:00:18, gab wrote: > I would prefer having this call at the top before the individual prefs for this > service are registered (i.e. delegate some work first and do remaining work > after). Done.
https://codereview.chromium.org/229763005/diff/170001/chrome/browser/search_e... File chrome/browser/search_engines/default_search_manager.cc (right): https://codereview.chromium.org/229763005/diff/170001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager.cc:27: Nit: If you're going to put a blank line here, put one above the closing brace as well. https://codereview.chromium.org/229763005/diff/170001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager.cc:99: if (search_url.empty()) Don't handle DCHECK failure. Either these can never be empty, or they can. If they can't, you don't need to handle it. If they can, the DCHECKs are wrong. https://codereview.chromium.org/229763005/diff/170001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager.cc:147: ++it) { Nit: It's legal either way, but personally I'd put this on the line above. (2 places) https://codereview.chromium.org/229763005/diff/170001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager.cc:204: std::vector<std::string>::const_iterator it = data.alternate_urls.begin(); Nit: Scope loop variables like this to the loop body, don't declare them above the loop. (2 places) https://codereview.chromium.org/229763005/diff/170001/chrome/browser/search_e... File chrome/browser/search_engines/default_search_manager.h (right): https://codereview.chromium.org/229763005/diff/170001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager.h:22: static const char* kID; Nit: Can you use static const char kID[]; instead? If not, then use "static const char* const". https://codereview.chromium.org/229763005/diff/170001/chrome/browser/search_e... File chrome/browser/search_engines/default_search_manager_unittest.cc (right): https://codereview.chromium.org/229763005/diff/170001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager_unittest.cc:53: DISALLOW_COPY_AND_ASSIGN(DefaultSearchManagerTest); Nit: We typically separate this from everything above with a blank line https://codereview.chromium.org/229763005/diff/170001/chrome/browser/search_e... File chrome/browser/search_engines/template_url_service.h (right): https://codereview.chromium.org/229763005/diff/170001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service.h:780: // Helper class to manage the default search engine. Nit: "... This will be NULL when using the testing-specific constructor."
https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... File chrome/browser/search_engines/default_search_manager.cc (right): https://codereview.chromium.org/229763005/diff/150001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager.cc:73: base::StringToInt64(id, &data->id); On 2014/04/23 23:20:34, gab wrote: > On 2014/04/23 21:01:17, gab wrote: > > On 2014/04/23 20:57:04, Peter Kasting wrote: > > > On 2014/04/23 20:56:21, gab wrote: > > > > On 2014/04/23 20:48:53, erikwright wrote: > > > > > On 2014/04/23 20:00:18, gab wrote: > > > > > > see PrefService::GetInt64 > > > > > > > > > > She's working with a dict, not a PrefService, here. > > > > > > > > Duh, of course, this is annoying. DictionaryValue should be augmented to > > > support > > > > this IMO, but that's another story... (this is not the first piece of code > > > doing > > > > this custom logic for int64s). > > > > > > Then how about someone writes that change now, and checks it in, and we use > it > > > here? > > > > Sounds good. I'm on it. > > CL up @ https://codereview.chromium.org/254473002/ My CL isn't as simple as I originally thought and nor does it seem to have consensus among reviewers. Please do not block your CL on it. We can fix this later if I can get to land my new proposed API.
https://codereview.chromium.org/229763005/diff/170001/chrome/browser/search_e... File chrome/browser/search_engines/default_search_manager.cc (right): https://codereview.chromium.org/229763005/diff/170001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager.cc:27: On 2014/04/23 23:36:30, Peter Kasting wrote: > Nit: If you're going to put a blank line here, put one above the closing brace > as well. Done. https://codereview.chromium.org/229763005/diff/170001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager.cc:99: if (search_url.empty()) On 2014/04/23 23:36:30, Peter Kasting wrote: > Don't handle DCHECK failure. Either these can never be empty, or they can. If > they can't, you don't need to handle it. If they can, the DCHECKs are wrong. Done. https://codereview.chromium.org/229763005/diff/170001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager.cc:147: ++it) { On 2014/04/23 23:36:30, Peter Kasting wrote: > Nit: It's legal either way, but personally I'd put this on the line above. (2 > places) Done. https://codereview.chromium.org/229763005/diff/170001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager.cc:204: std::vector<std::string>::const_iterator it = data.alternate_urls.begin(); On 2014/04/23 23:36:30, Peter Kasting wrote: > Nit: Scope loop variables like this to the loop body, don't declare them above > the loop. (2 places) Done. https://codereview.chromium.org/229763005/diff/170001/chrome/browser/search_e... File chrome/browser/search_engines/default_search_manager.h (right): https://codereview.chromium.org/229763005/diff/170001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager.h:22: static const char* kID; On 2014/04/23 23:36:30, Peter Kasting wrote: > Nit: Can you use > > static const char kID[]; > > instead? > > If not, then use "static const char* const". Done. https://codereview.chromium.org/229763005/diff/170001/chrome/browser/search_e... File chrome/browser/search_engines/default_search_manager_unittest.cc (right): https://codereview.chromium.org/229763005/diff/170001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager_unittest.cc:53: DISALLOW_COPY_AND_ASSIGN(DefaultSearchManagerTest); On 2014/04/23 23:36:30, Peter Kasting wrote: > Nit: We typically separate this from everything above with a blank line Done. https://codereview.chromium.org/229763005/diff/170001/chrome/browser/search_e... File chrome/browser/search_engines/template_url_service.h (right): https://codereview.chromium.org/229763005/diff/170001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service.h:780: // Helper class to manage the default search engine. On 2014/04/23 23:36:30, Peter Kasting wrote: > Nit: "... This will be NULL when using the testing-specific constructor." Done.
lgtm w/ peter's nits and comments/questions below. https://codereview.chromium.org/229763005/diff/170001/chrome/browser/search_e... File chrome/browser/search_engines/default_search_manager.cc (right): https://codereview.chromium.org/229763005/diff/170001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager.cc:99: if (search_url.empty()) On 2014/04/23 23:36:30, Peter Kasting wrote: > Don't handle DCHECK failure. Either these can never be empty, or they can. If > they can't, you don't need to handle it. If they can, the DCHECKs are wrong. While I agree that DCHECKs shouldn't be used for things that *can* happen, I think this case is special. It *should* never happen for prefs that are read as they were written by Chrome (hence the DCHECK documenting and ensuring that this never happens in regular Chrome flow); but it's not *impossible* for it to happen if we account for manual tampering with the Preferences file (which is not supported, but we have to be prepared to handle IMO -- or we could just say it's really *not* supported and let Chrome have arbitrary behaviour if it does..?). https://codereview.chromium.org/229763005/diff/170001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager.cc:147: ++it) { On 2014/04/23 23:36:30, Peter Kasting wrote: > Nit: It's legal either way, but personally I'd put this on the line above. (2 > places) I agree, but this is the format `git cl format` outputs these days and I've found it annoying to try to go against its will. https://codereview.chromium.org/229763005/diff/170001/chrome/browser/search_e... File chrome/browser/search_engines/template_url_service.cc (right): https://codereview.chromium.org/229763005/diff/170001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service.cc:313: dsp_change_origin_(DSP_CHANGE_OTHER) { Why not set it in the test constructor? (apologies if I missed an existing discussion about this)
pkasting: PTAL -- I've addressed the issues from your feedback, please let me know if you have any other concerns with this CL. Thanks!
LGTM https://codereview.chromium.org/229763005/diff/170001/chrome/browser/search_e... File chrome/browser/search_engines/default_search_manager.cc (right): https://codereview.chromium.org/229763005/diff/170001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager.cc:99: if (search_url.empty()) On 2014/04/24 15:00:44, gab wrote: > On 2014/04/23 23:36:30, Peter Kasting wrote: > > Don't handle DCHECK failure. Either these can never be empty, or they can. > If > > they can't, you don't need to handle it. If they can, the DCHECKs are wrong. > > While I agree that DCHECKs shouldn't be used for things that *can* happen, I > think this case is special. > > It *should* never happen for prefs that are read as they were written by Chrome > (hence the DCHECK documenting and ensuring that this never happens in regular > Chrome flow); but it's not *impossible* for it to happen if we account for > manual tampering with the Preferences file (which is not supported, but we have > to be prepared to handle IMO -- or we could just say it's really *not* supported > and let Chrome have arbitrary behaviour if it does..?). Then you must not have DCHECKs. DCHECKs say that the behavior in question can never occur under any circumstance, including things outside Chrome's control. For example, "I ran out of disk space in the midst of a profile write" is allowed to cause Chrome to behave badly, but it is NOT allowed to violate any DCHECKs. A DCHECK must hold at all times for all reasons. https://codereview.chromium.org/229763005/diff/170001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager.cc:147: ++it) { On 2014/04/24 15:00:44, gab wrote: > On 2014/04/23 23:36:30, Peter Kasting wrote: > > Nit: It's legal either way, but personally I'd put this on the line above. (2 > > places) > > I agree, but this is the format `git cl format` outputs these days and I've > found it annoying to try to go against its will. There's an easy answer to that: Never run git cl format. :) https://codereview.chromium.org/229763005/diff/190001/chrome/browser/search_e... File chrome/browser/search_engines/default_search_manager.cc (right): https://codereview.chromium.org/229763005/diff/190001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager.cc:203: it != data.alternate_urls.end(); ++it) { Nit: {} not necessary here (only on loops or conditionals whose body is >1 line) (2 places)
https://codereview.chromium.org/229763005/diff/170001/chrome/browser/search_e... File chrome/browser/search_engines/default_search_manager.cc (right): https://codereview.chromium.org/229763005/diff/170001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager.cc:147: ++it) { On 2014/04/24 20:48:46, Peter Kasting wrote: > On 2014/04/24 15:00:44, gab wrote: > > On 2014/04/23 23:36:30, Peter Kasting wrote: > > > Nit: It's legal either way, but personally I'd put this on the line above. > (2 > > > places) > > > > I agree, but this is the format `git cl format` outputs these days and I've > > found it annoying to try to go against its will. > > There's an easy answer to that: Never run git cl format. :) Hmmm... Then what's the point of it..? IMO it should be used as much as possible, getting rid of most accidental nits before the review even begins. https://codereview.chromium.org/229763005/diff/190001/chrome/browser/search_e... File chrome/browser/search_engines/default_search_manager.cc (right): https://codereview.chromium.org/229763005/diff/190001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager.cc:203: it != data.alternate_urls.end(); ++it) { On 2014/04/24 20:48:46, Peter Kasting wrote: > Nit: {} not necessary here (only on loops or conditionals whose body is >1 line) > (2 places) I've always thought the rule was no {} when *both* the conditional and the body are 1 liners. So here I would still put {}. Otherwise it looks weird IMO.
The CQ bit was checked by caitkp@chromium.org
Thanks very much for the review! https://codereview.chromium.org/229763005/diff/190001/chrome/browser/search_e... File chrome/browser/search_engines/default_search_manager.cc (right): https://codereview.chromium.org/229763005/diff/190001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager.cc:203: it != data.alternate_urls.end(); ++it) { On 2014/04/25 11:23:42, gab wrote: > On 2014/04/24 20:48:46, Peter Kasting wrote: > > Nit: {} not necessary here (only on loops or conditionals whose body is >1 > line) > > (2 places) > > I've always thought the rule was no {} when *both* the conditional and the body > are 1 liners. So here I would still put {}. Otherwise it looks weird IMO. I personally find it easier to follow with the {} especially with the multiple lines and indents in the conditional. Since the style guide is ambiguous here, I'd prefer to keep the {}.
https://codereview.chromium.org/229763005/diff/170001/chrome/browser/search_e... File chrome/browser/search_engines/default_search_manager.cc (right): https://codereview.chromium.org/229763005/diff/170001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager.cc:147: ++it) { On 2014/04/25 11:23:42, gab wrote: > On 2014/04/24 20:48:46, Peter Kasting wrote: > > On 2014/04/24 15:00:44, gab wrote: > > > On 2014/04/23 23:36:30, Peter Kasting wrote: > > > > Nit: It's legal either way, but personally I'd put this on the line above. > > > (2 > > > > places) > > > > > > I agree, but this is the format `git cl format` outputs these days and I've > > > found it annoying to try to go against its will. > > > > There's an easy answer to that: Never run git cl format. :) > > Hmmm... Then what's the point of it..? IMO it should be used as much as > possible, getting rid of most accidental nits before the review even begins. See many chromium-dev debate threads about this issue. https://codereview.chromium.org/229763005/diff/190001/chrome/browser/search_e... File chrome/browser/search_engines/default_search_manager.cc (right): https://codereview.chromium.org/229763005/diff/190001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager.cc:203: it != data.alternate_urls.end(); ++it) { On 2014/04/25 17:06:47, Cait Phillips wrote: > On 2014/04/25 11:23:42, gab wrote: > > On 2014/04/24 20:48:46, Peter Kasting wrote: > > > Nit: {} not necessary here (only on loops or conditionals whose body is >1 > > line) > > > (2 places) > > > > I've always thought the rule was no {} when *both* the conditional and the > body > > are 1 liners. So here I would still put {}. Otherwise it looks weird IMO. Both styles are legal. There has never been a rule against omitting {} for multi-line conditionals with a single-line body. That's a common misunderstanding of the style guide. > I personally find it easier to follow with the {} especially with the multiple > lines and indents in the conditional. Since the style guide is ambiguous here, > I'd prefer to keep the {}. That's fine.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/caitkp@chromium.org/229763005/210001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on win_chromium_rel
The CQ bit was checked by caitkp@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/caitkp@chromium.org/229763005/260001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on chromium_presubmit
The CQ bit was checked by caitkp@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/caitkp@chromium.org/229763005/260001
Message was sent while issue was closed.
Change committed as 266479
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/258933009/ by machenbach@chromium.org. The reason for reverting is: Speculative revert for introducing lots of leaks. See: http://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20LSan%20Te....
The CQ bit was checked by caitkp@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/caitkp@chromium.org/229763005/280001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on mac_chromium_rel
The CQ bit was checked by caitkp@chromium.org
I was just looking at this for 15 minutes searching for how this could've added a leak, and then realized that patch set 13 isn't what was committed :-/ (not your fault of course) https://codereview.chromium.org/229763005/diff/280001/chrome/browser/search_e... File chrome/browser/search_engines/default_search_manager.cc (right): https://codereview.chromium.org/229763005/diff/280001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager.cc:34: } // namespace This namespace isn't necessary. (Given that all the strings below aren't in a namespace, you know this; not sure why this string is in a namespace and the rest isn't… ;-) ) https://codereview.chromium.org/229763005/diff/280001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager.cc:217: data.search_terms_replacement_key); indent off by one
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/caitkp@chromium.org/229763005/280001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on win_chromium_x64_rel
The CQ bit was checked by caitkp@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/caitkp@chromium.org/229763005/280001
Message was sent while issue was closed.
Change committed as 266615
The CQ bit was checked by caitkp@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/caitkp@chromium.org/229763005/320001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_clang_dbg
The CQ bit was checked by caitkp@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/caitkp@chromium.org/229763005/320001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on chromium_presubmit
The CQ bit was checked by caitkp@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/caitkp@chromium.org/229763005/320001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on chromium_presubmit
https://codereview.chromium.org/229763005/diff/280001/chrome/browser/search_e... File chrome/browser/search_engines/default_search_manager.cc (right): https://codereview.chromium.org/229763005/diff/280001/chrome/browser/search_e... chrome/browser/search_engines/default_search_manager.cc:34: } // namespace On 2014/04/28 15:33:18, Nico wrote: > This namespace isn't necessary. (Given that all the strings below aren't in a > namespace, you know this; not sure why this string is in a namespace and the > rest isn't… ;-) ) The strings below aren't file-scoped. They're definitions of members declared in the header. Putting them in a namespace would be incorrect. The lone string in the namespace here _is_ file-scoped. http://dev.chromium.org/developers/coding-style says that all such objects should be wrapped in an anonymous namespace, regardless of their default visibility.
On Mon, Apr 28, 2014 at 2:50 PM, <pkasting@chromium.org> wrote: > > https://codereview.chromium.org/229763005/diff/280001/ > chrome/browser/search_engines/default_search_manager.cc > File chrome/browser/search_engines/default_search_manager.cc (right): > > https://codereview.chromium.org/229763005/diff/280001/ > chrome/browser/search_engines/default_search_manager.cc#newcode34 > chrome/browser/search_engines/default_search_manager.cc:34: } // > namespace > On 2014/04/28 15:33:18, Nico wrote: > >> This namespace isn't necessary. (Given that all the strings below >> > aren't in a > >> namespace, you know this; not sure why this string is in a namespace >> > and the > >> rest isn't... ;-) ) >> > > The strings below aren't file-scoped. They're definitions of members > declared in the header. Putting them in a namespace would be incorrect. > > The lone string in the namespace here _is_ file-scoped. > http://dev.chromium.org/developers/coding-style says that all such > objects should be wrapped in an anonymous namespace, regardless of their > default visibility. > Is that new? As written, it's factually wrong, and also doesn't match much of the codebase. > > https://codereview.chromium.org/229763005/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/04/28 21:56:45, Nico wrote: > On Mon, Apr 28, 2014 at 2:50 PM, <mailto:pkasting@chromium.org> wrote: > > The lone string in the namespace here _is_ file-scoped. > > http://dev.chromium.org/developers/coding-style says that all such > > objects should be wrapped in an anonymous namespace, regardless of their > > default visibility. > > Is that new? As written, it's factually wrong, and also doesn't match much > of the codebase. It is not new; that's been the rule for ages. It's quite possible the bits about Linux symbols used to be true and aren't anymore, feel free to remove those. That's not the primary reason for the rule, however; consistency is the primary reason. And regarding matching the codebase, I don't recall seeing many file-scoped-but-not-anonymous-namespaced variables in .cc files. I'm surprised you feel like this rule is exceptional.
The CQ bit was checked by caitkp@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/caitkp@chromium.org/229763005/320001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on chromium_presubmit
The CQ bit was checked by caitkp@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/caitkp@chromium.org/229763005/320001
Message was sent while issue was closed.
Committed patchset #15 manually as r266891 (presubmit successful). |
