Start using FocusedNodedChanged to restartInput.

Start using FocusedNodedChanged to restartInput.

Android requires us to call restart input when input node changes.
Previously we relied on onHandleGesture which is not the right way
to do it.

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

[aurimas (slooooooooow), 2013-11-01 23:39:35]

Hello James,

Could you please take a look at this change? I tried to follow what we talked
about offline.

Thanks,
Aurimas

[aurimas (slooooooooow), 2013-11-05 20:52:53]

Hello James,

Could you please take a look at this change? I tried to follow what we talked
about offline.

Thanks,
Aurimas

[jamesr, 2013-11-05 23:54:08]

I got really lost in these classes, but jam@ helped me out.  I think what you
want to do is define a virtual empty function on RenderWidgetHostViewBase, call
that unconditionally from the current IPC handler in RenderViewHost on all
platforms, and then subclass that with your implementation in
RenderWidgetHostViewAndroid.  I imagine other platforms are going to want to
switch to focus tracking this way too since IME is probably broken for them
sometimes, so having the hook set up for them on the base class seems nice.

Sorry this is all so messy.

https://codereview.chromium.org/56643003/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/56643003/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_view_host_impl.cc:1205: #if
!defined(OS_ANDROID)
Hmm, is this so the IPC is unhandled and thus gets routed to the view? I think
this works, but it's a bit fragile since it means the Notification doesn't work
the same on android as on other platforms. the notification doesn't appear to be
used for anything useful today, but it's a bit of a footgun to leave behind.

maybe you could forward this call to the view in the IPC handler? i always get a
bit lost in these classes

[aurimas (slooooooooow), 2013-11-15 18:09:38]

Hello James,

I have changed this CL to adjust to your comments. Please take another look at
this.

Thanks,
Aurimas

[jamesr, 2013-11-15 23:02:46]

https://chromiumcodereview.appspot.com/56643003/diff/70001/content/browser/re...
File content/browser/renderer_host/ime_adapter_android.cc (right):

https://chromiumcodereview.appspot.com/56643003/diff/70001/content/browser/re...
content/browser/renderer_host/ime_adapter_android.cc:184: if (!obj.is_null())
indentation here went off somehow. 183 should be indented 4 spaces from the
previous line, the if on 184 should be lined up with the start of line 182

https://chromiumcodereview.appspot.com/56643003/diff/70001/content/browser/re...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://chromiumcodereview.appspot.com/56643003/diff/70001/content/browser/re...
content/browser/renderer_host/render_widget_host_impl.cc:1337: void
RenderWidgetHostImpl::FocusedNodeChanged(bool is_editable_node) {
hmm, why do you need the trip to the base class? is view_ not accessible to
subclasses?

https://chromiumcodereview.appspot.com/56643003/diff/70001/content/browser/re...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://chromiumcodereview.appspot.com/56643003/diff/70001/content/browser/re...
content/browser/renderer_host/render_widget_host_view_android.cc:443: 
extra newlines

[aurimas (slooooooooow), 2013-11-20 00:25:43]

https://chromiumcodereview.appspot.com/56643003/diff/70001/content/browser/re...
File content/browser/renderer_host/ime_adapter_android.cc (right):

https://chromiumcodereview.appspot.com/56643003/diff/70001/content/browser/re...
content/browser/renderer_host/ime_adapter_android.cc:184: if (!obj.is_null())
On 2013/11/15 23:02:46, jamesr wrote:
> indentation here went off somehow. 183 should be indented 4 spaces from the
> previous line, the if on 184 should be lined up with the start of line 182

Done.

https://chromiumcodereview.appspot.com/56643003/diff/70001/content/browser/re...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://chromiumcodereview.appspot.com/56643003/diff/70001/content/browser/re...
content/browser/renderer_host/render_widget_host_impl.cc:1337: void
RenderWidgetHostImpl::FocusedNodeChanged(bool is_editable_node) {
On 2013/11/15 23:02:46, jamesr wrote:
> hmm, why do you need the trip to the base class? is view_ not accessible to
> subclasses?

Done.

https://chromiumcodereview.appspot.com/56643003/diff/70001/content/browser/re...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://chromiumcodereview.appspot.com/56643003/diff/70001/content/browser/re...
content/browser/renderer_host/render_widget_host_view_android.cc:443: 
On 2013/11/15 23:02:46, jamesr wrote:
> extra newlines

Done.

[jamesr, 2013-11-20 00:36:56]

lgtm - you'll need a content/ OWNER to look as well as I just own
content/renderer/

https://chromiumcodereview.appspot.com/56643003/diff/210001/content/browser/r...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://chromiumcodereview.appspot.com/56643003/diff/210001/content/browser/r...
content/browser/renderer_host/render_view_host_impl.cc:1772: if
(RenderWidgetHostImpl::view_)
why do you need the explicit RWHI:: qualification? why not just view_ ?

https://chromiumcodereview.appspot.com/56643003/diff/210001/content/browser/r...
content/browser/renderer_host/render_view_host_impl.cc:1773:
RenderWidgetHostImpl::view_->FocusedNodeChanged(is_editable_node);
ditto

[aurimas (slooooooooow), 2013-11-20 00:46:58]

https://chromiumcodereview.appspot.com/56643003/diff/210001/content/browser/r...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://chromiumcodereview.appspot.com/56643003/diff/210001/content/browser/r...
content/browser/renderer_host/render_view_host_impl.cc:1772: if
(RenderWidgetHostImpl::view_)
On 2013/11/20 00:36:56, jamesr wrote:
> why do you need the explicit RWHI:: qualification? why not just view_ ?

Done.

https://chromiumcodereview.appspot.com/56643003/diff/210001/content/browser/r...
content/browser/renderer_host/render_view_host_impl.cc:1773:
RenderWidgetHostImpl::view_->FocusedNodeChanged(is_editable_node);
On 2013/11/20 00:36:56, jamesr wrote:
> ditto

Done.

[aurimas (slooooooooow), 2013-11-20 00:48:25]

tedchoc@chromium.org: Please review changes in content/public/android/java/*

thakis@chromium.org: Please review changes in content/browser/*

[Ted C, 2013-11-20 00:52:47]

androidy bits lgtm w/ nits

https://chromiumcodereview.appspot.com/56643003/diff/270001/content/browser/r...
File content/browser/renderer_host/ime_adapter_android.cc (right):

https://chromiumcodereview.appspot.com/56643003/diff/270001/content/browser/r...
content/browser/renderer_host/ime_adapter_android.cc:184: if (!obj.is_null())
I was under the impression you needed braces if the statement was longer than
one line as well.

https://chromiumcodereview.appspot.com/56643003/diff/270001/content/public/an...
File
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java
(right):

https://chromiumcodereview.appspot.com/56643003/diff/270001/content/public/an...
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:495:
if (mInputConnection != null) {
Can this all fit on one line?

if (mInputConnection != null && isEditable) mInputConnection.restartInput();

[Nico, 2013-11-20 01:03:09]

Looks good, thanks. Any thoughts on writing a test for this? Like, maybe
programmatically focus various dom elements via js and then verify that the
FocusedNodeChange function is called as expected?

https://chromiumcodereview.appspot.com/56643003/diff/270001/content/browser/r...
File content/browser/renderer_host/ime_adapter_android.cc (right):

https://chromiumcodereview.appspot.com/56643003/diff/270001/content/browser/r...
content/browser/renderer_host/ime_adapter_android.cc:184: if (!obj.is_null())
style nit: If either if condition or statement span multiple lines, you have to
use braces

https://chromiumcodereview.appspot.com/56643003/diff/270001/content/browser/r...
File content/browser/renderer_host/ime_adapter_android.h (right):

https://chromiumcodereview.appspot.com/56643003/diff/270001/content/browser/r...
content/browser/renderer_host/ime_adapter_android.h:59: void
FocusedNodeChanged(bool is_editable_node);
Generally, prefer enums over bool parameters. This makes the calling sites more
readable. Compare:

  FocusedNodeChanged(true);

and

  FocusedNodeChanged(EDITABLE_NODE);

(not sure if this applies here since I haven't read the calling site yet)

https://chromiumcodereview.appspot.com/56643003/diff/270001/content/port/brow...
File content/port/browser/render_widget_host_view_port.h (right):

https://chromiumcodereview.appspot.com/56643003/diff/270001/content/port/brow...
content/port/browser/render_widget_host_view_port.h:108: virtual void
FocusedNodeChanged(bool is_editable_node) = 0;
needs comment

[aurimas (slooooooooow), 2013-11-20 01:12:39]

For tests, this IPC already is tested in browser_focus_uitest.cc

IN_PROC_BROWSER_TEST_F(BrowserFocusTest, MAYBE_FocusTraversal)
IN_PROC_BROWSER_TEST_F(BrowserFocusTest, MAYBE_FocusTraversalOnInterstitial)

https://chromiumcodereview.appspot.com/56643003/diff/270001/content/browser/r...
File content/browser/renderer_host/ime_adapter_android.cc (right):

https://chromiumcodereview.appspot.com/56643003/diff/270001/content/browser/r...
content/browser/renderer_host/ime_adapter_android.cc:184: if (!obj.is_null())
On 2013/11/20 00:52:47, Ted C wrote:
> I was under the impression you needed braces if the statement was longer than
> one line as well.

Done.

https://chromiumcodereview.appspot.com/56643003/diff/270001/content/browser/r...
File content/browser/renderer_host/ime_adapter_android.h (right):

https://chromiumcodereview.appspot.com/56643003/diff/270001/content/browser/r...
content/browser/renderer_host/ime_adapter_android.h:59: void
FocusedNodeChanged(bool is_editable_node);
On 2013/11/20 01:03:10, Nico (in Tokyo until Nov 25) wrote:
> Generally, prefer enums over bool parameters. This makes the calling sites
more
> readable. Compare:
> 
>   FocusedNodeChanged(true);
> 
> and
> 
>   FocusedNodeChanged(EDITABLE_NODE);
> 
> (not sure if this applies here since I haven't read the calling site yet)

It is a bool all the way from render_widget where it sends out an IPC. Do you
still want me to change it?

https://chromiumcodereview.appspot.com/56643003/diff/270001/content/port/brow...
File content/port/browser/render_widget_host_view_port.h (right):

https://chromiumcodereview.appspot.com/56643003/diff/270001/content/port/brow...
content/port/browser/render_widget_host_view_port.h:108: virtual void
FocusedNodeChanged(bool is_editable_node) = 0;
On 2013/11/20 01:03:10, Nico (in Tokyo until Nov 25) wrote:
> needs comment

Done.

https://chromiumcodereview.appspot.com/56643003/diff/270001/content/public/an...
File
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java
(right):

https://chromiumcodereview.appspot.com/56643003/diff/270001/content/public/an...
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:495:
if (mInputConnection != null) {
On 2013/11/20 00:52:47, Ted C wrote:
> Can this all fit on one line?
> 
> if (mInputConnection != null && isEditable) mInputConnection.restartInput();

Done.

[Nico, 2013-11-20 01:23:55]

On Wed, Nov 20, 2013 at 10:12 AM, <aurimas@chromium.org> wrote:

> For tests, this IPC already is tested in browser_focus_uitest.cc
>
> IN_PROC_BROWSER_TEST_F(BrowserFocusTest, MAYBE_FocusTraversal)
> IN_PROC_BROWSER_TEST_F(BrowserFocusTest, MAYBE_
> FocusTraversalOnInterstitial)


This tests the notification. The notification isn't used in production code
anywhere, so it'll be deleted soon. Also, that test works by sending key
events, which is brittle (proof: Test disabled everywhere).


>
>
>
> https://chromiumcodereview.appspot.com/56643003/diff/
> 270001/content/browser/renderer_host/ime_adapter_android.cc
> File content/browser/renderer_host/ime_adapter_android.cc (right):
>
> https://chromiumcodereview.appspot.com/56643003/diff/
> 270001/content/browser/renderer_host/ime_adapter_android.cc#newcode184
> content/browser/renderer_host/ime_adapter_android.cc:184: if
> (!obj.is_null())
> On 2013/11/20 00:52:47, Ted C wrote:
>
>> I was under the impression you needed braces if the statement was
>>
> longer than
>
>> one line as well.
>>
>
> Done.
>
>
> https://chromiumcodereview.appspot.com/56643003/diff/
> 270001/content/browser/renderer_host/ime_adapter_android.h
> File content/browser/renderer_host/ime_adapter_android.h (right):
>
> https://chromiumcodereview.appspot.com/56643003/diff/
> 270001/content/browser/renderer_host/ime_adapter_android.h#newcode59
> content/browser/renderer_host/ime_adapter_android.h:59: void
> FocusedNodeChanged(bool is_editable_node);
> On 2013/11/20 01:03:10, Nico (in Tokyo until Nov 25) wrote:
>
>> Generally, prefer enums over bool parameters. This makes the calling
>>
> sites more
>
>> readable. Compare:
>>
>
>     FocusedNodeChanged(true);
>>
>
>  and
>>
>
>     FocusedNodeChanged(EDITABLE_NODE);
>>
>
>  (not sure if this applies here since I haven't read the calling site
>>
> yet)
>
> It is a bool all the way from render_widget where it sends out an IPC.
> Do you still want me to change it?
>
>
> https://chromiumcodereview.appspot.com/56643003/diff/
> 270001/content/port/browser/render_widget_host_view_port.h
> File content/port/browser/render_widget_host_view_port.h (right):
>
> https://chromiumcodereview.appspot.com/56643003/diff/
> 270001/content/port/browser/render_widget_host_view_port.h#newcode108
> content/port/browser/render_widget_host_view_port.h:108: virtual void
> FocusedNodeChanged(bool is_editable_node) = 0;
> On 2013/11/20 01:03:10, Nico (in Tokyo until Nov 25) wrote:
>
>> needs comment
>>
>
> Done.
>
>
> https://chromiumcodereview.appspot.com/56643003/diff/
> 270001/content/public/android/java/src/org/chromium/content/
> browser/input/ImeAdapter.java
> File
> content/public/android/java/src/org/chromium/content/
> browser/input/ImeAdapter.java
> (right):
>
> https://chromiumcodereview.appspot.com/56643003/diff/
> 270001/content/public/android/java/src/org/chromium/content/
> browser/input/ImeAdapter.java#newcode495
> content/public/android/java/src/org/chromium/content/
> browser/input/ImeAdapter.java:495:
> if (mInputConnection != null) {
> On 2013/11/20 00:52:47, Ted C wrote:
>
>> Can this all fit on one line?
>>
>
>  if (mInputConnection != null && isEditable)
>>
> mInputConnection.restartInput();
>
> Done.
>
> https://chromiumcodereview.appspot.com/56643003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Nico, 2013-11-20 01:24:44]

https://chromiumcodereview.appspot.com/56643003/diff/270001/content/browser/r...
File content/browser/renderer_host/ime_adapter_android.h (right):

https://chromiumcodereview.appspot.com/56643003/diff/270001/content/browser/r...
content/browser/renderer_host/ime_adapter_android.h:59: void
FocusedNodeChanged(bool is_editable_node);
On 2013/11/20 01:12:39, aurimas wrote:
> On 2013/11/20 01:03:10, Nico (in Tokyo until Nov 25) wrote:
> > Generally, prefer enums over bool parameters. This makes the calling sites
> more
> > readable. Compare:
> > 
> >   FocusedNodeChanged(true);
> > 
> > and
> > 
> >   FocusedNodeChanged(EDITABLE_NODE);
> > 
> > (not sure if this applies here since I haven't read the calling site yet)
> 
> It is a bool all the way from render_widget where it sends out an IPC. Do you
> still want me to change it?

No, nobody seems to pass a literal "true" or "false". As I said, I hadn't read
the rest yet :-)

[aurimas (slooooooooow), 2014-01-10 01:22:11]

thakis: added a RenderViewImplTest. WDYT?

[aurimas (slooooooooow), 2014-01-10 23:14:36]

On 2014/01/10 01:22:11, aurimas wrote:
> thakis: added a RenderViewImplTest. WDYT?

thakis: ping?

[Nico, 2014-01-14 01:57:12]

On 2014/01/10 23:14:36, aurimas wrote:
> On 2014/01/10 01:22:11, aurimas wrote:
> > thakis: added a RenderViewImplTest. WDYT?
> 
> thakis: ping?

lgtm assuming the test fails without your code change.

Sorry about the delay!

[aurimas (slooooooooow), 2014-01-14 02:03:16]

joi@chromium.org: Please review changes in content/port/browser/*

[Jói, 2014-01-14 09:19:30]

//content/port LGTM.

[commit-bot: I haz the power, 2014-01-16 00:38:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aurimas@chromium.org/56643003/560001

[commit-bot: I haz the power, 2014-01-16 00:38:20]

Failed to apply patch for
content/browser/renderer_host/render_view_host_impl.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file content/browser/renderer_host/render_view_host_impl.cc
  Hunk #1 FAILED at 1784.
  1 out of 1 hunk FAILED -- saving rejects to file
content/browser/renderer_host/render_view_host_impl.cc.rej

Patch:       content/browser/renderer_host/render_view_host_impl.cc
Index: content/browser/renderer_host/render_view_host_impl.cc
diff --git a/content/browser/renderer_host/render_view_host_impl.cc
b/content/browser/renderer_host/render_view_host_impl.cc
index
94c4fc6ccdfdcf663ae1dd5c50e3d2acf9eb25a7..9bf887cb547d917a7d9c6874198b75d946c5d4ce
100644
--- a/content/browser/renderer_host/render_view_host_impl.cc
+++ b/content/browser/renderer_host/render_view_host_impl.cc
@@ -1784,6 +1784,8 @@ void RenderViewHostImpl::OnTakeFocus(bool reverse) {
 }
 
 void RenderViewHostImpl::OnFocusedNodeChanged(bool is_editable_node) {
+  if (view_)
+    view_->FocusedNodeChanged(is_editable_node);
 #if defined(OS_WIN) && defined(USE_AURA)
   if (!is_editable_node && virtual_keyboard_requested_) {
     virtual_keyboard_requested_ = false;

[commit-bot: I haz the power, 2014-01-16 01:25:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aurimas@chromium.org/56643003/790001

[commit-bot: I haz the power, 2014-01-16 05:12:13]

Change committed as 245130