1510314 - Allow setting closed (never opened) sessions on GeckoView

Status: RESOLVED FIXED

(GeckoView :: General, enhancement, P1)

Description

[Christian Sadilek [:csadilek]]

The onNewSession callback in NavigationDelegate is expected to return a newly created (closed and never opened) GeckoSession.

There is currently no way for an application to know when this new GeckoSession is ready to use, and setting it on a GeckoView will cause an IllegalArgumentException.

Allowing closed/never opened sessions to be set would make this much easier, as calls on GeckoSession are already queued up. If that's not possible, some form of notification/callback to indicate that the session is ready to use would also work.

This affects Android components as we can't (efficiently) implement support for opening new windows. One known workaround is to trigger an intent/activity (similar to the GV sample) app. Once this new activity is started, the new GeckoSession is opened and ready to use.

Comment 1

[David Bolter [:davidb] (NeedInfo me for attention)]

OK James thinks this might affect GV API...

Comment 2

[Chris Peterson [:cpeterson]]

Christian, is this feature needed for Fenix MVP? It fell off the GV radar.

"Add support for window requests to engine-gecko" AC issue: mozilla-mobile/android-components#1503

Comment 3

[Christian Sadilek [:csadilek]]

Spoke to Vesta. We need to support target="_blank" for MVP, yes. Maybe there's an alternative to the onNewSession callback or a better workaround we could use for the issue described above?

Comment 4

[Chris Peterson [:cpeterson]]

(In reply to Christian Sadilek [:csadilek] from comment #3)

Spoke to Vesta. We need to support target="_blank" for MVP, yes. Maybe there's an alternative to the onNewSession callback or a better workaround we could use for the issue described above?

Adding [geckoview:fenix:m6] whiteboard tag since we want to fix this issue for Fenix MVP.

Comment 6

[Chris Peterson [:cpeterson]]

Reassigning to James because Agi is going on PTO.

Comment 7

[Chris Peterson [:cpeterson]]

Fenix needs this for opening new tabs.

James says he'll post a patch today or we can reassign to another engineer.

Comment 8

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

Attachment: Bug 1510314 - Don't manage GeckoSession opening/closing in GeckoView class

This changes a few things:

1. Apps can now set a closed session on the GeckoView. This is
   convenient due to how GeckoSession.ContentDelegate.onNewSession()
   behaves.

2. The lifetime of GeckoSession is normally managed by apps anyway, and
   GeckoView's meddling here is just getting in the way. This change makes
   it clear who is responsible for ensuring sessions are opened (or
   closed) -- the app.

3. The GeckoView class will no longer do anything tricky when restoring
   the view state (as in an Activity recreation due to config change). The
   normal rules for setSession() apply. The main consequence of this is
   that if we restore state for an instance that already has an open
   session, this will now throw, whereas previously this would transfer
   the underlying window from the saved session (if it was open) to the
   current session.

Comment 9

[Krzysztof Jan Modras [:chrmod]]

please take a look on this onNewSession implementation I propose for geckoview example in https://phabricator.services.mozilla.com/D32796

        @Override
        public GeckoResult<GeckoSession> onNewSession(final GeckoSession session, final String uri) {
            TabSession newSession = createSession();

            GeckoSession.NavigationDelegate navigationDelegate = newSession.getNavigationDelegate();

            newSession.setNavigationDelegate(new GeckoSession.NavigationDelegate() {
                @Override
                public void onLocationChange(GeckoSession session, final String url) {
                    // first onLocationChange indicates that session is now open
                    newSession.setNavigationDelegate(navigationDelegate);

                    newSession.loadUri(uri);

                    mToolbarView.updateTabCount();
                    mTabSessionManager.setCurrentSession(newSession);

                    mGeckoView.releaseSession();
                    mGeckoView.setSession(newSession);
                }
            });

            return GeckoResult.fromValue(newSession);
        }

trick is to set a tempral NavigationDelegate that sets the GeckoSession state correctly and then gets replaced with browser implementation of NavigationDelegate.

Comment 10

[Chris Peterson [:cpeterson]]

Fenix MVP is going to release with or without this fix, so it's not technically a Fenix MVP blocker. I'll move this bug to the next Fenix milestone (M7).

Comment 11

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

Attachment: Bug 1510314 - Add some initial basic tests for the GeckoView class

Comment 12

[Chris Peterson [:cpeterson]]

Deferring this bug from Fenix's M7 (July) milestone to the M8 backlog for later in Q3.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b94fa94bfea9
Don't manage GeckoSession opening/closing in GeckoView class r=geckoview-reviewers,esawin,agi
https://hg.mozilla.org/integration/autoland/rev/cbcc723e1d6f
Add some initial basic tests for the GeckoView class r=geckoview-reviewers,esawin

Comment 14

[Chris Peterson [:cpeterson]]

68=affected because the Fenix team is asking us to uplift this fix to a GV 68.0.x dot release. An uplift decision is still pending.

Comment 15

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/b94fa94bfea9
https://hg.mozilla.org/mozilla-central/rev/cbcc723e1d6f

Comment 16

[Chris Peterson [:cpeterson]]

@ Christian, do you have STR to reproduce this bug? Can you verify if James' GV fix fixed the problem in R-B?

If so, then we can uplift to GV 69 Beta for Fenix. James isn't comfortable uplifting this somewhat risky fix to a GV 68.0.x dot release.

Comment 17

[Christian Sadilek [:csadilek]]

:cpeterson OK, I need to land some code for this in A-C first. I will work on it so we can test in R-B on Monday and let you know.

Comment 18

[Christian Sadilek [:csadilek]]

:cpeterson I've landed the changes in A-C /R-B now and verified that :snorp's fix works as expected. Latest R-B now opens target="_blank" links in a new tab. Thanks!

Comment 19

[Chris Peterson [:cpeterson]]

firefox69=wontfix because this fix can ride with 70.

Comment 20

[Chris Peterson [:cpeterson]]

(In reply to Chris Peterson [:cpeterson] from comment #16)

@ Christian, do you have STR to reproduce this bug? Can you verify if James' GV fix fixed the problem in R-B?

If so, then we can uplift to GV 69 Beta for Fenix. James isn't comfortable uplifting this somewhat risky fix to a GV 68.0.x dot release.

James, can we uplift your fix to GV 69 Beta? You said you weren't comfortable uplifting to a GV 68.0.x dot release, but GV 69 Beta might be OK if Christian tested your fix, which he did in comment 18:

(In reply to Christian Sadilek [:csadilek] from comment #18)

:cpeterson I've landed the changes in A-C /R-B now and verified that :snorp's fix works as expected. Latest R-B now opens target="_blank" links in a new tab. Thanks!

Comment 21

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

(In reply to Chris Peterson [:cpeterson] from comment #20)

(In reply to Chris Peterson [:cpeterson] from comment #16)

@ Christian, do you have STR to reproduce this bug? Can you verify if James' GV fix fixed the problem in R-B?

If so, then we can uplift to GV 69 Beta for Fenix. James isn't comfortable uplifting this somewhat risky fix to a GV 68.0.x dot release.

James, can we uplift your fix to GV 69 Beta? You said you weren't comfortable uplifting to a GV 68.0.x dot release, but GV 69 Beta might be OK if Christian tested your fix, which he did in comment 18:

(In reply to Christian Sadilek [:csadilek] from comment #18)

:cpeterson I've landed the changes in A-C /R-B now and verified that :snorp's fix works as expected. Latest R-B now opens target="_blank" links in a new tab. Thanks!

Yeah, that's fine.

Comment 22

[Chris Peterson [:cpeterson]]

Comment on attachment 9070682 [details]
Bug 1510314 - Don't manage GeckoSession opening/closing in GeckoView class

Beta/Release Uplift Approval Request

• User impact if declined: Without uplifting this fix to GV 69 Beta, Fenix 69 won't be able to open new tabs for target=_blank links, which breaks some websites that depend on opening new tabs. See the Fenix issue for more details: https://github.com/mozilla-mobile/fenix/issues/1486
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): These patches only touch GV Android code. There is no risk to desktop Firefox or Fennec. csadilek from the A-C team manually verified the fix using R-B and GV 70 Nightly.
• String changes made/needed: None

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9070682 [details]
Bug 1510314 - Don't manage GeckoSession opening/closing in GeckoView class

Adds the ability for Fenix to open target=_blank links, which is breaking websites otherwise. Approved for GV69.

Comment 24

[Bogdan Tara[:bogdan_tara | bogdant]]

I tried to uplift this to beta, but got conflicts:
grafting 555852:b94fa94bfea9 "Bug 1510314 - Don't manage GeckoSession opening/closing in GeckoView class r=geckoview-reviewers,esawin,agi"
merging mobile/android/geckoview/api.txt
merging mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/SessionLifecycleTest.kt
merging mobile/android/geckoview/src/main/java/org/mozilla/geckoview/doc-files/CHANGELOG.md
merging mobile/android/geckoview_example/src/main/java/org/mozilla/geckoview_example/GeckoViewActivity.java
warning: conflicts while merging mobile/android/geckoview/src/main/java/org/mozilla/geckoview/doc-files/CHANGELOG.md! (edit, then use 'hg resolve --mark')

Comment 25

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

Ugh. 70 will be in beta soon enough, let's just let this ride.