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

VERIFIED FIXED in seamonkey2.1b1

Status

SeaMonkey
Sidebar
VERIFIED FIXED
7 years ago
7 years ago

People

(Reporter: Karsten Düsterloh, Assigned: sgautherie)

Tracking

(Depends on: 1 bug, {regression})

Trunk
seamonkey2.1b1
regression
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

7 years ago
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
(Assignee)

Comment 1

7 years ago
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.
Keywords: regression, regressionwindow-wanted
OS: Linux → All
Hardware: x86_64 → All
(Assignee)

Updated

7 years ago
status-seamonkey2.1: --- → ?
(Reporter)

Updated

7 years ago
Component: Search → Sidebar
QA Contact: search → sidebar

Comment 2

7 years ago
Likely suspect: Bug 563738
http://hg.mozilla.org/mozilla-central/rev/5e7e8e3404c7
http://hg.mozilla.org/mozilla-central/rev/662d1650cc5c
(Assignee)

Updated

7 years ago
Blocks: 563738
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.
(Assignee)

Comment 4

7 years ago
(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.
Assignee: nobody → sgautherie.bz
Status: NEW → ASSIGNED
Keywords: regressionwindow-wanted
(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.
(Assignee)

Comment 6

7 years ago
Created attachment 474660 [details] [diff] [review]
(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?
Attachment #474660 - Flags: review?(mnyromyr)
(Assignee)

Comment 7

7 years ago
(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 ;-)
Depends on: 430235
Flags: in-testsuite-
(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

7 years ago
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.
(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

7 years ago
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>
(Assignee)

Comment 12

7 years ago
(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?
(Assignee)

Comment 13

7 years ago
Created attachment 474705 [details] [diff] [review]
(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...
Attachment #474705 - Flags: review?
(Assignee)

Updated

7 years ago
Attachment #474705 - Flags: review? → review?(dietrich)
(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 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).
Attachment #474705 - Flags: review?(dietrich) → review+
Attachment #474705 - Flags: approval2.0+
(Assignee)

Comment 16

7 years ago
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).
Attachment #474705 - Attachment description: (Bv1) Improve nsIURLFormatter.idl documentation → (Bv1) Improve nsIURLFormatter.idl documentation [Checked in: See comment 16]
(Assignee)

Comment 17

7 years ago
(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.
Why not? It's a much simpler/safer change.
(Assignee)

Comment 19

7 years ago
(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.
(Reporter)

Updated

7 years ago
Attachment #474660 - Flags: review?(mnyromyr) → review+
(Assignee)

Comment 20

7 years ago
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
Attachment #474660 - Attachment description: (Av1) Call replace()+formatURL(), instead of formatURLPref()+replace() → (Av1) Call replace()+formatURL(), instead of formatURLPref()+replace() [Checked in: Comment 20]
(Assignee)

Updated

7 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
status-seamonkey2.1: ? → ---
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.1b1
(Assignee)

Comment 21

7 years ago
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.