990642 - Regression: 'Share Image' shares link and not actual image

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Paul [pwd]]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux i686; rv:31.0) Gecko/20100101 Firefox/31.0 (Beta/Release)
Build ID: 20140329061102

Steps to reproduce:

The manner in which this is handled has been changed and the change has broken Share Image. I can't find the bug that changed the interaction behaviour though.

I suspected it was bug 780722 but that doesn't have any patches, so I'm unsure.

Comment 1

[Aaron Train [:aaronmt]]

In which regard is 'Share Image' broken? Recent sharing changes have added context-menu quick-share via bug 942270.

Comment 2

[Paul [pwd]]

(In reply to Aaron Train [:aaronmt] from comment #1)
> In which regard is 'Share Image' broken? 

Images can no longer be shared from Fennec? I've tried to sharing to Telegram, WhatsApp, Talon for Twitter and Twitter's official app. It fails in all of the proceeding apps and in the Twitter app simply tries to share the link to the image.

Comment 3

[Aaron Train [:aaronmt]]

Ah yes, it shares the link and not an image.

Comment 4

[Wesley Johnston (:wesj)]

Attachment: Patch

I removed this during quickshare because I didn't want to extend the prompt message just to add it, but thinking now, we already have the mimetype and we can just special case it.

I have a larger patch that revamps this getImageShareIntent code because its got some bugs. I'll upload that here too...

Comment 5

[Wesley Johnston (:wesj)]

Attachment: Patch 2

This is a bigger revamp. We're adding an IntentHelper class in the webactivities code, so this should gel well with it. This moves some (but not all) of the Intent code from GeckoAppShell into it. It also moves some of the file handling code from GeckoAppShell into FileUtils.java.

I removed some things we don't need anymore. For instance, some pre-Froyo base64 code in GeckoAppShell (I think we cut at Froyo now, right? We weren't using it for this shareImage code anyway...) The filename generation code from before was also pretty lacking. It only took extensions from the mimeType (and did it even if the mimetype was image/*. It also didn't grab filenames from urls. This does both, and will try and read the mimetype from data: uri's if possible. It also moves the special case for getting an image-share-intent vs normal-share-intent into the IntentHelper so that everyone can benefit from it.

I can split this up if you'd like.

Comment 6

[Paul [pwd]]

(In reply to Aaron Train [:aaronmt] from comment #3)
> Ah yes, it shares the link and not an image.

Note I've found that the only app that takes a link is Twitter. It simply fails in the others lists above.

Comment 7

[Paul [pwd]]

I'm unsure if it's a bug in itself or just a symptom of this bug, but attempting to share an image doesn't close the pop-up menu. It closes when sharing links though.

Comment 8

[Paul [pwd]]

32 is affected and thus I'd assume we're tracking those too? lsblakk/mfinkle?

Comment 9

[Wesley Johnston (:wesj)]

I started to play with this again. Currently all of the sharing is handled in Java. To do this, those java providers need to call into gecko to have it save the file, then return the new file url to java for sharing. It gets messy (but its a good usecase for having a way for JS to reply to java asynchronously...) Alternatively, me and mfinkle talked and decided the best way to do this is probably to just create a content provider for this. That way no one blocks the share intent from firing, and its up the the sharing app to request the full image/handle async feedback if they want/can handle it.

To make that work, we can fire up a content:// url ( maybe content://geckoImageCache/something?) as the share url, and register a content provider in java for them. Since queries to that content provider are actually sync, there may be some magic installed to make them appear sync while we asynchronously fetch the image from the cache. Java can then return different things (i.e. the file url or the image blob) based on what was requested from the content provider.

Comment 10

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

No need for this to track fx30. It would be nice to get this working, but sending the URL only is something other apps currently do and we won't have time to get the content provider idea working and uplifted to fx30.

Comment 11

[Paul [pwd]]

(In reply to Mark Finkle (:mfinkle) from comment #10)
> No need for this to track fx30. It would be nice to get this working, but
> sending the URL only is something other apps currently do and we won't have
> time to get the content provider idea working and uplifted to fx30.

From my testing, the only app that takes the URL is Twitter. Everything else just fails horribly. Also this is an actual feature which no other mobile browser has. I'd really like to champion getting this fixed instead of shipping a broken Firefox with a feature that suddenly just doesn't work. In bug 942270 comment 129, wesj says it's a simple fix to restore the old behaviour, can we not do that, prevent shipping with a broken feature and then there's no rush to get the reworking of the feature in place?

Comment 12

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

(In reply to Wesley Johnston (:wesj) from comment #4)
> Created attachment 8401088 [details] [diff] [review]
> Patch
> 
> I removed this during quickshare because I didn't want to extend the prompt
> message just to add it, but thinking now, we already have the mimetype and
> we can just special case it.

Wes - how isolated is this fix?
* Will we only call this code if someone taps on the "share" or quickshare for Images?
* It won't be called before the user taps on the share/quickshare so it won't block displaying the menu?

> I have a larger patch that revamps this getImageShareIntent code because its
> got some bugs. I'll upload that here too...

What bugs? I don't want to uplift Patch 2 to Aurora or Beta.

Comment 13

[Wesley Johnston (:wesj)]

(In reply to Mark Finkle (:mfinkle) from comment #12)
> Wes - how isolated is this fix?
> * Will we only call this code if someone taps on the "share" or quickshare
> for Images?
> * It won't be called before the user taps on the share/quickshare so it
> won't block displaying the menu?

Unfortunately, it does run before we show the menu. Looking at pushing later...
 
> What bugs? I don't want to uplift Patch 2 to Aurora or Beta.

These mostly dealt with the file name for the generated image:

1.) It only took extensions from the mimeType (it uses "*" if the type is "image/*"). We do that now, so we may need to fix it here :(
2.) It doesn't grab filenames from urls.
3.) It doesn't grab mimetypes from data-uris.

Comment 14

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

(In reply to Wesley Johnston (:wesj) from comment #13)

> > * It won't be called before the user taps on the share/quickshare so it
> > won't block displaying the menu?
> 
> Unfortunately, it does run before we show the menu. Looking at pushing
> later...

I think this is the only part that is blocking for me. I don't want to delay the menu and give the appearance that Fennec is not responsive. If we can get that changed, without too much complexity (thinking uplift), then I'd be OK with landing it.

Comment 15

[Wesley Johnston (:wesj)]

Attachment: Patch 1/2

This is kinda a hack, but it forces us to download the image before sharing (only if you actually try to share it though).

Comment 16

[Wesley Johnston (:wesj)]

Attachment: Patch 2/2

This does a better job getting the filename to use. I feel like we should take this as well. Otherwise, the new quickshare always uses a mimetype of "image/*" and with the original code we always share with a filename "image.*" (thats a real *, not a placeholder).

Comment 17

[Benjamin Kerensa [:bkerensa]]

(In reply to Paul [pwd] from comment #8)
> 32 is affected and thus I'd assume we're tracking those too? lsblakk/mfinkle?

If it is then yes we are indeed.

Comment 18

[Wesley Johnston (:wesj)]

Review ping?

Comment 19

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

Comment on attachment 8416343 [details] [diff] [review]
Patch 1/2

>diff --git a/mobile/android/base/GeckoAppShell.java b/mobile/android/base/GeckoAppShell.java
>+    /* Downloads the uri pointed to by a share intent, and lters the intent to point to the locally stored file.

lters -> alters

>+            final String type = intent.getType().replace("image/","");

nit: add space

("image/", "");


>+                int dataStart = src.indexOf(',');

nit: use " ?

>+                byte[] buf = Base64.decode(src.substring(dataStart+1), Base64.DEFAULT);

nit: use spaces

(dataStart + 1)

>diff --git a/mobile/android/base/widget/GeckoActionProvider.java b/mobile/android/base/widget/GeckoActionProvider.java

>+import org.mozilla.gecko.GeckoAppShell;

I always cry a little when we need to import GeckoAppShell (the kitchen sink) into a file. Didn't have a patch to move some of the intent stuff like sharing an image, into a separate file? Not for this bug, but maybe a nice refactor patch for later.

r+ with nits addressed

Comment 20

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

Comment on attachment 8416344 [details] [diff] [review]
Patch 2/2

I thought I reviewed this part too, but I guess I forgot to hit "submit"

>diff --git a/mobile/android/base/GeckoAppShell.java b/mobile/android/base/GeckoAppShell.java

>-                int dataStart = src.indexOf(',');
>+                final int dataStart = src.indexOf(',');

nit: use "

>+                // If we weren't given an explicit mimetype, try to dig one out of the data uri.
>+                if (TextUtils.isEmpty(extension) && dataStart > 5) {
>+                    type = src.substring(5, dataStart).replace(";base64", "");
>+                    extension = MimeTypeMap.getSingleton().getExtensionFromMimeType(type);
>+                }

This seems a little fragile, but let's see how it works in general.

Question: Should we check for an empty extension again and bail? Or is it OK to have an empty extension? Or shold we do something to force "image/*" ?

>+                // Onlhy alter the intent when we're sure everything has worked

"Only"

>+                    // Onlhy alter the intent when we're sure everything has worked

"Only"

r+, but answer the empty extension questions

Comment 21

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/fx-team/rev/f62600f021d9
https://hg.mozilla.org/integration/fx-team/rev/797584a984a1

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f62600f021d9
https://hg.mozilla.org/mozilla-central/rev/797584a984a1

Comment 23

[Paul [pwd]]

This fix is crashing for me.

Comment 24

[Aaron Train [:aaronmt]]

Thanks. I filed bug 1009560 and marked it blocking this bug.

Comment 25

[Lukas Blakk [:lsblakk] use ?needinfo]

Given the regression mentioned in comment 23 should we continue to track this for 30/31 uplift? or work on the regression in bug 1009560 and ride the trains?  We've got only one more beta for this sort of more speculative work to land in 30, then it will need to ride the trains from at least 31 depending on the risk.

Comment 26

[Wesley Johnston (:wesj)]

(In reply to Lukas Blakk [:lsblakk] from comment #25)
> Given the regression mentioned in comment 23 should we continue to track
> this for 30/31 uplift? or work on the regression in bug 1009560 and ride the
> trains?  We've got only one more beta for this sort of more speculative work
> to land in 30, then it will need to ride the trains from at least 31
> depending on the risk.

I wouldn't really consider this speculative work (we're mostly restoring old code, but its just running at a slightly different time/place here now). The fix should be simple. I'll put it together tonight.

Comment 28

[Wesley Johnston (:wesj)]

Comment on attachment 8416343 [details] [diff] [review]
Patch 1/2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 942270
User impact if declined: Sharing images shares the url, but not the image
Testing completed (on m-c, etc.): This has been on mc for a few days now. Working well I think. One fallout bug 1009560 that I will also nom.
Risk to taking this patch (and alternatives if risky): Pretty low risk. Restores some old code. We can ship the url share for now if we want. Its a regression, but most other browsers do the same thing.
String or IDL/UUID changes made by this patch: None.

Comment 29

[Wesley Johnston (:wesj)]

Comment on attachment 8416344 [details] [diff] [review]
Patch 2/2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 942270
User impact if declined: Sharing images shares the url, but not the image
Testing completed (on m-c, etc.): This has been on mc for a few days now. Working well I think. One fallout bug 1009560 that I will also nom.
Risk to taking this patch (and alternatives if risky): Pretty low risk. Restores some old code. We can ship the url share for now if we want. Its a regression, but most other browsers do the same thing.
String or IDL/UUID changes made by this patch: None.

Comment 30

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8416343 [details] [diff] [review]
Patch 1/2

We have two more mobile betas, let's get this into Beta 8.

Comment 31

[Cristina Madaras, QA [:CristinaM]]

Verified as fixed on 
Build: 32.0a1 (2014-05-26)
Device: Alcatel one touch 8008D (Android 4.1.2)

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2f69e76d4238
https://hg.mozilla.org/releases/mozilla-aurora/rev/4e7c0cef3305

https://hg.mozilla.org/releases/mozilla-beta/rev/f3247e4fa37d
https://hg.mozilla.org/releases/mozilla-beta/rev/4fa8558dd827

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/f3247e4fa37d
https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/4fa8558dd827

Comment 34

[Catalin Suciu [:csuciu]]

Sharing images is working correctly on Aurora 31.0a2 (2014-05-28) and Firefox 30 Beta 8