[subresource_filter] Display dev tools console message for subresource blocking

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

Index: third_party/WebKit/Source/core/loader/SubresourceFilter.cpp
diff --git a/third_party/WebKit/Source/core/loader/SubresourceFilter.cpp b/third_party/WebKit/Source/core/loader/SubresourceFilter.cpp
index bdfbdf10756a94ec33a0dce2aeb67cb83cc3a41a..98d99e24607e4ce785be0116ee2b92998c5cdcc8 100644
--- a/third_party/WebKit/Source/core/loader/SubresourceFilter.cpp
+++ b/third_party/WebKit/Source/core/loader/SubresourceFilter.cpp
@@ -6,13 +6,27 @@
 
 #include <utility>
 
+#include "core/dom/Document.h"
 #include "core/dom/TaskRunnerHelper.h"
+#include "core/frame/LocalFrame.h"
+#include "core/inspector/ConsoleMessage.h"
 #include "platform/WebTaskRunner.h"
 #include "platform/weborigin/KURL.h"
 #include "public/platform/WebTraceLocation.h"
 
 namespace blink {
 
+namespace {
+
+void emitErrorForDisallowedLoads(const String& url, Document& document) {

[Charlie Harrison, 2017/05/19 17:56:44]

Just inlining this in AllowLoad is better imo.

[shivanisha, 2017/05/22 14:26:27]

Compiler complains about declaring variables inside the case statement in
ReportLoad, so letting it remain in a separate function vs declaring the
variables outside of switch.

[Charlie Harrison, 2017/05/22 14:35:03]

How about renaming this method "GetErrorStringForDisallowedLoad" and just
returning the string built by the string builder?

[shivanisha, 2017/05/23 18:54:00]

done

+  String message =

[Charlie Harrison, 2017/05/19 17:56:44]

Use StringBuilder for more efficient string appending.

[shivanisha, 2017/05/22 14:26:27]

done

+      "Subresource filtering disallowed loading this resource, " + url + ".";

[Charlie Harrison, 2017/05/19 17:56:44]

Needs a TODO to update the string when we've decided on it.

[shivanisha, 2017/05/22 14:26:27]

done

+  document.AddConsoleMessage(
+      ConsoleMessage::Create(kJSMessageSource, kErrorMessageLevel, message));
+}
+
+}  // namespace
+
 // static
 SubresourceFilter* SubresourceFilter::Create(
     DocumentLoader* loader,
@@ -36,6 +50,15 @@ bool SubresourceFilter::AllowLoad(
   // Pair<url string, context> -> LoadPolicy.
   WebDocumentSubresourceFilter::LoadPolicy load_policy =
       subresource_filter_->GetLoadPolicy(resource_url, request_context);
+
+  Document* document = document_loader_->GetFrame()
+                           ? document_loader_->GetFrame()->GetDocument()
+                           : nullptr;
+  if (subresource_filter_->ShouldLogToConsole() &&

[Charlie Harrison, 2017/05/19 17:56:44]

This logic needs to be in ReportLoad, because its name is Report :) We
definitely don't want to log a message if the reporting policy isn't KReport
(e.g. for preloads).

Would you add a test for this case?
E.g. make sure that preloads from the preload scanner do not trigger a console
message.

[shivanisha, 2017/05/22 14:26:27]

Done. Thanks.
The layout tests mock out only the WebDocumentSubresourceFilter so there isn't a
way currently to mock the value of SecurityViolationReportingPolicy passed in to
 SubresourceFilter::AllowLoad()

+      load_policy == WebDocumentSubresourceFilter::kDisallow && document) {
+    emitErrorForDisallowedLoads(resource_url.GetString(), *document);
+  }
+
   if (reporting_policy == SecurityViolationReportingPolicy::kReport)
     ReportLoad(load_policy);
   return load_policy != WebDocumentSubresourceFilter::kDisallow;
@@ -67,8 +90,6 @@ void SubresourceFilter::ReportLoad(
       subresource_filter_->ReportDisallowedLoad();
     // fall through
     case WebDocumentSubresourceFilter::kWouldDisallow:
-      // TODO(csharrison): log console errors here based on
-      // subresource_filter_->ShouldLogToConsole().
       document_loader_->DidObserveLoadingBehavior(
           kWebLoadingBehaviorSubresourceFilterMatch);
       break;