Closed Bug 1441982 Opened 2 years ago Closed 2 years ago

Add tests for GeckoSession.NavigationListener.onNewSession()

Categories

(GeckoView :: General, enhancement)

enhancement
Not set

Tracking

(firefox60 fixed)

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: snorp, Assigned: snorp)

References

(Blocks 1 open bug)

Details

Attachments

(8 files)

59 bytes, text/x-review-board-request
jchen
: review+
Details
59 bytes, text/x-review-board-request
jchen
: review+
Details
59 bytes, text/x-review-board-request
jchen
: review+
Details
59 bytes, text/x-review-board-request
jchen
: review+
Details
59 bytes, text/x-review-board-request
jchen
: review+
Details
59 bytes, text/x-review-board-request
jchen
: review+
Details
59 bytes, text/x-review-board-request
jchen
: review+
Details
59 bytes, text/x-review-board-request
jchen
: review+
Details
Comment on attachment 8954920 [details]
Bug 1441982 - Add a WithDisplay annotation to GeckoSessionTestRule

https://reviewboard.mozilla.org/r/224078/#review230138

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/rule/GeckoSessionTestRule.java:83
(Diff revision 1)
>      /**
> +     * Specify the display size for the GeckoSession in device pixels
> +     */
> +    @Target({ElementType.METHOD, ElementType.TYPE})
> +    @Retention(RetentionPolicy.RUNTIME)
> +    public @interface WithDisplaySize {

I slightly prefer `WithDisplay`

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/rule/GeckoSessionTestRule.java:439
(Diff revision 1)
>              InstrumentationRegistry.getInstrumentation();
>      protected final GeckoSessionSettings mDefaultSettings;
>  
>      protected ErrorCollector mErrorCollector;
>      protected GeckoSession mSession;
> +    protected Point mDisplaySize;

`Size` technically? But not big deal

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/rule/GeckoSessionTestRule.java:544
(Diff revision 1)
>              } else if (Setting.List.class.equals(annotation.annotationType())) {
>                  for (final Setting setting : ((Setting.List) annotation).value()) {
>                      setting.key().set(settings, setting.value());
>                  }
> +            } else if (WithDisplaySize.class.equals(annotation.annotationType())) {
> +                WithDisplaySize displaySize = (WithDisplaySize)annotation;

final

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/rule/GeckoSessionTestRule.java:600
(Diff revision 1)
>          mSession = new GeckoSession(settings);
>  
> +        if (mDisplaySize != null) {
> +            final SurfaceTexture texture = new SurfaceTexture(0);
> +            final Surface surface = new Surface(texture);
> +            mSession.acquireDisplay().surfaceChanged(surface, mDisplaySize.x, mDisplaySize.y);

We should call `surfaceDestroyed` and `releaseDisplay` when cleaning up.
Attachment #8954920 - Flags: review?(nchen) → review+
Comment on attachment 8954923 [details]
Bug 1441982 - Don't wrap RuntimeException instances if they are already a RuntimeException

https://reviewboard.mozilla.org/r/224084/#review230144

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/rule/GeckoSessionTestRule.java:550
(Diff revision 1)
>                  mDisplaySize = new Point(displaySize.width(), displaySize.height());
>              }
>          }
>      }
>  
> +    private static RuntimeException createRuntimeException(Throwable e) {

Maybe `unwrapRuntimeException`?
Attachment #8954923 - Flags: review?(nchen) → review+
Comment on attachment 8954924 [details]
Bug 1441982 - Queue input events while waiting for APZ to attach

https://reviewboard.mozilla.org/r/224086/#review230146

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/gfx/NativePanZoomController.java:48
(Diff revision 1)
> +            this.source = source;
> +            this.event = event;
> +        }
> +    };
> +
> +    private ArrayDeque<QueuedMotionEvent> mQueuedEvents = new ArrayDeque<>();

I would just use `Pair<Integer, MotionEvent>`, where `Integer` is an event source constant, instead of creating two classes here.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/gfx/NativePanZoomController.java:261
(Diff revision 1)
> +        if (mQueuedEvents == null) {
> +            return;
> +        }
> +
> +        ArrayDeque<QueuedMotionEvent> events = mQueuedEvents;
> +        mQueuedEvents = null;

NPZC can be detached and later attached again (through a GeckoSession.closeWindow/openWindow cycle), and we need to re-enable queuing in that case.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/gfx/NativePanZoomController.java:511
(Diff revision 1)
>              mPointerState.pointers.remove(pointerIndex);
>          }
>      }
>  
>      @WrapForJNI(calledFrom = "ui")
> -    private void synthesizeNativeTouchPoint(int pointerId, int eventType, int clientX,
> +    public void synthesizeNativeTouchPoint(int pointerId, int eventType, int clientX,

We should not expose this as public API.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/gfx/NativePanZoomController.java:521
(Diff revision 1)
>          synthesizeNativePointer(InputDevice.SOURCE_TOUCHSCREEN, pointerId,
>                                  eventType, clientX, clientY, pressure, orientation);
>      }
>  
>      @WrapForJNI(calledFrom = "ui")
> -    private void synthesizeNativeMouseEvent(int eventType, int clientX, int clientY) {
> +    public void synthesizeNativeMouseEvent(int eventType, int clientX, int clientY) {

We should not expose this as public API.
Comment on attachment 8954921 [details]
Bug 1441982 - Add GeckoSessionTestRule.synthesizeTap()

https://reviewboard.mozilla.org/r/224080/#review230140

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/rule/GeckoSessionTestRule.java:996
(Diff revision 1)
>      public void delegateDuringNextWait(final Object callback) {
>          mWaitScopeDelegates.delegate(callback);
>      }
> +
> +    public void synthesizeClick(int x, int y) {
> +        mSession.getPanZoomController().synthesizeNativeMouseEvent(MotionEvent.ACTION_POINTER_DOWN, x, y);

We shouldn't be using private API here.
Comment on attachment 8954925 [details]
Bug 1441982 - Add a test for GeckoSession.NavigationListener.onNewSession()

https://reviewboard.mozilla.org/r/224088/#review230148
Attachment #8954925 - Flags: review?(nchen) → review+
Comment on attachment 8954926 [details]
Bug 1441982 - Remove old busted GeckoView NavigationTests

https://reviewboard.mozilla.org/r/224090/#review230150
Attachment #8954926 - Flags: review?(nchen) → review+
Comment on attachment 8954922 [details]
Bug 1441982 - Only allow an unopened GeckoSession as a response to onNewSession()

https://reviewboard.mozilla.org/r/224082/#review230142

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java:162
(Diff revision 1)
> +                                }
> +
> +                                if (session.isOpen()) {
> +                                    throw new IllegalArgumentException("Must use a new, unopened GeckoSession instance");
>                                  }
>  

We need to call `session.openWindow` here?

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java:163
(Diff revision 1)
> +
> +                                if (session.isOpen()) {
> +                                    throw new IllegalArgumentException("Must use a new, unopened GeckoSession instance");
>                                  }
>  
> -                                callback.sendSuccess(session != null ? session.getId() : null);
> +                                callback.sendSuccess(session);

`session.getId()`
Comment on attachment 8954921 [details]
Bug 1441982 - Add GeckoSessionTestRule.synthesizeTap()

https://reviewboard.mozilla.org/r/224080/#review230250

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/rule/GeckoSessionTestRule.java:996
(Diff revision 1)
>      public void delegateDuringNextWait(final Object callback) {
>          mWaitScopeDelegates.delegate(callback);
>      }
> +
> +    public void synthesizeClick(int x, int y) {
> +        mSession.getPanZoomController().synthesizeNativeMouseEvent(MotionEvent.ACTION_POINTER_DOWN, x, y);

I made it public in another patch, I'll move it here instead.
Comment on attachment 8954920 [details]
Bug 1441982 - Add a WithDisplay annotation to GeckoSessionTestRule

https://reviewboard.mozilla.org/r/224078/#review230258

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/rule/GeckoSessionTestRule.java:83
(Diff revision 1)
>      /**
> +     * Specify the display size for the GeckoSession in device pixels
> +     */
> +    @Target({ElementType.METHOD, ElementType.TYPE})
> +    @Retention(RetentionPolicy.RUNTIME)
> +    public @interface WithDisplaySize {

Agreed, fixed.
Comment on attachment 8954922 [details]
Bug 1441982 - Only allow an unopened GeckoSession as a response to onNewSession()

https://reviewboard.mozilla.org/r/224082/#review230262

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java:162
(Diff revision 1)
> +                                }
> +
> +                                if (session.isOpen()) {
> +                                    throw new IllegalArgumentException("Must use a new, unopened GeckoSession instance");
>                                  }
>  

Apparently that line made it into a different patch, so I'll fix that.
Comment on attachment 8954923 [details]
Bug 1441982 - Don't wrap RuntimeException instances if they are already a RuntimeException

https://reviewboard.mozilla.org/r/224084/#review230274

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/rule/GeckoSessionTestRule.java:550
(Diff revision 1)
>                  mDisplaySize = new Point(displaySize.width(), displaySize.height());
>              }
>          }
>      }
>  
> +    private static RuntimeException createRuntimeException(Throwable e) {

Fixed
Comment on attachment 8954924 [details]
Bug 1441982 - Queue input events while waiting for APZ to attach

https://reviewboard.mozilla.org/r/224086/#review230284

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/gfx/NativePanZoomController.java:511
(Diff revision 1)
>              mPointerState.pointers.remove(pointerIndex);
>          }
>      }
>  
>      @WrapForJNI(calledFrom = "ui")
> -    private void synthesizeNativeTouchPoint(int pointerId, int eventType, int clientX,
> +    public void synthesizeNativeTouchPoint(int pointerId, int eventType, int clientX,

I think it could be ok, but this code is clearly meant for tests only. I'll duplicate it in the test rule.
Comment on attachment 8954921 [details]
Bug 1441982 - Add GeckoSessionTestRule.synthesizeTap()

https://reviewboard.mozilla.org/r/224080/#review230510

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/gfx/NativePanZoomController.java:462
(Diff revision 2)
>          }
>      }
>  
>      @WrapForJNI(calledFrom = "ui")
>      private void synthesizeNativeTouchPoint(int pointerId, int eventType, int clientX,
> -                                            int clientY, double pressure, int orientation) {
> +                                           int clientY, double pressure, int orientation) {

Unnecessary white space change
Attachment #8954921 - Flags: review?(nchen) → review+
Comment on attachment 8954922 [details]
Bug 1441982 - Only allow an unopened GeckoSession as a response to onNewSession()

https://reviewboard.mozilla.org/r/224082/#review230512

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java:160
(Diff revision 2)
> -                                    throw new IllegalArgumentException("Must use a new GeckoSession instance");
> +                                    callback.sendSuccess(null);
> +                                    return;
> +                                }
> +
> +                                if (session.isOpen()) {
> +                                    throw new IllegalArgumentException("Must use a new, unopened GeckoSession instance");

Maybe "Must use a closed session"? "new" is technically not necessary.
Attachment #8954922 - Flags: review?(nchen) → review+
Comment on attachment 8954922 [details]
Bug 1441982 - Only allow an unopened GeckoSession as a response to onNewSession()

https://reviewboard.mozilla.org/r/224082/#review230516

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java:163
(Diff revision 2)
> +
> +                                if (session.isOpen()) {
> +                                    throw new IllegalArgumentException("Must use a new, unopened GeckoSession instance");
>                                  }
>  
> -                                callback.sendSuccess(session != null ? session.getId() : null);
> +                                session.openWindow(null);

BTW, `createContentWindow` has a "don't set opener" flag. Are we respecting that?
Comment on attachment 8954924 [details]
Bug 1441982 - Queue input events while waiting for APZ to attach

https://reviewboard.mozilla.org/r/224086/#review230518

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/gfx/NativePanZoomController.java:36
(Diff revision 2)
>      private float mPointerScrollFactor = 64.0f;
>      private long mLastDownTime;
>  
>      private SynthesizedEventState mPointerState;
>  
> +    private ArrayDeque<Pair<Integer, MotionEvent>> mQueuedEvents;

Just `ArrayList` should work here.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/gfx/NativePanZoomController.java:57
(Diff revision 2)
>              int action, long time, int metaState,
>              float x, float y, int buttons);
>  
>      private boolean handleMotionEvent(MotionEvent event) {
>          if (!mAttached) {
> +            if (mQueuedEvents != null) {

The way it is now, `mQueuedEvents` is actually always != null when `!mAttached`

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/gfx/NativePanZoomController.java:281
(Diff revision 2)
>              mAttached = true;
> +            flushEventQueue();
>          } else if (mAttached) {
>              mAttached = false;
>              disposeNative();
> +            enableEventQueue();

Technically we should enable queuing between calling "Compositor.attachNPZC" and "NPZC.setAttached" being called, so we don't queue when the NPZC is totally disconnected (when the session is not open), but this is okay I guess.
Attachment #8954924 - Flags: review?(nchen) → review+
Comment on attachment 8956585 [details]
Bug 1441982 - Don't set window opener if nsIBrowserDOMWindow.OPEN_NO_OPENER present

https://reviewboard.mozilla.org/r/225496/#review231424
Attachment #8956585 - Flags: review?(nchen) → review+
Pushed by jwillcox@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a6f6866c736
Add a WithDisplay annotation to GeckoSessionTestRule r=jchen
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c570aa49dfb
Add GeckoSessionTestRule.synthesizeTap() r=jchen
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf7689be97bc
Only allow an unopened GeckoSession as a response to onNewSession() r=jchen
https://hg.mozilla.org/integration/mozilla-inbound/rev/b71f6a4e18cb
Don't wrap RuntimeException instances if they are already a RuntimeException r=jchen
https://hg.mozilla.org/integration/mozilla-inbound/rev/64a19db957b2
Queue input events while waiting for APZ to attach r=jchen
https://hg.mozilla.org/integration/mozilla-inbound/rev/f8bc3fd98796
Add a test for GeckoSession.NavigationListener.onNewSession() r=jchen
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e915e03ab02
Remove old busted GeckoView NavigationTests r=jchen
https://hg.mozilla.org/integration/mozilla-inbound/rev/63a48ae95dd8
Don't set window opener if nsIBrowserDOMWindow.OPEN_NO_OPENER present r=jchen
Assignee: nobody → snorp
Product: Firefox for Android → GeckoView
Target Milestone: Firefox 60 → mozilla60
You need to log in before you can comment on or make changes to this bug.