[Extensions] Kill off ExtensionMsg_AddMessageToConsole

extensions/renderer/console.cc

Index: extensions/renderer/console.cc
diff --git a/extensions/renderer/console.cc b/extensions/renderer/console.cc
index 172c5915bd06430c3a63d8684f6beb2d98ada31d..efde53577ffa72df98394ca29a7e8fe5627fdd4e 100644
--- a/extensions/renderer/console.cc
+++ b/extensions/renderer/console.cc
@@ -10,50 +10,15 @@
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
-#include "content/public/renderer/render_view.h"
-#include "content/public/renderer/render_view_visitor.h"
+#include "content/public/renderer/render_frame.h"
 #include "extensions/renderer/dispatcher.h"
-#include "extensions/renderer/extension_helper.h"
-#include "third_party/WebKit/public/web/WebConsoleMessage.h"
-#include "third_party/WebKit/public/web/WebFrame.h"
-#include "third_party/WebKit/public/web/WebView.h"
+#include "extensions/renderer/extension_frame_helper.h"
 
 namespace extensions {
 namespace console {
 
 namespace {
 
-// Finds the RenderView associated with a context. Note: there will be multiple
-// contexts in each RenderView.
-class ByContextFinder : public content::RenderViewVisitor {
- public:
-  static content::RenderView* Find(v8::Local<v8::Context> context) {
-    ByContextFinder finder(context);
-    content::RenderView::ForEach(&finder);
-    return finder.found_;
-  }
-
- private:
-  explicit ByContextFinder(v8::Local<v8::Context> context)
-      : context_(context), found_(NULL) {}
-
-  bool Visit(content::RenderView* render_view) override {
-    ExtensionHelper* helper = ExtensionHelper::Get(render_view);
-    if (helper) {
-      ScriptContext* script_context =
-          helper->dispatcher()->script_context_set().GetByV8Context(context_);
-      if (script_context && script_context->GetRenderView() == render_view)
-        found_ = render_view;
-    }
-    return !found_;
-  }
-
-  v8::Local<v8::Context> context_;
-  content::RenderView* found_;
-
-  DISALLOW_COPY_AND_ASSIGN(ByContextFinder);
-};
-
 // Writes |message| to stack to show up in minidump, then crashes.
 void CheckWithMinidump(const std::string& message) {
   char minidump[1024];
@@ -92,53 +57,6 @@ void BindLogMethod(v8::Isolate* isolate,
 
 }  // namespace
 
-void Debug(content::RenderView* render_view, const std::string& message) {
-  AddMessage(render_view, content::CONSOLE_MESSAGE_LEVEL_DEBUG, message);
-}
-
-void Log(content::RenderView* render_view, const std::string& message) {
-  AddMessage(render_view, content::CONSOLE_MESSAGE_LEVEL_LOG, message);
-}
-
-void Warn(content::RenderView* render_view, const std::string& message) {
-  AddMessage(render_view, content::CONSOLE_MESSAGE_LEVEL_WARNING, message);
-}
-
-void Error(content::RenderView* render_view, const std::string& message) {
-  AddMessage(render_view, content::CONSOLE_MESSAGE_LEVEL_ERROR, message);
-}
-
-void Fatal(content::RenderView* render_view, const std::string& message) {
-  Error(render_view, message);
-  CheckWithMinidump(message);
-}
-
-void AddMessage(content::RenderView* render_view,
-                content::ConsoleMessageLevel level,
-                const std::string& message) {
-  blink::WebView* web_view = render_view->GetWebView();
-  if (!web_view || !web_view->mainFrame())
-    return;
-  blink::WebConsoleMessage::Level target_level =
-      blink::WebConsoleMessage::LevelLog;
-  switch (level) {
-    case content::CONSOLE_MESSAGE_LEVEL_DEBUG:
-      target_level = blink::WebConsoleMessage::LevelDebug;
-      break;
-    case content::CONSOLE_MESSAGE_LEVEL_LOG:
-      target_level = blink::WebConsoleMessage::LevelLog;
-      break;
-    case content::CONSOLE_MESSAGE_LEVEL_WARNING:
-      target_level = blink::WebConsoleMessage::LevelWarning;
-      break;
-    case content::CONSOLE_MESSAGE_LEVEL_ERROR:
-      target_level = blink::WebConsoleMessage::LevelError;
-      break;
-  }
-  web_view->mainFrame()->addMessageToConsole(
-      blink::WebConsoleMessage(target_level, base::UTF8ToUTF16(message)));
-}
-
 void Debug(v8::Local<v8::Context> context, const std::string& message) {
   AddMessage(context, content::CONSOLE_MESSAGE_LEVEL_DEBUG, message);
 }
@@ -167,12 +85,16 @@ void AddMessage(v8::Local<v8::Context> context,
     LOG(WARNING) << "Could not log \"" << message << "\": no context given";
     return;
   }
-  content::RenderView* render_view = ByContextFinder::Find(context);
-  if (!render_view) {
-    LOG(WARNING) << "Could not log \"" << message << "\": no render view found";
+  ScriptContext* script_context =
+      Dispatcher::Get()->script_context_set().GetByV8Context(context);
+  content::RenderFrame* render_frame =
+      ExtensionFrameHelper::RenderFrameForContext(script_context);

[not at google - send to devlin, 2015/06/19 23:14:58]

I preferred the old code, conceptually. You have a v8::Context which belongs to
some RenderView (or now, some RenderFrame). You want to find the RenderFrame.
Introducing dependencies on
extensions::Dispatcher/ScriptContextSet/ScriptContext/ExtensionFrameHelper just
to get that doesn't seem right.

"Finding the RenderFrame" no longer seems possible, so instead I'd just pass the
RenderFrame into these methods. It looks like they can find out the RenderFrame
without too many problems.

[Devlin, 2015/06/19 23:53:52]

Two things:
- Most importantly, please note that this *doesn't* introduce new dependencies.
:)  All of these already existed in the ByContextFinder - dispatcher,
scriptcontextset, extensionhelper, and scriptcontext were all used.  Now,
technically, we don't really use scriptcontext, so if anything, we removed a
single dependency.
- Passing in the render frame would be fine, but we use this from the log
methods bound below - which I think only get the context, right?

I think I'd prefer to make a big yak shave of all this, since it doesn't
introduce any new dependencies, and significantly reduces the line count and
complexity (RenderViewVisitor is just kinda weird as a way of finding things),
but if there's an easy fix for passing in the render frame in lieu of the v8
context, I'd be happy to change it.

[not at google - send to devlin, 2015/06/22 17:54:11]

Ok, I see where we're getting tangled up, in this review and the other in fact.

By "dependency" I don't literally mean an #include, I mean some functionality of
a class that depends on some other class but shouldn't. 2 functions that depend
on the same wrong thing == 2 dependencies, not 1 dependency, even though from a
DEPS file or an #include or whatever perspective it's only 1.

Basically RFH is the right thing to log to, at least, the way this code is
structured. If I change the way I think about it, then I suppose it might make
sense to log to a ScriptContext, and in that case we should move the logging to
actually be part of the ScriptContext object, like,

script_context->console()->Error("error") i.e. rather than making console a set
of utilities, actually make it properly OO.

That might be easy, it might not, but if we do want to stay with making this at
the context level rather than at the frame level, can you at least pass a
ScriptContext into these methods rather than the v8::Context? I really want to
avoid adding that Dispatcher::Get() method, like we discussed, it's not wrong to
have it. However, I think it will complicate any future work trying to pull
functionality out of Dispatcher (which I could imagine happening for the Service
Worker refactoring we'll probably need).

[Devlin, 2015/06/22 19:52:32]

Per offline discussion, changed all these to take in a RenderFrame, and made
static accessor on ScriptContextSet.

+  if (!render_frame) {
+    LOG(WARNING) << "Could not log \"" << message
+                 << "\": no render frame found";
     return;
   }
-  AddMessage(render_view, level, message);
+  render_frame->AddMessageToConsole(level, message);
 }
 
 v8::Local<v8::Object> AsV8Object(v8::Isolate* isolate) {