Closed Bug 758677 Opened 12 years ago Closed 12 years ago

Implement a Preference Pane for Offline Applications.

Categories

(SeaMonkey :: Preferences, defect)

Product:
SeaMonkey
Component:
Preferences
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
seamonkey2.13

People

(Reporter: philip.chee, Assigned: philip.chee)

References

Details

Attachments

(2 files, 3 obsolete files)

Patch v1.3 Fix issues.
28.15 KB, patch
iannbugzilla
: review+
Details | Diff | Splinter Review
Screenshot of the OfflineApps Preference Pane.
25.39 KB, image/png
Details 
From Bug 630092 Comment 12:
> Looks like we're still missing the preferences.
Attached patch Patch v1.0 (obsolete) — — Splinter Review 
Port prefpane parts from:
[Bug 394392] UI for enabling/disabling offline applications.
  [Bug 462856] offline app notification code doesn't handle subframes.
[Bug 399528] Add 'never for this site' option to the offline-app notification bar.
[Bug 413658] "UI for offline apps: missing accesskey for Remove button.
[bug 414931] remove cruft from browser.js and make offline storage website list in preferences.
[Bug 397417] monitor disk usage of offline apps in the UI.
  [Bug 424017] list of offline apps broken on non-mac platforms.
[Bug 442806] Use seperate, versioned caches for offline apps.
[Bug 442810] update ui for app cache changes.
[Bug 526230] nsIApplicationCacheService::getGroups count outparam should be optional.
[Bug 538588] Options -> Advanced -> Network -> Offline Storage confusing: Clear Now button only affects disk cache.
             (Split offline storage into http and offline cache in advanced network preferences dialog)

> -  if (permissionManager) {
> -    handleHostInput(document.getElementById("url"));
> -    loadPermissions();
> -  }
Jag asked IanN why he thought that the permissions manager would fail but never got an answer that I could see.

> -      if (permission.type == permissionType)
> +      if (permission.type == permissionType &&
> +          (!manageCapability ||
> +           permission.capability == manageCapability))

Drive by patch from Bug 334363: "can't change existing (cookie, popup blocking, xpi install, images) permissions for site, need to remove and then re-add"

> +offline-apppermissionshelp=offline_apps
TODO: File a followup Help bug.

Site used for testing: http://html5demos.com/offlineapp
Attachment #627285 - Flags: review?(iann_bugzilla)
Attachment #627285 - Flags: feedback?(neil)
Attachment #627285 - Flags: feedback?(jh)
Attachment #627285 - Attachment is patch: true
Comment on attachment 627285 [details] [diff] [review]
Patch v1.0

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

Note that I didn't test the webapps functionality itself thoroughly since I'm not too familiar with it.

Nit: Visiting the test case URL you provided didn't result in the addition of an entry in the list on the prefpane while it was open, and not while the Preferences window was open (switched panes) either. Only when I close the Preferences window and reopened it, the entry appeared. Ideally we'd attach a listener or something, but I'll leave that to the reviewer.

::: suite/common/pref/preferences.xul
@@ +256,5 @@
>                      url="chrome://communicator/content/pref/pref-cache.xul"/>
> +          <treeitem id="offlineAppsItem"
> +                    label="&offlineApps.label;"
> +                    prefpane="offlineapps_pane"
> +                    helpTopic="advanced_pref_cache"

Too much copy&paste? ;-) I think you should enter the correct topic here, even if it's not handled yet. Check with IanN if in doubt.

Just as a reminder, we'll have two locations which open Help: First the pref pane, an second the dialog accessible through the Exceptions button.

::: suite/locales/en-US/chrome/common/pref/prefutilities.properties
@@ +2,5 @@
>  #LOCALIZATION NOTE (%1$S) is the size and (%2$S) is the unit of disk space.
>  cacheSizeInfo=Your cache is currently using %1$S %2$S of disk space.
> +
> +# Offline apps
> +appCacheSizeInfo=Your application cache is currently using %1$S %2$S of disk space

Nit: Missing full stop (this is a sentence and appears as such in the UI)

@@ +4,5 @@
> +
> +# Offline apps
> +appCacheSizeInfo=Your application cache is currently using %1$S %2$S of disk space
> +offlineAppRemoveTitle=Remove offline website data
> +offlineAppRemovePrompt=After removing this data, %S will not be available offline.  Are you sure you want to remove this offline website?

To me, "this data" sounds strange, but AFAIK "data" can be used in both a singular or plural context in English. I guess Ian/Neil will have to check this.
Attachment #627285 - Flags: feedback?(jh) → feedback+
>> +                    helpTopic="advanced_pref_cache"
> 
> Too much copy&paste? ;-) I think you should enter the correct topic here,
> even if it's not handled yet. Check with IanN if in doubt.

What would you guys suggest? "advanced_offline_apps"?

> Just as a reminder, we'll have two locations which open Help: First the pref
> pane, an second the dialog accessible through the Exceptions button.

doHelpButton() calls:
permissionType + "permissionshelp"
So the help topic should be "offline-appspermissionhelp" :P

>> +appCacheSizeInfo=Your application cache is currently using %1$S %2$S of disk space

> Nit: Missing full stop (this is a sentence and appears as such in the UI)
Will do, but someone should tell Firefox that their periods are AWOL.

> To me, "this data" sounds strange, but AFAIK "data" can be used in both a
> singular or plural context in English. I guess Ian/Neil will have to check this.

Sometimes I'd use "datum" but then the language geeks would jump on me and point out that I'm writing English and not Latin.
Shouldn't we be adding this too data manager rather than permission manager (or as well as)?
> Shouldn't we be adding this too data manager rather than permission manager (or as well
> as)?

Sorry I tried to look at the DM code but got totally lost after a while. So I decided to keep to what I know. Also I don't know what parameters to pass to the DM to filter for *one* particular type of permission. Thirdly I think the DM is overkill and has lots of distracting superfluous UI for this particular usage.
(In reply to Philip Chee from comment #3)
> >> +                    helpTopic="advanced_pref_cache"
> > 
> > Too much copy&paste? ;-) I think you should enter the correct topic here,
> > even if it's not handled yet. Check with IanN if in doubt.
> 
> What would you guys suggest? "advanced_offline_apps"?
Others seem to be of the form advanced_pref_<pane_title>, so in this case would be advanced_pref_offline_apps or advanced_pref_offlineapps
> 
> > Just as a reminder, we'll have two locations which open Help: First the pref
> > pane, an second the dialog accessible through the Exceptions button.
> 
> doHelpButton() calls:
> permissionType + "permissionshelp"
This refers to the string from permissionsManager.properties which you have set to offline_apps, but it does depend on how the help page is written and whether it will be in a different section to the preferences help.

> So the help topic should be "offline-appspermissionhelp" :P

> 
> >> +appCacheSizeInfo=Your application cache is currently using %1$S %2$S of disk space
> 
> > Nit: Missing full stop (this is a sentence and appears as such in the UI)
> Will do, but someone should tell Firefox that their periods are AWOL.
Usually done by a creating a bug :P

> > To me, "this data" sounds strange, but AFAIK "data" can be used in both a
> > singular or plural context in English. I guess Ian/Neil will have to check this.
> 
> Sometimes I'd use "datum" but then the language geeks would jump on me and
> point out that I'm writing English and not Latin.
"this data" sounds okay to me.
Comment on attachment 627285 [details] [diff] [review]
Patch v1.0

Based on code inspection:
>+++ b/suite/common/jar.mn
>@@ -201,16 +201,18 @@ comm.jar:
>    content/communicator/pref/pref-locationbar.js                    (pref/pref-locationbar.js)
>    content/communicator/pref/pref-locationbar.xul                   (pref/pref-locationbar.xul)
>    content/communicator/pref/pref-mousewheel.js                     (pref/pref-mousewheel.js)
>    content/communicator/pref/pref-mousewheel.xul                    (pref/pref-mousewheel.xul)
>    content/communicator/pref/pref-navigator.js                      (pref/pref-navigator.js)
>    content/communicator/pref/pref-navigator.xul                     (pref/pref-navigator.xul)
>    content/communicator/pref/pref-passwords.xul                     (pref/pref-passwords.xul)
>    content/communicator/pref/pref-policies.xul                      (pref/pref-policies.xul)
>+   content/communicator/pref/pref-offlineapps.js                    (pref/pref-offlineapps.js)
>+   content/communicator/pref/pref-offlineapps.xul                   (pref/pref-offlineapps.xul)
Nit: these should go between pref-navigator and pref-passwords.

>+++ b/suite/common/permissions/permissionsManager.js
>+var manageCapability;
Globals should start with a g, so gManageCapability here

>+  var urlField = document.getElementById("url");
>+  var urlLabel = document.getElementById("urlLabel");
These two are only used the once each so might as well inline them.

>+  urlField.hidden = !urlFieldVisible;
>+  urlLabel.hidden = !urlFieldVisible;
>+
>+      if (permission.type == permissionType &&
>+          (!manageCapability ||
>+           permission.capability == manageCapability))
Nit: There is space for the two lines above to be on single line.

>+++ b/suite/common/permissions/permissionsManager.xul
>@@ -57,17 +57,17 @@
>   <script type="application/javascript" src="chrome://communicator/content/permissions/treeUtils.js"/>
>   <script type="application/javascript" src="chrome://help/content/contextHelp.js"/>
> 
>   <stringbundle id="permissionsBundle"
>                 src="chrome://communicator/locale/permissions/permissionsManager.properties"/>
> 
>   <description id="permissionsText"/>
>   <separator class="thin"/>
>-  <label value="&address.label;" accesskey="&address.accesskey;" control="url"/>
>+  <label id="urlLabel" value="&address.label;" accesskey="&address.accesskey;" control="url"/>
Nit: longer than 80 characters so one attribute per line.

>+++ b/suite/common/pref/pref-offlineapps.js

>+function UpdateActualCacheSize()
>+{
>+  var visitor = {
>+    visitDevice: function (deviceID, deviceInfo)
aDeviceID and aDeviceInfo

>+    visitEntry: function (deviceID, entryInfo)
aDeviceID and aEntryInfo

>+function _getOfflineAppUsage(host, groups)
aHost and aGroups
>+{
>+  var appCache = Components.classes["@mozilla.org/network/application-cache-service;1"]
>+                           .getService(Components.interfaces.nsIApplicationCacheService);
>+  if (!groups)
>+    groups = appCache.getGroups();
>+
>+  var usage = 0;
Can't you get the host usage here?
var usage = Services.domStorageManager.getUsage(aHost);

>+  for (let i = 0; i < groups.length; i++) {
>+    let uri = Services.io.newURI(groups[i], null, null);
>+    if (uri.asciiHost == host) {
>+      let cache = appCache.getActiveCache(groups[i]);
>+      usage += cache.usage;
Inline cache
>+    }
>+  }

>+/**
>+ * Updates the list of offline applications
Nit: full stop at the end of the comment.

>+function RemoveOfflineApp()
>+  if (Services.prompt.confirmEx(window, title, prompt, flags, confirm,
>+                                         null, null, null, {}))
Nit: indentation wrong.

>+  for (let i = 0; i < groups.length; i++) {
>+      var uri = Services.io.newURI(groups[i], null, null);
>+      if (uri.asciiHost == host)
>+      {
>+          var cache = appCache.getActiveCache(groups[i]);
>+          cache.discard();
Inline cache.
(In reply to Philip Chee from comment #5)
> Sorry I tried to look at the DM code but got totally lost after a while.
We need the permissions to show up with a friendly name rather than the internal "offline-app" string. Fortunately this is fairly easy as we just hit the .properties file for the perm.offline-app.label string.
I'm not sure whether it's necessary to be able to add offline-app permissions but this is not difficult either, simply add it to the permTypes array in addButtonClick.

> Also I don't know what parameters to pass
> to the DM to filter for *one* particular type of permission.
I don't think we support that, sorry. The images and popup windows pref panes simply have a "Manage Permissions" button, which should suffice.
Comment on attachment 627285 [details] [diff] [review]
Patch v1.0

>diff --git a/suite/common/permissions/permissionsManager.js b/suite/common/permissions/permissionsManager.js
(If you were to use the permissions manager, I'm not really keen on you not being able to allow or block sites.)

>+  while (list.lastChild) {
>+    list.removeChild(list.lastChild);
>+  }
Nit: no {}s

>+  var enumerator = Services.perms.enumerator;
(I don't like the way the offline cache is enumerated based on currently permitted hosts.)

>+  // remove the permission
>+  Services.perms.remove(host, "offline-app",
>+                        Components.interfaces.nsIPermissionManager.ALLOW_ACTION);
>+  Services.perms.remove(host, "offline-app",
>+                        Components.interfaces.nsIOfflineCacheUpdateService.ALLOW_NO_WARN);
remove() only takes two parameters. (But I'm not sure we should be removing permissions here.)

>+          <listbox id="offlineAppsList"
>+                   style="min-height: &offlineAppsList.height;"
>+                   flex="1"
Not sure what the point of the height is.

>+<!ENTITY offlineNotify.label               "Tell me when a website asks to store data for offline use">
"Notify me when a website wants to store data for offline use."
Attachment #627285 - Flags: feedback?(neil) → feedback-
(In reply to Philip Chee from comment #5)
> Sorry I tried to look at the DM code but got totally lost after a while.
Oh, one more thing: the "default" value of the permission (getDefault function) should be provided; it is governed by two preferences:
* if offline-apps.allow_by_default is true, then the default is ALLOW
* if browser.offline-apps.notify is false, then the default is BLOCK
* otherwise, the default is UNKNOWN
The allow_by_default pref probably doesn't deserve any UI though.

Does Firefox's Page Info show offline app permissions?
(In reply to neil@parkwaycc.co.uk from comment #8)
> (In reply to Philip Chee from comment #5)
> > Sorry I tried to look at the DM code but got totally lost after a while.
> We need the permissions to show up with a friendly name rather than the
> internal "offline-app" string. Fortunately this is fairly easy as we just
> hit the .properties file for the perm.offline-app.label string.
> I'm not sure whether it's necessary to be able to add offline-app
> permissions but this is not difficult either, simply add it to the permTypes
> array in addButtonClick.
Only question is what to do about ticking the "Use Default" tickbox. Does it remove the cache? If so, we should probably warn.
> 
> > Also I don't know what parameters to pass
> > to the DM to filter for *one* particular type of permission.
> I don't think we support that, sorry. The images and popup windows pref
> panes simply have a "Manage Permissions" button, which should suffice.

oncommand="toDataManager('|permissions');" will be what you need.
(In reply to Ian Neal from comment #4)
> Shouldn't we be adding this too data manager rather than permission manager
> (or as well as)?

Bug 588415 has my work for that, but some reviewer there didn't like my patch for it. ;-)
Attachment #627285 - Flags: review?(iann_bugzilla) → review-
Attached patch Patch v1.1 Fix issues. (obsolete) — — Splinter Review 
>> What would you guys suggest? "advanced_offline_apps"?
> Others seem to be of the form advanced_pref_<pane_title>, so in this case
> would be advanced_pref_offline_apps or advanced_pref_offlineapps

helpTopic now set to "advanced_pref_offlineapps"

>> +appCacheSizeInfo=Your application cache is currently using %1$S %2$S of disk space
> Nit: Missing full stop (this is a sentence and appears as such in the UI)
Fixed.

>>+   content/communicator/pref/pref-offlineapps.js                    (pref/pref-offlineapps.js)
>>+   content/communicator/pref/pref-offlineapps.xul                   (pref/pref-offlineapps.xul)
> Nit: these should go between pref-navigator and pref-passwords.
Fixed.

>>+var manageCapability;
> Globals should start with a g, so gManageCapability here
Fixed.

>>+          (!manageCapability ||
>>+           permission.capability == manageCapability))
> Nit: There is space for the two lines above to be on single line.
Fixed.

>>+  <label id="urlLabel" value="&address.label;" accesskey="&address.accesskey;" control="url"/>
> Nit: longer than 80 characters so one attribute per line.
Fixed.

> aDeviceID and aDeviceInfo
> aHost and aGroups
Fixed and Fixed.

> var usage = Services.domStorageManager.getUsage(aHost);
Fixed.

>>+      usage += cache.usage;
> Inline cache
Fixed.

>>+ * Updates the list of offline applications
> Nit: full stop at the end of the comment.
Fixed.

>>+function RemoveOfflineApp()
>>+  if (Services.prompt.confirmEx(window, title, prompt, flags, confirm,
>>+                                         null, null, null, {}))
> Nit: indentation wrong.
Fixed.

>>+  for (let i = 0; i < groups.length; i++) {
>>+      var uri = Services.io.newURI(groups[i], null, null);
>>+      if (uri.asciiHost == host)
>>+      {
>>+          var cache = appCache.getActiveCache(groups[i]);
>>+          cache.discard();
> Inline cache.
Fixed.

>> Sorry I tried to look at the DM code but got totally lost after a while.
> We need the permissions to show up with a friendly name rather than the
> internal "offline-app" string. Fortunately this is fairly easy as we just
> hit the .properties file for the perm.offline-app.label string.
> I'm not sure whether it's necessary to be able to add offline-app
> permissions but this is not difficult either, simply add it to the permTypes
> array in addButtonClick.
Fixed.

>> Also I don't know what parameters to pass
>> to the DM to filter for *one* particular type of permission.
> I don't think we support that, sorry. The images and popup windows pref
> panes simply have a "Manage Permissions" button, which should suffice.
Fixed.

>>+  var enumerator = Services.perms.enumerator;
> (I don't like the way the offline cache is enumerated based on currently permitted hosts.)
Fixed.

>>+  Services.perms.remove(host, "offline-app",
>>+                        Components.interfaces.nsIOfflineCacheUpdateService.ALLOW_NO_WARN);
> remove() only takes two parameters.
Fixed.

(But I'm not sure we should be removing permissions here.)
I think it's logical to do so, but I'll comment out this line for the time being.

>>+          <listbox id="offlineAppsList"
>>+                   style="min-height: &offlineAppsList.height;"
>>+                   flex="1"
> Not sure what the point of the height is.
Removed.

>>+<!ENTITY offlineNotify.label               "Tell me when a website asks to store data for offline use">
> "Notify me when a website wants to store data for offline use."
Fixed.

> Oh, one more thing: the "default" value of the permission (getDefault function)
> should be provided; it is governed by two preferences:
> * if offline-apps.allow_by_default is true, then the default is ALLOW
> * if browser.offline-apps.notify is false, then the default is BLOCK
> * otherwise, the default is UNKNOWN
> The allow_by_default pref probably doesn't deserve any UI though.
Fixed.

> Does Firefox's Page Info show offline app permissions?
I thought it did but on closer inspection it's just IndexedDB.

> oncommand="toDataManager('|permissions');" will be what you need.
Fixed.

> Visiting the test case URL you provided didn't result in the addition of an
> entry in the list on the prefpane while it was open, and not while the
> Preferences window was open (switched panes) either. Only when I close the
> Preferences window and reopened it, the entry appeared. Ideally we'd attach
> a listener or something, but I'll leave that to the reviewer.
I've added an observer for "perm-changed".
Attachment #627285 - Attachment is obsolete: true
Attachment #631001 - Flags: superreview?(neil)
Attachment #631001 - Flags: review?(iann_bugzilla)
Comment on attachment 631001 [details] [diff] [review]
Patch v1.1 Fix issues.

>diff --git a/suite/common/permissions/permissionsManager.js b/suite/common/permissions/permissionsManager.js
Surely we don't need these changes any more?

>+  update: function offlineAppsUpdate() {
>+      UpdateActualCacheSize();
>+      UpdateOfflineApps();
Nit: wrong indentation

>+  observe: function offlineAppsObserve(aSubject, aTopic, aData) {
>+    if (aTopic == "perm-changed")
>+      this.update();
I agree that this didn't work reliably, probably because the demo doesn't immediately store data. If I visit a second demo, then the data for this demo does appear.

>+function ShowOfflineExceptions()
Not used?

>+  for (let i = 0; i < aGroups.length; i++) {
I don't suppose it's possible to just list the groups, rather than worrying about what has permissions?

>+  if (Services.prompt.confirmEx(window, title, prompt, flags, confirm,
>+                                null, null, null, {}))
[I notice that clearing everything doesn't prompt. Then again, neither does clearing the cache.]

>+<!ENTITY offlineNotifyExceptions.label     "Exceptionsâ¦">
>+<!ENTITY offlineNotifyExceptions.accesskey "E">
This should probably be changed to "Manage &Permissions"> as per the Image preferences.
Attachment #631001 - Attachment is patch: true
Attached patch Patch v1.2 Fix more stuff. (obsolete) — — Splinter Review 
>>diff --git a/suite/common/permissions/permissionsManager.js b/suite/common/permissions/permissionsManager.js
> Surely we don't need these changes any more?
I would like to keep things roughly in sync with Firefox. For example a ported Firefox addon might want to use the Permissions UI.

>>+  update: function offlineAppsUpdate() {
>>+      UpdateActualCacheSize();
>>+      UpdateOfflineApps();
> Nit: wrong indentation
Fixed.

>>+  observe: function offlineAppsObserve(aSubject, aTopic, aData) {
>>+    if (aTopic == "perm-changed")
>>+      this.update();
> I agree that this didn't work reliably, probably because the demo doesn't immediately store data. If I visit a second demo, then the data for this demo does appear.

>>+function ShowOfflineExceptions()
> Not used?
Removed.

>>+  for (let i = 0; i < aGroups.length; i++) {
> I don't suppose it's possible to just list the groups, rather than worrying about what has permissions?

]   var usage = Services.domStorageManager.getUsage(aHost);

I don't know if it's possible for a domain/host to have DOM storage but not use the offline application cache. Or should I only look at the App Cache groups?

>>+  if (Services.prompt.confirmEx(window, title, prompt, flags, confirm,
>>+                                null, null, null, {}))
> [I notice that clearing everything doesn't prompt. Then again, neither does clearing the cache.]

>>+<!ENTITY offlineNotifyExceptions.label     "Exceptions…">
>>+<!ENTITY offlineNotifyExceptions.accesskey "E">
> This should probably be changed to "Manage &Permissions"> as per the Image preferences.
Fixed.
Attachment #631001 - Attachment is obsolete: true
Attachment #631001 - Flags: superreview?(neil)
Attachment #631001 - Flags: review?(iann_bugzilla)
Attachment #633918 - Flags: superreview?(neil)
Attachment #633918 - Flags: review?(iann_bugzilla)
Comment on attachment 633918 [details] [diff] [review]
Patch v1.2 Fix more stuff.

>+++ b/suite/common/pref/pref-offlineapps.js

>+function UpdateOfflineApps()
>+{
>+  var list = document.getElementById("offlineAppsList");
>+  while (list.lastChild)
>+    list.removeChild(list.lastChild);
>+
>+  var appCache = Components.classes["@mozilla.org/network/application-cache-service;1"]
>+                           .getService(Components.interfaces.nsIApplicationCacheService);
>+  var groups = appCache.getGroups();
Are appCache and groups used other than for one to create the other and then pass through to _getOfflineAppUsage which creates them both again anyway?

>+    let usage = _getOfflineAppUsage(perm.host, groups);
>+    if (usage) {
>+      let row = document.createElement("listitem");
>+      row.setAttribute("host", perm.host);
>+      let converted = DownloadUtils.convertByteUnits(usage);
>+      row.setAttribute("usage", bundle.getFormattedString("offlineAppUsage",
>+                                                          converted));
>+      list.appendChild(row);
When usage is zero then a row isn't added, if that is what you expect to happen then the label "The following websites are allowed to store data for offline use:" is not correct.

>+++ b/suite/common/pref/pref-offlineapps.xul

>+        <checkbox id="offlineNotify" flex="1"
Nit: One attribute per line.
>+                  label="&offlineNotify.label;"
>+                  accesskey="&offlineNotify.accesskey;"
>+                  preference="browser.offline-apps.notify"/>

>+++ b/suite/common/pref/preferences.xul

>+          <treeitem id="offlineAppsItem"
>+                    label="&offlineApps.label;"
>+                    prefpane="offlineapps_pane"
>+                    helpTopic="advanced_pref_offlineapps"
>+                    url="chrome://communicator/content/pref/pref-offlineapps.xul"/>
Follow-up bug or new bug to provide the help?

r- for the moment as I want to test a new patch further.
Attachment #633918 - Flags: review?(iann_bugzilla) → review-
>>+++ b/suite/common/pref/pref-offlineapps.js
> 
>>+function UpdateOfflineApps()
>>+{
>>+  var list = document.getElementById("offlineAppsList");
>>+  while (list.lastChild)
>>+    list.removeChild(list.lastChild);
>>+
>>+  var appCache = Components.classes["@mozilla.org/network/application-cache-service;1"]
>>+                           .getService(Components.interfaces.nsIApplicationCacheService);
>>+  var groups = appCache.getGroups();
> Are appCache and groups used other than for one to create the other and then pass through to _getOfflineAppUsage which creates them both again anyway?
Removed redundant usage of groups/appcache.

>>+    let usage = _getOfflineAppUsage(perm.host, groups);
>>+    if (usage) {
>>+      let row = document.createElement("listitem");
>>+      row.setAttribute("host", perm.host);
>>+      let converted = DownloadUtils.convertByteUnits(usage);
>>+      row.setAttribute("usage", bundle.getFormattedString("offlineAppUsage",
>>+                                                          converted));
>>+      list.appendChild(row);
> When usage is zero then a row isn't added, if that is what you expect to happen then the label "The following websites are allowed to store data for offline use:" is not correct.
Fixed. Now shows all permitted sites including those with not currently using any storage.

Question: Do I need to consider pm.UNKNOWN_ACTION here?:

> +    let perm = enumerator.getNext()
> +                         .QueryInterface(Components.interfaces.nsIPermission);
> +    if (perm.type != "offline-app" ||
> +        perm.capability != pm.ALLOW_ACTION)
> +      continue;

>>+++ b/suite/common/pref/pref-offlineapps.xul
> 
>>+        <checkbox id="offlineNotify" flex="1"
> Nit: One attribute per line.
>>+                  label="&offlineNotify.label;"
>>+                  accesskey="&offlineNotify.accesskey;"
>>+                  preference="browser.offline-apps.notify"/>
Fixed.

>>+++ b/suite/common/pref/preferences.xul
> 
>>+          <treeitem id="offlineAppsItem"
>>+                    label="&offlineApps.label;"
>>+                    prefpane="offlineapps_pane"
>>+                    helpTopic="advanced_pref_offlineapps"
>>+                    url="chrome://communicator/content/pref/pref-offlineapps.xul"/>
> Follow-up bug or new bug to provide the help?
Will do.
Attachment #633918 - Attachment is obsolete: true
Attachment #633918 - Flags: superreview?(neil)
Attachment #639688 - Flags: review?(iann_bugzilla)
Blocks: 771534
> Fixed. Now shows all permitted sites including those with not currently using
> any storage.
Attachment #639688 - Flags: review?(iann_bugzilla) → review+
Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/827520edfaaa
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.13
Comment on attachment 639688 [details] [diff] [review]
Patch v1.3 Fix issues.

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

This is a mess. And that we are adding admin capabilities on caching a few web pages locally but not for managing actual user data (localStore, indexedDB) is IMHO almost ridiculous. Almost looks like we have no plan behind what we are doing and are just about adding another set of UI clutter that our users have no idea how to use. But then, that's not too different from how SeaMonkey is acting overall nowadays.

::: suite/common/dataman/dataman.js
@@ +1290,5 @@
> +          // this pref isn't set by default, ignore failures
> +        }
> +        if (Services.prefs.getBoolPref("browser.offline-apps.notify"))
> +          return Services.perms.DENY_ACTION;
> +        return Services.perms.UNKNOWN_ACTION;

If this stays in the code in this way, I'll need to retreat as the code owner for data manager. This function is only used for initializing UI, but the UI doesn't support Services.perms.UNKNOWN_ACTION so you make it end up in a state where it lies to the user about what is set. This is not acceptable to me as a module owner of this code.

::: suite/locales/en-US/chrome/common/pref/pref-offlineapps.dtd
@@ +1,5 @@
> +<!-- extracted from content/pref-offlineapps.xul -->
> +
> +<!--LOCALIZATION NOTE : FILE Offline Apps prefs dialog -->
> +<!ENTITY pref.offlineapps.title             "Offline Web Applications">
> +<!ENTITY pref.offlineCache.caption          "Offline Web Content and User Data">

AFAIK, this is a lie. appCache doesn't store any user data.
> If this stays in the code in this way, I'll need to retreat as the code owner for data
> manager. This function is only used for initializing UI, but the UI doesn't support
> Services.perms.UNKNOWN_ACTION so you make it end up in a state where it lies to the
> user about what is set. This is not acceptable to me as a module owner of this code.

I assumed that .UNKNOWN_ACTION is fine because the existing code already uses this. As I said further up, I don't really understand the Data Manager code, I'm just adapting existing code there for a new permission. Please explain what changes are needed to fix whatever needs to be fixed.

> AFAIK, this is a lie. appCache doesn't store any user data.
Googling says it does. Please cite something authoritative that says it doesn't.
(In reply to Philip Chee from comment #21)
> I assumed that .UNKNOWN_ACTION is fine because the existing code already
> uses this.

Where?

> > AFAIK, this is a lie. appCache doesn't store any user data.
> Googling says it does. Please cite something authoritative that says it
> doesn't.

Please tell me what says that it does. I have been using appCache in my web apps and all I could find out that it is doing is it caching static files from the website locally. It's often used together with localStorage and/or indexedDB which can store user data, but appCache itself doesn't.
>> I assumed that .UNKNOWN_ACTION is fine because the existing code already
>> uses this.
> Where?
http://hg.mozilla.org/comm-central/annotate/433268decbc6/suite/common/dataman/dataman.js#l1299

> It's often used together with localStorage and/or indexedDB which can store user data,
> but appCache itself doesn't.
Right but the size column in the list of websites storing user data includes localStorage.
(In reply to Philip Chee from comment #23)
> >> I assumed that .UNKNOWN_ACTION is fine because the existing code already
> >> uses this.
> > Where?
> http://hg.mozilla.org/comm-central/annotate/433268decbc6/suite/common/
> dataman/dataman.js#l1299

Sh*t. That one probably causes UI problems at least. And with the recent habit of not adding tests added features, it naturally goes undetected. I should veto all reviews to Data Manager that don't have tests, I've grown too lenient there by just requesting followup bugs that add them, which naturally go without actions. :(

> > It's often used together with localStorage and/or indexedDB which can store user data,
> > but appCache itself doesn't.
> Right but the size column in the list of websites storing user data includes
> localStorage.

Ah, that's interesting. Does the delete also include it? The header surely doesn't mention any of that. And indexedDB is still missing, but I know it's a PITA as you can't even find out what domains have that (AFAIK same for localStorage so I wonder how you make sure the list is complete - I guess you take the same flawed approach as Firefox there).
> Ah, that's interesting. Does the delete also include it?
Yes it does.

> (AFAIK same for localStorage so I wonder how you make sure the list is complete - I
> guess you take the same flawed approach as Firefox there).
Can you suggest a better approach?
(In reply to Philip Chee from comment #25)
> > (AFAIK same for localStorage so I wonder how you make sure the list is complete - I
> > guess you take the same flawed approach as Firefox there).
> Can you suggest a better approach?

What I'm doing in bug 588415 is more complete but extremely hackish. The best way would be for toolkit to implement proper management interfaces, as we e.g. have them for cookies, passwords, etc.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: