Last Comment Bug 594744 - Update cache preferences for bug 559942
: Update cache preferences for bug 559942
Status: RESOLVED FIXED
[good first bug]
:
Product: SeaMonkey
Classification: Client Software
Component: Preferences (show other bugs)
: Trunk
: All All
: -- normal (vote)
: seamonkey2.1b2
Assigned To: Edmund Wong (:ewong)
:
:
Mentors:
Depends on: 559942
Blocks:
  Show dependency treegraph
 
Reported: 2010-09-09 06:09 PDT by Robert Kaiser
Modified: 2011-01-03 09:10 PST (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
final+


Attachments
Ported cache preferences from bug 559952. (6.75 KB, patch)
2010-11-07 08:09 PST, Edmund Wong (:ewong)
no flags Details | Diff | Splinter Review
Ported cache preferences from bug 559952. (6.76 KB, patch)
2010-11-07 09:07 PST, Edmund Wong (:ewong)
no flags Details | Diff | Splinter Review
Ported cach preferences from Bug 559952 (6.88 KB, patch)
2010-11-07 19:02 PST, Edmund Wong (:ewong)
no flags Details | Diff | Splinter Review
Ported cach preferences from Bug 559952 (6.88 KB, patch)
2010-11-07 19:28 PST, Edmund Wong (:ewong)
neil: superreview-
Details | Diff | Splinter Review
Ported cach preferences from Bug 559942 (8.18 KB, patch)
2010-11-08 06:49 PST, Edmund Wong (:ewong)
no flags Details | Diff | Splinter Review
Ported cach preferences from Bug 559942 (v6) (8.18 KB, patch)
2010-11-08 06:52 PST, Edmund Wong (:ewong)
iann_bugzilla: review-
Details | Diff | Splinter Review
Ported cache preferences from Bug 559942 (v7) (8.26 KB, patch)
2010-11-09 06:37 PST, Edmund Wong (:ewong)
no flags Details | Diff | Splinter Review
Ported cache preferences from Bug 559942 (8.19 KB, patch)
2010-11-10 01:58 PST, Edmund Wong (:ewong)
no flags Details | Diff | Splinter Review
Ported cache preferences from Bug 559942 (v8) (8.26 KB, patch)
2010-11-10 07:17 PST, Edmund Wong (:ewong)
no flags Details | Diff | Splinter Review
Ported cache preferences from Bug 559942 (v9) (8.19 KB, patch)
2010-11-10 07:29 PST, Edmund Wong (:ewong)
iann_bugzilla: review+
Details | Diff | Splinter Review
Ported cache preferences from Bug 559942. (v10) (8.19 KB, patch)
2010-11-18 23:26 PST, Edmund Wong (:ewong)
ewong: review+
neil: superreview-
Details | Diff | Splinter Review
Ported cach preferences from Bug 559942 (v11) (8.96 KB, patch)
2010-11-19 08:48 PST, Edmund Wong (:ewong)
ewong: review+
neil: superreview+
Details | Diff | Splinter Review
Ported cache preferences from Bug 559942. (v12) [Checkin: comment 28] (8.96 KB, patch)
2010-11-19 16:33 PST, Edmund Wong (:ewong)
ewong: review+
ewong: superreview+
Details | Diff | Splinter Review
Remove stops introduced by att 491995 [Checkin: comment 35] (1.42 KB, patch)
2011-01-02 04:29 PST, Ton
neil: review+
ewong: feedback+
Details | Diff | Splinter Review

Description Robert Kaiser 2010-09-09 06:09:02 PDT
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 2010-09-09 06:09:34 PDT
Probably need to block 2.1 final for this trivial task.
Comment 2 Byron Milligan [:byronm] 2010-09-09 10:22:29 PDT
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 2010-09-21 07:11:13 PDT
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) 2010-11-07 08:09:48 PST
Created attachment 488751 [details] [diff] [review]
Ported cache preferences from bug 559952.
Comment 5 Edmund Wong (:ewong) 2010-11-07 09:07:35 PST
Created attachment 488760 [details] [diff] [review]
Ported cache preferences from bug 559952.

Reordered the attributes within the <label> element.
Comment 6 Edmund Wong (:ewong) 2010-11-07 19:02:00 PST
Created attachment 488799 [details] [diff] [review]
Ported cach preferences from Bug 559952

Fixed a bug in the previous patch.
Comment 7 Edmund Wong (:ewong) 2010-11-07 19:28:57 PST
Created attachment 488804 [details] [diff] [review]
Ported cach preferences from Bug 559952

Changed a dtd definition so that localizers can determine the string property has changed.
Comment 8 neil@parkwaycc.co.uk 2010-11-08 04:15:19 PST
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) 2010-11-08 06:49:56 PST
Created attachment 488858 [details] [diff] [review]
Ported cach preferences from Bug 559942
Comment 10 Edmund Wong (:ewong) 2010-11-08 06:52:17 PST
Created attachment 488859 [details] [diff] [review]
Ported cach preferences from Bug 559942 (v6)
Comment 11 Ian Neal 2010-11-08 08:06:42 PST
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 2010-11-08 08:27:07 PST
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) 2010-11-09 06:37:18 PST
Created attachment 489155 [details] [diff] [review]
Ported cache preferences from Bug 559942 (v7)
Comment 14 neil@parkwaycc.co.uk 2010-11-09 09:30:11 PST
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) 2010-11-09 15:53:22 PST
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) 2010-11-10 01:58:24 PST
Created attachment 489457 [details] [diff] [review]
Ported cache preferences from Bug 559942
Comment 17 neil@parkwaycc.co.uk 2010-11-10 03:19:35 PST
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) 2010-11-10 07:17:20 PST
Created attachment 489482 [details] [diff] [review]
Ported cache preferences from Bug 559942 (v8)
Comment 19 Edmund Wong (:ewong) 2010-11-10 07:29:00 PST
Created attachment 489483 [details] [diff] [review]
Ported cache preferences from Bug 559942 (v9)
Comment 20 Edmund Wong (:ewong) 2010-11-10 23:21:39 PST
(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 2010-11-17 14:16:10 PST
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) 2010-11-18 23:26:53 PST
Created attachment 491781 [details] [diff] [review]
Ported cache preferences from Bug 559942. (v10)
Comment 23 neil@parkwaycc.co.uk 2010-11-19 03:03:24 PST
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) 2010-11-19 08:41:32 PST
(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) 2010-11-19 08:48:30 PST
Created attachment 491848 [details] [diff] [review]
Ported cach preferences from Bug 559942 (v11)
Comment 26 Ian Neal 2010-11-19 14:53:16 PST
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) 2010-11-19 16:33:13 PST
Created attachment 491995 [details] [diff] [review]
Ported cache preferences from Bug 559942. (v12) [Checkin: comment 28]

Fixed Nit from comment #26.
Comment 28 Jens Hatlak (:InvisibleSmiley) 2010-11-21 14:07:01 PST
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) 2010-11-21 14:08:04 PST
Leaving the status change decision to you.
Comment 30 Stefan [:stefanh] (away until December 6) 2010-11-22 00:10:04 PST
Edmund,
Is there a help bug for these changes filed? If not, would you mind filing one?
Comment 31 Edmund Wong (:ewong) 2010-11-22 06:38:37 PST
(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 2010-11-22 14:47:38 PST
Minor nit: remember never to use final stops in GUI options (like in pref-cache.dtd).
Comment 33 Ton 2011-01-02 04:29:31 PST
Created attachment 500662 [details] [diff] [review]
Remove stops introduced by att 491995 [Checkin: comment 35]

… to address comment 32.
Comment 34 Edmund Wong (:ewong) 2011-01-02 19:52:25 PST
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] (away until December 6) 2011-01-03 09:08:01 PST
http://hg.mozilla.org/comm-central/rev/7a4f9fdc7a74

Note You need to log in before you can comment on or make changes to this bug.