[Webkit-unassigned] [Bug 109884] Add CSS Property tracking to FeatureObserver. Creates new histogram for CSS Property usage data.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=109884


Ojan Vafai <ojan at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #194779|review?                     |review-
               Flag|                            |




--- Comment #22 from Ojan Vafai <ojan at chromium.org>  2013-03-25 16:43:35 PST ---
(From update of attachment 194779)
View in context: https://bugs.webkit.org/attachment.cgi?id=194779&action=review

> Source/WebCore/css/CSSParser.cpp:1327
> +    FeatureObserver::observe(m_context.m_document.get(), propertyID);

Don't we crash if you use a parser context created with the two argument constructor?

> Source/WebCore/page/FeatureObserver.cpp:41
> +static int mapCSSPropertyId(int id)
> +{
> +    switch (id) {

I'm not a huge fan of adding another switch statement that we need to update every time we add/remove a CSS property. The switch statement is fine, but lets only put the properties in this switch statement that we actually care to measure. Specifically, lets just put the WebKit prefixed properties for now since those are the properties we'd like to kill if they have sufficiently low usage.

> Source/WebCore/page/FeatureObserver.cpp:529
> +    if (flushCSSResults)
> +        HistogramSupport::histogramEnumeration("WebCore.FeatureObserver.CSSProperties", cssFlushPropertyId(), maximumCSSPropertyId());

I don't really follow what the flushing is all about. Can you add a description of this to the ChangeLog?

> Source/WebCore/page/FeatureObserver.h:137
> +    BitVector m_CSSFeatureBits;

Should this also be OwnPtr like m_featureBits? Then you also can initialize it lazily.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.