Add WebPageImportanceSignals::issuedNonGetFetchFromScript()

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

 /*
  * Copyright (C) 2011, 2012 Google Inc. All rights reserved.
  * Copyright (C) 2013, Intel Corporation
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
@@ skipping 30 matching lines @@
 #include "core/frame/FrameConsole.h"
 #include "core/frame/LocalFrame.h"
 #include "core/frame/csp/ContentSecurityPolicy.h"
 #include "core/inspector/InspectorInstrumentation.h"
 #include "core/inspector/InspectorTraceEvents.h"
 #include "core/loader/CrossOriginPreflightResultCache.h"
 #include "core/loader/DocumentThreadableLoaderClient.h"
 #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"
 #include "platform/weborigin/SchemeRegistry.h"
 #include "platform/weborigin/SecurityOrigin.h"
 #include "public/platform/Platform.h"
 #include "public/platform/WebURLRequest.h"
 #include "wtf/Assertions.h"
 
 namespace blink {
@@ skipping 94 matching lines @@
     m_requestStartedSeconds = monotonicallyIncreasingTime();
 
     // Save any CORS simple headers on the request here. If this request redirects cross-origin, we cancel the old request
     // create a new one, and copy these headers.
     const HTTPHeaderMap& headerMap = request.httpHeaderFields();
     for (const auto& header : headerMap) {
         if (FetchUtils::isSimpleHeader(header.key, header.value))
             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
     // return a opaque response which is from the other origin site and the
     // script in the page can read the content.
     //
     // We assume that ServiceWorker is skipped for sync requests and non-HTTP
     // familiy requests by content/ code.
     if (m_async && !request.skipServiceWorker() && request.url().protocolIsInHTTPFamily() && m_document.fetcher()->isControlledByServiceWorker()) {
         ResourceRequest newRequest(request);
@@ skipping 670 matching lines @@
         return DoNotAllowStoredCredentials;
     return m_resourceLoaderOptions.allowCredentials;
 }
 
 SecurityOrigin* DocumentThreadableLoader::securityOrigin() const
 {
     return m_securityOrigin ? m_securityOrigin.get() : m_document.securityOrigin();
 }
 
 } // namespace blink