Closed
Bug 394392
Opened 14 years ago
Closed 13 years ago
UI for enabling/disabling offline apps.
Categories
(Firefox :: General, defect, P2)
Firefox
General
Tracking
()
RESOLVED
FIXED
Firefox 3 beta3
People
(Reporter: dcamp, Assigned: dcamp)
References
Details
Attachments
(5 files, 4 obsolete files)
32.62 KB,
image/png
|
beltzner
:
ui-review-
|
Details |
67.73 KB,
image/png
|
beltzner
:
ui-review-
|
Details |
23.83 KB,
patch
|
beltzner
:
ui-review+
|
Details | Diff | Splinter Review |
23.76 KB,
patch
|
Details | Diff | Splinter Review | |
6.95 KB,
patch
|
mconnor
:
review+
|
Details | Diff | 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)
Assignee | ||
Updated•14 years ago
|
Attachment #279038 -
Flags: review?(enndeakin)
Assignee | ||
Comment 1•14 years ago
|
||
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 :)
Assignee | ||
Comment 2•14 years ago
|
||
Attachment #279039 -
Flags: ui-review?(madhava)
Assignee | ||
Comment 3•14 years ago
|
||
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 4•14 years ago
|
||
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•14 years ago
|
||
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 6•14 years ago
|
||
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-
Assignee | ||
Comment 7•14 years ago
|
||
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)
Assignee | ||
Updated•14 years ago
|
Attachment #280935 -
Flags: review?(enndeakin)
Comment 8•14 years ago
|
||
Comment on attachment 280935 [details] [diff] [review] v2 The DOM storage stuff looks good.
Comment 9•14 years ago
|
||
Comment on attachment 280935 [details] [diff] [review] v2 The DOM storage stuff looks good.
Attachment #280935 -
Flags: review?(enndeakin) → review+
Updated•14 years ago
|
Flags: blocking-firefox3? → blocking-firefox3+
Updated•14 years ago
|
Target Milestone: --- → Firefox 3 M9
Comment 10•14 years ago
|
||
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)
Assignee | ||
Comment 11•14 years ago
|
||
Updated for the DOMLinkAdded stuff.
Attachment #280935 -
Attachment is obsolete: true
Attachment #283395 -
Flags: review?(mconnor)
Assignee | ||
Comment 12•14 years ago
|
||
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?
Assignee | ||
Comment 13•14 years ago
|
||
Attachment #284192 -
Flags: review?(mconnor)
Assignee | ||
Updated•14 years ago
|
Attachment #283395 -
Attachment is obsolete: true
Attachment #283395 -
Flags: review?(mconnor)
Comment 14•14 years ago
|
||
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+
Comment 15•14 years ago
|
||
(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.
Assignee | ||
Comment 16•14 years ago
|
||
Attachment #284192 -
Attachment is obsolete: true
Assignee | ||
Comment 17•14 years ago
|
||
(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.
Updated•14 years ago
|
Attachment #284554 -
Flags: ui-review+
Comment 18•14 years ago
|
||
let's defer this until post-M9 given recent changes to WHATWG specs.
Target Milestone: Firefox 3 M9 → Firefox 3 M10
Updated•13 years ago
|
Target Milestone: Firefox 3 M10 → Firefox 3 M11
Updated•13 years ago
|
Priority: -- → P5
Comment 19•13 years ago
|
||
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
Assignee | ||
Comment 20•13 years ago
|
||
Updated for offline API changes.
Attachment #297040 -
Flags: review?(mconnor)
Assignee | ||
Comment 21•13 years ago
|
||
Attachment #298532 -
Flags: review?(mconnor)
Comment 22•13 years ago
|
||
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+
Assignee | ||
Comment 23•13 years ago
|
||
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: 13 years ago
Resolution: --- → FIXED
Comment 24•13 years ago
|
||
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•13 years ago
|
||
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
Updated•13 years ago
|
Attachment #297040 -
Flags: review?(mconnor)
You need to log in
before you can comment on or make changes to this bug.
Description
•