memory-infra: Fill the memory dump callback result (2/2)

memory-infra: Fill the memory dump callback result (2/2)

Fill in the part of the memory dump result struct
that applies to OS's view of Chrome when finalizing
a memory dump.

BUG=703184
Review-Url: https://codereview.chromium.org/2766173003
Cr-Commit-Position: refs/heads/master@{#462849}
Committed: https://chromium.googlesource.com/chromium/src/+/22fd5f0cf18a409dd72b96cb526a25a6cac88d24

[hjd, 2017-03-22 12:33:17]

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

[hjd, 2017-03-22 12:33:18]

ptal, thanks!

primiano@, can we take a look at the numbers together at somepoint I'm not sure
they are right.

[hjd, 2017-03-23 11:27:43]

Description was changed from

==========
Fill second half of the memory dump result struct

Fill in the part of the memory dump result struct
that applies to OS's veiw of Chrome when finalizing
a memory dump.

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

to

==========
memory-infra: Fill the memory dump callback result (2/2)

Fill in the part of the memory dump result struct
that applies to OS's veiw of Chrome when finalizing
a memory dump.

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

[hjd, 2017-03-23 11:27:52]

Description was changed from

==========
memory-infra: Fill the memory dump callback result (2/2)

Fill in the part of the memory dump result struct
that applies to OS's veiw of Chrome when finalizing
a memory dump.

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

to

==========
memory-infra: Fill the memory dump callback result (2/2)

Fill in the part of the memory dump result struct
that applies to OS's view of Chrome when finalizing
a memory dump.

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

[hjd, 2017-03-23 17:43:11]

On 2017/03/22 12:33:18, hjd wrote:
> ptal, thanks!
> 
> primiano@, can we take a look at the numbers together at somepoint I'm not
sure
> they are right.

Update as discussed offline :)

[Primiano Tucci (use gerrit), 2017-03-24 11:44:43]

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

https://codereview.chromium.org/2766173003/diff/40001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:736: void
MemoryDumpManager::FillOsDumpFromProcessMemoryDump(
move this to the anonymous namespace (and remove the decl from the header)

https://codereview.chromium.org/2766173003/diff/40001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:806:
FillOsDumpFromProcessMemoryDump(os_dump, process_memory_dump);
I think we want to do this also in the case above (pid == kNull)

[hjd, 2017-03-24 12:12:54]

Thanks!

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

https://codereview.chromium.org/2766173003/diff/40001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:736: void
MemoryDumpManager::FillOsDumpFromProcessMemoryDump(
On 2017/03/24 11:44:43, Primiano Tucci wrote:
> move this to the anonymous namespace (and remove the decl from the header)

Done.

https://codereview.chromium.org/2766173003/diff/40001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:806:
FillOsDumpFromProcessMemoryDump(os_dump, process_memory_dump);
On 2017/03/24 11:44:43, Primiano Tucci wrote:
> I think we want to do this also in the case above (pid == kNull)

oops, I must have lost it in a rebase, thanks!

[Primiano Tucci (use gerrit), 2017-04-03 08:49:27]

LGTM w/ comment addressed

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

https://codereview.chromium.org/2766173003/diff/60001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:96: MemoryDumpCallbackResult::OSMemDump&
osDump,
output arguments should:
1. use pointers, not references. references can be used only for input.
Specifically our style guide says:
"All parameters passed by reference must be labeled const." [1]
2. be at the end of the argument list "When ordering function parameters, put
all input-only parameters before any output parameters." [2]

On top of this, when you pass input arguments as pointers, I find "const" to
still help readability
Also variables/args should be lower_with_under [3] (osDump -> os_dump)

so this should be:
FillOsDumpFromProcessMemoryDump(const ProcessMemoryDump* pmd,
MemoryDumpCallbackResult::OSMemDump* os_dump) {
...
}


[1] https://google.github.io/styleguide/cppguide.html#Reference_Arguments
[2]
https://google.github.io/styleguide/cppguide.html#Function_Parameter_Ordering
[3] https://google.github.io/styleguide/cppguide.html#Variable_Names

https://codereview.chromium.org/2766173003/diff/60001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:99: auto* totals =
pmd->process_totals();
minor thing about "auto". There are no official rules on this, but my rule of
thumb is that if the explicit type fits still in one line (80 cols) I find the
overall code more readable / codesearcheable. Here I'd do

const ProcessMemoryTotals* totals = pmd->process_totals();

[hjd, 2017-04-07 12:56:51]

The CQ bit was checked by hjd@chromium.org

[hjd, 2017-04-07 12:56:51]

The patchset sent to the CQ was uploaded after l-g-t-m from
primiano@chromium.org
Link to the patchset: https://codereview.chromium.org/2766173003/#ps100001
(title: "add a const version")

[commit-bot: I haz the power, 2017-04-07 12:57:07]

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-07 14:28:41]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1491569811267930,
"parent_rev": "42cd1de28a6495c29e355dd4114f83ebb74a71bd", "commit_rev":
"22fd5f0cf18a409dd72b96cb526a25a6cac88d24"}

[commit-bot: I haz the power, 2017-04-07 14:29:31]

Description was changed from

==========
memory-infra: Fill the memory dump callback result (2/2)

Fill in the part of the memory dump result struct
that applies to OS's view of Chrome when finalizing
a memory dump.

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

to

==========
memory-infra: Fill the memory dump callback result (2/2)

Fill in the part of the memory dump result struct
that applies to OS's view of Chrome when finalizing
a memory dump.

BUG=703184

Review-Url: https://codereview.chromium.org/2766173003
Cr-Commit-Position: refs/heads/master@{#462849}
Committed:
https://chromium.googlesource.com/chromium/src/+/22fd5f0cf18a409dd72b96cb526a...
==========

[commit-bot: I haz the power, 2017-04-07 14:29:34]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/22fd5f0cf18a409dd72b96cb526a...

[hjd, 2017-04-07 15:10:30]

A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.chromium.org/2806553005/ by hjd@chromium.org.

The reason for reverting is: Causing build failure:
https://uberchromegw.corp.google.com/i/chromium.linux/builders/Linux%20Tests/....