[Chromium] ContextFeaturesClient::isEnabled is slow
authormorrita@google.com <morrita@google.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 9 Jul 2012 08:53:48 +0000 (08:53 +0000)
committermorrita@google.com <morrita@google.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 9 Jul 2012 08:53:48 +0000 (08:53 +0000)
https://bugs.webkit.org/show_bug.cgi?id=90367

Reviewed by Kent Tamura.

Source/WebCore:

* dom/ContextFeatures.h:
(WebCore::ContextFeaturesClient::urlDidChange): Added.
(WebCore::ContextFeatures::urlDidChange): Added.
(WebCore):
* dom/Document.cpp:
(WebCore::Document::setURL): Added an urlDidChange() call.

Source/WebKit/chromium:

ContextFeaturesClientImpl::isEnabled touches a heavy part in chrome
where locks are acquired for each invocation.

This change introduces a set of caches to avoid such slow calls.
The cache class ContextFeaturesCache is implemented as a
Supplement of ScriptExecutionContext because the flag bits
depend on the domain of each Document.

* src/ContextFeaturesClientImpl.cpp:
(ContextFeaturesCache): Added.
(Entry): Added.
(WebKit::ContextFeaturesCache::Entry::Entry):
(WebKit::ContextFeaturesCache::Entry::isEnabled):
(WebKit::ContextFeaturesCache::Entry::set):
(WebKit::ContextFeaturesCache::Entry::needsRefresh):
(WebKit::ContextFeaturesCache::entryFor):
(WebKit):
(WebKit::ContextFeaturesCache::supplementName):
(WebKit::ContextFeaturesCache::from):
(WebKit::ContextFeaturesCache::refreshAgainst):
(WebKit::ContextFeaturesClientImpl::isEnabled):
(WebKit::ContextFeaturesClientImpl::urlDidChange): Added to invoke refrshAgainst.
(WebKit::ContextFeaturesClientImpl::askIfIsEnabled):
* src/ContextFeaturesClientImpl.h:
(ContextFeaturesClientImpl):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@122099 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog
Source/WebCore/dom/ContextFeatures.h
Source/WebCore/dom/Document.cpp
Source/WebKit/chromium/ChangeLog
Source/WebKit/chromium/src/ContextFeaturesClientImpl.cpp
Source/WebKit/chromium/src/ContextFeaturesClientImpl.h

index 6c68596..6bf0705 100644 (file)
@@ -1,3 +1,17 @@
+2012-07-09  MORITA Hajime  <morrita@google.com>
+
+        [Chromium] ContextFeaturesClient::isEnabled is slow
+        https://bugs.webkit.org/show_bug.cgi?id=90367
+
+        Reviewed by Kent Tamura.
+
+        * dom/ContextFeatures.h:
+        (WebCore::ContextFeaturesClient::urlDidChange): Added.
+        (WebCore::ContextFeatures::urlDidChange): Added.
+        (WebCore):
+        * dom/Document.cpp:
+        (WebCore::Document::setURL): Added an urlDidChange() call.
+
 2012-07-09  Andrei Onea  <onea@adobe.com>
 
         [CSSRegions] Implement NamedFlow::firstEmptyRegionIndex attribute
index ef66220..fc3036d 100644 (file)
@@ -40,9 +40,10 @@ class Page;
 class ContextFeatures : public RefCountedSupplement<Page, ContextFeatures> {
 public:
     enum FeatureType {
-        ShadowDOM,
+        ShadowDOM = 0,
         StyleScoped,
-        PagePopup
+        PagePopup,
+        FeatureTypeSize // Should be the last enetry.
     };
 
     static const AtomicString& supplementName();
@@ -54,6 +55,7 @@ public:
     static bool pagePopupEnabled(Document*);
 
     bool isEnabled(Document*, FeatureType, bool) const;
+    void urlDidChange(Document*);
 
 private:
     explicit ContextFeatures(ContextFeaturesClient* client)
@@ -77,6 +79,7 @@ public:
 
     virtual ~ContextFeaturesClient() { }
     virtual bool isEnabled(Document*, ContextFeatures::FeatureType, bool defaultValue) { return defaultValue; }
+    virtual void urlDidChange(Document*) { }
 };
 
 void provideContextFeaturesTo(Page*, ContextFeaturesClient*);
@@ -94,6 +97,13 @@ inline bool ContextFeatures::isEnabled(Document* document, FeatureType type, boo
     return m_client->isEnabled(document, type, defaultValue);
 }
 
+inline void ContextFeatures::urlDidChange(Document* document)
+{
+    if (m_client)
+        return;
+    m_client->urlDidChange(document);
+}
+
 } // namespace WebCore
 
 #endif // ContextFeatures_h
index adde312..283dacc 100644 (file)
@@ -2684,6 +2684,7 @@ void Document::setURL(const KURL& url)
     m_url = newURL;
     m_documentURI = m_url.string();
     updateBaseURL();
+    contextFeatures()->urlDidChange(this);
 }
 
 void Document::updateBaseURL()
index e4f5aaa..d20bea4 100644 (file)
@@ -1,3 +1,36 @@
+2012-07-09  MORITA Hajime  <morrita@google.com>
+
+        [Chromium] ContextFeaturesClient::isEnabled is slow
+        https://bugs.webkit.org/show_bug.cgi?id=90367
+
+        Reviewed by Kent Tamura.
+
+        ContextFeaturesClientImpl::isEnabled touches a heavy part in chrome
+        where locks are acquired for each invocation.
+
+        This change introduces a set of caches to avoid such slow calls.
+        The cache class ContextFeaturesCache is implemented as a
+        Supplement of ScriptExecutionContext because the flag bits
+        depend on the domain of each Document.
+
+        * src/ContextFeaturesClientImpl.cpp:
+        (ContextFeaturesCache): Added.
+        (Entry): Added.
+        (WebKit::ContextFeaturesCache::Entry::Entry):
+        (WebKit::ContextFeaturesCache::Entry::isEnabled):
+        (WebKit::ContextFeaturesCache::Entry::set):
+        (WebKit::ContextFeaturesCache::Entry::needsRefresh):
+        (WebKit::ContextFeaturesCache::entryFor):
+        (WebKit):
+        (WebKit::ContextFeaturesCache::supplementName):
+        (WebKit::ContextFeaturesCache::from):
+        (WebKit::ContextFeaturesCache::refreshAgainst):
+        (WebKit::ContextFeaturesClientImpl::isEnabled):
+        (WebKit::ContextFeaturesClientImpl::urlDidChange): Added to invoke refrshAgainst.
+        (WebKit::ContextFeaturesClientImpl::askIfIsEnabled):
+        * src/ContextFeaturesClientImpl.h:
+        (ContextFeaturesClientImpl):
+
 2012-07-09  Vsevolod Vlasov  <vsevik@chromium.org>
 
         Unreviewed chromium inspector test fix.
index aada077..63e1672 100644 (file)
 #include "ContextFeaturesClientImpl.h"
 
 #include "Document.h"
+#include "SecurityOrigin.h"
 #include "WebDocument.h"
 #include "WebPermissionClient.h"
 
+using namespace WebCore;
+
 namespace WebKit {
 
-bool ContextFeaturesClientImpl::isEnabled(WebCore::Document* document, WebCore::ContextFeatures::FeatureType type, bool defaultValue)
+class ContextFeaturesCache : public Supplement<ScriptExecutionContext> {
+public:
+    class Entry {
+    public:
+        enum Value {
+            IsEnabled,
+            IsDisabled,
+            NeedsRefresh
+        };
+
+        Entry()
+            : m_value(NeedsRefresh)
+            , m_defaultValue(false)
+        { }
+
+        bool isEnabled() const
+        {
+            ASSERT(m_value != NeedsRefresh);
+            return m_value == IsEnabled;
+        }
+
+        void set(bool value, bool defaultValue)
+        {
+            m_value = value ? IsEnabled : IsDisabled;
+            m_defaultValue = defaultValue;
+        }
+
+        bool needsRefresh(bool defaultValue) const
+        {
+            return m_value == NeedsRefresh || m_defaultValue != defaultValue;
+        }
+
+    private:
+        Value m_value;
+        bool m_defaultValue; // Needs to be traked as a part of the signature since it can be changed dynamically.
+    };
+
+    static const AtomicString& supplementName();
+    static ContextFeaturesCache* from(Document*);
+
+    Entry& entryFor(ContextFeatures::FeatureType type)
+    {
+        size_t index = static_cast<size_t>(type);
+        ASSERT(index < ContextFeatures::FeatureTypeSize);
+        return m_entries[index];
+    }
+
+    void validateAgainst(Document*);
+
+private:
+    String m_domain;
+    Entry m_entries[ContextFeatures::FeatureTypeSize];
+};
+
+const AtomicString& ContextFeaturesCache::supplementName()
+{
+    DEFINE_STATIC_LOCAL(AtomicString, name, ("ContextFeaturesCache"));
+    return name;
+}
+
+ContextFeaturesCache* ContextFeaturesCache::from(Document* document)
+{
+    ContextFeaturesCache* cache = static_cast<ContextFeaturesCache*>(Supplement<ScriptExecutionContext>::from(document, supplementName()));
+    if (!cache) {
+        cache = new ContextFeaturesCache();
+        Supplement<ScriptExecutionContext>::provideTo(document, supplementName(), adoptPtr(cache));
+    }
+
+    return cache;
+}
+
+void ContextFeaturesCache::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();
+}
+
+bool ContextFeaturesClientImpl::isEnabled(Document* document, ContextFeatures::FeatureType type, bool defaultValue)
+{
+    ContextFeaturesCache::Entry& cache = ContextFeaturesCache::from(document)->entryFor(type);
+    if (cache.needsRefresh(defaultValue))
+        cache.set(askIfIsEnabled(document, type, defaultValue), defaultValue);
+    return cache.isEnabled();
+}
+
+void ContextFeaturesClientImpl::urlDidChange(Document* document)
+{
+    ContextFeaturesCache::from(document)->validateAgainst(document);
+}
+
+bool ContextFeaturesClientImpl::askIfIsEnabled(Document* document, ContextFeatures::FeatureType type, bool defaultValue)
 {
     if (!m_client)
         return defaultValue;
 
     switch (type) {
-    case WebCore::ContextFeatures::ShadowDOM:
-    case WebCore::ContextFeatures::StyleScoped:
+    case ContextFeatures::ShadowDOM:
+    case ContextFeatures::StyleScoped:
         return m_client->allowWebComponents(WebDocument(document), defaultValue);
     default:
         return defaultValue;
index ea1f954..26d9930 100644 (file)
@@ -44,9 +44,12 @@ public:
     { }
 
     virtual bool isEnabled(WebCore::Document*, WebCore::ContextFeatures::FeatureType, bool defaultValue) OVERRIDE;
+    virtual void urlDidChange(WebCore::Document*) OVERRIDE;
     void setPermissionClient(WebPermissionClient* client) { m_client = client; }
 
 private:
+    bool askIfIsEnabled(WebCore::Document*, WebCore::ContextFeatures::FeatureType, bool defaultValue);
+
     WebPermissionClient* m_client;
 };