938827 - Remove reflection from FennecNativeActions/Driver

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Testing, defect)

Description

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

While I have not throughly checked how possible it is, we should try to remove the reflection code from FennecNativeActions/Driver. Note that this is more likely as bug 916507 lands some changes that allow us to import org.mozilla.gecko classes directly.

Comment 1

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 1: Get LayerView in getSurfaceView.

If you feel ckitching or rnewman would be a better reviewer since they're
(probably) more familiar with the @RobocopTarget stuff, feel free to pass it
off to them. :P

Comment 2

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 2: Call PanningPerfAPI directly.

Comment 3

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 3: Call registerEventListener directly.

Comment 4

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 4: Remove remaining reflection from FennecNativeDriver.

Comment 5

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 3: Call registerEventListener directly. v2

Realized I should not have removed RobocopAPI.registerEventListener just yet.

Comment 6

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 5: Remove querySql reflection.

Comment 7

[Richard Newman [:rnewman]]

Comment on attachment 8343345 [details] [diff] [review]
Part 1: Get LayerView in getSurfaceView.

Saving Nick's sanity.

Comment 8

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8343351 [details] [diff] [review]
Part 3: Call registerEventListener directly. v2

(In reply to Richard Newman [:rnewman] from comment #7)
> Saving Nick's sanity.

<3

Comment 9

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 6: Remove BroadcastEvent reflection.

Comment 10

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 7: Remove preferences event reflection.

Comment 11

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 7: Remove preferences event reflection. v2

Forgot to compile.

Comment 12

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

parts 1-4: https://tbpl.mozilla.org/?tree=Try&rev=11cc12c0a24f

Comment 13

[Richard Newman [:rnewman]]

Comment on attachment 8343345 [details] [diff] [review]
Part 1: Get LayerView in getSurfaceView.

Review of attachment 8343345 [details] [diff] [review]:
-----------------------------------------------------------------

::: build/mobile/robocop/FennecNativeDriver.java
@@ +234,5 @@
>          return 0.0f;
>      }
>  
> +    private LayerView getSurfaceView() {
> +        return mSolo.getView(LayerView.class, 0);

Deliberate that you no longer log on failure?

Comment 14

[Richard Newman [:rnewman]]

Comment on attachment 8343359 [details] [diff] [review]
Part 5: Remove querySql reflection.

Review of attachment 8343359 [details] [diff] [review]:
-----------------------------------------------------------------

::: build/mobile/robocop/FennecNativeActions.java
@@ +496,5 @@
>          mSolo.drag(startingX, endingX, startingY, endingY, 10);
>      }
>  
> +    public Cursor querySql(final String dbPath, final String sql) {
> +        GeckoLoader.loadSQLiteLibs(mGeckoApp, mGeckoApp.getApplication().getPackageResourcePath());

Just call this once in the constructor (right after mGeckoApp is assigned)? Seems pointless to be hitting the synchronized block in GeckoLoader every time.

Also, while you're here, s/mGeckoApp/mActivity?

Comment 15

[Richard Newman [:rnewman]]

Comment on attachment 8343386 [details] [diff] [review]
Part 7: Remove preferences event reflection. v2

Review of attachment 8343386 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/GeckoEvent.java
@@ +708,1 @@
>      public static GeckoEvent createPreferencesObserveEvent(int requestId, String[] prefNames) {

We have too many of these. :/

Comment 16

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 8: Remove registerEventListener reflection.

I set up GeckoEventExpecter for re-use because this approach was cleaner than
recreating the old behavior to disable reuse (though it might be safer to
disallow it so future updates don't get it wrong - let me know what you think).

Additionally, I took some liberties in the listener created in expectGeckoEvent 
to not have hashCode return 314 and each method to print a statement because
it seems hashCode() is unused and I don't think we care which methods are
called on listener.

I also rearranged some imports.

Comment 17

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 9: Remove unregisterEventListener reflection.

Comment 18

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 8: Remove registerEventListener reflection. v2

Simplified the GeckoEventExpecter class by moving the listener
construction/registry into the class constructor (with inspiration from part
10).

Comment 19

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 9: Remove unregisterEventListener reflection.

Rebase.

Comment 20

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8343478 [details] [diff] [review]
Part 9: Remove unregisterEventListener reflection.

Move r+.

Comment 21

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 10: Remove remaining reflection from FennecNativeActions.

Last major chunk sans any review comment patches.

I store the GeckoLayerClient for unregistering the DrawListener which is not
faithful to the original implementation, but I feel it is more correct.

Like part 8, I took liberties with how each of the methods of the DrawListener
are handled because I assume we don't need that unexpected functionality.

Comment 22

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 5: Remove querySql reflection.

Review comments (call loadSQLiteLibs once & s/mGeckoApp/mActivity).

Comment 23

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 8: Remove registerEventListener reflection.

Rebase.

Comment 24

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 9: Remove unregisterEventListener reflection.

Rebase.

Comment 25

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 10: Remove remaining reflection from FennecNativeActions.

Rebase.

Comment 26

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8343485 [details] [diff] [review]
Part 9: Remove unregisterEventListener reflection.

Move r+'s.

Comment 27

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 11: Log error if LayerView is null.

(In reply to Richard Newman [:rnewman] from comment #13)
> Deliberate that you no longer log on failure?

Nope! ^_^

Comment 28

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

parts 1-11: https://tbpl.mozilla.org/?tree=Try&rev=d12b26a64c93

Comment 29

[Richard Newman [:rnewman]]

Comment on attachment 8343477 [details] [diff] [review]
Part 8: Remove registerEventListener reflection. v2

Review of attachment 8343477 [details] [diff] [review]:
-----------------------------------------------------------------

::: build/mobile/robocop/FennecNativeActions.java
@@ +78,2 @@
>  
> +        private boolean mIsRegistered;

This seems like something that should be volatile or an AtomicBoolean, unless you're sure that blocking and unregistering only ever happen on the same thread.

@@ +80,3 @@
>  
> +        private final String mGeckoEvent;
> +        private GeckoEventListener mListener;

final.

@@ +98,5 @@
> +                @Override
> +                public void handleMessage(final String event, final JSONObject message) {
> +                    FennecNativeDriver.log(FennecNativeDriver.LogLevel.DEBUG,
> +                            "handleMessage called for: " + event + "; expecting: " + mGeckoEvent);
> +                    mAsserter.is(event, mGeckoEvent, "Given message occured for registered event");

occurred.

@@ +212,2 @@
>              synchronized (this) {
>                  mEventEverReceived = true;

This is only ever assigned here, and read in eventReceived. Just make it volatile.

@@ +221,1 @@
>                  FennecNativeDriver.log(LogLevel.ERROR, e);

One line:

.log(LogLevel.ERROR, "..." + message.toString(), e);

Comment 30

[Richard Newman [:rnewman]]

Comment on attachment 8343487 [details] [diff] [review]
Part 10: Remove remaining reflection from FennecNativeActions.

Review of attachment 8343487 [details] [diff] [review]:
-----------------------------------------------------------------

::: build/mobile/robocop/FennecNativeActions.java
@@ +31,5 @@
>  import com.jayway.android.robotium.solo.Solo;
>  
>  import static org.mozilla.gecko.FennecNativeDriver.LogLevel;
>  
>  public class FennecNativeActions implements Actions {

I don't know if you've noticed, but FennecNativeActions has basically become RobocopAPI :P

Comment 31

[Richard Newman [:rnewman]]

Comment on attachment 8343489 [details] [diff] [review]
Part 11: Log error if LayerView is null.

Review of attachment 8343489 [details] [diff] [review]:
-----------------------------------------------------------------

::: build/mobile/robocop/FennecNativeDriver.java
@@ +179,5 @@
> +
> +        if (layerView == null) {
> +            log(LogLevel.WARN, "getSurfaceView could not find LayerView");
> +            for (final View v : mSolo.getViews()) {
> +                log(LogLevel.WARN, v.toString());

log(LogLevel.WARN, "  View: " + v);

Comment 32

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 8: Remove registerEventListener reflection. v3

Review comments.

Comment 33

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 9: Remove unregisterEventListener reflection.

Rebase.

Comment 34

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 10: Remove remaining reflection from FennecNativeActions.

Rebase.

Comment 35

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 11: Log error if LayerView is null.

Review comments.

Comment 36

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8343900 [details] [diff] [review]
Part 11: Log error if LayerView is null.

Move r+'s.

Comment 37

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://hg.mozilla.org/integration/fx-team/pushloghtml?changeset=f9cf4d4e6c4a

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/11159bb95e5c
https://hg.mozilla.org/mozilla-central/rev/0c262d398106
https://hg.mozilla.org/mozilla-central/rev/4c1017736f23
https://hg.mozilla.org/mozilla-central/rev/fc610be8d675
https://hg.mozilla.org/mozilla-central/rev/334bc44411df
https://hg.mozilla.org/mozilla-central/rev/dc0e2b5947dd
https://hg.mozilla.org/mozilla-central/rev/d5b75a0584f0
https://hg.mozilla.org/mozilla-central/rev/6607f4bf80e5
https://hg.mozilla.org/mozilla-central/rev/16a66a870a7a
https://hg.mozilla.org/mozilla-central/rev/3778c2024dbe
https://hg.mozilla.org/mozilla-central/rev/f9cf4d4e6c4a