Store seen URL history in auxiliary file instead of a pref

RESOLVED FIXED

Status

RESOLVED FIXED
11 years ago
9 years ago

People

(Reporter: bugzilla-mozilla-20000923, Assigned: bugzilla-mozilla-20000923)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [cz-0.9.86])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

11 years ago
We currently store all seen URLs in the array pref urls.list. This is not great for prefs as, although it works, it results in a huge entry in prefs.js (100 URLs, escaped, on one line).

We should use a simple text file, urls.txt, say. As performance is a possible issue, I would suggest that we open for append, write one URL, and close for normal operation. Periodically we can open, read all, discard old ones beyond the limit, and write it all out. Not sure if we want to keep the list loaded.

This would save memory, especially so if we don't keep the list loaded.
(Assignee)

Updated

10 years ago
Depends on: 384040
(Assignee)

Comment 1

9 years ago
Created attachment 394743 [details] [diff] [review]
Use TextLogger for storing the URLs in urls.txt

I love the TextLogger. Thanks Gijs! :D
Assignee: rginda → silver
Status: NEW → ASSIGNED
Attachment #394743 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 394743 [details] [diff] [review]
Use TextLogger for storing the URLs in urls.txt

>Index: static.js
<snip>
>+    catch (ex)
>+    {
>+        msg = getMsg(MSG_ERR_URLS_NOT_WRITABLE, urlsFile.path);
Your patch misses the definition of this...


>+    try
>+    {
>+        /* Migrate old list preference to file. We're doing this by hand, so
>+         * we can completely drop the prefs.js preference. The getCharPref()
>+         * call will throw if it can't find the preference.
>+         */
>+        var oldUrls = client.prefManager.prefBranch.getCharPref("urls.list");
>+        oldUrls = client.prefManager.stringToArray(oldUrls);
>+        if (client.urlLogger && (oldUrls.length > 0))
>+        {
>+            for (var i = 0; i < oldUrls.length; i++)
>+                client.urlLogger.append(oldUrls[i]);
>+        }
>+        client.prefManager.prefBranch.clearUserPref("urls.list");
>+    }
>+    catch (ex)
>+    {
>+    }

This leads to dataloss if creating the url logger failed (above). I'd like it to not clear the pref if it can't write the file out, so that the user can fix why-ever they can't write to that file, and then restart ChatZilla, and have things "Just Work".

(even if I never use the url list, and wouldn't really care myself :-))

I'd also technically like less of that code in the try/catch block, because it might fail in other mysterious ways (the write-to-file call that's implicit in urlLogger.append might fail, for instance), and right now those exceptions are going down the drain of no return, which is less than ideal, too.

Is there a cleaner way to check if the pref exists? That might make more sense, even if it does mean more XPCOM calls in the case where we do have to convert the list of URLs. Which only occurs once, in the normal case.

r- because of the dataloss.
Attachment #394743 - Flags: review?(gijskruitbosch+bugs) → review-
(Assignee)

Comment 3

9 years ago
Created attachment 400273 [details] [diff] [review]
Updated migration code

I also found a bug in the /urls command: it was showing the oldest not newest entries. I fixed with .reverse() (which does in-place reverse, which is fast) but could be persuaded otherwise.
Attachment #394743 - Attachment is obsolete: true
Attachment #400273 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 400273 [details] [diff] [review]
Updated migration code

<snip>
>Index: xul/content/prefs.js
>@@ -821,6 +820,14 @@
>             updateSpellcheck(newValue);
>             break;
> 
>+        case "urls.store.max":
>+            if (client.urlLogger)
>+            {
>+                client.urlLogger.autoLimit = client.prefs["urls.store.max"];
>+                client.urlLogger.limit(client.prefs["urls.store.max"]);
>+            }
>+            break;
>+

Nit: use newValue here, to avoid querying the pref service again, and keep the number of chars on the line much smaller :-).

Otherwise, r=me, thanks! :-)
Attachment #400273 - Flags: review?(gijskruitbosch+bugs) → review+
(Assignee)

Comment 5

9 years ago
Checked in --> FIXED.
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Whiteboard: [cz-0.9.86]
clearUserPref no longer throws. :-(
Status: RESOLVED → REOPENED
Depends on: 487059
Resolution: FIXED → ---
Ack, I misremembered how this worked. Sorry for bugspam.
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago9 years ago
No longer depends on: 487059
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.