359093 - need to encode version string for the app.update.never.X pref name

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[(not reading, please use [e-mail] instead)]

need to encode version string for the app.update.never.X pref name

from bug #359082

Since we use the version string as part of the "never" pref name, and the pref
API expect pref names to be ascii, there will be some expected results.

For example, for the jp snippet I attached (to bug #359082), we generate a never pref name of app.update.never.LzáY‹

To address this, I'd need to encode the potential non-ascii version string
(using encodeURIComponent()) in these two places:

http://lxr.mozilla.org/mozilla1.8/source/toolkit/mozapps/update/src/nsUpdateService.js.in#1392
http://lxr.mozilla.org/mozilla1.8/source/toolkit/mozapps/update/content/updates.js#233

Comment 1

[(not reading, please use [e-mail] instead)]

patch for the trunk coming, includes support for "app.update.log.all", per nick thomas (see https://bugzilla.mozilla.org/show_bug.cgi?id=349271#c3)

Comment 2

[(not reading, please use [e-mail] instead)]

Attachment: test case

the never pref for this file should be:

app.update.never.%E3%81%8C%E7%99%BA%E4%BF%A1%E3%81%99%E3%82%8B%E6%9C%80%E6%96%B0%E6%83%85%E5%A0%B1%E3%81%AE%E7%BF%BB%E8%A8%B3%E7%89%88%E3%81%A7%E3%81%99%E3%80%82

Comment 3

[(not reading, please use [e-mail] instead)]

Attachment: the fix (including the "all" logging module)

Comment 4

[(not reading, please use [e-mail] instead)]

Comment on attachment 244857 [details] [diff] [review]
the fix (including the "all" logging module)

seeking review from dietrich.

note, we don't currently have plans (yet) to send over non-ascii version numbers, but this has been discussed and may happen.

Comment 5

[Dietrich Ayala (:dietrich)]

Comment on attachment 244857 [details] [diff] [review]
the fix (including the "all" logging module)

Seth, this looks good to me, r+.

However, I'm not sure I have the credentials for toolkit reviewer-ship. You should probably request additional review from a toolkit peer.

Comment 6

[(not reading, please use [e-mail] instead)]

Comment on attachment 244857 [details] [diff] [review]
the fix (including the "all" logging module)

seeking toolkit peer review from robert.

Comment 7

[Robert Strong (they/them - no direct email)]

Comment on attachment 244857 [details] [diff] [review]
the fix (including the "all" logging module)

Looks good

Comment 8

[(not reading, please use [e-mail] instead)]

fixed.

Checking in content/updates.js;
/cvsroot/mozilla/toolkit/mozapps/update/content/updates.js,v  <--  updates.js
new revision: 1.71; previous revision: 1.70
done
Checking in src/nsUpdateService.js.in;
/cvsroot/mozilla/toolkit/mozapps/update/src/nsUpdateService.js.in,v  <--  nsUpdateService.js.in
new revision: 1.124; previous revision: 1.123
done

Comment 9

[(not reading, please use [e-mail] instead)]

I've added a litmus test case for this issue, see http://litmus.mozilla.org/show_test.cgi?id=2743