Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2769963005:
Add EnabledStateObserver to BackgroundTracingManager (Closed)
DescriptionAdd EnabledStateObserver to BackgroundTracingManager
This CL removes the memory-infra related code into memory-tracing
observer. Also the tests implement the observer instead of setting
callbacks.
BUG=700245
Review-Url: https://codereview.chromium.org/2769963005
Cr-Commit-Position: refs/heads/master@{#460293}
Committed: https://chromium.googlesource.com/chromium/src/+/f64ac5cb273e3abab3564f1a5997b3da164ab9ff
Patch Set 1 #
Total comments: 11
Patch Set 2 : Fixes. #Patch Set 3 : Add destructor and move CONTENT_EXPORT. #
Messages
Total messages: 42 (30 generated)
Patchset #1 (id:1) has been deleted
The CQ bit was checked by ssid@chromium.org to run a CQ dry run
Patchset #1 (id:20001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
ssid@chromium.org changed reviewers: + oysteine@chromium.org
+Oystein: I am planning to add more memory-infra specific stuff in the field trial config. So, before I do that I think it's better to split memory related stuff out of BackgroundTracingManagerImpl. There is still once case where we set memory dump config in the manager. I will come back to it later. wdyt?
Overall looks like a good solution! https://codereview.chromium.org/2769963005/diff/40001/content/browser/tracing... File content/browser/tracing/background_memory_tracing_observer.cc (right): https://codereview.chromium.org/2769963005/diff/40001/content/browser/tracing... content/browser/tracing/background_memory_tracing_observer.cc:14: static auto* instance = new BackgroundMemoryTracingObserver; Why isn't this a global base::LazyInstance? I don't think function static variables are allowed. https://codereview.chromium.org/2769963005/diff/40001/content/browser/tracing... File content/browser/tracing/background_memory_tracing_observer.h (right): https://codereview.chromium.org/2769963005/diff/40001/content/browser/tracing... content/browser/tracing/background_memory_tracing_observer.h:23: ~BackgroundMemoryTracingObserver(); override https://codereview.chromium.org/2769963005/diff/40001/content/browser/tracing... File content/browser/tracing/background_tracing_manager_browsertest.cc (right): https://codereview.chromium.org/2769963005/diff/40001/content/browser/tracing... content/browser/tracing/background_tracing_manager_browsertest.cc:30: ~TestBackgroundTracingObserver(); override https://codereview.chromium.org/2769963005/diff/40001/content/browser/tracing... content/browser/tracing/background_tracing_manager_browsertest.cc:44: tracing_enabled_callback_ = tracing_enabled_callback; nit: put this in the initializer list https://codereview.chromium.org/2769963005/diff/40001/content/browser/tracing... content/browser/tracing/background_tracing_manager_browsertest.cc:46: BackgroundTracingManager::GetInstance()) You should be able to call BackgroundTracingManagerImpl::GetInstance() directly here, since you're still inside content/browser/.
Patchset #2 (id:60001) has been deleted
Fixed, thanks. ptal https://codereview.chromium.org/2769963005/diff/40001/content/browser/tracing... File content/browser/tracing/background_memory_tracing_observer.cc (right): https://codereview.chromium.org/2769963005/diff/40001/content/browser/tracing... content/browser/tracing/background_memory_tracing_observer.cc:14: static auto* instance = new BackgroundMemoryTracingObserver; On 2017/03/28 20:11:18, oystein wrote: > Why isn't this a global base::LazyInstance? I don't think function static > variables are allowed. I think thread safe statics are the new way to use singleton in Chrome. Scott has been working on changing existing instances to statics. https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/lazyin... https://bugs.chromium.org/p/chromium/issues/detail?id=686866 https://codereview.chromium.org/2769963005/diff/40001/content/browser/tracing... File content/browser/tracing/background_memory_tracing_observer.h (right): https://codereview.chromium.org/2769963005/diff/40001/content/browser/tracing... content/browser/tracing/background_memory_tracing_observer.h:23: ~BackgroundMemoryTracingObserver(); On 2017/03/28 20:11:18, oystein wrote: > override Done. https://codereview.chromium.org/2769963005/diff/40001/content/browser/tracing... File content/browser/tracing/background_tracing_manager_browsertest.cc (right): https://codereview.chromium.org/2769963005/diff/40001/content/browser/tracing... content/browser/tracing/background_tracing_manager_browsertest.cc:30: ~TestBackgroundTracingObserver(); On 2017/03/28 20:11:18, oystein wrote: > override Done. https://codereview.chromium.org/2769963005/diff/40001/content/browser/tracing... content/browser/tracing/background_tracing_manager_browsertest.cc:44: tracing_enabled_callback_ = tracing_enabled_callback; On 2017/03/28 20:11:18, oystein wrote: > nit: put this in the initializer list Done. https://codereview.chromium.org/2769963005/diff/40001/content/browser/tracing... content/browser/tracing/background_tracing_manager_browsertest.cc:46: BackgroundTracingManager::GetInstance()) On 2017/03/28 20:11:18, oystein wrote: > You should be able to call BackgroundTracingManagerImpl::GetInstance() directly > here, since you're still inside content/browser/. Ah I did not see it is public. Did this because it was existing in this file earlier. Fixed.
ssid@chromium.org changed reviewers: + boliu@chromium.org
+Bo for BUILD.gn file change.
-Bo since I anyway need another content reviewer. Sorry for the spam.
Description was changed from ========== Add EnabledStateObserver to BackgroundTracingManager This CL removes the memory-infra related code into memory-tracing observer. Also the tests implement the observer instead of setting callbacks. BUG=700245 ========== to ========== Add EnabledStateObserver to BackgroundTracingManager This CL removes the memory-infra related code into memory-tracing observer. Also the tests implement the observer instead of setting callbacks. BUG=700245 ==========
ssid@chromium.org changed reviewers: - boliu@chromium.org
lgtm and thanks! https://codereview.chromium.org/2769963005/diff/40001/content/browser/tracing... File content/browser/tracing/background_memory_tracing_observer.cc (right): https://codereview.chromium.org/2769963005/diff/40001/content/browser/tracing... content/browser/tracing/background_memory_tracing_observer.cc:14: static auto* instance = new BackgroundMemoryTracingObserver; On 2017/03/28 at 21:02:03, ssid wrote: > On 2017/03/28 20:11:18, oystein wrote: > > Why isn't this a global base::LazyInstance? I don't think function static > > variables are allowed. > > I think thread safe statics are the new way to use singleton in Chrome. Scott has been working on changing existing instances to statics. > https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/lazyin... > > https://bugs.chromium.org/p/chromium/issues/detail?id=686866 Ah good to know, missed that. Thanks!
ssid@chromium.org changed reviewers: + alexmos@chromium.org
+alexmos: Please review: content/browser/BUILD.gn content/public/browser/background_tracing_manager.h Thanks!
On 2017/03/28 21:09:44, ssid wrote: > +alexmos: Please review: > content/browser/BUILD.gn > content/public/browser/background_tracing_manager.h > > Thanks! Those two files LGTM
The CQ bit was checked by ssid@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by ssid@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by ssid@chromium.org to run a CQ dry run
Patchset #4 (id:110001) has been deleted
Patchset #3 (id:100001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I moved the content_export to just required functions instead of whole class.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_TIMED_OUT, build hasn't started yet, builder probably lacks capacity)
The CQ bit was checked by ssid@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from oysteine@chromium.org, alexmos@chromium.org Link to the patchset: https://codereview.chromium.org/2769963005/#ps130001 (title: "Add destructor and move CONTENT_EXPORT.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 130001, "attempt_start_ts": 1490761510498590,
"parent_rev": "593eebedc4013ea212b05c21dba300d41a6d7977", "commit_rev":
"f64ac5cb273e3abab3564f1a5997b3da164ab9ff"}
Message was sent while issue was closed.
Description was changed from ========== Add EnabledStateObserver to BackgroundTracingManager This CL removes the memory-infra related code into memory-tracing observer. Also the tests implement the observer instead of setting callbacks. BUG=700245 ========== to ========== Add EnabledStateObserver to BackgroundTracingManager This CL removes the memory-infra related code into memory-tracing observer. Also the tests implement the observer instead of setting callbacks. BUG=700245 Review-Url: https://codereview.chromium.org/2769963005 Cr-Commit-Position: refs/heads/master@{#460293} Committed: https://chromium.googlesource.com/chromium/src/+/f64ac5cb273e3abab3564f1a5997... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:130001) as https://chromium.googlesource.com/chromium/src/+/f64ac5cb273e3abab3564f1a5997...
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:130001) has been created in https://codereview.chromium.org/2785663002/ by kolos@chromium.org. The reason for reverting is: This CL causes tests failures (browser_side_navigation_content_browsertests, content_browsertests, site_per_process_content_browsertests). https://findit-for-me.appspot.com/waterfall/build-failure?url=https://build.c.... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
