Add machinery to show touch editing context menus in OOPIFs.

Add machinery to show touch editing context menus in OOPIFs.

This CL adds the ability to WebFrameWidgetBase to show context
menus for touch-selection editing. It's based on the existing
implementation via WebViewImpl::ShowContextMenu(), but moves
the implementation to WebFrameWidgetBase, with access via a
single virtual function on WebWidget.

BUG=470662
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2894043002
Cr-Commit-Position: refs/heads/master@{#475125}
Committed: https://chromium.googlesource.com/chromium/src/+/ee244e0d9db7e2d348cef7f5b979b51336e18008

[wjmaclean, 2017-05-18 21:48:19]

Description was changed from

==========
Add machinery to show touch editing context menus in OOPIFs.

<better description coming>

BUG=tbd
==========

to

==========
Add machinery to show touch editing context menus in OOPIFs.

<better description coming>

BUG=tbd
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[wjmaclean, 2017-05-23 17:51:08]

This won't land until after https://codereview.chromium.org/2883653002/, but I
suspect we can start review in parallel.

kenrb@ - could you please take a look?
dcheng@ - would you look at the Blink changes?

[wjmaclean, 2017-05-23 20:50:52]

Description was changed from

==========
Add machinery to show touch editing context menus in OOPIFs.

<better description coming>

BUG=tbd
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Add machinery to show touch editing context menus in OOPIFs.

<better description coming>

BUG=tbd
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[wjmaclean, 2017-05-23 20:50:52]

wjmaclean@chromium.org changed reviewers:
+ dcheng@chromium.org, ekaramad@google.com, kenrb@chromium.org

[wjmaclean, 2017-05-23 20:51:52]

This won't land until after https://codereview.chromium.org/2883653002/, but I
suspect we can start review in parallel.

kenrb@ - could you please take a look?
dcheng@ - would you look at the Blink changes?
ekaramad@ - any issues with non-existence of the WebFrameWidgetBase behind
GetWebWidget()?

[EhsanK, 2017-05-23 22:31:22]

ekaramad@chromium.org changed reviewers:
+ ekaramad@chromium.org

[EhsanK, 2017-05-23 22:31:23]

https://codereview.chromium.org/2894043002/diff/40001/content/browser/rendere...
File
content/browser/renderer_host/input/touch_selection_controller_client_child_frame.cc
(right):

https://codereview.chromium.org/2894043002/diff/40001/content/browser/rendere...
content/browser/renderer_host/input/touch_selection_controller_client_child_frame.cc:187:
host->Send(new FrameMsg_ShowContextMenu(host->GetRoutingID(),
Just curious if host->ShowContextMenuAtPoint() would also work?

https://codereview.chromium.org/2894043002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/2894043002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebViewImpl.cpp:3510: NOTREACHED();
To me it looks like this could in fact hit. Here is the scenario I am thinking
of:

1- Browser calls TSCCA::RunContextMenu() and the IPC is sent to the renderer
process.

2- At the same time, renderer navigates to a cross origin site so RenderViewImpl
swaps out.

3- IPC arrives at RenderWidget which no longer has a WebFrameWidget so
GetWebWidget() returns a WebViewImpl.

A though: Is it possible to send the IPC to RenderFrameImpl instead? In
RenderFrameImpl there is a deterministic way of getting the WebFrameWidget.

[wjmaclean, 2017-05-24 13:05:52]

https://codereview.chromium.org/2894043002/diff/40001/content/browser/rendere...
File
content/browser/renderer_host/input/touch_selection_controller_client_child_frame.cc
(right):

https://codereview.chromium.org/2894043002/diff/40001/content/browser/rendere...
content/browser/renderer_host/input/touch_selection_controller_client_child_frame.cc:187:
host->Send(new FrameMsg_ShowContextMenu(host->GetRoutingID(),
On 2017/05/23 22:31:23, EhsanK wrote:
> Just curious if host->ShowContextMenuAtPoint() would also work?

It might ... in preparing this CL I followed the existing pathway in use for
TSE, but I did notice the other ... I'm not sure if they're equivalent or not.

https://codereview.chromium.org/2894043002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/2894043002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebViewImpl.cpp:3510: NOTREACHED();
On 2017/05/23 22:31:23, EhsanK wrote:
> To me it looks like this could in fact hit. Here is the scenario I am thinking
> of:
> 
> 1- Browser calls TSCCA::RunContextMenu() and the IPC is sent to the renderer
> process.
> 
> 2- At the same time, renderer navigates to a cross origin site so
RenderViewImpl
> swaps out.
> 
> 3- IPC arrives at RenderWidget which no longer has a WebFrameWidget so
> GetWebWidget() returns a WebViewImpl.

So then, would the previous patchset of this CL, in which
WebViewImpl::ShowContextMenu() then calls the WebFrameWidget implementation, be
better?

> A though: Is it possible to send the IPC to RenderFrameImpl instead? In
> RenderFrameImpl there is a deterministic way of getting the WebFrameWidget.

That's a possibility if the method from the previous patchset doesn't work.

[EhsanK, 2017-05-24 14:12:46]

On 2017/05/24 13:05:52, wjmaclean wrote:
>
https://codereview.chromium.org/2894043002/diff/40001/content/browser/rendere...
> File
>
content/browser/renderer_host/input/touch_selection_controller_client_child_frame.cc
> (right):
> 
>
https://codereview.chromium.org/2894043002/diff/40001/content/browser/rendere...
>
content/browser/renderer_host/input/touch_selection_controller_client_child_frame.cc:187:
> host->Send(new FrameMsg_ShowContextMenu(host->GetRoutingID(),
> On 2017/05/23 22:31:23, EhsanK wrote:
> > Just curious if host->ShowContextMenuAtPoint() would also work?
> 
> It might ... in preparing this CL I followed the existing pathway in use for
> TSE, but I did notice the other ... I'm not sure if they're equivalent or not.
> 
>
https://codereview.chromium.org/2894043002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/web/WebViewImpl.cpp (right):
> 
>
https://codereview.chromium.org/2894043002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/web/WebViewImpl.cpp:3510: NOTREACHED();
> On 2017/05/23 22:31:23, EhsanK wrote:
> > To me it looks like this could in fact hit. Here is the scenario I am
thinking
> > of:
> > 
> > 1- Browser calls TSCCA::RunContextMenu() and the IPC is sent to the renderer
> > process.
> > 
> > 2- At the same time, renderer navigates to a cross origin site so
> RenderViewImpl
> > swaps out.
> > 
> > 3- IPC arrives at RenderWidget which no longer has a WebFrameWidget so
> > GetWebWidget() returns a WebViewImpl.
> 
> So then, would the previous patchset of this CL, in which
> WebViewImpl::ShowContextMenu() then calls the WebFrameWidget implementation,
be
> better?
> 
> > A though: Is it possible to send the IPC to RenderFrameImpl instead? In
> > RenderFrameImpl there is a deterministic way of getting the WebFrameWidget.
> 
> That's a possibility if the method from the previous patchset doesn't work.
I think it should work except that we should check if MainFrameImpl() exists.
If the scenario above happens, WebViewImpl cannot have a WebLocalFrame as main
frame.

Also I believe a WebLocalFrame::FrameWidget() should always be non-null so long
as
WebLocalFrame itself is a local root.

[wjmaclean, 2017-05-25 16:14:54]

kenrb@ - could you take a look please? It's much simpler than it's size would
indicate ;-)

[wjmaclean, 2017-05-25 16:39:09]

bokan@ - did you want to look at the Blink parts of this? I noticed you just
reviewed some quite-related changes in this part of the codebase.

[wjmaclean, 2017-05-25 19:41:53]

wjmaclean@chromium.org changed reviewers:
+ bokan@chromium.org, creis@chromium.org

[wjmaclean, 2017-05-25 19:42:38]

bokan@ - did you want to look at the Blink parts of this? I noticed you just
reviewed some quite-related changes in this part of the codebase. (resending as
I forgot to add bokan@ to reviewers first ...)

[wjmaclean, 2017-05-25 19:46:49]

Description was changed from

==========
Add machinery to show touch editing context menus in OOPIFs.

<better description coming>

BUG=tbd
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Add machinery to show touch editing context menus in OOPIFs.

This CL adds the ability to WebFrameWidgetBase to show context
menus for touch-selection editing. It's based on the existing
implementation via WebViewImpl::ShowContextMenu(), but moves
the implementation to WebFrameWidgetBase, with access via a
single virtual function on WebWidget.

Since context menus are now frame-specific, we change the
ViewMsg to be a FrameMsg.

BUG=470662
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[wjmaclean, 2017-05-25 20:25:54]

The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-25 20:26:34]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-25 22:07:10]

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

[commit-bot: I haz the power, 2017-05-25 22:07:11]

Dry run: This issue passed the CQ dry run.

[kenrb, 2017-05-26 01:43:53]

This is fine, except I don't agree with changing it from a ViewMsg_ to a
FrameMsg_. FrameMsg_* is handled by RenderFrameImpl, not RenderWidget. On the
other hand, RenderWidget does have several ViewMsg_ handlers. Really, ViewMsgs
should be turned into either PageMsgs or WidgetMsgs, but the latter doesn't
exist, so it's fine to leave as ViewMsg for now.

[wjmaclean, 2017-05-26 11:49:20]

wjmaclean@chromium.org changed reviewers:
+ avi@chromium.org

[wjmaclean, 2017-05-26 12:02:52]

On 2017/05/26 01:43:53, kenrb wrote:
> This is fine, except I don't agree with changing it from a ViewMsg_ to a
> FrameMsg_. FrameMsg_* is handled by RenderFrameImpl, not RenderWidget. On the
> other hand, RenderWidget does have several ViewMsg_ handlers. Really, ViewMsgs
> should be turned into either PageMsgs or WidgetMsgs, but the latter doesn't
> exist, so it's fine to leave as ViewMsg for now.

Ok, I'll revert it to a ViewMsg then  ... thanks!

avi@ ... can you please take a look for content/ owners (I'll add the conversion
to ViewMsg later this morning as I'm currently having network issues where I'm
located)?

[wjmaclean, 2017-05-26 13:21:46]

The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-26 13:22:02]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[wjmaclean, 2017-05-26 13:22:24]

Description was changed from

==========
Add machinery to show touch editing context menus in OOPIFs.

This CL adds the ability to WebFrameWidgetBase to show context
menus for touch-selection editing. It's based on the existing
implementation via WebViewImpl::ShowContextMenu(), but moves
the implementation to WebFrameWidgetBase, with access via a
single virtual function on WebWidget.

Since context menus are now frame-specific, we change the
ViewMsg to be a FrameMsg.

BUG=470662
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Add machinery to show touch editing context menus in OOPIFs.

This CL adds the ability to WebFrameWidgetBase to show context
menus for touch-selection editing. It's based on the existing
implementation via WebViewImpl::ShowContextMenu(), but moves
the implementation to WebFrameWidgetBase, with access via a
single virtual function on WebWidget.

BUG=470662
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2017-05-26 13:24:12]

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

[commit-bot: I haz the power, 2017-05-26 13:24:13]

Dry run: Try jobs failed on following builders:
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Avi (use Gerrit), 2017-05-26 14:17:13]

lgtm

[wjmaclean, 2017-05-26 14:31:51]

The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-26 14:32:08]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bokan, 2017-05-26 14:41:58]

lgtm

+cc: yosin@, amaralp@ as FYI

https://codereview.chromium.org/2894043002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/2894043002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebViewImpl.cpp:3506: // If MainFrameImpl() is
non-null, then FrameWidget() will also be non-null.
Maybe make this a DCHECK?

[bokan, 2017-05-26 14:43:27]

+cc: yosin@, amaralp@ as FYI (for real this time)

[commit-bot: I haz the power, 2017-05-26 14:50:39]

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

[commit-bot: I haz the power, 2017-05-26 14:50:40]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[wjmaclean, 2017-05-26 14:59:30]

The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-26 14:59:44]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-26 16:38:44]

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

[commit-bot: I haz the power, 2017-05-26 16:38:45]

Dry run: This issue passed the CQ dry run.

[dcheng, 2017-05-26 17:18:46]

(I know rbyers is looking at this CL now, but going to leave a few drive-bys)

https://codereview.chromium.org/2894043002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/WebFrameWidgetBase.cpp (right):

https://codereview.chromium.org/2894043002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/WebFrameWidgetBase.cpp:298:
GetPage()->GetFocusController().FocusedOrMainFrame())) {
I think this should only be calling FocusedFrame()--if there's some IPC race
that causes the focused frame to be null, I don't think we want to fall back to
the main frame by default. This will also remove the need to downcast.

(Looking through this file, it looks like a more systemic issue that we should
fix)

https://codereview.chromium.org/2894043002/diff/140001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebWidget.h (right):

https://codereview.chromium.org/2894043002/diff/140001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebWidget.h:37: #include "WebTextDirection.h"
Btw since clang-format is changing these anyway, do you mind qualifying them
with public/platform?

(sorry, scope creep)

[wjmaclean, 2017-05-26 18:04:54]

dcheng@ - comments addressed, ptal?

[dcheng, 2017-05-26 18:22:28]

https://codereview.chromium.org/2894043002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/2894043002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebViewImpl.cpp:3508:
MainFrameImpl()->FrameWidget()->ShowContextMenu(source_type);
This frame widget is actually a WebViewFrameWidget. Since the default on
WebWidget is unimplemented, that means this won't do anything.

If we want to share behavior, I guess we should:
- move the method to WebFrameWidget instead of WebWidget
- put the original logic WebFrameWidgetBase and have WebViewFrameWidget and
WebFrameWidgetImpl both delegate to it.

WDYT?

[dcheng, 2017-05-26 18:34:29]

It's been pointed out to me that I can't read, and that this patch in fact
modifies WebFrameWidgetBase not WebFrameWidgetImpl. LGTM!

[wjmaclean, 2017-05-26 18:35:36]

The CQ bit was checked by wjmaclean@chromium.org

[wjmaclean, 2017-05-26 18:35:37]

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org,
bokan@chromium.org
Link to the patchset: https://codereview.chromium.org/2894043002/#ps160001
(title: "Don't fall back to mainframe, formatting.")

[commit-bot: I haz the power, 2017-05-26 18:36:16]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-26 21:06:10]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1495823736621140,
"parent_rev": "edf819c3372116d9a471502226903d4ed73dcf7a", "commit_rev":
"ee244e0d9db7e2d348cef7f5b979b51336e18008"}

[commit-bot: I haz the power, 2017-05-26 21:06:26]

Description was changed from

==========
Add machinery to show touch editing context menus in OOPIFs.

This CL adds the ability to WebFrameWidgetBase to show context
menus for touch-selection editing. It's based on the existing
implementation via WebViewImpl::ShowContextMenu(), but moves
the implementation to WebFrameWidgetBase, with access via a
single virtual function on WebWidget.

BUG=470662
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Add machinery to show touch editing context menus in OOPIFs.

This CL adds the ability to WebFrameWidgetBase to show context
menus for touch-selection editing. It's based on the existing
implementation via WebViewImpl::ShowContextMenu(), but moves
the implementation to WebFrameWidgetBase, with access via a
single virtual function on WebWidget.

BUG=470662
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2894043002
Cr-Commit-Position: refs/heads/master@{#475125}
Committed:
https://chromium.googlesource.com/chromium/src/+/ee244e0d9db7e2d348cef7f5b979...
==========

[commit-bot: I haz the power, 2017-05-26 21:06:27]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/ee244e0d9db7e2d348cef7f5b979...