Some immediate perf wins, lays the groundwork for more to come, some code cleanup.

Comment on attachment 442432 [details] [diff] [review] Remove remaining eELEMENT consumers in content/base Note that this content/base patch has one substantive change: the one in sNode3Tearoff::AreNodesEqual. Given that both things are nsIContent, the new code should be equivalent.

I changed the undisplayed codepath in ReResolveStyleContext because undisplayed->mContent really can't be null.

Comment on attachment 442437 [details] [diff] [review] Remove eELEMENT consumers in layout/base Where you're removing the null-check, could you add an assertion that undisplayed->mContent is not null? You filed bugs on the two "XXXbz" comments, right? r=dbaron

Comment on attachment 442438 [details] [diff] [review] Remove eELEMENT consumers in layout/generic >-static nsIContent* >+static Element* > FindElementAncestorForMozSelection(nsIContent* aContent) > { > NS_ENSURE_TRUE(aContent, nsnull); > while (aContent && aContent->IsInNativeAnonymousSubtree()) { > aContent = aContent->GetBindingParent(); > } > NS_ASSERTION(aContent, "aContent isn't in non-anonymous tree?"); >- while (aContent && !aContent->IsNodeOfType(nsINode::eELEMENT)) { >+ while (aContent && !aContent->IsElement()) { > aContent = aContent->GetParent(); > } >- return aContent; >+ return static_cast<Element*>(aContent); Shouldn't this return statement use AsElement rather than static_cast? r=dbaron with that (Though I wonder if it's better practice to do using namespace mozilla::dom; or typedef mozilla::dom::Element Element; .)

Comment on attachment 442439 [details] [diff] [review] Remove eELEMENT consumers in the rest of layout, except layout/style r=dbaron

Comment on attachment 442440 [details] [diff] [review] Remove eELEMENt consumers in layout/style, except in the ruleprocessor I think I saw bugs on the 2 XXXbz comments go by in bugmail, so that's good. Same aside comment about namespaces here. You should probably change the parameter type of nsSVGUtils::GetFontSize and GetFontXHeight to Element*. r=dbaron with that

Comment on attachment 442441 [details] [diff] [review] Eliminate eELEMENT consumers in ruleprocessor Same aside comment about namespaces. > struct ElementRuleProcessorData : public RuleProcessorData { > ElementRuleProcessorData(nsPresContext* aPresContext, >- nsIContent* aContent, >+ mozilla::dom::Element* aElement, > nsRuleWalker* aRuleWalker) >- : RuleProcessorData(aPresContext,aContent,aRuleWalker) >+ : RuleProcessorData(aPresContext,aElement,aRuleWalker) While you're changing this line, could you add spaces after the commas? r=dbaron

Comment on attachment 442429 [details] [diff] [review] Change some more nsIDocument APIs to return Element This *will* conflict with peterv's work in bug 560273, but you both probably know that already.

> could you add an assertion that undisplayed->mContent is not null? Done. > You filed bugs on the two "XXXbz" comments, right? Yep, blocked by this bug. > Shouldn't this return statement use AsElement rather than static_cast? I had it that way initially; it ends up being: return aContent ? aContent->AsElement() : nsnull; That looks to me like it might have an extra test compared to what I wrote, but maybe it doesn't really matter? > Though I wonder if it's better practice to do I'd rather do the using thing, because we'll end up putting more things in that namespace in the future. > You should probably change the parameter type of nsSVGUtils::GetFontSize and > GetFontXHeight to Element*. Done. This pushed the AsElement calls into nsSVGLength::EmLength and nsSVGLength::ExLength. > While you're changing this line, could you add spaces after the commas? Done.

(In reply to comment #24) > > Shouldn't this return statement use AsElement rather than static_cast? > > I had it that way initially; it ends up being: > > return aContent ? aContent->AsElement() : nsnull; > > That looks to me like it might have an extra test compared to what I wrote, but > maybe it doesn't really matter? Oh. It's fine either way, then, though the above might be clearer simply to show that it might be null...

Pushed: http://hg.mozilla.org/mozilla-central/rev/f62d143743a3 http://hg.mozilla.org/mozilla-central/rev/e0d610569c75 http://hg.mozilla.org/mozilla-central/rev/bca6860d1d62 http://hg.mozilla.org/mozilla-central/rev/bc10dcdc3b1e http://hg.mozilla.org/mozilla-central/rev/4872aec50aed http://hg.mozilla.org/mozilla-central/rev/f5ab6f6df219 http://hg.mozilla.org/mozilla-central/rev/95d587ae3c40 http://hg.mozilla.org/mozilla-central/rev/f12ff2c1e50e http://hg.mozilla.org/mozilla-central/rev/120ca10f1c08 http://hg.mozilla.org/mozilla-central/rev/66104f48f155 http://hg.mozilla.org/mozilla-central/rev/eff11dc0dd22 http://hg.mozilla.org/mozilla-central/rev/6ac70189d1b8 http://hg.mozilla.org/mozilla-central/rev/1b7b064ee77a http://hg.mozilla.org/mozilla-central/rev/27aa022a527d http://hg.mozilla.org/mozilla-central/rev/5dba6e283a1b http://hg.mozilla.org/mozilla-central/rev/37cd6605aea2 and tree is _so_ green.

Performance watcher script has this to blame on this checkin: Improvement: Dromaeo (CSS) increase 4.83% on MacOSX 10.5.8 Firefox Improvement: DHTML decrease 0.25% on XP Firefox Improvement: Dromaeo (CSS) increase 3.33% on MacOSX 10.6.2 Firefox I think there are some other things that it's mis-blaming (e.g. the Linux Dromaeo CSS 2% improvement) that are also to blame on this change.