Ensure ime adapter gets connected to RWHVA

Ensure ime adapter gets connected to RWHVA

A bug exists that makes ImeAdapter not establish connection to
RenderWidgetHostViewAndroid if RWHVA is created earlier than
ImeAdapter, since the WebContentsObserver events it relies on
to make connection would have been fired before it gets registered
in the observer list.

This CL checkes the presence of RWHVA upon creation of ImeAdapter
and ensure the connection is made regardless of the order in which
relevant objects are created.

BUG=713924
Review-Url: https://codereview.chromium.org/2832153002
Cr-Commit-Position: refs/heads/master@{#466455}
Committed: https://chromium.googlesource.com/chromium/src/+/7cf10b073c07ab7eb0ea156fcf1f4918109837cc

[Jinsuk Kim, 2017-04-21 07:27:19]

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

[commit-bot: I haz the power, 2017-04-21 07:27:29]

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-04-21 07:47:57]

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

[commit-bot: I haz the power, 2017-04-21 07:47:58]

Dry run: This issue passed the CQ dry run.

[Jinsuk Kim, 2017-04-21 08:33:32]

jinsukkim@chromium.org changed reviewers:
+ boliu@chromium.org

[boliu, 2017-04-21 16:20:02]

https://codereview.chromium.org/2832153002/diff/1/content/browser/android/ren...
File content/browser/android/render_widget_host_connector.cc (right):

https://codereview.chromium.org/2832153002/diff/1/content/browser/android/ren...
content/browser/android/render_widget_host_connector.cc:131:
render_widget_observer_->RenderViewReady();
what it does is correct, but the RenderViewReady call makes no sense. you have
to actually check it is ready before you fake the signal

https://codereview.chromium.org/2832153002/diff/1/content/browser/android/ren...
File content/browser/android/render_widget_host_connector.h (right):

https://codereview.chromium.org/2832153002/diff/1/content/browser/android/ren...
content/browser/android/render_widget_host_connector.h:26: void Initialize();
this is to avoid calling virtual function in constructor? meh, this is why
composition is better than inheritance...

[Jinsuk Kim, 2017-04-21 16:30:05]

https://codereview.chromium.org/2832153002/diff/1/content/browser/android/ren...
File content/browser/android/render_widget_host_connector.cc (right):

https://codereview.chromium.org/2832153002/diff/1/content/browser/android/ren...
content/browser/android/render_widget_host_connector.cc:131:
render_widget_observer_->RenderViewReady();
On 2017/04/21 16:20:02, boliu wrote:
> what it does is correct, but the RenderViewReady call makes no sense. you have
> to actually check it is ready before you fake the signal

It would be like this:
auto* rwhva = GetRenderWidgetViewAndroid();
if (rwhva)
  UpdateRenderWidgetHostView(rwhva);

But then |if| is kind of not necessary because if it is null, the call will be
just no-op. Choose to just use RenderViewReady. Do you see it done without using
it?

https://codereview.chromium.org/2832153002/diff/1/content/browser/android/ren...
File content/browser/android/render_widget_host_connector.h (right):

https://codereview.chromium.org/2832153002/diff/1/content/browser/android/ren...
content/browser/android/render_widget_host_connector.h:26: void Initialize();
On 2017/04/21 16:20:02, boliu wrote:
> this is to avoid calling virtual function in constructor? meh, this is why
> composition is better than inheritance...

Yes that was the reason.

[boliu, 2017-04-21 16:33:39]

https://codereview.chromium.org/2832153002/diff/1/content/browser/android/ren...
File content/browser/android/render_widget_host_connector.cc (right):

https://codereview.chromium.org/2832153002/diff/1/content/browser/android/ren...
content/browser/android/render_widget_host_connector.cc:131:
render_widget_observer_->RenderViewReady();
On 2017/04/21 16:30:04, Jinsuk Kim wrote:
> On 2017/04/21 16:20:02, boliu wrote:
> > what it does is correct, but the RenderViewReady call makes no sense. you
have
> > to actually check it is ready before you fake the signal
> 
> It would be like this:
> auto* rwhva = GetRenderWidgetViewAndroid();
> if (rwhva)
>   UpdateRenderWidgetHostView(rwhva);

Is that in RenderViewReady or in initialize? It should be correct to add a
DCHECK that GetRenderWidgetViewAndroid is non-null in RVReady, instead of the
runtime check.

If it's in Initialize, then yeah, just call UpdateRenderWidgetHostView without
the null check.

> 
> But then |if| is kind of not necessary because if it is null, the call will be
> just no-op. Choose to just use RenderViewReady. Do you see it done without
using
> it?

[Jinsuk Kim, 2017-04-21 16:53:17]

https://codereview.chromium.org/2832153002/diff/1/content/browser/android/ren...
File content/browser/android/render_widget_host_connector.cc (right):

https://codereview.chromium.org/2832153002/diff/1/content/browser/android/ren...
content/browser/android/render_widget_host_connector.cc:131:
render_widget_observer_->RenderViewReady();
On 2017/04/21 16:33:39, boliu wrote:
> On 2017/04/21 16:30:04, Jinsuk Kim wrote:
> > On 2017/04/21 16:20:02, boliu wrote:
> > > what it does is correct, but the RenderViewReady call makes no sense. you
> have
> > > to actually check it is ready before you fake the signal
> > 
> > It would be like this:
> > auto* rwhva = GetRenderWidgetViewAndroid();
> > if (rwhva)
> >   UpdateRenderWidgetHostView(rwhva);
> 
> Is that in RenderViewReady or in initialize? It should be correct to add a
> DCHECK that GetRenderWidgetViewAndroid is non-null in RVReady, instead of the
> runtime check.
> 
> If it's in Initialize, then yeah, just call UpdateRenderWidgetHostView without
> the null check.
> 
> > 
> > But then |if| is kind of not necessary because if it is null, the call will
be
> > just no-op. Choose to just use RenderViewReady. Do you see it done without
> using
> > it?
> 

Inititialize(). Done.

[Changwan Ryu, 2017-04-21 17:23:21]

I think we need tests to prevent serious regressions in this area. Would you
mind adding some?

[boliu, 2017-04-21 18:12:14]

lgtm

[Changwan Ryu, 2017-04-21 21:15:30]

The CQ bit was checked by changwan@chromium.org

[aelias_OOO_until_Jul13, 2017-04-21 21:16:11]

aelias@chromium.org changed reviewers:
+ aelias@chromium.org
- changwan@chromium.org

[aelias_OOO_until_Jul13, 2017-04-21 21:16:12]

OK, let's get this landed now so that we can make sure the feedback dies down on
canary channel after this goes in, and let's follow up on writing more tests
next week.

[commit-bot: I haz the power, 2017-04-21 21:16:58]

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-04-21 21:44:55]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1492809330856930,
"parent_rev": "b849e9c14781458ff65328c3fdbfb590da862adb", "commit_rev":
"7cf10b073c07ab7eb0ea156fcf1f4918109837cc"}

[commit-bot: I haz the power, 2017-04-21 21:45:05]

Description was changed from

==========
Ensure ime adapter gets connected to RWHVA

A bug exists that makes ImeAdapter not establish connection to
RenderWidgetHostViewAndroid if RWHVA is created earlier than
ImeAdapter, since the WebContentsObserver events it relies on
to make connection would have been fired before it gets registered
in the observer list.

This CL checkes the presence of RWHVA upon creation of ImeAdapter
and ensure the connection is made regardless of the order in which
relevant objects are created.

BUG=713924
==========

to

==========
Ensure ime adapter gets connected to RWHVA

A bug exists that makes ImeAdapter not establish connection to
RenderWidgetHostViewAndroid if RWHVA is created earlier than
ImeAdapter, since the WebContentsObserver events it relies on
to make connection would have been fired before it gets registered
in the observer list.

This CL checkes the presence of RWHVA upon creation of ImeAdapter
and ensure the connection is made regardless of the order in which
relevant objects are created.

BUG=713924

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

[commit-bot: I haz the power, 2017-04-21 21:45:06]

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