Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1859213002:
Move the thread hop for UMA user actions from content:: to base::. (Closed)
|
Created:
4 years, 8 months ago by beaudoin Modified:
4 years, 8 months ago Reviewers:
sgurun-gerrit only, slan, Lei Zhang, piman, Alexei Svitkine (slow), sdefresne, Ilya Sherman CC:
asvitkine+watch_chromium.org, chromium-reviews, darin-cc_chromium.org, jam Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMove the thread hop for UMA user actions from content:: to base::.
BUG=604367
Committed: https://crrev.com/d5c435e0b76ef29580d018cdbe8717fefd2ddd03
Cr-Commit-Position: refs/heads/master@{#389097}
Patch Set 1 : #
Total comments: 22
Patch Set 2 : Answered Ilya. #Patch Set 3 : #
Total comments: 16
Patch Set 4 : Fixed some tests, checking what I missed. #Patch Set 5 : Rebased #
Total comments: 4
Patch Set 6 : Updated dependencies. #Patch Set 7 : Hopefully fixed IOS and Android. #Patch Set 8 : Some more fixes for mobile. #Patch Set 9 : More test fixed. #Patch Set 10 : Fixing Chromecast (I hope!) #
Total comments: 6
Patch Set 11 : Answered Ilya. Rebased. #
Total comments: 12
Patch Set 12 : Answered Alexei and Ilya. #
Total comments: 4
Patch Set 13 : Moved call to SetRecordActionTaskRunner. #
Total comments: 4
Patch Set 14 : Fixed ios and cast issues. Answered reviewers. #Patch Set 15 : Fixing bugs. #
Total comments: 13
Patch Set 16 : Answered thestig@ and sgurun@ #
Total comments: 2
Patch Set 17 : Answered thestig@ #Patch Set 18 : Initializing in content. #
Total comments: 2
Messages
Total messages: 122 (46 generated)
Patchset #1 (id:1) has been deleted
beaudoin@chromium.org changed reviewers: + isherman@chromium.org
Hi Ilya, This is a tentative implementation of the thread hop done in base:: rather than content::. Seems to be working, although I wanted to make sure this is what you had in mind before updating the tests. What do you think?
https://codereview.chromium.org/1859213002/diff/20001/base/metrics/user_metri... File base/metrics/user_metrics.cc (right): https://codereview.chromium.org/1859213002/diff/20001/base/metrics/user_metri... base/metrics/user_metrics.cc:51: }; I don't think that it's appropriate, or necessary, to remove the thread checking from this class. Actions and callbacks should still be recorded on a single thread. Only the Record(Computed)Action functions should need the thread hop. https://codereview.chromium.org/1859213002/diff/20001/base/metrics/user_metri... base/metrics/user_metrics.cc:97: base::Bind(&SetRecordActionTaskRunner, task_runner)); The task runner should only be set once, IMO, so I don't really understand what the purpose of this code is. https://codereview.chromium.org/1859213002/diff/20001/base/metrics/user_metri... File base/metrics/user_metrics.h (right): https://codereview.chromium.org/1859213002/diff/20001/base/metrics/user_metri... base/metrics/user_metrics.h:59: // Set the task runner on which to do the thread hopping. nit: Perhaps "Set the task runner on which to record actions." or something like it? https://codereview.chromium.org/1859213002/diff/20001/base/metrics/user_metri... base/metrics/user_metrics.h:63: nit: Spurious newline https://codereview.chromium.org/1859213002/diff/20001/components/metrics/metr... File components/metrics/metrics_service.cc (right): https://codereview.chromium.org/1859213002/diff/20001/components/metrics/metr... components/metrics/metrics_service.cc:304: base::SetRecordActionTaskRunner(client_->GetUIThreadTaskRunner()); I think it makes sense to set this in the same place where the action callback is set. But I'm curious, why did you choose to implement this here rather than in that spot? https://codereview.chromium.org/1859213002/diff/20001/components/metrics/metr... File components/metrics/metrics_service_client.h (right): https://codereview.chromium.org/1859213002/diff/20001/components/metrics/metr... components/metrics/metrics_service_client.h:104: GetUIThreadTaskRunner() = 0; Please document this method. https://codereview.chromium.org/1859213002/diff/20001/components/metrics/metr... components/metrics/metrics_service_client.h:104: GetUIThreadTaskRunner() = 0; nit: I think "MainThread" would be better than "UIThread". https://codereview.chromium.org/1859213002/diff/20001/content/browser/user_me... File content/browser/user_metrics.cc (right): https://codereview.chromium.org/1859213002/diff/20001/content/browser/user_me... content/browser/user_metrics.cc:21: } These methods seem like they could be removed entirely if they're no longer providing the thread hop.
Ilya: PTAL. (Running the bots to figure out which tests I broke. I'll fix them in the next revision.) https://codereview.chromium.org/1859213002/diff/20001/base/metrics/user_metri... File base/metrics/user_metrics.cc (right): https://codereview.chromium.org/1859213002/diff/20001/base/metrics/user_metri... base/metrics/user_metrics.cc:51: }; On 2016/04/06 02:36:28, Ilya Sherman wrote: > I don't think that it's appropriate, or necessary, to remove the thread checking > from this class. Actions and callbacks should still be recorded on a single > thread. Only the Record(Computed)Action functions should need the thread hop. Ok. I've removed the thread hop for Add/Remove and added back the thread_checker_. I'm a bit annoyed by the fact that we have two "thread trackers" though: thread_checker_ and g_task_runner. If you're fine doing the DCHECK only in the exposed functions I could do DCHECK(g_task_runner.Get() && g_task_runner.Get()->BelongsToCurrentThread()); Truth is, we could get rid of Callbacks entirely and just have a global std::vector<ActionCallback>... https://codereview.chromium.org/1859213002/diff/20001/base/metrics/user_metri... base/metrics/user_metrics.cc:97: base::Bind(&SetRecordActionTaskRunner, task_runner)); On 2016/04/06 02:36:28, Ilya Sherman wrote: > The task runner should only be set once, IMO, so I don't really understand what > the purpose of this code is. Done. https://codereview.chromium.org/1859213002/diff/20001/base/metrics/user_metri... File base/metrics/user_metrics.h (right): https://codereview.chromium.org/1859213002/diff/20001/base/metrics/user_metri... base/metrics/user_metrics.h:59: // Set the task runner on which to do the thread hopping. On 2016/04/06 02:36:29, Ilya Sherman wrote: > nit: Perhaps "Set the task runner on which to record actions." or something like > it? Done. https://codereview.chromium.org/1859213002/diff/20001/base/metrics/user_metri... base/metrics/user_metrics.h:63: On 2016/04/06 02:36:29, Ilya Sherman wrote: > nit: Spurious newline Done. https://codereview.chromium.org/1859213002/diff/20001/components/metrics/metr... File components/metrics/metrics_service.cc (right): https://codereview.chromium.org/1859213002/diff/20001/components/metrics/metr... components/metrics/metrics_service.cc:304: base::SetRecordActionTaskRunner(client_->GetUIThreadTaskRunner()); On 2016/04/06 02:36:29, Ilya Sherman wrote: > I think it makes sense to set this in the same place where the action callback > is set. But I'm curious, why did you choose to implement this here rather than > in that spot? It seems that a RecordAction is being called before MetricsService::EnableRecording has been called. If I don't do this here I get a DCHECK triggered by: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/the... I imagine it worked before because thread_checker picked up the thread at that point, even though the action is never recorded since the Callbacks are not set yet. We should probably figure out why that RecordAction is happening too early though. https://codereview.chromium.org/1859213002/diff/20001/components/metrics/metr... File components/metrics/metrics_service_client.h (right): https://codereview.chromium.org/1859213002/diff/20001/components/metrics/metr... components/metrics/metrics_service_client.h:104: GetUIThreadTaskRunner() = 0; On 2016/04/06 02:36:29, Ilya Sherman wrote: > nit: I think "MainThread" would be better than "UIThread". Done. https://codereview.chromium.org/1859213002/diff/20001/components/metrics/metr... components/metrics/metrics_service_client.h:104: GetUIThreadTaskRunner() = 0; On 2016/04/06 02:36:29, Ilya Sherman wrote: > Please document this method. Done. https://codereview.chromium.org/1859213002/diff/20001/content/browser/user_me... File content/browser/user_metrics.cc (right): https://codereview.chromium.org/1859213002/diff/20001/content/browser/user_me... content/browser/user_metrics.cc:21: } On 2016/04/06 02:36:29, Ilya Sherman wrote: > These methods seem like they could be removed entirely if they're no longer > providing the thread hop. Yes. Will do. Since it affects a lot more files I was thinking of doing it in a separate clean-up CL though, what do you think?
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/1859213002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1859213002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
Thanks for working on this, by the way! =) https://codereview.chromium.org/1859213002/diff/20001/base/metrics/user_metri... File base/metrics/user_metrics.cc (right): https://codereview.chromium.org/1859213002/diff/20001/base/metrics/user_metri... base/metrics/user_metrics.cc:51: }; On 2016/04/06 15:06:13, beaudoin wrote: > On 2016/04/06 02:36:28, Ilya Sherman wrote: > > I don't think that it's appropriate, or necessary, to remove the thread > checking > > from this class. Actions and callbacks should still be recorded on a single > > thread. Only the Record(Computed)Action functions should need the thread hop. > > Ok. I've removed the thread hop for Add/Remove and added back the > thread_checker_. I'm a bit annoyed by the fact that we have two "thread > trackers" though: thread_checker_ and g_task_runner. If you're fine doing the > DCHECK only in the exposed functions I could do DCHECK(g_task_runner.Get() && > g_task_runner.Get()->BelongsToCurrentThread()); > > Truth is, we could get rid of Callbacks entirely and just have a global > std::vector<ActionCallback>... Yeah, I'd be fine with the DCHECKs you described, and getting rid of the Callbacks wrapper class. https://codereview.chromium.org/1859213002/diff/20001/components/metrics/metr... File components/metrics/metrics_service.cc (right): https://codereview.chromium.org/1859213002/diff/20001/components/metrics/metr... components/metrics/metrics_service.cc:304: base::SetRecordActionTaskRunner(client_->GetUIThreadTaskRunner()); On 2016/04/06 15:06:13, beaudoin wrote: > On 2016/04/06 02:36:29, Ilya Sherman wrote: > > I think it makes sense to set this in the same place where the action callback > > is set. But I'm curious, why did you choose to implement this here rather > than > > in that spot? > > It seems that a RecordAction is being called before > MetricsService::EnableRecording has been called. If I don't do this here I get a > DCHECK triggered by: > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/the... > > I imagine it worked before because thread_checker picked up the thread at that > point, even though the action is never recorded since the Callbacks are not set > yet. > > We should probably figure out why that RecordAction is happening too early > though. Hmm, yeah, that sounds like a bug for sure -- there's not much point in recording a metric if it's just silently dropped. Could you please file a bug for this? I think it would be nice to track that down and fix it before landing this code, since this code is currently more complicated to work around that bug. https://codereview.chromium.org/1859213002/diff/20001/content/browser/user_me... File content/browser/user_metrics.cc (right): https://codereview.chromium.org/1859213002/diff/20001/content/browser/user_me... content/browser/user_metrics.cc:21: } On 2016/04/06 15:06:13, beaudoin wrote: > On 2016/04/06 02:36:29, Ilya Sherman wrote: > > These methods seem like they could be removed entirely if they're no longer > > providing the thread hop. > > Yes. Will do. Since it affects a lot more files I was thinking of doing it in a > separate clean-up CL though, what do you think? Fair enough. Could you please file a bug and add TODOs for the cleanup?
https://codereview.chromium.org/1859213002/diff/20001/base/metrics/user_metri... File base/metrics/user_metrics.cc (right): https://codereview.chromium.org/1859213002/diff/20001/base/metrics/user_metri... base/metrics/user_metrics.cc:51: }; On 2016/04/06 21:48:48, Ilya Sherman wrote: > On 2016/04/06 15:06:13, beaudoin wrote: > > On 2016/04/06 02:36:28, Ilya Sherman wrote: > > > I don't think that it's appropriate, or necessary, to remove the thread > > checking > > > from this class. Actions and callbacks should still be recorded on a single > > > thread. Only the Record(Computed)Action functions should need the thread > hop. > > > > Ok. I've removed the thread hop for Add/Remove and added back the > > thread_checker_. I'm a bit annoyed by the fact that we have two "thread > > trackers" though: thread_checker_ and g_task_runner. If you're fine doing the > > DCHECK only in the exposed functions I could do DCHECK(g_task_runner.Get() && > > g_task_runner.Get()->BelongsToCurrentThread()); > > > > Truth is, we could get rid of Callbacks entirely and just have a global > > std::vector<ActionCallback>... > > Yeah, I'd be fine with the DCHECKs you described, and getting rid of the > Callbacks wrapper class. Done. https://codereview.chromium.org/1859213002/diff/20001/components/metrics/metr... File components/metrics/metrics_service.cc (right): https://codereview.chromium.org/1859213002/diff/20001/components/metrics/metr... components/metrics/metrics_service.cc:304: base::SetRecordActionTaskRunner(client_->GetUIThreadTaskRunner()); On 2016/04/06 21:48:48, Ilya Sherman wrote: > On 2016/04/06 15:06:13, beaudoin wrote: > > On 2016/04/06 02:36:29, Ilya Sherman wrote: > > > I think it makes sense to set this in the same place where the action > callback > > > is set. But I'm curious, why did you choose to implement this here rather > > than > > > in that spot? > > > > It seems that a RecordAction is being called before > > MetricsService::EnableRecording has been called. If I don't do this here I get > a > > DCHECK triggered by: > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/the... > > > > I imagine it worked before because thread_checker picked up the thread at that > > point, even though the action is never recorded since the Callbacks are not > set > > yet. > > > > We should probably figure out why that RecordAction is happening too early > > though. > > Hmm, yeah, that sounds like a bug for sure -- there's not much point in > recording a metric if it's just silently dropped. Could you please file a bug > for this? I think it would be nice to track that down and fix it before landing > this code, since this code is currently more complicated to work around that > bug. Filed as: https://crbug.com/601482 I'll take a look. Not sure I'll be able to do much better than remove the Record call, I wouldn't be comfortable fiddling with the startup sequence. https://codereview.chromium.org/1859213002/diff/20001/content/browser/user_me... File content/browser/user_metrics.cc (right): https://codereview.chromium.org/1859213002/diff/20001/content/browser/user_me... content/browser/user_metrics.cc:21: } On 2016/04/06 21:48:48, Ilya Sherman wrote: > On 2016/04/06 15:06:13, beaudoin wrote: > > On 2016/04/06 02:36:29, Ilya Sherman wrote: > > > These methods seem like they could be removed entirely if they're no longer > > > providing the thread hop. > > > > Yes. Will do. Since it affects a lot more files I was thinking of doing it in > a > > separate clean-up CL though, what do you think? > > Fair enough. Could you please file a bug and add TODOs for the cleanup? Done. Filed as: crbug.com/601483
Some comments below are redundant with what we discussed over IM -- figured it couldn't hurt to write them down here for posterity =) https://codereview.chromium.org/1859213002/diff/60001/base/metrics/user_metri... File base/metrics/user_metrics.cc (right): https://codereview.chromium.org/1859213002/diff/60001/base/metrics/user_metri... base/metrics/user_metrics.cc:34: return; Please add a DCHECK within this if-stmt to verify that |g_callbacks| is empty in this case. https://codereview.chromium.org/1859213002/diff/60001/base/metrics/user_metri... base/metrics/user_metrics.cc:34: return; nit: Please leave a blank line after this if-stmt. https://codereview.chromium.org/1859213002/diff/60001/base/metrics/user_metri... base/metrics/user_metrics.cc:39: } nit: Please leave a blank line after this if-stmt. https://codereview.chromium.org/1859213002/diff/60001/base/metrics/user_metri... base/metrics/user_metrics.cc:47: DCHECK(g_task_runner.Get() && g_task_runner.Get()->BelongsToCurrentThread()); nit: Please write this as two DCHECKs, so that if either condition fails, it's clear which one is failing. https://codereview.chromium.org/1859213002/diff/60001/base/metrics/user_metri... base/metrics/user_metrics.cc:52: DCHECK(g_task_runner.Get() && g_task_runner.Get()->BelongsToCurrentThread()); nit: Ditto here https://codereview.chromium.org/1859213002/diff/60001/components/metrics/test... File components/metrics/test_metrics_service_client.cc (right): https://codereview.chromium.org/1859213002/diff/60001/components/metrics/test... components/metrics/test_metrics_service_client.cc:8: #include "base/test/test_mock_time_task_runner.h" nit: Probably test_simple_task_runner would be a better choice. https://codereview.chromium.org/1859213002/diff/60001/components/metrics/test... File components/metrics/test_metrics_service_client.h (right): https://codereview.chromium.org/1859213002/diff/60001/components/metrics/test... components/metrics/test_metrics_service_client.h:48: nit: Spurious newline. https://codereview.chromium.org/1859213002/diff/60001/content/public/browser/... File content/public/browser/user_metrics.h (right): https://codereview.chromium.org/1859213002/diff/60001/content/public/browser/... content/public/browser/user_metrics.h:17: // thread hopping. Could you please also file a bug and add a reference to the bug here?
https://codereview.chromium.org/1859213002/diff/60001/base/metrics/user_metri... File base/metrics/user_metrics.cc (right): https://codereview.chromium.org/1859213002/diff/60001/base/metrics/user_metri... base/metrics/user_metrics.cc:34: return; On 2016/04/07 18:21:53, Ilya Sherman wrote: > Please add a DCHECK within this if-stmt to verify that |g_callbacks| is empty in > this case. Done. https://codereview.chromium.org/1859213002/diff/60001/base/metrics/user_metri... base/metrics/user_metrics.cc:34: return; On 2016/04/07 18:21:53, Ilya Sherman wrote: > nit: Please leave a blank line after this if-stmt. Done. https://codereview.chromium.org/1859213002/diff/60001/base/metrics/user_metri... base/metrics/user_metrics.cc:39: } On 2016/04/07 18:21:53, Ilya Sherman wrote: > nit: Please leave a blank line after this if-stmt. Done. https://codereview.chromium.org/1859213002/diff/60001/base/metrics/user_metri... base/metrics/user_metrics.cc:47: DCHECK(g_task_runner.Get() && g_task_runner.Get()->BelongsToCurrentThread()); On 2016/04/07 18:21:53, Ilya Sherman wrote: > nit: Please write this as two DCHECKs, so that if either condition fails, it's > clear which one is failing. Done. https://codereview.chromium.org/1859213002/diff/60001/base/metrics/user_metri... base/metrics/user_metrics.cc:52: DCHECK(g_task_runner.Get() && g_task_runner.Get()->BelongsToCurrentThread()); On 2016/04/07 18:21:53, Ilya Sherman wrote: > nit: Ditto here Done. https://codereview.chromium.org/1859213002/diff/60001/components/metrics/test... File components/metrics/test_metrics_service_client.cc (right): https://codereview.chromium.org/1859213002/diff/60001/components/metrics/test... components/metrics/test_metrics_service_client.cc:8: #include "base/test/test_mock_time_task_runner.h" On 2016/04/07 18:21:54, Ilya Sherman wrote: > nit: Probably test_simple_task_runner would be a better choice. Done. https://codereview.chromium.org/1859213002/diff/60001/components/metrics/test... File components/metrics/test_metrics_service_client.h (right): https://codereview.chromium.org/1859213002/diff/60001/components/metrics/test... components/metrics/test_metrics_service_client.h:48: On 2016/04/07 18:21:54, Ilya Sherman wrote: > nit: Spurious newline. Done. https://codereview.chromium.org/1859213002/diff/60001/content/public/browser/... File content/public/browser/user_metrics.h (right): https://codereview.chromium.org/1859213002/diff/60001/content/public/browser/... content/public/browser/user_metrics.h:17: // thread hopping. On 2016/04/07 18:21:54, Ilya Sherman wrote: > Could you please also file a bug and add a reference to the bug here? Done.
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/1859213002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1859213002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_rel_device_gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_gn...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
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/1859213002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1859213002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...) cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_blink_oilpan_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_blink_oil...) linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_gn...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
https://codereview.chromium.org/1859213002/diff/100001/base/metrics/user_metr... File base/metrics/user_metrics.cc (right): https://codereview.chromium.org/1859213002/diff/100001/base/metrics/user_metr... base/metrics/user_metrics.cc:70: g_task_runner.Get() = task_runner; Hmm, I don't like removing the DCHECK here to ensure that a task runner is only set once in production code. ResetRecordActionTaskRunnerForTest() is one (potentially good) option. Another would be to ignore any task runners that belong to the current thread, since the task runner is only used for thread-hopping.
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/1859213002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1859213002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_dbg_simulator_gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
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/1859213002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1859213002/140001
https://codereview.chromium.org/1859213002/diff/100001/base/metrics/user_metr... File base/metrics/user_metrics.cc (right): https://codereview.chromium.org/1859213002/diff/100001/base/metrics/user_metr... base/metrics/user_metrics.cc:70: g_task_runner.Get() = task_runner; On 2016/04/07 23:49:51, Ilya Sherman wrote: > Hmm, I don't like removing the DCHECK here to ensure that a task runner is only > set once in production code. ResetRecordActionTaskRunnerForTest() is one > (potentially good) option. Another would be to ignore any task runners that > belong to the current thread, since the task runner is only used for > thread-hopping. That second approach is clever, but unfortunately it will break the assumption (above) that RecordComputedAction is only called when either the g_task_runner is set or g_callbacks is empty. I'll go with SetRecordActionTaskRunnerForTesting().
Patchset #6 (id:120001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
The CQ bit was checked by beaudoin@chromium.org to run a CQ dry run
Ilya: PTAL. Patch grew quite a bit to fix problems in IOS, Android, GN.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1859213002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1859213002/200001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
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/1859213002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1859213002/220001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks! https://codereview.chromium.org/1859213002/diff/100001/base/metrics/user_metr... File base/metrics/user_metrics.cc (right): https://codereview.chromium.org/1859213002/diff/100001/base/metrics/user_metr... base/metrics/user_metrics.cc:70: g_task_runner.Get() = task_runner; On 2016/04/08 13:56:00, beaudoin wrote: > On 2016/04/07 23:49:51, Ilya Sherman wrote: > > Hmm, I don't like removing the DCHECK here to ensure that a task runner is > only > > set once in production code. ResetRecordActionTaskRunnerForTest() is one > > (potentially good) option. Another would be to ignore any task runners that > > belong to the current thread, since the task runner is only used for > > thread-hopping. > > That second approach is clever, but unfortunately it will break the assumption > (above) that RecordComputedAction is only called when either the g_task_runner > is set or g_callbacks is empty. > > I'll go with SetRecordActionTaskRunnerForTesting(). Hmm, maybe I didn't phrase my suggestion very clearly. I meant, in the case that a task runner is already set, it would be fine to ignore subsequent calls to SetRecordActionTaskRunner() in the case that the new and the old task runner run on the same thread. (If that still breaks the assumption that RecordComputedAction is only called when either the g_task_runner is set or g_callbacks is empty, then I'm missing something, and would very much appreciate a clarification =)) https://codereview.chromium.org/1859213002/diff/220001/chromecast/browser/met... File chromecast/browser/metrics/cast_metrics_service_client.cc (right): https://codereview.chromium.org/1859213002/diff/220001/chromecast/browser/met... chromecast/browser/metrics/cast_metrics_service_client.cc:270: nit: Spurious newline. https://codereview.chromium.org/1859213002/diff/220001/ios/chrome/browser/met... File ios/chrome/browser/metrics/ios_chrome_metrics_service_client.cc (right): https://codereview.chromium.org/1859213002/diff/220001/ios/chrome/browser/met... ios/chrome/browser/metrics/ios_chrome_metrics_service_client.cc:195: nit: Spurious newline https://codereview.chromium.org/1859213002/diff/220001/ios/chrome/browser/met... File ios/chrome/browser/metrics/ios_chrome_metrics_service_client.h (right): https://codereview.chromium.org/1859213002/diff/220001/ios/chrome/browser/met... ios/chrome/browser/metrics/ios_chrome_metrics_service_client.h:76: // Returns the task runner for the main thread. ?
beaudoin@google.com changed reviewers: + beaudoin@google.com
https://codereview.chromium.org/1859213002/diff/100001/base/metrics/user_metr... File base/metrics/user_metrics.cc (right): https://codereview.chromium.org/1859213002/diff/100001/base/metrics/user_metr... base/metrics/user_metrics.cc:70: g_task_runner.Get() = task_runner; On 2016/04/09 07:06:10, Ilya Sherman wrote: > On 2016/04/08 13:56:00, beaudoin wrote: > > On 2016/04/07 23:49:51, Ilya Sherman wrote: > > > Hmm, I don't like removing the DCHECK here to ensure that a task runner is > > only > > > set once in production code. ResetRecordActionTaskRunnerForTest() is one > > > (potentially good) option. Another would be to ignore any task runners that > > > belong to the current thread, since the task runner is only used for > > > thread-hopping. > > > > That second approach is clever, but unfortunately it will break the assumption > > (above) that RecordComputedAction is only called when either the g_task_runner > > is set or g_callbacks is empty. > > > > I'll go with SetRecordActionTaskRunnerForTesting(). > > Hmm, maybe I didn't phrase my suggestion very clearly. I meant, in the case > that a task runner is already set, it would be fine to ignore subsequent calls > to SetRecordActionTaskRunner() in the case that the new and the old task runner > run on the same thread. (If that still breaks the assumption that > RecordComputedAction is only called when either the g_task_runner is set or > g_callbacks is empty, then I'm missing something, and would very much appreciate > a clarification =)) Ah! I thought you meant to ignore it even the first time it's set (if it's on the same thread), but it would not make much sense. I like that approach better, I'll migrate to it.
Ilya, PTAL. https://codereview.chromium.org/1859213002/diff/220001/chromecast/browser/met... File chromecast/browser/metrics/cast_metrics_service_client.cc (right): https://codereview.chromium.org/1859213002/diff/220001/chromecast/browser/met... chromecast/browser/metrics/cast_metrics_service_client.cc:270: On 2016/04/09 07:06:11, Ilya Sherman wrote: > nit: Spurious newline. Done. https://codereview.chromium.org/1859213002/diff/220001/ios/chrome/browser/met... File ios/chrome/browser/metrics/ios_chrome_metrics_service_client.cc (right): https://codereview.chromium.org/1859213002/diff/220001/ios/chrome/browser/met... ios/chrome/browser/metrics/ios_chrome_metrics_service_client.cc:195: On 2016/04/09 07:06:11, Ilya Sherman wrote: > nit: Spurious newline Done. https://codereview.chromium.org/1859213002/diff/220001/ios/chrome/browser/met... File ios/chrome/browser/metrics/ios_chrome_metrics_service_client.h (right): https://codereview.chromium.org/1859213002/diff/220001/ios/chrome/browser/met... ios/chrome/browser/metrics/ios_chrome_metrics_service_client.h:76: // Returns the task runner for the main thread. On 2016/04/09 07:06:11, Ilya Sherman wrote: > ? Done.
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/1859213002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1859213002/240001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
Thanks, Philippe! LGTM % a final comment: https://codereview.chromium.org/1859213002/diff/240001/base/metrics/user_metr... File base/metrics/user_metrics.cc (right): https://codereview.chromium.org/1859213002/diff/240001/base/metrics/user_metr... base/metrics/user_metrics.cc:70: DCHECK(!g_task_runner.Get() || g_task_runner.Get()->BelongsToCurrentThread()); This DCHECK is a little bit odd to read right now, because it checks that the |g_task_runner| belongs to the current thread, but that might not be the same thread as the |task_runner|. I think it would make sense to either test that |g_task_runner| belongs to the same thread as |task_runner|, or to indirectly test this by also adding a DCHECK to verify that the |task_runner| belongs to the current thread.
asvitkine@chromium.org changed reviewers: + asvitkine@chromium.org
https://codereview.chromium.org/1859213002/diff/240001/base/metrics/user_metr... File base/metrics/user_metrics.cc (right): https://codereview.chromium.org/1859213002/diff/240001/base/metrics/user_metr... base/metrics/user_metrics.cc:32: if(!g_task_runner.Get()) { Nit: space after if. Maybe run git cl format? https://codereview.chromium.org/1859213002/diff/240001/base/metrics/user_metr... base/metrics/user_metrics.cc:44: for (size_t i = 0; i < callbacks.size(); ++i) { Nit: Use the following if it works: for (const ActionCallback& callback : g_callbacks.Get()) { } https://codereview.chromium.org/1859213002/diff/240001/base/metrics/user_metr... base/metrics/user_metrics.cc:59: std::vector<ActionCallback>& callbacks = g_callbacks.Get(); Nit: non-const refs are discouraged - suggest changing this to a pointer. https://codereview.chromium.org/1859213002/diff/240001/components/metrics/met... File components/metrics/metrics_service.cc (right): https://codereview.chromium.org/1859213002/diff/240001/components/metrics/met... components/metrics/metrics_service.cc:307: base::SetRecordActionTaskRunner(client_->GetMainThreadTaskRunner()); I wasn't following the full discussion here. I like the task runner approach, however it seems a bit strange to do this in MetricsService, since it's kind of orthogonal to the functionality of MetricsService. I would expect this to be done somewhere like StatisticsRecorder::Initialize() and then you don't need the plumbing of the main thread task runner through the client interface. (StatisticsRecorder::Initialize can then be made to take a TaskRunner param). Apologies if this was discussed previously as I didn't read through the previous discussions here.
beaudoin@chromium.org changed reviewers: - beaudoin@google.com
Ilya, Alexei: See discussion on where to call SetRecordActionTaskRunner. https://codereview.chromium.org/1859213002/diff/240001/base/metrics/user_metr... File base/metrics/user_metrics.cc (right): https://codereview.chromium.org/1859213002/diff/240001/base/metrics/user_metr... base/metrics/user_metrics.cc:32: if(!g_task_runner.Get()) { On 2016/04/13 14:43:12, Alexei Svitkine wrote: > Nit: space after if. Maybe run git cl format? Done. https://codereview.chromium.org/1859213002/diff/240001/base/metrics/user_metr... base/metrics/user_metrics.cc:44: for (size_t i = 0; i < callbacks.size(); ++i) { On 2016/04/13 14:43:12, Alexei Svitkine wrote: > Nit: Use the following if it works: > > for (const ActionCallback& callback : g_callbacks.Get()) { > > } Done. https://codereview.chromium.org/1859213002/diff/240001/base/metrics/user_metr... base/metrics/user_metrics.cc:59: std::vector<ActionCallback>& callbacks = g_callbacks.Get(); On 2016/04/13 14:43:12, Alexei Svitkine wrote: > Nit: non-const refs are discouraged - suggest changing this to a pointer. Done. https://codereview.chromium.org/1859213002/diff/240001/base/metrics/user_metr... base/metrics/user_metrics.cc:70: DCHECK(!g_task_runner.Get() || g_task_runner.Get()->BelongsToCurrentThread()); On 2016/04/13 01:06:27, Ilya Sherman wrote: > This DCHECK is a little bit odd to read right now, because it checks that the > |g_task_runner| belongs to the current thread, but that might not be the same > thread as the |task_runner|. I think it would make sense to either test that > |g_task_runner| belongs to the same thread as |task_runner|, or to indirectly > test this by also adding a DCHECK to verify that the |task_runner| belongs to > the current thread. That last DCHECK makes sense. It sounds like a bad idea to use thread A to set a task_runner that would post to thread B. Done. https://codereview.chromium.org/1859213002/diff/240001/components/metrics/met... File components/metrics/metrics_service.cc (right): https://codereview.chromium.org/1859213002/diff/240001/components/metrics/met... components/metrics/metrics_service.cc:307: base::SetRecordActionTaskRunner(client_->GetMainThreadTaskRunner()); On 2016/04/13 14:43:12, Alexei Svitkine wrote: > I wasn't following the full discussion here. I like the task runner approach, > however it seems a bit strange to do this in MetricsService, since it's kind of > orthogonal to the functionality of MetricsService. > > I would expect this to be done somewhere like StatisticsRecorder::Initialize() > and then you don't need the plumbing of the main thread task runner through the > client interface. (StatisticsRecorder::Initialize can then be made to take a > TaskRunner param). > > Apologies if this was discussed previously as I didn't read through the previous > discussions here. This wasn't specifically discussed. Here's what was discussed: at some point we need to be able to set a different task runner from the renderer (if and only if Chrome is running in multi-process mode). This will allow base::RecordAction to work from the browser or renderer process, even when Chrome runs in single process mode. I took a quick look and StatisticsRecorder::Initialize is called from a great many places, although mostly tests. In terms of added plumbing I don't know what it entails. It looks like it's called from the renderer even though RecordAction is not currently supported there. In a sense, since this task runner is only used by RecordAction and the likes, I feel it does work well in user_metrics' constructor. Ultimately, though, I'll leave the final call to you and Ilya.
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/1859213002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1859213002/260001
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/1859213002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1859213002/260001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks! https://codereview.chromium.org/1859213002/diff/240001/base/metrics/user_metr... File base/metrics/user_metrics.cc (right): https://codereview.chromium.org/1859213002/diff/240001/base/metrics/user_metr... base/metrics/user_metrics.cc:59: std::vector<ActionCallback>& callbacks = g_callbacks.Get(); On 2016/04/13 14:43:12, Alexei Svitkine wrote: > Nit: non-const refs are discouraged - suggest changing this to a pointer. Hmm, I'm not aware of non-const refs being discouraged within function implementations. (Note: I'm fine with the pointer as well, so there's no need to change anything here. I'd just be interested to know whether the style guide actually says something about this.) https://codereview.chromium.org/1859213002/diff/240001/components/metrics/met... File components/metrics/metrics_service.cc (right): https://codereview.chromium.org/1859213002/diff/240001/components/metrics/met... components/metrics/metrics_service.cc:307: base::SetRecordActionTaskRunner(client_->GetMainThreadTaskRunner()); On 2016/04/13 16:19:36, beaudoin wrote: > On 2016/04/13 14:43:12, Alexei Svitkine wrote: > > I wasn't following the full discussion here. I like the task runner approach, > > however it seems a bit strange to do this in MetricsService, since it's kind > of > > orthogonal to the functionality of MetricsService. > > > > I would expect this to be done somewhere like StatisticsRecorder::Initialize() > > and then you don't need the plumbing of the main thread task runner through > the > > client interface. (StatisticsRecorder::Initialize can then be made to take a > > TaskRunner param). > > > > Apologies if this was discussed previously as I didn't read through the > previous > > discussions here. > > This wasn't specifically discussed. > > Here's what was discussed: at some point we need to be able to set a different > task runner from the renderer (if and only if Chrome is running in multi-process > mode). This will allow base::RecordAction to work from the browser or renderer > process, even when Chrome runs in single process mode. > > I took a quick look and StatisticsRecorder::Initialize is called from a great > many places, although mostly tests. In terms of added plumbing I don't know what > it entails. It looks like it's called from the renderer even though RecordAction > is not currently supported there. > > In a sense, since this task runner is only used by RecordAction and the likes, I > feel it does work well in user_metrics' constructor. Ultimately, though, I'll > leave the final call to you and Ilya. I'm not too particular about where this initialization step happens. The MetricsService seemed like a natural enough place, since it's also the class that registers a callback. However, it wouldn't be out of place to set the task runner from ChromeMetricsServiceClient::Initialize(), or from the StatisticsRecorder::Initialize() method, or just about any metrics initialization code. StatisticsRecorder is a bit stranger a choice than others because, currently, that class deals exclusively with histograms... but it wouldn't be unreasonable to expand its role slightly. I think, all else equal, I'd be happiest with whatever codepath is simplest to support, i.e. requires the least plumbing of extra virtual methods, etc. https://codereview.chromium.org/1859213002/diff/260001/base/metrics/user_metr... File base/metrics/user_metrics.cc (right): https://codereview.chromium.org/1859213002/diff/260001/base/metrics/user_metr... base/metrics/user_metrics.cc:60: if (callbacks->at(i).Equals(callback)) { nit: Please don't use at(), as it can throw an exception. https://codereview.chromium.org/1859213002/diff/260001/base/metrics/user_metr... base/metrics/user_metrics.cc:70: DCHECK(task_runner->BelongsToCurrentThread()); Optional nit: I'd swap the order of these DCHECKs for clarity.
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/1859213002/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1859213002/280001
Alexei, Ilya: PTAL. Moved the call to SetRecordActionTaskRunner outside the MetricsService, placed them in chrome_browser_main and similar as discussed (offline) with Alexei. I think I got them all but going through the CQ now to make sure I got aw/cast/ios correctly. https://codereview.chromium.org/1859213002/diff/260001/base/metrics/user_metr... File base/metrics/user_metrics.cc (right): https://codereview.chromium.org/1859213002/diff/260001/base/metrics/user_metr... base/metrics/user_metrics.cc:60: if (callbacks->at(i).Equals(callback)) { On 2016/04/13 19:23:15, Ilya Sherman wrote: > nit: Please don't use at(), as it can throw an exception. Done. https://codereview.chromium.org/1859213002/diff/260001/base/metrics/user_metr... base/metrics/user_metrics.cc:70: DCHECK(task_runner->BelongsToCurrentThread()); On 2016/04/13 19:23:15, Ilya Sherman wrote: > Optional nit: I'd swap the order of these DCHECKs for clarity. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
lgtm Can you add BUG= and associate this change with it?
LGTM, thanks. I do think this is a cleaner spot to initialize the thread =) https://codereview.chromium.org/1859213002/diff/280001/chromecast/browser/cas... File chromecast/browser/cast_browser_main_parts.cc (right): https://codereview.chromium.org/1859213002/diff/280001/chromecast/browser/cas... chromecast/browser/cast_browser_main_parts.cc:448: // Set the thread on which to execute RecordAction. nit: This comment seems rather redundant with the code; I'd omit it. https://codereview.chromium.org/1859213002/diff/280001/ios/chrome/browser/ios... File ios/chrome/browser/ios_chrome_main_parts.mm (right): https://codereview.chromium.org/1859213002/diff/280001/ios/chrome/browser/ios... ios/chrome/browser/ios_chrome_main_parts.mm:193: // Set the thread on which to execute RecordAction. Ditto.
Description was changed from ========== Move the thread hop for UMA user actions from content:: to base::. ========== to ========== Move the thread hop for UMA user actions from content:: to base::. BUG=604367 ==========
https://codereview.chromium.org/1859213002/diff/280001/chromecast/browser/cas... File chromecast/browser/cast_browser_main_parts.cc (right): https://codereview.chromium.org/1859213002/diff/280001/chromecast/browser/cas... chromecast/browser/cast_browser_main_parts.cc:448: // Set the thread on which to execute RecordAction. On 2016/04/15 22:16:05, Ilya Sherman wrote: > nit: This comment seems rather redundant with the code; I'd omit it. Done. https://codereview.chromium.org/1859213002/diff/280001/ios/chrome/browser/ios... File ios/chrome/browser/ios_chrome_main_parts.mm (right): https://codereview.chromium.org/1859213002/diff/280001/ios/chrome/browser/ios... ios/chrome/browser/ios_chrome_main_parts.mm:193: // Set the thread on which to execute RecordAction. On 2016/04/15 22:16:05, Ilya Sherman wrote: > Ditto. Done.
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/1859213002/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1859213002/300001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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/1859213002/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1859213002/320001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
Patchset #15 (id:320001) has been deleted
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/1859213002/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1859213002/340001
Patchset #15 (id:340001) has been deleted
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/1859213002/360001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1859213002/360001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
beaudoin@chromium.org changed reviewers: + piman@chromium.org, sdefresne@chromium.org, sgurun@chromium.org, slan@chromium.org, thestig@chromium.org
For missing OWNERS review: sgurun: android_webview/browser/aw_browser_context.cc slan: chromecast/browser/cast_browser_main_parts.cc sdefresne: ios/chrome/browser/ios_chrome_main_parts.mm ios/chrome/browser/metrics/mobile_session_shutdown_metrics_provider_unittest.mm ios/web/user_metrics.cc thestig: base/test/user_action_tester.cc base/test/user_action_tester.h chrome/browser/browser_process_impl_unittest.cc chrome/browser/chrome_browser_main.cc piman: content/browser/user_metrics.cc content/public/browser/user_metrics.h (Other files are covered by asvitkine and isherman)
https://codereview.chromium.org/1859213002/diff/360001/chrome/browser/chrome_... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/1859213002/diff/360001/chrome/browser/chrome_... chrome/browser/chrome_browser_main.cc:671: BrowserThread::GetMessageLoopProxyForThread(BrowserThread::UI)); Should this be done in content/ ? There are tests (e.g. layout tests) and other embedders that don't use chrome/
https://codereview.chromium.org/1859213002/diff/360001/base/metrics/user_metr... File base/metrics/user_metrics.cc (right): https://codereview.chromium.org/1859213002/diff/360001/base/metrics/user_metr... base/metrics/user_metrics.cc:20: base::LazyInstance<std::vector<ActionCallback>> g_callbacks = nit: you don't need base:: inside namespace base. https://codereview.chromium.org/1859213002/diff/360001/base/metrics/user_metr... base/metrics/user_metrics.cc:58: std::vector<ActionCallback>* callbacks = &g_callbacks.Get(); Make |callbacks| a reference, or use g_callbacks.Pointer() ? https://codereview.chromium.org/1859213002/diff/360001/base/metrics/user_metr... File base/metrics/user_metrics.h (right): https://codereview.chromium.org/1859213002/diff/360001/base/metrics/user_metr... base/metrics/user_metrics.h:21: // This method *must* be called from the main thread. Does this need updating?
https://codereview.chromium.org/1859213002/diff/360001/base/metrics/user_metr... File base/metrics/user_metrics.h (right): https://codereview.chromium.org/1859213002/diff/360001/base/metrics/user_metr... base/metrics/user_metrics.h:60: // set once, unless ResetRecordActionTaskRunnerForTesting is called. is it optional to call this? if not called what is the default behavior.
Patchset #16 (id:380001) has been deleted
thestig@, sgurun@: PTAL. piman@: See reply. asvitkine@: See request for comment. https://codereview.chromium.org/1859213002/diff/360001/base/metrics/user_metr... File base/metrics/user_metrics.cc (right): https://codereview.chromium.org/1859213002/diff/360001/base/metrics/user_metr... base/metrics/user_metrics.cc:20: base::LazyInstance<std::vector<ActionCallback>> g_callbacks = On 2016/04/18 21:34:13, Lei Zhang wrote: > nit: you don't need base:: inside namespace base. Done. https://codereview.chromium.org/1859213002/diff/360001/base/metrics/user_metr... base/metrics/user_metrics.cc:58: std::vector<ActionCallback>* callbacks = &g_callbacks.Get(); On 2016/04/18 21:34:13, Lei Zhang wrote: > Make |callbacks| a reference, or use g_callbacks.Pointer() ? I used a reference first, but asvitkine@ mentioned that non-const reference were discouraged as local variables. isherman@ wasn't sure :) Anyway, changed to Pointer(). Done. https://codereview.chromium.org/1859213002/diff/360001/base/metrics/user_metr... File base/metrics/user_metrics.h (right): https://codereview.chromium.org/1859213002/diff/360001/base/metrics/user_metr... base/metrics/user_metrics.h:21: // This method *must* be called from the main thread. On 2016/04/18 21:34:13, Lei Zhang wrote: > Does this need updating? Done. https://codereview.chromium.org/1859213002/diff/360001/base/metrics/user_metr... base/metrics/user_metrics.h:60: // set once, unless ResetRecordActionTaskRunnerForTesting is called. On 2016/04/19 15:36:06, sgurun wrote: > is it optional to call this? if not called what is the default behavior. Must be called before any of the other functions here. Added a comment to clarify this. https://codereview.chromium.org/1859213002/diff/360001/chrome/browser/chrome_... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/1859213002/diff/360001/chrome/browser/chrome_... chrome/browser/chrome_browser_main.cc:671: BrowserThread::GetMessageLoopProxyForThread(BrowserThread::UI)); On 2016/04/18 21:13:08, piman wrote: > Should this be done in content/ ? There are tests (e.g. layout tests) and other > embedders that don't use chrome/ asvitkine@ for opinions. My opinion is that other embedders and/or tests are likely to need a different task runner and should set it up. I've done it in user_action_tester.cc using a custom SimpleTaskRunner.
lgtm
There's still a pending question from piman. https://codereview.chromium.org/1859213002/diff/360001/base/metrics/user_metr... File base/metrics/user_metrics.cc (right): https://codereview.chromium.org/1859213002/diff/360001/base/metrics/user_metr... base/metrics/user_metrics.cc:58: std::vector<ActionCallback>* callbacks = &g_callbacks.Get(); On 2016/04/19 16:57:27, beaudoin wrote: > On 2016/04/18 21:34:13, Lei Zhang wrote: > > Make |callbacks| a reference, or use g_callbacks.Pointer() ? > > I used a reference first, but asvitkine@ mentioned that non-const reference were > discouraged as local variables. isherman@ wasn't sure :) Anyway, changed to > Pointer(). > > Done. I'm ok with them. Sometimes they make code easier to read and reduce the amount of dereferencing. e.g. with (*callbacks)[i], but it's not a strong preference. https://codereview.chromium.org/1859213002/diff/400001/base/metrics/user_metr... File base/metrics/user_metrics.h (right): https://codereview.chromium.org/1859213002/diff/400001/base/metrics/user_metr... base/metrics/user_metrics.h:22: // SetRecordActionTaskRunner. nit: SetRecordActionTaskRunner() so it is obvious we are referring to a function here. Ditto below, including line 65.
https://codereview.chromium.org/1859213002/diff/360001/chrome/browser/chrome_... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/1859213002/diff/360001/chrome/browser/chrome_... chrome/browser/chrome_browser_main.cc:671: BrowserThread::GetMessageLoopProxyForThread(BrowserThread::UI)); On 2016/04/19 16:57:27, beaudoin wrote: > On 2016/04/18 21:13:08, piman wrote: > > Should this be done in content/ ? There are tests (e.g. layout tests) and > other > > embedders that don't use chrome/ > > asvitkine@ for opinions. > > My opinion is that other embedders and/or tests are likely to need a different > task runner and should set it up. I've done it in user_action_tester.cc using a > custom SimpleTaskRunner. Yeah, since this only affects cases where AddActionCallback() has been used to set a callback, I don't think it's needed in content - since that is not currently done in content. At least with this CL. I guess if we do want to start using this from content in a future CL, then it can be done at that point.
thestig@: PTAL. (There's an ongoing discussion regarding piman@'s comment, is there anything else I missed?) https://codereview.chromium.org/1859213002/diff/360001/base/metrics/user_metr... File base/metrics/user_metrics.cc (right): https://codereview.chromium.org/1859213002/diff/360001/base/metrics/user_metr... base/metrics/user_metrics.cc:58: std::vector<ActionCallback>* callbacks = &g_callbacks.Get(); On 2016/04/19 17:52:00, Lei Zhang wrote: > On 2016/04/19 16:57:27, beaudoin wrote: > > On 2016/04/18 21:34:13, Lei Zhang wrote: > > > Make |callbacks| a reference, or use g_callbacks.Pointer() ? > > > > I used a reference first, but asvitkine@ mentioned that non-const reference > were > > discouraged as local variables. isherman@ wasn't sure :) Anyway, changed to > > Pointer(). > > > > Done. > > I'm ok with them. Sometimes they make code easier to read and reduce the amount > of dereferencing. e.g. with (*callbacks)[i], but it's not a strong preference. Acknowledged. https://codereview.chromium.org/1859213002/diff/400001/base/metrics/user_metr... File base/metrics/user_metrics.h (right): https://codereview.chromium.org/1859213002/diff/400001/base/metrics/user_metr... base/metrics/user_metrics.h:22: // SetRecordActionTaskRunner. On 2016/04/19 17:52:00, Lei Zhang wrote: > nit: SetRecordActionTaskRunner() so it is obvious we are referring to a function > here. Ditto below, including line 65. Done.
On Tue, Apr 19, 2016 at 10:56 AM, <asvitkine@chromium.org> wrote: > > > https://codereview.chromium.org/1859213002/diff/360001/chrome/browser/chrome_... > File chrome/browser/chrome_browser_main.cc (right): > > > https://codereview.chromium.org/1859213002/diff/360001/chrome/browser/chrome_... > chrome/browser/chrome_browser_main.cc:671: > BrowserThread::GetMessageLoopProxyForThread(BrowserThread::UI)); > On 2016/04/19 16:57:27, beaudoin wrote: > > On 2016/04/18 21:13:08, piman wrote: > > > Should this be done in content/ ? There are tests (e.g. layout > tests) and > > other > > > embedders that don't use chrome/ > > > > asvitkine@ for opinions. > > > > My opinion is that other embedders and/or tests are likely to need a > different > > task runner and should set it up. I've done it in > user_action_tester.cc using a > > custom SimpleTaskRunner. > > Yeah, since this only affects cases where AddActionCallback() has been > used to set a callback, I don't think it's needed in content - since > that is not currently done in content. At least with this CL. > > I guess if we do want to start using this from content in a future CL, > then it can be done at that point. > I thought this was meant to enable https://codereview.chromium.org/1683583002 That one will need to set the task runner for the renderer in content/ It seems inconsistent to have one set in content/ and one set in chrome/ ? I'd rather have content define for its embedder "Action callbacks happen on the UI thread (browser) / renderer main thread (renderer)", so that all embedders are consistent. > https://codereview.chromium.org/1859213002/ > -- 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.
ios lgtm
On 2016/04/20 07:13:51, sdefresne wrote: > ios lgtm cast lgtm
On 2016/04/20 14:24:58, slan wrote: > On 2016/04/20 07:13:51, sdefresne wrote: > > ios lgtm > > cast lgtm I've been talking with Alexei offline and he suggested a different approach: Passing the task_runner as a second parameter to AddActionCallback. This would require doing something similar to patchset 12 to allow the task_runner can be accessed from wherever we call AddActionCallback. I want to discuss this before going ahead so everyone with an opinion can agree. Here's mine: 1) AddActionCallback can be called multiple times. However we should never change the task_runner (except when testing). Therefore it sounds like AddActionCallback doesn't have the right semantics for setting the task_runner. 2) RecordAction also needs a task_runner before it can run, yet there's nothing disallowing calling RecordAction before the first callback is registered. It happens in tests. It shouldn't happen in prod but it does (there's a bug tracking this). Setting the task_runner in AddActionCallback will therefore force us to do an extra check in RecordAction. 3) RemoveActionCallback is sometimes called with null (ie. the code that cleans up the action callback is called before an AddActionCallback was called). I've only seen this in test but cannot guarantee it never happens in prod. This means the task_runner is not necessarily set when RemoveActionCallback is called. We will therefore need an extra test in there as well. 4) There is nothing preventing AddActionCallback from being called from pretty deep, which means we'll have to pipe a task_runner to these arbitrary depths. With the SetRecordActionTaskRunner of PatchSet 12 we only have to pipe that task_runner to the initialization code. For these reasons, my favorite option is going back to PatchSet 12, where the task_runner was set in content, specifically by the component that needs this task_runner to be set.
On Thu, Apr 21, 2016 at 11:31 AM, <beaudoin@chromium.org> wrote: > On 2016/04/20 14:24:58, slan wrote: > > On 2016/04/20 07:13:51, sdefresne wrote: > > > ios lgtm > > > > cast lgtm > > I've been talking with Alexei offline and he suggested a different > approach: > Passing the task_runner as a second parameter to AddActionCallback. This > would > require doing something similar to patchset 12 to allow the task_runner > can be > accessed from wherever we call AddActionCallback. > > I want to discuss this before going ahead so everyone with an opinion can > agree. > Here's mine: > > 1) AddActionCallback can be called multiple times. However we should never > change the task_runner (except when testing). Therefore it sounds like > AddActionCallback doesn't have the right semantics for setting the > task_runner. > I think we may have a misunderstanding. My suggestion is to store a task runner with each callback, so that a given callback may have a different task runner. Basically, saying "here's the callback I want you to run on this task runner". This way, there is no "global task runner" - instead each callback can independently specify which task runner it needs and there's no conflict between separate ones. (e.g. if we ever need multiple callbacks to be run on different threads, the system would automatically support this with this proposal) > > 2) RecordAction also needs a task_runner before it can run, yet there's > nothing > disallowing calling RecordAction before the first callback is registered. > It > happens in tests. It shouldn't happen in prod but it does (there's a bug > tracking this). Setting the task_runner in AddActionCallback will therefore > force us to do an extra check in RecordAction. > So if there's no action callback registered, the RecordAction calls do nothing (since all it does is run the current set of callbacks). This would still be the case in the new system so there's no new issues that would be introduced. There's no extra check needed, I think. > > 3) RemoveActionCallback is sometimes called with null (ie. the code that > cleans > up the action callback is called before an AddActionCallback was called). > I've > only seen this in test but cannot guarantee it never happens in prod. This > means > the task_runner is not necessarily set when RemoveActionCallback is > called. We > will therefore need an extra test in there as well. > > 4) There is nothing preventing AddActionCallback from being called from > pretty > deep, which means we'll have to pipe a task_runner to these arbitrary > depths. > With the SetRecordActionTaskRunner of PatchSet 12 we only have to pipe that > task_runner to the initialization code. > > For these reasons, my favorite option is going back to PatchSet 12, where > the > task_runner was set in content, specifically by the component that needs > this > task_runner to be set. > > https://codereview.chromium.org/1859213002/ > -- 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/21 17:06:25, Alexei Svitkine wrote: > On Thu, Apr 21, 2016 at 11:31 AM, <mailto:beaudoin@chromium.org> wrote: > > > On 2016/04/20 14:24:58, slan wrote: > > > On 2016/04/20 07:13:51, sdefresne wrote: > > > > ios lgtm > > > > > > cast lgtm > > > > I've been talking with Alexei offline and he suggested a different > > approach: > > Passing the task_runner as a second parameter to AddActionCallback. This > > would > > require doing something similar to patchset 12 to allow the task_runner > > can be > > accessed from wherever we call AddActionCallback. > > > > I want to discuss this before going ahead so everyone with an opinion can > > agree. > > Here's mine: > > > > 1) AddActionCallback can be called multiple times. However we should never > > change the task_runner (except when testing). Therefore it sounds like > > AddActionCallback doesn't have the right semantics for setting the > > task_runner. > > > > I think we may have a misunderstanding. My suggestion is to store a task > runner with each callback, so that a given callback may have a different > task runner. Basically, saying "here's the callback I want you to run on > this task runner". > > This way, there is no "global task runner" - instead each callback can > independently specify which task runner it needs and there's no conflict > between separate ones. (e.g. if we ever need multiple callbacks to be run > on different threads, the system would automatically support this with this > proposal) Wouldn't we need a global task runner to ensure that operations on the list of callbacks all occur on the same thread? That is, AddActionCallback and RemoveActionCallback currently do the thread hop to sidestep that problem. Do we really need one task runner per callback? Isn't that over design? > > > > 2) RecordAction also needs a task_runner before it can run, yet there's > > nothing > > disallowing calling RecordAction before the first callback is registered. > > It > > happens in tests. It shouldn't happen in prod but it does (there's a bug > > tracking this). Setting the task_runner in AddActionCallback will therefore > > force us to do an extra check in RecordAction. > > > > So if there's no action callback registered, the RecordAction calls do > nothing (since all it does is run the current set of callbacks). This would > still be the case in the new system so there's no new issues that would be > introduced. There's no extra check needed, I think. See above: you need to go through the list of callbacks on a specific and unique thread (unless we use start using locks), so you need a global thread still.
That's a good point. :\ "For these reasons, my favorite option is going back to PatchSet 12, where the task_runner was set in content, specifically by the component that needs this task_runner to be set." I looked at patchset 12 and in that patchset you're setting the task runner from components/metrics - which is not content/. The components/metrics code doesn't get run by non-Chrome (e.g. content shell), so it has the exact same problems as the current patchset w.r.t. to that complaint. I'm fine with it being done in content/ if you find a good place to do it, but patchset 12 isn't it. On Thu, Apr 21, 2016 at 1:37 PM, <beaudoin@chromium.org> wrote: > On 2016/04/21 17:06:25, Alexei Svitkine wrote: > > > On Thu, Apr 21, 2016 at 11:31 AM, <mailto:beaudoin@chromium.org> wrote: > > > > > On 2016/04/20 14:24:58, slan wrote: > > > > On 2016/04/20 07:13:51, sdefresne wrote: > > > > > ios lgtm > > > > > > > > cast lgtm > > > > > > I've been talking with Alexei offline and he suggested a different > > > approach: > > > Passing the task_runner as a second parameter to AddActionCallback. > This > > > would > > > require doing something similar to patchset 12 to allow the task_runner > > > can be > > > accessed from wherever we call AddActionCallback. > > > > > > I want to discuss this before going ahead so everyone with an opinion > can > > > agree. > > > Here's mine: > > > > > > 1) AddActionCallback can be called multiple times. However we should > never > > > change the task_runner (except when testing). Therefore it sounds like > > > AddActionCallback doesn't have the right semantics for setting the > > > task_runner. > > > > > > > I think we may have a misunderstanding. My suggestion is to store a task > > runner with each callback, so that a given callback may have a different > > task runner. Basically, saying "here's the callback I want you to run on > > this task runner". > > > > This way, there is no "global task runner" - instead each callback can > > independently specify which task runner it needs and there's no conflict > > between separate ones. (e.g. if we ever need multiple callbacks to be run > > on different threads, the system would automatically support this with > this > > proposal) > > Wouldn't we need a global task runner to ensure that operations on the > list of > callbacks all occur on the same thread? That is, AddActionCallback and > RemoveActionCallback currently do the thread hop to sidestep that problem. > > Do we really need one task runner per callback? Isn't that over design? > > > > > > > 2) RecordAction also needs a task_runner before it can run, yet there's > > > nothing > > > disallowing calling RecordAction before the first callback is > registered. > > > It > > > happens in tests. It shouldn't happen in prod but it does (there's a > bug > > > tracking this). Setting the task_runner in AddActionCallback will > therefore > > > force us to do an extra check in RecordAction. > > > > > > > So if there's no action callback registered, the RecordAction calls do > > nothing (since all it does is run the current set of callbacks). This > would > > still be the case in the new system so there's no new issues that would > be > > introduced. There's no extra check needed, I think. > > See above: you need to go through the list of callbacks on a specific and > unique > thread (unless we use start using locks), so you need a global thread > still. > > > https://codereview.chromium.org/1859213002/ > -- 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/21 17:47:01, Alexei Svitkine wrote: > That's a good point. :\ > > "For these reasons, my favorite option is going back to PatchSet 12, where > the > task_runner was set in content, specifically by the component that needs > this > task_runner to be set." > > I looked at patchset 12 and in that patchset you're setting the task runner > from components/metrics - which is not content/. > > The components/metrics code doesn't get run by non-Chrome (e.g. content > shell), so it has the exact same problems as the current patchset w.r.t. to > that complaint. I'm fine with it being done in content/ if you find a good > place to do it, but patchset 12 isn't it. > > On Thu, Apr 21, 2016 at 1:37 PM, <mailto:beaudoin@chromium.org> wrote: > > > On 2016/04/21 17:06:25, Alexei Svitkine wrote: > > > > > On Thu, Apr 21, 2016 at 11:31 AM, <mailto:beaudoin@chromium.org> wrote: > > > > > > > On 2016/04/20 14:24:58, slan wrote: > > > > > On 2016/04/20 07:13:51, sdefresne wrote: > > > > > > ios lgtm > > > > > > > > > > cast lgtm > > > > > > > > I've been talking with Alexei offline and he suggested a different > > > > approach: > > > > Passing the task_runner as a second parameter to AddActionCallback. > > This > > > > would > > > > require doing something similar to patchset 12 to allow the task_runner > > > > can be > > > > accessed from wherever we call AddActionCallback. > > > > > > > > I want to discuss this before going ahead so everyone with an opinion > > can > > > > agree. > > > > Here's mine: > > > > > > > > 1) AddActionCallback can be called multiple times. However we should > > never > > > > change the task_runner (except when testing). Therefore it sounds like > > > > AddActionCallback doesn't have the right semantics for setting the > > > > task_runner. > > > > > > > > > > I think we may have a misunderstanding. My suggestion is to store a task > > > runner with each callback, so that a given callback may have a different > > > task runner. Basically, saying "here's the callback I want you to run on > > > this task runner". > > > > > > This way, there is no "global task runner" - instead each callback can > > > independently specify which task runner it needs and there's no conflict > > > between separate ones. (e.g. if we ever need multiple callbacks to be run > > > on different threads, the system would automatically support this with > > this > > > proposal) > > > > Wouldn't we need a global task runner to ensure that operations on the > > list of > > callbacks all occur on the same thread? That is, AddActionCallback and > > RemoveActionCallback currently do the thread hop to sidestep that problem. > > > > Do we really need one task runner per callback? Isn't that over design? > > > > > > > > > > 2) RecordAction also needs a task_runner before it can run, yet there's > > > > nothing > > > > disallowing calling RecordAction before the first callback is > > registered. > > > > It > > > > happens in tests. It shouldn't happen in prod but it does (there's a > > bug > > > > tracking this). Setting the task_runner in AddActionCallback will > > therefore > > > > force us to do an extra check in RecordAction. > > > > > > > > > > So if there's no action callback registered, the RecordAction calls do > > > nothing (since all it does is run the current set of callbacks). This > > would > > > still be the case in the new system so there's no new issues that would > > be > > > introduced. There's no extra check needed, I think. > > > > See above: you need to go through the list of callbacks on a specific and > > unique > > thread (unless we use start using locks), so you need a global thread > > still. > > > > > > https://codereview.chromium.org/1859213002/ > > > > -- > 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. SG. I don't know much about content and looking around I can't find the right place to do that initialization. piman@: Any hint?
On Thu, Apr 21, 2016 at 12:01 PM, <beaudoin@chromium.org> wrote: > On 2016/04/21 17:47:01, Alexei Svitkine wrote: > > That's a good point. :\ > > > > "For these reasons, my favorite option is going back to PatchSet 12, > where > > the > > task_runner was set in content, specifically by the component that needs > > this > > task_runner to be set." > > > > I looked at patchset 12 and in that patchset you're setting the task > runner > > from components/metrics - which is not content/. > > > > The components/metrics code doesn't get run by non-Chrome (e.g. content > > shell), so it has the exact same problems as the current patchset w.r.t. > to > > that complaint. I'm fine with it being done in content/ if you find a > good > > place to do it, but patchset 12 isn't it. > > > > On Thu, Apr 21, 2016 at 1:37 PM, <mailto:beaudoin@chromium.org> wrote: > > > > > On 2016/04/21 17:06:25, Alexei Svitkine wrote: > > > > > > > On Thu, Apr 21, 2016 at 11:31 AM, <mailto:beaudoin@chromium.org> > wrote: > > > > > > > > > On 2016/04/20 14:24:58, slan wrote: > > > > > > On 2016/04/20 07:13:51, sdefresne wrote: > > > > > > > ios lgtm > > > > > > > > > > > > cast lgtm > > > > > > > > > > I've been talking with Alexei offline and he suggested a different > > > > > approach: > > > > > Passing the task_runner as a second parameter to AddActionCallback. > > > This > > > > > would > > > > > require doing something similar to patchset 12 to allow the > task_runner > > > > > can be > > > > > accessed from wherever we call AddActionCallback. > > > > > > > > > > I want to discuss this before going ahead so everyone with an > opinion > > > can > > > > > agree. > > > > > Here's mine: > > > > > > > > > > 1) AddActionCallback can be called multiple times. However we > should > > > never > > > > > change the task_runner (except when testing). Therefore it sounds > like > > > > > AddActionCallback doesn't have the right semantics for setting the > > > > > task_runner. > > > > > > > > > > > > > I think we may have a misunderstanding. My suggestion is to store a > task > > > > runner with each callback, so that a given callback may have a > different > > > > task runner. Basically, saying "here's the callback I want you to > run on > > > > this task runner". > > > > > > > > This way, there is no "global task runner" - instead each callback > can > > > > independently specify which task runner it needs and there's no > conflict > > > > between separate ones. (e.g. if we ever need multiple callbacks to > be run > > > > on different threads, the system would automatically support this > with > > > this > > > > proposal) > > > > > > Wouldn't we need a global task runner to ensure that operations on the > > > list of > > > callbacks all occur on the same thread? That is, AddActionCallback and > > > RemoveActionCallback currently do the thread hop to sidestep that > problem. > > > > > > Do we really need one task runner per callback? Isn't that over design? > > > > > > > > > > > > > 2) RecordAction also needs a task_runner before it can run, yet > there's > > > > > nothing > > > > > disallowing calling RecordAction before the first callback is > > > registered. > > > > > It > > > > > happens in tests. It shouldn't happen in prod but it does (there's > a > > > bug > > > > > tracking this). Setting the task_runner in AddActionCallback will > > > therefore > > > > > force us to do an extra check in RecordAction. > > > > > > > > > > > > > So if there's no action callback registered, the RecordAction calls > do > > > > nothing (since all it does is run the current set of callbacks). This > > > would > > > > still be the case in the new system so there's no new issues that > would > > > be > > > > introduced. There's no extra check needed, I think. > > > > > > See above: you need to go through the list of callbacks on a specific > and > > > unique > > > thread (unless we use start using locks), so you need a global thread > > > still. > > > > > > > > > https://codereview.chromium.org/1859213002/ > > > > > > > -- > > 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. > > SG. I don't know much about content and looking around I can't find the > right > place to do that initialization. > > piman@: Any hint? > BrowserMainLoop::PostMainMessageLoopStart is probably the right place. > https://codereview.chromium.org/1859213002/ > -- 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.
Patchset #18 (id:440001) has been deleted
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/1859213002/460001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1859213002/460001
On 2016/04/22 01:46:40, commit-bot: I haz the power wrote: > Dry run: CQ is trying da patch. Follow status at > https://chromium-cq-status.appspot.com/patch-status/1859213002/460001 > View timeline at > https://chromium-cq-status.appspot.com/patch-timeline/1859213002/460001 piman@: PTAL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
lgtm
The CQ bit was checked by beaudoin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org, asvitkine@chromium.org, sgurun@chromium.org, slan@chromium.org, sdefresne@chromium.org Link to the patchset: https://codereview.chromium.org/1859213002/#ps460001 (title: "Initializing in content.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1859213002/460001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1859213002/460001
Message was sent while issue was closed.
Description was changed from ========== Move the thread hop for UMA user actions from content:: to base::. BUG=604367 ========== to ========== Move the thread hop for UMA user actions from content:: to base::. BUG=604367 ==========
Message was sent while issue was closed.
Committed patchset #18 (id:460001)
Message was sent while issue was closed.
lgtm https://codereview.chromium.org/1859213002/diff/460001/content/browser/browse... File content/browser/browser_main_loop.cc (right): https://codereview.chromium.org/1859213002/diff/460001/content/browser/browse... content/browser/browser_main_loop.cc:594: { Nit: why the {}'s? https://codereview.chromium.org/1859213002/diff/460001/ios/chrome/browser/ios... File ios/chrome/browser/ios_chrome_main_parts.mm (right): https://codereview.chromium.org/1859213002/diff/460001/ios/chrome/browser/ios... ios/chrome/browser/ios_chrome_main_parts.mm:193: base::SetRecordActionTaskRunner( Nit: Add a comment about where this is done on other platforms.
Message was sent while issue was closed.
Description was changed from ========== Move the thread hop for UMA user actions from content:: to base::. BUG=604367 ========== to ========== Move the thread hop for UMA user actions from content:: to base::. BUG=604367 Committed: https://crrev.com/d5c435e0b76ef29580d018cdbe8717fefd2ddd03 Cr-Commit-Position: refs/heads/master@{#389097} ==========
Message was sent while issue was closed.
Patchset 18 (id:??) landed as https://crrev.com/d5c435e0b76ef29580d018cdbe8717fefd2ddd03 Cr-Commit-Position: refs/heads/master@{#389097} |
