328988 - kill builtinURLs.rdf

Status: RESOLVED FIXED

(Core :: Spelling checker, defect)

Description

[Axel Hecht [:Pike]]

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.

Comment 1

[Robert Kaiser]

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 ;-)

Comment 2

[Scott MacGregor]

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?

Comment 3

[Benjamin Smedberg]

https://bugzilla.mozilla.org/show_bug.cgi?id=343076#c17

The URL should be
http://addons.mozilla.org/$locale/thunderbird/$appversion/dictionaries/

Comment 4

[Scott MacGregor]

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

Comment 5

[Axel Hecht [:Pike]]

Yes.

Comment 6

[Scott MacGregor]

Attachment: work in progress

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.

Comment 7

[Scott MacGregor]

Attachment: work in progress

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.

Comment 8

[Scott MacGregor]

Attachment: better fix

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

Comment 9

[[e-mail]]

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

Comment 10

[Robert Kaiser]

Neil, the discussion in bug 343076 may run into not using the accept_language var after all, but maybe the UI locale.

Comment 11

[Benjamin Smedberg]

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.

Comment 12

[Robert Kaiser]

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

[Robert Kaiser]

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/)

Comment 14

[Robert Kaiser]

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

Comment 15

[Scott MacGregor]

Attachment: updated fix

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?

Comment 16

[Robert Kaiser]

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 17

[[e-mail]]

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.

Comment 18

[Scott MacGregor]

Attachment: fix including neil's review comments

Daniel, can you give moa for the editor spell check dialog change in this patch to stop using the dictionary URL from builtinURLs? Thanks!

Comment 19

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Comment on attachment 242099 [details] [diff] [review]
fix including neil's review comments

moa=me for editor changes

Comment 20

[Scott MacGregor]

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.

Comment 21

[[e-mail]]

Attachment: Build urlformatter on non-xul-app suite

So as to avoid the try/catch special-casing Scott proposes for EdSpellCheck.js

Comment 22

[Scott MacGregor]

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 23

[Benjamin Smedberg]

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.

Comment 24

[Benjamin Smedberg]

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

Comment 25

[[e-mail]]

Attachment: Build urlformatter everywhere

Comment 26

[Scott MacGregor]

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

Comment 27

[Benjamin Smedberg]

http://wiki.mozilla.org/Update:Remora

Comment 28

[Robert Kaiser]

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

Comment 29

[[e-mail]]

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.

Comment 30

[Robert Kaiser]

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.

Comment 31

[Scott MacGregor]

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

Comment 32

[Robert Kaiser]

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)

Comment 33

[Scott MacGregor]

Attachment: breaking out just the editor and seamonkey changes

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.

Comment 34

[Scott MacGregor]

Attachment: Thunderbird 1.8.1 Changes

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.

Comment 35

[David :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"

Comment 36

[Robert Kaiser]

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!

Comment 37

[Scott MacGregor]

Fixed on the trunk for seamonkey and thunderbird.

I landed just the thunderbird portion on the branch.

thanks a lot everyone.

Comment 38

[Daniel Veditz [:dveditz]]

Comment on attachment 243206 [details] [diff] [review]
Build urlformatter everywhere

approved for 1.8 branch, a=dveditz for drivers

Comment 39

[[e-mail]]

urlformatter build changes checked in to the branch.

Comment 40

[alice nodelman [:alice] [:anode]]

What would be an appropriate testcase/testing scenario for this bug?

Comment 41

[Adam Guthrie]

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.

Comment 42

[alice nodelman [:alice] [:anode]]

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