Closed Bug 750977 Opened 12 years ago Closed 12 years ago

Hook async pan/zoom code up to gecko content

Categories

(Core :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla17
blocking-kilimanjaro +
blocking-basecamp +

People

(Reporter: cjones, Assigned: cjones)

References

Details

Attachments

(5 files, 4 obsolete files)

(deleted), image/png
Details
(deleted), patch
Details | Diff | Splinter Review
(deleted), patch
justin.lebar+bug
: review+
Details | Diff | Splinter Review
(deleted), patch
justin.lebar+bug
: review+
vingtetun
: review+
Details | Diff | Splinter Review
(deleted), patch
roc
: review+
Details | Diff | Splinter Review
Here we need to
 - plug the widget/gonk/nsAppShell input-event processing code into the async pan/zoom logic added by bug 750974
 - if necessary, hook the b2g "frontend code" up to the code added by bug 750975

After this work, async pan/zoom will work in b2g, for /some/ content: at least <iframe mozbrowser>, which is used for browser "tabs" by the b2g browser app.  We know when we're inside a mozbrowser in Gecko so we can enable whatever we need to get this working.  (The setup is quite similar to native-fennec's, and almost identical to xul-fennec's.)
It might be worth investing a little bit of dev time here or in a related bug to hook up a desktop widget backend to async pan/zoom, behind a pref, like what we do for omtc.  B2G can be built for desktop so this doesn't have to touch the FF frontend.
Any status on this?   Not having zooming on b2g browser is not an optimal experience.  Nom'ing for blocking-k90 and blocking-basecamp to discuss if this should be a v1 feature.
blocking-basecamp: --- → ?
blocking-kilimanjaro: --- → ?
blocking-basecamp: ? → +
blocking-kilimanjaro: ? → +
Our little hacky prototype forks the webapi.js logic into sync-pan-zoom.js and async-pan-zoom.js.  That's not the right thing to do for engineering reasons, and it's wrong functionally too since panning in general will be a mix of sync/async.  We also need to be aware of async pan/zoom state when hacking in <meta viewport>.

So I think the right approach for this bug is, on the content side
 - merge webapi.js sync pan/zoom logic into BrowserElementChild.js
 - add the async code to set displayport/resolution/scrolloffset there too
 - for the purposes of this bug, ignore touch events when async pan/zoom is in effect

In a followup, we'll need to hack in heuristics to decide when to override default handling (async) for subframes, which we probably won't be able to pan/zoom asynchronously for v1.  That would be a good time to honor content touch-event listeners too.
> So I think the right approach for this bug is, on the content side
>  - merge webapi.js sync pan/zoom logic into BrowserElementChild.js
>  - add the async code to set displayport/resolution/scrolloffset there too
>  - for the purposes of this bug, ignore touch events when async pan/zoom is in effect

This code seems different from most of the other code we have in BEC, which is there to interact with the embedder in some way (send child messages from embedder, or send messages from child to embedder).  In contrast, this code doesn't interact with the embedder at all.

So I might want to move this code out of BEC and into something else nearby.  But that's an aesthetic issue; you (or whoever) should write the patch how you think it should be done, and we'll figure out the aesthetics later.  I agree with the general idea.
We can do this fully platform neutrally, for v1 implementation, without complicating things.  In fact, simplifying things considerably.

We (well, I) have been looking at this problem from the wrong direction: how can we have widgetry drive pan/zoom but integrate with gecko content as much as possible.  That is, move from widget down into content.

But really, we should do this the opposite way: have content drive async pan/zoom and integrate into *omtc*.  That is, set things up in content so that they can run fast on the compositor.  This is more like what we do for async animations.

We always have to pay the cost of DOM-event dispatch into whatever DOM lives in the main process; system app, and in v1, browser.  The current prototype pays this overhead and still hits 60fps on target HW.  So we're fine there.

So here's my current plan
 - don't have AsyncPanZoomController anywhere near widget/
 - in TabParent, when we enable async pan/zoom (!mozapp), have it create an AsyncPanZoomController to manage this state.  Connect up the compositor and indirect layer tree to this.  Attach the TabParent's apzc to its layer tree by using the layer-ID machinery we build for bug 745148.
 - TabParent is the natural and obvious GeckoContentContoller
 - events targeted at TabParent are first handed off to apzc.  If apzc doesn't consume them, then they're forwarded to content.
 - when TabParent goes inactive / hidden, we pause its apzc

Going through TabParent lets us very easily keep BrowserElementChild in the loop for content-side actions like scroll/displayport updates and fallback synchronous scrolling.
Summary: Hook async pan/zoom code up to b2g/gonk widgetry and frontend → Hook async pan/zoom code up to gecko content
Assignee: nobody → jones.chris.g
jlebar, how would you like the display state bits related to BrowserElementChild to be integrated into the code?  I have a patch right now that basically does |cat webapi.js >> BrowserElementChild.js|, and it works fine, but not the most elegant design.

This is probably relevant to the meta viewport hackery philikon is looking at.
As brendan has noted, we're often not very good about design review, so here's something for consideration.  (Also so I don't forget what goes where.)

When a TabParent has async scrolling enabled, its RenderFrameParent will create an AsyncPanZoomController.  The RenderFrameParent will insert that controller into a CompositorParent map, for now using the same "layers ID" we create for direct compositing.  In the future, we'll want to (re-)enable/disable async scrolling per nsScrollableFrame.

When an input event targets an <iframe remote> frame, the TabParent will notify its RenderFrameParent.  If the RFP has an APZC, it will hand off the event to APZC.  Then the TabParent will always forward the event to the content thread.  In the future, we'll want to let APZC consume events, but I'm not 100% sure yet how that should work.

On the content side, the TabParent will dispatch the event, and the BrowseElementChild "panZoomThingie" will listen for input events.  This is what the hacky code in webapi.js does currently.  The panZoomThingie will be able to ask its TabChild whether async pan/zoom is enabled.  If it is, then events targeting the root scrollable frame will be ignored; the panZoomThingie will let AsyncPanZoomController in the UI thread handle pan/zoom.  If the event targets a non-root scrollable frame, panZoomThingie will use the current synchronous scrolling fallback code.  In the future, we'll want to have it notify RFP that it consumed the event, so that we don't async pan/zoom *and* sync pan.

In the future, we'll also want to have panZoomThingie notify RFP when a content touch-event listener might be present.  This will force a round-trip through content before employing async pan/zoom.
See above --- speak now or forever hold your peace! :)

Or r-, I guess ;).
I remember requesting we tag FrameMetrics with a paint count instead of spreading the ugly little firstPaint bool all over the place, but I momentarily forgot that we *already* track paint count in presShell

http://mxr.mozilla.org/mozilla-central/source/layout/base/nsIPresShell.h#1333

roc, do you anticipate any problems that might be caused by resetting the presshell paint count when a new page is loaded?

If not, I'm going to rip out the firstPaint stuff with great prejudice.
Attached patch WIP (obsolete) (deleted) — Splinter Review
vingtetun, jlebar, I'm requesting feedback very narrowly on the changes to webapi.js / BrowserElementChild.js.  I have two questions

 - how do I QI my way from docShell/content to nsITabChild?
 - how would you like the BrowserElementChild.js pan/zoom code to be organized?

The rest of the patch is complete, but blocked on the problems above and the reorganization needed in bug 750974.
Attachment #642511 - Flags: feedback?(justin.lebar+bug)
Attachment #642511 - Flags: feedback?(21)
Comment on attachment 642511 [details] [diff] [review]
WIP

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

::: dom/browser-element/BrowserElementChild.js
@@ +425,5 @@
> +      cwu.setDisplayPortForElement(aDisplayPort.left,
> +                                   aDisplayPort.top,
> +                                   aDisplayPort.width,
> +                                   aDisplayPort.height,
> +                                   element);

Not sure where aDisplayPort comes from.

@@ +582,5 @@
>  };
>  
>  var api = new BrowserElementChild();
> +
> +const ContentPanning = {

I guess the panning code could be added as a .jsm file.
Attachment #642511 - Flags: feedback?(21) → feedback+
Comment on attachment 642511 [details] [diff] [review]
WIP

Now that I've looked at this code, I would definitely prefer for it to live outside BrowserElementChild.js.  We can just put it in a JSM and have BrowserElementChild pass it the window and mm functions?
Attachment #642511 - Flags: feedback?(justin.lebar+bug)
I'll see if I can cargo-cult my way to creating a .jsm.

What would you like it to be called, and where should it live?
(In reply to Chris Jones [:cjones] [:warhammer] from comment #13)
> I'll see if I can cargo-cult my way to creating a .jsm.

There's one in dom/browser-element already; may be a good place to start.

> What would you like it to be called, and where should it live?

In dom/browser-element seems reasonable.  Maybe BrowserElementScroll, or BrowserScroll?  /bikeshed
Attached patch Fix firstpaint (obsolete) (deleted) — Splinter Review
It needs to go away, but at least this makes it work without frontend involvement.
No clue.  Apparently there are multiple interpretations of "first paint", which probably needs to change.
"first paint" as represented by that flag was added by me to indicate the paints where the document being painted is different from the last document painted. This happens on tab switch and new page load. These are the points at which the java viewport information needs to be completely thrown away and re-synced to whatever gecko thinks the viewport is. As of cset 88dd29d90c52 the first paint flag is also set when the java activity is restarted because that is another case where the java viewport information needs to be completely thrown away and re-synced to whatever gecko thinks the viewport is.

As far as I know nobody else has used the first-paint flag until now.
From gecko's perspective, the first paint of a document is when presshell.paintcount == 1.  (There's a list of asterisks though; drawWindow and incomplete transactions are the first two that come to mind.  Have to work through those.)  That should work for navigation.

Tab switching is a different case --- I think the underlying problem there is one pzc for all tabs.  When we switch, it's not the first paint for a new document, but rather switching state to that of a different document.  That problem could be solved by pzc-per-tab or keying state off of an outer window ID.  It also seems to me that a better UX for tab-switch would be restoring the user's previous pan/zoom.  Think of zooming in on a field to enter text, then switching to a new tab to look up some datum, then switching back to enter that datum.

I suspect the current impl might suffer from race conditions around page load, like resetting the pzc state for a visible document before the new one has been painted.  Not sure though.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #20)
> From gecko's perspective, the first paint of a document is when
> presshell.paintcount == 1.

This is the first time I'm hearing about this counter. Does it get reset during back/forward/pagecache-back navigation as well?

> Tab switching is a different case --- I think the underlying problem there
> is one pzc for all tabs.  When we switch, it's not the first paint for a new
> document, but rather switching state to that of a different document.  That
> problem could be solved by pzc-per-tab or keying state off of an outer
> window ID.

In general, yes, if you keep the viewport information for each tab separately in the "front-end" (e.g. the java code in fennec) then that problem should go away.

> It also seems to me that a better UX for tab-switch would be
> restoring the user's previous pan/zoom.  Think of zooming in on a field to
> enter text, then switching to a new tab to look up some datum, then
> switching back to enter that datum.

That is the point of the first-paint flag. The scroll position is stored in gecko on a per-tab basis, and so when we switch tabs the first-paint flag is used to clobber Java's viewport info with the Gecko one. More concretely, if the user scrolls to a text field in one tab, the window.scrollX/scrollY for that tab is updated in Gecko. Switching to a different tab and panning around doesn't affect it, although the java viewport info will be modified to reflect the new tab. When switching back to the first tab the saved scroll position is restored. Ideally the zoom level would follow this pattern as well but currently zoom information is not actually saved per-tab in Gecko (bug 737274 + blockers are tracking this) so we save it in Fennec's browser.js instead.

> I suspect the current impl might suffer from race conditions around page
> load, like resetting the pzc state for a visible document before the new one
> has been painted.  Not sure though.

That's why we only update the java PZC information in SetFirstPaintViewport, which runs on the compositor thread. That is as close to the user-visible display switching over to the other document as we can get. There's a bunch of code in browser.js to handle the periods in the middle of a tab switch where the content "active document" may not be what the user is seeing on their screen and so on. I'm fairly confident we have these race conditions handled properly in Fennec.
(In reply to Kartikaya Gupta (:kats) from comment #21)
> This is the first time I'm hearing about this counter. Does it get reset
> during back/forward/pagecache-back navigation as well?

No, it does not.
(In reply to Justin Lebar [:jlebar] from comment #12)
> Comment on attachment 642511 [details] [diff] [review]
> WIP
> 
> Now that I've looked at this code, I would definitely prefer for it to live
> outside BrowserElementChild.js.  We can just put it in a JSM and have
> BrowserElementChild pass it the window and mm functions?

We discussed this on IRC, and I'm too much of a coward to use a .jsm so we decided to go with an #include for the initial landing.
This is a pretty big chunk of patch, but pleasantly straightforward IMHO.  Hope you agree :).
Attachment #642511 - Attachment is obsolete: true
Attachment #643280 - Attachment is obsolete: true
Attachment #643771 - Flags: review?(roc)
Attachment #643772 - Flags: review?(justin.lebar+bug)
Note: I'm going to fold all these patches together for landing.  I didn't compile them separately, and they're highly interdependent.
Also, the first patch will change a bit after review iteration in bug 750974, but only trivially.
Comment on attachment 643773 [details] [diff] [review]
Have BrowserElementChild service repaint requests and handle fallback synchronous scrolling (for now)

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

I think you forgot to |hg mv b2g/chrome/content/webapi.js dom/browser-element/src/BrowserElementScrolling.js|

I would like to understand the |use strict| regression.

::: b2g/chrome/content/webapi.js
@@ +2,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>   * You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +// Uncomment me when I'm cleaned up.
> +//"use strict";

Why does 'use strict' does not work anymore here?

@@ +183,5 @@
> +  },
> +
> +  _recvViewportChange: function(data) {
> +    let aViewport = data.json;
> +    let aDisplayPort = aViewport.displayPort;

aViewport -> viewport, aDisplayPort -> displayPort since those are not arguments :)

Also you may want to declare displayPort close to the setDisplayPortForElement call to not declare it if it is unused.
Attachment #643773 - Flags: review?(21) → review-
(In reply to Vivien Nicolas (:vingtetun) from comment #30)
> Comment on attachment 643773 [details] [diff] [review]
> Have BrowserElementChild service repaint requests and handle fallback
> synchronous scrolling (for now)
> 
> Review of attachment 643773 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think you forgot to |hg mv b2g/chrome/content/webapi.js
> dom/browser-element/src/BrowserElementScrolling.js|
> 

No ... bugzilla doesn't display this correctly.

> I would like to understand the |use strict| regression.
> 
> ::: b2g/chrome/content/webapi.js
> @@ +2,5 @@
> >   * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> >   * You can obtain one at http://mozilla.org/MPL/2.0/. */
> >  
> > +// Uncomment me when I'm cleaned up.
> > +//"use strict";
> 
> Why does 'use strict' does not work anymore here?
> 

This file is #include'd.  The statement would be meaningless, and probably not pass "use strict";.

> @@ +183,5 @@
> > +  },
> > +
> > +  _recvViewportChange: function(data) {
> > +    let aViewport = data.json;
> > +    let aDisplayPort = aViewport.displayPort;
> 
> aViewport -> viewport, aDisplayPort -> displayPort since those are not
> arguments :)
> 

Changed.

> Also you may want to declare displayPort close to the
> setDisplayPortForElement call to not declare it if it is unused.

Not sure what you mean --- it's always used.
Attachment #643773 - Attachment is obsolete: true
Attachment #643773 - Flags: review?(justin.lebar+bug)
Attachment #643905 - Flags: review?(justin.lebar+bug)
Attachment #643905 - Flags: review?(21)
(In reply to Chris Jones [:cjones] [:warhammer] from comment #31)
> (In reply to Vivien Nicolas (:vingtetun) from comment #30)
> > Comment on attachment 643773 [details] [diff] [review]
> > Have BrowserElementChild service repaint requests and handle fallback
> > synchronous scrolling (for now)
> > 
> > Review of attachment 643773 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > I think you forgot to |hg mv b2g/chrome/content/webapi.js
> > dom/browser-element/src/BrowserElementScrolling.js|
> > 
> 
> No ... bugzilla doesn't display this correctly.
>

sigh.
 
> > I would like to understand the |use strict| regression.
> > 
> > ::: b2g/chrome/content/webapi.js
> > @@ +2,5 @@
> > >   * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> > >   * You can obtain one at http://mozilla.org/MPL/2.0/. */
> > >  
> > > +// Uncomment me when I'm cleaned up.
> > > +//"use strict";
> > 
> > Why does 'use strict' does not work anymore here?
> > 
> 
> This file is #include'd.  The statement would be meaningless, and probably
> not pass "use strict";.
> 

So let's remove it in this case. We can fix BrowserElementChild.js another day.

> 
> > Also you may want to declare displayPort close to the
> > setDisplayPortForElement call to not declare it if it is unused.
> 
> Not sure what you mean --- it's always used.

My bad I've not seen the cwu.setDisplayResolution call.
(In reply to Vivien Nicolas (:vingtetun) from comment #33)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #31)
> > (In reply to Vivien Nicolas (:vingtetun) from comment #30)
> > > I would like to understand the |use strict| regression.
> > > 
> > > ::: b2g/chrome/content/webapi.js
> > > @@ +2,5 @@
> > > >   * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> > > >   * You can obtain one at http://mozilla.org/MPL/2.0/. */
> > > >  
> > > > +// Uncomment me when I'm cleaned up.
> > > > +//"use strict";
> > > 
> > > Why does 'use strict' does not work anymore here?
> > > 
> > 
> > This file is #include'd.  The statement would be meaningless, and probably
> > not pass "use strict";.
> > 
> 
> So let's remove it in this case. We can fix BrowserElementChild.js another
> day.
> 

Done.
Comment on attachment 643772 [details] [diff] [review]
Add nsIDocShell.asyncPanZoomEnabled

> +   * True iff asynchronous panning and zooming is enabled for this
> +   * outer window.

Although this is basically correct, because there's an injective map from docshells to outer windows (ignoring the rare cases when docshells don't have a window, anyway), I'd prefer if you said "this docshell".

Also, could you mention that async pan/zoom only applies to docshells with no in-process parent?  (If I understand this correctly.)
Attachment #643772 - Flags: review?(justin.lebar+bug) → review+
(In reply to Justin Lebar [:jlebar] from comment #35)
> Comment on attachment 643772 [details] [diff] [review]
> Add nsIDocShell.asyncPanZoomEnabled
> 
> > +   * True iff asynchronous panning and zooming is enabled for this
> > +   * outer window.
> 
> Although this is basically correct, because there's an injective map from
> docshells to outer windows (ignoring the rare cases when docshells don't
> have a window, anyway), I'd prefer if you said "this docshell".
> 

Makes sense.  Changed.

> Also, could you mention that async pan/zoom only applies to docshells with
> no in-process parent?  (If I understand this correctly.)

I would rather not make that assumption.  We can implement async pan/zoom for same-process content (bug 775465, e.g.).
> I would rather not make that assumption.  We can implement async pan/zoom for same-process content 
> (bug 775465, e.g.).

sgtm

> +// The code in this included file depends on the |addEventListener|,
> +// |addMessageListener|, |content|, and |Services| symbols being
> +// "exported" from here.

Also |Geometry|, right?
Attachment #643905 - Flags: review?(justin.lebar+bug) → review+
(In reply to Justin Lebar [:jlebar] from comment #37)
> > +// The code in this included file depends on the |addEventListener|,
> > +// |addMessageListener|, |content|, and |Services| symbols being
> > +// "exported" from here.
> 
> Also |Geometry|, right?

Right-o, fixed comment.

Thanks very much for the quick review turnaround.
Comment on attachment 643771 [details] [diff] [review]
Glue async pan/zoom logic up between compositing, event dispatch, and repaint requests

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

::: dom/ipc/TabParent.cpp
@@ +207,5 @@
>  {
>      unused << SendUpdateDimensions(rect, size);
> +    if (RenderFrameParent* rfp = GetRenderFrame()) {
> +        rfp->NotifyDimensionsChanged(size.width, size.height);
> +    }

Fix indent

@@ +579,5 @@
>  
> +RenderFrameParent*
> +TabParent::GetRenderFrame()
> +{
> +  if (!ManagedPRenderFrameParent().Length()) {

Use IsEmpty()

@@ +880,4 @@
>                               int32_t* aMaxTextureSize,
>                               uint64_t* aLayersId)
>  {
> +  MOZ_ASSERT(ManagedPRenderFrameParent().Length() == 0);

Use IsEmpty()

::: gfx/layers/ipc/CompositorParent.cpp
@@ +617,5 @@
> +    float tempScaleDiffY = rootScaleY * mYScale;
> +
> +    nsIntPoint metricsScrollOffset(0, 0);
> +    if (metrics.IsScrollable())
> +      metricsScrollOffset = metrics.mViewportScrollOffset;

{}

::: layout/ipc/RenderFrameUtils.h
@@ +14,5 @@
> +enum ScrollingBehavior {
> +  DEFAULT_SCROLLING,
> +  ASYNC_PAN_ZOOM,
> +  SCROLLING_BEHAVIOR_SENTINEL
> +};

Document these
Attachment #643771 - Attachment is obsolete: false
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #40)
> Comment on attachment 643771 [details] [diff] [review]
> Glue async pan/zoom logic up between compositing, event dispatch, and
> repaint requests
> 
> Review of attachment 643771 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/ipc/TabParent.cpp
> @@ +207,5 @@
> >  {
> >      unused << SendUpdateDimensions(rect, size);
> > +    if (RenderFrameParent* rfp = GetRenderFrame()) {
> > +        rfp->NotifyDimensionsChanged(size.width, size.height);
> > +    }
> 
> Fix indent
> 

Done.

> @@ +579,5 @@
> >  
> > +RenderFrameParent*
> > +TabParent::GetRenderFrame()
> > +{
> > +  if (!ManagedPRenderFrameParent().Length()) {
> 
> Use IsEmpty()
> 

Changed

> @@ +880,4 @@
> >                               int32_t* aMaxTextureSize,
> >                               uint64_t* aLayersId)
> >  {
> > +  MOZ_ASSERT(ManagedPRenderFrameParent().Length() == 0);
> 
> Use IsEmpty()
> 

Changed

> ::: gfx/layers/ipc/CompositorParent.cpp
> @@ +617,5 @@
> > +    float tempScaleDiffY = rootScaleY * mYScale;
> > +
> > +    nsIntPoint metricsScrollOffset(0, 0);
> > +    if (metrics.IsScrollable())
> > +      metricsScrollOffset = metrics.mViewportScrollOffset;
> 
> {}
> 

While I'm here, sure.  Changed.

> ::: layout/ipc/RenderFrameUtils.h
> @@ +14,5 @@
> > +enum ScrollingBehavior {
> > +  DEFAULT_SCROLLING,
> > +  ASYNC_PAN_ZOOM,
> > +  SCROLLING_BEHAVIOR_SENTINEL
> > +};
> 
> Document these

Done.
https://hg.mozilla.org/mozilla-central/rev/19b28e14df61
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: