There're some virtual methods here that don't need to be.

Saves around 15KB of vtables.

The HasTextForTranslation implementation was just moved, with the nodetype check up front dropped because that's enforced statically now.

Please look at this one carefully... I wanted to devirtualize the things that touch mRefCnt, but I wasn't sure why the refcounting/CC setup was what it was. If you're not sure either, we should probably talk to Olli.

Comment on attachment 8963138 [details] [diff] [review] part 1. Get rid of nsIContent::SetText Review of attachment 8963138 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/forms/nsTextControlFrame.cpp @@ +1264,5 @@ > NS_PRECONDITION(!mEditorHasBeenInitialized, > "Do not call this after editor has been initialized"); > > + MOZ_ASSERT_IF(mRootNode->GetFirstChild(), > + mRootNode->GetFirstChild()->IsText()); Couldn't this assert be inside the else block without the IF stuff and in terms of childContent? It would be shorter. Also, generally, it is a little unfortunate there's no "MustGetAsText()" method with an assert, though I suppose that adding a virtual method would be counterproductive to the goal of shrinking the vtable...

Comment on attachment 8963140 [details] [diff] [review] part 3. Get rid of nsIContent::AppendTextTo Review of attachment 8963140 [details] [diff] [review]: ----------------------------------------------------------------- ::: accessible/base/nsTextEquivUtils.cpp @@ +116,5 @@ > nsresult > nsTextEquivUtils::AppendTextEquivFromTextContent(nsIContent *aContent, > nsAString *aString) > { > + if (aContent->IsText()) { What's the reason for these replacements? Is it just a better way to check the same thing, or is there a semantic change?

> Couldn't this assert be inside the else block without the IF stuff and in terms of childContent? Yes, fixed, but see below. > Also, generally, it is a little unfortunate there's no "MustGetAsText()" method with an assert Actually, I can just add one. I'll post an interdiff in a jiffy. > What's the reason for these replacements? Replacing a virtual call by a faster inlined check. They have identical semantics. Generally I would like to remove IsNodeOfType... ;)

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #9) > Actually, I can just add one. I'll post an interdiff in a jiffy. Sounds good. > > What's the reason for these replacements? > > Replacing a virtual call by a faster inlined check. They have identical > semantics. Generally I would like to remove IsNodeOfType... ;) Maybe file a bug about doing a mass replacement of IsNodeOfType, if one doesn't exist already?

Comment on attachment 8963269 [details] [diff] [review] Interdiff for part 1 adding asserting AsText() Review of attachment 8963269 [details] [diff] [review]: ----------------------------------------------------------------- Thanks.

> Maybe file a bug about doing a mass replacement of IsNodeOfType, if one doesn't exist already? Filed bug 1449669 and bug 1449670.

Comment on attachment 8963142 [details] [diff] [review] part 5. Move the cycle collected refcount on content nodes up to nsIContent Review of attachment 8963142 [details] [diff] [review]: ----------------------------------------------------------------- Yeah, Olli should look at this one. I don't know why nsIContent::OwnedOnlyByTheDOMTree() would return false...

(In reply to Andrew McCreight [:mccr8] from comment #14) > nsIContent::OwnedOnlyByTheDOMTree() would return false... IIRC the idea was that it is an opt-in optimization. So I didn't have to think the ownership model of some less used nsIContents, like DocumentFragment or so. But that was 6 (really!) years ago.

Pushed by bzbarsky@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/ad610b422f26 part 1. Get rid of nsIContent::SetText. r=mccr8 https://hg.mozilla.org/integration/mozilla-inbound/rev/e7a650eb7ae0 part 2. Get rid of nsIContent::AppendText. r=mccr8 https://hg.mozilla.org/integration/mozilla-inbound/rev/9e4c2c101487 part 3. Get rid of nsIContent::AppendTextTo. r=mccr8 https://hg.mozilla.org/integration/mozilla-inbound/rev/a2f5597047da part 4. Get rid of a few virtual nsIContent methods. r=mccr8 https://hg.mozilla.org/integration/mozilla-inbound/rev/1d3e400044fc part 5. Move the cycle collected refcount on content nodes up to nsIContent. r=smaug

https://hg.mozilla.org/mozilla-central/rev/ad610b422f26 https://hg.mozilla.org/mozilla-central/rev/e7a650eb7ae0 https://hg.mozilla.org/mozilla-central/rev/9e4c2c101487 https://hg.mozilla.org/mozilla-central/rev/a2f5597047da https://hg.mozilla.org/mozilla-central/rev/1d3e400044fc