1455674 - Remove nsIDOMElement

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P2)

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Remove the now-unnecessary nsITextControlElement::GetPlaceholderNode

MozReview-Commit-ID: FcVbeHEwkMz

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Remove the now-unnecessary nsITextControlElement::GetPreviewNode

MozReview-Commit-ID: 3goF5VXvQap

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Stop using nsIDOMElement in nsIEditor

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Mostly stop using nsIDOMElement in nsIHTMLEditor

MozReview-Commit-ID: Koxv2qS2gft

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Stop using nsIDOMElement in nsITableEditor

The patch is a bit large because all these methods except
SswitchTableCellHeaderType call each other; doing it piecemeal would have
required introducing, then removing, a bunch of QIs.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Stop using nsIDOMElement in nsIHTMLEditor::GetElementOrParentByTagName

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: part 5. Remove remaining IDL use of nsIDOMElement in editor

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: part 6. Mostly remove use of nsIDOMElement in editor

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: part 7. Simplify HTMLEditor::GetFocusedNode

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: part 8. Remove nsIDOMElement use from nsIDOMWindowUtils

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: part 9. Remove nsIDOMElement use from nsIFocusManager

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: part 10. Remove nsIDOMElement use from remaining dom/ xpidl files

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: part 11. Remove nsIDOMElement use from xpidl in toolkit

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: part 12. Remove nsIDOMElement use from xpidl in layout

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: part 13. Remove remaining xpidl uses of nsIDOMElement

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: part 14. Remove use of nsIDOMElement in layout

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: part 15. Remove use of nsIDOMElement in accessible

Comment 18

[Boris Zbarsky [:bzbarsky]]

Attachment: part 16. Remove most use of nsIDOMElement in dom

Comment 19

[Boris Zbarsky [:bzbarsky]]

Attachment: part 17. Remove use of nsIDOMElement in non-dom non-JS code

Comment 20

[Boris Zbarsky [:bzbarsky]]

Attachment: part 18. Add Element to importGlobalProperties

Comment 21

[Boris Zbarsky [:bzbarsky]]

Attachment: part 19. Get rid of JS uses of nsIDOMElement

In nsBlocklistService.js we know we're dealing with actual nodes, so .nodeType
checks should be fine.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Attachment: part 20. Get rid of nsIDOMElement

Comment 23

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 9/19-9/27)]

Comment on attachment 8971115 [details] [diff] [review]
part 1.  Stop using nsIDOMElement in nsIEditor

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

::: toolkit/components/find/nsFind.cpp
@@ +393,1 @@
>    textEditor->GetRootElement(getter_AddRefs(rootElement));

textEditor is mozilla::TextEditor. So, you must be able to write this as:

RefPtr<dom::Element> rootElement = textEditor->GetRoot();

(Perhaps, |Element*| is fine since innerRange isn't associated with Selection. So, its change won't cause destroying root element of textEditor. But up to you.)

Comment 24

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 9/19-9/27)]

Comment on attachment 8971116 [details] [diff] [review]
part 2.  Mostly stop using nsIDOMElement in nsIHTMLEditor

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

::: editor/composer/nsComposerCommands.cpp
@@ +1454,5 @@
>    if (NS_WARN_IF(!htmlEditor)) {
>      return NS_ERROR_FAILURE;
>    }
>  
> +  RefPtr<Element> domElem;

Could you rename domElem to newElement or something? We use "dom" prefix for nsIDOM* variables.

::: editor/libeditor/HTMLEditor.cpp
@@ +1583,5 @@
>        }
>  
>        EditorDOMPoint insertedPoint =
>          InsertNodeIntoProperAncestorWithTransaction(
> +          *aElement, pointToInsert, SplitAtEdges::eAllowToCreateEmptyContainer);

Although, aElement is not grabbed with local variable in this method but must be fine since all callers of this method grabs aElement before calling this.

@@ +3630,4 @@
>  {
> +  if (!aElement || !aElement->IsHTMLElement() ||
> +      !HTMLEditUtils::IsTableElement(aElement) ||
> +      !IsDescendantOfEditorRoot(aElement)) {

Selection::Collapse() will grab given node before changing selection actually. So, not grabbing aElement is safe.

Comment 25

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 9/19-9/27)]

Comment on attachment 8971117 [details] [diff] [review]
part 3.  Stop using nsIDOMElement in nsITableEditor

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

::: editor/libeditor/HTMLTableEditor.cpp
@@ +86,5 @@
>    }
>  };
>  
>  nsresult
> +HTMLEditor::InsertCell(Element* aDOMCell,

Could you rename this to aCell or aCellElement?

@@ +91,5 @@
>                         int32_t aRowSpan,
>                         int32_t aColSpan,
>                         bool aAfter,
>                         bool aIsHeader,
> +                       Element** aNewDOMCell)

And also rename this to aNewCell or aNewCellElement.

Comment 26

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 9/19-9/27)]

Comment on attachment 8971118 [details] [diff] [review]
part 4.  Stop using nsIDOMElement in nsIHTMLEditor::GetElementOrParentByTagName

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

::: editor/libeditor/HTMLEditor.cpp
@@ +2444,5 @@
>  {
>    NS_ENSURE_TRUE(!aTagName.IsEmpty(), NS_ERROR_NULL_POINTER);
>    NS_ENSURE_TRUE(aReturn, NS_ERROR_NULL_POINTER);
>  
> +  nsCOMPtr<Element> parent = GetElementOrParentByTagName(aTagName, aNode);

RefPtr<Element>?

Comment 27

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 9/19-9/27)]

Comment on attachment 8971119 [details] [diff] [review]
part 5.  Remove remaining IDL use of nsIDOMElement in editor

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

::: editor/libeditor/HTMLEditorObjectResizer.cpp
@@ +934,2 @@
>  {
> +  nsCOMPtr<Element> ret = mResizedObject;

RefPtr<Element>?

Comment 28

[Boris Zbarsky [:bzbarsky]]

> RefPtr<dom::Element> rootElement = textEditor->GetRoot();

Done.

> Could you rename domElem to newElement or something? We use "dom" prefix for nsIDOM* variables.

Done.

> Could you rename this to aCell or aCellElement?
> And also rename this to aNewCell or aNewCellElement.

Done, with aCell/aNewCell.

> RefPtr<Element>?

Done.

> RefPtr<Element>?

Done.  I had to make mResizedObject a RefPtr too, to get things to compile without the code being too ugly.

Comment 29

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8971124 [details] [diff] [review]
part 10.  Remove nsIDOMElement use from remaining dom/ xpidl files

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

::: dom/base/nsContentPermissionHelper.cpp
@@ +645,5 @@
>    return NS_OK;
>  }
>  
>  NS_IMETHODIMP
> +nsContentPermissionRequestProxy::GetElement(Element * *aRequestingElement)

nit: Might fix the weird space between **'s while this is happening?

::: dom/geolocation/nsGeolocation.cpp
@@ +395,5 @@
>    return NS_OK;
>  }
>  
>  NS_IMETHODIMP
> +nsGeolocationRequest::GetElement(Element * *aRequestingElement)

nit: Might fix the weird space between **'s while this is happening?

Comment 30

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8971127 [details] [diff] [review]
part 13.  Remove remaining xpidl uses of nsIDOMElement

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

::: widget/cocoa/nsStandaloneNativeMenu.mm
@@ +34,5 @@
>  {
>    NS_ASSERTION(mMenu == nullptr, "nsNativeMenu::Init - mMenu not null!");
>  
> +  NS_ENSURE_ARG(aElement);
> +  

nit: extra whitespace

Comment 31

[Boris Zbarsky [:bzbarsky]]

> nit: Might fix the weird space between **'s while this is happening?

Done, both places.

> nit: extra whitespace

Fixed.

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec41eab9c159
part 1.  Stop using nsIDOMElement in nsIEditor.  r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/52e2af8c5e62
part 2.  Mostly stop using nsIDOMElement in nsIHTMLEditor.  r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/667c981474f8
part 3.  Stop using nsIDOMElement in nsITableEditor.  r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/15071a2629d9
part 4.  Stop using nsIDOMElement in nsIHTMLEditor::GetElementOrParentByTagName.  r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/f283a9ca271b
part 5.  Remove remaining IDL use of nsIDOMElement in editor.  r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae8e688883d5
part 6.  Mostly remove use of nsIDOMElement in editor.  r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/36a91270aaf8
part 7.  Simplify HTMLEditor::GetFocusedNode.  r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/d6865d4b5520
part 8.  Remove nsIDOMElement use from nsIDOMWindowUtils.  r=qdot
https://hg.mozilla.org/integration/mozilla-inbound/rev/8311c9e2e414
part 9.  Remove nsIDOMElement use from nsIFocusManager.  r=qdot
https://hg.mozilla.org/integration/mozilla-inbound/rev/67be83ace312
part 10.  Remove nsIDOMElement use from remaining dom/ xpidl files.  r=qdot
https://hg.mozilla.org/integration/mozilla-inbound/rev/a19af55d0bd2
part 11.  Remove nsIDOMElement use from xpidl in toolkit.  r=mossop
https://hg.mozilla.org/integration/mozilla-inbound/rev/5add57b9a0cc
part 12.  Remove nsIDOMElement use from xpidl in layout.  r=mossop
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f3223b376bb
part 13.  Remove remaining xpidl uses of nsIDOMElement.  r=qdot
https://hg.mozilla.org/integration/mozilla-inbound/rev/995b14b30559
part 14.  Remove use of nsIDOMElement in layout.  r=qdot
https://hg.mozilla.org/integration/mozilla-inbound/rev/e165e82a8e85
part 15.  Remove use of nsIDOMElement in accessible.  r=qdot
https://hg.mozilla.org/integration/mozilla-inbound/rev/c7b96bb087f0
part 16.  Remove most use of nsIDOMElement in dom.  r=qdot
https://hg.mozilla.org/integration/mozilla-inbound/rev/f4428b451bba
part 17.  Remove use of nsIDOMElement in non-dom non-JS code.  r=qdot
https://hg.mozilla.org/integration/mozilla-inbound/rev/c29505c32af1
part 18.  Add Element to importGlobalProperties.  r=qdot
https://hg.mozilla.org/integration/mozilla-inbound/rev/c75434fd5100
part 19.  Get rid of JS uses of nsIDOMElement.  r=qdot
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d80f0cc731f
part 20.  Get rid of nsIDOMElement.  r=qdot

Comment 33

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/ec41eab9c159
https://hg.mozilla.org/mozilla-central/rev/52e2af8c5e62
https://hg.mozilla.org/mozilla-central/rev/667c981474f8
https://hg.mozilla.org/mozilla-central/rev/15071a2629d9
https://hg.mozilla.org/mozilla-central/rev/f283a9ca271b
https://hg.mozilla.org/mozilla-central/rev/ae8e688883d5
https://hg.mozilla.org/mozilla-central/rev/36a91270aaf8
https://hg.mozilla.org/mozilla-central/rev/d6865d4b5520
https://hg.mozilla.org/mozilla-central/rev/8311c9e2e414
https://hg.mozilla.org/mozilla-central/rev/67be83ace312
https://hg.mozilla.org/mozilla-central/rev/a19af55d0bd2
https://hg.mozilla.org/mozilla-central/rev/5add57b9a0cc
https://hg.mozilla.org/mozilla-central/rev/7f3223b376bb
https://hg.mozilla.org/mozilla-central/rev/995b14b30559
https://hg.mozilla.org/mozilla-central/rev/e165e82a8e85
https://hg.mozilla.org/mozilla-central/rev/c7b96bb087f0
https://hg.mozilla.org/mozilla-central/rev/f4428b451bba
https://hg.mozilla.org/mozilla-central/rev/c29505c32af1
https://hg.mozilla.org/mozilla-central/rev/c75434fd5100
https://hg.mozilla.org/mozilla-central/rev/2d80f0cc731f