Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(104)

Issue 2545173002: [tracing] Remove MemoryAllocatorDump::process_memory_dump (Closed)

Created:
4 years ago by ssid
Modified:
4 years ago
CC:
chromium-reviews, tracing+reviews_chromium.org, wfh+watch_chromium.org, sync-reviews_chromium.org, vmpstr+watch_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

[tracing] Remove MemoryAllocatorDump::process_memory_dump This function should not be exist as it is casusing misleading use of this function. The MAD is passed around to objects to dump memory instead of PMD being passed around like the case of Sync. The ReportMemoryUsage method should not be changing the PMD object when it is passed only MAD. BUG= Committed: https://crrev.com/1f4e53699f8b83e355bbb9b559cf334fce222635 Cr-Commit-Position: refs/heads/master@{#437328}

Patch Set 1 #

Total comments: 2

Patch Set 2 : Fix include. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+28 lines, -24 lines) Patch
M base/trace_event/memory_allocator_dump.h View 1 chunk +0 lines, -5 lines 0 comments Download
M components/sync/syncable/directory.cc View 1 chunk +1 line, -2 lines 0 comments Download
M components/sync/syncable/directory_backing_store.h View 2 chunks +3 lines, -2 lines 0 comments Download
M components/sync/syncable/directory_backing_store.cc View 1 chunk +3 lines, -2 lines 0 comments Download
M sql/connection.h View 2 chunks +4 lines, -3 lines 0 comments Download
M sql/connection.cc View 1 chunk +3 lines, -3 lines 0 comments Download
M sql/connection_memory_dump_provider.h View 1 2 chunks +9 lines, -3 lines 0 comments Download
M sql/connection_memory_dump_provider.cc View 1 chunk +5 lines, -4 lines 0 comments Download

Messages

Total messages: 19 (10 generated)
ssid
ptal, thanks
4 years ago (2016-12-02 20:12:40 UTC) #2
Primiano Tucci (use gerrit)
Ahh makes sense. LGTM. thanks for fixing this and reducing the API surface.
4 years ago (2016-12-05 15:44:42 UTC) #7
ssid
+Pavel, Scott Ptal, small cl, thanks
4 years ago (2016-12-05 18:09:26 UTC) #9
pavely
lgtm https://codereview.chromium.org/2545173002/diff/1/sql/connection_memory_dump_provider.h File sql/connection_memory_dump_provider.h (right): https://codereview.chromium.org/2545173002/diff/1/sql/connection_memory_dump_provider.h#newcode12 sql/connection_memory_dump_provider.h:12: #include "base/trace_event/memory_allocator_dump.h" Include process_memory_dump.h instead of memory_allocator_dump.h
4 years ago (2016-12-05 18:55:55 UTC) #10
Scott Hess - ex-Googler
lgtm
4 years ago (2016-12-05 19:36:50 UTC) #11
ssid
Thanks, fixed. https://codereview.chromium.org/2545173002/diff/1/sql/connection_memory_dump_provider.h File sql/connection_memory_dump_provider.h (right): https://codereview.chromium.org/2545173002/diff/1/sql/connection_memory_dump_provider.h#newcode12 sql/connection_memory_dump_provider.h:12: #include "base/trace_event/memory_allocator_dump.h" On 2016/12/05 18:55:54, pavely wrote: ...
4 years ago (2016-12-08 18:14:12 UTC) #12
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/2545173002/20001
4 years ago (2016-12-08 18:14:58 UTC) #15
commit-bot: I haz the power
Committed patchset #2 (id:20001)
4 years ago (2016-12-08 20:42:14 UTC) #17
commit-bot: I haz the power
4 years ago (2016-12-08 20:44:59 UTC) #19
Message was sent while issue was closed.
Patchset 2 (id:??) landed as
https://crrev.com/1f4e53699f8b83e355bbb9b559cf334fce222635
Cr-Commit-Position: refs/heads/master@{#437328}

Powered by Google App Engine
This is Rietveld 408576698