Migrate SeaMonkey's Cache preferences to new pref window

RESOLVED FIXED in seamonkey2.0a1

Status

SeaMonkey
Preferences
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: Manuel Reimer, Assigned: Manuel Reimer)

Tracking

Trunk
seamonkey2.0a1

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 5 obsolete attachments)

(Assignee)

Description

10 years ago
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.13) Gecko/20080328 SeaMonkey/1.1.9
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.13) Gecko/20080328 SeaMonkey/1.1.9

No need for much text here...

Reproducible: Always
(Assignee)

Comment 1

10 years ago
Created attachment 328540 [details] [diff] [review]
First patch
Attachment #328540 - Flags: review?(mnyromyr)
(Assignee)

Updated

10 years ago
Blocks: 394522
(Assignee)

Updated

10 years ago
Summary: Migrate SeaMonkey's Keyboard Navigation preferences to new pref window → Migrate SeaMonkey's Cache preferences to new pref window
Assignee: nobody → Manuel.Spam
Status: UNCONFIRMED → NEW
Ever confirmed: true
Target Milestone: --- → seamonkey2.0alpha
Version: unspecified → Trunk

Comment 2

10 years ago
-      <textbox id="browserCacheDiskCache" size="5" preftype="int"
-                  prefstring="browser.cache.disk.capacity" prefattribute="value"/>
+      <textbox id="browserCacheDiskCache" size="5"
+               onchange="gPrefCapacity.value = this.value * 1024"/>
       <label value="&mbytes;"/>

Better to use the preference attribute here? And then use onsyncfrom/to preference?

Comment 3

10 years ago
Comment on attachment 328540 [details] [diff] [review]
First patch

>Index: suite/common/pref/pref-cache.xul
>===================================================================

> <!ENTITY % brandDTD SYSTEM "chrome://branding/locale/brand.dtd" >
Nit: Extra space before >
> %brandDTD;
> <!ENTITY % prefCacheDTD SYSTEM "chrome://communicator/locale/pref/pref-cache.dtd" >
Nit: Extra space before >
> %prefCacheDTD;
> ]>
> 
>+<overlay xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
>+
>+<prefpane id="cache_pane"
>+          label="&pref.cache.title;"
>+          script="chrome://communicator/content/pref/pref-cache.js">
You've not indented <prefpane> element or the rest below.

>+
>+      <textbox id="browserCacheDiskCache" size="5"
>+               onchange="gPrefCapacity.value = this.value * 1024"/>
Convert this to a textbox of type="number" and as stefanh says onsync* could be of use.

>       <label value="&mbytes;"/>
>       <button label="&clearDiskCache.label;" accesskey="&clearDiskCache.accesskey;"
>               oncommand="prefClearDiskAndMemCache();"
>               id="clearDiskCache"
>-              prefstring="pref.advanced.cache.disable_button.clear_disk"/>
>+              preference="pref.advanced.cache.disable_button.clear_disk"/>
>     </hbox>
Need to make more accessible with aria-labelledby or control.

> 
>     <vbox>
>       <label value="&diskCacheFolder.label;"/>
>       <hbox align="center">
>         <textbox id="browserCacheDiskCacheFolder" flex="1" readonly="true" class="uri-element"/>
Convert to using filefield

>         <button label="&chooseDiskCacheFolder.label;" accesskey="&chooseDiskCacheFolder.accesskey;"
>               oncommand="prefCacheSelectFolder();" id="chooseDiskCacheFolder"/>
>       </hbox>
>     </vbox>
Need to make more accessible with aria-labelledby or control.
(Assignee)

Comment 4

10 years ago
Created attachment 328882 [details] [diff] [review]
Second patch
Attachment #328540 - Attachment is obsolete: true
Attachment #328882 - Flags: review?(neil)
Attachment #328540 - Flags: review?(mnyromyr)

Comment 5

10 years ago
Comment on attachment 328882 [details] [diff] [review]
Second patch

>+  document.getElementById("chooseDiskCacheFolder").disabled =
>+    document.getElementById("browser.cache.disk.parent_directory").locked;
[Side note: I wish there was an easier way to do this.]

>+  return pref.value / 1024;
Please restore the old calculation, which truncated the result.

>-var gCacheParentDirectory;
>+  if (!pref.value) {
>+    try {
>       // no disk cache folder pref set; default to profile directory
>       var dirSvc = Components.classes["@mozilla.org/file/directory_service;1"]
>+                             .getService(Components.interfaces.nsIProperties);
>       if (dirSvc.has("ProfLD"))
>+        pref.value = dirSvc.get("ProfLD", Components.interfaces.nsILocalFile);
Instant apply strikes again... better switch back to the global I'm afraid.

>+  return undefined;
You probably don't need this, it's the default return value.

>-function prefClearCache(aType)
>-{
>+function prefClearCache(aType) {
Please restore the original file style for all these functions.
Attachment #328882 - Flags: review?(neil) → review-

Comment 6

10 years ago
Comment on attachment 328882 [details] [diff] [review]
Second patch

>Index: suite/common/pref/pref-cache.js
>===================================================================
>+function writeCacheDiskCapacity() {
Nit: functions should start with a capital.

>+  var field = document.getElementById("browserCacheDiskCache");
If you pass "this" or "this.value" into the function you don't need to do the line above.

>+  return field.value * 1024;
>+}
>+function Startup() {
>+  document.getElementById("chooseDiskCacheFolder").disabled =
>+    document.getElementById("browser.cache.disk.parent_directory").locked;
>+}
If pref.advanced.cache.disable_button.choose_folder preference is added (see further down) would need to check that status too.

>+
>+function readCacheFolder() {
Nit: functions should start with a capital.

>+  var pref = document.getElementById("browser.cache.disk.parent_directory");
>+  var field = document.getElementById("browserCacheDiskCacheFolder");
If you pass "this" into the function you don't need to do the line above.

>+function writeCacheFolder() {
>+  // Just echo the value already in preference
>+  var pref = document.getElementById("browser.cache.disk.parent_directory");
>+  return pref.value;
> }
Just don't do the onsynctopreference and it will do this anyway, so drop this function.

>+function prefCacheSelectFolder() {
Nit: functions should start with a capital.

>+function prefClearCache(aType) {
Nit: functions should start with a capital.
As this function is only called from the one place it should be merged into that function.

>     var classID = Components.classes["@mozilla.org/network/cache-service;1"];
>     var cacheService = classID.getService(Components.interfaces.nsICacheService);
>     cacheService.evictEntries(aType);
> }
> 
>+function prefClearDiskAndMemCache() {
Nit: functions should start with a capital.

>     prefClearCache(Components.interfaces.nsICache.STORE_ANYWHERE);
> }

>Index: suite/common/pref/pref-cache.xul
>===================================================================

>+        <textbox id="browserCacheDiskCache"
>+                 size="5"
>+                 type="number"
>+                 aria-labelledby="browserCacheDiskCacheBefore browserCacheDiskCache browserCacheDiskCacheAfter"
>+                 preference="browser.cache.disk.capacity"
>+                 onsyncfrompreference="return document.getElementById('cache_pane').readCacheDiskCapacity();"
>+                 onsynctopreference="return document.getElementById('cache_pane').writeCacheDiskCapacity();"/>
Call with "this" or "this.value", see comments for pref-cache.js

>+          <filefield id="browserCacheDiskCacheFolder"
>+                     flex="1"
>+                     preference="browser.cache.disk.parent_directory"
>+                     preference-editable="true"
>+                     onsyncfrompreference="return document.getElementById('cache_pane').readCacheFolder();"
Call with "this", see comments for pref-cache.js

>+                     onsynctopreference="return document.getElementById('cache_pane').writeCacheFolder();"/>
You could drop this attribute, see comments for pref-cache.js

>+          <button label="&chooseDiskCacheFolder.label;"
>+                  accesskey="&chooseDiskCacheFolder.accesskey;"
>+                  oncommand="prefCacheSelectFolder();"
>+                  id="chooseDiskCacheFolder"/>
It may be worth adding a new preference pref.advanced.cache.disable_button.choose_folder

Comment 7

10 years ago
(In reply to comment #5)
> (From update of attachment 328882 [details] [diff] [review])
> >+  document.getElementById("chooseDiskCacheFolder").disabled =
> >+    document.getElementById("browser.cache.disk.parent_directory").locked;
> [Side note: I wish there was an easier way to do this.]
There is, possibly, you could put an observe element as a child of the button element and have it observing the filefield's disabled attribute (see patch on bug 445009 for more info).
(Assignee)

Comment 8

10 years ago
Created attachment 330772 [details] [diff] [review]
Another patch
Attachment #328882 - Attachment is obsolete: true
Attachment #330772 - Flags: review?(iann_bugzilla)

Comment 9

10 years ago
Comment on attachment 330772 [details] [diff] [review]
Another patch

>Index: suite/common/pref/pref-cache.js
>===================================================================
>+// because the cache is in kilobytes, and the UI is in megabytes.
>+function ReadCacheDiskCapacity()
>+{
>+  var pref = document.getElementById("browser.cache.disk.capacity");
>+  return pref.value << 10;
>+}
Nit: need a blank line between end of this function and beginning of next.

>+function WriteCacheDiskCapacity(aField)
>+{
>+  return aField.value >> 10;
>+}

>Index: suite/common/pref/pref-cache.xul
>===================================================================
> <?xml-stylesheet href="chrome://communicator/skin/" type="text/css"?>
>+<?xml-stylesheet href="chrome://mozapps/content/preferences/preferences.css"?>
In theory this won't be need once the patch on bug 441403 lands as preferences.xul will already have it, there are also css changes in that patch which makes filefield element look better in both modern and classic.

r=me with the above nit fixed.
Attachment #330772 - Flags: review?(iann_bugzilla) → review+
Comment on attachment 330772 [details] [diff] [review]
Another patch

>+        <!-- <vbox class="indent" flex="1">
>+             <description>&prefetchDesc.label;</description>
>+          </vbox> -->

I think we should remove this, as well as the entity in pref-cache.dtd:
http://mxr.mozilla.org/seamonkey/source/suite/locales/en-US/chrome/common/pref/pref-cache.dtd#29

Comment 11

10 years ago
Created attachment 336278 [details] [diff] [review]
pref-cache patch v1.0

Simple nits fixed and submitting for sr on behalf of Manuel
Attachment #330772 - Attachment is obsolete: true
Attachment #336278 - Flags: superreview?(neil)
Attachment #336278 - Flags: review+

Updated

10 years ago
Attachment #336278 - Flags: superreview?(neil) → superreview+

Comment 12

10 years ago
Comment on attachment 336278 [details] [diff] [review]
pref-cache patch v1.0

>+// because the cache is in kilobytes, and the UI is in megabytes.
>+function ReadCacheDiskCapacity()
>+{
>+  var pref = document.getElementById("browser.cache.disk.capacity");
>+  return pref.value << 10;
>+}
> 
>+function WriteCacheDiskCapacity(aField)
>+{
>+  return aField.value >> 10;
>+}
The << and >> are the wrong way around here. (This should have been obvious when you opened the pref panel, as the default cache size should be 50MB.)

>+  if (file) {
>+    aField.file = file;
Great, this works with instantApply after all. Sorry for doubting you!

>+function ClearDiskAndMemCache()
> {
>+  var classID = Components.classes["@mozilla.org/network/cache-service;1"];
>+  var cacheService = classID.getService(Components.interfaces.nsICacheService);
>+  cacheService.evictEntries(Components.interfaces.nsICache.STORE_ANYWHERE);
> }
If you're going to reindent this you might as well eliminate the variables:
Components.classes[...]
          .getService(...)
          .evictEntries(...);

>+          <filefield id="browserCacheDiskCacheFolder"
>+                     flex="1"
>+                     preference="browser.cache.disk.parent_directory"
>+                     preference-editable="true"
>+                     onsyncfrompreference="return document.getElementById('cache_pane').ReadCacheFolder(this);"
Missing />

>+                  preference="network.prefetch-next" />
Nit: don't need a space before />

sr=me with these fixed.

>--->
>           <treeitem id="proxiesItem"
>                     label="&proxies.label;"
>                     prefpane="proxies_pane"
>                     helpTopic="advanced_pref_proxies"
>                     url="chrome://communicator/content/pref/pref-proxies.xul"/>
> <!--
Bah, I pulled just before you pushed, and I had to pull again :-(
(Assignee)

Comment 13

10 years ago
Created attachment 336354 [details] [diff] [review]
Patch with the last few mistakes fixed (Checkin: Comment 17)

Added the missing fixes and removed the obsolete CSS line which already is added by the fix in Bug 448106
Attachment #336278 - Attachment is obsolete: true
Attachment #336354 - Flags: superreview?(neil)
Attachment #336354 - Flags: review+

Comment 14

10 years ago
Comment on attachment 336354 [details] [diff] [review]
Patch with the last few mistakes fixed (Checkin: Comment 17)

>-      <!-- <vbox class="indent" flex="1">
>-        <description>&prefetchDesc.label;</description>
>-      </vbox> -->
Are we going to remove this from the .dtd too?
Attachment #336354 - Flags: superreview?(neil) → superreview+
(Assignee)

Comment 15

10 years ago
Yes. IMHO there is no need to keep unreferenced values in DTD files.

Comment 16

10 years ago
Does this need anything more than Checkin? Manuel, you don't have checkin acces, right, so this is in checkin-needed state?

Comment 17

10 years ago
Comment on attachment 336354 [details] [diff] [review]
Patch with the last few mistakes fixed (Checkin: Comment 17)

http://hg.mozilla.org/comm-central/rev/6781fe39f229 with some bracing style consistency tweaks
Attachment #336354 - Attachment description: Patch with the last few mistakes fixed → Patch with the last few mistakes fixed (Checkin: Comment 17)

Updated

10 years ago
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
You did not remove obsolete entity from pref-cache.dtd, see comments #10, #14 and #15
Looks like this checkin made one mochitest fail on all three unit test boxen:
Double id='bundle_prefutilities' detected in chrome://communicator/content/pref/preferences.xul:
  Tree 0:
    stringbundle bundle_prefutilities [2]
    prefwindow prefDialog [37]
    #document undefined [0]
  Tree 1:
    stringbundle bundle_prefutilities [1]
    prefpane cache_pane [35]
    prefwindow prefDialog [37]
    #document undefined [0]
*** 2357 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/chrome/suite/common/tests/chrome/test_idcheck.xul | check id: preferences.xul#bundle_prefutilities
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 20

10 years ago
(In reply to comment #18)
> You did not remove obsolete entity from pref-cache.dtd, see comments #10, #14
> and #15

Sorry I checked for it not being in the xul and assumed it had been removed from dtd, patch coming up to correct that and the mochitest issue

Comment 21

10 years ago
Created attachment 337201 [details] [diff] [review]
pref-cache patch v2.0

This patch:
* Renames the id for stringbundle
* Removes commented out entity
Attachment #337201 - Flags: superreview?(neil)
Attachment #337201 - Flags: review?(neil)

Updated

10 years ago
Attachment #337201 - Flags: superreview?(neil)
Attachment #337201 - Flags: superreview-
Attachment #337201 - Flags: review?(neil)

Comment 22

10 years ago
Comment on attachment 337201 [details] [diff] [review]
pref-cache patch v2.0

>-    <stringbundle id="bundle_prefutilities"
>+    <stringbundle id="prefCacheBundle"
>                   src="chrome://communicator/locale/pref/prefutilities.properties"/>
Fortunately the element that it's duplicating is the main bundle_prefutilties that's always present due to preferences.xul so we can just remove this one.

Comment 23

10 years ago
Created attachment 337202 [details] [diff] [review]
pref-cache patch v2.0a (Checkin: Comment 24)

Changes since v2.0:
* Just remove stringbundle instead of changing id.
Attachment #337201 - Attachment is obsolete: true
Attachment #337202 - Flags: superreview?(neil)
Attachment #337202 - Flags: review?(neil)

Updated

10 years ago
Attachment #337202 - Flags: superreview?(neil)
Attachment #337202 - Flags: superreview+
Attachment #337202 - Flags: review?(neil)
Attachment #337202 - Flags: review+

Comment 24

10 years ago
Comment on attachment 337202 [details] [diff] [review]
pref-cache patch v2.0a (Checkin: Comment 24)

http://hg.mozilla.org/comm-central/rev/e131fbe05148
Attachment #337202 - Attachment description: pref-cache patch v2.0a → pref-cache patch v2.0a (Checkin: Comment 24)

Updated

10 years ago
Status: REOPENED → RESOLVED
Last Resolved: 10 years ago10 years ago
Resolution: --- → FIXED
(In reply to comment #24)
> http://hg.mozilla.org/comm-central/rev/e131fbe05148

V.Fixed, regression part.
[
Linux comm-central dep unit test on 2008/09/06 11:08:49
(MacOSX still building)
Win2k3 comm-central dep unit test on 2008/09/06 10:45:55
]
You need to log in before you can comment on or make changes to this bug.