Closed
Bug 678842
Opened 13 years ago
Closed 13 years ago
remember spell check setting per site
Categories
(Core :: Spelling checker, defect)
Tracking
()
RESOLVED
FIXED
mozilla9
People
(Reporter: arno, Assigned: arno)
References
Details
(Keywords: addon-compat, dev-doc-complete)
Attachments
(1 file, 3 obsolete files)
|
(deleted),
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
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.
|
Assignee | |
Comment 1•13 years ago
|
||
(In reply to arno renevier from comment #0)
> if it has been automatically set by user.
I meant *manually* here
|
||
Comment 2•13 years ago
|
||
+1
And the related pref could be cleared if the user re-selects the spellchecker language that is suggested by the page.
|
||
Comment 3•13 years ago
|
||
We can probably store the last used dictionary per site as a content pref or something, unless Gavin objects.
|
||
Comment 4•13 years ago
|
||
I can't think of any reason to object!
|
|
Assignee | |
Comment 5•13 years ago
|
||
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 | |
Comment 6•13 years ago
|
||
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)
|
|
Assignee | |
Comment 7•13 years ago
|
||
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 8•13 years ago
|
||
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)
|
|
Assignee | |
Comment 9•13 years ago
|
||
(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)
|
|
||
Comment 10•13 years ago
|
||
(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•13 years ago
|
||
(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. :-)
|
|
||
Updated•13 years ago
|
Attachment #554616 -
Flags: review?(ehsan) → review+
|
|
||
Updated•13 years ago
|
Keywords: dev-doc-needed
|
|
||
Comment 12•13 years ago
|
||
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•13 years ago
|
||
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.
|
|
Assignee | |
Comment 14•13 years ago
|
||
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)
|
||
Comment 15•13 years ago
|
||
(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•13 years ago
|
||
Comment on attachment 555104 [details] [diff] [review]
patch v1.4
I'll push this to inbound again.
Attachment #555104 -
Flags: review?(ehsan) → review+
|
||
Comment 17•13 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
|
|
Assignee | |
Comment 18•13 years ago
|
||
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•13 years ago
|
||
(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!
|
||
Updated•13 years ago
|
Keywords: addon-compat
|
||
Comment 20•13 years ago
|
||
Just for reference, this stored data will not be exposed in about:permissions, like zoom isn't?
|
|
||
Comment 21•13 years ago
|
||
(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•13 years ago
|
||
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
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•