Closed
Bug 124412
Opened 23 years ago
Closed 22 years ago
Make nsXULDocument inherit from nsDocument
Categories
(Core :: DOM: Core & HTML, defect, P3)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla1.4alpha
People
(Reporter: fabian, Assigned: jst)
References
Details
(Keywords: highrisk, memory-footprint)
Attachments
(4 files, 4 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review |
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...
Reporter | ||
Comment 1•23 years ago
|
||
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•22 years ago
|
||
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 4•22 years ago
|
||
Alex is right, please don't set dependencies, let jst do that (ask if you think
there might be an unrecorded one).
/be
No longer blocks: 39048
Comment 5•22 years ago
|
||
I think bug 173350 might be a dependency -- because IsIndex() will get
implemented "for free".
eta?
Updated•22 years ago
|
Component: DOM Mozilla Extensions → DOM Other
Assignee | ||
Comment 6•22 years ago
|
||
Assignee | ||
Comment 7•22 years ago
|
||
Assignee | ||
Updated•22 years ago
|
Target Milestone: --- → mozilla1.4alpha
Assignee | ||
Comment 8•22 years ago
|
||
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.
Assignee | ||
Updated•22 years ago
|
Attachment #116821 -
Attachment is obsolete: true
Attachment #116824 -
Attachment is obsolete: true
Assignee | ||
Comment 9•22 years ago
|
||
Assignee | ||
Updated•22 years ago
|
Attachment #117121 -
Flags: superreview?(peterv)
Attachment #117121 -
Flags: review?(bugmail)
Assignee | ||
Updated•22 years ago
|
Attachment #117121 -
Attachment is obsolete: true
Attachment #117121 -
Flags: superreview?(peterv)
Attachment #117121 -
Flags: review?(bugmail)
Assignee | ||
Comment 10•22 years ago
|
||
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.
Attachment #117120 -
Attachment is obsolete: true
Assignee | ||
Comment 11•22 years ago
|
||
Assignee | ||
Updated•22 years ago
|
Attachment #117128 -
Flags: superreview?(peterv)
Attachment #117128 -
Flags: review?(bugmail)
Comment 12•22 years ago
|
||
this is awesome, great work jst
Comment 13•22 years ago
|
||
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.
Assignee | ||
Comment 14•22 years ago
|
||
Good point, rbs. I took those out and we now support the notion of catalogue
sheets in nsXULDocument's as well.
since 1.4a got pushed a week i should have time to r this one
Comment 16•22 years ago
|
||
Assignee | ||
Comment 17•22 years ago
|
||
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.
Assignee | ||
Comment 18•22 years ago
|
||
Mass-reassigning bugs to dom_bugs@netscape.com
Assignee: jst → dom_bugs
> @@ -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...
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!!!
Assignee | ||
Comment 21•22 years ago
|
||
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=!
Status: NEW → ASSIGNED
Assignee | ||
Comment 22•22 years ago
|
||
This is what was finally checked in.
Assignee | ||
Comment 23•22 years ago
|
||
Fixed, yay!
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Comment 24•22 years ago
|
||
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•22 years ago
|
||
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•22 years ago
|
||
In bug 199768 (which looks quite ugly) this one is also mentioned as possible
cause...
Same apologies as above.
Updated•22 years ago
|
Attachment #117128 -
Flags: superreview?(peterv)
Attachment #117128 -
Flags: review?(bugmail)
Comment 27•22 years ago
|
||
Nice sideeffect:
Printing XUL documents now seems to work due this patch. Thanks! :-))
Blocks: xulprinting
Updated•14 years ago
|
Assignee: general → jst
QA Contact: ian → general
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•