226522 - More nsIContent and nsIDocument deCOMtamination...

Status: RESOLVED FIXED

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

Description

[Johnny Stenback (:jst)]

Should be the last big one, this one eliminates all (most) NS_IMETHOD* macros
from nsIContent and nsIDocument, and cleans up some final things (renames
[G|S]et*URL() to [G|S]et*URI() to reflect the types actually used). Huge patch
coming up.

Comment 1

[Johnny Stenback (:jst)]

Oh, and while I was at it, I eliminated nsIAttributeContent...

Comment 2

[Johnny Stenback (:jst)]

Attachment: No more NS_IMETHOD* in nsIContent and nsIDocument (gzip)

Comment 3

[Peter Van der Beken [:peterv]]

Comment on attachment 136188 [details]
No more NS_IMETHOD* in nsIContent and nsIDocument (gzip)

> Index: content/base/public/nsIContent.h
> ===================================================================
> RCS file: /cvsroot/mozilla/content/base/public/nsIContent.h,v
> retrieving revision 3.76
> diff -u -9 -p -w -r3.76 nsIContent.h
> --- content/base/public/nsIContent.h	19 Nov 2003 01:20:29 -0000	3.76
> +++ content/base/public/nsIContent.h	22 Nov 2003 19:55:05 -0000
> @@ -70,311 +70,311 @@ class nsIURI;
>  class nsIContent : public nsISupports {
>  public:
>    NS_DEFINE_STATIC_IID_ACCESSOR(NS_ICONTENT_IID)
>  
>    nsIContent()
>      : mDocument(nsnull), mParentPtrBits(0) { }
>  
>    /**
>     * Get the document for this content.
> -   * @return the document
> +   * @returns the document

Don't do this, the javadoc syntax is @return.

>     */
>    nsIDocument* GetDocument() const { return mDocument; }
>  

>    /**
>     * Get the parent content for this content.
> -   * @return the parent, or null if no parent
> +   * @returns the parent, or null if no parent

Same here.

>     */
>    nsIContent* GetParent() const
>    {
>      return NS_REINTERPRET_CAST(nsIContent *, mParentPtrBits & ~kParentBitMask);
>    }

>    /**
>     * Get the namespace that this element's tag is defined in
>     * @param aResult the namespace [OUT]
>     */
> -  NS_IMETHOD GetNameSpaceID(PRInt32* aResult) const = 0;
> +  virtual void GetNameSpaceID(PRInt32* aResult) const = 0;

We really should make this return PRInt32.

>    /**
>     * Get the NodeInfo for this element
> -   * @param aResult the tag [OUT]
> +   * @returns the nodes node info
>     */

@return

> -  NS_IMETHOD_(nsINodeInfo *) GetNodeInfo() const = 0;
> +  virtual nsINodeInfo * GetNodeInfo() const = 0;
>  
>    /**
>     * Tell whether this element can contain children
> -   * @param aResult whether this element can contain children [OUT]
> +   * @returns whether this element can contain children

@return

>     */
> -  NS_IMETHOD_(PRBool) CanContainChildren() const = 0;
> +  virtual PRBool CanContainChildren() const = 0;
>  
>    /**
>     * Get the number of children
> -   * @param aResult the number of children [OUT]
> +   * @returns the number of children

@return

>     */
> -  NS_IMETHOD_(PRUint32) GetChildCount() const = 0;
> +  virtual PRUint32 GetChildCount() const = 0;
>  
>    /**
>     * Get a child by index
> -   * @param aIndex the index of the child to get, or null if index out of bounds
> -   * @param aResult the child [OUT]
> +   * @param aIndex the index of the child to get, or null if index out
> +   * of bounds

Align "of" with "the".

> +   * @returns the child

@return

>     */
> -  NS_IMETHOD_(nsIContent *) GetChildAt(PRUint32 aIndex) const = 0;
> +  virtual nsIContent *GetChildAt(PRUint32 aIndex) const = 0;
>  
>    /**
>     * Get the index of a child within this content
>     * @param aPossibleChild the child to get the index
> -   * @param aIndex the index of the child, or -1 if not a child [OUT]
> +   * @returns the index of the child, or -1 if not a child

@return

>     */
> -  NS_IMETHOD_(PRInt32) IndexOf(nsIContent* aPossibleChild) const = 0;
> +  virtual PRInt32 IndexOf(nsIContent* aPossibleChild) const = 0;
>  
>    /**
>     * Determine if an attribute has been set (empty string or otherwise).
>     *
>     * @param aNameSpaceId the namespace id of the attribute
>     * @param aAttr the attribute name
> -   * @return whether an attribute exists
> +   * @returns whether an attribute exists

@return

>     */
> -  NS_IMETHOD_(PRBool) HasAttr(PRInt32 aNameSpaceID, nsIAtom* aName) const = 0;
> +  virtual PRBool HasAttr(PRInt32 aNameSpaceID, nsIAtom* aName) const = 0;
>  

>    /**
>     * Get the number of all specified attributes.
>     *
>     * @returns the number of attributes

@return

>     */
> -  NS_IMETHOD_(PRUint32) GetAttrCount() const = 0;
> +  virtual PRUint32 GetAttrCount() const = 0;

>    /**
>     * Get the base URL for any relative URLs within this piece
>     * of content. Generally, this is the document's base URL,
>     * but certain content carries a local base for backward
>     * compatibility.
>     *
> -   * @param aBaseURL the base URL [OUT]
> +   * @returns the base URI

@return

>     */
> -  NS_IMETHOD GetBaseURL(nsIURI** aBaseURL) const = 0;
> +  virtual already_AddRefed<nsIURI> GetBaseURI() const = 0;


> Index: content/html/content/src/nsFormSubmission.cpp
> ===================================================================

> @@ -1064,25 +1064,25 @@ static nsresult
>  SendJSWarning(nsIHTMLContent* aContent,
>                const nsAFlatString& aWarningName,
>                const PRUnichar** aWarningArgs, PRUint32 aWarningArgsLen)
>  {
>    nsresult rv = NS_OK;
>  
>    //
>    // Get the document URL to use as the filename
>    //
> -  nsCAutoString documentURLSpec;
> +  nsCAutoString documentURISpec;
>    {
>      nsIDocument* document = aContent->GetDocument();
>      if (document) {
> -      nsIURI *documentURL = document->GetDocumentURL();
> -      NS_ENSURE_TRUE(documentURL, NS_ERROR_UNEXPECTED);
> -      documentURL->GetPath(documentURLSpec);
> +      nsIURI *documentURI = document->GetDocumentURI();
> +      NS_ENSURE_TRUE(documentURI, NS_ERROR_UNEXPECTED);
> +      documentURI->GetPath(documentURISpec);
>      }
>    }

Seems these outer braces are unneeded?


> @@ -1102,19 +1102,19 @@ SendJSWarning(nsIHTMLContent* aContent,

>    rv = scriptError->Init(warningStr.get(),
> -                         NS_ConvertUTF8toUCS2(documentURLSpec).get(),
> +                         NS_ConvertUTF8toUCS2(documentURISpec).get(),

Make that toUTF16 while you're there.

> Index: content/html/content/src/nsGenericHTMLElement.h
> ===================================================================

> -
> +  // nsIDOM??? NS_METHOD???
>    NS_METHOD SetAttribute(const nsAString& aName,
>                           const nsAString& aValue)
>    {
>      return nsGenericHTMLElement::SetAttribute(aName, aValue);
>    }

What's with the comment?

> +  // nsIDOM??? NS_METHOD???
>    NS_METHOD SetAttribute(const nsAString& aName,
>                           const nsAString& aValue)
>    {
>      return nsGenericHTMLElement::SetAttribute(aName, aValue);
>    }

And here?

> Index: content/html/content/src/nsHTMLLabelElement.cpp
> ===================================================================

> +void
>  nsHTMLLabelElement::SetFocus(nsIPresContext* aContext)
>  {
>    // Since we don't have '-moz-user-focus: normal', the only time
>    // |SetFocus| will be called is when the accesskey is activated.
>    nsCOMPtr<nsIContent> content = GetForContent();
>    if (content)
>      return content->SetFocus(aContext);

No need for the return.

> Index: content/html/style/src/nsDOMCSSAttrDeclaration.cpp
> ===================================================================

> -  mContent->GetBaseURL(aBaseURI);
> +  nsCOMPtr<nsIURI> base = mContent->GetBaseURI();

> +
> +  NS_IF_ADDREF(*aBaseURI = base);

You could do

  nsCOMPtr<nsIURI> base = mContent->GetBaseURI();
  base.swap(*aBaseURI);

> Index: content/xbl/src/nsXBLBinding.cpp
> ===================================================================

> +PRBool PR_CALLBACK
> +BuildContentLists(nsHashKey* aKey, void* aData, void* aClosure)

I suppose these could be PR_STATIC_CALLBACK(PRBool)?

> Index: layout/html/style/src/nsCSSFrameConstructor.cpp
> ===================================================================

> @@ -1412,19 +1411,19 @@ nsCSSFrameConstructor::CreateGeneratedFr

>      case eStyleContentType_Attr:
>        {  
> -        nsIAtom* attrName = nsnull;
> +        nsCOMPtr<nsIAtom> attrName;
>          PRInt32 attrNameSpace = kNameSpaceID_None;
>          PRInt32 barIndex = contentString.FindChar('|'); // CSS namespace delimiter
>          if (-1 != barIndex) {
>            nsAutoString  nameSpaceVal;
>            contentString.Left(nameSpaceVal, barIndex);
>            PRInt32 error;
>            attrNameSpace = nameSpaceVal.ToInteger(&error, 10);
>            contentString.Cut(0, barIndex + 1);
>            if (contentString.Length()) {
> @@ -1434,40 +1433,35 @@ nsCSSFrameConstructor::CreateGeneratedFr
>          else {
>            attrName = NS_NewAtom(contentString);

You need to change this and the other NS_NewAtom's to do_GetAtom, you're
leaking attrName.

> Index: xpfe/appshell/src/nsWebShellWindow.cpp
> ===================================================================

> @@ -1410,26 +1410,26 @@ void nsWebShellWindow::LoadContentAreas(

> +      nsIURI *mainURL = doc->GetDocumentURI();
> +
>          nsCAutoString search;
>          nsCOMPtr<nsIURL> url = do_QueryInterface(mainURL);
>          if (url)
>            url->GetQuery(search);
> -        searchSpec = NS_ConvertUTF8toUCS2(search);
> -      }
> +
> +      CopyUTF8toUTF16(search, searchSpec);

You could move the CopyUTF8toUTF16 inside the if (url)

IMHO we should have a macro to provide stubs for CanContainChildren and all the
other child-related methods for content nodes that don't have children. Oh, and
some other macros to provide the declarations so I don't have to review 15000
lines next time you change some methods ;-).

Comment 4

[Johnny Stenback (:jst)]

Thanks for the review, made all those changes.

The whole content-elements-that-can't-have-children mess is going away (i.e. all
elements will be able to hold children, as they should be), so
CanContainChildren() should go away too, IMO. So I don't know what macros we'd
need after that...

Comment 5

[Johnny Stenback (:jst)]

Attachment: Final patch that was checked in.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This caused a codesize increase on Windows because it increased the size of the
vtables for all the HTML content nodes by 108 bytes.  I haven't tried to figure
out why that happened yet.

Comment 7

[Johnny Stenback (:jst)]

This accidentally made nsGenericHTMLElement::CopyInnerTo() virtual, so that
caused some code increase. That's fixed now, waiting to see what the result of
that change was...

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

On Linux, the nsGeneric* were increased by 108 bytes and the nsHTML*Element were
inccreased by only 68 bytes.  Comment 7 describes 4 bytes worth of it, and the
rest of it was due to:

+++ nsGeneric.vt.sort.new       2004-01-09 19:42:28.000000000 -0800
+       .long   _ZN16nsGenericElement4InitEP11nsINodeInfo
+       .long   _ZN20nsGenericHTMLElement12GetClassNameER9nsAString
+       .long   _ZN20nsGenericHTMLElement12GetOffsetTopEPi
+       .long   _ZN20nsGenericHTMLElement12GetScrollTopEPi
+       .long   _ZN20nsGenericHTMLElement12SetClassNameERK9nsAString
+       .long   _ZN20nsGenericHTMLElement12SetScrollTopEi
+       .long   _ZN20nsGenericHTMLElement13GetOffsetLeftEPi
+       .long   _ZN20nsGenericHTMLElement13GetScrollLeftEPi
+       .long   _ZN20nsGenericHTMLElement13SetScrollLeftEi
+       .long   _ZN20nsGenericHTMLElement14GetClientWidthEPi
+       .long   _ZN20nsGenericHTMLElement14GetOffsetWidthEPi
+       .long   _ZN20nsGenericHTMLElement14GetScrollWidthEPi
+       .long   _ZN20nsGenericHTMLElement14ScrollIntoViewEi
+       .long   _ZN20nsGenericHTMLElement15GetClientHeightEPi
+       .long   _ZN20nsGenericHTMLElement15GetOffsetHeightEPi
+       .long   _ZN20nsGenericHTMLElement15GetOffsetParentEPP13nsIDOMElement
+       .long   _ZN20nsGenericHTMLElement15GetScrollHeightEPi
+       .long   _ZN20nsGenericHTMLElement5GetIdER9nsAString
+       .long   _ZN20nsGenericHTMLElement5SetIdERK9nsAString
+       .long   _ZN20nsGenericHTMLElement6GetDirER9nsAString
+       .long   _ZN20nsGenericHTMLElement6SetDirERK9nsAString
+       .long   _ZN20nsGenericHTMLElement7GetLangER9nsAString
+       .long   _ZN20nsGenericHTMLElement7SetLangERK9nsAString
+       .long  
_ZN20nsGenericHTMLElement8GetStyleEPP25nsIDOMCSSStyleDeclaration+       .long  
_ZN20nsGenericHTMLElement8GetTitleER9nsAString
+       .long   _ZN20nsGenericHTMLElement8SetTitleERK9nsAString
+++ nsHTMLAnch.vtable.sort.new  2004-01-09 19:42:45.000000000 -0800
+       .long   _ZN16nsGenericElement4InitEP11nsINodeInfo
+       .long   _ZN20nsGenericHTMLElement12GetOffsetTopEPi
+       .long   _ZN20nsGenericHTMLElement12GetScrollTopEPi
+       .long   _ZN20nsGenericHTMLElement12SetScrollTopEi
+       .long   _ZN20nsGenericHTMLElement13GetOffsetLeftEPi
+       .long   _ZN20nsGenericHTMLElement13GetScrollLeftEPi
+       .long   _ZN20nsGenericHTMLElement13SetScrollLeftEi
+       .long   _ZN20nsGenericHTMLElement14GetClientWidthEPi
+       .long   _ZN20nsGenericHTMLElement14GetOffsetWidthEPi
+       .long   _ZN20nsGenericHTMLElement14GetScrollWidthEPi
+       .long   _ZN20nsGenericHTMLElement14ScrollIntoViewEi
+       .long   _ZN20nsGenericHTMLElement15GetClientHeightEPi
+       .long   _ZN20nsGenericHTMLElement15GetOffsetHeightEPi
+       .long   _ZN20nsGenericHTMLElement15GetOffsetParentEPP13nsIDOMElement
+       .long   _ZN20nsGenericHTMLElement15GetScrollHeightEPi
+       .long   _ZN20nsGenericHTMLElement8GetStyleEPP25nsIDOMCSSStyleDeclaration

Apparently these (jst knows the details) were methods forwarded to by the object
that actually implements the interface.

Comment 9

[Johnny Stenback (:jst)]

Yeah, I looked at those methods when I made this change and since the comment
said they were interface methods, I made them NS_IMETHOD methods, not realizing
that they were just the implementating what was needed for those methods, not
actually impelementing the methods directly, that was done primarily in
nsGenericHTMLElementTearoff, and with forwarding macros in the nsHTML*Element
classes. Fun.

Comment 10

[Johnny Stenback (:jst)]

Attachment: Bring the size of the vtables back to what they were before this change.

Comment 11

[Johnny Stenback (:jst)]

Closing now that this is checked in and the regressions appear to be fixed.