972081 - Highlighting seems broken with APZC enabled

Status: VERIFIED FIXED

(Core :: Panning and Zooming, defect)

Description

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

There is a few different things  that looks broken with APZC. One is related to the fact that we don't use the Kinetic helper and so it never reset it's values and it ends up clearing the highlight instantly. The other is related to how we dispatch the mouse events right after a touchend. This does not let the time for the content to highlight itself.

Comment 1

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: highlight.fixes.patch

Comment 2

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

Comment on attachment 8375185 [details] [diff] [review]
highlight.fixes.patch

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

I'm not too familiar with the BEP.js changes so you might want another reviewer for that. The changes look sane to me though. Some comments on TabChild changes - I'd like to see an updated version of the patch before r+'ing.

::: dom/ipc/TabChild.cpp
@@ +292,5 @@
>    , mUpdateHitRegion(false)
>    , mContextMenuHandled(false)
>    , mWaitingTouchListeners(false)
>  {
> +  Preferences::AddIntVarCache(&sActiveDurationMs,

This uses sActiveDurationMs instead of mActiveDurationMs. I think I would prefer making it static, in case TabChild instances can be destroyed on non-B2G platforms. We don't want to leave a instance variable from a destroyed object in the IntVarCache. Alternatively, if there's some way to clear the intvarcache in the TabChild destructor that's fine too.

@@ +1628,3 @@
>  
> +  mSingleTapTimer = NewRunnableMethod(this,
> +                                      &TabChild::FireSingleTapEvent);

You can pass currentPoint as an additional argument to NewRunnableMethod, so that you don't need mSingleTapPoint at all. See for instance the NewRunnableMethod invocation in AsyncPanZoomController::OnSingleTapUp.

Also, mSingleTapTimer doesn't look needed; it's assigned here but never cleared which seems like an unncessary footgun. Just use a local var instead.

Comment 3

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Thanks for the quick feedback. I will addressed your comments and ask to fabrice for the related BEC changes. I'm pretty sure he will be more than happy to kill the event loop spinning code.

Comment 4

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: highlight.fixes.patch

Patch v2.

Comment 5

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: highlight.fixes.patch

And voila the v3!

Comment 6

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

Comment on attachment 8375829 [details] [diff] [review]
highlight.fixes.patch

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

LGTM, thanks!

Comment 7

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

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

Comment 8

[Phil Ringnalda (:philor)]

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

Comment 9

[Joe Cheng [:jcheng] (please needinfo)]

nominating per https://bugzilla.mozilla.org/show_bug.cgi?id=975923#c30

Comment 10

[James Zhang (Spreadtrum)]

When can we land it on v1.3T?

Comment 12

[Joe Cheng [:jcheng] (please needinfo)]

1.3T+
James, can your team uplift this to resolve Bug 975923? thanks

Comment 13

[James Zhang (Spreadtrum)]

(In reply to Joe Cheng [:jcheng] from comment #12)
> 1.3T+
> James, can your team uplift this to resolve Bug 975923? thanks
Ying will.

Comment 14

[ying.xu]

Attachment: 972081_v1.3t.patch

patch for 1.3t, there are some conflicts and I fixed them

Please review it again

Comment 15

[Jason Smith [:jsmith]]

Fabrice - Did you intend this blocking decision to be 1.3+ or 1.3T+?

Comment 16

[[:fabrice] Fabrice Desré]

Comment on attachment 8394685 [details] [diff] [review]
972081_v1.3t.patch

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

Thanks Ying!

Comment 17

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/dcaf06de49f5

Comment 18

[John Hammink]

Verified fixed against the original STR in 975923.

Comment 19

[John Hammink]

...although we should probably do a testpass around APZC to verify this functionality on Tarako, as it's blocking us there. Let's investigate this some more.

Comment 20

[Jason Smith [:jsmith]]

QA Wanted isn't the right flag here - this patch is already landed here.