Closed Bug 1248817 Opened 4 years ago Closed 4 years ago

With new Android 4.3 AVD, dom/base/test/test_domwindowutils.html | check x - got +0, expected 1

Categories

(Firefox for Android :: Testing, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 47
Tracking Status
firefox47 --- fixed

People

(Reporter: gbrown, Assigned: gbrown)

References

Details

Attachments

(1 file)

In bug 1246797, I want to deploy a new Android 4.3 AVD, just like the old one, but with the screen width increased to 800. With that AVD, test_domWindowUtils.html consistently fails.


As seen at https://treeherder.mozilla.org/#/jobs?repo=try&revision=d9fdcae61708&filter-resultStatus=success&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable,

http://archive.mozilla.org/pub/mobile/try-builds/gbrown@mozilla.com-d9fdcae61708423e958c35abd5c590baa3b09426/try-android-api-15/try_ubuntu64_vm_armv7_large_test-mochitest-1-bm52-tests1-linux64-build187.txt.gz

16:50:32     INFO -  313 INFO TEST-START | dom/base/test/test_domwindowutils.html
16:50:32     INFO -  314 INFO TEST-UNEXPECTED-FAIL | dom/base/test/test_domwindowutils.html | check x - got +0, expected 1
16:50:32     INFO -      SimpleTest.is@SimpleTest/SimpleTest.js:267:5
16:50:32     INFO -      listener@dom/base/test/test_domwindowutils.html:23:5
16:50:32     INFO -      test_sendMouseEventDefaults@dom/base/test/test_domwindowutils.html:38:3
16:50:32     INFO -      next@dom/base/test/test_domwindowutils.html:71:3
16:50:32     INFO -  315 INFO TEST-PASS | dom/base/test/test_domwindowutils.html | check y
16:50:32     INFO -  316 INFO TEST-PASS | dom/base/test/test_domwindowutils.html | check button
16:50:32     INFO -  317 INFO TEST-PASS | dom/base/test/test_domwindowutils.html | check click count
16:50:32     INFO -  318 INFO TEST-PASS | dom/base/test/test_domwindowutils.html | check modifiers
16:50:32     INFO -  319 INFO TEST-PASS | dom/base/test/test_domwindowutils.html | check pressure
16:50:32     INFO -  320 INFO TEST-PASS | dom/base/test/test_domwindowutils.html | check input source
16:50:32     INFO -  321 INFO TEST-PASS | dom/base/test/test_domwindowutils.html | check isSynthesized is undefined in content
16:50:32     INFO -  322 INFO TEST-PASS | dom/base/test/test_domwindowutils.html | check isSynthesized is true from chrome
16:50:43     INFO -  323 INFO TEST-PASS | dom/base/test/test_domwindowutils.html | explicit input source is valid
16:50:43     INFO -  324 INFO TEST-PASS | dom/base/test/test_domwindowutils.html | we can dispatch event that don't look synthesized
16:50:43     INFO -  325 INFO TEST-OK | dom/base/test/test_domwindowutils.html | took 1328ms
If I modify the test such that x=2, or something other than 1, then the test passes. Only x=1 causes trouble (received as +0). Rounding? Loss of precision on coordinate translation?
nsContentUtils::SendMouseEvent() calls nsContentUtils::ToWidgetPoint() -- http://hg.mozilla.org/mozilla-central/annotate/a9e33d8c48b5/dom/base/nsContentUtils.cpp#l7695 -- which transforms the CSSPoint(1.0,2.0) to LayoutDeviceIntPoint(2,3):
 
  CSSPoint::ToAppUnits(1.0,2.0) -> nsPoint(60,120)
  nsPoint(60,120)+offset -> nsPoint(180,240)
  nsPoint(180,240).ApplyResolution(nsLayoutUtils::GetCurrentAPZResolutionScale(aPresContext->PresShell())) -> nsPoint(73,98)
  LayoutDeviceIntPoint::FromAppUnitsRounded(nsPoint(73,98),aPresContext->AppUnitsPerDevPixel()) -> LayoutDeviceIntPoint(2,3)

Prior to event handling, Event::GetClientCoords() -- http://hg.mozilla.org/mozilla-central/annotate/789a12291942/dom/events/Event.cpp#l960 -- transforms LayoutDeviceIntPoint(2,3) to a CSSIntPoint:

  nsLayoutUtils::GetEventCoordinatesRelativeTo(...) -> nsPoint(27,101)
  CSSIntPoint::FromAppUnitsRounded(nsPoint(27,101)) -> CSSIntPoint(0,2)
:kats -- dom/base/test/test_domwindowutils.html sends a "mousedown" event and verifies that the received event has the expected content. When I increase my Android emulator screen width, the test fails, because the x coordinate of the mousedown event changes in transit. nsContentUtils::ToWidgetPoint(1.0,2.0) produces LayoutDeviceIntPoint(2,3) while Event::GetClientCoords(2,3) produces CSSIntPoint(0,2), which is received in the test.
 
On the one hand, the test's expectation -- to receive what was sent -- seems not unreasonable. But seeing the coordinate transforms, I'm not surprised that there is sometimes a difference. Should I make the test more forgiving, or is there cause for concern here?
Flags: needinfo?(bugmail.mozilla)
I think in this case making the test more forgiving is probably ok, since it's rounding error. However I'm just concerned we might using different roundings in different places in the code which might be incorrect. See below.

(In reply to Geoff Brown [:gbrown] from comment #2)
> nsPoint(180,240).ApplyResolution(nsLayoutUtils::
> GetCurrentAPZResolutionScale(aPresContext->PresShell())) -> nsPoint(73,98)

Based on this it seems like the APZ's resolution scale is around 2.45, but then

>  nsLayoutUtils::GetEventCoordinatesRelativeTo(...) -> nsPoint(27,101)

I'm not sure what happens inside this conversion to produce 27, 101. I tried various combinations of that scale and an offset to get from LayoutDeviceIntPoint(2, 3) to get that 27,101 but wasn't successful - if it's not too much work could you trace through that code path like you did for the other one? In particular if there's any round-down or round-up (as opposed to round-to-nearest) then that might be a bug.
Flags: needinfo?(bugmail.mozilla)
No problem.

nsPoint
nsLayoutUtils::GetEventCoordinatesRelativeTo(nsIWidget* aWidget,
                                             const LayoutDeviceIntPoint& aPoint,
                                             nsIFrame* aFrame)
{
>>> aPoint = (2,3)

 ...

  nsPoint widgetToView = TranslateWidgetToView(rootFrame->PresContext(),
                                               aWidget, aPoint, rootView);

>>> widgetToView=(60,90)

  if (widgetToView == nsPoint(NS_UNCONSTRAINEDSIZE, NS_UNCONSTRAINEDSIZE)) {
    return nsPoint(NS_UNCONSTRAINEDSIZE, NS_UNCONSTRAINEDSIZE);
  }

  // Convert from root document app units to app units of the document aFrame
  // is in.
  int32_t rootAPD = rootFrame->PresContext()->AppUnitsPerDevPixel();
  int32_t localAPD = aFrame->PresContext()->AppUnitsPerDevPixel();
  widgetToView = widgetToView.ScaleToOtherAppUnits(rootAPD, localAPD);

>>> rootAPD=30, localAPD=30, widgetToView=(60,90)

  nsIPresShell* shell = aFrame->PresContext()->PresShell();

  // XXX Bug 1224748 - Update nsLayoutUtils functions to correctly handle nsPresShell resolution

  widgetToView = widgetToView.RemoveResolution(GetCurrentAPZResolutionScale(shell));

>>> GetCurrentAPZResolutionScale()=0.408163
>>> widgetToView=(147,221)

  /* If we encountered a transform, we can't do simple arithmetic to figure
   * out how to convert back to aFrame's coordinates and must use the CTM.
   */
  if (transformFound || aFrame->IsSVGText()) {
    return TransformRootPointToFrame(aFrame, widgetToView);
  }

  /* Otherwise, all coordinate systems are translations of one another,
   * so we can just subtract out the difference.
   */
  return widgetToView - aFrame->GetOffsetToCrossDoc(rootFrame);

>>> aFrame->GetOffsetToCrossDoc()=(120,120)
>>> returns (27, 101)

}
Yeah, ok, we should just make the test more lenient. The (73,98) basically gets rounded to (60,90) and at a scale of 2.45 the deltaX of 13 gets scaled up to ~32 app units. 30 app units is one LayoutDevice pixel here, so the final LayoutDevice value is off by more than one pixel. Rounding error at its finest.
Simple change to test_domwindowutils.html to allow for rounding of coordinates in transit. This will allow the test to pass on a wider range of Android devices and allow me to proceed with bug 1246797.
Attachment #8723215 - Flags: review?(poirot.alex)
Comment on attachment 8723215 [details] [diff] [review]
be more permissive when checking event .x and .y, to allow for rounding

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

::: dom/base/test/test_domwindowutils.html
@@ +19,5 @@
>  
>    window.addEventListener("mousedown", function listener(evt) {
>      window.removeEventListener("mousedown", listener);
>      // Mandatory args
> +    // co-ordinates may change slightly due to rounding

coordinates?

@@ +21,5 @@
>      window.removeEventListener("mousedown", listener);
>      // Mandatory args
> +    // co-ordinates may change slightly due to rounding
> +    ok((evt.clientX <= x+1) && (evt.clientX >= x-1), "check x");
> +    ok((evt.clientY <= y+1) && (evt.clientY >= y-1), "check y");

This is so weird. But why not if you really have to.

Couldn't you, instead, use another set of x, y that would be mapped to the same evt.clientX/evt.clientY?
Today we use hardcoded (1, 2), but we could use something else. Something computed based on the environment if needed?

Or, we could compute the expected value based on the environment?

I know about nothing in graphics, but note that you have access to nsIDOMWindowUtils interface (`utils` object):
http://mxr.mozilla.org/mozilla-central/source/dom/interfaces/base/nsIDOMWindowUtils.idl#1007
(In reply to Alexandre Poirot [:ochameau] from comment #8)
> > +    // co-ordinates may change slightly due to rounding
> 
> coordinates?

Sure. Or I could say "clientX and clientY", if you prefer.
 
> @@ +21,5 @@
> >      window.removeEventListener("mousedown", listener);
> >      // Mandatory args
> > +    // co-ordinates may change slightly due to rounding
> > +    ok((evt.clientX <= x+1) && (evt.clientX >= x-1), "check x");
> > +    ok((evt.clientY <= y+1) && (evt.clientY >= y-1), "check y");
> 
> This is so weird. But why not if you really have to.
> 
> Couldn't you, instead, use another set of x, y that would be mapped to the
> same evt.clientX/evt.clientY?

Yes, I could choose different x, y that are known to not be modified due to rounding with the new Android screen size. I'm not sure if it is possible to choose x, y that are guaranteed not to be rounded for any screen size, so the test would still be vulnerable to this type of failure. Also, if the internal algorithms for coordinate transformations change, that might change the test result for any chosen x, y. I would hate to see someone need to go through the same debugging effort in the future.

> Today we use hardcoded (1, 2), but we could use something else. Something
> computed based on the environment if needed?
> 
> Or, we could compute the expected value based on the environment?
> 
> I know about nothing in graphics, but note that you have access to
> nsIDOMWindowUtils interface (`utils` object):
> http://mxr.mozilla.org/mozilla-central/source/dom/interfaces/base/
> nsIDOMWindowUtils.idl#1007

Yes, we likely have access to the basic information we need, but I think we would need to parallel the computations done in nsContentUtils and nsLayoutUtils, which are quite complex. And again, those algorithms could change over time.

I think it is best to allow for x+/-1 so that the test tolerates rounding regardless of environment, but I could also choose non-problematic x, y, or skip this test on Android -- your choice?
Comment on attachment 8723215 [details] [diff] [review]
be more permissive when checking event .x and .y, to allow for rounding

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

(In reply to Geoff Brown [:gbrown] from comment #9)
> (In reply to Alexandre Poirot [:ochameau] from comment #8)
> > > +    // co-ordinates may change slightly due to rounding
> > 
> > coordinates?
> 
> Sure. Or I could say "clientX and clientY", if you prefer.

I just meant to replace co-ordinates by coordinates.

Ok, for some rounding then.

Do you think you could at least compute the bias limit?
Here you only accept +/-1, but in comment 6 :kats says it can be bigger than one.
I imagine the maximum possible difference can be computed?
Attachment #8723215 - Flags: review?(poirot.alex) → review+
I think the max difference would come from a rounding away of 15 app units, with a max resolution of 4.0, which would scale the 15 up to 60, which would be two LD pixels. So making it accept +/-2 should cover it.
https://hg.mozilla.org/mozilla-central/rev/9334bc77b215
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
You need to log in before you can comment on or make changes to this bug.