Almost finish moving context_menu_node_ from RenderViewImpl to RenderFrameImpl.

Almost finish moving context_menu_node_ from RenderViewImpl to RenderFrameImpl.

The remaining uses of context_menu_node_ are easy to convert, but I've left them to a future cl so as to not make this cl any bigger. The main part of this cl is to move the Copy edit command from RenderViewHost to RenderFrameHost. To do that, I also had to convert Cut and Paste at the same time because of BrowserView.

BUG=304341
R=nasko@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=255735

[jam, 2014-03-05 21:15:47]

This cl depends on this Blink change: https://codereview.chromium.org/184103030/

[nasko, 2014-03-05 21:50:47]

I have just a few minor comments on the code, but I think overall we could do
this differently.

Since longer term we will need to know the focused frame, exposing
WebContents::GetFocusedFrame() makes sense. If we agree on that, we can route
the IPC (FrameHostMsg_GotFocus or similar) through RFH, so we don't have to move
it later. RFH already dispatches OnMessageReceived to WebContentsImpl, so it is
trivial to store the focused_frame_routing_id_ there and implement
GetFocusedFrame().

I just want to avoid adding code to RVH that will have to be moved to RFH later
on.

https://codereview.chromium.org/183923030/diff/100001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/183923030/diff/100001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:136: Send(new
InputMsg_Cut(GetRoutingID()));
nit: routing_id_ here and in ::Paste, to be consistent with other usages and
::Copy.

https://codereview.chromium.org/183923030/diff/100001/content/browser/rendere...
File content/browser/renderer_host/ime_adapter_android.cc (right):

https://codereview.chromium.org/183923030/diff/100001/content/browser/rendere...
content/browser/renderer_host/ime_adapter_android.cc:275: RenderFrameHost*
ImeAdapterAndroid::GetRenderFrameHost() {
nit: Shouldn't this be GetFocusedRenderFrameHost? It specifically returns the
focused one, so the name doesn't convey that.

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

https://codereview.chromium.org/183923030/diff/100001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_mac.mm:30: #include
"content/browser/frame_host/render_frame_host_impl.h"
nit: Why include the impl when using only the public interface?

https://codereview.chromium.org/183923030/diff/100001/content/browser/web_con...
File content/browser/web_contents/touch_editable_impl_aura.cc (right):

https://codereview.chromium.org/183923030/diff/100001/content/browser/web_con...
content/browser/web_contents/touch_editable_impl_aura.cc:226: int
render_process_id, int render_frame_id) {
style: params should be on separate lines.

https://codereview.chromium.org/183923030/diff/100001/content/renderer/render...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/183923030/diff/100001/content/renderer/render...
content/renderer/render_frame_impl.h:353: // be Null.
nit: null or NULL

[jam, 2014-03-06 00:59:36]

Thanks, agreed that this makes the API consistent going forward. ptal

https://codereview.chromium.org/183923030/diff/100001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/183923030/diff/100001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:136: Send(new
InputMsg_Cut(GetRoutingID()));
On 2014/03/05 21:50:47, nasko wrote:
> nit: routing_id_ here and in ::Paste, to be consistent with other usages and
> ::Copy.

Done.

https://codereview.chromium.org/183923030/diff/100001/content/browser/rendere...
File content/browser/renderer_host/ime_adapter_android.cc (right):

https://codereview.chromium.org/183923030/diff/100001/content/browser/rendere...
content/browser/renderer_host/ime_adapter_android.cc:275: RenderFrameHost*
ImeAdapterAndroid::GetRenderFrameHost() {
On 2014/03/05 21:50:47, nasko wrote:
> nit: Shouldn't this be GetFocusedRenderFrameHost? It specifically returns the
> focused one, so the name doesn't convey that.

Done.

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

https://codereview.chromium.org/183923030/diff/100001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_mac.mm:30: #include
"content/browser/frame_host/render_frame_host_impl.h"
On 2014/03/05 21:50:47, nasko wrote:
> nit: Why include the impl when using only the public interface?

that's the recommended style for includes inside content, which was borrowed
from WebKit API.

https://codereview.chromium.org/183923030/diff/100001/content/browser/web_con...
File content/browser/web_contents/touch_editable_impl_aura.cc (right):

https://codereview.chromium.org/183923030/diff/100001/content/browser/web_con...
content/browser/web_contents/touch_editable_impl_aura.cc:226: int
render_process_id, int render_frame_id) {
On 2014/03/05 21:50:47, nasko wrote:
> style: params should be on separate lines.

Done.

https://codereview.chromium.org/183923030/diff/100001/content/renderer/render...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/183923030/diff/100001/content/renderer/render...
content/renderer/render_frame_impl.h:353: // be Null.
On 2014/03/05 21:50:47, nasko wrote:
> nit: null or NULL

Done.

[nasko, 2014-03-06 04:52:37]

I like this approach a lot better, thanks for changing it!
It looks there are some GTK compile failures. Once those are resolved, it should
be good to go.

[jam, 2014-03-07 17:22:59]

+aurimas for ime_adapter_android.*: this is mostly fixing the thread-safety
issues in that file which I found when converting a few methods to call
cut/copy/paste on the frame instead of render widget

[jam, 2014-03-07 18:02:54]

mohsen: fyi, with help from davemoore, I've pushed a build to a pixel and
verified that the touch menu works.

[aurimas (slooooooooow), 2014-03-07 18:04:38]

https://codereview.chromium.org/183923030/diff/480001/content/browser/rendere...
File content/browser/renderer_host/ime_adapter_android.cc (right):

https://codereview.chromium.org/183923030/diff/480001/content/browser/rendere...
content/browser/renderer_host/ime_adapter_android.cc:109:
base::Bind(&ImeAdapterAndroid::SendSyntheticKeyEventOnUI, this, event));
I was under impression that most of these calls in Java were made on UI thread.
Which ones did you find where not? I would rather fix this in Java and add
DCHECKs where for the correct thread.

[jam, 2014-03-07 18:15:44]

https://codereview.chromium.org/183923030/diff/480001/content/browser/rendere...
File content/browser/renderer_host/ime_adapter_android.cc (right):

https://codereview.chromium.org/183923030/diff/480001/content/browser/rendere...
content/browser/renderer_host/ime_adapter_android.cc:109:
base::Bind(&ImeAdapterAndroid::SendSyntheticKeyEventOnUI, this, event));
On 2014/03/07 18:04:40, aurimas wrote:
> I was under impression that most of these calls in Java were made on UI
thread.
> Which ones did you find where not? I would rather fix this in Java and add
> DCHECKs where for the correct thread.

sure that works too. i didn't know how they're called as I'm not familiar with
the java side.

http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...

shows the failures that I saw:

org.chromium.content.browser.ClipboardTest#testCopyDocumentFragment
org.chromium.content.browser.input.ImeTest#testImeCopy
org.chromium.content.browser.input.ImeTest#testImeCut
org.chromium.content.browser.input.ImeTest#testImePaste
org.chromium.content.browser.input.InsertionHandleTest#testPasteAtInsertionHandle

crashed in patchset 17 before I made these changes.

I also searched crbug.com for ime_adapter_android and found crbug.com/310458
which also looked like ImeAdapterAndroid::SendKeyEvent was being called on the
wrong thread. since it appeared that all of them are called the same way, I
converted them all to use posttask.

[aurimas (slooooooooow), 2014-03-07 18:20:06]

On 2014/03/07 18:15:44, jam wrote:
>
https://codereview.chromium.org/183923030/diff/480001/content/browser/rendere...
> File content/browser/renderer_host/ime_adapter_android.cc (right):
> 
>
https://codereview.chromium.org/183923030/diff/480001/content/browser/rendere...
> content/browser/renderer_host/ime_adapter_android.cc:109:
> base::Bind(&ImeAdapterAndroid::SendSyntheticKeyEventOnUI, this, event));
> On 2014/03/07 18:04:40, aurimas wrote:
> > I was under impression that most of these calls in Java were made on UI
> thread.
> > Which ones did you find where not? I would rather fix this in Java and add
> > DCHECKs where for the correct thread.
> 
> sure that works too. i didn't know how they're called as I'm not familiar with
> the java side.
> 
>
http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...
> 
> shows the failures that I saw:
> 
> org.chromium.content.browser.ClipboardTest#testCopyDocumentFragment
> org.chromium.content.browser.input.ImeTest#testImeCopy
> org.chromium.content.browser.input.ImeTest#testImeCut
> org.chromium.content.browser.input.ImeTest#testImePaste
>
org.chromium.content.browser.input.InsertionHandleTest#testPasteAtInsertionHandle
> 
> crashed in patchset 17 before I made these changes.
> 
> I also searched http://crbug.com for ime_adapter_android and found
crbug.com/310458
> which also looked like ImeAdapterAndroid::SendKeyEvent was being called on the
> wrong thread. since it appeared that all of them are called the same way, I
> converted them all to use posttask.

Good catch! These tests are calling to ImeAdapterAndroid directly from the
testrunner thread instead of UI as it is supposed to. I will send you CL with a
fix for that today.

[nasko, 2014-03-07 18:22:16]

LGTM

[jam, 2014-03-07 18:24:22]

On 2014/03/07 18:20:06, aurimas wrote:
> On 2014/03/07 18:15:44, jam wrote:
> >
>
https://codereview.chromium.org/183923030/diff/480001/content/browser/rendere...
> > File content/browser/renderer_host/ime_adapter_android.cc (right):
> > 
> >
>
https://codereview.chromium.org/183923030/diff/480001/content/browser/rendere...
> > content/browser/renderer_host/ime_adapter_android.cc:109:
> > base::Bind(&ImeAdapterAndroid::SendSyntheticKeyEventOnUI, this, event));
> > On 2014/03/07 18:04:40, aurimas wrote:
> > > I was under impression that most of these calls in Java were made on UI
> > thread.
> > > Which ones did you find where not? I would rather fix this in Java and add
> > > DCHECKs where for the correct thread.
> > 
> > sure that works too. i didn't know how they're called as I'm not familiar
with
> > the java side.
> > 
> >
>
http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...
> > 
> > shows the failures that I saw:
> > 
> > org.chromium.content.browser.ClipboardTest#testCopyDocumentFragment
> > org.chromium.content.browser.input.ImeTest#testImeCopy
> > org.chromium.content.browser.input.ImeTest#testImeCut
> > org.chromium.content.browser.input.ImeTest#testImePaste
> >
>
org.chromium.content.browser.input.InsertionHandleTest#testPasteAtInsertionHandle
> > 
> > crashed in patchset 17 before I made these changes.
> > 
> > I also searched http://crbug.com for ime_adapter_android and found
> crbug.com/310458
> > which also looked like ImeAdapterAndroid::SendKeyEvent was being called on
the
> > wrong thread. since it appeared that all of them are called the same way, I
> > converted them all to use posttask.
> 
> Good catch! These tests are calling to ImeAdapterAndroid directly from the
> testrunner thread instead of UI as it is supposed to. I will send you CL with
a
> fix for that today.

great, thanks!
two questions:
1) if i'm ready to enable this before that fix lands, can I disable these tests
in the meantime? if so can you tell me how to disable them? i've had a tough
time before trying to disable android tests. i'll revert my changes now
2) what about the ImeAdapterAndroid::SendKeyEvent crash from the linked bug, is
that something else?

[aurimas (slooooooooow), 2014-03-07 22:03:30]

> two questions:
> 1) if i'm ready to enable this before that fix lands, can I disable these
tests
> in the meantime? if so can you tell me how to disable them? i've had a tough
> time before trying to disable android tests. i'll revert my changes now
> 2) what about the ImeAdapterAndroid::SendKeyEvent crash from the linked bug,
is
> that something else?

1) My CL is making its way through to fix the tests:
https://chromiumcodereview.appspot.com/191133003/

2) I am not sure ImeAdapterAndroid::SendKeyEvent. +feng who might know.