[webkit-reviews] review denied: [Bug 90367] [Chromium] ContextFeaturesClient::isEnabled is slow : [Attachment 150551] Patch

[bugzilla-daemon at webkit.org]

Kent Tamura <tkent at chromium.org> has denied Hajime Morrita
<morrita at google.com>'s request for review:
Bug 90367: [Chromium] ContextFeaturesClient::isEnabled is slow
https://bugs.webkit.org/show_bug.cgi?id=90367

Attachment 150551: Patch
https://bugs.webkit.org/attachment.cgi?id=150551&action=review

------- Additional Comments from Kent Tamura <tkent at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=150551&action=review


> Source/WebCore/dom/ContextFeatures.h:46
> +	   FeatureTypeSize

Needs a comment for this like "This should be at the last" or something.

> Source/WebKit/chromium/src/ContextFeaturesClientImpl.cpp:59
> +	   bool isEnabled() const { return m_value == IsEnabled; }

Should have ASSERT(m_value != IsNotValid);

> Source/WebKit/chromium/src/ContextFeaturesClientImpl.cpp:70
> +	   bool isValid(bool defaultValue) const
> +	   {
> +	       return m_value != IsNotValid && m_defaultValue == defaultValue;
> +	   }

The function name looks wrong because this function checks not only IsNotValid
but also m_defaultValue.
This should be "needsToRefreshValue()" or something.

> Source/WebKit/chromium/src/ContextFeaturesClientImpl.cpp:74
> +	   bool m_defaultValue; // Needs to be traked as a part of the
signature since it can be changed dynamically.

Do we already have such case?

> Source/WebKit/chromium/src/ContextFeaturesClientImpl.cpp:88
> +    void validateAgainst(Document* document)
> +    {
> +	   String currentDomain = document->securityOrigin()->domain();
> +	   if (currentDomain == m_domain)
> +	       return;
> +	   m_domain = currentDomain;
> +	   for (size_t i = 0; i < ContextFeatures::FeatureTypeSize; ++i)
> +	       m_entries[i] = Entry();
> +    }

I prefer defining this function at outside of the class definition because this
function is not small.

> Source/WebKit/chromium/src/ContextFeaturesClientImpl.cpp:90
> +    Entry* at(ContextFeatures::FeatureType type)

* "at()" sounds strange.  entryFor()?

* You can make the return type "Entry&".

> Source/WebKit/chromium/src/ContextFeaturesClientImpl.cpp:116
> +    cache->validateAgainst(document);

Why we need to call validateAgainst() every time?