1336085 - Don't display pending update if there are no new promptable permissions

Status: VERIFIED FIXED

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

Description

[Vasilica Tamas, QA [:vtamas]]

[Affected versions]:
Firefox 53.0a2 (2017-02-02)
Firefox 54.0a1 (2017-02-02)

[Affected platforms]:
Windows 10 64-bit
Ubuntu 16.04 32-bit


[Steps to reproduce]:
1.Launch Firefox with clean profile.
2.Create extensions.webextPermissionPrompts and set it to true.
3.Create xpinstall.signatures.dev-root and set it to true.
4.Change extensions.update.background.url to point to dev server and set the update interval (extensions.update.interval) to “1”. 
5.Install the first version of the following webextension: https://addons-dev.allizom.org/en-US/firefox/addon/webext102/versions/
6.Restart the browser. 


[Expected Results]:
The new permissions are mentioned in update installation pop-up.


[Actual Results]:
There are no permissions mentioned in update installation pop-up.
See screenshot: https://www.screencast.com/t/31YVKeob

[Additional Notes]
The following permissions were added to the second version: "storage", "idle", "alarms", "contextMenus".

Comment 1

[Andrew Swan [:aswan]]

When designing the permissions copy, it was a deliberate decision not to include everything in the prompts.
See the doc linked from bug 1316996 for a comprehensive list of which permissions get prompts.

Comment 2

[Florian Quèze [:florian]]

(In reply to Andrew Swan [:aswan] from comment #1)
> When designing the permissions copy, it was a deliberate decision not to
> include everything in the prompts.
> See the doc linked from bug 1316996 for a comprehensive list of which
> permissions get prompts.

Andrew, see the screenshot from the original description: https://www.screencast.com/t/31YVKeob

It doesn't make sense to tell the user "You must approve new permissions" and not list any permission. Should the update be applied automatically in that case?

Comment 3

[Andrew Swan [:aswan]]

(In reply to Florian Quèze [:florian] [:flo] from comment #2)
> It doesn't make sense to tell the user "You must approve new permissions"
> and not list any permission. Should the update be applied automatically in
> that case?

Yep, thanks for catching that.  Updating the title to clarify the scenario.

Comment 4

[Andrew Swan [:aswan]]

Attachment: Bug 1336085 Apply updates with only non-promptable permissions automatically

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

Comment 5

[Andrew Swan [:aswan]]

This got messy and it required not using <stringbundle> any more (since we need to look up strings when considering an update when we might not have an open browser window).  But I think this is good to go now.

Comment 6

[Dave Townsend [:mossop]]

Comment on attachment 8834555 [details]
Bug 1336085 Apply updates with only non-promptable permissions automatically

https://reviewboard.mozilla.org/r/110442/#review111784

::: browser/modules/ExtensionsUI.jsm:72
(Diff revision 1)
>        }
>      });
>    },
>  
> -  showAddonsManager(browser, info) {
> -    let loadPromise = new Promise(resolve => {
> +  showAddonsManager(browser, strings, icon) {
> +    let tab = browser.addTab();

You shouldn't need to add a new tab, BrowserOpenAddonsMgr will do it if necessary.

::: browser/modules/ExtensionsUI.jsm:182
(Diff revision 1)
>  
> -    let win = target.ownerGlobal;
> +    let bundle = Services.strings.createBundle(BROWSER_PROPERTIES);
>  
>      let name = info.addon.name;
>      if (name.length > 50) {
>        name = name.slice(0, 49) + "…";

I know you didn't add this, but is this correct from an l10n standpoint? Does it work for RTL for example?

Comment 7

[Andrew Swan [:aswan]]

Comment on attachment 8834555 [details]
Bug 1336085 Apply updates with only non-promptable permissions automatically

https://reviewboard.mozilla.org/r/110442/#review111784

> You shouldn't need to add a new tab, BrowserOpenAddonsMgr will do it if necessary.

Hm, I did that so I could have a handle on the tab (actually on its <browser> which was needed right below.  Can I just call BrowserOpenAddonsMgr() and then synchronously read `browser.selectedTab` or `.selectedBrowser` and have that be safe?

> I know you didn't add this, but is this correct from an l10n standpoint? Does it work for RTL for example?

I don't know, who is the right person to ask?

Comment 8

[Florian Quèze [:florian]]

(In reply to Andrew Swan [:aswan] from comment #7)

> > I know you didn't add this, but is this correct from an l10n standpoint? Does it work for RTL for example?
> 
> I don't know, who is the right person to ask?

Ah, I had seen this hardcoded ellipsis, wanted to comment, and then forgot :-(.

See http://searchfox.org/mozilla-central/search?q=intl.ellipsis for what other code does.

And flod is the right person to ask.

Comment 9

[Andrew Swan [:aswan]]

Comment on attachment 8834555 [details]
Bug 1336085 Apply updates with only non-promptable permissions automatically

https://reviewboard.mozilla.org/r/110442/#review111784

> Hm, I did that so I could have a handle on the tab (actually on its <browser> which was needed right below.  Can I just call BrowserOpenAddonsMgr() and then synchronously read `browser.selectedTab` or `.selectedBrowser` and have that be safe?

Also, looking more closely, BrowserOpenAddonsMgr() will "take over" any existing about:addons tab if there is one which, if I understand the code right, might even entail switching focus to a different window...  I don't feel strongly about this, but I think it would surprise me as a user.

Comment 10

[Francesco Lodolo [:flod]]

Concatenating in this case shouldn't be an issue, even for RTL (I also expect the name of an add-on to use Latin alphabet most of the time). 

Using intl.ellipsis instead of hard-coding the … would be even better. I'm only aware of Japanese using ... instead …, so 3 letters instead of 1, but given that Japanese is a lot denser that English, it shouldn't really create problems with the layout.

Comment 11

[Andrew Swan [:aswan]]

Comment on attachment 8834555 [details]
Bug 1336085 Apply updates with only non-promptable permissions automatically

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

Comment 12

[Andrew Swan [:aswan]]

Filed bug 1337870 to handle the ellipsis issue separately.
Dave, the changes to avoid creating a tab from ExtensionsUI.jsm ended up being bigger than I expected, even though you already gave r+ can you do a quick sanity check?

Comment 13

[Dave Townsend [:mossop]]

Comment on attachment 8834555 [details]
Bug 1336085 Apply updates with only non-promptable permissions automatically

https://reviewboard.mozilla.org/r/110442/#review112090

::: browser/base/content/test/general/browser_extension_update.js:378
(Diff revisions 1 - 2)
> +
> +add_task(function*() {
> +  // Return to about:blank
> +  gBrowser.selectedBrowser.loadURI("about:blank");
> +  yield BrowserTestUtils.browserLoaded(gBrowser.selectedBrowser);
> +});

Can we do this in a cleanup function instead so it happens even if the test times out?

::: browser/modules/ExtensionsUI.jsm:73
(Diff revisions 1 - 2)
>      });
>    },
>  
>    showAddonsManager(browser, strings, icon) {
> -    let tab = browser.addTab();
> -    browser.selectedTab = tab;
> +    let global = browser.selectedBrowser.ownerGlobal;
> +    return global.BrowserOpenAddonsMgr("addons://list/extension").then(aomWin => {

Can you file a bug to make BrowserOpenAddonsMgr always return the window.

Comment 14

[Andrew Swan [:aswan]]

Comment on attachment 8834555 [details]
Bug 1336085 Apply updates with only non-promptable permissions automatically

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

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b423754c7ada
Apply updates with only non-promptable permissions automatically r=mossop

Comment 16

[Carsten Book [:Tomcat]]

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

Comment 17

[Vasilica Tamas, QA [:vtamas]]

Verified fixed on Firefox 54.0a1 (2017-02-21/22) under Windows 10 64-bit, Mac OS X 10.12.1 and Ubuntu 16.04 32-bit. Pending update is not displayed while upgrading via background method a webextension with no new promptable permissions. This issue will be verified also using interactive updates after the Bug 1340078 will be fixed.