Move external popup menus from WebViewClient to WebFrameClient, part 3/3.

Move external popup menus from WebViewClient to WebFrameClient, part 3/3.

BUG=304341
TEST=popup menus still work

Committed: https://crrev.com/485e5fd678f00a5e15798b1d66a9acd4368d3395
Cr-Commit-Position: refs/heads/master@{#291781}

[Avi (use Gerrit), 2014-08-23 01:52:59]

Keishi, you changed the Android code a few months ago; can you look at the
Android code?

Fady, this should be pretty straightforward; can you look at the
BrowserPluginGuest code?

Nasko, everything else, if you would.

[Fady Samuel, 2014-08-23 10:56:17]

browser_plugin lgtm

https://codereview.chromium.org/501583003/diff/60001/content/browser/browser_...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://codereview.chromium.org/501583003/diff/60001/content/browser/browser_...
content/browser/browser_plugin/browser_plugin_guest.cc:470:
render_frame_message_source_ = render_frame_host;
nit: There's no way to pass this along via the macro? This is a bit ugly.

[Avi (use Gerrit), 2014-08-23 18:36:44]

https://codereview.chromium.org/501583003/diff/60001/content/browser/browser_...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://codereview.chromium.org/501583003/diff/60001/content/browser/browser_...
content/browser/browser_plugin/browser_plugin_guest.cc:470:
render_frame_message_source_ = render_frame_host;
On 2014/08/23 10:56:17, Fady Samuel wrote:
> nit: There's no way to pass this along via the macro? This is a bit ugly.

No. If you take a look at WebContentsImpl::OnMessageReceived (the version that
takes both a RFH and a RVH), that's what it does. It's ugly, yeah. Mojo will
surely save us.

[Avi (use Gerrit), 2014-08-24 23:00:07]

https://codereview.chromium.org/501583003/diff/60001/content/browser/browser_...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://codereview.chromium.org/501583003/diff/60001/content/browser/browser_...
content/browser/browser_plugin/browser_plugin_guest.cc:470:
render_frame_message_source_ = render_frame_host;
On 2014/08/23 18:36:44, Avi wrote:
> On 2014/08/23 10:56:17, Fady Samuel wrote:
> > nit: There's no way to pass this along via the macro? This is a bit ugly.
> 
> No. If you take a look at WebContentsImpl::OnMessageReceived (the version that
> takes both a RFH and a RVH), that's what it does. It's ugly, yeah. Mojo will
> surely save us.

Actually, IPC_BEGIN_MESSAGE_MAP_WITH_PARAM works. Thanks for the nudge.

[Fady Samuel, 2014-08-25 00:58:05]

On 2014/08/24 23:00:07, Avi wrote:
>
https://codereview.chromium.org/501583003/diff/60001/content/browser/browser_...
> File content/browser/browser_plugin/browser_plugin_guest.cc (right):
> 
>
https://codereview.chromium.org/501583003/diff/60001/content/browser/browser_...
> content/browser/browser_plugin/browser_plugin_guest.cc:470:
> render_frame_message_source_ = render_frame_host;
> On 2014/08/23 18:36:44, Avi wrote:
> > On 2014/08/23 10:56:17, Fady Samuel wrote:
> > > nit: There's no way to pass this along via the macro? This is a bit ugly.
> > 
> > No. If you take a look at WebContentsImpl::OnMessageReceived (the version
that
> > takes both a RFH and a RVH), that's what it does. It's ugly, yeah. Mojo will
> > surely save us.
> 
> Actually, IPC_BEGIN_MESSAGE_MAP_WITH_PARAM works. Thanks for the nudge.

Thanks for digging deeper into this. Still lgtm.

[keishi, 2014-08-25 03:59:23]

The android code lgtm.

[nasko, 2014-08-25 16:54:13]

Mostly nits.

https://codereview.chromium.org/501583003/diff/80001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/501583003/diff/80001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:1015:
render_view_host_->delegate_->GetDelegateView();
Shouldn't we be going through RenderframeHostDelegate and getting a view from
there? It feels strange to continue going through RVHD. I'm ok with putting it
in a separate CL to avoid making this one even bigger, but let's put a TODO.

Alternatively, just use the delegate only and it can forward to the view itself.

https://codereview.chromium.org/501583003/diff/80001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:1254: #endif
nit: You used #elif in other places, any reason not to keep that one consistent?

https://codereview.chromium.org/501583003/diff/80001/content/renderer/render_...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/501583003/diff/80001/content/renderer/render_...
content/renderer/render_frame_impl.h:459: friend class ExternalPopupMenuTest;
Is this line needed if the test cases themselves are marked as friends?

[Avi (use Gerrit), 2014-08-25 18:17:34]

https://codereview.chromium.org/501583003/diff/80001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/501583003/diff/80001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:1015:
render_view_host_->delegate_->GetDelegateView();
On 2014/08/25 16:54:13, nasko wrote:
> Shouldn't we be going through RenderframeHostDelegate and getting a view from
> there? It feels strange to continue going through RVHD.

This patch got big quickly, so I wanted to cut off the change dependency there.

Re a TODO, search for "render_view_host_->delegate_" in RFHI. There's plenty of
RFHI using RVHI's delegate with no notes. Everyone knows we're going to kill it.

https://codereview.chromium.org/501583003/diff/80001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:1254: #endif
On 2014/08/25 16:54:12, nasko wrote:
> nit: You used #elif in other places, any reason not to keep that one
consistent?

Done.

https://codereview.chromium.org/501583003/diff/80001/content/renderer/render_...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/501583003/diff/80001/content/renderer/render_...
content/renderer/render_frame_impl.h:459: friend class ExternalPopupMenuTest;
On 2014/08/25 16:54:13, nasko wrote:
> Is this line needed if the test cases themselves are marked as friends?

In this case, no, but in the general case, not necessarily. For example, the
friending of the class RendererAccessibilityTest below is required because the
test base class is doing the fiddling rather than any particular test.

[nasko, 2014-08-25 18:27:00]

LGTM

[Avi (use Gerrit), 2014-08-25 18:34:11]

The CQ bit was checked by avi@chromium.org

[commit-bot: I haz the power, 2014-08-25 18:34:52]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/avi@chromium.org/501583003/100001

[commit-bot: I haz the power, 2014-08-25 19:25:07]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)

[commit-bot: I haz the power, 2014-08-25 19:26:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-25 19:26:47]

Try jobs failed on following builders:
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[Avi (use Gerrit), 2014-08-25 20:20:06]

The CQ bit was checked by avi@chromium.org

[commit-bot: I haz the power, 2014-08-25 20:20:34]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/avi@chromium.org/501583003/120001

[commit-bot: I haz the power, 2014-08-25 21:18:57]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[commit-bot: I haz the power, 2014-08-25 22:34:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-25 22:34:47]

Try jobs failed on following builders:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[Avi (use Gerrit), 2014-08-25 22:39:23]

The CQ bit was checked by avi@chromium.org

[commit-bot: I haz the power, 2014-08-25 22:41:08]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/avi@chromium.org/501583003/120001

[commit-bot: I haz the power, 2014-08-25 23:26:50]

Committed patchset #7 (120001) as 5b2401ba2612aa0dc79d260d533fc47fff5d156d

[commit-bot: I haz the power, 2014-09-10 02:38:18]

Patchset 7 (id:??) landed as
https://crrev.com/485e5fd678f00a5e15798b1d66a9acd4368d3395
Cr-Commit-Position: refs/heads/master@{#291781}