Last Comment Bug 750977 - Hook async pan/zoom code up to gecko content
: Hook async pan/zoom code up to gecko content
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: General (show other bugs)
: unspecified
: ARM Gonk (Firefox OS)
: -- normal (vote)
: mozilla17
Assigned To: Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
:
Mentors:
: 750975 (view as bug list)
Depends on: 745136 774139 808031
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-01 17:58 PDT by Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
Modified: 2012-11-02 10:05 PDT (History)
14 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
+


Attachments
Diagram of the various pieces will fit together (53.51 KB, image/png)
2012-07-15 15:56 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
no flags Details
WIP (86.50 KB, patch)
2012-07-16 02:40 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
21: feedback+
Details | Diff | Review
Fix firstpaint (648 bytes, patch)
2012-07-18 00:29 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
no flags Details | Diff | Review
Glue async pan/zoom logic up between compositing, event dispatch, and repaint requests (55.31 KB, patch)
2012-07-19 02:15 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
no flags Details | Diff | Review
Add nsIDocShell.asyncPanZoomEnabled (2.53 KB, patch)
2012-07-19 02:15 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
justin.lebar+bug: review+
Details | Diff | Review
Have BrowserElementChild service repaint requests and handle fallback synchronous scrolling (for now) (10.87 KB, patch)
2012-07-19 02:16 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
21: review-
Details | Diff | Review
Have BrowserElementChild service repaint requests and handle fallback synchronous scrolling (for now), v2 (10.86 KB, patch)
2012-07-19 10:15 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
justin.lebar+bug: review+
21: review+
Details | Diff | Review
Glue async pan/zoom logic up between compositing, event dispatch, and repaint requests, rebased (56.38 KB, patch)
2012-07-19 19:33 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
no flags Details | Diff | Review
Glue async pan/zoom logic up between compositing, event dispatch, and repaint requests, v2 (56.79 KB, patch)
2012-07-19 20:02 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
roc: review+
Details | Diff | Review

Description Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-05-01 17:58:29 PDT
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.)
Comment 1 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-05-01 18:01:04 PDT
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.
Comment 2 Tony Chung [:tchung] 2012-06-21 20:30:17 PDT
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.
Comment 3 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-12 16:09:10 PDT
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.
Comment 4 Justin Lebar (not reading bugmail) 2012-07-13 08:49:23 PDT
> 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.
Comment 5 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-13 23:57:36 PDT
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.
Comment 6 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-15 00:28:58 PDT
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.
Comment 7 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-15 15:56:57 PDT
Created attachment 642443 [details]
Diagram of the various pieces will fit together

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.
Comment 8 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-15 15:57:45 PDT
See above --- speak now or forever hold your peace! :)

Or r-, I guess ;).
Comment 9 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-15 17:18:09 PDT
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.
Comment 10 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-16 02:40:47 PDT
Created attachment 642511 [details] [diff] [review]
WIP

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.
Comment 11 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2012-07-16 08:10:36 PDT
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.
Comment 12 Justin Lebar (not reading bugmail) 2012-07-16 15:12:10 PDT
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?
Comment 13 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-16 15:16:42 PDT
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?
Comment 14 Justin Lebar (not reading bugmail) 2012-07-16 15:59:58 PDT
(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
Comment 15 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-16 16:14:57 PDT
sgtm
Comment 16 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-18 00:29:28 PDT
Created attachment 643280 [details] [diff] [review]
Fix firstpaint

It needs to go away, but at least this makes it work without frontend involvement.
Comment 17 Kartikaya Gupta (email:kats@mozilla.com) 2012-07-18 06:29:17 PDT
Won't this break fennec? (Also related bug: https://bugzilla.mozilla.org/show_bug.cgi?id=735029)
Comment 18 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-18 11:45:03 PDT
No clue.  Apparently there are multiple interpretations of "first paint", which probably needs to change.
Comment 19 Kartikaya Gupta (email:kats@mozilla.com) 2012-07-18 12:15:18 PDT
"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.
Comment 20 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-18 12:49:18 PDT
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.
Comment 21 Kartikaya Gupta (email:kats@mozilla.com) 2012-07-18 13:04:55 PDT
(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.
Comment 22 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-07-18 19:53:14 PDT
(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.
Comment 23 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-18 20:30:16 PDT
(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.
Comment 24 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-19 02:15:20 PDT
Created attachment 643771 [details] [diff] [review]
Glue async pan/zoom logic up between compositing, event dispatch, and repaint requests

This is a pretty big chunk of patch, but pleasantly straightforward IMHO.  Hope you agree :).
Comment 25 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-19 02:15:44 PDT
Created attachment 643772 [details] [diff] [review]
Add nsIDocShell.asyncPanZoomEnabled
Comment 26 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-19 02:16:14 PDT
Created attachment 643773 [details] [diff] [review]
Have BrowserElementChild service repaint requests and handle fallback synchronous scrolling (for now)
Comment 27 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-19 02:16:41 PDT
Note: I'm going to fold all these patches together for landing.  I didn't compile them separately, and they're highly interdependent.
Comment 28 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-19 02:17:35 PDT
Also, the first patch will change a bit after review iteration in bug 750974, but only trivially.
Comment 29 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-19 02:45:36 PDT
*** Bug 750975 has been marked as a duplicate of this bug. ***
Comment 30 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2012-07-19 05:47:50 PDT
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.
Comment 31 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-19 10:14:24 PDT
(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.
Comment 32 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-19 10:15:27 PDT
Created attachment 643905 [details] [diff] [review]
Have BrowserElementChild service repaint requests and handle fallback synchronous scrolling (for now), v2
Comment 33 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2012-07-19 10:19:54 PDT
(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.
Comment 34 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-19 10:25:34 PDT
(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 35 Justin Lebar (not reading bugmail) 2012-07-19 13:16:26 PDT
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.)
Comment 36 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-19 13:34:11 PDT
(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.).
Comment 37 Justin Lebar (not reading bugmail) 2012-07-19 14:09:23 PDT
> 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?
Comment 38 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-19 14:32:51 PDT
(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 39 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-19 19:33:46 PDT
Created attachment 644142 [details] [diff] [review]
Glue async pan/zoom logic up between compositing, event dispatch, and repaint requests, rebased
Comment 40 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-07-19 19:47:15 PDT
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
Comment 41 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-19 20:01:37 PDT
(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.
Comment 42 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-19 20:02:10 PDT
Created attachment 644151 [details] [diff] [review]
Glue async pan/zoom logic up between compositing, event dispatch, and repaint requests, v2
Comment 43 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-19 23:50:13 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/19b28e14df61
Comment 44 Ed Morley [:emorley] 2012-07-20 06:41:33 PDT
https://hg.mozilla.org/mozilla-central/rev/19b28e14df61

Note You need to log in before you can comment on or make changes to this bug.