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

We need to
 - recognize double-tap gestures
 - send a message to content requesting it to find an appropriate element to zoom at the double-tap focus point
 - reply with the element and its metrics
 - kick off the zoom animation on the compositor and start repainting in content

The heuristics for this have lived in browser.js forever.

Suggesting blocking basecamp because this is pretty much a table-stakes UI feature now in "mobile" browsers.

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

I'm using the compositor thread as a heartbeat signal to the gesture detector. I do this because we need to send a delayed single tap if we detect that a double tap isn't happening. This means that we have to schedule ~500 ms of composites for nothing other than gesture detection. Since there's a lot of overhead to composites, this sounds bad to me. Should we just dispatch a runnable somewhere? Any suggestions?

I will also need to look at and figure out what truly is the difference between SingleTapUp and SingleTapConfirmed, as well as whether or not we even need them. I assume we need them for Fennec, though if it uses its own gesture detector we won't need this implementation. Without them, this code is a lot simpler since we can detect double taps using only actual touch inputs (touch down, touch up, [first tap] touch down, touch up [second tap]).

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

We don't want to use the compositor heartbeat for that --- it's for sampling animations.  We can receive many input events that would result in the same pixels drawn on screen, so turning on the compositor just for timekeeping is quite inefficient.  We only want to turn it on for graphical effects.

You want to post a task delayed by the "can't-be-double-tap" interval.  ISTR that this was 300ms on xul-fennec, but it makes sense to use the same interval that android-fennec is currently using.

This will need to play well with content touch-event listeners when we tackle that problem, but I don't think that adds too much complexity here.

Running out of steam, will review tomorrow.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 3 • 12 years ago

(In reply to Doug Sherk (:dRdR) from comment #1)
> I will also need to look at and figure out what truly is the difference
> between SingleTapUp and SingleTapConfirmed, as well as whether or not we
> even need them. I assume we need them for Fennec, though if it uses its own
> gesture detector we won't need this implementation.

The order of events:
1) user touches down, we get a touchdown
2) user lifts finger, we get a touchup and then a SingleTapUp immediately
3a) user puts finger down again, we get a touchdown
4a) user lifts finger, we get a touchup and then a DoubleTap immediately

OR

1) user touches down, we get a touchdown
2) user lifts finger, we get a touchup and then a SingleTapUp immediately
3b) user does nothing, and <n> ms later, we get a SingleTapConfirmed (n is a system-wide setting on android accessible by ViewConfiguration.getDoubleTapTimeout)

The idea behind fennec responding to both SingleTapUp and SingleTapConfirmed is to avoid input lag - if zooming is disabled by the content then we know that double-tap will do nothing, and we can send the "tap" event as soon as the first tap is done without waiting for the <n>-ms delay.

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

Comment on attachment 646061 [details] [diff] [review]
Recognize double tap gestures while still supporting single taps

Other than the issues above, looks pretty good.

Let's use a PostDelayedTask(NewRunnableMethod(foo, &Bar::M), delay) to do the task management, instead of nsIRunnable.

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

Ok, I fixed it to use PostDelayedTask() (which I completely forgot about) and to do what I understand should be the functionality of TapUp/TapConfirmed based on kats' explanation.

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

Comment on attachment 646459 [details] [diff] [review]
Recognize double tap gestures while still supporting single taps

># HG changeset patch
># Parent fc17a0803716958bcba1ff1b06eb8edfe8302a93
># User Doug Sherk <dsherk2@mozilla.com>
>Bug 775463: Recognize double tap gestures while still supporting single taps
>
>diff --git a/gfx/layers/ipc/GestureEventListener.cpp b/gfx/layers/ipc/GestureEventListener.cpp

>+    if (event.mTime - mTapStartTime <= MAX_TAP_TIME) {
>+      if (mState == WaitingDoubleTap) {
>+        // We were waiting for a double tap and it has arrived.
>+        HandleDoubleTap(event);
>+        mState = NoGesture;
>+      } else {
>+        HandleSingleTapUpEvent(event);

This doesn't work for > 2 touch points right?

>diff --git a/gfx/layers/ipc/GestureEventListener.h b/gfx/layers/ipc/GestureEventListener.h

> protected:
>   enum GestureState {
>+    // There's no gesture going on, and we don't think we're about to enter one.
>     NoGesture = 0,
>-    InPinchGesture
>+    // There's a pinch happening, which occurs when there are two touch inputs.
>+    InPinchGesture,
>+    // A single tap has happened for sure, and we're waiting for a second tap.
>+    WaitingDoubleTap
>   };

Need to be consistent with style here --- local style is NO_GESTURE, etc.

The rest looks OK.  Clearing r? pending answer to question above.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 7 • 12 years ago

Comment on attachment 646459 [details] [diff] [review]
Recognize double tap gestures while still supporting single taps

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

I think there's a race condition that might happen if you tap multiple times in quick succession. Say you do a normal double-tap, which is processed as expected. However there will still be a TimeoutDoubleTap runnable is in the queue waiting to run. Normally the state guard at the top of the function will make it a no-op, but if you tap again once before the timeout expires, then that tap will get cancelled when the timeout expires. The net result is that you want to double-tap twice, but the second double-tap might get cancelled prematurely because of the runnable from the first double-tap. Does that make sense?

Other than that I didn't see any problems.

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

(In reply to Chris Jones [:cjones] [:warhammer] from comment #6)
> This doesn't work for > 2 touch points right?

You're right, there was a big mistake in how I dealt with this. There's now a proper WAITING_SINGLE_TAP state that we enter when there has only been 1 unique touch input since there was last 0 touch inputs.

(In reply to Kartikaya Gupta (:kats) from comment #7)
> I think there's a race condition that might happen if you tap multiple times

Ok, I now cancel the timeout task when a double tap comes in.

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

Comment on attachment 646733 [details] [diff] [review]
Recognize double tap gestures while still supporting single taps

"WaitingDoubleTap" => I will fix references to this in comments before landing.

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

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

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

Forgot to fix the comment, followup fix:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d0fb7ac961a

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

Backed out both:
https://hg.mozilla.org/integration/mozilla-inbound/rev/02d63f48e752

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

Pushed again:
https://hg.mozilla.org/integration/mozilla-inbound/rev/db790a34ba9a

	Ryan VanderMeulen [:RyanVM]
	Comment 14 • 12 years ago

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

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

Has random stuff scattered inside it, mostly small refactors to support this.

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

"Implement double-tap-to-zoom content" kind of sucks without this, at least on Otoro. This should make it easy to do a double tap.

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

https://tbpl.mozilla.org/?tree=Try&rev=24ee21078b33

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

(In reply to Doug Sherk (:dRdR) from comment #17)
> https://tbpl.mozilla.org/?tree=Try&rev=24ee21078b33

There's a leak, will deal with this. cjones says I have to add a ClearOnShutdown() to the StaticAutoPtr.

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

Unbitrotted

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

Unbitrotted

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

Comment on attachment 648799 [details] [diff] [review]
Implement double-tap-to-zoom content

>diff --git a/dom/browser-element/BrowserElementGestures.js b/dom/browser-element/BrowserElementGestures.js

This is really a gesture *handler*, just like the current code in
BrowserElementScrolling.js.  Splitting this into a separate file and
giving it an ambiguous name is confusing (I thought originally we were
*computing* gestures here, which made me do a double-take).

>+const ContentGestures = {
>+  init: function cg_init() {
>+    addMessageListener("Viewport:Change", this._recvViewportChange.bind(this));

This is going to process Viewport:Change twice, no?

>+  _recvDoubleTap: function(data) {

Is this code taken from browser.js?  If so, please note that, and I
won't review it.

>diff --git a/dom/ipc/PBrowser.ipdl b/dom/ipc/PBrowser.ipdl

>+    /**
>+     * Instructs the TabParent to forward a request to zoom to a rect given in
>+     * CSS pixels.

Relative to what?  The viewport or document?

>+     */
>+    BrowserZoomToRect(gfxRect aRect);

"Browser" in the name here is redundant, since we're in PBrowser
already ;);.

>+    HandleDoubleTap(nsIntPoint point);
> 

Document this, especially coordinate space.

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

>+      int numAssigned = sscanf(NS_ConvertUTF16toUTF8(aData).get(),
>+                               "{\"x\":%lf,\"y\":%lf,\"w\":%lf,\"h\":%lf}",
>+                               &rect.x, &rect.y, &rect.width, &rect.height);

May god have mercy on our souls.

>+void
>+TabChild::WrapAndSendJSON(const char* aTopic, const nsACString& aData)

DispatchMessageManagerMessage(const nsACString& aMessageName, const nsACString& aJSONData)

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

>+    // XXX: Do the work the browser chrome script does in C++ instead so we
>+    // don't need things like this.
>+    void WrapAndSendJSON(const char* aTopic, const nsACString& aData);

This wants to be a FIXME/bug XXXXXX comment.

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

>+StaticAutoPtr<css::ComputedTimingFunction> gComputedTimingFunction;

|using namespace mozilla::css;|

>+  if (!gComputedTimingFunction) {
>+    gComputedTimingFunction = new css::ComputedTimingFunction();
>+    gComputedTimingFunction->Init(nsTimingFunction(0, 0, 0.58, 1.0));

Would prefer to use a keyword function like
NS_STYLE_TRANSITION_TIMING_FUNCTION_EASE so that (i) the code is
easier to read; and (ii) the zoom "feels" more like common CSS
animations, instead of a custom animal.


>+void AsyncPanZoomController::ZoomToRect(const gfxRect& aRect) {
>+  mState = ANIMATED_ZOOM;

Needs the mutex, no?

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

>+  /**
>+   * Kicks an animation to zoom to a rect. This may be either a zoom out or zoom
>+   * in. The actual animation is done on the compositor thread after being set
>+   * up. |aRect| must be given in CSS pixels.
>+   */
>+  void ZoomToRect(const gfxRect& aRect);

Relative to what?

>   enum PanZoomState {

>+    ANIMATED_ZOOM   /* animated zoom to a new rect */

ANIMATING_ZOOM

>+  // Old metrics from before we started a zoom animation. This is only valid
>+  // when we are in the "ANIMATED_ZOOM" state. This is used so that we can
>+  // interpolate between the start and end frames. We only use the
>+  // |mViewportScrollOffset| and |mResolution| fields on this.
>+  FrameMetrics mStartZoomToMetrics;

What happens if content repaints in the middle of a zoom?  Should this
always be mLastContentPaintMetrics, and then re-adjust the animation
duration appropriately if a new paint comes in?

>diff --git a/gfx/layers/ipc/GeckoContentController.h b/gfx/layers/ipc/GeckoContentController.h

>+  /**
>+   * Requests handling of a double tap.
>+   */
>+  virtual void HandleDoubleTap(const nsIntPoint& aPoint) = 0;

Needs moar docs.  Coordinate space etc.

>diff --git a/gfx/layers/ipc/ShadowLayerUtils.h b/gfx/layers/ipc/ShadowLayerUtils.h

Why the change here?  Why can't we keep the FrameMetrics serializer in
ShadowLayerUtils.h and include it from PBrowser.ipdl?

>diff --git a/ipc/glue/IPCMessageUtils.h b/ipc/glue/IPCMessageUtils.h
> template<>
>+struct ParamTraits<gfxRect>
>+{

>+  static bool Read(const Message* aMsg, void** aIter, paramType* aResult)
>+  {
>+    if (ReadParam(aMsg, aIter, &aResult->x) &&
>+        ReadParam(aMsg, aIter, &aResult->y) &&
>+        ReadParam(aMsg, aIter, &aResult->width) &&
>+        ReadParam(aMsg, aIter, &aResult->height))

      return (ReadParam(aMsg, aIter, &aResult->x) &&
              ReadParam(aMsg, aIter, &aResult->y) &&
              ReadParam(aMsg, aIter, &aResult->width) &&
              ReadParam(aMsg, aIter, &aResult->height));

This looks good overall, but needs some work.

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

Comment on attachment 648800 [details] [diff] [review]
Implement tolerance in double tap detection

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

>+  /**
>+   * Constant describing the tolerance we use, multiplied by the device DPI,
>+   * before we start panning the screen. This is to prevent us from accidentally
>+   * processing taps as touch moves, and from very short/accidental touches
>+   * moving the screen
>+   */
>+  static const float TOUCH_START_TOLERANCE;

What's it a tolerance of?  The first sentence basically reads as,
"TOUCH_START_TOLERANCE is a tolerance for touch starts" ;).

r=me with better comment

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

Comment on attachment 648937 [details] [diff] [review]
Implement double-tap-to-zoom content

>diff --git a/dom/browser-element/BrowserElementScrolling.js b/dom/browser-element/BrowserElementScrolling.js

Didn't hear back about provenance of this code, but assuming it's
borrowed from browser.js, so didn't look closely.

>diff --git a/dom/ipc/PBrowser.ipdl b/dom/ipc/PBrowser.ipdl

>+    /**
>+     * Requests handling of a double tap. |point| is in CSS pixels, relative to
>+     * the scroll offset. This message is expected to send back a
>+     * "browser-zoom-to-rect" message using the observer service.
>+     */

It's expected to send a ZoomToRect() message, no?  The observer
service notification is just a hack to get us back into C++.

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

>+      int numAssigned = sscanf(NS_ConvertUTF16toUTF8(aData).get(),
>+                               "{\"x\":%lf,\"y\":%lf,\"w\":%lf,\"h\":%lf}",
>+                               &rect.x, &rect.y, &rect.width, &rect.height);
>+      NS_ABORT_IF_FALSE(numAssigned == 4, "Invalid JSON format");

MOZ_ASSERT()

>+void
>+TabChild::DispatchMessageManagerMessage(const char* aTopic, const nsACString& aData)

  DispatchMessageManagerMessage(const nsACString& aMessageName,
                                const nsACString& aJSONData)

Note change to parameter names: you're using observer service
terminology, but this is message manager.

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

>+    case ANIMATED_ZOOM:

ANIMATING_ZOOM

>+      requestAnimationFrame = requestAnimationFrame || DoFling(aSampleTime - mLastSampleTime);

requestAnimationFrame |= ...;

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

>+  static bool Read(const Message* aMsg, void** aIter, paramType* aResult)
>+  {
>+    if (ReadParam(aMsg, aIter, &aResult->x) &&
>+        ReadParam(aMsg, aIter, &aResult->y) &&
>+        ReadParam(aMsg, aIter, &aResult->width) &&
>+        ReadParam(aMsg, aIter, &aResult->height))
>+      return true;
>+
>+    return false;

return (ReadParam() && ...);

There are several unaddressed review comments here.  Please be sure to
look over them.  Need to see the next version.

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

(In reply to Chris Jones [:cjones] [:warhammer] from comment #21)
> Comment on attachment 648799 [details] [diff] [review]
> Is this code taken from browser.js?  If so, please note that, and I
> won't review it.

That's correct.
 
> >+  // Old metrics from before we started a zoom animation. This is only valid
> >+  // when we are in the "ANIMATED_ZOOM" state. This is used so that we can
> >+  // interpolate between the start and end frames. We only use the
> >+  // |mViewportScrollOffset| and |mResolution| fields on this.
> >+  FrameMetrics mStartZoomToMetrics;
> 
> What happens if content repaints in the middle of a zoom?  Should this
> always be mLastContentPaintMetrics, and then re-adjust the animation
> duration appropriately if a new paint comes in?

I don't understand how this would change it. Could you clarify? I could see a change in the CSS rect during an animated zoom messing it up, which is something I should deal with and will in a follow-up.

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

(In reply to Doug Sherk (:dRdR) from comment #25)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #21)
> > Comment on attachment 648799 [details] [diff] [review]
> > >+  // Old metrics from before we started a zoom animation. This is only valid
> > >+  // when we are in the "ANIMATED_ZOOM" state. This is used so that we can
> > >+  // interpolate between the start and end frames. We only use the
> > >+  // |mViewportScrollOffset| and |mResolution| fields on this.
> > >+  FrameMetrics mStartZoomToMetrics;
> > 
> > What happens if content repaints in the middle of a zoom?  Should this
> > always be mLastContentPaintMetrics, and then re-adjust the animation
> > duration appropriately if a new paint comes in?
> 
> I don't understand how this would change it. Could you clarify? I could see
> a change in the CSS rect during an animated zoom messing it up, which is
> something I should deal with and will in a follow-up.

That's what I was referring to.  Please to be filing and FIXME/bug XXXXX'ing here.  Or if it's simpler to handle here we can do that too.

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

Filed bug 781021.

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

Comment on attachment 649823 [details] [diff] [review]
Implement double-tap-to-zoom content

I like this patch :).

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/963aed158547
https://hg.mozilla.org/integration/mozilla-inbound/rev/b33f2b2dbbc5

	Phil Ringnalda (:philor)
	Comment 31 • 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 32 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/963aed158547
https://hg.mozilla.org/mozilla-central/rev/b33f2b2dbbc5

	Jared Wein [:jaws] (please needinfo? me)
	Comment 33 • 12 years ago

A changeset was checked in that referred incorrectly to bug 777463. I believe that it meant to refer to this bug (775463). This is the changeset:
https://hg.mozilla.org/mozilla-central/rev/8b3b879bc63f

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

This was backed out.

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/934749cb1749
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8049b1a5926

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

\o/

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

This needs additional work to be done correctly (right now the animation is weird). I have filed bug 781456 for this.

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

https://hg.mozilla.org/mozilla-central/rev/e8049b1a5926
https://hg.mozilla.org/mozilla-central/rev/934749cb1749