[tracing] Add SkResourceCache statistics to chrome://tracing (chrome side)

[tracing] Add SkResourceCache statistics to chrome://tracing (chrome side)

This CL adds the statistics of SkResourceCache to tracing.
See crrev.com/1313793004 for the skia side changes.

BUG=503168
Committed: https://crrev.com/70405030c538d9075a7ddaf7ba212e4be3b558cf
Cr-Commit-Position: refs/heads/master@{#347207}

[ssid, 2015-08-25 19:51:09]

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

[ssid, 2015-08-25 19:51:10]

PTAL, Thanks

[ericrk, 2015-08-25 20:14:50]

Looks good overall - a few comments.

https://codereview.chromium.org/1319473003/diff/80001/skia/ext/SkTraceMemoryD...
File skia/ext/SkTraceMemoryDump_chrome.cc (right):

https://codereview.chromium.org/1319473003/diff/80001/skia/ext/SkTraceMemoryD...
skia/ext/SkTraceMemoryDump_chrome.cc:34: auto mad =
process_memory_dump_->CreateAllocatorDump(dumpName);
Don't we want to find the existing allocator dump, like in dumpNumericValue?

https://codereview.chromium.org/1319473003/diff/80001/skia/ext/SkTraceMemoryD...
skia/ext/SkTraceMemoryDump_chrome.cc:45: reinterpret_cast<const
SkDiscardableMemoryChrome&>(
shouldn't we static_cast here?

nit: can we add a comment that there are no other implementations of
SkDiscardableMemory used by Skia when used with Chrome, so we can safely cast?

https://codereview.chromium.org/1319473003/diff/80001/skia/ext/SkTraceMemoryD...
skia/ext/SkTraceMemoryDump_chrome.cc:47: auto mad =
memory.CreateMemoryAllocatorDump(dumpName, process_memory_dump_);
same as above, should this try to find an existing dump? Should we add a helper
function "CreateOrFindAllocatorDump" or something?

https://codereview.chromium.org/1319473003/diff/80001/skia/ext/SkTraceMemoryD...
File skia/ext/SkTraceMemoryDump_chrome.h (right):

https://codereview.chromium.org/1319473003/diff/80001/skia/ext/SkTraceMemoryD...
skia/ext/SkTraceMemoryDump_chrome.h:8: #include
"third_party/skia/include/core/SkTraceMemoryDump.h"
nit: Combine / alphabetize includes here?

https://codereview.chromium.org/1319473003/diff/80001/skia/ext/SkTraceMemoryD...
skia/ext/SkTraceMemoryDump_chrome.h:18: namespace skia {
SkDiscardableMemory_Chrome isn't in a skia namespace, but this is, why the
distinction? Not a big deal, just want to make sure we follow the existing
pattern if there is one.

[ssid, 2015-08-25 20:43:54]

https://codereview.chromium.org/1319473003/diff/80001/skia/ext/SkTraceMemoryD...
File skia/ext/SkTraceMemoryDump_chrome.cc (right):

https://codereview.chromium.org/1319473003/diff/80001/skia/ext/SkTraceMemoryD...
skia/ext/SkTraceMemoryDump_chrome.cc:34: auto mad =
process_memory_dump_->CreateAllocatorDump(dumpName);
On 2015/08/25 20:14:50, ericrk wrote:
> Don't we want to find the existing allocator dump, like in dumpNumericValue?

This method will be called only once with a given name. Maybe it should be
enforced that first we set the backing and then add attributes?

https://codereview.chromium.org/1319473003/diff/80001/skia/ext/SkTraceMemoryD...
skia/ext/SkTraceMemoryDump_chrome.cc:45: reinterpret_cast<const
SkDiscardableMemoryChrome&>(
On 2015/08/25 20:14:50, ericrk wrote:
> shouldn't we static_cast here?
> 
> nit: can we add a comment that there are no other implementations of
> SkDiscardableMemory used by Skia when used with Chrome, so we can safely cast?

Yes, it should be static_cast.

https://codereview.chromium.org/1319473003/diff/80001/skia/ext/SkTraceMemoryD...
skia/ext/SkTraceMemoryDump_chrome.cc:47: auto mad =
memory.CreateMemoryAllocatorDump(dumpName, process_memory_dump_);
On 2015/08/25 20:14:49, ericrk wrote:
> same as above, should this try to find an existing dump? Should we add a
helper
> function "CreateOrFindAllocatorDump" or something?

hm, the issue is, this goes all the way into implementations of
base::DiscardableMemory. All of these asume that the dump does not exist. So, I
think this should also assume that the dump does not exist already. But, maybe
the setMemoryBacking can be changed to use getOrCreate.

https://codereview.chromium.org/1319473003/diff/80001/skia/ext/SkTraceMemoryD...
File skia/ext/SkTraceMemoryDump_chrome.h (right):

https://codereview.chromium.org/1319473003/diff/80001/skia/ext/SkTraceMemoryD...
skia/ext/SkTraceMemoryDump_chrome.h:18: namespace skia {
On 2015/08/25 20:14:50, ericrk wrote:
> SkDiscardableMemory_Chrome isn't in a skia namespace, but this is, why the
> distinction? Not a big deal, just want to make sure we follow the existing
> pattern if there is one.

um, I am not really sure, there are other files which use skia namespace, in
skia/ext. Just thought it is better to have namespace anyway.

[ericrk, 2015-08-25 20:49:56]

https://codereview.chromium.org/1319473003/diff/80001/skia/ext/SkTraceMemoryD...
File skia/ext/SkTraceMemoryDump_chrome.cc (right):

https://codereview.chromium.org/1319473003/diff/80001/skia/ext/SkTraceMemoryD...
skia/ext/SkTraceMemoryDump_chrome.cc:34: auto mad =
process_memory_dump_->CreateAllocatorDump(dumpName);
On 2015/08/25 at 20:43:54, ssid wrote:
> On 2015/08/25 20:14:50, ericrk wrote:
> > Don't we want to find the existing allocator dump, like in dumpNumericValue?
> 
> This method will be called only once with a given name. Maybe it should be
enforced that first we set the backing and then add attributes?

Fair enough... it's not really clear from the function names that there's a hard
order for calling these - but would rather not rename at this point.

Can we DCHECK(!GetAllocatorDump(dumpName)) to ensure that you call this first.

https://codereview.chromium.org/1319473003/diff/80001/skia/ext/SkTraceMemoryD...
skia/ext/SkTraceMemoryDump_chrome.cc:47: auto mad =
memory.CreateMemoryAllocatorDump(dumpName, process_memory_dump_);
On 2015/08/25 at 20:43:54, ssid wrote:
> On 2015/08/25 20:14:49, ericrk wrote:
> > same as above, should this try to find an existing dump? Should we add a
helper
> > function "CreateOrFindAllocatorDump" or something?
> 
> hm, the issue is, this goes all the way into implementations of
base::DiscardableMemory. All of these asume that the dump does not exist. So, I
think this should also assume that the dump does not exist already. But, maybe
the setMemoryBacking can be changed to use getOrCreate.

See my comment above... I'm OK with a DCHECK.

https://codereview.chromium.org/1319473003/diff/80001/skia/ext/SkTraceMemoryD...
File skia/ext/SkTraceMemoryDump_chrome.h (right):

https://codereview.chromium.org/1319473003/diff/80001/skia/ext/SkTraceMemoryD...
skia/ext/SkTraceMemoryDump_chrome.h:18: namespace skia {
On 2015/08/25 at 20:43:54, ssid wrote:
> On 2015/08/25 20:14:50, ericrk wrote:
> > SkDiscardableMemory_Chrome isn't in a skia namespace, but this is, why the
> > distinction? Not a big deal, just want to make sure we follow the existing
> > pattern if there is one.
> 
> um, I am not really sure, there are other files which use skia namespace, in
skia/ext. Just thought it is better to have namespace anyway.

Ok, that's fine. Let's leave this as-is. I prefer the namespace.

[ssid, 2015-08-25 21:03:24]

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

[ssid, 2015-08-25 21:03:25]

+primiano

In the discardable memory case I had assumed that the skia is not going to add
size. So, it is optional for skia to add attributes. So, it should be made clear
in the SkTraceDump that discardable backing is not for existing dumps, while
setMemoryBacking is for existing dumps?

https://codereview.chromium.org/1319473003/diff/80001/skia/ext/SkTraceMemoryD...
File skia/ext/SkTraceMemoryDump_chrome.cc (right):

https://codereview.chromium.org/1319473003/diff/80001/skia/ext/SkTraceMemoryD...
skia/ext/SkTraceMemoryDump_chrome.cc:34: auto mad =
process_memory_dump_->CreateAllocatorDump(dumpName);
On 2015/08/25 20:49:56, ericrk wrote:
> On 2015/08/25 at 20:43:54, ssid wrote:
> > On 2015/08/25 20:14:50, ericrk wrote:
> > > Don't we want to find the existing allocator dump, like in
dumpNumericValue?
> > 
> > This method will be called only once with a given name. Maybe it should be
> enforced that first we set the backing and then add attributes?
> 
> Fair enough... it's not really clear from the function names that there's a
hard
> order for calling these - but would rather not rename at this point.
> 
> Can we DCHECK(!GetAllocatorDump(dumpName)) to ensure that you call this first.

Hm, you were right, the comment in the api file says this if for existing dumps.
So this should be changed to use get-or-create. But discardable is a special
case here.

[ssid, 2015-08-26 14:17:54]

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

[ssid, 2015-08-26 14:17:54]

Had a chat offline, and we keep the setBacking api same and the discardable is
treated as special case. Later maybe we can change the api interface to reflect
this sense.

Is everything else fine with the impl?

[Primiano Tucci (use gerrit), 2015-08-26 15:34:00]

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

[Primiano Tucci (use gerrit), 2015-08-26 15:34:01]

some comment here and there but in general LGTM.
Wait for erivrk to confirm though

https://codereview.chromium.org/1319473003/diff/120001/skia/ext/SkTraceMemory...
File skia/ext/SkTraceMemoryDump_chrome.cc (right):

https://codereview.chromium.org/1319473003/diff/120001/skia/ext/SkTraceMemory...
skia/ext/SkTraceMemoryDump_chrome.cc:36: mad =
process_memory_dump_->CreateAllocatorDump(dumpName);
do you need the GetOrCreate semantic also here? if so you could extract a
GetOrCreateAllocatorDump method

https://codereview.chromium.org/1319473003/diff/120001/skia/ext/SkTraceMemory...
skia/ext/SkTraceMemoryDump_chrome.cc:47: const SkDiscardableMemoryChrome& memory
=
why this dcheck. don't you want the GetOrCreate semantic here? Why hard failing
if you already have a dump (which was intended to be the original plan)?

https://codereview.chromium.org/1319473003/diff/120001/skia/ext/SkTraceMemory...
skia/ext/SkTraceMemoryDump_chrome.cc:50: DCHECK(mad);
please s/memory/discardable_memory_obj/ or similar

https://codereview.chromium.org/1319473003/diff/120001/skia/ext/SkTraceMemory...
File skia/ext/SkTraceMemoryDump_chrome.h (right):

https://codereview.chromium.org/1319473003/diff/120001/skia/ext/SkTraceMemory...
skia/ext/SkTraceMemoryDump_chrome.h:20: public:
I'd add a comment to this class specifying that this shoyuld never outlive the
outer OnMemoryDump call (the one coming from MemoryDumpManager), because the
ProcessMemoryDump is valid onnly in that timeframe.

https://codereview.chromium.org/1319473003/diff/120001/skia/ext/SkTraceMemory...
skia/ext/SkTraceMemoryDump_chrome.h:23: ~SkTraceMemoryDump_Chrome() override;
just to doublecheck, i think that (probably in a future cl) ericrk will need an
extra constructor to pass more stuff (to get the right guid), right?

https://codereview.chromium.org/1319473003/diff/120001/skia/ext/skia_memory_d...
File skia/ext/skia_memory_dump_provider.cc (right):

https://codereview.chromium.org/1319473003/diff/120001/skia/ext/skia_memory_d...
skia/ext/skia_memory_dump_provider.cc:49:
SkResourceCache::DumpMemoryStatistics(&skia_dumper);
s/skia_dumper/skia_memoryinfra_wrapper/ or something similar. that obj is not a
skia dumper, jusdt a wrapper interfacve.

[ssid, 2015-08-26 15:45:19]

Thanks.
Sorry I had updated the CL while you were reviewing. Mostly the changes you
suggested.

https://codereview.chromium.org/1319473003/diff/120001/skia/ext/SkTraceMemory...
File skia/ext/SkTraceMemoryDump_chrome.cc (right):

https://codereview.chromium.org/1319473003/diff/120001/skia/ext/SkTraceMemory...
skia/ext/SkTraceMemoryDump_chrome.cc:36: mad =
process_memory_dump_->CreateAllocatorDump(dumpName);
On 2015/08/26 15:34:01, Primiano Tucci wrote:
> do you need the GetOrCreate semantic also here? if so you could extract a
> GetOrCreateAllocatorDump method

Ah, sorry. I just updated the CL. after i sent for review.

https://codereview.chromium.org/1319473003/diff/120001/skia/ext/SkTraceMemory...
skia/ext/SkTraceMemoryDump_chrome.cc:47: const SkDiscardableMemoryChrome& memory
=
On 2015/08/26 15:34:01, Primiano Tucci wrote:
> why this dcheck. don't you want the GetOrCreate semantic here? Why hard
failing
> if you already have a dump (which was intended to be the original plan)?

Hm, hard failing here because it will fail inside the discardable manager which
creates dump. Instead of failing at a random place, failing here is better to
debug. This is what I was discussing with you offline.

https://codereview.chromium.org/1319473003/diff/120001/skia/ext/SkTraceMemory...
skia/ext/SkTraceMemoryDump_chrome.cc:50: DCHECK(mad);
On 2015/08/26 15:34:01, Primiano Tucci wrote:
> please s/memory/discardable_memory_obj/ or similar

Done.

https://codereview.chromium.org/1319473003/diff/120001/skia/ext/SkTraceMemory...
File skia/ext/SkTraceMemoryDump_chrome.h (right):

https://codereview.chromium.org/1319473003/diff/120001/skia/ext/SkTraceMemory...
skia/ext/SkTraceMemoryDump_chrome.h:20: public:
On 2015/08/26 15:34:01, Primiano Tucci wrote:
> I'd add a comment to this class specifying that this shoyuld never outlive the
> outer OnMemoryDump call (the one coming from MemoryDumpManager), because the
> ProcessMemoryDump is valid onnly in that timeframe.

Done.

https://codereview.chromium.org/1319473003/diff/120001/skia/ext/SkTraceMemory...
skia/ext/SkTraceMemoryDump_chrome.h:23: ~SkTraceMemoryDump_Chrome() override;
On 2015/08/26 15:34:01, Primiano Tucci wrote:
> just to doublecheck, i think that (probably in a future cl) ericrk will need
an
> extra constructor to pass more stuff (to get the right guid), right?

Yes constructor and setMemoryBacking will be changed by eric later for his
purpose.

[ericrk, 2015-08-26 18:12:06]

LGTM - my only comment is that I think I'll need to subclass this, both to keep
the base impl simple as well as to avoid circular build dependencies. Can we add
a private getter for the pmd? If you'd prefer, I can add that in a future CL.

Thanks!

https://codereview.chromium.org/1319473003/diff/120001/skia/ext/SkTraceMemory...
File skia/ext/SkTraceMemoryDump_chrome.h (right):

https://codereview.chromium.org/1319473003/diff/120001/skia/ext/SkTraceMemory...
skia/ext/SkTraceMemoryDump_chrome.h:23: ~SkTraceMemoryDump_Chrome() override;
On 2015/08/26 at 15:34:01, Primiano Tucci wrote:
> just to doublecheck, i think that (probably in a future cl) ericrk will need
an extra constructor to pass more stuff (to get the right guid), right?

Yeah, will likely need this... that said, I think I may just override this class
to keep things simple in the base. Would only need to override constructor and
setMemoryBacking.

https://codereview.chromium.org/1319473003/diff/160001/skia/ext/SkTraceMemory...
File skia/ext/SkTraceMemoryDump_chrome.h (right):

https://codereview.chromium.org/1319473003/diff/160001/skia/ext/SkTraceMemory...
skia/ext/SkTraceMemoryDump_chrome.h:41: base::trace_event::ProcessMemoryDump*
process_memory_dump_;
As I think subclassing will lead to the most sane approach here, can you add a
protected getter for this? I can also add this in my future CL.

[ssid, 2015-08-26 18:36:46]

ssid@chromium.org changed reviewers:
+ reed@google.com

[ssid, 2015-08-26 18:36:46]

+reed
This CL makes the chrome side changes for implementing the SkTraceDump.
PTAL. Thanks

[ssid, 2015-08-27 14:18:55]

This is chrome side changes for tracing. PTAL, thanks.

[Primiano Tucci (use gerrit), 2015-08-30 18:28:36]

some extra small comments on the new patchset, LGTM still holds from my
viewpoint.

https://codereview.chromium.org/1319473003/diff/220001/skia/ext/SkTraceMemory...
File skia/ext/SkTraceMemoryDump_chrome.cc (right):

https://codereview.chromium.org/1319473003/diff/220001/skia/ext/SkTraceMemory...
skia/ext/SkTraceMemoryDump_chrome.cc:37: auto dump =
GetOrCreateAllocatorDump(dumpName);
why do we allow case insensitive compare here? I'd really like to avoid that the
code on the skia side can say either "malloc" or "MAllOc" unless there is a very
strong reason fgor that. it is harder to maintain and spot bugs otherwise.

https://codereview.chromium.org/1319473003/diff/220001/skia/ext/SkTraceMemory...
skia/ext/SkTraceMemoryDump_chrome.cc:42: }
also, please add a else{ NOTREACHED()} section at the end, so we are sure we
catch cases where skia code sets the backing to something which does not exist.

https://codereview.chromium.org/1319473003/diff/220001/skia/ext/SkTraceMemory...
skia/ext/SkTraceMemoryDump_chrome.cc:50: auto dump =
discardable_memory_obj.CreateMemoryAllocatorDump(
why are you not using the GetOrCreateAllocatorDump here?
Looking at this code, it seems that this will fail you first call
dumpNumericValue and then setDiscardableMemoryBacking (which, I know, probably
you will not do but would be nice if the api was supporting this, given the
name).
That is the reason why in the previous patchset I suggested to extract the
GetOrAllocate method.
Now you extracted the method but are ysing only in one place, which ironically
doesn't justify anymore the extra method.
Unless I am missing something, I'd put a call to GetOrCreate here.

https://codereview.chromium.org/1319473003/diff/220001/skia/ext/SkTraceMemory...
File skia/ext/SkTraceMemoryDump_chrome.h (right):

https://codereview.chromium.org/1319473003/diff/220001/skia/ext/SkTraceMemory...
skia/ext/SkTraceMemoryDump_chrome.h:24: SkTraceMemoryDump_Chrome(
typo: s/timefrace/timeframe/

https://codereview.chromium.org/1319473003/diff/220001/skia/ext/skia_memory_d...
File skia/ext/skia_memory_dump_provider.cc (right):

https://codereview.chromium.org/1319473003/diff/220001/skia/ext/skia_memory_d...
skia/ext/skia_memory_dump_provider.cc:30:
process_memory_dump->CreateAllocatorDump("skia/sk_glyph_cache");
tip: since you are wrapping on a newline here, you could expand the auto in the
actual name, which makes the code easier to read.
ditto below

[ssid, 2015-08-30 23:11:24]

I will make the style changes, thanks.

https://codereview.chromium.org/1319473003/diff/220001/skia/ext/SkTraceMemory...
File skia/ext/SkTraceMemoryDump_chrome.cc (right):

https://codereview.chromium.org/1319473003/diff/220001/skia/ext/SkTraceMemory...
skia/ext/SkTraceMemoryDump_chrome.cc:37: auto dump =
GetOrCreateAllocatorDump(dumpName);
On 2015/08/30 18:28:36, Primiano Tucci wrote:
> why do we allow case insensitive compare here? I'd really like to avoid that
the
> code on the skia side can say either "malloc" or "MAllOc" unless there is a
very
> strong reason fgor that. it is harder to maintain and spot bugs otherwise.

I mainly made it because skia could say malloc or Malloc. If you want to avoid
that I'm fine. I will change to sensitive compare.

https://codereview.chromium.org/1319473003/diff/220001/skia/ext/SkTraceMemory...
skia/ext/SkTraceMemoryDump_chrome.cc:50: auto dump =
discardable_memory_obj.CreateMemoryAllocatorDump(
On 2015/08/30 18:28:36, Primiano Tucci wrote:
> why are you not using the GetOrCreateAllocatorDump here?
> Looking at this code, it seems that this will fail you first call
> dumpNumericValue and then setDiscardableMemoryBacking (which, I know, probably
> you will not do but would be nice if the api was supporting this, given the
> name).
> That is the reason why in the previous patchset I suggested to extract the
> GetOrAllocate method.
> Now you extracted the method but are ysing only in one place, which ironically
> doesn't justify anymore the extra method.
> Unless I am missing something, I'd put a call to GetOrCreate here.

Yes it will fail if skia first calls dump and then tells discardable backed.
This is a special case. And I and Eric are the ones writing code and we will
keep that in mind while making dump provides.
No I cannot use get or create here.
Reason: this api is calling into
base::DiscardableMemory::allocateMemoryAllocatorDump. This api exposed by
discardable dumps with sizes. This is the reason why I wanted to change the set
discardable backing api to dump discardable backed memory api. But, you had
agreed that we will make this change later and not now.
I have extracted the function because it's used at two places already.

If you would like to add support for get our create from discardable memory api,
then please file a bug and I will change the discardable api and implementations
to use getOrCreate.

[ssid, 2015-09-02 12:59:52]

Made all changes suggested / discussed.

reed@ Please take a look.

ericrk@ I added a new constructor for appending prefix, maybe useful to your
work as well.

https://codereview.chromium.org/1319473003/diff/220001/skia/ext/SkTraceMemory...
File skia/ext/SkTraceMemoryDump_chrome.cc (right):

https://codereview.chromium.org/1319473003/diff/220001/skia/ext/SkTraceMemory...
skia/ext/SkTraceMemoryDump_chrome.cc:42: }
On 2015/08/30 18:28:36, Primiano Tucci wrote:
> also, please add a else{ NOTREACHED()} section at the end, so we are sure we
> catch cases where skia code sets the backing to something which does not
exist.

Done.

https://codereview.chromium.org/1319473003/diff/220001/skia/ext/SkTraceMemory...
File skia/ext/SkTraceMemoryDump_chrome.h (right):

https://codereview.chromium.org/1319473003/diff/220001/skia/ext/SkTraceMemory...
skia/ext/SkTraceMemoryDump_chrome.h:24: SkTraceMemoryDump_Chrome(
On 2015/08/30 18:28:36, Primiano Tucci wrote:
> typo: s/timefrace/timeframe/

Done.

https://codereview.chromium.org/1319473003/diff/220001/skia/ext/skia_memory_d...
File skia/ext/skia_memory_dump_provider.cc (right):

https://codereview.chromium.org/1319473003/diff/220001/skia/ext/skia_memory_d...
skia/ext/skia_memory_dump_provider.cc:30:
process_memory_dump->CreateAllocatorDump("skia/sk_glyph_cache");
On 2015/08/30 18:28:36, Primiano Tucci wrote:
> tip: since you are wrapping on a newline here, you could expand the auto in
the
> actual name, which makes the code easier to read.
> ditto below 

Done.

[reed1, 2015-09-02 15:03:26]

rubber-stamp lgtm

[ericrk, 2015-09-02 17:35:24]

On 2015/09/02 12:59:52, ssid wrote:
> Made all changes suggested / discussed.
> 
> reed@ Please take a look.
> 
> ericrk@ I added a new constructor for appending prefix, maybe useful to your
> work as well.
> 
>
https://codereview.chromium.org/1319473003/diff/220001/skia/ext/SkTraceMemory...
> File skia/ext/SkTraceMemoryDump_chrome.cc (right):
> 
>
https://codereview.chromium.org/1319473003/diff/220001/skia/ext/SkTraceMemory...
> skia/ext/SkTraceMemoryDump_chrome.cc:42: }
> On 2015/08/30 18:28:36, Primiano Tucci wrote:
> > also, please add a else{ NOTREACHED()} section at the end, so we are sure we
> > catch cases where skia code sets the backing to something which does not
> exist.
> 
> Done.
> 
>
https://codereview.chromium.org/1319473003/diff/220001/skia/ext/SkTraceMemory...
> File skia/ext/SkTraceMemoryDump_chrome.h (right):
> 
>
https://codereview.chromium.org/1319473003/diff/220001/skia/ext/SkTraceMemory...
> skia/ext/SkTraceMemoryDump_chrome.h:24: SkTraceMemoryDump_Chrome(
> On 2015/08/30 18:28:36, Primiano Tucci wrote:
> > typo: s/timefrace/timeframe/
> 
> Done.
> 
>
https://codereview.chromium.org/1319473003/diff/220001/skia/ext/skia_memory_d...
> File skia/ext/skia_memory_dump_provider.cc (right):
> 
>
https://codereview.chromium.org/1319473003/diff/220001/skia/ext/skia_memory_d...
> skia/ext/skia_memory_dump_provider.cc:30:
> process_memory_dump->CreateAllocatorDump("skia/sk_glyph_cache");
> On 2015/08/30 18:28:36, Primiano Tucci wrote:
> > tip: since you are wrapping on a newline here, you could expand the auto in
> the
> > actual name, which makes the code easier to read.
> > ditto below 
> 
> Done.

Still LGTM - curious about the constructor for adding a prefix - you don't seem
to be using it, and I'm not sure that I'll end up using it, so I'm wondering
what the expected use case is?

[ssid, 2015-09-02 17:37:26]

On 2015/09/02 17:35:24, ericrk wrote:
> On 2015/09/02 12:59:52, ssid wrote:
> > Made all changes suggested / discussed.
> > 
> > reed@ Please take a look.
> > 
> > ericrk@ I added a new constructor for appending prefix, maybe useful to your
> > work as well.
> > 
> >
>
https://codereview.chromium.org/1319473003/diff/220001/skia/ext/SkTraceMemory...
> > File skia/ext/SkTraceMemoryDump_chrome.cc (right):
> > 
> >
>
https://codereview.chromium.org/1319473003/diff/220001/skia/ext/SkTraceMemory...
> > skia/ext/SkTraceMemoryDump_chrome.cc:42: }
> > On 2015/08/30 18:28:36, Primiano Tucci wrote:
> > > also, please add a else{ NOTREACHED()} section at the end, so we are sure
we
> > > catch cases where skia code sets the backing to something which does not
> > exist.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/1319473003/diff/220001/skia/ext/SkTraceMemory...
> > File skia/ext/SkTraceMemoryDump_chrome.h (right):
> > 
> >
>
https://codereview.chromium.org/1319473003/diff/220001/skia/ext/SkTraceMemory...
> > skia/ext/SkTraceMemoryDump_chrome.h:24: SkTraceMemoryDump_Chrome(
> > On 2015/08/30 18:28:36, Primiano Tucci wrote:
> > > typo: s/timefrace/timeframe/
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/1319473003/diff/220001/skia/ext/skia_memory_d...
> > File skia/ext/skia_memory_dump_provider.cc (right):
> > 
> >
>
https://codereview.chromium.org/1319473003/diff/220001/skia/ext/skia_memory_d...
> > skia/ext/skia_memory_dump_provider.cc:30:
> > process_memory_dump->CreateAllocatorDump("skia/sk_glyph_cache");
> > On 2015/08/30 18:28:36, Primiano Tucci wrote:
> > > tip: since you are wrapping on a newline here, you could expand the auto
in
> > the
> > > actual name, which makes the code easier to read.
> > > ditto below 
> > 
> > Done.
> 
> Still LGTM - curious about the constructor for adding a prefix - you don't
seem
> to be using it, and I'm not sure that I'll end up using it, so I'm wondering
> what the expected use case is?

image pixelref objects allocated by skia should be accounted for blink's
webcache and not under skia since skia does not manage the resources, just knows
about the sizes. So, adding prefix of blink/web_cache to them.

[ericrk, 2015-09-02 22:32:00]

On 2015/09/02 17:37:26, ssid wrote:
> On 2015/09/02 17:35:24, ericrk wrote:
> > On 2015/09/02 12:59:52, ssid wrote:
> > > Made all changes suggested / discussed.
> > > 
> > > reed@ Please take a look.
> > > 
> > > ericrk@ I added a new constructor for appending prefix, maybe useful to
your
> > > work as well.
> > > 
> > >
> >
>
https://codereview.chromium.org/1319473003/diff/220001/skia/ext/SkTraceMemory...
> > > File skia/ext/SkTraceMemoryDump_chrome.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1319473003/diff/220001/skia/ext/SkTraceMemory...
> > > skia/ext/SkTraceMemoryDump_chrome.cc:42: }
> > > On 2015/08/30 18:28:36, Primiano Tucci wrote:
> > > > also, please add a else{ NOTREACHED()} section at the end, so we are
sure
> we
> > > > catch cases where skia code sets the backing to something which does not
> > > exist.
> > > 
> > > Done.
> > > 
> > >
> >
>
https://codereview.chromium.org/1319473003/diff/220001/skia/ext/SkTraceMemory...
> > > File skia/ext/SkTraceMemoryDump_chrome.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1319473003/diff/220001/skia/ext/SkTraceMemory...
> > > skia/ext/SkTraceMemoryDump_chrome.h:24: SkTraceMemoryDump_Chrome(
> > > On 2015/08/30 18:28:36, Primiano Tucci wrote:
> > > > typo: s/timefrace/timeframe/
> > > 
> > > Done.
> > > 
> > >
> >
>
https://codereview.chromium.org/1319473003/diff/220001/skia/ext/skia_memory_d...
> > > File skia/ext/skia_memory_dump_provider.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1319473003/diff/220001/skia/ext/skia_memory_d...
> > > skia/ext/skia_memory_dump_provider.cc:30:
> > > process_memory_dump->CreateAllocatorDump("skia/sk_glyph_cache");
> > > On 2015/08/30 18:28:36, Primiano Tucci wrote:
> > > > tip: since you are wrapping on a newline here, you could expand the auto
> in
> > > the
> > > > actual name, which makes the code easier to read.
> > > > ditto below 
> > > 
> > > Done.
> > 
> > Still LGTM - curious about the constructor for adding a prefix - you don't
> seem
> > to be using it, and I'm not sure that I'll end up using it, so I'm wondering
> > what the expected use case is?
> 
> image pixelref objects allocated by skia should be accounted for blink's
> webcache and not under skia since skia does not manage the resources, just
knows
> about the sizes. So, adding prefix of blink/web_cache to them.

Makes sense. Still LGTM

[ssid, 2015-09-03 19:03:45]

The CQ bit was checked by ssid@chromium.org

[ssid, 2015-09-03 19:03:45]

The patchset sent to the CQ was uploaded after l-g-t-m from
primiano@chromium.org, ericrk@chromium.org, reed@google.com
Link to the patchset: https://codereview.chromium.org/1319473003/#ps330008
(title: "Mac build fix.")

[commit-bot: I haz the power, 2015-09-03 19:03:59]

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

[commit-bot: I haz the power, 2015-09-03 19:10:21]

Committed patchset #21 (id:330008)

[commit-bot: I haz the power, 2015-09-03 19:10:52]

Patchset 21 (id:??) landed as
https://crrev.com/70405030c538d9075a7ddaf7ba212e4be3b558cf
Cr-Commit-Position: refs/heads/master@{#347207}