Closed Bug 909881 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox OS Graveyard :: Gaia, defect)

All
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: kats, Assigned: kats)

References

Details

Attachments

(2 files, 1 obsolete file)

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.
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+
> ::: 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+
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.
Attachment #804246 - Flags: review?(botond) → review+
You need to log in before you can comment on or make changes to this bug.