Implement GetSkiaTraceMemoryDump and AddSuballocation on chrome blink platform impl

Implement GetSkiaTraceMemoryDump and AddSuballocation on chrome blink platform impl

This is Chrome side implementation of GetSkiaTraceMemoryDump and
AddSuballocation interface added in
www.crrev.com/1313223007.

BUG=520842
Committed: https://crrev.com/207a9def5f4b68549e6ac143237292a0eb7f3bf8
Cr-Commit-Position: refs/heads/master@{#351643}

[ssid, 2015-09-04 08:52:39]

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

[ssid, 2015-09-04 08:52:39]

PTAL.

[ssid, 2015-09-04 08:54:00]

https://codereview.chromium.org/1308523004/diff/1/content/child/web_process_m...
File content/child/web_process_memory_dump_impl.cc (right):

https://codereview.chromium.org/1308523004/diff/1/content/child/web_process_m...
content/child/web_process_memory_dump_impl.cc:9: #include
"skia/ext/SkTraceMemoryDump_chrome.h"
Also, deps addition is not required, it is already added in parent directory
DEPS :)

[Primiano Tucci (use gerrit), 2015-09-04 10:02:17]

https://codereview.chromium.org/1308523004/diff/1/content/child/web_process_m...
File content/child/web_process_memory_dump_impl.cc (right):

https://codereview.chromium.org/1308523004/diff/1/content/child/web_process_m...
content/child/web_process_memory_dump_impl.cc:138: sk_trace_dump_.reset(new
skia::SkTraceMemoryDump_Chrome(
Hmm if you store it into a scoped_ptr implies that you can create only one
adapter (with one prefix) at all for each dump. Isn't this a bit limiting? 
I think the options here are:
 A. You keep the memory ownership of the skia wrappers here, but make
sk_trace_Dump a ScopedVector instead.

 B. You move the memory ownership of this to the caller (so return a naked
pointer) and the caller has the responsibility of owning and destroying properly
the wrapper.

I'd go for A as, IIRC, the skia wrapper cannot outlive the process memory dump.
BUt please document that in the comment of Create... method.

https://codereview.chromium.org/1308523004/diff/1/content/child/web_process_m...
File content/child/web_process_memory_dump_impl.h (right):

https://codereview.chromium.org/1308523004/diff/1/content/child/web_process_m...
content/child/web_process_memory_dump_impl.h:60: virtual SkTraceMemoryDump*
CreateSkiaTraceMemoryDump(
I'd probably call this CreateDumpAdapterForSkia(dumpNamePrefix) 
and add a comment like

// Creates an instance of the SkTraceMemoryDump proxy interface that can be
passed to Skia to dump into this ProcessMemoryDump.

[ssid, 2015-09-07 17:29:53]

I have also added AddSuballocation. See
https://codereview.chromium.org/1327793003/ for the use case. PTAL, thanks.

https://codereview.chromium.org/1308523004/diff/1/content/child/web_process_m...
File content/child/web_process_memory_dump_impl.cc (right):

https://codereview.chromium.org/1308523004/diff/1/content/child/web_process_m...
content/child/web_process_memory_dump_impl.cc:138: sk_trace_dump_.reset(new
skia::SkTraceMemoryDump_Chrome(
On 2015/09/04 10:02:16, Primiano Tucci wrote:
> Hmm if you store it into a scoped_ptr implies that you can create only one
> adapter (with one prefix) at all for each dump. Isn't this a bit limiting? 
> I think the options here are:
>  A. You keep the memory ownership of the skia wrappers here, but make
> sk_trace_Dump a ScopedVector instead.
> 
>  B. You move the memory ownership of this to the caller (so return a naked
> pointer) and the caller has the responsibility of owning and destroying
properly
> the wrapper.
> 
> I'd go for A as, IIRC, the skia wrapper cannot outlive the process memory
dump.
> BUt please document that in the comment of Create... method.

Done.

https://codereview.chromium.org/1308523004/diff/1/content/child/web_process_m...
File content/child/web_process_memory_dump_impl.h (right):

https://codereview.chromium.org/1308523004/diff/1/content/child/web_process_m...
content/child/web_process_memory_dump_impl.h:60: virtual SkTraceMemoryDump*
CreateSkiaTraceMemoryDump(
On 2015/09/04 10:02:16, Primiano Tucci wrote:
> I'd probably call this CreateDumpAdapterForSkia(dumpNamePrefix) 
> and add a comment like
> 
> // Creates an instance of the SkTraceMemoryDump proxy interface that can be
> passed to Skia to dump into this ProcessMemoryDump.

Done.

[Primiano Tucci (use gerrit), 2015-09-09 16:16:42]

LGTM with some minor comments.
Also please just few lines lines to the commit message to explain what this 
SkTrace...adapter is for.

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

https://codereview.chromium.org/1308523004/diff/40001/content/child/web_proce...
content/child/web_process_memory_dump_impl.cc:148: dumpNamePrefix.utf8().data(),
process_memory_dump_));
I realized this just now, but would have been a bit clearer if 
SkTraceMemoryDump_Chrome took a std::string& (that ctor is internal to chrome
and not exposed to skia, right?)
By looking at the header it is not clear whether the char* string is deep-copied
or you take the ptr.
If it was the latter this would have been wrong. But I checked the code and you
actually copy there the char* into a std::String.

If you get a change to re-iterate on that file, you should probably make the
prefix argument a std::string& (unless there is something here I'm not thingking
about).

This LGTM anyways.

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

https://codereview.chromium.org/1308523004/diff/40001/content/child/web_proce...
content/child/web_process_memory_dump_impl.h:92: // Stores SkTraceMemoryDump for
the current ProcessMemoryDump.
Please say: this class handles the memory ownership of the SkTraceMemoryDump
instace returned. It is guaranteed to be valid through all the lifetime of
WebProcessMemoryDumpImpl.

[Primiano Tucci (use gerrit), 2015-09-09 17:21:36]

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

https://codereview.chromium.org/1308523004/diff/40001/content/child/web_proce...
content/child/web_process_memory_dump_impl.cc:145:
blink::WebMemoryDumpLevelOfDetail levelOfDetail,
Wait I realized only now: you don't seem to make any use of levelOfDetail. Why
is this an rgument at all? Don't you knoe the level of detail int he current
WPMDImpl?

Let's have a quick chat on this, I think you can remove this argument both from
here and the other (blink) CL.

[ssid, 2015-09-10 11:08:25]

ssid@chromium.org changed reviewers:
+ jochen@chromium.org

[ssid, 2015-09-10 11:08:26]

+jochen
PTAL, thanks

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

https://codereview.chromium.org/1308523004/diff/40001/content/child/web_proce...
content/child/web_process_memory_dump_impl.cc:145:
blink::WebMemoryDumpLevelOfDetail levelOfDetail,
On 2015/09/09 17:21:36, Primiano Tucci wrote:
> Wait I realized only now: you don't seem to make any use of levelOfDetail. Why
> is this an rgument at all? Don't you knoe the level of detail int he current
> WPMDImpl?
> 
> Let's have a quick chat on this, I think you can remove this argument both
from
> here and the other (blink) CL.
I have uploaded the Cl in skia impl to add one more arg. So, I have added it
here since I don't have to change the blink code. I could just add it to this
class instead, so adding it here. Updated the skia construct as well.

https://codereview.chromium.org/1308523004/diff/40001/content/child/web_proce...
content/child/web_process_memory_dump_impl.cc:148: dumpNamePrefix.utf8().data(),
process_memory_dump_));
On 2015/09/09 16:16:41, Primiano Tucci wrote:
> I realized this just now, but would have been a bit clearer if 
> SkTraceMemoryDump_Chrome took a std::string& (that ctor is internal to chrome
> and not exposed to skia, right?)
> By looking at the header it is not clear whether the char* string is
deep-copied
> or you take the ptr.
> If it was the latter this would have been wrong. But I checked the code and
you
> actually copy there the char* into a std::String.
> 
> If you get a change to re-iterate on that file, you should probably make the
> prefix argument a std::string& (unless there is something here I'm not
thingking
> about).
> 
> This LGTM anyways.

hm, okay. Will fix this in next CL. Thanks

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

https://codereview.chromium.org/1308523004/diff/40001/content/child/web_proce...
content/child/web_process_memory_dump_impl.h:92: // Stores SkTraceMemoryDump for
the current ProcessMemoryDump.
On 2015/09/09 16:16:42, Primiano Tucci wrote:
> Please say: this class handles the memory ownership of the SkTraceMemoryDump
> instace returned. It is guaranteed to be valid through all the lifetime of
> WebProcessMemoryDumpImpl.

Done.

[Primiano Tucci (use gerrit), 2015-09-10 11:11:13]

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

https://codereview.chromium.org/1308523004/diff/60001/content/child/web_proce...
content/child/web_process_memory_dump_impl.h:85: const
base::trace_event::MemoryDumpArgs* dump_args_;
since MDArgs is very small and cheap to copy, can we keep a copy here? Having to
keep track of the lifetime of yet another method makes me a bit nervous? thanks
:)
Also, I think you want to store just levelofdetail, not the full args. Would
make the cl clearer.

For the default ctor case, it's fine setting them to low, as this will get fixed
soon anyways.

[ssid, 2015-09-10 11:22:05]

On 2015/09/10 11:11:13, Primiano Tucci wrote:
>
https://codereview.chromium.org/1308523004/diff/60001/content/child/web_proce...
> File content/child/web_process_memory_dump_impl.h (right):
> 
>
https://codereview.chromium.org/1308523004/diff/60001/content/child/web_proce...
> content/child/web_process_memory_dump_impl.h:85: const
> base::trace_event::MemoryDumpArgs* dump_args_;
> since MDArgs is very small and cheap to copy, can we keep a copy here? Having
to
> keep track of the lifetime of yet another method makes me a bit nervous?
thanks
> :)
> Also, I think you want to store just levelofdetail, not the full args. Would
> make the cl clearer.
> 
> For the default ctor case, it's fine setting them to low, as this will get
fixed
> soon anyways.

Done. Thanks.

[Primiano Tucci (use gerrit), 2015-09-10 11:43:41]

Patchset 7 LGTM, thanks

[jochen (gone - plz use gerrit), 2015-09-14 09:37:08]

lgtm

[ssid, 2015-09-30 12:44:27]

Patchset #8 (id:140001) has been deleted

[ssid, 2015-09-30 12:44:43]

Patchset #8 (id:160001) has been deleted

[ssid, 2015-09-30 12:46:21]

Patchset #8 (id:180001) has been deleted

[ssid, 2015-09-30 20:03:44]

Patchset #8 (id:200001) has been deleted

[ssid, 2015-09-30 21:26:01]

The CQ bit was checked by ssid@chromium.org

[ssid, 2015-09-30 21:26:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
primiano@chromium.org, jochen@chromium.org
Link to the patchset:
https://chromiumcodereview.appspot.com/1308523004/#ps220001 (title: "rebase.")

[commit-bot: I haz the power, 2015-09-30 21:27:00]

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

[commit-bot: I haz the power, 2015-09-30 21:33:50]

Committed patchset #8 (id:220001)

[commit-bot: I haz the power, 2015-09-30 21:34:26]

Patchset 8 (id:??) landed as
https://crrev.com/207a9def5f4b68549e6ac143237292a0eb7f3bf8
Cr-Commit-Position: refs/heads/master@{#351643}