Make GeckoSession non-final

RESOLVED FIXED in Firefox 64

Status

enhancement
P3
normal
RESOLVED FIXED
11 months ago
8 months ago

People

(Reporter: paul, Assigned: paul)

Tracking

unspecified
mozilla64

Firefox Tracking Flags

(geckoview62 wontfix, firefox62 wontfix, firefox63 wontfix, firefox64 fixed)

Details

Attachments

(1 attachment)

Bug 1486778 made GeckoSession final, making it impossible to extend GeckoSession (necessary for Servo support).
Posted patch 1492771-01Splinter Review
Assignee: nobody → paul
It generally does not make much sense for consumers to subclass GeckoSession, which is why I made it final. Additionally, since GeckoView is not intended to be an abstraction layer for other engines, I'm not inclined to make API changes that cater to this type of usage.

I really think Servo should reconsider GeckoView compatibility as a goal and try to solve this at a higher level. The Android Components[0] "engine" may be a good fit. 

[0] https://github.com/mozilla-mobile/android-components/
Attachment #9010631 - Flags: review?(nchen)
Attachment #9010631 - Flags: review?(nchen)
James, understood. I'll talk to the FxReality folks and see what should be the best approach then.

I already suggested to use Android Component: https://github.com/MozillaReality/FirefoxReality/issues/80
I think there are some arguments for subclassing GeckoSession. Subclassing is pretty common on Android to change the behavior of Application, Activity, View, etc... GeckoView itself is non-final.

There are some use cases where subclassing would be useful. For example, if the app wants to collect some telemetry whenever `GeckoSession.loadUri` is called, or if the app wants to associate every `GeckoSession` instance with some piece of data. In those cases there are workarounds, but subclassing would be the most straightforward way.
Jim and I talked about this a little more, and while it may make sense to subclass GeckoSession for reasons similar to the ones listed in comment #4, I think we need to be cautious about the protected stuff that's exposed and which methods can be overridden. From that perspective, LayerSession is a pretty big API wart (which is why I added 'final'). We'll accept the patch to remove 'final' from GeckoSession, but do not expect to be able to override every method in the future.
Attachment #9010631 - Flags: review?(snorp)
Attachment #9010631 - Flags: review?(snorp) → review+
https://hg.mozilla.org/mozilla-central/rev/a7f1d2b04291
Status: NEW → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
Product: Firefox for Android → GeckoView
Target Milestone: Firefox 64 → mozilla64
You need to log in before you can comment on or make changes to this bug.