Closed
Bug 1313986
Opened 6 years ago
Closed 6 years ago
Add CreateAnonymousElement with nsIAtom
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
(5 files)
58 bytes,
text/x-review-board-request
|
masayuki
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
masayuki
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
masayuki
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
masayuki
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
masayuki
:
review+
|
Details |
to clean up code...
Assignee | ||
Comment 1•6 years ago
|
||
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
Assignee | ||
Updated•6 years ago
|
Priority: -- → P3
Assignee | ||
Comment 2•6 years ago
|
||
https://hg.mozilla.org/try/pushloghtml?changeset=012fa1b89d893dfd364c8070e6415300efceec39
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 8•6 years ago
|
||
mozreview-review |
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 9•6 years ago
|
||
mozreview-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 10•6 years ago
|
||
mozreview-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 11•6 years ago
|
||
mozreview-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 12•6 years ago
|
||
mozreview-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 13•6 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 19•6 years ago
|
||
mozreview-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+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 24•6 years ago
|
||
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
Comment 25•6 years ago
|
||
bugherder |
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
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Updated•6 years ago
|
status-firefox52:
affected → ---
You need to log in
before you can comment on or make changes to this bug.
Description
•