Refactoring: Unify WebMemoryAllocatorDump and WebMemoryAllocatorDumpImpl

Refactoring: Unify WebMemoryAllocatorDump and WebMemoryAllocatorDumpImpl

Now that WebMemoryAllocatorDumpImpl has been moved from content/child to
platform, we can unify WebMemoryAllocatorDump and WebMemoryAllocatorDumpImpl

BUG=548254

[hajimehoshi, 2016-02-25 12:51:54]

hajimehoshi@chromium.org changed reviewers:
+ esprehn@chromium.org, haraken@chromium.org, tkent@chromium.org

[hajimehoshi, 2016-02-25 12:51:54]

PTAL

[haraken, 2016-02-25 14:38:56]

https://codereview.chromium.org/1738843002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/web_process_memory_dump_impl.cc (right):

https://codereview.chromium.org/1738843002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/web_process_memory_dump_impl.cc:64:
WebMemoryAllocatorDump* web_memory_allocator_dump_impl =

You should use OwnPtr<WebMemoryAllocatorDump>.

https://codereview.chromium.org/1738843002/diff/1/third_party/WebKit/public/p...
File third_party/WebKit/public/platform/WebMemoryAllocatorDump.h (right):

https://codereview.chromium.org/1738843002/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/WebMemoryAllocatorDump.h:54:
DISALLOW_COPY_AND_ASSIGN(WebMemoryAllocatorDump);

yutak@ has a plan to replace WTF_MAKE_NONCOPYABLE with DISALLOW_COPY_AND_ASSIGN.
Let's use WTF_MAKE_NONCOPYABLE for now. Then you don't need to include
base/macros.h.

[hajimehoshi, 2016-02-26 11:06:00]

https://codereview.chromium.org/1738843002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/web_process_memory_dump_impl.cc (right):

https://codereview.chromium.org/1738843002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/web_process_memory_dump_impl.cc:64:
WebMemoryAllocatorDump* web_memory_allocator_dump_impl =
On 2016/02/25 14:38:55, haraken wrote:
> 
> You should use OwnPtr<WebMemoryAllocatorDump>.

I can do this, but memory_allocator_dumps_ takes the ownership soon.

https://codereview.chromium.org/1738843002/diff/1/third_party/WebKit/public/p...
File third_party/WebKit/public/platform/WebMemoryAllocatorDump.h (right):

https://codereview.chromium.org/1738843002/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/WebMemoryAllocatorDump.h:54:
DISALLOW_COPY_AND_ASSIGN(WebMemoryAllocatorDump);
On 2016/02/25 14:38:55, haraken wrote:
> 
> yutak@ has a plan to replace WTF_MAKE_NONCOPYABLE with
DISALLOW_COPY_AND_ASSIGN.
> Let's use WTF_MAKE_NONCOPYABLE for now. Then you don't need to include
> base/macros.h.

Is it allowed to use wtf/Noncopyable.h from public?

[haraken, 2016-02-26 11:20:49]

https://codereview.chromium.org/1738843002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/web_process_memory_dump_impl.cc (right):

https://codereview.chromium.org/1738843002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/web_process_memory_dump_impl.cc:64:
WebMemoryAllocatorDump* web_memory_allocator_dump_impl =
On 2016/02/26 11:06:00, hajimehoshi wrote:
> On 2016/02/25 14:38:55, haraken wrote:
> > 
> > You should use OwnPtr<WebMemoryAllocatorDump>.
> 
> I can do this, but memory_allocator_dumps_ takes the ownership soon.

The common pattern is:

  OwnPtr<X> x = adoptPtr(new X);
  passOwnership(x.release());

https://codereview.chromium.org/1738843002/diff/1/third_party/WebKit/public/p...
File third_party/WebKit/public/platform/WebMemoryAllocatorDump.h (right):

https://codereview.chromium.org/1738843002/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/WebMemoryAllocatorDump.h:54:
DISALLOW_COPY_AND_ASSIGN(WebMemoryAllocatorDump);
On 2016/02/26 11:06:00, hajimehoshi wrote:
> On 2016/02/25 14:38:55, haraken wrote:
> > 
> > yutak@ has a plan to replace WTF_MAKE_NONCOPYABLE with
> DISALLOW_COPY_AND_ASSIGN.
> > Let's use WTF_MAKE_NONCOPYABLE for now. Then you don't need to include
> > base/macros.h.
> 
> Is it allowed to use wtf/Noncopyable.h from public?

I think it's ok. public/platform/ already includes wtf/ stuff.

That said, no one uses WTF_MAKE_NONCOPYABLE or DISALLOW_COPY_AND_ASSIGN. I think
classes should have these macros, but that would be a separate issue -- so the
easiest fix would be just to not add the macro in this CL.

[hajimehoshi, 2016-02-26 11:26:13]

https://codereview.chromium.org/1738843002/diff/1/third_party/WebKit/public/p...
File third_party/WebKit/public/platform/WebMemoryAllocatorDump.h (right):

https://codereview.chromium.org/1738843002/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/WebMemoryAllocatorDump.h:54:
DISALLOW_COPY_AND_ASSIGN(WebMemoryAllocatorDump);
On 2016/02/26 11:20:49, haraken wrote:
> On 2016/02/26 11:06:00, hajimehoshi wrote:
> > On 2016/02/25 14:38:55, haraken wrote:
> > > 
> > > yutak@ has a plan to replace WTF_MAKE_NONCOPYABLE with
> > DISALLOW_COPY_AND_ASSIGN.
> > > Let's use WTF_MAKE_NONCOPYABLE for now. Then you don't need to include
> > > base/macros.h.
> > 
> > Is it allowed to use wtf/Noncopyable.h from public?
> 
> I think it's ok. public/platform/ already includes wtf/ stuff.
> 
> That said, no one uses WTF_MAKE_NONCOPYABLE or DISALLOW_COPY_AND_ASSIGN. I
think
> classes should have these macros, but that would be a separate issue -- so the
> easiest fix would be just to not add the macro in this CL.

As for headers in public/platform, they use INSIDE_BLINK when they include wtf.
This class is used from both Chromium and Blink so I think it's impossible to
use WTF_MAKE_NONCOPYABLE.

[haraken, 2016-02-26 11:32:17]

https://codereview.chromium.org/1738843002/diff/1/third_party/WebKit/public/p...
File third_party/WebKit/public/platform/WebMemoryAllocatorDump.h (right):

https://codereview.chromium.org/1738843002/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/WebMemoryAllocatorDump.h:54:
DISALLOW_COPY_AND_ASSIGN(WebMemoryAllocatorDump);
On 2016/02/26 11:26:13, hajimehoshi wrote:
> On 2016/02/26 11:20:49, haraken wrote:
> > On 2016/02/26 11:06:00, hajimehoshi wrote:
> > > On 2016/02/25 14:38:55, haraken wrote:
> > > > 
> > > > yutak@ has a plan to replace WTF_MAKE_NONCOPYABLE with
> > > DISALLOW_COPY_AND_ASSIGN.
> > > > Let's use WTF_MAKE_NONCOPYABLE for now. Then you don't need to include
> > > > base/macros.h.
> > > 
> > > Is it allowed to use wtf/Noncopyable.h from public?
> > 
> > I think it's ok. public/platform/ already includes wtf/ stuff.
> > 
> > That said, no one uses WTF_MAKE_NONCOPYABLE or DISALLOW_COPY_AND_ASSIGN. I
> think
> > classes should have these macros, but that would be a separate issue -- so
the
> > easiest fix would be just to not add the macro in this CL.
> 
> As for headers in public/platform, they use INSIDE_BLINK when they include
wtf.
> This class is used from both Chromium and Blink so I think it's impossible to
> use WTF_MAKE_NONCOPYABLE.

No one in public/ is using DISALLOW_COPY_AND_ASSIGN. We might want to use it for
all classes at some point, but shall we defer it to a separate CL?

[hajimehoshi, 2016-02-26 12:04:51]

https://codereview.chromium.org/1738843002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/web_process_memory_dump_impl.cc (right):

https://codereview.chromium.org/1738843002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/web_process_memory_dump_impl.cc:64:
WebMemoryAllocatorDump* web_memory_allocator_dump_impl =
On 2016/02/26 11:20:49, haraken wrote:
> On 2016/02/26 11:06:00, hajimehoshi wrote:
> > On 2016/02/25 14:38:55, haraken wrote:
> > > 
> > > You should use OwnPtr<WebMemoryAllocatorDump>.
> > 
> > I can do this, but memory_allocator_dumps_ takes the ownership soon.
> 
> The common pattern is:
> 
>   OwnPtr<X> x = adoptPtr(new X);
>   passOwnership(x.release());
> 

Done.

https://codereview.chromium.org/1738843002/diff/1/third_party/WebKit/public/p...
File third_party/WebKit/public/platform/WebMemoryAllocatorDump.h (right):

https://codereview.chromium.org/1738843002/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/WebMemoryAllocatorDump.h:54:
DISALLOW_COPY_AND_ASSIGN(WebMemoryAllocatorDump);
On 2016/02/26 11:32:17, haraken wrote:
> On 2016/02/26 11:26:13, hajimehoshi wrote:
> > On 2016/02/26 11:20:49, haraken wrote:
> > > On 2016/02/26 11:06:00, hajimehoshi wrote:
> > > > On 2016/02/25 14:38:55, haraken wrote:
> > > > > 
> > > > > yutak@ has a plan to replace WTF_MAKE_NONCOPYABLE with
> > > > DISALLOW_COPY_AND_ASSIGN.
> > > > > Let's use WTF_MAKE_NONCOPYABLE for now. Then you don't need to include
> > > > > base/macros.h.
> > > > 
> > > > Is it allowed to use wtf/Noncopyable.h from public?
> > > 
> > > I think it's ok. public/platform/ already includes wtf/ stuff.
> > > 
> > > That said, no one uses WTF_MAKE_NONCOPYABLE or DISALLOW_COPY_AND_ASSIGN. I
> > think
> > > classes should have these macros, but that would be a separate issue -- so
> the
> > > easiest fix would be just to not add the macro in this CL.
> > 
> > As for headers in public/platform, they use INSIDE_BLINK when they include
> wtf.
> > This class is used from both Chromium and Blink so I think it's impossible
to
> > use WTF_MAKE_NONCOPYABLE.
> 
> No one in public/ is using DISALLOW_COPY_AND_ASSIGN. We might want to use it
for
> all classes at some point, but shall we defer it to a separate CL?

Done.

[haraken, 2016-02-27 16:15:51]

LGTM

https://codereview.chromium.org/1738843002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/web_process_memory_dump_impl.cc (right):

https://codereview.chromium.org/1738843002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/web_process_memory_dump_impl.cc:71: return
memory_allocator_dumps_.get(memory_allocator_dump);

It would be inefficient to call get() here. What Blink normally does is:

  memory_allocator_dump_impl = web_memory_allocator_dump.get();
  memory_allocator_dumps_.set(..., web_memory_allocator_dump.release());
  return memory_allocator_dump_impl;

[tkent, 2016-02-29 02:34:04]

lgtm

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

https://codereview.chromium.org/1738843002/diff/40001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebMemoryAllocatorDump.h:11: //
TODO(hajimehoshi): WTF_MAKE_NONCOPYABLE works only in Blink side so far.
Blink public classes don't have WTF_MAKE_NONCOPYABLE because of this issue.
You may remove WTF_MAKE_NONCOPYABLE, or you may land this as is.

[hajimehoshi, 2016-02-29 06:48:35]

The CQ bit was checked by hajimehoshi@chromium.org

[hajimehoshi, 2016-02-29 06:48:35]

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

[commit-bot: I haz the power, 2016-02-29 06:48:49]

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

[esprehn, 2016-02-29 06:51:20]

Let's remove them TODO and macro and just use WebNoncopyable

https://codereview.chromium.org/1738843002/diff/60001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebMemoryAllocatorDump.h (right):

https://codereview.chromium.org/1738843002/diff/60001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebMemoryAllocatorDump.h:33:
WTF_MAKE_NONCOPYABLE(WebMemoryAllocatorDump);
Don't do this. Use WebNoncopyable which does what you want.

[esprehn, 2016-02-29 06:51:21]

Let's remove them TODO and macro and just use WebNoncopyable

[haraken, 2016-02-29 06:55:30]

https://codereview.chromium.org/1738843002/diff/60001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebMemoryAllocatorDump.h (right):

https://codereview.chromium.org/1738843002/diff/60001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebMemoryAllocatorDump.h:33:
WTF_MAKE_NONCOPYABLE(WebMemoryAllocatorDump);
On 2016/02/29 06:51:20, esprehn wrote:
> Don't do this. Use WebNoncopyable which does what you want.

So as tkent@ and I are saying in the above, I'd suggest simply removing
NONCOPYABLE things from this CL. That's a separate issue and it should be
addressed in a separate CL.

[haraken, 2016-02-29 07:12:46]

On 2016/02/29 06:51:20, esprehn wrote:
> Let's remove them TODO and macro and just use WebNoncopyable
> 
>
https://codereview.chromium.org/1738843002/diff/60001/third_party/WebKit/publ...
> File third_party/WebKit/public/platform/WebMemoryAllocatorDump.h (right):
> 
>
https://codereview.chromium.org/1738843002/diff/60001/third_party/WebKit/publ...
> third_party/WebKit/public/platform/WebMemoryAllocatorDump.h:33:
> WTF_MAKE_NONCOPYABLE(WebMemoryAllocatorDump);
> Don't do this. Use WebNoncopyable which does what you want.

Ah, you're right. Using WebNoncopyable looks like the best solution :)

[hajimehoshi, 2016-02-29 08:08:18]

The CQ bit was unchecked by hajimehoshi@chromium.org

[hajimehoshi, 2016-02-29 08:21:48]

Thank you!

https://codereview.chromium.org/1738843002/diff/60001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebMemoryAllocatorDump.h (right):

https://codereview.chromium.org/1738843002/diff/60001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebMemoryAllocatorDump.h:33:
WTF_MAKE_NONCOPYABLE(WebMemoryAllocatorDump);
On 2016/02/29 06:55:30, haraken wrote:
> On 2016/02/29 06:51:20, esprehn wrote:
> > Don't do this. Use WebNoncopyable which does what you want.
> 
> So as tkent@ and I are saying in the above, I'd suggest simply removing
> NONCOPYABLE things from this CL. That's a separate issue and it should be
> addressed in a separate CL.
> 

Done.

[hajimehoshi, 2016-03-01 02:14:00]

The CQ bit was checked by hajimehoshi@chromium.org

[hajimehoshi, 2016-03-01 02:14:01]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, tkent@chromium.org
Link to the patchset: https://codereview.chromium.org/1738843002/#ps80001
(title: "Use WebNonCopyable")

[commit-bot: I haz the power, 2016-03-01 02:14:14]

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

[commit-bot: I haz the power, 2016-03-01 03:43:37]

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

[commit-bot: I haz the power, 2016-03-01 03:43:38]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)