Closed Bug 1510314 Opened Last year Closed 4 months ago

Allow setting closed (never opened) sessions on GeckoView

Categories

(GeckoView :: General, enhancement, P1)

Unspecified
Android
enhancement

Tracking

(geckoview66 wontfix, firefox-esr60 wontfix, firefox-esr68 wontfix, firefox65 wontfix, firefox66 wontfix, firefox67 wontfix, firefox67.0.1 wontfix, firefox68 wontfix, firefox69 wontfix, firefox70 fixed)

RESOLVED FIXED
mozilla70
Tracking Status
geckoview66 --- wontfix
firefox-esr60 --- wontfix
firefox-esr68 --- wontfix
firefox65 --- wontfix
firefox66 --- wontfix
firefox67 --- wontfix
firefox67.0.1 --- wontfix
firefox68 --- wontfix
firefox69 --- wontfix
firefox70 --- fixed

People

(Reporter: csadilek, Assigned: snorp)

References

Details

(Whiteboard: [geckoview:fenix:m8])

Attachments

(2 files)

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.
OK James thinks this might affect GV API...
Priority: -- → P2
Product: Firefox for Android → GeckoView
See Also: → 1547549

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

Flags: needinfo?(csadilek)
OS: Unspecified → Android
Whiteboard: [geckoview:fenix]

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?

Flags: needinfo?(csadilek)
Assignee: nobody → agi

(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.

Whiteboard: [geckoview:fenix] → [geckoview:fenix:m6]
Duplicate of this bug: 1547549

Reassigning to James because Agi is going on PTO.

Assignee: agi → snorp

Fenix needs this for opening new tabs.

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

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.

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.

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).

Whiteboard: [geckoview:fenix:m6] → [geckoview:fenix:m7]
Attachment #9070682 - Attachment description: Bug 1510314 - Don't manage GeckoSession opening/closing in GeckoView → Bug 1510314 - Don't manage GeckoSession opening/closing in GeckoView class

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

Whiteboard: [geckoview:fenix:m7] → [geckoview:fenix:m8]
Pushed by jwillcox@mozilla.com:
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

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.

Status: NEW → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla70

@ 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.

Flags: needinfo?(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.

: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!

Flags: needinfo?(csadilek)

firefox69=wontfix because this fix can ride with 70.

(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!

Flags: needinfo?(snorp)

(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.

Flags: needinfo?(snorp)
Regressions: 1572392

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
Attachment #9070682 - Flags: approval-mozilla-beta?
Attachment #9074358 - Flags: approval-mozilla-beta?
Flags: in-testsuite+

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.

Attachment #9070682 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9074358 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

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')

Flags: needinfo?(snorp)

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

Flags: needinfo?(snorp)
Attachment #9070682 - Flags: approval-mozilla-beta+ → approval-mozilla-beta-
Attachment #9074358 - Flags: approval-mozilla-beta+ → approval-mozilla-beta-
You need to log in before you can comment on or make changes to this bug.