Zoom constraints from child processes sent to the root could get dropped

RESOLVED FIXED

Status

Firefox OS
Gaia
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: kats, Assigned: kats)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Created attachment 796183 [details] [diff] [review]
Pull zoom constraints into APZC on creation

The call to SendUpdateZoomConstraints in TabChild.cpp can happen before the APZC tree for that child process is created. In such a case, the zoom constraints are dropped. In a world where APZC is turned on everywhere, this manifests as the homescreen being zoomable. Even if I put in a smallish hack to hang on to the zoom constraints and hand them to the APZC when it does get created, the APZC could be destroyed and recreated based on DLBI heuristics. In the above example this means that the zoom constraints get reset when locking the homescreen and re-entering it.

To fix this we need to store the zoom constraints somewhere and 'pull' them into the APZC in addition to 'pushing' them into the APZC when they change. The attached patch does that but probably could be cleaner.
Attachment #796183 - Flags: review?(botond)
Assignee: nobody → bugmail.mozilla
Comment on attachment 796183 [details] [diff] [review]
Pull zoom constraints into APZC on creation

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

::: gfx/layers/composite/APZCTreeManager.cpp
@@ +162,5 @@
> +          CSSToScreenScale minZoom, maxZoom;
> +          if (state->mController->GetZoomConstraints(&allowZoom, &minZoom, &maxZoom)) {
> +            controller->UpdateZoomConstraints(allowZoom, minZoom, maxZoom);
> +          }
> +        }

When reading this code, I found it confusing that there is a 'state->mController' which is a GeckoContentController, and a 'controller' which is an AsyncPanZoomController. Perhaps it would be clearer to name the local variables 'apzc' and 'newApzc'? This can be done in a separate change.

::: gfx/layers/ipc/GeckoContentController.h
@@ +69,5 @@
> +   * false if there are no last known zoom constraints.
> +   */
> +  virtual bool GetZoomConstraints(bool *aOutAllowZoom,
> +                                  CSSToScreenScale* aOutMinZoom,
> +                                  CSSToScreenScale* aOutMaxZoom)

nit: inconsistent spacing around *

@@ +72,5 @@
> +                                  CSSToScreenScale* aOutMinZoom,
> +                                  CSSToScreenScale* aOutMaxZoom)
> +  {
> +    return false;
> +  }

I wonder whether the benefit of having a default implementation here outweighs the risk of a derived class forgetting to implement this?

::: layout/ipc/RenderFrameParent.cpp
@@ +598,5 @@
> +      *aOutMaxZoom = mMaxZoom;
> +    }
> +    return mHaveZoomConstraints;
> +  }
> +

I think it would made the code a bit nicer to have a structure called ZoomConstraints that encapsulates allowZoom, minZoom, and maxZoom. I have no strong feelings about it though.
Attachment #796183 - Flags: review?(botond) → review+
Created attachment 804244 [details] [diff] [review]
Pull zoom constraints into APZC on creation (v2)

> ::: gfx/layers/ipc/GeckoContentController.h
> @@ +69,5 @@
> > +   * false if there are no last known zoom constraints.
> > +   */
> > +  virtual bool GetZoomConstraints(bool *aOutAllowZoom,
> > +                                  CSSToScreenScale* aOutMinZoom,
> > +                                  CSSToScreenScale* aOutMaxZoom)
> 
> nit: inconsistent spacing around *

Fixed nit, carrying r+.
Attachment #796183 - Attachment is obsolete: true
Attachment #804244 - Flags: review+
Created attachment 804246 [details] [diff] [review]
Rename controller and newController to apzc and newApzc

As per previous review comments, this is a straightforward change.
Attachment #804246 - Flags: review?(botond)
(In reply to Botond Ballo [:botond] from comment #1)
> I wonder whether the benefit of having a default implementation here
> outweighs the risk of a derived class forgetting to implement this?

In my WIP over in bug 912657 I actually make most of these functions have default implementations and leave only the two critical ones as pure virtual. I would say that this one is also not very critical.

> 
> I think it would made the code a bit nicer to have a structure called
> ZoomConstraints that encapsulates allowZoom, minZoom, and maxZoom. I have no
> strong feelings about it though.

Yeah, I agree. We have a similar ZoomConstraints class in Fennec (in Java though) because these constraints get passed around a lot. I filed bug 915985 for this.

Updated

4 years ago
Attachment #804246 - Flags: review?(botond) → review+
https://hg.mozilla.org/integration/b2g-inbound/rev/64751fbe352f
https://hg.mozilla.org/integration/b2g-inbound/rev/3bbea4db9ac6
https://hg.mozilla.org/mozilla-central/rev/64751fbe352f
https://hg.mozilla.org/mozilla-central/rev/3bbea4db9ac6
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Blocks: 949585
No longer blocks: 949585
You need to log in before you can comment on or make changes to this bug.