348076 - remove generated URLs from region.properties

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Phil Schwan]

Once the work in bug 347944 completes, we need to remove URLs that can be generated from region.properties -- especially those that will have version numbers embedded in them:

startup.homepage_override_url
startup.homepage_welcome_url
searchEnginesURL
more_plugins_url

and from extensions.properties:

extensions.getMoreExtensionsURL
extensions.getMoreThemesURL

Did I miss any?

Comment 1

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

Just wanted to add in a request that's carried forward from bug 314119, which is to generate different URIs for builds that have --enable-branding set. I'm suggesting that non-branded builds point to the mozilla.org domain, and branded-builds point to mozilla.com.

Comment 2

[Axel Hecht [:Pike]]

Just guessing with my brains off, should we have a branding.js to set those prefs, built from either other-licenses/branding or regular firefox content just like browserconfig.properties?

CCing :bs for comment.

Comment 3

[Benjamin Smedberg]

Yes

Comment 4

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

OK, great. Where are those files getting created? This bug? Some other bug? I'd like the non-branded URIs for startup.homepage_override_url and startup.homepage_welcome_url to point to mozilla.org instead of mozilla.com. The extensions and searchEngines and plugins URIs can stay the same no matter what.

Comment 5

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

oops, I accidentally resolved this as fixed ...

Comment 6

[Dietrich Ayala (:dietrich)]

Ok, trying to piece together the action items for this bug, as the title is misleading.

1. Convert more URLs: It sounds like bug 347944 needs to update any URLs listed here, yes? So aside from the startup.homepage_* prefs, it's patch should also include conversions for:

region.properties:
searchEnginesURL
more_plugins_url

extensions.properties:
extensions.getMoreExtensionsURL
extensions.getMoreThemesURL

2. Create a new branding.js that has some of the prefs that we're updating, except pointing to .com for branded builds. Update the non-branded prefs to point to .org (Beltzner if this is correct please list all of the prefs you want this treatment for).

Is this correct?

Comment 7

[Dietrich Ayala (:dietrich)]

(In reply to comment #0)
> 
> more_plugins_url
> 
> and from extensions.properties:
> 
> extensions.getMoreExtensionsURL
> extensions.getMoreThemesURL
> 

These are all used from toolkit. Should I move the formatter service there so it can be used from both /browser and /toolkit?

Comment 8

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

(In reply to comment #7)
> (In reply to comment #0)
> > 
> > more_plugins_url
> > 
> > and from extensions.properties:
> > 
> > extensions.getMoreExtensionsURL
> > extensions.getMoreThemesURL
> > 
> 
> These are all used from toolkit. Should I move the formatter service there so
> it can be used from both /browser and /toolkit?

Sounds right to me, and when you do that, please note that I've updated the wiki to reflect that for links to AMO, they should use the %SERVICE% variable with the value set to "add-ons" (I updated the wiki page http://wiki.mozilla.org/Firefox2/L10n_Requirements#addons.mozilla.org_.28AMO.29 to match).

Comment 9

[Dietrich Ayala (:dietrich)]

Attachment: base branding.js

this is a firefox.js cut down to just URLs that look like they might need formatter treatment.

can you please take a pass, determining what goes/stays and what needs com vs org, etc?

Comment 10

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

========================================
The following need formatter treatment, and 
I've updated them to use the proper schema
========================================

pref("startup.homepage_override_url", "http://%LOCALE%.www.mozilla.com/%LOCALE%/firefox/%VERSION%/whatsnew/");

pref("startup.homepage_welcome_url", "http://%LOCALE%.www.mozilla.com/%LOCALE%/firefox/%VERSION%/firstrun/");

pref("extensions.blocklist.detailsURL", "http://%LOCALE%.www.mozilla.com/%LOCALE%/blocklist/");

pref("app.update.url.manual", "http://%LOCALE%.www.mozilla.com/%LOCALE%/products/firefox/%VERSION%/");

========================================
Did we get the following prefs in their source files as referenced in these chrome URIs? 
========================================

pref("app.update.url.details", "chrome://browser-region/locale/region.properties");

pref("extensions.update.url", "chrome://mozapps/locale/extensions/extensions.properties");

// Non-symmetric (not shared by extensions) extension-specific [update] preferences
pref("extensions.getMoreExtensionsURL", "chrome://mozapps/locale/extensions/extensions.properties");
pref("extensions.getMoreThemesURL", "chrome://mozapps/locale/extensions/extensions.properties");

pref("xpinstall.whitelist.add", "update.mozilla.org");
pref("xpinstall.whitelist.add.103", "addons.mozilla.org");

pref("keyword.URL", "chrome://browser-region/locale/region.properties");

// plugin finder service url
pref("pfs.datasource.url", "https://pfs.mozilla.org/plugins/PluginFinderService.php?mimetype=%PLUGIN_MIMETYPE%&appID=%APP_ID%&appVersion=%APP_VERSION%&clientOS=%CLIENT_OS%&chromeLocale=%CHROME_LOCALE%");

========================================
This one is being handled in bug 348842:
========================================

pref("browser.dictionaries.download.url", "https://addons.mozilla.org/%LOCALE%/firefox/%VERSION%/dictionaries/");

========================================
These ones were handled by bug 346242
========================================


// HTML report pages
pref("browser.safebrowsing.provider.0.reportGenericURL", "http://www.mozilla.org/projects/bonecho/anti-phishing/report_general/?hl={moz:locale}");
pref("browser.safebrowsing.provider.0.reportErrorURL", "http://www.mozilla.org/projects/bonecho/anti-phishing/report_error/?hl={moz:locale}");
pref("browser.safebrowsing.provider.0.reportPhishURL", "http://www.mozilla.org/projects/bonecho/anti-phishing/report_phish/?hl={moz:locale}");


========================================
The following prefs should additionally have non-branded values as shown:
========================================

pref("startup.homepage_override_url", "http://www.mozilla.org/projects/firefox/%VERSION%/whatsnew/");

pref("startup.homepage_welcome_url", "http://www.mozilla.com/projects/firefox/%VERSION%/firstrun/");

pref("app.update.url.manual", "http://www.mozilla.org/projects/firefox/");


========================================
The following prefs DO NOT need new formatter treatment
========================================

pref("extensions.blocklist.url", "https://add-ons.mozilla.org/blocklist/1/%APP_ID%/%APP_VERSION%/");

pref("app.update.url", "https://aus2.mozilla.org/update/2/%PRODUCT%/%VERSION%/%BUILD_ID%/%BUILD_TARGET%/%LOCALE%/%CHANNEL%/%OS_VERSION%/update.xml");

pref("browser.safebrowsing.provider.0.updateURL", "http://sb.google.com/safebrowsing/update?client=navclient-auto-ffox2&");

pref("browser.safebrowsing.provider.0.name", "Google");
pref("browser.safebrowsing.provider.0.lookupURL", "http://sb.google.com/safebrowsing/lookup?sourceid=firefox-antiphish&features=TrustRank&client=navclient-auto-ffox2&");
pref("browser.safebrowsing.provider.0.keyURL", "https://www.google.com/safebrowsing/getkey?");
pref("browser.safebrowsing.provider.0.reportURL", "http://sb.google.com/safebrowsing/report?");

Comment 11

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

(In reply to comment #10)
> pref("app.update.url.manual",
> "http://%LOCALE%.www.mozilla.com/%LOCALE%/products/firefox/%VERSION%/");

Actually, make this just:
pref("app.update.url.manual", "http://%LOCALE%.www.mozilla.com/%LOCALE%/products/firefox/");

Comment 12

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

(In reply to comment #10)
Gah, these are misplaced:
> pref("xpinstall.whitelist.add", "update.mozilla.org");
> pref("xpinstall.whitelist.add.103", "addons.mozilla.org");

They don't need formatting. We'll deal with them later.

Comment 13

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

(In reply to comment #10)
And finally, just to finish off the slew, I might as well tell you what these should look like at the end of the day:

> ========================================
> Did we get the following prefs in their source files as referenced in these
> chrome URIs? 
> ========================================
> 
> pref("app.update.url.details",
> "chrome://browser-region/locale/region.properties");

branded: http://%LOCALE%.www.mozilla.com/%LOCALE%/firefox/releases/
unbranded: http://www.mozilla.org/projects/firefox

> pref("extensions.update.url",
> "chrome://mozapps/locale/extensions/extensions.properties");

this one actually shouldn't be touched at all

> // Non-symmetric (not shared by extensions) extension-specific [update]
> preferences
> pref("extensions.getMoreExtensionsURL",
> "chrome://mozapps/locale/extensions/extensions.properties");

http://%LOCALE%.add-ons.mozilla.com/%LOCALE%/%APP%/%VERSION%/extensions/

> pref("extensions.getMoreThemesURL",
> "chrome://mozapps/locale/extensions/extensions.properties");

http://%LOCALE%.add-ons.mozilla.com/%LOCALE%/%APP%/%VERSION%/themes/

> pref("keyword.URL", "chrome://browser-region/locale/region.properties");

Leave this for now, localizers will get it through region.properties.

> // plugin finder service url
> pref("pfs.datasource.url",
> "https://pfs.mozilla.org/plugins/PluginFinderService.php?mimetype=%PLUGIN_MIMETYPE%&appID=%APP_ID%&appVersion=%APP_VERSION%&clientOS=%CLIENT_OS%&chromeLocale=%CHROME_LOCALE%");

This one also doesn't need localization.

Comment 14

[Phil Schwan]

(In reply to comment #6)
> Ok, trying to piece together the action items for this bug, as the title is
> misleading.

Please choose a better title -- I won't be offended! :)

Comment 15

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

mwu: can you do a patch up for the URIs as shown in my comments below? I think Dietrich has collapsed ... :)

Comment 16

[Dietrich Ayala (:dietrich)]

Attachment: wip patch

This consolidated beltzners comments into firefox.js and brand.js. Still need to figure out how to only include brand.js when #ifdef OFFICIAL_BRANDING, such that it overwrites the values in firefox.js.

We can connoiter on IRC about splitting up doing the replacements in the code.

Comment 17

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

(In reply to comment #10)
> pref("startup.homepage_welcome_url",
> "http://www.mozilla.com/projects/firefox/%VERSION%/firstrun/");
                      ^^^

Ooops. That should be .org. Sorry. :(

Comment 18

[Michael Wu [:mwu]]

Attachment: WIP: *.region/firefox.js changes

Comment 19

[Dietrich Ayala (:dietrich)]

Attachment: combines mwu's patch w/ the replacements

First draft. This compiles, but I haven't tested it w/ official branding, nor tested many of the URLs. I'm going to start to track down testing scenarios for each.

Beltzner, can you do first review to see if the URLs are look correct?

Comment 20

[Dietrich Ayala (:dietrich)]

Comment on attachment 234301 [details] [diff] [review]
combines mwu's patch w/ the replacements

nm, missing the extensions.getMore* replacements.

Comment 21

[Dietrich Ayala (:dietrich)]

Attachment: prefs + replacements

Can you please review URLs for correctness?

Comment 22

[Phil Schwan]

Comment on attachment 234334 [details] [diff] [review]
prefs + replacements

Those URLs look correct to me, with the possible exception of the blocklist (the old one has app + version; was it intentional to remove them?)

Otherwise it looks all according to plan.

Comment 23

[Dietrich Ayala (:dietrich)]

(In reply to comment #22)
> (From update of attachment 234334 [details] [diff] [review] [edit])
> Those URLs look correct to me, with the possible exception of the blocklist
> (the old one has app + version; was it intentional to remove them?)
> 
> Otherwise it looks all according to plan.
> 

Thanks Phil. Which blocklist URL are you referring to? We changed detailsURL, which didn't have app or version:

-pref("extensions.blocklist.detailsURL", "http://www.mozilla.com/blocklist/");
+pref("extensions.blocklist.detailsURL", "http://%LOCALE%.www.mozilla.com/%LOCALE%/blocklist/");

Comment 24

[Dietrich Ayala (:dietrich)]

Comment on attachment 234334 [details] [diff] [review]
prefs + replacements

Can you review the code changes please?

Comment 25

[Phil Schwan]

(In reply to comment #23)
> 
> Thanks Phil. Which blocklist URL are you referring to? We changed detailsURL,
> which didn't have app or version:

doh.  That's what you get for asking me.  Never mind, carry on!

Comment 26

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

Hm. Axel noticed that the Web Forgery alert bubble uses a URL that will need to be localized:

http://lxr.mozilla.org/seamonkey/source/browser/locales/en-US/chrome/browser/safebrowsing/phishing-afterload-warning-message.dtd#9

I think the best approach here would be to:

 - update warning-overlay.xul to draw a formatURL pref from firefox.js (http://lxr.mozilla.org/seamonkey/source/browser/components/safebrowsing/content/warning-overlay.xul#78)

 - add that pref to firefox.js as browser.safebrowsing.warning.infoURL  = http://%LOCALE%.www.mozilla.com/%LOCALE%/firefox/phishing-protection

Sorry for the late notice :(

Comment 27

[Dietrich Ayala (:dietrich)]

Attachment: fix searchEngineURL

forgot this one in the original patch

Comment 28

[Dietrich Ayala (:dietrich)]

Comment on attachment 234548 [details] [diff] [review]
fix searchEngineURL

hold that thought, need to remove the old location still.

Comment 29

[Dietrich Ayala (:dietrich)]

Attachment: fix searchEnginesURL + remove old value

Comment 30

[Mike Connor [:mconnor]]

Comment on attachment 234550 [details] [diff] [review]
fix searchEnginesURL + remove old value

r+a=me on this straightforward followup, thanks!

Comment 31

[Dietrich Ayala (:dietrich)]

Attachment: WIP patch for safebrowsing change

Comment 32

[Dietrich Ayala (:dietrich)]

Attachment: safebrowsing FAQ URL change

Comment 33

[Mike Connor [:mconnor]]

Comment on attachment 234569 [details] [diff] [review]
safebrowsing FAQ URL change

ok, that looks good, please remove the obsolete .dtd string as well, and get this in ASAP

Comment 34

[Robert Kaiser]

Thanks for forcing every other toolkit/XULRunner app to use URLs that might not exist for them or have no resources available for them, and allowing them arbitrary L10n of their URLs (i.e. not following your MoCo-internal standards).

Comment 35

[Mark Banner (:standard8)]

The "prefs + replacements" patch on this bug will have broken Sunbird, Thunderbird and SeaMonkey (the toolkit version in work on trunk). When displaying Add-ons Manager in SeaMonkey's toolkit version I get:

[Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIPrefBranch.getComplexValue]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: file:///mozilla/master/mozilla/sm2/dist/bin/components/nsURLFormatter.js :: uf_formatURLPref :: line 129"  data: no]

and it then wants to try and open the link: "chrome://mozapps/locale/extensions/extensions.properties"

The main problem being that other applications can no longer specify an locale file for specific localizations (see http://lxr.mozilla.org/seamonkey/search?string=extensions.getMore), but have to use a predefined format for this. So one problem is that ALL the prefs weren't fixed-up, and another being that SeaMonkey wants to still be able to use locale specific versions.

Comment 36

[Mark Banner (:standard8)]

(In reply to comment #35)
> The "prefs + replacements" patch on this bug will have broken Sunbird,
> Thunderbird and SeaMonkey (the toolkit version in work on trunk). When
> displaying Add-ons Manager in SeaMonkey's toolkit version I get:

Sorry Neil pointed out that url formatter can handle chrome urls. So the actually problem here you've removed:

extensions.getMoreExtensionsURL=https://addons.mozilla.org/extensions/?application=%APPID%
extensions.getMoreThemesURL=https://addons.mozilla.org/themes/?application=%APPID%

from extensions.properties without giving everyone else the chance to update their prefs at the same time (but note that %APPID% is no longer supported, so it would have to be %ID%).

(In reply to comment #34)
> Thanks for forcing every other toolkit/XULRunner app to use URLs that might not
> exist for them or have no resources available for them, and allowing them
> arbitrary L10n of their URLs (i.e. not following your MoCo-internal standards).
> 

I think I may have misled KaiRo when discussing this one on IRC. Sorry.

Comment 37

[u60234]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1b2) Gecko/2006081903 BonEcho/2.0b2

A couple of things:

The "how Firefox protects you" link in the Web Forgery alert is now broken. Where is the pref for browser.safebrowsing.warning.infoURL?

The searchEnginesURL string was not removed from region.properties.

Why isn't the formatter service used for releaseNotesURL in region.properties?

Comment 38

[Dietrich Ayala (:dietrich)]

(In reply to comment #36)
> (In reply to comment #35)
> > The "prefs + replacements" patch on this bug will have broken Sunbird,
> > Thunderbird and SeaMonkey (the toolkit version in work on trunk). When
> > displaying Add-ons Manager in SeaMonkey's toolkit version I get:
> 
> Sorry Neil pointed out that url formatter can handle chrome urls. So the
> actually problem here you've removed:
> 
> extensions.getMoreExtensionsURL=https://addons.mozilla.org/extensions/?application=%APPID%
> extensions.getMoreThemesURL=https://addons.mozilla.org/themes/?application=%APPID%
> 
> from extensions.properties without giving everyone else the chance to update
> their prefs at the same time (but note that %APPID% is no longer supported, so
> it would have to be %ID%).
> 
> (In reply to comment #34)
> > Thanks for forcing every other toolkit/XULRunner app to use URLs that might not
> > exist for them or have no resources available for them, and allowing them
> > arbitrary L10n of their URLs (i.e. not following your MoCo-internal standards).
> > 
> 
> I think I may have misled KaiRo when discussing this one on IRC. Sorry.
> 

We obviously took non-Firefox apps into consideration when making these changes. The mistake was the removal of the extensions.getMore* prefs from extensions.properties, which was entirely my fault. I apologize and take full responsibility for that. This exact problem with removing those prefs had been discussed on IRC, and it was I who, in the throes of beta2 passion, forgot to remove that part of the patch. I'll fix this ASAP.

What makes this mistake especially egregious is that the patch changed the extensions.getMore* prefs in firefox.js from "chrome://mozapps/locale/extensions/extensions.properties" to our formatted URLs should be sufficient, making the change the extensions.properties entirely unnecessary.

Comment 39

[Mark Banner (:standard8)]

(In reply to comment #38)
> We obviously took non-Firefox apps into consideration when making these
> changes. The mistake was the removal of the extensions.getMore* prefs from
> extensions.properties, which was entirely my fault. I apologize and take full
> responsibility for that. This exact problem with removing those prefs had been
> discussed on IRC, and it was I who, in the throes of beta2 passion, forgot to
> remove that part of the patch. I'll fix this ASAP.

Thanks for the explanation and fix. Though I think there's still a problem, as extensions.js used to replace %APPID% whereas the url formatter uses %ID%. I'm not sure which, if any of the other extension.properties values this will affect, or if the url formatter change to %ID% was correct.

Comment 40

[Robert Kaiser]

(In reply to comment #38)
> What makes this mistake especially egregious is that the patch changed the
> extensions.getMore* prefs in firefox.js from
> "chrome://mozapps/locale/extensions/extensions.properties" to our formatted
> URLs should be sufficient, making the change the extensions.properties entirely
> unnecessary.

OK, your comment explains that :)
Anyways, if you do so, you probably should file a followup to remove those lines once they're unused - it's good to get rid of unused stuff, and whoever needs the URLs set should probably do so via their prefs. In that light, it might be a good idea to have a default to which EM falls back if the prefs are not set (or have the prefs set to some default somewhere within toolkit).

Comment 41

[Dietrich Ayala (:dietrich)]

(In reply to comment #37)
> Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1b2) Gecko/2006081903
> BonEcho/2.0b2
> 
> A couple of things:
> 
> The "how Firefox protects you" link in the Web Forgery alert is now broken.
> Where is the pref for browser.safebrowsing.warning.infoURL?

It's defined in firefox.js. However, on trunk the pref was missing from the patch, and that has been fixed. I think the problem you're experiencing is either that they gave us the wrong URL, or they haven't made the page yet. This:

http://en-US.www.mozilla.com/en-US/firefox/phishing-protection

redirects to:

http://www.mozilla.com/firefox/phishing-protection

Which doesn't exist. Anyone know who owns this page, or is supposed to create it? Beltzner? Mommy?

> The searchEnginesURL string was not removed from region.properties.

Thanks, that was branch only, and has been fixed there.

> 
> Why isn't the formatter service used for releaseNotesURL in region.properties?
> 

I think they decided on a server-side change for this, in bug 336338. There's some discussion about it on bug 347944.

Comment 42

[u60234]

(In reply to comment #41)
> (In reply to comment #37)
> > The "how Firefox protects you" link in the Web Forgery alert is now broken.
> > Where is the pref for browser.safebrowsing.warning.infoURL?
> 
> It's defined in firefox.js.

Really? I can't find it in rev. 1.71.2.66, which AFAIK is the current branch version.
 
> > Why isn't the formatter service used for releaseNotesURL in region.properties?
> > 
> 
> I think they decided on a server-side change for this, in bug 336338. There's
> some discussion about it on bug 347944.

After all the nice work on the other URLs, it just seem odd that the URL for Help -> Release Notes should be http://www.mozilla.org/projects/bonecho/releases/2.0.html for all locales, and then have the language preferences sorted out on the server.

Comment 43

[Reed Loden [:reed]]

releaseNotesURL needs changing, too.

For branded builds, please use:
http://%LOCALE%.www.mozilla.com/%LOCALE%/%APP%/%VERSION%/releasenotes/

For non-branded builds, please use:
http://www.mozilla.org/projects/%APP%/%VERSION%/releasenotes/

Comment 44

[Dietrich Ayala (:dietrich)]

Attachment: trunk patch for release notes changes + cleanup

More cleanup for URL changes. This patch adds:

- release notes URL changes
- replaces hard-coded "firefox" with variable replacement fed by nsIXULAppInfo
- removes searchEngineURL from region.properties

Comment 45

[Dietrich Ayala (:dietrich)]

Attachment: trunk patch for release notes changes + cleanup

sorry, upped wrong patch. here's the trunk patch.

Comment 46

[Dietrich Ayala (:dietrich)]

Attachment: branch patch for release notes changes + cleanup

same as previous trunk patch, except it adds the safebrowsing pref, which was missing from last night's SB patch.

Comment 47

[Steffen Wilberg]

about.xhtml links to the release notes as well:
http://lxr.mozilla.org/mozilla1.8/source/toolkit/content/about.xhtml#72
http://lxr.mozilla.org/mozilla1.8/source/other-licenses/branding/firefox/locales/en-US/brand.dtd#6
http://lxr.mozilla.org/mozilla1.8/source/browser/locales/en-US/chrome/branding/brand.dtd#6

Comment 48

[Robert Kaiser]

Steffen:
See bug 346186 comment #6 for the about: page.

Comment 49

[Dietrich Ayala (:dietrich)]

Attachment: trunk patch for release notes changes + cleanup + about page

I've added code to the About page to use the URL formatting code instead of the entity for the release notes URL.

I lxr'd the entity (&releaseBaseURL;), and Minimo is the only other project currently using it. If this gets +r I'll contact Doug about updating. (someone please correct me if i'm wrong, as the discussion in bug 346186 seems to indicate that other apps are using that same About page, but I didn't see any other references to that entity...)

Another change in this patch is that I renamed the browser.releaseNotesURL pref to app.releaseNotesURL.

Comment 50

[Robert Kaiser]

(In reply to comment #49)
> the discussion in bug 346186 seems to
> indicate that other apps are using that same About page

The toolkit-based SeaMonkey configuration internally codenamed "suiterunner" is using this about page, but it's not working correctly there yet - it complains about the missing entity that we didn't add yet as the scheme didn't fit our release notes URLs. We'll be able to correct this to a localized pref which even can include the %VERSION% variable with your patch, if I understood this correctly - and with that, we're completely happy :)

Comment 51

[Reed Loden [:reed]]

While some parts of this bug are fixed, there are still patches that need to be reviewed. Reopening.

Current problems with branch:
* releaseNotesURL is out of date (does not use new format) [causes problems elsewhere with about:, etc., too]
* "how &brandShortName; protects you" in Phishing Protection is broken (browser.safebrowsing.warning.infoURL is not set on branch)
* needs cleaning up

dietrich's latest patch seems to fix pretty much everything, but it needs to be reviewed. :)

Comment 52

[Mark Banner (:standard8)]

(In reply to comment #51)
> While some parts of this bug are fixed, there are still patches that need to be
> reviewed. Reopening.
> dietrich's latest patch seems to fix pretty much everything, but it needs to be
> reviewed. :)

I think this may have got lost in the comments as well:

(In reply to comment #39)
> Thanks for the explanation and fix. Though I think there's still a problem, as
> extensions.js used to replace %APPID% whereas the url formatter uses %ID%. I'm
> not sure which, if any of the other extension.properties values this will
> affect, or if the url formatter change to %ID% was correct.

I think these definitely need changing from %APPID% -> %ID%:

extensions.getMoreExtensionsURL
extensions.getMoreThemesURL

I think extensions.update.url is unaffected.

Comment 53

[Reed Loden [:reed]]

(In reply to comment #52)
> (In reply to comment #39)
> > Thanks for the explanation and fix. Though I think there's still a problem, as
> > extensions.js used to replace %APPID% whereas the url formatter uses %ID%. I'm
> > not sure which, if any of the other extension.properties values this will
> > affect, or if the url formatter change to %ID% was correct.
> 
> I think these definitely need changing from %APPID% -> %ID%:
> 
> extensions.getMoreExtensionsURL
> extensions.getMoreThemesURL

Firefox doesn't use the extensions.getMore*URL settings in /toolkit/locales/en-US/chrome/mozapps/extensions/extensions.properties anymore. Instead, it sets them in /browser/app/profile/firefox.js, which currently uses the correct values ("http://%LOCALE%.add-ons.mozilla.com/%LOCALE%/%APP%/%VERSION%/extensions/" and "http://%LOCALE%.add-ons.mozilla.com/%LOCALE%/%APP%/%VERSION%/themes/"). Therefore, your issue is no longer a problem in Firefox. :)

Comment 54

[Mark Banner (:standard8)]

(In reply to comment #53)
> Therefore, your issue is no longer a problem in Firefox. :)

Yes but it is an issue for Thunderbird/Sunbird/SeaMonkey/XULRunner(?) that was caused by the fix for this bug - a new bug can be filed if it really needs it, the result is the same either way, I just thought it'd be easier to fix in the bug that originally caused it especially as there are other things to fix anyway.

Comment 55

[Steffen Wilberg]

So Firefox has this in firefox.js:
pref("extensions.getMoreExtensionsURL", "http://%LOCALE%.add-ons.mozilla.com/%LOCALE%/%APP%/%VERSION%/extensions/");

Thunderbird and the other apps have this, e.g. in all-thunderbird.js:
pref("extensions.getMoreExtensionsURL", "chrome://mozapps/locale/extensions/extensions.properties");

extensions.properties then says:
extensions.getMoreExtensionsURL=https://addons.mozilla.org/extensions/?application=%APPID%
http://lxr.mozilla.org/seamonkey/search?string=getMoreExtensionsURL

Looks like the other apps should use the exact same pref as Firefox already does (and the line from extensions.properties removed), since the code in extensions.js (as well as the url formatter) is shared between all Toolkit apps: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/mozapps/extensions/content/extensions.js&rev=1.72.2.35&mark=323,334-343#321

Comment 56

[Robert Kaiser]

(In reply to comment #55)
> Looks like the other apps should use the exact same pref as Firefox already
> does

Not exactly, as for non-MoCo applications, the .com is wrong, it has to be some .org URL by default for those.

Comment 57

[Mike Connor [:mconnor]]

Comment on attachment 234632 [details] [diff] [review]
trunk patch for release notes changes + cleanup + about page

Doh, someone reopened but didn't knock the fixed1.8.1 keyword off here.

We should get this on trunk first thing, but since it only removes localized strings and doesn't add any, this should be safe for RC if we don't do any b2 repins

Comment 58

[Steffen Wilberg]

On a side note, the server redirects aren't in place yet:
http://en-US.add-ons.mozilla.com/en-US/firefox/2.0b2/extensions/ is still 404.

Comment 59

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

Moving to beta2 as per comment 57

Comment 60

[Dietrich Ayala (:dietrich)]

Attachment: fix for non-Fx apps

This is the fix for non-Fx apps, that Mark Banner is referring to in comment #52 and comment #54.

Drivers: WRT to Firefox this has zero risk, as it doesn't use the prefs in extensions.properties anymore.

Comment 61

[Dietrich Ayala (:dietrich)]

Attachment: branch patch for release notes changes + cleanup + about page

Trunk patch is checked in. Here's the branch patch.

The differences between the 2 patches:

- trunk patch removed defunct searchEnginesURL (already removed from branch)
- branch patch adds the browser.safebrowsing.warning.infoURL pref (accidentally left out from earlier sb patch)

Comment 62

[Steffen Wilberg]

*** Bug 337823 has been marked as a duplicate of this bug. ***

Comment 63

[Dietrich Ayala (:dietrich)]

Attachment: branch patch for release notes changes + cleanup + about page

Adds a trailing slash to the SB info URL.

Comment 64

[Scott MacGregor]

Hmm, this toolkit change seems to have broken some stuff in Thunderbird.

Also, this jumped out at me in extensions.js:

+      var formatter = Components.classes["@mozilla.org/browser/URLFormatterService;1"]
+                                .getService(Components.interfaces.nsIURLFormatter);

Why is toolkit using a browser service? That seems bad. The formatter object looks like it lives in toolkit. Does the class ID need fixed?

Comment 65

[Mike Schroepfer]

Comment on attachment 234851 [details] [diff] [review]
branch patch for release notes changes + cleanup + about page

a=schrep for drivers - approving all [181approval pending] bugs now that tree is open.

Comment 66

[Scott MacGregor]

Another question in addition to using /browser/ in the class id for a toolkit component...nsIURLFormatter adds a dependency on something called nsIDictionary which is defined in extensions\xml-rpc. 

Did we intend to make toolkit and this interface depend on xml-rpc? I'm not sure what the two have in common together. cc'ing benjamin in case he can provide some toolkit guidance on that question. Maybe nsIDictionary should be moved out of xml-rpc if we are using it for something other than xml-rpc support. 

Comment 67

[Sylvain Pasche]

One problem I see on trunk is that the "about:" page has apparently no chrome privileges, so it throws an exception:

Error: uncaught exception: Permission denied to get property UnnamedClass.classes

Comment 68

[Dietrich Ayala (:dietrich)]

(In reply to comment #66)
> Another question in addition to using /browser/ in the class id for a toolkit
> component...nsIURLFormatter adds a dependency on something called nsIDictionary
> which is defined in extensions\xml-rpc. 
> 
> Did we intend to make toolkit and this interface depend on xml-rpc? I'm not
> sure what the two have in common together. cc'ing benjamin in case he can
> provide some toolkit guidance on that question. Maybe nsIDictionary should be
> moved out of xml-rpc if we are using it for something other than xml-rpc
> support. 
> 

Hrm, I didn't realize that nsIDictionary was specific to the XML-RPC extension. I'll change the interface such that other apps don't have to build XML-RPC for this, as well as changing the class ID.

(In reply to comment #67)
> One problem I see on trunk is that the "about:" page has apparently no chrome
> privileges, so it throws an exception:
> 
> Error: uncaught exception: Permission denied to get property
> UnnamedClass.classes
> 

Which build/platform/etc are you seeing this? It seems to work a-ok for me.

Comment 69

[Sylvain Pasche]

(In reply to comment #68)
> Which build/platform/etc are you seeing this? It seems to work a-ok for me.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/2006082304 Minefield/3.0a1

When accessing chrome://global/content/about.xhtml the "Build identifier" line is displayed correctly. But when using the "about:" URL, the exception is thrown and the Build identifier line is not shown.

Comment 70

[Reed Loden [:reed]]

(In reply to comment #69)
> (In reply to comment #68)
> > Which build/platform/etc are you seeing this? It seems to work a-ok for me.
> 
> Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/2006082304
> Minefield/3.0a1
> 
> When accessing chrome://global/content/about.xhtml the "Build identifier" line
> is displayed correctly. But when using the "about:" URL, the exception is
> thrown and the Build identifier line is not shown.

That's bug 348069. See bug 345993, comment #21.

Comment 71

[Sylvain Pasche]

(In reply to comment #70)
> That's bug 348069. See bug 345993, comment #21.

I would say it's not. bug 348069 is triggered with non official builds when the build identifier is 0000000. The issue there is with an official release.

Comment 72

[Mike Connor [:mconnor]]

Comment on attachment 234816 [details] [diff] [review]
fix for non-Fx apps

r=me, this should fix apps that haven't converted to the new system, since nsURLFormatter handles localized prefs just fine.  This URL scheme should work for a long time yet, for backwards compat reasons (1.5, 1.0, etc)

Comment 73

[Mark Banner (:standard8)]

(In reply to comment #67)
> Error: uncaught exception: Permission denied to get property
> UnnamedClass.classes

(In reply to comment #69)
> > Which build/platform/etc are you seeing this? It seems to work a-ok for me.

> When accessing chrome://global/content/about.xhtml the "Build identifier" line
> is displayed correctly. But when using the "about:" URL, the exception is
> thrown and the Build identifier line is not shown.

I've seen this error using about: on the development SeaMonkey builds which use toolkit (codenamed suiterunner). I haven't tried the chrome link yet.

The problem doesn't just relate to the build identifier, it relates to the release notes URL as well - that is displayed with just the text and no link on suiterunner builds.

I did find that it is failing to get ["@mozilla.org/browser/URLFormatterService;1] as the about: seemingly doesn't have chrome permissions (I think).

I'd assumed that Firefox's about information was always displayed in a dialog which is why I hadn't mentioned it before, as I guessed it was something the SeaMonkey builds would have to sort and also not got round to discussing it with anywone.

Comment 74

[Mark Banner (:standard8)]

Comment on attachment 234632 [details] [diff] [review]
trunk patch for release notes changes + cleanup + about page

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 75

[Mike Connor [:mconnor]]

let's spin off the "trunk about: doesn't have chrome privs" issue into a separate bug.  that's not a branch blocker.  I don't think the change was intentionally, but bz did a bunch of cleanup of the hardcoded list on trunk, and I'm betting about: changed by accident.  If not, I'd like to see why not!

Comment 76

[Sylvain Pasche]

Apparently, the branch also has no chrome privs for about:

Comment 77

[Axel Hecht [:Pike]]

It seems that about: doesn't have any special treatment in terms of caps and js as the other about:foo with explicit modules have. Filed bug 349985 to clarify/clean that up.

Comment 78

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

(In reply to comment #72)
> (From update of attachment 234816 [details] [diff] [review] [edit])
> r=me, this should fix apps that haven't converted to the new system, since
> nsURLFormatter handles localized prefs just fine.  This URL scheme should work
> for a long time yet, for backwards compat reasons (1.5, 1.0, etc)

Hm. AMO is going to be following the new l10n-savvy URI scheme, which means that we should probably make these prefs:

+extensions.getMoreExtensionsURL=https://addons.mozilla.org/extensions/?application=%ID%
+extensions.getMoreThemesURL=https://addons.mozilla.org/themes/?application=%ID%

be more like:

+extensions.getMoreExtensionsURL=https://en-US.add-ons.mozilla.com/en-US/%ID%/extensions
+extensions.getMoreThemesURL=https://en-US.add-ons.mozilla.com/en-US/%ID%/themes

where "en-US" can either be localized by localizers, or if there's a variable that works in there using the existing URL formatting functions, we could use that.

Dietrich, let me know if you just want me to file this as a separate bug, or just fix that up as you check in?

Comment 79

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

Comment on attachment 234816 [details] [diff] [review]
fix for non-Fx apps

a=drivers for the MOZILLA_1_8_BRANCH, but please let me know what you want to do about changing the URIs to match the current l10n scheme (see previous comment)

Comment 80

[Dietrich Ayala (:dietrich)]

(In reply to comment #79)
> (From update of attachment 234816 [details] [diff] [review] [edit])
> a=drivers for the MOZILLA_1_8_BRANCH, but please let me know what you want to
> do about changing the URIs to match the current l10n scheme (see previous
> comment)
> 

In order to close this bug up (and keep it from becoming more convoluted), that issue has been spun off into bug 350123, along with some of the other url formatter followup issues mentioned in the recent comments.

(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?
> 

I'll address this in bug 346186, and have commented there noting so.

Comment 81

[Dietrich Ayala (:dietrich)]

Marking fixed. All patches have now been checked in to both trunk and branch. Fallout issues from this bug have been spun off into new or different bugs, as previously noted in comment #80.

Comment 82

[Axel Hecht [:Pike]]

Comment on attachment 234851 [details] [diff] [review]
branch patch for release notes changes + cleanup + about page

Oops, didn't watch this closely enough. We should have delayed landing this change to after B2.
Now localizers have to make up their mind to either fix B2 or 1.8 branch tip.

Localizers, as announced on .l10n, please choose b2, that are the Fx-Release l10n tinderboxens. Sorry.

Comment 83

[bugs1m.wisefool]

Hey, someone mentioned in https://bugzilla.mozilla.org/show_bug.cgi?id=341899#c62 that customization.xhtml <lxr.mozilla.org/mozilla1.8/source/browser/locales/en-US/chrome/help/customization.xhtml>  still has a generic "addons.m.o" instead of "en-US.addons.m.o" URL. Is this a bad thing, and if so is it way too late to change it?

Comment 84

[Axel Hecht [:Pike]]

Either way, it's way too late. I commented in the other bug, too.