Closed Bug 264517 Opened 20 years ago Closed 6 years ago

shrink nsComputedDOMStyle.cpp to improve maintainability and code re-use

Categories

(Core :: DOM: CSS Object Model, defect, P5)

defect

Tracking

()

RESOLVED WORKSFORME

People

(Reporter: aaronlev, Unassigned, Mentored)

References

Details

(Whiteboard: [leave open][lang=c++])

Attachments

(1 file, 1 obsolete file)

The code in nsComputedDOMStyle.cpp has too much copy & paste. It needs to use nsCSSPropList.h somehow.
Summary: Imrprove code re-use in nsComputedDOMStyle.cpp → Improve code re-use in nsComputedDOMStyle.cpp
Assignee: dbaron → general
Component: Style System (CSS) → DOM: CSSOM
Assignee: general → nobody
QA Contact: ian → general
QA Contact: general → style-system
Depends on: 649142
Depends on: 649119
I think there are a bunch of steps we could take here (which should probably be separate patches): 1. move the _LAYOUT variant of COMPUTED_STYLE_MAP_ENTRY into a property bit (defined in nsCSSProps.h and nsCSSPropList.h) 2. replace nsComputedDOMStyle::GetQueryablePropertyMap with something generated from nsCSSPropList.h 3. add a property bit that says "use a custom function" for computed style, and set it for all the properties. (It should actually be done in the style of CSS_PROPERTY_PARSE_PROPERTY_MASK, since it will end up being a value that takes up multiple bits.) 4, 5, 6, ...: one pattern at a time, replace common patterns for computed style with additional values in that property bit space, and remove the custom functions that can be replaced) The relevant code here is all in layout/style/, in the files nsComputedDOMStyle.{h,cpp}, nsCSSPropList.h, and nsCSSProps.h. It should be reasonably well covered by the mochitests in layout/style/test/.
Summary: Improve code re-use in nsComputedDOMStyle.cpp → shrink nsComputedDOMStyle.cpp to improve maintainability and code re-use
Whiteboard: [mentor=dbaron]
Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=fdcbcd21e39e By the way, I presume it is fine to work on one part at a time and land each one individually?
(In reply to Birunthan Mohanathas [:poiru] from comment #3) > By the way, I presume it is fine to work on one part at a time and land each > one individually? Yes, that's great as long as all the intermediate states work correctly.
Comment on attachment 808163 [details] [diff] [review] Move the _LAYOUT variant of COMPUTED_STYLE_MAP_ENTRY into a property bit >+bool nsComputedDOMStyle::ComputedStyleMapEntry::IsLayoutFlushNeeded() const >+{ >+ return nsCSSProps::PropHasFlags(mProperty, CSS_PROPERTY_NEEDS_LAYOUT_FLUSH); >+} This should be inline (meaning, defined in the .h so that it gets inlined at callers). >+// This property is a layout property that needs a flush. >+#define CSS_PROPERTY_NEEDS_LAYOUT_FLUSH (1<<20) >+ Could you name this CSS_PROPERTY_GETCS_NEEDS_LAYOUT_FLUSH instead? (That's easy to do by search-replace of the entire patch.) And make the comment say: // This property's getComputedStyle implementation requires layout to be // flushed. r=dbaron with those changes, and thanks for the patch. The bits with the directional shorthands (margin-left, margin-right, padding-left, padding-right, etc.) might get interesting in the next patch, but seem correct in this patch.
Attachment #808163 - Flags: review?(dbaron) → review+
(In reply to David Baron [:dbaron] (needinfo? me) from comment #5) > Comment on attachment 808163 [details] [diff] [review] Thanks, took care of those.
Assignee: nobody → birunthan
Attachment #808163 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #808257 - Flags: review+
Keywords: checkin-needed
Whiteboard: [mentor=dbaron] → [mentor=dbaron][leave open]
Attachment #808257 - Flags: checkin+
Whiteboard: [mentor=dbaron][leave open] → [mentor=dbaron][leave open][lang=c++]
Hi Birunthan. Are you still actively working on this one? If not, may I take it? Regards, Gorman
(In reply to Gorman Ho from comment #9) > Hi Birunthan. Are you still actively working on this one? If not, may I take > it? Sure, it's yours!
Assignee: birunthan → gormanchi
Assignee: gormanchi → nobody
Mentor: dbaron
Whiteboard: [mentor=dbaron][leave open][lang=c++] → [leave open][lang=c++]
https://bugzilla.mozilla.org/show_bug.cgi?id=1472046 Move all DOM bugs that haven’t been updated in more than 3 years and has no one currently assigned to P5. If you have questions, please contact :mdaly.
Priority: -- → P5
No assignee, updating the status.
Status: ASSIGNED → NEW
There is no nsCSSPropList.h in latest source code of firefox :/
Flags: needinfo?(sledru)
Redirecting to David who should know!
Flags: needinfo?(sledru) → needinfo?(dbaron)
Flags: needinfo?(dbaron) → needinfo?(cam)
GETCS_NEEDS_LAYOUT_FLUSH is now a flag set in servo/components/style/properties/longhands/*.mako.rs.
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: needinfo?(cam)
Resolution: --- → WORKSFORME
I think there was still a lot more to do here -- many of the nsComputedDOMStyle::DoGet* functions are quite similar to each other -- but I'm OK with that happening in other bugs.
Eventually all the DoGet* functions should be replaced by calling into Servo code to serialize.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: