901824 - Use image cache for 'Set Image As'

Status: RESOLVED INCOMPLETE

(Firefox for Android Graveyard :: General, defect)

Description

[Shilpan Bhagat]

Currently we download the whole picture synchronously by blocking the user on the main thread. Use the image cache to retrieve the data.

Comment 1

[Shilpan Bhagat]

Attachment: [WIP] Use Image Cache for "Set Image As"

Had to create listeners for downloading silently without any notifications.

Comment 2

[Shilpan Bhagat]

Attachment: [WIP] Using image cache from gecko to set up wallpaper

oops, forgot to delete the image after setting it as wallpaper

Comment 3

[Wesley Johnston (:wesj)]

Comment on attachment 786134 [details] [diff] [review]
[WIP] Using image cache from gecko to set up wallpaper

Review of attachment 786134 [details] [diff] [review]:
-----------------------------------------------------------------

Getting there, but needs some error handling and safety features. 

I wanted mark to look at this just to see if he's ok with the approach.

::: mobile/android/base/GeckoApp.java
@@ +974,5 @@
>      }
>  
>      // This method starts downloading an image synchronously and displays the Chooser activity to set the image as wallpaper.
> +    private void setImageAs(final String aFilePath) {
> +        if (aFilePath != null) {

I don't think you need this if anymore.

@@ +975,5 @@
>  
>      // This method starts downloading an image synchronously and displays the Chooser activity to set the image as wallpaper.
> +    private void setImageAs(final String aFilePath) {
> +        if (aFilePath != null) {
> +            try {

Or this try. We shouldn't be wrapping things in try-catch unless we have to.

::: mobile/android/chrome/content/browser.js
@@ +597,5 @@
>  
>      NativeWindow.contextmenus.add(Strings.browser.GetStringFromName("contextmenu.setImageAs"),
>        NativeWindow.contextmenus.imageSaveableContext,
>        function(aTarget) {
> +        Downloads.addSilentDownloadProgressListener(aTarget.currentURI.spec, {

lets just call this addListenerForUri(...);

@@ +605,5 @@
> +              sendMessageToJava({
> +                type: "Wallpaper:Set",
> +                url: aDownload.targetFile.path
> +              });
> +              Downloads.removeSilentDownloadProgressListener(aTarget.currentURI.spec);

removeListenerForUri(...).

You also need to be careful about handling error states here. If this fails, we should probably show the "Error setting wallpaper" (or a new string) toast right from JS.

::: mobile/android/chrome/content/downloads.js
@@ +15,5 @@
>    _initialized: false,
>    _dlmgr: null,
>    _progressAlert: null,
>    _privateDownloads: [],
> +  _silentDownloadProgressListener: {},

listeners

@@ +126,5 @@
>      return this;
> +  },
> +
> +  addSilentDownloadProgressListener: function(uri, downloadProgressListener) {
> +    this._silentDownloadProgressListener[uri] = downloadProgressListener;

So for safety, I wonder if we should throw if someone is already registered for this uri.

@@ +142,5 @@
>    //////////////////////////////////////////////////////////////////////////////
>    //// nsIDownloadProgressListener
>    onProgressChange: function(aWebProgress, aRequest, aCurSelfProgress, aMaxSelfProgress, aCurTotalProgress, aMaxTotalProgress, aDownload) {
> +    if(Downloads._silentDownloadProgressListener[aDownload.source.spec]) {
> +      if (typeof Downloads._silentDownloadProgressListener[aDownload.source.spec].onProgressChange === "function") {

I don't think we need a typeof check. Just check for the existence of the method. i.e.
if (Downloads._listeners[aDownload.source.spec].onProgressChange) {
}

Also, we should early return from these, and I think they should probably return true or false based on if they want to stop the normal behaviour. i.e. I think this should be written:

let listener = Downloads.getListenerFor(aDownload.source.spec);
if (listener && listener.onProgressChange) {
  if (listener.onProgressChange(...)
    return;
}

Since that's probably going to be repeated, you could even try to pull it out into a helper:

if (this._sendToListener("onProgressChange", aDownload.source, [aWebProgres, aRequest...])
  return;

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

I am not thrilled with the hoops we are jumping through. Adding a URI-lookup based silent download listener seems rip for problems. I need to think about this more.

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

Wes suggested I look at this again because we might have other uses for it.

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 786134 [details] [diff] [review]
[WIP] Using image cache from gecko to set up wallpaper

I could cozy up to this idea with some tweaks:
* addSilentDownloadProgressListener/removeSilentDownloadProgressListener ->
  addOverrideProgressListener/removeOverrideProgressListener
* Since this is async, what happens if we try to add the image twice, before the first image is downloaded? The URL used to add the listener would be in there twice. Or it would be in there once, but we would remove the single listener when the first image completes. We need to think about the edge case.
* Could we create defaultProgessChange, defaultDownloadStateChange, etc and then the real methods would be:

if (hasOverride) {
  use override handler
} else {
  use defaultXxx
}

Comment 7

[Wesley Johnston (:wesj)]

Attachment: WIP

Updated for new downloads code. There's some cruft in here as well because somethings up with my builds. This adds a few methods to DownloadNotifications:

public void override(url, Listener);
public void unoverride(url, Listener);

where listener has a signature

boolean Listener::(Download download, String eventType)

If the listener returns true, the normal handling of this download is overridden. What do you think of this approach mfinkle?

Getting this to work for "Share Image" will take some changes. Our context menu code isn't designed for async behavior (that's fixable without to much work), but really what we want here is to do this until we KNOW that you're going to share. Unfortunately, the QuickShare code doesn't ever ping back to JS when you quick share. Instead you just give it a url, and the rest of the logic happens in java.

We'll need to get a callback in javascript in order to get the correct url to share in order for this to work well. Maybe we can have the normal contextmenu callback still fire, with the details about the app that was selected (using the App objects that are in HelperApps?). If it gets no app, it knows to just use a generic intent. Javascript can then launch the intent itself....

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8408836 [details] [diff] [review]
WIP

There might be some extra cruft in this patch.

I like your download listener approach. It makes things cleaner. One bit of feedback:

You use a single function and pass in an event as a string. A different way of doing it would be to create an object and have optional onAdded, onChanged methods. In the _handleOverrides method you could call handler["onAdded"](download), if the method existed.

Just something to think about.

Comment 9

[BMO Automation]

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.