1474454 - Don't use GeckoResponse for return values in delegate methods

Status: RESOLVED FIXED

(GeckoView :: General, defect, P1)

Description

[James Willcox (:snorp) ([e-mail]) (he/him)]

We should use GeckoResult instead.

Comment 1

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Bug 1474454 - Use GeckoResult in GeckoSession.NavigationDelegate.onLoadRequest()

Review commit: https://reviewboard.mozilla.org/r/255860/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/255860/

Comment 2

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Bug 1474454 - Use GeckoResult in GeckoSession.NavigationDelegate.onNewSession()

Review commit: https://reviewboard.mozilla.org/r/255862/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/255862/

Comment 3

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8990843 [details]
Bug 1474454 - Use GeckoResult in GeckoSession.NavigationDelegate.onLoadRequest()

https://reviewboard.mozilla.org/r/255860/#review262846

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/ContentDelegateTest.kt:22
(Diff revision 1)
>  import android.support.test.runner.AndroidJUnit4
>  import org.hamcrest.Matchers.*
>  import org.junit.Assume.assumeThat
>  import org.junit.Test
>  import org.junit.runner.RunWith
> +import org.mozilla.geckoview.GeckoResult

Move to above

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/ContentDelegateTest.kt:48
(Diff revision 1)
>          sessionRule.waitUntilCalled(object : Callbacks.NavigationDelegate, Callbacks.ContentDelegate {
>  
>              @AssertCalled(count = 2)
>              override fun onLoadRequest(session: GeckoSession, uri: String,
> -                                       where: Int, flags: Int,
> -                                       response: GeckoResponse<Boolean>) {
> +                                       where: Int, flags: Int): GeckoResult<Boolean> {
> +                return GeckoResult.fromValue(false)

Should return null

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/NavigationDelegateTest.kt:23
(Diff revision 1)
>  import android.support.test.filters.MediumTest
>  import android.support.test.runner.AndroidJUnit4
>  import org.hamcrest.Matchers.*
>  import org.junit.Test
>  import org.junit.runner.RunWith
> +import org.mozilla.geckoview.GeckoResult

Move to above

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/NavigationDelegateTest.kt:112
(Diff revision 1)
>                  assertThat("URI should not be null", uri, notNullValue())
>                  assertThat("URI should match", uri, endsWith(HELLO_HTML_PATH))
>                  assertThat("Where should not be null", where, notNullValue())
>                  assertThat("Where should match", where,
>                             equalTo(GeckoSession.NavigationDelegate.TARGET_WINDOW_CURRENT))
> -                response.respond(false)
> +                return GeckoResult.fromValue(false)

Should return null

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/NavigationDelegateTest.kt:302
(Diff revision 1)
> -                                       flags: Int,
> -                                       response: GeckoResponse<Boolean>) {
>                  assertThat("URI should match", uri, endsWith(HELLO_HTML_PATH))
>                  assertThat("Where should match", where,
>                             equalTo(GeckoSession.NavigationDelegate.TARGET_WINDOW_CURRENT))
> -                response.respond(false)
> +                return GeckoResult.fromValue(false)

Should return null

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/NavigationDelegateTest.kt:351
(Diff revision 1)
> -                                       flags: Int,
> -                                       response: GeckoResponse<Boolean>) {
>                  assertThat("URI should match", uri, endsWith(HELLO_HTML_PATH))
>                  assertThat("Where should match", where,
>                             equalTo(GeckoSession.NavigationDelegate.TARGET_WINDOW_CURRENT))
> -                response.respond(false)
> +                return GeckoResult.fromValue(false)

Should return null

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/NavigationDelegateTest.kt:385
(Diff revision 1)
> -                                       flags: Int,
> -                                       response: GeckoResponse<Boolean>) {
>                  assertThat("URI should match", uri, endsWith(HELLO2_HTML_PATH))
>                  assertThat("Where should match", where,
>                             equalTo(GeckoSession.NavigationDelegate.TARGET_WINDOW_CURRENT))
> -                response.respond(false)
> +                return GeckoResult.fromValue(false)

Should return null

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/NavigationDelegateTest.kt:453
(Diff revision 1)
> -            override fun onLoadRequest(session: GeckoSession, uri: String, where: Int, flags: Int, response: GeckoResponse<Boolean>) {
> +            override fun onLoadRequest(session: GeckoSession, uri: String,
> +                                       where: Int, flags: Int): GeckoResult<Boolean> {
>                  assertThat("URI should be correct", uri, endsWith(NEW_SESSION_CHILD_HTML_PATH))
>                  assertThat("Where should be correct", where,
>                             equalTo(GeckoSession.NavigationDelegate.TARGET_WINDOW_NEW))
> +                return GeckoResult.fromValue(false)

Should return null

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/NavigationDelegateTest.kt:482
(Diff revision 1)
> -            override fun onLoadRequest(session: GeckoSession, uri: String, where: Int, flags: Int, response: GeckoResponse<Boolean>) {
> +            override fun onLoadRequest(session: GeckoSession, uri: String,
> +                                       where: Int, flags: Int): GeckoResult<Boolean> {
>                  assertThat("URI should be correct", uri, endsWith(NEW_SESSION_CHILD_HTML_PATH))
>                  assertThat("Where should be correct", where,
>                             equalTo(GeckoSession.NavigationDelegate.TARGET_WINDOW_NEW))
> +                return GeckoResult.fromValue(false)

Should return null

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/NavigationDelegateTest.kt:591
(Diff revision 1)
>              @AssertCalled(count = 2)
> -            override fun onLoadRequest(session: GeckoSession, uri: String, where: Int, flags: Int, response: GeckoResponse<Boolean>) {
> +            override fun onLoadRequest(session: GeckoSession, uri: String,
> +                                       where: Int, flags: Int): GeckoResult<Boolean> {
>                  assertThat("URI must match", uri,
>                             endsWith(forEachCall(NEW_SESSION_CHILD_HTML_PATH, NEW_SESSION_HTML_PATH)))
> +                return GeckoResult.fromValue(false)

Should return null

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/ProgressDelegateTest.kt:21
(Diff revision 1)
>  import org.hamcrest.Matchers.*
>  import org.junit.Assume.assumeThat
>  import org.junit.Ignore
>  import org.junit.Test
>  import org.junit.runner.RunWith
> +import org.mozilla.geckoview.GeckoResult

Move to above

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/ProgressDelegateTest.kt:72
(Diff revision 1)
>                  if (sessionRule.currentCall.counter == 1) {
>                      assertThat("URI should be " + testUri, uri, equalTo(testUri));
>                  } else {
>                      assertThat("URI should be about:neterror", uri, startsWith("about:neterror"));
>                  }
> -                response.respond(false)
> +                return GeckoResult.fromValue(false)

Should return null

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/util/Callbacks.kt:14
(Diff revision 1)
>  import org.mozilla.geckoview.GeckoSession
>  
>  import android.view.inputmethod.CursorAnchorInfo
>  import android.view.inputmethod.ExtractedText
>  import android.view.inputmethod.ExtractedTextRequest
> +import org.mozilla.geckoview.GeckoResult

Move to above

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java:2165
(Diff revision 1)
> -         * @param response A response which will state whether or not the load
> -         *                 was handled. If unhandled, Gecko will continue the
> -         *                 load as normal.
> +         *
> +         * @return A {@link GeckoResult} with a boolean value which indicates whether or
> +         *         not the load was handled. If unhandled, Gecko will continue the
> +         *         load as normal. May be null.
>           */
> -        void onLoadRequest(GeckoSession session, String uri,
> +        GeckoResult<Boolean> onLoadRequest(GeckoSession session, String uri,

`@Nullable`

Comment 4

[Dylan Roeh (:droeh) (he/him)]

Comment on attachment 8990843 [details]
Bug 1474454 - Use GeckoResult in GeckoSession.NavigationDelegate.onLoadRequest()

https://reviewboard.mozilla.org/r/255860/#review262906

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java:2163
(Diff revision 1)
>           *              One or more of {@link #LOAD_REQUEST_IS_USER_TRIGGERED
>           *              LOAD_REQUEST_*}.
> -         * @param response A response which will state whether or not the load
> -         *                 was handled. If unhandled, Gecko will continue the
> -         *                 load as normal.
> +         *
> +         * @return A {@link GeckoResult} with a boolean value which indicates whether or
> +         *         not the load was handled. If unhandled, Gecko will continue the
> +         *         load as normal. May be null.

I think this comment should explain what a null return means and when/if an app should use it -- it doesn't at all seem clear to me.

Comment 5

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8990844 [details]
Bug 1474454 - Use GeckoResult in GeckoSession.NavigationDelegate.onNewSession()

https://reviewboard.mozilla.org/r/255862/#review262904

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/NavigationDelegateTest.kt:606
(Diff revision 1)
>          })
>      }
>  
>      @WithDevToolsAPI
>      @Test(expected = IllegalArgumentException::class)
> +    @Ignore // FIXME: Bug 1474450

Comment out the `(expected = IllegalArgumentException::class)` part instead of using `@Ignore`, so the test fails when that bug is fixed.

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/rule/GeckoSessionTestRule.java:1189
(Diff revision 1)
>  
> -                if (call != null && sOnNewSession.equals(method)) {
> +                Object returnValue = null;
> +                try {
> +                    mCurrentMethodCall = call;
> +                    returnValue = method.invoke((call != null) ? call.target
> +                                                        : Callbacks.Default.INSTANCE, args);

Alignment

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/rule/GeckoSessionTestRule.java:1196
(Diff revision 1)
> +                    throw unwrapRuntimeException(e);
> +                } finally {
> +                    mCurrentMethodCall = null;
> +                }
> +
> +                if (call != null && returnValue != null && sOnNewSession.equals(method)) {

I would turn this into an early return.

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/rule/GeckoSessionTestRule.java:1199
(Diff revision 1)
> +                }
> +
> +                if (call != null && returnValue != null && sOnNewSession.equals(method)) {
>                      // We're delegating an onNewSession call.
>                      // Make sure we wait on the newly opened session, if any.
> +                    @SuppressWarnings("unchecked")

Not needed

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/rule/GeckoSessionTestRule.java:1204
(Diff revision 1)
> +                    @SuppressWarnings("unchecked")
>                      final GeckoSession oldSession = (GeckoSession) args[0];
> +
>                      @SuppressWarnings("unchecked")
> -                    final GeckoResponse<GeckoSession> realResponse =
> -                            (GeckoResponse<GeckoSession>) args[2];
> +                    final GeckoResult<GeckoSession> result = (GeckoResult<GeckoSession>)returnValue;
> +                    if (oldSession.isOpen() && result != null) {

Not needed. You already checked `result != null` above and you should check `oldSession.isOpen` right before `waitForOpenSession` like before.

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/rule/GeckoSessionTestRule.java:1205
(Diff revision 1)
>                      final GeckoSession oldSession = (GeckoSession) args[0];
> +
>                      @SuppressWarnings("unchecked")
> -                    final GeckoResponse<GeckoSession> realResponse =
> -                            (GeckoResponse<GeckoSession>) args[2];
> -                    args[2] = new GeckoResponse<GeckoSession>() {
> +                    final GeckoResult<GeckoSession> result = (GeckoResult<GeckoSession>)returnValue;
> +                    if (oldSession.isOpen() && result != null) {
> +                        final GeckoResult<GeckoSession> tmpResult = new GeckoResult<>();

I think the `GeckoResult` copy constructor should basically call `completeFrom(other)`, and you can use that to simplify things here. The current copy constructor doesn't make much sense.

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/rule/GeckoSessionTestRule.java:1218
(Diff revision 1)
> +                                // open here.
> +                                tmpResult.then(new OnValueListener<GeckoSession, Void>() {
> -                        @Override
> +                                    @Override
> -                        public void respond(final GeckoSession newSession) {
> -                            realResponse.respond(newSession);
> -                            // `realResponse` has opened the session at this point, so wait on it.
> +                                    public GeckoResult<Void> onValue(GeckoSession newSession) throws Throwable {
> +                                        if (newSession != null) {
> +                                            waitForOpenSession(newSession);

Please keep `GeckoSessionTestRule.this.`

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/rule/GeckoSessionTestRule.java:1225
(Diff revision 1)
> +                        }, new OnExceptionListener<Void>() {
> +                            @Override
> +                            public GeckoResult<Void> onException(Throwable exception) throws Throwable {
> +                                tmpResult.completeExceptionally(exception);
> +                                return null;
> -                }
> +                            }
> +                        });

I wouldn't bother with this.

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java:2192
(Diff revision 1)
>          * @param session The GeckoSession that initiated the callback.
>          * @param uri The URI to be loaded.
>          *
> -        * @param response A Response which will hold the returned GeckoSession
> +        * @return A {@link GeckoResult} which holds the returned GeckoSession. May be null.
>          */
> -        void onNewSession(GeckoSession session, String uri, GeckoResponse<GeckoSession> response);
> +        GeckoResult<GeckoSession> onNewSession(GeckoSession session, String uri);

`@Nullable`, and `@NonNull` for the args for that matter.

Comment 6

[Dylan Roeh (:droeh) (he/him)]

Comment on attachment 8990844 [details]
Bug 1474454 - Use GeckoResult in GeckoSession.NavigationDelegate.onNewSession()

https://reviewboard.mozilla.org/r/255862/#review262920

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java:2190
(Diff revision 1)
>          * returned GeckoSession must be a newly-created one.
>          *
>          * @param session The GeckoSession that initiated the callback.
>          * @param uri The URI to be loaded.
>          *
> -        * @param response A Response which will hold the returned GeckoSession
> +        * @return A {@link GeckoResult} which holds the returned GeckoSession. May be null.

Same issue as with the onLoadRequest patch; what does returning null from onNewSession mean?

Comment 7

[Chris Peterson [:cpeterson]]

James says he will land this patch today. We should uplift to 62 beta.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ebb68824d93
Use GeckoResult in GeckoSession.NavigationDelegate.onLoadRequest() r=jchen,droeh
https://hg.mozilla.org/integration/mozilla-inbound/rev/714b1d874ec9
Use GeckoResult in GeckoSession.NavigationDelegate.onNewSession() r=jchen,droeh

Comment 9

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/3ebb68824d93
https://hg.mozilla.org/mozilla-central/rev/714b1d874ec9

Comment 10

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8990843 [details]
Bug 1474454 - Use GeckoResult in GeckoSession.NavigationDelegate.onLoadRequest()

Approval Request Comment
[Feature/Bug causing the regression]: GeckoView
[User impact if declined]: Crappy API?
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: Both patches in the bug
[Is the change risky?]: No
[Why is the change risky/not risky?]: Only affects GeckoView
[String changes made/needed]: None

Comment 11

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8990843 [details]
Bug 1474454 - Use GeckoResult in GeckoSession.NavigationDelegate.onLoadRequest()

Only affects Geckoview, let's uplift to beta.

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/f7122adfbe8b
https://hg.mozilla.org/releases/mozilla-beta/rev/4137abca621d