409691 - XPCOMUtilify nsURLFormatter and add tests

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Ryan Flint [:rflint] (ping via IRC for reviews)]

Attachment: Patch

Found a few potential issues while writing the tests for this:
First is http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/toolkit/components/urlformatter/src/nsURLFormatter.js&rev=1.5&mark=82-94,103-104,108#82 where aFormat.replace() is using a case-insensitive RegExp but then checking against uppercase keys, so any lowercase key is simply removed.

Second is http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/toolkit/components/urlformatter/src/nsURLFormatter.js&rev=1.5&mark=94#93 where only the first space is replaced because the RegExp isn't global.

These looked a bit odd to me, but I wasn't sure if they were intentional or not and what effects changing them would have on consumers of this, so I left them out of this version of the patch.

Comment 1

[Dietrich Ayala (:dietrich)]

(In reply to comment #0)
> Created an attachment (id=294482) [details]
> Patch
> 
> Found a few potential issues while writing the tests for this:
> First is
> http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/toolkit/components/urlformatter/src/nsURLFormatter.js&rev=1.5&mark=82-94,103-104,108#82
> where aFormat.replace() is using a case-insensitive RegExp but then checking
> against uppercase keys, so any lowercase key is simply removed.
> 

could probably remove case-insenstivity from the regex.

> Second is
> http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/toolkit/components/urlformatter/src/nsURLFormatter.js&rev=1.5&mark=94#93
> where only the first space is replaced because the RegExp isn't global.
>

hm, i don't remember offhand why this is here at all. probably best to spin off and look at the consumers of this key.

Comment 2

[Dietrich Ayala (:dietrich)]

Comment on attachment 294482 [details] [diff] [review]
Patch

r=me, thanks. about the case insensitivity: i think we should probably just support uppercase keys in order to reduce error by requiring more specificity, as well as being able to easily pick out these keys when creating/editing prefs that use them.

Comment 3

[Ryan Flint [:rflint] (ping via IRC for reviews)]

Attachment: For checkin

Cleans up this component and adds regression tests.

Comment 4

[Mike Schroepfer]

Why do we want this?

Comment 5

[Ryan Flint [:rflint] (ping via IRC for reviews)]

(In reply to comment #4)
> Why do we want this?
> 

dascher recently blogged about the benefits of this type of cleanup: http://ascher.ca/blog/2008/01/04/gateway-drug-for-thunderbird-devs/

This patch also adds tests to a component that's a pretty instrumental to how our in-product pages work and will ensure any regressions that might affect them will be caught immediately.

Comment 6

[Mike Schroepfer]

Comment on attachment 295324 [details] [diff] [review]
For checkin

k - thanks!

Comment 7

[Ryan Flint [:rflint] (ping via IRC for reviews)]

mozilla/toolkit/components/urlformatter/Makefile.in 1.2
mozilla/toolkit/components/urlformatter/src/Makefile.in 1.3
mozilla/toolkit/components/urlformatter/src/nsURLFormatter.js 1.6
mozilla/toolkit/components/urlformatter/tests/unit/head_urlformatter.js 1.1
mozilla/toolkit/components/urlformatter/tests/unit/test_urlformatter.js 1.1