760708 - Add ability to remove shortcuts

Status: RESOLVED INCOMPLETE

(Firefox for Android Graveyard :: General, defect)

Description

[Wesley Johnston (:wesj)]

Attachment: Patch

I was digging through some code the other day and saw that there's an Intent for removing homescreen short cuts. We should use it! It basically matches in url (or the intent the shortcut is associated with) and name of the shortcut.

I tried to hook this up, but apps don't provide us with the name of the app after it is uninstalled. So I'm hoping to loop some of them in to see if they can help?

Comment 1

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

http://code.google.com/p/android-launcher-plus/source/browse/trunk/src/mobi/intuitit/android/p/launcher/UninstallShortcutReceiver.java?r=4#40

Looks like it will remove all duplicates by default, which is a good thing.

From the docs & posts I have seen online, the shortcutIntent needs to match the one used to create the shortcut. Looks like you missed adding the setAction part:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoAppShell.java#785

Not sure f this is really needed or not.

Comment 2

[Jason Smith [:jsmith]]

(In reply to Wesley Johnston (:wesj) from comment #0)
> Created attachment 629368 [details]
> Patch
> 
> I was digging through some code the other day and saw that there's an Intent
> for removing homescreen short cuts. We should use it! It basically matches
> in url (or the intent the shortcut is associated with) and name of the
> shortcut.
> 
> I tried to hook this up, but apps don't provide us with the name of the app
> after it is uninstalled. So I'm hoping to loop some of them in to see if
> they can help?

During an uninstall of a specific app, wouldn't you already be able to figure that information out (the name of the app and the origin + launch path of the app)? I would think you could get that information off the app object.

https://developer.mozilla.org/en/DOM/App

Comment 3

[Wesley Johnston (:wesj)]

We have info when WE are uninstalling the app (i.e. about:apps). Is that the only place where un-installation can happen? I looked through http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.js briefly to try and find a notification we could receive when apps were uninstalled, but I may have missed something.

Comment 4

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

Wes - Can't you listen for "webapps-sync-uninstall"? We already listen for "webapps-sync-install".

Comment 5

[Wesley Johnston (:wesj)]

Yeah. That's what I tried. For install we have to get the manifest by using:

DOMApplicationRegistry.getManifestFor(data.origin, callback);

For uninstall this will return null to the callback (i.e. the manifest is gone by this point).

Comment 6

[Wesley Johnston (:wesj)]

Attachment: Patch

Might as well review these bits for now. Added back in the type stuff.

Comment 7

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

Comment on attachment 630387 [details] [diff] [review]
Patch

This looks OK, but we should hook it up to about:apps so we can actually test it out.

We can worry about Apps In The Cloud uninstalling apps later. And we can make sure a pre-uninstall notification is fired too.

Comment 8

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

Also note that Brubeck is killing nsIShellService. I think he'll be able to unbitrot his patch without too much trouble.

Comment 9

[Matt Brubeck (:mbrubeck)]

(In reply to Mark Finkle (:mfinkle) from comment #8)
> Also note that Brubeck is killing nsIShellService. I think he'll be able to
> unbitrot his patch without too much trouble.

If you're curious, that change is in attachment 622564 [details] [diff] [review] from bug 753574.  It replaces the nsIShellService method with a JSON message.

Comment 10

[Jason Smith [:jsmith]]

(In reply to Mark Finkle (:mfinkle) from comment #7)
> Comment on attachment 630387 [details] [diff] [review]
> Patch
> 
> This looks OK, but we should hook it up to about:apps so we can actually
> test it out.

Could you file a bug for this?

Comment 11

[Wesley Johnston (:wesj)]

Attachment: Without shell service

Oh. Didn't notice this was r+ and refactored to remove the ShellService stuff.

Comment 12

[Wesley Johnston (:wesj)]

Attachment: Use in about:apps

This uses this in about:apps. in onuninstall, we also don't have the manifest anymore. i.e. the third argument here is null http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.js#366 . So instead, I had to do this in the context menu handler.

Comment 13

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/eb7306640e5f
https://hg.mozilla.org/integration/mozilla-inbound/rev/762be4801739

Comment 14

[Wesley Johnston (:wesj)]

Dang it. Multiple patches are not my forte. This got lost in the pushing:

https://hg.mozilla.org/integration/mozilla-inbound/rev/d529eceaccfa

Comment 15

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

I just realized that we do cache the "app" object and the "manifest" object in the DOM element:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/aboutApps.js#102

So we might be able to move this to "onUninstall"

Comment 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/762be4801739
https://hg.mozilla.org/mozilla-central/rev/eb7306640e5f
https://hg.mozilla.org/mozilla-central/rev/d529eceaccfa

Comment 17

[Aaron Train [:aaronmt]]

So, I',m running an inbound from this afternoon (06/07) I've added a few apps via (testmanifest.com), and added home-screen shortcuts, on uninstallation they are not removed on my home-screen. Am I missing something?

Comment 18

[Jason Smith [:jsmith]]

(In reply to Aaron Train [:aaronmt] from comment #17)
> So, I',m running an inbound from this afternoon (06/07) I've added a few
> apps via (testmanifest.com), and added home-screen shortcuts, on
> uninstallation they are not removed on my home-screen. Am I missing
> something?

Agreed - same behavior on my galaxy nexus phone - I get a message saying the shortcut was removed, but the shortcut is still on my phone. This patch also causes a regression in about:apps - doing an uninstall does not remove the app visually from about:apps until you reload the page. Reopening.

Comment 19

[Jason Smith [:jsmith]]

Logcat below. Looks like something is indeed failing out here:

06-11 09:57:48.049: I/GeckoApp(23074): Got message: Shortcut:Remove
06-11 09:57:48.073: W/GeckoAppShell(23074): removeShortcut for http://mozilla.clock6.com/ [World Clocks] > webapp
06-11 09:57:48.073: W/InputManagerService(181): Window already focused, ignoring focus gain of: com.android.internal.view.IInputMethodClient$Stub$Proxy@41a7bf10
06-11 09:57:48.128: E/GeckoConsole(23074): [JavaScript Error: "NS_ERROR_FAILURE: Failure arg 0 [nsIDOMEventListener.handleEvent]" {file: "jar:jar:file:///data/app/org.mozilla.fennec-2.apk!/omni.ja!/components/Webapps.js" line: 367}]
06-11 09:57:48.471: I/Launcher(469): setLoadOnResume
06-11 09:57:48.495: I/Launcher(469): setLoadOnResume
06-11 09:57:48.526: I/Launcher(469): setLoadOnResume
06-11 09:57:48.526: I/Launcher(469): setLoadOnResume
06-11 09:57:48.573: I/Launcher(469): setLoadOnResume
06-11 09:57:48.588: I/Launcher(469): setLoadOnResume
06-11 09:57:48.596: I/Launcher(469): setLoadOnResume
06-11 09:57:48.612: I/Launcher(469): setLoadOnResume
06-11 09:57:48.635: I/Launcher(469): setLoadOnResume
06-11 09:57:48.651: I/Launcher(469): setLoadOnResume
06-11 09:57:48.659: I/Launcher(469): setLoadOnResume
06-11 09:57:48.682: I/Launcher(469): setLoadOnResume
06-11 09:57:48.729: I/Launcher(469): setLoadOnResume
06-11 09:57:48.799: D/dalvikvm(469): GC_FOR_ALLOC freed 4147K, 36% free 22283K/34567K, paused 37ms
06-11 09:57:48.854: I/Launcher(469): setLoadOnResume

Comment 20

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

Webapps.js" line: 367 is:
http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.js#367

So I assume something is failing in our onuninstall handler:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/aboutApps.js#125

Comment 21

[Wesley Johnston (:wesj)]

Can you list devices? This works fine for me on at least two.

Comment 22

[Wesley Johnston (:wesj)]

Also, are the particular apps you're installing and seeing problems. I've tried with BarFight installed from https://apps.mozillalabs.com/appdir/ and World Clock from https://marketplace.mozilla.org/.

The other error seems unrelated, but I can't reproduce it either.

Comment 23

[Jason Smith [:jsmith]]

(In reply to Wesley Johnston (:wesj) from comment #21)
> Can you list devices? This works fine for me on at least two.

Still getting a reproduction on a Samsung Galaxy Nexus. Aaron - Can you still reproduce and on what devices?

Comment 24

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Grabbing this from Wes -- I've got some changes/fixes here that should resolve this and a few other bits.

Comment 25

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

So with my patch in bug 753102, we remove the shortcut (I believe the failure before had to do with a different launchURL vs. origin; fixed in the patch to be consistent), but something still fails:

E/GeckoConsole(15777): [JavaScript Error: "NS_ERROR_FAILURE: Failure arg 0 [nsIDOMEventListener.handleEvent]" {file: "jar:jar:file:///data/app/org.mozilla.fennec_vladimir-2.apk!/omni.ja!/components/Webapps.js" line: 367}]

that's the handleEvent call in:

      case "Webapps:Uninstall:Return:OK":
        if (this._onuninstall) {
          let event = new WebappsApplicationEvent(new WebappsApplication(this._window, msg.origin, null, null, null, null, 0));
          this._onuninstall.handleEvent(event);
        }
        break;

This is causing the app to not be removed from about:apps.  I'm not sure yet why this is happening.

Comment 26

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

From some reading -- it seems that UNINSTALL_SHORTCUT may be broken on ICS, though I've definitely seen it work.  Still trying to figure out what the conditions for breakage are... I get the toast notification that it uninstalled a shortcut, but the shortcut doesn't go away.  (If I use the wrong title or something I get no notification.)

Comment 27

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