Closed Bug 906877 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: Panning and Zooming, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla28
Tracking Status
firefox28 --- fixed

People

(Reporter: botond, Assigned: kats)

References

Details

(Whiteboard: [qa-])

Attachments

(1 file, 2 obsolete files)

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.
Blocks: multi-apzc
Depends on: 866232
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).
Attached patch WIP (obsolete) — Splinter Review
Blocks: 901607
Attached patch Patch (obsolete) — Splinter Review
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.
Assignee: nobody → bugmail.mozilla
Attachment #793515 - Attachment is obsolete: true
Attachment #794230 - Flags: feedback?(jmathies)
Comment on attachment 794230 [details] [diff] [review]
Patch

Works great, will post an update to my work in the other bug.
Attachment #794230 - Flags: feedback?(jmathies) → feedback+
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.
Attachment #794230 - Flags: review?(bgirard)
Attachment #794230 - Flags: review?(21)
Attachment #794230 - Flags: review?(bgirard) → review+
No longer blocks: 901607
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.
Attachment #794230 - Flags: review?(21) → review+
https://hg.mozilla.org/mozilla-central/rev/669d3fd3ea2b
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla26 → ---
Component: Graphics: Layers → Panning and Zooming
No longer blocks: multi-apzc
Depends on: multi-apzc
Attached patch Patch v2Splinter Review
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.
Attachment #794230 - Attachment is obsolete: true
Attachment #824765 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/b1941b03d0a6
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
I don't understand why you removed mDisableNextTouchBatch.
(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.
(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.
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.