394392 - UI for enabling/disabling offline apps.

Status: RESOLVED FIXED

(Firefox :: General, defect, P2)

Description

[Dave Camp (:dcamp)]

Attachment: v1

This patch adds UI for:

a) Notifying a user that the site uses the offline API
b) Allowing offline apps
c) Removing an offline app once added
d) Changing the offline cache size.

This needs a small change to DOM storage to clear storage on a domain when it is removed from the list.

Comment 1

[Dave Camp (:dcamp)]

Some things I'd like to leave for followups:

a) The offline app list should have the current usage for the app (this takes a bit more backend work than I wanted to shove in to this patch)
b) Right now there's no way to make the notification bar go away permanently.  This is waiting on the new notification UI that Madhava tells me is coming soon :)

Comment 2

[Dave Camp (:dcamp)]

Attachment: notification screenshot

Comment 3

[Dave Camp (:dcamp)]

Attachment: preferences screenshot

beltzner appointed madhava to handle the UI design here in his absence, so I'm assuming he's the right ui-r

Comment 4

[Neil Deakin]

Comment on attachment 279038 [details] [diff] [review]
v1

I only looked at the dom/src/storage changes.

>+nsDOMStorageDB::RemoveOwner(const nsAString& aOwner)
>+{
>+  mozStorageStatementScoper scope(mRemoveOwnerStatement);
>+
>+  if (aOwner == mCachedOwner) {
>+    mCachedUsage = 0;

I think it would be good to clear mCachedOwner here too so it doesn't get out of sync.

>+   * Removes all keys added by a diven domain.

diven -> given here

Comment 5

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 279039 [details]
notification screenshot

Oops, sorry, Dave, I realize that I screwed up in my initial mockups. Not only is the wording a little wrong, I think we're going to need to be able to provide the address of the website that's asking to save data. So can this be changed to:

This website (www.foo.com) is offering to store data on your computer for offline use    (Allow) (x)

(We might eventually need to consider a "don't ask me again" function, but let's save that for a follow-up)

Comment 6

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 279040 [details]
preferences screenshot

This is probably my fault, again, in being unclear with my original mockups. I meant for us to rename the "Cache" section with "Offline Storage" (since they're the same user concept of "stuff on my disk"). I also didn't think we were going to have an option for limiting the amount of space that offline apps could take, and instead would just list how much each app was using.

So the result would be:

Cache _______________

Use up to [50] MB for the web cache  (Clear Now )

The following websites have stored data for offline use:
www.foo.com       128 MB

etc.

Comment 7

[Dave Camp (:dcamp)]

Attachment: v2

Updated with beltzner and neil's comments/

Comment 8

[Neil Deakin]

Comment on attachment 280935 [details] [diff] [review]
v2

The DOM storage stuff looks good.

Comment 9

[Neil Deakin]

Comment on attachment 280935 [details] [diff] [review]
v2

The DOM storage stuff looks good.

Comment 10

[Mike Connor [:mconnor]]

Comment on attachment 280935 [details] [diff] [review]
v2

Now that Ryan's patch for byg 396203 has landed, can you update this patch to use the new DOMLinkListener?

Comment 11

[Dave Camp (:dcamp)]

Attachment: v3

Updated for the DOMLinkAdded stuff.

Comment 12

[Dave Camp (:dcamp)]

I wonder if the "Remove offline application" dialog should include something about the fact that unsynced data (email drafts, docs, whatever) will be lost when the application is removed?

Comment 13

[Dave Camp (:dcamp)]

Attachment: updated to trunk

Comment 14

[Mike Connor [:mconnor]]

Comment on attachment 284192 [details] [diff] [review]
updated to trunk

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js


>+var OfflineApps = {
>+  /////////////////////////////////////////////////////////////////////////////
>+  // OfflineApps Public Methods
>+  init: function ()
>+  {
>+    var obs = Components.classes["@mozilla.org/observer-service;1"].getService(Components.interfaces.nsIObserverService);
>+    obs.addObserver(this, "offline-app-requested", false);
>+  },
>+
>+  uninit: function ()
>+  {
>+    try {
>+      var obs = Components.classes["@mozilla.org/observer-service;1"].getService(Components.interfaces.nsIObserverService);
>+      obs.removeObserver(this, "offline-app-requested");
>+    } catch (ex) {
>+    }
>+  },

break the long long lines please.  You should also be able to use Cc/Ci to make these not massive throughout.

>+  /////////////////////////////////////////////////////////////////////////////
>+  // OfflineApps Implementation Methods
>+
>+  _relRegex: /(^|\s)offline-resource($|\s)/i,
>+
>+  _offlineAppRequested: function(window) {

The _ prefix generally is a private method, but we're calling it from the DOMLinkAdded function, I think we just drop the prefix now.  Probably the same thing with _allowSite


>+    // don't bother showing UI if the user has already made a decision
>+    if (pm.testExactPermission(currentURI, "offline-app") !=
>+        Components.interfaces.nsIPermissionManager.UNKNOWN_ACTION)
>+      return;

This seems possibly problematic.  We don't use testExactPermission elsehwere (cookies, popups, etc)

Also, there's no current UI to say Never For This Site, so users will get prompted every time.  Ok for now, but need to resolve one way or other moving forward.

>+    try {
>+      if (gPrefService.getBoolPref("offline-apps.allow_by_default")) {
>+        // all pages can use offline capabilities, no need to ask the user
>+        return;
>+      }
>+    } catch(e) {
>+      // this pref isn't set by default, ignore failures
>+    }
>+
>+    while(window.parent != window)
>+      window = window.parent;

Don't use 'window' as a variable here.  aWindow or win or something.  window already has a value in this scope.

>+    var tabbrowser = getBrowser();
>+    var browser;
>+    const browsers = tabbrowser.mPanelContainer.childNodes;
>+    for (var i = 0; i < browsers.length; i++) {
>+      if (tabbrowser.getBrowserAtIndex(i).contentWindow == window) {
>+        browser = tabbrowser.getBrowserAtIndex(i);
>+        break;
>+      }
>+    }

I wish there was a cleaner way to get this, we could probably help perf

>+  _allowSite: function() {
>+    var currentURI = gBrowser.selectedBrowser.webNavigation.currentURI;
>+    var pm = Components.classes["@mozilla.org/permissionmanager;1"]
>+                       .getService(Components.interfaces.nsIPermissionManager);
>+    pm.add(currentURI, "offline-app", Components.interfaces.nsIPermissionManager.ALLOW_ACTION);
>+
>+    // When a site is enabled while loading, <link rel="offline-resource">
>+    // resources will start fetching immediately.  This one time we need to
>+    // do it ourselves.
>+    OfflineApps._startFetching();
>+  },

this._startFetching, no?

>diff --git a/browser/components/preferences/advanced.js b/browser/components/preferences/advanced.js
>--- a/browser/components/preferences/advanced.js
>+++ b/browser/components/preferences/advanced.js
>@@ -50,16 +50,17 @@ var gAdvancedPane = {

I'm wondering, a little, whether it'd be better to have an "Allowed Sites" list like popups, etc, and reuse that code.

This is ok for now, though.

Comment 15

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

(In reply to comment #14)
> >+    var tabbrowser = getBrowser();
> >+    var browser;
> >+    const browsers = tabbrowser.mPanelContainer.childNodes;
> >+    for (var i = 0; i < browsers.length; i++) {
> >+      if (tabbrowser.getBrowserAtIndex(i).contentWindow == window) {
> >+        browser = tabbrowser.getBrowserAtIndex(i);
> >+        break;
> >+      }
> >+    }
> 
> I wish there was a cleaner way to get this, we could probably help perf

You should use the same code the feed handling code uses, at least. See _getBrowserWindowForContentWindow and _getBrowserForContentWindow in WebContentConverter.js. There may be a better way, but if there is it's easier to fix them both if they're using the same code.

Comment 16

[Dave Camp (:dcamp)]

Attachment: fixes for review comments.

Comment 17

[Dave Camp (:dcamp)]

(In reply to comment #14)

> This seems possibly problematic.  We don't use testExactPermission elsehwere
> (cookies, popups, etc)

Filed bug 399526.

> 
> Also, there's no current UI to say Never For This Site, so users will get
> prompted every time.  Ok for now, but need to resolve one way or other moving
> forward.

Filed bug 399528.

(In reply to comment #15)
> (In reply to comment #14)
> > I wish there was a cleaner way to get this, we could probably help perf
> 
> You should use the same code the feed handling code uses, at least. See
> _getBrowserWindowForContentWindow and _getBrowserForContentWindow in
> WebContentConverter.js. There may be a better way, but if there is it's easier
> to fix them both if they're using the same code.

Copied the code from WebContentConverter.js and filed bug 399529.

Other review comments are fixed.

Comment 18

[Mike Connor [:mconnor]]

let's defer this until post-M9 given recent changes to WHATWG specs.

Comment 19

[Mike Connor [:mconnor]]

dave, what's the status of the backend work?  This can land, or move out to v.next, depending on how that's looking.

Comment 20

[Dave Camp (:dcamp)]

Attachment: updated

Updated for offline API changes.

Comment 21

[Dave Camp (:dcamp)]

Attachment: interdiff from already-reviewed patch

Comment 22

[Mike Connor [:mconnor]]

Comment on attachment 298532 [details] [diff] [review]
interdiff from already-reviewed patch

Looks good. Maybe a comment at least in the checkin comment that the empty init/uninit methods are there for future planned use, but otherwise let's ship it.

Comment 23

[Dave Camp (:dcamp)]

Added comments to the init/uninit methods.

Checking in browser/base/content/browser.js;
/cvsroot/mozilla/browser/base/content/browser.js,v  <--  browser.js
new revision: 1.931; previous revision: 1.930
done
Checking in browser/components/preferences/advanced.js;
/cvsroot/mozilla/browser/components/preferences/advanced.js,v  <--  advanced.js
new revision: 1.29; previous revision: 1.28
done
Checking in browser/components/preferences/advanced.xul;
/cvsroot/mozilla/browser/components/preferences/advanced.xul,v  <--  advanced.xul
new revision: 1.45; previous revision: 1.44
done
Checking in browser/locales/en-US/chrome/browser/browser.properties;
/cvsroot/mozilla/browser/locales/en-US/chrome/browser/browser.properties,v  <--  browser.properties
new revision: 1.56; previous revision: 1.55
done
Checking in browser/locales/en-US/chrome/browser/preferences/advanced.dtd;
/cvsroot/mozilla/browser/locales/en-US/chrome/browser/preferences/advanced.dtd,v  <--  advanced.dtd
new revision: 1.29; previous revision: 1.28
done
Checking in browser/locales/en-US/chrome/browser/preferences/preferences.properties;
/cvsroot/mozilla/browser/locales/en-US/chrome/browser/preferences/preferences.properties,v  <--  preferences.properties
new revision: 1.24; previous revision: 1.23
done
Checking in dom/src/storage/nsDOMStorage.cpp;
/cvsroot/mozilla/dom/src/storage/nsDOMStorage.cpp,v  <--  nsDOMStorage.cpp
new revision: 1.20; previous revision: 1.19
done
Checking in dom/src/storage/nsDOMStorageDB.cpp;
/cvsroot/mozilla/dom/src/storage/nsDOMStorageDB.cpp,v  <--  nsDOMStorageDB.cpp
new revision: 1.10; previous revision: 1.9
done
Checking in dom/src/storage/nsDOMStorageDB.h;
/cvsroot/mozilla/dom/src/storage/nsDOMStorageDB.h,v  <--  nsDOMStorageDB.h
new revision: 1.9; previous revision: 1.8
done

Comment 24

[Marcia Knous [:marcia]]

dave: in trying to verify this, what is expected to work? Trying your test site doesn't give me the notification bar, and I get this error in the console: Error: uncaught exception: [Exception... "Persistent storage maximum size reached"  code: "1014" nsresult: "0x805303f6 (NS_ERROR_DOM_QUOTA_REACHED)"  location: "http://campd.org/stuff/tests/offline1.html Line: 33"]. If there is a link to documentation that would help me create a test plan for this feature, that would be great as well.

Comment 25

[Γριφεγ]

Marcia: Reference documentation for this is the HTML5 spec, see: http://www.whatwg.org/specs/web-apps/current-work/multipage/section-storage.html#storage and http://www.whatwg.org/specs/web-apps/current-work/multipage/section-sql.html#sql