Closed Bug 1421510 Opened 7 years ago Closed 6 years ago

Make nsITextServicesFilter builtin class

Categories

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

Product:
Core
Component:
DOM: Editor
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox59 --- wontfix
firefox60 --- wontfix
firefox61 --- wontfix
firefox62 --- fixed

People

(Reporter: m_kato, Assigned: m_kato)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

Bug 1421510 - Part 1. Make nsITextServicesFilter builtinclass.
59 bytes, text/x-review-board-request
masayuki
: review+
Details
Bug 1421510 - Part 2. Use nsComposeTxtSrvFilter directly.
59 bytes, text/x-review-board-request
masayuki
: review+
Details
Bug 1421510 - Part 3. Make nsITextServicesFilter.skip noscript access.
59 bytes, text/x-review-board-request
masayuki
: review+
Details 
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 on attachment 8985669 [details]
Bug 1421510 - Part 1. Make nsITextServicesFilter builtinclass.

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

Really nice!
Attachment #8985669 - Flags: review?(masayuki) → review+
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.
Attachment #8985670 - Flags: review?(masayuki) → review+
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...
Attachment #8985671 - Flags: review?(masayuki) → review+
Assignee: nobody → m_kato
Status: NEW → ASSIGNED
Pushed by m_kato@ga2.so-net.ne.jp:
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
https://hg.mozilla.org/mozilla-central/rev/9941588e6326
https://hg.mozilla.org/mozilla-central/rev/9afd36a4db26
https://hg.mozilla.org/mozilla-central/rev/9222fabb7ee6
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: