Kartikaya Gupta (email:kats@mozilla.staktrace.com) Reporter
	Description • 10 years ago

Bug 945203 added some regions to the layer tree for APZ hit-testing purposes. However when trying to determine what coordinate space these regions are in I ran into some problems.

Discussing with mattwoodrow on IRC, it seems like these regions should be in "layer pixel" space, but they don't seem to properly take into account the resolution. I was able to reproduce this on a OS X desktop build using the latest m-c code.

Steps:
1. Enable building of the event regions by uncommenting the commented-out code in IsBuildingLayerEventRegions() in nsDisplayList.h
2. Build desktop firefox with --enable-dump-painting
3. Run Firefox with the MOZ_DUMP_PAINT_LIST=1 env var set, starting with a fresh profile
3. In the new profile, I set the "layers.offmainthreadcomposition.async-animations" pref to true (may not be needed for this test case specifically, but I was testing a few other cases too) and also set the "devtools.chrome.enabled" pref to true
4. Load the attached test case

At this point, observe the layer tree output. I saw output for the content area that included this:

ClientThebesLayer (0x11c77f800) [transform=[ 1 0; 0 1; 0 77; ]] [visible=< (x=0, y=0, w=1080, h=578); >] [dispatchtocontentregion=< (x=-34, y=-34, w=284, h=284);>]

This makes sense and looks fine. Specifically, the dispatchtocontentregion rect is the bounding box of the rotated rect. The "77" in the transform is the height of the browser chrome above the content area.

Now:
5. Open the scratchpad from the developer tools menu
6. Switch the scratchpad over to the browser context from the "environment" menu
7. Enter and run the following snippet in the scratchpad (note this assumes the test page is the only tab you have open, adjust as needed):

let w = gBrowser.tabs[0].linkedBrowser._contentWindow;
let Ci = Components.interfaces;
let dwu = w.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindowUtils);
dwu.setResolution(2.0, 2.0);

The above snippet sets a 2.0 resolution on the content. Now observe the layer tree dump again.

In the new layer tree dump I see this:

ClientThebesLayer (0x11c741400) [transform=[ 1 0; 0 1; 0 154; ]] [visible=< (x=0, y=0, w=2160, h=1156); >] [dispatchtocontentregion=< (x=-34, y=-111, w=284, h=284); >]

The y-transform on the layer has doubled to 154, which makes sense consider the parent container layer now has a 2.0 transform. However, the dispatchtocontentregion *size* has not changed, but the y-offset *has* changed. These changes don't make sense to me in any coordinate system.

Matt pointed me to the code at [1] where I determined that mParameters.mXScale and mYScale were both 1.0 when they should be 2.0?

[1] http://mxr.mozilla.org/mozilla-central/source/layout/base/FrameLayerBuilder.cpp?rev=7193af53cd1e#2565

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 1 • 10 years ago

They definitely should be in layer pixel space!

FrameLayerBuilder has:
        data->AccumulateEventRegions(ScaleRegionToOutsidePixels(eventRegions->HitRegion()),
                                     ScaleRegionToOutsidePixels(eventRegions->MaybeHitRegion()),                                    ScaleRegionToOutsidePixels(eventRegions->DispatchToContentHitRegion()));

ScaleToOutsidePixels is the same conversion we use for converting display item coordinates in appunits to layer pixels.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Reporter
	Comment 2 • 10 years ago

Yeah that's what Matt said as well. See the last couple of lines of my comment 0 - mParameters seemed to have unexpected values.

Are you able to reproduce using the steps I provided? The STR are for a desktop build so it should be easier for you.

	Milan Sreckovic [:milan] (needinfo for best results)
	Comment 3 • 10 years ago

Rob, I'm going to assign this to you so that we don't leave a 1.4+ unowned, and you seem to be up to speed as to what is going on.  If you're not the right person, can you reassign?

	Milan Sreckovic [:milan] (needinfo for best results)
	Comment 4 • 10 years ago

Bug 918288 no longer 1.4 blocker.  Let's talk about this and related bugs during the work week.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Reporter
	Comment 5 • 10 years ago

tn, mind looking into this if you get a chance? As you said it might be trivial. (If it turns out to be nontrivial please let me know)

	Timothy Nikkel (:tnikkel) Assignee
	Comment 6 • 10 years ago

Inactive layer managers always have a scale of 1. So when we come across a layer event regions when processing display items we scale and round to layer pixels assuming a scale of one. When we pass these regions up to the active thebes layer containing them we don't do any adjustment to handle this.

It seems kind of silly to scale and round to completely fictitious and wrong pixels inside layer managers by assuming the scale is 1. This applies to all rects that we scale.

We could pass down the parent scale into inactive layer managers in some form (active parent scale) so that they do scaling and rounding correctly to fix this.

The cleanest way to fix this seemed to be to just keep event regions at app units relative to the reference frame until we actually put the regions onto their final layer. An existing bug was that event regions were already relative to the reference frame and not the thebes layer (even on the layer). When passing a region up from an inactive layer manager we just have to convert it to be relative to a new reference frame if the reference frame changes outside the inactive container layer.

Is there a better way?

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 8 • 10 years ago

Comment on attachment 8443077 [details] [diff] [review]
Make event regions be in app units relative to reference frame

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

nice

	Timothy Nikkel (:tnikkel) Assignee
	Comment 9 • 10 years ago

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

	Ed Morley [:emorley]
	Comment 10 • 10 years ago

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

	Markus Stange [:mstange]
	Comment 11 • 10 years ago

The patch here undoes bug 1016525.

	Timothy Nikkel (:tnikkel) Assignee
	Comment 12 • 10 years ago

(In reply to Markus Stange [:mstange] from comment #11)
> The patch here undoes bug 1016525.

Ah, thanks for the info.