Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Reporter
	Description • 12 years ago

The current async pan/zoom code just ignores them and always uses its heuristics.  We instead want to check if content has register touch-event listeners, and give them a chance to preventDefault() the pan/zoom animations before starting them.

We have all the platform support for this that we need.  This bug covers gluing that into the new pan/zoom system.

I think this should block basecamp because it will be a fairly important web-compat point, and isn't very risky.

	Doug Sherk (:drs) (inactive) Assignee
	Comment 1 • 12 years ago

Enable us to watch for adding and removing touch listeners. The removal code doesn't do anything because we don't get a "dom-touch-listener-removed" event, or anything like that.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Reporter
	Comment 2 • 12 years ago

Comment on attachment 646818 [details] [diff] [review]
Properly track number of DOM touch listeners in AsyncPanZoomController

The current mechanism for tracking this is not as precise as you're using it here.  We need to keep a bool "may have touch listeners", and then only reset that on first paint.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Reporter
	Comment 4 • 12 years ago

Comment on attachment 648585 [details] [diff] [review]
Properly track number of DOM touch listeners in AsyncPanZoomController

>diff --git a/gfx/layers/ipc/AsyncPanZoomController.cpp b/gfx/layers/ipc/AsyncPanZoomController.cpp

>+     mHasTouchListeners(false)

mMayHaveTouchListener.  We don't know for sure here.

>diff --git a/gfx/layers/ipc/AsyncPanZoomController.h b/gfx/layers/ipc/AsyncPanZoomController.h
>--- a/gfx/layers/ipc/AsyncPanZoomController.h
>+++ b/gfx/layers/ipc/AsyncPanZoomController.h
>@@ -93,16 +93,26 @@ public:
>    * using CSS pixels everywhere inside here, but in this case we need to know
>    * how large of a displayport to set so we use these dimensions plus some
>    * extra.
>    *
>    * XXX: Use nsIntRect instead.
>    */
>   void UpdateViewportSize(int aWidth, int aHeight);
> 
>+  /**
>+   * A DOM touch listener has been added. When called, we enable the machinery
>+   * that allows touch listeners to preventDefault any touch inputs. This should
>+   * not be called unless there are actually touch listeners as it introduces a

  it /can/ introduce unbounded lag because in general it requires a
  round-trip through the content main thread.

>+   * great amount of lag in response time. No locking is done here since we're
>+   * generally pretty lazy about actually allowing DOM to hook and prevent
>+   * touches to begin with.

I don't understand this part of the comment.

No need to mention locking here, but you do want to mention which
thread it's legal to call this method on.  I believe it should be
UI-thread only.

>+  // Stores whether or not there are touch listeners in the DOM for the frame
>+  // we're managing. If non-zero, we must allow them to intercept touches which
>+  // adds some additional latency to each touch.

Stale comment.  Be sure to note that this is a conservative
approximation of whether we /might/ have touch listeners.

r=me with those changes

	Doug Sherk (:drs) (inactive) Assignee
	Comment 5 • 12 years ago

Review comments fixed, r+ carried.

Also added a single new line to AsyncPanZoomController::NotifyLayersUpdated() to reset the mMayHaveTouchListeners flag to false on first paint.

	Doug Sherk (:drs) (inactive) Assignee
	Comment 6 • 12 years ago

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

	Phil Ringnalda (:philor)
	Comment 7 • 12 years ago

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/d8878c0bd67c - something in that push made Linux oddly unhappy.

	Ed Morley [:emorley]
	Comment 8 • 12 years ago

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

	Doug Sherk (:drs) (inactive) Assignee
	Comment 9 • 12 years ago

This was backed out.

	Doug Sherk (:drs) (inactive) Assignee
	Comment 10 • 12 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/986a9cafa237

	Ed Morley [:emorley]
	Comment 11 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/986a9cafa237

	Joe Drew (not getting mail)
	Comment 12 • 12 years ago

Can this be closed now, Doug? What's left?

	Doug Sherk (:drs) (inactive) Assignee
	Comment 13 • 12 years ago

This doesn't actually do anything yet, it tracks whether or not there are listeners. I'm working on actually doing stuff with that information.

	Doug Sherk (:drs) (inactive) Assignee
	Comment 14 • 12 years ago

This allows content to actually preventDefault touch events. Unfortunately we ended up not using the code I previously pushed in this bug.

	Doug Sherk (:drs) (inactive) Assignee
	Comment 15 • 12 years ago

This removes most of the code from attachment 648771 [details] [diff] [review]. It's not a direct backout since removing some of the code I added would break other stuff that has been added which depends on it now. For the purposes of review, I don't know that this needs a very serious look through.

	Doug Sherk (:drs) (inactive) Assignee
	Comment 16 • 12 years ago

Try push:
https://tbpl.mozilla.org/?tree=Try&rev=fb2b71f60006

(these patches are both in there; it got coalesced after I messed up and pushed only one of them to try)

	Doug Sherk (:drs) (inactive) Assignee
	Comment 17 • 12 years ago

Retrying that push here:
https://tbpl.mozilla.org/?tree=Try&rev=c395efd4bb80

There was an intermittent on inbound/central matching the one I keep getting for Win opt M1, so I rebased it and pushed again.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Reporter
	Comment 18 • 12 years ago

Comment on attachment 653044 [details] [diff] [review]
Let touch-event listeners cancel async pan/zoom

>diff --git a/dom/ipc/TabChild.cpp b/dom/ipc/TabChild.cpp

> bool
> TabChild::RecvRealTouchEvent(const nsTouchEvent& aEvent)
> {
>     nsTouchEvent localEvent(aEvent);
>     nsEventStatus status = DispatchWidgetEvent(localEvent);
>+    SendContentReceivedTouch(nsIPresShell::gPreventMouseEvents);

Let's do this only if mayHaveContentTouchListeners.

Otherwise looks fine.  Would like to see the next version.

	Doug Sherk (:drs) (inactive) Assignee
	Comment 19 • 12 years ago

Addressed review comment.

	Doug Sherk (:drs) (inactive) Assignee
	Comment 20 • 12 years ago

Forgot to qref. Much better method of getting the inner window.

	Doug Sherk (:drs) (inactive) Assignee
	Comment 21 • 12 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/746eb3e15dec
https://hg.mozilla.org/integration/mozilla-inbound/rev/408707dae837

	Ed Morley [:emorley]
	Comment 22 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/746eb3e15dec
https://hg.mozilla.org/mozilla-central/rev/408707dae837

	Doug Sherk (:drs) (inactive) Assignee
	Comment 23 • 12 years ago

Whoops, this should be closed.