346186 - Add more flexibility for release note URL in toolkit's about: page

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Robert Kaiser]

When I looked at the patches for bug 328563, I realized once again that toolkit's about: page currently forces all apps using it into naming their release notes <version>.html on the web server.

The place where this is enforced is http://lxr.mozilla.org/mozilla/source/toolkit/content/about.xhtml#71 - it links to "&releaseBaseURL;__MOZ_APP_VERSION__.html" for the relnotes.

This is bad, as some apps probly want to follow other conventions on their web pages, e.g. SeaMonkey, starting to use toolkit on trunk, has relnotes at http://www.mozilla.org/projects/seamonkey/releases/seamonkey<version>/ - and that doesn't fit with toolkit's about page.

I think probably the best solution here would be to link to "&releaseURL;" in about.xhtml and preprocess brand.dtd instead, adding the version number there, as the branding files are app-specific and can use their specific URL convention.


That said, we should really have some place to file such bugs in the Toolkit product instead of Firefox.

Comment 1

[Steffen Wilberg]

AFAIK there's a convention to not preprocess locale files though. Maybe you can use a pref instead. But please don't hardcode the version again.

Comment 2

[Robert Kaiser]

True, we have this convention, and it's quite good, I'm a big fan of that convention.
But I think that we can break it in very controlled areas, which I'd suppose includes those branding files.

Comment 3

[Benjamin Smedberg]

Any app-specific changes should be made in chrome://branding/content/brand.dtd, not chrome://branding/locale/brand.dtd

Comment 4

[Robert Kaiser]

Benjamin, that would need a toolkit-based app cpouldn't allow for localized relnotes (or at least not if they need a different URL)?

Comment 5

[Robert Kaiser]

OK, so after some discussion with Pike on IRC, I understand we could/should do it the following way:

1) include both locale/brand.dtd and content/brand.dtd from about.xhtml, in this order.
2) content could decide to either pull parts or complete urls from locale, or to "hardcode" the entity value for that URL

I may look into doing this, when I find the time to do so. My target is to have this working in some fashion before SeaMonkey flips to toolkit on trunk.

Comment 6

[Robert Kaiser]

I know this is XHTML and not JS itself, but can we utilize the new url formatter service for this?
As I'm told that the URL formatter service also allows pulling its URL through a localized pref, I think we can resolve all issues that way (and use one source for the relnotes URL everywhere probably).

Comment 7

[Mike Beltzner [:beltzner, not reading bugmail]]

Dietrich: will this be fixed in the last of your series of patches to bug 348076? Is this dialog changed so that it uses the new pref that you created there?

Comment 8

[Dietrich Ayala (:dietrich)]

(In reply to comment #7)
> Dietrich: will this be fixed in the last of your series of patches to bug
> 348076? Is this dialog changed so that it uses the new pref that you created
> there?
> 

Yes.

Comment 9

[Dietrich Ayala (:dietrich)]

The fix for this should address Mark's comments from bug 348076:

(In reply to comment #74)
> (From update of attachment 234632 [details] [diff] [review] [edit])
> I've also just remembered a couple of comments on the about.xhtml page:
> 
> +      var releaseNotesURL = formatURL("app.releaseNotesURL", null, true);
> ...
> +      function formatURL(aFormat, aVars, aIsPref) {
> +        var repl = null;
> +        if (aVars) {
> +          repl =
> Cc["@mozilla.org/dictionary;1"].createInstance(Ci.nsIDictionary);
> +          for (var key in aVars) {
> +            var val =
> Cc['@mozilla.org/supports-string;1'].createInstance(Ci.nsISupportsString);
> +            val.data = aVars[key];
> +            repl.setValue(key, val);
> +          }
> +        }
> 
> Cc and Ci aren't defined for non-firefox applications - I think they are
> defined in browser.js which is specific to ff.
> 
> Also, as this function is always called with aVars null, and aIsPref true, do
> you really need to use the formatURL function in this instance?
> 

Comment 10

[Dietrich Ayala (:dietrich)]

Attachment: cleanup as noted in comment #9

Fixes and simplification as described in comment #9.

Comment 11

[Mike Connor [:mconnor]]

Comment on attachment 235342 [details] [diff] [review]
cleanup as noted in comment #9

r=me

Comment 12

[Dietrich Ayala (:dietrich)]

Between the changes in bug 348076 and the attached patch, we've met the desired level of flexibility.

Comment 13

[Reed Loden [:reed]]

Comment on attachment 235342 [details] [diff] [review]
cleanup as noted in comment #9

Drivers: This patch fixes the "release notes" link in about: (currently broken) and allows non-Fx apps to benefit from the changes (some current code is Fx-only).

Comment 14

[Reed Loden [:reed]]

Will this fix "Error: uncaught exception: Permission denied to get property UnnamedClass.classes"?

Comment 15

[Mark Banner (:standard8)]

(In reply to comment #14)
> Will this fix "Error: uncaught exception: Permission denied to get property
> UnnamedClass.classes"?

I believe a fix is required for bug 349985 to fix that error.

Comment 16

[Mike Schroepfer]

Comment on attachment 235342 [details] [diff] [review]
cleanup as noted in comment #9

a=schrep for drivers

Comment 17

[Dietrich Ayala (:dietrich)]

Checking in about.xhtml;
/cvsroot/mozilla/toolkit/content/about.xhtml,v  <--  about.xhtml
new revision: 1.8.8.10; previous revision: 1.8.8.9
done