1421510 - Make nsITextServicesFilter builtin class

Status: RESOLVED FIXED

(Core :: DOM: Editor, enhancement, P3)

Description

[Makoto Kato [:m_kato]]

c-c and bluegriffon use nsITextServicesFilter interface, but they doesn't override and create own nsITextServicesFilter object.

So we can make nsITextServicesFilter builtin class.

Comment 1

[Makoto Kato [:m_kato]]

Attachment: Bug 1421510 - Part 1. Make nsITextServicesFilter builtinclass.

There is no JavaScript implementation (including c-c and bluegriffon) for
nsITextServicesFilter, so we make this builtin to access builtin C++
implementation directly.

Review commit: https://reviewboard.mozilla.org/r/251202/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/251202/

Comment 2

[Makoto Kato [:m_kato]]

Attachment: Bug 1421510 - Part 2. Use nsComposeTxtSrvFilter directly.

nsITextServicesFilter is builtin class, so we can store nsComposeTxtSrvFilter
instead of nsITextServicesFilter.

Review commit: https://reviewboard.mozilla.org/r/251204/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/251204/

Comment 3

[Makoto Kato [:m_kato]]

Attachment: Bug 1421510 - Part 3. Make nsITextServicesFilter.skip noscript access.

nsITextServicesFilter.skip isn't access from JavaScript (including c-c and
bluegriffon), so we can remove it from IDL.

Also, skip method should use early return style for clean up.

Review commit: https://reviewboard.mozilla.org/r/251206/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/251206/

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8985669 [details]
Bug 1421510 - Part 1. Make nsITextServicesFilter builtinclass.

https://reviewboard.mozilla.org/r/251202/#review257566

Really nice!

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8985670 [details]
Bug 1421510 - Part 2. Use nsComposeTxtSrvFilter directly.

https://reviewboard.mozilla.org/r/251204/#review257568

::: editor/spellchecker/EditorSpellCheck.h:19
(Diff revision 1)
>  #include "nsString.h"                   // for nsString
>  #include "nsTArray.h"                   // for nsTArray
>  #include "nscore.h"                     // for nsresult
>  
>  class mozSpellChecker;
> +class nsComposeTxtSrvFilter;

I'd be happy if it'd be renamed to mozilla::ComposeTextServiceFilter or something.

::: editor/spellchecker/EditorSpellCheck.h:61
(Diff revision 1)
>  
>  protected:
>    virtual ~EditorSpellCheck();
>  
>    RefPtr<mozSpellChecker> mSpellChecker;
> -  nsCOMPtr<nsITextServicesFilter> mTxtSrvFilter;
> +  RefPtr<nsComposeTxtSrvFilter> mTxtSrvFilter;

Perhaps we'd rename this more specific later.

::: editor/spellchecker/TextServicesDocument.h:61
(Diff revision 1)
>    nsCOMPtr<nsIContentIterator> mIterator;
>    nsCOMPtr<nsIContent> mPrevTextBlock;
>    nsCOMPtr<nsIContent> mNextTextBlock;
>    nsTArray<OffsetEntry*> mOffsetTable;
>    RefPtr<nsRange> mExtent;
> -  nsCOMPtr<nsITextServicesFilter> mTxtSvcFilter;
> +  RefPtr<nsComposeTxtSrvFilter> mTxtSvcFilter;

This also would be renamed to more specific name later.

::: editor/spellchecker/nsFilteredContentIterator.h:16
(Diff revision 1)
>  class nsINode;
> -class nsITextServicesFilter;
> +class nsComposeTxtSrvFilter;

Put before nsINode.

::: editor/spellchecker/nsFilteredContentIterator.h:80
(Diff revision 1)
>    RefPtr<nsAtom> mScriptAtom;
>    RefPtr<nsAtom> mTextAreaAtom;
>    RefPtr<nsAtom> mSelectAreaAtom;
>    RefPtr<nsAtom> mMapAtom;
>  
> -  nsCOMPtr<nsITextServicesFilter> mFilter;
> +  RefPtr<nsComposeTxtSrvFilter> mFilter;

This also would be renamed to more specific name later.

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8985671 [details]
Bug 1421510 - Part 3. Make nsITextServicesFilter.skip noscript access.

https://reviewboard.mozilla.org/r/251206/#review257572

::: commit-message-e9128:3
(Diff revision 1)
> +Bug 1421510 - Part 3. Make nsITextServicesFilter.skip noscript access. r?masayuki
> +
> +nsITextServicesFilter.skip isn't access from JavaScript (including c-c and

s/access/accessed

::: editor/spellchecker/nsComposeTxtSrvFilter.cpp:24
(Diff revision 1)
>  NS_IMPL_ISUPPORTS(nsComposeTxtSrvFilter, nsITextServicesFilter)
>  
> -NS_IMETHODIMP
> -nsComposeTxtSrvFilter::Skip(nsINode* aNode, bool *_retval)
> +bool
> +nsComposeTxtSrvFilter::Skip(nsINode* aNode) const
>  {
> -  *_retval = false;
> +  if (!aNode) {

NS_WARN_IF()? Or, there is only one caller of this method and it guarantees aNode is not nullptr. So, why don't you make it nsINode&?

And also this returns true only when the node is an element node. So, how about to check aNode->IsElement() first?

::: editor/spellchecker/nsComposeTxtSrvFilter.cpp:29
(Diff revision 1)
>    // For nodes that are blockquotes, we must make sure
>    // their type is "cite"

I think that this comment (last 2 lines) should be moved to immediately before |if (aNode->IsHTMLElement(nsGkAtoms::blockquote))|.

::: editor/spellchecker/nsITextServicesFilter.idl:9
(Diff revision 1)
>  #include "nsISupports.idl"
>  
> -webidl Node;
> -
>  [scriptable, builtinclass, uuid(5BEC321F-59AC-413a-A4AD-8A8D7C50A0D0)]
>  interface nsITextServicesFilter : nsISupports

Hmm... Becomes empty interface...

Comment 7

[Makoto Kato [:m_kato]]

Comment on attachment 8985670 [details]
Bug 1421510 - Part 2. Use nsComposeTxtSrvFilter directly.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/251204/diff/1-2/

Comment 8

[Makoto Kato [:m_kato]]

Comment on attachment 8985671 [details]
Bug 1421510 - Part 3. Make nsITextServicesFilter.skip noscript access.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/251206/diff/1-2/

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9941588e6326
Part 1. Make nsITextServicesFilter builtinclass. r=masayuki
https://hg.mozilla.org/integration/autoland/rev/9afd36a4db26
Part 2. Use nsComposeTxtSrvFilter directly. r=masayuki
https://hg.mozilla.org/integration/autoland/rev/9222fabb7ee6
Part 3. Make nsITextServicesFilter.skip noscript access. r=masayuki

Comment 10

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/9941588e6326
https://hg.mozilla.org/mozilla-central/rev/9afd36a4db26
https://hg.mozilla.org/mozilla-central/rev/9222fabb7ee6