Introduce Promise-like type for return values

RESOLVED FIXED in Firefox 62

Status

defect
P2
normal
RESOLVED FIXED
a year ago
5 months ago

People

(Reporter: snorp, Assigned: snorp)

Tracking

59 Branch
mozilla63
Unspecified
Android
Dependency tree / graph

Firefox Tracking Flags

(firefox61 unaffected, firefox62 fixed, firefox63 fixed)

Details

Attachments

(5 attachments)

Currently we use GeckoResponse as a sort of out param for things that need asynchronous return values. An example of this is GeckoSession.saveState() and GeckoSession.NavigationDelegate.onLoadRequest(). It's pretty clunky.

One problem with this is that in the onLoadRequest() case it's not obvious to an implementer that it *really* needs to call respond() on the GeckoResponse parameter. There will be no build errors or warnings if the body of that function is empty. When called by Gecko, though, we will spin an inner loop forever waiting for a response. That's not very good.

Things are better with saveState(), but Promise-like return value instead of supplying a callback should still yield some ergonomic benefits. This is even more true if we consider that the Android Components and our apps are written using Kotlin, which have coroutine support. If we are able to integrate with that, apps will be able to do things like `val state = session.saveState().await()` which we know has been very helpful with async programming in the JS world.

Lastly, we could consider returning a Promise even from `void` methods. In the case of GeckoSession.loadUri(), the returned asynchronous result would be resolved when the loadUri() call was completed. I'm not sure if many apps will care about this, but it seems like the right thing to do.
Attachment #8984717 - Flags: feedback?(rbarker)
Attachment #8984717 - Flags: feedback?(nchen)
Attachment #8984717 - Flags: feedback?(esawin)
Attachment #8984717 - Flags: feedback?(droeh)
Attachment #8984718 - Flags: feedback?(rbarker)
Attachment #8984718 - Flags: feedback?(nchen)
Attachment #8984718 - Flags: feedback?(esawin)
Attachment #8984718 - Flags: feedback?(droeh)
The attached patches are definitely not ready for prime time, but I wanted to get folks' opinions before we go further.
Sebastian and Christian, your opinions on this would be helpful too!
Flags: needinfo?(s.kaspari)
Flags: needinfo?(csadilek)
This would live in GeckoView and could be used by Android-components? Probably better than depending on a third party library we would eventually end up forking anyway because gecko is "special".
Attachment #8984717 - Flags: feedback?(rbarker)
Attachment #8984718 - Flags: feedback?(rbarker)
Adding the summary of our discussion:

- The onResult and onError listeners should be invoked immediately if registered after the GeckoResponse completed or ended in an error
- GeckoResult doesn't need to be provided onResult / onError
- The getValue method can be removed as it provides no added value over the listeners
- Both saveState and restoreState should return a GeckoResult
Flags: needinfo?(csadilek)
I'm good with the idea (including the changes in Christian's summary); the only (minor) downside I can think of is that it stops us from having the sync and async saveState functions share the same name, as they'll now have the same parameters.
Attachment #8984717 - Flags: feedback?(droeh) → feedback+
Attachment #8984718 - Flags: feedback?(droeh) → feedback+
Assigning this bug to James because he's already working on it.
Assignee: nobody → snorp
OS: Unspecified → Android
Clearing NI. We talked about this in SF.
Flags: needinfo?(s.kaspari)
Comment on attachment 8984717 [details]
Bug 1468048 - Introduce GeckoResult

Talked at all-hands to 
1) have `complete`, `completeExceptionally`, and `then` methods only.
2) chain `then` method calls.
3) not deal with multi-threading.
Attachment #8984717 - Flags: feedback?(nchen) → feedback+
Attachment #8984718 - Flags: feedback?(nchen)
Priority: -- → P2
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 15

11 months ago
mozreview-review
Comment on attachment 8984717 [details]
Bug 1468048 - Introduce GeckoResult

https://reviewboard.mozilla.org/r/250560/#review260360

completeExceptionally sounds odd for something that essentially signals failure by exception and not proper completion of the requested result.

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoDeferred.java:35
(Diff revision 2)
> +    /**
> +     * Completes the {@link GeckoResult} held by this instance with the specified value.
> +     *
> +     * @param value The value used to complete our {@link GeckoResult} instance.
> +     */
> +    public void complete(T value) {

final?

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoDeferred.java:44
(Diff revision 2)
> +    /**
> +     * Completes the {@link GeckoResult} held by this instance with the specified {@link Exception}.
> +     *
> +     * @param exception The {@link Exception} used to complete our {@link GeckoResult} instance.
> +     */
> +    public void completeExceptionally(Exception exception) {

final?

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoResult.java:80
(Diff revision 2)
> +    public int hashCode() {
> +        int result = 17;
> +        result = 31 * result + (mComplete ? 1 : 0);
> +        result = 31 * result + (mValue != null ? mValue.hashCode() : 0);
> +        result = 31 * result + (mError != null ? mError.hashCode() : 0);
> +        return result;

Can we use Objects.hash() instead?

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoResult.java:84
(Diff revision 2)
> +        result = 31 * result + (mError != null ? mError.hashCode() : 0);
> +        return result;
> +    }
> +
> +    @Override
> +    public boolean equals(Object other) {

final?

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoResult.java:88
(Diff revision 2)
> +    @Override
> +    public boolean equals(Object other) {
> +        if (other instanceof GeckoResult<?>) {
> +            final GeckoResult<?> result = (GeckoResult<?>)other;
> +            return result.mComplete == mComplete &&
> +                    result.mError == mError &&

Indentation.

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoResult.java:99
(Diff revision 2)
> +
> +    /**
> +     * Convenience method for {@link #then(OnValueListener, OnExceptionListener)}.
> +     *
> +     * @param valueListener
> +     * @param <U>

Incomplete docs?

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoResult.java:110
(Diff revision 2)
> +
> +    /**
> +     * Convenience method for {@link #then(OnValueListener, OnExceptionListener)}.
> +     *
> +     * @param exceptionListener
> +     * @param <U>

Incomplete docs?

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoResult.java:123
(Diff revision 2)
> +     * Adds listeners to be called when the {@link GeckoResult} is completed either with
> +     * a value or {@link Exception}. Listeners will be invoked on the same thread in which the
> +     * {@link GeckoResult} was completed.
> +     *
> +     * @param valueListener An instance of {@link OnValueListener}, called when the
> +     *                       {@link GeckoResult} is completed with a value.

Indentation.

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoResult.java:125
(Diff revision 2)
> +     * {@link GeckoResult} was completed.
> +     *
> +     * @param valueListener An instance of {@link OnValueListener}, called when the
> +     *                       {@link GeckoResult} is completed with a value.
> +     * @param exceptionListener An instance of {@link OnExceptionListener}, called when the
> +     *                      {@link GeckoResult} is completed with an {@link Exception}.

Indentation.

Comment 16

11 months ago
mozreview-review
Comment on attachment 8984718 [details]
Bug 1468048 - Make GeckoSession.saveState() return a GeckoResult

https://reviewboard.mozilla.org/r/250562/#review260370
Attachment #8984718 - Flags: review?(esawin) → review+
Assignee

Comment 17

11 months ago
mozreview-review-reply
Comment on attachment 8984717 [details]
Bug 1468048 - Introduce GeckoResult

https://reviewboard.mozilla.org/r/250560/#review260360

I agree it's kinda weird, but it's the terminology used by CompletableFuture in Java 8. Seemed like the best way to go.

https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/CompletableFuture.html#completeExceptionally-java.lang.Throwable-

> Can we use Objects.hash() instead?

I wanted to, but we can't since that's only in API 19. The stuff I have here is apparently the cargo-culted way of solving it without Objects.hash().

https://developer.android.com/reference/java/util/Objects.html#hash(java.lang.Object...)

Comment 18

11 months ago
mozreview-review
Comment on attachment 8988317 [details]
Bug 1468048 - Add GeckoSessionTestRule.waitForResult()

https://reviewboard.mozilla.org/r/253570/#review260382
Attachment #8988317 - Flags: review?(esawin) → review+

Comment 19

11 months ago
mozreview-review
Comment on attachment 8984717 [details]
Bug 1468048 - Introduce GeckoResult

https://reviewboard.mozilla.org/r/250560/#review260426

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoResult.java:40
(Diff revision 2)
> +    }
> +
> +    /**
> +     * This constructs a result from another result. All state except listeners is copied.
> +     *
> +      * @param from The {@link GeckoResult} to copy.

Spacing on the comments looks off here.
Attachment #8984717 - Flags: review?(droeh) → review+

Comment 20

11 months ago
mozreview-review
Comment on attachment 8984718 [details]
Bug 1468048 - Make GeckoSession.saveState() return a GeckoResult

https://reviewboard.mozilla.org/r/250562/#review260430
Attachment #8984718 - Flags: review?(droeh) → review+

Comment 21

11 months ago
mozreview-review
Comment on attachment 8988318 [details]
Bug 1468048 - Add tests for GeckoSession.saveState() and restoreState()

https://reviewboard.mozilla.org/r/253572/#review260432
Attachment #8988318 - Flags: review?(droeh) → review+

Comment 22

11 months ago
mozreview-review
Comment on attachment 8984717 [details]
Bug 1468048 - Introduce GeckoResult

https://reviewboard.mozilla.org/r/250560/#review260634

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/GeckoResultTest.java:14
(Diff revision 2)
> +import static org.hamcrest.Matchers.equalTo;
> +import static org.junit.Assert.assertThat;
> +
> +
> +
> +public class GeckoResultTest {

This should be a unit test, i.e. under "mobile/android/geckoview/src/test"

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoDeferred.java:9
(Diff revision 2)
> + * This class represents a deferred or asynchronous operation. It contains a {@link GeckoResult}
> + * to represent the result of this operation.
> + *
> + * @param <T> The type of the result value
> + */
> +public class GeckoDeferred<T> {

If you follow the `CompletableFuture` paradigm I think `GeckoDeferred` would extend `GeckoResult` directly

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoResult.java:18
(Diff revision 2)
> +public class GeckoResult<T> {
> +    private static final String LOGTAG = "GeckoResult";
> +
> +    private boolean mComplete;
> +    private T mValue;
> +    private Exception mError;

Should be `Throwable` in

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoResult.java:88
(Diff revision 2)
> +    @Override
> +    public boolean equals(Object other) {
> +        if (other instanceof GeckoResult<?>) {
> +            final GeckoResult<?> result = (GeckoResult<?>)other;
> +            return result.mComplete == mComplete &&
> +                    result.mError == mError &&

Use `equals`

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoResult.java:141
(Diff revision 2)
> +            public void onValue(T value) {
> +                if (valueListener == null) {
> +                    return;
> +                }
> +
> +                result.completeFrom(valueListener.onValue(value));

`onValue`/`onException` exceptions should be caught and used to completed the promise

Comment 23

11 months ago
mozreview-review
Comment on attachment 8988317 [details]
Bug 1468048 - Add GeckoSessionTestRule.waitForResult()

https://reviewboard.mozilla.org/r/253570/#review260648

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/rule/GeckoSessionTestRule.java:2380
(Diff revision 1)
>                                                        @NonNull final T impl) {
>          addExternalDelegateDuringNextWait(JvmClassMappingKt.getJavaClass(delegate),
>                                            register, unregister, impl);
>      }
> +
> +    public <T> T waitForResult(GeckoResult<T> result) {

Javadoc and this should call `beforeWait`and `afterWait`

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/rule/GeckoSessionTestRule.java:2384
(Diff revision 1)
> +
> +    public <T> T waitForResult(GeckoResult<T> result) {
> +        final ResultHolder<T> holder = new ResultHolder<>(result);
> +
> +        while (!holder.isComplete) {
> +            loopUntilIdle(getScaledTimeoutMillis());

`loopUntilIdle(mTimeout)`
Assignee

Comment 24

11 months ago
mozreview-review-reply
Comment on attachment 8984717 [details]
Bug 1468048 - Introduce GeckoResult

https://reviewboard.mozilla.org/r/250560/#review260360

> Indentation.

This is actually the correct indentation according to the Android style guide (which is what Andriod Studio uses). https://source.android.com/setup/contribute/code-style#use-spaces-for-indentation
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 34

11 months ago
mozreview-review
Comment on attachment 8990108 [details]
Bug 1468048 - Factor out loopUntilIdle() from GeckoSessionTestRule into UiThreadUtils

https://reviewboard.mozilla.org/r/255108/#review261992

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/UiThreadUtils.java:1
(Diff revision 1)
> +package org.mozilla.geckoview.test;

Should be under `org.mozilla.geckoview.test.util`

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/UiThreadUtils.java:18
(Diff revision 1)
> +    private static Method sGetNextMessage = null;
> +    static {
> +        try {
> +            sGetNextMessage = MessageQueue.class.getDeclaredMethod("next");
> +            sGetNextMessage.setAccessible(true);
> +        } catch (NoSuchMethodException e) {

rethrow

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/UiThreadUtils.java:59
(Diff revision 1)
> +            HANDLER.sendMessageAtFrontOfQueue(msg);
> +            return false; // Remove this idle handler.
> +        }
> +    };
> +
> +    public static RuntimeException unwrapRuntimeException(final Throwable e) {

private

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/rule/GeckoSessionTestRule.java:51
(Diff revision 1)
>  import android.os.Process;
>  import android.os.SystemClock;
>  import android.support.annotation.NonNull;
>  import android.support.annotation.Nullable;
>  import android.support.test.InstrumentationRegistry;
> +import android.support.test.annotation.UiThreadTest;

Not used

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/rule/GeckoSessionTestRule.java:101
(Diff revision 1)
>      private static final long DEFAULT_X86_EMULATOR_TIMEOUT_MILLIS = 5000;
>      private static final long DEFAULT_IDE_DEBUG_TIMEOUT_MILLIS = 86400000;
>  
>      public static final String APK_URI_PREFIX = "resource://android/";
>  
>      private static final Method sGetNextMessage;

Remove this

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/rule/GeckoSessionTestRule.java
(Diff revision 1)
> -            final long waitDuration = SystemClock.uptimeMillis() - startTime;
> -            if (waitDuration > sLongestWait) {
> -                sLongestWait = waitDuration;
> -                Log.i(LOGTAG, "New longest wait: " + waitDuration + "ms");
> -            }

Please keep this block. It's used to make sure we have sane timeout values for automation.
Attachment #8990108 - Flags: review?(nchen) → review+

Comment 35

11 months ago
mozreview-review
Comment on attachment 8984717 [details]
Bug 1468048 - Introduce GeckoResult

https://reviewboard.mozilla.org/r/250560/#review261994

r+ with nits.

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/GeckoResultTest.java:7
(Diff revision 4)
> +import org.mozilla.geckoview.GeckoResult;
> +import org.mozilla.geckoview.GeckoResult;

Repeated

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/GeckoResultTest.java:26
(Diff revision 4)
> +    @Rule
> +    public UiThreadTestRule mUiThreadTestRule = new UiThreadTestRule();

This is not needed

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/GeckoResultTest.java:63
(Diff revision 4)
> +                done();
> +                return null;
> +            }
> +        });
> +
> +        assertThat("We should not be done", mDone, equalTo(false));

Move to inside `waitUntilDone`

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/GeckoResultTest.java:206
(Diff revision 4)
> +            @Override
> +            public GeckoResult<Float> onValue(Float value) {
> +                assertThat("Value should match", value, equalTo(42.0f));
> +                return GeckoResult.fromException(new Exception("boom"));
> +            }
> +        }).then(new OnExceptionListener<MockException>() {

Should be `OnExceptionListener<Void>`

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoResult.java:18
(Diff revision 4)
> +    private static Handler sUIHandler;
> +    private static synchronized Handler getUIHandler() {
> +        if (sUIHandler == null) {
> +            sUIHandler = new Handler(Looper.getMainLooper());
> +        }
> +
> +        return sUIHandler;
> +    }

Use `ThreadUtils.sUiHandler`

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoResult.java:38
(Diff revision 4)
> +    /**
> +     * This constructs an incomplete GeckoResult. Call {@link #complete(Object)} or
> +     * {@link #completeExceptionally(Throwable)} in order to fulfill the result.
> +     */
> +    public GeckoResult() {
> +        mListeners = new ArrayList<>();

Create the list lazily and make initial capacity 1.

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoResult.java:59
(Diff revision 4)
> +     * This constructs a result that is completed with the specified value.
> +     *
> +     * @param value The value used to complete the newly created result.
> +     * @return The completed {@link GeckoResult}
> +     */
> +    public static <U> GeckoResult<U> fromValue(final U value) {

`@NonNull` return and `@Nullable` arg

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoResult.java:72
(Diff revision 4)
> +     * May not be null.
> +     *
> +     * @param error The exception used to complete the newly created result.
> +     * @return The completed {@link GeckoResult}
> +     */
> +    public static <T> GeckoResult<T> fromException(@NonNull final Throwable error) {

`@NonNull` return

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoResult.java:79
(Diff revision 4)
> +        result.completeExceptionally(error);
> +        return result;
> +    }
> +
> +    @Override
> +    public int hashCode() {

`hashCode` and `equals` should be `synchronized` as well.

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoResult.java:112
(Diff revision 4)
> +     * @param valueListener An instance of {@link OnValueListener}, called when the
> +     *                      {@link GeckoResult} is completed with a value.
> +     * @param <U>
> +     * @return
> +     */
> +    public synchronized <U> GeckoResult<U> then(@NonNull final OnValueListener<T, U> valueListener) {

`@NonNull` return

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoResult.java:124
(Diff revision 4)
> +     * @param exceptionListener An instance of {@link OnExceptionListener}, called when the
> +     *                          {@link GeckoResult} is completed with an {@link Exception}.
> +     * @param <U> The type contained in the returned {@link GeckoResult}
> +     * @return
> +     */
> +    public synchronized <U> GeckoResult<U> then(@NonNull final OnExceptionListener<U> exceptionListener) {

`@NonNull` return

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoResult.java:140
(Diff revision 4)
> +     *                      {@link GeckoResult} is completed with a value.
> +     * @param exceptionListener An instance of {@link OnExceptionListener}, called when the
> +     *                          {@link GeckoResult} is completed with an {@link Throwable}.
> +     * @param <U> The type contained in the returned {@link GeckoResult}
> +     */
> +    public synchronized <U> GeckoResult<U> then(@Nullable final OnValueListener<T, U> valueListener,

`@NonNull` return

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoResult.java:156
(Diff revision 4)
> +                    if (valueListener != null && haveValue()) {
> +                        result.completeFrom(valueListener.onValue(mValue));
> +                    } else if (exceptionListener != null && haveError()) {
> +                        result.completeFrom(exceptionListener.onException(mError));
> +                    }
> +                } catch (Exception e) {

`Throwable`

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoResult.java:173
(Diff revision 4)
> +        }
> +
> +        return result;
> +    }
> +
> +    private synchronized void dispatch() {

Rename to `dispatchLocked` and drop `synchronized`.

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoResult.java:178
(Diff revision 4)
> +        final ArrayList<Runnable> listeners = new ArrayList<>(mListeners);
> +        mListeners.clear();

You don't need a new array because by-design `mListeners` cannot be modified after `dispatch()` is called.

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoResult.java:191
(Diff revision 4)
> +                }
> +            }
> +        });
> +    }
> +
> +    private synchronized void dispatch(final Runnable runnable) {

Rename to `dispatchLocked` and drop `synchronized`.

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoResult.java:284
(Diff revision 4)
> +         *
> +         * @param value The value of the {@link GeckoResult}
> +         * @return A new {@link GeckoResult}, used for chaining results together.
> +         *         May be null.
> +         */
> +        GeckoResult<U> onValue(T value);

`throws Throwable`

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoResult.java:293
(Diff revision 4)
> +     * An interface used to deliver exceptions to listeners of a {@link GeckoResult}
> +     *
> +     * @param <V> This is the type of the vale for the result returned from {@link #onException(Throwable)}
> +     */
> +    public interface OnExceptionListener<V> {
> +        GeckoResult<V> onException(Throwable exception);

`throws Throwable`
Attachment #8984717 - Flags: review?(nchen) → review+

Comment 36

11 months ago
mozreview-review
Comment on attachment 8988317 [details]
Bug 1468048 - Add GeckoSessionTestRule.waitForResult()

https://reviewboard.mozilla.org/r/253570/#review262014

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/GeckoSessionTestRuleTest.kt:11
(Diff revision 3)
>  
> +import android.os.Handler
> +import android.os.Looper
>  import org.mozilla.geckoview.GeckoSession
>  import org.mozilla.geckoview.GeckoSessionSettings
> -import org.mozilla.geckoview.test.rule.GeckoSessionTestRule.AssertCalled
> +import org.mozilla.geckoview.GeckoResult

Put before `import org.mozilla.geckoview.GeckoSession`

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/rule/GeckoSessionTestRule.java:2292
(Diff revision 3)
>                                                        @NonNull final T impl) {
>          addExternalDelegateDuringNextWait(JvmClassMappingKt.getJavaClass(delegate),
>                                            register, unregister, impl);
>      }
> +
> +    public <T> T waitForResult(GeckoResult<T> result) {

Add javadoc

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/rule/GeckoSessionTestRule.java:2302
(Diff revision 3)
> +
> +        while (!holder.isComplete) {
> +            UiThreadUtils.loopUntilIdle(mTimeoutMillis);
> +        }
> +
> +        afterWait(index);

Should be `afterWait(mCallRecords.size());`, and ideally this should be inside a `finally {}` block
Attachment #8988317 - Flags: review?(nchen) → review+

Comment 37

11 months ago
mozreview-review
Comment on attachment 8984718 [details]
Bug 1468048 - Make GeckoSession.saveState() return a GeckoResult

https://reviewboard.mozilla.org/r/250562/#review262018

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java:1152
(Diff revision 4)
>       * this does not include settings on the GeckoSession).
>       *
> -     * @param response This is a response which will be called with the state once it has been
> +     * @return A {@link GeckoResult} containing the {@link SessionState}
> -     *                 saved. Can be null if we fail to save the state for any reason.
>       */
> -    public void saveState(final GeckoResponse<SessionState> response) {
> +    public GeckoResult<SessionState> saveState() {

`@NonNull`

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java:3025
(Diff revision 4)
>           * @param info Cursor-anchor information.
>           */
>          void updateCursorAnchorInfo(@NonNull GeckoSession session, @NonNull CursorAnchorInfo info);
>      }
> +
> +    private static abstract class CallbackResult<T> extends GeckoResult<T> implements EventCallback {

Should be near top of this file, or inside `GeckoResult`

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java:3032
(Diff revision 4)
> +        @Override
> +        public abstract void sendSuccess(Object response);
> +
> +        @Override
> +        public void sendError(Object response) {
> +            completeExceptionally(new Exception(response.toString()));

`response` can be null
Attachment #8984718 - Flags: review+

Comment 38

11 months ago
mozreview-review
Comment on attachment 8988318 [details]
Bug 1468048 - Add tests for GeckoSession.saveState() and restoreState()

https://reviewboard.mozilla.org/r/253572/#review262020

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/BaseSessionTest.kt:38
(Diff revision 3)
>          const val INPUTS_PATH = "/assets/www/inputs.html"
>          const val INVALID_URI = "http://www.test.invalid/"
>          const val NEW_SESSION_HTML_PATH = "/assets/www/newSession.html"
>          const val NEW_SESSION_CHILD_HTML_PATH = "/assets/www/newSession_child.html"
> +        const val SAVE_STATE_PATH = "/assets/www/saveState.html"
> +        const val SELECTION_ACTION_PATH = "/assets/www/selectionAction.html"

Remove `SELECTION_ACTION_PATH`

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/ContentDelegateTest.kt:129
(Diff revision 3)
> +
> +    @WithDevToolsAPI
> +    @WithDisplay(width = 400, height = 400)
> +    @Test fun saveAndRestoreState() {
> +        val startUri = createTestUrl(SAVE_STATE_PATH)
> +        sessionRule.session.loadUri(startUri)

`mainSession` instead of `sessionRule.session`

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/ContentDelegateTest.kt:132
(Diff revision 3)
> +    @Test fun saveAndRestoreState() {
> +        val startUri = createTestUrl(SAVE_STATE_PATH)
> +        sessionRule.session.loadUri(startUri)
> +        sessionRule.waitForPageStop()
> +
> +        sessionRule.session.evaluateJS("document.getElementById('name').value = 'the name'; window.scrollBy(0, 100);")

`$('#name')` instead of `document.getElementById('name')`
Attachment #8988318 - Flags: review?(nchen) → review+
Assignee

Comment 39

11 months ago
mozreview-review-reply
Comment on attachment 8984717 [details]
Bug 1468048 - Introduce GeckoResult

https://reviewboard.mozilla.org/r/250560/#review261994

> This is not needed

I didn't think I needed it either, but without it @UiThreadTest has no affect. I guess we need a newer support lib?
Assignee

Comment 40

11 months ago
mozreview-review-reply
Comment on attachment 8984717 [details]
Bug 1468048 - Introduce GeckoResult

https://reviewboard.mozilla.org/r/250560/#review260634

> This should be a unit test, i.e. under "mobile/android/geckoview/src/test"

Indeed. I had it here because it was using Handler/Looper before which roboelectric somehow didn't seem to support.

> If you follow the `CompletableFuture` paradigm I think `GeckoDeferred` would extend `GeckoResult` directly

I'm fine with this, but I think the only advantage is that you can `return deferred` rather than `return deferred.getResult()`. This would also make GeckoDeferred's only responsibility to make `complete()` and `completeExceptionlly()` public. Maybe we should just make them public in `GeckoResult` and kill `GeckoDeferred`?

> `onValue`/`onException` exceptions should be caught and used to completed the promise

I thought about this while writing it but I'm not convinced it's really what we want.

Consider GeckoSession.saveState(). If we break this in some unexpected way, apps will get failed results, but we won't get any telemetry indicating that this is happening (unless we specifically add it). I think the safer thing may be to have callbacks catch any exceptions they expect, and unhandled exceptions are treated as crashes, as we do elsewhere. IMHO this seems closer to the Java approach to exceptions, while JS does what you describe.

Comment 41

11 months ago
Pushed by jwillcox@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8876dca12853
Factor out loopUntilIdle() from GeckoSessionTestRule into UiThreadUtils r=jchen
https://hg.mozilla.org/integration/mozilla-inbound/rev/78730bb03d1a
Introduce GeckoResult r=droeh,jchen
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ff19c6d6085
Add GeckoSessionTestRule.waitForResult() r=esawin,jchen
https://hg.mozilla.org/integration/mozilla-inbound/rev/39576d99b3d6
Make GeckoSession.saveState() return a GeckoResult r=droeh,esawin
https://hg.mozilla.org/integration/mozilla-inbound/rev/a6523e5984c5
Add tests for GeckoSession.saveState() and restoreState() r=droeh,jchen
Comment on attachment 8984717 [details]
Bug 1468048 - Introduce GeckoResult

Approval Request Comment
[Feature/Bug causing the regression]: GeckoView
[User impact if declined]: Crappy API in GeckoView
[Is this code covered by automated tests?]: Yes, gv-junit
[Has the fix been verified in Nightly?]: No
[Needs manual test from QE? If yes, steps to reproduce]: No 
[List of other uplifts needed for the feature/fix]: All patches in this bug
[Is the change risky?]: No
[Why is the change risky/not risky?]: Only affects GV
[String changes made/needed]: None
Attachment #8984717 - Flags: approval-mozilla-beta?
Comment on attachment 8984717 [details]
Bug 1468048 - Introduce GeckoResult

Improvement for geckoview only, let's uplift. This should end up in beta 9 (I just said "beta 8" on similar uplifts but actually, have already gone to build with beta 8 today -- so all these geckoview changes will be in beta 9 next week)
Attachment #8984717 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Updated

5 months ago
Product: Firefox for Android → GeckoView
Version: Firefox 59 → 59 Branch
Target Milestone: Firefox 63 → mozilla63
You need to log in before you can comment on or make changes to this bug.