Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1683583002:
Report user actions when gesture starts and stops in user_model.
|
Created:
4 years, 10 months ago by beaudoin Modified:
4 years, 7 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, jam, darin-cc_chromium.org, asvitkine+watch_chromium.org, mkwst+moarreviews-renderer_chromium.org, scheduler-bugs_chromium.org, Ilya Sherman Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionReport user actions when gesture starts and stops in user_model.
This will makes it possible to build and validate a model for time-to-next gesture.
BUG=595431
Patch Set 1 #
Total comments: 14
Patch Set 2 : Answered comments. #Patch Set 3 : Rebase. #
Total comments: 6
Patch Set 4 : #Patch Set 5 : Fixed test #
Total comments: 2
Patch Set 6 : Caching feature. #
Total comments: 6
Patch Set 7 : #
Total comments: 2
Patch Set 8 : Fixed nit. #
Total comments: 22
Patch Set 9 : Answered Ilya. #Patch Set 10 : git cl format #Patch Set 11 : Added test removed by merging problem. #
Total comments: 6
Patch Set 12 : Rebased, fixed nits. #Patch Set 13 : Mini fix #
Total comments: 2
Patch Set 14 : Removed cached Feature, answered piman. #Patch Set 15 : Do not register action callback in single process. #Messages
Total messages: 56 (11 generated)
beaudoin@chromium.org changed reviewers: + alexclarke@chromium.org, isherman@chromium.org, skyostil@chromium.org
Following up on discussion on uma-users@ please let me know if this makes sense.
isherman@chromium.org changed reviewers: + asvitkine@chromium.org
This seems pretty reasonable to me! I'm not actually sure about the frequency of the actions. We do record a user action for every page load, which I'm guessing might be a similar order of magnitude of user actions? Or maybe these would actually be an order of magnitude more frequent -- wdyt? It might help to better understand your use case, in terms of offering guidance for the specific metrics. What sort of analysis are you hoping to perform based on these metrics? https://codereview.chromium.org/1683583002/diff/1/components/scheduler/render... File components/scheduler/renderer/user_model.cc (right): https://codereview.chromium.org/1683583002/diff/1/components/scheduler/render... components/scheduler/renderer/user_model.cc:51: task_runner_->PostTask(FROM_HERE, base::Bind(&base::RecordAction, This is pretty weirdly wrapped -- is this clang-formatted? https://codereview.chromium.org/1683583002/diff/1/components/scheduler/render... components/scheduler/renderer/user_model.cc:105: // NULL when testing. Are there any tests that do end up exercising this code? Should there be? https://codereview.chromium.org/1683583002/diff/1/content/renderer/render_thr... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/1683583002/diff/1/content/renderer/render_thr... content/renderer/render_thread_impl.cc:640: base::Unretained(this))); Should the callback be removed in the destructor or something?
https://codereview.chromium.org/1683583002/diff/1/components/scheduler/render... File components/scheduler/renderer/user_model.cc (right): https://codereview.chromium.org/1683583002/diff/1/components/scheduler/render... components/scheduler/renderer/user_model.cc:105: // NULL when testing. On 2016/02/09 22:03:25, Ilya Sherman wrote: > Are there any tests that do end up exercising this code? Should there be? Looks like it would be possible to test this via AddActionCallback. https://codereview.chromium.org/1683583002/diff/1/components/scheduler/render... File components/scheduler/renderer/user_model.h (right): https://codereview.chromium.org/1683583002/diff/1/components/scheduler/render... components/scheduler/renderer/user_model.h:75: scoped_refptr<base::SingleThreadTaskRunner> task_runner_; s/task_runner_/default_task_runner_ Reason being there are many types of task runner used in the scheduler and it would be good to know what sort this is when reading the UserModel code.
https://codereview.chromium.org/1683583002/diff/1/components/scheduler/render... File components/scheduler/renderer/user_model.cc (right): https://codereview.chromium.org/1683583002/diff/1/components/scheduler/render... components/scheduler/renderer/user_model.cc:49: // NULL when testing. Could you instead pass in a mock task runner in the test to avoid having to specialize for tests here? https://codereview.chromium.org/1683583002/diff/1/components/scheduler/render... components/scheduler/renderer/user_model.cc:52: base::UserMetricsAction( Is there a version of UserMetricsAction that takes an explicit timestamp? I think the post task round trip might introduce significant error (several hundred ms) in the results, especially if the main thread is busy. With the timestamp API we could also update the statistics later on the correct thread instead of posting a task. Alternatively we could use the control task runner instead of the default one since it is always high priority.
https://codereview.chromium.org/1683583002/diff/1/components/scheduler/render... File components/scheduler/renderer/user_model.cc (right): https://codereview.chromium.org/1683583002/diff/1/components/scheduler/render... components/scheduler/renderer/user_model.cc:52: base::UserMetricsAction( On 2016/02/10 10:50:40, Sami wrote: > Is there a version of UserMetricsAction that takes an explicit timestamp? I > think the post task round trip might introduce significant error (several > hundred ms) in the results, especially if the main thread is busy. With the > timestamp API we could also update the statistics later on the correct thread > instead of posting a task. > > Alternatively we could use the control task runner instead of the default one > since it is always high priority. The PostTask delay is only part of the total delay -- there's also the IPC message. In general, user actions have not been used to try to measure very fine timings -- the timestamps are meant to provide a general idea of user event sequences. We can definitely think about extending the API; but if you need fine timings, that might suggest that user actions are not really the right model for representing these metrics.
Ah right, I forgot about the IPC too. Is there something UMA-based we could do here?
(Doesn't include a new patch yet, but wanted to send my comments and clarification.) So, Ilya, the idea here is to use the user action timings to build a user model of the markovian behavior of gestures. That is, we know the time-to-next gesture strongly depends on the timing of preceding gestures and want to figure out a good statistical model for this. Building a good model of this needs rapid iteration, so we can't really parameterize the entire thing and use Finch to play with the parameters. What's needed is a log of timestamped events. As I'm saying below, there will likely be a bias and noise in the timestamps, but I don't think it's a problem. 1) We'll have more than enough data for the noise to cancel out; 2) the bias doesn't matter since we're only interested in relative timings. Aside from building our own infrastructure for doing something exactly like user actions, I don't see how to do this. (If this was server side I'd just log a user id, a label and a timestamp, which is what user actions seems to be doing. :)) As for making the user action interface accept a timestamp, it's a pretty involved process. The method that gets the time is buried pretty deep: https://code.google.com/p/chromium/codesearch#chromium/src/components/metrics... I suggest we only go this route if we find the noise is too high to work with. As for doing it in the control_task_runner I think posting the AddCallback to that thread (and forcing everyone to RecordAction on that thread) would be weird. Again I suggest we check if the noise is too much before we do this. https://codereview.chromium.org/1683583002/diff/1/components/scheduler/render... File components/scheduler/renderer/user_model.cc (right): https://codereview.chromium.org/1683583002/diff/1/components/scheduler/render... components/scheduler/renderer/user_model.cc:52: base::UserMetricsAction( On 2016/02/10 22:43:04, Ilya Sherman wrote: > On 2016/02/10 10:50:40, Sami wrote: > > Is there a version of UserMetricsAction that takes an explicit timestamp? I > > think the post task round trip might introduce significant error (several > > hundred ms) in the results, especially if the main thread is busy. With the > > timestamp API we could also update the statistics later on the correct thread > > instead of posting a task. > > > > Alternatively we could use the control task runner instead of the default one > > since it is always high priority. > > The PostTask delay is only part of the total delay -- there's also the IPC > message. In general, user actions have not been used to try to measure very > fine timings -- the timestamps are meant to provide a general idea of user event > sequences. We can definitely think about extending the API; but if you need > fine timings, that might suggest that user actions are not really the right > model for representing these metrics. [Answered above] https://codereview.chromium.org/1683583002/diff/1/components/scheduler/render... components/scheduler/renderer/user_model.cc:105: // NULL when testing. On 2016/02/10 10:17:01, alexclarke1 wrote: > On 2016/02/09 22:03:25, Ilya Sherman wrote: > > Are there any tests that do end up exercising this code? Should there be? > > Looks like it would be possible to test this via AddActionCallback. From a quick look, many tests seems to be called with Gesture*End but I don't think any ensures is_gesture_active_ == true before they do. Therefore I'd say this specific line is not exercised. Not sure it's worth checking the mock itself.
PTAL. (Still in the process of moving this behind a Finch flag.) https://codereview.chromium.org/1683583002/diff/1/components/scheduler/render... File components/scheduler/renderer/user_model.cc (right): https://codereview.chromium.org/1683583002/diff/1/components/scheduler/render... components/scheduler/renderer/user_model.cc:49: // NULL when testing. On 2016/02/10 10:50:40, Sami wrote: > Could you instead pass in a mock task runner in the test to avoid having to > specialize for tests here? Done. https://codereview.chromium.org/1683583002/diff/1/components/scheduler/render... components/scheduler/renderer/user_model.cc:51: task_runner_->PostTask(FROM_HERE, base::Bind(&base::RecordAction, On 2016/02/09 22:03:25, Ilya Sherman wrote: > This is pretty weirdly wrapped -- is this clang-formatted? Forgot to run it, oops. Done. https://codereview.chromium.org/1683583002/diff/1/components/scheduler/render... File components/scheduler/renderer/user_model.h (right): https://codereview.chromium.org/1683583002/diff/1/components/scheduler/render... components/scheduler/renderer/user_model.h:75: scoped_refptr<base::SingleThreadTaskRunner> task_runner_; On 2016/02/10 10:17:01, alexclarke1 wrote: > s/task_runner_/default_task_runner_ > > Reason being there are many types of task runner used in the scheduler and it > would be good to know what sort this is when reading the UserModel code. Done. https://codereview.chromium.org/1683583002/diff/1/content/renderer/render_thr... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/1683583002/diff/1/content/renderer/render_thr... content/renderer/render_thread_impl.cc:640: base::Unretained(this))); On 2016/02/09 22:03:25, Ilya Sherman wrote: > Should the callback be removed in the destructor or something? Done.
I guess we can compare this against the corresponding UMA metric to see if the noise behaves as expected? https://codereview.chromium.org/1683583002/diff/40001/components/scheduler/re... File components/scheduler/renderer/renderer_scheduler_impl.h (right): https://codereview.chromium.org/1683583002/diff/40001/components/scheduler/re... components/scheduler/renderer/renderer_scheduler_impl.h:344: AnyThread(const scoped_refptr<base::SingleThreadTaskRunner>& task_runner); Please call this |default_task_runner| too.
https://codereview.chromium.org/1683583002/diff/40001/components/scheduler/re... File components/scheduler/renderer/user_model.h (right): https://codereview.chromium.org/1683583002/diff/40001/components/scheduler/re... components/scheduler/renderer/user_model.h:23: UserModel(const scoped_refptr<base::SingleThreadTaskRunner>& task_runner); nit: explicit and s/task_runner/default_task_runner https://codereview.chromium.org/1683583002/diff/40001/components/scheduler/re... File components/scheduler/renderer/user_model_unittest.cc (right): https://codereview.chromium.org/1683583002/diff/40001/components/scheduler/re... components/scheduler/renderer/user_model_unittest.cc:256: } // namespace scheduler Consider adding a simple test which uses base::AddActionCallback() to check we get the expected callbacks.
Description was changed from ========== Report user actions when gesture starts and stops in user_model. This will makes it possible to build and validate a model for time-to-next gesture ========== to ========== Report user actions when gesture starts and stops in user_model. This will makes it possible to build and validate a model for time-to-next gesture. BUG=595431 ==========
PTAL. Sorry this took a while, got sidetracked. The latest CL has a unit test and hides the recording behind the RecordGestureActions feature that will be controlled with Finch. I've also manually verified that the UMA user actions are correctly recorded when the feature is enabled. I've created a companion Google3 CL for the Finch config, currently under review. The feature is set at 0% for now in that other CL. The feature will only be enabled on a fraction of canary to start with, in order to get an idea of the volume of UMA user actions. I'll talk with the metrics team before enabling it on a larger population. If you want me to include you in the review of these Finch CLs let me know. https://codereview.chromium.org/1683583002/diff/40001/components/scheduler/re... File components/scheduler/renderer/renderer_scheduler_impl.h (right): https://codereview.chromium.org/1683583002/diff/40001/components/scheduler/re... components/scheduler/renderer/renderer_scheduler_impl.h:344: AnyThread(const scoped_refptr<base::SingleThreadTaskRunner>& task_runner); On 2016/02/12 12:39:43, Sami wrote: > Please call this |default_task_runner| too. Done. https://codereview.chromium.org/1683583002/diff/40001/components/scheduler/re... File components/scheduler/renderer/user_model.h (right): https://codereview.chromium.org/1683583002/diff/40001/components/scheduler/re... components/scheduler/renderer/user_model.h:23: UserModel(const scoped_refptr<base::SingleThreadTaskRunner>& task_runner); On 2016/02/12 16:21:50, alexclarke1 wrote: > nit: explicit and s/task_runner/default_task_runner Done. https://codereview.chromium.org/1683583002/diff/40001/components/scheduler/re... File components/scheduler/renderer/user_model_unittest.cc (right): https://codereview.chromium.org/1683583002/diff/40001/components/scheduler/re... components/scheduler/renderer/user_model_unittest.cc:256: } // namespace scheduler On 2016/02/12 16:21:50, alexclarke1 wrote: > Consider adding a simple test which uses base::AddActionCallback() to check we > get the expected callbacks. Done.
Thanks for the test. Just the one issue left that I can see. https://codereview.chromium.org/1683583002/diff/80001/components/scheduler/re... File components/scheduler/renderer/user_model.cc (right): https://codereview.chromium.org/1683583002/diff/80001/components/scheduler/re... components/scheduler/renderer/user_model.cc:57: if (base::FeatureList::IsEnabled(features::kRecordGestureAction)) { I wonder if it's worth caching this? Sami WDYT?
https://codereview.chromium.org/1683583002/diff/80001/components/scheduler/re... File components/scheduler/renderer/user_model.cc (right): https://codereview.chromium.org/1683583002/diff/80001/components/scheduler/re... components/scheduler/renderer/user_model.cc:57: if (base::FeatureList::IsEnabled(features::kRecordGestureAction)) { On 2016/03/17 17:11:11, alexclarke1 wrote: > I wonder if it's worth caching this? Sami WDYT? Good call. Looking at the code, it's a map<> lookup from a string, so it's probably worth caching. I'll do it...
PTAL. (Added the caching.)
lgtm
https://codereview.chromium.org/1683583002/diff/100001/components/scheduler/r... File components/scheduler/renderer/user_model.cc (right): https://codereview.chromium.org/1683583002/diff/100001/components/scheduler/r... components/scheduler/renderer/user_model.cc:33: // Enables the recording of gesture as UMA user actions. Maybe add a comment about the reasoning for this being off by default. (i.e. too much data) https://codereview.chromium.org/1683583002/diff/100001/components/scheduler/r... components/scheduler/renderer/user_model.cc:34: const base::Feature kRecordGestureAction = {"RecordGestureActions", Nit: No need for =. https://codereview.chromium.org/1683583002/diff/100001/components/scheduler/r... File components/scheduler/renderer/user_model_unittest.cc (right): https://codereview.chromium.org/1683583002/diff/100001/components/scheduler/r... components/scheduler/renderer/user_model_unittest.cc:60: std::string lastActionRecorded_; hacker_style, please.
Answered Alexei. PTAL. https://codereview.chromium.org/1683583002/diff/100001/components/scheduler/r... File components/scheduler/renderer/user_model.cc (right): https://codereview.chromium.org/1683583002/diff/100001/components/scheduler/r... components/scheduler/renderer/user_model.cc:33: // Enables the recording of gesture as UMA user actions. On 2016/03/17 21:38:46, Alexei Svitkine (very slow) wrote: > Maybe add a comment about the reasoning for this being off by default. (i.e. too > much data) Done. https://codereview.chromium.org/1683583002/diff/100001/components/scheduler/r... components/scheduler/renderer/user_model.cc:34: const base::Feature kRecordGestureAction = {"RecordGestureActions", On 2016/03/17 21:38:46, Alexei Svitkine (very slow) wrote: > Nit: No need for =. Done. https://codereview.chromium.org/1683583002/diff/100001/components/scheduler/r... File components/scheduler/renderer/user_model_unittest.cc (right): https://codereview.chromium.org/1683583002/diff/100001/components/scheduler/r... components/scheduler/renderer/user_model_unittest.cc:60: std::string lastActionRecorded_; On 2016/03/17 21:38:46, Alexei Svitkine (very slow) wrote: > hacker_style, please. Done.
lgtm https://codereview.chromium.org/1683583002/diff/120001/components/scheduler/r... File components/scheduler/renderer/user_model.cc (right): https://codereview.chromium.org/1683583002/diff/120001/components/scheduler/r... components/scheduler/renderer/user_model.cc:34: // That feature is disabled by default and controlled with Finch so as not to Nit: Finch is an internal keyword. You can say "controlled from the server".
https://codereview.chromium.org/1683583002/diff/120001/components/scheduler/r... File components/scheduler/renderer/user_model.cc (right): https://codereview.chromium.org/1683583002/diff/120001/components/scheduler/r... components/scheduler/renderer/user_model.cc:34: // That feature is disabled by default and controlled with Finch so as not to On 2016/03/18 16:30:36, Alexei Svitkine (very slow) wrote: > Nit: Finch is an internal keyword. You can say "controlled from the server". (Sad, such a great name! :)) Done.
A few comments, though no need to block on my further review once you address these, since I'll be out for the next week. https://codereview.chromium.org/1683583002/diff/140001/components/scheduler/r... File components/scheduler/renderer/user_model.cc (right): https://codereview.chromium.org/1683583002/diff/140001/components/scheduler/r... components/scheduler/renderer/user_model.cc:33: // Enables the recording of gesture as UMA user actions. nit: s/gesture/gestures https://codereview.chromium.org/1683583002/diff/140001/components/scheduler/r... components/scheduler/renderer/user_model.cc:47: record_gesture_action_( Hmm, why do you need to cache this state? base::FeatureList::IsEnabled() should be pretty efficient. Did you find that it's not sufficiently efficient for your needs? https://codereview.chromium.org/1683583002/diff/140001/components/scheduler/r... File components/scheduler/renderer/user_model_unittest.cc (right): https://codereview.chromium.org/1683583002/diff/140001/components/scheduler/r... components/scheduler/renderer/user_model_unittest.cc:31: base::RemoveActionCallback(action_callback_); Why is this line needed here? Seems like something that should be done in teardown. (FWIW, I also find it pretty confusing that you both have a constructor and a setup method, both of which seem to do initialization work. Is there any particular reason not to do all of the work in the constructor?) https://codereview.chromium.org/1683583002/diff/140001/components/scheduler/r... components/scheduler/renderer/user_model_unittest.cc:48: void RecordComputedAction(const std::string& action) { nit: What does the "Computed" part of this name refer to? https://codereview.chromium.org/1683583002/diff/140001/components/scheduler/r... components/scheduler/renderer/user_model_unittest.cc:49: num_actions_recorded_++; nit: preincrement https://codereview.chromium.org/1683583002/diff/140001/components/scheduler/r... components/scheduler/renderer/user_model_unittest.cc:268: ""); nit: Prefer std::string() to an empty string literal. https://codereview.chromium.org/1683583002/diff/140001/components/scheduler/r... components/scheduler/renderer/user_model_unittest.cc:291: EXPECT_EQ("RendererScheduler.UserModel.GestureEnd", last_action_recorded_); It would probably be better to use the code from base/test/user_action_tester.h rather than re-implementing it, if possible. I'm not 100% sure whether the code would Just Work in the renderer process, though. https://codereview.chromium.org/1683583002/diff/140001/content/renderer/rende... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/1683583002/diff/140001/content/renderer/rende... content/renderer/render_thread_impl.cc:637: base::RemoveActionCallback(action_callback_); Hmm, why is this line needed? https://codereview.chromium.org/1683583002/diff/140001/tools/metrics/actions/... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/1683583002/diff/140001/tools/metrics/actions/... tools/metrics/actions/actions.xml:11737: scheduler is made aware of it. Are you sure you don't want to break this metric down by the specific gesture?
https://codereview.chromium.org/1683583002/diff/140001/components/scheduler/r... File components/scheduler/renderer/user_model.cc (right): https://codereview.chromium.org/1683583002/diff/140001/components/scheduler/r... components/scheduler/renderer/user_model.cc:47: record_gesture_action_( On 2016/03/21 08:09:43, Ilya Sherman (Away Mar 19-27) wrote: > Hmm, why do you need to cache this state? base::FeatureList::IsEnabled() should > be pretty efficient. Did you find that it's not sufficiently efficient for your > needs? Because I asked him to:) The UserModel gets evaluated quite a bit so I don't want to do unnecessary work when we could cache it.
I think this is ready to go. Sami: Should I wait for your LGTM on this? https://codereview.chromium.org/1683583002/diff/140001/components/scheduler/r... File components/scheduler/renderer/user_model.cc (right): https://codereview.chromium.org/1683583002/diff/140001/components/scheduler/r... components/scheduler/renderer/user_model.cc:33: // Enables the recording of gesture as UMA user actions. On 2016/03/21 08:09:43, Ilya Sherman (Away Mar 19-27) wrote: > nit: s/gesture/gestures Done. https://codereview.chromium.org/1683583002/diff/140001/components/scheduler/r... components/scheduler/renderer/user_model.cc:47: record_gesture_action_( On 2016/03/21 09:27:26, alexclarke1 wrote: > On 2016/03/21 08:09:43, Ilya Sherman (Away Mar 19-27) wrote: > > Hmm, why do you need to cache this state? base::FeatureList::IsEnabled() > should > > be pretty efficient. Did you find that it's not sufficiently efficient for > your > > needs? > > Because I asked him to:) The UserModel gets evaluated quite a bit so I don't > want to do unnecessary work when we could cache it. Suggest leaving it like that for now. I've added a comment. https://codereview.chromium.org/1683583002/diff/140001/components/scheduler/r... File components/scheduler/renderer/user_model_unittest.cc (right): https://codereview.chromium.org/1683583002/diff/140001/components/scheduler/r... components/scheduler/renderer/user_model_unittest.cc:31: base::RemoveActionCallback(action_callback_); On 2016/03/21 08:09:44, Ilya Sherman (Away Mar 19-27) wrote: > Why is this line needed here? Seems like something that should be done in > teardown. (FWIW, I also find it pretty confusing that you both have a > constructor and a setup method, both of which seem to do initialization work. > Is there any particular reason not to do all of the work in the constructor?) SetUp is the standard ways tests are initialized around this area, so I'll leave it as that. I've moved the new base::TestSimpleTaskRunner() to the SetUp function. I've removed the tear down part, it's no longer needed with UserActionTester. https://codereview.chromium.org/1683583002/diff/140001/components/scheduler/r... components/scheduler/renderer/user_model_unittest.cc:48: void RecordComputedAction(const std::string& action) { On 2016/03/21 08:09:44, Ilya Sherman (Away Mar 19-27) wrote: > nit: What does the "Computed" part of this name refer to? Gone with UserActionTester. https://codereview.chromium.org/1683583002/diff/140001/components/scheduler/r... components/scheduler/renderer/user_model_unittest.cc:49: num_actions_recorded_++; On 2016/03/21 08:09:44, Ilya Sherman (Away Mar 19-27) wrote: > nit: preincrement Gone. https://codereview.chromium.org/1683583002/diff/140001/components/scheduler/r... components/scheduler/renderer/user_model_unittest.cc:268: ""); On 2016/03/21 08:09:44, Ilya Sherman (Away Mar 19-27) wrote: > nit: Prefer std::string() to an empty string literal. Done. https://codereview.chromium.org/1683583002/diff/140001/components/scheduler/r... components/scheduler/renderer/user_model_unittest.cc:291: EXPECT_EQ("RendererScheduler.UserModel.GestureEnd", last_action_recorded_); On 2016/03/21 08:09:44, Ilya Sherman (Away Mar 19-27) wrote: > It would probably be better to use the code from base/test/user_action_tester.h > rather than re-implementing it, if possible. I'm not 100% sure whether the code > would Just Work in the renderer process, though. Looks like it's almost a drop-in replacement for what I did. Thanks for the pointer! https://codereview.chromium.org/1683583002/diff/140001/content/renderer/rende... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/1683583002/diff/140001/content/renderer/rende... content/renderer/render_thread_impl.cc:637: base::RemoveActionCallback(action_callback_); On 2016/03/21 08:09:44, Ilya Sherman (Away Mar 19-27) wrote: > Hmm, why is this line needed? Copied the pattern from components/metrics/metrics_service.cc Suggest leaving it as-is for now. https://codereview.chromium.org/1683583002/diff/140001/tools/metrics/actions/... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/1683583002/diff/140001/tools/metrics/actions/... tools/metrics/actions/actions.xml:11737: scheduler is made aware of it. On 2016/03/21 08:09:44, Ilya Sherman (Away Mar 19-27) wrote: > Are you sure you don't want to break this metric down by the specific gesture? They are all considered similar for the current purpose, not breaking it down for now.
beaudoin@chromium.org changed reviewers: + kinuko@chromium.org
R+kinuko for content/renderer/render_thread_impl.*
lgtm with some nits. https://codereview.chromium.org/1683583002/diff/200001/components/scheduler/r... File components/scheduler/renderer/renderer_scheduler_impl.h (right): https://codereview.chromium.org/1683583002/diff/200001/components/scheduler/r... components/scheduler/renderer/renderer_scheduler_impl.h:349: const scoped_refptr<base::SingleThreadTaskRunner>& default_task_runner); nit: no need for const-ref (see https://codereview.chromium.org/1800743003) https://codereview.chromium.org/1683583002/diff/200001/components/scheduler/r... File components/scheduler/renderer/user_model.cc (right): https://codereview.chromium.org/1683583002/diff/200001/components/scheduler/r... components/scheduler/renderer/user_model.cc:36: const base::Feature kRecordGestureAction{"RecordGestureActions", nit: Since features aren't namespaced, we might want to call this "RecordSchedulerGestureActions" or something to that effect. Also, let's be consistent about "action" vs "actions". https://codereview.chromium.org/1683583002/diff/200001/content/renderer/rende... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/1683583002/diff/200001/content/renderer/rende... content/renderer/render_thread_impl.cc:638: base::RemoveActionCallback(action_callback_); AFAICT Init() will only ever be called once, so is it necessary to do this?
https://codereview.chromium.org/1683583002/diff/200001/components/scheduler/r... File components/scheduler/renderer/renderer_scheduler_impl.h (right): https://codereview.chromium.org/1683583002/diff/200001/components/scheduler/r... components/scheduler/renderer/renderer_scheduler_impl.h:349: const scoped_refptr<base::SingleThreadTaskRunner>& default_task_runner); On 2016/03/23 11:33:16, Sami (OoO) wrote: > nit: no need for const-ref (see https://codereview.chromium.org/1800743003) Done. https://codereview.chromium.org/1683583002/diff/200001/components/scheduler/r... File components/scheduler/renderer/user_model.cc (right): https://codereview.chromium.org/1683583002/diff/200001/components/scheduler/r... components/scheduler/renderer/user_model.cc:36: const base::Feature kRecordGestureAction{"RecordGestureActions", On 2016/03/23 11:33:16, Sami (OoO) wrote: > nit: Since features aren't namespaced, we might want to call this > "RecordSchedulerGestureActions" or something to that effect. > > Also, let's be consistent about "action" vs "actions". Done. https://codereview.chromium.org/1683583002/diff/200001/content/renderer/rende... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/1683583002/diff/200001/content/renderer/rende... content/renderer/render_thread_impl.cc:638: base::RemoveActionCallback(action_callback_); On 2016/03/23 11:33:16, Sami (OoO) wrote: > AFAICT Init() will only ever be called once, so is it necessary to do this? I don't know. I copied the pattern from metrics_service.cc. Since two ppl commented on it, I'll remove it. :) Done.
beaudoin@chromium.org changed reviewers: + piman@chromium.org - isherman@chromium.org, kinuko@chromium.org
R+piman-kinuko-isherman CC+isherman piman: Can you review content/renderer/render_thread_impl.* ? I was hoping to land this today.
https://codereview.chromium.org/1683583002/diff/240001/content/renderer/rende... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/1683583002/diff/240001/content/renderer/rende... content/renderer/render_thread_impl.cc:866: base::RemoveActionCallback(action_callback_); Should this be in Shutdown for consistency?
isherman@chromium.org changed reviewers: + isherman@chromium.org
LGTM % quibbles https://codereview.chromium.org/1683583002/diff/140001/components/scheduler/r... File components/scheduler/renderer/user_model.cc (right): https://codereview.chromium.org/1683583002/diff/140001/components/scheduler/r... components/scheduler/renderer/user_model.cc:47: record_gesture_action_( On 2016/03/23 02:46:41, beaudoin wrote: > On 2016/03/21 09:27:26, alexclarke1 wrote: > > On 2016/03/21 08:09:43, Ilya Sherman (Away Mar 19-27) wrote: > > > Hmm, why do you need to cache this state? base::FeatureList::IsEnabled() > > should > > > be pretty efficient. Did you find that it's not sufficiently efficient for > > your > > > needs? > > > > Because I asked him to:) The UserModel gets evaluated quite a bit so I don't > > want to do unnecessary work when we could cache it. > > Suggest leaving it like that for now. I've added a comment. the base::FeatureList::IsEnabled() call is basically a single map lookup. Is your code really so performance-sensitive? IMO this seems like premature optimization which hurts readability. https://codereview.chromium.org/1683583002/diff/140001/content/renderer/rende... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/1683583002/diff/140001/content/renderer/rende... content/renderer/render_thread_impl.cc:637: base::RemoveActionCallback(action_callback_); On 2016/03/23 02:46:42, beaudoin wrote: > On 2016/03/21 08:09:44, Ilya Sherman (Away Mar 19-27) wrote: > > Hmm, why is this line needed? > > Copied the pattern from components/metrics/metrics_service.cc > > Suggest leaving it as-is for now. The example you gave is not an init function -- it's a method that can be called multiple times. It's still a little suspect there, but I'm almost completely certain that you don't need this line here... and I find the line pretty confusing to read here.
Alex/Sami: What's your take on the Ilya's "premature opimization" comment? https://codereview.chromium.org/1683583002/diff/140001/content/renderer/rende... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/1683583002/diff/140001/content/renderer/rende... content/renderer/render_thread_impl.cc:637: base::RemoveActionCallback(action_callback_); On 2016/03/24 05:02:35, Ilya Sherman (Away Mar 19-27) wrote: > On 2016/03/23 02:46:42, beaudoin wrote: > > On 2016/03/21 08:09:44, Ilya Sherman (Away Mar 19-27) wrote: > > > Hmm, why is this line needed? > > > > Copied the pattern from components/metrics/metrics_service.cc > > > > Suggest leaving it as-is for now. > > The example you gave is not an init function -- it's a method that can be called > multiple times. It's still a little suspect there, but I'm almost completely > certain that you don't need this line here... and I find the line pretty > confusing to read here. It's gone now. :)
On 2016/03/24 21:08:55, beaudoin wrote: > Alex/Sami: What's your take on the Ilya's "premature opimization" comment? My LGTM still stands. > > https://codereview.chromium.org/1683583002/diff/140001/content/renderer/rende... > File content/renderer/render_thread_impl.cc (right): > > https://codereview.chromium.org/1683583002/diff/140001/content/renderer/rende... > content/renderer/render_thread_impl.cc:637: > base::RemoveActionCallback(action_callback_); > On 2016/03/24 05:02:35, Ilya Sherman (Away Mar 19-27) wrote: > > On 2016/03/23 02:46:42, beaudoin wrote: > > > On 2016/03/21 08:09:44, Ilya Sherman (Away Mar 19-27) wrote: > > > > Hmm, why is this line needed? > > > > > > Copied the pattern from components/metrics/metrics_service.cc > > > > > > Suggest leaving it as-is for now. > > > > The example you gave is not an init function -- it's a method that can be > called > > multiple times. It's still a little suspect there, but I'm almost completely > > certain that you don't need this line here... and I find the line pretty > > confusing to read here. > > It's gone now. :)
piman: PTAL. https://codereview.chromium.org/1683583002/diff/240001/content/renderer/rende... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/1683583002/diff/240001/content/renderer/rende... content/renderer/render_thread_impl.cc:866: base::RemoveActionCallback(action_callback_); On 2016/03/23 19:48:08, piman wrote: > Should this be in Shutdown for consistency? Done.
lgtm
The CQ bit was checked by beaudoin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org, skyostil@chromium.org, alexclarke@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/1683583002/#ps260001 (title: "Removed cached Feature, answered piman.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1683583002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1683583002/260001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by beaudoin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1683583002/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1683583002/280001
Add/RemoveActionCallback must not be called when Chrome is in single process mode. This is what user_metrics.cc does. The reason being that UMA performs a thread check on Add/RemoveActionCallback and this confuses a future call to RecordAction in navigator_impl.cc.
On 2016/03/31 23:35:47, beaudoin wrote: > Add/RemoveActionCallback must not be called when Chrome is in single process > mode. This is what user_metrics.cc does. The reason being that UMA performs a > thread check on Add/RemoveActionCallback and this confuses a future call to > RecordAction in navigator_impl.cc. piman: PTAL at the change in render_thread_impl.cc.
On Thu, Mar 31, 2016 at 4:35 PM, <beaudoin@chromium.org> wrote: > Add/RemoveActionCallback must not be called when Chrome is in single > process > mode. This is what user_metrics.cc does. The reason being that UMA > performs a > thread check on Add/RemoveActionCallback and this confuses a future call to > RecordAction in navigator_impl.cc. > Mmh, I think the renderer code should use RenderThread::Get()->RecordAction(), not base::RecordAction. The former will send a message to the browser to record. > https://codereview.chromium.org/1683583002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/03/31 23:57:28, piman wrote: > On Thu, Mar 31, 2016 at 4:35 PM, <mailto:beaudoin@chromium.org> wrote: > > > Add/RemoveActionCallback must not be called when Chrome is in single > > process > > mode. This is what user_metrics.cc does. The reason being that UMA > > performs a > > thread check on Add/RemoveActionCallback and this confuses a future call to > > RecordAction in navigator_impl.cc. > > > > Mmh, I think the renderer code should use > RenderThread::Get()->RecordAction(), not base::RecordAction. The former > will send a message to the browser to record. AddActionCallback's goal is to make sure the base:RecordAction calls the RenderThreadImpl::RecordAction. This is the pattern used in the browser. > > > https://codereview.chromium.org/1683583002/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
On Thu, Mar 31, 2016 at 5:02 PM, <beaudoin@chromium.org> wrote: > On 2016/03/31 23:57:28, piman wrote: > > On Thu, Mar 31, 2016 at 4:35 PM, <mailto:beaudoin@chromium.org> wrote: > > > > > Add/RemoveActionCallback must not be called when Chrome is in single > > > process > > > mode. This is what user_metrics.cc does. The reason being that UMA > > > performs a > > > thread check on Add/RemoveActionCallback and this confuses a future > call to > > > RecordAction in navigator_impl.cc. > > > > > > > Mmh, I think the renderer code should use > > RenderThread::Get()->RecordAction(), not base::RecordAction. The former > > will send a message to the browser to record. > > AddActionCallback's goal is to make sure the base:RecordAction calls the > RenderThreadImpl::RecordAction. This is the pattern used in the browser. > But it can't work in single-process (e.g. Android WebView) ? > > > > > https://codereview.chromium.org/1683583002/ > > > > > > > -- > > You received this message because you are subscribed to the Google Groups > > "Chromium-reviews" group. > > To unsubscribe from this group and stop receiving emails from it, send an > email > > to mailto:chromium-reviews+unsubscribe@chromium.org. > > > > https://codereview.chromium.org/1683583002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/04/01 00:10:55, piman wrote: > On Thu, Mar 31, 2016 at 5:02 PM, <mailto:beaudoin@chromium.org> wrote: > > > On 2016/03/31 23:57:28, piman wrote: > > > On Thu, Mar 31, 2016 at 4:35 PM, <mailto:beaudoin@chromium.org> wrote: > > > > > > > Add/RemoveActionCallback must not be called when Chrome is in single > > > > process > > > > mode. This is what user_metrics.cc does. The reason being that UMA > > > > performs a > > > > thread check on Add/RemoveActionCallback and this confuses a future > > call to > > > > RecordAction in navigator_impl.cc. > > > > > > > > > > Mmh, I think the renderer code should use > > > RenderThread::Get()->RecordAction(), not base::RecordAction. The former > > > will send a message to the browser to record. > > > > AddActionCallback's goal is to make sure the base:RecordAction calls the > > RenderThreadImpl::RecordAction. This is the pattern used in the browser. > > > > But it can't work in single-process (e.g. Android WebView) ? Hmmm... Good point. I thought single-process was only used in tests. Let me try your approach. > > > > > > > > https://codereview.chromium.org/1683583002/ > > > > > > > > > > -- > > > You received this message because you are subscribed to the Google Groups > > > "Chromium-reviews" group. > > > To unsubscribe from this group and stop receiving emails from it, send an > > email > > > to mailto:chromium-reviews+unsubscribe@chromium.org. > > > > > > > > https://codereview.chromium.org/1683583002/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
On 2016/04/01 00:15:35, beaudoin wrote: > On 2016/04/01 00:10:55, piman wrote: > > On Thu, Mar 31, 2016 at 5:02 PM, <mailto:beaudoin@chromium.org> wrote: > > > > > On 2016/03/31 23:57:28, piman wrote: > > > > On Thu, Mar 31, 2016 at 4:35 PM, <mailto:beaudoin@chromium.org> wrote: > > > > > > > > > Add/RemoveActionCallback must not be called when Chrome is in single > > > > > process > > > > > mode. This is what user_metrics.cc does. The reason being that UMA > > > > > performs a > > > > > thread check on Add/RemoveActionCallback and this confuses a future > > > call to > > > > > RecordAction in navigator_impl.cc. > > > > > > > > > > > > > Mmh, I think the renderer code should use > > > > RenderThread::Get()->RecordAction(), not base::RecordAction. The former > > > > will send a message to the browser to record. > > > > > > AddActionCallback's goal is to make sure the base:RecordAction calls the > > > RenderThreadImpl::RecordAction. This is the pattern used in the browser. > > > > > > > But it can't work in single-process (e.g. Android WebView) ? > > Hmmm... Good point. I thought single-process was only used in tests. Let me try > your approach. The problem seems to be that I do not have access to content from the scheduler/renderer. Can't do RenderThread::Get() or access ChildThread's methods... :(
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Hi, what's the status of this CL?
On 2016/05/17 18:22:59, Ilya Sherman wrote: > Hi, what's the status of this CL? Just getting to this again using the new approach enabled by this: https://codereview.chromium.org/1859213002/ |
