424504 - Store seen URL history in auxiliary file instead of a pref

Status: RESOLVED FIXED

(Other Applications :: ChatZilla, defect)

Description

[James Ross]

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.

Comment 1

[James Ross]

Attachment: Use TextLogger for storing the URLs in urls.txt

I love the TextLogger. Thanks Gijs! :D

Comment 2

[:Gijs (he/him)]

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.

Comment 3

[James Ross]

Attachment: 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.

Comment 4

[:Gijs (he/him)]

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! :-)

Comment 5

[James Ross]

Checked in --> FIXED.

Comment 6

[:Gijs (he/him)]

clearUserPref no longer throws. :-(

Comment 7

[:Gijs (he/him)]

Ack, I misremembered how this worked. Sorry for bugspam.