Closed
Bug 1324996
Opened 7 years ago
Closed 7 years ago
Add nsIAtom version of SetAttribute/RemoveAttribute/CloneAttirubte to Editor
Categories
(Core :: DOM: Editor, defect, P3)
Core
DOM: Editor
Tracking
()
RESOLVED
FIXED
mozilla53
Tracking | Status | |
---|---|---|
firefox53 | --- | fixed |
People
(Reporter: m_kato, Assigned: m_kato)
References
Details
Attachments
(2 files)
To reduce string usage for attribute name, we should add nsIAtom version of SetAttribute/RemoveAttribute/CloneAttirubte.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 3•7 years ago
|
||
mozreview-review |
Comment on attachment 8820582 [details] Bug 1324996 - Part 1. Implement nsIAtom version of SetAttribute/RemoveAttribute/CloneAttirubte. https://reviewboard.mozilla.org/r/100066/#review100882 ::: editor/libeditor/EditorBase.cpp:1206 (Diff revision 1) > const nsAString& aAttribute, > const nsAString& aValue) > { > nsCOMPtr<Element> element = do_QueryInterface(aElement); > NS_ENSURE_TRUE(element, NS_ERROR_NULL_POINTER); > nsCOMPtr<nsIAtom> attribute = NS_Atomize(aAttribute); Hmm, looks like that when aAttribute is empty string, NS_Atomize() creates an Atom instance for it. However, I feel it doesn't make sense. Shouldn't we return error when aAttribute is empty string? ::: editor/libeditor/EditorBase.cpp:1248 (Diff revision 1) > EditorBase::RemoveAttribute(nsIDOMElement* aElement, > const nsAString& aAttribute) > { > nsCOMPtr<Element> element = do_QueryInterface(aElement); > NS_ENSURE_TRUE(element, NS_ERROR_NULL_POINTER); > nsCOMPtr<nsIAtom> attribute = NS_Atomize(aAttribute); Here too. ::: editor/libeditor/EditorBase.cpp:2203 (Diff revision 1) > > - nsCOMPtr<nsIDOMElement> destElement = do_QueryInterface(aDestNode); > - nsCOMPtr<nsIDOMElement> sourceElement = do_QueryInterface(aSourceNode); > + nsCOMPtr<Element> destElement = do_QueryInterface(aDestNode); > + nsCOMPtr<Element> sourceElement = do_QueryInterface(aSourceNode); > NS_ENSURE_TRUE(destElement && sourceElement, NS_ERROR_NO_INTERFACE); > > + nsCOMPtr<nsIAtom> attribute = NS_Atomize(aAttribute); And also here. ::: editor/libeditor/EditorBase.cpp:2212 (Diff revision 1) > +nsresult > +EditorBase::CloneAttribute(nsIAtom* aAttribute, > + Element* aDestElement, > + Element* aSourceElement) > +{ > + nsresult rv; You can remove this. ::: editor/libeditor/EditorBase.cpp:2215 (Diff revision 1) > + Element* aSourceElement) > +{ > + nsresult rv; > nsAutoString attrValue; > - bool isAttrSet; > - nsresult rv = GetAttributeValue(sourceElement, > + if (aSourceElement->GetAttr(kNameSpaceID_None, aAttribute, attrValue)) { > + rv = SetAttribute(aDestElement, aAttribute, attrValue); s/rv =/return ::: editor/libeditor/EditorBase.cpp:2216 (Diff revision 1) > } else { > - rv = RemoveAttribute(destElement, aAttribute); > + rv = RemoveAttribute(aDestElement, aAttribute); Then, remove else block and use return for RemoveAttribute(...). ::: editor/libeditor/EditorBase.cpp:4622 (Diff revision 1) > { > - return SetAttribute(aElement, aAttribute, aValue); > + nsCOMPtr<Element> element = do_QueryInterface(aElement); > + if (NS_WARN_IF(!element)) { > + return NS_ERROR_NULL_POINTER; > + } > + nsCOMPtr<nsIAtom> attribute = NS_Atomize(aAttribute); And here (if aAttribute is empty string). ::: editor/libeditor/HTMLEditor.h:132 (Diff revision 1) > + using EditorBase::RemoveAttributeOrEquivalent; > + using EditorBase::SetAttributeOrEquivalent; Why do we need these using statement? ::: editor/libeditor/HTMLEditor.cpp:4450 (Diff revision 1) > -HTMLEditor::SetAttributeOrEquivalent(nsIDOMElement* aElement, > - const nsAString& aAttribute, > +HTMLEditor::SetAttributeOrEquivalent(Element* aElement, > + nsIAtom* aAttribute, > const nsAString& aValue, > bool aSuppressTransaction) I guess that you assume that neither aElement nor aAttribute is nullptr. So, I think that as current code, putting MOZ_ASSERT()s here may help us. (Or using Element& and nsIAtom& is better?) ::: editor/libeditor/HTMLEditor.cpp:4504 (Diff revision 1) > -HTMLEditor::RemoveAttributeOrEquivalent(nsIDOMElement* aElement, > - const nsAString& aAttribute, > +HTMLEditor::RemoveAttributeOrEquivalent(Element* aElement, > + nsIAtom* aAttribute, > bool aSuppressTransaction) Same for this method too. I think that you should put MOZ_ASSERT()s here. ::: editor/libeditor/TextEditor.h:66 (Diff revision 1) > // Overrides of EditorBase interface methods > - NS_IMETHOD SetAttributeOrEquivalent(nsIDOMElement* aElement, > - const nsAString& aAttribute, > - const nsAString& aValue, > + virtual nsresult RemoveAttributeOrEquivalent( > + Element* aElement, > + nsIAtom* aAttribute, > - bool aSuppressTransaction) override; > + bool aSuppressTransaction) override; > - NS_IMETHOD RemoveAttributeOrEquivalent(nsIDOMElement* aElement, > - const nsAString& aAttribute, > + virtual nsresult SetAttributeOrEquivalent(Element* aElement, > + nsIAtom* aAttribute, > + const nsAString& aValue, > - bool aSuppressTransaction) override; > + bool aSuppressTransaction) override; These methods are not interface methods and we don't need to know if overriding methods from nsIEditor or EditorBase *from* comment. So, I think that the comment should be changed to "Overrides of EditorBase". ::: editor/libeditor/TextEditor.h:75 (Diff revision 1) > + using EditorBase::RemoveAttributeOrEquivalent; > + using EditorBase::SetAttributeOrEquivalent; Why do we need these using statement?
Attachment #8820582 -
Flags: review?(masayuki) → review+
Comment 4•7 years ago
|
||
mozreview-review |
Comment on attachment 8820583 [details] Bug 1324996 - Part 2. Use nsIAtom to change attirbute if possible. https://reviewboard.mozilla.org/r/100068/#review100888 ::: editor/libeditor/EditorBase.cpp:2259 (Diff revision 1) > > // Clear existing attributes > RefPtr<nsDOMAttributeMap> destAttributes = aDest->Attributes(); > while (RefPtr<Attr> attr = destAttributes->Item(0)) { > if (destInBody) { > - RemoveAttribute(static_cast<nsIDOMElement*>(GetAsDOMNode(aDest)), > + RemoveAttribute(aDest, attr->NodeInfo()->NameAtom()); I wonder, Attr should have NameAtom()? But it's out of scope of this bug.
Attachment #8820583 -
Flags: review?(masayuki) → review+
Assignee | ||
Comment 5•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8820582 [details] Bug 1324996 - Part 1. Implement nsIAtom version of SetAttribute/RemoveAttribute/CloneAttirubte. https://reviewboard.mozilla.org/r/100066/#review100882 > Hmm, looks like that when aAttribute is empty string, NS_Atomize() creates an Atom instance for it. However, I feel it doesn't make sense. Shouldn't we return error when aAttribute is empty string? OK, I should return NS_ERROR_FAILURE if empty string. > Why do we need these using statement? Recent g++ and clang require this for NS_IMETHOD SetAttributeOrEquivalent etc. See http://stackoverflow.com/questions/9995421/gcc-woverloaded-virtual-warnings > Why do we need these using statement? Recent g++ and clang require this for NS_IMETHOD SetAttributeOrEquivalent etc. See http://stackoverflow.com/questions/9995421/gcc-woverloaded-virtual-warnings
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Pushed by m_kato@ga2.so-net.ne.jp: https://hg.mozilla.org/integration/autoland/rev/5329971fb85c Part 1. Implement nsIAtom version of SetAttribute/RemoveAttribute/CloneAttirubte. r=masayuki https://hg.mozilla.org/integration/autoland/rev/487f977ef67d Part 2. Use nsIAtom to change attirbute if possible. r=masayuki
Comment 9•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/5329971fb85c https://hg.mozilla.org/mozilla-central/rev/487f977ef67d
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in
before you can comment on or make changes to this bug.
Description
•