> 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.
> >- 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
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.
Created attachment 552033
patch v2.6
> 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 ? hg.mozilla. org/mozilla- central/ file/a0e3c589c8 fa/layout/ tools/reftest/ reftest. js#l859
Because, if I understand correctly, needs-focus just check if content if out-of-process
http://
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. GetCurrentDicti onary() ; GetCurrentDicti onary() ;
> >+ var curlang = "";
> >+ try {
> >+ curlang = spellchecker.
> >+ } 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. CheckCurrentWor d(this. mMisspelling) ) CheckCurrentWor d(this. mMisspelling) )
> >- return 0; // word seems not misspelled after all (?)
> >+ try {
> >+ if (! spellchecker.
> >+ 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.mOverMissp elling should not be true if CheckCurrentWor d is as much
there is no misspelled word. But then, spellchecker.
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 hecker: :ResumeCheck if there is not dictionary available.
mozInlineSpellC
> >diff -r 61bb2bb510c9 extensions/ spellcheck/ src/mozInlineSp ellChecker. 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 OUT_OF_ MEMORY for example)
results. (This call may return with NS_ERROR_
Also, this iteration fixes indentation nit.
> > About, when the blur is made inside double timeout, updateCurrentDi ctionary 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' ); addEventListene r("focus" , function() {
window. setTimeout( function( ) {
editor. blur();
editor.
// not reached in reftest
}, false);
}, 0);
And we need to trigger the focus listener because that's when UpdateCurrentDi ctionary 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' ); addEventListene r("focus" , function() {
window. setTimeout( function( ) {
editor. addEventListene r("blur" , function() {
document. documentElement .className = '';
editor. blur();
editor.
}, false);
}, 0);
}, false);
editor.focus();
Unfortunately, this fails on windows platform tinderbox. mozilla. org/showlog. cgi?log= Try/1312952687. 1312955723. 27999.gz
http://
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.