Bug 80787

Created attachment 131224 [details] Patch

Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.

Comment on attachment 131224 [details] Patch Attachment 131224 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11935479

Comment on attachment 131224 [details] Patch Attachment 131224 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/11936427

Comment on attachment 131224 [details] Patch Attachment 131224 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/11934506

Comment on attachment 131224 [details] Patch Attachment 131224 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/11938482

Comment on attachment 131224 [details] Patch Attachment 131224 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/11940442

Comment on attachment 131224 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=131224&action=review > Source/WebCore/page/MemoryInfo.cpp:64 > + return PlatformSupport::processPrivateMemorySize(); I.e. total/user heap is static, while this private memory is alive on a given instance? We should be consistent, preserve console.memory singleton and make all the properties of the performance.* consistent? > Source/WebCore/page/MemoryInfo.h:61 > + RefPtr<Frame> m_frame; You are at risk of leaking the frame here.

Created attachment 131225 [details] Patch

(In reply to comment #8) > (From update of attachment 131224 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=131224&action=review > > > Source/WebCore/page/MemoryInfo.cpp:64 > > + return PlatformSupport::processPrivateMemorySize(); > > I.e. total/user heap is static, while this private memory is alive on a given instance? We should be consistent, preserve console.memory singleton and make all the properties of the performance.* consistent? > Done. Private and shared memory usage is calculated on MemoryInfo construction(which is consistent with JS heap size calculation). > > Source/WebCore/page/MemoryInfo.h:61 > > + RefPtr<Frame> m_frame; > > You are at risk of leaking the frame here. Got rid of RefPtr here as all data are now collected in the constructor.

Comment on attachment 131225 [details] Patch Attachment 131225 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11933552

Comment on attachment 131225 [details] Patch Attachment 131225 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/11933554

Created attachment 131226 [details] Patch for landing Fixed gtk port.

Comment on attachment 131225 [details] Patch Attachment 131225 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11936459 New failing tests: fast/dom/Window/window-properties-performance.html

Comment on attachment 131225 [details] Patch Attachment 131225 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/11938494

Comment on attachment 131226 [details] Patch for landing Attachment 131226 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11935526 New failing tests: fast/dom/Window/window-properties-performance.html

Comment on attachment 131226 [details] Patch for landing Attachment 131226 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11935552 New failing tests: fast/dom/Window/window-properties-performance.html

(In reply to comment #2) > Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API. This patch does not appear to have been reviewed by fishd. Please don't make changes to the Chromium public API without his review. (If he reviewed this patch in another forum, it would be helpful to note that somewhere in the bug.)

Comment on attachment 131226 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=131226&action=review > Source/WebCore/page/MemoryInfo.cpp:53 > : m_totalJSHeapSize(0), > m_usedJSHeapSize(0), > - m_jsHeapSizeLimit(0) > + m_jsHeapSizeLimit(0), > + m_processPrivateMemorySize(0), > + m_processSharedMemorySize(0) This initializer list isn't in the correct style. The "," should be vertically aligned with the ":" > Source/WebCore/page/MemoryInfo.idl:42 > + readonly attribute unsigned long long processPrivateMemorySize; > + readonly attribute unsigned long long processSharedMemorySize; Should these APIs only be exposed in PLATFORM(CHROMIUM) since they'll always be zero elsewhere? Should we implement them on other platforms as well? Is there a spec somewhere for this interface?

Created attachment 131289 [details] Patch

(In reply to comment #18) > (In reply to comment #2) > > Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API. > > This patch does not appear to have been reviewed by fishd. Please don't make changes to the Chromium public API without his review. (If he reviewed this patch in another forum, it would be helpful to note that somewhere in the bug.) Of cause we will wait for fishd@ approval before submitting as advised by the WebKit Review Bot. (In reply to comment #19) > (From update of attachment 131226 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=131226&action=review > > > Source/WebCore/page/MemoryInfo.cpp:53 > > : m_totalJSHeapSize(0), > > m_usedJSHeapSize(0), > > - m_jsHeapSizeLimit(0) > > + m_jsHeapSizeLimit(0), > > + m_processPrivateMemorySize(0), > > + m_processSharedMemorySize(0) > > This initializer list isn't in the correct style. The "," should be vertically aligned with the ":" > Fixed. > > Source/WebCore/page/MemoryInfo.idl:42 > > + readonly attribute unsigned long long processPrivateMemorySize; > > + readonly attribute unsigned long long processSharedMemorySize; > > Should these APIs only be exposed in PLATFORM(CHROMIUM) since they'll always be zero elsewhere? Should we implement them on other platforms as well? I don't think we should hide these attribute behind PLATFORM(CHROMIUM) guard. I don't see any properties exposed in PLATFORM(FOO) in any other WebCore idls. MemoryInfo.jsHeapSizeLimit in MemoryInfo.idl doesn't make sense for JSC and is still exposed there without preprocessor guards, though with a custom getter. I think the proper way would be to have it implemented on other platforms as well, but I would leave it to the platform maintainers. > Is there a spec somewhere for this interface? No that I know of. This instrumentation is still experimental and controlled by a setting which is disabled by default(can be enabled in Chromium by passing --enable-memory-info command line switch). It may make sense to have it a part of Web Performnce API eventually. Tony Gentilcore should know better if someone worked on the spec in that direction.

> > Is there a spec somewhere for this interface? > No that I know of. This instrumentation is still experimental and controlled by a setting which is disabled by default(can be enabled in Chromium by passing --enable-memory-info command line switch). It may make sense to have it a part of Web Performnce API eventually. What happened here? It looks to me like these are public now... My command line (from about:version): /Applications/Google Chrome.app/Contents/MacOS/Google Chrome -psn_0_172074 --flag-switches-begin --enable-print-preview --enable-sync-tabs --flag-switches-end My console: > window.performance.memory.usedJSHeapSize 8117948 > Tony Gentilcore should know better if someone worked on the spec in that direction. My understanding was that this was completely abandoned because of the security/privacy objections in: https://lists.webkit.org/pipermail/webkit-dev/2010-June/013053.html I'm quite surprised to see that this API appears to be public and to see new things being added to it.

(In reply to comment #22) > > > Is there a spec somewhere for this interface? > > No that I know of. This instrumentation is still experimental and controlled by a setting which is disabled by default(can be enabled in Chromium by passing --enable-memory-info command line switch). It may make sense to have it a part of Web Performnce API eventually. > > What happened here? It looks to me like these are public now... > > My command line (from about:version): > /Applications/Google Chrome.app/Contents/MacOS/Google Chrome -psn_0_172074 --flag-switches-begin --enable-print-preview --enable-sync-tabs --flag-switches-end > > My console: > > window.performance.memory.usedJSHeapSize > 8117948 > This option must be enabled with a local policy for your browser which can override command line switches.

Google enables it on all machines on the corporate network. Bug 80444 is also related in that it tries to address some of the security concerns.

(In reply to comment #24) > Google enables it on all machines on the corporate network. Whew, thanks for clearing that up for me. > Bug 80444 is also related in that it tries to address some of the security concerns. Oh, cool idea. Do you expect this will help get us to a point where this could be exposed to the web platform? In any case, how do you think we should be proceeding with this interface? Since it is disabled by default, does that mean we shouldn't bother with a vendor prefix? A counterpoint would be that if we are able to expose it to the web in the future, we might already be somewhat locked in to the non-prefixed version, so maybe it is better to have the prefix now. Also, should we be documenting this in a spec in parallel with the implementation?

Comment on attachment 131289 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=131289&action=review > Source/WebKit/chromium/public/platform/WebKitPlatformSupport.h:197 > + virtual bool getProcessMemorySize(size_t* privateBytes, size_t* sharedBytes) { return false; } nit: please preserve the two new lines before a section header.

> > Bug 80444 is also related in that it tries to address some of the security concerns. > > Oh, cool idea. Do you expect this will help get us to a point where this could be exposed to the web platform? That's the goal. It's still behind a flag, but we're going to experiment with turning that flag on by default. > In any case, how do you think we should be proceeding with this interface? Since it is disabled by default, does that mean we shouldn't bother with a vendor prefix? A counterpoint would be that if we are able to expose it to the web in the future, we might already be somewhat locked in to the non-prefixed version, so maybe it is better to have the prefix now. Also, should we be documenting this in a spec in parallel with the implementation? Either Yury or I should put up a proposal on the WhatWG wiki http://wiki.whatwg.org/wiki/Main_Page in the proposals category. This doesn't need to be much, but it should say what each of the properties are called and what they mean. That's something we're planning to do as part of Bug 80444, but it's relevant here too.

> Either Yury or I should put up a proposal on the WhatWG wiki http://wiki.whatwg.org/wiki/Main_Page in the proposals category. This doesn't need to be much, but it should say what each of the properties are called and what they mean. That's something we're planning to do as part of Bug 80444, but it's relevant here too. Sounds like a great idea. I'd be interested in seeing that and some plan regarding vendor prefixes along with patches to the memory interface like this one. You might also consider sending a link to that wiki entry to public-web-perf@w3.org. That's where ongoing additions to window.performance are being specced and I'm sure those folks would be very interested in this as well.

Comment on attachment 131289 [details] Patch Sounds like there's some more standards work to be done before we land this patch.

Closing as invalid, as this bug pertains to the old inspector UI and/or its tests. Please file a new bug (https://www.webkit.org/new-inspector-bug) if the bug/feature/issue is still relevant to WebKit trunk.