Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
27030002:
Added collecting of data to be fed to the JTL interpreter. (Closed)
|
Created:
7 years, 2 months ago by engedy Modified:
7 years, 1 month ago CC:
erikwright (departed) Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdded collecting of data to be fed to the JTL interpreter.
BUG=298036
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=231317
Patch Set 1 #
Total comments: 12
Patch Set 2 : Addressed comments by battre@. #
Total comments: 2
Patch Set 3 : Fixed capitalization of enum constants. #Patch Set 4 : Added information about loaded modules. #Patch Set 5 : Minor changes, plus added some additional unit-tests. #
Total comments: 54
Patch Set 6 : Service now runs in a deferred manner, fixed a unittest on Win. #Patch Set 7 : Addressed comments by vasilii@. #
Total comments: 49
Patch Set 8 : Addressed all comments by pkasting@. #
Total comments: 22
Patch Set 9 : Addressed next round of comments. #Patch Set 10 : Addressed comments from vasilii@. #
Total comments: 42
Patch Set 11 : Addressed next round of comments from pkasting@ and vasilii@. #
Total comments: 2
Patch Set 12 : Addressed one more comment by vasilii@. #
Total comments: 8
Patch Set 13 : Final touches. #
Total comments: 4
Patch Set 14 : Fixed nits. #
Total comments: 8
Patch Set 15 : Addressed comments by robertshield@. #Patch Set 16 : Fixed one comment, removed an unnecessary include and namespace prefix usage. #Patch Set 17 : Rebased to ToT. #Messages
Total messages: 37 (0 generated)
Please take a look.
https://codereview.chromium.org/27030002/diff/1/chrome/browser/profile_resett... File chrome/browser/profile_resetter/automatic_profile_resetter.cc (right): https://codereview.chromium.org/27030002/diff/1/chrome/browser/profile_resett... chrome/browser/profile_resetter/automatic_profile_resetter.cc:85: value_tree->Set(it.key(), it.value().DeepCopy()); Should this and the one in line 88 be SetWithoutPathExpansion? I am not sure. You seem to copy the dictionary twice. Is that useful? Should this just be target_dictionary->Set(value_true_key, pref_name_to_value_map.release()); https://codereview.chromium.org/27030002/diff/1/chrome/browser/profile_resett... chrome/browser/profile_resetter/automatic_profile_resetter.cc:199: DCHECK_EQ(state_, STATE_WAITING_ON_SERVICES); nit: please swap order of parameters. https://codereview.chromium.org/27030002/diff/1/chrome/browser/profile_resett... chrome/browser/profile_resetter/automatic_profile_resetter.cc:211: DCHECK_EQ(state_, STATE_READY); same here. https://codereview.chromium.org/27030002/diff/1/chrome/browser/profile_resett... File chrome/browser/profile_resetter/automatic_profile_resetter.h (right): https://codereview.chromium.org/27030002/diff/1/chrome/browser/profile_resett... chrome/browser/profile_resetter/automatic_profile_resetter.h:46: void MaybeActivate(); nit: I think I would prefer to drop the Maybe here and in MaybeInitialize. https://codereview.chromium.org/27030002/diff/1/chrome/browser/profile_resett... File chrome/browser/profile_resetter/automatic_profile_resetter_delegate.cc (right): https://codereview.chromium.org/27030002/diff/1/chrome/browser/profile_resett... chrome/browser/profile_resetter/automatic_profile_resetter_delegate.cc:126: size_t default_search_index; please initialize to 0. https://codereview.chromium.org/27030002/diff/1/chrome/browser/profile_resett... File chrome/browser/profile_resetter/automatic_profile_resetter_delegate.h (right): https://codereview.chromium.org/27030002/diff/1/chrome/browser/profile_resett... chrome/browser/profile_resetter/automatic_profile_resetter_delegate.h:44: // "default_search_provider" prefix. Please add comment on ownership of returned value. Also below.
Please take another look. https://codereview.chromium.org/27030002/diff/1/chrome/browser/profile_resett... File chrome/browser/profile_resetter/automatic_profile_resetter.cc (right): https://codereview.chromium.org/27030002/diff/1/chrome/browser/profile_resett... chrome/browser/profile_resetter/automatic_profile_resetter.cc:85: value_tree->Set(it.key(), it.value().DeepCopy()); Avoided the double-copy as we discussed. https://codereview.chromium.org/27030002/diff/1/chrome/browser/profile_resett... chrome/browser/profile_resetter/automatic_profile_resetter.cc:199: DCHECK_EQ(state_, STATE_WAITING_ON_SERVICES); On 2013/10/11 15:05:33, battre wrote: > nit: please swap order of parameters. As discussed, the original order actually looked slightly more natural in the output, so we decided to just go with that. https://codereview.chromium.org/27030002/diff/1/chrome/browser/profile_resett... chrome/browser/profile_resetter/automatic_profile_resetter.cc:211: DCHECK_EQ(state_, STATE_READY); On 2013/10/11 15:05:33, battre wrote: > same here. Same as above. https://codereview.chromium.org/27030002/diff/1/chrome/browser/profile_resett... File chrome/browser/profile_resetter/automatic_profile_resetter.h (right): https://codereview.chromium.org/27030002/diff/1/chrome/browser/profile_resett... chrome/browser/profile_resetter/automatic_profile_resetter.h:46: void MaybeActivate(); On 2013/10/11 15:05:33, battre wrote: > nit: I think I would prefer to drop the Maybe here and in MaybeInitialize. Done. https://codereview.chromium.org/27030002/diff/1/chrome/browser/profile_resett... File chrome/browser/profile_resetter/automatic_profile_resetter_delegate.cc (right): https://codereview.chromium.org/27030002/diff/1/chrome/browser/profile_resett... chrome/browser/profile_resetter/automatic_profile_resetter_delegate.cc:126: size_t default_search_index; On 2013/10/11 15:05:33, battre wrote: > please initialize to 0. Done. https://codereview.chromium.org/27030002/diff/1/chrome/browser/profile_resett... File chrome/browser/profile_resetter/automatic_profile_resetter_delegate.h (right): https://codereview.chromium.org/27030002/diff/1/chrome/browser/profile_resett... chrome/browser/profile_resetter/automatic_profile_resetter_delegate.h:44: // "default_search_provider" prefix. On 2013/10/11 15:05:33, battre wrote: > Please add comment on ownership of returned value. Also below. Done.
LGTM https://codereview.chromium.org/27030002/diff/8001/chrome/browser/profile_res... File chrome/browser/profile_resetter/automatic_profile_resetter_unittest.cc (right): https://codereview.chromium.org/27030002/diff/8001/chrome/browser/profile_res... chrome/browser/profile_resetter/automatic_profile_resetter_unittest.cc:306: HasExpectedDefaultSearchProvider = 1 << 0, Nit: Enums are supposed to be ALL_CAPS_STYLE.
https://codereview.chromium.org/27030002/diff/8001/chrome/browser/profile_res... File chrome/browser/profile_resetter/automatic_profile_resetter_unittest.cc (right): https://codereview.chromium.org/27030002/diff/8001/chrome/browser/profile_res... chrome/browser/profile_resetter/automatic_profile_resetter_unittest.cc:306: HasExpectedDefaultSearchProvider = 1 << 0, On 2013/10/11 16:48:25, battre wrote: > Nit: Enums are supposed to be ALL_CAPS_STYLE. Done.
Hi Everyone. Could you please review... @Vasilii: chrome/browser/profile_resetter/* in patch set 4 and 5, also for OWNER's approval? @Finnur: chrome/browser/enumerate_modules_model_win.cc for OWNER's approval? @Peter: chrome/browser/profile_resetter/automatic_profile_resetter_delegate* for the search engine related parts? (Especially with regards to the currently DISABLED unit test.) @Erik: chrome/browser/profile_resetter/automatic_profile_resetter_delegate* for the module list related parts?
https://codereview.chromium.org/27030002/diff/55001/chrome/browser/profile_re... File chrome/browser/profile_resetter/automatic_profile_resetter_delegate.cc (right): https://codereview.chromium.org/27030002/diff/55001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter_delegate.cc:102: EnumerateModulesModel::GetInstance()->ScanNow(); Is there ever a chance we'll get to this point during the startup sequence or is this always triggered by the user resetting the profile?
https://codereview.chromium.org/27030002/diff/55001/chrome/browser/profile_re... File chrome/browser/profile_resetter/automatic_profile_resetter_delegate.cc (right): https://codereview.chromium.org/27030002/diff/55001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter_delegate.cc:102: EnumerateModulesModel::GetInstance()->ScanNow(); On 2013/10/14 13:08:23, Finnur wrote: > Is there ever a chance we'll get to this point during the startup sequence or is > this always triggered by the user resetting the profile? We will actually get to here during start-up. I assume you are asking because this might be pretty expensive IO-wise. If this is really the case, I am more than happy to defer the entire process to a later time (e.g. 1 minute after the browser window is shown, or something).
On 2013/10/14 13:22:37, engedy wrote: > https://codereview.chromium.org/27030002/diff/55001/chrome/browser/profile_re... > File chrome/browser/profile_resetter/automatic_profile_resetter_delegate.cc > (right): > > https://codereview.chromium.org/27030002/diff/55001/chrome/browser/profile_re... > chrome/browser/profile_resetter/automatic_profile_resetter_delegate.cc:102: > EnumerateModulesModel::GetInstance()->ScanNow(); > On 2013/10/14 13:08:23, Finnur wrote: > > Is there ever a chance we'll get to this point during the startup sequence or > is > > this always triggered by the user resetting the profile? > > We will actually get to here during start-up. I assume you are asking because > this might be pretty expensive IO-wise. If this is really the case, I am more > than happy to defer the entire process to a later time (e.g. 1 minute after the > browser window is shown, or something). Yes, we've been doing that for other things that need this. It was decided in another code review that 45 seconds after startup would be a good time to wait before triggering this.
PTAL. https://codereview.chromium.org/27030002/diff/55001/chrome/browser/profile_re... File chrome/browser/profile_resetter/automatic_profile_resetter_delegate.cc (right): https://codereview.chromium.org/27030002/diff/55001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter_delegate.cc:102: EnumerateModulesModel::GetInstance()->ScanNow(); On 2013/10/14 13:22:38, engedy wrote: > On 2013/10/14 13:08:23, Finnur wrote: > > Is there ever a chance we'll get to this point during the startup sequence or > is > > this always triggered by the user resetting the profile? > > We will actually get to here during start-up. I assume you are asking because > this might be pretty expensive IO-wise. If this is really the case, I am more > than happy to defer the entire process to a later time (e.g. 1 minute after the > browser window is shown, or something). Done.
https://codereview.chromium.org/27030002/diff/55001/chrome/browser/profile_re... File chrome/browser/profile_resetter/automatic_profile_resetter.cc (right): https://codereview.chromium.org/27030002/diff/55001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter.cc:45: const char kSatisfiedCriteriaMaskKeys[][29] = {"satisfied_criteria_mask_bit1", Do we need these magic numbers here and below? https://codereview.chromium.org/27030002/diff/55001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter.cc:76: base::DictionaryValue* target_dictionary, It's the only output parameter. Therefore it should be the last one. https://codereview.chromium.org/27030002/diff/55001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter.cc:102: target_dictionary->Set(value_tree_key, value_tree); Why can't we use pref_name_to_value_map here directly? https://codereview.chromium.org/27030002/diff/55001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter.cc:130: (1 << AutomaticProfileResetter::kSatisfiedCriteriaMaskBits); Here and below "1u << ..." https://codereview.chromium.org/27030002/diff/55001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter.cc:137: ::AutomaticProfileResetter::kSatisfiedCriteriaMaskBits, What is the reason to specify "::AutomaticProfileResetter" here? https://codereview.chromium.org/27030002/diff/55001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter.cc:218: template_url_service_ready_ = true; OnTemplateURLServiceBecameReady() and OnEnumerationOfLoadedModulesReady() are actually duplicate. Everything (including DCHECK) except this line can be moved to the common method. Alternatively you can use the same method as a callback in Activate() with one more boolean parameter passed to the Bind(). https://codereview.chromium.org/27030002/diff/55001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter.cc:350: scoped_ptr<EvaluationResults> results(new EvaluationResults()); scoped_ptr<EvaluationResults> results(new EvaluationResults); https://codereview.chromium.org/27030002/diff/55001/chrome/browser/profile_re... File chrome/browser/profile_resetter/automatic_profile_resetter.h (right): https://codereview.chromium.org/27030002/diff/55001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter.h:14: #include "base/values.h" Used here? https://codereview.chromium.org/27030002/diff/55001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter.h:25: // "mementos"). All methods in this class shall be called on the UI thread. What is "mementos"? https://codereview.chromium.org/27030002/diff/55001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter.h:30: static const size_t kSatisfiedCriteriaMaskBits; kSatisfiedCriteriaMaskBitsCount or something. I also find strange that the mask is uint32 and the number of bits is size_t which is bigger. https://codereview.chromium.org/27030002/diff/55001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter.h:75: bool ShouldPerformDryRun() const; Those two method shouldn't be in the header. They can reside in the anonymous namespace. https://codereview.chromium.org/27030002/diff/55001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter.h:108: static scoped_ptr<EvaluationResults> EvaluateConditionsOnWorkerPoolThread( That one can be removed from the header too. https://codereview.chromium.org/27030002/diff/55001/chrome/browser/profile_re... File chrome/browser/profile_resetter/automatic_profile_resetter_delegate.cc (right): https://codereview.chromium.org/27030002/diff/55001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter_delegate.cc:14: #include "chrome/browser/profile_resetter/automatic_profile_resetter.h" Logically AutomaticProfileResetterDelegateImpl doesn't depend on AutomaticProfileResetter. https://codereview.chromium.org/27030002/diff/55001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter_delegate.cc:96: registrar_.RemoveAll(); registrar_ is design to do this implicitly. https://codereview.chromium.org/27030002/diff/55001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter_delegate.cc:197: void AutomaticProfileResetterDelegateImpl::ReportStatistics( This method seems to be in the wrong place. It should be in AutomaticProfileResetter. https://codereview.chromium.org/27030002/diff/55001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter_delegate.cc:228: modules_have_been_enumerated_event_.Signal(); Move the line below Signal() https://codereview.chromium.org/27030002/diff/55001/chrome/browser/profile_re... File chrome/browser/profile_resetter/automatic_profile_resetter_delegate.h (right): https://codereview.chromium.org/27030002/diff/55001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter_delegate.h:31: virtual void WaitOnEnumerationOfLoadedModules( Here and below SetCallbackOnEnumerationOfLoadedModules(). The method doesn't wait... https://codereview.chromium.org/27030002/diff/55001/chrome/browser/profile_re... File chrome/browser/profile_resetter/automatic_profile_resetter_delegate_unittest.cc (right): https://codereview.chromium.org/27030002/diff/55001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter_delegate_unittest.cc:168: EXPECT_TRUE(details.Get(keys_to_verify[i], &actual_value) && Split this condition. https://codereview.chromium.org/27030002/diff/55001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter_delegate_unittest.cc:200: testing::Mock::VerifyAndClearExpectations(&mock_target); I think it would fail anyway in the line above. https://codereview.chromium.org/27030002/diff/55001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter_delegate_unittest.cc:342: DISABLED_IsDefaultSearchProviderManagedSubtle) { We shouldn't commit the broken test. https://codereview.chromium.org/27030002/diff/55001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter_delegate_unittest.cc:383: prefs->GetPreferenceValues()); Why don't you use GetPreferenceValue("default_search_provider")?
@Vasilii, Finnur: Please take another look. @Peter, Erik: Could you please take a look at the search-engine and module digest related parts, respectively, in "automatic_profile_resetter_delegate*"? https://codereview.chromium.org/27030002/diff/55001/chrome/browser/profile_re... File chrome/browser/profile_resetter/automatic_profile_resetter.cc (right): https://codereview.chromium.org/27030002/diff/55001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter.cc:45: const char kSatisfiedCriteriaMaskKeys[][29] = {"satisfied_criteria_mask_bit1", On 2013/10/14 16:43:33, vasilii wrote: > Do we need these magic numbers here and below? I believe so, yes. The alternative is: const char* kSatisfiedCriteriaMaskKeys[] = {...} But this seems to be more uncommon. What do you think? https://codereview.chromium.org/27030002/diff/55001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter.cc:76: base::DictionaryValue* target_dictionary, On 2013/10/14 16:43:33, vasilii wrote: > It's the only output parameter. Therefore it should be the last one. Done. https://codereview.chromium.org/27030002/diff/55001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter.cc:102: target_dictionary->Set(value_tree_key, value_tree); On 2013/10/14 16:43:33, vasilii wrote: > Why can't we use pref_name_to_value_map here directly? That one does not yet have path expansion (splitting on '.' characters). You might ask why cannot we call source->GetPreference() in the first place, but then we cannot use FindPreference() anymore. https://codereview.chromium.org/27030002/diff/55001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter.cc:130: (1 << AutomaticProfileResetter::kSatisfiedCriteriaMaskBits); On 2013/10/14 16:43:33, vasilii wrote: > Here and below "1u << ..." Done. https://codereview.chromium.org/27030002/diff/55001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter.cc:137: ::AutomaticProfileResetter::kSatisfiedCriteriaMaskBits, On 2013/10/14 16:43:33, vasilii wrote: > What is the reason to specify "::AutomaticProfileResetter" here? Nice catch, it just left there from times when this was in a namespace. Done. https://codereview.chromium.org/27030002/diff/55001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter.cc:218: template_url_service_ready_ = true; On 2013/10/14 16:43:33, vasilii wrote: > OnTemplateURLServiceBecameReady() and OnEnumerationOfLoadedModulesReady() are > actually duplicate. Everything (including DCHECK) except this line can be moved > to the common method. Alternatively you can use the same method as a callback in > Activate() with one more boolean parameter passed to the Bind(). Done. https://codereview.chromium.org/27030002/diff/55001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter.cc:350: scoped_ptr<EvaluationResults> results(new EvaluationResults()); On 2013/10/14 16:43:33, vasilii wrote: > scoped_ptr<EvaluationResults> results(new EvaluationResults); Done. https://codereview.chromium.org/27030002/diff/55001/chrome/browser/profile_re... File chrome/browser/profile_resetter/automatic_profile_resetter.h (right): https://codereview.chromium.org/27030002/diff/55001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter.h:14: #include "base/values.h" On 2013/10/14 16:43:33, vasilii wrote: > Used here? Not really, forward declarations are enough. So removed here, and also in automatic_profile_resetter_delegate.h. https://codereview.chromium.org/27030002/diff/55001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter.h:25: // "mementos"). All methods in this class shall be called on the UI thread. On 2013/10/14 16:43:33, vasilii wrote: > What is "mementos"? Added reference to where they are explained. https://codereview.chromium.org/27030002/diff/55001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter.h:30: static const size_t kSatisfiedCriteriaMaskBits; On 2013/10/14 16:43:33, vasilii wrote: > kSatisfiedCriteriaMaskBitsCount or something. Done. > I also find strange that the mask is uint32 and the number of bits is size_t > which is bigger. I agree that this is strange, I am not sure what we can do against this. The style guide is pretty strict in this matter. https://codereview.chromium.org/27030002/diff/55001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter.h:75: bool ShouldPerformDryRun() const; On 2013/10/14 16:43:33, vasilii wrote: > Those two method shouldn't be in the header. They can reside in the anonymous > namespace. Done. https://codereview.chromium.org/27030002/diff/55001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter.h:108: static scoped_ptr<EvaluationResults> EvaluateConditionsOnWorkerPoolThread( On 2013/10/14 16:43:33, vasilii wrote: > That one can be removed from the header too. Then we have to make the struct "EvaluationResults" public. I am not sure which one is more desirable. What do you think? https://codereview.chromium.org/27030002/diff/55001/chrome/browser/profile_re... File chrome/browser/profile_resetter/automatic_profile_resetter_delegate.cc (right): https://codereview.chromium.org/27030002/diff/55001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter_delegate.cc:14: #include "chrome/browser/profile_resetter/automatic_profile_resetter.h" On 2013/10/14 16:43:33, vasilii wrote: > Logically AutomaticProfileResetterDelegateImpl doesn't depend on > AutomaticProfileResetter. Done. https://codereview.chromium.org/27030002/diff/55001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter_delegate.cc:96: registrar_.RemoveAll(); On 2013/10/14 16:43:33, vasilii wrote: > registrar_ is design to do this implicitly. Done. https://codereview.chromium.org/27030002/diff/55001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter_delegate.cc:197: void AutomaticProfileResetterDelegateImpl::ReportStatistics( On 2013/10/14 16:43:33, vasilii wrote: > This method seems to be in the wrong place. It should be in > AutomaticProfileResetter. Done. As discussed, I moved this into AutomaticProfileResetter. https://codereview.chromium.org/27030002/diff/55001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter_delegate.cc:228: modules_have_been_enumerated_event_.Signal(); On 2013/10/14 16:43:33, vasilii wrote: > Move the line below Signal() Done. https://codereview.chromium.org/27030002/diff/55001/chrome/browser/profile_re... File chrome/browser/profile_resetter/automatic_profile_resetter_delegate.h (right): https://codereview.chromium.org/27030002/diff/55001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter_delegate.h:31: virtual void WaitOnEnumerationOfLoadedModules( On 2013/10/14 16:43:33, vasilii wrote: > Here and below SetCallbackOnEnumerationOfLoadedModules(). The method doesn't > wait... Done. https://codereview.chromium.org/27030002/diff/55001/chrome/browser/profile_re... File chrome/browser/profile_resetter/automatic_profile_resetter_delegate_unittest.cc (right): https://codereview.chromium.org/27030002/diff/55001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter_delegate_unittest.cc:168: EXPECT_TRUE(details.Get(keys_to_verify[i], &actual_value) && On 2013/10/14 16:43:33, vasilii wrote: > Split this condition. Done. https://codereview.chromium.org/27030002/diff/55001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter_delegate_unittest.cc:200: testing::Mock::VerifyAndClearExpectations(&mock_target); On 2013/10/14 16:43:33, vasilii wrote: > I think it would fail anyway in the line above. You are correct, done. https://codereview.chromium.org/27030002/diff/55001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter_delegate_unittest.cc:342: DISABLED_IsDefaultSearchProviderManagedSubtle) { On 2013/10/14 16:43:33, vasilii wrote: > We shouldn't commit the broken test. Agreed. I wanted to have this here for the review, though. https://codereview.chromium.org/27030002/diff/55001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter_delegate_unittest.cc:383: prefs->GetPreferenceValues()); On 2013/10/14 16:43:33, vasilii wrote: > Why don't you use GetPreferenceValue("default_search_provider")? That is not possible here, because preferences are stored and can only be queried without path expansion. Agreed, though, that this needs to be spelled out, so added comments above to clarify.
https://codereview.chromium.org/27030002/diff/74001/chrome/browser/profile_re... File chrome/browser/profile_resetter/automatic_profile_resetter_delegate.cc (right): https://codereview.chromium.org/27030002/diff/74001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter_delegate.cc:29: base::DictionaryValue* BuildSubTreeFromTemplateURL( Is there similar code to this elsewhere in the browser? Can this be shared with anyone? https://codereview.chromium.org/27030002/diff/74001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter_delegate.cc:50: // For consistency with Prefs. Nit: This comment is vague; what is for consistency, and why (and what would be the consequences of not doing it)? https://codereview.chromium.org/27030002/diff/74001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter_delegate.cc:70: if (template_url_service) { Some functions in this class check whether a TemplateURLService exists, while others don't. It's not immediately obvious what governs the difference. Also, instead of always asking for the TemplateURLService for |profile_|, perhaps we should store it as a member? This might not help anything, OTOH maybe it could make the relative lifetimes clearer. https://codereview.chromium.org/27030002/diff/74001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter_delegate.cc:93: if (template_url_service) { Nit: No need for {} on one-line bodies (3 places) https://codereview.chromium.org/27030002/diff/74001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter_delegate.cc:110: const base::Closure& ready_callback) const { Nit: Indent 4, not 8 (2 places) https://codereview.chromium.org/27030002/diff/74001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter_delegate.cc:130: AutomaticProfileResetterDelegateImpl::GetLoadedModuleNameDigests() const { Nit: Indent 4 (3 places) https://codereview.chromium.org/27030002/diff/74001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter_delegate.cc:142: ++it) { Nit: Commonly we don't linebreak before this, but either is fine (2 places) https://codereview.chromium.org/27030002/diff/74001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter_delegate.cc:165: else Nit: No else after return. Consider: return default_search_provider ? BuildSubTreeFromTemplateURL(default_search_provider) : NULL; https://codereview.chromium.org/27030002/diff/74001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter_delegate.cc:170: const { Nit: I'd probably break after "::" instead (2 places) https://codereview.chromium.org/27030002/diff/74001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter_delegate.cc:181: ScopedVector<TemplateURL> engines; Nit: Declare this in the statement that inits it https://codereview.chromium.org/27030002/diff/74001/chrome/browser/profile_re... File chrome/browser/profile_resetter/automatic_profile_resetter_delegate.h (right): https://codereview.chromium.org/27030002/diff/74001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter_delegate.h:43: virtual base::ListValue* GetLoadedModuleNameDigests() const = 0; Try to return scoped_ptr<>s instead of raw pointers from functions that pass ownership to the caller. This makes ownership clear and compile-time checks that the caller deals with it. It also avoids the need for comments about the situation, like on GetPrepopulatedSearchProvidersDetails() below. https://codereview.chromium.org/27030002/diff/74001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter_delegate.h:46: // NULL if none. The returned dictionary will contain the following keys: Avoid a big list like this. It's liable to get out of date and is hard to compare with anything else. If it's necessary to compare with the TemplateURLService prefs, I suggest noting the differences, as it's more informative to say "all the prefs except x and y, which aren't needed because of z". https://codereview.chromium.org/27030002/diff/74001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter_delegate.h:82: // AutomaticProfileResetterDelegate overrides: Nit: Omit "overrides" (2 places) https://codereview.chromium.org/27030002/diff/74001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter_delegate.h:86: Nit: Blank lines inside the group of overrides for a single base class are mildly confusing (several places) https://codereview.chromium.org/27030002/diff/74001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter_delegate.h:94: OVERRIDE; Nit: Try to avoid breaking before OVERRIDE; instead I suggest breaking between the return type and the function name. (2 places) https://codereview.chromium.org/27030002/diff/74001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter_delegate.h:122: // This event is signaled once TemplateURLService::loaded() returns true. Nit: How about: This event is signaled once the TemplateURLService has loaded. If the TemplateURLService was already loaded prior to this class' creation, the event will be signaled during construction. https://codereview.chromium.org/27030002/diff/74001/chrome/browser/profile_re... File chrome/browser/profile_resetter/automatic_profile_resetter_delegate_unittest.cc (right): https://codereview.chromium.org/27030002/diff/74001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter_delegate_unittest.cc:31: using base::UTF8ToUTF16; This is unnecessary (you already qualify the two calls) https://codereview.chromium.org/27030002/diff/74001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter_delegate_unittest.cc:36: const char kDefaultSearchProviderPrefix[] = "default_search_provider"; (1) For any of these constants used in just one place, consider just inlining the constant there (naming it in this case doesn't buy us a ton of readability). (2) Otherwise consider scoping these constants within the classes below if possible so they're even more narrowly-scoped than "whole file" https://codereview.chromium.org/27030002/diff/74001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter_delegate_unittest.cc:59: class GenericTestBase : public testing::Test { I'm surprised there isn't already some sort of "TestWithTestingProfile" or similar base class like this available. https://codereview.chromium.org/27030002/diff/74001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter_delegate_unittest.cc:84: TemplateURL* CreateTestTemplateURL() { Nit: Again, consider returning scoped_ptr. Another option, since this is only called by one place, is to inline this code at that one place. It can always be factored back out later if someone else wants it. https://codereview.chromium.org/27030002/diff/74001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter_delegate_unittest.cc:154: const char* keys_to_verify[] = { Again, consider listing the differences rather than the similarities? https://codereview.chromium.org/27030002/diff/74001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter_delegate_unittest.cc:307: // below is needed. Nit: This comment is just sort of confusing... it's not really clear to me what below constitutes "trickery". https://codereview.chromium.org/27030002/diff/74001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter_delegate_unittest.cc:326: true, kTestName, kTestKeyword, kTestSearchURL, "", "", "", "", ""); Nit: "" -> std::string() where possible (many places) https://codereview.chromium.org/27030002/diff/74001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter_delegate_unittest.cc:387: // This assumes that the order of engines are preserved. Are preserved where? In the prefs? That seems like a questionable assumption.
https://codereview.chromium.org/27030002/diff/55001/chrome/browser/profile_re... File chrome/browser/profile_resetter/automatic_profile_resetter_delegate.cc (right): https://codereview.chromium.org/27030002/diff/55001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter_delegate.cc:102: EnumerateModulesModel::GetInstance()->ScanNow(); Where's the code that delays starting this?
Peter, Finnur: Please take another look. https://codereview.chromium.org/27030002/diff/55001/chrome/browser/profile_re... File chrome/browser/profile_resetter/automatic_profile_resetter_delegate.cc (right): https://codereview.chromium.org/27030002/diff/55001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter_delegate.cc:102: EnumerateModulesModel::GetInstance()->ScanNow(); On 2013/10/15 10:41:07, Finnur wrote: > Where's the code that delays starting this? This is called from AutomaticProfileResetter::PrepareEvaluationFlow, which is posted on line automatic_profile_resetter:210. https://codereview.chromium.org/27030002/diff/74001/chrome/browser/profile_re... File chrome/browser/profile_resetter/automatic_profile_resetter_delegate.cc (right): https://codereview.chromium.org/27030002/diff/74001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter_delegate.cc:29: base::DictionaryValue* BuildSubTreeFromTemplateURL( On 2013/10/15 01:31:28, Peter Kasting wrote: > Is there similar code to this elsewhere in the browser? Can this be shared with > anyone? This is somewhat similar to TemplateURLService::SaveDefaultSearchProviderToPrefs. However, we do not have the "default_search_engine." prefix, which is hardcoded into preference name constants, are not writing values piece-wise into Prefs, and also ignoring a few fields. So, also somewhat different. https://codereview.chromium.org/27030002/diff/74001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter_delegate.cc:50: // For consistency with Prefs. On 2013/10/15 01:31:28, Peter Kasting wrote: > Nit: This comment is vague; what is for consistency, and why (and what would be > the consequences of not doing it)? Reworked this part so the comment is no longer required. https://codereview.chromium.org/27030002/diff/74001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter_delegate.cc:70: if (template_url_service) { On 2013/10/15 01:31:28, Peter Kasting wrote: > Some functions in this class check whether a TemplateURLService exists, while > others don't. It's not immediately obvious what governs the difference. > > Also, instead of always asking for the TemplateURLService for |profile_|, > perhaps we should store it as a member? This might not help anything, OTOH > maybe it could make the relative lifetimes clearer. I have added comments above the class declaration of AutomaticProfileResetterDelegateImpl to clarify. PTAL. https://codereview.chromium.org/27030002/diff/74001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter_delegate.cc:93: if (template_url_service) { On 2013/10/15 01:31:28, Peter Kasting wrote: > Nit: No need for {} on one-line bodies (3 places) Done. In all places in automatic_profile_resetter*, where appropriate. https://codereview.chromium.org/27030002/diff/74001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter_delegate.cc:110: const base::Closure& ready_callback) const { On 2013/10/15 01:31:28, Peter Kasting wrote: > Nit: Indent 4, not 8 (2 places) Done. In all places in automatic_profile_resetter*, where appropriate. https://codereview.chromium.org/27030002/diff/74001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter_delegate.cc:130: AutomaticProfileResetterDelegateImpl::GetLoadedModuleNameDigests() const { On 2013/10/15 01:31:28, Peter Kasting wrote: > Nit: Indent 4 (3 places) Done. In all places in automatic_profile_resetter*, where appropriate. https://codereview.chromium.org/27030002/diff/74001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter_delegate.cc:142: ++it) { On 2013/10/15 01:31:28, Peter Kasting wrote: > Nit: Commonly we don't linebreak before this, but either is fine (2 places) Done. In all places in automatic_profile_resetter*, where appropriate. https://codereview.chromium.org/27030002/diff/74001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter_delegate.cc:165: else On 2013/10/15 01:31:28, Peter Kasting wrote: > Nit: No else after return. Consider: > > return default_search_provider ? > BuildSubTreeFromTemplateURL(default_search_provider) : NULL; Done. https://codereview.chromium.org/27030002/diff/74001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter_delegate.cc:170: const { On 2013/10/15 01:31:28, Peter Kasting wrote: > Nit: I'd probably break after "::" instead (2 places) Done. In all places in automatic_profile_resetter*, where appropriate. https://codereview.chromium.org/27030002/diff/74001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter_delegate.cc:181: ScopedVector<TemplateURL> engines; On 2013/10/15 01:31:28, Peter Kasting wrote: > Nit: Declare this in the statement that inits it Done. https://codereview.chromium.org/27030002/diff/74001/chrome/browser/profile_re... File chrome/browser/profile_resetter/automatic_profile_resetter_delegate.h (right): https://codereview.chromium.org/27030002/diff/74001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter_delegate.h:43: virtual base::ListValue* GetLoadedModuleNameDigests() const = 0; On 2013/10/15 01:31:28, Peter Kasting wrote: > Try to return scoped_ptr<>s instead of raw pointers from functions that pass > ownership to the caller. This makes ownership clear and compile-time checks > that the caller deals with it. It also avoids the need for comments about the > situation, like on GetPrepopulatedSearchProvidersDetails() below. Done. Also replaced all such usages of raw pointers with scoped_ptr-s within this CL where it was reasonable to do so. https://codereview.chromium.org/27030002/diff/74001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter_delegate.h:46: // NULL if none. The returned dictionary will contain the following keys: On 2013/10/15 01:31:28, Peter Kasting wrote: > Avoid a big list like this. It's liable to get out of date and is hard to > compare with anything else. > > If it's necessary to compare with the TemplateURLService prefs, I suggest noting > the differences, as it's more informative to say "all the prefs except x and y, > which aren't needed because of z". Done. https://codereview.chromium.org/27030002/diff/74001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter_delegate.h:82: // AutomaticProfileResetterDelegate overrides: On 2013/10/15 01:31:28, Peter Kasting wrote: > Nit: Omit "overrides" (2 places) Done. Removed everywhere in automatic_profile_resetter*. https://codereview.chromium.org/27030002/diff/74001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter_delegate.h:86: On 2013/10/15 01:31:28, Peter Kasting wrote: > Nit: Blank lines inside the group of overrides for a single base class are > mildly confusing (several places) Done. Removed everywhere in automatic_profile_resetter*. https://codereview.chromium.org/27030002/diff/74001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter_delegate.h:94: OVERRIDE; On 2013/10/15 01:31:28, Peter Kasting wrote: > Nit: Try to avoid breaking before OVERRIDE; instead I suggest breaking between > the return type and the function name. (2 places) Done. https://codereview.chromium.org/27030002/diff/74001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter_delegate.h:122: // This event is signaled once TemplateURLService::loaded() returns true. On 2013/10/15 01:31:28, Peter Kasting wrote: > Nit: How about: > > This event is signaled once the TemplateURLService has loaded. If the > TemplateURLService was already loaded prior to this class' creation, the event > will be signaled during construction. Done. https://codereview.chromium.org/27030002/diff/74001/chrome/browser/profile_re... File chrome/browser/profile_resetter/automatic_profile_resetter_delegate_unittest.cc (right): https://codereview.chromium.org/27030002/diff/74001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter_delegate_unittest.cc:31: using base::UTF8ToUTF16; On 2013/10/15 01:31:28, Peter Kasting wrote: > This is unnecessary (you already qualify the two calls) Done. https://codereview.chromium.org/27030002/diff/74001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter_delegate_unittest.cc:36: const char kDefaultSearchProviderPrefix[] = "default_search_provider"; On 2013/10/15 01:31:28, Peter Kasting wrote: > (1) For any of these constants used in just one place, consider just inlining > the constant there (naming it in this case doesn't buy us a ton of readability). > > (2) Otherwise consider scoping these constants within the classes below if > possible so they're even more narrowly-scoped than "whole file" Done. https://codereview.chromium.org/27030002/diff/74001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter_delegate_unittest.cc:59: class GenericTestBase : public testing::Test { On 2013/10/15 01:31:28, Peter Kasting wrote: > I'm surprised there isn't already some sort of "TestWithTestingProfile" or > similar base class like this available. I might have missed something, but could not find any that did not have lots of superfluous parts. https://codereview.chromium.org/27030002/diff/74001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter_delegate_unittest.cc:84: TemplateURL* CreateTestTemplateURL() { On 2013/10/15 01:31:28, Peter Kasting wrote: > Nit: Again, consider returning scoped_ptr. > > Another option, since this is only called by one place, is to inline this code > at that one place. It can always be factored back out later if someone else > wants it. Done. https://codereview.chromium.org/27030002/diff/74001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter_delegate_unittest.cc:154: const char* keys_to_verify[] = { On 2013/10/15 01:31:28, Peter Kasting wrote: > Again, consider listing the differences rather than the similarities? Done. https://codereview.chromium.org/27030002/diff/74001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter_delegate_unittest.cc:307: // below is needed. On 2013/10/15 01:31:28, Peter Kasting wrote: > Nit: This comment is just sort of confusing... it's not really clear to me what > below constitutes "trickery". Clarified comment. https://codereview.chromium.org/27030002/diff/74001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter_delegate_unittest.cc:326: true, kTestName, kTestKeyword, kTestSearchURL, "", "", "", "", ""); On 2013/10/15 01:31:28, Peter Kasting wrote: > Nit: "" -> std::string() where possible (many places) Done. https://codereview.chromium.org/27030002/diff/74001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter_delegate_unittest.cc:387: // This assumes that the order of engines are preserved. On 2013/10/15 01:31:28, Peter Kasting wrote: > Are preserved where? In the prefs? That seems like a questionable assumption. Replaced with a more solid assumption, and clarified wording.
https://codereview.chromium.org/27030002/diff/74001/chrome/browser/profile_re... File chrome/browser/profile_resetter/automatic_profile_resetter_delegate.cc (right): https://codereview.chromium.org/27030002/diff/74001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter_delegate.cc:29: base::DictionaryValue* BuildSubTreeFromTemplateURL( On 2013/10/15 22:13:07, engedy wrote: > On 2013/10/15 01:31:28, Peter Kasting wrote: > > Is there similar code to this elsewhere in the browser? Can this be shared > with > > anyone? > > This is somewhat similar to > TemplateURLService::SaveDefaultSearchProviderToPrefs. However, we do not have > the "default_search_engine." prefix, which is hardcoded into preference name > constants, are not writing values piece-wise into Prefs, and also ignoring a few > fields. So, also somewhat different. Hmm. I wonder if there is some way we could write a templated function that takes "an object which can do SetString" along with a "pref name prefix" and used that function to write the common pieces (then augmented it at the caller side for TemplateURLService). This probably isn't very feasible, though. https://codereview.chromium.org/27030002/diff/141001/chrome/browser/profile_r... File chrome/browser/profile_resetter/automatic_profile_resetter_delegate.cc (right): https://codereview.chromium.org/27030002/diff/141001/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter_delegate.cc:160: ? BuildSubTreeFromTemplateURL(default_search_provider) Nit: Operators go on the ends of lines, and this is also indented 11 instead of 4. https://codereview.chromium.org/27030002/diff/141001/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter_delegate.cc:161: : scoped_ptr<base::DictionaryValue>(new base::DictionaryValue); If you're not going to return NULL, places like AutomaticProfileResetter::BuildEvaluatorProgramInput() shouldn't NULL-check. Or just return NULL inside the scoped_ptr<> instead of returning an empty dictionary. https://codereview.chromium.org/27030002/diff/141001/chrome/browser/profile_r... File chrome/browser/profile_resetter/automatic_profile_resetter_delegate.h (right): https://codereview.chromium.org/27030002/diff/141001/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter_delegate.h:60: // separated joined string, so as to be usable for the JTL programs. Nit: semicolons separated joined string -> single string with token delimited by semicolons I would probably also remove the final clause; it's not clear why one form is "usable" and the other isn't, or what "JTL" means since that's not defined elsewhere in these comments. https://codereview.chromium.org/27030002/diff/141001/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter_delegate.h:78: // required when using search engine related methods. This comment just made things more confusing. It's not clear what "having the TemplateURLService available" means precisely, or which methods are "search engine related". I still think it makes sense to store the TemplateURLService* in a member. Because TemplateURLService exposes a profile() method, you can replace |profile| with this (and then call profile() in the one spot where we need it), and you can also pass in just the TemplateURLService* instead of a whole Profile* -- this makes dependency injection a little nicer. Then you can write a comment on this like: "May be NULL in unit tests. Must be non-NULL for callers who wish to call LoadTemplateURLServiceIfNeeded(), GetDefaultSerachProviderDetails(), or IsDefaultSearchManaged()." Then you can remove this second sentence if you instead make those functions fail gracefully when this member is NULL. https://codereview.chromium.org/27030002/diff/141001/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter_delegate.h:124: // construction of this class. Nit: Hmm, how about: This event is signaled once module enumeration has been attempted. If during construction the EnumerateModulesModel can supply a non-empty list of modules, module enumeration has clearly already happened, so the event will be signaled immediately; otherwise, if EnumerateLoadedModulesIfNeeded() is called, it will ask the model to scan the modules and then signal the event. https://codereview.chromium.org/27030002/diff/141001/chrome/browser/profile_r... File chrome/browser/profile_resetter/automatic_profile_resetter_delegate_unittest.cc (right): https://codereview.chromium.org/27030002/diff/141001/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter_delegate_unittest.cc:34: using testing::ElementsAreArray; Nit: Avoid using directives when they don't save lines overall, especially when (as in this case) they're only referenced once. https://codereview.chromium.org/27030002/diff/141001/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter_delegate_unittest.cc:144: ? scoped_ptr<base::DictionaryValue>(dsp_details->DeepCopy()) Nit: See comments in delegate .cc regarding wrapping/indenting of ?: You could also simplify this: return scoped_ptr<base::DictionaryValue>( dsp_details ? dsp_details->DeepCopy() : (new base::DictionaryValue)); https://codereview.chromium.org/27030002/diff/141001/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter_delegate_unittest.cc:166: // Encoding list is stored in Prefs as a semicolon-separated string. This is sure ugly... I don't have much good advice here, I was thinking that if you could get to two STL containers (even of different types) you could sort() each and then use equal() to compare them, but I don't think that's any easier. https://codereview.chromium.org/27030002/diff/141001/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter_delegate_unittest.cc:323: // that preferences are originally stored without path expansion. Unfortunately, I still don't really know what this comment is trying to say :( I'm sorry for being dense. https://codereview.chromium.org/27030002/diff/141001/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter_delegate_unittest.cc:411: // a unique keyword specified. All engines in the database must always have unique keywords; this is a core assertion of the TemplateURLService and is documented and enforced there, so you need not even mention it here. https://codereview.chromium.org/27030002/diff/141001/chrome/browser/profile_r... File chrome/browser/profile_resetter/automatic_profile_resetter_unittest.cc (right): https://codereview.chromium.org/27030002/diff/141001/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter_unittest.cc:92: // method. Use the "Mock" versions for setting/testing expectations. Is it possible to avoid GMock entirely and just use real function declarations everywhere? If not, fine, but GMock is frowned on in Chrome code anyway since it often makes things needlessly obscure and confusing, and most of our tests mock out functions without using it.
@Peter: please take another look. @Vasilii: please see one response addressed to you. https://codereview.chromium.org/27030002/diff/141001/chrome/browser/profile_r... File chrome/browser/profile_resetter/automatic_profile_resetter_delegate.cc (right): https://codereview.chromium.org/27030002/diff/141001/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter_delegate.cc:160: ? BuildSubTreeFromTemplateURL(default_search_provider) On 2013/10/16 00:40:21, Peter Kasting wrote: > Nit: Operators go on the ends of lines, and this is also indented 11 instead of > 4. Done. https://codereview.chromium.org/27030002/diff/141001/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter_delegate.cc:161: : scoped_ptr<base::DictionaryValue>(new base::DictionaryValue); On 2013/10/16 00:40:21, Peter Kasting wrote: > If you're not going to return NULL, places like > AutomaticProfileResetter::BuildEvaluatorProgramInput() shouldn't NULL-check. > > Or just return NULL inside the scoped_ptr<> instead of returning an empty > dictionary. Done. Not checking for NULLs anymore. https://codereview.chromium.org/27030002/diff/141001/chrome/browser/profile_r... File chrome/browser/profile_resetter/automatic_profile_resetter_delegate.h (right): https://codereview.chromium.org/27030002/diff/141001/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter_delegate.h:60: // separated joined string, so as to be usable for the JTL programs. On 2013/10/16 00:40:21, Peter Kasting wrote: > Nit: semicolons separated joined string -> single string with token delimited by > semicolons > > I would probably also remove the final clause; it's not clear why one form is > "usable" and the other isn't, or what "JTL" means since that's not defined > elsewhere in these comments. Done. https://codereview.chromium.org/27030002/diff/141001/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter_delegate.h:78: // required when using search engine related methods. On 2013/10/16 00:40:21, Peter Kasting wrote: > This comment just made things more confusing. It's not clear what "having the > TemplateURLService available" means precisely, or which methods are "search > engine related". > > I still think it makes sense to store the TemplateURLService* in a member. > Because TemplateURLService exposes a profile() method, you can replace |profile| > with this (and then call profile() in the one spot where we need it), and you > can also pass in just the TemplateURLService* instead of a whole Profile* -- > this makes dependency injection a little nicer. > > Then you can write a comment on this like: "May be NULL in unit tests. Must be > non-NULL for callers who wish to call LoadTemplateURLServiceIfNeeded(), > GetDefaultSerachProviderDetails(), or IsDefaultSearchManaged()." Then you can > remove this second sentence if you instead make those functions fail gracefully > when this member is NULL. Done. I have updated the tests to take advantage of this, I hope for the better. https://codereview.chromium.org/27030002/diff/141001/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter_delegate.h:124: // construction of this class. On 2013/10/16 00:40:21, Peter Kasting wrote: > Nit: Hmm, how about: > > This event is signaled once module enumeration has been attempted. If during > construction the EnumerateModulesModel can supply a non-empty list of modules, > module enumeration has clearly already happened, so the event will be signaled > immediately; otherwise, if EnumerateLoadedModulesIfNeeded() is called, it will > ask the model to scan the modules and then signal the event. Done. Great suggestion, thanks! https://codereview.chromium.org/27030002/diff/141001/chrome/browser/profile_r... File chrome/browser/profile_resetter/automatic_profile_resetter_delegate_unittest.cc (right): https://codereview.chromium.org/27030002/diff/141001/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter_delegate_unittest.cc:34: using testing::ElementsAreArray; On 2013/10/16 00:40:21, Peter Kasting wrote: > Nit: Avoid using directives when they don't save lines overall, especially when > (as in this case) they're only referenced once. Done. https://codereview.chromium.org/27030002/diff/141001/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter_delegate_unittest.cc:144: ? scoped_ptr<base::DictionaryValue>(dsp_details->DeepCopy()) On 2013/10/16 00:40:21, Peter Kasting wrote: > Nit: See comments in delegate .cc regarding wrapping/indenting of ?: > > You could also simplify this: > > return scoped_ptr<base::DictionaryValue>( > dsp_details ? dsp_details->DeepCopy() : (new base::DictionaryValue)); Done. https://codereview.chromium.org/27030002/diff/141001/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter_delegate_unittest.cc:166: // Encoding list is stored in Prefs as a semicolon-separated string. On 2013/10/16 00:40:21, Peter Kasting wrote: > This is sure ugly... I don't have much good advice here, I was thinking that if > you could get to two STL containers (even of different types) you could sort() > each and then use equal() to compare them, but I don't think that's any easier. Agreed that this is very ugly. Vasilii, as Owner, do you have preferences/good ideas here? https://codereview.chromium.org/27030002/diff/141001/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter_delegate_unittest.cc:323: // that preferences are originally stored without path expansion. On 2013/10/16 00:40:21, Peter Kasting wrote: > Unfortunately, I still don't really know what this comment is trying to say :( > I'm sorry for being dense. Done. On second read, this really does not make much sense. Removed all but the first sentence. https://codereview.chromium.org/27030002/diff/141001/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter_delegate_unittest.cc:411: // a unique keyword specified. On 2013/10/16 00:40:21, Peter Kasting wrote: > All engines in the database must always have unique keywords; this is a core > assertion of the TemplateURLService and is documented and enforced there, so you > need not even mention it here. Done. https://codereview.chromium.org/27030002/diff/141001/chrome/browser/profile_r... File chrome/browser/profile_resetter/automatic_profile_resetter_unittest.cc (right): https://codereview.chromium.org/27030002/diff/141001/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter_unittest.cc:92: // method. Use the "Mock" versions for setting/testing expectations. On 2013/10/16 00:40:21, Peter Kasting wrote: > Is it possible to avoid GMock entirely and just use real function declarations > everywhere? If not, fine, but GMock is frowned on in Chrome code anyway since > it often makes things needlessly obscure and confusing, and most of our tests > mock out functions without using it. I got rid of these 3 abominations below and made things less obscure. In general, we are really interested in testing interactions here, so I would prefer not getting rid of using GMock altogether.
https://codereview.chromium.org/27030002/diff/55001/chrome/browser/profile_re... File chrome/browser/profile_resetter/automatic_profile_resetter.cc (right): https://codereview.chromium.org/27030002/diff/55001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter.cc:45: const char kSatisfiedCriteriaMaskKeys[][29] = {"satisfied_criteria_mask_bit1", On 2013/10/14 19:35:50, engedy wrote: > On 2013/10/14 16:43:33, vasilii wrote: > > Do we need these magic numbers here and below? > > I believe so, yes. The alternative is: > > const char* kSatisfiedCriteriaMaskKeys[] = {...} > > But this seems to be more uncommon. What do you think? Then leave it. https://codereview.chromium.org/27030002/diff/55001/chrome/browser/profile_re... File chrome/browser/profile_resetter/automatic_profile_resetter.h (right): https://codereview.chromium.org/27030002/diff/55001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter.h:30: static const size_t kSatisfiedCriteriaMaskBits; On 2013/10/14 19:35:50, engedy wrote: > On 2013/10/14 16:43:33, vasilii wrote: > > kSatisfiedCriteriaMaskBitsCount or something. > > Done. > > > I also find strange that the mask is uint32 and the number of bits is size_t > > which is bigger. > > I agree that this is strange, I am not sure what we can do against this. The > style guide is pretty strict in this matter. > Does it say something about kCombinedStatusMaskBits type? https://codereview.chromium.org/27030002/diff/55001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter.h:108: static scoped_ptr<EvaluationResults> EvaluateConditionsOnWorkerPoolThread( On 2013/10/14 19:35:50, engedy wrote: > On 2013/10/14 16:43:33, vasilii wrote: > > That one can be removed from the header too. > > Then we have to make the struct "EvaluationResults" public. I am not sure which > one is more desirable. What do you think? Its definition will stay in .cc? Then it's OK.
OWNERS approval for enumerate_modules_model_win.cc. LGTM.
With regards to module information, we are moving away from using enumerate_modules_model. We have another CL that wants more detailed information and it turns out to be easier to access it directly. FWIU, that probably doesn't change the overall plan but might negate the need to integrate data from enumerate_modules_model. I'll CC you on that CL when it goes up today.
@Vasilii: PTAL. https://codereview.chromium.org/27030002/diff/55001/chrome/browser/profile_re... File chrome/browser/profile_resetter/automatic_profile_resetter.cc (right): https://codereview.chromium.org/27030002/diff/55001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter.cc:45: const char kSatisfiedCriteriaMaskKeys[][29] = {"satisfied_criteria_mask_bit1", On 2013/10/16 13:39:48, vasilii wrote: > On 2013/10/14 19:35:50, engedy wrote: > > On 2013/10/14 16:43:33, vasilii wrote: > > > Do we need these magic numbers here and below? > > > > I believe so, yes. The alternative is: > > > > const char* kSatisfiedCriteriaMaskKeys[] = {...} > > > > But this seems to be more uncommon. What do you think? > > Then leave it. Done. https://codereview.chromium.org/27030002/diff/55001/chrome/browser/profile_re... File chrome/browser/profile_resetter/automatic_profile_resetter.h (right): https://codereview.chromium.org/27030002/diff/55001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter.h:30: static const size_t kSatisfiedCriteriaMaskBits; On 2013/10/16 13:39:48, vasilii wrote: > On 2013/10/14 19:35:50, engedy wrote: > > On 2013/10/14 16:43:33, vasilii wrote: > > > kSatisfiedCriteriaMaskBitsCount or something. > > > > Done. > > > > > I also find strange that the mask is uint32 and the number of bits is size_t > > > which is bigger. > > > > I agree that this is strange, I am not sure what we can do against this. The > > style guide is pretty strict in this matter. > > > > Does it say something about kCombinedStatusMaskBits type? As discussed, we will keep the size_t, as this is used in the sense of being an array size. https://codereview.chromium.org/27030002/diff/55001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter.h:108: static scoped_ptr<EvaluationResults> EvaluateConditionsOnWorkerPoolThread( On 2013/10/16 13:39:48, vasilii wrote: > On 2013/10/14 19:35:50, engedy wrote: > > On 2013/10/14 16:43:33, vasilii wrote: > > > That one can be removed from the header too. > > > > Then we have to make the struct "EvaluationResults" public. I am not sure > which > > one is more desirable. What do you think? > > Its definition will stay in .cc? Then it's OK. Done.
On 2013/10/16 14:31:57, erikwright wrote: > With regards to module information, we are moving away from using > enumerate_modules_model. We have another CL that wants more detailed information > and it turns out to be easier to access it directly. > > FWIU, that probably doesn't change the overall plan but might negate the need to > integrate data from enumerate_modules_model. > > I'll CC you on that CL when it goes up today. Thanks, will keep that in mind, and looking forward to that CL!
LGTM https://codereview.chromium.org/27030002/diff/185001/chrome/browser/profile_r... File chrome/browser/profile_resetter/automatic_profile_resetter.cc (right): https://codereview.chromium.org/27030002/diff/185001/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter.cc:29: namespace { Nit: I'd just move all these down to the anonymous namespace section you have lower down. Bonus points if many of these constants can be moved to a more specific, narrow scope in the code below instead of being declared file-scope. https://codereview.chromium.org/27030002/diff/185001/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter.cc:83: // AutomaticProfileResetter implementation details --------------------------- Nit: Normally I just call this section "Helpers". I also usually put two blank lines above these sorts of section dividers. Both of these are, of course, up to you. https://codereview.chromium.org/27030002/diff/185001/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter.cc:145: // Performs the bulk of the work. Invokes the interpreter to run the |program| The bulk of what work? Invokes what interpreter? This comment feels like it used to be in a more comprehensive context and was moved to this more isolated location. https://codereview.chromium.org/27030002/diff/185001/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter.cc:156: std::string program_str(program.as_string()); Nit: Any particular reason to have temps for these two instead of just inlining the .as_string() calls into the construction below? https://codereview.chromium.org/27030002/diff/185001/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter.cc:240: // The |template_url_service| might be NULL during unit tests. Nit: This comment seems unnecessary, as this callsite doesn't care about the NULLity of the service. https://codereview.chromium.org/27030002/diff/185001/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter.cc:244: new AutomaticProfileResetterDelegateImpl(template_url_service)); Nit: More compact: delegate_.reset(new AutomaticProfileResetterDelegateImpl( TemplateURLServiceFactory::GetForProfile(profile_))); https://codereview.chromium.org/27030002/diff/185001/chrome/browser/profile_r... File chrome/browser/profile_resetter/automatic_profile_resetter_mementos.cc (right): https://codereview.chromium.org/27030002/diff/185001/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter_mementos.cc:59: value : ""; Nit: "" -> std::string()
https://codereview.chromium.org/27030002/diff/55001/chrome/browser/profile_re... File chrome/browser/profile_resetter/automatic_profile_resetter_delegate_unittest.cc (right): https://codereview.chromium.org/27030002/diff/55001/chrome/browser/profile_re... chrome/browser/profile_resetter/automatic_profile_resetter_delegate_unittest.cc:342: DISABLED_IsDefaultSearchProviderManagedSubtle) { On 2013/10/14 19:35:50, engedy wrote: > On 2013/10/14 16:43:33, vasilii wrote: > > We shouldn't commit the broken test. > > Agreed. I wanted to have this here for the review, though. Either fix it or remove it. https://codereview.chromium.org/27030002/diff/185001/chrome/browser/profile_r... File chrome/browser/profile_resetter/automatic_profile_resetter.cc (right): https://codereview.chromium.org/27030002/diff/185001/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter.cc:48: const char kLoadedModuleDigestsKey[] = "loaded_modules"; Alphabetical order. https://codereview.chromium.org/27030002/diff/185001/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter.cc:69: EvaluationResults() Add destructor and DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/27030002/diff/185001/chrome/browser/profile_r... File chrome/browser/profile_resetter/automatic_profile_resetter.h (right): https://codereview.chromium.org/27030002/diff/185001/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter.h:114: scoped_ptr<base::DictionaryValue> BuildEvaluatorProgramInput( Order of declaration should math the order of definition in .cc. https://codereview.chromium.org/27030002/diff/185001/chrome/browser/profile_r... File chrome/browser/profile_resetter/automatic_profile_resetter_delegate_unittest.cc (right): https://codereview.chromium.org/27030002/diff/185001/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter_delegate_unittest.cc:42: AutomaticProfileResetterDelegateTest() {} Here and below add virtual destructor. https://codereview.chromium.org/27030002/diff/185001/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter_delegate_unittest.cc:44: virtual void SetUp() { Here and below use "OVERRIDE" https://codereview.chromium.org/27030002/diff/185001/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter_delegate_unittest.cc:98: data.short_name = base::UTF8ToUTF16("name"); I would say this is ASCIIToUTF16 https://codereview.chromium.org/27030002/diff/185001/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter_delegate_unittest.cc:158: it != actual_encodings_list->end(); ++it) { Optional: you can write small helper function 'std::string ValueToString(Value*)'. Instead of the loop you can call std::transform(actual_encodings_list->begin(), actual_encodings_list->end(), std::back_inserter(actual_encodings_vector), &ValueToString); https://codereview.chromium.org/27030002/diff/185001/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter_delegate_unittest.cc:173: MockCallbackTarget() {} Destructor https://codereview.chromium.org/27030002/diff/185001/chrome/browser/profile_r... File chrome/browser/profile_resetter/automatic_profile_resetter_mementos.cc (right): https://codereview.chromium.org/27030002/diff/185001/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter_mementos.cc:26: PreferenceHostedPromptMemento::~PreferenceHostedPromptMemento() {} Blank line above. https://codereview.chromium.org/27030002/diff/185001/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter_mementos.cc:44: LocalStateHostedPromptMemento::~LocalStateHostedPromptMemento() {} Blank line above. https://codereview.chromium.org/27030002/diff/185001/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter_mementos.cc:55: return ""; return std::string(); https://codereview.chromium.org/27030002/diff/185001/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter_mementos.cc:85: FileHostedPromptMemento::~FileHostedPromptMemento() {} Blank line. https://codereview.chromium.org/27030002/diff/185001/chrome/browser/profile_r... File chrome/browser/profile_resetter/automatic_profile_resetter_unittest.cc (left): https://codereview.chromium.org/27030002/diff/185001/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter_unittest.cc:5: #include <string> The first include is automatic_profile_resetter.h
Most of the changed lines are reorderings. @Vasilii: PTAL. Removed the disabled unit test, and also a part of another test, because they checked behavior in a configuration that can never normally arise. Even if this configuration were to ever occur somehow, this class/test is not the place to deal with that. https://codereview.chromium.org/27030002/diff/185001/chrome/browser/profile_r... File chrome/browser/profile_resetter/automatic_profile_resetter.cc (right): https://codereview.chromium.org/27030002/diff/185001/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter.cc:29: namespace { On 2013/10/16 22:36:51, Peter Kasting wrote: > Nit: I'd just move all these down to the anonymous namespace section you have > lower down. Bonus points if many of these constants can be moved to a more > specific, narrow scope in the code below instead of being declared file-scope. Moved down. Cannot really move them into narrower scopes, though, as some are needed in more than one place, and for consistency, I would not want to have some of them -- just because they were only needed in one place -- defined not where the rest are. https://codereview.chromium.org/27030002/diff/185001/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter.cc:48: const char kLoadedModuleDigestsKey[] = "loaded_modules"; On 2013/10/17 10:57:57, vasilii wrote: > Alphabetical order. Done. https://codereview.chromium.org/27030002/diff/185001/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter.cc:69: EvaluationResults() On 2013/10/17 10:57:57, vasilii wrote: > Add destructor and DISALLOW_COPY_AND_ASSIGN I was under the impression that those are discouraged for structs, when declared in .cc files. https://codereview.chromium.org/27030002/diff/185001/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter.cc:83: // AutomaticProfileResetter implementation details --------------------------- On 2013/10/16 22:36:51, Peter Kasting wrote: > Nit: Normally I just call this section "Helpers". I also usually put two blank > lines above these sorts of section dividers. Both of these are, of course, up > to you. Done. https://codereview.chromium.org/27030002/diff/185001/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter.cc:145: // Performs the bulk of the work. Invokes the interpreter to run the |program| On 2013/10/16 22:36:51, Peter Kasting wrote: > The bulk of what work? Invokes what interpreter? This comment feels like it > used to be in a more comprehensive context and was moved to this more isolated > location. Yes, that is exactly what has happened. :-) Added a bit of info about the interpreter. However, even with reworked comments, this still seemed very much out of context, so I moved it back into its original habitat -- even so that this means having a static function declaration in the header that could be technically declared in the .cc file. https://codereview.chromium.org/27030002/diff/185001/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter.cc:156: std::string program_str(program.as_string()); On 2013/10/16 22:36:51, Peter Kasting wrote: > Nit: Any particular reason to have temps for these two instead of just inlining > the .as_string() calls into the construction below? None, fixed. https://codereview.chromium.org/27030002/diff/185001/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter.cc:240: // The |template_url_service| might be NULL during unit tests. On 2013/10/16 22:36:51, Peter Kasting wrote: > Nit: This comment seems unnecessary, as this callsite doesn't care about the > NULLity of the service. True, done. https://codereview.chromium.org/27030002/diff/185001/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter.cc:244: new AutomaticProfileResetterDelegateImpl(template_url_service)); On 2013/10/16 22:36:51, Peter Kasting wrote: > Nit: More compact: > > delegate_.reset(new AutomaticProfileResetterDelegateImpl( > TemplateURLServiceFactory::GetForProfile(profile_))); Done. https://codereview.chromium.org/27030002/diff/185001/chrome/browser/profile_r... File chrome/browser/profile_resetter/automatic_profile_resetter.h (right): https://codereview.chromium.org/27030002/diff/185001/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter.h:114: scoped_ptr<base::DictionaryValue> BuildEvaluatorProgramInput( On 2013/10/17 10:57:57, vasilii wrote: > Order of declaration should math the order of definition in .cc. Done. https://codereview.chromium.org/27030002/diff/185001/chrome/browser/profile_r... File chrome/browser/profile_resetter/automatic_profile_resetter_delegate_unittest.cc (right): https://codereview.chromium.org/27030002/diff/185001/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter_delegate_unittest.cc:42: AutomaticProfileResetterDelegateTest() {} On 2013/10/17 10:57:57, vasilii wrote: > Here and below add virtual destructor. Done. https://codereview.chromium.org/27030002/diff/185001/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter_delegate_unittest.cc:44: virtual void SetUp() { On 2013/10/17 10:57:57, vasilii wrote: > Here and below use "OVERRIDE" Done. https://codereview.chromium.org/27030002/diff/185001/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter_delegate_unittest.cc:98: data.short_name = base::UTF8ToUTF16("name"); On 2013/10/17 10:57:57, vasilii wrote: > I would say this is ASCIIToUTF16 True, done. https://codereview.chromium.org/27030002/diff/185001/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter_delegate_unittest.cc:158: it != actual_encodings_list->end(); ++it) { On 2013/10/17 10:57:57, vasilii wrote: > Optional: you can write small helper function 'std::string > ValueToString(Value*)'. Instead of the loop you can call > std::transform(actual_encodings_list->begin(), actual_encodings_list->end(), > std::back_inserter(actual_encodings_vector), &ValueToString); Will do as soon as we need to do this in at least one other place. For now, I would stick to the current solution, purely because it is just fewer lines of code. https://codereview.chromium.org/27030002/diff/185001/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter_delegate_unittest.cc:173: MockCallbackTarget() {} On 2013/10/17 10:57:57, vasilii wrote: > Destructor Done. https://codereview.chromium.org/27030002/diff/185001/chrome/browser/profile_r... File chrome/browser/profile_resetter/automatic_profile_resetter_mementos.cc (right): https://codereview.chromium.org/27030002/diff/185001/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter_mementos.cc:26: PreferenceHostedPromptMemento::~PreferenceHostedPromptMemento() {} On 2013/10/17 10:57:57, vasilii wrote: > Blank line above. Done. https://codereview.chromium.org/27030002/diff/185001/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter_mementos.cc:44: LocalStateHostedPromptMemento::~LocalStateHostedPromptMemento() {} On 2013/10/17 10:57:57, vasilii wrote: > Blank line above. Done. https://codereview.chromium.org/27030002/diff/185001/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter_mementos.cc:55: return ""; On 2013/10/17 10:57:57, vasilii wrote: > return std::string(); Done. https://codereview.chromium.org/27030002/diff/185001/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter_mementos.cc:59: value : ""; On 2013/10/16 22:36:51, Peter Kasting wrote: > Nit: "" -> std::string() Done. https://codereview.chromium.org/27030002/diff/185001/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter_mementos.cc:85: FileHostedPromptMemento::~FileHostedPromptMemento() {} On 2013/10/17 10:57:57, vasilii wrote: > Blank line. Done. https://codereview.chromium.org/27030002/diff/185001/chrome/browser/profile_r... File chrome/browser/profile_resetter/automatic_profile_resetter_unittest.cc (left): https://codereview.chromium.org/27030002/diff/185001/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter_unittest.cc:5: #include <string> On 2013/10/17 10:57:57, vasilii wrote: > The first include is automatic_profile_resetter.h Done.
lgtm https://codereview.chromium.org/27030002/diff/234001/chrome/browser/profile_r... File chrome/browser/profile_resetter/automatic_profile_resetter_delegate_unittest.cc (right): https://codereview.chromium.org/27030002/diff/234001/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter_delegate_unittest.cc:100: data.short_name = base::UTF8ToUTF16("name"); ASCIIToUTF16
https://codereview.chromium.org/27030002/diff/234001/chrome/browser/profile_r... File chrome/browser/profile_resetter/automatic_profile_resetter_delegate_unittest.cc (right): https://codereview.chromium.org/27030002/diff/234001/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter_delegate_unittest.cc:100: data.short_name = base::UTF8ToUTF16("name"); Ah, I meant to replace all occurrences. Done.
(Still LGTM) https://codereview.chromium.org/27030002/diff/185001/chrome/browser/profile_r... File chrome/browser/profile_resetter/automatic_profile_resetter.cc (right): https://codereview.chromium.org/27030002/diff/185001/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter.cc:69: EvaluationResults() On 2013/10/17 15:13:47, engedy wrote: > On 2013/10/17 10:57:57, vasilii wrote: > > Add destructor and DISALLOW_COPY_AND_ASSIGN > > I was under the impression that those are discouraged for structs, when declared > in .cc files. The style guide isn't perfectly clear on this point. There's definitely no ".h vs. .cc" aspect of the rule, but I would say that a common pattern in Chrome code is to not declare anything other than a constructor in these kinds of structs. I think not disallowing copy is weakly preferable to disallowing it. https://codereview.chromium.org/27030002/diff/190001/chrome/browser/profile_r... File chrome/browser/profile_resetter/automatic_profile_resetter.h (right): https://codereview.chromium.org/27030002/diff/190001/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter.h:27: // This service becomes busy shortly after start-up, and is responsible for Nit: "becomes busy" is confusing, do you mean "is started"? Perhaps say who starts this and when? https://codereview.chromium.org/27030002/diff/190001/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter.h:28: // evaluating if the criteria for showing the one-time profile reset prompt Nit: if -> whether https://codereview.chromium.org/27030002/diff/190001/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter.h:29: // are satisfied, and will potentially trigger the prompt, but only a single Nit: End sentence after "are satisfied, then begin new sentences, something like "This will trigger a profile reset prompt at most once for any given profile. To ensure the prompt never appears again, a "memento" that the prompt has appeared is written to the profile on disk; see automatic_profile_resetter_mementos.h." https://codereview.chromium.org/27030002/diff/190001/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter.h:106: // Prepare the input of the evaluator program. This will contain all the state Nit: Prepare -> Prepares
I rephrased the comments in question, and moved four declarations to the .cc file. https://codereview.chromium.org/27030002/diff/185001/chrome/browser/profile_r... File chrome/browser/profile_resetter/automatic_profile_resetter.cc (right): https://codereview.chromium.org/27030002/diff/185001/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter.cc:69: EvaluationResults() On 2013/10/17 19:06:14, Peter Kasting wrote: > On 2013/10/17 15:13:47, engedy wrote: > > On 2013/10/17 10:57:57, vasilii wrote: > > > Add destructor and DISALLOW_COPY_AND_ASSIGN > > > > I was under the impression that those are discouraged for structs, when > declared > > in .cc files. > > The style guide isn't perfectly clear on this point. There's definitely no ".h > vs. .cc" aspect of the rule, but I would say that a common pattern in Chrome > code is to not declare anything other than a constructor in these kinds of > structs. I think not disallowing copy is weakly preferable to disallowing it. Thanks for elaborating! Then I will leave this as is. https://codereview.chromium.org/27030002/diff/190001/chrome/browser/profile_r... File chrome/browser/profile_resetter/automatic_profile_resetter.h (right): https://codereview.chromium.org/27030002/diff/190001/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter.h:27: // This service becomes busy shortly after start-up, and is responsible for On 2013/10/17 19:06:14, Peter Kasting wrote: > Nit: "becomes busy" is confusing, do you mean "is started"? Perhaps say who > starts this and when? Done. https://codereview.chromium.org/27030002/diff/190001/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter.h:28: // evaluating if the criteria for showing the one-time profile reset prompt On 2013/10/17 19:06:14, Peter Kasting wrote: > Nit: if -> whether Done. https://codereview.chromium.org/27030002/diff/190001/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter.h:29: // are satisfied, and will potentially trigger the prompt, but only a single On 2013/10/17 19:06:14, Peter Kasting wrote: > Nit: End sentence after "are satisfied, then begin new sentences, something like > "This will trigger a profile reset prompt at most once for any given profile. > To ensure the prompt never appears again, a "memento" that the prompt has > appeared is written to the profile on disk; see > automatic_profile_resetter_mementos.h." I have rephrased to something very similar to this, PTAL. https://codereview.chromium.org/27030002/diff/190001/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter.h:106: // Prepare the input of the evaluator program. This will contain all the state On 2013/10/17 19:06:14, Peter Kasting wrote: > Nit: Prepare -> Prepares Done.
Seems fine. https://codereview.chromium.org/27030002/diff/264001/chrome/browser/profile_r... File chrome/browser/profile_resetter/automatic_profile_resetter.cc (right): https://codereview.chromium.org/27030002/diff/264001/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter.cc:101: kSatisfiedCriteriaMaskNumberOfBits, satisfied_criteria_mask_bits_mismatch); Tiny nit: Consider linebreaking these like: COMPILE_ASSERT( arraysize(kSatisfiedCriteriaMaskKeys) == kSatisfiedCriteriaMaskNumberOfBits, satisfied_criteria_mask_bits_mismatch); This makes it less likely someone will read this like "a == (b, c)" https://codereview.chromium.org/27030002/diff/264001/chrome/browser/profile_r... File chrome/browser/profile_resetter/automatic_profile_resetter.h (right): https://codereview.chromium.org/27030002/diff/264001/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter.h:31: // the profile on disk; see "automatic_profile_resetter_mementos.h" for details. Tiny nit: I'd avoid quotes around the filename here.
https://codereview.chromium.org/27030002/diff/264001/chrome/browser/profile_r... File chrome/browser/profile_resetter/automatic_profile_resetter.cc (right): https://codereview.chromium.org/27030002/diff/264001/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter.cc:101: kSatisfiedCriteriaMaskNumberOfBits, satisfied_criteria_mask_bits_mismatch); On 2013/10/18 18:39:16, Peter Kasting wrote: > Tiny nit: Consider linebreaking these like: > > COMPILE_ASSERT( > arraysize(kSatisfiedCriteriaMaskKeys) == kSatisfiedCriteriaMaskNumberOfBits, > satisfied_criteria_mask_bits_mismatch); > > This makes it less likely someone will read this like "a == (b, c)" Done. https://codereview.chromium.org/27030002/diff/264001/chrome/browser/profile_r... File chrome/browser/profile_resetter/automatic_profile_resetter.h (right): https://codereview.chromium.org/27030002/diff/264001/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter.h:31: // the profile on disk; see "automatic_profile_resetter_mementos.h" for details. On 2013/10/18 18:39:16, Peter Kasting wrote: > Tiny nit: I'd avoid quotes around the filename here. Done.
LGTM, very nicely structured patch. Couple of minor drive by nits and a suggestion for a follow up cl. https://codereview.chromium.org/27030002/diff/694013/chrome/browser/profile_r... File chrome/browser/profile_resetter/automatic_profile_resetter.cc (right): https://codereview.chromium.org/27030002/diff/694013/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter.cc:85: nit: extra blank line https://codereview.chromium.org/27030002/diff/694013/chrome/browser/profile_r... File chrome/browser/profile_resetter/automatic_profile_resetter_delegate.cc (right): https://codereview.chromium.org/27030002/diff/694013/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter_delegate.cc:32: tree->SetString("search_url", template_url->url()); fwiw, these string values seem to be duplicated in a couple of other non-test places. May want to consider constant-izing them in a follow up CL https://codereview.chromium.org/27030002/diff/694013/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter_delegate.cc:149: // 2.) directly tampering the Preferences and/or the SQLite DBs. nit: tampering with https://codereview.chromium.org/27030002/diff/694013/chrome/browser/profile_r... File chrome/browser/profile_resetter/automatic_profile_resetter_delegate.h (right): https://codereview.chromium.org/27030002/diff/694013/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter_delegate.h:87: // * GetDefaultSerachProviderDetails(), Serach -> Search
https://codereview.chromium.org/27030002/diff/694013/chrome/browser/profile_r... File chrome/browser/profile_resetter/automatic_profile_resetter.cc (right): https://codereview.chromium.org/27030002/diff/694013/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter.cc:85: On 2013/10/24 00:36:33, robertshield wrote: > nit: extra blank line Done. https://codereview.chromium.org/27030002/diff/694013/chrome/browser/profile_r... File chrome/browser/profile_resetter/automatic_profile_resetter_delegate.cc (right): https://codereview.chromium.org/27030002/diff/694013/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter_delegate.cc:32: tree->SetString("search_url", template_url->url()); On 2013/10/24 00:36:33, robertshield wrote: > fwiw, these string values seem to be duplicated in a couple of other non-test > places. May want to consider constant-izing them in a follow up CL Yes, we have been discussing just that with pkasting@. I will be looking into what can we do. https://codereview.chromium.org/27030002/diff/694013/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter_delegate.cc:149: // 2.) directly tampering the Preferences and/or the SQLite DBs. On 2013/10/24 00:36:33, robertshield wrote: > nit: tampering with Done. Also removed the last line of the comment, as I have discovered that this is not the case. https://codereview.chromium.org/27030002/diff/694013/chrome/browser/profile_r... File chrome/browser/profile_resetter/automatic_profile_resetter_delegate.h (right): https://codereview.chromium.org/27030002/diff/694013/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter_delegate.h:87: // * GetDefaultSerachProviderDetails(), On 2013/10/24 00:36:33, robertshield wrote: > Serach -> Search Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/engedy@chromium.org/27030002/994003
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/engedy@chromium.org/27030002/1354001
Message was sent while issue was closed.
Change committed as 231317 |
