Closed Bug 1313986 Opened 4 years ago Closed 4 years ago

Add CreateAnonymousElement with nsIAtom

Categories

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

Product:
Core
Component:
DOM: Editor
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: m_kato, Assigned: m_kato)

References

Details

Attachments

(5 files)

Bug 1313986 - Part 1. Add test for inlineTableEditing and objectResizing.
58 bytes, text/x-review-board-request
masayuki
: review+
Details
Bug 1313986 - Part 2. Add CreateAnonymousElement with nsIAtom.
58 bytes, text/x-review-board-request
masayuki
: review+
Details
Bug 1313986 - Part 3. Use CreateAnonymousElement with nsIAtom for simple case.
58 bytes, text/x-review-board-request
masayuki
: review+
Details
Bug 1313986 - Part 4. Use Element instead of nsIDOMElement for resizer.
58 bytes, text/x-review-board-request
masayuki
: review+
Details
Bug 1313986 - Part 5. Remove createAnonymousElement from nsIHTMLEditor.
58 bytes, text/x-review-board-request
masayuki
: review+
Details 
to clean up code...
Priority: -- → P3
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"?
Attachment #8813125 - Flags: review?(masayuki) → review+
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.
Attachment #8813126 - Flags: review?(masayuki) → review-
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.
Attachment #8813127 - Flags: review?(masayuki) → review+
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 {|?
Attachment #8813128 - Flags: review?(masayuki) → review+
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 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.
Attachment #8813129 - Flags: review?(masayuki) → review+
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.
Attachment #8813126 - Flags: review?(masayuki) → review+
Pushed by m_kato@ga2.so-net.ne.jp:
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
Regressions: 1663638
You need to log in before you can comment on or make changes to this bug.