Closed Bug 802849 Opened 7 years ago Closed 7 years ago

Refactor nsPresContext::MediaFeatureValuesChanged so that it can handle rebuilding style data for all callers

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla19

People

(Reporter: seth, Assigned: seth)

References

Details

Attachments

(1 file, 2 obsolete files)

nsPresContext::MediaFeatureValuesChanged currently takes an |aCallerWillRebuildStyleData| argument that determines whether it or the caller is responsible for rebuilding style data. This is undesirable because it means the callers depend in a non-obvious way on the actions that MediaFeatureValuesChanged implicitly takes, and whether the style rebuilding happens before or after MediumFeaturesChanged listeners are notified depends upon whether the caller or the callee does the rebuilding.

The plan to solve this is to make MediaFeatureValuesChanged take a required first argument specifying a policy (either it should always rebuild styles, or it should rebuild styles only if it believes this to be necessary) and an optional second argument specifying the nsChangeHint to be used if the style data is rebuilt. This change will allow all callers to rely upon MediaFeatureValuesChanged to perform the style rebuilding, and will ensure that in all these cases the behavior is consistent.
Comment on attachment 672566 [details] [diff] [review]
Refactor nsPresContext::MediaFeatureValuesChanged so that it can handle rebuilding style data for all callers.

> void
>-nsPresContext::MediaFeatureValuesChanged(bool aCallerWillRebuildStyleData)
>+nsPresContext::MediaFeatureValuesChanged(nsPresContext_StyleRebuildPolicy rebuildPolicy,
>+                                         nsChangeHint rebuildChangeHint)

rebuildPolicy -> aShouldRebuild
rebuildChangeHint -> aChangeHint

And please add an assertion that
  aShouldRebuild == eAlwaysRebuild || aChangeHint == 0


>diff --git a/layout/base/nsPresContext.h b/layout/base/nsPresContext.h

>+// Style data rebuild policies. Used by MediaFeatureValuesChanged.
>+enum nsPresContext_StyleRebuildPolicy {
>+  kPresContext_RebuildStylesIfNeeded = 1,
>+  kPresContext_RebuildStylesAlways
>+};

Make this enum a member of nsPresContext, and give the type a shorter name (say, StyleRebuildType) and the values shorther names (say, eRebuildStyleIfNeeded, eAlwaysRebuildStyle).

>-  void MediaFeatureValuesChanged(bool aCallerWillRebuildStyleData);
>+  void MediaFeatureValuesChanged(nsPresContext_StyleRebuildPolicy rebuildPolicy,
>+                                 nsChangeHint rebuildChangeHint = nsChangeHint(0));

rename params here too

r=dbaron with that (this should have been a review request, not a feedback request)
Attachment #672566 - Flags: feedback?(dbaron) → review+
Depends on: 503720
Thanks, David. I've made the changes you recommended. Carrying forward r+ and starting a try job.
Attachment #679956 - Flags: review+
Attachment #679956 - Attachment is obsolete: true
Attachment #672566 - Attachment is obsolete: true
Bitten by my love of C99. =p Retrying without an errant comma gumming up the works.
Try run for 75bea3c23cb0 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=75bea3c23cb0
Results (out of 241 total builds):
    success: 219
    warnings: 20
    failure: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mfowler@mozilla.com-75bea3c23cb0
Try looks OK. Requesting checkin.
Keywords: checkin-needed
No, it's just code cleanup/simplification.
Flags: in-testsuite? → in-testsuite-
https://hg.mozilla.org/mozilla-central/rev/e9c6c3ac7a76
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
You need to log in before you can comment on or make changes to this bug.