Closed
Bug 1451972
Opened 7 years ago
Closed 7 years ago
Remove more nsIDOMDocument useages from editor
Categories
(Core :: DOM: Editor, enhancement, P3)
Core
DOM: Editor
Tracking
()
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: m_kato, Assigned: m_kato)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
to avoid QI and unnecessary addrefs
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → m_kato
Comment hidden (mozreview-request) |
Comment 2•7 years ago
|
||
mozreview-review |
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+
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•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Updated•5 years ago
|
Blocks: redesign-editor-scriptable-API
You need to log in
before you can comment on or make changes to this bug.
Description
•