715736 - Add 'Save as Wallpaper' Android functionality in context menu when viewing images

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, enhancement, P5)

Description

[Aaron Train [:aaronmt]]

Stock browser has a 'Save as Wallpaper' feature in the context menu when viewing images.

http://developer.android.com/reference/android/app/WallpaperManager.html

Comment 1

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

Lucas - Sound like a good mentor bug?

Comment 2

[Josh Matthews [:jdm]]

If somebody wants to add a lang=whatever tag as well, that would be useful.

Comment 3

[Sriram [:sriramr]]

Hi.. I'm new here. And I'd like to work on this as my first bug.

Comment 4

[:Margaret Leibovic]

(In reply to Sriram [:sr] from comment #3)
> Hi.. I'm new here. And I'd like to work on this as my first bug.

Welcome! First of all, do you have a build environment set up? If not, you should follow the directions here:
https://wiki.mozilla.org/Mobile/Fennec/Android#Building_Fennec

After that, you should look around our existing context menu code. We already have code that adds context menu items for images:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#445

You'll want to add a new context menu item for images, and you'll have to add some Java code to actually implement the "Save as Wallpaper" functionality. That code could probably go in GeckoApp, near where we implement the "Share Image" functionality:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoApp.java#1079

You should also feel free to join us on IRC to ask questions. We hang out in #mobile on irc.mozilla.org.

Comment 5

[Aaron Train [:aaronmt]]

WallpaperManager also looks like the Android API to use here for the Java side
http://developer.android.com/reference/android/app/WallpaperManager.html

Comment 6

[Sriram [:sriramr]]

Attachment: Added "Set as Wallpaper" context menu option and functionality.

Comment 7

[:Margaret Leibovic]

Comment on attachment 682130 [details] [diff] [review]
Added "Set as Wallpaper" context menu option and functionality.

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

Wow, that was fast! This is looking good, but I have some comments.

As a general comment, please replace tab characters with spaces. We use 2-space indentation for JS, and 4-space indentation for Java.

::: mobile/android/base/AndroidManifest.xml.in
@@ +25,5 @@
>      <uses-permission android:name="com.android.browser.permission.READ_HISTORY_BOOKMARKS"/>
>  
>      <uses-permission android:name="android.permission.WAKE_LOCK"/>
>      <uses-permission android:name="android.permission.VIBRATE"/>
> +    <uses-permission android:name="android.permission.SET_WALLPAPER"/>

I'm not sure what our process is for adding new permissions, but I believe we'll need to update support text explaining why we're using this. Not something you need to worry about, but a reminder to myself to follow up on this :)

::: mobile/android/base/GeckoApp.java
@@ +1080,5 @@
>              } else if (event.equals("Share:Image")) {
>                  String src = message.getString("url");
>                  String type = message.getString("mime");
>                  GeckoAppShell.shareImage(src, type);
> +            } else if (event.equals("Wallpaper:Image")){

I think that "Set:Wallpaper" would be a better name for this message. mfinkle can be kinda crazy about these message names, so maybe he'll jump in with an opinion as well ;)

::: mobile/android/base/GeckoAppShell.java
@@ +1094,5 @@
> +	    		int dataStart = aSrc.indexOf(',');
> +	    		byte[] buf = Base64.decode(aSrc.substring(dataStart+1), Base64.DEFAULT);
> +	    		image = BitmapFactory.decodeByteArray(buf, 0, buf.length);
> +	    	}
> +	    	else{

Please look at other methods in these files for examples of the code style we use. In general, we put else and catch on the same line as the close brace. We also usually include a space before the open brace.

@@ +1105,5 @@
> +	    	mgr.setBitmap(image);
> +	    	Toast.makeText(GeckoApp.mAppContext, "Wallpaper set.", Toast.LENGTH_SHORT).show();
> +    	}
> +    	catch(IOException e){
> +    		Toast.makeText(GeckoApp.mAppContext, "Unable to set Wallpaper.", Toast.LENGTH_SHORT).show();

We need to make this strings l10n-friendly. To do this on the Java side, you'll need to add entities to android_strings.dtd and strings.xml.in, and then you can get at them with R.string.entity_name (where entity_name is the new entity you added). You can see examples in other places where we call Toast.makeText.

@@ +1107,5 @@
> +    	}
> +    	catch(IOException e){
> +    		Toast.makeText(GeckoApp.mAppContext, "Unable to set Wallpaper.", Toast.LENGTH_SHORT).show();
> +    	}
> +    }

Is there a reason why we need to put this in GeckoAppShell, instead of keeping all the logic in GeckoApp? I ask because keeping it in GeckoApp would avoid the references to GeckoApp.mAppContext.

::: mobile/android/chrome/content/browser.js
@@ +488,5 @@
> +            mime: type,
> +          }
> +        });
> +      });
> +    

Most of this code is duplicated from the share image context menu item. I think it would be better to factor this out to a shared function that takes a message type as a parameter.

Comment 8

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

(In reply to Margaret Leibovic [:margaret] from comment #7)

> >              } else if (event.equals("Share:Image")) {
> >                  String src = message.getString("url");
> >                  String type = message.getString("mime");
> >                  GeckoAppShell.shareImage(src, type);
> > +            } else if (event.equals("Wallpaper:Image")){
> 
> I think that "Set:Wallpaper" would be a better name for this message.
> mfinkle can be kinda crazy about these message names, so maybe he'll jump in
> with an opinion as well ;)

"Wallpaper:Set" is more consistent with our other messages.

Comment 9

[Sriram [:sriramr]]

Thanks Margaret, Mark. 

I will do the clean up and create a new patch soon. I might have some questions on browser.js code. I will let you guys know if I can't figure it out myself.

Comment 10

[Sriram [:sriramr]]

Attachment: Wallpaper patch - Cleaner version

- Changed message name to "Wallpaper:Set" (as per mfinkle's suggestion)
 - Added entries to android_strings.dtd and strings.xml.in to make the string l10n friendly
 - Moved the Wallpaper setting logic to GeckoApp.
 - Factored the code for callback methods of Share Image and Set as Wallpaper context menus, to a shared function in browser.js
 - Standard code style followed :)

Comment 11

[:Margaret Leibovic]

Comment on attachment 682955 [details] [diff] [review]
Wallpaper patch - Cleaner version

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

Thanks for the updated patch. I tried this out and it worked, but I'm unsure if your setImageAsWallpaper method is doing everything it needs to do. I'm asking snorp to take a look at this as well, since he reviewed the patch for sharing images.

Also, how does this deal with various image sizes? Should we do something like sample part of the image based on the resolution of the device?

::: mobile/android/base/GeckoApp.java
@@ +1373,5 @@
>              }
>          });
>      }
>      
> +    private void setImageAsWallpaper(String aSrc, String aType){

I noticed you're not using this aType parameter for anything. Looking into the shareImage method this is modeled after, that method does a lot more with the image, including storing it in a temporary file. I'm not familiar with the history of that code, so I'm going to ask snorp to take a look at this and tell us if it's okay.

@@ +1380,5 @@
> +        try{
> +            if(isDataURI){
> +                int dataStart = aSrc.indexOf(',');
> +                byte[] buf = Base64.decode(aSrc.substring(dataStart+1), Base64.DEFAULT);
> +                image = BitmapFactory.decodeByteArray(buf, 0, buf.length);

What if this image is really large? Is this going to take a ton of memory?

@@ +1381,5 @@
> +            if(isDataURI){
> +                int dataStart = aSrc.indexOf(',');
> +                byte[] buf = Base64.decode(aSrc.substring(dataStart+1), Base64.DEFAULT);
> +                image = BitmapFactory.decodeByteArray(buf, 0, buf.length);
> +                } else {

Nit: Align this else line with the previous if line.

@@ +1390,5 @@
> +            }
> +            WallpaperManager mgr = WallpaperManager.getInstance(mAppContext);
> +            mgr.setBitmap(image);
> +            showToast(R.string.wallpaper_success, Toast.LENGTH_SHORT);
> +            } catch(IOException e) {

Nit: Align this catch line with the previous try line.

@@ +1394,5 @@
> +            } catch(IOException e) {
> +            showToast(R.string.wallpaper_fail, Toast.LENGTH_SHORT);
> +        }
> +    }
> +    

Nit: Remove trailing whitespace.

@@ +1786,5 @@
>          registerEventListener("WebApps:Uninstall");
>          registerEventListener("DesktopMode:Changed");
>          registerEventListener("Share:Text");
>          registerEventListener("Share:Image");
> +        registerEventListener("Wallpaper:Set");

You also need to add a line to unregister the event listener. Search for |unregisterEventListener("Share:Image")| to see where to do that.

::: mobile/android/base/locales/en-US/android_strings.dtd
@@ +132,5 @@
>  <!ENTITY desktop_mode "Request Desktop Site">
>  <!ENTITY tools "Tools">
>  <!ENTITY new_pb_tab "New Private Browsing Tab">
>  
> +<!ENTITY wallpaper_success "Wallpaper set.">

To be consistent with our other toast messages, remove the period at the end of this string.

::: mobile/android/base/strings.xml.in
@@ +238,5 @@
>    <string name="updater_apply_select">&updater_apply_select2;</string>
>  
>    <!-- Search suggestions opt-in -->
>    <string name="suggestions_prompt">&suggestions_prompt;</string>
> +  

Nit: Remove trailing whitespace.

::: mobile/android/chrome/content/browser.js
@@ +449,2 @@
>        });
> +    

Nit: Remove trailing whitespace. There's also extra whitespace on other empty lines in this patch.

@@ +494,5 @@
> +            url: src,
> +            mime: type,
> +          }
> +        });
> +    }

If it turns out we don't need the mime type for the "Wallpaper:Set" case in GeckoApp, I don't think it makes sense to include that in a shared function. In fact, if we don't need the mime type, we also don't need to get props or imageCache, so the callback for the wallpaper context menu item can actually be really simple.

But let's wait to hear back from snorp before changing this around.

Comment 12

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 682955 [details] [diff] [review]
Wallpaper patch - Cleaner version

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

Overall looks pretty good, but need to see another patch.

::: mobile/android/base/GeckoApp.java
@@ +1373,5 @@
>              }
>          });
>      }
>      
> +    private void setImageAsWallpaper(String aSrc, String aType){

Yeah, nothing in the WallpaperManager API needs mime type, so this argument should be removed.

@@ +1380,5 @@
> +        try{
> +            if(isDataURI){
> +                int dataStart = aSrc.indexOf(',');
> +                byte[] buf = Base64.decode(aSrc.substring(dataStart+1), Base64.DEFAULT);
> +                image = BitmapFactory.decodeByteArray(buf, 0, buf.length);

Yes, this is a bad idea. I recommend using WallpaperManager.setStream (InputStream data). You can create an InputStream from the byte array using ByteArrayInputStream. The heap space for bitmaps is very small, so we need to avoid creating them whenever we can.

Comment 13

[Sriram Ramasubramanian [:sriram]]

I would recommend listening for the event in BrowserApp and not GeckoApp. GeckoApp is way too generic (and common for WebApp) too.

Comment 14

[:Margaret Leibovic]

Wes, do these context menus for images exist in web apps? If so, this should stay in GeckoApp, but if not, this could be moved to BrowserApp (along with the logic for the other context menu items).

Comment 15

[Wesley Johnston (:wesj)]

Right now, we don't show any context menus for images in WebApps, and I kinda don't want to add them (other than using HTML5 context menus, which we don't support yet, but I could be convinced otherwise).

Comment 16

[:Margaret Leibovic]

(In reply to Wesley Johnston (:wesj) from comment #15)
> Right now, we don't show any context menus for images in WebApps, and I
> kinda don't want to add them (other than using HTML5 context menus, which we
> don't support yet, but I could be convinced otherwise).

Okay, well in either case, we can move the logic for handling these context menu messages to BrowserApp. In this bug we can put the "Wallpaper:Set" logic in BrowserApp, and we can file a separate bug about moving the other context menu event listeners in there as well. Maybe that's something that Sriram would want to do after this bug is fixed :)

Comment 17

[Wesley Johnston (:wesj)]

Since this is directly driving the browser ui, just providing functionality, I personally really don't mind keeping it in GeckoApp. I would probably make setImageAsWallpaper static and put it in GeckoAppShell so that its eas(ier) for us to use JNI to call from C++ to sometime, and leave the message handling code in GeckoApp.

Comment 18

[Sriram [:sriramr]]

(In reply to Margaret Leibovic [:margaret] from comment #11)

> Also, how does this deal with various image sizes? Should we do something
> like sample part of the image based on the resolution of the device?

I can probably scale the images according to the resolution of the display. This should solve the problem of only a portion of large image being set as wallpaper. And because we are scaling the image to a lower resolution, it should also save memory. 

Please let me know your thoughts. I will create another patch soon.

Comment 19

[Lucas Rocha (:lucasr)]

(In reply to Sriram [:sr] from comment #18)
> (In reply to Margaret Leibovic [:margaret] from comment #11)
> 
> > Also, how does this deal with various image sizes? Should we do something
> > like sample part of the image based on the resolution of the device?
> 
> I can probably scale the images according to the resolution of the display.
> This should solve the problem of only a portion of large image being set as
> wallpaper. And because we are scaling the image to a lower resolution, it
> should also save memory. 
> 
> Please let me know your thoughts. I will create another patch soon.

You should probably use WallpaperManager's getDesiredMinimumWidth() and getDesiredMinimumHeight() to figure out the ideal image size.

Comment 20

[Sriram [:sriramr]]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #12)

> Yes, this is a bad idea. I recommend using WallpaperManager.setStream
> (InputStream data). You can create an InputStream from the byte array using
> ByteArrayInputStream. The heap space for bitmaps is very small, so we need
> to avoid creating them whenever we can.

I don't think we can scale the images without creating a Bitmap object. And yes, it takes some precious memory. An image size of 960 x 800 (wallpaper size of WVGA800 screens) will use approx 3 MB of memory as Bitmap. 

Or we can skip the scaling and set the wallpaper directly from the stream.

Comment 21

[Sriram [:sriramr]]

Attachment: 3rd Patch for "Set as Wallpaper" context menu (Proposed patch)

- Removed the code that was duplicated from "Share:Image" context menu in browser.js
 - Moved the Wallpaper setting logic off the main thread. 
 - Used a ProgressDialog to indicate the processing.
 - Hopefully, have taken care of the white spaces :)

Comment 22

[Sriram [:sriramr]]

I was not sure about using a bitmap at first. But without creating a Bitmap, scaling would not have been possible. And without scaling, large / very large images cannot be set as wallpaper. 

I was experimenting with WallpaperManager's setStream, setResource and setBitmap methods. None of them would work with a very large image.

I tried to set an image with a resolution of 5005 x 3274 as wallpaper, using setStream method. But to my surprise, the Wallpaper of the device changed to default stock wallpaper. No exceptions, errors or logs. I tried the same using setResource and setBitmap methods, but still the result was the same. I tried the same in different devices, with different screen sizes. But still the result was the same.

I was not able to find any documentation about limits in image size in WallpaperManager's docs. Can anybody help me understand why this happens?

Comment 23

[Sriram [:sriramr]]

Attachment: Proposed patch for "Set as Wallpaper" context menu

- Removed the code that was duplicated from "Share:Image" context menu in browser.js
 - Moved the Wallpaper setting logic off the main thread. 
 - Used a ProgressDialog to indicate the processing.
 - Unregistered the "Wallpaper:Set" event
 - Hopefully, have taken care of the white spaces :)

Comment 24

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 684966 [details] [diff] [review]
Proposed patch for "Set as Wallpaper" context menu

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

Looking much better, but I think we need one more iteration

::: mobile/android/base/GeckoApp.java
@@ +1378,5 @@
>      }
> +
> +    private void setImageAsWallpaper(final String aSrc) {
> +
> +        mMainHandler.post(new Runnable(){

Why do we need this if all we are doing is running a GeckoAsyncTask?

@@ +1460,5 @@
> +                                    os.write(buf, 0, byteRead);
> +                                }
> +                                byte[] imgBuffer = os.toByteArray();
> +                                is.close();
> +                                os.close();

These streams should be closed in a finally clause, otherwise they could be left open.

@@ +1487,5 @@
> +            }
> +        });
> +    }
> +
> +    private int getInSampleSize(BitmapFactory.Options options, int idealWidth, int idealHeight) {

maybe getBitmapSampleSize is a better name?

@@ +1491,5 @@
> +    private int getInSampleSize(BitmapFactory.Options options, int idealWidth, int idealHeight) {
> +        int width = options.outWidth;
> +        int height = options.outHeight;
> +        int inSampleSize = 1;
> +        if (height > idealHeight || width > idealWidth) {

looks like this condition is redundant

Comment 25

[:Margaret Leibovic]

Before we land this, I'd like to get some feedback from Ian, our UX person, about these strings:

+<!ENTITY wallpaper_prog_title "Please wait">
+<!ENTITY wallpaper_progress "Setting Wallpaper">
+<!ENTITY wallpaper_fail "Unable to set wallpaper">

I'm also not sure we need the "Please wait" message. It feels like an odd use of as toast message.

Comment 26

[:Margaret Leibovic]

Comment on attachment 684966 [details] [diff] [review]
Proposed patch for "Set as Wallpaper" context menu

Other than my concern about the strings for the toast messages, the front-end side of things look good to me, so we can just let snorp do the final review on the setImageAsWallpaper logic.

Comment 27

[Sriram [:sriramr]]

(In reply to Margaret Leibovic [:margaret] from comment #25)

> I'm also not sure we need the "Please wait" message. It feels like an odd
> use of as toast message.

The "Please wait" is title of the ProgressDialog that spins when processing large images. It is not a Toast message.

Comment 28

[:Margaret Leibovic]

(In reply to Sriram [:sriramr] from comment #27)
> (In reply to Margaret Leibovic [:margaret] from comment #25)
> 
> > I'm also not sure we need the "Please wait" message. It feels like an odd
> > use of as toast message.
> 
> The "Please wait" is title of the ProgressDialog that spins when processing
> large images. It is not a Toast message.

My apologies, I looked at the patch too quickly. I didn't realize we were showing a progress dialog. I'll let Ian decide, but I don't think we should do that. I think that the setImageAsWallpaper code should just run in the background, then notify the user with a toast when it's done.

Comment 29

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

(In reply to Margaret Leibovic [:margaret] from comment #28)
> (In reply to Sriram [:sriramr] from comment #27)
> > (In reply to Margaret Leibovic [:margaret] from comment #25)
> > 
> > > I'm also not sure we need the "Please wait" message. It feels like an odd
> > > use of as toast message.
> > 
> > The "Please wait" is title of the ProgressDialog that spins when processing
> > large images. It is not a Toast message.
> 
> My apologies, I looked at the patch too quickly. I didn't realize we were
> showing a progress dialog. I'll let Ian decide, but I don't think we should
> do that. I think that the setImageAsWallpaper code should just run in the
> background, then notify the user with a toast when it's done.

I kinda agree with Margaret, mainly because we don't use the ProgressDialog in any other place. We just use a toast at the end of the background task.

Comment 30

[Ian Barlow (:ibarlow)]

(In reply to Margaret Leibovic [:margaret] from comment #28)

> My apologies, I looked at the patch too quickly. I didn't realize we were
> showing a progress dialog. I'll let Ian decide, but I don't think we should
> do that. I think that the setImageAsWallpaper code should just run in the
> background, then notify the user with a toast when it's done.

Yeah, I agree. It's not something a user is actively waiting on to make sure it's done, a simple confirmation toast should be plenty here.

Comment 31

[Sriram [:sriramr]]

(In reply to Ian Barlow (:ibarlow) from comment #30)

> Yeah, I agree. It's not something a user is actively waiting on to make sure
> it's done, a simple confirmation toast should be plenty here.

When the user tries to set large images with a size, say 4 MB, as wallpaper, it may take a few seconds before the wallpaper is actually set. During this time, an eager user trying to see his new wallpaper, may click on set wallpaper option again. This will trigger the whole process again. Because we are dealing with Bitmaps here,(even though it is scaled) it involves a lot of memory. Any one who uses the "Set as Wallpaper" option three or more times on a very large image, can easily make the application run out of memory. That was the reason I came up with ProgressDialog. Please let me know your suggestions.

Comment 32

[Ian Barlow (:ibarlow)]

Can we be smarter and more subtle about how we solve this then? Is it possible to even utilize the notification bar to signal that a wallpaper is downloading and being applied?

Comment 33

[Sriram [:sriramr]]

 (In reply to Ian Barlow (:ibarlow) from comment #32)
> Can we be smarter and more subtle about how we solve this then? Is it
> possible to even utilize the notification bar to signal that a wallpaper is
> downloading and being applied?

Sure, I think we can do that. I will try using the Notification bar and will let you know how that came up.

Comment 34

[Sriram [:sriramr]]

Attachment: Preview patch

As per ibarlow's suggestion, I have used the notifications to indicate the status of wallpaper. But I was not sure about the icon to be used. So I have used the alert_download icon.

Comment 35

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 690115 [details] [diff] [review]
Preview patch

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

Looks good to me with those nits fixed

::: mobile/android/base/GeckoApp.java
@@ +1384,5 @@
> +        final String failureText = mAppContext.getString(R.string.wallpaper_fail);
> +        final String fileName = aSrc.substring(aSrc.lastIndexOf("/") + 1);
> +        final PendingIntent emptyIntent = PendingIntent.getActivity(mAppContext, 0, new Intent(), 0);
> +        final AlertNotification notification = new AlertNotification(mAppContext, fileName.hashCode(), 
> +                                R.drawable.alert_download, fileName, progText, System.currentTimeMillis() );

Actually I think R.ic_status_logo might be better, but of course ibarlow would know for sure.

@@ +1391,5 @@
> +        notification.show();
> +        new GeckoAsyncTask<Void, Void, Integer>(mAppContext, GeckoAppShell.getHandler()){
> +
> +            @Override
> +            protected Integer doInBackground(Void... params) {

You only return 0 or 1, so maybe boolean is a better return type?

@@ +1472,5 @@
> +                    }
> +                    mgr.setBitmap(image);
> +                    return 0;
> +                } catch(OutOfMemoryError ome) {
> +                    Log.w(LOGTAG, "Decoding byte array ran out of memory", ome);

Should be Log.e, I think, and you can pass in the exception

@@ +1475,5 @@
> +                } catch(OutOfMemoryError ome) {
> +                    Log.w(LOGTAG, "Decoding byte array ran out of memory", ome);
> +                    return 1;
> +                } catch(IOException ioe) {
> +                    Log.w(LOGTAG, "I/O Exception while setting wallpaper", ioe);

Same as above

Comment 36

[Lucas Rocha (:lucasr)]

(In reply to Sriram [:sriramr] from comment #34)
> Created attachment 690115 [details] [diff] [review]
> Preview patch
> 
> As per ibarlow's suggestion, I have used the notifications to indicate the
> status of wallpaper. But I was not sure about the icon to be used. So I have
> used the alert_download icon.

Providing a link to a test build (apk) would probably help ibarlow give his approval btw :-)

Comment 37

[:Margaret Leibovic]

Comment on attachment 690115 [details] [diff] [review]
Preview patch

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

I'm clearing my review request, since snorp gave a review, but I have one small comment.

::: mobile/android/base/locales/en-US/android_strings.dtd
@@ +133,5 @@
>  <!ENTITY tools "Tools">
>  <!ENTITY new_pb_tab "New Private Browsing Tab">
>  
> +<!ENTITY wallpaper_success "Wallpaper set">
> +<!ENTITY wallpaper_progress "Your wallpaper is being downloaded">

I think "Downloading wallpaper" would be more consistent with our other notifications, or maybe "Setting wallpaper", since the context menu item is "Set as Wallpaper", but ibarlow should decide what we say here.

Comment 38

[Sriram [:sriramr]]

(In reply to Lucas Rocha (:lucasr) from comment #36)

> Providing a link to a test build (apk) would probably help ibarlow give his
> approval btw :-)

Below is the link to my build.

http://ubuntuone.com/5de5azGOLRNyFQ3jjRLaDB

ibarlow, can you please take a look at this..

Comment 39

[Ian Barlow (:ibarlow)]

This is really cool! I have a feeling I'm going to be using this to change up my wallpaper a lot from now on :)

Couple of comments. 

Let's adjust the menu order for where we put this function. I would probably move Set wallpaper down a notch, since I think saving an image is a slightly more common use case. 

So, 

Copy image location
Share image
Save image
Set as wallpaper
etc...

Comment 40

[Ian Barlow (:ibarlow)]

And as for the notification, let's change it to read 

"Setting wallpaper"

And then

"Wallpaper updated"

Comment 41

[Sriram [:sriramr]]

Attachment: Final patch

- Changed the notification strings to Setting wallpaper and Wallpaper updated
 - Changed the order of the Set as Wallpaper menu (after Save Image now)

Comment 42

[:Margaret Leibovic]

Comment on attachment 692991 [details] [diff] [review]
Final patch

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

Overall looking good! I just have one question and a nit, and then we'll be ready to land this!

::: mobile/android/base/GeckoApp.java
@@ +1411,5 @@
> +                        while((byteRead = is.read(buf)) != -1) {
> +                            os.write(buf, 0, byteRead);
> +                        }
> +                        byte[] imgBuffer = os.toByteArray();
> +                        BitmapFactory.decodeByteArray(imgBuffer, 0, imgBuffer.length, options);

I noticed this line wasn't in the previous version of the patch. Is there a reason why you added it? Does it even do anything?

@@ +1451,5 @@
> +            protected void onPostExecute(Boolean success) {
> +                notification.cancel();
> +                notification.flags = 0;
> +                notification.flags |= Notification.FLAG_AUTO_CANCEL;
> +                if(!success) {

Nit: You can just check success, and flip the cases around in here.

Comment 43

[Sriram [:sriramr]]

(In reply to Margaret Leibovic [:margaret] from comment #42)

> ::: mobile/android/base/GeckoApp.java
> @@ +1411,5 @@
> > +                        while((byteRead = is.read(buf)) != -1) {
> > +                            os.write(buf, 0, byteRead);
> > +                        }
> > +                        byte[] imgBuffer = os.toByteArray();
> > +                        BitmapFactory.decodeByteArray(imgBuffer, 0, imgBuffer.length, options);
> 
> I noticed this line wasn't in the previous version of the patch. Is there a
> reason why you added it? Does it even do anything?

I have to read the content of InputStream twice, i) to determine the real size of the image, ii) to decode the image into a Bitmap. So in the above lines, I'm converting the InputStream to a ByteArray. Because, the InputStream cannot be read twice.

Comment 44

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Sriram [:sriramr] from comment #43)
> (In reply to Margaret Leibovic [:margaret] from comment #42)
> 
> > ::: mobile/android/base/GeckoApp.java
> > @@ +1411,5 @@
> > > +                        while((byteRead = is.read(buf)) != -1) {
> > > +                            os.write(buf, 0, byteRead);
> > > +                        }
> > > +                        byte[] imgBuffer = os.toByteArray();
> > > +                        BitmapFactory.decodeByteArray(imgBuffer, 0, imgBuffer.length, options);
> > 
> > I noticed this line wasn't in the previous version of the patch. Is there a
> > reason why you added it? Does it even do anything?
> 
> I have to read the content of InputStream twice, i) to determine the real
> size of the image, ii) to decode the image into a Bitmap. So in the above
> lines, I'm converting the InputStream to a ByteArray. Because, the
> InputStream cannot be read twice.

Yeah that looks alright to me. I do worry about reading very large files here, though. A good followup patch might be to download to disk if the file is above some threshold (5MB?).

Comment 45

[:Margaret Leibovic]

https://hg.mozilla.org/integration/mozilla-inbound/rev/37ad8c11c3fb

Sriram, I just landed your patch on mozilla-inbound, which will be merged into mozilla-central later today or tomorrow, which means it will be in Nightly builds in 1-2 days!

Thanks so much for your hard work, this was a really big patch for a first-time contributor! :)

Comment 46

[Sriram [:sriramr]]

(In reply to Margaret Leibovic [:margaret] from comment #45)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/37ad8c11c3fb
> 
> Sriram, I just landed your patch on mozilla-inbound, which will be merged
> into mozilla-central later today or tomorrow, which means it will be in
> Nightly builds in 1-2 days!
> 
> Thanks so much for your hard work, this was a really big patch for a
> first-time contributor! :)

Thanks Margaret. Its a great experience to be contributing.

Comment 47

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/37ad8c11c3fb

Comment 48

[Ioana Chiorean]

TCs were added in MozTrap for Firefox 20, Firefox 20 Tablets in the Context menu suite :
https://moztrap.mozilla.org/manage/cases/?filter-suite=94

https://moztrap.mozilla.org/manage/case/6028/ Context menu options for images
https://moztrap.mozilla.org/manage/case/6029/ Images - Set as Wallpaper

Comment 49

[:Margaret Leibovic]

Due to the permissions change, this feature is temporarily disabled for Firefox 20 and 21. See bug 844935 for more details.