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.

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.

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; }

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

Updated to work correctly with non 1:1 css:device pixel ratios when rotation is involved

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.

(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

(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

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/3622141b2165 https://hg.mozilla.org/integration/mozilla-inbound/rev/a2395a987881 https://hg.mozilla.org/integration/mozilla-inbound/rev/d6f58fa82ea7 https://hg.mozilla.org/integration/mozilla-inbound/rev/ef2e0154fc2e

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

https://hg.mozilla.org/mozilla-central/rev/3622141b2165 https://hg.mozilla.org/mozilla-central/rev/a2395a987881 https://hg.mozilla.org/mozilla-central/rev/d6f58fa82ea7 https://hg.mozilla.org/mozilla-central/rev/ef2e0154fc2e