Cleaning up element implementations

RESOLVED FIXED

Status

()

Core
DOM
--
enhancement
RESOLVED FIXED
13 years ago
12 years ago

People

(Reporter: smaug, Assigned: smaug)

Tracking

Trunk
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

I have some cleanups for element implementations
(which are now all inheriting nsGenericElement).
I'll attach a patch which contains some trivial fixes, but
it is adding also some new (virtual :( ) methods. Not sure
whether that will be bad for performance, but code savings
are significant (+285/-810)
I can of course split the patch, so that it is easier to
test it.

Comments welcome.
Created attachment 193613 [details] [diff] [review]
patch
Severity: normal → enhancement

Updated

13 years ago
Attachment #193613 - Attachment description: . → patch
So could you summarize the changes?  I'm not sure why you're making some of
these modifications (eg the change to when we fire attrmodified events looks wrong).

If you're going to add new methods like WillChangeContent, it'd really help to
document them.

And I'm not too happy with HasAttr needing an extra virtual call....
(In reply to comment #2)
> So could you summarize the changes?  I'm not sure why you're making some of
> these modifications (eg the change to when we fire attrmodified events looks
wrong).

Hmm, did I change that, then that is a bug.
Anyway, atm we do have *lots of* duplicated code and I'd like to remove some of it.

> 
> If you're going to add new methods like WillChangeContent, it'd really help to
> document them.

Sure.

> 
> And I'm not too happy with HasAttr needing an extra virtual call....

I'm not too happy with that either. I just wanted to try to simplify the code.

But I'll start splitting up this. I think the changes to InsertChildAt and
AppendChildTo are the easiest ones. With that it is possible to remove some code
from nsXULElement and nsHTMLXXXElements.
Created attachment 194674 [details] [diff] [review]
InsertChildAt, AppendChildTo

This removes nsXULElement::InsertChildAt and 
nsXULElement::AppendChildTo.
Attachment #193613 - Attachment is obsolete: true
Attachment #194674 - Flags: review?(bzbarsky)
Depends on: 308270
Comment on attachment 194674 [details] [diff] [review]
InsertChildAt, AppendChildTo

>Index: base/src/nsGenericElement.cpp

> nsGenericElement::InsertChildAt(nsIContent* aKid,

>+    // XXX this screws up ranges in XUL, no?  Need to figure out...
>+  if (!IsContentOfType(eXUL)) {
>+    nsRange::OwnerChildInserted(this, aIndex);
>+  }

Please file a followup bug on this.  I think we want to just remove this if
check and see what breaks; 1.9a is a perfect time for that!  ;)

>Index: base/src/nsGenericElement.h

>+   * Will be called when InsertChildAt, AppendChildTo or RemoveChildAt is used.

Document that it will be called before anything is actually done.

Also document that for RemoveChildAt aKid is the kid at aIndex and in fact that
aKid is never null when this method is called.

>+   * The processing of the calling method will be cancelled if this method
>+   * does not return NS_OK.

You mean does not return success, right?  There are success codes other than
NS_OK.

I'd prefer you not inline the NS_OK return here; put that part in the .cpp

> Index: html/content/src/nsHTMLOptGroupElement.cpp

Is there a reason you're not calling the superclass's WillAddOrRemoveChild?

>Index: html/content/src/nsHTMLSelectElement.cpp
>+nsHTMLSelectElement:: WillAddOrRemoveChild(nsIContent* aKid,

Remove the space after ::, please.

Again, why is the superclass not being called?

>Index: xul/content/src/nsXULElement.h

Again, please put the method impl in the .cpp, ok?

r=bzbarsky with those nits.

jst, what do you think?
Attachment #194674 - Flags: superreview?(jst)
Attachment #194674 - Flags: review?(bzbarsky)
Attachment #194674 - Flags: review+
Comment on attachment 194674 [details] [diff] [review]
InsertChildAt, AppendChildTo

sr=jst with bz's issues addressed, and I'd say un-inline virtual methods, at
least ones that aren't often calld with a fully qualified name (i.e. the ones
in nsGenericElement) since the compiler won't be able to inline any of those
begin with.
Attachment #194674 - Flags: superreview?(jst) → superreview+
Created attachment 195924 [details] [diff] [review]
Patch for check in
Attachment #194674 - Attachment is obsolete: true
Checked in.
SetAttr cleanups will be handled in bug 308270.
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
You'll file a followup on the range thing?  And other bugs on other parts of
this cleanup?
You need to log in before you can comment on or make changes to this bug.