Closed Bug 1201547 Opened 4 years ago Closed 4 years ago

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

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 44
Tracking Status
firefox43 --- affected
firefox44 --- verified
fennec 43+ ---

People

(Reporter: u421692, Assigned: vivek)

References

Details

(Keywords: regression)

Attachments

(3 files)

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
tracking-fennec: --- → ?
tracking-fennec: ? → 43+
Assignee: nobody → margaret.leibovic
Mihai, which add-on were you using to test this?
Flags: needinfo?(mihai.g.pop)
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
Flags: needinfo?(mihai.g.pop)
(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?
Flags: needinfo?(dtownsend)
Summary: Error pop-up is not generated for an add-on that is not compatible → Error pop-up does not appear when add-on install is blocked from direct link to XPI
(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
Flags: needinfo?(dtownsend)
Bug 1201547 : Error popup for blocked  addon install implemented  r?margaret
Attachment #8673343 - Flags: review?(margaret.leibovic)
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
Flags: needinfo?(alam)
Attachment #8673343 - Flags: feedback?(dtownsend)
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).
Attachment #8673343 - Flags: review?(margaret.leibovic) → review+
Attached image 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."
Flags: needinfo?(alam) → needinfo?(vivekb.balakrishnan)
Assignee: margaret.leibovic → vivekb.balakrishnan
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
Attachment #8673343 - Flags: feedback?(dtownsend)
Screenshots 
https://www.dropbox.com/sh/6jgdx6ybe1vtf4f/AACPYoicDVr-HOQC4TubgSEba?dl=0
Flags: needinfo?(vivekb.balakrishnan)
(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.
Flags: needinfo?(vivekb.balakrishnan)
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
(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).
https://hg.mozilla.org/integration/fx-team/rev/69f447f55be7075795ce2ef7d3add7439a8ae894
Bug 1201547 : Error popup for blocked  addon install implemented  r=margaret
https://hg.mozilla.org/mozilla-central/rev/69f447f55be7
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
Depends on: 1217016
Duplicate of this bug: 1192784
Depends on: 1218129
Tested using:
Build: Firefox 44.0a1 (2015-10-27)
Device: Prestigio PSP5508 (Android 4.4.2)
Flags: needinfo?(vivekb.balakrishnan)
You need to log in before you can comment on or make changes to this bug.