TabChild should not assume that APZ being turned on implies there is a root APZC

RESOLVED INVALID

Status

()

RESOLVED INVALID
5 years ago
5 years ago

People

(Reporter: botond, Assigned: botond)

Tracking

Trunk
ARM
Gonk (Firefox OS)
Points:
---

Firefox Tracking Flags

(blocking-b2g:1.3+)

Details

Attachments

(3 obsolete attachments)

TabChild appears to assume that if APZ is turned on (TabChild::IsAsyncPanZoomEnabled()), then there will be at least one (root) APZC.

For example, in TabChild::Observe(), the code that handles "before-first-paint" sets the viewport and resolution to some default values, expecting that the following call to HandlePossibleViewportChange() will set them to correct values. Both pieces of code only execute if APZ is enabled; the code in HandlePossibleViewportChange() only executes if there is also a root APZC.

With the patch for bug 942995, we can now have APZ enabled without having a root APZC, which means the resolution will be set to the (undesirable) default values, and then never set to the correct values.
Created attachment 8338176 [details] [diff] [review]
Part 1 - Do not exit early from TabChild::HandlePossibleViewportChange() if there is no root APZC
Assignee: nobody → botond
Attachment #8338176 - Flags: review?(bgirard)
blocking-b2g: --- → 1.3+
Flags: needinfo?(nhirata.bugzilla)
Attachment #8338176 - Attachment description: bug943164.patch → Part 1 - Do not exit early from TabChild::HandlePossibleViewportChange() if there is no root APZC
Attachment #8338176 - Flags: review?(chrislord.net)

Updated

5 years ago
Attachment #8338176 - Flags: review?(bgirard) → review+
Created attachment 8338796 [details] [diff] [review]
Part 2 - Do not increase CSS viewport height beyond screen height if there is no root APZC
Attachment #8338796 - Flags: review?(chrislord.net)
Attachment #8338796 - Flags: review?(bgirard)
Comment on attachment 8338796 [details] [diff] [review]
Part 2 - Do not increase CSS viewport height beyond screen height if there is no root APZC

Whoops, this is not quite finished.
Attachment #8338796 - Attachment is obsolete: true
Attachment #8338796 - Flags: review?(chrislord.net)
Attachment #8338796 - Flags: review?(bgirard)
Created attachment 8338809 [details] [diff] [review]
Part 2 - Do not increase CSS viewport height beyond screen height if there is no root APZC
Attachment #8338809 - Flags: review?(chrislord.net)
Attachment #8338809 - Flags: review?(bgirard)
Comment on attachment 8338809 [details] [diff] [review]
Part 2 - Do not increase CSS viewport height beyond screen height if there is no root APZC

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

IMO I'm not convinced this is the right place at all for us to have logic that changes the viewport like this. But you're not making it worse.
Attachment #8338809 - Flags: review?(bgirard) → review+
To be clear, I was referring to the original code you modified, not your changes.
Attachment #8338176 - Attachment is obsolete: true
Attachment #8338176 - Flags: review?(chrislord.net)
Attachment #8338809 - Attachment is obsolete: true
Attachment #8338809 - Flags: review?(chrislord.net)
After further discussion (see bug 940691), we do always want to keep a root APZC when APZ is enabled. Resolving as invalid.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → INVALID
Flags: needinfo?(nhirata.bugzilla)
You need to log in before you can comment on or make changes to this bug.