Closed Bug 1216431 Opened 4 years ago Closed 4 years ago

revisit approach to fixing bug 1209603

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed
b2g-v2.5 --- fixed

People

(Reporter: dbaron, Assigned: dbaron)

References

Details

Attachments

(8 files)

1.86 KB, text/plain
Details
2.99 KB, patch
heycam
: review+
Details | Diff | Splinter Review
7.37 KB, patch
heycam
: review+
Details | Diff | Splinter Review
3.26 KB, patch
heycam
: review+
Details | Diff | Splinter Review
6.30 KB, patch
heycam
: review+
Details | Diff | Splinter Review
1.84 KB, patch
heycam
: review+
Details | Diff | Splinter Review
9.79 KB, patch
heycam
: review+
Details | Diff | Splinter Review
4.78 KB, patch
heycam
: review+
Details | Diff | Splinter Review
In hindsight, I think I made the wrong choice when fixing bug 1209603.  CalcStyleDifference's optimizations related to PeekStyleData aren't really that valuable, since they really only help for elements that haven't yet been displayed.  So I think we should replace some of the patches there with the alternative I described in bug 1209603 comment 62.
With this approach we end up with test failures because we're computing a new nsStyleVisibility inside of nsStylePosition::CalcDifference.

Presumably with the current approach we end up not calling nsStylePosition::CalcDifference because having PeekStyleData be exact means we don't call it.

Interestingly, nsRuleNode::ComputePositionData gets the writing mode and does things as a function of it, but *doesn't* set conditions properly depending on it.  So there are some other latent bugs near there.
(In reply to David Baron [:dbaron] ⌚UTC-7 from comment #2)
> Interestingly, nsRuleNode::ComputePositionData gets the writing mode and
> does things as a function of it, but *doesn't* set conditions properly
> depending on it.  So there are some other latent bugs near there.

I filed bug 1216747 on this.
This is needed to avoid hitting the assertion:
Assertion failure: !!(structsFound & (1 << uint64_t(eStyleStruct_Visibility))) == !!PeekStyleVisibility() (PeekStyleData results must not change in the middle of difference calculation.), at ./nsStyleStructList.h:62
once exact PeekStyleData is backed out in the later patches.

Without this patch, we can compute a new nsStyleVisibility struct inside
of nsStylePosition::CalcDifference.  This patch ensures we use
PeekStyleVisibility() instead of StyleVisibility().
Attachment #8677180 - Flags: review?(cam)
This leaves the half related to inherited structs, since we can still
bail early for them even without mBits.
Attachment #8677183 - Flags: review?(cam)
This requires a little bit of gymnastics since it has to add the inverse
of tests, since the is-a-reset-struct tests originally added in patch 8
were made unconditional in patch 9, and with this backout we now want to
execute the code only for inherited structs.

This also doesn't back out the cleanup to use NS_STYLE_INHERIT_BIT() for
constants rather than nsCachedStyleData::GetBitForSID.
Attachment #8677184 - Flags: review?(cam)
This backs out all of the patch except that it retains some of the
comment changes for nsStyleContext::mBits.
Attachment #8677185 - Flags: review?(cam)
Attachment #8677179 - Flags: review?(cam) → review+
Attachment #8677180 - Flags: review?(cam) → review+
Attachment #8677181 - Flags: review?(cam) → review+
Comment on attachment 8677182 [details] [diff] [review]
patch 4 - Don't trigger computation of new structs via testing of conditions for conditionally-stored structs on the rule node

Review of attachment 8677182 [details] [diff] [review]:
-----------------------------------------------------------------

Maybe aComputeData on GetStyleData should be called something else, since the existing aComputeData (template) parameter means "if we don't have the struct cached, compute it", whereas here it's being used to mean "if we need to compute a dependent struct, allow that".
Attachment #8677182 - Flags: review?(cam) → review+
Attachment #8677183 - Flags: review?(cam) → review+
Attachment #8677184 - Flags: review?(cam) → review+
Attachment #8677185 - Flags: review?(cam) → review+
(In reply to Cameron McCormack (:heycam) (away Oct 28) from comment #15)
> Maybe aComputeData on GetStyleData should be called something else, since
> the existing aComputeData (template) parameter means "if we don't have the
> struct cached, compute it", whereas here it's being used to mean "if we need
> to compute a dependent struct, allow that".

I renamed to aCanComputeData; I hope you think that's an improvement.
Yes, that's an improvement, thanks.
Depends on: 1234966
You need to log in before you can comment on or make changes to this bug.