1313986 - Add CreateAnonymousElement with nsIAtom

Status: RESOLVED FIXED

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

Description

[Makoto Kato [:m_kato]]

to clean up code...

Comment 1

[Makoto Kato [:m_kato]]

Also, no addon to use https://dxr.mozilla.org/addons/search?q=createAnonymousElement.  And c-c doesn't use it. https://dxr.mozilla.org/comm-central/search?q=createAnonymousElement&redirect=false

Comment 2

[Makoto Kato [:m_kato]]

https://hg.mozilla.org/try/pushloghtml?changeset=012fa1b89d893dfd364c8070e6415300efceec39

Comment 3

[Makoto Kato [:m_kato]]

Attachment: Bug 1313986 - Part 1. Add test for inlineTableEditing and objectResizing.

We have no mochitest for objectResizing and inlineTableEditing.  So I add simple test for this.

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

Comment 4

[Makoto Kato [:m_kato]]

Attachment: Bug 1313986 - Part 2. Add CreateAnonymousElement with nsIAtom.

I would like to nsIAtom and mozilla::dom::Element version of CreateAnonymousElement to clean up code.  When getting/setting attirubte, editor sometimes use string, not nsGkAtoms.  We should use new mozilla::dom::Element methods.

Also, we should add  _moz_anonclass to atom list that uses on editor.

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

Comment 5

[Makoto Kato [:m_kato]]

Attachment: Bug 1313986 - Part 3. Use CreateAnonymousElement with nsIAtom for simple case.

Now we can return Element directly via CreateAnonymousElement. We should use it.

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

Comment 6

[Makoto Kato [:m_kato]]

Attachment: Bug 1313986 - Part 4. Use Element instead of nsIDOMElement for resizer.

Resider and etc attributes on table editor still use nsIDOMElement.  To convert to Element, it make simple.

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

Comment 7

[Makoto Kato [:m_kato]]

Attachment: Bug 1313986 - Part 5. Remove createAnonymousElement from nsIHTMLEditor.

c-c and addons don't seem to use it.  We should remove this.

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

Comment 8

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

Comment on attachment 8813125 [details]
Bug 1313986 - Part 1. Add test for inlineTableEditing and objectResizing.

https://reviewboard.mozilla.org/r/94648/#review95130

::: editor/libeditor/tests/test_inlineTableEditing.html:34
(Diff revision 1)
> +  let tableNode = document.getElementById("table1");
> +  synthesizeMouseAtCenter(tableNode, {});
> +  isnot(tableNode.getAttribute("_moz_resizing"), "true",
> +         "_moz_resizing attribute shouldn't be true without object resizing");
> +
> +  ok(document.getElementById("tr2"), "id=tr2 must exist");

nit: s/must/should

::: editor/libeditor/tests/test_inlineTableEditing.html:36
(Diff revision 1)
> +  isnot(tableNode.getAttribute("_moz_resizing"), "true",
> +         "_moz_resizing attribute shouldn't be true without object resizing");
> +
> +  ok(document.getElementById("tr2"), "id=tr2 must exist");
> +  synthesizeMouse(tr2, 0, tr2.clientHeight / 2, {});
> +  ok(!document.getElementById("tr2"), "id=tr2 must be removed by button");

nit: s/must/should and "by a click in the row"?

::: editor/libeditor/tests/test_objectResizing.html:30
(Diff revision 1)
> +  document.execCommand("enableObjectResizing", false, "true");
> +
> +  let tableNode = document.getElementById("table1");
> +  synthesizeMouseAtCenter(tableNode, {});
> +  is(tableNode.getAttribute("_moz_resizing"), "true",
> +     "_moz_resizing attribute must be true with object resizing");

nit: s/must/should

::: editor/libeditor/tests/test_objectResizing.html:33
(Diff revision 1)
> +  synthesizeMouse(tableNode, 0, originalHeight, { type: "mousedown" });
> +  synthesizeMouse(tableNode, 0, originalHeight + 100, { type: "mousemove" });
> +  synthesizeMouse(tableNode, 0, originalHeight + 100, { type: "mouseup" });

I hope this won't cause random orange, though... (it might be unstable to use d&d.)

::: editor/libeditor/tests/test_objectResizing.html:37
(Diff revision 1)
> +  let originalHeight = tableNode.clientHeight;
> +  synthesizeMouse(tableNode, 0, originalHeight, { type: "mousedown" });
> +  synthesizeMouse(tableNode, 0, originalHeight + 100, { type: "mousemove" });
> +  synthesizeMouse(tableNode, 0, originalHeight + 100, { type: "mouseup" });
> +  isnot(originalHeight, tableNode.clientHeight,
> +        "table height must be changed by resizer");

nit: s/must/should and "by dragging a resizer grip"?

::: editor/libeditor/tests/test_objectResizing.html:44
(Diff revision 1)
> +  let originalWidth = tableNode.clientWidth;
> +  synthesizeMouse(tableNode, originalWidth, 0, { type: "mousedown" });
> +  synthesizeMouse(tableNode, originalWidth + 100, 0, { type: "mousemove" });
> +  synthesizeMouse(tableNode, originalWidth + 100, 0, { type: "mouseup" });
> +  isnot(originalWidth, tableNode.clientWidth,
> +        "table width must be changed by resizer");

nit: s/must/should and "by dragging a resizer grip"?

Comment 9

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

Comment on attachment 8813126 [details]
Bug 1313986 - Part 2. Add CreateAnonymousElement with nsIAtom.

https://reviewboard.mozilla.org/r/94650/#review95132

::: editor/libeditor/HTMLAnonymousNodeEditor.cpp:185
(Diff revision 1)
> +    CreateAnonymousElement(atom, aParentNode, aAnonClass, aIsCreatedHidden);
> +  if (NS_WARN_IF(!element)) {
> +    return NS_ERROR_FAILURE;
> +  }
> +  nsCOMPtr<nsIDOMElement> newElement =
> +    static_cast<nsIDOMElement*>(GetAsDOMNode(element));

Hmm, it might be better if there were "GetAsDOMElement()", but it's not scope of this bug.

::: editor/libeditor/HTMLAnonymousNodeEditor.cpp:200
(Diff revision 1)
> +{
> +  if (NS_WARN_IF(!aParentNode)) {
> +    return nullptr;
> +  }
> +
>    nsCOMPtr<nsIContent> parentContent( do_QueryInterface(aParentNode) );

Not your fault, though, could you rewrite this with "="?

FYI: 6th list item of https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#CC_practices

::: editor/libeditor/HTMLAnonymousNodeEditor.cpp:223
(Diff revision 1)
> +  nsresult rv;
>    if (aIsCreatedHidden) {
> -    nsresult rv = newElement->SetAttribute(NS_LITERAL_STRING("class"),
> +    rv = newContent->SetAttr(kNameSpaceID_None, nsGkAtoms::_class,

I like the old code better because it declares the variable in smaller scope. Smaller scope explicitly means that the variable won't be referred later.

::: editor/libeditor/HTMLEditor.h:1101
(Diff revision 1)
>                           InsertedOrAppended aInsertedOrAppended);
>    already_AddRefed<Element> GetElementOrParentByTagName(
>                                const nsAString& aTagName, nsINode* aNode);
>    already_AddRefed<Element> CreateElementWithDefaults(
>                                const nsAString& aTagName);
> +  already_AddRefed<Element> CreateAnonymousElement(

I'd like you to explain how to use this method for the other developers with

/**
 *
 */

style comment. It's really helpful if you document if aTag and aParentNode are nullable and what aAnonClass and aIsCreatedHidden mean.

Comment 10

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

Comment on attachment 8813127 [details]
Bug 1313986 - Part 3. Use CreateAnonymousElement with nsIAtom for simple case.

https://reviewboard.mozilla.org/r/94652/#review95136

::: editor/libeditor/HTMLAbsPositionEditor.cpp:238
(Diff revision 1)
>  
>  already_AddRefed<Element>
>  HTMLEditor::CreateGrabber(nsINode* aParentNode)
>  {
>    // let's create a grabber through the element factory
> -  nsCOMPtr<nsIDOMElement> retDOM;
> +  nsCOMPtr<Element> ret =

Use RefPtr for concrete class.

::: editor/libeditor/HTMLAbsPositionEditor.cpp:246
(Diff revision 1)
> -                         getter_AddRefs(retDOM));
> -
> -  NS_ENSURE_TRUE(retDOM, nullptr);
> +  if (NS_WARN_IF(!ret)) {
> +    return nullptr;
> +  }
>  
>    // add the mouse listener so we can detect a click on a resizer
> -  nsCOMPtr<nsIDOMEventTarget> evtTarget(do_QueryInterface(retDOM));
> +  nsCOMPtr<nsIDOMEventTarget> evtTarget(do_QueryInterface(ret));

I'd be happy if you'd rewrite this with "=".

::: editor/libeditor/HTMLEditorObjectResizer.cpp:141
(Diff revision 1)
>  
>  already_AddRefed<Element>
>  HTMLEditor::CreateResizer(int16_t aLocation,
>                            nsIDOMNode* aParentNode)
>  {
> -  nsCOMPtr<nsIDOMElement> retDOM;
> +  nsCOMPtr<Element> ret =

Use RefPtr.

::: editor/libeditor/HTMLEditorObjectResizer.cpp:185
(Diff revision 1)
> -  nsCOMPtr<Element> ret = do_QueryInterface(retDOM);
> +  nsresult rv;
>    rv = ret->SetAttr(kNameSpaceID_None, nsGkAtoms::anonlocation, locationStr,

nsresult rv =
  ret->SetAttr(...

::: editor/libeditor/HTMLEditorObjectResizer.cpp:203
(Diff revision 1)
>    if (HTMLEditUtils::IsImage(aOriginalObject)) {
> -    name.AssignLiteral("img");
> +    name = nsGkAtoms::img;
>    } else {
> -    name.AssignLiteral("span");
> +    name = nsGkAtoms::span;
>    }
> -  nsCOMPtr<nsIDOMElement> retDOM;
> +  nsCOMPtr<Element> ret =

RefPtr.

::: editor/libeditor/HTMLEditorObjectResizer.cpp:213
(Diff revision 1)
>  
>  already_AddRefed<Element>
>  HTMLEditor::CreateResizingInfo(nsIDOMNode* aParentNode)
>  {
>    // let's create an info box through the element factory
> -  nsCOMPtr<nsIDOMElement> retDOM;
> +  nsCOMPtr<Element> ret =

RefPtr.

Comment 11

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

Comment on attachment 8813128 [details]
Bug 1313986 - Part 4. Use Element instead of nsIDOMElement for resizer.

https://reviewboard.mozilla.org/r/94654/#review95138

::: editor/libeditor/HTMLEditor.h:1065
(Diff revision 1)
> -  nsCOMPtr<nsIDOMElement> mAddColumnBeforeButton;
> -  nsCOMPtr<nsIDOMElement> mRemoveColumnButton;
> -  nsCOMPtr<nsIDOMElement> mAddColumnAfterButton;
> -
> -  nsCOMPtr<nsIDOMElement> mAddRowBeforeButton;
> -  nsCOMPtr<nsIDOMElement> mRemoveRowButton;
> -  nsCOMPtr<nsIDOMElement> mAddRowAfterButton;
> +  nsCOMPtr<Element> mAddColumnBeforeButton;
> +  nsCOMPtr<Element> mRemoveColumnButton;
> +  nsCOMPtr<Element> mAddColumnAfterButton;
> +
> +  nsCOMPtr<Element> mAddRowBeforeButton;
> +  nsCOMPtr<Element> mRemoveRowButton;
> +  nsCOMPtr<Element> mAddRowAfterButton;

Use RefPtr.

::: editor/libeditor/HTMLInlineTableEditor.cpp:239
(Diff revision 1)
>  #ifdef DISABLE_TABLE_DELETION
> -  NS_NAMED_LITERAL_STRING(classStr, "class");
> -
>    if (colCount== 1) {
> -    mRemoveColumnButton->SetAttribute(classStr,
> -                                      NS_LITERAL_STRING("hidden"));
> +    mRemoveColumnButton->SetAttr(kNameSpaceID_None, nsGkAtoms::_class,
> +                                      NS_LITERAL_STRING("hidden"), true);

Please fix the odd indent.

::: editor/libeditor/HTMLInlineTableEditor.cpp:240
(Diff revision 1)
>    }
>    else {

Could you rewrite this as |} else {|?

::: editor/libeditor/HTMLInlineTableEditor.cpp:257
(Diff revision 1)
>    }
>    else {

Could you rewrite this as |} else {|?

Comment 12

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

Comment on attachment 8813128 [details]
Bug 1313986 - Part 4. Use Element instead of nsIDOMElement for resizer.

https://reviewboard.mozilla.org/r/94654/#review95140

> Bug 1313986 - Part 4. Use Element instead of nsIDOMElement for resider. r?masayuki

s/resider/resizer

>
> Resider and etc attributes on table editor still use nsIDOMElement.

s/Resider/Resizer

> To convert to Element, it make simple.

Perhaps, "Converting to Element makes both implementation and the callers simpler."?

Comment 13

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

Comment on attachment 8813129 [details]
Bug 1313986 - Part 5. Remove createAnonymousElement from nsIHTMLEditor.

https://reviewboard.mozilla.org/r/94656/#review95144

> c-c and addons don't seem to use it.

Perhaps, you meant "c-c nor add-ons seem not to use it". ("not" contradicts all following words, but perhaps, you don't contradict "seem".)

::: editor/libeditor/HTMLAnonymousNodeEditor.cpp:164
(Diff revision 1)
> -// Returns in *aReturn an anonymous nsDOMElement of type aTag,
> +// Returns an anonymous Element of type aTag,
>  // child of aParentNode. If aIsCreatedHidden is true, the class
>  // "hidden" is added to the created element. If aAnonClass is not
>  // the empty string, it becomes the value of the attribute "_moz_anonclass"

I think that we can get rid of this comment if you documents CreateAnonymousEelement() in the header file at part 2.

Comment 14

[Makoto Kato [:m_kato]]

Comment on attachment 8813125 [details]
Bug 1313986 - Part 1. Add test for inlineTableEditing and objectResizing.

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

Comment 15

[Makoto Kato [:m_kato]]

Comment on attachment 8813126 [details]
Bug 1313986 - Part 2. Add CreateAnonymousElement with nsIAtom.

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

Comment 16

[Makoto Kato [:m_kato]]

Comment on attachment 8813127 [details]
Bug 1313986 - Part 3. Use CreateAnonymousElement with nsIAtom for simple case.

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

Comment 17

[Makoto Kato [:m_kato]]

Comment on attachment 8813128 [details]
Bug 1313986 - Part 4. Use Element instead of nsIDOMElement for resizer.

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

Comment 18

[Makoto Kato [:m_kato]]

Comment on attachment 8813129 [details]
Bug 1313986 - Part 5. Remove createAnonymousElement from nsIHTMLEditor.

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

Comment 19

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

Comment on attachment 8813126 [details]
Bug 1313986 - Part 2. Add CreateAnonymousElement with nsIAtom.

https://reviewboard.mozilla.org/r/94650/#review95494

::: editor/libeditor/HTMLAnonymousNodeEditor.cpp:179
(Diff revision 2)
> -  NS_ENSURE_ARG_POINTER(aParentNode);
>    NS_ENSURE_ARG_POINTER(aReturn);
>    *aReturn = nullptr;
>  
> -  nsCOMPtr<nsIContent> parentContent( do_QueryInterface(aParentNode) );
> -  NS_ENSURE_TRUE(parentContent, NS_OK);
> +  nsCOMPtr<nsIAtom> atom = NS_Atomize(aTag);
> +  nsCOMPtr<Element> element =

RefPtr.

::: editor/libeditor/HTMLAnonymousNodeEditor.cpp:217
(Diff revision 2)
> -  NS_ENSURE_TRUE(ps, NS_ERROR_NOT_INITIALIZED);
> +  if (NS_WARN_IF(!ps)) {
> +    return nullptr;
> +  }
>  
>    // Create a new node through the element factory
> -  nsCOMPtr<nsIAtom> tagAtom = NS_Atomize(aTag);
> +  nsCOMPtr<Element> newContent = CreateHTMLContent(aTag);

RefPtr.

Comment 20

[Makoto Kato [:m_kato]]

Comment on attachment 8813126 [details]
Bug 1313986 - Part 2. Add CreateAnonymousElement with nsIAtom.

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

Comment 21

[Makoto Kato [:m_kato]]

Comment on attachment 8813127 [details]
Bug 1313986 - Part 3. Use CreateAnonymousElement with nsIAtom for simple case.

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

Comment 22

[Makoto Kato [:m_kato]]

Comment on attachment 8813128 [details]
Bug 1313986 - Part 4. Use Element instead of nsIDOMElement for resizer.

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

Comment 23

[Makoto Kato [:m_kato]]

Comment on attachment 8813129 [details]
Bug 1313986 - Part 5. Remove createAnonymousElement from nsIHTMLEditor.

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

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c72334e5c900
Part 1. Add test for inlineTableEditing and objectResizing. r=masayuki
https://hg.mozilla.org/integration/autoland/rev/4205c3ed2ce5
Part 2. Add CreateAnonymousElement with nsIAtom. r=masayuki
https://hg.mozilla.org/integration/autoland/rev/458c739c920a
Part 3. Use CreateAnonymousElement with nsIAtom for simple case. r=masayuki
https://hg.mozilla.org/integration/autoland/rev/f2593e79b289
Part 4. Use Element instead of nsIDOMElement for resizer. r=masayuki
https://hg.mozilla.org/integration/autoland/rev/efbc0c055fbc
Part 5. Remove createAnonymousElement from nsIHTMLEditor. r=masayuki

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c72334e5c900
https://hg.mozilla.org/mozilla-central/rev/4205c3ed2ce5
https://hg.mozilla.org/mozilla-central/rev/458c739c920a
https://hg.mozilla.org/mozilla-central/rev/f2593e79b289
https://hg.mozilla.org/mozilla-central/rev/efbc0c055fbc