Closed Bug 394392 Opened 12 years ago Closed 12 years ago

UI for enabling/disabling offline apps.

Categories

(Firefox :: General, defect, P2)

defect

Tracking

()

RESOLVED FIXED
Firefox 3 beta3

People

(Reporter: dcamp, Assigned: dcamp)

References

Details

Attachments

(5 files, 4 obsolete files)

Attached patch v1 (obsolete) — Splinter Review
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.
Flags: blocking-firefox3?
Attachment #279038 - Flags: review?(mconnor)
Attachment #279038 - Flags: review?(enndeakin)
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 :)

Attached image notification screenshot
Attachment #279039 - Flags: ui-review?(madhava)
Attached image preferences screenshot
beltzner appointed madhava to handle the UI design here in his absence, so I'm assuming he's the right ui-r
Attachment #279040 - Flags: ui-review?(madhava)
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 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)
Attachment #279039 - Flags: ui-review?(madhava) → ui-review-
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.
Attachment #279040 - Flags: ui-review?(madhava) → ui-review-
Attached patch v2 (obsolete) — Splinter Review
Updated with beltzner and neil's comments/
Assignee: nobody → dcamp
Attachment #279038 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #280935 - Flags: ui-review?(beltzner)
Attachment #280935 - Flags: review?(mconnor)
Attachment #279038 - Flags: review?(mconnor)
Attachment #279038 - Flags: review?(enndeakin)
Attachment #280935 - Flags: review?(enndeakin)
Comment on attachment 280935 [details] [diff] [review]
v2

The DOM storage stuff looks good.
Comment on attachment 280935 [details] [diff] [review]
v2

The DOM storage stuff looks good.
Attachment #280935 - Flags: review?(enndeakin) → review+
Blocks: 397417
Flags: blocking-firefox3? → blocking-firefox3+
Target Milestone: --- → Firefox 3 M9
Blocks: 398161
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?
Attachment #280935 - Flags: ui-review?(beltzner)
Attachment #280935 - Flags: review?(mconnor)
Attached patch v3 (obsolete) — Splinter Review
Updated for the DOMLinkAdded stuff.
Attachment #280935 - Attachment is obsolete: true
Attachment #283395 - Flags: review?(mconnor)
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?
Attached patch updated to trunk (obsolete) — Splinter Review
Attachment #284192 - Flags: review?(mconnor)
Attachment #283395 - Attachment is obsolete: true
Attachment #283395 - Flags: review?(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.
Attachment #284192 - Flags: review?(mconnor) → review+
(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.
Attachment #284192 - Attachment is obsolete: true
(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.
let's defer this until post-M9 given recent changes to WHATWG specs.
Target Milestone: Firefox 3 M9 → Firefox 3 M10
Target Milestone: Firefox 3 M10 → Firefox 3 M11
Priority: -- → P5
dave, what's the status of the backend work?  This can land, or move out to v.next, depending on how that's looking.
Priority: P5 → P2
Target Milestone: Firefox 3 Mx → Firefox 3 M11
Attached patch updatedSplinter Review
Updated for offline API changes.
Attachment #297040 - Flags: review?(mconnor)
Attachment #298532 - Flags: review?(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.
Attachment #298532 - Flags: review?(mconnor) → review+
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
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
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.
Attachment #297040 - Flags: review?(mconnor)
Depends on: 462854
You need to log in before you can comment on or make changes to this bug.