1324996 - Add nsIAtom version of SetAttribute/RemoveAttribute/CloneAttirubte to Editor

Status: RESOLVED FIXED

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

Description

[Makoto Kato [:m_kato]]

To reduce string usage for attribute name, we should add nsIAtom version of SetAttribute/RemoveAttribute/CloneAttirubte.

Comment 1

[Makoto Kato [:m_kato]]

Attachment: Bug 1324996 - Part 1. Implement nsIAtom version of SetAttribute/RemoveAttribute/CloneAttirubte.

Add nsIAtom version of the following.
 - CloneAttribute
 - RemoveAttribute
 - RemoveAttributeOrEquivalent
 - SetAttribute
 - SetAttributeOrEquivalent

Review commit: https://reviewboard.mozilla.org/r/100066/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/100066/

Comment 2

[Makoto Kato [:m_kato]]

Attachment: Bug 1324996 - Part 2. Use nsIAtom to change attirbute if possible.

We can replace old nsIEditor API with nsIAtom version.

Review commit: https://reviewboard.mozilla.org/r/100068/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/100068/

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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?

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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.

Comment 5

[Makoto Kato [:m_kato]]

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 6

[Makoto Kato [:m_kato]]

Comment on attachment 8820582 [details]
Bug 1324996 - Part 1. Implement nsIAtom version of SetAttribute/RemoveAttribute/CloneAttirubte.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100066/diff/1-2/

Comment 7

[Makoto Kato [:m_kato]]

Comment on attachment 8820583 [details]
Bug 1324996 - Part 2. Use nsIAtom to change attirbute if possible.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100068/diff/1-2/

Comment 8

[Pulsebot]

Pushed by [e-mail]:
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

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5329971fb85c
https://hg.mozilla.org/mozilla-central/rev/487f977ef67d