Last Comment Bug 659248 - Increase pseudo-element enum storage from 4 bits to 5 bits
: Increase pseudo-element enum storage from 4 bits to 5 bits
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla7
Assigned To: Yoan TEBOUL
:
Mentors:
Depends on:
Blocks: 657953
  Show dependency treegraph
 
Reported: 2011-05-24 02:26 PDT by Vincent LAMOTTE
Modified: 2011-08-26 08:05 PDT (History)
7 users (show)
mounir: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch_v1.0 (1.67 KB, patch)
2011-05-24 10:28 PDT, Yoan TEBOUL
bzbarsky: review-
Details | Diff | Review
patch_v1.1 (1.67 KB, patch)
2011-05-25 03:50 PDT, Yoan TEBOUL
dbaron: review+
Details | Diff | Review

Description Vincent LAMOTTE 2011-05-24 02:26:50 PDT
User-Agent:       Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0.1) Gecko/20100101 Firefox/4.0.1
Build Identifier: 

Adding a new CSS Pseudo-Element for <meter> made this assert to fault during compilation : (nsStyleContext.cpp, line 79)

PR_STATIC_ASSERT((PR_UINT32_MAX >> NS_STYLE_CONTEXT_TYPE_SHIFT) >=
                  nsCSSPseudoElements::ePseudo_MAX);
 


Reproducible: Always
Comment 1 Boris Zbarsky [:bz] 2011-05-24 07:26:18 PDT
Uh... this is not a 32-bit vs 64-bit issue.

Right now nsStyleContext stores the pseudo-element type in mBits.  It also stores the NS_STYLE_INHERIT_MASK and some additional boolean bits there.  That leaves 4 bits for the pseudo-element type.  So your pseudo-element enum needs to fit in 4 bits right now.

I believe that we currently have 23 style structs but NS_STYLE_INHERIT_MASK is 0x00ffffff.  So what you could do is change NS_STYLE_INHERIT_MASK to 0x008fffff (if this is not safe the static assert in nsStyleStruct.cpp will tell you) and then shift down by one the 4 style context boolean bits and change NS_STYLE_CONTEXT_TYPE_SHIFT to 27.  That will give you 5 bits for the pseudo-element type, which should be good enough for now.
Comment 2 Yoan TEBOUL 2011-05-24 10:28:47 PDT
Created attachment 534824 [details] [diff] [review]
patch_v1.0

Changed NS_STYLE_INHERIT_MASK to 0x007fffff and adjusted the other masks.
Comment 3 Boris Zbarsky [:bz] 2011-05-24 12:03:15 PDT
NS_STYLE_HAS_TEXT_DECORATION_LINES seems to be pretty wrong with that patch.
Comment 4 Boris Zbarsky [:bz] 2011-05-24 12:03:55 PDT
Comment on attachment 534824 [details] [diff] [review]
patch_v1.0

That said, since I suggested the approach better to have dbaron review.
Comment 5 Yoan TEBOUL 2011-05-25 03:50:02 PDT
Created attachment 535017 [details] [diff] [review]
patch_v1.1

Sorry for this stupid mistake...
Comment 6 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-05-25 15:28:44 PDT
Comment on attachment 535017 [details] [diff] [review]
patch_v1.1

r=dbaron
Comment 7 Mounir Lamouri (:mounir) 2011-05-26 08:50:51 PDT
This is ready to be pushed, isn't it?
Comment 8 Mounir Lamouri (:mounir) 2011-05-27 03:54:14 PDT
This patch has been pushed to cedar repository.
Comment 9 Mounir Lamouri (:mounir) 2011-05-27 08:26:33 PDT
Pushed:
http://hg.mozilla.org/mozilla-central/rev/ef6562c76cee
Comment 10 Virgil Dicu [:virgil] [QA] 2011-08-26 00:50:30 PDT
Hello.

Could anyone provide a test case or some STR to verify this issue?
Comment 11 Mounir Lamouri (:mounir) 2011-08-26 00:57:41 PDT
There is no need to verify this.
Comment 12 Boris Zbarsky [:bz] 2011-08-26 08:05:13 PDT
This change does not affect user-visible behavior.

It does allow new pseudo-elements to be added.

Note You need to log in before you can comment on or make changes to this bug.