Chromium Code Reviews
Help | Chromium Project | Sign in
(5)

Issue 2819413002: [memory-infra] Remove MemoryDumpManagerDelegate (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 week, 4 days ago by ssid
Modified:
3 days, 14 hours ago
CC:
chrome-grc-reviews_chromium.org, chromium-reviews, danakj+watch_chromium.org, darin-cc_chromium.org, jam, tracing+reviews_chromium.org, vmpstr+watch_chromium.org, wfh+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

[memory-infra] Remove MemoryDumpManagerDelegate This CL removes the unnecessary Delegate in MemoryDumpManager and just uses a callback to request global dumps. BUG=703184 Review-Url: https://codereview.chromium.org/2819413002 Cr-Commit-Position: refs/heads/master@{#467197} Committed: https://chromium.googlesource.com/chromium/src/+/f91ad00d0d78fe00a5d1ca4fc50ace6cd63718a7

Patch Set 1 #

Total comments: 22

Patch Set 2 : Renames, fix build.gn #

Patch Set 3 : Add export macros #

Total comments: 1

Patch Set 4 : rebase. #

Patch Set 5 : mojom stuff in component. #

Patch Set 6 : rebase. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+257 lines, -534 lines) Patch
M base/trace_event/memory_dump_manager.h View 1 2 3 4 5 9 chunks +29 lines, -42 lines 0 comments Download
M base/trace_event/memory_dump_manager.cc View 1 2 3 4 5 6 chunks +14 lines, -10 lines 0 comments Download
M base/trace_event/memory_dump_manager_unittest.cc View 1 2 3 4 5 36 chunks +67 lines, -66 lines 0 comments Download
M content/child/BUILD.gn View 1 1 chunk +1 line, -1 line 0 comments Download
M content/child/child_thread_impl.cc View 1 2 chunks +4 lines, -6 lines 0 comments Download
M content/common/BUILD.gn View 1 2 3 4 5 1 chunk +1 line, -1 line 0 comments Download
M services/resource_coordinator/BUILD.gn View 1 2 3 4 2 chunks +3 lines, -5 lines 0 comments Download
M services/resource_coordinator/memory/coordinator/coordinator_impl.cc View 1 2 chunks +3 lines, -5 lines 0 comments Download
M services/resource_coordinator/public/cpp/BUILD.gn View 1 2 3 4 3 chunks +11 lines, -5 lines 0 comments Download
D services/resource_coordinator/public/cpp/memory/memory_dump_manager_delegate_impl.h View 1 chunk +0 lines, -100 lines 0 comments Download
D services/resource_coordinator/public/cpp/memory/memory_dump_manager_delegate_impl.cc View 1 chunk +0 lines, -96 lines 0 comments Download
D services/resource_coordinator/public/cpp/memory/memory_dump_manager_delegate_impl_unittest.cc View 1 chunk +0 lines, -135 lines 0 comments Download
A services/resource_coordinator/public/cpp/memory/memory_export.h View 1 2 1 chunk +32 lines, -0 lines 0 comments Download
M services/resource_coordinator/public/cpp/memory/memory_instrumentation_struct_traits.h View 1 2 3 6 chunks +17 lines, -12 lines 0 comments Download
A + services/resource_coordinator/public/cpp/memory/process_local_dump_manager_impl.h View 1 2 3 4 4 chunks +21 lines, -22 lines 0 comments Download
A + services/resource_coordinator/public/cpp/memory/process_local_dump_manager_impl.cc View 1 5 chunks +34 lines, -18 lines 0 comments Download
A + services/resource_coordinator/public/cpp/memory/process_local_dump_manager_impl_unittest.cc View 6 chunks +10 lines, -9 lines 0 comments Download
M services/resource_coordinator/public/interfaces/BUILD.gn View 1 2 3 4 2 chunks +10 lines, -1 line 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 107 (66 generated)
ssid
ptal thanks
1 week, 4 days ago (2017-04-18 02:25:40 UTC) #4
fmeawad
https://codereview.chromium.org/2819413002/diff/1/base/trace_event/memory_dump_manager.h File base/trace_event/memory_dump_manager.h (right): https://codereview.chromium.org/2819413002/diff/1/base/trace_event/memory_dump_manager.h#newcode180 base/trace_event/memory_dump_manager.h:180: // todo fix Don't forget this TODO
1 week, 4 days ago (2017-04-18 14:48:11 UTC) #9
chiniforooshan
https://codereview.chromium.org/2819413002/diff/1/services/resource_coordinator/public/cpp/memory/process_local_dump_manager_impl.cc File services/resource_coordinator/public/cpp/memory/process_local_dump_manager_impl.cc (right): https://codereview.chromium.org/2819413002/diff/1/services/resource_coordinator/public/cpp/memory/process_local_dump_manager_impl.cc#newcode23 services/resource_coordinator/public/cpp/memory/process_local_dump_manager_impl.cc:23: static ProcessLocalDumpManagerImpl* instance = nullptr; Did you test this ...
1 week, 4 days ago (2017-04-18 15:11:25 UTC) #11
ssid
https://codereview.chromium.org/2819413002/diff/1/services/resource_coordinator/public/cpp/memory/process_local_dump_manager_impl.cc File services/resource_coordinator/public/cpp/memory/process_local_dump_manager_impl.cc (right): https://codereview.chromium.org/2819413002/diff/1/services/resource_coordinator/public/cpp/memory/process_local_dump_manager_impl.cc#newcode23 services/resource_coordinator/public/cpp/memory/process_local_dump_manager_impl.cc:23: static ProcessLocalDumpManagerImpl* instance = nullptr; On 2017/04/18 15:11:24, chiniforooshan ...
1 week, 3 days ago (2017-04-18 19:11:45 UTC) #12
Primiano Tucci
Sorry didn't have any time to look at this today, will do tomorrow. I haven't ...
1 week, 3 days ago (2017-04-18 19:31:25 UTC) #13
chiniforooshan
On 2017/04/18 19:31:25, Primiano Tucci wrote: > Sorry didn't have any time to look at ...
1 week, 3 days ago (2017-04-18 19:49:10 UTC) #14
Primiano Tucci
On 2017/04/18 19:49:10, chiniforooshan wrote: > And I disagree that creating an object > with ...
1 week, 3 days ago (2017-04-18 19:51:55 UTC) #15
Primiano Tucci
Okay I took a look to the CL (which looks great % this discussion), now ...
1 week, 3 days ago (2017-04-19 14:24:09 UTC) #16
chiniforooshan
On 2017/04/19 14:24:09, Primiano Tucci wrote: > Okay I took a look to the CL ...
1 week, 3 days ago (2017-04-19 14:48:04 UTC) #17
Primiano Tucci
Took a look to the rest, just minor comments, LG on the rest if we ...
1 week, 2 days ago (2017-04-19 15:44:36 UTC) #18
ssid
Thanks made suggested changes. https://codereview.chromium.org/2819413002/diff/1/base/trace_event/memory_dump_manager.h File base/trace_event/memory_dump_manager.h (right): https://codereview.chromium.org/2819413002/diff/1/base/trace_event/memory_dump_manager.h#newcode69 base/trace_event/memory_dump_manager.h:69: using RequestGlobalDumpCallback = On 2017/04/19 ...
1 week, 2 days ago (2017-04-20 01:18:21 UTC) #19
ssid
On 2017/04/20 01:18:21, ssid wrote: > Thanks made suggested changes. except the 2 changes in ...
1 week, 2 days ago (2017-04-20 01:19:03 UTC) #20
ssid
+jochen for 2 small changes in content/child.
1 week, 2 days ago (2017-04-20 01:21:39 UTC) #22
jochen (slow until May 2)
lgtm
1 week, 2 days ago (2017-04-20 09:49:51 UTC) #23
fmeawad
1 week, 1 day ago (2017-04-20 16:06:50 UTC) #25
Primiano Tucci
And when I wrote LG I actually meant lgtm assuming comments have been addressed in ...
1 week, 1 day ago (2017-04-20 19:28:29 UTC) #26
ssid
On 2017/04/20 19:28:29, Primiano Tucci wrote: > And when I wrote LG I actually meant ...
1 week, 1 day ago (2017-04-20 19:31:41 UTC) #27
Primiano Tucci
https://codereview.chromium.org/2819413002/diff/1/base/trace_event/memory_dump_manager.h File base/trace_event/memory_dump_manager.h (right): https://codereview.chromium.org/2819413002/diff/1/base/trace_event/memory_dump_manager.h#newcode69 base/trace_event/memory_dump_manager.h:69: using RequestGlobalDumpCallback = On 2017/04/20 01:18:21, ssid wrote: > ...
1 week, 1 day ago (2017-04-20 20:30:05 UTC) #32
ssid
Sorry, I hadn't checked the build before sending out CL. I had to add EXPORT ...
1 week, 1 day ago (2017-04-20 23:49:57 UTC) #50
Primiano Tucci
+rockot can you please take a look to the diff between PS2 and PS3, where ...
1 week ago (2017-04-21 17:11:51 UTC) #54
Ken Rockot
On 2017/04/21 at 17:11:51, primiano wrote: > +rockot can you please take a look to ...
1 week ago (2017-04-21 17:32:52 UTC) #55
Primiano Tucci
> If you make your client library (i.e. public/cpp) a component -- which is uncommon ...
1 week ago (2017-04-21 18:53:41 UTC) #56
Primiano Tucci
On 2017/04/21 18:53:41, Primiano Tucci wrote: > > If you make your client library (i.e. ...
1 week ago (2017-04-21 18:56:12 UTC) #57
Ken Rockot
Ah I see re: the static global. Didn't realize it had one. On Fri, Apr ...
1 week ago (2017-04-21 19:04:42 UTC) #58
Primiano Tucci
On 2017/04/21 19:04:42, Ken Rockot wrote: > Ah I see re: the static global. Didn't ...
5 days, 1 hour ago (2017-04-24 13:58:06 UTC) #59
ssid
Ken, ptal. I think I have made changes as you suggested and the bots that ...
4 days, 12 hours ago (2017-04-25 03:23:56 UTC) #68
chiniforooshan
On 2017/04/25 03:23:56, ssid wrote: > Ken, ptal. I think I have made changes as ...
4 days ago (2017-04-25 14:40:09 UTC) #72
chiniforooshan
On 2017/04/25 14:40:09, chiniforooshan wrote: > On 2017/04/25 03:23:56, ssid wrote: > > Ken, ptal. ...
4 days ago (2017-04-25 14:44:25 UTC) #73
Primiano Tucci
On 2017/04/25 14:44:25, chiniforooshan wrote: > On 2017/04/25 14:40:09, chiniforooshan wrote: > > On 2017/04/25 ...
4 days ago (2017-04-25 15:01:36 UTC) #74
Ken Rockot
On Tue, Apr 25, 2017 at 8:01 AM, <primiano@chromium.org> wrote: > On 2017/04/25 14:44:25, chiniforooshan ...
4 days ago (2017-04-25 15:13:35 UTC) #75
chiniforooshan
On 2017/04/25 15:01:36, Primiano Tucci wrote: > On 2017/04/25 14:44:25, chiniforooshan wrote: > > On ...
4 days ago (2017-04-25 15:22:38 UTC) #76
ssid
So, I agree with priminao about not having to worry about the statics, singleton issues ...
3 days, 21 hours ago (2017-04-25 18:28:08 UTC) #77
Ken Rockot
OK - LGTM, thanks for the discussion. I'm going to attempt to move forward with ...
3 days, 21 hours ago (2017-04-25 18:30:42 UTC) #78
Primiano Tucci
> If we're going to require every client library to be a component out of ...
3 days, 20 hours ago (2017-04-25 18:33:46 UTC) #79
Primiano Tucci
> We use source_set() in chromium because they make sense most of the times and ...
3 days, 20 hours ago (2017-04-25 18:37:31 UTC) #80
ssid
+rsesek for the memory_instrumentation_struct_traits.h. Changes the mojo structs to include export macros.
3 days, 20 hours ago (2017-04-25 18:48:55 UTC) #82
Robert Sesek
struct_traits lgtm
3 days, 20 hours ago (2017-04-25 19:02:20 UTC) #83
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2819413002/200001
3 days, 20 hours ago (2017-04-25 19:03:49 UTC) #86
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/419960)
3 days, 20 hours ago (2017-04-25 19:13:49 UTC) #88
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2819413002/240001
3 days, 14 hours ago (2017-04-26 01:15:49 UTC) #104
commit-bot: I haz the power
3 days, 14 hours ago (2017-04-26 01:25:40 UTC) #107
Message was sent while issue was closed.
Committed patchset #6 (id:240001) as
https://chromium.googlesource.com/chromium/src/+/f91ad00d0d78fe00a5d1ca4fc50a...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld cc6ac46