594744 - Update cache preferences for bug 559942

Status: RESOLVED FIXED

(SeaMonkey :: Preferences, defect)

Description

[Robert Kaiser]

Bug 559942 has changed how disk cache is being sized, we need to update our cache pref pnael for that, possibly quite easy to do, as FF did change their prefs as well.

Comment 1

[Robert Kaiser]

Probably need to block 2.1 final for this trivial task.

Comment 2

[Byron Milligan [:byronm]]

Bug 559942 did update the cache preferences panel, found under Preferences-->Advanced-->Network. The update shows how much space is currently being used on disk, and enables the user to specify if they would like the functionality introduced in 559942 or not.

I'm not sure if you had something else in mind?

Comment 3

[Robert Kaiser]

I didn't read the other bug in detail, but from all I know, exactly what you explained is what I meant, we need to port those changes.

Comment 4

[Edmund Wong (:ewong)]

Attachment: Ported cache preferences from bug 559952.

Comment 5

[Edmund Wong (:ewong)]

Attachment: Ported cache preferences from bug 559952.

Reordered the attributes within the <label> element.

Comment 6

[Edmund Wong (:ewong)]

Attachment: Ported cach preferences from Bug 559952

Fixed a bug in the previous patch.

Comment 7

[Edmund Wong (:ewong)]

Attachment: Ported cach preferences from Bug 559952

Changed a dtd definition so that localizers can determine the string property has changed.

Comment 8

[[e-mail]]

Comment on attachment 488804 [details] [diff] [review]
Ported cach preferences from Bug 559952

[I think this panel would have looked better with a radiogroup instead of a checkbox to choose between manual and smart sizing. We do something similar for software installation preferences. I should file a followup bug.]

>+	updateActualCacheSize();
A lot of tabs crept into this patch. Spaces please!

>+      // Do not enumerate entries
>+
>+      return false;
>+
>+    },
>+
>+    visitEntry: function (deviceID, entryInfo)
>+    {
>+
>+      // Do not enumerate entries.
>+      return false;
>+
>+    }
>+  };
Nit: a lot of these blank lines are unnecessary.

>+  var cacheService =
>+    Components.classes["@mozilla.org/network/cache-service;1"]
>+              .getService(Components.interfaces.nsICacheService);
>+  cacheService.visitEntries(visitor);
[In theory you could write this in one statement.]

>\ No newline at end of file
Please put it back!

>       <hbox align="center">
>+      	<label id="cacheSizeInfo" flex="1"/>
>+      </hbox>
This should work as a plain <label id="cacheSizeInfo"/> without a box. 

>+      <hbox align="center">
>+      	<checkbox id="allowSmartSize" flex="1"
Similarly for the checkbox. But if you want to be really sneaky, put it inside a <vbox align="start"> instead. (Still lose the flex though.)

>+                  label="&cacheCheck.label;"
This is missing an access key.

>                value="&diskCache.label;"
>-               accesskey="&diskCache.accesskey;"
This access key should not have been removed.

>       </hbox>
>-
>       <vbox>
Don't need to remove that blank line.

>       <description>&docCache;</description>
What I overlooked is that this needs to be a label for the menulist, so it can have an accesskey and a control attribute. Sorry about that.

>+<!ENTITY diskCache.label                 "Use up to ">
Need to rename this entity too, I'm afraid.

>+<!ENTITY spaceMbytes                     "MB of space for the cache.">
You forgot to rename this in the XUL file...

>+cacheSizeInfo=Your cache is currently using %S MB of space.
Please change this and the space entity above to "disk space".

Comment 9

[Edmund Wong (:ewong)]

Attachment: Ported cach preferences from Bug 559942

Comment 10

[Edmund Wong (:ewong)]

Attachment: Ported cach preferences from Bug 559942 (v6)

Comment 11

[Ian Neal]

Just as a note the number of decimal places displayed is governed by the code in convertByteUnits http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/mozapps/downloads/DownloadUtils.jsm#394
In most cases this means being rounded to nearest integer.
The same code seems to get reused in various places throughout the codebase.

Comment 12

[Ian Neal]

Comment on attachment 488859 [details] [diff] [review]
Ported cach preferences from Bug 559942 (v6)

>+++ b/suite/common/pref/pref-cache.js
>@@ -38,6 +38,12 @@
>  * the terms of any one of the MPL, the GPL or the LGPL.
>  *
>  * ***** END LICENSE BLOCK ***** */
>+Components.utils.import("resource://gre/modules/DownloadUtils.jsm");
>+
>+function Startup()
>+{
>+	updateActualCacheSize();
>+}
Nit: fix indentation.

>+function updateActualCacheSize()
>+{
>+	var visitor = {
Nit: fix indentation.

>+  var cacheService =
>+    Components.classes["@mozilla.org/network/cache-service;1"]
>+              .getService(Components.interfaces.nsICacheService);
>+  cacheService.visitEntries(visitor);
Nit: as Neil said this could be written as one statement (.classes[] .getService() .visitEntries())

>+++ b/suite/common/pref/pref-cache.xul
>@@ -55,6 +55,9 @@
>       <preference id="browser.cache.disk.capacity"
>                   name="browser.cache.disk.capacity"
>                   type="int"/>
>+      <preference id="browser.cache.disk.smart_size.enabled"
>+      	          name="browser.cache.disk.smart_size.enabled"
>+                  type="bool"/>           
Nit: still a tab in name line and trailing spaces in type line.

>+      <description control="browserCacheCheckDocFrequency">&docCache.label;</description>
Nit: missing an accesskey so that the menu can be accessed from the keyboard (you've added it in the dtd file but not here).

Could you also make sure a bug is filed for updating the help content to reflect the changes to the pref pane.
r- (just so I see the revised patch)

Comment 13

[Edmund Wong (:ewong)]

Attachment: Ported cache preferences from Bug 559942 (v7)

Comment 14

[[e-mail]]

Comment on attachment 489155 [details] [diff] [review]
Ported cache preferences from Bug 559942 (v7)

> function ClearDiskAndMemCache()
> {
>   Components.classes["@mozilla.org/network/cache-service;1"]
>             .getService(Components.interfaces.nsICacheService)
>             .evictEntries(Components.interfaces.nsICache.STORE_ANYWHERE);
> }
Now you know where one of my comments came from :-)

>+        var sizeStrings = [Math.round(size),unit];
I'm not convinced that it's worth rounding further. If it's good enough for download manager, it should be good enough here ;-) (It also rounds 11,000,000 to 11MB when in fact it's actually closer to 10MB!) In fact, to be consistent with the input, maybe we should stick to MB, rather than guessing units.

>+                  type="bool"/>           
Nit: trailing whitespace

>+      <description control="browserCacheCheckDocFrequency"
>+                   accesskey="&docCache.accesskey;">&docCache.label;</description>
This needs to be a <label>, not a <description>, otherwise control= won't work.

>+<!ENTITY diskCacheUpTo.label             "Use up to ">
Don't put a trailing space in the text, there are already margins between elements.

Comment 15

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 489155 [details] [diff] [review]
Ported cache preferences from Bug 559942 (v7)

SCNR, drive-by:

>  * ***** END LICENSE BLOCK ***** */
>+Components.utils.import("resource://gre/modules/DownloadUtils.jsm");

Nit: Missing empty line after license block (cf. other JS files in the same directory).

>+function updateCacheSizeUI(cacheSizeEnabled)
>+{
>+   document.getElementById("browserCacheDiskCacheBefore").disabled = cacheSizeEnabled;
>+   document.getElementById("browserCacheDiskCache").disabled = cacheSizeEnabled;
>+   document.getElementById("browserCacheDiskCacheAfter").disabled = cacheSizeEnabled;
>+}
>+
>+function ReadSmartSizeEnabled()
>+{
>+   var enabled = document.getElementById("browser.cache.disk.smart_size.enabled").value;
>+   updateCacheSizeUI(enabled);
>+   return enabled;
>+}

Nit: 6x one space too much indentation.

>+function updateActualCacheSize()
>+{
>+  var visitor = {
>+
>+    visitDevice: function (deviceID, deviceInfo)

Nit: Useless empty line (note: this one might actually depend on the reviewer's taste).

Comment 16

[Edmund Wong (:ewong)]

Attachment: Ported cache preferences from Bug 559942

Comment 17

[[e-mail]]

Comment on attachment 489457 [details] [diff] [review]
Ported cache preferences from Bug 559942

>+      <hbox align="center">
>+        <checkbox id="allowSmartSize"
I'm not sure that's exactly the effect I wanted. You could remove the hbox, or you could change it to a <vbox align="start">.

Comment 18

[Edmund Wong (:ewong)]

Attachment: Ported cache preferences from Bug 559942 (v8)

Comment 19

[Edmund Wong (:ewong)]

Attachment: Ported cache preferences from Bug 559942 (v9)

Comment 20

[Edmund Wong (:ewong)]

(In reply to comment #12)
> Could you also make sure a bug is filed for updating the help content to
> reflect the changes to the pref pane.

Filed bug 611249 for this purpose

Comment 21

[Ian Neal]

Comment on attachment 489483 [details] [diff] [review]
Ported cache preferences from Bug 559942 (v9)

>+++ b/suite/locales/en-US/chrome/common/pref/pref-cache.dtd
>+<!ENTITY docCache.label                  "Compare the page in the cache to the page on the network:">
>+<!ENTITY docCache.accesskey              "M">
Nit: the accesskey should match the case of the letter in the label, so in this case "m" as it is in "Compare"

r=me with that fixed.

Comment 22

[Edmund Wong (:ewong)]

Attachment: Ported cache preferences from Bug 559942. (v10)

Comment 23

[[e-mail]]

Comment on attachment 491781 [details] [diff] [review]
Ported cache preferences from Bug 559942. (v10)

Sadly there were too many nits for me to give you sr-with-nits-addressed.

>  * ***** END LICENSE BLOCK ***** */
>+ 
Nit: unnecessary space character on blank line

>+  var visitor = {
>+
Nit: unnecessary blank line

>+        actualSizeLabel.value = sizeStr;
[I wonder whether there's enough room for all languages. Fortunately if it becomes a problem it's easy to switch this to use .textContent instead.]

>+     }
>+     // Do not enumerate entries
>+     return false;
The indentation on these lines is one space too few.

>+                  type="bool"/> 
Nit: trailing space

>+      <vbox align="start">
>+        <label id="cacheSizeInfo"/>
>+        <checkbox id="allowSmartSize"
Even better than my suggestion :-)

>+             accesskey="&docCache.accesskey;"
>+             value="&docCache.label;"/>
Nit: list the access key after the label.

>-            <radio value="1"
>-                   label="&everyTimeRadio.label;"
>-                   accesskey="&everyTimeRadio.accesskey;"/>
>-            <radio value="3"
>-                   label="&autoRadio.label;"
>-                   accesskey="&autoRadio.accesskey;"/>
>-          </vbox>
>-          <vbox flex="1">
>-            <radio value="0"
>-                   label="&oncePsessionRadio.label;"
>-                   accesskey="&oncePsessionRadio.accesskey;"/>
>-            <radio value="2"
>-                   label="&neverRadio.label;"
>-                   accesskey="&neverRadio.accesskey;"/>
Need to remove the access keys from the DTD.

>+            <menuitem value="1" label="&everyTimeRadio.label;"/>
>+            <menuitem value="3" label="&autoRadio.label;"/>
>+            <menuitem value="0" label="&oncePsessionRadio.label;"/>
>+            <menuitem value="2" label="&neverRadio.label;"/>
[Hmm, should we rename these, since they're no longer Radios?]

>+<!ENTITY diskCacheUpTo.label             "Use up to ">
No trailing space in the string please, the textbox already has a margin.

>+cacheSizeInfo=Your cache is currently using %1$S %2$S of disk space.
It might be a good idea to give this a l10n note.

Comment 24

[Edmund Wong (:ewong)]

(In reply to comment #23)
> Comment on attachment 491781 [details] [diff] [review]
> Ported cache preferences from Bug 559942. (v10)
> 
> Sadly there were too many nits for me to give you sr-with-nits-addressed.
Sorry for the nits.  

> >+  var visitor = {
> >+
> Nit: unnecessary blank line

Yes,  my bad and deserve a 'told you so' by InvisibleSmiley.  Sorry.

> 
> >+        actualSizeLabel.value = sizeStr;
> [I wonder whether there's enough room for all languages. Fortunately if it
> becomes a problem it's easy to switch this to use .textContent instead.]
> 
Changed to .textContent.

> >+     }
> >+     // Do not enumerate entries
> >+     return false;
> The indentation on these lines is one space too few.
> 
> >+                  type="bool"/> 
> Nit: trailing space
> 
> >+      <vbox align="start">
> >+        <label id="cacheSizeInfo"/>
> >+        <checkbox id="allowSmartSize"
> Even better than my suggestion :-)
> 
> >+             accesskey="&docCache.accesskey;"
> >+             value="&docCache.label;"/>
> Nit: list the access key after the label.
> 
> 
> >+<!ENTITY diskCacheUpTo.label             "Use up to ">
> No trailing space in the string please, the textbox already has a margin.
Again.  Me bad.

Comment 25

[Edmund Wong (:ewong)]

Attachment: Ported cach preferences from Bug 559942 (v11)

Comment 26

[Ian Neal]

Comment on attachment 491848 [details] [diff] [review]
Ported cach preferences from Bug 559942 (v11)

>+<!ENTITY docCache.label                  "Compare the page in the cache to the page on the network:">
>+<!ENTITY docCache.accesskey              "M">
This has the wrong case, should be "m"

Comment 27

[Edmund Wong (:ewong)]

Attachment: Ported cache preferences from Bug 559942. (v12) [Checkin: comment 28]

Fixed Nit from comment #26.

Comment 28

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 491995 [details] [diff] [review]
Ported cache preferences from Bug 559942. (v12) [Checkin: comment 28]

http://hg.mozilla.org/comm-central/rev/e5d2d6c17145

Comment 29

[Jens Hatlak (:InvisibleSmiley)]

Leaving the status change decision to you.

Comment 30

[Stefan [:stefanh]]

Edmund,
Is there a help bug for these changes filed? If not, would you mind filing one?

Comment 31

[Edmund Wong (:ewong)]

(In reply to comment #30)
> Edmund,
> Is there a help bug for these changes filed? If not, would you mind filing one?

Sorry for missing that step.  Just filed Bug # 613947.

Comment 32

[Ton [:Tonnes]]

Minor nit: remember never to use final stops in GUI options (like in pref-cache.dtd).

Comment 33

[Ton [:Tonnes]]

Attachment: Remove stops introduced by att 491995 [Checkin: comment 35]

… to address comment 32.

Comment 34

[Edmund Wong (:ewong)]

Comment on attachment 500662 [details] [diff] [review]
Remove stops introduced by att 491995 [Checkin: comment 35]

I'm not a reviewer for this code.  It looks ok from the feedback point of view.
Requesting review from Neil.  Also, this probably would've been better if a
new bug was filed.

Comment 35

[Stefan [:stefanh]]

http://hg.mozilla.org/comm-central/rev/7a4f9fdc7a74