124412 - Make nsXULDocument inherit from nsDocument

Status: RESOLVED FIXED

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

Description

[Fabian Guisset]

Some time ago it was said that nsXULDocument should inherit from nsDocument, for
various reasons:

- less code duplication
- unimplemented XUL methods are implemented "for free"
- clean up nsCOMPtr vs raw pointer issues in nsDocument

I'm filing this bug now so I have a bug to mark all the others dependent on.
Unless a patch is contributed, this is probably post-1.0 work as the fix is very
"highrisk" (leaks and other regressions), but at least we have a bug about it...

Comment 1

[Fabian Guisset]

adding keywords, setting priority to P3, hopefully this can be done before year
2006, because some people (mainly Peter Wilson, who is/was making a XUL editor)
have asked for not-yet-implemented XUL methods (see dependent bugs)

Comment 2

[Alex Vincent [:WeirdAl]]

jetifi, how do you figure bug 39048 depends on this bug?  document.write is not
a part of nsDocument or of the Core DOM Document node.  document.write is a
holdover from DOM Level 0 that was incorporated into DOM Level 1 HTML (not Core,
which is what XUL must conform to).  document.write is also, imho, a risky piece
of code to include.

I'll let someone else decide that for now, but I disagree with you on this one.

Comment 3

[Alex Vincent [:WeirdAl]]

Mozilla 1.1 is out the door.

Comment 4

[Brendan Eich [:brendan]]

Alex is right, please don't set dependencies, let jst do that (ask if you think
there might be an unrecorded one).

/be

Comment 5

[Aaron Leventhal]

I think bug 173350 might be a dependency -- because IsIndex() will get
implemented "for free".

eta?

Comment 6

[Johnny Stenback (:jst)]

Attachment: WIP, mozilla starts, and things seem to work, but I'm sure there's more to be done.

Comment 7

[Johnny Stenback (:jst)]

Attachment: diff -w of the above attachment.

Comment 8

[Johnny Stenback (:jst)]

Attachment: Make nsXULDocument inherit nsXMLDocument

This should do it, AFAICT. This makes nsXULDocument inherit nsXMLDocument which
means we can share even more code, this does not however make an nsXULDocument
claim to implement all the interfaces that nsXMLDocument does, and
intentionally so since we don't yet know how to deal with things like
document.load() on a XUL document.

In addition to that, this also removes some unused interfaces that I found
while working on this, and I also got rid of nsMarkupDocument which isn't
needed at all. I also cleaned up some things in nsIDocument and also changed
the inheritance of some of the XUL specific interfaces to eliminate the need
for all the nsIDOMDocument related methods in nsXULDocument, in stead we just
inherit those and callers can get at them by QI'ing to nsIDOMDocument, and this
patch changes the users of that interface to not use it where not needed.

Other than that, there's a ton of random cleanup here, some memory owhership
changes (nsCOMPtr<> stuff), and so on.

diff -w coming up.

Comment 9

[Johnny Stenback (:jst)]

Attachment: diff -w

Comment 10

[Johnny Stenback (:jst)]

Attachment: Make nsXULDocument inherit nsXMLDocument

Same as above with some of my XXX comments removed, and a change to make
nsDocument::mChildNodes be of type nsRefPtr<nsDocumentChildNodes> to eliminate
some manual refcounting.

Comment 11

[Johnny Stenback (:jst)]

Attachment: diff -w

Comment 12

[Jan Varga [:janv]]

this is awesome, great work jst

Comment 13

[rbs]

Any need to specialize nsXULDocument::InternalInsertStyleSheetAt & friends?
I suspect doing so might break MathML in XUL (chrome). Might be safe just to
rely on the base methods which are already doing the necessary housekeeping.

Comment 14

[Johnny Stenback (:jst)]

Good point, rbs. I took those out and we now support the notion of catalogue
sheets in nsXULDocument's as well.

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

since 1.4a got pushed a week i should have time to r this one

Comment 16

[Peter Van der Beken [:peterv]]

Attachment: peterv's comments (part 1)

Comment 17

[Johnny Stenback (:jst)]

Comment on attachment 117730 [details]
peterv's comments (part 1)

>>+nsDocument::CreateEvent(const nsAString& aEventType, nsIDOMEvent** aReturn)
>...
>>   nsCOMPtr<nsIPresShell> shell;
>>   GetShellAt(0, getter_AddRefs(shell));
>>-  if (!shell)
>>-    return NS_ERROR_FAILURE;
>
>Why remove this?

It's ok to create events w/o a pres shell.

>>Index: content/xul/templates/src/nsXULTemplateBuilder.cpp
>>===================================================================
>
>>@@ -1408,13 +1408,13 @@ nsXULTemplateBuilder::GetTemplateRoot(ns
>>         if (! doc)
>>             return NS_ERROR_FAILURE;
>> 
>>-        nsCOMPtr<nsIDOMXULDocument> xulDoc = do_QueryInterface(doc);
>>-        NS_ASSERTION(xulDoc != nsnull, "expected a XUL document");
>>-        if (! xulDoc)
>>+        nsCOMPtr<nsIDOMDocument> domDoc = do_QueryInterface(doc);
>>+        NS_ASSERTION(domDoc != nsnull, "expected a XUL document");
>
>NS_ASSERTION(domDoc, ...)

Fixed.

>
>>+        if (! domDoc)
>
>Remove that space.

Hmm, I'd love to, but the style in this file is to put a space after '!', not
what I would do, but I'll leave it for now (here and in the other places where
you commented about the same issue where the style in the file is to do that).

The rest of the changes are made.

Comment 18

[Johnny Stenback (:jst)]

Mass-reassigning bugs to [e-mail]

Comment 19

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

> @@ -956,19 +969,20 @@ NS_IMETHODIMP
>  nsDocument::SetBaseURL(nsIURI* aURL)
>  {
>    nsresult rv = NS_OK;
> +
> +  mDocumentBaseURL = aURL;
> +
this will break the security-check.


> +nsDocument::SetDocumentCharacterSet(const nsAString& aCharSetID)
>  {
>    if (!mCharacterSet.Equals(aCharSetID)) {
> -    mCharacterSet = aCharSetID;
> +    mCharacterSet.Assign(aCharSetID);
nit: personally i prefer the old way


In nsDocument::SetDocumentCharacterSet
> +      observer->Observe((nsIDocument *)this, "charset",
> +                        PromiseFlatString(aCharSetID).get());
You can remove the explicit cast for |this|


> @@ -2798,13 +2915,16 @@ nsDocument::GetBoxObjectFor(nsIDOMElemen
>        contractID += "-iframe";
>      else if (tag.get() == nsXULAtoms::menu)
>        contractID += "-menu";
> -    else if (tag.get() == nsXULAtoms::popup || tag.get() == nsXULAtoms::...
> +    else if (tag.get() == nsXULAtoms::popup ||
> +             tag.get() == nsXULAtoms::menupopup ||
feel free to remove these |.get()|s if you want.


>  nsDocument::SetDir(const nsAString& aDirection)
>  {
> -  if (mPresShells.Count() != 0) {
> -    nsCOMPtr<nsIPresShell> shell = (nsIPresShell*)mPresShells.ElementAt(0);
> -    if (shell) {
> +  nsIPresShell *shell = (nsIPresShell *)mPresShells.SafeElementAt(0);
wanna use NS_STATIC_CAST?


> -nsDocument::ReplaceChild(nsIDOMNode* aNewChild, nsIDOMNode* aOldChild, nsI...
> +nsDocument::ReplaceChild(nsIDOMNode* aNewChild, nsIDOMNode* aOldChild,
> +                         nsIDOMNode** aReturn)
>  {
> -  NS_ASSERTION(((nsnull != aNewChild) && (nsnull != aOldChild)), "null ptr");
> +  NS_ASSERTION(aNewChild && aOldChild, "null ptr");
seems a bit harch to assert if a null is provided since this is exposed to
webpage-scripts. An ENSURE should be enough (which would replace the below
|if|). Same in nsDocument::RemoveChild


> mListenerManager->SetListenerTarget(NS_STATIC_CAST(nsIDocument*,this));
no need to have an explicit cast.


in nsDocument::CreateEvent
> -  if (presContext) {
>      nsCOMPtr<nsIEventListenerManager> manager;
>      GetListenerManager(getter_AddRefs(manager));
>      if (manager) {
>        return manager->CreateEvent(presContext, nsnull, aEventType, aReturn);
In the old code we would return NS_ERROR_FAILURE if the presContext was null. Is
it really safe to assume that all shells have a context?


>    ~nsDocHeaderData(void)
>    {
> -    NS_IF_RELEASE(mField);
> -    if (nsnull != mNext) {
>        delete mNext;
>        mNext = nsnull;
the mNext=nsnull is unneccesary too.


in nsHTMLDocument::nsHTMLDocument
> +  : nsDocument(),
No need to explicitly do this. The empty ctor is the default one to be called
anyway.

in nsHTMLDocument::nsHTMLDocument
> -  mIsWriting = 0;
> -  mWriteLevel = 0;
> -  mWyciwygSessionCnt = 0;
Why remove the initialization of these?


in nsHTMLDocument::TryBookmarkCharset
> +  nsresult rv = gRDF->GetDataSource("rdf:bookmarks",
> +                                    getter_AddRefs(datasource));
> +
> +  if (NS_FAILED(rv)) {
> +    return rv;
> +  }
This should be |return PR_FALSE|


nsHTMLDocument::StartDocumentLoad
> +    mParser = do_CreateInstance(kCParserCID, &rv);
> +
> +    if (NS_FAILED(rv)) {
> +      return rv;
> +    }
please use ENSURE


>  NS_IMETHODIMP
>  nsHTMLDocument::GetBaseURL(nsIURI*& aURL) const
>  {
> -  if (mDocumentBaseURL) {    
> -    aURL = mDocumentBaseURL.get();
> -    NS_ADDREF(aURL);
> -  }
> -  else {
> -    GetDocumentURL(&aURL);
> -  }
> -  return NS_OK;
> +  return nsDocument::GetBaseURL(aURL);
>  }
Couldn't you remove this implementation entierly, and rely on inheritance?

In nsHTMLDocument::GetCSSLoader
> NS_IF_ADDREF(aLoader);
You can remove the IF


In nsHTMLDocument::GetDomainURI
> +  nsCOMPtr<nsIURI> uri;
> +  nsresult rv = codebase->GetURI(getter_AddRefs(uri));
> +
> +  if (NS_FAILED(rv)) {
> +    return;
> +  }
> +
> +  *aUri = uri;
> +  NS_IF_ADDREF(*aUri);
You can use aUri directly, GetURI should null it out anyway if it returns an
errorcode.


>  nsHTMLDocument::GetBodyElement(nsIDOMHTMLBodyElement** aBody)
>  {
> -  if (!mBodyContent && !GetBodyContent()) {
> -    return NS_ERROR_FAILURE;
> +  *aBody = nsnull;
> +
> +  if (!mBodyContent && PR_FALSE == GetBodyContent()) {
comparing to PR_FALSE looks bad, please keep the '!'


in nsXULDocument::nsXULDocument
> +    // Override the default in nsDocument
> +    mCharacterSet.Assign(NS_LITERAL_STRING("UTF-8"));
You could do this in the init-list. nsXULDocuments initlist will be run after
nsDocuments ctor is fully run


In the nsXULDocument dtor you are reversing the order that the observers are
notified. I recall something about layout relying on the order of the
notifications for some event, but i'm not sure if the DocumentWillBeDestroyed
was the one.


more to come...

Comment 20

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

in nsXULDocument::InternalGetNumberOfStyleSheets you can remove the second |if|
and always decrease |count|. The other functions assume that mAttrStyleSheet is
there.

r=sicking with that!!!

Comment 21

[Johnny Stenback (:jst)]

Ok, made all those changes except the ones we discussed on irc. I'll attach a
final patch and check this in. Thanks peterv and sicking for the r= and sr=!

Comment 22

[Johnny Stenback (:jst)]

Attachment: Final diff

This is what was finally checked in.

Comment 23

[Johnny Stenback (:jst)]

Fixed, yay!

Comment 24

[Alec Flett]

Awesome!! This gave us a 30k reduction on linux, and a 15k reduction on
windows... this is just the kind of thing that footprint people have been asking
for. nice job jst.

Comment 25

[Bernd]

I am not sure but, this checkin may have caused bug 199756. I apologize in
advance for finger pointing if I am wrong.

Comment 26

[Andreas Kunz]

In bug 199768 (which looks quite ugly) this one is also mentioned as possible
cause...
Same apologies as above.

Comment 27

[Roland Mainz]

Nice sideeffect:
Printing XUL documents now seems to work due this patch. Thanks! :-))