Closed Bug 678842 Opened 13 years ago Closed 13 years ago

remember spell check setting per site

Categories

(Core :: Spelling checker, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla9

People

(Reporter: arno, Assigned: arno)

References

Details

(Keywords: addon-compat, dev-doc-complete)

Attachments

(1 file, 3 obsolete files)

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.
(In reply to arno renevier from comment #0)

> if it has been automatically set by user.

I meant *manually* here
+1
And the related pref could be cleared if the user re-selects the spellchecker language that is suggested by the page.
We can probably store the last used dictionary per site as a content pref or something, unless Gavin objects.
I can't think of any reason to object!
Attached patch patch v1 (obsolete) — Splinter Review
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.
Assignee: nobody → arno
Status: NEW → ASSIGNED
Attachment #554114 - Flags: review?(ehsan)
Comment on attachment 554114 [details] [diff] [review]
patch v1

removing request flag because I may have found a minor bug
Attachment #554114 - Flags: review?(ehsan)
Attached patch patch v1.2 (obsolete) — Splinter Review
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
Attachment #554114 - Attachment is obsolete: true
Attachment #554429 - Flags: review?(ehsan)
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?
Attachment #554429 - Flags: review?(ehsan)
Attached patch patch v1.3 (obsolete) — Splinter Review
(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
Attachment #554429 - Attachment is obsolete: true
Attachment #554616 - Flags: review?(ehsan)
(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!
(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.  :-)
Attachment #554616 - Flags: review?(ehsan) → review+
Keywords: dev-doc-needed
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 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.
Attached patch patch v1.4Splinter Review
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
Attachment #554616 - Attachment is obsolete: true
Attachment #555104 - Flags: review?(ehsan)
(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 on attachment 555104 [details] [diff] [review]
patch v1.4

I'll push this to inbound again.
Attachment #555104 - Flags: review?(ehsan) → review+
http://hg.mozilla.org/mozilla-central/rev/a624f57a9e6f
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
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 ?
Blocks: 684315
(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!
Just for reference, this stored data will not be exposed in about:permissions, like zoom isn't?
(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.
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
You need to log in before you can comment on or make changes to this bug.