Update cache preferences for bug 559942

RESOLVED FIXED in seamonkey2.1b2

Status

SeaMonkey
Preferences
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: Robert Kaiser, Assigned: ewong)

Tracking

Trunk
seamonkey2.1b2

Firefox Tracking Flags

(blocking-seamonkey2.1 final+)

Details

(Whiteboard: [good first bug])

Attachments

(2 attachments, 12 obsolete attachments)

8.96 KB, patch
ewong
: review+
Details | Diff | Splinter Review
1.42 KB, patch
neil@parkwaycc.co.uk
: review+
ewong
: feedback+
Details | Diff | Splinter Review
(Reporter)

Description

7 years ago
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.
(Reporter)

Comment 1

7 years ago
Probably need to block 2.1 final for this trivial task.
blocking-seamonkey2.1: --- → ?
(Reporter)

Updated

7 years ago
Whiteboard: [good first bug]
(Assignee)

Updated

7 years ago
Assignee: nobody → ewong
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?

Updated

7 years ago
blocking-seamonkey2.1: ? → final+
Version: unspecified → Trunk
(Assignee)

Updated

7 years ago
Status: NEW → ASSIGNED
(Reporter)

Comment 3

7 years ago
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.
(Assignee)

Comment 4

7 years ago
Created attachment 488751 [details] [diff] [review]
Ported cache preferences from bug 559952.
Attachment #488751 - Flags: superreview?(neil)
Attachment #488751 - Flags: review?
(Assignee)

Updated

7 years ago
Attachment #488751 - Flags: review? → review?(iann_bugzilla)
(Assignee)

Updated

7 years ago
Attachment #488751 - Flags: superreview?(neil)
Attachment #488751 - Flags: review?(iann_bugzilla)
(Assignee)

Comment 5

7 years ago
Created attachment 488760 [details] [diff] [review]
Ported cache preferences from bug 559952.

Reordered the attributes within the <label> element.
Attachment #488751 - Attachment is obsolete: true
(Assignee)

Comment 6

7 years ago
Created attachment 488799 [details] [diff] [review]
Ported cach preferences from Bug 559952

Fixed a bug in the previous patch.
Attachment #488760 - Attachment is obsolete: true
Attachment #488799 - Flags: superreview?(neil)
Attachment #488799 - Flags: review?
(Assignee)

Updated

7 years ago
Attachment #488799 - Flags: review? → review?(iann_bugzilla)
(Assignee)

Comment 7

7 years ago
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.
Attachment #488799 - Attachment is obsolete: true
Attachment #488804 - Flags: superreview?(neil)
Attachment #488804 - Flags: review?(iann_bugzilla)
Attachment #488799 - Flags: superreview?(neil)
Attachment #488799 - Flags: review?(iann_bugzilla)

Comment 8

7 years ago
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".
Attachment #488804 - Flags: superreview?(neil) → superreview-
(Assignee)

Comment 9

7 years ago
Created attachment 488858 [details] [diff] [review]
Ported cach preferences from Bug 559942
Attachment #488804 - Attachment is obsolete: true
Attachment #488858 - Flags: review?(iann_bugzilla)
Attachment #488804 - Flags: review?(iann_bugzilla)
(Assignee)

Updated

7 years ago
Attachment #488858 - Attachment description: Ported cach preferences from Bug 559952 → Ported cach preferences from Bug 559942
(Assignee)

Comment 10

7 years ago
Created attachment 488859 [details] [diff] [review]
Ported cach preferences from Bug 559942 (v6)
Attachment #488858 - Attachment is obsolete: true
Attachment #488859 - Flags: review?(iann_bugzilla)
Attachment #488858 - Flags: review?(iann_bugzilla)

Updated

7 years ago
Attachment #488859 - Attachment is patch: true
Attachment #488859 - Attachment mime type: application/octet-stream → text/plain

Comment 11

7 years ago
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

7 years ago
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)
Attachment #488859 - Flags: review?(iann_bugzilla) → review-
(Assignee)

Comment 13

7 years ago
Created attachment 489155 [details] [diff] [review]
Ported cache preferences from Bug 559942 (v7)
Attachment #488859 - Attachment is obsolete: true
Attachment #489155 - Flags: review?(iann_bugzilla)
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 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).
(Assignee)

Comment 16

7 years ago
Created attachment 489457 [details] [diff] [review]
Ported cache preferences from Bug 559942
Attachment #489155 - Attachment is obsolete: true
Attachment #489457 - Flags: review?(iann_bugzilla)
Attachment #489155 - Flags: review?(iann_bugzilla)
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">.
(Assignee)

Comment 18

7 years ago
Created attachment 489482 [details] [diff] [review]
Ported cache preferences from Bug 559942 (v8)
Attachment #489457 - Attachment is obsolete: true
Attachment #489482 - Flags: review?(iann_bugzilla)
Attachment #489457 - Flags: review?(iann_bugzilla)
(Assignee)

Comment 19

7 years ago
Created attachment 489483 [details] [diff] [review]
Ported cache preferences from Bug 559942 (v9)
Attachment #489482 - Attachment is obsolete: true
Attachment #489483 - Flags: review?(iann_bugzilla)
Attachment #489482 - Flags: review?(iann_bugzilla)
(Assignee)

Comment 20

7 years ago
(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

7 years ago
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.
Attachment #489483 - Flags: review?(iann_bugzilla) → review+
(Assignee)

Comment 22

7 years ago
Created attachment 491781 [details] [diff] [review]
Ported cache preferences from Bug 559942. (v10)
Attachment #489483 - Attachment is obsolete: true
Attachment #491781 - Flags: superreview?(neil)
Attachment #491781 - Flags: review+
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.
Attachment #491781 - Flags: superreview?(neil) → superreview-
(Assignee)

Comment 24

7 years ago
(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.
(Assignee)

Comment 25

7 years ago
Created attachment 491848 [details] [diff] [review]
Ported cach preferences from Bug 559942 (v11)
Attachment #491781 - Attachment is obsolete: true
Attachment #491848 - Flags: superreview?(neil)
Attachment #491848 - Flags: review+

Updated

7 years ago
Attachment #491848 - Flags: superreview?(neil) → superreview+

Comment 26

7 years ago
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"
(Assignee)

Comment 27

7 years ago
Created attachment 491995 [details] [diff] [review]
Ported cache preferences from Bug 559942. (v12) [Checkin: comment 28]

Fixed Nit from comment #26.
Attachment #491848 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Attachment #491995 - Flags: superreview+
Attachment #491995 - Flags: review+

Updated

7 years ago
Keywords: checkin-needed
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
Attachment #491995 - Attachment description: Ported cache preferences from Bug 559942. (v12) → Ported cache preferences from Bug 559942. (v12) [Checkin: comment 28]
Leaving the status change decision to you.
Keywords: checkin-needed
Target Milestone: --- → seamonkey2.1b2
(Assignee)

Updated

7 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED

Comment 30

7 years ago
Edmund,
Is there a help bug for these changes filed? If not, would you mind filing one?
(Assignee)

Comment 31

7 years ago
(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

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

Comment 33

7 years ago
Created attachment 500662 [details] [diff] [review]
Remove stops introduced by att 491995 [Checkin: comment 35]

… to address comment 32.
Attachment #500662 - Flags: review?(ewong)
(Assignee)

Comment 34

7 years ago
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.
Attachment #500662 - Flags: review?(neil)
Attachment #500662 - Flags: review?(ewong)
Attachment #500662 - Flags: feedback+

Updated

7 years ago
Attachment #500662 - Flags: review?(neil) → review+

Updated

7 years ago
Keywords: checkin-needed

Comment 35

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

Updated

7 years ago
Attachment #500662 - Attachment description: Remove stops introduced by att 491995 → Remove stops introduced by att 491995 [Checkin: comment 34]

Updated

7 years ago
Keywords: checkin-needed

Updated

7 years ago
Attachment #500662 - Attachment description: Remove stops introduced by att 491995 [Checkin: comment 34] → Remove stops introduced by att 491995 [Checkin: comment 35]
You need to log in before you can comment on or make changes to this bug.