1298989 - Remove the the undo option for theme install on discovery.addons.mozilla.org

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect, P3)

Description

[Andy McKay]

Based on the comments in bug 1286985 it feels to me pretty clear that the "undo" dialog for installing themes from the disco pane is not very useful.

There's quite a bit of work and UX we can do in that bug, but this is just to suppress that dialog for certain sites. Let's start with discovery.addons.mozilla.org (and the dev and stage sites) and aim to expand it where we can. The reasoning being that on the disco pane, there is an undo right there where the user pressed the button. Something that never existed before.

Comment 1

[Andrew Swan [:aswan]]

So can we close bug 1284561 in favor of this?  (they're subtly different, that one suggests removing the undo bar upon uninstall, this one is to never show it in the first place.  the latter sounds both better and easier to do)

Comment 3

[Andrew Swan [:aswan]]

Attachment: Bug 1298989 Don't show notification on theme install from discovery pane

Review commit: https://reviewboard.mozilla.org/r/77154/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/77154/

Comment 4

[Andrew Swan [:aswan]]

Comment on attachment 8788755 [details]
Bug 1298989 Don't show notification on theme install from discovery pane

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77154/diff/1-2/

Comment 5

[Andrew Swan [:aswan]]

The attached patch on reviewboard gets the job done but it is not suitable for landing as-is given the clunky handling of the origin.  I could use some guidance from the front-end folks though about how to best manage that.  Existing code uses Services.perms to check for the ability to use xpinstall in the first place.  To follow that pattern would mean creating a new type of permission ("notificationless-theme-install"?  ugh).  Or we could hard-code the list in the source as we do at http://searchfox.org/mozilla-central/rev/95b0ecf4b59a01e0524ca02f6c96ecaabe38f4d5/toolkit/mozapps/extensions/AddonManager.jsm#70-76 and http://searchfox.org/mozilla-central/rev/95b0ecf4b59a01e0524ca02f6c96ecaabe38f4d5/toolkit/mozapps/extensions/AddonManager.jsm#2913-2919  Or maybe there's something more elegant that I'm overlooking.

jaws, you're a Firefox peer and you touched other nearby code recently, care to weigh in?  or if not, to redirect to somebody more appropriate?

Comment 6

[Andrew Swan [:aswan]]

Oops, looks like I neglected to actually set ni? as part of the last comment.
See above.

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8788755 [details]
Bug 1298989 Don't show notification on theme install from discovery pane

https://reviewboard.mozilla.org/r/77154/#review79400

::: browser/base/content/browser-addons.js:501
(Diff revision 2)
> +      let notify = url.origin != "https://discovery.addons.mozilla.org";
> +      this._install(data, notify);

Please include a comment here saying,

"Don't show the notification bar for discovery.addons.mozilla.org since it is only used internally by the add-ons manager which includes it's own toggle switch for Undo (and managing)."

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Andrew Swan [:aswan] from comment #6)
> Oops, looks like I neglected to actually set ni? as part of the last comment.
> See above.

The patch seems reasonable and gets the job done without sacrificing too much. It will be easy to undo or enhance in the future if need be. I'm OK with taking it as-is plus the requested comment above.

Comment 9

[Andrew Swan [:aswan]]

Comment on attachment 8788755 [details]
Bug 1298989 Don't show notification on theme install from discovery pane

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77154/diff/2-3/

Comment 10

[Andrew Swan [:aswan]]

Comment on attachment 8788755 [details]
Bug 1298989 Don't show notification on theme install from discovery pane

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77154/diff/3-4/

Comment 11

[Andrew Swan [:aswan]]

Jared, a couple of notes:
- the first patch inadvertently disabled post-install notifications on all non-AMO sites, that's fixed here :-)
- I wasn't clear about this originally but the test for whether to suppress the notification is a little more complicated than just checking against a single origin, I put the logic in-line here, if you think that handling that information elsewhere is preferable, I'm happy to do that as well but could use some guidance on how to do it.

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8788755 [details]
Bug 1298989 Don't show notification on theme install from discovery pane

https://reviewboard.mozilla.org/r/77154/#review87124

Comment 13

[Andrew Swan [:aswan]]

Comment on attachment 8788755 [details]
Bug 1298989 Don't show notification on theme install from discovery pane

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77154/diff/4-5/

Comment 14

[Andrew Swan [:aswan]]

Clean try run for latest revision:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=277688afa8250a09db64237d4618981175848c36

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ea865d058776
Don't show notification on theme install from discovery pane r=jaws

Comment 16

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/ea865d058776

Comment 17

[Valentina Virlics [:vvalentina], VPN QA]

Verified as fixed on FF52.0a1(Build 20161026030210)and FF53.0a1 using Win 7.
The notification while installing a theme from about:addons is not longer displayed.
Postfix video: http://screencast.com/t/uC5XWl46Rx