Remove reflection from FennecNativeActions/Driver

RESOLVED FIXED in Firefox 28

Status

()

Firefox for Android
Testing
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: mcomella, Assigned: mcomella)

Tracking

unspecified
Firefox 28
All
Android
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(11 attachments, 12 obsolete attachments)

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
(Assignee)

Description

4 years ago
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.
(Assignee)

Updated

4 years ago
Component: General → Testing
(Assignee)

Updated

4 years ago
Assignee: nobody → michael.l.comella
(Assignee)

Comment 1

4 years ago
Created attachment 8343345 [details] [diff] [review]
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
Attachment #8343345 - Flags: review?(nalexander)
(Assignee)

Comment 2

4 years ago
Created attachment 8343346 [details] [diff] [review]
Part 2: Call PanningPerfAPI directly.
Attachment #8343346 - Flags: review?(nalexander)
(Assignee)

Comment 3

4 years ago
Created attachment 8343347 [details] [diff] [review]
Part 3: Call registerEventListener directly.
Attachment #8343347 - Flags: review?(nalexander)
(Assignee)

Comment 4

4 years ago
Created attachment 8343349 [details] [diff] [review]
Part 4: Remove remaining reflection from FennecNativeDriver.
Attachment #8343349 - Flags: review?(nalexander)
(Assignee)

Comment 5

4 years ago
Created attachment 8343351 [details] [diff] [review]
Part 3: Call registerEventListener directly. v2

Realized I should not have removed RobocopAPI.registerEventListener just yet.
(Assignee)

Updated

4 years ago
Attachment #8343347 - Attachment is obsolete: true
Attachment #8343347 - Flags: review?(nalexander)
(Assignee)

Comment 6

4 years ago
Created attachment 8343359 [details] [diff] [review]
Part 5: Remove querySql reflection.
Attachment #8343359 - Flags: review?(rnewman)
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)
(Assignee)

Comment 8

4 years ago
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)
(Assignee)

Comment 9

4 years ago
Created attachment 8343378 [details] [diff] [review]
Part 6: Remove BroadcastEvent reflection.
Attachment #8343378 - Flags: review?(rnewman)
(Assignee)

Comment 10

4 years ago
Created attachment 8343384 [details] [diff] [review]
Part 7: Remove preferences event reflection.
Attachment #8343384 - Flags: review?(rnewman)
(Assignee)

Comment 11

4 years ago
Created attachment 8343386 [details] [diff] [review]
Part 7: Remove preferences event reflection. v2

Forgot to compile.
Attachment #8343386 - Flags: review?(rnewman)
(Assignee)

Updated

4 years ago
Attachment #8343384 - Attachment is obsolete: true
Attachment #8343384 - Flags: review?(rnewman)
(Assignee)

Comment 12

4 years ago
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?
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+
(Assignee)

Comment 16

4 years ago
Created attachment 8343458 [details] [diff] [review]
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.
Attachment #8343458 - Flags: review?(rnewman)
(Assignee)

Comment 17

4 years ago
Created attachment 8343460 [details] [diff] [review]
Part 9: Remove unregisterEventListener reflection.
Attachment #8343460 - Flags: review?(rnewman)
Attachment #8343460 - Flags: review?(rnewman) → review+
(Assignee)

Comment 18

4 years ago
Created attachment 8343477 [details] [diff] [review]
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).
Attachment #8343477 - Flags: review?(rnewman)
(Assignee)

Updated

4 years ago
Attachment #8343458 - Attachment is obsolete: true
Attachment #8343458 - Flags: review?(rnewman)
(Assignee)

Comment 19

4 years ago
Created attachment 8343478 [details] [diff] [review]
Part 9: Remove unregisterEventListener reflection.

Rebase.
(Assignee)

Updated

4 years ago
Attachment #8343460 - Attachment is obsolete: true
(Assignee)

Comment 20

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

Move r+.
Attachment #8343478 - Flags: review+
(Assignee)

Comment 21

4 years ago
Created attachment 8343481 [details] [diff] [review]
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.
Attachment #8343481 - Flags: review?(rnewman)
(Assignee)

Comment 22

4 years ago
Created attachment 8343483 [details] [diff] [review]
Part 5: Remove querySql reflection.

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

Updated

4 years ago
Attachment #8343359 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 23

4 years ago
Created attachment 8343484 [details] [diff] [review]
Part 8: Remove registerEventListener reflection.

Rebase.
Attachment #8343484 - Flags: review?(rnewman)
(Assignee)

Updated

4 years ago
Attachment #8343477 - Attachment is obsolete: true
Attachment #8343477 - Flags: review?(rnewman)
(Assignee)

Comment 24

4 years ago
Created attachment 8343485 [details] [diff] [review]
Part 9: Remove unregisterEventListener reflection.

Rebase.
(Assignee)

Updated

4 years ago
Attachment #8343478 - Attachment is obsolete: true
(Assignee)

Comment 25

4 years ago
Created attachment 8343487 [details] [diff] [review]
Part 10: Remove remaining reflection from FennecNativeActions.

Rebase.
Attachment #8343487 - Flags: review?(rnewman)
(Assignee)

Updated

4 years ago
Attachment #8343481 - Attachment is obsolete: true
Attachment #8343481 - Flags: review?(rnewman)
(Assignee)

Updated

4 years ago
Attachment #8343483 - Flags: review+
(Assignee)

Comment 26

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

Move r+'s.
Attachment #8343485 - Flags: review+
(Assignee)

Comment 27

4 years ago
Created attachment 8343489 [details] [diff] [review]
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! ^_^
Attachment #8343489 - Flags: review?(rnewman)
(Assignee)

Comment 28

4 years ago
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);
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+
(Assignee)

Comment 32

4 years ago
Created attachment 8343893 [details] [diff] [review]
Part 8: Remove registerEventListener reflection. v3

Review comments.
(Assignee)

Updated

4 years ago
Attachment #8343477 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #8343484 - Attachment is obsolete: true
(Assignee)

Comment 33

4 years ago
Created attachment 8343895 [details] [diff] [review]
Part 9: Remove unregisterEventListener reflection.

Rebase.
(Assignee)

Updated

4 years ago
Attachment #8343485 - Attachment is obsolete: true
(Assignee)

Comment 34

4 years ago
Created attachment 8343898 [details] [diff] [review]
Part 10: Remove remaining reflection from FennecNativeActions.

Rebase.
(Assignee)

Updated

4 years ago
Attachment #8343487 - Attachment is obsolete: true
(Assignee)

Comment 35

4 years ago
Created attachment 8343900 [details] [diff] [review]
Part 11: Log error if LayerView is null.

Review comments.
(Assignee)

Updated

4 years ago
Attachment #8343489 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #8343893 - Flags: review+
(Assignee)

Updated

4 years ago
Attachment #8343895 - Flags: review+
(Assignee)

Updated

4 years ago
Attachment #8343898 - Flags: review+
(Assignee)

Comment 36

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

Move r+'s.
Attachment #8343900 - Flags: review+
(Assignee)

Comment 37

4 years ago
https://hg.mozilla.org/integration/fx-team/pushloghtml?changeset=f9cf4d4e6c4a
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
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 28
You need to log in before you can comment on or make changes to this bug.