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

Status: RESOLVED WONTFIX

(Core :: CSS Parsing and Computation, defect)

Description

[Xidorn Quan [:xidorn] UTC+10]

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)

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=02f2c75a51ff

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

After fixing assertions because of insufficient max difference:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ac118b36524c

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 1 - move fields, methods, constructor code and change property declaration

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 2 - move calc difference & max difference code

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 3 - fix a method in nsStyleDisplay which uses moved methods

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 4 - move style computation code and remove dependency declaration

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 5 - fix computed style

Comment 8

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 6 - fix other property usage

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.

Comment 9

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

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

Comment 10

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

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)

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

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

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

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

Comment 13

[Xidorn Quan [:xidorn] UTC+10]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a6dbd23da598

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/a6dbd23da598

Comment 15

[Xidorn Quan [:xidorn] UTC+10]

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.

Comment 16

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

If you want to back out the patch, you should do it sooner rather than later.

Comment 17

[Xidorn Quan [:xidorn] UTC+10]

Thanks for reminding. I'll try as soon as possible.

Comment 18

[Xidorn Quan [:xidorn] UTC+10]

Backout in https://hg.mozilla.org/integration/mozilla-inbound/rev/24700c20b325

Comment 19

[Xidorn Quan [:xidorn] UTC+10]

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.

Comment 20

[Xidorn Quan [:xidorn] UTC+10]

With the architecture of stylo, I think we no longer need to move properties around to remove dependencies, so closing this as WONTFIX.