Fixed the inconsistent behavior when long pressing on editable area in Chrome.

Fixed the inconsistent behavior when long pressing on editable area in Chrome.

When we long press on an input field with empty text, we should show paste
popup, we shouldn't show the IME.
This change takes care of preventing IME appearance when we do long press on
empty edit field to make it uniform among all Android apps.

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

[AKVT, 2014-08-18 14:56:32]

@jamesr and aurimas PTAL.

[aurimas (slooooooooow), 2014-08-18 16:05:24]

https://codereview.chromium.org/484653003/diff/1/content/renderer/render_widg...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/484653003/diff/1/content/renderer/render_widg...
content/renderer/render_widget.cc:1977: UpdateTextInputState(SHOW_IME_IF_NEEDED,
FROM_NON_IME);
Conditionally calling UpdateTextInputState will cause all sorts of issues as
browser side IME state will not be initialized (AdapterInputConnection.java).
There are many callsites of UpdateTextInputState and the next one to call it
will push the update to Java defeating your conditional call.

[AKVT, 2014-08-18 16:14:30]

https://codereview.chromium.org/484653003/diff/1/content/renderer/render_widg...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/484653003/diff/1/content/renderer/render_widg...
content/renderer/render_widget.cc:1977: UpdateTextInputState(SHOW_IME_IF_NEEDED,
FROM_NON_IME);
On 2014/08/18 16:05:24, aurimas wrote:
> Conditionally calling UpdateTextInputState will cause all sorts of issues as
> browser side IME state will not be initialized (AdapterInputConnection.java).
> There are many callsites of UpdateTextInputState and the next one to call it
> will push the update to Java defeating your conditional call.

Thanks. In that case, how do we overcome this problem ? We need to handle from
Webkit side itself ? or let the call goes to AdapterInputConnection and prevent
IME showing from there ?
Please suggest your opinion

[AKVT, 2014-08-19 14:41:45]

@aurimas, Please share your opinion about my inline queries. If it needs to be
fixed from blink side, please add relevant folks, so that I can work with them
for resolving this issue.

[AKVT, 2014-08-19 17:01:03]

@aurimas PTAL PS2

[aurimas (slooooooooow), 2014-08-19 17:02:36]

On 2014/08/19 17:01:03, AKV wrote:
> @aurimas PTAL PS2

Can you explain exactly what you are trying to achieve in terms of behavior?

[aurimas (slooooooooow), 2014-08-19 17:03:01]

On 2014/08/19 17:02:36, aurimas wrote:
> On 2014/08/19 17:01:03, AKV wrote:
> > @aurimas PTAL PS2
> 
> Can you explain exactly what you are trying to achieve in terms of behavior?

Nevermind, just read the description.

[aurimas (slooooooooow), 2014-08-19 17:08:56]

I think this should work now that we are sending updateTextInputState in both
cases. Could you add some tests for this new behavior?

+kevers as FYI as he worked on Aura virtual keyboard.

[AKVT, 2014-08-19 17:39:37]

On 2014/08/19 17:08:56, aurimas wrote:
> I think this should work now that we are sending updateTextInputState in both
> cases. Could you add some tests for this new behavior?
> 
> +kevers as FYI as he worked on Aura virtual keyboard.

Thanks aurimas. Do I need to write unittests in render_widget_unittests.cc file
or inside ImeTest.java file. When I checked the history of
render_widget_unittests.cc, there is no existing related tests available. Please
suggest

[jdduke (slow), 2014-08-19 18:19:49]

https://codereview.chromium.org/484653003/diff/20001/content/renderer/render_...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/484653003/diff/20001/content/renderer/render_...
content/renderer/render_widget.cc:1975: new_info = webwidget_->textInputInfo();
Is there no other way of determining whether the input field is empty? This
query does some non-trivial work (string copying, etc...), which I suppose isn't
a huge deal as GestureLongPress is relatively infrequent, I just wonder if
there's a cheaper way of obtaining the same information.

[AKVT, 2014-08-20 15:00:43]

https://codereview.chromium.org/484653003/diff/20001/content/renderer/render_...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/484653003/diff/20001/content/renderer/render_...
content/renderer/render_widget.cc:1975: new_info = webwidget_->textInputInfo();
On 2014/08/19 18:19:49, jdduke wrote:
> Is there no other way of determining whether the input field is empty? This
> query does some non-trivial work (string copying, etc...), which I suppose
isn't
> a huge deal as GestureLongPress is relatively infrequent, I just wonder if
> there's a cheaper way of obtaining the same information.

As I checked the code, there is no other way we can do this from existing flow,
we may have to add extra method to extract only required info. Please suggest
your opinion on this whether I can keep same or I have to write new function
from WebViewImpl.

[jdduke (slow), 2014-08-20 15:37:05]

https://codereview.chromium.org/484653003/diff/20001/content/renderer/render_...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/484653003/diff/20001/content/renderer/render_...
content/renderer/render_widget.cc:1975: new_info = webwidget_->textInputInfo();
On 2014/08/20 15:00:43, AKV wrote:
> On 2014/08/19 18:19:49, jdduke wrote:
> > Is there no other way of determining whether the input field is empty? This
> > query does some non-trivial work (string copying, etc...), which I suppose
> isn't
> > a huge deal as GestureLongPress is relatively infrequent, I just wonder if
> > there's a cheaper way of obtaining the same information.
> 
> As I checked the code, there is no other way we can do this from existing
flow,
> we may have to add extra method to extract only required info. Please suggest
> your opinion on this whether I can keep same or I have to write new function
> from WebViewImpl.

I guess this is fine, but |didHandleGestureEvent| is a WebWidgetClient so we
should definitely still have a |webwidget_|, so you should instead do something
like:

DCHECK(webwidget_);
if (webwidget_->textInputInfo().value.isEmpty())
  UpdateTextInputState(NO_SHOW_IME, FROM_NON_IME);
else
  UpdateTextInputState(SHOW_IME_IF_NEEDED, FROM_NON_IME);

[jdduke (slow), 2014-08-20 15:37:49]

https://codereview.chromium.org/484653003/diff/20001/content/renderer/render_...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/484653003/diff/20001/content/renderer/render_...
content/renderer/render_widget.cc:1975: new_info = webwidget_->textInputInfo();
On 2014/08/20 15:37:05, jdduke wrote:
> I guess this is fine, but |didHandleGestureEvent| is a WebWidgetClient so we

is a WebWidgetClient *method* that is.

[AKVT, 2014-08-20 18:20:17]

@jdduke @ aurimas PTAL, I have added Unit test case for the patch.

https://codereview.chromium.org/484653003/diff/20001/content/renderer/render_...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/484653003/diff/20001/content/renderer/render_...
content/renderer/render_widget.cc:1975: new_info = webwidget_->textInputInfo();
On 2014/08/20 15:37:05, jdduke wrote:
> On 2014/08/20 15:00:43, AKV wrote:
> > On 2014/08/19 18:19:49, jdduke wrote:
> > > Is there no other way of determining whether the input field is empty?
This
> > > query does some non-trivial work (string copying, etc...), which I suppose
> > isn't
> > > a huge deal as GestureLongPress is relatively infrequent, I just wonder if
> > > there's a cheaper way of obtaining the same information.
> > 
> > As I checked the code, there is no other way we can do this from existing
> flow,
> > we may have to add extra method to extract only required info. Please
suggest
> > your opinion on this whether I can keep same or I have to write new function
> > from WebViewImpl.
> 
> I guess this is fine, but |didHandleGestureEvent| is a WebWidgetClient so we
> should definitely still have a |webwidget_|, so you should instead do
something
> like:
> 
> DCHECK(webwidget_);
> if (webwidget_->textInputInfo().value.isEmpty())
>   UpdateTextInputState(NO_SHOW_IME, FROM_NON_IME);
> else
>   UpdateTextInputState(SHOW_IME_IF_NEEDED, FROM_NON_IME);

Done. Thanks

https://codereview.chromium.org/484653003/diff/20001/content/renderer/render_...
content/renderer/render_widget.cc:1975: new_info = webwidget_->textInputInfo();
On 2014/08/20 15:37:49, jdduke wrote:
> On 2014/08/20 15:37:05, jdduke wrote:
> > I guess this is fine, but |didHandleGestureEvent| is a WebWidgetClient so we
> 
> is a WebWidgetClient *method* that is.

Done.

[aurimas (slooooooooow), 2014-08-20 18:22:15]

lgtm

[AKVT, 2014-08-20 18:22:50]

The CQ bit was checked by ajith.v@samsung.com

[commit-bot: I haz the power, 2014-08-20 18:24:03]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ajith.v@samsung.com/484653003/60001

[aurimas (slooooooooow), 2014-08-20 18:24:42]

The CQ bit was unchecked by aurimas@chromium.org

[aurimas (slooooooooow), 2014-08-20 18:24:59]

You need OWNERS LGTM before you check the CQ flag.

[AKVT, 2014-08-20 18:26:23]

On 2014/08/20 18:24:59, aurimas wrote:
> You need OWNERS LGTM before you check the CQ flag.

Thanks aurimas. @jamesr PTAL

[jamesr, 2014-08-20 19:00:56]

content/renderer lgtm

[AKVT, 2014-08-21 02:01:43]

The CQ bit was checked by ajith.v@samsung.com

[commit-bot: I haz the power, 2014-08-21 02:02:26]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ajith.v@samsung.com/484653003/60001

[commit-bot: I haz the power, 2014-08-21 02:06:04]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[commit-bot: I haz the power, 2014-08-21 02:09:49]

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

[commit-bot: I haz the power, 2014-08-21 02:09:50]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[AKVT, 2014-08-21 02:10:23]

@jdduke PTAL. presubmit is failing due to OWNERS LGTM

[jdduke (slow), 2014-08-21 02:23:05]

On 2014/08/21 02:10:23, AKV wrote:
> @jdduke PTAL. presubmit is failing due to OWNERS LGTM

ImeTest lgtm.

[AKVT, 2014-08-21 05:46:17]

The CQ bit was checked by ajith.v@samsung.com

[commit-bot: I haz the power, 2014-08-21 05:46:57]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ajith.v@samsung.com/484653003/60001

[commit-bot: I haz the power, 2014-08-21 08:33:53]

Committed patchset #4 (60001) as 291021