DevTools: Extract CSS resources preloading from css agent to page agent controlled loader.

DevTools: Extract CSS resources preloading from css agent to page agent controlled loader.

R=pfeldman
BUG=354447
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=176443

[vsevik, 2014-06-18 13:53:50]

PTAL

[apavlov, 2014-06-18 14:34:00]

https://chromiumcodereview.appspot.com/347543002/diff/1/Source/core/inspector...
File Source/core/inspector/InspectorCSSAgent.h (right):

https://chromiumcodereview.appspot.com/347543002/diff/1/Source/core/inspector...
Source/core/inspector/InspectorCSSAgent.h:145: static void
collectAllDocumentStyleSheets(Document*, Vector<CSSStyleSheet*>&);
statics go first

https://chromiumcodereview.appspot.com/347543002/diff/1/Source/core/inspector...
Source/core/inspector/InspectorCSSAgent.h:166: static void
collectStyleSheets(CSSStyleSheet*, Vector<CSSStyleSheet*>&);
ditto

https://chromiumcodereview.appspot.com/347543002/diff/1/Source/core/inspector...
File Source/core/inspector/InspectorResourceContentLoader.cpp (right):

https://chromiumcodereview.appspot.com/347543002/diff/1/Source/core/inspector...
Source/core/inspector/InspectorResourceContentLoader.cpp:25: class
InspectorResourceContentLoader::ResourceClient FINAL : private
StyleSheetResourceClient {
IRCL::CSSResourceClient for clarity?

https://chromiumcodereview.appspot.com/347543002/diff/1/Source/core/inspector...
Source/core/inspector/InspectorResourceContentLoader.cpp:30: private:
blank line above

https://chromiumcodereview.appspot.com/347543002/diff/1/Source/core/inspector...
Source/core/inspector/InspectorResourceContentLoader.cpp:38:
InspectorResourceContentLoader::ResourceClient::ResourceClient(InspectorResourceContentLoader*
loader)
This could be inlined into the class declaration

https://chromiumcodereview.appspot.com/347543002/diff/1/Source/core/inspector...
Source/core/inspector/InspectorResourceContentLoader.cpp:43: void
InspectorResourceContentLoader::ResourceClient::waitForResource(Resource*
resource)
ditto - it is trivial

https://chromiumcodereview.appspot.com/347543002/diff/1/Source/core/inspector...
Source/core/inspector/InspectorResourceContentLoader.cpp:67: for
(Vector<Document*>::const_iterator it = documents.begin(); it !=
documents.end(); ++it) {
docIt?

https://chromiumcodereview.appspot.com/347543002/diff/1/Source/core/inspector...
Source/core/inspector/InspectorResourceContentLoader.cpp:73: for
(Vector<CSSStyleSheet*>::const_iterator it2 = styleSheets.begin(); it2 !=
styleSheets.end(); ++it2) {
cssIt?

https://chromiumcodereview.appspot.com/347543002/diff/1/Source/core/inspector...
Source/core/inspector/InspectorResourceContentLoader.cpp:77: String url =
styleSheet->contents()->baseURL().string();
styleSheet->baseURL().string() for brevity

https://chromiumcodereview.appspot.com/347543002/diff/1/Source/core/inspector...
Source/core/inspector/InspectorResourceContentLoader.cpp:84:
m_resources.append(resource.get());
This is not going to hold a reference to the resource, is it? get() will give
you a raw pointer with no counted reference copying involved.

https://chromiumcodereview.appspot.com/347543002/diff/1/Source/core/inspector...
File Source/core/inspector/InspectorResourceContentLoader.h (right):

https://chromiumcodereview.appspot.com/347543002/diff/1/Source/core/inspector...
Source/core/inspector/InspectorResourceContentLoader.h:26: virtual
~InspectorResourceContentLoader();
should not be virtual

[vsevik, 2014-06-18 14:49:56]

https://chromiumcodereview.appspot.com/347543002/diff/1/Source/core/inspector...
File Source/core/inspector/InspectorCSSAgent.h (right):

https://chromiumcodereview.appspot.com/347543002/diff/1/Source/core/inspector...
Source/core/inspector/InspectorCSSAgent.h:145: static void
collectAllDocumentStyleSheets(Document*, Vector<CSSStyleSheet*>&);
On 2014/06/18 14:33:58, apavlov wrote:
> statics go first

Done.

https://chromiumcodereview.appspot.com/347543002/diff/1/Source/core/inspector...
Source/core/inspector/InspectorCSSAgent.h:166: static void
collectStyleSheets(CSSStyleSheet*, Vector<CSSStyleSheet*>&);
On 2014/06/18 14:33:58, apavlov wrote:
> ditto

Done.

https://chromiumcodereview.appspot.com/347543002/diff/1/Source/core/inspector...
File Source/core/inspector/InspectorResourceContentLoader.cpp (right):

https://chromiumcodereview.appspot.com/347543002/diff/1/Source/core/inspector...
Source/core/inspector/InspectorResourceContentLoader.cpp:25: class
InspectorResourceContentLoader::ResourceClient FINAL : private
StyleSheetResourceClient {
It is going to be used for document resource as well in the nearest future.

https://chromiumcodereview.appspot.com/347543002/diff/1/Source/core/inspector...
Source/core/inspector/InspectorResourceContentLoader.cpp:30: private:
On 2014/06/18 14:33:59, apavlov wrote:
> blank line above

Done.

https://chromiumcodereview.appspot.com/347543002/diff/1/Source/core/inspector...
Source/core/inspector/InspectorResourceContentLoader.cpp:38:
InspectorResourceContentLoader::ResourceClient::ResourceClient(InspectorResourceContentLoader*
loader)
On 2014/06/18 14:33:59, apavlov wrote:
> This could be inlined into the class declaration

Done.

https://chromiumcodereview.appspot.com/347543002/diff/1/Source/core/inspector...
Source/core/inspector/InspectorResourceContentLoader.cpp:43: void
InspectorResourceContentLoader::ResourceClient::waitForResource(Resource*
resource)
On 2014/06/18 14:33:59, apavlov wrote:
> ditto - it is trivial

Done.

https://chromiumcodereview.appspot.com/347543002/diff/1/Source/core/inspector...
Source/core/inspector/InspectorResourceContentLoader.cpp:67: for
(Vector<Document*>::const_iterator it = documents.begin(); it !=
documents.end(); ++it) {
On 2014/06/18 14:33:59, apavlov wrote:
> docIt?

Done.

https://chromiumcodereview.appspot.com/347543002/diff/1/Source/core/inspector...
Source/core/inspector/InspectorResourceContentLoader.cpp:73: for
(Vector<CSSStyleSheet*>::const_iterator it2 = styleSheets.begin(); it2 !=
styleSheets.end(); ++it2) {
On 2014/06/18 14:33:59, apavlov wrote:
> cssIt?

Done.

https://chromiumcodereview.appspot.com/347543002/diff/1/Source/core/inspector...
Source/core/inspector/InspectorResourceContentLoader.cpp:77: String url =
styleSheet->contents()->baseURL().string();
On 2014/06/18 14:33:59, apavlov wrote:
> styleSheet->baseURL().string() for brevity

Done.

https://chromiumcodereview.appspot.com/347543002/diff/1/Source/core/inspector...
Source/core/inspector/InspectorResourceContentLoader.cpp:84:
m_resources.append(resource.get());
This will call an implicit ResourcePtr constructor. Resources have an
alternative home-made "ref" counting :)

https://chromiumcodereview.appspot.com/347543002/diff/1/Source/core/inspector...
File Source/core/inspector/InspectorResourceContentLoader.h (right):

https://chromiumcodereview.appspot.com/347543002/diff/1/Source/core/inspector...
Source/core/inspector/InspectorResourceContentLoader.h:26: virtual
~InspectorResourceContentLoader();
On 2014/06/18 14:33:59, apavlov wrote:
> should not be virtual

Done.

[apavlov, 2014-06-18 15:07:57]

lgtm

[commit-bot: I haz the power, 2014-06-18 15:08:54]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vsevik@chromium.org/347543002/20001

[commit-bot: I haz the power, 2014-06-18 16:15:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-18 16:15:21]

Failed to apply patch for Source/core/inspector/InspectorCSSAgent.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/inspector/InspectorCSSAgent.cpp
  Hunk #2 FAILED at 48.
  Hunk #3 succeeded at 126 (offset -1 lines).
  Hunk #4 succeeded at 384 (offset -1 lines).
  Hunk #5 succeeded at 417 (offset -1 lines).
  Hunk #6 succeeded at 436 (offset -1 lines).
  Hunk #7 succeeded at 502 (offset -1 lines).
  Hunk #8 succeeded at 1124 (offset -1 lines).
  Hunk #9 succeeded at 1143 (offset -1 lines).
  1 out of 9 hunks FAILED -- saving rejects to file
Source/core/inspector/InspectorCSSAgent.cpp.rej

Patch:       Source/core/inspector/InspectorCSSAgent.cpp
Index: Source/core/inspector/InspectorCSSAgent.cpp
diff --git a/Source/core/inspector/InspectorCSSAgent.cpp
b/Source/core/inspector/InspectorCSSAgent.cpp
index
c54fe65b658c9cd8bc39f031025f3208366027f9..ae6ebd82298d9e159a55f95a230fbda1bdebd1cb
100644
--- a/Source/core/inspector/InspectorCSSAgent.cpp
+++ b/Source/core/inspector/InspectorCSSAgent.cpp
@@ -28,7 +28,6 @@
 #include "bindings/v8/ExceptionState.h"
 #include "bindings/v8/ExceptionStatePlaceholder.h"
 #include "core/CSSPropertyNames.h"
-#include "core/FetchInitiatorTypeNames.h"
 #include "core/InspectorTypeBuilder.h"
 #include "core/StylePropertyShorthand.h"
 #include "core/css/CSSComputedStyleDeclaration.h"
@@ -49,15 +48,13 @@
 #include "core/css/resolver/StyleResolver.h"
 #include "core/dom/Node.h"
 #include "core/dom/NodeList.h"
-#include "core/fetch/CSSStyleSheetResource.h"
-#include "core/fetch/ResourceClient.h"
-#include "core/fetch/ResourceFetcher.h"
-#include "core/fetch/StyleSheetResourceClient.h"
 #include "core/frame/LocalFrame.h"
 #include "core/html/HTMLHeadElement.h"
+#include "core/html/VoidCallback.h"
 #include "core/inspector/InspectorHistory.h"
 #include "core/inspector/InspectorPageAgent.h"
 #include "core/inspector/InspectorResourceAgent.h"
+#include "core/inspector/InspectorResourceContentLoader.h"
 #include "core/inspector/InspectorState.h"
 #include "core/inspector/InstrumentingAgents.h"
 #include "core/loader/DocumentLoader.h"
@@ -128,45 +125,30 @@ public:
     }
 };
 
-class InspectorCSSAgent::EnableResourceClient FINAL : public
StyleSheetResourceClient {
+class InspectorCSSAgent::InspectorResourceContentLoaderCallback FINAL : public
VoidCallback {
 public:
-    EnableResourceClient(InspectorCSSAgent*, const
Vector<InspectorStyleSheet*>&, PassRefPtr<EnableCallback>);
-
-    virtual void setCSSStyleSheet(const String&, const KURL&, const String&,
const CSSStyleSheetResource*) OVERRIDE;
+    InspectorResourceContentLoaderCallback(InspectorCSSAgent*,
PassRefPtr<EnableCallback>);
+    virtual void handleEvent() OVERRIDE;
 
 private:
-    RefPtr<EnableCallback> m_callback;
     InspectorCSSAgent* m_cssAgent;
-    int m_pendingResources;
-    Vector<InspectorStyleSheet*> m_styleSheets;
+    RefPtr<EnableCallback> m_callback;
 };
 
-InspectorCSSAgent::EnableResourceClient::EnableResourceClient(InspectorCSSAgent*
cssAgent, const Vector<InspectorStyleSheet*>& styleSheets,
PassRefPtr<EnableCallback> callback)
-    : m_callback(callback)
-    , m_cssAgent(cssAgent)
-    , m_pendingResources(styleSheets.size())
-    , m_styleSheets(styleSheets)
+InspectorCSSAgent::InspectorResourceContentLoaderCallback::InspectorResourceContentLoaderCallback(InspectorCSSAgent*
cssAgent, PassRefPtr<EnableCallback> callback)
+    : m_cssAgent(cssAgent)
+    , m_callback(callback)
 {
-    for (size_t i = 0; i < styleSheets.size(); ++i) {
-        InspectorStyleSheet* styleSheet = styleSheets.at(i);
-        Document* document = styleSheet->ownerDocument();
-        FetchRequest request(ResourceRequest(styleSheet->finalURL()),
FetchInitiatorTypeNames::internal);
-        ResourcePtr<Resource> resource =
document->fetcher()->fetchCSSStyleSheet(request);
-        resource->addClient(this);
-    }
 }
 
-void InspectorCSSAgent::EnableResourceClient::setCSSStyleSheet(const String&,
const KURL& url, const String&, const CSSStyleSheetResource* resource)
+void InspectorCSSAgent::InspectorResourceContentLoaderCallback::handleEvent()
 {
-    const_cast<CSSStyleSheetResource*>(resource)->removeClient(this);
-    --m_pendingResources;
-    if (m_pendingResources)
+    // enable always succeeds.
+    if (!m_callback->isActive())
         return;
 
-    // enable always succeeds.
-    if (m_callback->isActive())
-        m_cssAgent->wasEnabled(m_callback.release());
-    delete this;
+    m_cssAgent->wasEnabled();
+    m_callback->sendSuccess();
 }
 
 class InspectorCSSAgent::SetStyleSheetTextAction FINAL : public
InspectorCSSAgent::StyleSheetAction {
@@ -401,7 +383,7 @@ void InspectorCSSAgent::discardAgent()
 void InspectorCSSAgent::restore()
 {
     if (m_state->getBoolean(CSSAgentState::cssAgentEnabled))
-        wasEnabled(nullptr);
+        wasEnabled();
 }
 
 void InspectorCSSAgent::flushPendingFrontendMessages()
@@ -434,39 +416,15 @@ void InspectorCSSAgent::resetNonPersistentData()
 void InspectorCSSAgent::enable(ErrorString*, PassRefPtr<EnableCallback>
prpCallback)
 {
     m_state->setBoolean(CSSAgentState::cssAgentEnabled, true);
-
-    Vector<InspectorStyleSheet*> styleSheets;
-    collectAllStyleSheets(styleSheets);
-
-    // Re-issue stylesheet requets for resources that are no longer in memory
cache.
-    Vector<InspectorStyleSheet*> styleSheetsToFetch;
-    HashSet<String> urlsToFetch;
-    for (size_t i = 0; i < styleSheets.size(); ++i) {
-        InspectorStyleSheet* styleSheet = styleSheets.at(i);
-        String url = styleSheet->finalURL();
-        if (urlsToFetch.contains(url))
-            continue;
-        CSSStyleSheet* pageStyleSheet = styleSheet->pageStyleSheet();
-        if (pageStyleSheet->isInline() ||
!pageStyleSheet->contents()->loadCompleted())
-            continue;
-        Document* document = styleSheet->ownerDocument();
-        if (!document)
-            continue;
-        Resource* cachedResource =
document->fetcher()->cachedResource(document->completeURL(url));
-        if (cachedResource)
-            continue;
-        urlsToFetch.add(styleSheet->finalURL());
-        styleSheetsToFetch.append(styleSheet);
-    }
-
-    if (styleSheetsToFetch.isEmpty()) {
-        wasEnabled(prpCallback);
+    if (!m_pageAgent->resourceContentLoader()) {
+        wasEnabled();
+        prpCallback->sendSuccess();
         return;
     }
-    new EnableResourceClient(this, styleSheetsToFetch, prpCallback);
+    m_pageAgent->resourceContentLoader()->addListener(adoptPtr(new
InspectorCSSAgent::InspectorResourceContentLoaderCallback(this, prpCallback)));
 }
 
-void InspectorCSSAgent::wasEnabled(PassRefPtr<EnableCallback> callback)
+void InspectorCSSAgent::wasEnabled()
 {
     if (!m_state->getBoolean(CSSAgentState::cssAgentEnabled)) {
         // We were disabled while fetching resources.
@@ -477,9 +435,6 @@ void
InspectorCSSAgent::wasEnabled(PassRefPtr<EnableCallback> callback)
     Vector<Document*> documents = m_domAgent->documents();
     for (Vector<Document*>::iterator it = documents.begin(); it !=
documents.end(); ++it)
         updateActiveStyleSheets(*it, InitialFrontendLoad);
-
-    if (callback)
-        callback->sendSuccess();
 }
 
 void InspectorCSSAgent::disable(ErrorString*)
@@ -546,7 +501,7 @@ void InspectorCSSAgent::activeStyleSheetsUpdated(Document*
document)
 void InspectorCSSAgent::updateActiveStyleSheets(Document* document,
StyleSheetsUpdateType styleSheetsUpdateType)
 {
     Vector<CSSStyleSheet*> newSheetsVector;
-    collectAllDocumentStyleSheets(document, newSheetsVector);
+    InspectorCSSAgent::collectAllDocumentStyleSheets(document,
newSheetsVector);
     setActiveStyleSheets(document, newSheetsVector, styleSheetsUpdateType);
 }
 
@@ -1168,25 +1123,17 @@ Element* InspectorCSSAgent::elementForId(ErrorString*
errorString, int nodeId)
     return toElement(node);
 }
 
-void InspectorCSSAgent::collectAllStyleSheets(Vector<InspectorStyleSheet*>&
result)
-{
-    Vector<CSSStyleSheet*> cssStyleSheets;
-    Vector<Document*> documents = m_domAgent->documents();
-    for (Vector<Document*>::iterator it = documents.begin(); it !=
documents.end(); ++it)
-        collectAllDocumentStyleSheets(*it, cssStyleSheets);
-    for (Vector<CSSStyleSheet*>::iterator it = cssStyleSheets.begin(); it !=
cssStyleSheets.end(); ++it)
-        result.append(bindStyleSheet(*it));
-}
-
+// static
 void InspectorCSSAgent::collectAllDocumentStyleSheets(Document* document,
Vector<CSSStyleSheet*>& result)
 {
     const WillBeHeapVector<RefPtrWillBeMember<CSSStyleSheet> >
activeStyleSheets = document->styleEngine()->activeStyleSheetsForInspector();
     for (WillBeHeapVector<RefPtrWillBeMember<CSSStyleSheet> >::const_iterator
it = activeStyleSheets.begin(); it != activeStyleSheets.end(); ++it) {
         CSSStyleSheet* styleSheet = it->get();
-        collectStyleSheets(styleSheet, result);
+        InspectorCSSAgent::collectStyleSheets(styleSheet, result);
     }
 }
 
+// static
 void InspectorCSSAgent::collectStyleSheets(CSSStyleSheet* styleSheet,
Vector<CSSStyleSheet*>& result)
 {
     result.append(styleSheet);
@@ -1195,7 +1142,7 @@ void InspectorCSSAgent::collectStyleSheets(CSSStyleSheet*
styleSheet, Vector<CSS
         if (rule->type() == CSSRule::IMPORT_RULE) {
             CSSStyleSheet* importedStyleSheet =
toCSSImportRule(rule)->styleSheet();
             if (importedStyleSheet)
-                collectStyleSheets(importedStyleSheet, result);
+                InspectorCSSAgent::collectStyleSheets(importedStyleSheet,
result);
         }
     }
 }

[vsevik, 2014-06-18 16:47:04]

The CQ bit was checked by vsevik@chromium.org

[commit-bot: I haz the power, 2014-06-18 16:48:08]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vsevik@chromium.org/347543002/40001

[commit-bot: I haz the power, 2014-06-18 18:50:11]

Change committed as 176443