Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
256143006:
Refactor MetricsStateManager class out of MetricsService. (Closed)
|
Created:
6 years, 7 months ago by Alexei Svitkine (slow) Modified:
6 years, 7 months ago CC:
chromium-reviews, Ilya Sherman, jar (doing other things), asvitkine+watch_chromium.org, Steven Holte, sky Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionRefactor MetricsStateManager class out of MetricsService.
The new class is responsible for managing various MetricsService state prefs,
such as client id, low entropy source and the UMA opt-in state, as well as the
cloned install detector.
Also, moves IsMetricsReportingEnabled() from chrome_browser_main.cc to
the new class as well as changing a couple MetricsService browser tests
to instead be unit tests of the new class.
BUG=368413
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=268232
Patch Set 1 : #Patch Set 2 : #
Total comments: 10
Patch Set 3 : #
Total comments: 56
Patch Set 4 : #
Total comments: 6
Patch Set 5 : #Patch Set 6 : #
Total comments: 2
Patch Set 7 : #Patch Set 8 : #
Total comments: 6
Patch Set 9 : #Messages
Total messages: 35 (0 generated)
Ilya, Jesse can you take an initial look? (The two patchsets are identical in content, but patchset 2 tries to branch the new files off from the previous metrics_service* files, whereas patchset 1 adds them normally. Not sure which diff is more convenient to look at, so I included both.) By the way, I realise that even more clean up can be done on top of this, but since this patch is already pretty large, I'd like to avoid doing additional big changes in this CL unless necessary. Thanks!
friendly ping
https://codereview.chromium.org/256143006/diff/140001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_service.h (right): https://codereview.chromium.org/256143006/diff/140001/chrome/browser/metrics/... chrome/browser/metrics/metrics_service.h:175: // Returns the preferred entropy provider used to seed persistent activities Update comment. https://codereview.chromium.org/256143006/diff/140001/chrome/browser/metrics/... chrome/browser/metrics/metrics_service.h:192: void ForceClientIdCreation(); Is this still needed in the metrics service? https://codereview.chromium.org/256143006/diff/140001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_state_manager.cc (right): https://codereview.chromium.org/256143006/diff/140001/chrome/browser/metrics/... chrome/browser/metrics/metrics_state_manager.cc:181: ResetMetricsIDsIfNecessary(); I wonder if calls to this can be centralized somewhere (maybe in constructor?), or if it makes more sense to keep it near the code that inits each ID. https://codereview.chromium.org/256143006/diff/140001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_state_manager.h (right): https://codereview.chromium.org/256143006/diff/140001/chrome/browser/metrics/... chrome/browser/metrics/metrics_state_manager.h:38: void ForceClientIdCreation(); Is this needed public? Since it's always called on construction. https://codereview.chromium.org/256143006/diff/140001/chrome/browser/metrics/... chrome/browser/metrics/metrics_state_manager.h:48: // client and pass that state in as |reporting_will_be_enabled|. Update comment re: |reporting_will_be_enabled|
https://codereview.chromium.org/256143006/diff/140001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_service.h (right): https://codereview.chromium.org/256143006/diff/140001/chrome/browser/metrics/... chrome/browser/metrics/metrics_service.h:175: // Returns the preferred entropy provider used to seed persistent activities On 2014/05/01 15:51:14, Jesse Doherty wrote: > Update comment. Done. https://codereview.chromium.org/256143006/diff/140001/chrome/browser/metrics/... chrome/browser/metrics/metrics_service.h:192: void ForceClientIdCreation(); On 2014/05/01 15:51:14, Jesse Doherty wrote: > Is this still needed in the metrics service? No, it's not. Good catch! https://codereview.chromium.org/256143006/diff/140001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_state_manager.cc (right): https://codereview.chromium.org/256143006/diff/140001/chrome/browser/metrics/... chrome/browser/metrics/metrics_state_manager.cc:181: ResetMetricsIDsIfNecessary(); On 2014/05/01 15:51:14, Jesse Doherty wrote: > I wonder if calls to this can be centralized somewhere (maybe in constructor?), > or if it makes more sense to keep it near the code that inits each ID. That's a good point. Putting it into the constructor makes a lot of sense now with this refactoring. Done! (And also removed metrics_ids_reset_check_performed_) https://codereview.chromium.org/256143006/diff/140001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_state_manager.h (right): https://codereview.chromium.org/256143006/diff/140001/chrome/browser/metrics/... chrome/browser/metrics/metrics_state_manager.h:38: void ForceClientIdCreation(); On 2014/05/01 15:51:14, Jesse Doherty wrote: > Is this needed public? Since it's always called on construction. It's public because it's also called from MetricsService::EnableRecording(), which can be done later than start up (e.g. after the user enables metrics reporting half-way through a session). https://codereview.chromium.org/256143006/diff/140001/chrome/browser/metrics/... chrome/browser/metrics/metrics_state_manager.h:48: // client and pass that state in as |reporting_will_be_enabled|. On 2014/05/01 15:51:14, Jesse Doherty wrote: > Update comment re: |reporting_will_be_enabled| Done.
Sorry, I started to look at this last night, but then got a little scared off because there's a lot of changes, and some of this code is pretty subtle. Could you possibly split this CL up into several smaller pieces? Regardless, I'll take a proper look and send out some review comments today.
On 2014/05/01 21:52:18, Ilya Sherman wrote: > Sorry, I started to look at this last night, but then got a little scared off > because there's a lot of changes, and some of this code is pretty subtle. Could > you possibly split this CL up into several smaller pieces? Regardless, I'll > take a proper look and send out some review comments today. Not sure if there's a good way to split this up. It's mostly moving state/methods from MetricsService to the new class as well as moving a method from chrome_browser_main.cc. One possibility would be to create a new class and move things to it one by one, but it would be a bit ugly since the goal of this class is to encompass the functionality that I'm moving there - so having it only be partially moved would be actually kind of bad design for the intermediate state... Let me know what you think.
https://codereview.chromium.org/256143006/diff/180001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/256143006/diff/180001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:638: browser_process_->system_request_context()); Hmm, are you sure that it's safe to move this code? It looks like you might be disabling RAPPOR on Android by doing so. https://codereview.chromium.org/256143006/diff/180001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_service.cc (right): https://codereview.chromium.org/256143006/diff/180001/chrome/browser/metrics/... chrome/browser/metrics/metrics_service.cc:189: #include "chrome/browser/metrics/machine_id_provider.h" nit: Still needed? https://codereview.chromium.org/256143006/diff/180001/chrome/browser/metrics/... chrome/browser/metrics/metrics_service.cc:467: g_browser_process->local_state())), nit: Can this be a direct class member, rather than a scoped_ptr? https://codereview.chromium.org/256143006/diff/180001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_service.h (right): https://codereview.chromium.org/256143006/diff/180001/chrome/browser/metrics/... chrome/browser/metrics/metrics_service.h:72: class ClonedInstallDetector; nit: Still needed? https://codereview.chromium.org/256143006/diff/180001/chrome/browser/metrics/... chrome/browser/metrics/metrics_service.h:134: }; nit: Still needed? https://codereview.chromium.org/256143006/diff/180001/chrome/browser/metrics/... chrome/browser/metrics/metrics_service.h:143: // is enabled. nit: Please update the docs. https://codereview.chromium.org/256143006/diff/180001/chrome/browser/metrics/... chrome/browser/metrics/metrics_service.h:149: void Start(); When is it appropriate to call Start(), without regard for the reporting enabled state? https://codereview.chromium.org/256143006/diff/180001/chrome/browser/metrics/... chrome/browser/metrics/metrics_service.h:153: bool StartIfMetricsReportingEnabled(); nit: Perhaps drop "Metrics" from this method name? https://codereview.chromium.org/256143006/diff/180001/chrome/browser/metrics/... chrome/browser/metrics/metrics_service.h:316: }; nit: Still needed? https://codereview.chromium.org/256143006/diff/180001/chrome/browser/metrics/... chrome/browser/metrics/metrics_service.h:490: scoped_ptr<metrics::MetricsStateManager> state_manager_; nit: Docs, please. https://codereview.chromium.org/256143006/diff/180001/chrome/browser/metrics/... chrome/browser/metrics/metrics_service.h:624: static bool IsMetricsReportingEnabled(); How does this compare to MetricsStateManager::IsMetricsReportingEnabled()? https://codereview.chromium.org/256143006/diff/180001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_state_manager.cc (right): https://codereview.chromium.org/256143006/diff/180001/chrome/browser/metrics/... chrome/browser/metrics/metrics_state_manager.cc:41: bool IsMetricsReportingEnabledImpl(PrefService* local_state) { nit: Docs. https://codereview.chromium.org/256143006/diff/180001/chrome/browser/metrics/... chrome/browser/metrics/metrics_state_manager.cc:50: // The debug build doesn't send UMA logs when FieldTrials are forced. I know that you copied this line verbatim, but what's the relevance of "debug build" in this comment? Is that part just out of date? https://codereview.chromium.org/256143006/diff/180001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_state_manager.h (right): https://codereview.chromium.org/256143006/diff/180001/chrome/browser/metrics/... chrome/browser/metrics/metrics_state_manager.h:30: bool IsMetricsReportingEnabled(); I'm not comfortable with exposing this as a public method, in any class interface. It makes it too easy for someone to try to branch production logic based on whether or not UMA is enabled, perhaps thereby worsening the user experience for UMA users (and this discouraging UMA usage), rather than simply making metrics reporting efficient. Note that the existing MetricsServiceHelper class used friend declarations to enforce an OWNERS review on any new clients of this functionality. I think that we should do something similar here. https://codereview.chromium.org/256143006/diff/180001/chrome/browser/metrics/... chrome/browser/metrics/metrics_state_manager.h:40: // Check if this install was cloned or imaged from another machine. If a nit: "Check" -> "Checks" https://codereview.chromium.org/256143006/diff/180001/chrome/browser/metrics/... chrome/browser/metrics/metrics_state_manager.h:41: // clone is detected, reset the client id and low entropy source. This nit: "reset" -> "resets" https://codereview.chromium.org/256143006/diff/180001/chrome/browser/metrics/... chrome/browser/metrics/metrics_state_manager.h:46: // based on whether or not metrics reporting will be permitted on this client. nit: "will be permitted" -> "is permitted"? https://codereview.chromium.org/256143006/diff/180001/chrome/browser/metrics/... chrome/browser/metrics/metrics_state_manager.h:65: // Designates which entropy source was returned from this MetricsService. nit: Please update the comment. https://codereview.chromium.org/256143006/diff/180001/chrome/browser/metrics/... chrome/browser/metrics/metrics_state_manager.h:68: enum EntropySourceReturned { nit: Perhaps name this "LastEntropySource", to match the constant names? https://codereview.chromium.org/256143006/diff/180001/chrome/browser/metrics/... chrome/browser/metrics/metrics_state_manager.h:92: // TODO(asvitkine): Update this when the user toggles this state. Are you intending to address this TODO as part of this CL? If not, why is it safe not to? https://codereview.chromium.org/256143006/diff/180001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_state_manager_unittest.cc (right): https://codereview.chromium.org/256143006/diff/180001/chrome/browser/metrics/... chrome/browser/metrics/metrics_state_manager_unittest.cc:22: MetricsStateManager::RegisterPrefs(prefs.registry()); nit: Perhaps extract this out to a common SetUp() method? https://codereview.chromium.org/256143006/diff/180001/chrome/browser/metrics/... chrome/browser/metrics/metrics_state_manager_unittest.cc:136: // Make sure the initial client id isn't reset by the metrics service. nit: "metrics service" -> "metrics state manager" or similar
+cc holte for Android RAPPOR change Thanks for the detailed comments! https://codereview.chromium.org/256143006/diff/180001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/256143006/diff/180001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:638: browser_process_->system_request_context()); On 2014/05/02 05:12:38, Ilya Sherman wrote: > Hmm, are you sure that it's safe to move this code? It looks like you might be > disabling RAPPOR on Android by doing so. You're correct that this will disable RAPPOR on Android. I believe that should be the only behaviour change as a result of this CL. However, I believe we're not yet using RAPPOR for anything on Android, so I think this is OK to do. I also actually believe that the previous code was wrong because ChromeBrowserMainParts::IsMetricsReportingEnabled() didn't use the correct prefs on Android for UMA enabled state. See http://crbug.com/362192 I think this is a good enough compromise for this CL (especially since it actually makes things better - i.e. we're not incorrectly enabling RAPPOR when UMA is off) and because it's very hard to do this refactoring without any behaviour changes unfortunately. This is in part due to the current broken-ish situation where different prefs are used on different platforms, so e.g. fixing the logic will result in a functional change. My plan to fix this down the line is the following: -> Change IsMetricsReportingEnabled() to work correctly for Android. -> Change Android private code to also go through StartIfMetricsReportingEnabled(). For now, I've also added a TODO to both of our existing IsMetricsReportingEnabled() functions about their current issues. I'd prefer to fix the logic in follow-up CLs, so that it's not mixed in with refactoring. https://codereview.chromium.org/256143006/diff/180001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_service.cc (right): https://codereview.chromium.org/256143006/diff/180001/chrome/browser/metrics/... chrome/browser/metrics/metrics_service.cc:189: #include "chrome/browser/metrics/machine_id_provider.h" On 2014/05/02 05:12:38, Ilya Sherman wrote: > nit: Still needed? Nope. Removed. https://codereview.chromium.org/256143006/diff/180001/chrome/browser/metrics/... chrome/browser/metrics/metrics_service.cc:467: g_browser_process->local_state())), On 2014/05/02 05:12:38, Ilya Sherman wrote: > nit: Can this be a direct class member, rather than a scoped_ptr? It could be if just this CL is taken into account. But per our previous discussion that we want to make this class be owned externally from MetricsService in the future, I prefer to keep it as a scoped_ptr, so that later on when it becomes a regular weak pointer and owned externally, we don't need to change .'s to ->'s. (i.e. avoid extra churn in the followup CLs). https://codereview.chromium.org/256143006/diff/180001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_service.h (right): https://codereview.chromium.org/256143006/diff/180001/chrome/browser/metrics/... chrome/browser/metrics/metrics_service.h:72: class ClonedInstallDetector; On 2014/05/02 05:12:38, Ilya Sherman wrote: > nit: Still needed? Nope. Removed. https://codereview.chromium.org/256143006/diff/180001/chrome/browser/metrics/... chrome/browser/metrics/metrics_service.h:134: }; On 2014/05/02 05:12:38, Ilya Sherman wrote: > nit: Still needed? Nope. Removed. https://codereview.chromium.org/256143006/diff/180001/chrome/browser/metrics/... chrome/browser/metrics/metrics_service.h:143: // is enabled. On 2014/05/02 05:12:38, Ilya Sherman wrote: > nit: Please update the docs. Done. https://codereview.chromium.org/256143006/diff/180001/chrome/browser/metrics/... chrome/browser/metrics/metrics_service.h:149: void Start(); On 2014/05/02 05:12:38, Ilya Sherman wrote: > When is it appropriate to call Start(), without regard for the reporting enabled > state? It's still used by private Android code that uses a different pref. Ideally, we should eventually refactor things so that this one is not exposed. But it would have to be a more involved cross-repo change, so out of scope for this CL. More details about how Android does things here: http://crbug.com/362192 https://codereview.chromium.org/256143006/diff/180001/chrome/browser/metrics/... chrome/browser/metrics/metrics_service.h:153: bool StartIfMetricsReportingEnabled(); On 2014/05/02 05:12:38, Ilya Sherman wrote: > nit: Perhaps drop "Metrics" from this method name? I prefer not to change the name, as otherwise it would make it confusing w.r.t. EnableReporting()/DisableReporting() - whereas those simply control the state of MetricsService and not the UMA opt in pref. Even the current name is not great, but at least I've kept it consistent with the terminology we use elsewhere (i.e. the previous method name from chrome_browser_main.cc and the IsMetricsReportingEnabled() name from the helper below). Though if you have another suggestion for a name, I'm all ears. https://codereview.chromium.org/256143006/diff/180001/chrome/browser/metrics/... chrome/browser/metrics/metrics_service.h:316: }; On 2014/05/02 05:12:38, Ilya Sherman wrote: > nit: Still needed? Nope! Removed. https://codereview.chromium.org/256143006/diff/180001/chrome/browser/metrics/... chrome/browser/metrics/metrics_service.h:490: scoped_ptr<metrics::MetricsStateManager> state_manager_; On 2014/05/02 05:12:38, Ilya Sherman wrote: > nit: Docs, please. Done. https://codereview.chromium.org/256143006/diff/180001/chrome/browser/metrics/... chrome/browser/metrics/metrics_service.h:624: static bool IsMetricsReportingEnabled(); On 2014/05/02 05:12:38, Ilya Sherman wrote: > How does this compare to MetricsStateManager::IsMetricsReportingEnabled()? It's different and sometimes incorrect (e.g. see http://crbug.com/362192). We should fix it. But I opted not to do it in this CL to avoid making it even bigger. The MetricsStateManager::IsMetricsReportingEnabled() is a direct move of the version in chrome_browser_main.cc and is meant to not change any behavior in this CL. I've added a TODO. https://codereview.chromium.org/256143006/diff/180001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_state_manager.cc (right): https://codereview.chromium.org/256143006/diff/180001/chrome/browser/metrics/... chrome/browser/metrics/metrics_state_manager.cc:41: bool IsMetricsReportingEnabledImpl(PrefService* local_state) { On 2014/05/02 05:12:38, Ilya Sherman wrote: > nit: Docs. Removed this Impl method and inlined it into the method on the class. https://codereview.chromium.org/256143006/diff/180001/chrome/browser/metrics/... chrome/browser/metrics/metrics_state_manager.cc:50: // The debug build doesn't send UMA logs when FieldTrials are forced. On 2014/05/02 05:12:38, Ilya Sherman wrote: > I know that you copied this line verbatim, but what's the relevance of "debug > build" in this comment? Is that part just out of date? Done. Yeah, the comment was out of date. https://codereview.chromium.org/256143006/diff/180001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_state_manager.h (right): https://codereview.chromium.org/256143006/diff/180001/chrome/browser/metrics/... chrome/browser/metrics/metrics_state_manager.h:30: bool IsMetricsReportingEnabled(); On 2014/05/02 05:12:38, Ilya Sherman wrote: > I'm not comfortable with exposing this as a public method, in any class > interface. It makes it too easy for someone to try to branch production logic > based on whether or not UMA is enabled, perhaps thereby worsening the user > experience for UMA users (and this discouraging UMA usage), rather than simply > making metrics reporting efficient. Note that the existing MetricsServiceHelper > class used friend declarations to enforce an OWNERS review on any new clients of > this functionality. I think that we should do something similar here. It's not a static method, so someone needs to have a pointer to an instance of this class to call it. The intention is that there's only one instance of this class owned by the metrics sub-system and that other code can't access it. Therefore, it's safe to keep it public. To clarify the above intention, I've expanded the class comment to mention that code outside the metrics directory shouldn't be using this class directly. https://codereview.chromium.org/256143006/diff/180001/chrome/browser/metrics/... chrome/browser/metrics/metrics_state_manager.h:40: // Check if this install was cloned or imaged from another machine. If a On 2014/05/02 05:12:38, Ilya Sherman wrote: > nit: "Check" -> "Checks" Done. https://codereview.chromium.org/256143006/diff/180001/chrome/browser/metrics/... chrome/browser/metrics/metrics_state_manager.h:41: // clone is detected, reset the client id and low entropy source. This On 2014/05/02 05:12:38, Ilya Sherman wrote: > nit: "reset" -> "resets" Done. https://codereview.chromium.org/256143006/diff/180001/chrome/browser/metrics/... chrome/browser/metrics/metrics_state_manager.h:46: // based on whether or not metrics reporting will be permitted on this client. On 2014/05/02 05:12:38, Ilya Sherman wrote: > nit: "will be permitted" -> "is permitted"? Done. https://codereview.chromium.org/256143006/diff/180001/chrome/browser/metrics/... chrome/browser/metrics/metrics_state_manager.h:65: // Designates which entropy source was returned from this MetricsService. On 2014/05/02 05:12:38, Ilya Sherman wrote: > nit: Please update the comment. Done. https://codereview.chromium.org/256143006/diff/180001/chrome/browser/metrics/... chrome/browser/metrics/metrics_state_manager.h:68: enum EntropySourceReturned { On 2014/05/02 05:12:38, Ilya Sherman wrote: > nit: Perhaps name this "LastEntropySource", to match the constant names? I actually went with EntropySourceType and renamed the constants to match, which I think are a bit better as names. https://codereview.chromium.org/256143006/diff/180001/chrome/browser/metrics/... chrome/browser/metrics/metrics_state_manager.h:92: // TODO(asvitkine): Update this when the user toggles this state. On 2014/05/02 05:12:38, Ilya Sherman wrote: > Are you intending to address this TODO as part of this CL? If not, why is it > safe not to? It was safe because it was only checked during startup, so the cached value would be correct. However, thinking about it more, I don't find it's worth the trouble to cache it - so I've removed this now and made the function do the full check each time now (it shouldn't be a very big overhead). https://codereview.chromium.org/256143006/diff/180001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_state_manager_unittest.cc (right): https://codereview.chromium.org/256143006/diff/180001/chrome/browser/metrics/... chrome/browser/metrics/metrics_state_manager_unittest.cc:22: MetricsStateManager::RegisterPrefs(prefs.registry()); On 2014/05/02 05:12:38, Ilya Sherman wrote: > nit: Perhaps extract this out to a common SetUp() method? Done. I put it in the constructor instead since that's run for each test invocation anyway (and not shared between tests). https://codereview.chromium.org/256143006/diff/180001/chrome/browser/metrics/... chrome/browser/metrics/metrics_state_manager_unittest.cc:136: // Make sure the initial client id isn't reset by the metrics service. On 2014/05/02 05:12:38, Ilya Sherman wrote: > nit: "metrics service" -> "metrics state manager" or similar Done.
https://codereview.chromium.org/256143006/diff/180001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/256143006/diff/180001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:638: browser_process_->system_request_context()); On 2014/05/02 15:21:46, Alexei Svitkine wrote: > On 2014/05/02 05:12:38, Ilya Sherman wrote: > > Hmm, are you sure that it's safe to move this code? It looks like you might > be > > disabling RAPPOR on Android by doing so. > > You're correct that this will disable RAPPOR on Android. I believe that should > be the only behaviour change as a result of this CL. > > However, I believe we're not yet using RAPPOR for anything on Android, so I > think this is OK to do. I also actually believe that the previous code was wrong > because ChromeBrowserMainParts::IsMetricsReportingEnabled() didn't use the > correct prefs on Android for UMA enabled state. See http://crbug.com/362192 > > I think this is a good enough compromise for this CL (especially since it > actually makes things better - i.e. we're not incorrectly enabling RAPPOR when > UMA is off) and because it's very hard to do this refactoring without any > behaviour changes unfortunately. This is in part due to the current broken-ish > situation where different prefs are used on different platforms, so e.g. fixing > the logic will result in a functional change. > > My plan to fix this down the line is the following: > > -> Change IsMetricsReportingEnabled() to work correctly for Android. > -> Change Android private code to also go through > StartIfMetricsReportingEnabled(). > > For now, I've also added a TODO to both of our existing > IsMetricsReportingEnabled() functions about their current issues. > > I'd prefer to fix the logic in follow-up CLs, so that it's not mixed in with > refactoring. Actually, I think my comment above is not quite correct. Due to the different prefs issue, I think RAPPOR was already disabled on Android. To confirm, I tried to look at Rappor.* histograms on Android and there was no data for them. So I think this CL doesn't actually change any behavior.
https://codereview.chromium.org/256143006/diff/180001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_service.cc (right): https://codereview.chromium.org/256143006/diff/180001/chrome/browser/metrics/... chrome/browser/metrics/metrics_service.cc:467: g_browser_process->local_state())), On 2014/05/02 15:21:46, Alexei Svitkine wrote: > On 2014/05/02 05:12:38, Ilya Sherman wrote: > > nit: Can this be a direct class member, rather than a scoped_ptr? > > It could be if just this CL is taken into account. But per our previous > discussion that we want to make this class be owned externally from > MetricsService in the future, I prefer to keep it as a scoped_ptr, so that later > on when it becomes a regular weak pointer and owned externally, we don't need to > change .'s to ->'s. (i.e. avoid extra churn in the followup CLs). Ok. Keep in mind that using a weak pointer will introduce churn regardless, because you'll need to add existence checks. But, I don't feel strongly about scoped_ptr vs. direct class member for now. https://codereview.chromium.org/256143006/diff/180001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_service.h (right): https://codereview.chromium.org/256143006/diff/180001/chrome/browser/metrics/... chrome/browser/metrics/metrics_service.h:149: void Start(); On 2014/05/02 15:21:46, Alexei Svitkine wrote: > On 2014/05/02 05:12:38, Ilya Sherman wrote: > > When is it appropriate to call Start(), without regard for the reporting > enabled > > state? > > It's still used by private Android code that uses a different pref. Ideally, we > should eventually refactor things so that this one is not exposed. > > But it would have to be a more involved cross-repo change, so out of scope for > this CL. > > More details about how Android does things here: http://crbug.com/362192 Ok. If this is used only by Android: (1) Can we rename the method to something less seductive for non-Android clients? (2) If not, let's at least add documentation to indicate that this method should not be used by any new clients. https://codereview.chromium.org/256143006/diff/180001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_state_manager.h (right): https://codereview.chromium.org/256143006/diff/180001/chrome/browser/metrics/... chrome/browser/metrics/metrics_state_manager.h:30: bool IsMetricsReportingEnabled(); On 2014/05/02 15:21:46, Alexei Svitkine wrote: > On 2014/05/02 05:12:38, Ilya Sherman wrote: > > I'm not comfortable with exposing this as a public method, in any class > > interface. It makes it too easy for someone to try to branch production logic > > based on whether or not UMA is enabled, perhaps thereby worsening the user > > experience for UMA users (and this discouraging UMA usage), rather than simply > > making metrics reporting efficient. Note that the existing > MetricsServiceHelper > > class used friend declarations to enforce an OWNERS review on any new clients > of > > this functionality. I think that we should do something similar here. > > It's not a static method, so someone needs to have a pointer to an instance of > this class to call it. The intention is that there's only one instance of this > class owned by the metrics sub-system and that other code can't access it. > Therefore, it's safe to keep it public. > > To clarify the above intention, I've expanded the class comment to mention that > code outside the metrics directory shouldn't be using this class directly. It's pretty easy to construct an instance of this class, though. I'd very much prefer to implement the code in some way that requires a metrics OWNERS code review in order for new clients to depend on this functionality. https://codereview.chromium.org/256143006/diff/200001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/256143006/diff/200001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:636: // TODO(asvitkine): Since this function is not ran on Android, RAPPOR is nit: "ran" -> "run" https://codereview.chromium.org/256143006/diff/200001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_service.cc (right): https://codereview.chromium.org/256143006/diff/200001/chrome/browser/metrics/... chrome/browser/metrics/metrics_service.cc:189: #include "chrome/browser/metrics/machine_id_provider.h" nit: I think you meant to remove this, but it has regenerative powers. Try again? :) https://codereview.chromium.org/256143006/diff/200001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_state_manager.cc (right): https://codereview.chromium.org/256143006/diff/200001/chrome/browser/metrics/... chrome/browser/metrics/metrics_state_manager.cc:63: bool enabled = false; nit: Let's move this down by a few lines, to be declared a little closer to where it's used.
https://codereview.chromium.org/256143006/diff/180001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_service.h (right): https://codereview.chromium.org/256143006/diff/180001/chrome/browser/metrics/... chrome/browser/metrics/metrics_service.h:149: void Start(); On 2014/05/02 19:35:06, Ilya Sherman wrote: > On 2014/05/02 15:21:46, Alexei Svitkine wrote: > > On 2014/05/02 05:12:38, Ilya Sherman wrote: > > > When is it appropriate to call Start(), without regard for the reporting > > enabled > > > state? > > > > It's still used by private Android code that uses a different pref. Ideally, > we > > should eventually refactor things so that this one is not exposed. > > > > But it would have to be a more involved cross-repo change, so out of scope for > > this CL. > > > > More details about how Android does things here: http://crbug.com/362192 > > Ok. If this is used only by Android: > (1) Can we rename the method to something less seductive for non-Android > clients? > (2) If not, let's at least add documentation to indicate that this method should > not be used by any new clients. We can't rename it in this CL since the Android code that uses it is in another repo. So it would take several cross-repo iterations to get to that. At which point, we might as well just fix it properly with those same iterations. I'm okay to add documentation for it, if you feel strongly about it, but to be honest I see this going away in the medium term future and don't really see new things appearing that will start to use this, so it doesn't seem worth it to me... Let me know what you prefer. https://codereview.chromium.org/256143006/diff/180001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_state_manager.h (right): https://codereview.chromium.org/256143006/diff/180001/chrome/browser/metrics/... chrome/browser/metrics/metrics_state_manager.h:30: bool IsMetricsReportingEnabled(); On 2014/05/02 19:35:06, Ilya Sherman wrote: > On 2014/05/02 15:21:46, Alexei Svitkine wrote: > > On 2014/05/02 05:12:38, Ilya Sherman wrote: > > > I'm not comfortable with exposing this as a public method, in any class > > > interface. It makes it too easy for someone to try to branch production > logic > > > based on whether or not UMA is enabled, perhaps thereby worsening the user > > > experience for UMA users (and this discouraging UMA usage), rather than > simply > > > making metrics reporting efficient. Note that the existing > > MetricsServiceHelper > > > class used friend declarations to enforce an OWNERS review on any new > clients > > of > > > this functionality. I think that we should do something similar here. > > > > It's not a static method, so someone needs to have a pointer to an instance of > > this class to call it. The intention is that there's only one instance of this > > class owned by the metrics sub-system and that other code can't access it. > > Therefore, it's safe to keep it public. > > > > To clarify the above intention, I've expanded the class comment to mention > that > > code outside the metrics directory shouldn't be using this class directly. > > It's pretty easy to construct an instance of this class, though. I'd very much > prefer to implement the code in some way that requires a metrics OWNERS code > review in order for new clients to depend on this functionality. In the previous patchset, I added this comment to the class def: "Code outside the metrics directory should not be instantiating or using this class directly." I think that's a strong enough wording to discourage use of this class. In general, if someone is instantiating multiple instances of this class, it would be quite bad. One possibility would be to make the constructor a factory method instead and have a check that it's not called more than once. But this will require changing tests to not hit it. Personally, I don't think it's worth it. (I'm not a big fan of friend declarations for this purpose, even though I realise we have precedent for it in some MetricsService code.) https://codereview.chromium.org/256143006/diff/200001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/256143006/diff/200001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:636: // TODO(asvitkine): Since this function is not ran on Android, RAPPOR is On 2014/05/02 19:35:06, Ilya Sherman wrote: > nit: "ran" -> "run" Done. https://codereview.chromium.org/256143006/diff/200001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_service.cc (right): https://codereview.chromium.org/256143006/diff/200001/chrome/browser/metrics/... chrome/browser/metrics/metrics_service.cc:189: #include "chrome/browser/metrics/machine_id_provider.h" On 2014/05/02 19:35:06, Ilya Sherman wrote: > nit: I think you meant to remove this, but it has regenerative powers. Try > again? :) Done. Not sure what happened there. https://codereview.chromium.org/256143006/diff/200001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_state_manager.cc (right): https://codereview.chromium.org/256143006/diff/200001/chrome/browser/metrics/... chrome/browser/metrics/metrics_state_manager.cc:63: bool enabled = false; On 2014/05/02 19:35:06, Ilya Sherman wrote: > nit: Let's move this down by a few lines, to be declared a little closer to > where it's used. Done.
https://codereview.chromium.org/256143006/diff/180001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_service.h (right): https://codereview.chromium.org/256143006/diff/180001/chrome/browser/metrics/... chrome/browser/metrics/metrics_service.h:149: void Start(); On 2014/05/02 20:21:31, Alexei Svitkine wrote: > On 2014/05/02 19:35:06, Ilya Sherman wrote: > > On 2014/05/02 15:21:46, Alexei Svitkine wrote: > > > On 2014/05/02 05:12:38, Ilya Sherman wrote: > > > > When is it appropriate to call Start(), without regard for the reporting > > > enabled > > > > state? > > > > > > It's still used by private Android code that uses a different pref. Ideally, > > we > > > should eventually refactor things so that this one is not exposed. > > > > > > But it would have to be a more involved cross-repo change, so out of scope > for > > > this CL. > > > > > > More details about how Android does things here: http://crbug.com/362192 > > > > Ok. If this is used only by Android: > > (1) Can we rename the method to something less seductive for non-Android > > clients? > > (2) If not, let's at least add documentation to indicate that this method > should > > not be used by any new clients. > > We can't rename it in this CL since the Android code that uses it is in another > repo. So it would take several cross-repo iterations to get to that. At which > point, we might as well just fix it properly with those same iterations. > > I'm okay to add documentation for it, if you feel strongly about it, but to be > honest I see this going away in the medium term future and don't really see new > things appearing that will start to use this, so it doesn't seem worth it to > me... Let me know what you prefer. I'd prefer to include the documentation as part of this CL. It's common for cleanup work to get preempted, so I'd prefer to assume that it's *possible* that the code implemented in this CL will remain in place for long enough for someone to come along and wonder whether they should use it. https://codereview.chromium.org/256143006/diff/180001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_state_manager.h (right): https://codereview.chromium.org/256143006/diff/180001/chrome/browser/metrics/... chrome/browser/metrics/metrics_state_manager.h:30: bool IsMetricsReportingEnabled(); On 2014/05/02 20:21:31, Alexei Svitkine wrote: > On 2014/05/02 19:35:06, Ilya Sherman wrote: > > On 2014/05/02 15:21:46, Alexei Svitkine wrote: > > > On 2014/05/02 05:12:38, Ilya Sherman wrote: > > > > I'm not comfortable with exposing this as a public method, in any class > > > > interface. It makes it too easy for someone to try to branch production > > logic > > > > based on whether or not UMA is enabled, perhaps thereby worsening the user > > > > experience for UMA users (and this discouraging UMA usage), rather than > > simply > > > > making metrics reporting efficient. Note that the existing > > > MetricsServiceHelper > > > > class used friend declarations to enforce an OWNERS review on any new > > clients > > > of > > > > this functionality. I think that we should do something similar here. > > > > > > It's not a static method, so someone needs to have a pointer to an instance > of > > > this class to call it. The intention is that there's only one instance of > this > > > class owned by the metrics sub-system and that other code can't access it. > > > Therefore, it's safe to keep it public. > > > > > > To clarify the above intention, I've expanded the class comment to mention > > that > > > code outside the metrics directory shouldn't be using this class directly. > > > > It's pretty easy to construct an instance of this class, though. I'd very > much > > prefer to implement the code in some way that requires a metrics OWNERS code > > review in order for new clients to depend on this functionality. > > In the previous patchset, I added this comment to the class def: > > "Code outside the metrics directory should not be instantiating or using this > class directly." > > I think that's a strong enough wording to discourage use of this class. In > general, if someone is instantiating multiple instances of this class, it would > be quite bad. > > One possibility would be to make the constructor a factory method instead and > have a check that it's not called more than once. But this will require changing > tests to not hit it. Personally, I don't think it's worth it. > > (I'm not a big fan of friend declarations for this purpose, even though I > realise we have precedent for it in some MetricsService code.) I saw the comment -- thanks for adding it! -- but I don't think that a comment is sufficient. It's easy to miss a comment; and it's generally not obvious to someone who hasn't worked on UMA why it's important not to branch on the UMA enabled state in feature code. This isn't just a theoretical concern: Somebody recently IMed me asking how to query whether UMA is enabled, for just such a reason. If they had instead chosen to try code search, there's a good chance they would have just implemented a CL that did the branching. So, I think it's quite important to ensure that nobody can add a dependency on this method without triggering a metrics OWNERS review that blocks the CL. I'm not wed to using friend declarations to achieve this result, but it's the best mechanism that I've found so far.
https://codereview.chromium.org/256143006/diff/180001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_state_manager.h (right): https://codereview.chromium.org/256143006/diff/180001/chrome/browser/metrics/... chrome/browser/metrics/metrics_state_manager.h:30: bool IsMetricsReportingEnabled(); On 2014/05/02 19:35:06, Ilya Sherman wrote: > On 2014/05/02 15:21:46, Alexei Svitkine wrote: > > On 2014/05/02 05:12:38, Ilya Sherman wrote: > > > I'm not comfortable with exposing this as a public method, in any class > > > interface. It makes it too easy for someone to try to branch production > logic > > > based on whether or not UMA is enabled, perhaps thereby worsening the user > > > experience for UMA users (and this discouraging UMA usage), rather than > simply > > > making metrics reporting efficient. Note that the existing > > MetricsServiceHelper > > > class used friend declarations to enforce an OWNERS review on any new > clients > > of > > > this functionality. I think that we should do something similar here. > > > > It's not a static method, so someone needs to have a pointer to an instance of > > this class to call it. The intention is that there's only one instance of this > > class owned by the metrics sub-system and that other code can't access it. > > Therefore, it's safe to keep it public. > > > > To clarify the above intention, I've expanded the class comment to mention > that > > code outside the metrics directory shouldn't be using this class directly. > > It's pretty easy to construct an instance of this class, though. I'd very much > prefer to implement the code in some way that requires a metrics OWNERS code > review in order for new clients to depend on this functionality. Hm, it looks like there is a fairly common xyz::internal namespace convention used in chrome for "package" privacy. Would that be appropriate here?
https://codereview.chromium.org/256143006/diff/180001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_state_manager.h (right): https://codereview.chromium.org/256143006/diff/180001/chrome/browser/metrics/... chrome/browser/metrics/metrics_state_manager.h:30: bool IsMetricsReportingEnabled(); On 2014/05/02 20:39:01, Steven Holte wrote: > On 2014/05/02 19:35:06, Ilya Sherman wrote: > > On 2014/05/02 15:21:46, Alexei Svitkine wrote: > > > On 2014/05/02 05:12:38, Ilya Sherman wrote: > > > > I'm not comfortable with exposing this as a public method, in any class > > > > interface. It makes it too easy for someone to try to branch production > > logic > > > > based on whether or not UMA is enabled, perhaps thereby worsening the user > > > > experience for UMA users (and this discouraging UMA usage), rather than > > simply > > > > making metrics reporting efficient. Note that the existing > > > MetricsServiceHelper > > > > class used friend declarations to enforce an OWNERS review on any new > > clients > > > of > > > > this functionality. I think that we should do something similar here. > > > > > > It's not a static method, so someone needs to have a pointer to an instance > of > > > this class to call it. The intention is that there's only one instance of > this > > > class owned by the metrics sub-system and that other code can't access it. > > > Therefore, it's safe to keep it public. > > > > > > To clarify the above intention, I've expanded the class comment to mention > > that > > > code outside the metrics directory shouldn't be using this class directly. > > > > It's pretty easy to construct an instance of this class, though. I'd very > much > > prefer to implement the code in some way that requires a metrics OWNERS code > > review in order for new clients to depend on this functionality. > > Hm, it looks like there is a fairly common xyz::internal namespace convention > used in chrome for "package" privacy. Would that be appropriate here? I'm okay with doing this as well, but perhaps this should wait till we move things to the component? I'd imagine we'd want to do this on a lot of the helper classes... Ilya, what do you think? (I'm okay with doing that in this CL as well, if you prefer.)
https://codereview.chromium.org/256143006/diff/180001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_state_manager.h (right): https://codereview.chromium.org/256143006/diff/180001/chrome/browser/metrics/... chrome/browser/metrics/metrics_state_manager.h:30: bool IsMetricsReportingEnabled(); On 2014/05/02 20:45:51, Alexei Svitkine wrote: > On 2014/05/02 20:39:01, Steven Holte wrote: > > On 2014/05/02 19:35:06, Ilya Sherman wrote: > > > On 2014/05/02 15:21:46, Alexei Svitkine wrote: > > > > On 2014/05/02 05:12:38, Ilya Sherman wrote: > > > > > I'm not comfortable with exposing this as a public method, in any class > > > > > interface. It makes it too easy for someone to try to branch production > > > logic > > > > > based on whether or not UMA is enabled, perhaps thereby worsening the > user > > > > > experience for UMA users (and this discouraging UMA usage), rather than > > > simply > > > > > making metrics reporting efficient. Note that the existing > > > > MetricsServiceHelper > > > > > class used friend declarations to enforce an OWNERS review on any new > > > clients > > > > of > > > > > this functionality. I think that we should do something similar here. > > > > > > > > It's not a static method, so someone needs to have a pointer to an > instance > > of > > > > this class to call it. The intention is that there's only one instance of > > this > > > > class owned by the metrics sub-system and that other code can't access it. > > > > Therefore, it's safe to keep it public. > > > > > > > > To clarify the above intention, I've expanded the class comment to mention > > > that > > > > code outside the metrics directory shouldn't be using this class directly. > > > > > > It's pretty easy to construct an instance of this class, though. I'd very > > much > > > prefer to implement the code in some way that requires a metrics OWNERS code > > > review in order for new clients to depend on this functionality. > > > > Hm, it looks like there is a fairly common xyz::internal namespace convention > > used in chrome for "package" privacy. Would that be appropriate here? > > I'm okay with doing this as well, but perhaps this should wait till we move > things to the component? I'd imagine we'd want to do this on a lot of the helper > classes... Ilya, what do you think? (I'm okay with doing that in this CL as > well, if you prefer.) Yeah, once we move to a component, I bet we can enforce hiding things via DEPS. (Though, it's tricky: There is code like the crash reporter that needs to know whether UMA is enabled.) Even before then, though, I think it's important to enforce an OWNERS review to ensure that the API remains private.
Thanks, PTAL! https://codereview.chromium.org/256143006/diff/180001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_state_manager.h (right): https://codereview.chromium.org/256143006/diff/180001/chrome/browser/metrics/... chrome/browser/metrics/metrics_state_manager.h:30: bool IsMetricsReportingEnabled(); On 2014/05/02 20:49:48, Ilya Sherman wrote: > Yeah, once we move to a component, I bet we can enforce hiding things via DEPS. > (Though, it's tricky: There is code like the crash reporter that needs to know > whether UMA is enabled.) Even before then, though, I think it's important to > enforce an OWNERS review to ensure that the API remains private. Per offline discussion, went with a Create() factory method that enforces that a single instance of the class exists at a time. We can re-evaluated once this moves to a component.
LGTM. Thanks, Alexei! https://codereview.chromium.org/256143006/diff/300001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_state_manager.cc (right): https://codereview.chromium.org/256143006/diff/300001/chrome/browser/metrics/... chrome/browser/metrics/metrics_state_manager.cc:26: nit: Spurious newline.
lgtm
Thanks! +sky for chrome/ OWNERS https://codereview.chromium.org/256143006/diff/300001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_state_manager.cc (right): https://codereview.chromium.org/256143006/diff/300001/chrome/browser/metrics/... chrome/browser/metrics/metrics_state_manager.cc:26: On 2014/05/02 23:12:03, Ilya Sherman wrote: > nit: Spurious newline. Done.
Also, +cc rohitrao@ since this is changing chrome_browser_main.cc which iOS has a different version of.
Looks like sky@ is busy today according to his calendar, trying another reviewer for chrome/ +thakis - Nico, could you approve the chrome/ bits?
stampy lgtm https://codereview.chromium.org/256143006/diff/320002/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/256143006/diff/320002/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:637: // currently disabled there. Is there a bug for this? Else, people will git blame to see where the TODO is from and then find a "refactor $FOO" cl, which is a bit confusing. https://codereview.chromium.org/256143006/diff/320002/chrome/browser/metrics/... File chrome/browser/metrics/metrics_service.h (right): https://codereview.chromium.org/256143006/diff/320002/chrome/browser/metrics/... chrome/browser/metrics/metrics_service.h:174: // returned. (nit: First sentence uses active voice, second uses passive voice. "Otherwise, it returns an…" reads better) https://codereview.chromium.org/256143006/diff/320002/chrome/browser/metrics/... File chrome/browser/metrics/metrics_state_manager.h (right): https://codereview.chromium.org/256143006/diff/320002/chrome/browser/metrics/... chrome/browser/metrics/metrics_state_manager.h:53: // returned. (ditto)
https://codereview.chromium.org/256143006/diff/320002/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/256143006/diff/320002/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:637: // currently disabled there. On 2014/05/05 15:59:12, Nico wrote: > Is there a bug for this? Else, people will git blame to see where the TODO is > from and then find a "refactor $FOO" cl, which is a bit confusing. Done. Added crbug link. https://codereview.chromium.org/256143006/diff/320002/chrome/browser/metrics/... File chrome/browser/metrics/metrics_service.h (right): https://codereview.chromium.org/256143006/diff/320002/chrome/browser/metrics/... chrome/browser/metrics/metrics_service.h:174: // returned. On 2014/05/05 15:59:12, Nico wrote: > (nit: First sentence uses active voice, second uses passive voice. "Otherwise, > it returns an…" reads better) Done. https://codereview.chromium.org/256143006/diff/320002/chrome/browser/metrics/... File chrome/browser/metrics/metrics_state_manager.h (right): https://codereview.chromium.org/256143006/diff/320002/chrome/browser/metrics/... chrome/browser/metrics/metrics_state_manager.h:53: // returned. On 2014/05/05 15:59:12, Nico wrote: > (ditto) Done.
The CQ bit was checked by asvitkine@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/asvitkine@chromium.org/256143006/350001
Message was sent while issue was closed.
Change committed as 268232
Message was sent while issue was closed.
Hmm, this got reverted because apparently it hit the static initializers check. The only thing suspicious in this change is a static bool class variable, i.e.: // static bool MetricsStateManager::instance_exists_ = false; But since this is a primitive, my understanding is it shouldn't result in a static initializer.... so it seems pretty bizarre to me. Anyone have any ideas why this CL would introduce a static initializer?
Message was sent while issue was closed.
On 2014/05/05 19:58:58, Alexei Svitkine wrote:
> Hmm, this got reverted because apparently it hit the static initializers
check.
>
> The only thing suspicious in this change is a static bool class variable,
i.e.:
>
> // static
> bool MetricsStateManager::instance_exists_ = false;
>
> But since this is a primitive, my understanding is it shouldn't result in a
> static initializer.... so it seems pretty bizarre to me.
>
> Anyone have any ideas why this CL would introduce a static initializer?
For example, how is the above different than these cases from field_trial.cc:
const int FieldTrial::kNotFinalized = -1;
const int FieldTrial::kDefaultGroupNumber = 0;
bool FieldTrial::enable_benchmarking_ = false;
const char FieldTrialList::kPersistentStringSeparator('/');
int FieldTrialList::kNoExpirationYear = 0;
Which presumably don't generate static initializers?
Message was sent while issue was closed.
On 2014/05/05 20:00:22, Alexei Svitkine wrote: > > // static > > bool MetricsStateManager::instance_exists_ = false; Seems strange to me too (that this would trip the check). Granted, as sheriff, I'm supposed to revert suspicious things first. It's possible there was another CL responsible, and I will find out shortly (once the next Linux build happens). Out of curiosity, if MetricsStateManager is a singleton, why have this bool at all, and not use one of the appropriate mechanisms in src/base? Also, relevant past discussion on this issue: http://neugierig.org/software/chromium/notes/2011/08/static-initializers.html
Message was sent while issue was closed.
Diff of the static initializers shows that these are the two new ones: # ppb_nacl_private_impl.cc nacl::(anonymous namespace)::kPNaClManifestId # shadervars.cpp gl::BlockMemberInfo::defaultBlockInfo # shadervars.cpp gl::BlockMemberInfo::defaultBlockInfo+0x4 # shadervars.cpp gl::BlockMemberInfo::defaultBlockInfo+0x8 # shadervars.cpp gl::BlockMemberInfo::defaultBlockInfo+0xc Your CL touches neither of these files, so I think you got misreverted.
Yep, just discovered this myself. I'll reapply Alexei's change. -Yuri On Mon, May 5, 2014 at 1:17 PM, <thakis@chromium.org> wrote: > Diff of the static initializers shows that these are the two new ones: > > # ppb_nacl_private_impl.cc nacl::(anonymous namespace)::kPNaClManifestId > # shadervars.cpp gl::BlockMemberInfo::defaultBlockInfo > # shadervars.cpp gl::BlockMemberInfo::defaultBlockInfo+0x4 > # shadervars.cpp gl::BlockMemberInfo::defaultBlockInfo+0x8 > # shadervars.cpp gl::BlockMemberInfo::defaultBlockInfo+0xc > > Your CL touches neither of these files, so I think you got misreverted. > > https://codereview.chromium.org/256143006/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
On 2014/05/05 20:17:14, Nico wrote: > Diff of the static initializers shows that these are the two new ones: > > # ppb_nacl_private_impl.cc nacl::(anonymous namespace)::kPNaClManifestId > # shadervars.cpp gl::BlockMemberInfo::defaultBlockInfo > # shadervars.cpp gl::BlockMemberInfo::defaultBlockInfo+0x4 > # shadervars.cpp gl::BlockMemberInfo::defaultBlockInfo+0x8 > # shadervars.cpp gl::BlockMemberInfo::defaultBlockInfo+0xc > > Your CL touches neither of these files, so I think you got misreverted. I think the culprits are https://codereview.chromium.org/264943003 for the nacl one, and the angle roll for the shadervars.cpp one.
Agreed. Reverting now. On Mon, May 5, 2014 at 1:19 PM, <thakis@chromium.org> wrote: > On 2014/05/05 20:17:14, Nico wrote: > >> Diff of the static initializers shows that these are the two new ones: >> > > # ppb_nacl_private_impl.cc nacl::(anonymous namespace)::kPNaClManifestId >> # shadervars.cpp gl::BlockMemberInfo::defaultBlockInfo >> # shadervars.cpp gl::BlockMemberInfo::defaultBlockInfo+0x4 >> # shadervars.cpp gl::BlockMemberInfo::defaultBlockInfo+0x8 >> # shadervars.cpp gl::BlockMemberInfo::defaultBlockInfo+0xc >> > > Your CL touches neither of these files, so I think you got misreverted. >> > > I think the culprits are https://codereview.chromium.org/264943003 for > the nacl > one, and the angle roll for the shadervars.cpp one. > > https://codereview.chromium.org/256143006/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Thanks! On Mon, May 5, 2014 at 4:21 PM, Yuri Wiitala <miu@chromium.org> wrote: > Agreed. Reverting now. > > > On Mon, May 5, 2014 at 1:19 PM, <thakis@chromium.org> wrote: > >> On 2014/05/05 20:17:14, Nico wrote: >> >>> Diff of the static initializers shows that these are the two new ones: >>> >> >> # ppb_nacl_private_impl.cc nacl::(anonymous namespace)::kPNaClManifestId >>> # shadervars.cpp gl::BlockMemberInfo::defaultBlockInfo >>> # shadervars.cpp gl::BlockMemberInfo::defaultBlockInfo+0x4 >>> # shadervars.cpp gl::BlockMemberInfo::defaultBlockInfo+0x8 >>> # shadervars.cpp gl::BlockMemberInfo::defaultBlockInfo+0xc >>> >> >> Your CL touches neither of these files, so I think you got misreverted. >>> >> >> I think the culprits are https://codereview.chromium.org/264943003 for >> the nacl >> one, and the angle roll for the shadervars.cpp one. >> >> https://codereview.chromium.org/256143006/ >> > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Mon, May 5, 2014 at 4:05 PM, <miu@chromium.org> wrote: > > Out of curiosity, if MetricsStateManager is a singleton, why have this > bool at > all, and not use one of the appropriate mechanisms in src/base? Also, > relevant > past discussion on this issue: > http://neugierig.org/software/chromium/notes/2011/08/static- > initializers.html A singleton generally lets any piece of code call GetInstance() to get access to it. Here, we wanted to restrict the class's direct use to be only by the MetricsService and related code, so we went with this technique instead. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. |
