595394 - Error: formatURL: Couldn't find value for key: SIDEBAR_VERSION

Status: VERIFIED FIXED

(SeaMonkey :: Sidebar, defect)

Description

[Karsten Düsterloh]

On trunk browser startup, I get

Error: formatURL: Couldn't find value for key: SIDEBAR_VERSION
Source File: file:///XXX/obj/sr/mozilla/dist/bin/components/nsURLFormatter.js
Line: 131

Comment 1

[Serge Gautherie (:sgautherie)]

Reproduced locally with an Opt Windows downloaded tinderbox-build:
error reported in Error console.

Where it's defined:
http://mxr.mozilla.org/comm-central/search?string=SIDEBAR_VERSION&case=1&find=%2Fsuite%2F.*%5C.js%24
{
/suite/browser/browser-prefs.js
    * line 553 -- pref("sidebar.customize.all_panels.url", "http://sidebar-rdf.netscape.com/%LOCALE%/sidebar-rdf/%SIDEBAR_VERSION%/all-panels.rdf");

/suite/common/sidebar/sidebarOverlay.js
    * line 89 -- const SIDEBAR_VERSION = "0.1";
    * line 959 -- // %SIDEBAR_VERSION% --> Sidebar file format version (e.g. 0.1).
    * line 965 -- url = url.replace(/%SIDEBAR_VERSION%/g, SIDEBAR_VERSION);
}

Where it fails:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/urlformatter/src/nsURLFormatter.js#135
{
136 nsURLFormatterService.prototype = {
...
140   _defaults: {
...
159   },
160 
161   formatURL: function uf_formatURL(aFormat) {
}

I'm not sure how all this works exactly:
it looks like we either miss to include sidebarOverlay.js somewhere,
or that formatURL() just can't access to SIDEBAR_VERSION.

***

SeaMonkey 2.0.6 seems unaffected: regression.

Comment 2

[Philip Chee]

Likely suspect: Bug 563738
http://hg.mozilla.org/mozilla-central/rev/5e7e8e3404c7
http://hg.mozilla.org/mozilla-central/rev/662d1650cc5c

Comment 3

[:Gavin Sharp [email: [e-mail]]]

get_remote_datasource_url does %SIDEBAR_VERSION% replacement manually, after calling formatURL. What changed in bug 563738 is that the URL formatter now attempts to replace variables that include "_" (for BUILD_TARGET), and prints a message when it can't find a replacement. It's a bit odd to Cu.reportError there, since it doesn't actually fail (things work just as they did before). But maybe it is useful to catch unexpected failures to replace, I guess.

You can work around it by doing your %SIDEBAR_VERSION% replacement before calling formatURL.

Comment 4

[Serge Gautherie (:sgautherie)]

(In reply to comment #3)

Thanks for the explanation.

> get_remote_datasource_url does %SIDEBAR_VERSION% replacement manually, after
> calling formatURL.

(Almost) Right:
964   var url = formatter.formatURLPref("sidebar.customize.all_panels.url");
965   url = url.replace(/%SIDEBAR_VERSION%/g, SIDEBAR_VERSION);

Though it's formatURLPref() actually.

> You can work around it by doing your %SIDEBAR_VERSION% replacement before
> calling formatURL.

I'll do,
but it would be even better if formatURL*() had a second parameter to pass additional values.

Comment 5

[Marco Bonardo [:mak]]

(In reply to comment #3)
> You can work around it by doing your %SIDEBAR_VERSION% replacement before
> calling formatURL.

I'd not call this work around, since when you call a method that is expected to do replacements, should be common practice to do your specialized replacements before it.
But actually, the suggestion from Serge should have its own bug since it would be nice, one should be able to pass in an object with personal associations { SIDEBAR_VERSION: "1" } or even overrides of existing values and let the component do it for him.

Comment 6

[Serge Gautherie (:sgautherie)]

Attachment: (Av1) Call replace()+formatURL(), instead of formatURLPref()+replace() [Checked in: Comment 20]

NB: Implicitly using 'prefs' and 'urlFormatter' from Services.jsm. Should the latter be explicitly included?

Comment 7

[Serge Gautherie (:sgautherie)]

(In reply to comment #5)
> one should be able to pass in an object with personal associations {
> SIDEBAR_VERSION: "1" } or even overrides of existing values and let the
> component do it for him.

Bug 430235 ;-)

Comment 8

[Marco Bonardo [:mak]]

(In reply to comment #7)
> (In reply to comment #5)
> > one should be able to pass in an object with personal associations {
> > SIDEBAR_VERSION: "1" } or even overrides of existing values and let the
> > component do it for him.
> 
> Bug 430235 ;-)

yeah, even if urlformatter is used only by js, probably it should even be deCOM and converted to a module.

Comment 9

[Robert Kaiser]

Comment on attachment 474660 [details] [diff] [review]
(Av1) Call replace()+formatURL(), instead of formatURLPref()+replace()
[Checked in: Comment 20]

>+    url = prefs.getComplexValue("sidebar.customize.all_panels.url",
>+                                Components.interfaces.nsISupportsString)
>+               .data

Is this a localized pref? If so, does that call take that into account? URLformatter is written so that the pref can be localized, so we need to see if it is in this case react accordingly.

Unfortunately, the code is becoming uglier due to that. I don't agree with Marco as I think a replacement service should not need to care about things it cannot replace but just leave them unchanged.

Comment 10

[Marco Bonardo [:mak]]

(In reply to comment #9)
>  I don't agree with
> Marco as I think a replacement service should not need to care about things it
> cannot replace but just leave them unchanged.

This did not change, that code always worked that way (reporting errors for unknown entities) and this code was working just by chance (due to the _ replacement). I think working by chance is a bug regardless, and thanks we have that reportError or we would have never found it.

The fact the IDL forgets to tell what happens to unknown entities, is something we should also fix.

Imo, once we have the above possibility to define user specified mappings, the urlformatter should directly throw if it finds any unrecognized entity, this to avoid typo errors like %OS_VESRION% or implementers forgetting to remove a no more replaced variable.

Comment 11

[Robert Kaiser]

Well, I just don't agree that any error should be reported as long as custom fields are not possible. If they are, that's surely a good option.

IMHO, there is no bug right here, but still we need to work around this (non-)error message in a complicated way (i.e. multiple steps), by caring about fetching the (possibly localized) pref manually and then doing the replacement steps. In a world with nice code, that should not be needed.

But then, I'm much more for making things work with what we have and care about improvements later, so let's just do it. <rant>There's worse things around like thinking we need to re-invent wheels just because we don like the color of wheels we could get from elsewhere like FF.</rant>

Comment 12

[Serge Gautherie (:sgautherie)]

(In reply to comment #9)

> Is this a localized pref?

Its value is copied in comment 1.
I'm no expert, but it doesn't look like a localized pref to me...

> If so, does that call take that into account?

Implicitly not, as is.

> URLformatter is written so that the pref can be localized, so we need to see if
> it is in this case react accordingly.

Obviously, as I copied (part of) that code.

> Unfortunately, the code is becoming uglier due to that.

Any suggestion on how to make it less ugly?

Comment 13

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1) Improve nsIURLFormatter.idl documentation [Checked in: See comment 16]

(In reply to comment #10)

> The fact the IDL forgets to tell what happens to unknown entities, is something
> we should also fix.

Obviously! Here a patch, based on _current_ behavior ;-)

> Imo, once we have the above possibility to define user specified mappings, the
> urlformatter should directly throw if it finds any unrecognized entity, this to
> avoid typo errors like %OS_VESRION% or implementers forgetting to remove a no
> more replaced variable.

I agree with the rest of the discussion, but it's bug 430235 really...

Comment 14

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #11)
> IMHO, there is no bug right here, but still we need to work around this
> (non-)error message in a complicated way (i.e. multiple steps)

You could also just change the replacement variable syntax (e.g. using $SIDEBAR_VERSION$).

Comment 15

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 474705 [details] [diff] [review]
(Bv1) Improve nsIURLFormatter.idl documentation
[Checked in: See comment 16]

>diff --git a/toolkit/components/urlformatter/public/nsIURLFormatter.idl b/toolkit/components/urlformatter/public/nsIURLFormatter.idl

> interface nsIURLFormatter: nsISupports

>+   * Replacements of such variables should be done before calling this method.

I would just omit this comment (or recommend using a different variable format, but that's probably overkill).

Comment 16

[Serge Gautherie (:sgautherie)]

Comment on attachment 474705 [details] [diff] [review]
(Bv1) Improve nsIURLFormatter.idl documentation
[Checked in: See comment 16]

http://hg.mozilla.org/mozilla-central/rev/713b60f14ff8
Bv1, with comment 15 suggestion(s).

Comment 17

[Serge Gautherie (:sgautherie)]

(In reply to comment #14)
> You could also just change the replacement variable syntax (e.g. using
> $SIDEBAR_VERSION$).

True. Though I would rather not go that way.

Comment 18

[:Gavin Sharp [email: [e-mail]]]

Why not? It's a much simpler/safer change.

Comment 19

[Serge Gautherie (:sgautherie)]

(In reply to comment #18)

Because I prefer not to touch the preference value.
Because I'm more interested in bug 430235 in the long run.
Yet, if the reviewer prefers the '$' solution, I would be happy to do it, as long as this bug gets fixed.

Comment 20

[Serge Gautherie (:sgautherie)]

Comment on attachment 474660 [details] [diff] [review]
(Av1) Call replace()+formatURL(), instead of formatURLPref()+replace()
[Checked in: Comment 20]

http://hg.mozilla.org/comm-central/rev/ffb9bb96ed39

Comment 21

[Serge Gautherie (:sgautherie)]

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1285026303.1285029906.31507.gz&fulltext=1
Linux comm-central-trunk debug test mochitest-other on 2010/09/20 16:45:03
{
TEST-INFO | chrome://mochikit/content/browser/docshell/test/browser/browser_bug388121-2.js | Console message: [JavaScript Error: "formatURL: Couldn't find value for key: SIDEBAR_VERSION" {file: "file:///builds/slave/comm-central-trunk-linux-debug-unittest-mochitest-other/build/seamonkey/components/nsURLFormatter.js" line: 131}]

and other tests.
}

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1285144344.1285147704.25009.gz&fulltext=1
Linux comm-central-trunk debug test mochitest-other on 2010/09/22 01:32:24

V.Fixed