Closed Bug 348076 Opened 14 years ago Closed 14 years ago

remove generated URLs from region.properties

Categories

(Firefox :: General, defect)

2.0 Branch
x86
All
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 2

People

(Reporter: phil, Assigned: dietrich)

References

Details

(Keywords: fixed1.8.1, late-l10n)

Attachments

(5 files, 11 obsolete files)

18.37 KB, patch
phil
: review+
mconnor
: superreview+
mconnor
: approval1.8.1+
Details | Diff | Splinter Review
3.58 KB, patch
mconnor
: review+
mconnor
: approval1.8.1+
Details | Diff | Splinter Review
3.49 KB, patch
mconnor
: review+
mconnor
: approval1.8.1+
Details | Diff | Splinter Review
1.99 KB, patch
mconnor
: review+
beltzner
: approval1.8.1+
Details | Diff | Splinter Review
9.96 KB, patch
mtschrep
: approval1.8.1+
Details | Diff | Splinter Review
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?
Flags: blocking-firefox2?
Flags: blocking-firefox2? → blocking-firefox2+
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.
Target Milestone: --- → Firefox 2 beta2
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.
Assignee: nobody → dietrich
Yes
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.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
oops, I accidentally resolved this as fixed ...
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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?
(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?
Status: REOPENED → NEW
(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).
Attached file base branding.js (obsolete) —
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?
Attachment #234156 - Flags: review?(beltzner)
Attachment #234156 - Attachment mime type: application/x-javascript → text/plain
========================================
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?");
(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/");
(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.
(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.
(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! :)
Attachment #234156 - Flags: review?(beltzner) → review-
mwu: can you do a patch up for the URIs as shown in my comments below? I think Dietrich has collapsed ... :)
Assignee: dietrich → michael.wu
Attached patch wip patch (obsolete) — Splinter Review
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.
(In reply to comment #10)
> pref("startup.homepage_welcome_url",
> "http://www.mozilla.com/projects/firefox/%VERSION%/firstrun/");
                      ^^^

Ooops. That should be .org. Sorry. :(
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?
Assignee: michael.wu → dietrich
Attachment #234256 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #234301 - Flags: review?(beltzner)
Comment on attachment 234301 [details] [diff] [review]
combines mwu's patch w/ the replacements

nm, missing the extensions.getMore* replacements.
Attachment #234301 - Flags: review?(beltzner)
Can you please review URLs for correctness?
Attachment #234156 - Attachment is obsolete: true
Attachment #234245 - Attachment is obsolete: true
Attachment #234301 - Attachment is obsolete: true
Attachment #234334 - Flags: review?(beltzner)
Attachment #234334 - Flags: review?(beltzner) → review?(phil)
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.
Attachment #234334 - Flags: review?(phil) → review+
(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 on attachment 234334 [details] [diff] [review]
prefs + replacements

Can you review the code changes please?
Attachment #234334 - Flags: superreview?(mconnor)
(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!
Whiteboard: [needs review mconnor]
Attachment #234334 - Flags: superreview?(mconnor) → superreview+
Whiteboard: [needs review mconnor] → [needs checkin]
Whiteboard: [needs checkin] → [checkin needed]
Keywords: late-l10n
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 :(
Whiteboard: [checkin needed] → [approval needed]
Attachment #234334 - Flags: approval1.8.1+
Attached patch fix searchEngineURL (obsolete) — Splinter Review
forgot this one in the original patch
Attachment #234548 - Flags: review?(mconnor)
Attachment #234548 - Flags: approval1.8.1?
Comment on attachment 234548 [details] [diff] [review]
fix searchEngineURL

hold that thought, need to remove the old location still.
Attachment #234548 - Flags: review?(mconnor)
Attachment #234548 - Attachment is obsolete: true
Attachment #234550 - Flags: review?(mconnor)
Attachment #234550 - Flags: approval1.8.1?
Attachment #234548 - Flags: approval1.8.1?
Comment on attachment 234550 [details] [diff] [review]
fix searchEnginesURL + remove old value

r+a=me on this straightforward followup, thanks!
Attachment #234550 - Flags: review?(mconnor)
Attachment #234550 - Flags: review+
Attachment #234550 - Flags: approval1.8.1?
Attachment #234550 - Flags: approval1.8.1+
Whiteboard: [approval needed] → [checkin needed] [checkin needed (1.8 branch)]
Attachment #234567 - Attachment is obsolete: true
Attachment #234569 - Flags: review?(mconnor)
Attachment #234569 - Flags: approval1.8.1?
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
Attachment #234569 - Flags: review?(mconnor)
Attachment #234569 - Flags: review+
Attachment #234569 - Flags: approval1.8.1?
Attachment #234569 - Flags: approval1.8.1+
Status: ASSIGNED → RESOLVED
Closed: 14 years ago14 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
Whiteboard: [checkin needed] [checkin needed (1.8 branch)]
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).
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.
(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.
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?
(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.
(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.


(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).
(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.
(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.

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/
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
Attachment #234619 - Flags: review?(mconnor)
sorry, upped wrong patch. here's the trunk patch.
Attachment #234619 - Attachment is obsolete: true
Attachment #234620 - Flags: review?(mconnor)
Attachment #234619 - Flags: review?(mconnor)
same as previous trunk patch, except it adds the safebrowsing pref, which was missing from last night's SB patch.
Attachment #234621 - Flags: review?(mconnor)
Attachment #234621 - Flags: approval1.8.1?
Steffen:
See bug 346186 comment #6 for the 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.
Attachment #234620 - Attachment is obsolete: true
Attachment #234632 - Flags: review?(mconnor)
Attachment #234620 - Flags: review?(mconnor)
Attachment #234621 - Attachment is obsolete: true
Attachment #234621 - Flags: review?(mconnor)
Attachment #234621 - Flags: approval1.8.1?
(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 :)
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. :)
Status: RESOLVED → REOPENED
Keywords: fixed1.8.1
Resolution: FIXED → ---
(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.

(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. :)
(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.
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
(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 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
Attachment #234632 - Flags: review?(mconnor)
Attachment #234632 - Flags: review+
Attachment #234632 - Flags: approval1.8.1?
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.
Blocks: 346186
Moving to beta2 as per comment 57
Target Milestone: Firefox 2 beta2 → Firefox 2
Whiteboard: [Fx2b2 rider]
Whiteboard: [Fx2b2 rider] → [Fx2b2 rider][181approval pending]
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.
Attachment #234816 - Flags: review?(mconnor)
Attachment #234816 - Flags: approval1.8.1?
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)
Attachment #234632 - Attachment is obsolete: true
Attachment #234825 - Flags: approval1.8.1?
Attachment #234632 - Flags: approval1.8.1?
*** Bug 337823 has been marked as a duplicate of this bug. ***
Adds a trailing slash to the SB info URL.
Attachment #234825 - Attachment is obsolete: true
Attachment #234851 - Flags: approval1.8.1?
Attachment #234825 - Flags: approval1.8.1?
Depends on: 349574
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 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.
Attachment #234851 - Flags: approval1.8.1? → approval1.8.1+
Whiteboard: [Fx2b2 rider][181approval pending] → [Fx2b2 rider]
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. 
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
(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.
(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.

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

(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 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)
Attachment #234816 - Flags: review?(mconnor) → review+
Whiteboard: [Fx2b2 rider] → [has patch][needs approval]
(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 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?
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!
Apparently, the branch also has no chrome privs for about:
Depends on: 349985
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.
No longer depends on: 349985
Depends on: 349985
(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 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)
Attachment #234816 - Flags: approval1.8.1? → approval1.8.1+
Depends on: 349729
(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.
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.
Status: REOPENED → RESOLVED
Closed: 14 years ago14 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
Whiteboard: [has patch][needs approval]
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.
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?
Either way, it's way too late. I commented in the other bug, too.
You need to log in before you can comment on or make changes to this bug.