Add nsIAtom version of SetAttribute/RemoveAttribute/CloneAttirubte to Editor

RESOLVED FIXED in Firefox 53

Status

()

P3
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: m_kato, Assigned: m_kato)

Tracking

Trunk
mozilla53
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Assignee)

Description

2 years ago
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

2 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

2 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

2 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)

Comment 8

2 years ago
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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5329971fb85c
https://hg.mozilla.org/mozilla-central/rev/487f977ef67d
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox53: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Depends on: 1350772
You need to log in before you can comment on or make changes to this bug.