Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
441013002:
Eliminate MetricsProvider::RecordCurrentState() (Closed)
|
Created:
6 years, 4 months ago by gayane -on leave until 09-2017 Modified:
6 years, 4 months ago Reviewers:
Alexei Svitkine (slow) CC:
chromium-reviews, Ilya Sherman, asvitkine+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionSync plugin list to local state after plugin change with delayed tasks
BUG=379148
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=287920
Patch Set 1 #
Total comments: 16
Patch Set 2 : RecordCurrentState is called before reading from prefs and delayed calls are canceled. #
Total comments: 10
Patch Set 3 : minor fixes and name improvements #
Total comments: 10
Patch Set 4 : Minor fixes and new unittest #
Total comments: 6
Patch Set 5 : last changes #
Messages
Total messages: 27 (0 generated)
Here is my first draft.
Looks great, some comments below. Thanks! https://codereview.chromium.org/441013002/diff/1/chrome/browser/metrics/plugi... File chrome/browser/metrics/plugin_metrics_provider.cc (right): https://codereview.chromium.org/441013002/diff/1/chrome/browser/metrics/plugi... chrome/browser/metrics/plugin_metrics_provider.cc:26: const int kRecordStateDelaySec = 10 * 60; I think we can make the delay much shorter - e.g. 15s. Also, move it to the anon namespace just below. https://codereview.chromium.org/441013002/diff/1/chrome/browser/metrics/plugi... chrome/browser/metrics/plugin_metrics_provider.cc:132: metrics::SystemProfileProto* system_profile_proto) { So this function actually reads the info from prefs. I think we need a bit of special handling here to handle the case where this is called and there's a pending delayed task running. In that case, I think we should write out the prefs immediately and cancel the task before executing the rest of this function. https://codereview.chromium.org/441013002/diff/1/chrome/browser/metrics/plugi... chrome/browser/metrics/plugin_metrics_provider.cc:363: if (!weak_ptr_factory_.HasWeakPtrs()) { Nit: Chromium style prefers early returns rather than nesting - so reverse the cond and do an early return here. https://codereview.chromium.org/441013002/diff/1/chrome/browser/metrics/plugi... File chrome/browser/metrics/plugin_metrics_provider.h (right): https://codereview.chromium.org/441013002/diff/1/chrome/browser/metrics/plugi... chrome/browser/metrics/plugin_metrics_provider.h:28: extern const int kRecordStateDelaySec; I don't think this needs to be in the header. https://codereview.chromium.org/441013002/diff/1/chrome/browser/metrics/plugi... chrome/browser/metrics/plugin_metrics_provider.h:62: // Posts delayed execution for RecordCurrentState. Returns true if new task is Nit: "delayed execution" -> "a delayed task" https://codereview.chromium.org/441013002/diff/1/chrome/browser/metrics/plugi... chrome/browser/metrics/plugin_metrics_provider.h:64: bool DelayedRecordCurrentState(int delay_sec); Move this function definition above the static functions above. Document that the param is for testing and probably make the unit millis so that the test can specify an even smaller interval. https://codereview.chromium.org/441013002/diff/1/chrome/browser/metrics/plugi... File chrome/browser/metrics/plugin_metrics_provider_unittest.cc (right): https://codereview.chromium.org/441013002/diff/1/chrome/browser/metrics/plugi... chrome/browser/metrics/plugin_metrics_provider_unittest.cc:113: EXPECT_EQ(true, provider.DelayedRecordCurrentState(delay_sec)); Nit: use EXPECT_TRUE and EXPECT_FALSE https://codereview.chromium.org/441013002/diff/1/chrome/browser/metrics/plugi... chrome/browser/metrics/plugin_metrics_provider_unittest.cc:116: sleep(delay_sec); Use base::PlatformThread::Sleep() instead.
https://codereview.chromium.org/441013002/diff/1/chrome/browser/metrics/plugi... File chrome/browser/metrics/plugin_metrics_provider.cc (right): https://codereview.chromium.org/441013002/diff/1/chrome/browser/metrics/plugi... chrome/browser/metrics/plugin_metrics_provider.cc:26: const int kRecordStateDelaySec = 10 * 60; On 2014/08/05 20:13:11, Alexei Svitkine wrote: > I think we can make the delay much shorter - e.g. 15s. > > Also, move it to the anon namespace just below. Done. https://codereview.chromium.org/441013002/diff/1/chrome/browser/metrics/plugi... chrome/browser/metrics/plugin_metrics_provider.cc:132: metrics::SystemProfileProto* system_profile_proto) { On 2014/08/05 20:13:12, Alexei Svitkine wrote: > So this function actually reads the info from prefs. > > I think we need a bit of special handling here to handle the case where this is > called and there's a pending delayed task running. In that case, I think we > should write out the prefs immediately and cancel the task before executing the > rest of this function. Added a ForcedRecordCurrentState function and called it in the beginning of this function. https://codereview.chromium.org/441013002/diff/1/chrome/browser/metrics/plugi... chrome/browser/metrics/plugin_metrics_provider.cc:363: if (!weak_ptr_factory_.HasWeakPtrs()) { On 2014/08/05 20:13:11, Alexei Svitkine wrote: > Nit: Chromium style prefers early returns rather than nesting - so reverse the > cond and do an early return here. Done. https://codereview.chromium.org/441013002/diff/1/chrome/browser/metrics/plugi... File chrome/browser/metrics/plugin_metrics_provider.h (right): https://codereview.chromium.org/441013002/diff/1/chrome/browser/metrics/plugi... chrome/browser/metrics/plugin_metrics_provider.h:28: extern const int kRecordStateDelaySec; On 2014/08/05 20:13:12, Alexei Svitkine wrote: > I don't think this needs to be in the header. Done. https://codereview.chromium.org/441013002/diff/1/chrome/browser/metrics/plugi... chrome/browser/metrics/plugin_metrics_provider.h:62: // Posts delayed execution for RecordCurrentState. Returns true if new task is On 2014/08/05 20:13:12, Alexei Svitkine wrote: > Nit: "delayed execution" -> "a delayed task" Done. https://codereview.chromium.org/441013002/diff/1/chrome/browser/metrics/plugi... chrome/browser/metrics/plugin_metrics_provider.h:64: bool DelayedRecordCurrentState(int delay_sec); On 2014/08/05 20:13:12, Alexei Svitkine wrote: > Move this function definition above the static functions above. > > Document that the param is for testing and probably make the unit millis so that > the test can specify an even smaller interval. Done. https://codereview.chromium.org/441013002/diff/1/chrome/browser/metrics/plugi... File chrome/browser/metrics/plugin_metrics_provider_unittest.cc (right): https://codereview.chromium.org/441013002/diff/1/chrome/browser/metrics/plugi... chrome/browser/metrics/plugin_metrics_provider_unittest.cc:113: EXPECT_EQ(true, provider.DelayedRecordCurrentState(delay_sec)); On 2014/08/05 20:13:12, Alexei Svitkine wrote: > Nit: use EXPECT_TRUE and EXPECT_FALSE Done. https://codereview.chromium.org/441013002/diff/1/chrome/browser/metrics/plugi... chrome/browser/metrics/plugin_metrics_provider_unittest.cc:116: sleep(delay_sec); On 2014/08/05 20:13:12, Alexei Svitkine wrote: > Use base::PlatformThread::Sleep() instead. Done.
Almost there, few more comments. https://codereview.chromium.org/441013002/diff/40001/chrome/browser/metrics/p... File chrome/browser/metrics/plugin_metrics_provider.cc (right): https://codereview.chromium.org/441013002/diff/40001/chrome/browser/metrics/p... chrome/browser/metrics/plugin_metrics_provider.cc:366: } else { Nit: No else after early return. Here and in the function below. https://codereview.chromium.org/441013002/diff/40001/chrome/browser/metrics/p... File chrome/browser/metrics/plugin_metrics_provider.h (right): https://codereview.chromium.org/441013002/diff/40001/chrome/browser/metrics/p... chrome/browser/metrics/plugin_metrics_provider.h:56: bool DelayedRecordCurrentState(int delay_ms); Nit: How about RecordCurrentStateWithDelay(). (It's better to stay method names with a verb.) https://codereview.chromium.org/441013002/diff/40001/chrome/browser/metrics/p... chrome/browser/metrics/plugin_metrics_provider.h:58: // If a delayed RecordCurrnetState task exists than cancel it, call Nit: Use the same verb tense here as a above (i.e. instead of "cancel", say "cancels"... "calls", "returns", etc). https://codereview.chromium.org/441013002/diff/40001/chrome/browser/metrics/p... chrome/browser/metrics/plugin_metrics_provider.h:60: bool ForcedRecordCurrentState(); Nit: How about RecordCurrentStateIfPending(). https://codereview.chromium.org/441013002/diff/40001/chrome/browser/metrics/p... File chrome/browser/metrics/plugin_metrics_provider_unittest.cc (right): https://codereview.chromium.org/441013002/diff/40001/chrome/browser/metrics/p... chrome/browser/metrics/plugin_metrics_provider_unittest.cc:137: // if ForcedRecordCurrentState was successful then we should be able to post Nit: Capitalize if.
https://codereview.chromium.org/441013002/diff/40001/chrome/browser/metrics/p... File chrome/browser/metrics/plugin_metrics_provider.cc (right): https://codereview.chromium.org/441013002/diff/40001/chrome/browser/metrics/p... chrome/browser/metrics/plugin_metrics_provider.cc:366: } else { On 2014/08/05 21:35:21, Alexei Svitkine wrote: > Nit: No else after early return. Here and in the function below. Done. https://codereview.chromium.org/441013002/diff/40001/chrome/browser/metrics/p... File chrome/browser/metrics/plugin_metrics_provider.h (right): https://codereview.chromium.org/441013002/diff/40001/chrome/browser/metrics/p... chrome/browser/metrics/plugin_metrics_provider.h:56: bool DelayedRecordCurrentState(int delay_ms); On 2014/08/05 21:35:21, Alexei Svitkine wrote: > Nit: How about RecordCurrentStateWithDelay(). (It's better to stay method names > with a verb.) Done. https://codereview.chromium.org/441013002/diff/40001/chrome/browser/metrics/p... chrome/browser/metrics/plugin_metrics_provider.h:58: // If a delayed RecordCurrnetState task exists than cancel it, call On 2014/08/05 21:35:21, Alexei Svitkine wrote: > Nit: Use the same verb tense here as a above (i.e. instead of "cancel", say > "cancels"... "calls", "returns", etc). Done. https://codereview.chromium.org/441013002/diff/40001/chrome/browser/metrics/p... chrome/browser/metrics/plugin_metrics_provider.h:60: bool ForcedRecordCurrentState(); On 2014/08/05 21:35:21, Alexei Svitkine wrote: > Nit: How about RecordCurrentStateIfPending(). Done. https://codereview.chromium.org/441013002/diff/40001/chrome/browser/metrics/p... File chrome/browser/metrics/plugin_metrics_provider_unittest.cc (right): https://codereview.chromium.org/441013002/diff/40001/chrome/browser/metrics/p... chrome/browser/metrics/plugin_metrics_provider_unittest.cc:137: // if ForcedRecordCurrentState was successful then we should be able to post On 2014/08/05 21:35:21, Alexei Svitkine wrote: > Nit: Capitalize if. Done.
https://codereview.chromium.org/441013002/diff/60001/chrome/browser/metrics/p... File chrome/browser/metrics/plugin_metrics_provider.cc (right): https://codereview.chromium.org/441013002/diff/60001/chrome/browser/metrics/p... chrome/browser/metrics/plugin_metrics_provider.cc:133: Nit: No blank line at start of method body. Instead, I suggest adding one after the method call. https://codereview.chromium.org/441013002/diff/60001/chrome/browser/metrics/p... File chrome/browser/metrics/plugin_metrics_provider.h (right): https://codereview.chromium.org/441013002/diff/60001/chrome/browser/metrics/p... chrome/browser/metrics/plugin_metrics_provider.h:45: void RecordCurrentState(); Now that this is no longer an implementation of the interface, please add a comment for the method and move it to the private section. https://codereview.chromium.org/441013002/diff/60001/chrome/browser/metrics/p... chrome/browser/metrics/plugin_metrics_provider.h:56: bool RecordCurrentStateWithDelay(int delay_ms); The new methods you're adding shouldn't be public. Instead, make them private and you can set the new test to be a friend. (See variations_service.h for an example how to do that with the FRIEND_TEST macros). https://codereview.chromium.org/441013002/diff/60001/chrome/browser/metrics/p... chrome/browser/metrics/plugin_metrics_provider.h:58: // If a delayed RecordCurrnetState task exists than cancels it, calls nit: than -> then https://codereview.chromium.org/441013002/diff/60001/chrome/browser/metrics/p... File chrome/browser/metrics/plugin_metrics_provider_unittest.cc (right): https://codereview.chromium.org/441013002/diff/60001/chrome/browser/metrics/p... chrome/browser/metrics/plugin_metrics_provider_unittest.cc:122: TEST(PluginMetricsProviderTest, CheckForcedRecordCurrentState) { Can you also add a test that verifies that ProvideStabilityMetrics() will save the prefs if there's a pending task? You can do that by actually calling some of the listener methods (e.g. BrowserChildProcessInstanceCreated) which should queue up a task and then calling ProvideStabilityMetrics() and checking if the proto is filled with the correct values.
https://codereview.chromium.org/441013002/diff/60001/chrome/browser/metrics/p... File chrome/browser/metrics/plugin_metrics_provider.cc (right): https://codereview.chromium.org/441013002/diff/60001/chrome/browser/metrics/p... chrome/browser/metrics/plugin_metrics_provider.cc:133: On 2014/08/06 15:10:01, Alexei Svitkine wrote: > Nit: No blank line at start of method body. Instead, I suggest adding one after > the method call. Done. https://codereview.chromium.org/441013002/diff/60001/chrome/browser/metrics/p... File chrome/browser/metrics/plugin_metrics_provider.h (right): https://codereview.chromium.org/441013002/diff/60001/chrome/browser/metrics/p... chrome/browser/metrics/plugin_metrics_provider.h:45: void RecordCurrentState(); On 2014/08/06 15:10:01, Alexei Svitkine wrote: > Now that this is no longer an implementation of the interface, please add a > comment for the method and move it to the private section. Done. https://codereview.chromium.org/441013002/diff/60001/chrome/browser/metrics/p... chrome/browser/metrics/plugin_metrics_provider.h:56: bool RecordCurrentStateWithDelay(int delay_ms); On 2014/08/06 15:10:01, Alexei Svitkine wrote: > The new methods you're adding shouldn't be public. Instead, make them private > and you can set the new test to be a friend. (See variations_service.h for an > example how to do that with the FRIEND_TEST macros). Done. https://codereview.chromium.org/441013002/diff/60001/chrome/browser/metrics/p... chrome/browser/metrics/plugin_metrics_provider.h:58: // If a delayed RecordCurrnetState task exists than cancels it, calls On 2014/08/06 15:10:01, Alexei Svitkine wrote: > nit: than -> then Done. https://codereview.chromium.org/441013002/diff/60001/chrome/browser/metrics/p... File chrome/browser/metrics/plugin_metrics_provider_unittest.cc (right): https://codereview.chromium.org/441013002/diff/60001/chrome/browser/metrics/p... chrome/browser/metrics/plugin_metrics_provider_unittest.cc:122: TEST(PluginMetricsProviderTest, CheckForcedRecordCurrentState) { On 2014/08/06 15:10:01, Alexei Svitkine wrote: > Can you also add a test that verifies that ProvideStabilityMetrics() will save > the prefs if there's a pending task? > > You can do that by actually calling some of the listener methods (e.g. > BrowserChildProcessInstanceCreated) which should queue up a task and then > calling ProvideStabilityMetrics() and checking if the proto is filled with the > correct values. Done.
LGTM % remaining comments, good work! https://codereview.chromium.org/441013002/diff/80001/chrome/browser/metrics/p... File chrome/browser/metrics/plugin_metrics_provider.h (right): https://codereview.chromium.org/441013002/diff/80001/chrome/browser/metrics/p... chrome/browser/metrics/plugin_metrics_provider.h:77: // Saves plugin information to local state Nit: add a . https://codereview.chromium.org/441013002/diff/80001/chrome/browser/metrics/p... File chrome/browser/metrics/plugin_metrics_provider_unittest.cc (right): https://codereview.chromium.org/441013002/diff/80001/chrome/browser/metrics/p... chrome/browser/metrics/plugin_metrics_provider_unittest.cc:43: class PluginMetricsProviderTest : public ::testing::Test { Move this to the anon namespace above. https://codereview.chromium.org/441013002/diff/80001/chrome/browser/metrics/p... chrome/browser/metrics/plugin_metrics_provider_unittest.cc:49: PluginMetricsProvider::RegisterPrefs(prefs()->registry()); Nit: You can do this in the ctor and remove this method.
The CQ bit was checked by gayane@chromium.org
The CQ bit was unchecked by gayane@chromium.org
The CQ bit was checked by gayane@chromium.org
The CQ bit was unchecked by gayane@chromium.org
The CQ bit was checked by gayane@chromium.org
The CQ bit was unchecked by gayane@chromium.org
The CQ bit was checked by gayane@chromium.org
The CQ bit was unchecked by gayane@chromium.org
The CQ bit was checked by gayane@chromium.org
The CQ bit was unchecked by gayane@chromium.org
The CQ bit was checked by gayane@chromium.org
The CQ bit was unchecked by gayane@chromium.org
The CQ bit was checked by gayane@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gayane@chromium.org/441013002/100001
last changes https://codereview.chromium.org/441013002/diff/80001/chrome/browser/metrics/p... File chrome/browser/metrics/plugin_metrics_provider.h (right): https://codereview.chromium.org/441013002/diff/80001/chrome/browser/metrics/p... chrome/browser/metrics/plugin_metrics_provider.h:77: // Saves plugin information to local state On 2014/08/06 19:43:40, Alexei Svitkine wrote: > Nit: add a . Done. https://codereview.chromium.org/441013002/diff/80001/chrome/browser/metrics/p... File chrome/browser/metrics/plugin_metrics_provider_unittest.cc (right): https://codereview.chromium.org/441013002/diff/80001/chrome/browser/metrics/p... chrome/browser/metrics/plugin_metrics_provider_unittest.cc:43: class PluginMetricsProviderTest : public ::testing::Test { On 2014/08/06 19:43:40, Alexei Svitkine wrote: > Move this to the anon namespace above. Done. https://codereview.chromium.org/441013002/diff/80001/chrome/browser/metrics/p... chrome/browser/metrics/plugin_metrics_provider_unittest.cc:49: PluginMetricsProvider::RegisterPrefs(prefs()->registry()); On 2014/08/06 19:43:40, Alexei Svitkine wrote: > Nit: You can do this in the ctor and remove this method. Done.
The CQ bit was unchecked by gayane@chromium.org
The CQ bit was checked by gayane@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gayane@chromium.org/441013002/100001
Message was sent while issue was closed.
Change committed as 287920 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
