[webkit-reviews] review denied: [Bug 109884] Add CSS Property tracking to FeatureObserver. Creates new histogram for CSS Property usage data. : [Attachment 194779] Corrected ChangeLog

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has denied Kassy Coan <kassycoan at gmail.com>'s
request for review:
Bug 109884: Add CSS Property tracking to FeatureObserver. Creates new histogram
for CSS Property usage data.
https://bugs.webkit.org/show_bug.cgi?id=109884

Attachment 194779: Corrected ChangeLog
https://bugs.webkit.org/attachment.cgi?id=194779&action=review

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
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.