240559 - nsTableCellFrame::InitCellFrame does lots of unnecessary work

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Two things here:

1)  Why is this function calling GetTableFrame() ?  It never uses it....

2)  SetColIndex sets the colindex on the content node, with mumbling about
    nsHTMLStyleSheet, but nsHTMLStyleSheet knows nothing about colindex.  I think
    we can probably remove that QI and set, remove the colindex stuff from
    nsHTMLTableCellElement, and maybe remove the nsIHTMLTableCellElement
    interface.

Comment 1

[Bernd]

isnt that nsI... interface heavily used by editor ?

Comment 2

[Boris Zbarsky [:bzbarsky]]

No.  See http://lxr.mozilla.org/seamonkey/search?string=nsIHTMLTableCellElement

You may be thinking nsI_DOM_HTMLTableCellElement.  Totally different beast.

Comment 3

[Bernd]

sorry, I was looking for consumers of the GetColIndex as 
http://lxr.mozilla.org/seamonkey/source/layout/html/table/public/nsITableCellLayout.h
and that is editor

Comment 4

[Boris Zbarsky [:bzbarsky]]

Ah, right.  That's implemented by nsTableCellFrame and doesn't involve the
content node at all.

Comment 5

[Bernd]

This looks like residium stuff from an earlier style system
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/html/table/src/nsTableRowFrame.cpp#56

Comment 6

[Bernd]

GetTableFrame() is the secret code to make sure the cellmap exists, Boris you
know the cellmap good enough (are old enough...) that I can share this secret
with you. Probably most of these GetTableFrame calls are bogus now as I placed
in outer table frame a requirement that mInner != nsnull.

I think we can remove the whole stuff 
I mean:
- InitCellFrame
- SetColIndex
- and the content stuff 
-
http://lxr.mozilla.org/seamonkey/source/content/html/content/src/nsIHTMLTableCellElement.h

Comment 7

[Boris Zbarsky [:bzbarsky]]

That sounds perfect.

Comment 8

[Bernd]

Attachment: patch

One cannot remove SetColIndex on the frame. But we can remove that on the
content node as nobody uses the data on the content node ( I believe so).
This patch saves us one QI and 4 bytes per table cell. I also removed that
GetSpanValue from the col frame that according to lxr and compile nobody uses.

Comment 9

[Bernd]

taking the bug

Comment 10

[Bernd]

Attachment: revised

Comment 11

[Bernd]

And yes I will remove the nsIHTMLTable*.h files too.

Comment 12

[Bernd]

Attachment: patch

there is even more to remove

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 146408 [details] [diff] [review]
patch

First of all, this saves _8_ bytes per table cell -- one 32-bit member and one
vtable pointer.  ;)

>Index: layout/html/table/src/nsTableCellFrame.h

>   virtual nsresult GetColIndex(PRInt32 &aColIndex) const;
>-  virtual nsresult SetColIndex(PRInt32 aColIndex);
>+  void SetColIndex(PRInt32 aColIndex);

No reason for GetColIndex to stay virtual, is there?

r+sr=bzbarsky.	Thanks for cleaning this up!

Comment 14

[Bernd]

I would like to leave it virtual see 
http://lxr.mozilla.org/seamonkey/source/layout/html/table/public/nsITableCellLayout.h#66

Comment 15

[Boris Zbarsky [:bzbarsky]]

Ah, ok.  Sounds good.

Comment 16

[Markus Hübner]

What effect will this have on bug 54542 ?

Comment 17

[Boris Zbarsky [:bzbarsky]]

About 0.5% improvement.

Comment 18

[Bernd]

fix checked in as Boris promised btek went up 0.5% :-).

Markus its up to you to verify what impact it has on bug 54542 with tomorrows
nightly.
There have been a couple of performance related checkins that would deserve a
quantitative estimate: 
the checkin for bug 236703 from 2004-04-12
the checkin for bug 240318 from 2004-04-12
and the change of background painting in quirks mode from 2004-04-18 

neither of them did show significant changes in pageload. So Markus comming back
with some real numbers might be helpfull.

Comment 19

[Boris Zbarsky [:bzbarsky]]

> fix checked in as Boris promised btek went up 0.5% :-).

It's been doing that randomly for a while...  ;(

Comment 20

[Bernd]

fix completely checked in