912657 - Create an APZC for the <div> that contains the toolbar and the browser iframe

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Botond Ballo [:botond]]

As part of the dynamic toolbar implementation for B2G, bug 835152 adds a scrollable <div> grouping the toolbar and the browser iframe. To get the desired behaviour, we need overscroll hand-off (bug 898478) to work between the iframe and the <div>. This means we need the <div> to have an APZC.

Currently, the <div> exists in the parent process (this will change when dzbarsky's patches for making the browser app have its own process land, but we likely want dynamic toolbar working before that). Currently we don't have APZCs for things in the parent process.

Introducing APZCs for everything in the parent process right away may be too big of a disruption, so we are proposing to create an APZC *only* for the <div> in question, for now.

Comment 1

[Botond Ballo [:botond]]

APZCs need a GeckoContentController implementation to interface with Gecko. Currently, APZCs on B2G use RemoteContentController, but that's only suitable for layers that reside in a child process. A new implementation of GeckoContentController - a "LocalContentController" will need to be written for layers that reside in the parent process (bug 880024).

Once a LocalContentController is written and registered with the parent process, all scrollable layers in the parent process will have APZCs. Since we only actually want one of them (the <div> containing the browser iframe) to have an APZC, we'll need to disable the APZCs for other scrollable layers in the parent process.

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Attachment: WIP

I have verified that the controller is getting picked up by the APZCTreeManager, but I haven't been able to check that APZCs get created with it yet. It would be easier if bug 898444 were fixed because then I could just test with Botond's patch from bug 835152. As it is though I don't see any scrollable layers in the root process at all so I'm going to have hack something in to test that code. Posting the WIP for now anyway.

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Attachment: WIP v2

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Attachment: WIP v2

This time without the printfs. This patch (combined with bug 880024 and bug 898444) will allow the APZC to be created although the behaviour leaves much to be desired.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

I've been working on this as much as I can between reviewing a bajillion patches and other things. Current state is that in order for this to work something needs to deliver the touch events to the APZCTreeManager in the root process before it goes through Gecko hit detection and arrives at the TabChild. Otherwise Gecko just handles the input events and it never gets to the APZC so things in the root process will not scroll with APZC properly. With this patch alone the scrolling ends up being done by BrowserElementPanning.js which we want to avoid.

Getting this fixed will mean taking a detour to straighten out the input event flow on B2G (and possibly Metro since they are now using the same functions). In my head I have a fairly clear idea of what needs to be done.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

So dzbarsky is close to landing bug 879475 and if that lands then this problem is much simplified. With that bug done, the browser chrome will live in a separate process and so we won't need to tinker around with the root process. Instead, we will just need to turn on APZC in the new browser-chrome process and make sure that works. Even the input handling should get routed through TabParent/TabChild and work the same as for content processes (although I need to verify that), so bug 920036 will no longer block getting this done. We might still want that bug for other reasons.

Comment 7

[Botond Ballo [:botond]]

One missing piece of functionality in this patch is that no one calls AsyncPanZoomController::UpdateCompositionBounds() when the composition bounds of a parent-process APZC's frame metrics change (this can happen for instance when the screen orientation changes).

For child-process APZC's, AsyncPanZoomController::UpdateCompositionBounds() is called via TabParent::UpdateDimensions() -> RenderFrameParent::NotifyDimensionsChanged() -> APZCTreeManager::UpdateCompositionBounds().

Comment 8

[Botond Ballo [:botond]]

We're at the stage in the dynamic toolbar work where we need to decide how to go forward with this. The options are:

  1) Assume the browser will remain in the parent process, and continue the work Kats started
     in this bug to make APZC work in the parent process.

  2) Move the browser to the child process, which basically means landing bug 879475.

Kats and I are gravitating towards (1) because we feel like we have a better grasp on its scope than (2). We are open to suggestions though.

Comment 9

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #8)
> We're at the stage in the dynamic toolbar work where we need to decide how
> to go forward with this. The options are:
> 
>   1) Assume the browser will remain in the parent process, and continue the
> work Kats started
>      in this bug to make APZC work in the parent process.
> 
>   2) Move the browser to the child process, which basically means landing
> bug 879475.
> 
> Kats and I are gravitating towards (1) because we feel like we have a better
> grasp on its scope than (2). We are open to suggestions though.

For (1), there are two known issues that need to be resolved:

  a) Calling UpdateCompositionBounds() when the screen orientation changes (see comment #7).

  b) When the user pans on the toolbar itself, the panning does not work correctly
     (the toolbar becomes black). This is caused by the issue described in comment #5,
     buts Kats suggested a way of fixing it that doesn't depend on bug 920036 being
     resolved: cancel touch events sent to the toolbar from BrowserElementPanning.js.

Comment 10

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #9)
>   a) Calling UpdateCompositionBounds() when the screen orientation changes
> (see comment #7).

Turns out that the composition bounds are now updated in every layers update thanks to bug 935624, so this isn't necessary any more (in fact, UpdateCompositionBounds() can probably be removed altogether). However, I still see misbehaviour when changing the orientation with the dynamic toolbar patches (bug 912666). Will investigate further.

Comment 11

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #10)
> (In reply to Botond Ballo [:botond] from comment #9)
> >   a) Calling UpdateCompositionBounds() when the screen orientation changes
> > (see comment #7).
> 
> Turns out that the composition bounds are now updated in every layers update
> thanks to bug 935624, so this isn't necessary any more (in fact,
> UpdateCompositionBounds() can probably be removed altogether). However, I
> still see misbehaviour when changing the orientation with the dynamic
> toolbar patches (bug 912666). Will investigate further.

So one problem is that in NotifyLayersUpdated(), we just update the composition bounds themselves, but in UpdateCompositionBounds() we also adjust mZoom (we multiply it by the ratio of the new bounds to the old bounds).

Modifying NotifyLayersUpdated() to call UpdateCompositionBounds() rather than just assigning the new composition bounds helps, but I still see strange behaviour (an unexpected zoom level after a screen orientation change) for scrollable subframes.

Comment 12

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #11)
> (In reply to Botond Ballo [:botond] from comment #10)
> > (In reply to Botond Ballo [:botond] from comment #9)
> > >   a) Calling UpdateCompositionBounds() when the screen orientation changes
> > > (see comment #7).
> > 
> > Turns out that the composition bounds are now updated in every layers update
> > thanks to bug 935624, so this isn't necessary any more (in fact,
> > UpdateCompositionBounds() can probably be removed altogether). However, I
> > still see misbehaviour when changing the orientation with the dynamic
> > toolbar patches (bug 912666). Will investigate further.
> 
> So one problem is that in NotifyLayersUpdated(), we just update the
> composition bounds themselves, but in UpdateCompositionBounds() we also
> adjust mZoom (we multiply it by the ratio of the new bounds to the old
> bounds).
> 
> Modifying NotifyLayersUpdated() to call UpdateCompositionBounds() rather
> than just assigning the new composition bounds helps, but I still see
> strange behaviour (an unexpected zoom level after a screen orientation
> change) for scrollable subframes.

Kats and I discussed this, and agreed that it is not appropriate to change the zoom in the way UpdateCompositionBounds() does in NotifyLayersUpdated(), since UpdateCompositionBounds() is meant to only be called on the root scroll frame.

The remaining misbehaviour with screen orientation changes seems related to bug 937896, which I just filed.

Comment 13

[Botond Ballo [:botond]]

Attachment: bug912657.patch

Renamed DynamicToolbarController to ParentProcessController and conditioned setting wantSubApzc to true on the platform being B2G.

I tested the patch with other parent-process scrollable elements like select dialogs, they are still working fine.

I think this patch is ready to be reviewed and land.

Comment 14

[Botond Ballo [:botond]]

Try run: https://tbpl.mozilla.org/?tree=Try&rev=5f6e17a0f65d

Comment 15

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8334632 [details] [diff] [review]
bug912657.patch

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

It might be a good idea to announce this on the dev-gaia or dev-b2g lists, just to let people know. I'm worried that this might have unexpected side effects and I'd like people to report them as soon as possible if they run into them.

Comment 16

[Botond Ballo [:botond]]

(In reply to (PTO Nov21-Dec01) Kartikaya Gupta (email:[e-mail]) from comment #15)
> Comment on attachment 8334632 [details] [diff] [review]
> bug912657.patch
> 
> Review of attachment 8334632 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It might be a good idea to announce this on the dev-gaia or dev-b2g lists,
> just to let people know. I'm worried that this might have unexpected side
> effects and I'd like people to report them as soon as possible if they run
> into them.

I've posted on dev-b2g: https://groups.google.com/forum/#!topic/mozilla.dev.b2g/QJEF4sILb3Q.

Comment 17

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #14)
> Try run: https://tbpl.mozilla.org/?tree=Try&rev=5f6e17a0f65d

Looks clean.

Comment 18

[Botond Ballo [:botond]]

Attachment: bug912657.patch

Removed the 'wantSubApzc = true' line which caused APZCs to be created for all scrollable subframes in the parent process. This causes problems (some of the scrollable subframes in the parent process are hidden but on top of other layers, and steal input from those layers - this is technically a flaw in our current hit testing implementation, which will be fixed once we do hit testing based on touch-sensitive regions).

For the dynamic toolbar work, it's sufficient to create APZCs for scroll-grabbing subframes in the parent process, and part 3 of the bug 912666 already does that, so there is no need to create them for all subframes in the parent process.

Comment 19

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Comment on attachment 8336235 [details] [diff] [review]
bug912657.patch

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

::: gfx/layers/ipc/GeckoContentController.h
@@ +33,5 @@
>     * the current scroll offset. This should eventually round-trip back to
>     * AsyncPanZoomController::ZoomToRect with the dimensions that we want to zoom
>     * to.
>     */
> +  virtual void HandleDoubleTap(const CSSIntPoint& aPoint) {}

Suggestion: I'd prefer stubs in ParentProcessController to leave this pure virtual.

Comment 20

[Botond Ballo [:botond]]

Attachment: bug912657.patch

Updated patch to address Anthony's review comment.

Comment 21

[Botond Ballo [:botond]]

https://hg.mozilla.org/integration/b2g-inbound/rev/c67f5daf8e21

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c67f5daf8e21