Refactoring: Move some classes from content/child to platform

Refactoring: Move some classes from content/child to platform

This CL moves these classes:
- WebMemoryAllocatorDumpImpl
- WebMemoryDumpProviderAdapter
- WebProcessMemoryDumpImpl(Test)
from content/child to platform. Now that platform can depend on base,
we don't need adapter classes in Chromium side.

After this CL, we will be able to reduce glue classes by unifying:
- WebMemoryAllocatorDump and WebMemoryAllocatorDumpImpl
- WebProcessMemoryDump and WebProcessMemoryDumpImpl
- WebMemoryDumpProvider and WebMemoryDumpProviderAdapter
so that we can reduce lots of code.

BUG=548254
Committed: https://crrev.com/a8227af5f6016e10e78b539ffa55aeb634ca7601
Cr-Commit-Position: refs/heads/master@{#377552}

[hajimehoshi, 2016-02-03 12:39:50]

Description was changed from

==========
WIP: Refactoring: Move some classes form content/child to platform

NOTE: The coding style is kept, and I needed --bypass-hook to upload
this CL to avoid coding style check.

BUG=548254
==========

to

==========
WIP: Refactoring: Move some classes form content/child to platform

NOTE: The coding style is kept, and I needed --bypass-hook to upload
this CL to avoid coding style check.

BUG=548254
==========

[hajimehoshi, 2016-02-03 12:39:50]

hajimehoshi@chromium.org changed reviewers:
+ bashi@chromium.org, haraken@chromium.org

[hajimehoshi, 2016-02-03 12:46:06]

Description was changed from

==========
WIP: Refactoring: Move some classes form content/child to platform

NOTE: The coding style is kept, and I needed --bypass-hook to upload
this CL to avoid coding style check.

BUG=548254
==========

to

==========
WIP: Refactoring: Move some classes form content/child to platform

This CL moves these classes:
- WebMemoryAllocatorDumpImpl
- WebMemoryDumpProviderAdapter
- WebProcessMemoryDumpImpl
from content/child to platform. Now that platform can depend on base,
we don't need adapter classes in content.

After this CL, we still might need to unify FooImpl and Foo classes
into Foo class.

NOTE: The coding style is kept, and I needed --bypass-hook to upload
this CL to avoid coding style check.

BUG=548254
==========

[hajimehoshi, 2016-02-03 12:46:26]

Description was changed from

==========
WIP: Refactoring: Move some classes form content/child to platform

This CL moves these classes:
- WebMemoryAllocatorDumpImpl
- WebMemoryDumpProviderAdapter
- WebProcessMemoryDumpImpl
from content/child to platform. Now that platform can depend on base,
we don't need adapter classes in content.

After this CL, we still might need to unify FooImpl and Foo classes
into Foo class.

NOTE: The coding style is kept, and I needed --bypass-hook to upload
this CL to avoid coding style check.

BUG=548254
==========

to

==========
WIP: Refactoring: Move some classes form content/child to platform

This CL moves these classes:
- WebMemoryAllocatorDumpImpl
- WebMemoryDumpProviderAdapter
- WebProcessMemoryDumpImpl
from content/child to platform. Now that platform can depend on base,
we don't need adapter classes in content.

After this CL, we might need to unify FooImpl and Foo classes into Foo
class.

NOTE: The coding style is kept, and I needed --bypass-hook to upload
this CL to avoid coding style check.

BUG=548254
==========

[hajimehoshi, 2016-02-03 12:46:44]

Description was changed from

==========
WIP: Refactoring: Move some classes form content/child to platform

This CL moves these classes:
- WebMemoryAllocatorDumpImpl
- WebMemoryDumpProviderAdapter
- WebProcessMemoryDumpImpl
from content/child to platform. Now that platform can depend on base,
we don't need adapter classes in content.

After this CL, we might need to unify FooImpl and Foo classes into Foo
class.

NOTE: The coding style is kept, and I needed --bypass-hook to upload
this CL to avoid coding style check.

BUG=548254
==========

to

==========
WIP: Refactoring: Move some classes form content/child to platform

This CL moves these classes:
- WebMemoryAllocatorDumpImpl
- WebMemoryDumpProviderAdapter
- WebProcessMemoryDumpImpl
from content/child to platform. Now that platform can depend on base,
we don't need adapter classes in Chromium side.

After this CL, we might need to unify FooImpl and Foo classes into Foo
class.

NOTE: The coding style is kept, and I needed --bypass-hook to upload
this CL to avoid coding style check.

BUG=548254
==========

[hajimehoshi, 2016-02-04 09:11:41]

Description was changed from

==========
WIP: Refactoring: Move some classes form content/child to platform

This CL moves these classes:
- WebMemoryAllocatorDumpImpl
- WebMemoryDumpProviderAdapter
- WebProcessMemoryDumpImpl
from content/child to platform. Now that platform can depend on base,
we don't need adapter classes in Chromium side.

After this CL, we might need to unify FooImpl and Foo classes into Foo
class.

NOTE: The coding style is kept, and I needed --bypass-hook to upload
this CL to avoid coding style check.

BUG=548254
==========

to

==========
WIP: Refactoring: Move some classes form content/child to platform

This CL moves these classes:
- WebMemoryAllocatorDumpImpl
- WebMemoryDumpProviderAdapter
- WebProcessMemoryDumpImpl
from content/child to platform. Now that platform can depend on base,
we don't need adapter classes in Chromium side.

After this CL, we might need to unify FooImpl and Foo classes into Foo
class since FooImpl classes are just glue and no longer needed.

NOTE: The coding style is kept, and I needed --bypass-hook to upload
this CL to avoid coding style check.

BUG=548254
==========

[hajimehoshi, 2016-02-04 09:11:54]

Description was changed from

==========
WIP: Refactoring: Move some classes form content/child to platform

This CL moves these classes:
- WebMemoryAllocatorDumpImpl
- WebMemoryDumpProviderAdapter
- WebProcessMemoryDumpImpl
from content/child to platform. Now that platform can depend on base,
we don't need adapter classes in Chromium side.

After this CL, we might need to unify FooImpl and Foo classes into Foo
class since FooImpl classes are just glue and no longer needed.

NOTE: The coding style is kept, and I needed --bypass-hook to upload
this CL to avoid coding style check.

BUG=548254
==========

to

==========
WIP: Refactoring: Move some classes form content/child to platform

This CL moves these classes:
- WebMemoryAllocatorDumpImpl
- WebMemoryDumpProviderAdapter
- WebProcessMemoryDumpImpl
from content/child to platform. Now that platform can depend on base,
we don't need adapter classes in Chromium side.

After this CL, we might need to unify FooImpl and Foo classes into Foo
class since FooImpl classes are just glue and no longer needed.

BUG=548254
==========

[hajimehoshi, 2016-02-04 09:17:47]

hajimehoshi@chromium.org changed reviewers:
+ thakis@chromium.org

[hajimehoshi, 2016-02-04 09:18:50]

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

[hajimehoshi, 2016-02-04 09:19:44]

Description was changed from

==========
WIP: Refactoring: Move some classes form content/child to platform

This CL moves these classes:
- WebMemoryAllocatorDumpImpl
- WebMemoryDumpProviderAdapter
- WebProcessMemoryDumpImpl
from content/child to platform. Now that platform can depend on base,
we don't need adapter classes in Chromium side.

After this CL, we might need to unify FooImpl and Foo classes into Foo
class since FooImpl classes are just glue and no longer needed.

BUG=548254
==========

to

==========
Refactoring: Move some classes form content/child to platform

This CL moves these classes:
- WebMemoryAllocatorDumpImpl
- WebMemoryDumpProviderAdapter
- WebProcessMemoryDumpImpl
from content/child to platform. Now that platform can depend on base,
we don't need adapter classes in Chromium side.

After this CL, we might need to unify FooImpl and Foo classes into Foo
class since FooImpl classes are just glue and no longer needed.

BUG=548254
==========

[hajimehoshi, 2016-02-04 09:20:00]

Description was changed from

==========
Refactoring: Move some classes form content/child to platform

This CL moves these classes:
- WebMemoryAllocatorDumpImpl
- WebMemoryDumpProviderAdapter
- WebProcessMemoryDumpImpl
from content/child to platform. Now that platform can depend on base,
we don't need adapter classes in Chromium side.

After this CL, we might need to unify FooImpl and Foo classes into Foo
class since FooImpl classes are just glue and no longer needed.

BUG=548254
==========

to

==========
Refactoring: Move some classes form content/child to platform

This CL moves these classes:
- WebMemoryAllocatorDumpImpl
- WebMemoryDumpProviderAdapter
- WebProcessMemoryDumpImpl(Test)
from content/child to platform. Now that platform can depend on base,
we don't need adapter classes in Chromium side.

After this CL, we might need to unify FooImpl and Foo classes into Foo
class since FooImpl classes are just glue and no longer needed.

BUG=548254
==========

[hajimehoshi, 2016-02-04 09:26:03]

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

[hajimehoshi, 2016-02-04 09:26:04]

PTAL

+kinuko content/child
+thakis PRESUBMIT.py and DEPS
+tkent third_party/WebKit/public
+haraken third_party/WebKit/Source/platform

Now new files' style are not checked in the presubmit hook since 1. these files
are temporary and should be removed IMO and 2. include-guard convention wouldn't
match Chromium style.

[hajimehoshi, 2016-02-04 09:26:56]

CC +primiano

[tkent, 2016-02-04 09:39:24]

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

[tkent, 2016-02-04 09:42:05]

This CL adds some Blink->base dependencies.  esprehn@ should review this.

[kinuko, 2016-02-04 11:25:34]

Ok so this one's more like simple file moving from content -> blink.  I have
some questions inline

https://codereview.chromium.org/1660383002/diff/40001/third_party/WebKit/PRES...
File third_party/WebKit/PRESUBMIT.py (right):

https://codereview.chromium.org/1660383002/diff/40001/third_party/WebKit/PRES...
third_party/WebKit/PRESUBMIT.py:119: # and no longer needed.
If we're going to use this list for more files that are moved from chromium to
blink (until the blink-side style changes) should the TODO comment be added
right before the file names below?

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

https://codereview.chromium.org/1660383002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/WebProcessMemoryDumpImpl.h:12: #include
"base/memory/scoped_vector.h"
Hmm... I haven't realized but are we just going to use scoped_ptr etc as is in
platform/ (as well as wtf containers)?

https://codereview.chromium.org/1660383002/diff/40001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/Platform.h (right):

https://codereview.chromium.org/1660383002/diff/40001/third_party/WebKit/publ...
third_party/WebKit/public/platform/Platform.h:461: BLINK_PLATFORM_EXPORT void
registerMemoryDumpProvider(blink::WebMemoryDumpProvider*, const char* name);
Does this change mean that they're also called by code outside blink?

[haraken, 2016-02-04 11:30:56]

https://codereview.chromium.org/1660383002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/DEPS (right):

https://codereview.chromium.org/1660383002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/DEPS:4:
"+base/containers/scoped_ptr_hash_map.h",

I don't think we want to allow this.

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

https://codereview.chromium.org/1660383002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/WebProcessMemoryDumpImpl.h:12: #include
"base/memory/scoped_vector.h"
On 2016/02/04 11:25:34, kinuko wrote:
> Hmm... I haven't realized but are we just going to use scoped_ptr etc as is in
> platform/ (as well as wtf containers)?

We don't want to allow scoped_ptr.

By the way, what doesn't this cause a compile error? scoped_ptr.h is not written
in platform/DEPS so it should cause a compile error.

[hajimehoshi, 2016-02-04 12:17:52]

Thank you!

https://codereview.chromium.org/1660383002/diff/40001/third_party/WebKit/PRES...
File third_party/WebKit/PRESUBMIT.py (right):

https://codereview.chromium.org/1660383002/diff/40001/third_party/WebKit/PRES...
third_party/WebKit/PRESUBMIT.py:119: # and no longer needed.
On 2016/02/04 11:25:34, kinuko wrote:
> If we're going to use this list for more files that are moved from chromium to
> blink (until the blink-side style changes) should the TODO comment be added
> right before the file names below?

Done.

https://codereview.chromium.org/1660383002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/DEPS (right):

https://codereview.chromium.org/1660383002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/DEPS:4:
"+base/containers/scoped_ptr_hash_map.h",
On 2016/02/04 11:30:56, haraken wrote:
> 
> I don't think we want to allow this.

Done.

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

https://codereview.chromium.org/1660383002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/WebProcessMemoryDumpImpl.h:12: #include
"base/memory/scoped_vector.h"
On 2016/02/04 11:30:56, haraken wrote:
> On 2016/02/04 11:25:34, kinuko wrote:
> > Hmm... I haven't realized but are we just going to use scoped_ptr etc as is
in
> > platform/ (as well as wtf containers)?
> 
> We don't want to allow scoped_ptr.
> 
> By the way, what doesn't this cause a compile error? scoped_ptr.h is not
written
> in platform/DEPS so it should cause a compile error.
> 
> 

Done.

https://codereview.chromium.org/1660383002/diff/40001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/Platform.h (right):

https://codereview.chromium.org/1660383002/diff/40001/third_party/WebKit/publ...
third_party/WebKit/public/platform/Platform.h:461: BLINK_PLATFORM_EXPORT void
registerMemoryDumpProvider(blink::WebMemoryDumpProvider*, const char* name);
On 2016/02/04 11:25:34, kinuko wrote:
> Does this change mean that they're also called by code outside blink?

Compiling fails without this: this is called only from web/WebKit.cpp. Maybe the
implementation should be in another place?

[kinuko, 2016-02-04 13:15:54]

https://codereview.chromium.org/1660383002/diff/80001/third_party/WebKit/PRES...
File third_party/WebKit/PRESUBMIT.py (right):

https://codereview.chromium.org/1660383002/diff/80001/third_party/WebKit/PRES...
third_party/WebKit/PRESUBMIT.py:117: ignore = [
This 'ignore' variable itself should also have some comments, I think.

https://codereview.chromium.org/1660383002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/DEPS (right):

https://codereview.chromium.org/1660383002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/DEPS:8: "+base/memory",
If including base/memory/scoped_ptr doesn't make compile fail because of this
line could we explicitly blacklist banned files too?  Like:

# These files are explicitly banned as they conflict with wtf templates.
"-base/memory/scoped_ptr.h",
"-base/memory/scoped_vector.h",
...

https://codereview.chromium.org/1660383002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/DEPS:10: "+base/thread_task_runner_handle.h",
This one's slightly questionable to me too, while it's probably safe it feels a
bit confusing too...

https://codereview.chromium.org/1660383002/diff/80001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/Platform.h (right):

https://codereview.chromium.org/1660383002/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/platform/Platform.h:461: BLINK_PLATFORM_EXPORT void
registerMemoryDumpProvider(blink::WebMemoryDumpProvider*, const char* name);
If these are not called outside blink I don't think they should be in public/;
please move them from public/platform to Source/platform.

Also I don't think they need to be kept as Platform:: methods, they could be
just moved to a separate utility file in Source/platform, couldn't they?  (I
haven't looked at the callsite yet though)

[Nico, 2016-02-04 15:08:15]

DEPS and PRESUBMIT lgtm with kinuko's comments addressed (assuming esprehn and
kinuko are fine with the thread_task_runner_handle -- seems ok to me)

https://codereview.chromium.org/1660383002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/DEPS (right):

https://codereview.chromium.org/1660383002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/DEPS:8: "+base/memory",
On 2016/02/04 13:15:54, kinuko wrote:
> If including base/memory/scoped_ptr doesn't make compile fail because of this
> line could we explicitly blacklist banned files too?  Like:
> 
> # These files are explicitly banned as they conflict with wtf templates.
> "-base/memory/scoped_ptr.h",
> "-base/memory/scoped_vector.h",
> ...

(If the "could we" means "is it possible", then yes, we do this elsewhere:
https://code.google.com/p/chromium/codesearch#chromium/src/content/DEPS&l=21)

[hajimehoshi, 2016-02-05 07:41:55]

Thank you!

https://codereview.chromium.org/1660383002/diff/80001/third_party/WebKit/PRES...
File third_party/WebKit/PRESUBMIT.py (right):

https://codereview.chromium.org/1660383002/diff/80001/third_party/WebKit/PRES...
third_party/WebKit/PRESUBMIT.py:117: ignore = [
On 2016/02/04 13:15:54, kinuko wrote:
> This 'ignore' variable itself should also have some comments, I think.

Done.

https://codereview.chromium.org/1660383002/diff/80001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/Platform.h (right):

https://codereview.chromium.org/1660383002/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/platform/Platform.h:461: BLINK_PLATFORM_EXPORT void
registerMemoryDumpProvider(blink::WebMemoryDumpProvider*, const char* name);
On 2016/02/04 13:15:54, kinuko wrote:
> If these are not called outside blink I don't think they should be in public/;
> please move them from public/platform to Source/platform.
> 
> Also I don't think they need to be kept as Platform:: methods, they could be
> just moved to a separate utility file in Source/platform, couldn't they?  (I
> haven't looked at the callsite yet though)

I've just now found that TestBlinkWebUnitTestSupport, which is another class
inheriting Platform, must have empty (un)registerMemoryDumpProvider
implementation . This means The actual implementation BlinkPlatformImpl in
Chromium side should call this logic in Blink. I'll change their actual
implementation as static functions and leave BLINK_PLATFORM_EXPORT for the new
functions. Sorry for the confusion.

https://codereview.chromium.org/1660383002/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/platform/Platform.h:461: BLINK_PLATFORM_EXPORT void
registerMemoryDumpProvider(blink::WebMemoryDumpProvider*, const char* name);
On 2016/02/04 13:15:54, kinuko wrote:
> If these are not called outside blink I don't think they should be in public/;
> please move them from public/platform to Source/platform.
> 
> Also I don't think they need to be kept as Platform:: methods, they could be
> just moved to a separate utility file in Source/platform, couldn't they?  (I
> haven't looked at the callsite yet though)

Done.

[haraken, 2016-02-05 09:01:35]

I want to have primiano@ take a look and confirm if this is doing a right thing
overall.

[Primiano Tucci (use gerrit), 2016-02-05 10:05:18]

On 2016/02/05 09:01:35, haraken wrote:
> I want to have primiano@ take a look and confirm if this is doing a right
thing
> overall.

The CL seems to just move classes around. I think I am missing some context /
failing to see the overall goal.
What is the benefit of moving the wrapper classes to platform?
If platform can depend on base/ why don't we just just the base classes?
From the description of the CL I cannot understand why moving these classes is
desireable. 
To be clear, I am not saying that it is NOT, it's just not clear to me why.
Is this a transition step required to then make the various MDP in blink use
directly base?
If not can you give some explanation?
If it can help, this is the design doc with the diagram which explains the
presence and the rationale of the original classes:

https://docs.google.com/document/d/1jP2Zib7csojdueWaC95J5eZN2p0wTUKQrabumXC3K...

[hajimehoshi, 2016-02-05 10:30:34]

On 2016/02/05 10:05:18, Primiano Tucci wrote:
> On 2016/02/05 09:01:35, haraken wrote:
> > I want to have primiano@ take a look and confirm if this is doing a right
> thing
> > overall.
> 
> The CL seems to just move classes around. I think I am missing some context /
> failing to see the overall goal.
> What is the benefit of moving the wrapper classes to platform?
> If platform can depend on base/ why don't we just just the base classes?
> From the description of the CL I cannot understand why moving these classes is
> desireable. 
> To be clear, I am not saying that it is NOT, it's just not clear to me why.
> Is this a transition step required to then make the various MDP in blink use
> directly base?

Actually, moving those classes is not must. We can implement Oilpan heap
profiler without this moving. Well, IIUC, what I need to do is just implement
OilpanMemoryDumpProvider inheriting WebMemoryDumpProvider.

However, in general, glue classes in content/child should be moved to public and
finally removed. In this CL, the adapter class or the impl classes are the glue
classes. This moving would affect the way to implement OilpanMemoryDumpProvider,
and I think we should do such refactoring when possible.

> If not can you give some explanation?
> If it can help, this is the design doc with the diagram which explains the
> presence and the rationale of the original classes:
> 
>
https://docs.google.com/document/d/1jP2Zib7csojdueWaC95J5eZN2p0wTUKQrabumXC3K...

[hajimehoshi, 2016-02-05 10:31:10]

On 2016/02/05 10:30:34, hajimehoshi wrote:
> On 2016/02/05 10:05:18, Primiano Tucci wrote:
> > On 2016/02/05 09:01:35, haraken wrote:
> > > I want to have primiano@ take a look and confirm if this is doing a right
> > thing
> > > overall.
> > 
> > The CL seems to just move classes around. I think I am missing some context
/
> > failing to see the overall goal.
> > What is the benefit of moving the wrapper classes to platform?
> > If platform can depend on base/ why don't we just just the base classes?
> > From the description of the CL I cannot understand why moving these classes
is
> > desireable. 
> > To be clear, I am not saying that it is NOT, it's just not clear to me why.
> > Is this a transition step required to then make the various MDP in blink use
> > directly base?
> 
> Actually, moving those classes is not must. We can implement Oilpan heap
> profiler without this moving. Well, IIUC, what I need to do is just implement
> OilpanMemoryDumpProvider inheriting WebMemoryDumpProvider.
> 
> However, in general, glue classes in content/child should be moved to public
and
> finally removed. In this CL, the adapter class or the impl classes are the
glue
> classes. This moving would affect the way to implement
OilpanMemoryDumpProvider,
> and I think we should do such refactoring when possible.

Sorry but s/public/platform/
> 
> > If not can you give some explanation?
> > If it can help, this is the design doc with the diagram which explains the
> > presence and the rationale of the original classes:
> > 
> >
>
https://docs.google.com/document/d/1jP2Zib7csojdueWaC95J5eZN2p0wTUKQrabumXC3K...

[hajimehoshi, 2016-02-05 11:38:38]

https://codereview.chromium.org/1660383002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/WebProcessMemoryDumpImplTest.cpp
(right):

https://codereview.chromium.org/1660383002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/WebProcessMemoryDumpImplTest.cpp:20:
scoped_refptr<base::trace_event::TracedValue> traced_value(
Ooooh, we can't use scoped_refptr in Blink, but what should we use instead?

[esprehn, 2016-02-10 01:18:48]

On 2016/02/05 at 11:38:38, hajimehoshi wrote:
>
https://codereview.chromium.org/1660383002/diff/100001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/platform/WebProcessMemoryDumpImplTest.cpp
(right):
> 
>
https://codereview.chromium.org/1660383002/diff/100001/third_party/WebKit/Sou...
> third_party/WebKit/Source/platform/WebProcessMemoryDumpImplTest.cpp:20:
scoped_refptr<base::trace_event::TracedValue> traced_value(
> Ooooh, we can't use scoped_refptr in Blink, but what should we use instead?

You can use scoped_refptr in platform for handling chromium classes.

[kinuko, 2016-02-10 01:33:22]

https://codereview.chromium.org/1660383002/diff/100001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/Platform.h (right):

https://codereview.chromium.org/1660383002/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/platform/Platform.h:141: BLINK_PLATFORM_EXPORT static
void unregisterMemoryDumpProviderImpl(blink::WebMemoryDumpProvider*);
We have number of methods that can be actually implemented in blink but need
different code for testing, I'm not sure if we want to add these impl methods
exposed here for all those methods.

Could we- or does it make sense to rather implement these as a default impl for
(un)registerMemoryDumpProvider and have empty impl in other subclasses like
TestBlinkWebUnitTestSupport when necessary?

BLINK_PLATFORM_EXPORT virtual void (un)registerMemoryDumpProvider(...);

[esprehn, 2016-02-12 00:29:46]

Description was changed from

==========
Refactoring: Move some classes form content/child to platform

This CL moves these classes:
- WebMemoryAllocatorDumpImpl
- WebMemoryDumpProviderAdapter
- WebProcessMemoryDumpImpl(Test)
from content/child to platform. Now that platform can depend on base,
we don't need adapter classes in Chromium side.

After this CL, we might need to unify FooImpl and Foo classes into Foo
class since FooImpl classes are just glue and no longer needed.

BUG=548254
==========

to

==========
Refactoring: Move some classes from content/child to platform

This CL moves these classes:
- WebMemoryAllocatorDumpImpl
- WebMemoryDumpProviderAdapter
- WebProcessMemoryDumpImpl(Test)
from content/child to platform. Now that platform can depend on base,
we don't need adapter classes in Chromium side.

After this CL, we might need to unify FooImpl and Foo classes into Foo
class since FooImpl classes are just glue and no longer needed.

BUG=548254
==========

[hajimehoshi, 2016-02-15 09:26:48]

On 2016/02/10 01:18:48, esprehn wrote:
> On 2016/02/05 at 11:38:38, hajimehoshi wrote:
> >
>
https://codereview.chromium.org/1660383002/diff/100001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/platform/WebProcessMemoryDumpImplTest.cpp
> (right):
> > 
> >
>
https://codereview.chromium.org/1660383002/diff/100001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/platform/WebProcessMemoryDumpImplTest.cpp:20:
> scoped_refptr<base::trace_event::TracedValue> traced_value(
> > Ooooh, we can't use scoped_refptr in Blink, but what should we use instead?
> 
> You can use scoped_refptr in platform for handling chromium classes.

Thanks. I left them as they are.

[hajimehoshi, 2016-02-15 09:27:08]

Thank you!

https://codereview.chromium.org/1660383002/diff/100001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/Platform.h (right):

https://codereview.chromium.org/1660383002/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/platform/Platform.h:141: BLINK_PLATFORM_EXPORT static
void unregisterMemoryDumpProviderImpl(blink::WebMemoryDumpProvider*);
On 2016/02/10 01:33:22, kinuko wrote:
> We have number of methods that can be actually implemented in blink but need
> different code for testing, I'm not sure if we want to add these impl methods
> exposed here for all those methods.
> 
> Could we- or does it make sense to rather implement these as a default impl
for
> (un)registerMemoryDumpProvider and have empty impl in other subclasses like
> TestBlinkWebUnitTestSupport when necessary?
> 
> BLINK_PLATFORM_EXPORT virtual void (un)registerMemoryDumpProvider(...);
> 

Done.

[haraken, 2016-02-15 09:40:50]

https://codereview.chromium.org/1660383002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/WebProcessMemoryDumpImpl.h (right):

https://codereview.chromium.org/1660383002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/WebProcessMemoryDumpImpl.h:90:
WTF::OwnPtr<base::trace_event::ProcessMemoryDump> owned_process_memory_dump_;

It looks strange to use WTF::OwnPtr for Chromium classes. We should keep using
scoped_ptr.

However, if you need to introduce a bunch of scoped_ptr and scoped_refptr to
Blink's platform (like this CL), it implies that you're doing something strange.
This CL is just moving glue classes from content/ to platform/ without removing
any code. As primiano@ pointed out, I don't see much advantage of doing the
migration.

[hajimehoshi, 2016-02-15 09:48:48]

On 2016/02/15 09:40:50, haraken wrote:
>
https://codereview.chromium.org/1660383002/diff/140001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/platform/WebProcessMemoryDumpImpl.h (right):
> 
>
https://codereview.chromium.org/1660383002/diff/140001/third_party/WebKit/Sou...
> third_party/WebKit/Source/platform/WebProcessMemoryDumpImpl.h:90:
> WTF::OwnPtr<base::trace_event::ProcessMemoryDump> owned_process_memory_dump_;
> 
> It looks strange to use WTF::OwnPtr for Chromium classes. We should keep using
> scoped_ptr.
> 
> However, if you need to introduce a bunch of scoped_ptr and scoped_refptr to
> Blink's platform (like this CL), it implies that you're doing something
strange.
> This CL is just moving glue classes from content/ to platform/ without
removing
> any code. As primiano@ pointed out, I don't see much advantage of doing the
> migration.

IIUC, we can reduce code size by unifying FooImpl and Foo classes after this CL.
As you said, this CL just moves classes but it is intentional. You mean I should
remove those classes in this CL at the same time?

[haraken, 2016-02-15 09:52:45]

On 2016/02/15 09:48:48, hajimehoshi wrote:
> On 2016/02/15 09:40:50, haraken wrote:
> >
>
https://codereview.chromium.org/1660383002/diff/140001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/platform/WebProcessMemoryDumpImpl.h (right):
> > 
> >
>
https://codereview.chromium.org/1660383002/diff/140001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/platform/WebProcessMemoryDumpImpl.h:90:
> > WTF::OwnPtr<base::trace_event::ProcessMemoryDump>
owned_process_memory_dump_;
> > 
> > It looks strange to use WTF::OwnPtr for Chromium classes. We should keep
using
> > scoped_ptr.
> > 
> > However, if you need to introduce a bunch of scoped_ptr and scoped_refptr to
> > Blink's platform (like this CL), it implies that you're doing something
> strange.
> > This CL is just moving glue classes from content/ to platform/ without
> removing
> > any code. As primiano@ pointed out, I don't see much advantage of doing the
> > migration.
> 
> IIUC, we can reduce code size by unifying FooImpl and Foo classes after this
CL.
> As you said, this CL just moves classes but it is intentional. You mean I
should
> remove those classes in this CL at the same time?

Sorry, I think I (and probably primiano@) am behind.

Would you elaborate on what classes are going to be migrated and then removed?
(what does FooImpl mean specifically?)

[hajimehoshi, 2016-02-15 09:59:01]

On 2016/02/15 09:52:45, haraken wrote:
> On 2016/02/15 09:48:48, hajimehoshi wrote:
> > On 2016/02/15 09:40:50, haraken wrote:
> > >
> >
>
https://codereview.chromium.org/1660383002/diff/140001/third_party/WebKit/Sou...
> > > File third_party/WebKit/Source/platform/WebProcessMemoryDumpImpl.h
(right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1660383002/diff/140001/third_party/WebKit/Sou...
> > > third_party/WebKit/Source/platform/WebProcessMemoryDumpImpl.h:90:
> > > WTF::OwnPtr<base::trace_event::ProcessMemoryDump>
> owned_process_memory_dump_;
> > > 
> > > It looks strange to use WTF::OwnPtr for Chromium classes. We should keep
> using
> > > scoped_ptr.
> > > 
> > > However, if you need to introduce a bunch of scoped_ptr and scoped_refptr
to
> > > Blink's platform (like this CL), it implies that you're doing something
> > strange.
> > > This CL is just moving glue classes from content/ to platform/ without
> > removing
> > > any code. As primiano@ pointed out, I don't see much advantage of doing
the
> > > migration.
> > 
> > IIUC, we can reduce code size by unifying FooImpl and Foo classes after this
> CL.
> > As you said, this CL just moves classes but it is intentional. You mean I
> should
> > remove those classes in this CL at the same time?
> 
> Sorry, I think I (and probably primiano@) am behind.
> 
> Would you elaborate on what classes are going to be migrated and then removed?
> (what does FooImpl mean specifically?)

- Unify WebMemoryAllocatorDump and WebMemoryAllocatorDumpImpl
- Unify WebProcessMemoryDumpImpl and WebProcessMemoryDumpImpl
- Unify WebMemoryDumpProvider and WebMemoryDumpProviderAdapter

Sorry but I had to rewrite the description to be more clearer.

[hajimehoshi, 2016-02-15 10:01:49]

Description was changed from

==========
Refactoring: Move some classes from content/child to platform

This CL moves these classes:
- WebMemoryAllocatorDumpImpl
- WebMemoryDumpProviderAdapter
- WebProcessMemoryDumpImpl(Test)
from content/child to platform. Now that platform can depend on base,
we don't need adapter classes in Chromium side.

After this CL, we might need to unify FooImpl and Foo classes into Foo
class since FooImpl classes are just glue and no longer needed.

BUG=548254
==========

to

==========
Refactoring: Move some classes from content/child to platform

This CL moves these classes:
- WebMemoryAllocatorDumpImpl
- WebMemoryDumpProviderAdapter
- WebProcessMemoryDumpImpl(Test)
from content/child to platform. Now that platform can depend on base,
we don't need adapter classes in Chromium side.

After this CL, we can reduce glue classes by unifying:

- WebMemoryAllocatorDump and WebMemoryAllocatorDumpImpl
- WebProcessMemoryDump and WebProcessMemoryDumpImpl
- WebMemoryDumpProvider and WebMemoryDumpProviderAdapter

so that we can reduce lots of code.

BUG=548254
==========

[hajimehoshi, 2016-02-15 10:02:05]

Description was changed from

==========
Refactoring: Move some classes from content/child to platform

This CL moves these classes:
- WebMemoryAllocatorDumpImpl
- WebMemoryDumpProviderAdapter
- WebProcessMemoryDumpImpl(Test)
from content/child to platform. Now that platform can depend on base,
we don't need adapter classes in Chromium side.

After this CL, we can reduce glue classes by unifying:

- WebMemoryAllocatorDump and WebMemoryAllocatorDumpImpl
- WebProcessMemoryDump and WebProcessMemoryDumpImpl
- WebMemoryDumpProvider and WebMemoryDumpProviderAdapter

so that we can reduce lots of code.

BUG=548254
==========

to

==========
Refactoring: Move some classes from content/child to platform

This CL moves these classes:
- WebMemoryAllocatorDumpImpl
- WebMemoryDumpProviderAdapter
- WebProcessMemoryDumpImpl(Test)
from content/child to platform. Now that platform can depend on base,
we don't need adapter classes in Chromium side.

After this CL, we will be able to reduce glue classes by unifying:

- WebMemoryAllocatorDump and WebMemoryAllocatorDumpImpl
- WebProcessMemoryDump and WebProcessMemoryDumpImpl
- WebMemoryDumpProvider and WebMemoryDumpProviderAdapter

so that we can reduce lots of code.

BUG=548254
==========

[hajimehoshi, 2016-02-15 10:03:36]

Description was changed from

==========
Refactoring: Move some classes from content/child to platform

This CL moves these classes:
- WebMemoryAllocatorDumpImpl
- WebMemoryDumpProviderAdapter
- WebProcessMemoryDumpImpl(Test)
from content/child to platform. Now that platform can depend on base,
we don't need adapter classes in Chromium side.

After this CL, we will be able to reduce glue classes by unifying:

- WebMemoryAllocatorDump and WebMemoryAllocatorDumpImpl
- WebProcessMemoryDump and WebProcessMemoryDumpImpl
- WebMemoryDumpProvider and WebMemoryDumpProviderAdapter

so that we can reduce lots of code.

BUG=548254
==========

to

==========
Refactoring: Move some classes from content/child to platform

This CL moves these classes:
- WebMemoryAllocatorDumpImpl
- WebMemoryDumpProviderAdapter
- WebProcessMemoryDumpImpl(Test)
from content/child to platform. Now that platform can depend on base,
we don't need adapter classes in Chromium side.

After this CL, we will be able to reduce glue classes by unifying:
- WebMemoryAllocatorDump and WebMemoryAllocatorDumpImpl
- WebProcessMemoryDump and WebProcessMemoryDumpImpl
- WebMemoryDumpProvider and WebMemoryDumpProviderAdapter
so that we can reduce lots of code.

BUG=548254
==========

[haraken, 2016-02-15 10:26:21]

On 2016/02/15 09:59:01, hajimehoshi wrote:
> On 2016/02/15 09:52:45, haraken wrote:
> > On 2016/02/15 09:48:48, hajimehoshi wrote:
> > > On 2016/02/15 09:40:50, haraken wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/1660383002/diff/140001/third_party/WebKit/Sou...
> > > > File third_party/WebKit/Source/platform/WebProcessMemoryDumpImpl.h
> (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1660383002/diff/140001/third_party/WebKit/Sou...
> > > > third_party/WebKit/Source/platform/WebProcessMemoryDumpImpl.h:90:
> > > > WTF::OwnPtr<base::trace_event::ProcessMemoryDump>
> > owned_process_memory_dump_;
> > > > 
> > > > It looks strange to use WTF::OwnPtr for Chromium classes. We should keep
> > using
> > > > scoped_ptr.
> > > > 
> > > > However, if you need to introduce a bunch of scoped_ptr and
scoped_refptr
> to
> > > > Blink's platform (like this CL), it implies that you're doing something
> > > strange.
> > > > This CL is just moving glue classes from content/ to platform/ without
> > > removing
> > > > any code. As primiano@ pointed out, I don't see much advantage of doing
> the
> > > > migration.
> > > 
> > > IIUC, we can reduce code size by unifying FooImpl and Foo classes after
this
> > CL.
> > > As you said, this CL just moves classes but it is intentional. You mean I
> > should
> > > remove those classes in this CL at the same time?
> > 
> > Sorry, I think I (and probably primiano@) am behind.
> > 
> > Would you elaborate on what classes are going to be migrated and then
removed?
> > (what does FooImpl mean specifically?)
> 
> - Unify WebMemoryAllocatorDump and WebMemoryAllocatorDumpImpl
> - Unify WebProcessMemoryDumpImpl and WebProcessMemoryDumpImpl
> - Unify WebMemoryDumpProvider and WebMemoryDumpProviderAdapter
> 
> Sorry but I had to rewrite the description to be more clearer.

Thanks for the clarification!

primiano@: Are you on the same page?

BTW, would it be possible to split the CLs into the following three pieces?

1) Move WebMemoryAllocatorDumpImpl into Blink and unify it with
WebMemoryAllocatorDump.
2) Move WebProcessMemoryDumpImpl into Blink and unify it with
WebProcessMemoryDump.
3) Move WebMemoryDumpProviderAdapter into Blink and unify it with
WebMemoryDumpProvider.

It will clarify what you want to accomplish.

[hajimehoshi, 2016-02-15 11:00:32]

On 2016/02/15 10:26:21, haraken wrote:
> On 2016/02/15 09:59:01, hajimehoshi wrote:
> > On 2016/02/15 09:52:45, haraken wrote:
> > > On 2016/02/15 09:48:48, hajimehoshi wrote:
> > > > On 2016/02/15 09:40:50, haraken wrote:
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1660383002/diff/140001/third_party/WebKit/Sou...
> > > > > File third_party/WebKit/Source/platform/WebProcessMemoryDumpImpl.h
> > (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1660383002/diff/140001/third_party/WebKit/Sou...
> > > > > third_party/WebKit/Source/platform/WebProcessMemoryDumpImpl.h:90:
> > > > > WTF::OwnPtr<base::trace_event::ProcessMemoryDump>
> > > owned_process_memory_dump_;
> > > > > 
> > > > > It looks strange to use WTF::OwnPtr for Chromium classes. We should
keep
> > > using
> > > > > scoped_ptr.
> > > > > 
> > > > > However, if you need to introduce a bunch of scoped_ptr and
> scoped_refptr
> > to
> > > > > Blink's platform (like this CL), it implies that you're doing
something
> > > > strange.
> > > > > This CL is just moving glue classes from content/ to platform/ without
> > > > removing
> > > > > any code. As primiano@ pointed out, I don't see much advantage of
doing
> > the
> > > > > migration.
> > > > 
> > > > IIUC, we can reduce code size by unifying FooImpl and Foo classes after
> this
> > > CL.
> > > > As you said, this CL just moves classes but it is intentional. You mean
I
> > > should
> > > > remove those classes in this CL at the same time?
> > > 
> > > Sorry, I think I (and probably primiano@) am behind.
> > > 
> > > Would you elaborate on what classes are going to be migrated and then
> removed?
> > > (what does FooImpl mean specifically?)
> > 
> > - Unify WebMemoryAllocatorDump and WebMemoryAllocatorDumpImpl
> > - Unify WebProcessMemoryDumpImpl and WebProcessMemoryDumpImpl
> > - Unify WebMemoryDumpProvider and WebMemoryDumpProviderAdapter
> > 
> > Sorry but I had to rewrite the description to be more clearer.
> 
> Thanks for the clarification!
> 
> primiano@: Are you on the same page?
> 
> BTW, would it be possible to split the CLs into the following three pieces?
> 
> 1) Move WebMemoryAllocatorDumpImpl into Blink and unify it with
> WebMemoryAllocatorDump.
> 2) Move WebProcessMemoryDumpImpl into Blink and unify it with
> WebProcessMemoryDump.
> 3) Move WebMemoryDumpProviderAdapter into Blink and unify it with
> WebMemoryDumpProvider.
> 
> It will clarify what you want to accomplish.

I have no thought to stick to this CL, so your suggestion is fine to me.

[hajimehoshi, 2016-02-17 10:21:48]

On 2016/02/15 11:00:32, hajimehoshi wrote:
> On 2016/02/15 10:26:21, haraken wrote:
> > On 2016/02/15 09:59:01, hajimehoshi wrote:
> > > On 2016/02/15 09:52:45, haraken wrote:
> > > > On 2016/02/15 09:48:48, hajimehoshi wrote:
> > > > > On 2016/02/15 09:40:50, haraken wrote:
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1660383002/diff/140001/third_party/WebKit/Sou...
> > > > > > File third_party/WebKit/Source/platform/WebProcessMemoryDumpImpl.h
> > > (right):
> > > > > > 
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1660383002/diff/140001/third_party/WebKit/Sou...
> > > > > > third_party/WebKit/Source/platform/WebProcessMemoryDumpImpl.h:90:
> > > > > > WTF::OwnPtr<base::trace_event::ProcessMemoryDump>
> > > > owned_process_memory_dump_;
> > > > > > 
> > > > > > It looks strange to use WTF::OwnPtr for Chromium classes. We should
> keep
> > > > using
> > > > > > scoped_ptr.
> > > > > > 
> > > > > > However, if you need to introduce a bunch of scoped_ptr and
> > scoped_refptr
> > > to
> > > > > > Blink's platform (like this CL), it implies that you're doing
> something
> > > > > strange.
> > > > > > This CL is just moving glue classes from content/ to platform/
without
> > > > > removing
> > > > > > any code. As primiano@ pointed out, I don't see much advantage of
> doing
> > > the
> > > > > > migration.
> > > > > 
> > > > > IIUC, we can reduce code size by unifying FooImpl and Foo classes
after
> > this
> > > > CL.
> > > > > As you said, this CL just moves classes but it is intentional. You
mean
> I
> > > > should
> > > > > remove those classes in this CL at the same time?
> > > > 
> > > > Sorry, I think I (and probably primiano@) am behind.
> > > > 
> > > > Would you elaborate on what classes are going to be migrated and then
> > removed?
> > > > (what does FooImpl mean specifically?)
> > > 
> > > - Unify WebMemoryAllocatorDump and WebMemoryAllocatorDumpImpl
> > > - Unify WebProcessMemoryDumpImpl and WebProcessMemoryDumpImpl
> > > - Unify WebMemoryDumpProvider and WebMemoryDumpProviderAdapter
> > > 
> > > Sorry but I had to rewrite the description to be more clearer.
> > 
> > Thanks for the clarification!
> > 
> > primiano@: Are you on the same page?
> > 
> > BTW, would it be possible to split the CLs into the following three pieces?
> > 
> > 1) Move WebMemoryAllocatorDumpImpl into Blink and unify it with
> > WebMemoryAllocatorDump.
> > 2) Move WebProcessMemoryDumpImpl into Blink and unify it with
> > WebProcessMemoryDump.
> > 3) Move WebMemoryDumpProviderAdapter into Blink and unify it with
> > WebMemoryDumpProvider.
> > 
> > It will clarify what you want to accomplish.
> 
> I have no thought to stick to this CL, so your suggestion is fine to me.

WebMemoryDumpProviderAdapter uses WebProcessMemoryDumpImpl, and
WebProcessMemoryDumpImpl uses WebMemoryAllocatorDumpImpl. As long as
WebKit/Source can't depend on content/child, would it be difficult to move only
one class of them?

[haraken, 2016-02-17 10:40:22]

On 2016/02/17 10:21:48, hajimehoshi wrote:
> On 2016/02/15 11:00:32, hajimehoshi wrote:
> > On 2016/02/15 10:26:21, haraken wrote:
> > > On 2016/02/15 09:59:01, hajimehoshi wrote:
> > > > On 2016/02/15 09:52:45, haraken wrote:
> > > > > On 2016/02/15 09:48:48, hajimehoshi wrote:
> > > > > > On 2016/02/15 09:40:50, haraken wrote:
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1660383002/diff/140001/third_party/WebKit/Sou...
> > > > > > > File third_party/WebKit/Source/platform/WebProcessMemoryDumpImpl.h
> > > > (right):
> > > > > > > 
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1660383002/diff/140001/third_party/WebKit/Sou...
> > > > > > > third_party/WebKit/Source/platform/WebProcessMemoryDumpImpl.h:90:
> > > > > > > WTF::OwnPtr<base::trace_event::ProcessMemoryDump>
> > > > > owned_process_memory_dump_;
> > > > > > > 
> > > > > > > It looks strange to use WTF::OwnPtr for Chromium classes. We
should
> > keep
> > > > > using
> > > > > > > scoped_ptr.
> > > > > > > 
> > > > > > > However, if you need to introduce a bunch of scoped_ptr and
> > > scoped_refptr
> > > > to
> > > > > > > Blink's platform (like this CL), it implies that you're doing
> > something
> > > > > > strange.
> > > > > > > This CL is just moving glue classes from content/ to platform/
> without
> > > > > > removing
> > > > > > > any code. As primiano@ pointed out, I don't see much advantage of
> > doing
> > > > the
> > > > > > > migration.
> > > > > > 
> > > > > > IIUC, we can reduce code size by unifying FooImpl and Foo classes
> after
> > > this
> > > > > CL.
> > > > > > As you said, this CL just moves classes but it is intentional. You
> mean
> > I
> > > > > should
> > > > > > remove those classes in this CL at the same time?
> > > > > 
> > > > > Sorry, I think I (and probably primiano@) am behind.
> > > > > 
> > > > > Would you elaborate on what classes are going to be migrated and then
> > > removed?
> > > > > (what does FooImpl mean specifically?)
> > > > 
> > > > - Unify WebMemoryAllocatorDump and WebMemoryAllocatorDumpImpl
> > > > - Unify WebProcessMemoryDumpImpl and WebProcessMemoryDumpImpl
> > > > - Unify WebMemoryDumpProvider and WebMemoryDumpProviderAdapter
> > > > 
> > > > Sorry but I had to rewrite the description to be more clearer.
> > > 
> > > Thanks for the clarification!
> > > 
> > > primiano@: Are you on the same page?
> > > 
> > > BTW, would it be possible to split the CLs into the following three
pieces?
> > > 
> > > 1) Move WebMemoryAllocatorDumpImpl into Blink and unify it with
> > > WebMemoryAllocatorDump.
> > > 2) Move WebProcessMemoryDumpImpl into Blink and unify it with
> > > WebProcessMemoryDump.
> > > 3) Move WebMemoryDumpProviderAdapter into Blink and unify it with
> > > WebMemoryDumpProvider.
> > > 
> > > It will clarify what you want to accomplish.
> > 
> > I have no thought to stick to this CL, so your suggestion is fine to me.
> 
> WebMemoryDumpProviderAdapter uses WebProcessMemoryDumpImpl, and
> WebProcessMemoryDumpImpl uses WebMemoryAllocatorDumpImpl. As long as
> WebKit/Source can't depend on content/child, would it be difficult to move
only
> one class of them?

ah, makes sense.

primiano@: How do you want to handle this CL?

[hajimehoshi, 2016-02-18 11:21:53]

https://codereview.chromium.org/1660383002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/WebProcessMemoryDumpImpl.h (right):

https://codereview.chromium.org/1660383002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/WebProcessMemoryDumpImpl.h:90:
WTF::OwnPtr<base::trace_event::ProcessMemoryDump> owned_process_memory_dump_;
On 2016/02/15 09:40:49, haraken wrote:
> 
> It looks strange to use WTF::OwnPtr for Chromium classes. We should keep using
> scoped_ptr.
> 
> However, if you need to introduce a bunch of scoped_ptr and scoped_refptr to
> Blink's platform (like this CL), it implies that you're doing something
strange.
> This CL is just moving glue classes from content/ to platform/ without
removing
> any code. As primiano@ pointed out, I don't see much advantage of doing the
> migration.

You mean we should use smart pointers in base for Chromium (and base) classes?
This is NOT a necessary change as long as platform can depend on base's smart
pointers.

https://codereview.chromium.org/1660383002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/WebProcessMemoryDumpImpl.h:109:
WTF::Vector<WTF::OwnPtr<skia::SkiaTraceMemoryDumpImpl>> sk_trace_dump_list_;
On the other hand, ScopedVector is not permitted in platform and I had to use
WTF::OwnPtr here. Is this correct?

[esprehn, 2016-02-19 02:54:23]

On 2016/02/18 at 11:21:53, hajimehoshi wrote:
> ...
> 
>
https://codereview.chromium.org/1660383002/diff/140001/third_party/WebKit/Sou...
> third_party/WebKit/Source/platform/WebProcessMemoryDumpImpl.h:109:
WTF::Vector<WTF::OwnPtr<skia::SkiaTraceMemoryDumpImpl>> sk_trace_dump_list_;
> On the other hand, ScopedVector is not permitted in platform and I had to use
WTF::OwnPtr here. Is this correct?

You shouldn't need ScopedVector, use std::vector<scoped_ptr<>> instead.

[hajimehoshi, 2016-02-22 12:54:37]

On 2016/02/19 02:54:23, esprehn wrote:
> On 2016/02/18 at 11:21:53, hajimehoshi wrote:
> > ...
> > 
> >
>
https://codereview.chromium.org/1660383002/diff/140001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/platform/WebProcessMemoryDumpImpl.h:109:
> WTF::Vector<WTF::OwnPtr<skia::SkiaTraceMemoryDumpImpl>> sk_trace_dump_list_;
> > On the other hand, ScopedVector is not permitted in platform and I had to
use
> WTF::OwnPtr here. Is this correct?
> 
> You shouldn't need ScopedVector, use std::vector<scoped_ptr<>> instead.

Done.

[esprehn, 2016-02-23 00:11:59]

lgtm, please don't abbreviate in blink though. :)

https://codereview.chromium.org/1660383002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/exported/Platform.cpp (right):

https://codereview.chromium.org/1660383002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/exported/Platform.cpp:95: void
Platform::registerMemoryDumpProvider(WebMemoryDumpProvider* wmdp, const char*
name)
don't abbreviate, wmdp -> |provider|

https://codereview.chromium.org/1660383002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/exported/Platform.cpp:105: void
Platform::unregisterMemoryDumpProvider(WebMemoryDumpProvider* wmdp)
ditto

[Nico, 2016-02-23 01:08:08]

(don't abbreviate in non-blink chromium code either :-) )

[hajimehoshi, 2016-02-23 07:47:35]

Thank you!

https://codereview.chromium.org/1660383002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/exported/Platform.cpp (right):

https://codereview.chromium.org/1660383002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/exported/Platform.cpp:95: void
Platform::registerMemoryDumpProvider(WebMemoryDumpProvider* wmdp, const char*
name)
On 2016/02/23 00:11:59, esprehn wrote:
> don't abbreviate, wmdp -> |provider|

Done.

https://codereview.chromium.org/1660383002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/exported/Platform.cpp:105: void
Platform::unregisterMemoryDumpProvider(WebMemoryDumpProvider* wmdp)
On 2016/02/23 00:11:59, esprehn wrote:
> ditto

Done.

[hajimehoshi, 2016-02-23 07:49:07]

hajimehoshi@chromium.org changed reviewers:
+ xhwang@chromium.org

[hajimehoshi, 2016-02-23 07:49:08]

+xhwang for media/blink

PTAL

[xhwang, 2016-02-23 07:53:02]

media/blink lgtm

[tkent, 2016-02-23 08:07:30]

https://codereview.chromium.org/1660383002/diff/240001/third_party/WebKit/PRE...
File third_party/WebKit/PRESUBMIT.py (right):

https://codereview.chromium.org/1660383002/diff/240001/third_party/WebKit/PRE...
third_party/WebKit/PRESUBMIT.py:117: # Ignore the style check for files that are
moved from Chromium side to
Can you borrow the _CheckStyle change of
https://codereview.chromium.org/1538803002/, and keep Chromium file names?

[hajimehoshi, 2016-02-23 10:08:51]

Thank you!

https://codereview.chromium.org/1660383002/diff/240001/third_party/WebKit/PRE...
File third_party/WebKit/PRESUBMIT.py (right):

https://codereview.chromium.org/1660383002/diff/240001/third_party/WebKit/PRE...
third_party/WebKit/PRESUBMIT.py:117: # Ignore the style check for files that are
moved from Chromium side to
On 2016/02/23 08:07:30, tkent wrote:
> Can you borrow the _CheckStyle change of
> https://codereview.chromium.org/1538803002/, and keep Chromium file names?

Done.

[hajimehoshi, 2016-02-23 10:35:47]

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

[haraken, 2016-02-23 10:37:01]

LGTM

https://codereview.chromium.org/1660383002/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/DEPS (right):

https://codereview.chromium.org/1660383002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/DEPS:10: "+base/strings/stringprintf.h",

Is this needed?

https://codereview.chromium.org/1660383002/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/web_process_memory_dump_impl.h (right):

https://codereview.chromium.org/1660383002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/web_process_memory_dump_impl.h:107:
scoped_ptr<WebMemoryAllocatorDumpImpl>>

Can we use a HashMap<base::trace_event::MemoryAllocatorDump*,
OwnPtr<WebMemoryAllocatorDumpImp>>?

Given that WebMemoryAllocatorDumpImpl is now in the Blink side, we should use an
OwnPtr.

However, if you're planning to remove the WebMemoryAllocatorDumpImpl in the next
CL (and replace it with WebMemoryAllocatorDump?), I'm fine with keeping using
the scoped_ptr in this CL.

[hajimehoshi, 2016-02-23 10:39:36]

primiano@, I'd like to know whether you are OK about this change just in case
because IIUC you designed and implemented these classes.

[hajimehoshi, 2016-02-23 11:10:39]

Thank you!

https://codereview.chromium.org/1660383002/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/DEPS (right):

https://codereview.chromium.org/1660383002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/DEPS:10: "+base/strings/stringprintf.h",
On 2016/02/23 10:37:01, haraken wrote:
> 
> Is this needed?

Yes. This is used at
third_party/WebKit/Source/platform/web_process_memory_dump_impl.cc

https://codereview.chromium.org/1660383002/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/web_process_memory_dump_impl.h (right):

https://codereview.chromium.org/1660383002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/web_process_memory_dump_impl.h:107:
scoped_ptr<WebMemoryAllocatorDumpImpl>>
On 2016/02/23 10:37:01, haraken wrote:
> 
> Can we use a HashMap<base::trace_event::MemoryAllocatorDump*,
> OwnPtr<WebMemoryAllocatorDumpImp>>?
> 
> Given that WebMemoryAllocatorDumpImpl is now in the Blink side, we should use
an
> OwnPtr.
> 
> However, if you're planning to remove the WebMemoryAllocatorDumpImpl in the
next
> CL (and replace it with WebMemoryAllocatorDump?), I'm fine with keeping using
> the scoped_ptr in this CL.

This implementation is not removed but just will be unified with
WebMemoryAllocatorDump, so I need to fix as you said anyway. Done.

[tkent, 2016-02-23 23:46:57]

third_party/WebKit lgtm

[kinuko, 2016-02-25 08:42:45]

lgtm

[hajimehoshi, 2016-02-25 08:43:46]

The CQ bit was checked by hajimehoshi@chromium.org

[hajimehoshi, 2016-02-25 08:43:47]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org,
esprehn@chromium.org, xhwang@chromium.org, haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/1660383002/#ps320001
(title: "Add PLATFORM_EXPORT; Address haraken's review")

[commit-bot: I haz the power, 2016-02-25 08:44:20]

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

[commit-bot: I haz the power, 2016-02-25 09:48:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-25 09:48:29]

Exceeded global retry quota

[hajimehoshi, 2016-02-25 09:49:10]

The CQ bit was checked by hajimehoshi@chromium.org

[commit-bot: I haz the power, 2016-02-25 09:49:52]

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

[commit-bot: I haz the power, 2016-02-25 11:06:22]

Description was changed from

==========
Refactoring: Move some classes from content/child to platform

This CL moves these classes:
- WebMemoryAllocatorDumpImpl
- WebMemoryDumpProviderAdapter
- WebProcessMemoryDumpImpl(Test)
from content/child to platform. Now that platform can depend on base,
we don't need adapter classes in Chromium side.

After this CL, we will be able to reduce glue classes by unifying:
- WebMemoryAllocatorDump and WebMemoryAllocatorDumpImpl
- WebProcessMemoryDump and WebProcessMemoryDumpImpl
- WebMemoryDumpProvider and WebMemoryDumpProviderAdapter
so that we can reduce lots of code.

BUG=548254
==========

to

==========
Refactoring: Move some classes from content/child to platform

This CL moves these classes:
- WebMemoryAllocatorDumpImpl
- WebMemoryDumpProviderAdapter
- WebProcessMemoryDumpImpl(Test)
from content/child to platform. Now that platform can depend on base,
we don't need adapter classes in Chromium side.

After this CL, we will be able to reduce glue classes by unifying:
- WebMemoryAllocatorDump and WebMemoryAllocatorDumpImpl
- WebProcessMemoryDump and WebProcessMemoryDumpImpl
- WebMemoryDumpProvider and WebMemoryDumpProviderAdapter
so that we can reduce lots of code.

BUG=548254
==========

[commit-bot: I haz the power, 2016-02-25 11:06:23]

Committed patchset #17 (id:320001)

[commit-bot: I haz the power, 2016-02-25 11:08:02]

Description was changed from

==========
Refactoring: Move some classes from content/child to platform

This CL moves these classes:
- WebMemoryAllocatorDumpImpl
- WebMemoryDumpProviderAdapter
- WebProcessMemoryDumpImpl(Test)
from content/child to platform. Now that platform can depend on base,
we don't need adapter classes in Chromium side.

After this CL, we will be able to reduce glue classes by unifying:
- WebMemoryAllocatorDump and WebMemoryAllocatorDumpImpl
- WebProcessMemoryDump and WebProcessMemoryDumpImpl
- WebMemoryDumpProvider and WebMemoryDumpProviderAdapter
so that we can reduce lots of code.

BUG=548254
==========

to

==========
Refactoring: Move some classes from content/child to platform

This CL moves these classes:
- WebMemoryAllocatorDumpImpl
- WebMemoryDumpProviderAdapter
- WebProcessMemoryDumpImpl(Test)
from content/child to platform. Now that platform can depend on base,
we don't need adapter classes in Chromium side.

After this CL, we will be able to reduce glue classes by unifying:
- WebMemoryAllocatorDump and WebMemoryAllocatorDumpImpl
- WebProcessMemoryDump and WebProcessMemoryDumpImpl
- WebMemoryDumpProvider and WebMemoryDumpProviderAdapter
so that we can reduce lots of code.

BUG=548254

Committed: https://crrev.com/a8227af5f6016e10e78b539ffa55aeb634ca7601
Cr-Commit-Position: refs/heads/master@{#377552}
==========

[commit-bot: I haz the power, 2016-02-25 11:08:03]

Patchset 17 (id:??) landed as
https://crrev.com/a8227af5f6016e10e78b539ffa55aeb634ca7601
Cr-Commit-Position: refs/heads/master@{#377552}

[Mostyn Bramley-Moore, 2016-02-25 13:35:34]

It looks like the last change breaks GCC builds, when linking libblink_web.so (I
have not had time to investigate yet):

obj/third_party/WebKit/Source/platform/libblink_platform_test_support.a(obj/third_party/WebKit/Source/platform/testing/blink_platform_test_support.TestingPlatformSupport.o):TestingPlatformSupport.cpp:function
blink::TestingPlatformSupport::~TestingPlatformSupport(): error: undefined
reference to 'vtable for blink::Platform'
/usr/bin/ld.gold: the vtable symbol may be undefined because the class is
missing its key function
obj/third_party/WebKit/Source/platform/libblink_platform_test_support.a(obj/third_party/WebKit/Source/platform/testing/blink_platform_test_support.TestingPlatformSupport.o):TestingPlatformSupport.cpp:function
blink::TestingPlatformSupport::~TestingPlatformSupport(): error: undefined
reference to 'vtable for blink::Platform'
/usr/bin/ld.gold: the vtable symbol may be undefined because the class is
missing its key function
obj/third_party/WebKit/Source/platform/libblink_platform_test_support.a(obj/third_party/WebKit/Source/platform/testing/blink_platform_test_support.TestingPlatformSupport.o):TestingPlatformSupport.cpp:function
blink::TestingPlatformSupportWithMockScheduler::~TestingPlatformSupportWithMockScheduler():
error: undefined reference to 'vtable for blink::Platform'
/usr/bin/ld.gold: the vtable symbol may be undefined because the class is
missing its key function
collect2: error: ld returned 1 exit status

[Zhen Wang, 2016-02-25 22:31:12]

A revert of this CL (patchset #17 id:320001) has been created in
https://codereview.chromium.org/1739113002/ by zhenw@chromium.org.

The reason for reverting is: This CL makes perf benchmarks fail on several
platforms.

The bisect result:
https://chromeperf.appspot.com/group_report?bug_id=589850.

[Primiano Tucci (use gerrit), 2016-02-26 03:13:44]

Yes this needz fix or revert, is crashing somewhere in memory traces.
Backtrace here:

warning: Executable module changed from
"/Users/primiano/code/chrome/src/out/Release/Chromium.app/Contents/MacOS/Chromium"
to
"/Users/primiano/code/chrome/src/out/Release/Chromium.app/Contents/Versions/50.0.2660.0/Chromium
Helper.app/Contents/MacOS/Chromium Helper".
(lldb) c
Process 88296 resuming
Process 88296 stopped
* thread #1: tid = 0x21c12b, 0x000000010cc52936
libblink_platform.dylib`blink::PurgeableVector::onMemoryDump(this=0x00000092d226bc30,
dumpName=<unavailable>, memoryDump=0x00007fff58139ea8) const + 86 at
PurgeableVector.cpp:88, name = 'CrRendererMain', queue =
'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
    frame #0: 0x000000010cc52936
libblink_platform.dylib`blink::PurgeableVector::onMemoryDump(this=0x00000092d226bc30,
dumpName=<unavailable>, memoryDump=0x00007fff58139ea8) const + 86 at
PurgeableVector.cpp:88
   85  	    ASSERT(!(m_discardable && m_vector.size()));
   86  	    if (m_discardable) {
   87  	        WebMemoryAllocatorDump* dump =
m_discardable->createMemoryAllocatorDump(dumpName, memoryDump);
-> 88  	        dump->addScalar("discardable_size", "bytes", m_discardableSize);
   89  	    } else if (m_vector.size()) {
   90  	        WebMemoryAllocatorDump* dump =
memoryDump->createMemoryAllocatorDump(dumpName);
   91  	        dump->addScalar("size", "bytes", m_vector.size());
(lldb) bt
(lldb) bt

* thread #1: tid = 0x21c12b, 0x000000010cc52936
libblink_platform.dylib`blink::PurgeableVector::onMemoryDump(this=0x00000092d226bc30,
dumpName=<unavailable>, memoryDump=0x00007fff58139ea8) const + 86 at
PurgeableVector.cpp:88, name = 'CrRendererMain', queue =
'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
  * frame #0: 0x000000010cc52936
libblink_platform.dylib`blink::PurgeableVector::onMemoryDump(this=0x00000092d226bc30,
dumpName=<unavailable>, memoryDump=0x00007fff58139ea8) const + 86 at
PurgeableVector.cpp:88
    frame #1: 0x000000010cc54a9b
libblink_platform.dylib`blink::SharedBuffer::onMemoryDump(this=<unavailable>,
dumpPrefix=<unavailable>, memoryDump=0x00007fff58139ea8) const + 219 at
SharedBuffer.cpp:412
    frame #2: 0x00000001185855aa
libwebcore_shared.dylib`blink::Resource::onMemoryDump(this=0x0000396d9bdd11b8,
levelOfDetail=Detailed, memoryDump=0x00007fff58139ea8) const + 362 at
Resource.cpp:770
    frame #3: 0x000000011857937d
libwebcore_shared.dylib`blink::MemoryCache::onMemoryDump(this=<unavailable>,
levelOfDetail=<unavailable>, memoryDump=<unavailable>) + 333 at
MemoryCache.cpp:744
    frame #4: 0x000000011859fdd0
libwebcore_shared.dylib`blink::WebCacheMemoryDumpProvider::onMemoryDump(this=0x00000092d1c48150,
levelOfDetail=Detailed, memoryDump=0x00007fff58139ea8) + 48 at
WebCacheMemoryDumpProvider.cpp:23
    frame #5: 0x000000010ce12587
libblink_platform.dylib`blink::WebMemoryDumpProviderAdapter::OnMemoryDump(this=0x00007fc5d8418610,
args=<unavailable>, pmd=<unavailable>) + 55 at
web_memory_dump_provider_adapter.cc:40
    frame #6: 0x000000010c8a2ee5
libbase.dylib`base::trace_event::MemoryDumpManager::InvokeOnMemoryDump(this=<unavailable>,
owned_pmd_async_state=0x00007fc5dd094ea0) + 661 at memory_dump_manager.cc:527
    frame #7: 0x000000010c8a2082
libbase.dylib`base::trace_event::MemoryDumpManager::SetupNextMemoryDump(this=0x00007fc5d856f270,
pmd_async_state=scoped_ptr<base::trace_event::MemoryDumpManager::ProcessMemoryDumpAsyncState,
std::__1::default_delete<base::trace_event::MemoryDumpManager::ProcessMemoryDumpAsyncState>
> @ 0x00007fff5813ab80) + 418 at memory_dump_manager.cc:440
    frame #8: 0x000000010c8a2f96
libbase.dylib`base::trace_event::MemoryDumpManager::InvokeOnMemoryDump(this=<unavailable>,
owned_pmd_async_state=<unavailable>) + 838 at memory_dump_manager.cc:533
    frame #9: 0x000000010c8a2082
libbase.dylib`base::trace_event::MemoryDumpManager::SetupNextMemoryDump(this=0x00007fc5d856f270,
pmd_async_state=scoped_ptr<base::trace_event::MemoryDumpManager::ProcessMemoryDumpAsyncState,
std::__1::default_delete<base::trace_event::MemoryDumpManager::ProcessMemoryDumpAsyncState>
> @ 0x00007fff5813b3e0) + 418 at memory_dump_manager.cc:440
    frame #10: 0x000000010c8a2f96
libbase.dylib`base::trace_event::MemoryDumpManager::InvokeOnMemoryDump(this=<unavailable>,
owned_pmd_async_state=<unavailable>) + 838 at memory_dump_manager.cc:533
    frame #11: 0x000000010c8a2082
libbase.dylib`base::trace_event::MemoryDumpManager::SetupNextMemoryDump(this=0x00007fc5d856f270,
pmd_async_state=scoped_ptr<base::trace_event::MemoryDumpManager::ProcessMemoryDumpAsyncState,
std::__1::default_delete<base::trace_event::MemoryDumpManager::ProcessMemoryDumpAsyncState>
> @ 0x00007fff5813bc40) + 418 at memory_dump_manager.cc:440
    frame #12: 0x000000010c8a2f96
libbase.dylib`base::trace_event::MemoryDumpManager::InvokeOnMemoryDump(this=<unavailable>,
owned_pmd_async_state=<unavailable>) + 838 at memory_dump_manager.cc:533
    frame #13: 0x000000010c8a2082
libbase.dylib`base::trace_event::MemoryDumpManager::SetupNextMemoryDump(this=0x00007fc5d856f270,
pmd_async_state=scoped_ptr<base::trace_event::MemoryDumpManager::ProcessMemoryDumpAsyncState,
std::__1::default_delete<base::trace_event::MemoryDumpManager::ProcessMemoryDumpAsyncState>
> @ 0x00007fff5813c4a0) + 418 at memory_dump_manager.cc:440
    frame #14: 0x000000010c8a2f96
libbase.dylib`base::trace_event::MemoryDumpManager::InvokeOnMemoryDump(this=<unavailable>,
owned_pmd_async_state=<unavailable>) + 838 at memory_dump_manager.cc:533
    frame #15: 0x000000010c8a2082
libbase.dylib`base::trace_event::MemoryDumpManager::SetupNextMemoryDump(this=0x00007fc5d856f270,
pmd_async_state=scoped_ptr<base::trace_event::MemoryDumpManager::ProcessMemoryDumpAsyncState,
std::__1::default_delete<base::trace_event::MemoryDumpManager::ProcessMemoryDumpAsyncState>
> @ 0x00007fff5813cd00) + 418 at memory_dump_manager.cc:440
    frame #16: 0x000000010c8a2f96
libbase.dylib`base::trace_event::MemoryDumpManager::InvokeOnMemoryDump(this=<unavailable>,
owned_pmd_async_state=<unavailable>) + 838 at memory_dump_manager.cc:533
    frame #17: 0x000000010c8a2082
libbase.dylib`base::trace_event::MemoryDumpManager::SetupNextMemoryDump(this=0x00007fc5d856f270,
pmd_async_state=scoped_ptr<base::trace_event::MemoryDumpManager::ProcessMemoryDumpAsyncState,
std::__1::default_delete<base::trace_event::MemoryDumpManager::ProcessMemoryDumpAsyncState>
> @ 0x00007fff5813d560) + 418 at memory_dump_manager.cc:440
    frame #18: 0x000000010c8a2f96
libbase.dylib`base::trace_event::MemoryDumpManager::InvokeOnMemoryDump(this=<unavailable>,
owned_pmd_async_state=<unavailable>) + 838 at memory_dump_manager.cc:533
    frame #19: 0x000000010c8a2082
libbase.dylib`base::trace_event::MemoryDumpManager::SetupNextMemoryDump(this=0x00007fc5d856f270,
pmd_async_state=scoped_ptr<base::trace_event::MemoryDumpManager::ProcessMemoryDumpAsyncState,
std::__1::default_delete<base::trace_event::MemoryDumpManager::ProcessMemoryDumpAsyncState>
> @ 0x00007fff5813ddc0) + 418 at memory_dump_manager.cc:440
    frame #20: 0x000000010c8a2f96
libbase.dylib`base::trace_event::MemoryDumpManager::InvokeOnMemoryDump(this=<unavailable>,
owned_pmd_async_state=<unavailable>) + 838 at memory_dump_manager.cc:533
    frame #21: 0x000000010c8a2082
libbase.dylib`base::trace_event::MemoryDumpManager::SetupNextMemoryDump(this=0x00007fc5d856f270,
pmd_async_state=scoped_ptr<base::trace_event::MemoryDumpManager::ProcessMemoryDumpAsyncState,
std::__1::default_delete<base::trace_event::MemoryDumpManager::ProcessMemoryDumpAsyncState>
> @ 0x00007fff5813e620) + 418 at memory_dump_manager.cc:440
    frame #22: 0x000000010c8a2f96
libbase.dylib`base::trace_event::MemoryDumpManager::InvokeOnMemoryDump(this=<unavailable>,
owned_pmd_async_state=<unavailable>) + 838 at memory_dump_manager.cc:533
    frame #23: 0x000000010c8040ab
libbase.dylib`base::debug::TaskAnnotator::RunTask(char const*, base::PendingTask
const&) [inlined] base::Callback<void ()>::Run() const + 8 at callback.h:394
    frame #24: 0x000000010c8040a3
libbase.dylib`base::debug::TaskAnnotator::RunTask(this=<unavailable>,
queue_function="TaskQueueManager::PostTask", pending_task=0x00007fff5813e900) +
179 at task_annotator.cc:51
    frame #25: 0x000000011ae29c93
libscheduler.dylib`scheduler::TaskQueueManager::ProcessTaskFromWorkQueue(this=<unavailable>,
work_queue=<unavailable>, out_previous_task=<unavailable>) + 755 at
task_queue_manager.cc:288

[Primiano Tucci (use gerrit), 2016-02-26 03:27:28]

Re-attempting revert in https://codereview.chromium.org/1741673002/

[Primiano Tucci (use gerrit), 2016-02-26 04:10:05]

On 2016/02/26 03:27:28, Primiano (throttled til Mar 4) wrote:
> Re-attempting revert in https://codereview.chromium.org/1741673002/

Ok I think I know what broke... typo which exposed the NOTREACHED base method.
I unticked my revert, it's probably easier to just fix it:
https://codereview.chromium.org/1731893004

[bashi, 2016-02-26 04:13:24]

On 2016/02/26 04:10:05, Primiano (throttled til Mar 4) wrote:
> On 2016/02/26 03:27:28, Primiano (throttled til Mar 4) wrote:
> > Re-attempting revert in https://codereview.chromium.org/1741673002/
> 
> Ok I think I know what broke... typo which exposed the NOTREACHED base method.
> I unticked my revert, it's probably easier to just fix it:
> https://codereview.chromium.org/1731893004

I think the following will fix the crash.

diff --git a/third_party/WebKit/Source/platform/web_process_memory_dump_impl.h
b/third_party/WebKit/Source/platform/web_process_memory_dump_impl.h
index 7775449..2c5db22 100644
--- a/third_party/WebKit/Source/platform/web_process_memory_dump_impl.h
+++ b/third_party/WebKit/Source/platform/web_process_memory_dump_impl.h
@@ -72,9 +72,9 @@ class PLATFORM_EXPORT WebProcessMemoryDumpImpl final
     return process_memory_dump_;
   }
 
-  blink::WebMemoryAllocatorDump* CreateDiscardableMemoryAllocatorDump(
+  blink::WebMemoryAllocatorDump* createDiscardableMemoryAllocatorDump(
       const std::string& name,
-      base::DiscardableMemory* discardable);
+      base::DiscardableMemory* discardable) override;