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

Status: RESOLVED WORKSFORME

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

Description

[Aaron Leventhal]

The code in nsComputedDOMStyle.cpp has too much copy & paste.
It needs to use nsCSSPropList.h somehow.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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/.

Comment 2

[Biru [:poiru]]

Attachment: Move the _LAYOUT variant of COMPUTED_STYLE_MAP_ENTRY into a property bit

Comment 3

[Biru [:poiru]]

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?

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 6

[Biru [:poiru]]

Attachment: Move the _LAYOUT variant of COMPUTED_STYLE_MAP_ENTRY into a property bit

(In reply to David Baron [:dbaron] (needinfo? me) from comment #5)
> Comment on attachment 808163 [details] [diff] [review]

Thanks, took care of those.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/00a758bf68ed

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/00a758bf68ed

Comment 9

[Gorman Ho]

Hi Birunthan. Are you still actively working on this one? If not, may I take it?

Regards,
Gorman

Comment 10

[Biru [:poiru]]

(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!

Comment 11

[Firefox Bug Husbandry Bot]

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.

Comment 12

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 13

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 14

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 15

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 16

[Divyansh Sharma [:spiro]]

There is no nsCSSPropList.h in latest source code of firefox :/

Comment 17

[Sylvestre Ledru [:Sylvestre]]

Redirecting to David who should know!

Comment 18

[Cameron McCormack (:heycam)]

GETCS_NEEDS_LAYOUT_FLUSH is now a flag set in servo/components/style/properties/longhands/*.mako.rs.

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 20

[Cameron McCormack (:heycam)]

Eventually all the DoGet* functions should be replaced by calling into Servo code to serialize.