[Extensions] Kill off ExtensionMsg_AddMessageToConsole

[Extensions] Kill off ExtensionMsg_AddMessageToConsole

ExtensionMsg_AddMessageToConsole does the same things as
RenderFrameHost::AddMessageToConsole, and adds a fair amount of code bloat.
Remove it.
In addition to being a nice cleanup, this kills off a bunch of
RenderViewHost dependencies, as well.

BUG=455776
TBR=timvolodine@chromium.org

Committed: https://crrev.com/b2cec918668eb5b49df32d4983df84f777e9d37f
Cr-Commit-Position: refs/heads/master@{#335991}

[Devlin, 2015-06-19 22:55:11]

Patchset #1 (id:1) has been deleted

[Devlin, 2015-06-19 22:55:21]

Patchset #1 (id:20001) has been deleted

[Devlin, 2015-06-19 22:55:35]

rdevlin.cronin@chromium.org changed reviewers:
+ kalman@chromium.org

[Devlin, 2015-06-19 22:55:36]

Woo!  Cleanup!

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

https://codereview.chromium.org/1200503002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/mdns/mdns_api.cc (right):

https://codereview.chromium.org/1200503002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/mdns/mdns_api.cc:215: content::RenderFrameHost*
rfh =
This should probably use ProcessManager::GetRenderFrameHostsForExtension()
rather than just the background host (which obviously may not exist).

https://codereview.chromium.org/1200503002/diff/40001/chrome/browser/speech/e...
File chrome/browser/speech/extension_api/tts_engine_extension_api.cc (right):

https://codereview.chromium.org/1200503002/diff/40001/chrome/browser/speech/e...
chrome/browser/speech/extension_api/tts_engine_extension_api.cc:62:
constants::kErrorMissingPauseOrResume);
Same here.

https://codereview.chromium.org/1200503002/diff/40001/extensions/browser/exte...
File extensions/browser/extension_function.cc (right):

https://codereview.chromium.org/1200503002/diff/40001/extensions/browser/exte...
extensions/browser/extension_function.cc:482:
WebContents::FromRenderFrameHost(render_frame_host_)
Can you just call render_frame_host_->AddMessageToConsole()?

https://codereview.chromium.org/1200503002/diff/40001/extensions/browser/sugg...
File extensions/browser/suggest_permission_util.h (right):

https://codereview.chromium.org/1200503002/diff/40001/extensions/browser/sugg...
extensions/browser/suggest_permission_util.h:24: content::WebContents*
web_contents);
RenderFrameHost would be more appropriate.

https://codereview.chromium.org/1200503002/diff/40001/extensions/renderer/con...
File extensions/renderer/console.cc (right):

https://codereview.chromium.org/1200503002/diff/40001/extensions/renderer/con...
extensions/renderer/console.cc:91:
ExtensionFrameHelper::RenderFrameForContext(script_context);
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.

https://codereview.chromium.org/1200503002/diff/40001/extensions/renderer/dis...
File extensions/renderer/dispatcher.h (right):

https://codereview.chromium.org/1200503002/diff/40001/extensions/renderer/dis...
extensions/renderer/dispatcher.h:74: static Dispatcher* Get();
Can you avoid adding this if you pass the RenderFrame into the console methods?

[Devlin, 2015-06-19 23:53:51]

Just responding, no code changes

https://codereview.chromium.org/1200503002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/mdns/mdns_api.cc (right):

https://codereview.chromium.org/1200503002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/mdns/mdns_api.cc:215: content::RenderFrameHost*
rfh =
On 2015/06/19 23:14:58, kalman wrote:
> This should probably use ProcessManager::GetRenderFrameHostsForExtension()
> rather than just the background host (which obviously may not exist).

That would be a change in behavior, and it would be kind of weird to log a
console message in a random render frame (or all render frames).  WDYT?

https://codereview.chromium.org/1200503002/diff/40001/extensions/browser/exte...
File extensions/browser/extension_function.cc (right):

https://codereview.chromium.org/1200503002/diff/40001/extensions/browser/exte...
extensions/browser/extension_function.cc:482:
WebContents::FromRenderFrameHost(render_frame_host_)
On 2015/06/19 23:14:58, kalman wrote:
> Can you just call render_frame_host_->AddMessageToConsole()?

We should only call AddMessageToConsole on the main frame (two reasons: the main
frame is the only one that has dev tools installed [see render frame impl] and
this matches current behavior, in which console::AddMessage always used the main
frame).

https://codereview.chromium.org/1200503002/diff/40001/extensions/browser/sugg...
File extensions/browser/suggest_permission_util.h (right):

https://codereview.chromium.org/1200503002/diff/40001/extensions/browser/sugg...
extensions/browser/suggest_permission_util.h:24: content::WebContents*
web_contents);
On 2015/06/19 23:14:58, kalman wrote:
> RenderFrameHost would be more appropriate.

But that means that then callers have to remember to pass in the main frame (see
other comment for why).  But we could do it, and it wouldn't be too confusing if
we just call the RFH "main_frame".  Still want it in light of only being able to
call it on the main frame?

https://codereview.chromium.org/1200503002/diff/40001/extensions/renderer/con...
File extensions/renderer/console.cc (right):

https://codereview.chromium.org/1200503002/diff/40001/extensions/renderer/con...
extensions/renderer/console.cc:91:
ExtensionFrameHelper::RenderFrameForContext(script_context);
On 2015/06/19 23:14:58, kalman wrote:
> 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.

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.

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

Just responding, no code changes

https://codereview.chromium.org/1200503002/diff/40001/extensions/browser/sugg...
File extensions/browser/suggest_permission_util.h (right):

https://codereview.chromium.org/1200503002/diff/40001/extensions/browser/sugg...
extensions/browser/suggest_permission_util.h:24: content::WebContents*
web_contents);
On 2015/06/19 23:14:58, kalman wrote:
> RenderFrameHost would be more appropriate.

But that means that then callers have to remember to pass in the main frame (see
other comment for why).  But we could do it, and it wouldn't be too confusing if
we just call the RFH "main_frame".  Still want it in light of only being able to
call it on the main frame?

https://codereview.chromium.org/1200503002/diff/40001/extensions/renderer/con...
File extensions/renderer/console.cc (right):

https://codereview.chromium.org/1200503002/diff/40001/extensions/renderer/con...
extensions/renderer/console.cc:91:
ExtensionFrameHelper::RenderFrameForContext(script_context);
On 2015/06/19 23:14:58, kalman wrote:
> 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.

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]

https://codereview.chromium.org/1200503002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/mdns/mdns_api.cc (right):

https://codereview.chromium.org/1200503002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/mdns/mdns_api.cc:215: content::RenderFrameHost*
rfh =
On 2015/06/19 23:53:51, Devlin wrote:
> On 2015/06/19 23:14:58, kalman wrote:
> > This should probably use ProcessManager::GetRenderFrameHostsForExtension()
> > rather than just the background host (which obviously may not exist).
> 
> That would be a change in behavior, and it would be kind of weird to log a
> console message in a random render frame (or all render frames).  WDYT?

I think it's more weird to log it to the background page, since there might not
be a background page. Better would be to log straight to the error console
actually.

https://codereview.chromium.org/1200503002/diff/40001/extensions/browser/exte...
File extensions/browser/extension_function.cc (right):

https://codereview.chromium.org/1200503002/diff/40001/extensions/browser/exte...
extensions/browser/extension_function.cc:482:
WebContents::FromRenderFrameHost(render_frame_host_)
On 2015/06/19 23:53:51, Devlin wrote:
> On 2015/06/19 23:14:58, kalman wrote:
> > Can you just call render_frame_host_->AddMessageToConsole()?
> 
> We should only call AddMessageToConsole on the main frame (two reasons: the
main
> frame is the only one that has dev tools installed [see render frame impl] and
> this matches current behavior, in which console::AddMessage always used the
main
> frame).

> "the main frame is the only one that has dev tools installed"
Seems like a devtools bug, at least in their API, but ok.

> "this matches current behavior"
Yes, I was hoping that logging to the RFH would be more correct and internally
devtools could figure out where to actually render those logs.

https://codereview.chromium.org/1200503002/diff/40001/extensions/browser/sugg...
File extensions/browser/suggest_permission_util.h (right):

https://codereview.chromium.org/1200503002/diff/40001/extensions/browser/sugg...
extensions/browser/suggest_permission_util.h:24: content::WebContents*
web_contents);
On 2015/06/19 23:53:52, Devlin wrote:
> On 2015/06/19 23:14:58, kalman wrote:
> > RenderFrameHost would be more appropriate.
> 
> But that means that then callers have to remember to pass in the main frame
(see
> other comment for why).  But we could do it, and it wouldn't be too confusing
if
> we just call the RFH "main_frame".  Still want it in light of only being able
to
> call it on the main frame?

I do think that passing in render frame is more correct, whether or not this is
the main frame. If devtools chooses to have an odd API and drop logs for
anything other than the main frame then so be it, but internally this method
could do the WebContents::FromRFH(rfh)->GetMainFrame() etc.

These devtools caveats should have quick comments btw.

https://codereview.chromium.org/1200503002/diff/40001/extensions/renderer/con...
File extensions/renderer/console.cc (right):

https://codereview.chromium.org/1200503002/diff/40001/extensions/renderer/con...
extensions/renderer/console.cc:91:
ExtensionFrameHelper::RenderFrameForContext(script_context);
On 2015/06/19 23:53:52, Devlin wrote:
> On 2015/06/19 23:14:58, kalman wrote:
> > 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.
> 
> 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.

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:33]

https://codereview.chromium.org/1200503002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/mdns/mdns_api.cc (right):

https://codereview.chromium.org/1200503002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/mdns/mdns_api.cc:215: content::RenderFrameHost*
rfh =
On 2015/06/22 17:54:10, kalman wrote:
> On 2015/06/19 23:53:51, Devlin wrote:
> > On 2015/06/19 23:14:58, kalman wrote:
> > > This should probably use ProcessManager::GetRenderFrameHostsForExtension()
> > > rather than just the background host (which obviously may not exist).
> > 
> > That would be a change in behavior, and it would be kind of weird to log a
> > console message in a random render frame (or all render frames).  WDYT?
> 
> I think it's more weird to log it to the background page, since there might
not
> be a background page. Better would be to log straight to the error console
> actually.

Added a TODO.  I'd like to (try) to avoid behavior changes in this CL.

https://codereview.chromium.org/1200503002/diff/40001/chrome/browser/speech/e...
File chrome/browser/speech/extension_api/tts_engine_extension_api.cc (right):

https://codereview.chromium.org/1200503002/diff/40001/chrome/browser/speech/e...
chrome/browser/speech/extension_api/tts_engine_extension_api.cc:62:
constants::kErrorMissingPauseOrResume);
On 2015/06/19 23:14:58, kalman wrote:
> Same here.

This one doesn't even check that the background host exists, so maybe it knows
it does (otherwise there's a crash)?  In either case, same idea about trying to
avoid behavior changes.

https://codereview.chromium.org/1200503002/diff/40001/extensions/browser/exte...
File extensions/browser/extension_function.cc (right):

https://codereview.chromium.org/1200503002/diff/40001/extensions/browser/exte...
extensions/browser/extension_function.cc:482:
WebContents::FromRenderFrameHost(render_frame_host_)
On 2015/06/22 17:54:10, kalman wrote:
> On 2015/06/19 23:53:51, Devlin wrote:
> > On 2015/06/19 23:14:58, kalman wrote:
> > > Can you just call render_frame_host_->AddMessageToConsole()?
> > 
> > We should only call AddMessageToConsole on the main frame (two reasons: the
> main
> > frame is the only one that has dev tools installed [see render frame impl]
and
> > this matches current behavior, in which console::AddMessage always used the
> main
> > frame).
> 
> > "the main frame is the only one that has dev tools installed"
> Seems like a devtools bug, at least in their API, but ok.
> 
> > "this matches current behavior"
> Yes, I was hoping that logging to the RFH would be more correct and internally
> devtools could figure out where to actually render those logs.

Added comment.

https://codereview.chromium.org/1200503002/diff/40001/extensions/browser/sugg...
File extensions/browser/suggest_permission_util.h (right):

https://codereview.chromium.org/1200503002/diff/40001/extensions/browser/sugg...
extensions/browser/suggest_permission_util.h:24: content::WebContents*
web_contents);
On 2015/06/22 17:54:11, kalman wrote:
> On 2015/06/19 23:53:52, Devlin wrote:
> > On 2015/06/19 23:14:58, kalman wrote:
> > > RenderFrameHost would be more appropriate.
> > 
> > But that means that then callers have to remember to pass in the main frame
> (see
> > other comment for why).  But we could do it, and it wouldn't be too
confusing
> if
> > we just call the RFH "main_frame".  Still want it in light of only being
able
> to
> > call it on the main frame?
> 
> I do think that passing in render frame is more correct, whether or not this
is
> the main frame. If devtools chooses to have an odd API and drop logs for
> anything other than the main frame then so be it, but internally this method
> could do the WebContents::FromRFH(rfh)->GetMainFrame() etc.
> 
> These devtools caveats should have quick comments btw.

Changed.

https://codereview.chromium.org/1200503002/diff/40001/extensions/renderer/con...
File extensions/renderer/console.cc (right):

https://codereview.chromium.org/1200503002/diff/40001/extensions/renderer/con...
extensions/renderer/console.cc:91:
ExtensionFrameHelper::RenderFrameForContext(script_context);
On 2015/06/22 17:54:11, kalman wrote:
> On 2015/06/19 23:53:52, Devlin wrote:
> > On 2015/06/19 23:14:58, kalman wrote:
> > > 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.
> > 
> > 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.
> 
> 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).

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

https://codereview.chromium.org/1200503002/diff/40001/extensions/renderer/dis...
File extensions/renderer/dispatcher.h (right):

https://codereview.chromium.org/1200503002/diff/40001/extensions/renderer/dis...
extensions/renderer/dispatcher.h:74: static Dispatcher* Get();
On 2015/06/19 23:14:58, kalman wrote:
> Can you avoid adding this if you pass the RenderFrame into the console
methods?

Done.

[not at google - send to devlin, 2015-06-22 21:20:16]

lgtm

https://codereview.chromium.org/1200503002/diff/60001/extensions/renderer/ext...
File extensions/renderer/extension_frame_helper.cc (right):

https://codereview.chromium.org/1200503002/diff/60001/extensions/renderer/ext...
extensions/renderer/extension_frame_helper.cc:102: content::RenderFrame*
ExtensionFrameHelper::RenderFrameForContext(
This method isn't needed anymore?

https://codereview.chromium.org/1200503002/diff/60001/extensions/renderer/scr...
File extensions/renderer/script_context_set.h (right):

https://codereview.chromium.org/1200503002/diff/60001/extensions/renderer/scr...
extensions/renderer/script_context_set.h:80: // Static equivalent of the above.
I'd rather a static Get() method of ScriptContextSet, it composes with all the
other instances methods on ScriptContextSet, while this doesn't.

[Devlin, 2015-06-22 21:23:12]

https://codereview.chromium.org/1200503002/diff/60001/extensions/renderer/scr...
File extensions/renderer/script_context_set.h (right):

https://codereview.chromium.org/1200503002/diff/60001/extensions/renderer/scr...
extensions/renderer/script_context_set.h:80: // Static equivalent of the above.
On 2015/06/22 21:20:15, kalman wrote:
> I'd rather a static Get() method of ScriptContextSet, it composes with all the
> other instances methods on ScriptContextSet, while this doesn't.

But since ScriptContextSet can be null while testing, then everything needs to
do:
ScriptContext* script_context = nullptr;
ScriptContextSet* script_context_set = ScriptContextSet::Get();
if (script_context_set)  // ScriptContextSet can be null while testing
  script_context = script_context->FromV8Context(context);

and that's just annoying. :/  Since this is (currently) the only thing we need
statically, I figured we'd start here, and abstract away the nasty "maybe null"
part.

[not at google - send to devlin, 2015-06-22 21:26:38]

https://codereview.chromium.org/1200503002/diff/60001/extensions/renderer/scr...
File extensions/renderer/script_context_set.h (right):

https://codereview.chromium.org/1200503002/diff/60001/extensions/renderer/scr...
extensions/renderer/script_context_set.h:80: // Static equivalent of the above.
On 2015/06/22 21:23:11, Devlin wrote:
> On 2015/06/22 21:20:15, kalman wrote:
> > I'd rather a static Get() method of ScriptContextSet, it composes with all
the
> > other instances methods on ScriptContextSet, while this doesn't.
> 
> But since ScriptContextSet can be null while testing, then everything needs to
> do:
> ScriptContext* script_context = nullptr;
> ScriptContextSet* script_context_set = ScriptContextSet::Get();
> if (script_context_set)  // ScriptContextSet can be null while testing
>   script_context = script_context->FromV8Context(context);
> 
> and that's just annoying. :/  Since this is (currently) the only thing we need
> statically, I figured we'd start here, and abstract away the nasty "maybe
null"
> part.

If you *promise* to pull ownership of ScriptContextSet off Dispatcher and into a
Singleton, ok.

[Devlin, 2015-06-22 23:39:50]

https://codereview.chromium.org/1200503002/diff/60001/extensions/renderer/ext...
File extensions/renderer/extension_frame_helper.cc (right):

https://codereview.chromium.org/1200503002/diff/60001/extensions/renderer/ext...
extensions/renderer/extension_frame_helper.cc:102: content::RenderFrame*
ExtensionFrameHelper::RenderFrameForContext(
On 2015/06/22 21:20:15, kalman wrote:
> This method isn't needed anymore?

Right, done.

[Devlin, 2015-06-22 23:42:47]

rdevlin.cronin@chromium.org changed reviewers:
+ dmazzoni@chromium.org, nasko@chromium.org, stevenjb@chromium.org,
timvolodine@chromium.org

[Devlin, 2015-06-22 23:42:47]

+Steven for c/b/chromeos, c/b/notifications, c/b/ui/app_list
+Tim for c/b/geolocation
+Dominic for c/b/speech/extension_api/
(Changing ExtensionMsg_AddMessageToConsole to
RenderFrameHost::AddMessageToConsole)

+Nasko for extension_messages and content/
(exposing AddMessageToConsole; the equivalent on RFH is already exposed).

[nasko, 2015-06-23 10:53:13]

https://codereview.chromium.org/1200503002/diff/80001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1200503002/diff/80001/content/renderer/render...
content/renderer/render_frame_impl.cc:1013:
IPC_MESSAGE_HANDLER(FrameMsg_AddMessageToConsole, AddMessageToConsole)
Let's leave OnAddMessageToConsole as the handler, which will just call the
AddMessageToConsole method. It keeps things consistent with IPC handlers and the
compiler should be smart enough to optimize it.
Additionally, we aren't making a private IPC handler a public method.

[stevenjb, 2015-06-23 15:49:01]

owner lgtm

[Devlin, 2015-06-23 16:49:19]

https://codereview.chromium.org/1200503002/diff/80001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1200503002/diff/80001/content/renderer/render...
content/renderer/render_frame_impl.cc:1013:
IPC_MESSAGE_HANDLER(FrameMsg_AddMessageToConsole, AddMessageToConsole)
On 2015/06/23 10:53:13, nasko (paris) wrote:
> Let's leave OnAddMessageToConsole as the handler, which will just call the
> AddMessageToConsole method. It keeps things consistent with IPC handlers and
the
> compiler should be smart enough to optimize it.
> Additionally, we aren't making a private IPC handler a public method.

Done.

[nasko, 2015-06-24 07:29:36]

content/ and IPC LGTM

[dmazzoni, 2015-06-24 17:48:59]

lgtm

[Devlin, 2015-06-24 19:29:53]

Patchset #5 (id:120001) has been deleted

[Devlin, 2015-06-24 20:28:26]

The CQ bit was checked by rdevlin.cronin@chromium.org

[Devlin, 2015-06-24 20:28:26]

The patchset sent to the CQ was uploaded after l-g-t-m from kalman@chromium.org,
stevenjb@chromium.org, dmazzoni@chromium.org, nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/1200503002/#ps140001
(title: "Latest master")

[commit-bot: I haz the power, 2015-06-24 20:28:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1200503002/140001

[commit-bot: I haz the power, 2015-06-24 20:36:14]

Committed patchset #5 (id:140001)

[commit-bot: I haz the power, 2015-06-24 20:37:08]

Patchset 5 (id:??) landed as
https://crrev.com/b2cec918668eb5b49df32d4983df84f777e9d37f
Cr-Commit-Position: refs/heads/master@{#335991}