869940 - [MP] Story - Widget\WinRT Metro APZC - Part II: Development

Status: RESOLVED FIXED

(Tracking Graveyard :: Metro Operations, defect, P1)

Description

[Marco Mucci [:MarcoM]]

+++ This bug was initially created as a clone of Bug #849266 +++

Part II - Development based on findings from APZC research and development.

Comment 1

[Brian R. Bondy [:bbondy]]

Attachment: WIP rev0.2

Comment 2

[Brian R. Bondy [:bbondy]]

Attachment: WIP rev 0.3

Hey Jim, you mentioned you'd take a look at why the PostDelayedTask callback event never gets run.

Ignore a lot of the ugliness and commented out MetroInput stuff happening in the patch, I know how to fix that already.

You may get a crash on startup, just re-start it again and it should work.  I'm not concerned about that and think I know how to fix that.
So before trying to reproduce you can try loading a web page and panning and zooming.
Manually hide the scrollbars before pannning or else it'll be very jittery. I'm also fixing that.

So the problem can be reproduced on the Metro start page because it handles touch input. 

0.
Apply the patch

pymake -sC gfx
pymake -sC widget
pymake -sC layout/media
pymake -sC toolkit/library
pymake -sC browser/metro

1. 
Inside C:\projects\mozilla\mozilla-central\gfx\layers\ipc\AsyncPanZoomController.cpp
Search for mTouchListenerTimeoutTask and set a breakpoint on line 317 and 318
Also set a breakpoint for the call to mTouchListenerTimeoutTask->Cancel() (just to show it won't get hit)

2.
Set a breakpoint in AsyncPanZoomController::TimeoutTouchListeners
which is the actual delayed posted delayed task that should be executed

3. Set a breapoint in MetroWidget.cpp on this line:
MessageLoop::current()->PostDelayedTask
Note: You can move this code directly in MetroWidget::PostDelayedTask if you want to but there is no need.

4. You'll notice after trying to pan the start page twice, the breakpoint on line 317 gets hit, but not the one on line 318 because the task never gets executed.
You'll notice that the step #3 breakpoint was executed but the step #2 one was not hit at the point of the problem.

Comment 3

[Jim Mathies [:jimm]]

Not really seeing this problem. When debugging using break points in a release build, I did see what seemed to be a lost callback. However using OutputDebugString statements to track everything I get reliable callbacks every time, both in the async pan zoom controller and in the gesture event listener object.

Comment 4

[Brian R. Bondy [:bbondy]]

In case anyone is following along in the bug, we found that the problem only happens when going into suspend mode (I.e. switching away from the Metro browser).  If this happens while a task is posted, it gets eaten up.  I'm close to a fix for that but I will likely post the result/fix in another bug unrelated to apzc.

Comment 5

[Brian R. Bondy [:bbondy]]

Attachment: APZC WinRT/Metro front end Implementation. - rev1.

Notes:
- All of the functionality is gated around the pref: layers.async-pan-zoom.enabled which is disabled in this patch.
- Simply set its value to true inside about:config to enable everything
- SendAsyncScrollDOMEvent is intentionally left blank for this patch, it's not implemented yet on android either.  We can do this in the future after Android implements its end but there's no rush.
- I hide the main wide scrollbars when panning (although there are small ones that overlay on top), this gets rid of most of the panning jitter since axis locking isn't implemented yet. 
- It can be jiterry when zoomed and sometimes you'll see a black flash come up, those are issues in common with android and we'll try to get them fixed in the apzc polishing bug.

Comment 6

[Brian R. Bondy [:bbondy]]

Attachment: Pan Begin and Pan End notifications from gfx/layers/APZC - rev1.

Comment 7

[Jim Mathies [:jimm]]

(In reply to Brian R. Bondy [:bbondy] from comment #5)
> Created attachment 777258 [details] [diff] [review]
> APZC WinRT/Metro front end Implementation. - rev1.

Would you mind splitting the front end js and backed work up into two patches? I think someone like mbrubeck should look over the front end as he's familiar with how the viewport stuff worked in fennec.

Comment 8

[Brian R. Bondy [:bbondy]]

np will do.

Comment 9

[Brian R. Bondy [:bbondy]]

Comment on attachment 777260 [details] [diff] [review]
Pan Begin and Pan End notifications from gfx/layers/APZC - rev1.

ugh canceled the wrong one.

Comment 10

[Brian R. Bondy [:bbondy]]

Attachment: APZC WinRT Implementation. rev1

Comment 11

[Brian R. Bondy [:bbondy]]

Attachment: APZC Metro front end Implementation - rev1.

Comment 12

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 777260 [details] [diff] [review]
Pan Begin and Pan End notifications from gfx/layers/APZC - rev1.

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

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +1410,5 @@
> +  if (mGeckoContentController) {
> +    if (mState == PANNING && aState != PANNING) {
> +      mGeckoContentController->HandlePanEnd();
> +    } else if (mState != PANNING && aState == PANNING) {
> +      mGeckoContentController->HandlePanBegin();

I'm not a fan of running these callbacks while holding the mMonitor lock, because it could basically do arbitrary things and hold the lock for a long time, which will interfere with composition and other critical things. Can we bump this to a callback or something?

Comment 13

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 777300 [details] [diff] [review]
APZC WinRT Implementation. rev1

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

Note that the code in MetroWidget.cpp will be much simplified once the patches from bug 866232 get landed. Just an FYI in case I land that first.

Comment 14

[Matt Brubeck (:mbrubeck)]

Comment on attachment 777301 [details] [diff] [review]
APZC Metro front end Implementation - rev1.

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

This looks okay, though there were some parts I wasn't sure about (see questions below). It makes me wish I had finished bug 851130 so you wouldn't have to write special cases for the start page. :(

::: browser/metro/base/content/NavButtonSlider.js
@@ +151,5 @@
>          this._back.setAttribute("mousedrag", true);
>          this._plus.setAttribute("mousedrag", true);
>          break;
> +
> +      case "touchend":

I think you'll want "if (aEvent.touches.length != 0) break;" here, for similar reasons to the touchstart and touchmove cases.

::: browser/metro/base/content/bindings/browser.js
@@ +454,5 @@
>          if (document.documentURIObject.spec == "about:blank")
>            return;
>  
>          sendAsyncMessage("DOMContentLoaded", { });
> +        Services.obs.notifyObservers(null, "viewport-needs-updating", null);

Do we want to notify observers even if this event happens in a background tab?  If we move this to a message listener in the chrome window, we could filter based on whether the tab is active.

Is DOMContentLoaded the best event to listen for?  In many cases "pageshow" is better because of bfcache behavior.  Or see the MozScrolledAreaChanged/MozAfterPaint listeners in the ContentScroll object in this file.

::: browser/metro/base/content/browser.js
@@ +1366,5 @@
> +        windowUtils = Browser.windowUtils;
> +        element = Elements.startUI;
> +      } else {
> +        windowUtils = Browser.selectedBrowser.contentWindow.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindowUtils);
> +        element = Browser.selectedBrowser.contentDocument.documentElement;

"element" isn't actually used in this case except by the debugging dumps below...

If we can get by without the dump calls, this code could be simplified a bit by merging the code from the if/else statement below into this if/else statement.

@@ +1399,5 @@
> +      if (InputSourceHelper.isPrecise) {
> +        this._apzcWasPrecise = true;
> +        let event = document.createEvent("Events");
> +        event.initEvent("MozImprecisePointer", true, true);
> +        window.dispatchEvent(event);

Why do we fire this "fake" MozImprecisePointer event when panning starts?  Could you add a comment to explain it, please?

Comment 15

[Brian R. Bondy [:bbondy]]

> Why do we fire this "fake" MozImprecisePointer event when panning starts?  
> Could you add a comment to explain it, please?

We do it to hide the scrollbar, for some reason when the scrollbar is shown panning up and down is a bit jittery left to right (within 1-3 pixels).  yup I'll put a comment.  I think this part can be removed once we implement axis locking in the polishing Part III APZC bug.

Comment 16

[Jim Mathies [:jimm]]

Comment on attachment 777300 [details] [diff] [review]
APZC WinRT Implementation. rev1

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

::: widget/windows/winrt/MetroWidget.cpp
@@ +781,5 @@
> +  NS_DECL_ISUPPORTS
> +  MetroCompositorParent(MetroWidget* aMetroWidget, bool aRenderToEGLSurface,
> +                          int aSurfaceWidth, int aSurfaceHeight)
> +      : CompositorParent(aMetroWidget, aRenderToEGLSurface, aSurfaceHeight, aSurfaceHeight),
> +        mMetroWidget(aMetroWidget)

nit - weird indentation here.

@@ +799,5 @@
> +                                   bool isFirstPaint) MOZ_OVERRIDE
> +  {
> +    CompositorParent::ShadowLayersUpdated(aLayerTree, aTargetConfig, isFirstPaint);
> +    Layer* targetLayer = GetLayerManager()->GetPrimaryScrollableLayer();
> +    if (targetLayer && targetLayer->AsContainerLayer() && MetroWidget::sAPZC && targetLayer->AsContainerLayer()->GetFrameMetrics().IsScrollable()) {

nit - wrapping

@@ +815,5 @@
> +
> +      Layer* targetLayer = GetLayerManager()->GetPrimaryScrollableLayer();
> +      if (targetLayer && targetLayer->AsContainerLayer() && MetroWidget::sAPZC) {
> +          FrameMetrics frameMetrics = targetLayer->AsContainerLayer()->GetFrameMetrics();
> +          frameMetrics.mDisplayPort = AsyncPanZoomController::CalculatePendingDisplayPort(frameMetrics, mozilla::gfx::Point(0.0f, 0.0f), mozilla::gfx::Point(0.0f, 0.0f), 0.0);

nit - wrapping

@@ +824,5 @@
> +    return NS_OK;
> +  }
> +
> +protected:
> +  MetroWidget *mMetroWidget;

ick, can this go in an nsCOMPtr?

@@ +1354,5 @@
> +        CSSToScreenScale resolution = mFrameMetrics.CalculateResolution(); /* should be dividing by mFrameMetrics.mDevPixelsPerCSSPixel to get high-dpi to work */
> +        CSSRect compositedRect = mFrameMetrics.CalculateCompositedRectInCssPixels();
> +        ScreenRect dp = (mFrameMetrics.mDisplayPort + mFrameMetrics.mScrollOffset) * resolution;
> +
> +        NS_ConvertASCIItoUTF16 data(nsPrintfCString("{ \"resolution\": %f, \"compositedRect\": { \"width\": %d, \"height\": %d }, \"displayPort\": { \"x\": %d, \"y\": %d, \"width\": %d, \"height\": %d }, \"scrollTo\": { \"x\": %d, \"y\": %d }}", (float)resolution.scale, (int)compositedRect.width, (int)compositedRect.height, (int)mFrameMetrics.mDisplayPort.x, (int)mFrameMetrics.mDisplayPort.y, (int)mFrameMetrics.mDisplayPort.width, (int)mFrameMetrics.mDisplayPort.height, (int)mFrameMetrics.mScrollOffset.x, (int)mFrameMetrics.mScrollOffset.y));

Are we using this in the front end or is this debug related? If it's debug, lets wrap it in a DEBUGGING (or similar) define and default it to off.

Comment 17

[Jim Mathies [:jimm]]

Generally speaking, this isn't working so well on my surface pro. :/ A few oddities:

1) when navigating to a page, the resulting page is stretched to the right. Also UI elements cease to work correctly when this happens (like the overlay buttons and nav bar). This makes the browser unusable and I have to shut down and restart.
2) scrolling is really janky.
3) when tapping on top site tile, the tile briefly expands in size for some reason.
4) pinch/zoom basically don't work.

I recognize this is wip, so I'm find with landing this prefed off, however, the overlay scrollbar button actions don't work when prefed off. I think we need to track that down before this lands.

Comment 18

[Brian R. Bondy [:bbondy]]

(In reply to Jim Mathies [:jimm] from comment #17)
> Generally speaking, this isn't working so well on my surface pro. :/ A few
> oddities:
> 
> 1) when navigating to a page, the resulting page is stretched to the right.
> Also UI elements cease to work correctly when this happens (like the overlay
> buttons and nav bar). This makes the browser unusable and I have to shut
> down and restart.
> 2) scrolling is really janky.
> 3) when tapping on top site tile, the tile briefly expands in size for some
> reason.
> 4) pinch/zoom basically don't work.
> 
> I recognize this is wip, so I'm find with landing this prefed off, however,
> the overlay scrollbar button actions don't work when prefed off. I think we
> need to track that down before this lands.

This isn't a WIP, and I haven't seen those issues. I'll try on my surface pro, maybe high dpi related problems.

Comment 19

[Brian R. Bondy [:bbondy]]

(In reply to Jim Mathies [:jimm] from comment #16)
> > +        NS_ConvertASCIItoUTF16 data(nsPrintfCString("{ \"resolution\": %f, \"compositedRect\": { \"width\": %d, \"height\": %d }, \"displayPort\": { \"x\": %d, \"y\": %d, \"width\": %d, \"height\": %d }, \"scrollTo\": { \"x\": %d, \"y\": %d }}", (float)resolution.scale, (int)compositedRect.width, (int)compositedRect.height, (int)mFrameMetrics.mDisplayPort.x, (int)mFrameMetrics.mDisplayPort.y, (int)mFrameMetrics.mDisplayPort.width, (int)mFrameMetrics.mDisplayPort.height, (int)mFrameMetrics.mScrollOffset.x, (int)mFrameMetrics.mScrollOffset.y));
> 
> Are we using this in the front end or is this debug related? If it's debug,
> lets wrap it in a DEBUGGING (or similar) define and default it to off.

This creates a json object used by the front end code. I'll add a comment.

Comment 20

[Jim Mathies [:jimm]]

Comment on attachment 777301 [details] [diff] [review]
APZC Metro front end Implementation - rev1.

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

plus the nav buttons click problem.

::: browser/metro/base/content/browser.js
@@ +1348,1 @@
>  var ActivityObserver = {

I'm not sure ActivityObserver is the right place for this code. (In fact, I think ActivityObserver is basically dead code we don't need and can be removed.)

How about breaking apzc code out into it's own js file so we can find it?

@@ +1393,5 @@
> +        windowUtils.setDisplayPortForElement(displayPort.x * resolution, displayPort.y * resolution, displayPort.width * resolution, displayPort.height * resolution, element);
> +      }
> +
> +    } else if (aTopic == "apzc-handle-pan-begin") {
> +      dump("APZC pan-begin\n");

Note you can use Util.dumpLn to clean these up.

@@ +1399,5 @@
> +      if (InputSourceHelper.isPrecise) {
> +        this._apzcWasPrecise = true;
> +        let event = document.createEvent("Events");
> +        event.initEvent("MozImprecisePointer", true, true);
> +        window.dispatchEvent(event);

Plus, if we need it I'd suggest making this a call to InputSourceHelper so we have a reference back to where we fire this from.

@@ +1406,5 @@
> +    } else if (aTopic == "apzc-handle-pan-end") {
> +      dump("APZC pan-end\n");
> +      if (this._apzcWasPrecise) {
> +        let event = document.createEvent("Events");
> +        event.initEvent("MozPrecisePointer", true, true);

ditto.

Comment 21

[Brian R. Bondy [:bbondy]]

Attachment: Pan Begin and Pan End notifications from gfx/layers/APZC - rev2.

Without holding the mutex but after state is updated.

Comment 22

[Jim Mathies [:jimm]]

(In reply to Brian R. Bondy [:bbondy] from comment #18)
> This isn't a WIP, and I haven't seen those issues. I'll try on my surface
> pro, maybe high dpi related problems.

well that's good news! :)

Comment 23

[Jim Mathies [:jimm]]

I'll get this going on my series 7 for testing purposes.

Comment 24

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 777847 [details] [diff] [review]
Pan Begin and Pan End notifications from gfx/layers/APZC - rev2.

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

Thanks

Comment 25

[Jim Mathies [:jimm]]

(In reply to Jim Mathies [:jimm] from comment #23)
> I'll get this going on my series 7 for testing purposes.

On this device things improve compared to the surface pro. I'm still having problems with zoom, the page seems to scroll around after you take your fingers off the screen. Also the origin of a pinch is off. I don't think that needs to block initial landing but there's definitely some follow up work to do there.

Also the overlay nav button clicks aren't working on this device either.

Comment 26

[Brian R. Bondy [:bbondy]]

So I figured out where the zoom jumping and various other problems come from, I hadn't disabled the old panning code in input.js

A quick test by commenting these out makes everything run much much more smooth.

//this._onTouchStart(aEvent);
//this._onTouchMove(aEvent);
//this._onTouchEnd(aEvent);

Comment 27

[Jim Mathies [:jimm]]

(In reply to Brian R. Bondy [:bbondy] from comment #26)
> So I figured out where the zoom jumping and various other problems come
> from, I hadn't disabled the old panning code in input.js
> 
> A quick test by commenting these out makes everything run much much more
> smooth.
> 
> //this._onTouchStart(aEvent);
> //this._onTouchMove(aEvent);
> //this._onTouchEnd(aEvent);

One of the things this module does is control touch scrollbar visibility on our custom scroll boxes. Curious if layout supports something like this with apzc, or if we'll still need to handle this in the front end?

Comment 28

[Brian R. Bondy [:bbondy]]

I'll be uncommenting that part of these by the way, and just removing the parts causing the problem, I added on pan start/end notifications into apzc so we can do it there as well if needed.

Comment 29

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 777847 [details] [diff] [review]
Pan Begin and Pan End notifications from gfx/layers/APZC - rev2.

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

I just realized that there are other places in AsyncPanZoomController.cpp where mState is assigned to directly, without going through SetState. This patch will miss those state changes.

(Also, I just realized how supremely un-searchable the summary of this bug is).

Comment 30

[Brian R. Bondy [:bbondy]]

Attachment: Scale not working correctly on high DPI from gfx/layers/APZC - rev1

So on my 140% high dpi surface, I noticed scaling doesn't work most of the time. And if you can get it to work, if you try to scale back down it won't go below 1.4x scale factor.

This fixes both problems.

Comment 31

[Brian R. Bondy [:bbondy]]

Comment on attachment 777847 [details] [diff] [review]
Pan Begin and Pan End notifications from gfx/layers/APZC - rev2.

From what I can tell we only set it directly when we're in a FLING state, from CancelAnimation (which is only called from FLING) and from ANIMATION_ZOOM state.

So I think the patch is still valid.
If you still don't think so though, please let me know the recommended way to fix. Thanks!

Comment 32

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 780159 [details] [diff] [review]
Scale not working correctly on high DPI from gfx/layers/APZC - rev1

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

::: gfx/layers/ipc/Axis.cpp
@@ +316,5 @@
>    const FrameMetrics& metrics = mAsyncPanZoomController->GetFrameMetrics();
>  
>    CSSRect cssContentRect = metrics.mScrollableRect;
>  
> +  CSSToScreenScale scale(metrics.mZoom.scale * aScale * metrics.mDevPixelsPerCSSPixel.scale);

After staring at the code for a while I think this should actually be:
 CSSToScreenScale scale(metrics.CalculateResolution() * aScale);

Can you check with that and see if that fixes it too?

Comment 33

[Kartikaya Gupta (email:[e-mail])]

(In reply to Brian R. Bondy [:bbondy] from comment #31)
> Comment on attachment 777847 [details] [diff] [review]
> Pan Begin and Pan End notifications from gfx/layers/APZC - rev2.
> 
> From what I can tell we only set it directly when we're in a FLING state,
> from CancelAnimation (which is only called from FLING) and from
> ANIMATION_ZOOM state.
> 
> So I think the patch is still valid.
> If you still don't think so though, please let me know the recommended way
> to fix. Thanks!

It might also get set to NOTHING in NotifyLayersUpdated if a first-paint comes in while in pretty much any state. Even without that it feels like a footgun. I would prefer refactoring all the mState = ... lines into calls to SetState, but I guess there will be some tricky locking issues to deal with. In that case I guess I'm fine with this patch landing for now if you file a follow-up bug to ensure all state changes are dealt with uniformly.

Comment 34

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 777847 [details] [diff] [review]
Pan Begin and Pan End notifications from gfx/layers/APZC - rev2.

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

Flipping back to plus for now. I still view this as incomplete as stated above, but ok for now.

Comment 35

[Brian R. Bondy [:bbondy]]

Attachment: APZC WinRT Implementation. rev2

Implemented review comments, various fixes including high dpi working

Comment 36

[Brian R. Bondy [:bbondy]]

Attachment: APZC Metro front end Implementation - rev2.

Implemented jimm and mbrubeck review comments.

Comment 37

[Jim Mathies [:jimm]]

Comment on attachment 780438 [details] [diff] [review]
APZC WinRT Implementation. rev2

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

Have you tried pushing to try with apzc turned on to see what happens with our tests? (Just curious.)

::: widget/windows/winrt/MetroWidget.cpp
@@ +826,5 @@
> +                                                              0.0);
> +        MetroWidget::sAPZC->NotifyLayersUpdated(frameMetrics, true);
> +        mMetroWidget->RequestContentRepaint(frameMetrics);
> +      }
> +

nit - extra line here

@@ +1358,5 @@
> +    NS_IMETHOD Run() {
> +        // This event shuts down the worker thread and so must be main thread.
> +        MOZ_ASSERT(NS_IsMainThread());
> +
> +        CSSToScreenScale resolution = mFrameMetrics.CalculateResolution(); /* should be dividing by mFrameMetrics.mDevPixelsPerCSSPixel to get high-dpi to work */

So does this comment still apply? Is it a TODO of some sort?

::: widget/windows/winrt/MetroWidget.h
@@ +236,5 @@
> +  Microsoft::WRL::ComPtr<mozilla::widget::winrt::MetroInput> mMetroInput;
> +  mozilla::layers::FrameMetrics mFrameMetrics;
> +
> +public:
> +  static nsRefPtr<mozilla::layers::AsyncPanZoomController> sAPZC;

I don't see us nulling this out. I think it might show up in leak tracking.

Comment 38

[Brian R. Bondy [:bbondy]]

Attachment: Scale not working correctly on high DPI from gfx/layers/APZC - rev2

Yep CalculateReolution also fixes the problem and works the same (correct) way on 140% high dpi.

Comment 39

[Brian R. Bondy [:bbondy]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #33)
> (In reply to Brian R. Bondy [:bbondy] from comment #31)
> > Comment on attachment 777847 [details] [diff] [review]
> > Pan Begin and Pan End notifications from gfx/layers/APZC - rev2.
> > 
> > From what I can tell we only set it directly when we're in a FLING state,
> > from CancelAnimation (which is only called from FLING) and from
> > ANIMATION_ZOOM state.
> > 
> > So I think the patch is still valid.
> > If you still don't think so though, please let me know the recommended way
> > to fix. Thanks!
> 
> It might also get set to NOTHING in NotifyLayersUpdated if a first-paint
> comes in while in pretty much any state. Even without that it feels like a
> footgun. I would prefer refactoring all the mState = ... lines into calls to
> SetState, but I guess there will be some tricky locking issues to deal with.
> In that case I guess I'm fine with this patch landing for now if you file a
> follow-up bug to ensure all state changes are dealt with uniformly.

I'll file a followup and land it as is as you suggested for now.

Comment 40

[Jim Mathies [:jimm]]

Comment on attachment 780439 [details] [diff] [review]
APZC Metro front end Implementation - rev2.

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

::: browser/metro/base/content/apzc.js
@@ +35,5 @@
> +    switch (aEvent.type) {
> +      case 'pageshow':
> +      case 'TabSelect':
> +        Services.obs.notifyObservers(null, "viewport-needs-updating", null);
> +        Services.obs.notifyObservers(null, "viewport-needs-updating", null);

duplicate lines here.

@@ +74,5 @@
> +      if (StartUI.isStartPageVisible) {
> +        let windowUtils = Browser.windowUtils;
> +        Browser.selectedBrowser.contentWindow.scrollTo(scrollTo.x, scrollTo.y);
> +        windowUtils.setResolution(resolution, resolution);
> +        windowUtils.setDisplayPortForElement(displayPort.x * resolution, displayPort.y * resolution, displayPort.width * resolution, displayPort.height * resolution, Elements.startUI);

nit - wrap this down to ~120 chars plz.

@@ +94,5 @@
> +      Util.dumpLn("APZC scrollTo.x: " + scrollTo.x + ", scrollTo.y: " + scrollTo.y);
> +      Util.dumpLn("APZC setResolution: " + resolution);
> +      Util.dumpLn("APZC setDisplayPortForElement: displayPort.x: " + displayPort.x + ", displayPort.y: " + displayPort.y + ", displayPort.width: " + displayPort.width + ", displayort.height: " + displayPort.height);
> +    } else if (aTopic == "apzc-handle-pan-begin") {
> +      // When we're panning, hide the main scrollbars by setting imprecise

Can you pan with the mouse or only with touch? If only touch, I'd suggest we just drop the this._apzcWasPrecise stuff and leave the browser in imprecise mode. It'll come back to precise as soon as the user moves the mouse again.

Comment 41

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 780452 [details] [diff] [review]
Scale not working correctly on high DPI from gfx/layers/APZC - rev2

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

Thanks.

Comment 42

[Jim Mathies [:jimm]]

So far I've testing on a non-high dpi device. This is greatly improved over the last rev. Panning simple text pages works great. More complex pages are smooth, although I see a lot of jitter at the end of flings. Zoom is nice and fluid as well, although panning while zoomed is still pretty broken render wise. All in all good for an initial pref'd off landing with nits addressed so we can test and file follow ups.

(My surface pro is still building, so not sure how well high-dip is yet.)

Comment 43

[Brian R. Bondy [:bbondy]]

> > Services.obs.notifyObservers(null, "viewport-needs-updating", null);
> > Services.obs.notifyObservers(null, "viewport-needs-updating", null);
> duplicate lines here.

oops will remove one.

> nit - wrap this down to ~120 chars plz.

Will do.


> Can you pan with the mouse or only with touch? If only touch, I'd suggest we just drop
>  the this._apzcWasPrecise stuff and leave the browser in imprecise mode. It'll come
> back to precise as soon as the user moves the mouse again.

Only with touch, I'll remove the setting it back to old state.


> So far I've testing on a non-high dpi device. This is greatly improved over the last
> rev. Panning simple text pages works great. More complex pages are smooth, although 

Great, ditto!

> I see a lot of jitter at the end of flings. 

I see this too, I'll post for it in part 3. I think this is in /gfx

> Zoom is nice and fluid as well, although panning while zoomed is still pretty 
> broken render wise.

I see it working in general but needs improvements, I'll post for it in part 3. I think this is in /gfx too.

> > +        CSSToScreenScale resolution = mFrameMetrics.CalculateResolution();
> >  /* should be dividing by mFrameMetrics.mDevPixelsPerCSSPixel to get high-dpi 
> > to work */

That' an old comment I'll remove it. I divide a little lower in the code.
So does this comment still apply? Is it a TODO of some sort?

> > static nsRefPtr<mozilla::layers::AsyncPanZoomController> sAPZC;
> I don't see us nulling this out. I think it might show up in leak tracking.

Will do, I'll also call sAPZC->Destroy() before doing it.


> Have you tried pushing to try with apzc turned on to see what happens with our tests? (Just curious.)

I have not but I'll be pushing both with thing preffed off and on before landing. 
If all goes well with those pushes I'll push all of this with nits addressed by tomorrow.

Comment 44

[Brian R. Bondy [:bbondy]]

> So does this comment still apply? Is it a TODO of some sort?

That' an old comment I'll remove it. I divide a little lower in the code.

Comment 45

[Brian R. Bondy [:bbondy]]

Attachment: APZC Metro front end Implementation - rev3.

Implemented nit, carrying forward r+.

Comment 46

[Brian R. Bondy [:bbondy]]

Attachment: APZC WinRT Implementation. rev3

Implemented nits, carrying forward r+.

Comment 47

[Brian R. Bondy [:bbondy]]

Pushed the 4 patches to try:

Prefed off: all tests on all platforms
https://tbpl.mozilla.org/?tree=Try&rev=19bc46e030fc

Prefed on: metro mochitest only
https://tbpl.mozilla.org/?tree=Try&rev=19c709ee3d7a

Comment 48

[Brian R. Bondy [:bbondy]]

With it pref'ed off, as one would hope verything passes.
With it pref'ed on, we have some selection failures.
I'll try to fix these tomorrow.

Comment 49

[Brian R. Bondy [:bbondy]]

Attachment: Fix selection for metro front end when APZC is enabled - rev1

So apparently input.js does more than I thought. I went for a minimalist approach of disabling only what's needed when apzc is enabled and that fixes selection. 

New push to try:
https://tbpl.mozilla.org/?tree=Try&rev=262b4d1bf077

Comment 50

[Jim Mathies [:jimm]]

(In reply to Brian R. Bondy [:bbondy] from comment #49)
> Created attachment 780801 [details] [diff] [review]
> Fix selection for metro front end when APZC is enabled - rev1


I wonder if we could do better filtering here down in the scrollbox finder code - 

http://mxr.mozilla.org/mozilla-central/source/browser/metro/base/content/input.js#425

which is called by _onTouchStart - 

http://mxr.mozilla.org/mozilla-central/source/browser/metro/base/content/input.js#222

Looks like we need to connect the layer selection logic down in MetroCompositorParent's ShadowLayersUpdated up with what scrollable surfaces we choose up here somehow?

Comment 51

[Jim Mathies [:jimm]]

Maybe for a follow up, I wonder if we could tag the scrollable frame(s?) we apply apzc to.  How does android deal with this? Maybe since they only have a gecko content view it's not a problem for them.

Comment 52

[Brian R. Bondy [:bbondy]]

(In reply to Jim Mathies [:jimm] from comment #51)
> Maybe for a follow up, I wonder if we could tag the scrollable frame(s?) we
> apply apzc to.  How does android deal with this? Maybe since they only have
> a gecko content view it's not a problem for them.

I think that may be simply calling preventDefault when we don't want apzc to do its thing.

Comment 53

[Brian R. Bondy [:bbondy]]

(In reply to Jim Mathies [:jimm] from comment #50)
> (In reply to Brian R. Bondy [:bbondy] from comment #49)
> > Created attachment 780801 [details] [diff] [review]
> > Fix selection for metro front end when APZC is enabled - rev1
> 
> 
> I wonder if we could do better filtering here down in the scrollbox finder
> code - 
> 


So I tried returning early because no targetElement is found and it still does the crazy scroll jittering and pan on zoom etc. 

> http://mxr.mozilla.org/mozilla-central/source/browser/metro/base/content/
> input.js#425
> 
> which is called by _onTouchStart - 
> 
> http://mxr.mozilla.org/mozilla-central/source/browser/metro/base/content/
> input.js#222
> 
> Looks like we need to connect the layer selection logic down in
> MetroCompositorParent's ShadowLayersUpdated up with what scrollable surfaces
> we choose up here somehow?

So what happens is APZC calls into RequestContentRepaint with some dimensions. We call into front end code which does a winUtils.setDisplayPortForElement amongst other things. That in turn calls back into ShadowLayersUpdated.   The setDisplayPortForElement can choose any element it wants to, although right now always uses the root document of the browser.  We need some support for multi element though from the gfx guys before we do this.

Comment 54

[Brian R. Bondy [:bbondy]]

Comment on attachment 780801 [details] [diff] [review]
Fix selection for metro front end when APZC is enabled - rev1

Although the selection is working now tests still break, will re-attach when I get a fix that doesn't make panning bad and doesn't break selection and selection tests.

Comment 55

[Kartikaya Gupta (email:[e-mail])]

Yeah currently you can only use APZC on a single layer, and I'm working on multi-layer support over in bug 866232. What brian said in comment 53 is accurate. Once my code lands a new APZC will be automatically created for any layer that we detect as being scrollable. I have yet to figure out how that will interact with this bug. For the most part the code changes should be straighforward but there might be some unexpected consequences like things becoming scrollable when they shouldn't be, or the wrong thing becoming scrollable. I'll need to sync up with you after you land these patches to coordinate that.

Comment 56

[Brian R. Bondy [:bbondy]]

Attachment: Fix selection for metro front end when APZC is enabled - rev2

i) Makes selection work
ii) Passes all selection tests locally
iii) Doesn't get in the way of APZC

Comment 57

[Brian R. Bondy [:bbondy]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #55)
> Yeah currently you can only use APZC on a single layer, and I'm working on
> multi-layer support over in bug 866232. What brian said in comment 53 is
> accurate. Once my code lands a new APZC will be automatically created for
> any layer that we detect as being scrollable. I have yet to figure out how
> that will interact with this bug. For the most part the code changes should
> be straighforward but there might be some unexpected consequences like
> things becoming scrollable when they shouldn't be, or the wrong thing
> becoming scrollable. I'll need to sync up with you after you land these
> patches to coordinate that.

Great, if you can give me the set of patches when you're close to landing, then I can try to fixup things in the context of the linked bug to this bug, bug 886321.    This bug will likely be landing today with apzc pref disabled.

Comment 58

[Jim Mathies [:jimm]]

Comment on attachment 780978 [details] [diff] [review]
Fix selection for metro front end when APZC is enabled - rev2

You could probably just skip the deck assignments to avoid variable allocation.

Comment 59

[Brian R. Bondy [:bbondy]]

Attachment: Fix selection for metro front end when APZC is enabled - rev3

Implemented nits. Carrying forward r+.

Comment 60

[Brian R. Bondy [:bbondy]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/37e9bb9ca6f2
https://hg.mozilla.org/integration/mozilla-inbound/rev/cab7b1a00897
https://hg.mozilla.org/integration/mozilla-inbound/rev/d8c0cdc0bc22
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a8926881a44
https://hg.mozilla.org/integration/mozilla-inbound/rev/692b48b7c022

Comment 63

[Brian R. Bondy [:bbondy]]

The stories for the 2 duped bugs were uploaded to bug 886321.

Comment 64

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/37e9bb9ca6f2
https://hg.mozilla.org/mozilla-central/rev/cab7b1a00897
https://hg.mozilla.org/mozilla-central/rev/d8c0cdc0bc22
https://hg.mozilla.org/mozilla-central/rev/6a8926881a44
https://hg.mozilla.org/mozilla-central/rev/692b48b7c022