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.

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

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

Comment on attachment 8343345 [details] [diff] [review] Part 1: Get LayerView in getSurfaceView. Saving Nick's sanity.

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

Forgot to compile.

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

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 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 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. :/

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.

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

Rebase.

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

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.

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

Rebase.

Rebase.

Rebase.

Comment on attachment 8343485 [details] [diff] [review] Part 9: Remove unregisterEventListener reflection. Move r+'s.

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

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

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 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 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);

Review comments.

Rebase.

Rebase.

Review comments.

Comment on attachment 8343900 [details] [diff] [review] Part 11: Log error if LayerView is null. Move r+'s.

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