Bug 72316 – [MutationObservers] Add histogram collection for usage of DOM Mutation Events

• Component: DOM
• Status: RESOLVED FIXED

[Rafael Weinstein]

So we can make an informed decision at some point about removing their support

[Rafael Weinstein]

Created attachment 115041 [details]
Patch

[Early Warning System Bot]

Comment on attachment 115041 [details]
Patch

Attachment 115041 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10485078

[Ryosuke Niwa]

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)

[Rafael Weinstein]

Created attachment 115048 [details]
Patch

[Rafael Weinstein]

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

[Ryosuke Niwa]

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.

[Rafael Weinstein]

Created attachment 115055 [details]
Patch

[Rafael Weinstein]

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

[Rafael Weinstein]

Created attachment 115061 [details]
Patch

[Rafael Weinstein]

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.

[WebKit Review Bot]

Comment on attachment 115061 [details]
Patch

Clearing flags on attachment: 115061

Committed r100222: <http://trac.webkit.org/changeset/100222>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Sam Weinig]

It seems unfortunate adding platform specific code to Document.cpp.  Is there another cross-platform way to do this?

[Ojan Vafai]

(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.

[Rafael Weinstein]

I'm happy to do whatever you guys think is most appropriate.

[Adam Barth]

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.

[Sam Weinig]

(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.

[Rafael Weinstein]

opened 73026 for cleanup