[Android] Should not update IME status when gesture needs to be disambiguated

[Android] Should not update IME status when gesture needs to be disambiguated

When a gesture needs to be disambiguated, WebKit doesn't update cursor focus.
We added an extra status for didHandleGestureEvent(), so we can distinguish
the case whether the event is actually delivered to the web page or cancelled.
In the cancelled case we should not update the IME status.

WebKit CL: https://webkit.org/b/104427

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

[trchen, 2013-01-12 01:14:13]

Hello James,
this is the chromium-side change for
https://bugs.webkit.org/show_bug.cgi?id=104427
Do you think this looks okay? Thanks!

[trchen, 2013-01-14 22:13:45]

Ping dear reviewer. Does this patch look reasonable to you? :)

[trchen, 2013-01-15 22:51:03]

Hello, can someone take a quick look at this?
This patch reflects the prototype change of
WebWidgetClient::didHandleGestureEvent(). Thank you!

[darin (slow to review), 2013-01-15 23:06:03]

https://codereview.chromium.org/11473027/diff/2001/content/renderer/render_wi...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/11473027/diff/2001/content/renderer/render_wi...
content/renderer/render_widget.cc:1965: if (event_status ==
EventStatusCancelled)
why did you plumb an EventStatus enum if you only needed to know if the event
was cancelled?

[trchen, 2013-01-15 23:28:52]

On 2013/01/15 23:06:03, darin wrote:
>
https://codereview.chromium.org/11473027/diff/2001/content/renderer/render_wi...
> File content/renderer/render_widget.cc (right):
> 
>
https://codereview.chromium.org/11473027/diff/2001/content/renderer/render_wi...
> content/renderer/render_widget.cc:1965: if (event_status ==
> EventStatusCancelled)
> why did you plumb an EventStatus enum if you only needed to know if the event
> was cancelled?

I've thought of a few alternatives:
1. skip didHandleGestureEvent when the event is cancelled -- currently
didHandleGestureEvent is only used to update IME status, for both Win8 and
Android, but I don't know what might be other potential use of it. I don't want
someone in the future needs to waste time to figure out why
didHandleGestureEvent isn't called sometimes. 

2. Add another bool flag to the parameters -- this should work too, but I think
enum is easier to understand than a bunch of flags.

3. Completely remove didHandleGestureEvent, instead, interpret the returned
value from WebViewImpl::handleInputEvent -- I think it is actually the best, but
will involve mass amount of work.

And, I think link disambiguation is a case special enough to have its own status
value. The event is not delivered to WebCore, but isn't really unhandled either.
It is more like a reroute request.

[trchen, 2013-01-17 01:44:14]

Hi Darin,

James seems to very busy this week. Could you help to approve this change?
Thanks!

[trchen, 2013-01-17 22:31:07]

Dear reviewers, have time to take a look? Thanks!

[darin (slow to review), 2013-01-19 00:08:15]

On 2013/01/15 23:28:52, trchen wrote:
> On 2013/01/15 23:06:03, darin wrote:
> >
>
https://codereview.chromium.org/11473027/diff/2001/content/renderer/render_wi...
> > File content/renderer/render_widget.cc (right):
> > 
> >
>
https://codereview.chromium.org/11473027/diff/2001/content/renderer/render_wi...
> > content/renderer/render_widget.cc:1965: if (event_status ==
> > EventStatusCancelled)
> > why did you plumb an EventStatus enum if you only needed to know if the
event
> > was cancelled?
> 
> I've thought of a few alternatives:
> 1. skip didHandleGestureEvent when the event is cancelled -- currently
> didHandleGestureEvent is only used to update IME status, for both Win8 and
> Android, but I don't know what might be other potential use of it. I don't
want
> someone in the future needs to waste time to figure out why
> didHandleGestureEvent isn't called sometimes. 

I agree that passing a parameter of some sort to distinguish the cancelled state
makes sense.


> 2. Add another bool flag to the parameters -- this should work too, but I
think
> enum is easier to understand than a bunch of flags.

Why would there be more than one bool?  It seems like you only care about
whether or not the event was cancelled.


> 3. Completely remove didHandleGestureEvent, instead, interpret the returned
> value from WebViewImpl::handleInputEvent -- I think it is actually the best,
but
> will involve mass amount of work.

I'm not sure what this change would look like.  In general, I favor doing things
the cleaner way even if it means more work in the short term.  It sounds like
this solution might be cleaner, but I don't have a good sense for the changes
that would be required.


> And, I think link disambiguation is a case special enough to have its own
status
> value. The event is not delivered to WebCore, but isn't really unhandled
either.
> It is more like a reroute request.

I'm still confused about why you passed an EventStatus enum, which has three
states, when you only really need to know if the event was cancelled.  The code
in this CL treats EventStatusUnprocessed and EventStatusProcessed equivalently,
so why bother distinguishing them in the WebKit API?

[trchen, 2013-01-19 00:30:38]

On 2013/01/19 00:08:15, darin wrote:
> On 2013/01/15 23:28:52, trchen wrote:
> > On 2013/01/15 23:06:03, darin wrote:
> > 3. Completely remove didHandleGestureEvent, instead, interpret the returned
> > value from WebViewImpl::handleInputEvent -- I think it is actually the best,
> but
> > will involve mass amount of work.
> 
> I'm not sure what this change would look like.  In general, I favor doing
things
> the cleaner way even if it means more work in the short term.  It sounds like
> this solution might be cleaner, but I don't have a good sense for the changes
> that would be required.

Alright the amount of work is not that massive, but is definitely messy.
We just need to return the EventStatus all the way up the calling chain:

RenderWidget::OnHandleInputEvent
  WebViewImpl::handleInputEvent
    PageWidgetDelegate::handleInputEvent
      WebViewImpl::handleGestureEvent

Then we can handle it in OnHandleInputEvent. In this case, we then really need
an enum for the status since we have to propagate the event back to browser only
if the event is unprocessed by JS.

> > And, I think link disambiguation is a case special enough to have its own
> status
> > value. The event is not delivered to WebCore, but isn't really unhandled
> either.
> > It is more like a reroute request.
> 
> I'm still confused about why you passed an EventStatus enum, which has three
> states, when you only really need to know if the event was cancelled.  The
code
> in this CL treats EventStatusUnprocessed and EventStatusProcessed
equivalently,
> so why bother distinguishing them in the WebKit API?

Ah, I see what you mean. You're right, neither Android or Win8 doesn't really
care if some JS handled the event. It might be either future-proof or
historical. I can remove it.

Which way do you prefer? I'm happy to do either.

[darin (slow to review), 2013-01-19 16:26:41]

On Fri, Jan 18, 2013 at 4:30 PM, <trchen@chromium.org> wrote:

> On 2013/01/19 00:08:15, darin wrote:
>
>> On 2013/01/15 23:28:52, trchen wrote:
>> > On 2013/01/15 23:06:03, darin wrote:
>> > 3. Completely remove didHandleGestureEvent, instead, interpret the
>> returned
>> > value from WebViewImpl::handleInputEvent -- I think it is actually the
>> best,
>> but
>> > will involve mass amount of work.
>>
>
>  I'm not sure what this change would look like.  In general, I favor doing
>>
> things
>
>> the cleaner way even if it means more work in the short term.  It sounds
>> like
>> this solution might be cleaner, but I don't have a good sense for the
>> changes
>> that would be required.
>>
>
> Alright the amount of work is not that massive, but is definitely messy.
> We just need to return the EventStatus all the way up the calling chain:
>
> RenderWidget::**OnHandleInputEvent
>   WebViewImpl::handleInputEvent
>     PageWidgetDelegate::**handleInputEvent
>       WebViewImpl::**handleGestureEvent
>
> Then we can handle it in OnHandleInputEvent. In this case, we then really
> need
> an enum for the status since we have to propagate the event back to
> browser only
> if the event is unprocessed by JS.
>
>
>  > And, I think link disambiguation is a case special enough to have its
>> own
>> status
>> > value. The event is not delivered to WebCore, but isn't really unhandled
>> either.
>> > It is more like a reroute request.
>>
>
>  I'm still confused about why you passed an EventStatus enum, which has
>> three
>> states, when you only really need to know if the event was cancelled.  The
>>
> code
>
>> in this CL treats EventStatusUnprocessed and EventStatusProcessed
>>
> equivalently,
>
>> so why bother distinguishing them in the WebKit API?
>>
>
> Ah, I see what you mean. You're right, neither Android or Win8 doesn't
> really
> care if some JS handled the event. It might be either future-proof or
> historical. I can remove it.
>
> Which way do you prefer? I'm happy to do either.
>

I'm OK with the EventStatus -> "bool was_cancelled" change for now.  I
think that plumbing EventStatus as the return value from handleInputEvent
will take a lot of work and be difficult to do on a branch.  However, it
sounds like the proper long term solution.  Maybe it can be done post M25?

My point in pushing back here is that I don't want us to add unnecessary
code because in aggregate that results in a more complicated code base.

-Darin


>
>
https://chromiumcodereview.**appspot.com/11473027/<https://chromiumcodereview...
>

[trchen, 2013-01-23 01:56:03]

New CL uploaded.
Let's do the quick "bool event_cancelled" change for now.
Will try if we can completely remove didHandleGestureEvent() later.

[darin (slow to review), 2013-01-23 06:58:22]

LGTM

https://codereview.chromium.org/11473027/diff/19001/content/renderer/render_w...
File content/renderer/render_widget.h (right):

https://codereview.chromium.org/11473027/diff/19001/content/renderer/render_w...
content/renderer/render_widget.h:145: bool event_cancelled) OVERRIDE;
NOTE: We normally do not add the OVERRIDE suffix when implementing WebKit API
methods.  The reason is that OVERRIDE makes it more painful to rev interfaces.

[commit-bot: I haz the power, 2013-01-23 22:31:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/trchen@chromium.org/11473027/25001

[commit-bot: I haz the power, 2013-01-23 23:28:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/trchen@chromium.org/11473027/25001

[commit-bot: I haz the power, 2013-01-24 00:36:58]

Change committed as 178438