Closed Bug 938827 Opened 6 years ago Closed 6 years ago

Remove reflection from FennecNativeActions/Driver

Categories

(Firefox for Android :: Testing, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 28

People

(Reporter: mcomella, Assigned: mcomella)

References

Details

Attachments

(11 files, 12 obsolete files)

7.39 KB, patch
rnewman
: review+
Details | Diff | Splinter Review
10.94 KB, patch
rnewman
: review+
Details | Diff | Splinter Review
2.68 KB, patch
rnewman
: review+
Details | Diff | Splinter Review
6.36 KB, patch
rnewman
: review+
Details | Diff | Splinter Review
7.59 KB, patch
rnewman
: review+
Details | Diff | Splinter Review
7.12 KB, patch
rnewman
: review+
Details | Diff | Splinter Review
7.88 KB, patch
mcomella
: review+
Details | Diff | Splinter Review
13.05 KB, patch
mcomella
: review+
Details | Diff | Splinter Review
5.15 KB, patch
mcomella
: review+
Details | Diff | Splinter Review
12.36 KB, patch
mcomella
: review+
Details | Diff | Splinter Review
1.30 KB, patch
mcomella
: review+
Details | Diff | Splinter Review
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.
Component: General → Testing
Assignee: nobody → michael.l.comella
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
Attachment #8343345 - Flags: review?(nalexander)
Realized I should not have removed RobocopAPI.registerEventListener just yet.
Attachment #8343347 - Attachment is obsolete: true
Attachment #8343347 - Flags: review?(nalexander)
Comment on attachment 8343345 [details] [diff] [review]
Part 1: Get LayerView in getSurfaceView.

Saving Nick's sanity.
Attachment #8343345 - Flags: review?(nalexander) → review?(rnewman)
Attachment #8343346 - Flags: review?(nalexander) → review?(rnewman)
Attachment #8343349 - Flags: review?(nalexander) → review?(rnewman)
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
Attachment #8343351 - Flags: review?(rnewman)
Attachment #8343384 - Attachment is obsolete: true
Attachment #8343384 - Flags: review?(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?
Attachment #8343345 - Flags: review?(rnewman) → review+
Attachment #8343346 - Flags: review?(rnewman) → review+
Attachment #8343351 - Flags: review?(rnewman) → review+
Attachment #8343349 - Flags: review?(rnewman) → review+
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?
Attachment #8343359 - Flags: review?(rnewman) → review+
Attachment #8343378 - Flags: review?(rnewman) → review+
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. :/
Attachment #8343386 - Flags: review?(rnewman) → review+
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.
Attachment #8343458 - Flags: review?(rnewman)
Attachment #8343460 - Flags: review?(rnewman) → review+
Simplified the GeckoEventExpecter class by moving the listener
construction/registry into the class constructor (with inspiration from part
10).
Attachment #8343477 - Flags: review?(rnewman)
Attachment #8343458 - Attachment is obsolete: true
Attachment #8343458 - Flags: review?(rnewman)
Attachment #8343460 - Attachment is obsolete: true
Comment on attachment 8343478 [details] [diff] [review]
Part 9: Remove unregisterEventListener reflection.

Move r+.
Attachment #8343478 - Flags: review+
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.
Attachment #8343481 - Flags: review?(rnewman)
Review comments (call loadSQLiteLibs once & s/mGeckoApp/mActivity).
Attachment #8343359 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8343477 - Attachment is obsolete: true
Attachment #8343477 - Flags: review?(rnewman)
Attachment #8343478 - Attachment is obsolete: true
Attachment #8343481 - Attachment is obsolete: true
Attachment #8343481 - Flags: review?(rnewman)
Comment on attachment 8343485 [details] [diff] [review]
Part 9: Remove unregisterEventListener reflection.

Move r+'s.
Attachment #8343485 - Flags: review+
(In reply to Richard Newman [:rnewman] from comment #13)
> Deliberate that you no longer log on failure?

Nope! ^_^
Attachment #8343489 - Flags: review?(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);
Attachment #8343477 - Attachment is obsolete: false
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
Attachment #8343487 - Flags: review?(rnewman) → review+
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);
Attachment #8343489 - Flags: review?(rnewman) → review+
Attachment #8343484 - Flags: review?(rnewman) → review+
Attachment #8343477 - Attachment is obsolete: true
Attachment #8343484 - Attachment is obsolete: true
Attachment #8343485 - Attachment is obsolete: true
Attachment #8343487 - Attachment is obsolete: true
Attachment #8343489 - Attachment is obsolete: true
Comment on attachment 8343900 [details] [diff] [review]
Part 11: Log error if LayerView is null.

Move r+'s.
Attachment #8343900 - Flags: review+
You need to log in before you can comment on or make changes to this bug.