Add WebPageImportanceSignals::issuedNonGetFetchFromScript()

third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp

Index: third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp
diff --git a/third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp b/third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp
index b06ea8c34b2826f8e7d618ec4a85ca98258167ac..22e7de7838aef765d9d08fc9e0b1e6180221dbb6 100644
--- a/third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp
+++ b/third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp
@@ -48,6 +48,8 @@
 #include "core/loader/FrameLoader.h"
 #include "core/loader/FrameLoaderClient.h"
 #include "core/loader/ThreadableLoaderClient.h"
+#include "core/page/ChromeClient.h"
+#include "core/page/Page.h"
 #include "platform/SharedBuffer.h"
 #include "platform/Task.h"
 #include "platform/network/ResourceRequest.h"
@@ -162,6 +164,26 @@ DocumentThreadableLoader::DocumentThreadableLoader(Document& document, Threadabl
             m_simpleRequestHeaders.add(header.key, header.value);
     }
 
+    // DocumentThreadableLoader is used by all javascript initiated fetch, so
+    // we use this chance to update PageImportanceSignals.

[tkent, 2015/09/27 23:40:51]

DocumentThreadableLoader should not have knowledge of PageImportanceSignals.
 - Passing httpMethod() to observedNonGetFetchFromScript(), and
 - Move this comment to ChromeClientImpl
might be better.  But the code logic depends on the details of ThreadableLoader.
 So the current structure is also acceptable.

[kouhei (in TOK), 2015/09/28 00:42:44]

Thanks for your suggestion. I thought over this again, but I prefer to keep this
comment/code here:
- The ThreadableLoader design is what is important here, so the comment would be
more benefitial when written here.
- I updated the comment slightly to remove explicit reference to
PageImportanceSignals.

+    // However, this is based on the following assumptions, so please be careful
+    // when adding similar logic:
+    // - ThreadableLoader is used as backend for all javascript initiated network
+    //   fetches.
+    //   - Note that ThreadableLoader is also used for non-network fetch such as
+    //     FileReaderLoader. However it emulates GET method so signal is not
+    //     recorded here.
+    // - ThreadableLoader w/ non-GET request is only created from javascript
+    //   initiated fetch.
+    //   - Some non-script initiated fetches such as WorkerScriptLoader also use
+    //     ThreadableLoader, but they are guaranteed to use GET method.
+    if (request.httpMethod() != "GET") {

[tkent, 2015/09/27 23:40:52]

Is it safe to assume httpMethod() is always upper-cased?

[kouhei (in TOK), 2015/09/28 00:42:44]

Yes. Follows other use cases:
https://code.google.com/p/chromium/codesearch#search/&q=%22httpMethod()%20==%...

[tkent, 2015/09/28 00:48:08]

Someone should document it in ResourceRequest.h.

+        if (LocalFrame* frame = m_document.frame()) {

[tkent, 2015/09/27 23:40:51]

m_document.page() is simpler.

[kouhei (in TOK), 2015/09/28 00:42:44]

Done.

+            if (Page* page = frame->page())
+                page->chromeClient().observedNonGetFetchFromScript();
+        }
+    }
+
     // If the fetch request will be handled by the ServiceWorker, the
     // FetchRequestMode of the request must be FetchRequestModeCORS or
     // FetchRequestModeCORSWithForcedPreflight. Otherwise the ServiceWorker can