Fixed some edit commands for OOPIF-<webivew> (Mac)

Fixed some edit commands for OOPIF-<webivew> (Mac)

OS edit commands for Mac are sent through WebContentsImpl which sends them to the
focused RenderFrame. This however does not play well with nested (inner)
WebContentses as GetFocusedFrame() returns the focused frame within the frame
tree of the WebContents only.

This CL fixes the issue with OOPIF-webviews by calling the corresponding edit
commands on the focused RenderWidgetHostDelegate (WebContentsImpl).

The CL also enables the WebViewInteractiveTest.EditCommands which was failing
on OOPIF-<webview>.

BUG=721984, 582562
Review-Url: https://codereview.chromium.org/2878153002
Cr-Commit-Position: refs/heads/master@{#472131}
Committed: https://chromium.googlesource.com/chromium/src/+/7b9e08e23f6e26b385619215fc80ac93d374a604

[EhsanK, 2017-05-13 03:27:49]

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

[commit-bot: I haz the power, 2017-05-13 03:28:01]

Dry run: CQ is trying da patch.

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

[EhsanK, 2017-05-13 03:29:26]

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

[EhsanK, 2017-05-13 03:29:27]

PTAL.

creis@: content/ review.

wjmaclean@: OWNER review for test.

Thanks!

[commit-bot: I haz the power, 2017-05-13 04:04:10]

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

[commit-bot: I haz the power, 2017-05-13 04:04:11]

Dry run: This issue passed the CQ dry run.

[Charlie Reis, 2017-05-15 21:36:58]

Thanks!  LGTM with one question about fallback behavior.

https://codereview.chromium.org/2878153002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view_mac.h (right):

https://codereview.chromium.org/2878153002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_mac.h:492: // when there
are focused inner WebContentses on the page.
Might mention it's null if there are no focused widgets, since I didn't expect
that at first.

(Will that change behavior in any cases, or is there no way for these commands
to work without a focused widget?  Just wondering if we should default to the
main delegate rather than null.)

[EhsanK, 2017-05-16 04:34:32]

Thanks Charlie! I tried to answer the question.

I still need wjmaclean's owner LGTM for the test.

Thanks. PTAL!

https://codereview.chromium.org/2878153002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view_mac.h (right):

https://codereview.chromium.org/2878153002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_mac.h:492: // when there
are focused inner WebContentses on the page.
On 2017/05/15 21:36:57, Charlie Reis wrote:
> Might mention it's null if there are no focused widgets, since I didn't expect
> that at first.
> 
> (Will that change behavior in any cases, or is there no way for these commands
> to work without a focused widget?  Just wondering if we should default to the
> main delegate rather than null.)

Yes. Done (fallback idea). Thanks!
But still, even with fallback plan, it seems possible that
|render_widget_host_->delegate()| might be nullptr (we were assuming this before
this patch too).

I am also not sure if I should first check if |render_widget_host_ != nullptr|
before calling |delegate()| but to be clear, we were not checking that before
either.

https://codereview.chromium.org/2878153002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/2878153002/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_mac.mm:730: return
render_widget_host_->delegate();
We were not checking |render_widget_host_ != nullptr| before so I am not doing
it either. 

That being said, there are cases where |render_widget_host_| could be nullptr,
e.g., RenderWidgetHostViewMac::ForwardMouseEvent checks for that. I believe for
popups it might be possible that |render_widget_host_| is null when mouse input
arrives. But for popups we do not need edit commands so (I guess) we are (and
have been) safe here.

[wjmaclean, 2017-05-16 11:57:34]

On 2017/05/16 04:34:32, EhsanK wrote:
> Thanks Charlie! I tried to answer the question.
> 
> I still need wjmaclean's owner LGTM for the test.
> 
> Thanks. PTAL!
> 
>
https://codereview.chromium.org/2878153002/diff/1/content/browser/renderer_ho...
> File content/browser/renderer_host/render_widget_host_view_mac.h (right):
> 
>
https://codereview.chromium.org/2878153002/diff/1/content/browser/renderer_ho...
> content/browser/renderer_host/render_widget_host_view_mac.h:492: // when there
> are focused inner WebContentses on the page.
> On 2017/05/15 21:36:57, Charlie Reis wrote:
> > Might mention it's null if there are no focused widgets, since I didn't
expect
> > that at first.
> > 
> > (Will that change behavior in any cases, or is there no way for these
commands
> > to work without a focused widget?  Just wondering if we should default to
the
> > main delegate rather than null.)
> 
> Yes. Done (fallback idea). Thanks!
> But still, even with fallback plan, it seems possible that
> |render_widget_host_->delegate()| might be nullptr (we were assuming this
before
> this patch too).
> 
> I am also not sure if I should first check if |render_widget_host_ != nullptr|
> before calling |delegate()| but to be clear, we were not checking that before
> either.
> 
>
https://codereview.chromium.org/2878153002/diff/20001/content/browser/rendere...
> File content/browser/renderer_host/render_widget_host_view_mac.mm (right):
> 
>
https://codereview.chromium.org/2878153002/diff/20001/content/browser/rendere...
> content/browser/renderer_host/render_widget_host_view_mac.mm:730: return
> render_widget_host_->delegate();
> We were not checking |render_widget_host_ != nullptr| before so I am not doing
> it either. 
> 
> That being said, there are cases where |render_widget_host_| could be nullptr,
> e.g., RenderWidgetHostViewMac::ForwardMouseEvent checks for that. I believe
for
> popups it might be possible that |render_widget_host_| is null when mouse
input
> arrives. But for popups we do not need edit commands so (I guess) we are (and
> have been) safe here.

lgtm

[EhsanK, 2017-05-16 13:53:53]

Thank you all! I will CQ later today.

[EhsanK, 2017-05-16 13:54:02]

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

[commit-bot: I haz the power, 2017-05-16 13:54:17]

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-16 14:28:14]

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

[commit-bot: I haz the power, 2017-05-16 14:28:15]

Dry run: This issue passed the CQ dry run.

[Charlie Reis, 2017-05-16 16:24:56]

https://codereview.chromium.org/2878153002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/2878153002/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_mac.mm:730: return
render_widget_host_->delegate();
On 2017/05/16 04:34:32, EhsanK wrote:
> We were not checking |render_widget_host_ != nullptr| before so I am not doing
> it either. 
> 
> That being said, there are cases where |render_widget_host_| could be nullptr,
> e.g., RenderWidgetHostViewMac::ForwardMouseEvent checks for that. I believe
for
> popups it might be possible that |render_widget_host_| is null when mouse
input
> arrives. But for popups we do not need edit commands so (I guess) we are (and
> have been) safe here.

Sounds fine.  Thanks!

[EhsanK, 2017-05-16 16:49:45]

The CQ bit was checked by ekaramad@chromium.org

[EhsanK, 2017-05-16 16:49:46]

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/2878153002/#ps20001
(title: "Addressing creis@'s comments")

[commit-bot: I haz the power, 2017-05-16 16:50:48]

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-16 16:58:40]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1494953385920120,
"parent_rev": "8d6cc389014e25bb903075473ef9bba3c50faf87", "commit_rev":
"7b9e08e23f6e26b385619215fc80ac93d374a604"}

[commit-bot: I haz the power, 2017-05-16 16:58:54]

Description was changed from

==========
Fixed some edit commands for OOPIF-<webivew> (Mac)

OS edit commands for Mac are sent through WebContentsImpl which sends them to
the
focused RenderFrame. This however does not play well with nested (inner)
WebContentses as GetFocusedFrame() returns the focused frame within the frame
tree of the WebContents only.

This CL fixes the issue with OOPIF-webviews by calling the corresponding edit
commands on the focused RenderWidgetHostDelegate (WebContentsImpl).

The CL also enables the WebViewInteractiveTest.EditCommands which was failing
on OOPIF-<webview>.

BUG=721984, 582562
==========

to

==========
Fixed some edit commands for OOPIF-<webivew> (Mac)

OS edit commands for Mac are sent through WebContentsImpl which sends them to
the
focused RenderFrame. This however does not play well with nested (inner)
WebContentses as GetFocusedFrame() returns the focused frame within the frame
tree of the WebContents only.

This CL fixes the issue with OOPIF-webviews by calling the corresponding edit
commands on the focused RenderWidgetHostDelegate (WebContentsImpl).

The CL also enables the WebViewInteractiveTest.EditCommands which was failing
on OOPIF-<webview>.

BUG=721984, 582562

Review-Url: https://codereview.chromium.org/2878153002
Cr-Commit-Position: refs/heads/master@{#472131}
Committed:
https://chromium.googlesource.com/chromium/src/+/7b9e08e23f6e26b385619215fc80...
==========

[commit-bot: I haz the power, 2017-05-16 16:58:55]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/7b9e08e23f6e26b385619215fc80...