[research] ActionOverlayService is only killed by the system

NEW
Unassigned

Status

()

Firefox for Android
General
3 years ago
2 years ago

People

(Reporter: mcomella, Unassigned, Mentored)

Tracking

(Blocks: 2 bugs)

unspecified
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(fennec+)

Details

(Whiteboard: [lang=java])

(Reporter)

Description

3 years ago
The service does not stop itself via stopSelf() nor does ShareDialog close it.
(Reporter)

Updated

3 years ago
tracking-fennec: --- → ?
tracking-fennec: ? → +
(Reporter)

Comment 1

3 years ago
Richard, any easy wins here?

Not the easiest win but: I don't see any reason why we have to start the service in ShareDialog.onResume - we can get the clients in the background and convert OverlayActionService into an IntentService which will just handle sending data (tabs, bookmarks, etc.) in the background.

Was the OverlayActionService designed to do more? It seems like there's a lot of unneeded abstraction here.
Flags: needinfo?(rnewman)
(In reply to Michael Comella (:mcomella) from comment #1)
> Richard, any easy wins here?

Remember: a service that remains running will make startService a no-op. In theory, then (I haven't looked too deeply), once you choose Add To Firefox, subsequent interactions will be faster -- the service is already running.

Think carefully about whether it's worth stopping this service. Ideally measure its memory usage -- as far as I can tell, its footprint is a single EnumMap and a single Service class instance, and a little bit of encouragement for Android to keep Fennec memory-resident.

Services are cheap. This one (re)uses Fennec's background thread for work. Think of this is an Androidy lazy singleton.

Consequently, I'm inclined to WONTFIX this bug.


> Was the OverlayActionService designed to do more? It seems like there's a
> lot of unneeded abstraction here.

Yeah, see the comment at the top of the file:

 * Currently supported operations are:
 *
 * Add bookmark*
 * Add to reading list*
 * Send tab (delegates to Sync's existing handler)
 * Future: Load page in background.

ckitching had a pretty much complete implementation of "chat head" style background page loading implemented last summer. That was intended to hook in here.

(The only reason it didn't land, IIRC, is because it effectively implemented a widget toolkit to be able to do the sophisticated overlay stuff, and ckitching didn't want to leave the maintenance burden.)


Of course, doing background page loads kinda requires a semi-persistent service, rather than the temporary-thread-to-process-request pattern that IntentService implements.

That's my recollection, anyway.
Flags: needinfo?(rnewman)
(Reporter)

Comment 3

3 years ago
NI self to investigate comment 2.
Flags: needinfo?(michael.l.comella)
(Reporter)

Comment 4

2 years ago
Realistically not going to get to investigate this.
Blocks: 1157964
Mentor: michael.l.comella@gmail.com, rnewman@mozilla.com
Flags: needinfo?(michael.l.comella)
Summary: ActionOverlayService is only killed by the system → [research] ActionOverlayService is only killed by the system
Whiteboard: [lang=java]
You need to log in before you can comment on or make changes to this bug.