Bug 72316 - [MutationObservers] Add histogram collection for usage of DOM Mutation Events
Summary: [MutationObservers] Add histogram collection for usage of DOM Mutation Events
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Rafael Weinstein
URL:
Keywords:
Depends on:
Blocks: 68729 73026
  Show dependency treegraph
 
Reported: 2011-11-14 15:15 PST by Rafael Weinstein
Modified: 2011-11-23 10:04 PST (History)
7 users (show)

See Also:


Attachments
Patch (2.93 KB, patch)
2011-11-14 15:19 PST, Rafael Weinstein
no flags Details | Formatted Diff | Diff
Patch (3.03 KB, patch)
2011-11-14 15:48 PST, Rafael Weinstein
no flags Details | Formatted Diff | Diff
Patch (2.98 KB, patch)
2011-11-14 16:34 PST, Rafael Weinstein
no flags Details | Formatted Diff | Diff
Patch (3.09 KB, patch)
2011-11-14 16:53 PST, Rafael Weinstein
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rafael Weinstein 2011-11-14 15:15:51 PST
So we can make an informed decision at some point about removing their support
Comment 1 Rafael Weinstein 2011-11-14 15:19:21 PST
Created attachment 115041 [details]
Patch
Comment 2 Early Warning System Bot 2011-11-14 15:23:48 PST
Comment on attachment 115041 [details]
Patch

Attachment 115041 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10485078
Comment 3 Ryosuke Niwa 2011-11-14 15:27:07 PST
Comment on attachment 115041 [details]
Patch

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

> Source/WebCore/dom/Document.cpp:499
> +static void histogramMutationEventUsage(const unsigned short& listenerTypes)
> +{
> +    PlatformSupport::histogramEnumeration("DOMAPI.PerDocumentMutationEventUsage.DOMSubtreeModified", listenerTypes & Document::DOMSUBTREEMODIFIED_LISTENER, 2);

I don't these functions are available on non-Chromium ports. You need to wrap it in #if PLATFORM(CHROMIUM)
Comment 4 Rafael Weinstein 2011-11-14 15:48:34 PST
Created attachment 115048 [details]
Patch
Comment 5 Rafael Weinstein 2011-11-14 15:52:57 PST
Comment on attachment 115041 [details]
Patch

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

>> Source/WebCore/dom/Document.cpp:499
>> +    PlatformSupport::histogramEnumeration("DOMAPI.PerDocumentMutationEventUsage.DOMSubtreeModified", listenerTypes & Document::DOMSUBTREEMODIFIED_LISTENER, 2);
> 
> I don't these functions are available on non-Chromium ports. You need to wrap it in #if PLATFORM(CHROMIUM)

ifdef'd
Comment 6 Ryosuke Niwa 2011-11-14 16:06:33 PST
Comment on attachment 115048 [details]
Patch

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

> Source/WebCore/dom/Document.cpp:118
> +#if PLATFORM(CHROMIUM)
> +#include "PlatformSupport.h"
> +#endif

This should be listed separately see below for examples.
Comment 7 Rafael Weinstein 2011-11-14 16:34:10 PST
Created attachment 115055 [details]
Patch
Comment 8 Rafael Weinstein 2011-11-14 16:34:27 PST
Comment on attachment 115048 [details]
Patch

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

>> Source/WebCore/dom/Document.cpp:118
>> +#endif
> 
> This should be listed separately see below for examples.

done
Comment 9 Rafael Weinstein 2011-11-14 16:53:46 PST
Created attachment 115061 [details]
Patch
Comment 10 Rafael Weinstein 2011-11-14 16:55:03 PST
Sorry. I realized that I don't want to collect the bit in each of these cases, I actually want a 1 or a 0.
Comment 11 WebKit Review Bot 2011-11-14 17:23:53 PST
Comment on attachment 115061 [details]
Patch

Clearing flags on attachment: 115061

Committed r100222: <http://trac.webkit.org/changeset/100222>
Comment 12 WebKit Review Bot 2011-11-14 17:24:03 PST
All reviewed patches have been landed.  Closing bug.
Comment 13 Sam Weinig 2011-11-15 11:31:18 PST
It seems unfortunate adding platform specific code to Document.cpp.  Is there another cross-platform way to do this?
Comment 14 Ojan Vafai 2011-11-15 11:34:35 PST
(In reply to comment #13)
> It seems unfortunate adding platform specific code to Document.cpp.  Is there another cross-platform way to do this?

We could have a stub implementation of PlatformSupport.h for other platforms? This would be similar to the other *Client interfaces we have.
Comment 15 Rafael Weinstein 2011-11-15 13:27:36 PST
I'm happy to do whatever you guys think is most appropriate.
Comment 16 Adam Barth 2011-11-15 13:34:35 PST
IMHO, we should have a header that exposes only the histogram functions which has stub implementations on non-Chromium platforms.  The Chromium implementation would just trampoline through PlatformSupport.
Comment 17 Sam Weinig 2011-11-16 11:37:16 PST
(In reply to comment #16)
> IMHO, we should have a header that exposes only the histogram functions which has stub implementations on non-Chromium platforms.  The Chromium implementation would just trampoline through PlatformSupport.

I think this would be the right approach.
Comment 18 Rafael Weinstein 2011-11-23 10:04:52 PST
opened 73026 for cleanup