Replace WindowCallbackWrapper with a generated instance.

Replace WindowCallbackWrapper with a generated instance.

Rather than having to manually define each function in Window.Callback
and forward it along, instead use reflection to automatically forward
all calls that aren't interesting.

BUG=487378
Committed: https://crrev.com/879d892afd2a9bf04443064ea2836a8c9a9f1019
Cr-Commit-Position: refs/heads/master@{#330015}

[Yaron, 2015-05-12 19:46:31]

yfriedman@chromium.org changed reviewers:
+ aurimas@chromium.org, dtrainor@chromium.org

[Yaron, 2015-05-12 19:46:32]

dtrainor: I think you added windowfocus
aurimas: you added the appcompat fix

This appears to be working locally through random logs but I'm not 100% sure how
to test that the callers are actually doing what's expected. Suggestions welcome

[Yaron, 2015-05-12 19:47:51]

https://codereview.chromium.org/1139913005/diff/1/base/android/java/src/org/c...
File base/android/java/src/org/chromium/base/BaseChromiumApplication.java
(right):

https://codereview.chromium.org/1139913005/diff/1/base/android/java/src/org/c...
base/android/java/src/org/chromium/base/BaseChromiumApplication.java:46: private
final Activity mActivity;
I was worried about this Activity reference, but we were already binding it in
onWindowFocusChanged. I could be mistaken but I think this is equivalent.

[David Trainor- moved to gerrit, 2015-05-12 21:11:29]

On 2015/05/12 19:47:51, Yaron wrote:
>
https://codereview.chromium.org/1139913005/diff/1/base/android/java/src/org/c...
> File base/android/java/src/org/chromium/base/BaseChromiumApplication.java
> (right):
> 
>
https://codereview.chromium.org/1139913005/diff/1/base/android/java/src/org/c...
> base/android/java/src/org/chromium/base/BaseChromiumApplication.java:46:
private
> final Activity mActivity;
> I was worried about this Activity reference, but we were already binding it in
> onWindowFocusChanged. I could be mistaken but I think this is equivalent.

Reflection ftw haha!  Okay two mutually exclusive suggestions:

1. Add some unit tests to catch if the signature of these methods change.  If
they do, we won't get a compile time break we'll just stop getting the override
calls right?

2. We could do away with reflection and add an interface called
BaseChromiumWindowCallback that just has the two methods we're interested in. 
Then we could either:
  - Have a BaseChromiumApplication method with a signature like "Window.Callback
buildWindowCallback(BaseChromiumWindowCallback callback)" that gets overridden
downstream to build a different Window.Callback and upstream builds the default
WindowCallbackWrapper.
  - We could have a factory that does something similar and downstream sets a
different factory to build a different Window.Callback wrapper.  Newton set up
some hook for setting factories like this IIRC, although the former bullet point
is probably cleaner than this way.

wdyt?

ps: how's the weather up there?

[Yaron, 2015-05-12 21:32:31]

On 2015/05/12 21:11:29, David Trainor wrote:
> On 2015/05/12 19:47:51, Yaron wrote:
> >
>
https://codereview.chromium.org/1139913005/diff/1/base/android/java/src/org/c...
> > File base/android/java/src/org/chromium/base/BaseChromiumApplication.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/1139913005/diff/1/base/android/java/src/org/c...
> > base/android/java/src/org/chromium/base/BaseChromiumApplication.java:46:
> private
> > final Activity mActivity;
> > I was worried about this Activity reference, but we were already binding it
in
> > onWindowFocusChanged. I could be mistaken but I think this is equivalent.
> 
> Reflection ftw haha!  Okay two mutually exclusive suggestions:
> 
> 1. Add some unit tests to catch if the signature of these methods change.  If
> they do, we won't get a compile time break we'll just stop getting the
override
> calls right?
> 
> 2. We could do away with reflection and add an interface called
> BaseChromiumWindowCallback that just has the two methods we're interested in. 
> Then we could either:
>   - Have a BaseChromiumApplication method with a signature like
"Window.Callback
> buildWindowCallback(BaseChromiumWindowCallback callback)" that gets overridden
> downstream to build a different Window.Callback and upstream builds the
default
> WindowCallbackWrapper.

Hmm. Even if it's overridden, the upstream one still won't compile if it's
defining a new Window.Callback that's supposed to implement all functions and
forward them.

>   - We could have a factory that does something similar and downstream sets a
> different factory to build a different Window.Callback wrapper.  Newton set up
> some hook for setting factories like this IIRC, although the former bullet
point
> is probably cleaner than this way.

Same issue? Maybe I just don't fully get the proposal
> 
> wdyt?
> 
> ps: how's the weather up there?

Heh. It got colder today (~15C) but can't complain :)

[David Trainor- moved to gerrit, 2015-05-12 21:39:20]

On 2015/05/12 21:32:31, Yaron wrote:
> On 2015/05/12 21:11:29, David Trainor wrote:
> > On 2015/05/12 19:47:51, Yaron wrote:
> > >
> >
>
https://codereview.chromium.org/1139913005/diff/1/base/android/java/src/org/c...
> > > File base/android/java/src/org/chromium/base/BaseChromiumApplication.java
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1139913005/diff/1/base/android/java/src/org/c...
> > > base/android/java/src/org/chromium/base/BaseChromiumApplication.java:46:
> > private
> > > final Activity mActivity;
> > > I was worried about this Activity reference, but we were already binding
it
> in
> > > onWindowFocusChanged. I could be mistaken but I think this is equivalent.
> > 
> > Reflection ftw haha!  Okay two mutually exclusive suggestions:
> > 
> > 1. Add some unit tests to catch if the signature of these methods change. 
If
> > they do, we won't get a compile time break we'll just stop getting the
> override
> > calls right?
> > 
> > 2. We could do away with reflection and add an interface called
> > BaseChromiumWindowCallback that just has the two methods we're interested
in. 
> > Then we could either:
> >   - Have a BaseChromiumApplication method with a signature like
> "Window.Callback
> > buildWindowCallback(BaseChromiumWindowCallback callback)" that gets
overridden
> > downstream to build a different Window.Callback and upstream builds the
> default
> > WindowCallbackWrapper.
> 
> Hmm. Even if it's overridden, the upstream one still won't compile if it's
> defining a new Window.Callback that's supposed to implement all functions and
> forward them.

Crap good point I forgot about that.  I guess we could make the class abstract
and do some more trickery :(.  Maybe option 1 is better along with some tests...

> 
> >   - We could have a factory that does something similar and downstream sets
a
> > different factory to build a different Window.Callback wrapper.  Newton set
up
> > some hook for setting factories like this IIRC, although the former bullet
> point
> > is probably cleaner than this way.
> 
> Same issue? Maybe I just don't fully get the proposal
> > 
> > wdyt?
> > 
> > ps: how's the weather up there?
> 
> Heh. It got colder today (~15C) but can't complain :)

[Yaron, 2015-05-13 21:12:01]

added robolectric test

[Yaron, 2015-05-14 16:55:30]

On 2015/05/13 21:12:01, Yaron wrote:
> added robolectric test

updated test to be a little more accurate (I think). It also highlights an
interesting case where the Menu event actually can get dispatched twice. Not
sure if that was intentional, Aurimas?

[David Trainor- moved to gerrit, 2015-05-14 17:34:03]

Thanks for the tests!  This lgtm.

[Yaron, 2015-05-14 18:50:52]

On 2015/05/14 17:34:03, David Trainor wrote:
> Thanks for the tests!  This lgtm.

Disdu

[Yaron, 2015-05-14 18:50:56]

The CQ bit was checked by yfriedman@chromium.org

[Yaron, 2015-05-14 18:52:05]

On 2015/05/14 18:50:52, Yaron wrote:
> On 2015/05/14 17:34:03, David Trainor wrote:
> > Thanks for the tests!  This lgtm.
> 
> Disdu

arg. I lost my longer comment but it was discussed off-thread and this is
intentional and harmless since it's only called twice when event isn't consumed
anyway.

[commit-bot: I haz the power, 2015-05-14 18:52:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1139913005/40001

[Yaron, 2015-05-15 00:21:22]

The CQ bit was unchecked by yfriedman@chromium.org

[Yaron, 2015-05-15 01:29:16]

On 2015/05/14 18:52:21, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/1139913005/40001

Fixed the assert. It was testing that it was an instance of the
InvocationHandler when in reality it should be a proxy object. Updated to match
that.

[Yaron, 2015-05-15 01:29:24]

The CQ bit was checked by yfriedman@chromium.org

[Yaron, 2015-05-15 01:29:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtrainor@chromium.org
Link to the patchset: https://codereview.chromium.org/1139913005/#ps60001
(title: "update assert")

[commit-bot: I haz the power, 2015-05-15 01:30:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1139913005/60001

[commit-bot: I haz the power, 2015-05-15 01:39:43]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-05-15 01:40:40]

Patchset 4 (id:??) landed as
https://crrev.com/879d892afd2a9bf04443064ea2836a8c9a9f1019
Cr-Commit-Position: refs/heads/master@{#330015}