Last Comment Bug 591780 - spell checker should support dictionaries from restartless addons
: spell checker should support dictionaries from restartless addons
Status: RESOLVED FIXED
: dev-doc-needed
Product: Core
Classification: Components
Component: Spelling checker (show other bugs)
: unspecified
: All All
: -- enhancement with 5 votes (vote)
: mozilla10
Assigned To: Jesper Kristensen
:
:
Mentors:
: 338074 759088 (view as bug list)
Depends on: 687319 697981 751157 782115 782118
Blocks: 338074 641304 687420 692765 700631 760817
  Show dependency treegraph
 
Reported: 2010-08-29 07:43 PDT by Jesper Kristensen
Modified: 2012-11-08 03:31 PST (History)
24 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch v1 (82.38 KB, patch)
2011-05-09 05:08 PDT, Jesper Kristensen
ehsan: review-
Details | Diff | Splinter Review
test addon (5.02 KB, application/x-xpinstall)
2011-05-09 05:18 PDT, Jesper Kristensen
no flags Details
script from test addon (3.28 KB, text/plain)
2011-05-09 05:19 PDT, Jesper Kristensen
no flags Details
bootstrap.js (347 bytes, text/plain)
2011-05-09 05:20 PDT, Jesper Kristensen
no flags Details
patch v2 (62.70 KB, patch)
2011-05-15 04:49 PDT, Jesper Kristensen
no flags Details | Diff | Splinter Review
patch v3 (86.09 KB, patch)
2011-05-19 11:23 PDT, Jesper Kristensen
dtownsend: feedback+
Details | Diff | Splinter Review
patch v4 (124.23 KB, patch)
2011-05-20 15:07 PDT, Jesper Kristensen
no flags Details | Diff | Splinter Review
patch v5, editor part (72.29 KB, patch)
2011-05-21 11:32 PDT, Jesper Kristensen
ehsan: review-
Details | Diff | Splinter Review
patch v5, addons manager part (62.17 KB, patch)
2011-05-21 11:33 PDT, Jesper Kristensen
dtownsend: review-
Details | Diff | Splinter Review
patch v6, editor part (64.27 KB, patch)
2011-05-29 08:16 PDT, Jesper Kristensen
no flags Details | Diff | Splinter Review
patch v6, addons manager part (74.99 KB, patch)
2011-05-29 08:16 PDT, Jesper Kristensen
no flags Details | Diff | Splinter Review
patch v7, editor part (64.44 KB, patch)
2011-06-11 03:30 PDT, Jesper Kristensen
ehsan: review+
Details | Diff | Splinter Review
patch v7, addons manager part (75.06 KB, patch)
2011-06-11 03:31 PDT, Jesper Kristensen
no flags Details | Diff | Splinter Review
patch v8, editor part (65.29 KB, patch)
2011-06-14 01:41 PDT, Jesper Kristensen
bugs: review+
Details | Diff | Splinter Review
patch v9, editor part (65.31 KB, patch)
2011-06-28 13:56 PDT, Jesper Kristensen
bugs: review+
Details | Diff | Splinter Review
Dictionary Icon - Gnomestripe (Linux) (1.26 KB, image/png)
2011-08-08 13:55 PDT, Stephen Horlander [:shorlander]
no flags Details
Dictionary Icon - Pinstripe (OS X) (1.73 KB, image/png)
2011-08-08 13:56 PDT, Stephen Horlander [:shorlander]
no flags Details
Dictionary Icon - Winstripe (Windows) (1.63 KB, image/png)
2011-08-08 13:57 PDT, Stephen Horlander [:shorlander]
no flags Details
patch v10, editor part (65.26 KB, patch)
2011-08-09 05:25 PDT, Jesper Kristensen
no flags Details | Diff | Splinter Review
patch v10, addons manager part (86.69 KB, patch)
2011-08-09 05:28 PDT, Jesper Kristensen
dtownsend: review+
Details | Diff | Splinter Review
Dictionary Icon - Gnomestripe (Linux) - 16x16 (584 bytes, image/png)
2011-08-16 12:46 PDT, Stephen Horlander [:shorlander]
no flags Details
Dictionary Icon - Pinstripe (OS X) - 16x16 (742 bytes, image/png)
2011-08-16 12:47 PDT, Stephen Horlander [:shorlander]
no flags Details
Dictionary Icon - Winstripe (Windows) - 16x16 (733 bytes, image/png)
2011-08-16 12:48 PDT, Stephen Horlander [:shorlander]
no flags Details
patch v11, editor part (65.52 KB, patch)
2011-08-17 12:29 PDT, Jesper Kristensen
ehsan: review+
Details | Diff | Splinter Review
patch v11, addons manager part (93.77 KB, patch)
2011-08-17 12:32 PDT, Jesper Kristensen
dtownsend: review-
Details | Diff | Splinter Review
patch v12, editor part (64.90 KB, patch)
2011-09-13 11:44 PDT, Jesper Kristensen
ehsan: review+
Details | Diff | Splinter Review
patch v12, addons manager part (97.23 KB, patch)
2011-09-16 10:48 PDT, Jesper Kristensen
ehsan: review+
dtownsend: review+
Details | Diff | Splinter Review

Description Jesper Kristensen 2010-08-29 07:43:58 PDT
Now that the addons manager in Firefox 4 supports addons which can be installed without a restart, I would like to make it possible for such addons to provide a dictionary, such that dictionary addons can be made restartless.

Currently it seems that the list of dictionaries is found at startup and then cashed in various places throughout the code.
Comment 1 Jesper Kristensen 2011-05-09 05:08:55 PDT
Created attachment 531016 [details] [diff] [review]
patch v1

This patch allows dictionaries to be added and removed via an an API from restartless addons. My overall approach is to remove any part of the code which keep track of the list of installed dictionaries and the current dictionary when possible, and put that into a separate component.

This means that there the currently active dictionary is a global setting. I am not 100% sure if that is what is wanted, but the current code for allowing multiple concurrent active dictionaries in different editors does not work, and I don't think it would be very useful as a base for a working version anyway. 

Before this patch, two separate editors could have different active dictionaries, but only if those dictionaries came from two different engined. In practice however, there is only one engine, which is hunspell.

Before, if you had some text in a spell checked editor, and you changed the currently active dictionary from the context menu of a different editor, the spell check red underlines would not update in your editor, but the context menu for the editor would show the updated current dictionary, and typing in new text would spell check using the updated dictionary. So the internal state of the spell checker did not match what was shown in the editor. This patch fixes that by updating all editors when the current dictionary changes.
Comment 2 Jesper Kristensen 2011-05-09 05:18:01 PDT
Created attachment 531018 [details]
test addon

This addon makes some tests of how the API for adding and removing dictionaries affects the spell checking API and spell checked editors.

It can probably be converted into an automated test, but I don't know a lot about the Mozilla testing frameworks, so I have made it as an XPI.

Run the test by loading chrome://spellchecktest/content/spellchecktest.xul in the browser and see the result in the error console:

Error: RESULTS:
PASS: base is available was '2' didn't expect '-1'
PASS: map is available was '0' didn't expect '-1'
PASS: base misspellings was 'Frühstückqwertyu' expected 'Frühstückqwertyu'
PASS: current dictionary was 'base_utf' expected 'base_utf'
PASS: map misspellings was 'createdimplytomorrowqwertyu' expected 'createdimplytomorrowqwertyu'
PASS: current dictionary was 'maputf' expected 'maputf'
PASS: map misspellings was 'Frühstückqwertyu' didn't expect 'createdimplytomorrowqwertyu'
PASS: current dictionary was 'en-US' didn't expect 'maputf'
PASS: base is available was '1' didn't expect '-1'
PASS: map is unavailable was '-1' expected '-1'

Source File: chrome://spellchecktest/content/spellchecktest.js
Line: 11
Comment 3 Jesper Kristensen 2011-05-09 05:19:33 PDT
Created attachment 531020 [details]
script from test addon

The test addon I just attached runs this test.
Comment 4 Jesper Kristensen 2011-05-09 05:20:52 PDT
Created attachment 531022 [details]
bootstrap.js

The bootstrap.js file from a restartless dictionary addon.
Comment 5 :Ehsan Akhgari 2011-05-10 12:28:28 PDT
Comment on attachment 531016 [details] [diff] [review]
patch v1

I took a general look at this patch, and I think a big design decision made here is incorrect.  I don't think that we want dictionary manager to be global across the application.  We do want individual editors to be able to use their own dictionaries, but I think we should just let individual spellcheckers know when a new dictionary is available.

Also, this patch requires tests.  You should also change the IID of the interfaces that you're changing too.

In order to make it easier to review this, I would really appreciate if you can split this patch into logical units.  For example, if you need to add some new APIs, you can add them + the required tests in part 1, and then build on top of that in the other parts of your patch.

Thanks a lot for taking on this mission.  :-)
Comment 6 :Ehsan Akhgari 2011-05-10 12:29:30 PDT
Also, I think you should talk to Mossop as far as integrating with the add-ons manager goes.  There may be better ways to make restartless dictionaries, and he can tell you whether that's true or not.
Comment 7 Dave Townsend [:mossop] 2011-05-10 12:33:32 PDT
We've long had bug 564681 on file and I've avoided fixing it because it wouldn't support all existing dictionaries, but if you're looking to change them anyway, why not just give them a new em:type in install.rdf and the add-ons manager could do everything without needing a bootstrap.js.
Comment 8 Jesper Kristensen 2011-05-10 12:51:23 PDT
(In reply to comment #5)
> Comment on attachment 531016 [details] [diff] [review] [review]
> patch v1
> 
> I took a general look at this patch, and I think a big design decision made
> here is incorrect.  I don't think that we want dictionary manager to be
> global across the application.  We do want individual editors to be able to
> use their own dictionaries, but I think we should just let individual
> spellcheckers know when a new dictionary is available.

I somewhat expected that. But what would the target then be? Currently handling different dictionaries in different editors is broken. I can try to keep it in the same broken state, but since I would most likely need to touch the code in any case, I would like to know what the ideal target would be in order to not get farther away from something that would work.

> Also, this patch requires tests.  You should also change the IID of the
> interfaces that you're changing too.

I found this https://developer.mozilla.org/en/Mozilla_automated_testing . I would guess mochitest-chrome would be the one I need to look at, correct?
Comment 9 :Ehsan Akhgari 2011-05-10 14:08:01 PDT
(In reply to comment #8)
> (In reply to comment #5)
> > Comment on attachment 531016 [details] [diff] [review] [review]
> > patch v1
> > 
> > I took a general look at this patch, and I think a big design decision made
> > here is incorrect.  I don't think that we want dictionary manager to be
> > global across the application.  We do want individual editors to be able to
> > use their own dictionaries, but I think we should just let individual
> > spellcheckers know when a new dictionary is available.
> 
> I somewhat expected that. But what would the target then be? Currently handling
> different dictionaries in different editors is broken. I can try to keep it in
> the same broken state, but since I would most likely need to touch the code in
> any case, I would like to know what the ideal target would be in order to not
> get farther away from something that would work.

At some point, we'd want to switch to language specific spell checking (i.e., using the lang attribute).

I agree that things are not perfect right now, but we shouldn't switch to a global architecture which would make it even harder than it already is to support this (and similar features) in the future.

> > Also, this patch requires tests.  You should also change the IID of the
> > interfaces that you're changing too.
> 
> I found this https://developer.mozilla.org/en/Mozilla_automated_testing . I
> would guess mochitest-chrome would be the one I need to look at, correct?

Yes.  Let me know if you have questions there.  :-)
Comment 10 Jesper Kristensen 2011-05-11 12:57:15 PDT
Would it be ok to load the spellchecker objects associated with an editor eagerly instead of lazily? If the editor decides whether to load the spellchecker or not based on if dictionaries are available or not, then the code checking if dictionaries are available cannot be part of that spellchecker. Either I could just always load the spellchecker or I could turn mozInlineSpellChecker::CanEnableInlineSpellChecking and mozInlineSpellCheckerConstructor into something bigger that can watch dictionary availability updates and notify each editor.
Comment 11 Jesper Kristensen 2011-05-15 04:49:45 PDT
Created attachment 532510 [details] [diff] [review]
patch v2

In this patch I have tried to change as little as possible in the logic choosing the current dictionary.

I have tried to add a test, but I could not make it work. It is as if the test framework could not find it. I also don't know how to find the current directory in the test. Can you help me here?

I agree that it would be better to let the addons manager handle loading dictionaries instead of a bootstrap.js script. However, I think that the API I made would work for that too, and I think doing it is fairly independent, so I would not want to do that in one big patch. That could be done in bug 564681 after this bug is done. Another problem with the bootstrap.js approach is that <em:bootstrap> is only a boolean as far as I know, so adding it would make the dictionary incompatible with older versions.
Comment 12 :Ehsan Akhgari 2011-05-15 15:59:49 PDT
Comment on attachment 532510 [details] [diff] [review]
patch v2

Review of attachment 532510 [details] [diff] [review]:
-----------------------------------------------------------------

I think that we need to do what Mossop suggested in comment 7 here.  In other words, I really don't think that it's ideal to require extension dictionaries to include a bootstrap.js which calls addDictionary/removeDictionary.  This will make it unnecessary to add a bunch of APIs to handle dictionary addition/removal.

I'll provide some general review comments, but they may end up being invalid when you change the design...

::: editor/composer/src/nsEditorSpellCheck.cpp
@@ +440,5 @@
>    return mSpellChecker->SetCurrentDictionary(nsDependentString(aDictionary));
>  }
>  
>  NS_IMETHODIMP    
> +nsEditorSpellCheck::CheckCurrentDictionary()

This function is not really necessary, since it just forwards its task to mSpellChecker, right?

@@ -456,5 @@
> -  // to uninitialize
> -#ifdef DEBUG
> -  nsresult rv =
> -#endif
> -  SaveDefaultDictionary();

I think you can just remove SaveDefaultDictionary altogether.

::: extensions/spellcheck/tests/chrome/Makefile.in
@@ +44,5 @@
> +include $(DEPTH)/config/autoconf.mk
> +include $(topsrcdir)/config/rules.mk
> +
> +_TEST_FILES = 	test_add_remove_dictionaries.xul \
> +		$(NULL)

You should also add the .dic and .aff files you need for the test.  Otherwise, they won't be packaged with the tests, and the test would not be able to find them when running.

::: extensions/spellcheck/tests/chrome/test_add_remove_dictionaries.xul
@@ +19,5 @@
> +  // <textbox id="textbox" spellcheck="true" value="created imply Frühstück tomorrow qwertyu"/>
> +  var textbox = document.getElementById('textbox');
> +  var editor = textbox.editor;
> +
> +  var dir = ...TODO...;

See <https://developer.mozilla.org/en/Code_snippets/File_I%2f%2fO#Getting_special_files>.  You should use "CurWorkD".

@@ +73,5 @@
> +
> +  // select base dictionary
> +  setCurrentDictionary(editor, "base_utf");
> +
> +  setTimeout(function(){ // TODO how do we know when spell checking finished?

You usually need to hit the event loop once (unless the textarea is very big).  You can do that with SimpleTest.executeSoon().  We have a bunch of tests which do this type of thing, see <http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/html/tests/test_bug484181.html> for example.
Comment 13 :Ehsan Akhgari 2011-05-15 16:01:54 PDT
(In reply to comment #10)
> Would it be ok to load the spellchecker objects associated with an editor
> eagerly instead of lazily? If the editor decides whether to load the
> spellchecker or not based on if dictionaries are available or not, then the
> code checking if dictionaries are available cannot be part of that
> spellchecker. Either I could just always load the spellchecker or I could
> turn mozInlineSpellChecker::CanEnableInlineSpellChecking and
> mozInlineSpellCheckerConstructor into something bigger that can watch
> dictionary availability updates and notify each editor.

I'm wondering if we need to do this at all.  If I install a new dictionary, I may not expect all of the open tabs to switch to using that dictionary immediately.  We might need to handle the case where a dictionary goes away though...
Comment 14 :Ehsan Akhgari 2011-05-15 16:05:41 PDT
(In reply to comment #11)
> I have tried to add a test, but I could not make it work. It is as if the
> test framework could not find it. I also don't know how to find the current
> directory in the test. Can you help me here?

I think the test file itself should be copied, but the dictionary files are not.  I commented about why.

> I agree that it would be better to let the addons manager handle loading
> dictionaries instead of a bootstrap.js script. However, I think that the API
> I made would work for that too, and I think doing it is fairly independent,
> so I would not want to do that in one big patch. That could be done in bug
> 564681 after this bug is done. Another problem with the bootstrap.js
> approach is that <em:bootstrap> is only a boolean as far as I know, so
> adding it would make the dictionary incompatible with older versions.

What if we preserve the em:type for dictionaries, but only allow <em:bootstrap> for them (without requiring an actual bootstrap.js)?  That way, the dictionaries will be backwards compatible, and we would still get the benefit of not requiring dictionary authors to provide a bootstrap.js file.

Mossop, what do you think?
Comment 15 Dave Townsend [:mossop] 2011-05-15 16:32:57 PDT
(In reply to comment #14)
> What if we preserve the em:type for dictionaries, but only allow
> <em:bootstrap> for them (without requiring an actual bootstrap.js)?  That
> way, the dictionaries will be backwards compatible, and we would still get
> the benefit of not requiring dictionary authors to provide a bootstrap.js
> file.
> 
> Mossop, what do you think?

We don't currently have an em:type for dictionaries, they just share it with extensions so the only way we could do some special handling without breaking backwards compatibility would be a new property in install.rdf and I really don't want to do that.
Comment 16 Jesper Kristensen 2011-05-16 03:44:39 PDT
(In reply to comment #12)
> I think that we need to do what Mossop suggested in comment 7 here.  In
> other words, I really don't think that it's ideal to require extension
> dictionaries to include a bootstrap.js which calls
> addDictionary/removeDictionary.  This will make it unnecessary to add a
> bunch of APIs to handle dictionary addition/removal.

I see. Though I believe the API between the addons manager and the spell checker could be the same as the one I made between the addon and the spell checker, I cannot know before I have looked at the addons manager.

> ::: editor/composer/src/nsEditorSpellCheck.cpp
> @@ +440,5 @@
> >    return mSpellChecker->SetCurrentDictionary(nsDependentString(aDictionary));
> >  }
> >  
> >  NS_IMETHODIMP    
> > +nsEditorSpellCheck::CheckCurrentDictionary()
> 
> This function is not really necessary, since it just forwards its task to
> mSpellChecker, right?

We either need a function to forward the task, or a function to access mSpellChecker. Since there were other functions in nsEditorSpellCheck which used the forwarding approach, I did the same. But I can just add a GetSpellChecker function instead.

> 
> @@ -456,5 @@
> > -  // to uninitialize
> > -#ifdef DEBUG
> > -  nsresult rv =
> > -#endif
> > -  SaveDefaultDictionary();
> 
> I think you can just remove SaveDefaultDictionary altogether.

And by that you mean that you mean that InlineSpellChecker.jsm should just set the preference itself instead of calling SaveDefaultDictionary to do so?

(In reply to comment #13)
> I'm wondering if we need to do this at all.  If I install a new dictionary,
> I may not expect all of the open tabs to switch to using that dictionary
> immediately.  We might need to handle the case where a dictionary goes away
> though...

My patch doesn't do that. Only when the first dictionary is installed, all editors with spell checking active switches to that. That is the same behavior as Firefox has now, except that there is a restart in between.

(In reply to comment #14)
> What if we preserve the em:type for dictionaries, but only allow
> <em:bootstrap> for them (without requiring an actual bootstrap.js)?  That
> way, the dictionaries will be backwards compatible, and we would still get
> the benefit of not requiring dictionary authors to provide a bootstrap.js
> file.

Adding an <em:bootstrap> would make dictionaries backwards incompatible, as the current code only searches for dictionaries within addons without <em:bootstrap>. I also find it a strange assumption that <em:bootstrap> + no bootstrap.js = dictionary addon.

(In reply to comment #15)
> We don't currently have an em:type for dictionaries, they just share it with
> extensions so the only way we could do some special handling without
> breaking backwards compatibility would be a new property in install.rdf and
> I really don't want to do that.

So none of our options are very attractive. Then what do we do? I do not see any other options than adding an em:type, adding a new property, or overloading the existing em:bootstrap.
Comment 17 Dave Townsend [:mossop] 2011-05-16 09:40:13 PDT
(In reply to comment #16)
> (In reply to comment #14)
> > What if we preserve the em:type for dictionaries, but only allow
> > <em:bootstrap> for them (without requiring an actual bootstrap.js)?  That
> > way, the dictionaries will be backwards compatible, and we would still get
> > the benefit of not requiring dictionary authors to provide a bootstrap.js
> > file.
> 
> Adding an <em:bootstrap> would make dictionaries backwards incompatible, as
> the current code only searches for dictionaries within addons without
> <em:bootstrap>. I also find it a strange assumption that <em:bootstrap> + no
> bootstrap.js = dictionary addon.
> 
> (In reply to comment #15)
> > We don't currently have an em:type for dictionaries, they just share it with
> > extensions so the only way we could do some special handling without
> > breaking backwards compatibility would be a new property in install.rdf and
> > I really don't want to do that.
> 
> So none of our options are very attractive. Then what do we do? I do not see
> any other options than adding an em:type, adding a new property, or
> overloading the existing em:bootstrap.

Doesn't seem like there is a way to do this without breaking backwards compatibility for the dictionary XPIs. Is that terrible? As we move into rapid releases the number of users of less than current versions of Firefox should dwindle and the modification required to an XPI to just set an em:type is pretty trivial.
Comment 18 Jesper Kristensen 2011-05-16 09:58:27 PDT
(In reply to comment #17)
It is not terrible, but also not perfect. I would certainly wait longer to update my dictionary addon if it made it backwards incompatible. For how long would depend on how quickly we get users of old Firefox and Thunderbird branches onto the rapid release cycle.

My main intension was to say, that since none of the options are obviously perfect, someone with the decision making power (you?) should state what route is preferred before I want to try to implement it.
Comment 19 Dave Townsend [:mossop] 2011-05-16 10:04:47 PDT
(In reply to comment #18)
> (In reply to comment #17)
> It is not terrible, but also not perfect. I would certainly wait longer to
> update my dictionary addon if it made it backwards incompatible. For how
> long would depend on how quickly we get users of old Firefox and Thunderbird
> branches onto the rapid release cycle.
> 
> My main intension was to say, that since none of the options are obviously
> perfect, someone with the decision making power (you?) should state what
> route is preferred before I want to try to implement it.

It is a little more work but having a custom em:type makes it much easier to handle dictionaries in custom ways in the future so I'd prefer that.
Comment 20 Jesper Kristensen 2011-05-19 11:23:28 PDT
Created attachment 533719 [details] [diff] [review]
patch v3

Ehsan:
I think I have done the changed to the test you said, but I still cannot get the test to run. I try to run it with 

python runtests.py --chrome --test-path=extensions/spellcheck/tests/chrome/test_add_remove_dictionaries.xul

It opens up a browser window where the list of tests to run is empty. And clicking the Run Chrome Tests button in the top does not give any reaction. I do not have any ideas about what I am doing wrong.

Dave:
Is this the right approach to use for the addons manager? I just register all of the spellcheck type as bootstrapped, and then execute some built in js instead of bootstrap.js from the addon.

I am also pretty lost on how to write a test for it. There are lots of different tests for the addons manager, and I cannot see which ones are appropriate, or find any test that is similar to what I think I should do.
Comment 21 Dave Townsend [:mossop] 2011-05-19 12:07:40 PDT
Comment on attachment 533719 [details] [diff] [review]
patch v3

Review of attachment 533719 [details] [diff] [review]:
-----------------------------------------------------------------

Certainly along the right lines. Using something like test_bootstrap.js would probably be good, mainly you need to check that the changes happen without restart. If you override the hunspell service you can probably test that the calls happen correctly.

::: toolkit/mozapps/extensions/AddonRepository.jsm
@@ +930,5 @@
>                break;
>              case 2:
>                addon.type = "theme";
>                break;
> +// TODO something needed here?

I think there is a bug filed on this somewhere else, but AMO's type ID for dictionaries seems to be 3. Dunno if it'll just work if you put that there.

::: toolkit/mozapps/extensions/XPIProvider.jsm
@@ +148,5 @@
>    extension: 2,
>    theme: 4,
>    locale: 8,
> +  multipackage: 32,
> +  spellcheck: 64

dictionary might be better?

@@ +629,5 @@
>    }
> +  // spell check dictionaries never require a restart
> +  else if (addon.type == "spellcheck") {
> +    addon.bootstrap = true;
> +  }

Don't do this, the type is all we need to know. Instead make installRequiresRestart etc. behave right for the new type.

::: toolkit/mozapps/extensions/content/extensions.xul
@@ +233,5 @@
>                      tooltiptext="&view.features.label;"/>
> +      <richlistitem id="category-spellcheck" value="addons://list/spellcheck"
> +                    class="category"
> +                    name="&view.spellcheck.label;"
> +                    tooltiptext="&view.spellcheck.label;"/>

Probably want this to hide when there are none installed like locales. Taking a guess that this will land after bug 595848 in which case you'll have to update it from that.
Comment 22 Jesper Kristensen 2011-05-19 12:40:01 PDT
(In reply to comment #21)
> ::: toolkit/mozapps/extensions/XPIProvider.jsm
> @@ +148,5 @@
> >    extension: 2,
> >    theme: 4,
> >    locale: 8,
> > +  multipackage: 32,
> > +  spellcheck: 64
> 
> dictionary might be better?

ok. I chose "spellcheck" because I think "dictionary" might be too ambiguous (hyphenation, etc.), and "spellcheckdictionary" is too long. But then again, we might want to group spell check dictionaries with other potential types of dictionaries.

> @@ +629,5 @@
> >    }
> > +  // spell check dictionaries never require a restart
> > +  else if (addon.type == "spellcheck") {
> > +    addon.bootstrap = true;
> > +  }
> 
> Don't do this, the type is all we need to know. Instead make
> installRequiresRestart etc. behave right for the new type.

Do you mean that I should not use callBootstrapMethod, or that I should just update all 'if (addon.bootstrap)' to 'if (addon.bootstrap || addon.type == "spellcheck")'?
Comment 23 Dave Townsend [:mossop] 2011-05-19 14:22:48 PDT
(In reply to comment #22)
> (In reply to comment #21)
> > ::: toolkit/mozapps/extensions/XPIProvider.jsm
> > @@ +148,5 @@
> > >    extension: 2,
> > >    theme: 4,
> > >    locale: 8,
> > > +  multipackage: 32,
> > > +  spellcheck: 64
> > 
> > dictionary might be better?
> 
> ok. I chose "spellcheck" because I think "dictionary" might be too ambiguous
> (hyphenation, etc.), and "spellcheckdictionary" is too long. But then again,
> we might want to group spell check dictionaries with other potential types
> of dictionaries.

Grouping should be handled at the UI level, we should have different types for all those things (if we need them). Are these actually different kinds of dictionaries? If so then I guess 

> > @@ +629,5 @@
> > >    }
> > > +  // spell check dictionaries never require a restart
> > > +  else if (addon.type == "spellcheck") {
> > > +    addon.bootstrap = true;
> > > +  }
> > 
> > Don't do this, the type is all we need to know. Instead make
> > installRequiresRestart etc. behave right for the new type.
> 
> Do you mean that I should not use callBootstrapMethod, or that I should just
> update all 'if (addon.bootstrap)' to 'if (addon.bootstrap || addon.type ==
> "spellcheck")'?

Damn I thought the code didn't use that property much, guess I didn't keep it as abstract as I'd hoped. Guess I'll do that cleanup another time, leave it as-is for now, but make sure the optionsURL and aboutURL still get reset.
Comment 24 Jesper Kristensen 2011-05-20 15:07:42 PDT
Created attachment 534123 [details] [diff] [review]
patch v4

Updated the addons manager part. I do not know how to make an icon for the new category, so I just included a placeholder.
Comment 25 Jesper Kristensen 2011-05-21 11:32:18 PDT
Created attachment 534239 [details] [diff] [review]
patch v5, editor part

(In reply to comment #20)
> Ehsan:
> I think I have done the changes to the test you said, but I still cannot get
> the test to run.

I found out why. I used https://developer.mozilla.org/en/Mochitest#Choosing_a_location which refers to https://bugzilla.mozilla.org/show_bug.cgi?id=368531 which contains some make code that no longer works.
Comment 26 Jesper Kristensen 2011-05-21 11:33:42 PDT
Created attachment 534240 [details] [diff] [review]
patch v5, addons manager part
Comment 27 :Ehsan Akhgari 2011-05-23 11:49:22 PDT
Sorry for my delay here.  I'll do my best to review the latest version of the patch either tomorrow or on Wednesday.
Comment 28 Dave Townsend [:mossop] 2011-05-23 14:18:36 PDT
Comment on attachment 534240 [details] [diff] [review]
patch v5, addons manager part

Review of attachment 534240 [details] [diff] [review]:
-----------------------------------------------------------------

This is basically right, but I think it needs UX input on a couple of points and so I'd like to skim over it one last time once anything has been addressed from there.

::: toolkit/locales/en-US/chrome/mozapps/extensions/extensions.properties
@@ +105,5 @@
>  #LOCALIZATION NOTE (eulaHeader) %S is name of the add-on asking the user to agree to the EULA
>  eulaHeader=%S requires that you accept the following End User License Agreement before installation can proceed:
>  
>  type.extension.name=Extensions
> +type.dictionary.name=Spell check dictionaries

This seems unfortunately long. Can you get UI input on this (Boriss is probably the likely candidate)

::: toolkit/mozapps/extensions/XPIProvider.jsm
@@ +3394,5 @@
>     *         The name of the bootstrap method to call
>     * @param  aReason
>     *         The reason flag to pass to the bootstrap's startup method
>     */
> +  callBootstrapMethod: function XPI_callBootstrapMethod(aId, aVersion, aFile, aType,

Should have said this before but could you put the aType parameter before the aFile one.

@@ +7694,5 @@
>                                      STRING_TYPE_NAME,
>                                      AddonManager.VIEW_TYPE_LIST, 4000),
> +  new AddonManagerPrivate.AddonType("dictionary", URI_EXTENSION_STRINGS,
> +                                    STRING_TYPE_NAME,
> +                                    AddonManager.VIEW_TYPE_LIST, 2000,

This will cause locales and dictionaries to potentially display in a different order in different locales. Ask Boriss what order we should use and update accordingly.

::: toolkit/themes/gnomestripe/mozapps/extensions/extensions.css
@@ +266,5 @@
>    list-style-image: url("chrome://mozapps/skin/extensions/category-extensions.png");
>  }
> +#category-dictionary > .category-icon {
> +  list-style-image: url("chrome://mozapps/skin/extensions/category-dictionaries.png");
> +}

You also need stuff in the theme to set the default icon for the list view if the add-on doesn't provide an icon of its own. See http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/winstripe/mozapps/extensions/extensions.css#546

Talk to Boriss about sourcing proper icons.
Comment 29 :Ehsan Akhgari 2011-05-27 14:30:15 PDT
(In reply to comment #20)
> Created attachment 533719 [details] [diff] [review] [review]
> patch v3
> 
> Ehsan:
> I think I have done the changed to the test you said, but I still cannot get
> the test to run. I try to run it with 
> 
> python runtests.py --chrome
> --test-path=extensions/spellcheck/tests/chrome/test_add_remove_dictionaries.
> xul
> 
> It opens up a browser window where the list of tests to run is empty. And
> clicking the Run Chrome Tests button in the top does not give any reaction.
> I do not have any ideas about what I am doing wrong.

try:

ls -lR objdir/_tests/testing/mochitest/chrome/extensions/spellcheck

and see if the test file is really where you expected it to be.

Also, what happens if you run the test suite with --test-path=extensions/?
Comment 30 Jesper Kristensen 2011-05-27 14:47:30 PDT
(In reply to comment #29)
> > It opens up a browser window where the list of tests to run is empty. And
> > clicking the Run Chrome Tests button in the top does not give any reaction.
> > I do not have any ideas about what I am doing wrong.
> 
> try:
> 
> ls -lR objdir/_tests/testing/mochitest/chrome/extensions/spellcheck
> 
> and see if the test file is really where you expected it to be.
> 
> Also, what happens if you run the test suite with --test-path=extensions/?

I already said I found out why, and the test runs in my latest patch. https://bug368531.bugzilla.mozilla.org/attachment.cgi?id=253183 which is linked from developer.mozilla.org contains "ifdef MOZ_MOCHITEST" which seems to no longer exist.
Comment 31 :Ehsan Akhgari 2011-05-27 15:22:32 PDT
Comment on attachment 534239 [details] [diff] [review]
patch v5, editor part

Review of attachment 534239 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the delay here.  :-)

::: editor/composer/src/nsEditorSpellCheck.cpp
@@ -457,5 @@
> -#ifdef DEBUG
> -  nsresult rv =
> -#endif
> -  SaveDefaultDictionary();
> -  NS_WARN_IF_FALSE(NS_SUCCEEDED(rv), "failed to set default dictionary");

Hmm, on second thought, do we want to remove SaveDefaultDictionary from here?

::: editor/idl/nsIEditorSpellCheck.idl
@@ -163,5 @@
> -   * current selected language as the default for future use.
> -   *
> -   * If you have called CanSpellCheck but not InitSpellChecker, you can still
> -   * call this function to clear the cached spell check object, and no
> -   * preference saving will happen.

Is this comment not valid any more?

::: editor/libeditor/base/nsEditor.cpp
@@ +320,5 @@
> +    nsCOMPtr<nsIObserverService> obs = do_GetService("@mozilla.org/observer-service;1");
> +    if (obs)
> +      obs->AddObserver(this,
> +                       SPELLCHECK_DICTIONARY_UPDATE_NOTIFICATION,
> +                       PR_TRUE);

Here's an idea: how about we listen for the observer topic only if we don't have a dictionary?  Because, that is the only case where we would need to actually do something, right?

Also, why do you need to pass true as the third parameter?

@@ +1313,5 @@
> +  PRBool canSpell = mozInlineSpellChecker::CanEnableInlineSpellChecking();
> +  if (!canSpell) {
> +    *aInlineSpellChecker = nsnull;
> +    return NS_ERROR_FAILURE;
> +  }

Why does this patch need to do this?

::: extensions/spellcheck/hunspell/src/mozHunspell.cpp
@@ +333,5 @@
> +static PLDHashOperator
> +FindFirstString(const nsAString& aString, nsIFile* aFile, void* aClosure)
> +{
> +  FindFirstStruct *ans = (FindFirstStruct*) aClosure;
> +  ans->dic = ToNewUnicode(aString);

Where does this string get freed?

::: extensions/spellcheck/hunspell/src/mozHunspell.h
@@ +111,5 @@
>    // Hashtable matches dictionary name to .aff file
>    nsInterfaceHashtable<nsStringHashKey, nsIFile> mDictionaries;
>    nsString  mDictionary;
>    nsString  mLanguage;
> +  nsCOMPtr<nsIFile> mAffFile;

Can't we store the affix file name instead?

@@ +114,5 @@
>    nsString  mLanguage;
> +  nsCOMPtr<nsIFile> mAffFile;
> +
> +  // dynamic dirs used to search for dictionaries (key == value)
> +  nsInterfaceHashtable<nsISupportsHashKey, nsIFile> mDirs;

I don't think we need to use a hashtable here.  I think a simple array would do just fine.

@@ +119,3 @@
>  
>    Hunspell  *mHunspell;
> +  nsCOMPtr<nsIObserverService> mObs;

Why do we need to hold on to the observer service?  We can just retrieve it when we need to.

::: extensions/spellcheck/idl/mozIHunspellEngine.idl
@@ +48,5 @@
> +
> +
> + */
> +
> +interface mozIHunspellEngine : nsISupports {

No need for a new interface, please add AddDirectory and RemoveDirectory to mozISpellCheckingEngine.

::: extensions/spellcheck/src/mozInlineSpellChecker.cpp
@@ +632,5 @@
> +void // static
> +mozInlineSpellChecker::UpdateCanEnableInlineSpellChecking()
> +{
> +  gCanEnableSpellChecking = SpellCheck_Uninitialized;
> +}

I really don't like this, but I still can't think of a better way to do this...

::: extensions/spellcheck/src/mozSpellChecker.cpp
@@ +322,3 @@
>  
> +  // For catching duplicates
> +  nsClassHashtable<nsStringHashKey, nsCString> mDictionariesMap;

Please don't reuse the mDictionariesMap name.  It's confusing.

::: extensions/spellcheck/tests/chrome/Makefile.in
@@ +42,5 @@
> +relativesrcdir  = extensions/spellcheck/tests/chrome
> +
> +include $(DEPTH)/config/autoconf.mk
> +
> +DIRS = base map

Can you please put these files in the same directory?

::: extensions/spellcheck/tests/chrome/test_add_remove_dictionaries.xul
@@ +1,1 @@
> +<?xml version="1.0"?>

I haven't reviewed the test yet, since it seemed to me that you still haven't managed to get it to work.

::: toolkit/content/InlineSpellChecker.jsm
@@ +255,5 @@
>        return;
>      var spellchecker = this.mInlineSpellChecker.spellChecker;
>      spellchecker.SetCurrentDictionary(this.mDictionaryNames[index]);
> +    Services.prefs.setCharPref("spellchecker.dictionary",
> +                               this.mDictionaryNames[index]);

Does this really work?  The old code used to call SetComplexPref for this...
Comment 32 Jesper Kristensen 2011-05-27 16:25:21 PDT
(In reply to comment #31)
> Comment on attachment 534239 [details] [diff] [review] [review]
> patch v5, editor part
> 
> Review of attachment 534239 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> Sorry for the delay here.  :-)
> 
> ::: editor/composer/src/nsEditorSpellCheck.cpp
> @@ -457,5 @@
> > -#ifdef DEBUG
> > -  nsresult rv =
> > -#endif
> > -  SaveDefaultDictionary();
> > -  NS_WARN_IF_FALSE(NS_SUCCEEDED(rv), "failed to set default dictionary");
> 
> Hmm, on second thought, do we want to remove SaveDefaultDictionary from here?

I must admit that I haven't really tested it. I removed it since it seems bogus, and since it might give trouble with restartless addons, since they are unloaded earlier than normal addons. But I have not tested if it does in fact give trouble. The restartless dictionary addons will be unloaded during Firefox shutdown. If the addons gets unloaded before the editor, then any editor, which is open when Firefox is being closed, will change the preference to a restart-required or built in dictionary. If the editor is unloaded before the addon, then the SaveDefaultDictionary would not be a problem. It would however still be bogus, as we have already saved the dictionary preference when we changed the dictionary. I will experiment to see if it is really needed to remove it.

> ::: editor/idl/nsIEditorSpellCheck.idl
> @@ -163,5 @@
> > -   * current selected language as the default for future use.
> > -   *
> > -   * If you have called CanSpellCheck but not InitSpellChecker, you can still
> > -   * call this function to clear the cached spell check object, and no
> > -   * preference saving will happen.
> 
> Is this comment not valid any more?

If UninitSpellChecker does not touch the dictionary preference anymore, then the comment talking about that preference being saved at UninitSpellChecker would not be valid anymore.

> ::: editor/libeditor/base/nsEditor.cpp
> @@ +320,5 @@
> > +    nsCOMPtr<nsIObserverService> obs = do_GetService("@mozilla.org/observer-service;1");
> > +    if (obs)
> > +      obs->AddObserver(this,
> > +                       SPELLCHECK_DICTIONARY_UPDATE_NOTIFICATION,
> > +                       PR_TRUE);
> 
> Here's an idea: how about we listen for the observer topic only if we don't
> have a dictionary?  Because, that is the only case where we would need to
> actually do something, right?

Well, I was trying to sneak in a fix for another bug here. The editor very often gets confused about what language it is using. For example, some text in an editor is spell checked according to dictionary A, but when you type new text into the editor, it is spell checked according to dictionary B. Now you want the existing contents of the editor to be spell checked according to dictionary B, but you cannot do that, since when you open the context menu, dictionary B is already selected. I find that quite annoying, but if it is important that this behavior stays, then I can add this condition to the observer.

> 
> @@ +1313,5 @@
> > +  PRBool canSpell = mozInlineSpellChecker::CanEnableInlineSpellChecking();
> > +  if (!canSpell) {
> > +    *aInlineSpellChecker = nsnull;
> > +    return NS_ERROR_FAILURE;
> > +  }
> 
> Why does this patch need to do this?

Before mInlineSpellChecker would always be null whenever there were no dictionaries. Now, if there were dictionaries, but the last one got uninstalled mInlineSpellChecker would not become null, and callers expect null when there are no dictionaries installed. Instead I could make mInlineSpellChecker null when the last dictionary is removed, but I didn't understand the code around unloading mInlineSpellChecker (namely the comment in nsEditor::PreDestroy), so I didn't want to touch it.

> ::: extensions/spellcheck/hunspell/src/mozHunspell.h
> @@ +111,5 @@
> >    // Hashtable matches dictionary name to .aff file
> >    nsInterfaceHashtable<nsStringHashKey, nsIFile> mDictionaries;
> >    nsString  mDictionary;
> >    nsString  mLanguage;
> > +  nsCOMPtr<nsIFile> mAffFile;
> 
> Can't we store the affix file name instead?

mDictionary already holds the name of the file. I want to know the entire path, since when a dictionary is added or removed, one dictionary might override another, and in that case we need to detect that the dictionary has changed even if the language has not, or we might continue using a dictionary which is no longer installed.

Or do you mean that we should store the entire path as a string instead of as a nsIFile?

> ::: extensions/spellcheck/tests/chrome/Makefile.in
> @@ +42,5 @@
> > +relativesrcdir  = extensions/spellcheck/tests/chrome
> > +
> > +include $(DEPTH)/config/autoconf.mk
> > +
> > +DIRS = base map
> 
> Can you please put these files in the same directory?

The code can only install all dictionaries in a directory, so to be able to install and uninstall the two independently of each other in the test, they must be in different dictionaries. The alternative would be to move scanning directories for dictionaries from the spell checker into the addons manager, but I do not think it belongs there.

> ::: toolkit/content/InlineSpellChecker.jsm
> @@ +255,5 @@
> >        return;
> >      var spellchecker = this.mInlineSpellChecker.spellChecker;
> >      spellchecker.SetCurrentDictionary(this.mDictionaryNames[index]);
> > +    Services.prefs.setCharPref("spellchecker.dictionary",
> > +                               this.mDictionaryNames[index]);
> 
> Does this really work?  The old code used to call SetComplexPref for this...

I just tested, and it does remember the preference.
Comment 33 Dave Townsend [:mossop] 2011-05-27 16:46:34 PDT
(In reply to comment #32)
> > ::: toolkit/content/InlineSpellChecker.jsm
> > @@ +255,5 @@
> > >        return;
> > >      var spellchecker = this.mInlineSpellChecker.spellChecker;
> > >      spellchecker.SetCurrentDictionary(this.mDictionaryNames[index]);
> > > +    Services.prefs.setCharPref("spellchecker.dictionary",
> > > +                               this.mDictionaryNames[index]);
> > 
> > Does this really work?  The old code used to call SetComplexPref for this...
> 
> I just tested, and it does remember the preference.

Can these names ever be anything other than ascii? I think it'd fail in that case.
Comment 34 Jesper Kristensen 2011-05-28 02:54:44 PDT
(In reply to comment #33)
> Can these names ever be anything other than ascii? I think it'd fail in that
> case.

In that case it fails. I will change it to setComplexPref.

Unrelated bug: non ascii names cannot occur, since the add-ons manager will fail to install the add-on in that case with the error message "This add-on could not be installed because it appears to be corrupt." The error console contains:

Warning: WARN addons.xpi: Invalid XPI: [Exception... "Component returned failure code: 0x80520006 (NS_ERROR_FILE_TARGET_DOES_NOT_EXIST) [nsIZipReader.getEntry]"  nsresult: "0x80520006 (NS_ERROR_FILE_TARGET_DOES_NOT_EXIST)"  location: "JS frame :: resource://gre/modules/XPIProvider.jsm :: loadManifestFromZipReader :: line 824"  data: no]
Source File: resource://gre/modules/XPIProvider.jsm
Line: 824

Where 824 is 
addon.size += aZipReader.getEntry(entries.getNext()).realSize;
Comment 35 Dave Townsend [:mossop] 2011-05-28 09:35:53 PDT
Oh yeah, I don't think our zip reading code supports non-ascii characters well right now (the original zip spec didn't include them).
Comment 36 Jesper Kristensen 2011-05-29 08:16:07 PDT
Created attachment 535938 [details] [diff] [review]
patch v6, editor part

(In reply to comment #31)
> ::: editor/composer/src/nsEditorSpellCheck.cpp
> @@ -457,5 @@
> > -#ifdef DEBUG
> > -  nsresult rv =
> > -#endif
> > -  SaveDefaultDictionary();
> > -  NS_WARN_IF_FALSE(NS_SUCCEEDED(rv), "failed to set default dictionary");
> 
> Hmm, on second thought, do we want to remove SaveDefaultDictionary from here?

I tested it, and it does not hurt having the code here, so I have put it back.

> ::: extensions/spellcheck/hunspell/src/mozHunspell.h
> @@ +111,5 @@
> >    // Hashtable matches dictionary name to .aff file
> >    nsInterfaceHashtable<nsStringHashKey, nsIFile> mDictionaries;
> >    nsString  mDictionary;
> >    nsString  mLanguage;
> > +  nsCOMPtr<nsIFile> mAffFile;
> 
> Can't we store the affix file name instead?

I have changed it to store the path of the file.
Comment 37 Jesper Kristensen 2011-05-29 08:16:54 PDT
Created attachment 535939 [details] [diff] [review]
patch v6, addons manager part
Comment 38 :Ehsan Akhgari 2011-06-09 17:24:34 PDT
(In reply to comment #32)
> (In reply to comment #31)
> > Comment on attachment 534239 [details] [diff] [review] [review] [review]
> > patch v5, editor part
> > 
> > Review of attachment 534239 [details] [diff] [review] [review] [review]:
> > -----------------------------------------------------------------
> > 
> > Sorry for the delay here.  :-)
> > 
> > ::: editor/composer/src/nsEditorSpellCheck.cpp
> > @@ -457,5 @@
> > > -#ifdef DEBUG
> > > -  nsresult rv =
> > > -#endif
> > > -  SaveDefaultDictionary();
> > > -  NS_WARN_IF_FALSE(NS_SUCCEEDED(rv), "failed to set default dictionary");
> > 
> > Hmm, on second thought, do we want to remove SaveDefaultDictionary from here?
> 
> I must admit that I haven't really tested it. I removed it since it seems
> bogus, and since it might give trouble with restartless addons, since they
> are unloaded earlier than normal addons. But I have not tested if it does in
> fact give trouble. The restartless dictionary addons will be unloaded during
> Firefox shutdown. If the addons gets unloaded before the editor, then any
> editor, which is open when Firefox is being closed, will change the
> preference to a restart-required or built in dictionary. If the editor is
> unloaded before the addon, then the SaveDefaultDictionary would not be a
> problem. It would however still be bogus, as we have already saved the
> dictionary preference when we changed the dictionary. I will experiment to
> see if it is really needed to remove it.

Hmm, does comment 36 mean that you think you were mistaken here?

> > ::: editor/idl/nsIEditorSpellCheck.idl
> > @@ -163,5 @@
> > > -   * current selected language as the default for future use.
> > > -   *
> > > -   * If you have called CanSpellCheck but not InitSpellChecker, you can still
> > > -   * call this function to clear the cached spell check object, and no
> > > -   * preference saving will happen.
> > 
> > Is this comment not valid any more?
> 
> If UninitSpellChecker does not touch the dictionary preference anymore, then
> the comment talking about that preference being saved at UninitSpellChecker
> would not be valid anymore.

Given the above, this needs to be added back, right?

> > ::: editor/libeditor/base/nsEditor.cpp
> > @@ +320,5 @@
> > > +    nsCOMPtr<nsIObserverService> obs = do_GetService("@mozilla.org/observer-service;1");
> > > +    if (obs)
> > > +      obs->AddObserver(this,
> > > +                       SPELLCHECK_DICTIONARY_UPDATE_NOTIFICATION,
> > > +                       PR_TRUE);
> > 
> > Here's an idea: how about we listen for the observer topic only if we don't
> > have a dictionary?  Because, that is the only case where we would need to
> > actually do something, right?
> 
> Well, I was trying to sneak in a fix for another bug here. The editor very
> often gets confused about what language it is using. For example, some text
> in an editor is spell checked according to dictionary A, but when you type
> new text into the editor, it is spell checked according to dictionary B. Now
> you want the existing contents of the editor to be spell checked according
> to dictionary B, but you cannot do that, since when you open the context
> menu, dictionary B is already selected. I find that quite annoying, but if
> it is important that this behavior stays, then I can add this condition to
> the observer.

I think the fix to that bug should be more extensive than this, and I'd really like to keep this bug as small scoped as possible, so let's not worry about that other bug here.

> > @@ +1313,5 @@
> > > +  PRBool canSpell = mozInlineSpellChecker::CanEnableInlineSpellChecking();
> > > +  if (!canSpell) {
> > > +    *aInlineSpellChecker = nsnull;
> > > +    return NS_ERROR_FAILURE;
> > > +  }
> > 
> > Why does this patch need to do this?
> 
> Before mInlineSpellChecker would always be null whenever there were no
> dictionaries. Now, if there were dictionaries, but the last one got
> uninstalled mInlineSpellChecker would not become null, and callers expect
> null when there are no dictionaries installed. Instead I could make
> mInlineSpellChecker null when the last dictionary is removed, but I didn't
> understand the code around unloading mInlineSpellChecker (namely the comment
> in nsEditor::PreDestroy), so I didn't want to touch it.

Ah, yes, you're right!

> > ::: extensions/spellcheck/hunspell/src/mozHunspell.h
> > @@ +111,5 @@
> > >    // Hashtable matches dictionary name to .aff file
> > >    nsInterfaceHashtable<nsStringHashKey, nsIFile> mDictionaries;
> > >    nsString  mDictionary;
> > >    nsString  mLanguage;
> > > +  nsCOMPtr<nsIFile> mAffFile;
> > 
> > Can't we store the affix file name instead?
> 
> mDictionary already holds the name of the file. I want to know the entire
> path, since when a dictionary is added or removed, one dictionary might
> override another, and in that case we need to detect that the dictionary has
> changed even if the language has not, or we might continue using a
> dictionary which is no longer installed.
> 
> Or do you mean that we should store the entire path as a string instead of
> as a nsIFile?

Yes, that's what I meant.

> > ::: extensions/spellcheck/tests/chrome/Makefile.in
> > @@ +42,5 @@
> > > +relativesrcdir  = extensions/spellcheck/tests/chrome
> > > +
> > > +include $(DEPTH)/config/autoconf.mk
> > > +
> > > +DIRS = base map
> > 
> > Can you please put these files in the same directory?
> 
> The code can only install all dictionaries in a directory, so to be able to
> install and uninstall the two independently of each other in the test, they
> must be in different dictionaries. The alternative would be to move scanning
> directories for dictionaries from the spell checker into the addons manager,
> but I do not think it belongs there.

OK, no big deal, let's keep the dirs then.
Comment 39 :Ehsan Akhgari 2011-06-09 17:25:54 PDT
(In reply to comment #34)
> (In reply to comment #33)
> > Can these names ever be anything other than ascii? I think it'd fail in that
> > case.
> 
> In that case it fails. I will change it to setComplexPref.

Thanks!

> Unrelated bug: non ascii names cannot occur, since the add-ons manager will
> fail to install the add-on in that case with the error message "This add-on
> could not be installed because it appears to be corrupt." The error console
> contains:
> 
> Warning: WARN addons.xpi: Invalid XPI: [Exception... "Component returned
> failure code: 0x80520006 (NS_ERROR_FILE_TARGET_DOES_NOT_EXIST)
> [nsIZipReader.getEntry]"  nsresult: "0x80520006
> (NS_ERROR_FILE_TARGET_DOES_NOT_EXIST)"  location: "JS frame ::
> resource://gre/modules/XPIProvider.jsm :: loadManifestFromZipReader :: line
> 824"  data: no]
> Source File: resource://gre/modules/XPIProvider.jsm
> Line: 824
> 
> Where 824 is 
> addon.size += aZipReader.getEntry(entries.getNext()).realSize;

Hmm, can you please file this?  ZIP software packages are definitely able to handle non-ascii file names, so I think we should be too.
Comment 40 :Ehsan Akhgari 2011-06-09 17:26:33 PDT
Oh, and the reason that I CCed smaug is that I think he knows some stuff about the spell checker code, so I'd like to ask him to take a look at the patch too.
Comment 41 Dave Townsend [:mossop] 2011-06-09 21:34:01 PDT
(In reply to comment #39)
> > Unrelated bug: non ascii names cannot occur, since the add-ons manager will
> > fail to install the add-on in that case with the error message "This add-on
> > could not be installed because it appears to be corrupt." The error console
> > contains:
> > 
> > Warning: WARN addons.xpi: Invalid XPI: [Exception... "Component returned
> > failure code: 0x80520006 (NS_ERROR_FILE_TARGET_DOES_NOT_EXIST)
> > [nsIZipReader.getEntry]"  nsresult: "0x80520006
> > (NS_ERROR_FILE_TARGET_DOES_NOT_EXIST)"  location: "JS frame ::
> > resource://gre/modules/XPIProvider.jsm :: loadManifestFromZipReader :: line
> > 824"  data: no]
> > Source File: resource://gre/modules/XPIProvider.jsm
> > Line: 824
> > 
> > Where 824 is 
> > addon.size += aZipReader.getEntry(entries.getNext()).realSize;
> 
> Hmm, can you please file this?  ZIP software packages are definitely able to
> handle non-ascii file names, so I think we should be too.

We already have this files, see bug 296795 comment 11 in particular.
Comment 42 Jesper Kristensen 2011-06-10 04:55:37 PDT
(In reply to comment #38)
> > > ::: editor/composer/src/nsEditorSpellCheck.cpp
> > > @@ -457,5 @@
> > > > -#ifdef DEBUG
> > > > -  nsresult rv =
> > > > -#endif
> > > > -  SaveDefaultDictionary();
> > > > -  NS_WARN_IF_FALSE(NS_SUCCEEDED(rv), "failed to set default dictionary");
> Hmm, does comment 36 mean that you think you were mistaken here?

Correct. I believe that the code does not make sense to have here, but on the other hand it does not hurt, so I will leave it untouched.

> > > ::: editor/libeditor/base/nsEditor.cpp
> > > @@ +320,5 @@
> > > > +    nsCOMPtr<nsIObserverService> obs = do_GetService("@mozilla.org/observer-service;1");
> > > > +    if (obs)
> > > > +      obs->AddObserver(this,
> > > > +                       SPELLCHECK_DICTIONARY_UPDATE_NOTIFICATION,
> > > > +                       PR_TRUE);
> > > 
> > > Here's an idea: how about we listen for the observer topic only if we don't
> > > have a dictionary?  Because, that is the only case where we would need to
> > > actually do something, right?

I have a correction to this: We need it in two places:
1. When the first dictionary is installed, in order to mimic the same behavior that restart-required dictionary add-ons have.
2. When the current dictionary is uninstalled, in order to avoid using a dictionary, which does not exist.

The only case where we will not need it is when the current dictionary has changed, but no dictionary has been installed or uninstalled. I could update the spell checker to only send the notification when the current dictionary changed because of an addition or removal of a dictionary. Should I do that? I think it would just complicate things with no win.

I just noticed that my comments in mozISpellCheckingEngine.idl says the notification will be sent in some situations where it actually will not (getDictionaryList). I will fix that.
Comment 43 :Ehsan Akhgari 2011-06-10 14:29:01 PDT
(In reply to comment #42)
> The only case where we will not need it is when the current dictionary has
> changed, but no dictionary has been installed or uninstalled. I could update
> the spell checker to only send the notification when the current dictionary
> changed because of an addition or removal of a dictionary. Should I do that? I
> think it would just complicate things with no win.

Hmm, ok, let's keep it then.  :/
Comment 44 Jesper Kristensen 2011-06-11 03:30:18 PDT
Created attachment 538676 [details] [diff] [review]
patch v7, editor part
Comment 45 Jesper Kristensen 2011-06-11 03:31:04 PDT
Created attachment 538677 [details] [diff] [review]
patch v7, addons manager part
Comment 46 :Ehsan Akhgari 2011-06-13 10:41:46 PDT
Comment on attachment 538676 [details] [diff] [review]
patch v7, editor part

Review of attachment 538676 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the comments addressed.  Please also ask smaug for review once you're done making these changes.  Thanks a lot for working on this.  :-)

::: editor/libeditor/base/nsEditor.cpp
@@ +315,5 @@
>      // update the UI with our state
>      NotifyDocumentListeners(eDocumentCreated);
>      NotifyDocumentListeners(eDocumentStateChanged);
> +
> +    nsCOMPtr<nsIObserverService> obs = do_GetService("@mozilla.org/observer-service;1");

Nit: please use mozilla::services::GetObserverService.

@@ +440,5 @@
>  {
>    if (mDidPreDestroy)
>      return NS_OK;
>  
> +  nsCOMPtr<nsIObserverService> obs = do_GetService("@mozilla.org/observer-service;1");

Nit: please use mozilla::services::GetObserverService.

::: extensions/spellcheck/hunspell/src/mozHunspell.cpp
@@ +194,5 @@
>      mLanguage = Substring(mDictionary, 0, pos);
>  
> +
> +  nsCOMPtr<nsIObserverService> obs =
> +    do_GetService("@mozilla.org/observer-service;1");

Ditto.

@@ +313,5 @@
>  }
>  
> +struct FindFirstStruct
> +{
> +  PRUnichar *dic;

You can just use the string pointer directly, right?  No need to wrap it in a struct.

@@ +381,5 @@
>      if (dictDir)
>        LoadDictionariesFromDir(dictDir);
>    }
> +
> +  // find dictionaries from no-restart extensions

Nit: restartless.

@@ +420,5 @@
> +    mDecoder = nsnull;
> +    mEncoder = nsnull;
> +
> +    nsCOMPtr<nsIObserverService> obs =
> +      do_GetService("@mozilla.org/observer-service;1");

Ditto.

::: extensions/spellcheck/hunspell/src/mozHunspell.h
@@ +108,5 @@
>    // Hashtable matches dictionary name to .aff file
>    nsInterfaceHashtable<nsStringHashKey, nsIFile> mDictionaries;
>    nsString  mDictionary;
>    nsString  mLanguage;
> +  nsCString mAffFileName;

Nit: mAffixFileName.

@@ +111,5 @@
>    nsString  mLanguage;
> +  nsCString mAffFileName;
> +
> +  // dynamic dirs used to search for dictionaries
> +  nsTArray<nsIFile*> mDirs;

You should use an nsCOMArray.  Also, you should probably call it something more descriptive, such as mDynamicDirectories.

::: extensions/spellcheck/src/mozSpellChecker.cpp
@@ +326,2 @@
>  
> +  nsTArray<nsCOMPtr<mozISpellCheckingEngine>> spellCheckingEngines;

Use nsCOMArray please.

@@ +327,5 @@
> +  nsTArray<nsCOMPtr<mozISpellCheckingEngine>> spellCheckingEngines;
> +  rv = GetEngineList(&spellCheckingEngines);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  for (PRUint32 i=0;i < spellCheckingEngines.Length();i++){

Nit: space after semi-colon.

@@ +333,5 @@
> +
> +    PRUint32 count,k;
> +    PRUnichar **words;
> +    engine->GetDictionaryList(&words,&count);
> +    for(k=0;k<count;k++){

Nit: space after semi-colon.

@@ +380,2 @@
>    nsresult rv;
> +  nsTArray<nsCOMPtr<mozISpellCheckingEngine>> spellCheckingEngines;

Ditto.

@@ +402,5 @@
>  
> +NS_IMETHODIMP 
> +mozSpellChecker::CheckCurrentDictionary()
> +{
> +  if (mSpellCheckingEngine) {

Please add a comment describing what this block does.

@@ +426,5 @@
> +    nsXPIDLString dictname;
> +
> +    engine->GetDictionary(getter_Copies(dictname));
> +
> +    if (!dictname.IsEmpty()) {

I think we need a comment for this block too.

::: extensions/spellcheck/tests/chrome/test_add_remove_dictionaries.xul
@@ +71,5 @@
> +
> +  // select base dictionary
> +  setCurrentDictionary(editor, "base_utf");
> +
> +  SimpleTest.executeSoon(function() {

Nit: please fix indentation in this block and its nesting children.
Comment 47 Jesper Kristensen 2011-06-14 01:41:48 PDT
Created attachment 539155 [details] [diff] [review]
patch v8, editor part
Comment 48 Olli Pettay [:smaug] 2011-06-27 08:15:02 PDT
Comment on attachment 539155 [details] [diff] [review]
patch v8, editor part

> interface nsIEditor;
> interface nsITextServicesFilter;
> 
>-[scriptable, uuid(90c93610-c116-44ab-9793-62dccb9f43ce)]
>+[scriptable, uuid(334946c3-0e93-4aac-b662-e1b56f95d68b)]
> interface nsIEditorSpellCheck : nsISupports
> {
> 
>+  /**
>+   * Get the spell checker used by this editor.
>+   */
>+  [noscript] readonly attribute nsISpellChecker spellChecker;
>+
Could this be even notxpcom, so that the method wouldn't return
nsresult but nsISpellchecker* ?


> NS_IMETHODIMP
>@@ -304,16 +311,22 @@ nsEditor::PostCreate()
> 
>     // nuke the modification count, so the doc appears unmodified
>     // do this before we notify listeners
>     ResetModificationCount();
> 
>     // update the UI with our state
>     NotifyDocumentListeners(eDocumentCreated);
>     NotifyDocumentListeners(eDocumentStateChanged);
>+
>+    nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService();
>+    if (obs)
>+      obs->AddObserver(this,
>+                       SPELLCHECK_DICTIONARY_UPDATE_NOTIFICATION,
>+                       PR_FALSE);

Nit, 
if (expr) {
  stmt;
}

> 
>+  nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService();
>+  if (obs)
>+    obs->RemoveObserver(this,
>+                        SPELLCHECK_DICTIONARY_UPDATE_NOTIFICATION);
ditto



>+NS_IMETHODIMP nsEditor::Observe(nsISupports* aSubj, const char *aTopic,
>+                         const PRUnichar *aData)
Align parameters


>+  nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService();
>+  if (obs)
>+    obs->NotifyObservers((mozISpellCheckingEngine*) this,
Please use C++ casting, not C casting.

>+                         SPELLCHECK_DICTIONARY_UPDATE_NOTIFICATION,
>+                         nsnull);
And here


>+static PLDHashOperator
>+FindFirstString(const nsAString& aString, nsIFile* aFile, void* aClosure)
>+{
>+  PRUnichar **dic = (PRUnichar**) aClosure;
>+  *dic = ToNewUnicode(aString);
>+  return PL_DHASH_STOP;
>+}
>+

>+  // If we didn't find a dictionary equal to the current dictionary or we had
>+  // no current dictionary, just pick an arbitrary dictionary.
>+  PRUnichar *dic = nsnull;
>+  mDictionaries.EnumerateRead(FindFirstString, &dic);
This is strange.
First, why PRUnichar* and not nsString?
Then, could you explain "just pick an arbitrary dictionary"? Why do we want
such behavior?

r=me, if I get some explanation why "pick an arbitrary dictionary" is ok.
Comment 49 Jesper Kristensen 2011-06-27 12:43:34 PDT
(In reply to comment #48)
> Then, could you explain "just pick an arbitrary dictionary"? Why do we want
> such behavior?

Because we want to keep the existing behavior. nsEditorSpellCheck.cpp did the same thing when an editor was initialized, but since now the set of dictionaries can change after editors have been initialized, we need to do the same when the set of dictionaries changes.

Generally the behavior of choosing a dictionary could use some work, but that is another bug.
Comment 50 Jesper Kristensen 2011-06-28 13:56:18 PDT
Created attachment 542584 [details] [diff] [review]
patch v9, editor part

(In reply to comment #48)
> > interface nsIEditorSpellCheck : nsISupports
> > {
> > 
> >+  /**
> >+   * Get the spell checker used by this editor.
> >+   */
> >+  [noscript] readonly attribute nsISpellChecker spellChecker;
> >+
> Could this be even notxpcom, so that the method wouldn't return
> nsresult but nsISpellchecker* ?

I don't know what you mean. From what I can see from existing code, notxpcom attributes still return nsresult.

> >+  nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService();
> >+  if (obs)
> >+    obs->NotifyObservers((mozISpellCheckingEngine*) this,
> Please use C++ casting, not C casting.
> 
> >+                         SPELLCHECK_DICTIONARY_UPDATE_NOTIFICATION,
> >+                         nsnull);

I looked at some existing uses, and it looks like it is ok to just pass nsnull here, so I did that instead.
Comment 51 Olli Pettay [:smaug] 2011-07-22 01:37:58 PDT
Comment on attachment 542584 [details] [diff] [review]
patch v9, editor part


>+    PRUint32 count, k;
>+    PRUnichar **words;
>+    engine->GetDictionaryList(&words, &count);

Please initialize count to before using it 0.


>+  for (PRUint32 i=0;i < spellCheckingEngines.Count();i++){
space before and after '=' and after ; 



>+    // We still have a dictionary, so keep using that.
>+    if (!dictname.IsEmpty())
>+      return NS_OK;
if (expr) {
  stmt;
}



>+  for (PRUint32 i=0;i < spellCheckingEngines.Count();i++){
space before and after '=', and after ';'

>+DEPTH		= ../../..
>+topsrcdir	= @top_srcdir@
>+srcdir		= @srcdir@
Extra tab after srcdir ?

>+srcdir		= @srcdir@
ditto


>+srcdir		= @srcdir@
ditto
Comment 52 Jesper Kristensen 2011-07-24 02:22:53 PDT
When I look at the bugs with the uiwanted keyword, it looks like it could take years before someone would reach this bug. Would there be a way to make the changes to the addons manager without any UI changes, such that addons of types "extension" and "dictionary" are shown in the same category. It looks like the categories in the UI are tightly coupled with the AddonType object, and the AddonType object is tightly coupled with em:type, so I don't think the code showing them in the same category would be pretty.

Alternatively is it possible to land the changes to the editor and spell checker before the changes to the addons manager are finished?

(In reply to comment #51)
> >+DEPTH		= ../../..
> >+topsrcdir	= @top_srcdir@
> >+srcdir		= @srcdir@
> Extra tab after srcdir ?

Are you sure? I cannot find any tab width where adding an extra tab after srcdir would line up the values.
Comment 53 Olli Pettay [:smaug] 2011-07-24 05:01:59 PDT
I meant you have an extra tab after srcdir
Comment 54 Olli Pettay [:smaug] 2011-07-24 05:05:14 PDT
....but now I'm not sure about that anymore. Some tool did show wrong
alignment, but I can't see it anymore.
Comment 55 :Gijs Kruitbosch 2011-07-24 07:54:37 PDT
(In reply to comment #52)
> When I look at the bugs with the uiwanted keyword, it looks like it could
> take years before someone would reach this bug. <snip>

Have you tried pinging Boriss about this bug on IRC / mail? It seems to me that a naming decision for the category shouldn't take particularly long.
Comment 56 Jesper Kristensen 2011-07-24 09:18:41 PDT
(In reply to comment #55)
> (In reply to comment #52)
> > When I look at the bugs with the uiwanted keyword, it looks like it could
> > take years before someone would reach this bug. <snip>
> 
> Have you tried pinging Boriss about this bug on IRC / mail? It seems to me
> that a naming decision for the category shouldn't take particularly long.

I can try. It is not just naming. We also need an icon.
Comment 57 :Ehsan Akhgari 2011-07-25 16:29:14 PDT
I've CCed everybody that I know on the UX team.  :-)
Comment 58 Jesper Kristensen 2011-07-26 01:41:16 PDT
Over e-mail:

Den 26-07-2011 01:15, Boriss skrev:
> Let's just call the category /dictionaries/. I think that word, at least
> in English, implies spelling.
>
> As you probably know, it should only ever appear when someone has
> installed a different spelling check dictionary (much like languages
> only appears in the add-ons manager when the user has installed one now).
>
> If the user has installed a dictionary, the category should display last
> in the list of add-ons.  This is so that adding a new category does not
> change the current order of categories, which the user may have learned
> physically.

Note that the "Languages" category currently appears at the top, between "Get Add-ons" and "Extensions".

> Thanks, and let me know if you need anything for this!

Do you have some icons for this new category?
Comment 59 Stephen Horlander [:shorlander] 2011-08-05 11:05:06 PDT
(In reply to Jesper Kristensen from comment #58)
> Do you have some icons for this new category?

Working on icons for this. Unless Boriss has them already?
Comment 60 Jesper Kristensen 2011-08-07 08:51:49 PDT
(In reply to Stephen Horlander from comment #59)
> Working on icons for this. Unless Boriss has them already?

Thanks.

I haven't seen any. Boriss said that she would find someone to make the icons "tomorrow", but that was more than a week ago.
Comment 61 Stephen Horlander [:shorlander] 2011-08-08 13:55:36 PDT
Created attachment 551561 [details]
Dictionary Icon - Gnomestripe (Linux)
Comment 62 Stephen Horlander [:shorlander] 2011-08-08 13:56:38 PDT
Created attachment 551562 [details]
Dictionary Icon - Pinstripe (OS X)
Comment 63 Stephen Horlander [:shorlander] 2011-08-08 13:57:25 PDT
Created attachment 551563 [details]
Dictionary Icon - Winstripe (Windows)
Comment 64 Jesper Kristensen 2011-08-09 05:25:22 PDT
Created attachment 551728 [details] [diff] [review]
patch v10, editor part
Comment 65 Jesper Kristensen 2011-08-09 05:28:54 PDT
Created attachment 551733 [details] [diff] [review]
patch v10, addons manager part

I also changed the sort position of the Languages category per e-mail discussions with Boriss, since it is a one character change right next to the new category I added.
Comment 66 Dave Townsend [:mossop] 2011-08-11 14:03:52 PDT
I've asked shorlander to add 16px versions of the icons which we'll need once bug 596343 lands.
Comment 67 Dave Townsend [:mossop] 2011-08-15 13:17:04 PDT
Comment on attachment 551733 [details] [diff] [review]
patch v10, addons manager part

Review of attachment 551733 [details] [diff] [review]:
-----------------------------------------------------------------

Please add the icon to the UIs added by bug 476430 and bug 596343. The latter will require a 16px icon which shorlander should have in progress

::: toolkit/mozapps/extensions/XPIProvider.jsm
@@ +713,5 @@
>    else {
> +    // spell check dictionaries never require a restart
> +    if (addon.type == "dictionary") {
> +      addon.bootstrap = true;
> +    }

No braces needed around single statements
Comment 68 Stephen Horlander [:shorlander] 2011-08-16 12:46:27 PDT
Created attachment 553557 [details]
Dictionary Icon - Gnomestripe (Linux) - 16x16
Comment 69 Stephen Horlander [:shorlander] 2011-08-16 12:47:37 PDT
Created attachment 553558 [details]
Dictionary Icon - Pinstripe (OS X) - 16x16
Comment 70 Stephen Horlander [:shorlander] 2011-08-16 12:48:21 PDT
Created attachment 553560 [details]
Dictionary Icon - Winstripe (Windows) - 16x16
Comment 71 Stephen Horlander [:shorlander] 2011-08-16 12:49:04 PDT
(In reply to Dave Townsend (:Mossop) from comment #66)
> I've asked shorlander to add 16px versions of the icons which we'll need
> once bug 596343 lands.

Added!
Comment 72 Jesper Kristensen 2011-08-17 07:55:36 PDT
Status:
The editor part has bitrotted significantly due to bug 338427, which I am trying to resolve.
The test for the addons manager part has stopped working with an exception "invalid 'instanceof' operand DBAddonInternal" when calling "b1.uninstall();", and I don't know why yet.
Comment 73 Dave Townsend [:mossop] 2011-08-17 09:37:07 PDT
(In reply to Jesper Kristensen from comment #72)
> The test for the addons manager part has stopped working with an exception
> "invalid 'instanceof' operand DBAddonInternal" when calling
> "b1.uninstall();", and I don't know why yet.

This indicates a fault in the test, you shouldn't be continuing to use a reference to an add-on after you've called restartManager, instead you have to query for the add-on again. See line 554 of your test.

This didn't used to be a problem but bug 669541 made the simulated restart an even better simulation.
Comment 74 Jesper Kristensen 2011-08-17 09:48:25 PDT
Ah thanks. I was sure I had looked for changes in test_bootstrap.js and found none, but that was because I used the "hg log" link on MXR, which is currently out of date and does not show the changeset from bug 669541.
Comment 75 :Ehsan Akhgari 2011-08-17 10:19:03 PDT
(In reply to comment #72)
> Status:
> The editor part has bitrotted significantly due to bug 338427, which I am
> trying to resolve.

Once you have a patch which applies cleanly, can we land the editor part while we're waiting for the add-on manager part?
Comment 76 Jesper Kristensen 2011-08-17 12:29:58 PDT
Created attachment 553867 [details] [diff] [review]
patch v11, editor part

unbitrotted. Ehsan, do you want to take a look if it still looks OK after bug 338427?

(In reply to Ehsan Akhgari [:ehsan] from comment #75)
> Once you have a patch which applies cleanly, can we land the editor part
> while we're waiting for the add-on manager part?

I would very much like that, as I said in comment 52.
Comment 77 Jesper Kristensen 2011-08-17 12:32:54 PDT
Created attachment 553870 [details] [diff] [review]
patch v11, addons manager part

Do these additions for bug 476430 and bug 596343 look right? I haven't tested it in any way, as I am not sure what would be the easiest way to bring up these UIs.
Comment 78 :Ehsan Akhgari 2011-08-17 13:54:43 PDT
Comment on attachment 553867 [details] [diff] [review]
patch v11, editor part

If this patch on its own has passed the try server, we can land it!
Comment 79 Jesper Kristensen 2011-08-17 14:56:51 PDT
woo, I can push things to the try server. I didn't know I had access to that.
Comment 80 Jesper Kristensen 2011-08-18 04:48:14 PDT
(In reply to Ehsan Akhgari [:ehsan] from comment #78)
> Comment on attachment 553867 [details] [diff] [review]
> patch v11, editor part
> 
> If this patch on its own has passed the try server, we can land it!

Here are the results, but I don't know how to interpret them:

http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=958503f49ed4
Comment 81 Dave Townsend [:mossop] 2011-08-23 12:54:36 PDT
Comment on attachment 553870 [details] [diff] [review]
patch v11, addons manager part

Review of attachment 553870 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good but you've missed the icon for newaddon.css (I think we need a better way to do this but that's a separate bug). Sorry for the delay, with that extra change I'll try to get the last review done quickly.
Comment 82 Jesper Kristensen 2011-09-02 10:06:42 PDT
Just to make things clear: As per Ehsan's comment 78, I have posted the editor patch to try server. I don't know how to interpret those results, and I am waiting hoping that someone with better knowledge than me on this can tell me what is wrong, if anything.

Regarding the addons manager patch, I have decided not to touch it before the editor patch is sorted out, doing one thing at a time. The editor patch can land before the addons manager patch, and I would prefer to do that to keep the number of things, which can go wrong at once to a minimum.
Comment 83 :Ehsan Akhgari 2011-09-02 14:50:40 PDT
The try server results don't seem to be available any more.  :(  Sorry, I was a bit behind on the volume of bugmail, so I didn't see the email in time.  Can you please push this to try again?  Thanks!
Comment 84 Jesper Kristensen 2011-09-04 14:11:01 PDT
Here it is:
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=88ef9a2618e1

Looks a lot more green this time, but please take a look.
Comment 85 :Ehsan Akhgari 2011-09-05 16:03:48 PDT
The try server results look good!  I've landed the backend part of the patch on mozilla-inbound:

http://hg.mozilla.org/integration/mozilla-inbound/rev/a422b9ff0a9e
Comment 86 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-09-06 04:07:25 PDT
http://hg.mozilla.org/mozilla-central/rev/a422b9ff0a9e

Not sure if I should be closing this bug ...
Comment 87 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-09-06 05:30:24 PDT
This appears to have caused an 8% Ts regression.
Comment 88 Ed Morley [:emorley] 2011-09-06 06:44:55 PDT
Part 1 backed out of inbound for 8% Ts regression on multiple platforms:
http://hg.mozilla.org/integration/mozilla-inbound/rev/47e7f8b690a5

...and also backed out of mozilla-central since it had been merged:
http://hg.mozilla.org/mozilla-central/rev/db9e99d537f2

For more details on the regression, see:
https://groups.google.com/forum/#!topic/mozilla.dev.tree-management/h6kTVP8dgmo
Comment 89 :Ehsan Akhgari 2011-09-06 09:16:36 PDT
So, it's not immediately evident to me why this patch should regress startup time.  In order to investigate, I suggest you put a breakpoint in mozHunspell::LoadDictionaryList and see if it gets hit during the startup process.  The code added there is doing I/O and it might be the culprit...
Comment 90 Jesper Kristensen 2011-09-07 00:24:46 PDT
From the debugger I can see that my patch does load a dictionary at startup, which did not happen before. When the current dictionary is uninstalled, I made the spell checking engine responsible for updating the value of the current dictionary. This means that it is updated on startup even if no editor is using spell checking. I will try to transfer the responsibility of updating the current dictionary when a dictionary is removed back to each editor instance (from mozHunspell::LoadDictionaryList to mozSpellChecker::CheckCurrentDictionary), such that the update doesn't happen when there is no editors using spell checking. Ehsan, does that sound right?
Comment 91 :Ehsan Akhgari 2011-09-07 08:31:06 PDT
Yes, that makes a lot of sense to me.  Once you do that, in order to verify that you are not regressing Ts, you could either run Talos locally <https://wiki.mozilla.org/StandaloneTalos> or push to try, trigger multiple Talos runs, and use the talos-compare tool: <http://perf.snarkfest.net/compare-talos/>
Comment 92 Jesper Kristensen 2011-09-13 11:44:50 PDT
Created attachment 560003 [details] [diff] [review]
patch v12, editor part

Patch updated to avoid loading a dictionary at startup. Tested with https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=e12f1a7828c6
Comment 94 Matt Brubeck (:mbrubeck) 2011-09-14 07:02:22 PDT
https://hg.mozilla.org/mozilla-central/rev/e3cfe5ae818c
Comment 95 :Ehsan Akhgari 2011-09-14 14:52:15 PDT
Half of this still needs to land.

Jesper, looking at comment 81, seems like the add-on manager part is also very near, isn't it?  It would be awesome if we can get this in Firefox 9.  Let me know how I can help.  :-)
Comment 96 Jesper Kristensen 2011-09-15 13:45:40 PDT
I hope to upload an updated patch before the end of the weekend. Some things:
* I still haven't tried to invoke the new addons manager UI for accepting third party addons. Some additional instructions for doing that have been posted since last I tried, and I hope I can use those.
* Right now I cannot get the xpcshell test to run.
* I found a bug when both restart required dictionary addons and restartless dictionary addons are installed, but I beleive the fix will be simple.
Comment 97 :Ehsan Akhgari 2011-09-16 09:17:52 PDT
(In reply to Jesper Kristensen from comment #96)
> * Right now I cannot get the xpcshell test to run.

Do you mean that it does not run at all?  Maybe I can help with that.  Can you please post the latest version of your patch?
Comment 98 Jesper Kristensen 2011-09-16 10:48:41 PDT
Created attachment 560582 [details] [diff] [review]
patch v12, addons manager part

EDITOR:
The first file in the patch is a small update to mozHunspell.cpp, which I think Ehsan should review. The reason for adding it is in nsXREDirProvider::DoStartup() which contains the following lines of code in the given order:

em->Observe(nsnull, "addons-startup", nsnull);
  This will initialize restartless addons. If there are restartless dictionary extensions, they will initialize mozHunspell at this point. Otherwise mozHunspell will not be initialized until much later

LoadExtensionBundleDirectories();
  This will initialize the list of restart-requireing addons. If mozHunspell is initialized before this point, dictionaries from restart-requireing addons will not be available.

obsSvc->NotifyObservers(nsnull, "profile-after-change", kStartup);
  With the observer added, mozHunspell will re-initialize here, in case it was already initialized, to pick up restart-requireing dictionary addons.

I don't know how to write a test for this, as it would require restarting the application, and I am not sure if that is possible in a test. The addons manager tests only simulates a restart of the addons manager.

ADDONS MANAGER:
I haven't found a way to trigger the third party addons dialog, so I cannot see if my changes to it will look correct.
Comment 99 :Ehsan Akhgari 2011-09-16 12:16:29 PDT
Comment on attachment 560582 [details] [diff] [review]
patch v12, addons manager part

Review of attachment 560582 [details] [diff] [review]:
-----------------------------------------------------------------

r=me on the spellcheck change.  I'm not sure if there is an easy way to test this...
Comment 100 Jorge Villalobos [:jorgev] 2011-09-30 15:02:38 PDT
Is it safe to assume that this won't land on 9?
Comment 101 :Ehsan Akhgari 2011-09-30 15:23:33 PDT
(In reply to Jorge Villalobos [:jorgev] from comment #100)
> Is it safe to assume that this won't land on 9?

The front-end changes won't make it.  The core changes will be in 9, but it doesn't mean that restartless dictionary add-ons are going to work in 9.
Comment 102 Dave Townsend [:mossop] 2011-10-04 16:17:26 PDT
Comment on attachment 560582 [details] [diff] [review]
patch v12, addons manager part

Sorry, I claimed it would be quick then promptly went on vacation. This looks good apart from missing dictionaryGeneric-16.png out of the jar.mn files.
Comment 104 Ed Morley [:emorley] 2011-10-07 04:14:02 PDT
https://hg.mozilla.org/mozilla-central/rev/6f4259bdadf5
Comment 106 Jesper Kristensen 2011-12-11 03:55:12 PST
(In reply to Eric Shepherd [:sheppy] from comment #105)

I think [1] is still correct, though it is not very well explained. However [2] is incorrect, and the text in the top of [3], which links to [2] is misleading.

[2] has always been incorrect, since it does not take [1] into account. Since bug 338427, [2] is even worse. The code in bug 338427, and code derived from [2] may get into a fight over which dictionary to use, and in the best case result in the wrong dictionary being used (like bug 688860), and in the worst case in an infinite loop.

[2] should use the nsIEditorSpellCheck interface if there is an editor around or nsISpellChecker if there is no editor around. The problem is: Using nsISpellChecker when there is an editor will cause problems, and you cannot get a nsIEditorSpellCheck when there is no editor. Furthermore nsISpellChecker is not scriptable. Maybe [2] should be rewritten to assume that you have an editor and then use nsIEditorSpellCheck.

[1] https://developer.mozilla.org/En/Using_an_External_Spell-checker
[2] https://developer.mozilla.org/en/Using_spell_checking_in_XUL
[3] https://developer.mozilla.org/en/XPCOM_Interface_Reference/mozISpellCheckingEngine
Comment 107 neil@parkwaycc.co.uk 2012-03-20 09:37:59 PDT
Comment on attachment 560582 [details] [diff] [review]
patch v12, addons manager part

>+function shutdown() {
>+  hunspell.removeDirectory(dir);
>+}
Nit: needs to check reason to avoid useless uninstall on quit.
Comment 108 :Ehsan Akhgari 2012-03-22 16:50:50 PDT
(In reply to neil@parkwaycc.co.uk from comment #107)
> Comment on attachment 560582 [details] [diff] [review]
> patch v12, addons manager part
> 
> >+function shutdown() {
> >+  hunspell.removeDirectory(dir);
> >+}
> Nit: needs to check reason to avoid useless uninstall on quit.

Would you mind filing a new bug for this?
Comment 109 Paul [pwd] 2012-05-29 03:15:55 PDT
*** Bug 759088 has been marked as a duplicate of this bug. ***
Comment 110 Paul [pwd] 2012-05-29 04:05:58 PDT
I recently (this week had to reinstalled my en-GB dictionary and was asked to restart which prompted me to file bug 759088. This definitely isn't fixed.
Comment 111 Jesper Kristensen 2012-05-29 09:57:31 PDT
(In reply to Paul [sabret00the] from comment #110)
> I recently (this week had to reinstalled my en-GB dictionary and was asked
> to restart which prompted me to file bug 759088. This definitely isn't fixed.

It is fixed. You may ask the author of the en-GB dictionary addon to update his addon to take advantage of the feature introduced in this bug.
Comment 112 Blair McBride [:Unfocused] (UNAVAILABLE) 2012-11-08 03:31:31 PST
*** Bug 338074 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.