1170841 - Show warning in add-on manager for add-ons that aren't properly signed

Status: VERIFIED FIXED

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

Description

[:Margaret Leibovic]

Android version of bug 1149702.

antlam, do you have any ideas about a (super simple) indicator we could use on add-ons that aren't properly signed?

Comment 1

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

Attachment: prev_aboutaddons.png

Indeed! we could use the same icon (as the dialog) and place it to the right hand side (not unlike the styling for the Home panels layouts I've been working on)

Comment 2

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

Attachment: icon_greycaution.svg

Here's the icon in .SVG

Comment 3

[:Margaret Leibovic]

I think we should also include a message in the details page that explains why this add-on is disabled, since it's not clear what the warning icon there means.

On desktop, there's also a warning even if we don't force these add-ons to be disabled. We should look at the destkop about:addons UI for inspiration here.

Comment 4

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

Attachment: prev_aboutaddons2.png

Agree, I had this put aside for what happens when the user presses into an add-on. The copy is mostly taken from what's currently in desktop preferences. I made it more concise cause I think it's a bit dated by now.

Comment 5

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

^ Also, I noticed something weird going on with the buttons (uninstall and install) in that page. That's not in the design, I just moved the image around for the mock. 

We should just clean up the strokes and corners to be consistent.

Comment 6

[:Margaret Leibovic]

(In reply to Anthony Lam (:antlam) from comment #5)
> ^ Also, I noticed something weird going on with the buttons (uninstall and
> install) in that page. That's not in the design, I just moved the image
> around for the mock. 
> 
> We should just clean up the strokes and corners to be consistent.

Can you elaborate more on what's going wrong with the buttons? We can file a separate bug for this, but I'm happy to clean things up while I'm in here.

I think this design looks good, I'll get to working on this. But just FYI, if add-on signing is required, the "Disable" button in this case would actually be "Enable", and it would be disabled, since we wouldn't allow the user to change that state.

Comment 7

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

Done! Filed bug 1175197 for the clean up.

Comment 8

[:Margaret Leibovic]

Attachment: MozReview Request: Bug 1170841 - Show warning in add-on manager for add-ons that aren't properly signed. r=liuche

Bug 1170841 - Show warning in add-on manager for add-ons that aren't properly signed. r=liuche

Comment 9

[:Margaret Leibovic]

Attachment: screenshot

Comment 10

[Chenxia Liu [:liuche]]

Comment on attachment 8623985 [details]
MozReview Request: Bug 1170841 - Show warning in add-on manager for add-ons that aren't properly signed. r=liuche

https://reviewboard.mozilla.org/r/11639/#review10409

::: mobile/android/chrome/content/aboutAddons.js:237
(Diff revision 1)
> +    item.setAttribute("isUnsigned", aAddon.signedState <= AddonManager.SIGNEDSTATE_MISSING);

So did this used to display a "corrupt file" error for the addon (instead of "missing)?

It seems like this possible null value should be checked explicitly in the AddonInstall code when checking for SIGNEDSTATE_MISSING, instead of being implicitly checked by a null <= SIGNEDSTATE_MISSING and then falling through to an ERROR_CORRUPT_FILE - the caller shouldn't need to explicitly set it.

http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#5472

However, this seems much less low risk because it only touches code here, so I'm also okay with that.

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/e796ac2a6f7a

Comment 12

[:Margaret Leibovic]

(In reply to Chenxia Liu [:liuche] from comment #10)
> Comment on attachment 8623985 [details]
> MozReview Request: Bug 1170841 - Show warning in add-on manager for add-ons
> that aren't properly signed. r=liuche
> 
> https://reviewboard.mozilla.org/r/11639/#review10409
> 
> ::: mobile/android/chrome/content/aboutAddons.js:237
> (Diff revision 1)
> > +    item.setAttribute("isUnsigned", aAddon.signedState <= AddonManager.SIGNEDSTATE_MISSING);
> 
> So did this used to display a "corrupt file" error for the addon (instead of
> "missing)?

I don't think I understand the question... do you mean did we display some message about the add-on if it had another problem? Our about:addons logic is pretty bare-bones, so I don't think we displayed any message about add-on problems.

> It seems like this possible null value should be checked explicitly in the
> AddonInstall code when checking for SIGNEDSTATE_MISSING, instead of being
> implicitly checked by a null <= SIGNEDSTATE_MISSING and then falling through
> to an ERROR_CORRUPT_FILE - the caller shouldn't need to explicitly set it.
> 
> http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/
> internal/XPIProvider.jsm#5472

I basically just copied this logic from the desktop implementation, and I believe the idea here is to display a generic error to the user any time there's a problem with signing, so that's why we don't have more nuanced messages for the different types of signing error states.

We can check with Mossop to see if this is something that we would ever want to change.

Comment 13

[Dave Townsend [:mossop]]

(In reply to :Margaret Leibovic from comment #12)
> (In reply to Chenxia Liu [:liuche] from comment #10)
> > Comment on attachment 8623985 [details]
> > MozReview Request: Bug 1170841 - Show warning in add-on manager for add-ons
> > that aren't properly signed. r=liuche
> > 
> > https://reviewboard.mozilla.org/r/11639/#review10409
> > 
> > ::: mobile/android/chrome/content/aboutAddons.js:237
> > (Diff revision 1)
> > > +    item.setAttribute("isUnsigned", aAddon.signedState <= AddonManager.SIGNEDSTATE_MISSING);
> > 
> > So did this used to display a "corrupt file" error for the addon (instead of
> > "missing)?
> 
> I don't think I understand the question... do you mean did we display some
> message about the add-on if it had another problem? Our about:addons logic
> is pretty bare-bones, so I don't think we displayed any message about add-on
> problems.
> 
> > It seems like this possible null value should be checked explicitly in the
> > AddonInstall code when checking for SIGNEDSTATE_MISSING, instead of being
> > implicitly checked by a null <= SIGNEDSTATE_MISSING and then falling through
> > to an ERROR_CORRUPT_FILE - the caller shouldn't need to explicitly set it.
> > 
> > http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/
> > internal/XPIProvider.jsm#5472
> 
> I basically just copied this logic from the desktop implementation, and I
> believe the idea here is to display a generic error to the user any time
> there's a problem with signing, so that's why we don't have more nuanced
> messages for the different types of signing error states.
> 
> We can check with Mossop to see if this is something that we would ever want
> to change.

Yeah we decided that the user didn't need to know what the specific problem was, just that the add-ons wasn't signed properly. They can't really act on anything more specific than that.

Note that if an add-on doesn't need to be signed its signedState is undefined, not null and (undefined <= SIGNEDSTATE_MISSING) === false so things should work out fine.

Comment 14

[:Margaret Leibovic]

Looks like this missed getting closed...

http://hg.mozilla.org/mozilla-central/rev/e796ac2a6f7a

Comment 15

[u421692]

Verified as fixed on Firefox 41 Beta 1