[Memory UMA] Return the extra_processes_dump map as part of the ResponseCallback

[Memory UMA] Return the extra_processes_dump map as part of the ResponseCallback

This CL is a follow up CL to http://crrev.com/277709300 where we add the missing
extra_processes_dump map from the MemoryDumpCallbackResult struct into its
corresponding mojo struct.

BUG=703184
Review-Url: https://codereview.chromium.org/2822593002
Cr-Commit-Position: refs/heads/master@{#466021}
Committed: https://chromium.googlesource.com/chromium/src/+/33bb7a42b9ea6c9138ac975e35b0242b9da21c07

[fmeawad, 2017-04-13 22:13:46]

The CQ bit was checked by fmeawad@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-13 22:14:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[fmeawad, 2017-04-14 22:07:42]

fmeawad@chromium.org changed reviewers:
+ hjd@chromium.org, primiano@chromium.org, rockot@chromium.org

[fmeawad, 2017-04-14 22:07:43]

PTAL when you get back :)

[hjd, 2017-04-18 11:00:38]

lgtm

[Ken Rockot(use gerrit already), 2017-04-18 22:11:25]

Great, just some nits and one request for a change in testing approach

https://codereview.chromium.org/2822593002/diff/20001/mojo/common/process_id....
File mojo/common/process_id.mojom (right):

https://codereview.chromium.org/2822593002/diff/20001/mojo/common/process_id....
mojo/common/process_id.mojom:8: // This is the storage for the pid, on windows
tje pid is a DWORD an the
nit: tje=the, an=and

https://codereview.chromium.org/2822593002/diff/20001/mojo/common/process_id....
File mojo/common/process_id.typemap (right):

https://codereview.chromium.org/2822593002/diff/20001/mojo/common/process_id....
mojo/common/process_id.typemap:8: "//ipc/ipc_message_utils.h",
not used

https://codereview.chromium.org/2822593002/diff/20001/mojo/common/process_id....
mojo/common/process_id.typemap:12: "//ipc",
not used

https://codereview.chromium.org/2822593002/diff/20001/mojo/common/test_common...
File mojo/common/test_common_custom_types.mojom (right):

https://codereview.chromium.org/2822593002/diff/20001/mojo/common/test_common...
mojo/common/test_common_custom_types.mojom:26: interface TestProcessId {
Please avoid copying this pattern for testing new typemaps. Please see
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-mojo/_m5qIDZgAf8
- TLDR is you can just directly Serialize and Deserialize a base::ProcessId
to/from a mojo::common::mojom::ProcessId without using a new mojom interface.

[Primiano Tucci (use gerrit), 2017-04-19 12:03:46]

lgtm with one minor comment

https://codereview.chromium.org/2822593002/diff/20001/base/trace_event/memory...
File base/trace_event/memory_dump_manager.cc (right):

https://codereview.chromium.org/2822593002/diff/20001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:774: result =
MemoryDumpCallbackResult();
why not just doing this directly up on line 745 when declaring the var, i.e.:

base::Optional<MemoryDumpCallbackResult> result(MemoryDumpCallbackResult());

from the code looks like this if is going to be always taken right? (the
process_dumps maps should contain always at least one argument)

[fmeawad, 2017-04-19 15:59:56]

https://codereview.chromium.org/2822593002/diff/20001/base/trace_event/memory...
File base/trace_event/memory_dump_manager.cc (right):

https://codereview.chromium.org/2822593002/diff/20001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:774: result =
MemoryDumpCallbackResult();
On 2017/04/19 12:03:45, Primiano Tucci wrote:
> why not just doing this directly up on line 745 when declaring the var, i.e.:
> 
> base::Optional<MemoryDumpCallbackResult> result(MemoryDumpCallbackResult());
> 
> from the code looks like this if is going to be always taken right? (the
> process_dumps maps should contain always at least one argument)

I do see code paths where this is never reached, but if the logic is that it has
to have at least one argument then I will change it.

[Primiano Tucci (use gerrit), 2017-04-19 16:03:24]

https://codereview.chromium.org/2822593002/diff/20001/base/trace_event/memory...
File base/trace_event/memory_dump_manager.cc (right):

https://codereview.chromium.org/2822593002/diff/20001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:774: result =
MemoryDumpCallbackResult();
On 2017/04/19 15:59:56, fmeawad wrote:
> On 2017/04/19 12:03:45, Primiano Tucci wrote:
> > why not just doing this directly up on line 745 when declaring the var,
i.e.:
> > 
> > base::Optional<MemoryDumpCallbackResult> result(MemoryDumpCallbackResult());
> > 
> > from the code looks like this if is going to be always taken right? (the
> > process_dumps maps should contain always at least one argument)
> 
> I do see code paths where this is never reached, but if the logic is that it
has
> to have at least one argument then I will change it.

Ahh sorry I didn't see the == DETAILED check right above. you are right. Ignore
my comment

[fmeawad, 2017-04-19 21:07:21]

Ken: PTAL

https://codereview.chromium.org/2822593002/diff/20001/mojo/common/process_id....
File mojo/common/process_id.mojom (right):

https://codereview.chromium.org/2822593002/diff/20001/mojo/common/process_id....
mojo/common/process_id.mojom:8: // This is the storage for the pid, on windows
tje pid is a DWORD an the
On 2017/04/18 22:11:24, Ken Rockot wrote:
> nit: tje=the, an=and

Done.

https://codereview.chromium.org/2822593002/diff/20001/mojo/common/process_id....
File mojo/common/process_id.typemap (right):

https://codereview.chromium.org/2822593002/diff/20001/mojo/common/process_id....
mojo/common/process_id.typemap:8: "//ipc/ipc_message_utils.h",
On 2017/04/18 22:11:25, Ken Rockot wrote:
> not used

Done.

https://codereview.chromium.org/2822593002/diff/20001/mojo/common/process_id....
mojo/common/process_id.typemap:12: "//ipc",
On 2017/04/18 22:11:24, Ken Rockot wrote:
> not used

Done.

https://codereview.chromium.org/2822593002/diff/20001/mojo/common/test_common...
File mojo/common/test_common_custom_types.mojom (right):

https://codereview.chromium.org/2822593002/diff/20001/mojo/common/test_common...
mojo/common/test_common_custom_types.mojom:26: interface TestProcessId {
On 2017/04/18 22:11:25, Ken Rockot wrote:
> Please avoid copying this pattern for testing new typemaps. Please see
>
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-mojo/_m5qIDZgAf8
> - TLDR is you can just directly Serialize and Deserialize a base::ProcessId
> to/from a mojo::common::mojom::ProcessId without using a new mojom interface.

Done.

[fmeawad, 2017-04-19 21:07:49]

The CQ bit was checked by fmeawad@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-19 21:08:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ken Rockot(use gerrit already), 2017-04-19 21:19:38]

https://codereview.chromium.org/2822593002/diff/40001/mojo/common/test_common...
File mojo/common/test_common_custom_types.mojom (right):

https://codereview.chromium.org/2822593002/diff/40001/mojo/common/test_common...
mojo/common/test_common_custom_types.mojom:9: import
"mojo/common/process_id.mojom";
nit: not used

https://codereview.chromium.org/2822593002/diff/40001/services/resource_coord...
File
services/resource_coordinator/public/cpp/memory/memory_instrumentation_struct_traits.h
(right):

https://codereview.chromium.org/2822593002/diff/40001/services/resource_coord...
services/resource_coordinator/public/cpp/memory/memory_instrumentation_struct_traits.h:100:
static std::map<base::ProcessId,
This should return a const ref, otherwise it will be copied (twice, in fact)
unnecessarily.

[fmeawad, 2017-04-19 21:40:21]

Addressed

https://codereview.chromium.org/2822593002/diff/40001/mojo/common/test_common...
File mojo/common/test_common_custom_types.mojom (right):

https://codereview.chromium.org/2822593002/diff/40001/mojo/common/test_common...
mojo/common/test_common_custom_types.mojom:9: import
"mojo/common/process_id.mojom";
On 2017/04/19 21:19:37, Ken Rockot wrote:
> nit: not used

It was left there intentionally instead of adding the .mojom.h in the unittest
file. But I understand that this would be confusing, removed.

https://codereview.chromium.org/2822593002/diff/40001/services/resource_coord...
File
services/resource_coordinator/public/cpp/memory/memory_instrumentation_struct_traits.h
(right):

https://codereview.chromium.org/2822593002/diff/40001/services/resource_coord...
services/resource_coordinator/public/cpp/memory/memory_instrumentation_struct_traits.h:100:
static std::map<base::ProcessId,
On 2017/04/19 21:19:37, Ken Rockot wrote:
> This should return a const ref, otherwise it will be copied (twice, in fact)
> unnecessarily.

Done.

[Ken Rockot(use gerrit already), 2017-04-19 21:54:42]

On 2017/04/19 at 21:40:21, fmeawad wrote:
> Addressed
> 
>
https://codereview.chromium.org/2822593002/diff/40001/mojo/common/test_common...
> File mojo/common/test_common_custom_types.mojom (right):
> 
>
https://codereview.chromium.org/2822593002/diff/40001/mojo/common/test_common...
> mojo/common/test_common_custom_types.mojom:9: import
"mojo/common/process_id.mojom";
> On 2017/04/19 21:19:37, Ken Rockot wrote:
> > nit: not used
> 
> It was left there intentionally instead of adding the .mojom.h in the unittest
file. But I understand that this would be confusing, removed.

I see. To be sure, that would definitely not be a good reason to leave the
import there. It's not correct to include something somewhere only to satisfy an
indirect dependency.

> 
>
https://codereview.chromium.org/2822593002/diff/40001/services/resource_coord...
> File
services/resource_coordinator/public/cpp/memory/memory_instrumentation_struct_traits.h
(right):
> 
>
https://codereview.chromium.org/2822593002/diff/40001/services/resource_coord...
>
services/resource_coordinator/public/cpp/memory/memory_instrumentation_struct_traits.h:100:
static std::map<base::ProcessId,
> On 2017/04/19 21:19:37, Ken Rockot wrote:
> > This should return a const ref, otherwise it will be copied (twice, in fact)
> > unnecessarily.
> 
> Done.

Hmm. Still not returning a ref

[fmeawad, 2017-04-19 22:17:19]

On 2017/04/19 21:54:42, Ken Rockot wrote:
> On 2017/04/19 at 21:40:21, fmeawad wrote:
> > Addressed
> > 
> >
>
https://codereview.chromium.org/2822593002/diff/40001/mojo/common/test_common...
> > File mojo/common/test_common_custom_types.mojom (right):
> > 
> >
>
https://codereview.chromium.org/2822593002/diff/40001/mojo/common/test_common...
> > mojo/common/test_common_custom_types.mojom:9: import
> "mojo/common/process_id.mojom";
> > On 2017/04/19 21:19:37, Ken Rockot wrote:
> > > nit: not used
> > 
> > It was left there intentionally instead of adding the .mojom.h in the
unittest
> file. But I understand that this would be confusing, removed.
> 
> I see. To be sure, that would definitely not be a good reason to leave the
> import there. It's not correct to include something somewhere only to satisfy
an
> indirect dependency.
> 
> > 
> >
>
https://codereview.chromium.org/2822593002/diff/40001/services/resource_coord...
> > File
>
services/resource_coordinator/public/cpp/memory/memory_instrumentation_struct_traits.h
> (right):
> > 
> >
>
https://codereview.chromium.org/2822593002/diff/40001/services/resource_coord...
> >
>
services/resource_coordinator/public/cpp/memory/memory_instrumentation_struct_traits.h:100:
> static std::map<base::ProcessId,
> > On 2017/04/19 21:19:37, Ken Rockot wrote:
> > > This should return a const ref, otherwise it will be copied (twice, in
fact)
> > > unnecessarily.
> > 
> > Done.
> 
> Hmm. Still not returning a ref
Sorry about that, fixed.

[Ken Rockot(use gerrit already), 2017-04-19 22:19:33]

Thanks! LGTM

[fmeawad, 2017-04-19 22:26:16]

fmeawad@chromium.org changed reviewers:
+ mkwst@chromium.org

[fmeawad, 2017-04-19 22:26:17]

+mkwst for IPC review/mojom

[Mike West, 2017-04-20 07:22:33]

LGTM

[fmeawad, 2017-04-20 14:36:40]

The CQ bit was checked by fmeawad@chromium.org

[fmeawad, 2017-04-20 14:36:41]

The patchset sent to the CQ was uploaded after l-g-t-m from
primiano@chromium.org, hjd@chromium.org
Link to the patchset: https://codereview.chromium.org/2822593002/#ps80001
(title: "Fixing the return type (adding the missing ref)")

[commit-bot: I haz the power, 2017-04-20 14:37:13]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-20 15:42:07]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1492699000779430,
"parent_rev": "cebec16b950f85d2808eac2ff103f6e6ac1f1823", "commit_rev":
"33bb7a42b9ea6c9138ac975e35b0242b9da21c07"}

[commit-bot: I haz the power, 2017-04-20 15:42:14]

Description was changed from

==========
[Memory UMA] Return the extra_processes_dump map as part of the ResponseCallback

This CL is a follow up CL to http://crrev.com/277709300 where we add the missing
extra_processes_dump map from the MemoryDumpCallbackResult struct into its
corresponding mojo struct.

BUG=703184
==========

to

==========
[Memory UMA] Return the extra_processes_dump map as part of the ResponseCallback

This CL is a follow up CL to http://crrev.com/277709300 where we add the missing
extra_processes_dump map from the MemoryDumpCallbackResult struct into its
corresponding mojo struct.

BUG=703184

Review-Url: https://codereview.chromium.org/2822593002
Cr-Commit-Position: refs/heads/master@{#466021}
Committed:
https://chromium.googlesource.com/chromium/src/+/33bb7a42b9ea6c9138ac975e35b0...
==========

[commit-bot: I haz the power, 2017-04-20 15:42:15]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/33bb7a42b9ea6c9138ac975e35b0...

[xlai (Olivia), 2017-04-20 17:04:27]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/2828333002/ by xlai@chromium.org.

The reason for reverting is: Suspecting CL that caused compile failure in 
https://build.chromium.org/p/chromium/builders/Win/builds/54274.

Need to monitor whether the Win build goes back to green after reverting..