1201547 - Error pop-up does not appear when add-on install is blocked from direct link to XPI

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Add-on Manager, defect)

Description

[u421692]

1. Try to install an cross-origin add-on from the device

Expected result:
Error pop-up is displayed "add-on could not be installed because it is not compatible with Firefox"

Actual result:
Nothing happens

Regression window:
http://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=bf111f5f87d711abda967afd943e6bd6f4c88bbe&tochange=6012f4bbe2a973c4246e527889079392a2e48f9c

regressed from Bug 1185927

Comment 1

[:Margaret Leibovic]

Mihai, which add-on were you using to test this?

Comment 2

[u421692]

http://people.mozilla.org/~tmielczarek/crashme/
Downloaded the crashme.xpi
Copy crashme.xpi to device
Open Crashme.xpi from device in Firefox

Result:
Nothing happens

Comment 3

[:Margaret Leibovic]

(In reply to Mihai Pop from comment #2)
> http://people.mozilla.org/~tmielczarek/crashme/
> Downloaded the crashme.xpi
> Copy crashme.xpi to device
> Open Crashme.xpi from device in Firefox
> 
> Result:
> Nothing happens

I was able to install this without a problem. Did you try loading the .xpi in the urlbar directly? If so, you're probably seeing bug 1201534.

However, we should probably display some sort of error message, the way desktop does.

Mossop, where's the logic to handle showing that error message on desktop? Or if you're feeling really helpful, do you know why we're not catching a similar error on mobile?

Comment 4

[Dave Townsend [:mossop]]

(In reply to :Margaret Leibovic from comment #3)
> (In reply to Mihai Pop from comment #2)
> > http://people.mozilla.org/~tmielczarek/crashme/
> > Downloaded the crashme.xpi
> > Copy crashme.xpi to device
> > Open Crashme.xpi from device in Firefox
> > 
> > Result:
> > Nothing happens
> 
> I was able to install this without a problem. Did you try loading the .xpi
> in the urlbar directly? If so, you're probably seeing bug 1201534.
> 
> However, we should probably display some sort of error message, the way
> desktop does.
> 
> Mossop, where's the logic to handle showing that error message on desktop?
> Or if you're feeling really helpful, do you know why we're not catching a
> similar error on mobile?

Sorry I really should have patched mobile here. As well as listening for "addon-install-blocked" (https://dxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#5830) you should listen for "addon-install-origin-blocked". The desktop implementation just displays an error: https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser-addons.js#261

Comment 5

[Vivek Balakrishnan[:vivek]]

Attachment: MozReview Request: Bug 1201547 : Error popup for blocked addon install implemented r?margaret

Bug 1201547 : Error popup for blocked  addon install implemented  r?margaret

Comment 6

[Vivek Balakrishnan[:vivek]]

Comment on attachment 8673343 [details]
MozReview Request: Bug 1201547 : Error popup for blocked  addon install implemented  r?margaret

@Antlam : I am currently using the error message [1] for addon-install-origin-blocked scenario which is different both what is shown in Desktop [2] and what was expected here.

Please let me know if that is okay.

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/locales/en-US/chrome/browser.properties#99
[2] http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/browser.properties#22

Comment 7

[:Margaret Leibovic]

Comment on attachment 8673343 [details]
MozReview Request: Bug 1201547 : Error popup for blocked  addon install implemented  r?margaret

https://reviewboard.mozilla.org/r/21911/#review19723

This looks fine to me (with a few pieces of feedback), but it would also be good for Mossop to give it a quick look.

I would love to see some tests for this. We should see what kinds of tests desktop has for these notifications and see if we can create similar ones ourselves.

::: mobile/android/chrome/content/browser.js:5865
(Diff revision 1)
> -        if (!enabled) {
> +    switch (aTopic) {

Was this enabled check just unecessary? I assume so, since the add-on manager code does a similar check.

::: mobile/android/chrome/content/browser.js:5867
(Diff revision 1)
> +        NativeWindow.toast.show(Strings.browser.GetStringFromName("alertAddonsDownloading"), "short");

Nit: You could use the `strings` local variable that was declared above.

I actually feel like that local variable doesn't give us much, but I see it already exists, so I don't want to scope creep this patch to get rid of it.

::: mobile/android/chrome/content/browser.js:5871
(Diff revision 1)
> +          return;

When would we not have a tab here? It looks like the desktop code bails even earlier if there isn't a valid browser or tab (gBrowser.browsers is similar to our list of tabs):
https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser-addons.js#224

::: mobile/android/chrome/content/browser.js:5876
(Diff revision 1)
> +        let notificationName = "xpinstall-disabled";

I don't see this variable used anywhere, you should remove it (not your fault, looks like an existing bug).

Comment 8

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: prev_installerror.png

(In reply to Vivek Balakrishnan[:vivek] from comment #6)
> Comment on attachment 8673343 [details]
> MozReview Request: Bug 1201547 : Error popup for blocked  addon install
> implemented  r?margaret
> 
> @Antlam : I am currently using the error message [1] for
> addon-install-origin-blocked scenario which is different both what is shown
> in Desktop [2] and what was expected here.
> 
> Please let me know if that is okay.
> 
> [1]
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/locales/en-US/
> chrome/browser.properties#99
> [2]
> http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/
> browser/browser.properties#22

If I'm understanding this correctly...

I feel like this UI should leverage dialogs instead of our doorhangers - similar to bug 1170043. Can we reuse the UI? 

From a UX point of view, dialogs make more sense than doorhangers because this is more pressing and requires user attention.

Attaching the "error" dialog here (from bug 1170043). 

For non-compatible add-ons, we could say "The add-on could not be installed because it is not compatible."

Comment 9

[Vivek Balakrishnan[:vivek]]

Comment on attachment 8673343 [details]
MozReview Request: Bug 1201547 : Error popup for blocked  addon install implemented  r?margaret

Bug 1201547 : Error popup for blocked  addon install implemented  r?margaret

Comment 10

[Vivek Balakrishnan[:vivek]]

Screenshots 
https://www.dropbox.com/sh/6jgdx6ybe1vtf4f/AACPYoicDVr-HOQC4TubgSEba?dl=0

Comment 11

[:Margaret Leibovic]

(In reply to Vivek Balakrishnan[:vivek] from comment #10)
> Screenshots 
> https://www.dropbox.com/sh/6jgdx6ybe1vtf4f/AACPYoicDVr-HOQC4TubgSEba?dl=0

I don't think the "incompatible" add-on warning makes sense... shouldn't that message explain that the add-on installation was blocked?

I'd like for us to be able to uplift a patch for this bug to 43, so it would be great if we could have a fix that avoids adding a new string.

Sorry I missed this in my previous review. Could you upload an updated patch?

I think that it would be good to take some time to audit all the different error dialogs we show for add-on installation, but that's outside the scope of this bug, so let's just focus on addressing this issue of a missing error dialog here.

Comment 12

[Vivek Balakrishnan[:vivek]]

Comment on attachment 8673343 [details]
MozReview Request: Bug 1201547 : Error popup for blocked  addon install implemented  r?margaret

Bug 1201547 : Error popup for blocked  addon install implemented  r?margaret

Comment 13

[:Margaret Leibovic]

(In reply to Vivek Balakrishnan[:vivek] from comment #12)
> Comment on attachment 8673343 [details]
> MozReview Request: Bug 1201547 : Error popup for blocked  addon install
> implemented  r?margaret
> 
> Bug 1201547 : Error popup for blocked  addon install implemented  r?margaret

Thanks for the update, this looks good! Let's land this and request uplift to 43 (assuming there are no problems when we land it).

Comment 14

[Vivek Balakrishnan[:vivek]]

https://hg.mozilla.org/integration/fx-team/rev/69f447f55be7075795ce2ef7d3add7439a8ae894
Bug 1201547 : Error popup for blocked  addon install implemented  r=margaret

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/69f447f55be7

Comment 17

[Teodora Vermesan (:TeoVermesan)]

Attachment: Screenshot_2015-10-28-09-52-32.png

Tested using:
Build: Firefox 44.0a1 (2015-10-27)
Device: Prestigio PSP5508 (Android 4.4.2)