1053779 - Clean up nsEditor::CreateHTMLContent, ReplaceContainer, CloneAttributes

Status: RESOLVED FIXED

(Core :: DOM: Editor, enhancement)

Comment 1

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch part 1 -- Clean up nsEditor::CreateHTMLContent

Comment 2

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch part 2 -- Clean up nsEditor::ReplaceContainer

Comment 3

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch part 3 -- Clean up nsEditor::CloneAttributes

Comment 4

[(no longer active)]

Comment on attachment 8473074 [details] [diff] [review]
Patch part 2 -- Clean up nsEditor::ReplaceContainer

Review of attachment 8473074 [details] [diff] [review]:
-----------------------------------------------------------------

::: editor/libeditor/html/nsTableEditor.cpp
@@ +1957,5 @@
>    nsAutoSelectionReset selectionResetter(selection, this);
>  
>    // Set to the opposite of current type
>    nsCOMPtr<nsIAtom> atom = nsEditor::GetTag(aSourceCell);
> +  nsCOMPtr<nsIAtom> newCellType = atom == nsEditProperty::td

Nit: nsIAtom*.

Comment 5

[(no longer active)]

Comment on attachment 8473119 [details] [diff] [review]
Patch part 3 -- Clean up nsEditor::CloneAttributes

Review of attachment 8473119 [details] [diff] [review]:
-----------------------------------------------------------------

::: editor/libeditor/base/nsEditor.cpp
@@ +2245,5 @@
> +  // Clear existing attributes
> +  nsRefPtr<nsDOMAttributeMap> destAttributes = aDest->Attributes();
> +  while (nsRefPtr<Attr> attr = destAttributes->Item(0)) {
> +    if (destInBody) {
> +      RemoveAttribute(static_cast<nsIDOMElement*>(GetAsDOMNode(aDest)),

These static casts are unfortunately not safe because nsIDOMElement is not builtinclass.  Please QI instead.

Comment 6

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #5)
> These static casts are unfortunately not safe because nsIDOMElement is not
> builtinclass.  Please QI instead.

Hrm?  aDest is an Element, and every Element is an nsIDOMElement, isn't it?  I understand why nsIDOMElement can't be cast to Element, because maybe someone passed in a weird made-up user class, but I thought the other way was safe, just like nsINode to nsIDOMNode is safe.

Comment 7

[(no longer active)]

Hmm, I was worried about this function which is exposed to JS consumers: <http://mxr.mozilla.org/mozilla-central/source/editor/nsIEditor.idl#398>  But on a second look, the other overload of CloneAttributes will return early if you pass it such made up JS objects.  So, I guess this is actually fine.  Sorry for the confusion, this stuff is hard to reason about.  :/

Comment 8

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Try: https://tbpl.mozilla.org/?tree=Try&rev=52ee018a8367

https://hg.mozilla.org/integration/mozilla-inbound/rev/ef56950fd9ae
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a321b56d982
https://hg.mozilla.org/integration/mozilla-inbound/rev/d4f36554d461

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ef56950fd9ae
https://hg.mozilla.org/mozilla-central/rev/6a321b56d982
https://hg.mozilla.org/mozilla-central/rev/d4f36554d461