Closed Bug 1451972 Opened 7 years ago Closed 7 years ago

Remove more nsIDOMDocument useages from editor

Categories

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

enhancement

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: nobody → m_kato
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
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: