<a class="header-button" href="https://bugzilla-dev.allizom.org/home" title="Go to home page"> Bugzilla

Updated

•

14 years ago

OS: Windows XP → All

Version: 2.0 Branch → Trunk

Comment 34

•

14 years ago

(In reply to comment #7) > (In reply to comment #4) > > But it is my understanding that this nsEditorSpellCheck::InitSpellChecker() is > > called only once per browser instance? > > nsEditorSpellCheck is no service and you get one instance per input/textarea > element for which spell checking is possible. InitSpellChecker is thus called > once per element and would thus be the appropriate place for figuring out the > page's default language. > > You should be able to start searching from aEditor->GetRootElement. Make sure > to consider HTML's lang attribute and xml:lang (whichever is appropriate) and > as a bonus it would be nice to also select not completely specified languages > (e.g. select es-ES for lang="es"). > > (In reply to comment #2) > > but it'd be nice to have for those bilingual people out there. > > Of which there are an awful lot on the English dominated Internet of a polyglot > world. :) Unfortunately, even if it's not too hard to check the lang that has to be used during the call to nsEditorSpellCheck::InitSpellChecker() (for each editor's instance), the problem is quite hard because there is only one instance of nsISpellChecker (implemented by extensions/spellcheck/) which is used to set the dictionary. In other words, changing the dictionary for one editor will propagate the change for all the editors. That's exactly the behavior you have when you change the language with right click -> Languages -> <language>. And that's exactly what we don't want for @lang. Actually, we may don't want that when changing the language manually too. On top of my head, I would say the best way to fix this bug would be to set the desired dictionary when the editor got the focus so it could be possible to set the dictionary to the language corresponding to @lang and still be able to revert it when using a field with another @lang or with the default one.

Updated

•

14 years ago

Summary: inline form spellchecking should respect the page/field language → Spellchecker should respect @lang

Comment 35

•

14 years ago

Attached patch Patch v1 (obsolete) (deleted) — Details — Splinter Review

Ehsan, could you review the editor changes? Who would you recommend for spellchecker changes review? This patch updates the spellchecker's dictionary each time an editor is focused, taking in account @lang. For the moment, @lang has to correspond exactly to the name of one of your dictionary (like 'en-US' or 'fr-FR'). Checking for the first part of the lang code will be a follow-up. Unfortunately this patch comes with one regression: if the user change the dictionary for one field (using right click -> languages -> <dictionary>) this will be forgotten as soon as he/she focus another field. I think the current behavior is really annoying (this is saved as long as you don't 'revert' it). If that can be done in a follow-up, I would prefer to have this saved to be used by default for the entire document and not the entire session. By the way, my previous comment had some mistakes. There isn't one instance of nsISpellChecker but on for each editor. Actually, I think that's the spellcheck engine service but I will not bet on it...

•

•

14 years ago

Blocks: 660506

Assignee

Comment 48

•

13 years ago

Attached patch updated patch (obsolete) (deleted) — Details — Splinter Review

updated patch. This patch choose dictionary according to editor element @lang. @lang is computed with html5 algorithm http://www.whatwg.org/specs/web-apps/current-work/multipage/#language If no @lang is set, try to use last selected dictionary, or dictionary for current locale. If there is no dictionary for required language (for example, language is pt-BR), try to use dictionary for "generic language" (ie: pt). If this does not work, try to use any dictionary matching language (ie: for example, pt-PT). If no dictionary was found. Either: - if no @lang was set, try to get en-US or any available dictionary. - if some @lang was set, do not set a default dictionary. I think it makes sense to disable dictionaries instead of enabling a wrong dictionary. If I understand correctly, that would also fix #660506 Also, with this patch, default dictionary is not stored automatically in prefs anymore. It's only stored when user sets it manually (otherwise, it would have been set to the lang of last visited page, which is probably wrong). It does not support @lang modification. Ideally, it would be nice to have following behaviour when @lang attribute is changed by script: - if user has changed spellcheck lang manually, don't do anything. - otherwise, sets spellcheck lang to the new computed one. But that means I must observe attributes change, creation and deletion. I'm not sure if it's worth it. What are the usecase of changing lang attribute for some element ? Also, I couldn't write test for this patch because I don't known how to add a dictionary at runtime. If I don't want to do that, what are the other possibilities for testing ?

Attachment #244939 - Attachment is obsolete: true

Attachment #547702 - Flags: review?(ehsan)

Comment 49

•

Comment 55

•

13 years ago

Attached patch wip (obsolete) (deleted) — Details — Splinter Review

•

13 years ago

Well, yes, but if that'll just never work we might as well not even start down this route. David, what do you think of the above bits about making the style system's concept of "language" take into account xml:lang?

Assignee

Comment 62

•

13 years ago

Attached patch patch v2 (obsolete) (deleted) — Details — Splinter Review

new patch. dictionary language is updated each time editor is focused. To implement this, I mainly copied stuff from volkmar's patch. The main difference is I initiate a full spell check (with SpellCheckRange) if language has changed. Otherwise, first focus in a contenteditable area does not check spell (but second focus does).

Attachment #548559 - Attachment is obsolete: true

Attachment #548731 - Flags: review?(ehsan)

Assignee

Comment 63

•

13 years ago

PRUnichar *previousDictionary = nsnull; nsDependentString previousDictionaryStr; if (NS_SUCCEEDED(mSpellCheck->GetCurrentDictionary(&previousDictionary))) { previousDictionaryStr.Assign(previousDictionary); } I wonder if there's a better way to do that. May be nsEditorSpellCheck::GetCurrentDictionary could return a nsAString instead PRUnichar** ?

Comment 64

•

13 years ago

Comment on attachment 548731 [details] [diff] [review] patch v2 Review of attachment 548731 [details] [diff] [review]: ----------------------------------------------------------------- The patch looks very good, r=me on it with the comments below addressed. Thanks a lot for working on this! ::: editor/composer/src/nsEditorSpellCheck.cpp @@ +188,5 @@ > rv = mSpellChecker->SetDocument(tsDoc, PR_TRUE); > NS_ENSURE_SUCCESS(rv, rv); > > + // do not fail if UpdateCurrentDictionary fails because this method may > + // success later. Nit: succeed ::: layout/reftests/editor/338427-ref.html @@ +1,4 @@ > +<!DOCTYPE html> > +<html> > +<body> > + <textarea lang="testing-XX">strangeimpossibleword</textarea> Technically, the -ref file is the reference rendering, and the other file is the thing to be tested. So, these two files should probably be swapped. ::: layout/reftests/editor/reftest.list @@ +62,5 @@ > == 642800.html 642800-ref.html > == selection_visibility_after_reframe.html selection_visibility_after_reframe-ref.html > != selection_visibility_after_reframe-2.html selection_visibility_after_reframe-ref.html > == 672709.html 672709-ref.html > +== 338427.html 338427-ref.html Can you add another test here for the dynamic change case? You can put lang="en-US" on the textarea, then upon load change it to "testing-XX", then .focus() and .blur(). Also, can you please add equivalent tests for contenteditable?

Attachment #548731 - Flags: review?(ehsan) → review+

Assignee

Updated

•

13 years ago

Blocks: 674685

Assignee

Comment 65

•

13 years ago

Attached patch patch v2.1 (obsolete) (deleted) — Details — Splinter Review

(In reply to comment #64) > Nit: succeed fixed > Technically, the -ref file is the reference rendering, and the other file is > the thing to be tested. So, these two files should probably be swapped. fixed > Can you add another test here for the dynamic change case? You can put > lang="en-US" on the textarea, then upon load change it to "testing-XX", then > .focus() and .blur(). do you mean adding commented tests for bug #674685 ? > Also, can you please add equivalent tests for contenteditable? Fixed. This was slightly tricky because it looks like spellcheck="disable" is not supported on contenteditable. So, the reference document contains a non editable area. Then, in the test document, I must first focus the contenteditable, to update dictionary. Then, blur to compare with non editable reference document. But if I focus then blur, I don't capture a focus event in editor listener. Neither if I set a 0 or small timeout. So, I've set a 1000 timeout. I known this is fragile, but I didn't find a better solution.

Assignee: mounir → arno

Attachment #548731 - Attachment is obsolete: true

Assignee

Comment 66

•

13 years ago

> Can you add another test here for the dynamic change case? You can put > lang="en-US" on the textarea, then upon load change it to "testing-XX", then > .focus() and .blur(). Actually, this does not work. Calling SetCurrentDictionary to an invalid language, does nothing. So an invalid language at startup (or first focus) does not initialize spellchecker. But an invalid language after spellchecker has been initialized to a language does not reset it.

Comment 67

•

13 years ago

(In reply to comment #66) > > Can you add another test here for the dynamic change case? You can put > > lang="en-US" on the textarea, then upon load change it to "testing-XX", then > > .focus() and .blur(). > > Actually, this does not work. Calling SetCurrentDictionary to an invalid > language, does nothing. So an invalid language at startup (or first focus) does > not initialize spellchecker. But an invalid language after spellchecker has > been initialized to a language does not reset it. Hmm, well, nevermind those tests then, I guess. :(

Assignee

Comment 68

•

13 years ago

Attached patch patch v2.2 (obsolete) (deleted) — Details — Splinter Review

(In reply to comment #67) > (In reply to comment #66) > > > Can you add another test here for the dynamic change case? You can put > > > lang="en-US" on the textarea, then upon load change it to "testing-XX", then > > > .focus() and .blur(). > > > > Actually, this does not work. Calling SetCurrentDictionary to an invalid > > language, does nothing. So an invalid language at startup (or first focus) does > > not initialize spellchecker. But an invalid language after spellchecker has > > been initialized to a language does not reset it. > > Hmm, well, nevermind those tests then, I guess. :( A possibility is to make SetCurrentDictionary handle empty string by disabling spell check. Then, calling SetCurrentDictionary with empty string at start of UpdateCurrentDirectionary. Can you review this ?

Attachment #548933 - Attachment is obsolete: true

Attachment #549230 - Flags: review?(ehsan)

Assignee

Comment 69

•

13 years ago

Comment on attachment 549230 [details] [diff] [review] patch v2.2 Something is wrong with the patch. I'll work on it again later.

Attachment #549230 - Flags: review?(ehsan)

Assignee

Comment 70

•

13 years ago

Comment on attachment 549230 [details] [diff] [review] patch v2.2 Sorry, I was mistaken

Attachment #549230 - Flags: review?(ehsan)

Comment 71

•

13 years ago

Comment on attachment 549230 [details] [diff] [review] patch v2.2 Review of attachment 549230 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the below comments fixed. ::: content/base/public/nsIContent.h @@ +948,5 @@ > + * Determing language. Look at the nearest ancestor element that has a lang > + * attribute in the XML namespace or is an HTML element and has a lang in > + * no namespace attribute. > + */ > + void GetLang(nsAString& aResult) { Oh, I almost forgot about this! Make this a const method please? ::: layout/reftests/editor/338427-2.html @@ +6,5 @@ > + editor.focus(); > + window.setTimeout(function() { > + editor.blur(); > + document.documentElement.className = ''; > + }, 1000); // XXX: 0 timeout is not enought to trigger a focus event. Change this to: setTimeout(function() { setTimeout(function() { editor.blur(); document.... }, 0); }, 0); ::: layout/reftests/editor/338427-3.html @@ +7,5 @@ > + editor.focus(); > + window.setTimeout(function() { > + editor.blur(); > + document.documentElement.className = ''; > + }, 1000); // XXX: 0 timeout is not enought to trigger a focus event. Same thing here.

Attachment #549230 - Flags: review?(ehsan) → review+

Dão Gottwald [:dao]

Comment 72

•

13 years ago

(In reply to comment #71) > ::: layout/reftests/editor/338427-2.html > @@ +6,5 @@ > > + editor.focus(); > > + window.setTimeout(function() { > > + editor.blur(); > > + document.documentElement.className = ''; > > + }, 1000); // XXX: 0 timeout is not enought to trigger a focus event. > > Change this to: > > setTimeout(function() { > setTimeout(function() { > editor.blur(); > document.... > }, 0); > }, 0); > > ::: layout/reftests/editor/338427-3.html > @@ +7,5 @@ > > + editor.focus(); > > + window.setTimeout(function() { > > + editor.blur(); > > + document.documentElement.className = ''; > > + }, 1000); // XXX: 0 timeout is not enought to trigger a focus event. > > Same thing here. Err, why isn't this using focus event listeners?

Assignee

Comment 73

•

13 years ago

Attached patch patch v2.3 (obsolete) (deleted) — Details — Splinter Review

(In reply to comment #71) Updateded patch with nsIContent::GetLang a const method, and with reftests using focus listeners. > r=me with the below comments fixed. Should I mark this patch as r+ myself then ?

Attachment #549230 - Attachment is obsolete: true

•

13 years ago

Failing mochitest-1 tests on MacOS X and Windows. In addition, it's failing reftests: http://tbpl.mozilla.org/?tree=Try&rev=1af7ee004fb3

André Pirard

Comment 77

•

13 years ago

Check this for checking several languages simultaneously https://bugzilla.mozilla.org/show_bug.cgi?id=660506

Assignee

Comment 78

•

13 years ago

Attached patch patch v2.5 (obsolete) (deleted) — Details — Splinter Review

Assignee

Comment 89

•

13 years ago

I run the test on my system before submitting a patch either here, or on try server. Tests can succeed on my machine but fail in other environments. For example, as I told in comment 78, this can be caused by different font settings. About, when the blur is made inside double timeout, updateCurrentDictionary is not called. > Why is this change needed? because when editor has an unkown language, there is no language set, and CheckWords returns a failure error. Thanks for review and commiting the patch :)

Comment 90

•

13 years ago

(In reply to comment #89) > I run the test on my system before submitting a patch either here, or on try > server. Tests can succeed on my machine but fail in other environments. For > example, as I told in comment 78, this can be caused by different font > settings. Hmm, I was talking about this failure http://tinderbox.mozilla.org/showlog.cgi?log=Try/1312534392.1312536112.31421.gz in comment 78. But I just remembered that Android can't handle focus correctly yet. So, can you just mark the test as needs-focus in the reftest manifest? That should make Android skip the test. > About, when the blur is made inside double timeout, updateCurrentDictionary is > not called. Why? > > Why is this change needed? > > because when editor has an unkown language, there is no language set, and > CheckWords returns a failure error. Then can we just not attempt to do a spell check in that case?

Assignee

Comment 91

•

13 years ago

Attached patch patch v2.6 (obsolete) (deleted) — Details — Splinter Review

> But I just remembered that Android can't handle focus > correctly yet. So, can you just mark the test as needs-focus in the reftest > manifest? That should make Android skip the test. What do you mean with: can't handle focus correctly ? Because, if I understand correctly, needs-focus just check if content if out-of-process http://hg.mozilla.org/mozilla-central/file/a0e3c589c8fa/layout/tools/reftest/reftest.js#l859 Is this the cause focus fails on Android, or should the directive be skip-if(Android) instead ? I've investigated more about the CheckWords call > >- var curlang = spellchecker.GetCurrentDictionary(); > >+ var curlang = ""; > >+ try { > >+ curlang = spellchecker.GetCurrentDictionary(); > >+ } catch(e) {} > > Also, why is this one needed? this code is executed when selecting a dictionary in context menu. This can happen even when no dictionary is active. So this check is strictly needed. > >- if (! spellchecker.CheckCurrentWord(this.mMisspelling)) > >- return 0; // word seems not misspelled after all (?) > >+ try { > >+ if (! spellchecker.CheckCurrentWord(this.mMisspelling)) > >+ return 0; // word seems not misspelled after all (?) > >+ } catch(e) { > >+ return 0; > >+ } > > Why is this change needed? > I think it's less needed, because this.mOverMisspelling should not be true if there is no misspelled word. But then, spellchecker.CheckCurrentWord is as much unneeded as the try/catch around it. > Then can we just not attempt to do a spell check in that case? Good idea. In attached patch, I return early from mozInlineSpellChecker::ResumeCheck if there is not dictionary available. > >diff -r 61bb2bb510c9 extensions/spellcheck/src/mozInlineSpellChecker.cpp > >+ if (NS_FAILED(rv)) > >+ continue; > > Why is this change needed? Then, it should be less needed. But isn't it better to check for functions results. (This call may return with NS_ERROR_OUT_OF_MEMORY for example) Also, this iteration fixes indentation nit. > > About, when the blur is made inside double timeout, updateCurrentDictionary is > > not called. > > Why? I don't known exactly why. But with something like that *in a reftest*, the focus listener is not triggered, event if wrapping the setTimeout inside another setTimeout. var editor = document.getElementById('editor'); editor.addEventListener("focus", function() { // not reached in reftest }, false); window.setTimeout(function() { editor.blur(); }, 0); And we need to trigger the focus listener because that's when UpdateCurrentDictionary is called. Otherwise, test seem to pass, but for the wrong reason: because no dictionary has been set. > Because spellchecking doesn't happen on focus, we post an event to the event > loop and start the spellchecking when that event is processed. So, what I tried to do was using setTimeout inside the focus handler var editor = document.getElementById('editor'); editor.addEventListener("focus", function() { window.setTimeout(function() { editor.addEventListener("blur", function() { document.documentElement.className = ''; }, false); editor.blur(); }, 0); }, false); editor.focus(); Unfortunately, this fails on windows platform http://tinderbox.mozilla.org/showlog.cgi?log=Try/1312952687.1312955723.27999.gz Actually, I already spent more time than initially desired on this issue, and I really don't want to try again to guess what's wrong on a platform I don't even have access to. So, I'll currently stop working and this bug. If someone wants to step in and fix that issue, and the possible other ones remaining, that would be nice; otherwise, I may resume on this later.

Attachment #551030 - Attachment is obsolete: true

Assignee

Updated

•

13 years ago

Status: ASSIGNED → NEW

Mozilla RelEng Bot

Comment 92

•

13 years ago

Try run for 49167a17b8af is complete. Detailed breakdown of the results available here: http://tbpl.mozilla.org/?tree=Try&rev=49167a17b8af Results (out of 223 total builds): success: 202 warnings: 18 failure: 3 Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/arno@renevier.net-49167a17b8af

Assignee

Comment 93

•

13 years ago

Attached patch patch v2.7 (deleted) — Details — Splinter Review

I fixed the reftest with kaze and volkmar help. We came to a better solution: make the test and the reference exactly similar except the reference will have spellcheck=false. Then, there's no need anymore to blur the editor. I also modified another thing since last patch: > > Then can we just not attempt to do a spell check in that case? > > Good idea. In attached patch, I return early from > mozInlineSpellChecker::ResumeCheck if there is not dictionary available. > Actually, I also need to cleanup previously underline words, by calling RemoveRange. Otherwise, a word may stay underlined, even if editor has changed @lang to a unknown one. That case is coved by test-3 I've tested attached patch on tryserver, and it looks like it's correct: http://tbpl.mozilla.org/?tree=Try&rev=3adb6f0528ac

Attachment #552033 - Attachment is obsolete: true

Attachment #552655 - Flags: review?(ehsan)

Comment 94

•

13 years ago

Comment on attachment 552655 [details] [diff] [review] patch v2.7 Review of attachment 552655 [details] [diff] [review]: ----------------------------------------------------------------- This looks great! Thanks a lot for your hard work on this. :-)

Attachment #552655 - Flags: review?(ehsan) → review+

Comment 95

•

13 years ago

Landed on mozilla-inbound.

Component: General → Spelling checker

Flags: blocking-firefox2-

Keywords: dev-doc-needed

Product: Firefox → Core

QA Contact: general → spelling-checker

Kyle Huey (Exited; not receiving bugmail, old account, do not use)

Updated

•

13 years ago

Target Milestone: --- → mozilla8

Dão Gottwald [:dao]

Updated

•

13 years ago

Attachment #461208 - Attachment is obsolete: true

Comment 96

•

13 years ago

http://hg.mozilla.org/mozilla-central/rev/f3f7872db0ae

Status: NEW → RESOLVED

Closed: 13 years ago

Flags: in-testsuite+

Resolution: --- → FIXED

:Ms2ger (he/him; ⌚ UTC+1/+2)

Comment 97

•

13 years ago

> SetCurrentDictionary(NS_LITERAL_STRING("").get()); Can we fix that signature, pretty please?

Assignee

Updated

•

13 years ago

Blocks: 678842

Assignee

Comment 98

•

13 years ago

Oups. Look like there's a serious bug in this patch :( When user manually sets a non default dictionary, it's dismissed as soon as editor is blured and focused again (because we go the UpdateCurrentDictionary path again).

Assignee

Comment 99

•

13 years ago

Attached patch part2: do not override user choice (obsolete) (deleted) — Details — Splinter Review

quick wip patch. I'll try to add some tests tomorrow.

Assignee

Comment 100

•

13 years ago

Attached patch part2: do not override manually set dictionary (deleted) — Details — Splinter Review

patch to fix issue described in comment 98. This works by setting a boolean member mDictWasSetManually to PR_TRUE when SetCurrentDictionary is called, but not from UpdateCurrentDictionary Once that member is true, UpdateCurrentDictionary (and SaveDefaultDictionary) will do nothing. This patch also fixes a typo in a variable name (s/currentDictonary/currentDictionary/)

Attachment #553025 - Attachment is obsolete: true

Attachment #553143 - Flags: review?(ehsan)

Assignee

Comment 101

•

13 years ago

Attached patch fix Get/SetCurrentDictionary signature (obsolete) (deleted) — Details — Splinter Review

(In reply to Ms2ger from comment #97) > > SetCurrentDictionary(NS_LITERAL_STRING("").get()); > > Can we fix that signature, pretty please? Here is a patch changing SetCurrentDictionary and GetCurrentDictionary to use nsAString& instead of PRUnichar*

Attachment #553145 - Flags: review?(ehsan)

:Ms2ger (he/him; ⌚ UTC+1/+2)

Assignee

Comment 102

•

13 years ago

I've tested both patches on try server, and they look correct http://tbpl.mozilla.org/?tree=Try&rev=8300b6949f82

Comment 103

•

13 years ago

Comment on attachment 553145 [details] [diff] [review] fix Get/SetCurrentDictionary signature Some nits: >--- a/editor/composer/src/nsEditorSpellCheck.cpp Mon Aug 15 09:56:38 2011 +0200 >+++ b/editor/composer/src/nsEditorSpellCheck.cpp Mon Aug 15 10:26:10 2011 +0200 >+nsEditorSpellCheck::GetCurrentDictionary(nsAString& aDictionary) > { >+ nsresult rv = mSpellChecker->GetCurrentDictionary(aDictionary); > return rv; "return mSpellChecker->GetCurrentDictionary(aDictionary);" > nsEditorSpellCheck::SaveDefaultDictionary() > { > if (!mDictWasSetManually) { > return NS_OK; > } >- PRUnichar *dictName = nsnull; >- nsresult rv = GetCurrentDictionary(&dictName); > >- if (NS_SUCCEEDED(rv) && dictName && *dictName) { >- rv = Preferences::SetString("spellchecker.dictionary", dictName); >- } >+ nsAutoString dictName; >+ nsresult rv = GetCurrentDictionary(dictName); >+ NS_ENSURE_SUCCESS(rv, rv); > >- if (dictName) { >- nsMemory::Free(dictName); >- } >- >+ rv = Preferences::SetString("spellchecker.dictionary", dictName); > return rv; Same comment, and I think you're changing behaviour here if dictName is the empty string (can that happen?) >@@ -501,65 +485,65 @@ nsEditorSpellCheck::UpdateCurrentDiction >- SetCurrentDictionary(NS_LITERAL_STRING("").get()); >+ SetCurrentDictionary(NS_LITERAL_STRING("")); EmptyString() >--- a/extensions/spellcheck/src/mozInlineSpellChecker.cpp Mon Aug 15 09:56:38 2011 +0200 >+++ b/extensions/spellcheck/src/mozInlineSpellChecker.cpp Mon Aug 15 10:26:10 2011 +0200 >@@ -1755,36 +1755,27 @@ nsresult mozInlineSpellChecker::KeyPress > } > > NS_IMETHODIMP mozInlineSpellChecker::UpdateCurrentDictionary() > { >+ previousDictionary.Assign(NS_LITERAL_STRING("")); "previousDictionary.Truncate();" >+ currentDictionary.Assign(NS_LITERAL_STRING("")); And here But overall, this looks much better!

Attachment #553145 - Flags: feedback+

Assignee

Comment 104

•

13 years ago

Attached patch part3: fix Get/SetCurrentDictionary signature (deleted) — Details — Splinter Review

patch for the Get/SetCurrentDictionary signature, resolving issues raised my Ms2ger. > Same comment, and I think you're changing behaviour here if dictName is the > empty string (can that happen?) If SetCurrentDictionary has been called with empty string, GetCurrentDictionary will return an error.

Attachment #553145 - Attachment is obsolete: true

Attachment #553150 - Flags: review?

Attachment #553145 - Flags: review?(ehsan)

Comment 105

•

13 years ago

Comment on attachment 553143 [details] [diff] [review] part2: do not override manually set dictionary >+ PRBool mUpdateDictionaryRunning; >+ PRBool mDictWasSetManually; Please use PRPackedBool. >+class UpdateDictionnaryHolder { >+ private: >+ nsEditorSpellCheck* mSpellCheck; >+ public: >+ UpdateDictionnaryHolder(nsEditorSpellCheck* esc): mSpellCheck(esc) { >+ if (mSpellCheck) { >+ mSpellCheck->BeginUpdateDictionary(); >+ } >+ } >+ ~UpdateDictionnaryHolder() { >+ if (mSpellCheck) { >+ mSpellCheck->EndUpdateDictionary(); >+ } >+ } > }; Also, please move this class to the cpp file.

Attachment #553143 - Flags: review?(ehsan) → review+