Remove more nsIDOMDocument useages from editor

RESOLVED FIXED in Firefox 61

Status

()

enhancement
P3
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: m_kato, Assigned: m_kato)

Tracking

Trunk
mozilla61
Points:
---

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

a year ago
to avoid QI and unnecessary addrefs
(Assignee)

Updated

a year ago
Assignee: nobody → m_kato
Comment hidden (mozreview-request)
Comment on attachment 8965583 [details]
Bug 1451972 - Remove more nsIDOMDocument usages from editor.

https://reviewboard.mozilla.org/r/234402/#review239988

::: editor/libeditor/HTMLEditor.cpp:4989
(Diff revision 1)
> +{
> +  nsIDocument* doc = GetDocument();
> +  if (NS_WARN_IF(!doc)) {
> +    return nullptr;
> +  }
> +  nsCOMPtr<nsIHTMLDocument> htmlDoc = do_QueryInterface(doc);

Must be able to use nsIDocument::AsHTMLDocument() intead of QI. Then, please change the result from already_AddRefed from raw pointer since caller may not want to grab it strongly.

::: editor/libeditor/TextEditorDataTransfer.cpp:187
(Diff revision 1)
>        return NS_OK;
>      }
>    }
>  
>    // Current doc is destination
> -  nsCOMPtr<nsIDOMDocument> destdomdoc = GetDOMDocument();
> +  nsCOMPtr<nsIDocument> destdoc = GetDocument();

same here.

::: editor/libeditor/TextEditorDataTransfer.cpp:244
(Diff revision 1)
> -      if (srcdomdoc == destdomdoc) {
> +      if (srcdoc == destdoc) {
>          return NS_OK;
>        }
>  
>        // Dragging from another window onto a selection
>        // XXX Decision made to NOT do this,
>        //     note that 4.x does replace if dropped on
>        //deleteSelection = true;
>      } else {
>        // We are NOT over the selection
> -      if (srcdomdoc == destdomdoc) {
> +      if (srcdoc == destdoc) {

Additionally, those checks can be done immediately after retrieving destdoc...

::: editor/libeditor/TextEditorDataTransfer.cpp:279
(Diff revision 1)
>        content = content->GetParent();
>      }
>    }
>  
>    for (uint32_t i = 0; i < numItems; ++i) {
> -    InsertFromDataTransfer(dataTransfer, i, srcdomdoc,
> +    InsertFromDataTransfer(dataTransfer, i, srcdoc,

srcdoc needs to be strong pointer here if numItems is  2 or more.

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

I think that you don't need |mozilla::| here.

::: editor/spellchecker/EditorSpellCheck.cpp:78
(Diff revision 1)
> -static nsresult
> -GetDocumentURI(nsIEditor* aEditor, nsIURI * *aURI)
> +static nsIURI*
> +GetDocumentURI(EditorBase* aEditor)
>  {
> -  NS_ENSURE_ARG_POINTER(aEditor);
> +  MOZ_ASSERT(aEditor);
> -  NS_ENSURE_ARG_POINTER(aURI);
> -
> -  nsCOMPtr<nsIDOMDocument> domDoc;
> -  aEditor->GetDocument(getter_AddRefs(domDoc));
> -  NS_ENSURE_TRUE(domDoc, NS_ERROR_FAILURE);
>  
> -  nsCOMPtr<nsIDocument> doc = do_QueryInterface(domDoc);
> -  NS_ENSURE_TRUE(doc, NS_ERROR_FAILURE);
> -
> -  nsCOMPtr<nsIURI> docUri = doc->GetDocumentURI();
> +  nsIDocument* doc = aEditor->AsEditorBase()->GetDocument();
> +  if (NS_WARN_IF(!doc)) {
> +    return nullptr;
> +  }
> -  NS_ENSURE_TRUE(docUri, NS_ERROR_FAILURE);
>  
> -  *aURI = docUri;
> +  return doc->GetDocumentURI();
> -  NS_ADDREF(*aURI);
> -  return NS_OK;
>  }
>  
> -static already_AddRefed<nsILoadContext>
> +static nsILoadContext*
>  GetLoadContext(nsIEditor* aEditor)
>  {
> -  nsCOMPtr<nsIDOMDocument> domDoc;
> -  aEditor->GetDocument(getter_AddRefs(domDoc));
> -  NS_ENSURE_TRUE(domDoc, nullptr);
> -
> +  nsIDocument* doc = aEditor->AsEditorBase()->GetDocument();
> +  if (NS_WARN_IF(!doc)) {
> +    return nullptr;
> +  }
> -  nsCOMPtr<nsIDocument> doc = do_QueryInterface(domDoc);
> -  NS_ENSURE_TRUE(doc, nullptr);
>  
> -  nsCOMPtr<nsILoadContext> loadContext = doc->GetLoadContext();
> +  return doc->GetLoadContext();
> -  return loadContext.forget();
>  }

I wonder, it's fine to implement those methods as member of EditorBase, up to you.

::: editor/spellchecker/EditorSpellCheck.cpp:162
(Diff revision 1)
>  {
>  public:
>    ContentPrefInitializerRunnable(nsIEditor* aEditor,
>                                   nsIContentPrefCallback2* aCallback)
>      : Runnable("ContentPrefInitializerRunnable")
> -    , mEditor(aEditor)
> +    , mEditor(aEditor->AsEditorBase())

Could you rename mEditor to mEditorBase? Then, it becomes clearer what is the type of the member.

::: editor/spellchecker/EditorSpellCheck.cpp:227
(Diff revision 1)
>  
>  /**
>   * Stores the current dictionary for aEditor's document URL.
>   */
>  static nsresult
> -StoreCurrentDictionary(nsIEditor* aEditor, const nsAString& aDictionary)
> +StoreCurrentDictionary(EditorBase* aEditor, const nsAString& aDictionary)

Could you rename aEditor to aEditorBase?

::: editor/spellchecker/EditorSpellCheck.cpp:259
(Diff revision 1)
>  
>  /**
>   * Forgets the current dictionary stored for aEditor's document URL.
>   */
>  static nsresult
> -ClearCurrentDictionary(nsIEditor* aEditor)
> +ClearCurrentDictionary(EditorBase* aEditor)

Could you rename aEditor to aEditorBase?

::: editor/spellchecker/TextServicesDocument.h:242
(Diff revision 1)
>  
>  private:
>    nsresult CreateContentIterator(nsRange* aRange,
>                                   nsIContentIterator** aIterator);
>  
> -  already_AddRefed<nsINode> GetDocumentContentRootNode();
> +  mozilla::dom::Element* GetDocumentContentRootNode() const;

I think that |mozilla::| is not necessary here.
Attachment #8965583 - Flags: review?(masayuki) → review+

Comment 3

a year ago
Pushed by m_kato@ga2.so-net.ne.jp:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d00dacc41af0
Remove more nsIDOMDocument usages from editor. r=masayuki

Comment 4

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d00dacc41af0
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.