811449 - Merge nsGenericElement and Element

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Reducing the ugly.

Comment 1

[Mounir Lamouri (:mounir)]

It probably doesn't need review yet ;)

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Move everything from nsGenericElement.h to Element.h. This is just moving code as-is, with absolutely no changes

outside of the Link bits that are needed to make mock_Link compile.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Combine the forward-declares and includes in Element.h. is just moving includes and forward-declares and some inline

implementations around.  No substantive changes.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Eliminate nsGenericElement as a separate class.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Move the data members of Element to the end of the class.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: part 5. Drop no longer needed namespacing on arguments and return values of Element methods.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: part 6. Rename nsGenericElement.cpp to Element.cpp.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: part 7. Get rid of nsGenericElement in Element.cpp.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: part 8. Get rid of nsGenericElement.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 8, but not crashing

Comment 11

[Peter Van der Beken [:peterv]]

Comment on attachment 681386 [details] [diff] [review]
part 1.  Move everything from nsGenericElement.h to Element.h.   This is just moving code as-is, with absolutely no changes

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

::: content/base/public/Element.h
@@ +412,5 @@
> +class nsDOMTokenList;
> +struct nsRect;
> +
> +already_AddRefed<nsContentList>
> +NS_GetContentList(nsINode* aRootNode, 

Trailing whitespace.

Comment 12

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 681388 [details] [diff] [review]
part 2.  Combine the forward-declares and includes in Element.h.   is just moving includes and forward-declares and some inline

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

::: content/base/public/Element.h
@@ +43,5 @@
> +#include "nsIScrollableFrame.h"
> +#include "nsIDOMAttr.h"
> +#include "nsISMILAttr.h"
> +#include "nsClientRect.h"
> +#include "nsIDOMDOMTokenList.h"

Just sort those alphabetically while you're taking blame :)

@@ +67,5 @@
>  class nsEventStateManager;
>  class nsFocusManager;
>  class nsGlobalWindow;
>  class nsICSSDeclaration;
>  class nsISMILAttr;

And hey, these too

Comment 13

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 681393 [details] [diff] [review]
part 7.  Get rid of nsGenericElement in Element.cpp.

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

::: content/base/src/Element.cpp
@@ +2513,1 @@
>                                       ErrorResult& aError)

Missed this

@@ +2542,1 @@
>                                   nsAttrValue& aResult)

And this

Comment 14

[Boris Zbarsky [:bzbarsky]]

> Just sort those alphabetically while you're taking blame :)

I'd really rather not, actually.  It increases risk, unfortunately.

Fixed the two indentation bits.

Comment 15

[Peter Van der Beken [:peterv]]

Comment on attachment 681401 [details] [diff] [review]
Part 8, but not crashing

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

::: content/base/src/nsDOMTokenList.h
@@ +34,1 @@
>    typedef mozilla::dom::Element Element;

As discussed, let's make this protected.

::: content/base/src/nsStyledElement.cpp
@@ +129,1 @@
>                                          aNotify);

Rewrap.

::: content/html/content/src/nsHTMLFrameSetElement.cpp
@@ +21,5 @@
>  }
>  
>  
> +NS_IMPL_ADDREF_INHERITED(nsHTMLFrameSetElement, Element) 
> +NS_IMPL_RELEASE_INHERITED(nsHTMLFrameSetElement, Element) 

Trailing whitespace.

::: content/html/content/src/nsHTMLHRElement.cpp
@@ +67,5 @@
>  }
>  
>  
> +NS_IMPL_ADDREF_INHERITED(nsHTMLHRElement, Element) 
> +NS_IMPL_RELEASE_INHERITED(nsHTMLHRElement, Element) 

Trailing whitespace.

::: content/html/content/src/nsHTMLSharedObjectElement.cpp
@@ +210,5 @@
>    nsObjectLoadingContent::Traverse(tmp, cb);
>  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END
>  
> +NS_IMPL_ADDREF_INHERITED(nsHTMLSharedObjectElement, Element) 
> +NS_IMPL_RELEASE_INHERITED(nsHTMLSharedObjectElement, Element) 

Trailing whitespace.

::: content/html/content/src/nsHTMLStyleElement.cpp
@@ +118,5 @@
>    tmp->nsStyleLinkElement::Unlink();
>  NS_IMPL_CYCLE_COLLECTION_UNLINK_END
>  
> +NS_IMPL_ADDREF_INHERITED(nsHTMLStyleElement, Element) 
> +NS_IMPL_RELEASE_INHERITED(nsHTMLStyleElement, Element) 

Trailing whitespace.

::: content/html/content/src/nsHTMLTableCellElement.cpp
@@ +78,5 @@
>  }
>  
>  
> +NS_IMPL_ADDREF_INHERITED(nsHTMLTableCellElement, Element) 
> +NS_IMPL_RELEASE_INHERITED(nsHTMLTableCellElement, Element) 

Trailing whitespace.

::: content/html/content/src/nsHTMLTableColElement.cpp
@@ +71,5 @@
>  }
>  
>  
> +NS_IMPL_ADDREF_INHERITED(nsHTMLTableColElement, Element) 
> +NS_IMPL_RELEASE_INHERITED(nsHTMLTableColElement, Element) 

Trailing whitespace.

::: content/html/content/src/nsHTMLTableElement.cpp
@@ +357,5 @@
>    NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR(mRows)
>  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END
>  
> +NS_IMPL_ADDREF_INHERITED(nsHTMLTableElement, Element) 
> +NS_IMPL_RELEASE_INHERITED(nsHTMLTableElement, Element) 

Trailing whitespace.

::: content/html/content/src/nsHTMLTableRowElement.cpp
@@ +83,5 @@
>                                                         nsIDOMNodeList)
>  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END
>  
> +NS_IMPL_ADDREF_INHERITED(nsHTMLTableRowElement, Element) 
> +NS_IMPL_RELEASE_INHERITED(nsHTMLTableRowElement, Element) 

Trailing whitespace.

::: content/html/content/src/nsHTMLTableSectionElement.cpp
@@ +79,5 @@
>                                                         nsIDOMNodeList)
>  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END
>  
> +NS_IMPL_ADDREF_INHERITED(nsHTMLTableSectionElement, Element) 
> +NS_IMPL_RELEASE_INHERITED(nsHTMLTableSectionElement, Element) 

Trailing whitespace.

::: content/html/content/src/nsHTMLTextAreaElement.cpp
@@ +310,5 @@
>    tmp->mState.Traverse(cb);
>  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END
>  
> +NS_IMPL_ADDREF_INHERITED(nsHTMLTextAreaElement, Element) 
> +NS_IMPL_RELEASE_INHERITED(nsHTMLTextAreaElement, Element) 

Trailing whitespace.

::: content/html/content/src/nsHTMLTitleElement.cpp
@@ +77,5 @@
>  }
>  
>  
> +NS_IMPL_ADDREF_INHERITED(nsHTMLTitleElement, Element) 
> +NS_IMPL_RELEASE_INHERITED(nsHTMLTitleElement, Element) 

Trailing whitespace.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Addressed the comments, and:

   https://hg.mozilla.org/integration/mozilla-inbound/rev/b86df83428cc
   https://hg.mozilla.org/integration/mozilla-inbound/rev/ff05c0955ca8
   https://hg.mozilla.org/integration/mozilla-inbound/rev/ea21cee0a151
   https://hg.mozilla.org/integration/mozilla-inbound/rev/01ba4b64e577
   https://hg.mozilla.org/integration/mozilla-inbound/rev/c939d4f18cc4
   https://hg.mozilla.org/integration/mozilla-inbound/rev/45c37804c654
   https://hg.mozilla.org/integration/mozilla-inbound/rev/87aa54d557dd
   https://hg.mozilla.org/integration/mozilla-inbound/rev/ac584cdc2b14

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b86df83428cc
https://hg.mozilla.org/mozilla-central/rev/ff05c0955ca8
https://hg.mozilla.org/mozilla-central/rev/ea21cee0a151
https://hg.mozilla.org/mozilla-central/rev/01ba4b64e577
https://hg.mozilla.org/mozilla-central/rev/c939d4f18cc4
https://hg.mozilla.org/mozilla-central/rev/45c37804c654
https://hg.mozilla.org/mozilla-central/rev/87aa54d557dd
https://hg.mozilla.org/mozilla-central/rev/ac584cdc2b14