Bug 105261 – Web Inspector: show cached images under MemoryCache -> Images section

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Yury Semikhatsky]

Native memory profiler should show specific CachedImages with their sizes under Memory cache resources: Image section so that user can figure out which images make up the cache.

[Yury Semikhatsky]

Created attachment 179891 [details]
Patch

[Alexei Filippov]

Comment on attachment 179891 [details]
Patch

lgtm

[Alexei Filippov]

Comment on attachment 179891 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=179891&action=review

> Source/WebCore/inspector/front-end/NativeMemorySnapshotView.js:272
> +    _addChildrenFromGraph: function(memoryBlock)

Missing function annotations here and below.

> Source/WebCore/inspector/front-end/NativeMemorySnapshotView.js:294
> +        for (var i = 0; i < edges.length; i++) {
> +            var target = edges[i].target();
> +            if (target.className() === "CachedImage") {

Perhaps a helper iterator function forAllChildrenOfClass?

> Source/WebCore/inspector/front-end/NativeMemorySnapshotView.js:302
> +                if (image.className() === "BitmapImage") {

What about other classes? Later?

> Source/WebCore/inspector/front-end/NativeMemorySnapshotView.js:439
> +        return this.targetsOfAllEdges(edgeName)[0];

Pure functional code doesn't work well in JS. ;-)
This one doesn't seem too effective. Add a TODO/FIXME?

[Yury Semikhatsky]

(In reply to comment #3)
> (From update of attachment 179891 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=179891&action=review
> 
> > Source/WebCore/inspector/front-end/NativeMemorySnapshotView.js:272
> > +    _addChildrenFromGraph: function(memoryBlock)
> 
> Missing function annotations here and below.
> 
Fixed.

> > Source/WebCore/inspector/front-end/NativeMemorySnapshotView.js:294
> > +        for (var i = 0; i < edges.length; i++) {
> > +            var target = edges[i].target();
> > +            if (target.className() === "CachedImage") {
> 
> Perhaps a helper iterator function forAllChildrenOfClass?
> 
I'd change that when we have another client for the code to better understand which API we need.

> > Source/WebCore/inspector/front-end/NativeMemorySnapshotView.js:302
> > +                if (image.className() === "BitmapImage") {
> 
> What about other classes? Later?
> 
Yes, it is a first step. Show just referenced pixel buffer which we know may be quite big. If you feel that we should show some other objects please speak up.

> > Source/WebCore/inspector/front-end/NativeMemorySnapshotView.js:439
> > +        return this.targetsOfAllEdges(edgeName)[0];
> 
> Pure functional code doesn't work well in JS. ;-)
> This one doesn't seem too effective. Add a TODO/FIXME?
We can optimize this code when we find it inefficient for the current use case it seems to be good enough.

[Yury Semikhatsky]

Created attachment 179910 [details]
Patch

[WebKit Review Bot]

Comment on attachment 179910 [details]
Patch

Clearing flags on attachment: 179910

Committed r138005: <http://trac.webkit.org/changeset/138005>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.