Closed Bug 328988 Opened 14 years ago Closed 13 years ago

kill builtinURLs.rdf

Categories

(Core :: Spelling checker, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: Pike, Assigned: mscott)

References

()

Details

(Keywords: verified1.8.1.1, Whiteboard: url consolidation)

Attachments

(5 files, 4 obsolete files)

The only real usecase for builtinURLs.rdf is rn:clienturl:composer:spellcheckers,
linking to http://www.mozilla.org/products/thunderbird/dictionaries.html.

I'd like to remove builtinURLs.rdf completely and replace it with a properties
file in branding. Though the precise location of that depends to some extent
on how we deal with spell checker inside Firefox, ie., how we use this URL
in firefox, what we want to expose there.
Whiteboard: url consolidation
Would be nice to go for removing that file in SeaMonkey as well, as it's just useless.
If it comes down to a reference in toolkit for spell checking, it's certainly what we want to use as well :)

We'll be watching progress here closely ;-)
While we are fixing this bug, I think we want to change the dictionary URL for Thunderbird to:

https://addons.mozilla.org/search.php?cat=68&app=thunderbird&type=E

or maybe even better
https://addons.mozilla.org/search.php?cat=68&app=%APP_ID%&type=E

substituting in the appropriate APP ID if the property lives in toolkit like we do in the extension manager for addon URLs.

I don't think this URL is going to be brand or region specific. What do you think Axel?

Thanks Benjamin. 

It looks like Firefox made this just a pref and not a property at all. Ok with you if I do the same in thunderbird axel?

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/browser/app/profile&command=DIFF_FRAMESET&file=firefox.js&rev1=1.71.2.55&rev2=1.71.2.56&root=/cvsroot
Yes.
Attached patch work in progress (obsolete) — Splinter Review
This patch doesn't actually work for me because the value of intl.accept_languages for me is: "en-US, en"

So the URL we try to load looks like:

https://addons.mozilla.org/en-us%2C%20en/thunderbird/3.0a1/dictionaries/

Someone else has already reported this in the firefox bug for this.
Attached patch work in progress (obsolete) — Splinter Review
I didn't include the all-thunderbird.js changes with my earlier patch. While I was here, I removed a bunch of dead prefs for update that were removed from Firefox's firefox.js file when I wasn't looking.
Attachment #232637 - Attachment is obsolete: true
Attached patch better fix (obsolete) — Splinter Review
This patch obsoletes the thunderbird fork of EdSpellCheck.js, sharing code with the suite. 

Per discussion with Kairo and Neil, I'm leaving the seamonkey URL alone (since that one works while the new format doesn't work on AUS yet).
Attachment #232638 - Attachment is obsolete: true
(In reply to comment #6)
>This patch doesn't actually work for me because the value of
>intl.accept_languages for me is: "en-US, en"
Since intl.accept_languages is already a localised string, why not put the URL in the .properties file directly? The langpack will have to change for each version and language anyway.
Neil, the discussion in bug 343076 may run into not using the accept_language var after all, but maybe the UI locale.
We'd like to avoid any URLs in l10n packages. The point of using intl.accept_locales was to obey user preferenes, but that's been vetoed anyway.
(In reply to comment #11)
> We'd like to avoid any URLs in l10n packages.

That's your plan for FF and maybe TB, but not ours for SeaMonkey. We'd like to keep URLs in L10n, so localizers can set their local pages if wanted/needed for any of those services. Note that I've been informed that the new url formatter does support that (and I still hope that's true) via localized prefs, so it can work for all of us.
Comment on attachment 234032 [details] [diff] [review]
better fix

This patch looks good to me, except that we'd still like it to be a localized pref in SeaMonkey.
Actually, for Thunderbird it might be best to use the new URL formatter for that, I think - unfortunately that one's not available in xpfe-based SeaMonkey, only in the toolkit-based version (else it would also solved the other issue as it supports reading a localized pref as well, I'm told).

BTW, you can kill the toolkit builtinURLs.rdf as well, as you're the only user of that file (and actually the only user of the xpfe builtinURLs.js file, as we have forked communicator chrome into suite/)
mscott:
Any progress on this? I'd really love to get my removal of builtinURLs in SeaMonkey into the tree that I have ready and reviewed in bug 339415, but we share EdSpellCheck.xul, which still refers to it...
Attached patch updated fixSplinter Review
This patch uses the new url formatter for Thunderbird. It leaves the string alone for seamonkey which I believe is what we want per kairo. I also removed toolkit's builtinURLs.rdf.

Neil/kairo, can one of you review the shared seamonkey portion of this patch, i.e. the mozilla/editor changes?
Attachment #234032 - Attachment is obsolete: true
Attachment #241892 - Flags: review?(neil)
Comment on attachment 241892 [details] [diff] [review]
updated fix

This looks good to me, though the comment may better say "xpfe-based SeaMonkey" instead of just "seamonkey", as the actual SeaMonkey release based Gecko 1.9 should get the toolkit-based code that also supports the URL formatter.

For the SeaMonkey side, we should add a hardcoded AMO URL to the xpfe-based browser-prefs.js and a localized pref with URL formatter variables in the suiterunner browser-prefs.js and the relevant brand.properties.
Comment on attachment 241892 [details] [diff] [review]
updated fix

>-pref("editor.spellcheckers.url","chrome://editor-region/locale/region.properties");
We still have code using this pref. r=me if you leave it in for now.

>+        opener.open(dictionaryUrl);
This is actually my fault, it should be opener.openDialog(getBrowserURL(), "_blank", "all,dialog=no", dictionaryUrl);

>+  // if we didn't have access to the formatter (i.e. seamonkey)
Nit: Adding the formatter to SeaMonkey is a simple Makefile.in change.
Attachment #241892 - Flags: review?(neil) → review+
Daniel, can you give moa for the editor spell check dialog change in this patch to stop using the dictionary URL from builtinURLs? Thanks!
Attachment #242099 - Flags: review?(daniel)
Comment on attachment 242099 [details] [diff] [review]
fix including neil's review comments

moa=me for editor changes
Comment on attachment 242099 [details] [diff] [review]
fix including neil's review comments

benjamin, can you give moa for the toolkit change that cvs removes builtinURLS.rdf? Thunderbird was the only consumer and we were only using it for the dictionary URL. Thanks.
Attachment #242099 - Flags: review?(daniel) → review?(benjamin)
So as to avoid the try/catch special-casing Scott proposes for EdSpellCheck.js
Attachment #242506 - Flags: review?(benjamin)
Neil, don't forget the packaging changes for the urlformatter too for the suite...I know I forgot it for thunderbird the first time around :)
Comment on attachment 242099 [details] [diff] [review]
fix including neil's review comments

>Index: editor/ui/composer.js

>+pref("editor.dictionaries.download.url", "https://addons.mozilla.org/search.php?cat=68&app=seamonkey&type=E");

This URL is likely to break with Remora. Please work with the addons team to find an official/standard entrypoint. r=me with an official URL.
Attachment #242099 - Flags: review?(benjamin) → review+
Comment on attachment 242506 [details] [diff] [review]
Build urlformatter on non-xul-app suite

We should build this component unconditionally, http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/components/Makefile.in&rev=1.62&mark=46-50#44
Attachment #242506 - Flags: review?(benjamin) → review-
Attachment #242506 - Attachment is obsolete: true
Attachment #243206 - Flags: review?(benjamin)
Attachment #243206 - Flags: review?(benjamin) → review+
(In reply to comment #23)
> (From update of attachment 242099 [details] [diff] [review] [edit])
> >Index: editor/ui/composer.js
> 
> >+pref("editor.dictionaries.download.url", "https://addons.mozilla.org/search.php?cat=68&app=seamonkey&type=E");
> 
> This URL is likely to break with Remora. Please work with the addons team to
> find an official/standard entrypoint. r=me with an official URL.
> 

Benjamin, what's Remora? That's the first time I've heard of that.

Kairo, can you work with the addons team to figure out the right URL for seamonkey? I'm happy to plug that URL into the patch. 

> Kairo, can you work with the addons team to figure out the right URL for
> seamonkey? I'm happy to plug that URL into the patch. 

https://%LOCALE%.add-ons.mozilla.org/%LOCALE%/%APP%/%VERSION%/dictionaries/ should be correct for any of our applications.

I'd prefer though if we'd have it as a localized pref in our SeaMonkey-specific browser-prefs.js file(s) - maybe the correct solution is overriding composer.js from there, just as you do it for Thunderbird, if understand what you're doing correctly.
Attachment #243206 - Flags: approval1.8.1.1?
Comment on attachment 242099 [details] [diff] [review]
fix including neil's review comments

>+function getDictionaryURL()
>+{
>+  try {
>+    var formatter = Components.classes["@mozilla.org/toolkit/URLFormatterService;1"]
>+                    .getService(Components.interfaces.nsIURLFormatter);
>+                    
>+    return formatter.formatURLPref("editor.dictionaries.download.url");
>+  }
>+  catch (e) {}
>+  
>+  // if we didn't have access to the formatter (i.e. xpfe seamonkey), just return
>+  // the value of the pref.
>+  var prefService = GetPrefs();
>+  return prefService.getCharPref("editor.dictionaries.download.url");  
>+}
At least on trunk, and hopefully on branch soon, we don't need this try/catch stuff because all apps have the formatter.
Neil:
We're about to do a string freeze on branch for all of SeaMonkey, so I'm not for taking the builtinURLs removal itself on the branch.

The addition of urlformatter for everyone would be nice to have on both trunk and branch though, that's correct.
(In reply to comment #28)
> > Kairo, can you work with the addons team to figure out the right URL for
> > seamonkey? I'm happy to plug that URL into the patch. 
> 
> https://%LOCALE%.add-ons.mozilla.org/%LOCALE%/%APP%/%VERSION%/dictionaries/
> should be correct for any of our applications.
> 
> I'd prefer though if we'd have it as a localized pref in our SeaMonkey-specific
> browser-prefs.js file(s) - maybe the correct solution is overriding composer.js
> from there, just as you do it for Thunderbird, if understand what you're doing
> correctly.
> 

We should either:

1) use 
https://%LOCALE%.add-ons.mozilla.org/%LOCALE%/%APP%/%VERSION%/dictionaries/
in composer.js (and I will remove the pref in thunderbird.js)

2) set the pref on a per app basis. In thunderbird.js and in the suite's JS file. 

I'm happy to do either. If you want seamonkey to have control of the pref we can make it an app pref.
I think an app pref is probably the best way to go, given we have something in to fail gracefully if the pref is not set (possible app programmer's error if someone else will/would reuse the code)
Kairo,

Here are the changes that add the pref to the two suite browser-prefs.js files. Do you want to review that change and make sure it's what you want?

I also got rid of the try/catch in the editor spell check routine since Neil now has the suite building the urlformatter on the trunk. 

I'll land the entire patch on the trunk once I get the green light for the seamonkey prefs in this patch.
Attachment #243850 - Flags: review?
Attachment #243850 - Flags: review? → review?(kairo)
Changes for Thunderbird only for the 1.8.1 branch. Use the url formatter and the new url format for the spell check dictionaries. 

For the 1.8.1 branch, we'll leave out the builtinURLS.rdf removal in toolkit (per kairo's earlier comment), leaving out merging EdSpellCheck.js with seamonkey, etc.
Attachment #243869 - Flags: superreview?(bienvenu)
Comment on attachment 243869 [details] [diff] [review]
Thunderbird 1.8.1 Changes

is this part of the change?  :

                 <button id="editButton" label="&editDirectories.label;" 
-                        preference="pref.ldap.disable_button.edit_directories"
Attachment #243869 - Flags: superreview?(bienvenu) → superreview+
Comment on attachment 243850 [details] [diff] [review]
breaking out just the editor and seamonkey changes

r=me for the browser-prefs.js changes, thanks for doing that!
Attachment #243850 - Flags: review?(kairo) → review+
Fixed on the trunk for seamonkey and thunderbird.

I landed just the thunderbird portion on the branch.

thanks a lot everyone.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Blocks: 359174
Comment on attachment 243206 [details] [diff] [review]
Build urlformatter everywhere

approved for 1.8 branch, a=dveditz for drivers
Attachment #243206 - Flags: approval1.8.1.1? → approval1.8.1.1+
urlformatter build changes checked in to the branch.
Keywords: fixed1.8.1.1
What would be an appropriate testcase/testing scenario for this bug?

Whiteboard: url consolidation → url consolidation, [need testcase]
If I understand it correctly, verifying that the "Add dictionaries..." item in the editor context menu (as seen in a textarea or message compose window when right clicking on a misspelled word) works (e.g. takes you to addons.mozilla.org) would be enough to verify this bug.
verified that the link has changed from 
http://www.mozilla.org/products/thunderbird/dictionaries.html
to 
http://addons.mozilla.org/$locale/thunderbird/$appversion/dictionaries/

on fedora core 5, thunderbird 2.0.0.1pre 20061201 (first confirmed bad url on an older build).
Whiteboard: url consolidation, [need testcase] → url consolidation
You need to log in before you can comment on or make changes to this bug.