906877 - Remove observer messages in BrowserElementPanning.js that have been obsoleted by multi-apzc

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Botond Ballo [:botond]]

Now that subframes are async-scrollable, some messages sent from BrowserElementPanning.js to APZC, such as detect-scrollable-subframe, are no longer used. These messages, and the corresponding APZC methods, can be removed.

Comment 1

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

Actually we can get rid of cancel-default-pan-zoom as well. I forgot but was recently reminded by mbrubeck that the code path to cancel pan/zoom via touch events is at http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/AsyncPanZoomController.cpp#1297 (The ContentReceivedTouch codepath).

Comment 2

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

Attachment: WIP

Comment 3

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

Attachment: Patch

I believe this removes all the things that are unnecessary with APZC now handling subframe scrolling as well. Metro is planning on using BrowserElementPanning.js for scrolling subframes (I think) but it will be mutually exclusive with APZC, so it shouldn't affect removal of this code.

Jim, can you check that applying this patch doesn't break the intended behaviour of bug 901607? I'm not sure I know exactly how to test that.

Comment 4

[Jim Mathies [:jimm]]

Comment on attachment 794230 [details] [diff] [review]
Patch

Works great, will post an update to my work in the other bug.

Comment 5

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

Comment on attachment 794230 [details] [diff] [review]
Patch

Requesting review from Vivien for the BrowserElementPanning.js bits (and anything else you care to look at) and BenWa for the rest. It's mostly code deletion.

Comment 6

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

Comment on attachment 794230 [details] [diff] [review]
Patch

Less code, yeah! As a nit I would remove the _asyncPanZoomForViewportFrame and replace it directly by a call to docshell.asyncPanZoomEnabled since it is use in only one place.

Comment 7

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

https://hg.mozilla.org/integration/b2g-inbound/rev/669d3fd3ea2b

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/669d3fd3ea2b

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Backed out for causing Marionette failures that I didn't notice until after the merge.
https://hg.mozilla.org/mozilla-central/rev/6a2fa1dcddf0

https://tbpl.mozilla.org/php/getParsedLog.php?id=27339664&tree=B2g-Inbound

Comment 10

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

Attachment: Patch v2

I finally got around to debugging this, turns out my patch to BrowserElementPanning.js removed a non-dead line of code (the return value was dead but the function call had side-effects). Putting that back fixed it for me locally. Carrying r+ on the new patch, pushed to try; will land it once the tree reopens.

Comment 11

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

Try was green at https://tbpl.mozilla.org/?tree=Try&rev=cb39e3c2d164, so pushed to b2g-inbound:

https://hg.mozilla.org/integration/b2g-inbound/rev/b1941b03d0a6

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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

Comment 13

[Jim Mathies [:jimm]]

I don't understand why you removed mDisableNextTouchBatch.

Comment 14

[Jim Mathies [:jimm]]

(In reply to Jim Mathies [:jimm] from comment #13)
> I don't understand why you removed mDisableNextTouchBatch.

I've brought this back and improved the tracking a bit. Will post updated patches in bug 918273.

Comment 15

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

(In reply to Jim Mathies [:jimm] from comment #13)
> I don't understand why you removed mDisableNextTouchBatch.

Well it was only ever set to true in a function that was also being removed, so I ripped out all the dependent code too. But yeah, you might need to bring that back as well.