Closed Bug 408933 Opened 12 years ago Closed 12 years ago

get rid of nsStyleStruct base type

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: dbaron, Assigned: dwitte)

Details

(Keywords: memory-footprint)

Attachments

(1 file, 2 obsolete files)

We should get rid of the nsStyleStruct base class and just make all the style structs simple structs that don't inherit from anything (and use void* where we currently use nsStyleStruct*).  This will remove the potential extra word of padding needed to give nsStyleStruct a nonzero size.
Attached patch v1 (obsolete) — Splinter Review
Assignee: nobody → dwitte
Status: NEW → ASSIGNED
Attachment #293984 - Flags: superreview?
Attachment #293984 - Flags: review?
Attachment #293984 - Flags: superreview?(dbaron)
Attachment #293984 - Flags: superreview?
Attachment #293984 - Flags: review?(dbaron)
Attachment #293984 - Flags: review?
Looks good at first glance; I'll look more after (at least some of) the holidays.  This'll cause roc a bit of merge pain, but I think the best solution there is for him to land first, since this patch is probably a bit easier to redo in the necessary parts.

Also might be good to see if roc/boris agree that the idea is reasonable, but I think they well.

Also, probably better to update adding-style-props.html to have the current syntax, which would skip the line you changed and the one before it and change the one after to begin with GetStyleUIReset()->.  Though that doc really needs a lot more changes...
Sounds reasonable to me. it's probably not hard for me to merge, I have my MathML patch in a Mercurial tree so merging is fairly easy.
While you're here, want to remove the (now unused) nsIFrame::GetStyleData and nsIFrame::GetStyleDataExternal methods and update the docs accordingly?

The rest of this looks good to me.
Attachment #293984 - Flags: superreview?(dbaron)
Attachment #293984 - Flags: review?(dbaron)
Attached patch v2 (obsolete) — Splinter Review
incorporate adding-style-props.html fix and remove nsIFrame::GetStyleData{External}.

two questions:
1) GetStyleDataExternal is still used at http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/generic/nsIFrame.h&rev=3.389&mark=673#673. i removed the entire #else block, and the #ifdef wrapping the block above, but i'm not sure if that's the right thing to do... maybe we at least want to keep the #ifdef?
2) should i rev the nsIFrame IID?

i don't mind landing after roc, so either way - this patch didn't take long to roll. ;)
Attachment #293984 - Attachment is obsolete: true
Attachment #294019 - Flags: superreview?(dbaron)
Attachment #294019 - Flags: review?(dbaron)
Oh, hmm.  We need to keep GetStyleDataExternal, then.  I somehow missed that caller.  :(
Attachment #294019 - Flags: superreview?(dbaron)
Attachment #294019 - Flags: review?(dbaron)
Attached patch v3Splinter Review
alrighty then. third time's the charm :)
Attachment #294019 - Attachment is obsolete: true
Attachment #294205 - Flags: superreview?(dbaron)
Attachment #294205 - Flags: review?(dbaron)
Comment on attachment 294205 [details] [diff] [review]
v3

-    (*aRuleData->mPostResolveCallback)((nsStyleStruct*)res, aRuleData);
+    (*aRuleData->mPostResolveCallback)((void*)res, aRuleData);
 
Could you make that cast a const_cast ?

-      (ruleData.mPostResolveCallback)((nsStyleStruct*)aFont, &ruleData);
+      (ruleData.mPostResolveCallback)((void*)aFont, &ruleData);

same here

+  NS_HIDDEN_(const void*) 
     GetTableBorderData(nsStyleContext* aContext);

+  NS_HIDDEN_(const void*) 
     GetUserInterfaceData(nsStyleContext* aContext);

Could you unwrap these now that they fit in less than 80 characters?


r+sr=dbaron.  Sorry for the delay.
Attachment #294205 - Flags: superreview?(dbaron)
Attachment #294205 - Flags: superreview+
Attachment #294205 - Flags: review?(dbaron)
Attachment #294205 - Flags: review+
Comment on attachment 294205 [details] [diff] [review]
v3

thanks! will fix those up on checkin.

requesting approval for a low-risk cleanup patch.
Attachment #294205 - Flags: approval1.9?
Comment on attachment 294205 [details] [diff] [review]
v3

a=beltzner for 1.9
Attachment #294205 - Flags: approval1.9? → approval1.9+
checked in, with nits fixed. (i removed the void* cast on the first mPostResolveCallback call, rather than use const_cast, since |res| is already a non-const pointer.)
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
er, i mean second call, and |aFont|.
Depends on: 432801
No longer depends on: 432801
You need to log in before you can comment on or make changes to this bug.