Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla] Reporter
	Description • 9 years ago

The service does not stop itself via stopSelf() nor does ShareDialog close it.

	Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla] Reporter
	Comment 1 • 9 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.

	Richard Newman [:rnewman]
	Comment 2 • 9 years ago

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

	Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla] Reporter
	Comment 3 • 9 years ago

NI self to investigate comment 2.

	Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla] Reporter
	Comment 4 • 9 years ago

Realistically not going to get to investigate this.

	BMO Automation
	Comment 5 • 4 years ago

We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.