Closed Bug 1251797 Opened 9 years ago Closed 9 years ago

only make style data conditional on writing mode when a logical declaration *could* win the cascade

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox47 --- affected
firefox48 --- fixed

People

(Reporter: dbaron, Assigned: dbaron)

References

Details

Attachments

(1 file)

In bug 649142 we made logical declarations fault a style struct out of the rule tree when they're present in the cascade for the computation of a value (except when all values in the struct are specified higher in the cascade -- then we don't, nsRuleNode::WalkRuleTree will optimize away the call that does this). We've since made this a writing mode dependency rather than faulting. However, shortly after bug 649142 I wrote a followup patch to change this so that we only do this if the logical declaration could potentially win the cascade (i.e., there's an unspecified value for one of the relevant physical properties). I'm inclined to think we should do this. I've had this patch in my tree for a while.
(I'll post this for review once heycam is back.)
Attachment #8730062 - Flags: review?(cam) → review+
Comment on attachment 8730062 [details] MozReview Request: Bug 1251797 - Don't fault struct out of rule tree if all of the potential physical property destinations already have a winning value in the cascade. r?heycam https://reviewboard.mozilla.org/r/37019/#review36305 ::: layout/style/nsCSSDataBlock.cpp:301 (Diff revision 1) > -#ifdef DEBUG > - { > - // Table-length is 1 for single prop, 2 for axis prop, 4 for block prop. > + // Table-length is 1 for single prop, 2 for axis prop, 4 for block prop. > - size_t len = isSingleProperty ? 1 : (isAxisProperty ? 2 : 4); > + size_t len = isSingleProperty ? 1 : (isAxisProperty ? 2 : 4); > +#ifdef DEBUG > + { These braces can go now that we have moved |len| out of the #ifdef. ::: layout/style/nsCSSDataBlock.cpp:314 (Diff revision 1) > #endif > + > + for (size_t i = 0; i < len; i++) { > + if (aRuleData->ValueFor(props[i])->GetUnit() == eCSSUnit_Null) { > + // A declaration of one of the logical properties in this logical > + // group would be the winning declaration in the cascade. This Maybe s/would be/is/? ::: layout/style/nsCSSDataBlock.cpp:318 (Diff revision 1) > + // mean that for sure, though. For example, if this is a > + // block-start logical property, and all but the bottom physical > + // property were set. But the common case we want to hit here is In this "For example" maybe mention that the writing mode would need to be horizontal too?
(In reply to Cameron McCormack (:heycam) (busy Mar 14–18) from comment #4) > ::: layout/style/nsCSSDataBlock.cpp:314 > (Diff revision 1) > > #endif > > + > > + for (size_t i = 0; i < len; i++) { > > + if (aRuleData->ValueFor(props[i])->GetUnit() == eCSSUnit_Null) { > > + // A declaration of one of the logical properties in this logical > > + // group would be the winning declaration in the cascade. This > > Maybe s/would be/is/? I think instead I'll add "(but maybe not aProperty)", since that's why I didn't use "is". > ::: layout/style/nsCSSDataBlock.cpp:318 > (Diff revision 1) > > + // mean that for sure, though. For example, if this is a > > + // block-start logical property, and all but the bottom physical > > + // property were set. But the common case we want to hit here is > > In this "For example" maybe mention that the writing mode would need to be > horizontal too? I don't see why -- the block-start side can never be the bottom; it can be top (for horizontal writing mode), right, or left.
https://hg.mozilla.org/integration/mozilla-inbound/rev/718aa12e555b11e5980ea59929bf37c7416fd1db Bug 1251797 - Don't fault struct out of rule tree if all of the potential physical property destinations already have a winning value in the cascade. r=heycam
(In reply to David Baron [:dbaron] ⌚️UTC-7 from comment #5) > > In this "For example" maybe mention that the writing mode would need to be > > horizontal too? > > I don't see why -- the block-start side can never be the bottom; it can be > top (for horizontal writing mode), right, or left. I understand now.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Depends on: CVE-2017-7753
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: