The goal of this API is to provide instant response to user pans, while awaiting the new pixels from an asynchronous re-paint request. The difference between this API and CSS transforms is that the platform can automatically, atomically adjust its transform matrix when new content pixels arrive asynchronously. It might be possible to implement the same scheme with CSS transforms and an event fired upon new pixel data, but a direct <browser> interface seems simpler for now. The sequence of events for a pan might be something like - pan action detected - compute region to be repainted based on estimated pan target - send async re-paint request (scrollBy) to content process - start animating "old" content pixels with browser.viewportScrollBy/To() API. viewportScroll* sets a translation matrix for the old pixels based on its document top/left. (the next two events could happen in either order) - "new" content pixels arrive. The translation matrix determined by viewportScroll* changes based on new document top/left. Animation continues with new pixels. - animation completes. Attached is the proposed IDL. I don't know where this interface should live; nsIDOMXULElement doesn't feel right. But I'll go forward with this impl unless anyone has better ideas. (Where's bz when one needs him!?!?!) (Spamming feedback?. No need for everyone to reply if someone signs off quickly.)

As stated on IRC, we probably want this instead: http://mxr.mozilla.org/mozilla-central/source/content/base/public/nsIFrameLoader.idl

OK. Still doesn't quite feel right, but I doubt anything will ;).

In a little more detail from the platform side of things, here's what we'll be implementing with this bug - extra layer tree attributes to describe the document metrics when the tree was last updated. The metrics will probably be scrollX+scrollY and some kind of scale factor value (something app-unit-per-css-pixel-ish?). - shadow-layer transactions forwarding the metrics attributes, so that the update is atomic wrt shadow layer pixels being updated. IOW, the shadow layer tree and its document metrics will always be consistent in the browser process. This API proposal takes advantage of that consistency to automatically set a "compensation transform" on behalf of the frontend. The viewport* attributes create a sort of goal-state metrics G. When we have old pixels drawn with document metrics M, the remote frame code will transform the pixels from M to G. When the new pixels come in with metrics M', the remote frame code will transform the pixels from M' to G. (Commonly M' and G will be the same, but might be different if the new pixels come in while the frontend is still animating the pan/zoom.) Does this make sense?

Comment on attachment 468800 [details] [diff] [review] Proposed <browser> API I don't think any of these should live on nsIDOMXULElement... see comments below. Zooming: This should probably live on the browser/iframe/whatever-object-we-use-now. A way to set the zooming like viewportZoom should be ok. We can animate that on the animation frame timer and do the right thing there. I'm still a bit unsure about exactly how the fuzzy zoom/pixels swap from one process to the other will be implemented, but it should work if done right. Scrolling: For a given point, we need an API to return an object that has the scroll APIs on it. Be it the outer browser, an iframe, or a div. Even if it just always returns the outer browser for the time being until the layer tree knows what areas are scrollable, that object should be the thing that you scroll. Something similar to nsIDOMWindowUtils::ElementFromPoint? What coordinate space are these going to be in (as it relates to the scale?) Once we have the right object, scrollTo/scrollBy should be enough to drop in to our kinetic scrolling code. It might be nice to have scrolling actually do the scrolls in the content process as well to keep things in sync with when we hit an edge of the page. We'll most likely end up spamming scrollBy from our kinetic code.

> It might be nice to have scrolling actually do the > scrolls in the content process as well to keep things in sync with when we hit > an edge of the page. We'll most likely end up spamming scrollBy from our > kinetic code. From what Chris is proposing, the viewport position and the document position are completely independent from the vantage point of the platform. It is up to the frontend to keep them in sync. Fennec works this way now, but I do think keeping the browser and viewport in step is a task platform is supposed to do. So for now, when we are animating, frontend will be telling content to scroll/zoom instead of platform. If a browser scrolls on its own (for instance, you click a hyperlink) Fennec will be responsible for telling viewport. Re: when we hit the edge of the page, this is a very good point. Hopefully the parent process will know if we asked it to scrollby too far? Fennec could also keep track of this by itself (currently it does this in mobile2).

(In reply to comment #5) > From what Chris is proposing, the viewport position and the document position > are completely independent from the vantage point of the platform. It is up to > the frontend to keep them in sync. Fennec works this way now, but I do think > keeping the browser and viewport in step is a task platform is supposed to do. One advantage of having Fennec keep them in sync is that that puts control over triggering child process re-rendering in Fennec JS instead of instead of platform code. This all looks good to me.

Posting these patches for reference in further discussion. Two more patches are needed: first is to set the "frame metrics" during layer-tree transactions. I'll bug roc after (my) dinner to find out what we need here. The second is to compute the compensation transform and set it on the shadow layer root in layout/ipc/RemoteFrameParent.cpp. This should be relatively easy, modulo coordinate space gunk.

This patch shows how parts 1-4 fit together. Clicking the button in the top-right of the window kicks off a 3-second animation that sets the viewport scroll offsets every 30ms. The test artifically waits until the animation is halfway over before doing the content.scrollBy(). If all goes well, the animation should stay smooth, flip suddenly with new pixels halfway in, then stay smooth with the new pixels until it finishes.

I had hoped the |scale| API could be confined to gfx/layers code. For scale>1.0, (I think) it could be. However, for scale<1.0, gfx/layers needs layout/ to compute a display list that takes into account the overflow areas of the document "pulled in" by the scale, that wouldn't otherwise have been visible. We could go forward with this known-broken scale approach; it might be enough for b1. But, it would be easy to construct cases in which we'd get rendering glitches with scale<1.0. Luckily, the impl of CSS transforms appears to do just what we want. The attached test program sets a CSS transform on content.document.body (yeah, we don't want to do that); awesomely cooly, the scale transform pulls in overflow and re-rasterizes fonts at the scaled "resolution". I think we're very close to a |scale| API (it'll probably be |scaleX| and |scaleY|, BTW, but that's not terribly important atm). What I'm missing is a "thing" outside of content.document on which to set a scale attribute that triggers CSS-transform-like behavior. This "thing" needs to (1) be accessible to content-browser script (2) participate in or affect layout of content.document (3) have its |scale| attribute be accessible by something that can touch the document's root ContainerLayer If such a thing exists already, please let me know. If not, I'd like guidance on how to create it; I can work out the goop. (With the content-process side of things resolved, there are still some remaining chrome-process issues with drawing the shadow layer tree correctly, but I know how to work these out.) Help?

> (1) be accessible to content-browser script Not sure what this means. Do we just mean script running in the content process, or untrusted script, or something else? > (3) have its |scale| attribute be accessible by something that can touch the document's root ContainerLayer Again, not sure what this means in practice. Can presshell touch the ContainerLayer?

(In reply to comment #16) > > (1) be accessible to content-browser script > > Not sure what this means. Do we just mean script running in the content > process, or untrusted script, or something else? > Yeah, terminology for this is hard. I meant "frontend, chrome-privileged script running in the content process". > > (3) have its |scale| attribute be accessible by something that can touch the > document's root ContainerLayer > > Again, not sure what this means in practice. Can presshell touch the > ContainerLayer? Indirectly, yes. Ideally the "thing" needs would be accessible from nsDisplayList::PaintForFrame or thereabouts.

(In reply to comment #17) > (In reply to comment #16) > > > (3) have its |scale| attribute be accessible by something that can touch the > > document's root ContainerLayer > > > > Again, not sure what this means in practice. Can presshell touch the > > ContainerLayer? > > Indirectly, yes. Ideally the "thing" needs would be accessible from > nsDisplayList::PaintForFrame or thereabouts. IOW, ideally, I'd like to do something similar for |scale| as part 2 above does for |scrollLeft/Top|. It's fine if the "thing" propagates its |scale| to something else that I can access and associate with the root layer, when we paint the layer tree.

I tried the patches with the qt port. There seems to be a problem in placing the layer after scrolling. It is scrolled, but it is not positioned in the top-left corner, but on its old position. the scrolled area is not rendered anymore (which is basically correct, - so just the placement is off) Also zooming is not working (through double-click / pinch) - in qt port.

> Also zooming is not working (through double-click / pinch) - in qt port. Jeremias you should use test-ipcbrowser.xul, or we should fix fennec to use new viewport API.

I tried to load attached page with test-ipcbrowser.xul, and new viewport scroll implementation also moving footer frame (which should stay on the same position while scrolling). Is it minor or major problem of current implementation?

Chris, sounds like one possible approach is to just add a way to set the scale via nsIDOMWindowUtils. This can then set it on the nsIDocument or whatever, and the layout code can read it from there. Sound reasonable?

Just FTR, "wiewport" scaling isn't implemented yet, not even in test-ipcbrowser.xul. (In reply to comment #21) > Created attachment 469451 [details] > footer scrolling test > > I tried to load attached page with test-ipcbrowser.xul, and new viewport scroll > implementation also moving footer frame (which should stay on the same position > while scrolling). > Is it minor or major problem of current implementation? I don't know what you mean by "stay in same position" --- not move on the screen while scrolling? This is a design decision; we decide what the desired behavior is (and this decision can change after b1!). What's happening in test-ipcbrowser is that the CSS viewport is exactly the size of the <browser> element. When the animated scroll sets browser.viewportScrollTop, we translate all old pixels on the screen, position:fixed "footer" included. When the "new pixels" come in after the content process sets document.scrollTop, the position:fixed "footer" is positioned at the new scrollTop, but if the viewportScrollTop is still being animated, that is, scrollTop != viewportScrollTop, then the "footer" is translated along with all the other new pixels. In fennec 2.0a1, the "footer" is always positioned relative to other document pixels (that is, position:fixed is "broken"), because fennec 2.0a1 doesn't change document.scrollTop. Mobilesafari behaves the same way. MicroB appears to do something in between: the "footer" disappears when panning (immediately when the pan begins), then reappears later. If we want to preserve the fennec 2.0a1/mobilesafari behavior, we need to use a different content document "scrolling" mechanism than scrollLeft/scrollTop. (I think a CSS translation would do what we want ... hmmm.) In the long term, it would be possible to make position:fixed frames always do the right thing, even with viewportScroll*-type translations, but we don't have enough information in the layer tree for that yet. We're not going to block b1 on that. So, the question for b1 is: continue to use scrollTop and get odd behavior in cases like this, or use a CSS-translate-like mechanism instead of scrollTop?

I think we probably want CSS-translate-style behavior for b1. That might fold with scaling nicely, assuming we can find the right place to interpose that in the content process.

Found another issue to ponder, that we may or may not be able to solve with available mechanisms for b1. Navigate to nytimes.com in fennec 2.0a1 (or MicroB). There's a "Welcome to TimesPeople" strip along the top of the page that appears to be sizing itself based on window.innerWidth (can't easily find the code that creates the strip though :/). However, the strip is rendered narrower than the content's visible width (viewport? however that's implemented). The attached screenshot is from test-ipcbrowser with a 0.5 scale-transform applied, but the effect is approximately the same as in fennec proper. So it appears to me that in the content process, the CSS viewport needs to be what the frontend or web content chooses for <meta viewport>, and window.innerWidth/Height needs to report that same viewport. Does that sound right?

On second glance, it appears that nytimes.com is setting its main content to a fixed size of 972px. Is fennec applying some heuristics to make draw nytimes so that it fills the screen? I wonder if that's wise, if it's going to lead to discordance with content-visible window metrics, and since nytimes can fix its CSS easily enough.

A goal has (I think) emerged through various discussions. (1) decouple the CSS viewport ("layout viewport" in terms of http://quirksmode.org/mobile/viewports2.html) from widgetry. Add platform API to allow the viewport to be set arbitrarily, to implement <meta viewport>. (2) create a notion of what I'll call a "displayport", which is completely separate from layout ("display" is chosen to bring display lists to mind). The displayport represents a projection of a document from the viewport's CSS-pixel space to device-pixel space. Changing the displayport does not affect layout whatsoever; it never triggers reflow. We'll use the displayport to implement scaling-zoom for b1. (3) add APIs for rendering a displayport rect to be drawn to screen. These pixels will be drawn in the <browser> in the chrome process. (There's a (4), but it's a layers-level implementation detail to be fixed in another bug.) Current plan of attack for (1) - allow the frontend to override nsPresContent::GetVisibleArea for (2) - I don't entirely know yet. We'll need a DOM-y/*Shell-y thing on which to define the displayport projection (just scale or scaleX/Y for b1). When painting, I think we'll need to have something derived from the DOM-y/*Shell-y thing inject display items to implement the displayport projection. for (3) - don't entirely know either. Rendering will need to go through the WIDGET_LAYERS path for perf; we'll need to set up dummy widgets et al. to ensure that that happens. When we render the displayport, we'll stamp the root layer with the displayport projection and probably the viewport-space top/left that was rendered. We'll use that stamp like the patches above do to allow fast, properly synchronized panning/scaling of shadow layers. How does this sound?

I don't know how scrollbars or other scroll indicators would work in this regime. I suspect we'll need chrome to draw them over the <browser remote> in the browser process, since we'd probably like them to update while we're doing the panning animation of shadow layers.

I don't think these patches are going to make the final cut, so no need to waste reviewer time.

(In reply to comment #27) > (2) create a notion of what I'll call a "displayport", which is completely > separate from layout ("display" is chosen to bring display lists to mind). The > displayport represents a projection of a document from the viewport's CSS-pixel > space to device-pixel space. Changing the displayport does not affect layout > whatsoever; it never triggers reflow. We'll use the displayport to implement > scaling-zoom for b1. I wonder if what's described here is close to what PresContext is supposed to be. If so, I can add what we need to PresContext.

nsPresContext::mVisibleArea is the size of the layout viewport. (Which I think would be better called CSS viewport, since it's the only viewport CSS knows about.) You can set that viewport size by sizing your puppet widget. As far as I can tell, all you need here is an extra API to let you force layout to build a display list/layer tree for an area that's larger than the CSS viewport. (Doing it for an area that's smaller than the CSS viewport isn't all that useful and might confuse things, so let's not.) That could be a property on nsPresContext that gets consulted by nsLayoutUtils::PaintFrame where it sets visibleRegion.

And that property could be set by a DOM API in the content process or by any other mechanism. I think that's pretty close to what you meant by displayport, but I'm not 100% sure. I don't think we need a different API to trigger rendering to this larger area.

(In reply to comment #31) > (Doing it for an area that's smaller than the CSS viewport isn't all > that useful and might confuse things, so let's not.) I suppose for performance you want to do that. So OK, let's allow that.

(In reply to comment #31) > As far as I can tell, all you need here is an extra API to let you force layout > to build a display list/layer tree for an area that's larger than the CSS > viewport. (Doing it for an area that's smaller than the CSS viewport isn't all > that useful and might confuse things, so let's not.) That could be a property > on nsPresContext that gets consulted by nsLayoutUtils::PaintFrame where it sets > visibleRegion. That sounds about right. Do you have any recommendations for how that API should work with with scaled drawing? I think we could use the scale information to set some kind of layer-manager "resolution" property like you've proposed, that ThebesLayers (and maybe later Image*) would consult when repainting. Not sure if there's a more elegant solution using display list hackery (nsDisplayTransform?) or something.

I think a separate nsIPresShell API should set the resolution.

Does it make sense for fennec to allow the user to pan into view pixels outside the viewport's top/left boundaries? That is, if window.scrollX/Y = <100, 100>, would it make sense to allow panning to viewport <-50, -50>, which would show pixels left of scrollX and above scrollY? (I'm thinking "no".) I'll poke around to see what other browsers do.

(In reply to comment #38) > I'll poke around to see what other browsers do. Does mobile safari allow you to "pull" beyond the pan limits, and then snap back? We have talked about doing that. I noticed that the Android browser does not do this.

Our APIs should support that. Whether the Fennec UI wants to do that is up to someone else. If you want the kinetic "bounce" when you hit the scroll limit, you might want to do that.

(In reply to comment #38) > Does it make sense for fennec to allow the user to pan into view pixels outside > the viewport's top/left boundaries? That is, if window.scrollX/Y = <100, 100>, > would it make sense to allow panning to viewport <-50, -50>, which would show > pixels left of scrollX and above scrollY? (I'm thinking "no".) > On second thought, this makes total sense. I think this question may have been unclear --- I didn't mean "allow requests to draw pixels outside the document bounds", I meant "allow requests to draw document pixels left/top of what's currently scrolled into the CSS viewport." (Duh! /me needs more coffee.) (In reply to comment #39) > (In reply to comment #38) > > > I'll poke around to see what other browsers do. > > Does mobile safari allow you to "pull" beyond the pan limits, and then snap > back? We have talked about doing that. I noticed that the Android browser does > not do this. Yes. I kind of like that effect. We can allow requests for pixels outside document bounds, no problem. Those pixels will just not get painted.

(In reply to comment #41) > On second thought, this makes total sense. > > I think this question may have been unclear --- I didn't mean "allow requests > to draw pixels outside the document bounds", I meant "allow requests to draw > document pixels left/top of what's currently scrolled into the CSS viewport." > (Duh! /me needs more coffee.) That should be allowed too. > We can allow requests for pixels outside document bounds, no problem. Those > pixels will just not get painted. Arguably, we should paint the viewport background there. (The background of the root element, or the HTML <body> element, is propagated to the viewport.) So a repeating image tile, or a color, could be painted there. This would require some changes to nsDisplayCanvasBackground though. And it may not be important.

For scrolling/panning, the API I want to add is setDisplayport(x, y, w, h), with all quantities in CSS pixels in "document space". (Completely distinct from the CSS viewport). The effect would be to treat the rect <x, y, w, h> as the "visible rect" for the purposes of invalidation and painting. (Instead of computing <w, h> from widget bounds, and ~using window scrollX/scrollY as <x, y>.) The effect will be something like canvas.drawWindow(x, y, w, h), except with <x, y, w, h> persisting, re-painted pixels automatically being propagated up to the chrome process, and all our layer retaining machinery making canvas/video/scrolling fast. But there's a problem. Stuart/Mark/Ben/Matt: how well does fennec need to play with pages that call window.scrollBy/To from script, for b1? setViewport() is *not* going to work well with that. I think the effect will be to completely ignore scrollBy/To, but I'm not 100% sure yet. The other possibility is weird translation effects. Maybe the frontend can deal with scrollTo/By? At any rate, my inclination is to not care about pages that call scrollBy/To, for b1. (Are there important sites that do that?) I don't believe setDisplayPort() would treat position:fixed elements differently than a1 does. setDisplayPort() would support scale-zooming through a separate |resolution| attribute that the frontend would set. So to scale by a factor of 2.0, you would setDisplayPort(x, y, prevW/2, prevH/2), and set resolution=2.0. (I'm not worried about what clientWidth/Height and innerWidth/Height values we hand out to script, because those should be easy to twiddle.) Sound OK, modulo concerns about scrollTo/By?

Allows setting displayport, but doesn't stamp layers with the displayport. This is so easy it's ridiculous! Essentially all we need for panning with a prefetch region is here. Posting for demonstration purposes.

(In reply to comment #43) > For scrolling/panning, the API I want to add is setDisplayport(x, y, w, h), > with all quantities in CSS pixels in "document space". (Completely distinct > from the CSS viewport). The effect would be to treat the rect <x, y, w, h> as > the "visible rect" for the purposes of invalidation and painting. (Instead of > computing <w, h> from widget bounds, and ~using window scrollX/scrollY as <x, > y>.) The effect will be something like canvas.drawWindow(x, y, w, h), except > with <x, y, w, h> persisting, re-painted pixels automatically being propagated > up to the chrome process, and all our layer retaining machinery making > canvas/video/scrolling fast. But there's a problem. So for setDisplayPort, (0,0) means the top-left of the CSS viewport? Or does it compensate for scrolling like drawWindow does, so that when the document is scrolled down by 10px, (0,0) means the top-left of the document while (10,0) is the top-left of the CSS viewport? I think you mean the latter but I'm not sure. > Stuart/Mark/Ben/Matt: how well does fennec need to play with pages that call > window.scrollBy/To from script, for b1? setViewport() is *not* going to work > well with that. You're having setViewport set the scroll offset? I had imagined it would only set the size.

If we can agree on the setDisplayport() API, next up is to - stamp content-process layers with the displayport they were rendered with, so the chrome process can translate them properly - allow setting a resolution on layer managers, paint ThebesLayers with that resolution, render other layer types to the transaction target with a scale=resolution, and stamp content-process layers with that resolution - render shadow layers with the right resolution (not exactly sure how to do this yet) - add a chrome-process API for setting a "goal" displayport+resolution for shadow layers, and figure out how to set up the compensating transform (will need to be per layer type)

(In reply to comment #46) > (In reply to comment #43) > > For scrolling/panning, the API I want to add is setDisplayport(x, y, w, h), > > with all quantities in CSS pixels in "document space". (Completely distinct > > from the CSS viewport). The effect would be to treat the rect <x, y, w, h> as > > the "visible rect" for the purposes of invalidation and painting. (Instead of > > computing <w, h> from widget bounds, and ~using window scrollX/scrollY as <x, > > y>.) The effect will be something like canvas.drawWindow(x, y, w, h), except > > with <x, y, w, h> persisting, re-painted pixels automatically being propagated > > up to the chrome process, and all our layer retaining machinery making > > canvas/video/scrolling fast. But there's a problem. > > So for setDisplayPort, (0,0) means the top-left of the CSS viewport? Or does it > compensate for scrolling like drawWindow does, so that when the document is > scrolled down by 10px, (0,0) means the top-left of the document while (10,0) is > the top-left of the CSS viewport? I think you mean the latter but I'm not sure. > Well neither really, but closer to the latter. I'm proposing that displayport be the only thing that does "scrolling", so displayport (0,0) is always document (0,0), and the CSS viewport isn't involved. It's not clear to me how displayport should interact with pages scrolling themselves. Could use some input, but too late my time for me to think more about it. Will resume tomorrow. > > Stuart/Mark/Ben/Matt: how well does fennec need to play with pages that call > > window.scrollBy/To from script, for b1? setViewport() is *not* going to work > > well with that. > > You're having setViewport set the scroll offset? I had imagined it would only > set the size. Gah sorry, I meant setDisplayport :/. Yes, setCSSViewport would only affect reflow.

(In reply to comment #48) > Well neither really, but closer to the latter. I'm proposing that displayport > be the only thing that does "scrolling", so displayport (0,0) is always > document (0,0), and the CSS viewport isn't involved. I don't think we can really do that. In fact, I don't understand how that would work. Because scrolling isn't just a translation of all elements on the page, adjusting the displayport doesn't produce the same effect as scrolling. (In reply to comment #47) > - allow setting a resolution on layer managers, paint ThebesLayers with that > resolution, render other layer types to the transaction target with a > scale=resolution, and stamp content-process layers with that resolution I would like the resolution API to not require changes to anything except ThebesLayers and temporary surfaces used for containers. In my mind, all it should do is cause buffers to be used at higher resolution so that if you apply a transform in some ancestor layer, you'll get better results for the ThebesLayers. E.g for ThebesLayers with a resolution of 2.0 we'd allocate buffers twice the size in each direction, scale by 2 when drawing into them, and scale by 0.5 when drawing out of them. Here's something I could understand: -- setCSSViewport(w,h) sets the size of the viewport. w/h are in CSS pixels. -- setDisplayPort(x,w,h,y) sets a rect to render (build a layer tree for) in CSS pixels, where 0,0 is the top-left of the CSS viewport -- setResolution(h,v) sets the horizontal and vertical resolution. Buffers will be sized to provide optimal results if the layer tree is scaled by h,v when drawing. -- no new scrolling APIs. Do scrollTo/scrollBy in the content process. -- the layer tree is generated so that the root layer's coordinates are in device pixels with 0,0 being the top-left of the CSS viewport, as now.

-- each generated layer tree is stamped with the CSS viewport, the displayport, and the viewport scroll offset. There should be no need to stamp with the resolution. Just apply a scale transform to the layer tree in the compositor to scale it to the desired size. If that scale matches the resolution, you'll get ideal quality, otherwise you'll get the best approximation.

(In reply to comment #49) > (In reply to comment #48) > > Well neither really, but closer to the latter. I'm proposing that displayport > > be the only thing that does "scrolling", so displayport (0,0) is always > > document (0,0), and the CSS viewport isn't involved. > > I don't think we can really do that. In fact, I don't understand how that would > work. Because scrolling isn't just a translation of all elements on the page, > adjusting the displayport doesn't produce the same effect as scrolling. > Yes, I agree. But the only difference I know of so far is position:fixed elements. Are we going to hit other reasonably-common cases that a displayport-type "scrolling" impl will fall over on? (Thinking of what we need for b1 here.) > (In reply to comment #47) > > - allow setting a resolution on layer managers, paint ThebesLayers with that > > resolution, render other layer types to the transaction target with a > > scale=resolution, and stamp content-process layers with that resolution > > I would like the resolution API to not require changes to anything except > ThebesLayers and temporary surfaces used for containers. In my mind, all it > should do is cause buffers to be used at higher resolution so that if you apply > a transform in some ancestor layer, you'll get better results for the > ThebesLayers. E.g for ThebesLayers with a resolution of 2.0 we'd allocate > buffers twice the size in each direction, scale by 2 when drawing into them, > and scale by 0.5 when drawing out of them. > OK, cool. That sounds simpler and more direct than what I had in mind. > Here's something I could understand: > -- setCSSViewport(w,h) sets the size of the viewport. w/h are in CSS pixels. > -- setDisplayPort(x,w,h,y) sets a rect to render (build a layer tree for) in > CSS pixels, where 0,0 is the top-left of the CSS viewport > -- setResolution(h,v) sets the horizontal and vertical resolution. Buffers will > be sized to provide optimal results if the layer tree is scaled by h,v when > drawing. > -- no new scrolling APIs. Do scrollTo/scrollBy in the content process. > -- the layer tree is generated so that the root layer's coordinates are in > device pixels with 0,0 being the top-left of the CSS viewport, as now. I agree with this except that I don't understand "no new scrolling APIs. Do scrollTo/scrollBy". I didn't think I proposed a new scrolling API; are you suggesting using scrollTo/By to implement b1 panning? If that's what you mean, I'm not sure I agree (yet! for b1). Since layer trees don't understand position:fixed (yet!), translating their shadows in the chrome process before newly-scrolled pixels arrive will cause position:fixed elements to jump around in odd and observably non-standard ways. I'm not sure what we would win by doing this, especially since competitors seem to mostly ignore position:fixed for panning (MicroB excepted, but we don't want to copy their implementation). If scrollTo/By is off the table, then the CSS viewport top-left would always be (0,0), and I think we would be on the same page.

(In reply to comment #43) > Maybe the frontend can deal with scrollTo/By? At any rate, my inclination is > to not care about pages that call scrollBy/To, for b1. (Are there important > sites that do that?) Yes, a number of mobile sites use scrollTo/By, for example http://www.google.com/reader/i How about in-page links (e.g. href="#top") - will those be affected too?

(In reply to comment #43) > For scrolling/panning, the API I want to add is setDisplayport(x, y, w, h), > with all quantities in CSS pixels in "document space". (Completely distinct > from the CSS viewport). The effect would be to treat the rect <x, y, w, h> as > the "visible rect" for the purposes of invalidation and painting. (Instead of > computing <w, h> from widget bounds, and ~using window scrollX/scrollY as <x, > y>.) The effect will be something like canvas.drawWindow(x, y, w, h), except > with <x, y, w, h> persisting, re-painted pixels automatically being propagated > up to the chrome process, and all our layer retaining machinery making > canvas/video/scrolling fast. But there's a problem. Yes, perfect! And this is as opposed to viewportScrollX and Y? > Stuart/Mark/Ben/Matt: how well does fennec need to play with pages that call > window.scrollBy/To from script, for b1? setViewport() is *not* going to work > well with that. I think the effect will be to completely ignore scrollBy/To, > but I'm not 100% sure yet. The other possibility is weird translation effects. > Maybe the frontend can deal with scrollTo/By? At any rate, my inclination is > to not care about pages that call scrollBy/To, for b1. (Are there important > sites that do that?) Like I said above, it sounded like Fennec would be in charge of syncing the CSS viewport and the projection viewport (ack I keep switching terms, we need to settle on something). I was imagining that Fennec would update the projection viewport if the position of the CSS viewport changed (by watching "scroll" events, just like we do now!). This isn't just important for scripts, but for when the user taps on a hashtag link that navigates to a different part of the page. I think it's absolutely fine if platform doesn't take care of CSS viewport position change => projection viewport position syncing for b1, but I do think that the responsibility ultimately lies in the platform.

Also, I'm a little worried now about specifying the projection viewport in CSS pixels, especially since these values are longs and not doubles. How do I scroll the viewport by one pixel if viewport is 3x the size of the document?

Oh, I see from the test that you are still using scrollViewportBy even though you marked those patches as obsolete. I am missing something...

OK, let's do what drawWindow() does. New spec is what roc proposed -- setDisplayPort(x,w,h,y) sets a rect to render (build a layer tree for) in CSS pixels, where 0,0 is the top-left of the CSS viewport The frontend could use scrollTo/By to move pixels onto screen, or it could change the displayport, or some combination of the two depending on the situation. (In reply to comment #53) > (In reply to comment #43) > > For scrolling/panning, the API I want to add is setDisplayport(x, y, w, h), > > with all quantities in CSS pixels in "document space". (Completely distinct > > from the CSS viewport). The effect would be to treat the rect <x, y, w, h> as > > the "visible rect" for the purposes of invalidation and painting. (Instead of > > computing <w, h> from widget bounds, and ~using window scrollX/scrollY as <x, > > y>.) The effect will be something like canvas.drawWindow(x, y, w, h), except > > with <x, y, w, h> persisting, re-painted pixels automatically being propagated > > up to the chrome process, and all our layer retaining machinery making > > canvas/video/scrolling fast. But there's a problem. > > Yes, perfect! And this is as opposed to viewportScrollX and Y? > Yes, they're two peas in a pod. The displayport API is entirely in the content process. In the chrome process, we'll have a complementary <browser> API, like viewportScrollX/Y, for setting up shadow-layer translations. And when we have resolution support, we'll add something like a viewportScale API that the chrome process can use. (Not sure if "viewport" is the right name, but details details.) > > Stuart/Mark/Ben/Matt: how well does fennec need to play with pages that call > > window.scrollBy/To from script, for b1? setViewport() is *not* going to work > > well with that. I think the effect will be to completely ignore scrollBy/To, > > but I'm not 100% sure yet. The other possibility is weird translation effects. > > Maybe the frontend can deal with scrollTo/By? At any rate, my inclination is > > to not care about pages that call scrollBy/To, for b1. (Are there important > > sites that do that?) > > Like I said above, it sounded like Fennec would be in charge of syncing the CSS > viewport and the projection viewport (ack I keep switching terms, we need to > settle on something). I was imagining that Fennec would update the projection > viewport if the position of the CSS viewport changed (by watching "scroll" > events, just like we do now!). > > I think it's absolutely fine if platform doesn't take care of CSS viewport > position change => projection viewport position syncing for b1, but I do think > that the responsibility ultimately lies in the platform. With roc's setDisplayport() proposal, the displayport coordinates are relative to the CSS viewport, so they would always be in sync by definition. (In reply to comment #54) > Also, I'm a little worried now about specifying the projection viewport in CSS > pixels, especially since these values are longs and not doubles. How do I > scroll the viewport by one pixel if viewport is 3x the size of the document? That's a good point. Will fix. (In reply to comment #55) > Oh, I see from the test that you are still using scrollViewportBy even though > you marked those patches as obsolete. I am missing something... Nope, you're right, it's broken. I just left the old JS code in because I didn't want to rewrite the animation gunk when the new API is ready.

Did some thinking last night. If fennec b1 dropped support for <meta viewport width/height>, would anyone notice?

Yes. It's a pretty core visible feature of Fennec that's been there since the beginning (especially zoom to fit).

(In reply to comment #58) > Yes. It's a pretty core visible feature of Fennec that's been there since the > beginning (especially zoom to fit). To be clear, I'm talking about the <meta viewport> options that allow pages to force re-layout at a specified width/height (not sure height is meaningful). Fennec already ignores this if the width is smaller than the "widget", right? Scale-zoom to fit is a UA feature, right? Not specified by <meta viewport>. That's a separate issue.

> To be clear, I'm talking about the <meta viewport> options that allow pages to > force re-layout at a specified width/height (not sure height is meaningful). > Fennec already ignores this if the width is smaller than the "widget", right? I don't think height is meaningful, but not absolutely certain. Width smaller than widget is a huge usecase, actually, for instance, 320px. The CSS viewport will have a width of 320px, and then we'll zoom in to fit that on the screen. > Scale-zoom to fit is a UA feature, right? Not specified by <meta viewport>. > That's a separate issue. In landscape mode, we still tell content that's its CSS viewport width is 800px.

(In reply to comment #57) > Did some thinking last night. > > If fennec b1 dropped support for <meta viewport width/height>, would anyone > notice? Let's pretend like I never said this ;). Moving on.

The biggest thing I'm unsure about is using IGNORE_VIEWPORT_SCROLLPORT_SCROLLING but not flushing the layers (since that would kill perf for fennec). AFAICT this should be pretty much the same as normal widget-layers rendering; seems to work OK in simple cases (I know; "ha! ha!") locally.

Crap, this patch was supposed to come before the displayport ones :/. I can repost if preferable. The latest versions are in my at http://hg.mozilla.org/users/cjones_mozilla.com/mcmq.

Update: this "rollup" patch has all the big pieces we need for fennec b1. There are several little lawyer-y bugs I've found so far, with translations and scaling affecting each other and turning out wrong. Shouldn't be too hard to iron these out. This patch as it stands has all the APIs the frontend needs, so it should be enough to unblock that work, with the caveat of the aforementioned offsets-not-quite-right bugs. Now is probably a good time for a summary comment describing what these patches implement. - window.setCSSViewport(w, h): set the CSS viewport to <w, h> for the purposes of reflow. Triggers reflow (by design!). (And BTW, the height param is meaningful.) - window.setDisplayport(x, y, w, h): set the visible region wrt painting; the platform will draw enough layer pixels to cover this region. <x, y> is relative to the CSS viewport and in units of CSS pixels. The parameters are interpreted as for canvas.drawWindow(), except that the region persists and the pixels within are retained in the layer tree and updated automatically by the platform. Doesn't trigger reflow. - window.setResolution(xres, yres): allocate backing buffers for scalabale content (ThebesLayer) that are larger/smaller by a factor of <xres, yres>. ThebesLayers automatically draw themselves with an anti-scale to offset their resolution. (This API isn't very useful on its own, at least AFAICT.) Doesn't trigger reflow. - browser.scrollViewportBy(dx, dy), browser.scrollViewportTo(x, y): set a "desired scroll offset" for <browser>'s content pixels, which the <browser> remembers. When the <browser> draws its content pixels, if the actual content-document scrollOffset (as recorded in shadow layers) doesn't match the desired <browser> scrollOffset, then a translation is applied to the shadow layers to move their pixels to the desired scrollOffset. Note that this API requires the frontend to keep the browser.viewportScroll* values up-to-date (per whatever definition it chooses) with the scroll offset for web content in the content process. - browser.setViewportScale(xscale, yscale): tell the <browser> to apply a scale factor of <xscale, yscale> when drawing its content pixels. These APIs are designed to be used for - <meta viewport="width, height"> <==> content.setCSSViewport(w, h) - preparing a pre-fetch region for panning and scale-zooming: content.setDisplayport(). Just forces the platform to paint more pixels than it would otherwise. Frontend will have lots of twiddle room with this. - non-reflowing, scale-zoom by a factor of F: <==> browser.setViewportScale(F, F) and content.setResolution(F, F). Could be done in either order; the idea is that the scale-zoom operation would be animated through browser.setViewportScale(), and the "real" request for scaled content would be made by content.setResolution(). Would want to use content.setResolution() in concert with content.setDisplayport(). Does all this make sense?

Yes, this all makes sense I think. Getting to work on using it now!

The rollup patch doesn't include viewportScrollX/Y. Could we get those back?

ScrollBy seems to not scroll very far. I think this is scrolling in app units?

If I give setDisplayport negative coordinates (like -10, -10, 800, 500) the viewport has content that starts at 10, 10 (even though I didn't call scrollViewportBy/To).

When I begin using setResolution, I begin seeing a lot of black boxes (this is on Linux desktop).

Also with setResolution, there are offset problems like you suggested. There seems to be a miscalculation with the browser widget's offset. Because of the URL bar, the drawing starts a few pixels down where it should. When I pan left or right to the sidebars, the viewport will move a few pixels right or left, even though ScrollBy and ScrollTo haven't been called. I assume it is easy to reproduce by adding a spacer to the test XUL app.

+#include "nsPresContext.h" // for pixel/app-unit conversion No, we don't want appunits leaking down into layers. + nsSize viewport; + nsPoint viewportScrollOffset; + nsRect displayport; m* please. In this module we respect the style guide! :-) + nsSize viewport; nsIntSize mViewportSize. In device pixels (same as other layout coordinates) + nsPoint viewportScrollOffset; nsIntPoint + nsRect displayport; nsIntRect Snap edges/points before setting. Otherwise looks fine.

Is "Implement DisplayPort rendering" the right patch? I think not.

In http://hg.mozilla.org/users/cjones_mozilla.com/mcmq/file/7549f2a4fd14/590294-displayport-api: Personally I think displayport should CamelCase as DisplayPort, since "displayport" is not a word. Use rootFrame->InvalidateOverflowRect() instead of the invalidate you have, it's much safer and will invalidate necessary layers (with patches awaiting landing).

PAINT_IGNORE_VIEWPORT_SCROLLING is going to hide the viewport scrollbar. Doesn't matter for Fennec maybe, but not what we want for desktop. + else { + NS_ASSERTION(usingDisplayport, + "a rendering context is expected when not using viewport rendering!"); + } Why not? Can't someone do drawWindow on a window that has a displayport set on it? We probably need to make PAINT_IGNORE_VIEWPORT_SCROLLING into a flag which just hides the viewport scrollbars. All callers that want to paint outside the viewport (e.g. drawWIndow via RenderDocument) should probably set the displayport temporarily to do that. Make sense?

(In reply to comment #73) > +#include "nsPresContext.h" // for pixel/app-unit conversion > > No, we don't want appunits leaking down into layers. > OK. > + nsSize viewport; > + nsPoint viewportScrollOffset; > + nsRect displayport; > > m* please. In this module we respect the style guide! :-) > Fine. (Except are you referring to the "gfx" module? All the pure-data-container-with-only-public-member types that keep no invariants among members that I've used, (gfx|ns)(Int)?(Point|Rect|Size) and gfx*Matrix, use this same style, because |point.mX| is silly.) > + nsSize viewport; > > nsIntSize mViewportSize. In device pixels (same as other layout coordinates) > > + nsPoint viewportScrollOffset; > > nsIntPoint > > + nsRect displayport; > OK. > nsIntRect > > Snap edges/points before setting. > Might need to ask for help with this.

(In reply to comment #75) > In > http://hg.mozilla.org/users/cjones_mozilla.com/mcmq/file/7549f2a4fd14/590294-displayport-api: > > Personally I think displayport should CamelCase as DisplayPort, since > "displayport" is not a word. > Neither is "viewport" ;). I chose "displayport" to be consistent with "viewport", since they're intertwined. But I don't care all that much. > Use rootFrame->InvalidateOverflowRect() instead of the invalidate you have, > it's much safer and will invalidate necessary layers (with patches awaiting > landing). Will do.

(In reply to comment #74) > Is "Implement DisplayPort rendering" the right patch? I think not. Yes :P. That's all it took! I can pull out the Layer-->ContainerLayer twiddling to make things clearer. (In reply to comment #76) > PAINT_IGNORE_VIEWPORT_SCROLLING is going to hide the viewport scrollbar. > Doesn't matter for Fennec maybe, but not what we want for desktop. > Why not? We wouldn't want to use displayport rendering on desktop unless we wanted async scrolling. If we're using that, then the chrome process will need to draw the scrollbars, because in the chrome process the scroll position won't necessarily be where the content process thought it was when it painted those pixels. > + else { > + NS_ASSERTION(usingDisplayport, > + "a rendering context is expected when not using viewport > rendering!"); > + } > > Why not? Can't someone do drawWindow on a window that has a displayport set on > it? > Sure! But if that happens, then PaintFrame() would have been passed a rendering context and we wouldn't get to this else branch. > We probably need to make PAINT_IGNORE_VIEWPORT_SCROLLING into a flag which just > hides the viewport scrollbars. All callers that want to paint outside the > viewport (e.g. drawWIndow via RenderDocument) should probably set the > displayport temporarily to do that. Make sense? Maybe ... like I said before, I don't really understand the layer flushing here, and it seems a little sketchy. Was that done as some kind of performance heuristic? I'm not sure I understand all the cases and factors involved yet, but I agree that this impl probably isn't something we'd want in the long term. But for now, for fennec b1, it seems excessively risky to tie drawWindow in with a new fennec-only API. Would you be OK with pushing possible unification off to a follow-up bug? (That is, is the current displayport impl OK enough by your for a fennec b1 release.)

(In reply to comment #79) > (In reply to comment #74) > > + else { > > + NS_ASSERTION(usingDisplayport, > > + "a rendering context is expected when not using viewport > > rendering!"); > > + } > > > > Why not? Can't someone do drawWindow on a window that has a displayport set on > > it? > > > > Sure! But if that happens, then PaintFrame() would have been passed a > rendering context and we wouldn't get to this else branch. > Oh sorry, I think misunderstood your point here. Will fix the assertion message (something to the effect of, "!context => this is a displayport paint").

(In reply to comment #68) > The rollup patch doesn't include viewportScrollX/Y. Could we get those back? Done. (In reply to comment #69) > ScrollBy seems to not scroll very far. I think this is scrolling in app units? It was missing a CSS-pixel-to-app-unit conversion. Fixed. (In reply to comment #70) > If I give setDisplayport negative coordinates (like -10, -10, 800, 500) the > viewport has content that starts at 10, 10 (even though I didn't call > scrollViewportBy/To). I'm going to sit down and try to clear out the offset/scaling bugs, will make sure I fix this. The chrome-process drawing code is definitely missing at least one bit of magic for displayport pixels.

(In reply to comment #77) > > + nsSize viewport; > > + nsPoint viewportScrollOffset; > > + nsRect displayport; > > > > m* please. In this module we respect the style guide! :-) > > Fine. (Except are you referring to the "gfx" module? All the > pure-data-container-with-only-public-member types that keep no invariants among > members that I've used, (gfx|ns)(Int)?(Point|Rect|Size) and gfx*Matrix, use > this same style, because |point.mX| is silly.) I'm not sure the m* prefixes are silly, but anyway it's what the style guide says. The point/rect/size and matrix types are exceptions for no good reason. (In reply to comment #79) > Why not? We wouldn't want to use displayport rendering on desktop unless we > wanted async scrolling. If we're using that, then the chrome process will > need to draw the scrollbars, because in the chrome process the scroll > position won't necessarily be where the content process thought it was when > it painted those pixels. Having the chrome process draw the scrollbars is going to be difficult. There's a complicated dance where adding a horizontal or vertical scrollbar changes the available height/width which needs new reflows to be generated, without hurting performance or going into an infinite loop. nsGfxScrollFrame takes care of this; moving that out across a process boundary is going to be painful. Let's not deal with this now. > Sure! But if that happens, then PaintFrame() would have been passed a > rendering context and we wouldn't get to this else branch. OK. > > We probably need to make PAINT_IGNORE_VIEWPORT_SCROLLING into a flag which just > > hides the viewport scrollbars. All callers that want to paint outside the > > viewport (e.g. drawWIndow via RenderDocument) should probably set the > > displayport temporarily to do that. Make sense? > > Maybe ... like I said before, I don't really understand the layer flushing > here, and it seems a little sketchy. Was that done as some kind of > performance heuristic? No, it was a bug fix. If you draw with IGNORE_VIEWPORT_SCROLLING, the viewport scrollbars are hidden. If you then draw without IGNORE_VIEWPORT_SCROLLING and use the same retained ThebesLayer, there's no scrollbar where there should be one. There are similar issues around changing the size of the area we render a display list for, if we didn't flush layers for such changes. > But for now, for fennec b1, it seems excessively risky to tie drawWindow in > with a new fennec-only API. Would you be OK with pushing possible unification > off to a follow-up bug? (That is, is the current displayport impl OK enough > by your for a fennec b1 release.) I don't think the drawWindow (really RenderDocument) change I proposed is risky. It's a small change, and it will be tested when we run reftests on any machine with an available screen height < 1000. I'd like to do this now, it would simplify the logic in nsLayoutUtils::PaintFrame and make it more clear that layer flushing happens correctly.

Comment on attachment 470741 [details] [diff] [review] part 0: Add an nsDOMWindowUtils::GetPresShell() helper >+ nsresult rv; >+ nsCOMPtr<nsIPresShell> presShell; >+ rv = docShell->GetPresShell(getter_AddRefs(presShell)); >+ >+ return NS_FAILED(rv) ? nsnull : presShell; Just return presShell here.

Working better, still a clipping bug. Need to make sure scaling is working correctly with displayport.

I don't know who should be reviewing what here. In my queue I currently have Bug 590294, part 0: Add an nsDOMWindowUtils::GetPresShell() helper. r=smaug Bug 590294, part 1: Add a setCSSViewport() API to allow the frontend (or whatever) to override the natural viewport inherited from widget/view. sr=dbaron Bug 590294, part 2: Add a ContainerLayer attribute FrameMetrics that stores relevant information from layout/ on the root layer. r=roc sr=vlad Bug 590294, part 3: Have presshell manage whether it wants to ignore viewport scrolling during painting. r=roc Bug 590294, part 4: IGNORE_VIEWPORT_SCROLLING currently implies interpreting the visible region as being relative to document space. Displayport rendering wants everything IGNORE_VIEWPORT_SCROLLING implies, except it wants the visible region to be relative to the viewport. So, split the coordinate-space interpretation into a new flag DOCUMENT_RELATIVE, which interprets the visible region as document-relative. r=tn sr=roc Bug 590294, part 5: Keep ContainerLayer type info around in a few places. r=rn Bug 590294, part 6: Add a setDisplayPort() API to allow the frontend (or whatever else) to set an arbitrary "visible region" of a window, use it for viewport-scroll-ignoring canvas.drawWindow(), and stamp the root layer with relevant frame metrics. r=roc sr=dbaron Bug 590294, part 7: Add a window.setResolution(xres, yres) API to control the number of backing pixels allocated to scalable content. r=roc sr=dbaron Bug 590294, part 8: Add APIs for recording the resolution at which ThebesLayers were painted and for requesting resolution-scaled drawing for basic layers. r=roc sr=vlad Bug 590294, part 9: Implement resolution-scaled drawing for basic layers. r=roc Bug 590294, part 10: Add API to set a <browser> expectation of the content window's viewport, and to draw content pixels in the <browser> at particular scale factors. sr=smaug Bug 590294, part 11: Implement <browser>-configured viewport drawing for shadow layers. r=tn with guessed reviewers updated per smaug->dbaron switch just made. Are these correct? Halp!

Biggest showstoppers with rollup patch now: * When zoomed in, panning is a lot jerkier (both device and desktop) * There seems to be a memory leak; I will eventually get an OOM message * As I pan down, while setResolution is applied, I will begin to see black boxes (try pmo for instance) * When panning I will see a lot of artifacts on device (not an issue on desktop) * Displayport updates don't come in as quickly sometimes (not an issue on desktop)

<browser> should be clipped properly now.

Fix another clipping bug. Seems to be working pretty well in mobile2. Looking into reported zooming issue.

Comment on attachment 470744 [details] [diff] [review] part 3: Have presshell manage whether it wants to ignore viewport scrolling during painting This patch, on its own, isn't correct. - if ((aFlags & PAINT_WIDGET_LAYERS) && - !(aFlags & PAINT_IGNORE_VIEWPORT_SCROLLING)) { + if ((aFlags & PAINT_WIDGET_LAYERS) && !ignoreViewportScrolling) { // This layer tree will be reused, so we'll need to calculate it // for the whole visible area of the window visibleRegion = aFrame->GetOverflowRectRelativeToSelf(); } else { visibleRegion = aDirtyRegion; } When ignoreViewportScrolling and PAINT_WIDGET_LAYERS are set, we set visibleRegion to aDirtyRegion, which can vary per call with no intervening flush of the layer tree. A layer tree that was correct for one aDirtyRegion might be incorrect for the next call with a different aDirtyRegion. I think you need to fold in your displayport patch here and always set visibleRegion to the displayport when PAINT_WIDGET_LAYERS is set. + if (aIgnore != mIgnoreViewportScrolling) { + LayerManager* manager = GetLayerManager(); + if (manager) { + FrameLayerBuilder::InvalidateAllLayers(manager); + } + } + mIgnoreViewportScrolling = aIgnore; Slight improvement would be if (aIgnore == mIgnoreViewportScrolling) return; etc Otherwise looks good.

Header was truncated. It is part 4: IGNORE_VIEWPORT_SCROLLING currently implies interpreting the visible region as being relative to document space. Displayport rendering wants everything IGNORE_VIEWPORT_SCROLLING implies, except it wants the visible region to be relative to the viewport. So, split the coordinate-space interpretation into a new flag DOCUMENT_RELATIVE, which interprets the visible region as document-relative

Hm, that was truncated too. part 5: Add a setDisplayPort() API to allow the frontend (or whatever else) to set an arbitrary "visible region" of a window, use it for viewport-scroll-ignoring canvas.drawWindow(), and stamp the root layer with relevant frame metrics.

Fixes the correctness issues I can repro on desktop.

(In reply to comment #95) > Comment on attachment 470744 [details] [diff] [review] > part 3: Have presshell manage whether it wants to ignore viewport scrolling > during painting > + if (aIgnore != mIgnoreViewportScrolling) { > + LayerManager* manager = GetLayerManager(); > + if (manager) { > + FrameLayerBuilder::InvalidateAllLayers(manager); > + } > + } > + mIgnoreViewportScrolling = aIgnore; > > Slight improvement would be > if (aIgnore == mIgnoreViewportScrolling) > return; > etc > I forgot to make this change before posting the folded patch. I have it locally now.

Comment on attachment 471029 [details] [diff] [review] part 5: Add a setDisplayPort() API to allow the frontend (or whatever else) to set an arbitrary "visible region" of a window, use it for viewport-scroll-ignoring canvas.drawWindow(), and stamp the roo Rev IID on nsIDOMWindowUtils Parameter names in nsIDOMWindowUtils should start with 'a' + if (nsIPresShell* presShell = GetPresShell()) { + nsRect displayport( + nsPresContext::CSSPixelsToAppUnits(aXPx), + nsPresContext::CSSPixelsToAppUnits(aYPx), + nsPresContext::CSSPixelsToAppUnits(aWidthPx), + nsPresContext::CSSPixelsToAppUnits(aHeightPx)); + + presShell->SetDisplayPort(displayport); + + return NS_OK; + } + return NS_ERROR_FAILURE; Generally prefer the error path to come first ("if (...) return NS_ERROR_FAILURE;") and the straight-line code to be the normal success path. + presShell->GetPresContext()->GetVisibleArea() + .ToNearestPixels(auPerCSSPixel).Size(); Same line Heaps of call to GetPresContext()/PresContext() in nsDisplayList::PaintForFrame. Combine them into a local variable. Maybe another local for the presshell. + if ((aFlags & PAINT_WIDGET_LAYERS) && !ignoreViewportScrolling) { // This layer tree will be reused, so we'll need to calculate it // for the whole visible area of the window visibleRegion = aFrame->GetOverflowRectRelativeToSelf(); + } else if ((aFlags & PAINT_WIDGET_LAYERS) && presShell->UsingDisplayPort()) { + // A displayport has been set, so override whatever the window + // thinks is visible with that displayport + visibleRegion = presShell->GetDisplayPort(); } else { visibleRegion = aDirtyRegion; } if aFlags & PAINT_WIDGET_LAYERS and ignoreViewportScrolling and !UsingDisplayPort, we need to flush layers (or possibly clear PAINT_WIDGET_LAYERS). - flags |= nsLayoutUtils::PAINT_IGNORE_VIEWPORT_SCROLLING; + SetDisplayPort(aRect); Having SetDisplayPort also set SetIgnoreViewportScrolling is a bit confusing. It seems to me these could be orthogonal. Sure, you probably want to set them both at the same time, but you don't have to. + nsIFrame* rootFrame = FrameManager()->GetRootFrame(); + if (rootFrame) { + // XXX can we be smarter about the invalidate here? + rootFrame->InvalidateOverflowRect(); + } I don't think we actually need to do this. I think in nsLayoutUtils::PaintFrame we need a clearer separation of the paths for USE_WIDGET_LAYERS and the paths for !USE_WIDGET_LAYERS. In the latter case we don't need to flush any layers (I don't know why we're setting willFlushLyers anyway) because the retained layer tree will not be affected. Hmm, maybe the thing to do is to have RenderDocument compare its needs to the current state of the presshell's displayport/ignoreViewportScrolling. If they don't match, then instead of messing up the retained layer tree, clear USE_WIDGET_LAYERS if it's set and pass a IGNORE_VIEWPORT_SCROLLING flag into nsLayoutUtils::PaintFrame, with the requested rect as aDirtyRegion. Then in nsLayoutUtils::PaintFrame, the USE_WIDGET_LAYERS path can assume that the stored presshell displayport/ignoreViewportScrolling is exactly what's needed. Maybe it's even worth splitting nsLayoutUtils::PaintFrame into nsLayoutUtils::PaintFrameWithRetainedLayers and nsLayoutUtils::PaintFrame. For example all the print-related code only needs to be in PaintFrame.

Comment on attachment 471041 [details] [diff] [review] part 6: Add a window.setResolution(xres, yres) API to control the number of backing pixels allocated to scalable content + // It's not clear what upper bound to put on |res| values. On one + // hand, scaling a 1x1px window by a factor of 10000 is probably OK. + // On the other, scaling a 10000x10000px window by a factor of 2 is + // probably not OK. So for now, just push the decision into lower + // levels where a better decision can be made. I don't think we should try to do anything here. + * The effect of is API for gfx code to allocate more or fewer + * pixels for rescalable content by a factor of |resolution| in + * either or both dimensions. setResolution() together with + * setDisplayport() can be used to implement a non-reflowing + * scale-zoom; e.g., to scale by a factor of 2.0, + * + * setDisplayport(x, y, oldW / 2.0, oldH / 2.0); + * setResolution(2.0, 2.0); The way I had imagined this, these calls alone would not result in changed output. You would also need to apply a transform somewhere --- namely, a container layer containing the shadow layer tree --- to actually get scaled output. In fact, that seems to be what you actually did in part 8, so you'd better make that clear here.

Comment on attachment 471042 [details] [diff] [review] part 7: Add APIs for recording the resolution at which ThebesLayers were painted and for requesting resolution-scaled drawing for basic layers + // The interpretation of resolution is also specific to each layer + // type, which is why this also isn't a Layer API. For some layer + // types (at least CanvasLayer), proper support for resolution will + // never be feasible. I would leave this out, it's a bit confusing IMHO. + /** + * Set a target resolution for managed layers that are scalable. It + * might make sense to call this outside of a transaction, but + * currently it's only allowed during the construction phase of + * transactions. + */ + void SetResolution(float aXResolution, float aYResolution) Add an assertion that we're in the construction phase.

+ScaledSize(nsIntSize aSize, float aXScale, float aYScale) const nsIntSize& + // XXX It looks like |gfxContext::UserToDevicePixelSnapped(aSize, + // false)| with our scale applied might be the "right" way to + // compute the size. Is there a way to get the same result without + // a gfxContext? The thing to do would be to walk up the layer tree transforming the ThebesLayer bounds up to the root and then try to snap to nearest pixels. Once you have the transformed, snapped quadrilateral in device pixels, plus the desired resolution, you can multiply them together to get a size for the original ThebesLayer. If you wanted to do it "right" for 3D transforms, you'd have to do something a bit more tricky. But we don't need to do any of that here, I think. + nscoord ausPerPixel = nsIDeviceContext::AppUnitsPerCSSPixel(); + nsRect bounds = nsIntRect(nsIntPoint(0, 0), aSize).ToAppUnits(ausPerPixel); + return bounds.ScaleRoundOut(aXScale, aYScale).ToInsidePixels(ausPerPixel).Size(); Why are we pulling in appunits here? Layers shouldn't need to know anything about appunits or CSS pixels. + // Transform from buffer -> user space. + gfxMatrix transform; + transform.Translate(quadrantTranslation); + transform.Scale(1 / aXRes, 1 / aYRes); + + // Somewhat confusingly, the cairo_pattern transform is from user + // to pattern space. So we'll just invert the transform above + // that makes more sense in this context. + NS_ASSERTION(!transform.IsSingular(), "transform is singular???"); + transform.Invert(); I think you might as well just build the matrix the right way. It's simpler actually. + // SetSource() is just a convenience API for dealing with + // patterns, so these two cases could be unified. Leaving a + // special case in case SetSource() is ever fast-pathed. I wouldn't bother. In fact, I'd remove the extra path for better testing of the general path. Did you consider leaving the ThebesLayerBuffer resolution-ignorant and handling resolutions entirely in the BasicThebesLayer code? I wonder if that would be simpler. I'm a bit concerned about what happens when a quadrant boundary, after resolution scaling, falls on a non-pixel-boundary. Seems to me we'll get seams when we draw adjacent quadrants. I think we can make sure that doesn't happen, but I haven't worked out the details.

Comment on attachment 470742 [details] [diff] [review] part 1: Add a setCSSViewport() API to allow the frontend (or whatever) to override the natural viewport inherited from widget/view >+ if (!(aWidthPx > 0.0 && aHeightPx > 0.0)) { >+ return NS_ERROR_ILLEGAL_VALUE; >+ } I think you should allow 0, and just forbid negatives. >+ if (nsIPresShell* presShell = GetPresShell()) { >+ nscoord width = nsPresContext::CSSPixelsToAppUnits(PRInt32(aWidthPx)); >+ nscoord height = nsPresContext::CSSPixelsToAppUnits(PRInt32(aHeightPx)); >+ >+ presShell->ResizeReflow(width, height); >+ >+ return NS_OK; >+ } >+ return NS_ERROR_FAILURE; Could you make the error case an early return and the normal control flow non-indented? >+ /** >+ * Set the CSS viewport to be |widthPx| x |heightPx| in units of CSS >+ * pixels, regardless of the size of the enclosing widget/view. >+ * This will trigger reflow. >+ * >+ * The caller of this method must have UniversalXPConnect >+ * privileges. >+ */ >+ void setCSSViewport(in float widthPx, in float heightPx); Given that you're casting these to integer types and then checking that they're at least 0, why not just make these unsigned long instead of float? If there's a good reason for them to be float, though, I'm ok with it. sr=dbaron

Comment on attachment 470742 [details] [diff] [review] part 1: Add a setCSSViewport() API to allow the frontend (or whatever) to override the natural viewport inherited from widget/view Oh, yeah, and you need to rev the nsIDOMWindowUtils IID.

(In reply to comment #107) > Comment on attachment 471029 [details] [diff] [review] > part 5: Add a setDisplayPort() API to allow the frontend (or whatever else) to > set an arbitrary "visible region" of a window, use it for > viewport-scroll-ignoring canvas.drawWindow(), and stamp the roo > > Rev IID on nsIDOMWindowUtils > Gah, I've forgotten this in all my patches. > Parameter names in nsIDOMWindowUtils should start with 'a' > Oh sorry, I left out 'a' on IDL params all over the place. FTR wasn't trying to go maverick or anything ;), just assumed C++ param-naming conventions didn't apply in IDL. Should have checked the style guide or looked more closely enough at surrounding methods. > + if (nsIPresShell* presShell = GetPresShell()) { > + nsRect displayport( > + nsPresContext::CSSPixelsToAppUnits(aXPx), > + nsPresContext::CSSPixelsToAppUnits(aYPx), > + nsPresContext::CSSPixelsToAppUnits(aWidthPx), > + nsPresContext::CSSPixelsToAppUnits(aHeightPx)); > + > + presShell->SetDisplayPort(displayport); > + > + return NS_OK; > + } > + return NS_ERROR_FAILURE; > > Generally prefer the error path to come first ("if (...) return > NS_ERROR_FAILURE;") and the straight-line code to be the normal success path. > OK. > + presShell->GetPresContext()->GetVisibleArea() > + .ToNearestPixels(auPerCSSPixel).Size(); > > Same line > > Heaps of call to GetPresContext()/PresContext() in > nsDisplayList::PaintForFrame. Combine them into a local variable. Maybe another > local for the presshell. > OK. > + if ((aFlags & PAINT_WIDGET_LAYERS) && !ignoreViewportScrolling) { > // This layer tree will be reused, so we'll need to calculate it > // for the whole visible area of the window > visibleRegion = aFrame->GetOverflowRectRelativeToSelf(); > + } else if ((aFlags & PAINT_WIDGET_LAYERS) && presShell->UsingDisplayPort()) > { > + // A displayport has been set, so override whatever the window > + // thinks is visible with that displayport > + visibleRegion = presShell->GetDisplayPort(); > } else { > visibleRegion = aDirtyRegion; > } > > if aFlags & PAINT_WIDGET_LAYERS and ignoreViewportScrolling and > !UsingDisplayPort, we need to flush layers (or possibly clear > PAINT_WIDGET_LAYERS). > That can't happen in this implementation, but per below will fix. > - flags |= nsLayoutUtils::PAINT_IGNORE_VIEWPORT_SCROLLING; > + SetDisplayPort(aRect); > > Having SetDisplayPort also set SetIgnoreViewportScrolling is a bit confusing. > It seems to me these could be orthogonal. Sure, you probably want to set them > both at the same time, but you don't have to. > Alright. > + nsIFrame* rootFrame = FrameManager()->GetRootFrame(); > + if (rootFrame) { > + // XXX can we be smarter about the invalidate here? > + rootFrame->InvalidateOverflowRect(); > + } > > I don't think we actually need to do this. > Well in fennec, changing the displayport needs to trigger a repaint. Yeah, we don't need to do this for drawWindow. Hm. Probably need to use an internal setter for drawWindow. > I think in nsLayoutUtils::PaintFrame we need a clearer separation of the paths > for USE_WIDGET_LAYERS and the paths for !USE_WIDGET_LAYERS. In the latter case > we don't need to flush any layers (I don't know why we're setting > willFlushLyers anyway) because the retained layer tree will not be affected. > > Hmm, maybe the thing to do is to have RenderDocument compare its needs to the > current state of the presshell's displayport/ignoreViewportScrolling. If they > don't match, then instead of messing up the retained layer tree, clear > USE_WIDGET_LAYERS if it's set and pass a IGNORE_VIEWPORT_SCROLLING flag into > nsLayoutUtils::PaintFrame, with the requested rect as aDirtyRegion. Then in > nsLayoutUtils::PaintFrame, the USE_WIDGET_LAYERS path can assume that the > stored presshell displayport/ignoreViewportScrolling is exactly what's needed. > > Maybe it's even worth splitting nsLayoutUtils::PaintFrame into > nsLayoutUtils::PaintFrameWithRetainedLayers and nsLayoutUtils::PaintFrame. For > example all the print-related code only needs to be in PaintFrame. I like all this. Will do. Although I prefer PaintFrame and PaintFrameNoRetainedLayers (or something to that effect) since the former is the common case. Sound OK?

(In reply to comment #108) > Comment on attachment 471041 [details] [diff] [review] > part 6: Add a window.setResolution(xres, yres) API to control the number of > backing pixels allocated to scalable content > > + // It's not clear what upper bound to put on |res| values. On one > + // hand, scaling a 1x1px window by a factor of 10000 is probably OK. > + // On the other, scaling a 10000x10000px window by a factor of 2 is > + // probably not OK. So for now, just push the decision into lower > + // levels where a better decision can be made. > > I don't think we should try to do anything here. > By that you mean just do what this impl is currently doing (basically nothing) and drop the "So for now, " from the comment? > + * The effect of is API for gfx code to allocate more or fewer > + * pixels for rescalable content by a factor of |resolution| in > + * either or both dimensions. setResolution() together with > + * setDisplayport() can be used to implement a non-reflowing > + * scale-zoom; e.g., to scale by a factor of 2.0, > + * > + * setDisplayport(x, y, oldW / 2.0, oldH / 2.0); > + * setResolution(2.0, 2.0); > > The way I had imagined this, these calls alone would not result in changed > output. You would also need to apply a transform somewhere --- namely, a > container layer containing the shadow layer tree --- to actually get scaled > output. In fact, that seems to be what you actually did in part 8, so you'd > better make that clear here. Yes, this comment is missing that bit. Parts 9 and 10 add a browser.scaleViewport() API, so this should read * window.setDisplayport(x, y, oldW / 2.0, oldH / 2.0); * window.setResolution(2.0, 2.0); * browser.scaleViewport(2.0, 2.0); This works great BTW, thanks for the idea!

(In reply to comment #109) > Comment on attachment 471042 [details] [diff] [review] > part 7: Add APIs for recording the resolution at which ThebesLayers were > painted and for requesting resolution-scaled drawing for basic layers > > + // The interpretation of resolution is also specific to each layer > + // type, which is why this also isn't a Layer API. For some layer > + // types (at least CanvasLayer), proper support for resolution will > + // never be feasible. > > I would leave this out, it's a bit confusing IMHO. > OK. > + /** > + * Set a target resolution for managed layers that are scalable. It > + * might make sense to call this outside of a transaction, but > + * currently it's only allowed during the construction phase of > + * transactions. > + */ > + void SetResolution(float aXResolution, float aYResolution) > > Add an assertion that we're in the construction phase. + void SetResolution(float aXResolution, float aYResolution) + { + NS_ASSERTION(InConstruction(), "resolution must be set before drawing"); ;).

(In reply to comment #110) > +ScaledSize(nsIntSize aSize, float aXScale, float aYScale) > > const nsIntSize& > > + // XXX It looks like |gfxContext::UserToDevicePixelSnapped(aSize, > + // false)| with our scale applied might be the "right" way to > + // compute the size. Is there a way to get the same result without > + // a gfxContext? > > The thing to do would be to walk up the layer tree transforming the ThebesLayer > bounds up to the root and then try to snap to nearest pixels. Once you have the > transformed, snapped quadrilateral in device pixels, plus the desired > resolution, you can multiply them together to get a size for the original > ThebesLayer. If you wanted to do it "right" for 3D transforms, you'd have to do > something a bit more tricky. But we don't need to do any of that here, I think. > That's not possible for shadow layers, obviously, but I don't think it matters here. There are a lot of things I don't understand about what you're proposing here. I'll think through it some more and ping you on IRC if I still have questions. > + nscoord ausPerPixel = nsIDeviceContext::AppUnitsPerCSSPixel(); > + nsRect bounds = nsIntRect(nsIntPoint(0, 0), aSize).ToAppUnits(ausPerPixel); > + return bounds.ScaleRoundOut(aXScale, > aYScale).ToInsidePixels(ausPerPixel).Size(); > > Why are we pulling in appunits here? Layers shouldn't need to know anything > about appunits or CSS pixels. > This is just an ugly hack. > + // Transform from buffer -> user space. > + gfxMatrix transform; > + transform.Translate(quadrantTranslation); > + transform.Scale(1 / aXRes, 1 / aYRes); > + > + // Somewhat confusingly, the cairo_pattern transform is from user > + // to pattern space. So we'll just invert the transform above > + // that makes more sense in this context. > + NS_ASSERTION(!transform.IsSingular(), "transform is singular???"); > + transform.Invert(); > > I think you might as well just build the matrix the right way. It's simpler > actually. > OK. Maybe I'm in a minority that finds this confusing :). > + // SetSource() is just a convenience API for dealing with > + // patterns, so these two cases could be unified. Leaving a > + // special case in case SetSource() is ever fast-pathed. > > I wouldn't bother. In fact, I'd remove the extra path for better testing of the > general path. > OK. > Did you consider leaving the ThebesLayerBuffer resolution-ignorant and handling > resolutions entirely in the BasicThebesLayer code? I wonder if that would be > simpler. > I didn't really consider it because my first thought on implementing this was ThebesLayerBuffer::BeginPaint calling CreateBuffer(size * resolution). But the more I think about it the more I think you're right that this would be simpler. > I'm a bit concerned about what happens when a quadrant boundary, after > resolution scaling, falls on a non-pixel-boundary. Seems to me we'll get seams > when we draw adjacent quadrants. I think we can make sure that doesn't happen, > but I haven't worked out the details. OK.

(In reply to comment #111) > Comment on attachment 470742 [details] [diff] [review] > part 1: Add a setCSSViewport() API to allow the frontend (or whatever) to > override the natural viewport inherited from widget/view > > >+ if (!(aWidthPx > 0.0 && aHeightPx > 0.0)) { > >+ return NS_ERROR_ILLEGAL_VALUE; > >+ } > > I think you should allow 0, and just forbid negatives. > OK. > >+ if (nsIPresShell* presShell = GetPresShell()) { > >+ nscoord width = nsPresContext::CSSPixelsToAppUnits(PRInt32(aWidthPx)); > >+ nscoord height = nsPresContext::CSSPixelsToAppUnits(PRInt32(aHeightPx)); > >+ > >+ presShell->ResizeReflow(width, height); > >+ > >+ return NS_OK; > >+ } > >+ return NS_ERROR_FAILURE; > > Could you make the error case an early return and the normal control flow > non-indented? > Sure. > >+ /** > >+ * Set the CSS viewport to be |widthPx| x |heightPx| in units of CSS > >+ * pixels, regardless of the size of the enclosing widget/view. > >+ * This will trigger reflow. > >+ * > >+ * The caller of this method must have UniversalXPConnect > >+ * privileges. > >+ */ > >+ void setCSSViewport(in float widthPx, in float heightPx); > > Given that you're casting these to integer types and then checking that they're > at least 0, why not just make these unsigned long instead of float? If there's > a good reason for them to be float, though, I'm ok with it. > Ugh, the cast-to-integer is a vestige of the v0 patch when these params were indeed |unsigned long|s :(. That's a bug, thanks for the catch. The use case for float was to allow positioning the viewport precisely on device pixels on high dpi screens.

(In reply to comment #113) > I like all this. Will do. Although I prefer PaintFrame and > PaintFrameNoRetainedLayers (or something to that effect) since the former is > the common case. Sound OK? Yes. (In reply to comment #114) > By that you mean just do what this impl is currently doing (basically nothing) > and drop the "So for now, " from the comment? I mean I don't think we should be trying to validate resolutions since a) only chrome code can cause a problem and b) we don't know exactly what the invalidation should be. Actually what I think should happen is that any resolution should be allowed, but in ThebesLayer we can compute the scaled surface size using doubles, and if it's "too large" we simply choose a smaller surface size, effectively using a lower resolution. (In reply to comment #116) > > The thing to do would be to walk up the layer tree transforming the ThebesLayer > > bounds up to the root and then try to snap to nearest pixels. Once you have the > > transformed, snapped quadrilateral in device pixels, plus the desired > > resolution, you can multiply them together to get a size for the original > > ThebesLayer. If you wanted to do it "right" for 3D transforms, you'd have to do > > something a bit more tricky. But we don't need to do any of that here, I think. > > > > That's not possible for shadow layers, obviously, but I don't think it matters > here. There are a lot of things I don't understand about what you're proposing > here. I'll think through it some more and ping you on IRC if I still have > questions. Well, I don't think you should bother with it now. So don't worry about it. > > + nscoord ausPerPixel = nsIDeviceContext::AppUnitsPerCSSPixel(); > > + nsRect bounds = nsIntRect(nsIntPoint(0, 0), aSize).ToAppUnits(ausPerPixel); > > + return bounds.ScaleRoundOut(aXScale, > > aYScale).ToInsidePixels(ausPerPixel).Size(); > > > > Why are we pulling in appunits here? Layers shouldn't need to know anything > > about appunits or CSS pixels. > > > > This is just an ugly hack. What is it trying to fix? > > I'm a bit concerned about what happens when a quadrant boundary, after > > resolution scaling, falls on a non-pixel-boundary. Seems to me we'll get seams > > when we draw adjacent quadrants. I think we can make sure that doesn't happen, > > but I haven't worked out the details. > > OK. You probably should test a scale of say 1.5, of a page containing solid rectangle objects, scrolling in that page. I think you're going to see hairline artifacts. Although ... if your final scale draws everything to a single temporary surface and then scales that --- as it would if you're just using the layer 2D transform --- I guess we'll be OK.

(In reply to comment #118) > (In reply to comment #113) > (In reply to comment #114) > > By that you mean just do what this impl is currently doing (basically nothing) > > and drop the "So for now, " from the comment? > > I mean I don't think we should be trying to validate resolutions since a) only > chrome code can cause a problem and b) we don't know exactly what the > invalidation should be. > I agree that it's a probably a losing game to try to set an upper limit. I'm checking for >0.0 so that we can throw a useful exception back to chrome code if they accidentally pass a negative or NaN resolution, but I'm pretty sure you're not referring to that check. > Actually what I think should happen is that any resolution should be allowed, > but in ThebesLayer we can compute the scaled surface size using doubles, and if > it's "too large" we simply choose a smaller surface size, effectively using a > lower resolution. > Interesting. I'll keep that in mind. > (In reply to comment #116) > > > The thing to do would be to walk up the layer tree transforming the ThebesLayer > > > bounds up to the root and then try to snap to nearest pixels. Once you have the > > > transformed, snapped quadrilateral in device pixels, plus the desired > > > resolution, you can multiply them together to get a size for the original > > > ThebesLayer. If you wanted to do it "right" for 3D transforms, you'd have to do > > > something a bit more tricky. But we don't need to do any of that here, I think. > > > > > > > That's not possible for shadow layers, obviously, but I don't think it matters > > here. There are a lot of things I don't understand about what you're proposing > > here. I'll think through it some more and ping you on IRC if I still have > > questions. > > Well, I don't think you should bother with it now. So don't worry about it. > OK. > > > + nscoord ausPerPixel = nsIDeviceContext::AppUnitsPerCSSPixel(); > > > + nsRect bounds = nsIntRect(nsIntPoint(0, 0), aSize).ToAppUnits(ausPerPixel); > > > + return bounds.ScaleRoundOut(aXScale, > > > aYScale).ToInsidePixels(ausPerPixel).Size(); > > > > > > Why are we pulling in appunits here? Layers shouldn't need to know anything > > > about appunits or CSS pixels. > > > > > > > This is just an ugly hack. > > What is it trying to fix? > I was trying to re-use the round-to-integer-pixels code in nsRect. > > > I'm a bit concerned about what happens when a quadrant boundary, after > > > resolution scaling, falls on a non-pixel-boundary. Seems to me we'll get seams > > > when we draw adjacent quadrants. I think we can make sure that doesn't happen, > > > but I haven't worked out the details. > > > > OK. > > You probably should test a scale of say 1.5, of a page containing solid > rectangle objects, scrolling in that page. I think you're going to see hairline > artifacts. > Yes, I've seen seams already with google.com scaled even by 2.0 and 0.5. > Although ... if your final scale draws everything to a single temporary surface > and then scales that --- as it would if you're just using the layer 2D > transform --- I guess we'll be OK. I'm not familiar with that code yet, but a quick glance led me to believe we don't create temporary surfaces just for transforms (PushGroup() <=> temporary surface, right?). I would think that we probably really really don't want to be creating temporary surfaces to draw layers with simple scale/translate transforms to screen on mobile. But again to be clear this stuff pretty well beyond my ken atm, trying to learn :).

(In reply to comment #119) > I agree that it's a probably a losing game to try to set an upper limit. I'm > checking for >0.0 so that we can throw a useful exception back to chrome code > if they accidentally pass a negative or NaN resolution, but I'm pretty sure > you're not referring to that check. No, that's fine. > I was trying to re-use the round-to-integer-pixels code in nsRect. Use gfxRect::RoundOut or Round then. > > Although ... if your final scale draws everything to a single temporary > > surface and then scales that --- as it would if you're just using the layer 2D > > transform --- I guess we'll be OK. > > I'm not familiar with that code yet, but a quick glance led me to believe we > don't create temporary surfaces just for transforms (PushGroup() <=> temporary > surface, right?). I would think that we probably really really don't want to > be creating temporary surfaces to draw layers with simple scale/translate > transforms to screen on mobile. But again to be clear this stuff pretty well > beyond my ken atm, trying to learn :). I see.

Comment on attachment 471028 [details] [diff] [review] part 4: IGNORE_VIEWPORT_SCROLLING currently implies interpreting the visible region as being relative to document space. Displayport rendering wants everything IGNORE_VIEWPORT_SCROLLING [see below] If nsLayoutUtils::PaintFrame is passed the PAINT_DOCUMENT_RELATIVE flag then I don't think we want to be trying to use the dirty region to UpdatePossiblyTransparentRegion lower down. So I think make that conditional on not having that flag.

Comment on attachment 471045 [details] [diff] [review] part 10: Implement <browser>-configured viewport drawing for shadow layers SetTransformFor is using ViewportConfig::mScrollOffset for something, but I can't figure out what because I don't know what ViewportConfig::mScrollOffset is supposed to represent (I looked at the previous patch adding). Can you expand on the meanings of the ViewportConfig structure, its members, the FrameMetrics structure, and it's members in the comments? Also SetTransformFor is setting the transform on a layer in CSS pixels; I thought layers were all in device pixels.

(In reply to comment #122) > Comment on attachment 471028 [details] [diff] [review] > part 4: IGNORE_VIEWPORT_SCROLLING currently implies interpreting the visible > region as being relative to document space. Displayport rendering wants > everything IGNORE_VIEWPORT_SCROLLING [see below] > > If nsLayoutUtils::PaintFrame is passed the PAINT_DOCUMENT_RELATIVE flag then I > don't think we want to be trying to use the dirty region to > UpdatePossiblyTransparentRegion lower down. So I think make that conditional on > not having that flag. Thanks for the catch, fixed. (In reply to comment #123) > Comment on attachment 471045 [details] [diff] [review] > part 10: Implement <browser>-configured viewport drawing for shadow layers > > SetTransformFor is using ViewportConfig::mScrollOffset for something, but I > can't figure out what because I don't know what ViewportConfig::mScrollOffset > is supposed to represent (I looked at the previous patch adding). Can you > expand on the meanings of the ViewportConfig structure, its members, the > FrameMetrics structure, and it's members in the comments? > I tried. Let me know if the comments are still insufficient. > Also SetTransformFor is setting the transform on a layer in CSS pixels; I > thought layers were all in device pixels. Eep! This bug has been around since RenderFrameParent.cpp. Thanks, fixed.

This attempts to move persistent rendering state into PresShell. If that state changes drastically, then PresShell throws away retained layers. I pondered making |struct RenderingState| instead of individual members, and having a single mRenderingState. I may end up doing this regardless when we need mIgnoringCaret. Thoughts?

Decided to keep ThebesLayerBuffer aware of resolution after trying it the other way.

Added more comments.

More comments, dev-pixel fix, and s/RootOf/ShadowRootOf/.

Comment on attachment 471431 [details] [diff] [review] part 5: Add a setDisplayPort() API to allow the frontend (or whatever else) to set an arbitrary visible region for the purposes of invalidation and layer retaining + if ((aFlags & PAINT_WIDGET_LAYERS)) { Redundant () + PRBool willFlushRetainedLayers = aFlags & PAINT_HIDE_CARET; (aFlags & PAINT_HIDE_CARET) != 0 +void PresShell::SetRenderingState(PRBool aIgnoreViewportScrolling, + PRBool aUseDisplayPort, nsRect aDisplayPort) const nsRect& Also, this needs a flags word. Boolean parameters suck, especially multiple booleans. Although instead of a PRBool and an nsRect&, you could pass an nsRect* and allow null. + nsIFrame* rootFrame = FrameManager()->GetRootFrame(); + if (rootFrame) { + // XXX be smarter about the invalidation if the displayport only + // moved around a bit + rootFrame->InvalidateOverflowRect(); + } I don't think you need to invalidate for displayport changes. I think you need to invalidate for mIgnoringViewportScrolling changes, and you should use rootFrame->InvalidateFrameSubtree. r=me with all that fixed.

Comment on attachment 471432 [details] [diff] [review] part 6: Add a window.setResolution(xres, yres) API to control the number of backing pixels allocated to scalable content + * The effect of is API for gfx code to allocate more or fewer + * pixels for rescalable content by a factor of |resolution| in + * either or both dimensions. setResolution() together with + * setDisplayport() can be used to implement a non-reflowing + * scale-zoom; e.g., to scale by a factor of 2.0, + * + * setDisplayport(x, y, oldW / 2.0, oldH / 2.0); + * setResolution(2.0, 2.0); Still needs a comment to make it clear that no visible scaling will take place unless you scale the layout output somehow.

(In reply to comment #113) > Well in fennec, changing the displayport needs to trigger a repaint. I see. Well, I think you should invalidate the union of the old and new displayports in your patch then. The rootFrame's overflow rect does not include areas outside the CSS viewport. Actually, it seems to me you're going to need to do something to ensure that invalidation outside the CSS viewport is not clipped by the root scroll frame (in nsHTMLScrollFrame::InvalidateInternal).

Comment on attachment 471433 [details] [diff] [review] part 8: Implement resolution-scaled drawing for basic layers This is OK, modulo the issue of quadrant-drawing leading to seams in the output when scaled. We need to figure out a plan for that.

(In reply to comment #130) > Comment on attachment 471431 [details] [diff] [review] > part 5: Add a setDisplayPort() API to allow the frontend (or whatever else) to > set an arbitrary visible region for the purposes of invalidation and layer > retaining > > + if ((aFlags & PAINT_WIDGET_LAYERS)) { > > Redundant () > Fixed. > + PRBool willFlushRetainedLayers = aFlags & PAINT_HIDE_CARET; > > (aFlags & PAINT_HIDE_CARET) != 0 > Fixed. > +void PresShell::SetRenderingState(PRBool aIgnoreViewportScrolling, > + PRBool aUseDisplayPort, nsRect aDisplayPort) > > const nsRect& > > Also, this needs a flags word. Boolean parameters suck, especially multiple > booleans. Although instead of a PRBool and an nsRect&, you could pass an > nsRect* and allow null. > I made |struct RenderingState| and now use that for SetRenderingState() and AutoSaveRestoreRenderingState. I'll post the corrected patch. > + nsIFrame* rootFrame = FrameManager()->GetRootFrame(); > + if (rootFrame) { > + // XXX be smarter about the invalidation if the displayport only > + // moved around a bit > + rootFrame->InvalidateOverflowRect(); > + } > > I don't think you need to invalidate for displayport changes. I think you need > to invalidate for mIgnoringViewportScrolling changes, and you should use > rootFrame->InvalidateFrameSubtree. > OK. I think per below, we're on the same page wrt displayport invalidation now. (In reply to comment #132) > (In reply to comment #113) > > Well in fennec, changing the displayport needs to trigger a repaint. > > I see. Well, I think you should invalidate the union of the old and new > displayports in your patch then. The rootFrame's overflow rect does not include > areas outside the CSS viewport. > > Actually, it seems to me you're going to need to do something to ensure that > invalidation outside the CSS viewport is not clipped by the root scroll frame > (in nsHTMLScrollFrame::InvalidateInternal). I was afraid that was going to happen somewhere. Will investigate, thanks.

(In reply to comment #131) > Comment on attachment 471432 [details] [diff] [review] > part 6: Add a window.setResolution(xres, yres) API to control the number of > backing pixels allocated to scalable content > > + * The effect of is API for gfx code to allocate more or fewer > + * pixels for rescalable content by a factor of |resolution| in > + * either or both dimensions. setResolution() together with > + * setDisplayport() can be used to implement a non-reflowing > + * scale-zoom; e.g., to scale by a factor of 2.0, > + * > + * setDisplayport(x, y, oldW / 2.0, oldH / 2.0); > + * setResolution(2.0, 2.0); > > Still needs a comment to make it clear that no visible scaling will take place > unless you scale the layout output somehow. Gah, sorry, I forgot about that. (In reply to comment #133) > Comment on attachment 471433 [details] [diff] [review] > part 8: Implement resolution-scaled drawing for basic layers > > This is OK, modulo the issue of quadrant-drawing leading to seams in the output > when scaled. We need to figure out a plan for that. Sure.

Comment on attachment 471621 [details] [diff] [review] part 5: Add a setDisplayPort() API to allow the frontend (or whatever else) to set an arbitrary visible region for the purposes of invalidation and layer retaining, v3 Carrying over roc r+.

No change in functionality, but the shadow layer leak has been plugged on cedar trunk.

(In reply to comment #92) > Biggest showstoppers with rollup patch now: > * When zoomed in, panning is a lot jerkier (both device and desktop) How's this looking? FTR, panning with a scale applied to the <browser> *before* the matching setResolution() will always be slower than without a scale (until HW acceleration ;) ), although stuart's plan to use nearest-neighbor interpolation could make this better. Panning with a scale applied to the <browser> *after* the matching setResolution() shouldn't be slower, because the scale and setResolution cancel each other out. Or are supposed to at least ;). > * There seems to be a memory leak; I will eventually get an OOM message Fixed. > * As I pan down, while setResolution is applied, I will begin to see black > boxes (try pmo for instance) Fixed. > * When panning I will see a lot of artifacts on device (not an issue on > desktop) Is this still happening? http://hg.mozilla.org/projects/cedar/rev/92f995b19432 will fix some bad artifacts, like those on stechz.com, but you would have seen those on desktop too. > * Displayport updates don't come in as quickly sometimes (not an issue on > desktop) If these are still lagging by O(secs), we need to spin off a new bug and get to the root of this. There are some suspects. Ben/Matt, any other nasty problems with the latest patches? If not, I'll land these patches soon and we can fix smaller stuff in followups.

Wow, this is pretty usable on device, already. Nice work guys! Some frontend things - scale-zoom works great! The flash going from old-canvas to new-canvas when the new pixels arrive is gone. - I'm not seeing flashing artifacts near the URL bar anymore when panning. - prefetching pixels for panning seems to be "checkerboarding" (default widget background for cedar/mobile2) less than in 2.0a1, but I wonder what the memory trade-off is. - the scale-zoom-in/out animation seems to take longer than in 2.0a1. I'm not sure if this is because the platform is slowing things down or if the animation params changed. I like the shorter-duration animation, to the point where I'd prefer skipped frames to longer duration. - the frontend has some rough edges around content-process crashes. The default widget background is drawn and confusingly, the URL bar continues to show the last loaded page, and if the page was still loading, the progress indicator keeps spinning. I think this needs some UX love; being able to handle crashes gracefully is a competitive advantage over at least mobilesafari, which just falls over. - clicking a link from "Your tabs from last time" sometimes caused a translucent blue box to appear over the link that persisted into the page load. Platform things - we're still OOMing too easily :S. Scale-zooming an article on nytimes.com triggers OOM reliably for me. Not sure what's going on here. - panning perf is considerably worse than in 2.0a1, even for simple layer trees. This is likely a combination of ShadowThebesLayer being a client-side gfxImageSurface and possibly using 32-bit pixels. - from what I saw, rendering was less apparently glitchy for common things than with shmem canvas. There are plenty of bugs in the displayport/viewport* setup, but perf is too bad to consider a b1 yet. IMHO time to put rendering bugs on pause for a bit and hammer on perf. (In reply to comment #139) > (In reply to comment #92) > > Biggest showstoppers with rollup patch now: > > * When zoomed in, panning is a lot jerkier (both device and desktop) > > How's this looking? > > FTR, panning with a scale applied to the <browser> *before* the matching > setResolution() will always be slower than without a scale (until HW > acceleration ;) ), although stuart's plan to use nearest-neighbor interpolation > could make this better. Panning with a scale applied to the <browser> *after* > the matching setResolution() shouldn't be slower, because the scale and > setResolution cancel each other out. Or are supposed to at least ;). > This is what I saw on device. Panning on a scaled <browser> before new setResolution()'d pixels come in is slow, but after they arrive panning goes back to normal (less slow ;) ). > > * When panning I will see a lot of artifacts on device (not an issue on > > desktop) > > Is this still happening? http://hg.mozilla.org/projects/cedar/rev/92f995b19432 > will fix some bad artifacts, like those on stechz.com, but you would have seen > those on desktop too. > Seemed OK to me while poking around. If you can still repro, can you be more specific about what kinds of artifacts and on what sites you see them?

(In reply to comment #140) > Wow, this is pretty usable on device, already. Nice work guys! > > Some frontend things > - clicking a link from "Your tabs from last time" sometimes caused a > translucent blue box to appear over the link that persisted into the page load. > This bug is not related to cedar/mobile2. He is here since the e10s merge. I have just filed bug 593308 for it.

Comment on attachment 471434 [details] [diff] [review] part 9: Add API to set a <browser> expectation of the content window's viewport, and to draw content pixels in the <browser> at particular scale factors I'm not really a fan of naming the methods scroll*, but I'm not sure what would be better name.

http://hg.mozilla.org/projects/cedar/rev/b3039d94296e http://hg.mozilla.org/projects/cedar/rev/38d1ce121661 http://hg.mozilla.org/projects/cedar/rev/8129639ab610 http://hg.mozilla.org/projects/cedar/rev/2e7de8c7da34 http://hg.mozilla.org/projects/cedar/rev/99204e5b40f2 http://hg.mozilla.org/projects/cedar/rev/99279c1c33cc http://hg.mozilla.org/projects/cedar/rev/a5f171570672 http://hg.mozilla.org/projects/cedar/rev/0b90c6b07933 http://hg.mozilla.org/projects/cedar/rev/3467965f4d9f http://hg.mozilla.org/projects/cedar/rev/8321155e8dad http://hg.mozilla.org/projects/cedar/rev/a1526ab475c9 http://hg.mozilla.org/projects/cedar/rev/15302bb0843b http://hg.mozilla.org/projects/cedar/rev/0ef3696734ae Many follow-ups to, er, follow.