Refactoring: Move functions from WebMemoryDumpProviderAdapter to PartitionAllocMemoryDumpProvider

Refactoring: Move functions from WebMemoryDumpProviderAdapter to PartitionAllocMemoryDumpProvider

Before this CL, allocation hook registerings for heap profiling existed
at child/content/WebMemoryDumpProviderAdapter as global functions for
only one allocater (PartitionAlloc). It is because platform directory
couldn't depend on base/trace_event and those logic needed to be in
Chromium side.

Now platform directory can refer base/trace_events, we can move those
functions from Chromium side to Blink side so that we can easily add
heap profiling for other allocaters like Oilpan.

Next I'm going to remove WebMemoryDumpProviderAdapter which was needed
when platform couldn't depend on base/trace_event.

BUG=548254
TEST=n/a

Committed: https://crrev.com/7e94fbc7dd28ce78aa12234b93bc52052f243448
Cr-Commit-Position: refs/heads/master@{#373204}

[hajimehoshi, 2016-01-29 08:42:26]

The CQ bit was checked by hajimehoshi@chromium.org

[hajimehoshi, 2016-01-29 08:47:47]

hajimehoshi@chromium.org changed reviewers:
+ kinuko@chromium.org, tkent@chromium.org

[hajimehoshi, 2016-01-29 08:47:48]

PTAL

+kinuko content/child
+tkent third_party/Webkit/public, third_party/Webkit/Source/platform

[tkent, 2016-01-29 08:57:29]

tkent@chromium.org changed reviewers:
+ esprehn@chromium.org

[tkent, 2016-01-29 08:57:29]

+esprehn

https://codereview.chromium.org/1642023007/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebProcessMemoryDump.h (right):

https://codereview.chromium.org/1642023007/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebProcessMemoryDump.h:15: namespace base {
Was the dependency from public/platofrm/ to base/ allowed?

[esprehn, 2016-01-29 09:29:51]

Yes public/platform can depend on base.

[tkent, 2016-01-29 11:02:39]

On 2016/01/29 at 09:29:51, esprehn wrote:
> Yes public/platform can depend on base.

Thanks.  LGTM

[haraken, 2016-01-29 14:06:52]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org

[haraken, 2016-01-29 14:06:53]

LGTM

https://codereview.chromium.org/1642023007/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/PartitionAllocMemoryDumpProvider.h
(right):

https://codereview.chromium.org/1642023007/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/PartitionAllocMemoryDumpProvider.h:13:
#include "wtf/ThreadingPrimitives.h"

This wouldn't be needed.

https://codereview.chromium.org/1642023007/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebProcessMemoryDump.h (right):

https://codereview.chromium.org/1642023007/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebProcessMemoryDump.h:115: //
TODO(hajimehoshi): This is a temporal hack to use ProcessMemoryDump

temporary

[hajimehoshi, 2016-02-01 06:19:35]

Thank you!

https://codereview.chromium.org/1642023007/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/PartitionAllocMemoryDumpProvider.h
(right):

https://codereview.chromium.org/1642023007/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/PartitionAllocMemoryDumpProvider.h:13:
#include "wtf/ThreadingPrimitives.h"
On 2016/01/29 14:06:52, haraken wrote:
> 
> This wouldn't be needed.

This is needed for WTF::Mutex.

https://codereview.chromium.org/1642023007/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebProcessMemoryDump.h (right):

https://codereview.chromium.org/1642023007/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebProcessMemoryDump.h:115: //
TODO(hajimehoshi): This is a temporal hack to use ProcessMemoryDump
On 2016/01/29 14:06:52, haraken wrote:
> 
> temporary

Done.

[Primiano Tucci (use gerrit), 2016-02-01 13:49:59]

primiano@chromium.org changed reviewers:
+ primiano@chromium.org

[Primiano Tucci (use gerrit), 2016-02-01 13:50:00]

https://codereview.chromium.org/1642023007/diff/40001/content/child/web_proce...
File content/child/web_process_memory_dump_impl.h (right):

https://codereview.chromium.org/1642023007/diff/40001/content/child/web_proce...
content/child/web_process_memory_dump_impl.h:78:
base::trace_event::ProcessMemoryDump* getProcessMemoryDump() override;
The only weird thing here is that with this CL you are now in a state where most
of the code that uses memory-infra use the wrapped WebProcessMemoryDump, but now
you are cutting a hole through the layers here to get directly to the
base::trace_event::PMD.

It seems that here you want just the SessionState, so why not just adding a
method like:

base::trace_event__MemoryDumpSessionState* getMemoryDumpSessionState()
here ?

https://codereview.chromium.org/1642023007/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/PartitionAllocMemoryDumpProvider.h
(right):

https://codereview.chromium.org/1642023007/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/PartitionAllocMemoryDumpProvider.h:46: bool
m_isHeapProfilingEnabled;
This field seemd to be written but never read. Maybe you don't need this after
all?

[kinuko, 2016-02-01 14:41:29]

content/ lgtm (for Primiano's comment I'd defer the decision to him/you)

[hajimehoshi, 2016-02-02 08:44:44]

Thank you! PTAL

esprehn: This change adds '+base/trace_event' to public/platform/DEPS to include
base/trace_event header files in public/platform files. Is this OK?

https://codereview.chromium.org/1642023007/diff/40001/content/child/web_proce...
File content/child/web_process_memory_dump_impl.h (right):

https://codereview.chromium.org/1642023007/diff/40001/content/child/web_proce...
content/child/web_process_memory_dump_impl.h:78:
base::trace_event::ProcessMemoryDump* getProcessMemoryDump() override;
On 2016/02/01 13:50:00, Primiano Tucci wrote:
> The only weird thing here is that with this CL you are now in a state where
most
> of the code that uses memory-infra use the wrapped WebProcessMemoryDump, but
now
> you are cutting a hole through the layers here to get directly to the
> base::trace_event::PMD.
> 
> It seems that here you want just the SessionState, so why not just adding a
> method like:
> 
> base::trace_event__MemoryDumpSessionState* getMemoryDumpSessionState()
> here ?

Makes sense. Unfortunately, not only the session state but also calling DumpInto
are required here. I crated WebProcessMemoryDump::dumpPartitionAllocHeapUsage
instead of getProcessMemoryDump, and moved the logic using ProcessMemoryDump* to
the new function. I think we would need more refactoring since the new method is
used only for Parition Alloc.

https://codereview.chromium.org/1642023007/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/PartitionAllocMemoryDumpProvider.h
(right):

https://codereview.chromium.org/1642023007/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/PartitionAllocMemoryDumpProvider.h:46: bool
m_isHeapProfilingEnabled;
On 2016/02/01 13:50:00, Primiano Tucci wrote:
> This field seemd to be written but never read. Maybe you don't need this after
> all?

This is used at onMemoryDump.

[esprehn, 2016-02-02 12:24:41]

lgtm

[Primiano Tucci (use gerrit), 2016-02-02 12:30:00]

LGTM from my side, looks better now.
But you could probably make the function non-partition-alloc specific by just
passing a const char* :)

https://codereview.chromium.org/1642023007/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/PartitionAllocMemoryDumpProvider.h
(right):

https://codereview.chromium.org/1642023007/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/PartitionAllocMemoryDumpProvider.h:46: bool
m_isHeapProfilingEnabled;
On 2016/02/02 08:44:44, hajimehoshi wrote:
> On 2016/02/01 13:50:00, Primiano Tucci wrote:
> > This field seemd to be written but never read. Maybe you don't need this
after
> > all?
> 
> This is used at onMemoryDump.

Ohh just realized that if a string wraps in codereview, search is not able to
find it, as it is split into two nodes :-/

https://codereview.chromium.org/1642023007/diff/60001/content/child/web_proce...
File content/child/web_process_memory_dump_impl.cc (right):

https://codereview.chromium.org/1642023007/diff/60001/content/child/web_proce...
content/child/web_process_memory_dump_impl.cc:178:
process_memory_dump_->AddHeapDump("partition_alloc", heap_dump);
why don't you pass this string as an argument and make this function generic,
i.e. dumpHeapUsage(...)

[esprehn, 2016-02-02 12:35:29]

On 2016/02/02 at 12:30:00, primiano wrote:
> ...
>
https://codereview.chromium.org/1642023007/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/platform/PartitionAllocMemoryDumpProvider.h:46: bool
m_isHeapProfilingEnabled;
> On 2016/02/02 08:44:44, hajimehoshi wrote:
> > On 2016/02/01 13:50:00, Primiano Tucci wrote:
> > > This field seemd to be written but never read. Maybe you don't need this
after
> > > all?
> > 
> > This is used at onMemoryDump.
> 
> Ohh just realized that if a string wraps in codereview, search is not able to
find it, as it is split into two nodes :-/
> 

If you use the non-deprecated UI (change it in codereview settings) it doesn't
do that. :)

[Primiano Tucci (use gerrit), 2016-02-02 14:38:23]

On 2016/02/02 12:35:29, esprehn wrote:
> If you use the non-deprecated UI (change it in codereview settings) it doesn't
> do that. :)

The non-deprecated UI has deprecated features I cannot live without (mainly
crbug.com/537557 and crbug.com/537545).
I'll train my brain to get used to that UI once it will have inter-patchset
diffs.

[hajimehoshi, 2016-02-03 08:20:26]

Thank you!

https://codereview.chromium.org/1642023007/diff/60001/content/child/web_proce...
File content/child/web_process_memory_dump_impl.cc (right):

https://codereview.chromium.org/1642023007/diff/60001/content/child/web_proce...
content/child/web_process_memory_dump_impl.cc:178:
process_memory_dump_->AddHeapDump("partition_alloc", heap_dump);
On 2016/02/02 12:29:59, Primiano Tucci wrote:
> why don't you pass this string as an argument and make this function generic,
> i.e. dumpHeapUsage(...)

Done.

[hajimehoshi, 2016-02-03 08:20:31]

The CQ bit was checked by hajimehoshi@chromium.org

[hajimehoshi, 2016-02-03 08:20:32]

The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org,
haraken@chromium.org, kinuko@chromium.org, esprehn@chromium.org,
primiano@chromium.org
Link to the patchset: https://codereview.chromium.org/1642023007/#ps80001
(title: "primiano's review")

[commit-bot: I haz the power, 2016-02-03 08:20:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1642023007/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1642023007/80001

[commit-bot: I haz the power, 2016-02-03 09:52:16]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-02-03 09:53:16]

Description was changed from

==========
Refactoring: Move functions from WebMemoryDumpProviderAdapter to
PartitionAllocMemoryDumpProvider

Before this CL, allocation hook registerings for heap profiling existed
at child/content/WebMemoryDumpProviderAdapter as global functions for
only one allocater (PartitionAlloc). It is because platform directory
couldn't depend on base/trace_event and those logic needed to be in
Chromium side.

Now platform directory can refer base/trace_events, we can move those
functions from Chromium side to Blink side so that we can easily add
heap profiling for other allocaters like Oilpan.

Next I'm going to remove WebMemoryDumpProviderAdapter which was needed
when platform couldn't depend on base/trace_event.

BUG=548254
TEST=n/a
==========

to

==========
Refactoring: Move functions from WebMemoryDumpProviderAdapter to
PartitionAllocMemoryDumpProvider

Before this CL, allocation hook registerings for heap profiling existed
at child/content/WebMemoryDumpProviderAdapter as global functions for
only one allocater (PartitionAlloc). It is because platform directory
couldn't depend on base/trace_event and those logic needed to be in
Chromium side.

Now platform directory can refer base/trace_events, we can move those
functions from Chromium side to Blink side so that we can easily add
heap profiling for other allocaters like Oilpan.

Next I'm going to remove WebMemoryDumpProviderAdapter which was needed
when platform couldn't depend on base/trace_event.

BUG=548254
TEST=n/a

Committed: https://crrev.com/7e94fbc7dd28ce78aa12234b93bc52052f243448
Cr-Commit-Position: refs/heads/master@{#373204}
==========

[commit-bot: I haz the power, 2016-02-03 09:53:17]

Patchset 5 (id:??) landed as
https://crrev.com/7e94fbc7dd28ce78aa12234b93bc52052f243448
Cr-Commit-Position: refs/heads/master@{#373204}