857868 - Convert storage quota notifications to a doorhanger panel

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Mark Hammond [:markh] [:mhammond]]

Bug 588240 is calling for all site based notification bars to be converted to doorhanger panels.  The notification for offline storage quota being exceeded for a site is currently displayed in a notification bar, so should be converted to a panel.

Comment 1

[Mark Hammond [:markh] [:mhammond]]

Attachment: Convert storage quota notifications to a doorhanger panel

This patch is ontop of the patch in bug 588305 (but is largely independent from it).

There seems to be no tests for this, so I created a fairly simple one.  However, I failed to get a mochitest-plain test working due to issues when running under the test runner - when running just the single test it all worked, but when running the test suite, _getBrowserForCacheUpdate() failed to locate the correct page - it only saw the test runner's URL, not the URL of the specific test.  For this reason the test is mochitest-browser (which actually makes the test quite a bit simpler, at the cost of an extra test file).  Also, FWIW, the test doesn't attempt to use iframes like the offlineNotification test does due to bug 857897.

Finally, this patch uses 'let anchorID = "webapps-notification-icon";', but it should probably use the same as bug 588305 - see bug 588305 comment 22.

Comment 2

[Mark Hammond [:markh] [:mhammond]]

green try at https://tbpl.mozilla.org/?tree=Try&rev=ef0ade9b041a

Comment 3

[Mark Hammond [:markh] [:mhammond]]

taking this bug, and now sounds like a good opportunity for a review-ping for MattN.

Comment 4

[Matthew N. [:MattN]]

Comment on attachment 733714 [details] [diff] [review]
Convert storage quota notifications to a doorhanger panel

Review of attachment 733714 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for writing a test :)
Was the jetpack failure on TBPL unrelated?

::: browser/base/content/browser.js
@@ +5962,5 @@
> +    let message = gNavigatorBundle.getFormattedString("offlineApps.usage",
> +                                                      [ aURI.host,
> +                                                        warnQuota / 1024 ]);
> +
> +    let anchorID = "webapps-notification-icon";

As you mentioned, the indexedDB anchor is better (IMO).

::: browser/base/content/test/browser_offlineQuotaNotification.js
@@ +2,5 @@
> + * Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/
> + */
> +
> +// Test offline quota warnings - must be run as a mochitest-browser test else

…or else the test runner gets in the way of notifications due to bug 857897. (assuming my understanding is correct that the test runner issue is because it uses an iframe)

@@ +22,5 @@
> +    gBrowser.selectedBrowser.removeEventListener("load", onload, true);
> +    gBrowser.selectedBrowser.contentWindow.applicationCache.oncached = function() {
> +      executeSoon(function() {
> +        // We got cached - now we should have provoked the quota warning.
> +        let win = Services.wm.getMostRecentWindow("navigator:browser");

|win| is unused

@@ +23,5 @@
> +    gBrowser.selectedBrowser.contentWindow.applicationCache.oncached = function() {
> +      executeSoon(function() {
> +        // We got cached - now we should have provoked the quota warning.
> +        let win = Services.wm.getMostRecentWindow("navigator:browser");
> +        let notifications = PopupNotifications._currentNotifications;

tsk-tsk for using private members ;) I see toolkit/components/passwordmgr/test/notification_common.js is already using this function too and I don't see a better way to test check the number of notifications. We'd probably be fine without this check and just use getNotification but meh.

@@ +27,5 @@
> +        let notifications = PopupNotifications._currentNotifications;
> +        is(notifications.length, 1, "should be one current notification");
> +        is(notifications[0].id, "offline-app-usage", "our notification is current");
> +        // dismiss it.
> +        PopupNotifications.panel.hidePopup();

Nit: It would be nice to click on the mainAction and ensure the preferences window is opened (using a window watcher).

@@ +31,5 @@
> +        PopupNotifications.panel.hidePopup();
> +
> +        finish();
> +      });
> +    }

Nit: missing semicolon

@@ +34,5 @@
> +      });
> +    }
> +    Services.prefs.setIntPref("offline-apps.quota.warn", 1);
> +
> +    // Click the notification box's "Allow" button.  This should kick

Nit: s/box/panel/

@@ +38,5 @@
> +    // Click the notification box's "Allow" button.  This should kick
> +    // off updates which will call our oncached handler above.
> +    let panel = PopupNotifications.panel;
> +    let button = document.getAnonymousElementByAttribute(panel.firstElementChild, "anonid", "button");
> +    button.click();

PopupNotifications.panel.firstElementChild.button.click();

@@ +41,5 @@
> +    let button = document.getAnonymousElementByAttribute(panel.firstElementChild, "anonid", "button");
> +    button.click();
> +  }, true);
> +
> +  content.location = URL;

Nit: gBrowser.selectedBrowser.contentWindow.location = URL; (for consistency with the usage above and because I find "content" to be non-obvious)

::: browser/base/content/test/offlineQuotaNotification.cacheManifest
@@ +1,2 @@
> +CACHE MANIFEST
> +offlineQuotaNotification.html

OOC, why did you specify the document itself in the manifest? The page would automatically be cached as a master entry https://developer.mozilla.org/en-US/docs/HTML/Using_the_application_cache#Master_entries although my understanding is that the master entry concept may go away in v2 of appcache.

::: browser/base/content/test/offlineQuotaNotification.html
@@ +1,1 @@
> +<!DOCTYPE HTML>

The public domain header is missing

@@ +1,3 @@
> +<!DOCTYPE HTML>
> +<html manifest="offlineQuotaNotification.cacheManifest">
> +<head>

Nit: add <meta charset="utf-8"> at the beginning of <head> to avoid the warning in the error console.

::: browser/themes/windows/browser.css
@@ +2216,5 @@
>  .popup-notification-icon[popupid="addon-install-blocked"],
>  .popup-notification-icon[popupid="addon-install-failed"],
>  .popup-notification-icon[popupid="addon-install-complete"],
> +.popup-notification-icon[popupid*="offline-app-requested"],
> +.popup-notification-icon[popupid="offline-app-usage"] {

Same as bug 588305, I don't think this should use the extension icon. The question mark is ok for now, but a storage icon would be the better long-term solution.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

(In reply to Matthew N. [:MattN] from comment #4)
> Nit: gBrowser.selectedBrowser.contentWindow.location = URL; (for consistency
> with the usage above and because I find "content" to be non-obvious)

Nit nit: gBrowser.contentWindow.location.

Comment 6

[Mark Hammond [:markh] [:mhammond]]

Attachment: Updated per feedback

(In reply to Matthew N. [:MattN] from comment #4)

> Thanks for writing a test :)
> Was the jetpack failure on TBPL unrelated?

Good question :)  I can't see how it was possibly related, but a new try is at https://tbpl.mozilla.org/?tree=Try&rev=e4efb20b98ac

> tsk-tsk for using private members ;) I see
> toolkit/components/passwordmgr/test/notification_common.js is already using
> this function too and I don't see a better way to test check the number of
> notifications. We'd probably be fine without this check and just use
> getNotification but meh.

Yeah - changed to use getNotification.

> Nit: It would be nice to click on the mainAction and ensure the preferences
> window is opened (using a window watcher).

Done - and a review of this code would be appreciated as I'm not particularly confident with how I did this and failed to find any other good tests of the permissions dialog.

> OOC, why did you specify the document itself in the manifest? 

As I didn't know any better :)  I've removed that.

Everything not mentioned above has been done as requested.

Comment 7

[Matthew N. [:MattN]]

Comment on attachment 739449 [details] [diff] [review]
Updated per feedback

Review of attachment 739449 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Mark Hammond (:markh) from comment #6)
> > Nit: It would be nice to click on the mainAction and ensure the preferences
> > window is opened (using a window watcher).
> 
> Done - and a review of this code would be appreciated as I'm not
> particularly confident with how I did this and failed to find any other good
> tests of the permissions dialog.

I would have been happy with just checking that the dialog opened but this is better :) Yes, it's true that we have very few tests for the the preferences dialog.

The bad news is that I forgot to mention the in-content preferences and this will fail when we switch to them. I think that the window watcher callback can have most of its contents moved to a common function (e.g. checkPreferences) for both in-content and the current window like so (untested):

if (Services.prefs.getBoolPref("browser.preferences.inContent")) {
  gBrowser.addEventListener("Initialized", function PrefInit() {
    gBrowser.removeEventListener("Initialized", PrefInit, true);
    checkPreferences(gBrowser.contentWindow);
  }, true);
} else {
  Services.ww.registerNotification(function wwobserver(aSubject, aTopic, aData) {
    if (aTopic != "domwindowopened")
      return;
    Services.ww.unregisterNotification(wwobserver);
    checkPreferences(aSubject);
  });
}

We can also consider an early return for in-content prefs (for now) and deal with this in a follow-up blocking bug 718011. r- for now due to this slowing down enabling in-content prefs by default.

::: browser/base/content/test/browser_offlineQuotaNotification.js
@@ +33,5 @@
> +          Services.ww.unregisterNotification(wwobserver);
> +          let prefsWin = aSubject.QueryInterface(Ci.nsIDOMEventTarget);
> +          prefsWin.addEventListener("paneload", function paneload(evt) {
> +            prefsWin.removeEventListener("paneload", paneload);
> +            is(evt.target.getAttribute("id"), "paneAdvanced", "advanced pane loaded");

Nit: s/getAttribute("id")/id/

@@ +37,5 @@
> +            is(evt.target.getAttribute("id"), "paneAdvanced", "advanced pane loaded");
> +            let tabPanels = evt.target.getElementsByTagName("tabpanels")[0];
> +            tabPanels.addEventListener("select", function tabselect() {
> +              tabPanels.removeEventListener("select", tabselect);
> +              is(tabPanels.selectedPanel.getAttribute("id"), "networkPanel", "networkPanel is selected\n");

ditto. Nit: "\n" is not necessary.

Comment 8

[Mark Hammond [:markh] [:mhammond]]

Attachment: Avoid breaking when inContent prefs land

What I've done is added the check you suggested, but disabled the test in that case as I don't think it is easy to test that case yet.  There are 2 places where "Bug XXXXX" appears, and if this gets r+ I'll open a new bug as you suggest and update the patch with the new bug number.

Comment 9

[Mark Hammond [:markh] [:mhammond]]

Attachment: other nits addressed

Oops - sorry for the bugspam - I forgot to address the other nits in your previous review.

Comment 10

[Matthew N. [:MattN]]

Comment on attachment 744386 [details] [diff] [review]
other nits addressed

Sorry for the delay.

Don't forgot to file the bug and move the commented code there.

Thanks

Comment 11

[Shane Caraveo (:mixedpuppy)]

ping.  any chance we can land this?  I'm in the final stages for bug 809085 and would like to get that landed this week.

Comment 12

[Mark Hammond [:markh] [:mhammond]]

Try at https://tbpl.mozilla.org/?tree=Try&rev=2fd8d3fb4145

I'll land as soon as inbound opens.

Comment 13

[Mark Hammond [:markh] [:mhammond]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bd14ca67cf7a

Comment 14

[Ed Morley [:emorley]]

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