1465118 - Some small nsDisplayTransform improvements

Status: RESOLVED FIXED

(Core :: Web Painting, enhancement)

Description

[Miko Mynttinen]

These patches disable/remove dead code, and cache the inverted transform matrix, since retained DL allows us to do that.

Comment 1

[Miko Mynttinen]

Attachment: Bug 1465118 - Part 1: Disable broken nsDisplayTransform::IsUniform() logic to avoid unnecessary work

Review commit: https://reviewboard.mozilla.org/r/247612/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/247612/

Comment 2

[Miko Mynttinen]

Attachment: Bug 1465118 - Part 2: Remove unused nsDisplayTransform contructor

Review commit: https://reviewboard.mozilla.org/r/247614/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/247614/

Comment 3

[Miko Mynttinen]

Attachment: Bug 1465118 - Part 3: Use Maybe<T> to cache transformation matrix

Review commit: https://reviewboard.mozilla.org/r/247616/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/247616/

Comment 4

[Miko Mynttinen]

Attachment: Bug 1465118 - Part 4: Cache inverted transformation matrix for nsDisplayTransform

Review commit: https://reviewboard.mozilla.org/r/247618/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/247618/

Comment 5

[Miko Mynttinen]

Comment on attachment 8981497 [details]
Bug 1465118 - Part 2: Remove unused nsDisplayTransform contructor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/247614/diff/1-2/

Comment 6

[Miko Mynttinen]

Comment on attachment 8981498 [details]
Bug 1465118 - Part 3: Use Maybe<T> to cache transformation matrix

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/247616/diff/1-2/

Comment 7

[Miko Mynttinen]

Comment on attachment 8981499 [details]
Bug 1465118 - Part 4: Cache inverted transformation matrix for nsDisplayTransform

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/247618/diff/1-2/

Comment 8

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8981496 [details]
Bug 1465118 - Part 1: Disable broken nsDisplayTransform::IsUniform() logic to avoid unnecessary work

https://reviewboard.mozilla.org/r/247612/#review253712

::: layout/painting/nsDisplayList.cpp:9036
(Diff revision 1)
>   * works.
>   */
>  Maybe<nscolor>
>  nsDisplayTransform::IsUniform(nsDisplayListBuilder *aBuilder) const
>  {
> +#if 0

Can we just delete this code instead?

Comment 9

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8981497 [details]
Bug 1465118 - Part 2: Remove unused nsDisplayTransform contructor

https://reviewboard.mozilla.org/r/247614/#review253714

Comment 10

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8981498 [details]
Bug 1465118 - Part 3: Use Maybe<T> to cache transformation matrix

https://reviewboard.mozilla.org/r/247616/#review253716

Comment 11

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8981499 [details]
Bug 1465118 - Part 4: Cache inverted transformation matrix for nsDisplayTransform

https://reviewboard.mozilla.org/r/247618/#review253718

::: layout/painting/nsDisplayList.cpp:8511
(Diff revision 2)
> +{
> +  if (mInverseTransform) {
> +    return *mInverseTransform;
> +  }
> +
> +  MOZ_ASSERT(!GetTransform().IsSingular());

How do we prevent this from being hit?

CSS can specify an arbitrary 4x4 matrix, which can definitely be singular.

Maybe that means we end up with an empty visible region and don't build the display item?

Comment 12

[Miko Mynttinen]

(In reply to Matt Woodrow (:mattwoodrow) from comment #8)
> Comment on attachment 8981496 [details]
> Bug 1465118 - Part 1: Disable broken nsDisplayTransform::IsUniform() logic
> to avoid unnecessary work
> 
> https://reviewboard.mozilla.org/r/247612/#review253712
> 
> ::: layout/painting/nsDisplayList.cpp:9036
> (Diff revision 1)
> >   * works.
> >   */
> >  Maybe<nscolor>
> >  nsDisplayTransform::IsUniform(nsDisplayListBuilder *aBuilder) const
> >  {
> > +#if 0
> 
> Can we just delete this code instead?

I was thinking of fixing this as part of the bug 1464923. Or do you think that it is not worth it?

(In reply to Matt Woodrow (:mattwoodrow) from comment #11)
> Comment on attachment 8981499 [details]
> Bug 1465118 - Part 4: Cache inverted transformation matrix for
> nsDisplayTransform
> 
> https://reviewboard.mozilla.org/r/247618/#review253718
> 
> ::: layout/painting/nsDisplayList.cpp:8511
> (Diff revision 2)
> > +{
> > +  if (mInverseTransform) {
> > +    return *mInverseTransform;
> > +  }
> > +
> > +  MOZ_ASSERT(!GetTransform().IsSingular());
> 
> How do we prevent this from being hit?
> 
> CSS can specify an arbitrary 4x4 matrix, which can definitely be singular.
> 
> Maybe that means we end up with an empty visible region and don't build the
> display item?

This same check is done at the only call site of this function, nsDisplayTransform::UntransformRect().

Comment 13

[Matt Woodrow (:mattwoodrow)]

(In reply to Miko Mynttinen [:miko] from comment #12)
> (In reply to Matt Woodrow (:mattwoodrow) from comment #8)
> > Comment on attachment 8981496 [details]
> > Bug 1465118 - Part 1: Disable broken nsDisplayTransform::IsUniform() logic
> > to avoid unnecessary work
> > 
> > https://reviewboard.mozilla.org/r/247612/#review253712
> > 
> > ::: layout/painting/nsDisplayList.cpp:9036
> > (Diff revision 1)
> > >   * works.
> > >   */
> > >  Maybe<nscolor>
> > >  nsDisplayTransform::IsUniform(nsDisplayListBuilder *aBuilder) const
> > >  {
> > > +#if 0
> > 
> > Can we just delete this code instead?
> 
> I was thinking of fixing this as part of the bug 1464923. Or do you think
> that it is not worth it?

Without a use case where we can see that it matters, I certainly wouldn't bother.

Comment 14

[Miko Mynttinen]

Comment on attachment 8981496 [details]
Bug 1465118 - Part 1: Disable broken nsDisplayTransform::IsUniform() logic to avoid unnecessary work

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/247612/diff/1-2/

Comment 15

[Miko Mynttinen]

Comment on attachment 8981497 [details]
Bug 1465118 - Part 2: Remove unused nsDisplayTransform contructor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/247614/diff/2-3/

Comment 16

[Miko Mynttinen]

Comment on attachment 8981498 [details]
Bug 1465118 - Part 3: Use Maybe<T> to cache transformation matrix

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/247616/diff/2-3/

Comment 17

[Miko Mynttinen]

Comment on attachment 8981499 [details]
Bug 1465118 - Part 4: Cache inverted transformation matrix for nsDisplayTransform

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/247618/diff/2-3/

Comment 18

[Miko Mynttinen]

(In reply to Matt Woodrow (:mattwoodrow) from comment #13)
> (In reply to Miko Mynttinen [:miko] from comment #12)
> > (In reply to Matt Woodrow (:mattwoodrow) from comment #8)
> > > Comment on attachment 8981496 [details]
> > > Bug 1465118 - Part 1: Disable broken nsDisplayTransform::IsUniform() logic
> > > to avoid unnecessary work
> > > 
> > > https://reviewboard.mozilla.org/r/247612/#review253712
> > > 
> > > ::: layout/painting/nsDisplayList.cpp:9036
> > > (Diff revision 1)
> > > >   * works.
> > > >   */
> > > >  Maybe<nscolor>
> > > >  nsDisplayTransform::IsUniform(nsDisplayListBuilder *aBuilder) const
> > > >  {
> > > > +#if 0
> > > 
> > > Can we just delete this code instead?
> > 
> > I was thinking of fixing this as part of the bug 1464923. Or do you think
> > that it is not worth it?
> 
> Without a use case where we can see that it matters, I certainly wouldn't
> bother.

This code is now removed.

Comment 19

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8981496 [details]
Bug 1465118 - Part 1: Disable broken nsDisplayTransform::IsUniform() logic to avoid unnecessary work

https://reviewboard.mozilla.org/r/247612/#review254910

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2bb74a17a766
Part 1: Disable broken nsDisplayTransform::IsUniform() logic to avoid unnecessary work r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/6a9d2ad04ed2
Part 2: Remove unused nsDisplayTransform contructor r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/e6e9ebdf189f
Part 3: Use Maybe<T> to cache transformation matrix r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/74cadc564216
Part 4: Cache inverted transformation matrix for nsDisplayTransform r=mattwoodrow

Comment 21

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/2bb74a17a766
https://hg.mozilla.org/mozilla-central/rev/6a9d2ad04ed2
https://hg.mozilla.org/mozilla-central/rev/e6e9ebdf189f
https://hg.mozilla.org/mozilla-central/rev/74cadc564216