OOPIF: Fix window.open to work from frames with remote parent.

OOPIF: Fix window.open to work from frames with remote parent.

This CL addresses these remaining issues with that scenario:

1. WebContentsImpl::CreateNewWindow kills a renderer opening a new
   window if the renderer's process doesn't match the process for
   WebContents' main frame.  This CL modifies this check to instead
   look at all renderer processes under the current WebContents.

2. The popup was getting created in the wrong SiteInstance,
   always using the SiteInstance of the WebContents' main frame rather
   than the SiteInstance of the source frame.  This CL fixes this by
   plumbing the source frame's SiteInstance into
   WebContentsImpl::CreateNewWindow.

3. Once created, the popup wasn't being shown.  This is because the
   message to show it (ViewHostMsg_ShowView) is sent via the opener's
   RenderView, which in this case is swapped out.  This caused
   RenderViewHostImpl::OnShowView to exit early because it checked
   is_active_.  To fix this, this CL removes this check from ShowView.
   Eventually, this IPC should be moved to RenderFrameHost.

BUG=463949, 225940
Committed: https://crrev.com/4cf2aa39e2220a3b5ef50a95fc11cf4ff35a85d5
Cr-Commit-Position: refs/heads/master@{#338949}

[alexmos, 2015-07-09 22:14:05]

alexmos@chromium.org changed reviewers:
+ creis@chromium.org

[alexmos, 2015-07-09 22:14:06]

Charlie, can you please take a look?  With this, we should be able to open
popups from OOP frames.

https://codereview.chromium.org/1224363002/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_view_host_impl.cc (left):

https://codereview.chromium.org/1224363002/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_view_host_impl.cc:1006: if (is_active_) {
Is it ok to remove these?  I saw you introduced them way back in
https://codereview.chromium.org/6319001/, but it seems creating a popup with a
remote parent is a valid scenario where this should go through despite
is_active_ being false, since this gets sent to the opener's (swapped-out) RV. 
The two other IPCs below are also routed to the opener's RV, so I updated them
too.

[Charlie Reis, 2015-07-09 23:42:51]

Nice-- I'm excited to have this case working!

This is probably ok as is, but do you have a sense for how much work it is to
move those IPCs to RenderFrameHost?  I'm wondering if it's cleaner to take care
of that now.

Otherwise a few nits and one other case to test.  Thanks!

https://codereview.chromium.org/1224363002/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_view_host_delegate.h (right):

https://codereview.chromium.org/1224363002/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_view_host_delegate.h:198: // should be
created associated with the given |route_id| in SiteInstance
nit: in the process of |source_site_instance|,

(Routing IDs are always process specific, so it's worth spelling out that the
process in this case is the one from the SiteInstance.)

https://codereview.chromium.org/1224363002/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_view_host_delegate.h:210: virtual void
CreateNewWindow(
General question: does this belong on RenderFrameHost(Delegate) instead?  It
doesn't seem to be particularly view-specific, and the message is going to come
from a frame.

That could be done in a separate CL, but we could leave a TODO here if it's the
right plan.

https://codereview.chromium.org/1224363002/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_view_host_impl.cc (left):

https://codereview.chromium.org/1224363002/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_view_host_impl.cc:1006: if (is_active_) {
On 2015/07/09 22:14:06, alexmos wrote:
> Is it ok to remove these?  I saw you introduced them way back in
> https://codereview.chromium.org/6319001/, but it seems creating a popup with a
> remote parent is a valid scenario where this should go through despite
> is_active_ being false, since this gets sent to the opener's (swapped-out) RV.

> The two other IPCs below are also routed to the opener's RV, so I updated them
> too.

I agree with your CL description that these should really move to
RenderFrameHost, where they can work whether the main frame is local or not.

In the meantime, it's not ideal to remove these checks, since they're intended
for racy situations with the main frame, where a swapped out page might be able
to show a popup.  I suppose that's not a huge problem if it happens, though.  We
can probably live with it if you have plans to move the IPCs over soon.

https://codereview.chromium.org/1224363002/diff/40001/content/browser/site_pe...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1224363002/diff/40001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:2718:
"window.domAutomationController.send(!!window.open('about:blank'));",
Can you also add a test for window.open to a cross-site URL?  That should work
and end up in a new process.  (If it requires additional work to support, we can
do it in a followup CL.)

https://codereview.chromium.org/1224363002/diff/40001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1224363002/diff/40001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:211: bool
FindMatchingProcess(int render_process_id,
nit: Put this up below CollectSites.

[alexmos, 2015-07-10 18:35:01]

https://codereview.chromium.org/1224363002/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_view_host_delegate.h (right):

https://codereview.chromium.org/1224363002/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_view_host_delegate.h:198: // should be
created associated with the given |route_id| in SiteInstance
On 2015/07/09 23:42:51, Charlie Reis wrote:
> nit: in the process of |source_site_instance|,
> 
> (Routing IDs are always process specific, so it's worth spelling out that the
> process in this case is the one from the SiteInstance.)

Done.

https://codereview.chromium.org/1224363002/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_view_host_delegate.h:210: virtual void
CreateNewWindow(
On 2015/07/09 23:42:51, Charlie Reis wrote:
> General question: does this belong on RenderFrameHost(Delegate) instead?  It
> doesn't seem to be particularly view-specific, and the message is going to
come
> from a frame.
> 
> That could be done in a separate CL, but we could leave a TODO here if it's
the
> right plan.

Yeah, I agree this should be moved to RenderFrameHostDelegate, and the
corresponding ViewHostMsg_CreateWindow should really be a FrameHostMsg sent to
the creator frame's RFH.  Added a TODO.

https://codereview.chromium.org/1224363002/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_view_host_impl.cc (left):

https://codereview.chromium.org/1224363002/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_view_host_impl.cc:1006: if (is_active_) {
On 2015/07/09 23:42:51, Charlie Reis wrote:
> On 2015/07/09 22:14:06, alexmos wrote:
> > Is it ok to remove these?  I saw you introduced them way back in
> > https://codereview.chromium.org/6319001/, but it seems creating a popup with
a
> > remote parent is a valid scenario where this should go through despite
> > is_active_ being false, since this gets sent to the opener's (swapped-out)
RV.
> 
> > The two other IPCs below are also routed to the opener's RV, so I updated
them
> > too.
> 
> I agree with your CL description that these should really move to
> RenderFrameHost, where they can work whether the main frame is local or not.
> 
> In the meantime, it's not ideal to remove these checks, since they're intended
> for racy situations with the main frame, where a swapped out page might be
able
> to show a popup.  I suppose that's not a huge problem if it happens, though. 
We
> can probably live with it if you have plans to move the IPCs over soon.

I took a look at it, and it should be relatively straightforward at least for
the ShowView IPC.  It is only sent for renderer-created popups, and we should be
able to save/get the opener frame (which should always be local for this case)
for sending this IPC.  I can try this in a follow-up CL.

I'm less certain how much work is needed for the other two IPCs below since I'm
not really familiar in how they're used.  Looks like ShowWidget is involved in
things like DateTimeChooseImpl/ColorChooserPopupUIController/PopupMenuImpl, and
ShowFullscreenWidget is sent from CreatePepperFullscreenContainer, but I don't
know anything about those.  Maybe I should leave the is_active_ check alone for
those two for now, since they aren't involved in the "new popup" flow?

https://codereview.chromium.org/1224363002/diff/40001/content/browser/site_pe...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1224363002/diff/40001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:2718:
"window.domAutomationController.send(!!window.open('about:blank'));",
On 2015/07/09 23:42:51, Charlie Reis wrote:
> Can you also add a test for window.open to a cross-site URL?  That should work
> and end up in a new process.  (If it requires additional work to support, we
can
> do it in a followup CL.)

Done.  It works as-is. :)  

I factored out the popup creation into a helper, OpenPopup, which is very close
to the one I introduced in render_frame_host_manager_browsertest.cc.  Is there a
good place to put this so that both files can use it?

https://codereview.chromium.org/1224363002/diff/40001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1224363002/diff/40001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:211: bool
FindMatchingProcess(int render_process_id,
On 2015/07/09 23:42:51, Charlie Reis wrote:
> nit: Put this up below CollectSites.

Done.

[Charlie Reis, 2015-07-10 21:44:49]

Great, thanks.  LGTM with nits!

https://codereview.chromium.org/1224363002/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_view_host_impl.cc (left):

https://codereview.chromium.org/1224363002/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_view_host_impl.cc:1006: if (is_active_) {
On 2015/07/10 18:35:00, alexmos wrote:
> On 2015/07/09 23:42:51, Charlie Reis wrote:
> > On 2015/07/09 22:14:06, alexmos wrote:
> > > Is it ok to remove these?  I saw you introduced them way back in
> > > https://codereview.chromium.org/6319001/, but it seems creating a popup
with
> a
> > > remote parent is a valid scenario where this should go through despite
> > > is_active_ being false, since this gets sent to the opener's (swapped-out)
> RV.
> > 
> > > The two other IPCs below are also routed to the opener's RV, so I updated
> them
> > > too.
> > 
> > I agree with your CL description that these should really move to
> > RenderFrameHost, where they can work whether the main frame is local or not.
> > 
> > In the meantime, it's not ideal to remove these checks, since they're
intended
> > for racy situations with the main frame, where a swapped out page might be
> able
> > to show a popup.  I suppose that's not a huge problem if it happens, though.

> We
> > can probably live with it if you have plans to move the IPCs over soon.
> 
> I took a look at it, and it should be relatively straightforward at least for
> the ShowView IPC.  It is only sent for renderer-created popups, and we should
be
> able to save/get the opener frame (which should always be local for this case)
> for sending this IPC.  I can try this in a follow-up CL.
> 
> I'm less certain how much work is needed for the other two IPCs below since
I'm
> not really familiar in how they're used.  Looks like ShowWidget is involved in
> things like DateTimeChooseImpl/ColorChooserPopupUIController/PopupMenuImpl,
and
> ShowFullscreenWidget is sent from CreatePepperFullscreenContainer, but I don't
> know anything about those.  Maybe I should leave the is_active_ check alone
for
> those two for now, since they aren't involved in the "new popup" flow?

Yeah, let's leave these two checks until we're ready to move those cases to
RenderFrameHost.

https://codereview.chromium.org/1224363002/diff/40001/content/browser/site_pe...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1224363002/diff/40001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:2718:
"window.domAutomationController.send(!!window.open('about:blank'));",
On 2015/07/10 18:35:00, alexmos wrote:
> On 2015/07/09 23:42:51, Charlie Reis wrote:
> > Can you also add a test for window.open to a cross-site URL?  That should
work
> > and end up in a new process.  (If it requires additional work to support, we
> can
> > do it in a followup CL.)
> 
> Done.  It works as-is. :)  
> 
> I factored out the popup creation into a helper, OpenPopup, which is very
close
> to the one I introduced in render_frame_host_manager_browsertest.cc.  Is there
a
> good place to put this so that both files can use it?

Yes, content_browser_test_utils_internal.h is probably the right place.  There's
also content_browser_test_utils.h if it gets used outside content.

https://codereview.chromium.org/1224363002/diff/60001/content/browser/rendere...
File content/browser/renderer_host/render_view_host_impl.cc (left):

https://codereview.chromium.org/1224363002/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_view_host_impl.cc:1015: if (is_active_)
Let's leave this.

https://codereview.chromium.org/1224363002/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_view_host_impl.cc:1021: if (is_active_)
Let's leave this.

https://codereview.chromium.org/1224363002/diff/60001/content/browser/site_pe...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1224363002/diff/60001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:79: Shell* OpenPopup(const
internal::ToRenderFrameHost& opener,
Move to content_browser_test_utils_internal (and share with RFHM browser test).

[alexmos, 2015-07-10 23:03:03]

Thanks!  You may want to take a quick look at the moved and slightly changed
OpenPopup as a sanity check.

https://codereview.chromium.org/1224363002/diff/60001/content/browser/rendere...
File content/browser/renderer_host/render_view_host_impl.cc (left):

https://codereview.chromium.org/1224363002/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_view_host_impl.cc:1015: if (is_active_)
On 2015/07/10 21:44:49, Charlie Reis wrote:
> Let's leave this.

Done.

https://codereview.chromium.org/1224363002/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_view_host_impl.cc:1021: if (is_active_)
On 2015/07/10 21:44:49, Charlie Reis wrote:
> Let's leave this.

Done.

https://codereview.chromium.org/1224363002/diff/60001/content/browser/site_pe...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1224363002/diff/60001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:79: Shell* OpenPopup(const
internal::ToRenderFrameHost& opener,
On 2015/07/10 21:44:49, Charlie Reis wrote:
> Move to content_browser_test_utils_internal (and share with RFHM browser
test).

Done.  Also refactored slightly to remove the EXPECTs here and check the
returned shell in the tests instead.

[Charlie Reis, 2015-07-11 00:12:08]

Thanks!  LGTM with nit.

https://codereview.chromium.org/1224363002/diff/80001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager_browsertest.cc
(right):

https://codereview.chromium.org/1224363002/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_browsertest.cc:570:
OpenPopup(shell()->web_contents(), GURL("about:blank"), "foo");
url::kAboutBlankURL (here and below)

[alexmos, 2015-07-13 21:40:03]

Hey Charlie, turns out that moving OpenPopup to
content_browser_test_utils_internal.cc introduced some build failures (see PS5
tryjobs), since OpenPopup uses ShellAddedObserver from
content_browser_test_utils.cc, which was part of a different build target.  I've
moved the two to be part of the same target -- please take a look at PS7 and PS8
to see if that makes sense.  Trybots seem happy with it.

[Charlie Reis, 2015-07-13 22:40:45]

On 2015/07/13 21:40:03, alexmos wrote:
> Hey Charlie, turns out that moving OpenPopup to
> content_browser_test_utils_internal.cc introduced some build failures (see PS5
> tryjobs), since OpenPopup uses ShellAddedObserver from
> content_browser_test_utils.cc, which was part of a different build target. 
I've
> moved the two to be part of the same target -- please take a look at PS7 and
PS8
> to see if that makes sense.  Trybots seem happy with it.

Hmm, I'm not confident in that build change.  It seems like we should be able to
solve this with a dependency rule, but someone more familiar with the test build
targets would know better.

And actually, ShellAddedObserver doesn't appear to be used outside content...? 
Maybe we can just move that within content_browser_tests_internal?

[alexmos, 2015-07-13 23:47:52]

On 2015/07/13 22:40:45, Charlie Reis wrote:
> On 2015/07/13 21:40:03, alexmos wrote:
> > Hey Charlie, turns out that moving OpenPopup to
> > content_browser_test_utils_internal.cc introduced some build failures (see
PS5
> > tryjobs), since OpenPopup uses ShellAddedObserver from
> > content_browser_test_utils.cc, which was part of a different build target. 
> I've
> > moved the two to be part of the same target -- please take a look at PS7 and
> PS8
> > to see if that makes sense.  Trybots seem happy with it.
> 
> Hmm, I'm not confident in that build change.  It seems like we should be able
to
> solve this with a dependency rule, but someone more familiar with the test
build
> targets would know better.
> 
> And actually, ShellAddedObserver doesn't appear to be used outside content...?

> Maybe we can just move that within content_browser_tests_internal?

That seemed like a good idea, but unfortunately when I tried it, it broke at
least some unit test targets in GN builds (e.g., content_unittests,
extensions_unittests) during link:
../../content/test/content_browser_test_utils_internal.cc:227: error: undefined
reference to 'content::Shell::SetShellCreatedCallback(base::Callback<void
(content::Shell*)>)'

which seems like it also needs build config changes. (It also required adding
content_browser_test_utils_internal.h to a bunch of tests.)

The rationale behind my change was that content_browser_test_utils.cc was being
built as part of the "content_browser_test_support", which is only used to build
content_browsertests.  content_browser_test_utils_internal.cc, however, is built
as part of "test_support_content" which is used not only for
content_browsertests, but also for a bunch of other targets, including
content_unittests, content_perftests, content_gl_tests, etc.  And the build
error was not being to find ShellAddedObserver for OpenPopup when building
something like content_unittests (which shouldn't need OpenPopup in the first
place).  Since the stuff in content_browser_test_utils_internal.cc only seems to
be used in content_browsertests, I moved it over to the narrower
"content_browser_test_support" (and updated the corresponding GN target as
well), given that there appeared to be no good reason for it to be needed in the
other test targets.  That said, I don't really understand gn or gyp well enough
to know whether what I'm doing really makes sense.  I also tried some stuff with
dependencies and ran into a cycle. :)  Who would be more familiar with test
build targets?

[Charlie Reis, 2015-07-13 23:56:14]

creis@chromium.org changed reviewers:
+ phajdan.jr@chromium.org

[Charlie Reis, 2015-07-13 23:57:25]

On 2015/07/13 23:47:52, alexmos wrote:
> On 2015/07/13 22:40:45, Charlie Reis wrote:
> > On 2015/07/13 21:40:03, alexmos wrote:
> > > Hey Charlie, turns out that moving OpenPopup to
> > > content_browser_test_utils_internal.cc introduced some build failures (see
> PS5
> > > tryjobs), since OpenPopup uses ShellAddedObserver from
> > > content_browser_test_utils.cc, which was part of a different build target.

> > I've
> > > moved the two to be part of the same target -- please take a look at PS7
and
> > PS8
> > > to see if that makes sense.  Trybots seem happy with it.
> > 
> > Hmm, I'm not confident in that build change.  It seems like we should be
able
> to
> > solve this with a dependency rule, but someone more familiar with the test
> build
> > targets would know better.
> > 
> > And actually, ShellAddedObserver doesn't appear to be used outside
content...?
> 
> > Maybe we can just move that within content_browser_tests_internal?
> 
> That seemed like a good idea, but unfortunately when I tried it, it broke at
> least some unit test targets in GN builds (e.g., content_unittests,
> extensions_unittests) during link:
> ../../content/test/content_browser_test_utils_internal.cc:227: error:
undefined
> reference to 'content::Shell::SetShellCreatedCallback(base::Callback<void
> (content::Shell*)>)'
> 
> which seems like it also needs build config changes. (It also required adding
> content_browser_test_utils_internal.h to a bunch of tests.)
> 
> The rationale behind my change was that content_browser_test_utils.cc was
being
> built as part of the "content_browser_test_support", which is only used to
build
> content_browsertests.  content_browser_test_utils_internal.cc, however, is
built
> as part of "test_support_content" which is used not only for
> content_browsertests, but also for a bunch of other targets, including
> content_unittests, content_perftests, content_gl_tests, etc.  And the build
> error was not being to find ShellAddedObserver for OpenPopup when building
> something like content_unittests (which shouldn't need OpenPopup in the first
> place).  Since the stuff in content_browser_test_utils_internal.cc only seems
to
> be used in content_browsertests, I moved it over to the narrower
> "content_browser_test_support" (and updated the corresponding GN target as
> well), given that there appeared to be no good reason for it to be needed in
the
> other test targets.  That said, I don't really understand gn or gyp well
enough
> to know whether what I'm doing really makes sense.  I also tried some stuff
with
> dependencies and ran into a cycle. :)  Who would be more familiar with test
> build targets?

phajdan.jr@: Can you offer some advice on this?

[alexmos, 2015-07-15 15:32:35]

Pawel - friendly ping

[Paweł Hajdan Jr., 2015-07-15 17:30:33]

I'm fine with the gyp/GN changes (content_browser_test_support is more specific
than test_support_content so moving
content/test/content_browser_test_utils_internal to a more specific target seems
OK).

Sorry for the delay, above was somewhat non-trivial to thoroughly check.

Just one minor question.

https://codereview.chromium.org/1224363002/diff/140001/content/test/content_b...
File content/test/content_browser_test_utils_internal.h (right):

https://codereview.chromium.org/1224363002/diff/140001/content/test/content_b...
content/test/content_browser_test_utils_internal.h:17: #include
"content/public/test/browser_test_utils.h"
Why is this needed?

[alexmos, 2015-07-15 17:40:23]

Thanks!

https://codereview.chromium.org/1224363002/diff/140001/content/test/content_b...
File content/test/content_browser_test_utils_internal.h (right):

https://codereview.chromium.org/1224363002/diff/140001/content/test/content_b...
content/test/content_browser_test_utils_internal.h:17: #include
"content/public/test/browser_test_utils.h"
On 2015/07/15 17:30:33, Paweł Hajdan Jr. wrote:
> Why is this needed?

For internal::ToRenderFrameHost, which is used in OpenPopup's first argument.

[Paweł Hajdan Jr., 2015-07-15 17:50:15]

https://codereview.chromium.org/1224363002/diff/140001/content/test/content_b...
File content/test/content_browser_test_utils_internal.h (right):

https://codereview.chromium.org/1224363002/diff/140001/content/test/content_b...
content/test/content_browser_test_utils_internal.h:17: #include
"content/public/test/browser_test_utils.h"
On 2015/07/15 at 17:40:23, alexmos wrote:
> On 2015/07/15 17:30:33, Paweł Hajdan Jr. wrote:
> > Why is this needed?
> 
> For internal::ToRenderFrameHost, which is used in OpenPopup's first argument.

Oh. internal:: suggests it should not be used outside of that file. However, I
see no reason for that (FWIW it was added in
https://chromiumcodereview.appspot.com/11753009).

Could you remove the internal namespace from
content/public/test/browser_test_utils.h and forward-declare ToRenderFrameHost
in content/test/content_browser_test_utils_internal.h ?

[alexmos, 2015-07-15 18:29:04]

https://codereview.chromium.org/1224363002/diff/140001/content/test/content_b...
File content/test/content_browser_test_utils_internal.h (right):

https://codereview.chromium.org/1224363002/diff/140001/content/test/content_b...
content/test/content_browser_test_utils_internal.h:17: #include
"content/public/test/browser_test_utils.h"
On 2015/07/15 17:50:15, Paweł Hajdan Jr. wrote:
> On 2015/07/15 at 17:40:23, alexmos wrote:
> > On 2015/07/15 17:30:33, Paweł Hajdan Jr. wrote:
> > > Why is this needed?
> > 
> > For internal::ToRenderFrameHost, which is used in OpenPopup's first
argument.
> 
> Oh. internal:: suggests it should not be used outside of that file. However, I
> see no reason for that (FWIW it was added in
> https://chromiumcodereview.appspot.com/11753009).
> 
> Could you remove the internal namespace from
> content/public/test/browser_test_utils.h and forward-declare ToRenderFrameHost
> in content/test/content_browser_test_utils_internal.h ?

Done.  Good idea.

[Paweł Hajdan Jr., 2015-07-15 22:18:42]

LGTM

[alexmos, 2015-07-15 23:31:50]

The CQ bit was checked by alexmos@chromium.org

[alexmos, 2015-07-15 23:31:50]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org
Link to the patchset: https://codereview.chromium.org/1224363002/#ps180001
(title: "Remove internal:: from ToRenderFrameHost")

[commit-bot: I haz the power, 2015-07-15 23:32:10]

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

[commit-bot: I haz the power, 2015-07-15 23:40:51]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-07-15 23:42:54]

Patchset 10 (id:??) landed as
https://crrev.com/4cf2aa39e2220a3b5ef50a95fc11cf4ff35a85d5
Cr-Commit-Position: refs/heads/master@{#338949}