Last Comment Bug 678842 - remember spell check setting per site
: remember spell check setting per site
Status: RESOLVED FIXED
: addon-compat, dev-doc-complete
Product: Core
Classification: Components
Component: Spelling checker (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: mozilla9
Assigned To: arno renevier
:
Mentors:
Depends on: 338427
Blocks: 684315
  Show dependency treegraph
 
Reported: 2011-08-14 10:25 PDT by arno renevier
Modified: 2012-02-04 16:51 PST (History)
12 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch v1 (26.01 KB, patch)
2011-08-18 09:34 PDT, arno renevier
no flags Details | Diff | Splinter Review
patch v1.2 (27.43 KB, patch)
2011-08-19 08:43 PDT, arno renevier
no flags Details | Diff | Splinter Review
patch v1.3 (27.07 KB, patch)
2011-08-20 00:01 PDT, arno renevier
ehsan: review+
Details | Diff | Splinter Review
patch v1.4 (27.07 KB, patch)
2011-08-23 08:00 PDT, arno renevier
ehsan: review+
Details | Diff | Splinter Review

Description arno renevier 2011-08-14 10:25:43 PDT
Hi,
since #338427 is fixed, there is a problem which needs to be addressed:
If a website provides a wrong language information, or if user really wants to override it, he will have to switch manually to correct dictionary. If user visits this this site often, it will become a hassle.
So may be, spellchecker language could be remembered per site (per domain?) if it has been automatically set by user.
Comment 1 arno renevier 2011-08-14 10:41:41 PDT
(In reply to arno renevier from comment #0)

> if it has been automatically set by user.

I meant *manually* here
Comment 2 Fabien Cazenave [:kaze] 2011-08-14 11:03:02 PDT
+1
And the related pref could be cleared if the user re-selects the spellchecker language that is suggested by the page.
Comment 3 :Ehsan Akhgari 2011-08-15 08:16:10 PDT
We can probably store the last used dictionary per site as a content pref or something, unless Gavin objects.
Comment 4 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-08-16 17:56:52 PDT
I can't think of any reason to object!
Comment 5 arno renevier 2011-08-18 09:34:50 PDT
Created attachment 554114 [details] [diff] [review]
patch v1

Here is a patch proposal.
Here is tinderbox link:
http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=b655f08d2c5a
It currently nearly finished, and so far, seems to have caused no failure.
Comment 6 arno renevier 2011-08-19 01:56:23 PDT
Comment on attachment 554114 [details] [diff] [review]
patch v1

removing request flag because I may have found a minor bug
Comment 7 arno renevier 2011-08-19 08:43:14 PDT
Created attachment 554429 [details] [diff] [review]
patch v1.2

Updated patch. I realized I have to add spellcheck.lang to white list in content pref service (I don't fully understand why though).

Here is tinderbox link:
http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=71284ac3cdd5
Comment 8 :Ehsan Akhgari 2011-08-19 12:23:16 PDT
Comment on attachment 554429 [details] [diff] [review]
patch v1.2

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

Generally this patch looks very good.  But I'd like to look at the next version when you fix the points below, please!  :-)

::: editor/composer/src/nsEditorSpellCheck.cpp
@@ +58,3 @@
>  #include "nsServiceManagerUtils.h"
>  #include "nsIChromeRegistry.h"
> +#include "nsIPrivateBrowsingService.h"

I cried a little bit out of happiness when I saw you handled the private browsing mode here.  :-)

@@ +96,5 @@
> +    do_GetService(NS_PRIVATE_BROWSING_SERVICE_CONTRACTID);
> +  if (pbService) {
> +    pbService->GetPrivateBrowsingEnabled(&mInPrivateBrowsing);
> +  }
> +  mMemoryStorage.Init();

You can probably avoid initing the hashtable if the PB service doesn't exist.

@@ +120,5 @@
> +    rootContent = do_QueryInterface(rootElement);
> +  }
> +  NS_ENSURE_TRUE(rootContent, NS_ERROR_FAILURE);
> +
> +  nsCOMPtr<nsIDocument> doc = rootContent->GetOwnerDoc();

Can't you just call nsIEditor::GetDocument?

@@ +265,5 @@
> +  } 
> +  return NS_OK;
> +}
> +
> +LastDictionary* nsEditorSpellCheck::gDictionaryStore = NULL;

Nit: nsnull.

@@ +278,5 @@
>  NS_INTERFACE_MAP_END
>  
>  NS_IMPL_CYCLE_COLLECTION_2(nsEditorSpellCheck,
>                             mSpellChecker,
>                             mTxtSrvFilter)

Make mEditor participate in cycle collection too.

@@ +283,5 @@
>  
>  nsEditorSpellCheck::nsEditorSpellCheck()
>    : mSuggestedWordIndex(0)
>    , mDictionaryIndex(0)
> +  , mEditor(NULL)

Ditto.

::: editor/composer/src/nsEditorSpellCheck.h
@@ +55,5 @@
>      0x75656ad9, 0xbd13, 0x4c5d,                       \
>      { 0x93, 0x9a, 0xec, 0x63, 0x51, 0xee, 0xa0, 0xcc }\
>  }
>  
> +class LastDictionary : public nsIObserver, public nsSupportsWeakReference {

Please forward declare this class, and move it and the headers needed by it to the .cpp file.

@@ +56,5 @@
>      { 0x93, 0x9a, 0xec, 0x63, 0x51, 0xee, 0xa0, 0xcc }\
>  }
>  
> +class LastDictionary : public nsIObserver, public nsSupportsWeakReference {
> +  NS_DECL_ISUPPORTS

I'd rather if you would say public: explicitly here.

@@ +81,5 @@
> +  /**
> +   * get uri of editor's document.
> +   *
> +   */
> +  static nsresult GetEditorUri(nsIEditor* aEditor, nsIURI * *aURI);

Nit: Please use GetDocumentURI.

::: editor/composer/test/Makefile.in
@@ +48,5 @@
>  		test_bug348497.html \
>  		test_bug384147.html \
>  		test_bug389350.html \
>  		test_bug519928.html \
> +			 bug678842_subframe.html \

Nit: please fix indentation.

::: editor/composer/test/bug678842_subframe.html
@@ +2,5 @@
> +<html>
> +<head>
> +<script>
> +function init() {
> +  netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");

Can you please load this page from a chrome URL, so that you can remove this line?  The PrivilegeManager API is going away soon...

::: editor/composer/test/test_bug678842.html
@@ +16,5 @@
> +<pre id="test">
> +<script class="testbody" type="text/javascript">
> +
> +/** Test for Bug 678842 **/
> +// just a fake test

???!

@@ +22,5 @@
> +var content = document.getElementById('content');
> +// load a subframe containing an editor with a defined unknown lang. At first
> +// load, it will set dictionary to en-US. At second load, it will return current
> +// dictionary. So, we can check, dictionary is correctly remembered between
> +// loads.

I assume you test things this way in order to avoid installing a dictionary, right?
Comment 9 arno renevier 2011-08-20 00:01:28 PDT
Created attachment 554616 [details] [diff] [review]
patch v1.3

(In reply to Ehsan Akhgari [:ehsan] from comment #8)
> Comment on attachment 554429 [details] [diff] [review]
> patch v1.2
> 
>
> @@ +58,3 @@
> >  #include "nsServiceManagerUtils.h"
> >  #include "nsIChromeRegistry.h"
> > +#include "nsIPrivateBrowsingService.h"
> 
> I cried a little bit out of happiness when I saw you handled the private
> browsing mode here.  :-)

What's more, with #679784, that handling will be centralized, and we can remove it from spell check.

> 
> @@ +96,5 @@
> > +    do_GetService(NS_PRIVATE_BROWSING_SERVICE_CONTRACTID);
> > +  if (pbService) {
> > +    pbService->GetPrivateBrowsingEnabled(&mInPrivateBrowsing);
> > +  }
> > +  mMemoryStorage.Init();
> 
> You can probably avoid initing the hashtable if the PB service doesn't exist.

fixed

> @@ +120,5 @@
> > +    rootContent = do_QueryInterface(rootElement);
> > +  }
> > +  NS_ENSURE_TRUE(rootContent, NS_ERROR_FAILURE);
> > +
> > +  nsCOMPtr<nsIDocument> doc = rootContent->GetOwnerDoc();
> 
> Can't you just call nsIEditor::GetDocument?

fixed

> @@ +265,5 @@
> > +  } 
> > +  return NS_OK;
> > +}
> > +
> > +LastDictionary* nsEditorSpellCheck::gDictionaryStore = NULL;
> 
> Nit: nsnull.

> @@ +283,5 @@
> >  
> >  nsEditorSpellCheck::nsEditorSpellCheck()
> >    : mSuggestedWordIndex(0)
> >    , mDictionaryIndex(0)
> > +  , mEditor(NULL)
> 
> Ditto.

Ok. But I had chosen NULL because https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide#C.2fC.2b.2b_practices tells
In new code, use NULL for pointers. In old code, using nsnull or 0 for consistency is allowed. Is this advice wrong ?

> 
> @@ +278,5 @@
> >  NS_INTERFACE_MAP_END
> >  
> >  NS_IMPL_CYCLE_COLLECTION_2(nsEditorSpellCheck,
> >                             mSpellChecker,
> >                             mTxtSrvFilter)
> 
> Make mEditor participate in cycle collection too.

fixed


> ::: editor/composer/src/nsEditorSpellCheck.h
> @@ +55,5 @@
> >      0x75656ad9, 0xbd13, 0x4c5d,                       \
> >      { 0x93, 0x9a, 0xec, 0x63, 0x51, 0xee, 0xa0, 0xcc }\
> >  }
> >  
> > +class LastDictionary : public nsIObserver, public nsSupportsWeakReference {
> 
> Please forward declare this class, and move it and the headers needed by it
> to the .cpp file.

fixed

> @@ +56,5 @@
> >      { 0x93, 0x9a, 0xec, 0x63, 0x51, 0xee, 0xa0, 0xcc }\
> >  }
> >  
> > +class LastDictionary : public nsIObserver, public nsSupportsWeakReference {
> > +  NS_DECL_ISUPPORTS
> 
> I'd rather if you would say public: explicitly here.

fixed

> @@ +81,5 @@
> > +  /**
> > +   * get uri of editor's document.
> > +   *
> > +   */
> > +  static nsresult GetEditorUri(nsIEditor* aEditor, nsIURI * *aURI);
> 
> Nit: Please use GetDocumentURI.

fixed

> ::: editor/composer/test/Makefile.in
> @@ +48,5 @@
> >  		test_bug348497.html \
> >  		test_bug384147.html \
> >  		test_bug389350.html \
> >  		test_bug519928.html \
> > +			 bug678842_subframe.html \
> 
> Nit: please fix indentation.

fixed

> ::: editor/composer/test/bug678842_subframe.html
> @@ +2,5 @@
> > +<html>
> > +<head>
> > +<script>
> > +function init() {
> > +  netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");
> 
> Can you please load this page from a chrome URL, so that you can remove this
> line?  The PrivilegeManager API is going away soon...

This page must be loaded from http protocol. (We are trying to test the saving of a pref according to a http domain). We in this patch, I make all operations and checks from chrome parent. This also makes the test simpler.

> ::: editor/composer/test/test_bug678842.html
> @@ +16,5 @@
> > +<pre id="test">
> > +<script class="testbody" type="text/javascript">
> > +
> > +/** Test for Bug 678842 **/
> > +// just a fake test
> 
> ???!

fixed

> @@ +22,5 @@
> > +var content = document.getElementById('content');
> > +// load a subframe containing an editor with a defined unknown lang. At first
> > +// load, it will set dictionary to en-US. At second load, it will return current
> > +// dictionary. So, we can check, dictionary is correctly remembered between
> > +// loads.
> 
> I assume you test things this way in order to avoid installing a dictionary,
> right?

I don't understand. Do you see other ways to test ?

Here is tryserver result:
http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=ca908b79d99e
http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=ca908b79d99e
Comment 10 :Ehsan Akhgari 2011-08-22 16:21:48 PDT
(In reply to arno renevier from comment #9)
> Ok. But I had chosen NULL because
> https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide#C.2fC.2b.
> 2b_practices tells
> In new code, use NULL for pointers. In old code, using nsnull or 0 for
> consistency is allowed. Is this advice wrong ?

Probably not, but this is not new code, so let's use nsnull for consistency.  :-)

> This page must be loaded from http protocol. (We are trying to test the
> saving of a pref according to a http domain). We in this patch, I make all
> operations and checks from chrome parent. This also makes the test simpler.

Ah, right!

> > I assume you test things this way in order to avoid installing a dictionary,
> > right?
> 
> I don't understand. Do you see other ways to test ?

No, just confirming my own sanity.

> Here is tryserver result:
> http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=ca908b79d99e

Looks good. I'll push this to inbound.  Thanks a lot for the patch!
Comment 11 :Ehsan Akhgari 2011-08-22 16:22:17 PDT
(In reply to Ehsan Akhgari [:ehsan] from comment #10)
> (In reply to arno renevier from comment #9)
> > Ok. But I had chosen NULL because
> > https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide#C.2fC.2b.
> > 2b_practices tells
> > In new code, use NULL for pointers. In old code, using nsnull or 0 for
> > consistency is allowed. Is this advice wrong ?
> 
> Probably not, but this is not new code, so let's use nsnull for consistency.
> :-)

Actually, people on IRC seemed to think that I'm wrong, so nevermind.  :-)
Comment 12 :Ehsan Akhgari 2011-08-22 19:34:49 PDT
So, I had to backout the patch:

http://hg.mozilla.org/integration/mozilla-inbound/rev/562229c22e97

The reason is that the test went orange: <http://tbpl.allizom.org/php/getParsedLog.php?id=6077272&full=1>.  When I looked more closely at the test, I noticed that it never calls SimpleTest.finish, which is very wrong.

What puzzles me is why this test would *ever* finish successfully!  Here is the log from a successful run:

9408 INFO TEST-START | chrome://mochitests/content/chrome/editor/composer/test/test_bug678842.html
9409 INFO TEST-END | chrome://mochitests/content/chrome/editor/composer/test/test_bug678842.html | finished in 300ms

Ted, is it now OK to call waitForExplicitFinish without any call to finish?

Arno, you probably want to fix the test to call finish when the second load happens, and also remove the bogus comment, both of which I failed to catch in the final review.  :-)
Comment 13 :Ms2ger (⌚ UTC+1/+2) 2011-08-23 01:55:11 PDT
Comment on attachment 554616 [details] [diff] [review]
patch v1.3

>+LastDictionary::GetDocumentURI(nsIEditor* aEditor, nsIURI * *aURI)
>+{
>+  nsCOMPtr<nsIURI> docUri = doc->GetDocumentURI();
>+  *aURI = docUri;
>+  NS_ADDREF(*aURI);

For future reference, "docUri.forget(aURI);" would have saved an addref-release pair.
Comment 14 arno renevier 2011-08-23 08:00:40 PDT
Created attachment 555104 [details] [diff] [review]
patch v1.4

This patch fixes the test. Sorry for forgetting the SimpleTest.finish()
But actually, with previous patch, tests run correctly on my machine, and also on try server. This was slightly frustrating to see this patch running correctly on try server, but not correctly when pushed to inbound
Comment 15 Ted Mielczarek [:ted.mielczarek] 2011-08-23 10:20:32 PDT
(In reply to Ehsan Akhgari [:ehsan] from comment #12)
> Ted, is it now OK to call waitForExplicitFinish without any call to finish?

Not that I'm aware. I have no idea why this would work!
Comment 16 :Ehsan Akhgari 2011-08-23 12:03:10 PDT
Comment on attachment 555104 [details] [diff] [review]
patch v1.4

I'll push this to inbound again.
Comment 17 Marco Bonardo [::mak] (Away 6-20 Aug) 2011-08-24 01:39:14 PDT
http://hg.mozilla.org/mozilla-central/rev/a624f57a9e6f
Comment 18 arno renevier 2011-08-24 07:53:07 PDT
If I understand correctly, bug #338427 will go in mozilla8, but this bug will go in mozilla9. As explained in comment 1, there is one case (a user visiting often a website which sets a wrong lang) when bug #338427 makes things more less convenient than current behaviour.
So, should both bugs target the same milestone ?
Comment 19 :Ehsan Akhgari 2011-09-02 14:13:47 PDT
(In reply to arno renevier from comment #18)
> If I understand correctly, bug #338427 will go in mozilla8, but this bug
> will go in mozilla9. As explained in comment 1, there is one case (a user
> visiting often a website which sets a wrong lang) when bug #338427 makes
> things more less convenient than current behaviour.
> So, should both bugs target the same milestone ?

I think we want to backout bug 338427 from Aurora.  Can you please file a bug about that, and attach a backout patch for approval?  Thanks!
Comment 20 :aceman 2011-10-13 00:36:26 PDT
Just for reference, this stored data will not be exposed in about:permissions, like zoom isn't?
Comment 21 :Ehsan Akhgari 2011-10-13 13:29:23 PDT
(In reply to aceman from comment #20)
> Just for reference, this stored data will not be exposed in
> about:permissions, like zoom isn't?

The stored data is a content pref.  I think you should file a new bug for that.
Comment 22 Eric Shepherd [:sheppy] 2011-11-17 14:04:19 PST
This is mentioned on Firefox 9 for developers. I've also now created an initial version of the docs for nsIEditorSpellCheck, although work remains to be done on it:

https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIEditorSpellCheck

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