Force-enable event-regions when APZ is enabled

RESOLVED FIXED in Firefox 39

Status

()

Core
Panning and Zooming
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: kats, Assigned: kats)

Tracking

unspecified
mozilla39
All
Gonk (Firefox OS)
Points:
---

Firefox Tracking Flags

(firefox39 fixed)

Details

(Whiteboard: gfx-noted)

Attachments

(2 attachments, 1 obsolete attachment)

I think we should force-enable event regions if APZ is enabled. That way we can always assume event regions in the APZ side of things and start ripping out a bunch of unnecessary code. The other reason for doing this is that turning on event-regions incurs a talos regression which is kind of expected because of the extra region operations. However it seems unfair to pin this on event-regions since the real reason we need this is for APZ, and so if we auto-enable event regions with APZ then we can more easily justify the talos regression with the APZ improvements.

I plan on leaving the event-regions pref separate as well so we can enable event-regions without enabling APZ.
Created attachment 8569929 [details] [diff] [review]
Part 1 - Enforce event regions enabled when APZ is enabled
Created attachment 8569930 [details] [diff] [review]
Part 2 - Look all this code that's not needed anymore!
(Assignee)

Comment 3

3 years ago
greentry
https://treeherder.mozilla.org/#/jobs?repo=try&revision=59c1c268ac50
Created attachment 8570107 [details] [diff] [review]
Part 1 - Enforce event regions enabled when APZ is enabled

Try is green. I updated this slightly to get rid of the SCOPED_GFX_PREF stuff in the gtests also which is no longer needed.
Attachment #8569929 - Attachment is obsolete: true
Attachment #8570107 - Flags: review?(tnikkel)
Attachment #8570107 - Flags: review?(botond)
Comment on attachment 8569930 [details] [diff] [review]
Part 2 - Look all this code that's not needed anymore!

Mostly code deletion so it should be a quick review. But please let me know if anybody needs to keep any of this code for things I've overlooked. Note in particular I kept the mMayHaveTouchListener stuff in the window, because it's used to call RegisterTouchWindow() and UnregisterTouchWindow() on some platforms.
Attachment #8569930 - Flags: review?(dvander)
Attachment #8569930 - Flags: review?(bugs)
Attachment #8569930 - Flags: review?(botond)

Comment 6

3 years ago
Comment on attachment 8569930 [details] [diff] [review]
Part 2 - Look all this code that's not needed anymore!

Surprising we can remove this all, especially all MayHaveTouchCaret, but great.
Attachment #8569930 - Flags: review?(bugs) → review+
Comment on attachment 8569930 [details] [diff] [review]
Part 2 - Look all this code that's not needed anymore!

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

Sweet.
Attachment #8569930 - Flags: review?(dvander) → review+
Comment on attachment 8570107 [details] [diff] [review]
Part 1 - Enforce event regions enabled when APZ is enabled

Would I be correct in saying that we still create scroll info layers on android, and gfxPrefs::AsyncPanZoomEnabled() is false on android? If so, then in the unlikely event of no visible scrolled content the scroll info layer wouldn't flatten away and we'd also create no layer for the scroll info layer.
Currently, yes, I guess we do create scrollinfo layers on Android, and gfxPrefs::AsyncPanZoomEnabled() is false there. However we don't do subframe scrolling on android, and as far as I know we never use the scrollinfo layers. So if they aren't there it doesn't matter. The only layer(s) that we manipulate on Android are the root scrollable layers, as determined by [1]. If the root content isn't scrollable we should still be handling it gracefully. Anyway I'll throw the fennec build on my device and test it to make sure there's no problems.

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/Layers.cpp?rev=23c78316747f#86
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #9)
> Currently, yes, I guess we do create scrollinfo layers on Android, and
> gfxPrefs::AsyncPanZoomEnabled() is false there. However we don't do subframe
> scrolling on android, and as far as I know we never use the scrollinfo
> layers. So if they aren't there it doesn't matter. The only layer(s) that we
> manipulate on Android are the root scrollable layers, as determined by [1].
> If the root content isn't scrollable we should still be handling it
> gracefully. Anyway I'll throw the fennec build on my device and test it to
> make sure there's no problems.
> 
> [1]
> http://mxr.mozilla.org/mozilla-central/source/gfx/layers/Layers.
> cpp?rev=23c78316747f#86

Actually I take that back, we still have containerfull scrolling on android, so we don't get to the scroll info layer code for the root scroll frame. So with containerless scrolling if there is no visible scrolled content the scroll info layer will also create no layer, so there will be no scrollable content layer in the tree.
Is that a problem?
I see no problems with the Fennec try build from comment 3.

Updated

3 years ago
Attachment #8570107 - Flags: review?(botond) → review+
Comment on attachment 8569930 [details] [diff] [review]
Part 2 - Look all this code that's not needed anymore!

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

::: gfx/layers/FrameMetrics.h
@@ -584,5 @@
> -  bool mMayHaveTouchListeners;
> -
> -  // Whether or not this frame may have a touch caret.
> -  bool mMayHaveTouchCaret;
> -

Yay fewer FrameMetrics members!
Attachment #8569930 - Flags: review?(botond) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #11)
> Is that a problem?

If there was content to scroll (but just not currently visible) we wouldn't be able to scroll it.
(In reply to Timothy Nikkel (:tn) from comment #14)
> If there was content to scroll (but just not currently visible) we wouldn't
> be able to scroll it.

Sorry, this isn't clear to me. What I understand from this is if you have a page like http://people.mozilla.org/~kgupta/bug/1137267.html with content down at the bottom that's not initially visible, AND if layout.scroll.root-frame-containers is set to false (i.e. containerless root is enabled), then the page is not going to be scrollable.

And indeed, that is the case. But that happens with or without these patches, and seems to be a problem with containerless-root scrolling that is independent of these patches. If I reset layout.scroll.root-frame-containers back to true (the default) then everything that was scrollable on Fennec before is still scrollable.

If you disagree can you provide more details and explanation?
Flags: needinfo?(tnikkel)
Whiteboard: gfx-noted
Okay, I guess we'll have to investigate that pre-existing issue separately from this patch. I did not expect it to already be an issue.
Flags: needinfo?(tnikkel)
Attachment #8570107 - Flags: review?(tnikkel) → review+
(Assignee)

Comment 17

3 years ago
landing
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/3ec104010430
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/420b643efff1
https://hg.mozilla.org/mozilla-central/rev/3ec104010430
https://hg.mozilla.org/mozilla-central/rev/420b643efff1
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Depends on: 1154478

Updated

3 years ago
See Also: → bug 1151667
You need to log in before you can comment on or make changes to this bug.