Allow setting closed (never opened) sessions on GeckoView
Categories
(GeckoView :: General, enhancement, P1)
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)
People
(Reporter: csadilek, Assigned: snorp)
References
Details
(Whiteboard: [geckoview:fenix:m8])
Attachments
(2 files)
47 bytes,
text/x-phabricator-request
|
RyanVM
:
approval-mozilla-beta-
|
Details | Review |
47 bytes,
text/x-phabricator-request
|
RyanVM
:
approval-mozilla-beta-
|
Details | Review |
Updated•6 years ago
|
Comment 2•6 years ago
|
||
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
Reporter | ||
Comment 3•6 years ago
|
||
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?
Updated•6 years ago
|
Comment 4•6 years ago
|
||
(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 7•6 years ago
|
||
Fenix needs this for opening new tabs.
James says he'll post a patch today or we can reassign to another engineer.
Assignee | ||
Comment 8•6 years ago
|
||
This changes a few things:
-
Apps can now set a closed session on the GeckoView. This is
convenient due to how GeckoSession.ContentDelegate.onNewSession()
behaves. -
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. -
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 forsetSession()
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•5 years ago
|
||
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•5 years ago
|
||
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).
Updated•5 years ago
|
Updated•5 years ago
|
Assignee | ||
Comment 11•5 years ago
|
||
Comment 12•5 years ago
|
||
Deferring this bug from Fenix's M7 (July) milestone to the M8 backlog for later in Q3.
Comment 13•5 years ago
|
||
Comment 14•5 years ago
|
||
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•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b94fa94bfea9
https://hg.mozilla.org/mozilla-central/rev/cbcc723e1d6f
Comment 16•5 years ago
|
||
@ 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.
Reporter | ||
Comment 17•5 years ago
|
||
: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.
Reporter | ||
Comment 18•5 years ago
|
||
: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•5 years ago
|
||
firefox69=wontfix because this fix can ride with 70.
Comment 20•5 years ago
|
||
(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!
Assignee | ||
Comment 21•5 years ago
|
||
(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•5 years ago
|
||
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
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Comment 23•5 years ago
|
||
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.
Updated•5 years ago
|
Comment 24•5 years ago
|
||
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')
Updated•5 years ago
|
Assignee | ||
Comment 25•5 years ago
|
||
Ugh. 70 will be in beta soon enough, let's just let this ride.
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Description
•