Move InputEventAckState from port to public

Move InputEventAckState from port to common

Parties outside of content may be interested in responding to or passing along
the result of an input event response.  Eventually, InputEventAckState will
be replaced with a more descriptive and general container.

[jdduke (slow), 2013-05-24 18:14:23]

@joi: I wanted to run this move by you first, before I send out for further
review.

It's really a change of convenience; we could always add an intermediate public
type with appropriate translation back and forth, but InputEventAckState will
likely be replaced altogether in the near future. Thoughts?

[Jói, 2013-05-24 21:31:54]

I'm not sure I understand, where are you planning to use it from that you
currently can't?  I don't see you changing any APIs in content/public to use
this enum, so code outside of //content don't really have an opportunity to
apply it to anything, as far as I understand.

Please explain :)  I don't think it's a big deal to move this enum to public,
but not without some reason.

Cheers,
Jói

[jdduke (slow), 2013-05-24 21:38:42]

On 2013/05/24 21:31:54, Jói wrote:
> I'm not sure I understand, where are you planning to use it from that you
> currently can't?  I don't see you changing any APIs in content/public to use
> this enum, so code outside of //content don't really have an opportunity to
> apply it to anything, as far as I understand.
> 
> Please explain :)  I don't think it's a big deal to move this enum to public,
> but not without some reason.
> 
> Cheers,
> Jói

Context, yes, sorry! 

I originally had this move in crrev.com/15920002.  For WebView we'd like to
perform in-process input event handling between the browser and renderer.  The
bridge for the input response lives in
android_webview/browser/in_process_renderer/in_process_view_renderer.h, which
does not have access to content/port.  That patch also exposes the ACK in a
public header, content/public/browser/android/content_view_core.h, which
wouldn't be possible without this move.  Hopefully that's a bit more clear.

[jdduke (slow), 2013-05-24 21:40:33]

On 2013/05/24 21:38:42, jdduke wrote:
> On 2013/05/24 21:31:54, Jói wrote:
> > I'm not sure I understand, where are you planning to use it from that you
> > currently can't?  I don't see you changing any APIs in content/public to use
> > this enum, so code outside of //content don't really have an opportunity to
> > apply it to anything, as far as I understand.
> > 
> > Please explain :)  I don't think it's a big deal to move this enum to
public,
> > but not without some reason.
> > 
> > Cheers,
> > Jói
> 
> Context, yes, sorry! 
> 
> I originally had this move in crrev.com/15920002.  For WebView we'd like to
> perform in-process input event handling between the browser and renderer.  The
> bridge for the input response lives in
> android_webview/browser/in_process_renderer/in_process_view_renderer.h, which
> does not have access to content/port.  That patch also exposes the ACK in a
> public header, content/public/browser/android/content_view_core.h, which
> wouldn't be possible without this move.  Hopefully that's a bit more clear.

I factored out the move to this standalone patch mainly to avoid confusion, but
it makes little difference to me in which patch it lands.

[Jói, 2013-05-24 21:51:33]

LGTM, thanks for the explanation.

[commit-bot: I haz the power, 2013-05-24 23:06:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jdduke@chromium.org/15720007/1

[jdduke (slow), 2013-05-24 23:13:17]

@jam: Looks like I need an owner for content/. This is a single header move,
with appropriate #include changes. That is all. Thanks!

[commit-bot: I haz the power, 2013-05-24 23:15:44]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[joth, 2013-05-25 01:41:41]

lgtm

[jam, 2013-05-28 17:42:12]

I looked at https://codereview.chromium.org/15920002 some more which this is
split off from.

I don't see why we need to move the input event ack state to public. this seems
like an implementation detail of content (ACKing the input events IPCs).

ContentRendererClient::DidCreateSynchronousInputEventHandler is called by
RenderThreadImpl to pass in content::SynchronousInputEventFilter. That gets
cached by
android_webview/browser/in_process_renderer/in_process_view_renderer.cc only for
it to pass the SynchronousInputEventFilter back to content in
content_view_core_->SetInputEventFilterCallback

why not keep this logic of passing around the filter wholly inside content? it
seems the code would be simpler and there would be minimal content api changes.

am i missing something?

[jdduke (slow), 2013-05-29 14:59:01]

On 2013/05/28 17:42:12, jam wrote:
> I looked at https://codereview.chromium.org/15920002 some more which this is
> split off from.
> 
> I don't see why we need to move the input event ack state to public. this
seems
> like an implementation detail of content (ACKing the input events IPCs).
> 
> ContentRendererClient::DidCreateSynchronousInputEventHandler is called by
> RenderThreadImpl to pass in content::SynchronousInputEventFilter. That gets
> cached by
> android_webview/browser/in_process_renderer/in_process_view_renderer.cc only
for
> it to pass the SynchronousInputEventFilter back to content in
> content_view_core_->SetInputEventFilterCallback
> 
> why not keep this logic of passing around the filter wholly inside content? it
> seems the code would be simpler and there would be minimal content api
changes.
> 
> am i missing something?

@jam: You're not missing anything.  In review of crrev.com/15920002, a complaint
was made about passing around a handle to a non-public interface
(SynchronousInputEventHandler), even though it is only over used in content. By
making the interface public, we would by necessity have to move
InputEventAckState out of port and into public.

Now, android_webview has some components that are logically part of content.  I
think we're planning to move such pieces into content/*/android, which renders
this change unnecessary.  Pending those changes, I'll remove this CL.  In any
case thanks, for the review!

[jam, 2013-05-29 15:07:20]

On 2013/05/29 14:59:01, jdduke wrote:
> On 2013/05/28 17:42:12, jam wrote:
> > I looked at https://codereview.chromium.org/15920002 some more which this is
> > split off from.
> > 
> > I don't see why we need to move the input event ack state to public. this
> seems
> > like an implementation detail of content (ACKing the input events IPCs).
> > 
> > ContentRendererClient::DidCreateSynchronousInputEventHandler is called by
> > RenderThreadImpl to pass in content::SynchronousInputEventFilter. That gets
> > cached by
> > android_webview/browser/in_process_renderer/in_process_view_renderer.cc only
> for
> > it to pass the SynchronousInputEventFilter back to content in
> > content_view_core_->SetInputEventFilterCallback
> > 
> > why not keep this logic of passing around the filter wholly inside content?
it
> > seems the code would be simpler and there would be minimal content api
> changes.
> > 
> > am i missing something?
> 
> @jam: You're not missing anything.  In review of crrev.com/15920002, a
complaint
> was made about passing around a handle to a non-public interface
> (SynchronousInputEventHandler), even though it is only over used in content.
By
> making the interface public, we would by necessity have to move
> InputEventAckState out of port and into public.

Perhaps I wasn't clear. What I meant is can we avoid passing the handle
(independent of whether it's public or not) from content to the embedder, and
then back to content? i.e. why can't content manage this handle internally and
not have to pass it to the embedder, which all it does is cache it for a bit and
then send it back to content (based on a call from content to the embedder). it
seems like this is something that content should manage.

> 
> Now, android_webview has some components that are logically part of content. 
I
> think we're planning to move such pieces into content/*/android, which renders
> this change unnecessary.  Pending those changes, I'll remove this CL.  In any
> case thanks, for the review!

[jdduke (slow), 2013-05-29 15:28:50]

> Perhaps I wasn't clear. What I meant is can we avoid passing the handle
> (independent of whether it's public or not) from content to the embedder, and
> then back to content? i.e. why can't content manage this handle internally and
> not have to pass it to the embedder, which all it does is cache it for a bit
and
> then send it back to content (based on a call from content to the embedder).
it
> seems like this is something that content should manage.

At the moment, the embedder (in android_webview) facilitates synchronous
interfacing between content/browser and content/renderer.  If there's a saner
way of pushing a handle from the renderer to the browser while maintaining
ownership in the renderer, I'm all ears.

[jam, 2013-05-29 15:40:06]

On Wed, May 29, 2013 at 8:28 AM, <jdduke@chromium.org> wrote:

> Perhaps I wasn't clear. What I meant is can we avoid passing the handle
>> (independent of whether it's public or not) from content to the embedder,
>> and
>> then back to content? i.e. why can't content manage this handle
>> internally and
>> not have to pass it to the embedder, which all it does is cache it for a
>> bit
>>
> and
>
>> then send it back to content (based on a call from content to the
>> embedder).
>>
> it
>
>> seems like this is something that content should manage.
>>
>
> At the moment, the embedder (in android_webview) facilitates synchronous
> interfacing between content/browser and content/renderer.  If there's a
> saner
> way of pushing a handle from the renderer to the browser while maintaining
> ownership in the renderer, I'm all ears.
>

What do you think of my suggestion? I don't have time to code it up (and
I'm not setup for buildling Android).

I'm referring specifically to the fact that content calls the embedder with
a handle which the embedder caches. Then content calls the embedder later,
and the embedder gives back to content the cached handle. It seems that
content can cache the handle itself.

>
>
https://codereview.chromium.**org/15720007/<https://codereview.chromium.org/1...
>

[jdduke (slow), 2013-05-29 15:54:06]

> What do you think of my suggestion? I don't have time to code it up (and
> I'm not setup for buildling Android).
> 
> I'm referring specifically to the fact that content calls the embedder with
> a handle which the embedder caches. Then content calls the embedder later,
> and the embedder gives back to content the cached handle. It seems that
> content can cache the handle itself.

I think it's a great suggestion... As I said, I'm not exactly sure how one might
go about calling an entity contained in the renderer (in RenderThreadImpl) from
the browser (RenderWidgetHostViewAndroid).  Sorry, perhaps I'm missing the
obvious here?

[jdduke (slow), 2013-05-30 00:17:53]

With crrev.com/16119003/, this is no longer necessary. Thanks.