Remove plugin-related methods from GeckoInterface

RESOLVED FIXED in Firefox 55

Status

enhancement
RESOLVED FIXED
2 years ago
6 months ago

People

(Reporter: jchen, Assigned: jchen)

Tracking

50 Branch
mozilla55
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(3 attachments)

No description provided.
Remove the addPluginView and removePluginView methods from
GeckoInterface. Instead, move the JNI calls directly to GeckoApp itself.
GeckoApp then uses GeckoActivityMonitor to find the current activity,
instead of using GeckoAppShell.getGeckoInterface().
Attachment #8867315 - Flags: review?(rbarker)
Move the native call onFullScreenPluginHidden from GeckoAppShell to
GeckoApp itself.
Attachment #8867316 - Flags: review?(rbarker)
GeckoInterface.getPluginContainer is no longer used anywhere and can be
removed. r=me for trivial patch.
Attachment #8867320 - Flags: review+
Attachment #8867315 - Flags: review?(rbarker) → review+
Attachment #8867316 - Flags: review?(rbarker) → review+
Before you land, a question: If the GeckoActivityMonitor didn't exist, what would you have done instead? I'm asking because Sebastian wasn't entirely happy about it when I added it in bug 1352997, and since I've found a way to no longer need it for that purpose, I was intending to remove it again in bug 1359531.
Flags: needinfo?(nchen)
We would need some other way to keep track of the active GeckoApp if we don't have GeckoActivityMonitor. What are the arguments against GeckoActivityMonitor?
Flags: needinfo?(nchen) → needinfo?(jh+bugzilla)
Correction: The GAM was added in bug 1351739.

That aside, you'd have to ask Sebastian what his objections were exactly, and whether they were more general, or just specific to the context of my patch.

Since you're just interested in the current activity if it's something GeckoApp-based, a more limited version of the activity monitor could be recreated by building on our current onActivityPause/Resume-tracking in GeckoApplication (https://dxr.mozilla.org/mozilla-central/rev/73b3fc64525b6816842c737e104ef2ac5482d217/mobile/android/base/java/org/mozilla/gecko/GeckoApplication.java#144-182).

Having said that, even if soon the activity monitor won't be needed for web app/custom tab activity switching any more, our application-background/-foreground tracking in GeckoApplication has some gaps and to be honest I'm pretty undecided on whether the GAM could help there or not. So I'm no longer quite as certain about immediately removing it without experimenting a little in that direction ...
Flags: needinfo?(jh+bugzilla) → needinfo?(s.kaspari)

Comment 7

2 years ago
Pushed by nchen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/06142f9787e7
1. Remove addPluginView/removePluginView methods; r=rbarker
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f9d8749eb5e
2. Move onFullScreenPluginHidden to GeckoApp; r=rbarker
https://hg.mozilla.org/integration/mozilla-inbound/rev/49c90446a20f
3. Remove GeckoInterface.getPluginContainer; r=me
We talked on IRC briefly yesterday.

My objections were more general than about this particular implementation. There are a bunch of edge cases that make tracking this hard and it can lead to wrong assumptions (e.g. a paused activity doesn't mean that it's not visible). For tab switching we are able to avoid it - if we need it for other features then I guess we will have to keep it for now.
Flags: needinfo?(s.kaspari)
Assignee: nobody → nchen

Updated

6 months ago
Product: Firefox for Android → GeckoView
Target Milestone: Firefox 55 → mozilla55
You need to log in before you can comment on or make changes to this bug.