Closed Bug 979658 Opened 6 years ago Closed 6 years ago

Add a version of GetOMTAStyle that *only* gets the OMTA style

Categories

(Core :: Graphics: Layers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: birtles, Assigned: birtles)

References

Details

Attachments

(4 files, 2 obsolete files)

In testing OMTA it is necessary to be able to test the value on the compositor thread. DOMWindowUtils.getOMTAOrComputedStyle, however, falls back to getting the computed style when there is no value on the compositor thread. This masks a range of bugs where animations fail to run at all (since the main thread thinks the animation is running on the compositor but the compositor thread didn't get the message or failed to run it for some reason).

I'd like to add DOMWindowUtils.getOMTAStyle which *only* gets the style on the compositor and returns an empty string otherwise.

Originally I patches for this for bug 964646, but I need them for bug 975261 so I'm splitting them off into a separate bug.
Every other exposed method in nsDOMWindowUtils except getViewPortInfo and
getViewId performs this check. This patch makes getOMTAOrComputedStyle check the
called is chrome as well.
Assignee: nobody → birtles
Status: NEW → ASSIGNED
nsDOMWindowUtils.getOMTAOrComputedStyle falls back to using getComputedStyle
when an OMTA style is not available. However, in order to be sure we are testing
OMTA this patch adds getOMTAStyle that returns an empty string if no OMTA style
is available.

This patch also includes some minor stylistic tweaks. The method signature for
getOMTAOrComputedStyle now takes an nsIDOMElement parameter rather than
nsIDOMNode in order to simplify error-checking. (When we support OMTA of
pseudo-elements we will have to adjust the method signature but for now we only
support elements.) Also, some lines have been wrapped, ErrorResult is
declared closer to where it is used, and the return value aResult is only
truncated when returning NS_OK.
LayerTransactionParent::RecvGetTransform takes care to reverse all the
transformations applied by AsyncCompositionManager::SampleValue to the CSS
values calculated so that it can return CSS values for testing. However, it
fails to revert the conversion from CSS pixels to device pixels applied to the
translation components of the transform by
nsStyleTransformMatrix::ProcessTranslatePart as called by
nsDisplayTransform::GetResultingTransformMatrix.

This patch converts the resulting transform's translation components from device
pixels back to CSS pixels. It also adds documentation for the other operations
in LayerTransactionParent::RecvGetTransform.
PLayerTransaction.GetTransform doesn't actually return the same kind of value
when the transform on the layer is not set by animation. This is because it uses
information stored with the animation to undo various transforms. We shouldn't
pretend to return something useful/similar when we don't have that information
available.

This patch renames GetTransform to GetAnimationTransform and makes it take an
extra parameter to indicate if the layer's transform is set by animation or not.
Attachment #8385757 - Flags: review?(dzbarsky)
Attachment #8385759 - Flags: review?(dbaron)
Attachment #8385762 - Flags: review?(dzbarsky)
Attachment #8385764 - Flags: review?(dzbarsky)
Comment on attachment 8385764 [details] [diff] [review]
part 4 - Rename PLayerTransaction.GetTransform to GetAnimationTransform

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

::: gfx/layers/ipc/PLayerTransaction.ipdl
@@ +81,5 @@
> +  // factoring out translation components introduced to account for the offset
> +  // of the corresponding frame and transform origin and after converting to CSS
> +  // pixels.
> +  sync GetAnimationTransform(PLayer layer) returns (gfx3DMatrix transform,
> +                                                    bool isAnimated);

It seems weird to return a transform that's not animated here...How about returning something like

union MaybeTransform {
  gfx3DMatrix;
  void_t;
}
Attachment #8385757 - Flags: review?(dzbarsky) → review+
There's a chance part 3 might be wrong. I'm currently investigating getting test_transitions_per_property.html to pass on my machine (which has a non 1:1 diplay pixel:css pixel setting) and it fails all over the place both with and without these patches. (Although it fails slightly less with these patches).
Comment on attachment 8385759 [details] [diff] [review]
part 2 - Add nsDOMWindowUtils.getOMTAStyle

r=dbaron
Attachment #8385759 - Flags: review?(dbaron) → review+
Updated to work correctly with non 1:1 css:device pixel ratios when rotation is involved
Attachment #8385762 - Attachment is obsolete: true
Attachment #8385762 - Flags: review?(dzbarsky)
Attachment #8386559 - Flags: review?(dzbarsky)
PLayerTransaction.GetTransform doesn't actually return the same kind of value
when the transform on the layer is not set by animation. This is because it uses
information stored with the animation to undo various transforms. We shouldn't
pretend to return something useful/similar when we don't have that information
available.

This patch renames GetTransform to GetAnimationTransform and makes it return
a union that has type void_t if the layer is not transformed by animation.
Attachment #8385764 - Attachment is obsolete: true
Attachment #8385764 - Flags: review?(dzbarsky)
Attachment #8386573 - Flags: review?(dzbarsky)
(In reply to David Zbarsky (:dzbarsky) from comment #5)
> Comment on attachment 8385764 [details] [diff] [review]
> part 4 - Rename PLayerTransaction.GetTransform to GetAnimationTransform
> 
> Review of attachment 8385764 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/ipc/PLayerTransaction.ipdl
> @@ +81,5 @@
> > +  // factoring out translation components introduced to account for the offset
> > +  // of the corresponding frame and transform origin and after converting to CSS
> > +  // pixels.
> > +  sync GetAnimationTransform(PLayer layer) returns (gfx3DMatrix transform,
> > +                                                    bool isAnimated);
> 
> It seems weird to return a transform that's not animated here...How about
> returning something like
> 
> union MaybeTransform {
>   gfx3DMatrix;
>   void_t;
> }

Fixed.
Comment on attachment 8386559 [details] [diff] [review]
part 3 - Make LayerTransactionParent::RecvGetTransform convert to CSS pixels

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

I think I managed to convince myself that this undoes the transformations properly, and this is testing code, so if it passes try, r=me

But I think it would be better to try to use the Point and Scale types (in layout/base/Units.h) to prevent these kinds of mistakes.

::: gfx/layers/ipc/LayerTransactionParent.cpp
@@ +591,5 @@
>    for (uint32_t i=0; i < layer->GetAnimations().Length(); i++) {
>      if (layer->GetAnimations()[i].data().type() == AnimationData::TTransformData) {
>        const TransformData& data = layer->GetAnimations()[i].data().get_TransformData();
>        scale = data.appUnitsPerDevPixel();
>        scaledOrigin =

WDYT of calling this rounded to match the variable in GetResultingTransformMatrix?

@@ +608,4 @@
>    aTransform->Translate(-scaledOrigin);
> +
> +  // Undo the rebasing applied by
> +  // nsDisplayTransform::GetResultingTransformMatrix

GetResultingTransformMatrixInternal
Attachment #8386559 - Flags: review?(dzbarsky) → review+
(In reply to David Zbarsky (:dzbarsky) from comment #11)
> I think I managed to convince myself that this undoes the transformations
> properly, and this is testing code, so if it passes try, r=me

As far as I can tell, on try we don't have any machines with OMTA enabled and a non 1:1 dev:css pixel ratio so try doesn't help. But I've tested it locally and added debugging so that at each point the transformation is manipulated it is printed out. Then I've verified that we perform the exact opposite series of transformations when recovering the transform values for the test cases in test_transitions_per_property.html. So I'm pretty confident that for that set of inputs it's correct.

> But I think it would be better to try to use the Point and Scale types (in
> layout/base/Units.h) to prevent these kinds of mistakes.
> 
> ::: gfx/layers/ipc/LayerTransactionParent.cpp
> @@ +591,5 @@
> >    for (uint32_t i=0; i < layer->GetAnimations().Length(); i++) {
> >      if (layer->GetAnimations()[i].data().type() == AnimationData::TTransformData) {
> >        const TransformData& data = layer->GetAnimations()[i].data().get_TransformData();
> >        scale = data.appUnitsPerDevPixel();
> >        scaledOrigin =
> 
> WDYT of calling this rounded to match the variable in
> GetResultingTransformMatrix?
> 
> @@ +608,4 @@
> >    aTransform->Translate(-scaledOrigin);
> > +
> > +  // Undo the rebasing applied by
> > +  // nsDisplayTransform::GetResultingTransformMatrix
> 
> GetResultingTransformMatrixInternal

Thanks, I'll look into these next week.
Comment on attachment 8386573 [details] [diff] [review]
part 4 - Rename PLayerTransaction.GetTransform to GetAnimationTransform

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

::: gfx/layers/ipc/LayerTransactionParent.h
@@ +98,1 @@
>  

nit: align these
Attachment #8386573 - Flags: review?(dzbarsky) → review+
(In reply to David Zbarsky (:dzbarsky) from comment #11)
> Comment on attachment 8386559 [details] [diff] [review]
> part 3 - Make LayerTransactionParent::RecvGetTransform convert to CSS pixels
...
> But I think it would be better to try to use the Point and Scale types (in
> layout/base/Units.h) to prevent these kinds of mistakes.

I'm going to try doing this in a follow-up patch.

> ::: gfx/layers/ipc/LayerTransactionParent.cpp
> @@ +591,5 @@
> >    for (uint32_t i=0; i < layer->GetAnimations().Length(); i++) {
> >      if (layer->GetAnimations()[i].data().type() == AnimationData::TTransformData) {
> >        const TransformData& data = layer->GetAnimations()[i].data().get_TransformData();
> >        scale = data.appUnitsPerDevPixel();
> >        scaledOrigin =
> 
> WDYT of calling this rounded to match the variable in
> GetResultingTransformMatrix?

I looked at doing this, but the first time 'scaledOrigin' is used here it is to undo the translation in SampleValue where it is called 'scaledOrigin'. It's just that 'rounded' and 'scaledOrigin' happen to have the same value for the limited set of inputs we're interested here (e.g. no SVG etc.).

(Also, I think introducing an extra variable 'rounded' with the same value as 'scaledOrigin' would just make the code more confusing. Long term, as we expand the range of cases we test, we may be able to factor out some pairs of methods that do both the forwards/reverse transforms and make keeping them in sync easier.)

> @@ +608,4 @@
> >    aTransform->Translate(-scaledOrigin);
> > +
> > +  // Undo the rebasing applied by
> > +  // nsDisplayTransform::GetResultingTransformMatrix
> 
> GetResultingTransformMatrixInternal

Done.
(In reply to David Zbarsky (:dzbarsky) from comment #11)
> But I think it would be better to try to use the Point and Scale types (in
> layout/base/Units.h) to prevent these kinds of mistakes.

I tried using these but it made the code more complex and error-prone since there's no 3D version.
You need to log in before you can comment on or make changes to this bug.