Closed Bug 1139283 Opened 5 years ago Closed Last year

Move some properties out of nsStyleDisplay to remove its dependency to nsStyleVisibility

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED WONTFIX
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: xidorn, Assigned: xidorn)

References

(Blocks 1 open bug)

Details

Attachments

(6 files)

As discussed in bug 1055672, several properties should be moved from nsStyleDisplay to nsStylePosition for the removal of the dependency.

The properties to be moved are:
* clip (for the rect length computation)
* transform-origin (coord)
* perspective-origin (coord)
* perspective (coord)
* transform-style (together with other transform properties)
* backface-visibility (together with other transform properties)
* will-change (interaction with transform properties)
After fixing assertions because of insufficient max difference:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ac118b36524c
Assignee: nobody → quanxunzhen
Attachment #8573040 - Flags: review?(dbaron)
Attachment #8573043 - Flags: review?(dbaron)
Attachment #8573047 - Flags: review?(dbaron)
One thing in this patch probably needs a little concerns which is nsIFrame::GetClipPropClipRect. I think that change should be fine, but just mention it because it is a little more than a simple replace.
Attachment #8573051 - Flags: review?(dbaron)
Comment on attachment 8573040 [details] [diff] [review]
patch 1 - move fields, methods, constructor code and change property declaration

r=dbaron

(Not sure how useful the patch splitting is when the pieces don't compile on their own, though.  You may want to merge at least some of them for landing.)
Attachment #8573040 - Flags: review?(dbaron) → review+
Attachment #8573041 - Flags: review?(dbaron) → review+
Comment on attachment 8573043 [details] [diff] [review]
patch 3 - fix a method in nsStyleDisplay which uses moved methods

>+  if (!aContextFrame->IsSVGText()) {

Please change this to be:

if (aContextFrame->IsSVGText()) {
  return false;
}

and then unindent the rest of the function

r=dbaron with that (athough maybe this shouldn't really stay on nsStyleDisplay)
Attachment #8573043 - Flags: review?(dbaron) → review+
Attachment #8573046 - Flags: review?(dbaron) → review+
Attachment #8573047 - Flags: review?(dbaron) → review+
(In reply to David Baron [:dbaron] (UTC-8) from comment #9)
> Comment on attachment 8573040 [details] [diff] [review]
> patch 1 - move fields, methods, constructor code and change property
> declaration
> 
> r=dbaron
> 
> (Not sure how useful the patch splitting is when the pieces don't compile on
> their own, though.  You may want to merge at least some of them for landing.)

I just wanted to make it clearer for review. If every commit should compile on its own, I'll merge all of them together when landing.
Comment on attachment 8573051 [details] [diff] [review]
patch 6 - fix other property usage

In nsDisplayTransform::GetDeltaToTransformOrigin, please declare pos
where display was declared rather than lower; the other stuff below
it is also closely related to the for loop.

Same for nsDisplayTransform::GetDeltaToPerspectiveOrigin.


ApplyClipPropClipping should also lose its aDisp parameter.


r=dbaron with that
Attachment #8573051 - Flags: review?(dbaron) → review+
https://hg.mozilla.org/mozilla-central/rev/a6dbd23da598
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
As inter-character ruby is not landing currently, and some new properties have been added to Display and reintroduce the dependency, I want to reopen this bug, and backout the landed patch.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
See Also: → 1140623
If you want to back out the patch, you should do it sooner rather than later.
Flags: needinfo?(quanxunzhen)
Thanks for reminding. I'll try as soon as possible.
Blocks: 1165538
As the properties from CSS Scroll Snap Points which depends on length structures have landed in nsStyleDisplay, if we want to do this again, in addition to the properties I listed in comment 0, we will also need to move all scroll-snap-* properties.
No longer blocks: 1165538
With the architecture of stylo, I think we no longer need to move properties around to remove dependencies, so closing this as WONTFIX.
Status: REOPENED → RESOLVED
Closed: 5 years agoLast year
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.