590294 - Add platform API for translating and scaling <browser remote> shadow layers

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Proposed <browser> API

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.)

Comment 1

[Benjamin Stover (:stechz)]

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

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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 4

[Stuart Parmenter]

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.

Comment 5

[Benjamin Stover (:stechz)]

> 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).

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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.

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: New Layer API and shadow layers goop for forwarding frame metrics

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: <browser> API for manipulating "viewport"

Comment 9

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need 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.

Comment 10

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 1: Add a ContainerLayer attribute 'FrameMetrics' that for now just stores the content document's scrollLeft/scrollTop when it last updated the layers

Comment 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 2: In PuppetWidget-using processes, stamp the root layer with the document metrics on paints

Comment 12

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 3: Add viewportScrollBy()/viewportScrollTo() APIs to <browser>

Comment 13

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 4: Use the shadow root's frame metrics and the <browser>'s viewport scroll offsets to set a compensation transform when painting shadow layers

Comment 14

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Test app that uses <browser> viewport scroll offsets to implement a simple panning animation

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.

Comment 15

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Test app with panning and nascent scale.

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?

Comment 16

[Boris Zbarsky [:bzbarsky]]

>  (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?

Comment 17

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(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.

Comment 18

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(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.

Comment 19

[Jeremias Bosch (:jbos)]

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.

Comment 20

[Oleg Romashin (:romaxa)]

> 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.

Comment 21

[Oleg Romashin (:romaxa)]

Attachment: 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?

Comment 22

[Boris Zbarsky [:bzbarsky]]

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?

Comment 23

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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?

Comment 24

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 25

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Screenshot of innerWidth mismatch with viewport

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?

Comment 26

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 27

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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?

Comment 28

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 29

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Cancel reviews

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

Comment 30

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(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.

Comment 31

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 32

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 33

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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.

Comment 34

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Add a setCSSViewport() API to allow the frontend (or whatever) to override the natural viewport inherited from widget/view

Comment 35

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Test app that uses setCSSViewport()

Comment 36

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(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.

Comment 37

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think a separate nsIPresShell API should set the resolution.

Comment 38

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 39

[Mark Finkle (:mfinkle) (use needinfo?)]

(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.

Comment 40

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 41

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(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.

Comment 42

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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.

Comment 43

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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?

Comment 44

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: WIP - Add a setDisplayport() API to allow the frontend (or whatever else) to set the visible region of a document

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.

Comment 45

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Test app that uses setCSSViewport() and setDisplayport()

Comment 46

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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.

Comment 47

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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)

Comment 48

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(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.

Comment 49

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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.

Comment 50

[Robert O'Callahan (:roc) (email my personal email if necessary)]

-- 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.

Comment 51

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(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.

Comment 52

[Matt Brubeck (:mbrubeck)]

(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?

Comment 53

[Benjamin Stover (:stechz)]

(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.

Comment 54

[Benjamin Stover (:stechz)]

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?

Comment 55

[Benjamin Stover (:stechz)]

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

Comment 56

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 57

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Did some thinking last night.

If fennec b1 dropped support for <meta viewport width/height>, would anyone notice?

Comment 58

[Benjamin Stover (:stechz)]

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

Comment 59

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(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.

Comment 60

[Benjamin Stover (:stechz)]

> 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.

Comment 61

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(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.

Comment 62

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Add a setDisplayport() API to allow the frontend (or whatever else) to set an arbitrary "visible region" of a window

Comment 63

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Implement displayport rendering

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.

Comment 64

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Test app that uses setCSSViewport() and setDisplayport()

Comment 65

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Add a ContainerLayer attribute 'FrameMetrics' that stores relevant information from layout/ on the root layer

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.

Comment 66

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Rollup of setCSSViewport(), setDisplayport(), setResolution(), setViewport*(), and test app patches

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?

Comment 67

[Benjamin Stover (:stechz)]

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

Comment 68

[Benjamin Stover (:stechz)]

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

Comment 69

[Benjamin Stover (:stechz)]

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

Comment 70

[Benjamin Stover (:stechz)]

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).

Comment 71

[Benjamin Stover (:stechz)]

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

Comment 72

[Benjamin Stover (:stechz)]

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.

Comment 73

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+#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.

Comment 74

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 75

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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).

Comment 76

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 77

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(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.

Comment 78

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(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.

Comment 79

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(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.)

Comment 80

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(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").

Comment 81

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: "Rollup" v2

(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.

Comment 82

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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 83

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Cancel reviews

Comment 84

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 0: Add an nsDOMWindowUtils::GetPresShell() helper

Comment 85

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 1: Add a setCSSViewport() API to allow the frontend (or whatever) to override the natural viewport inherited from widget/view

Comment 86

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 2: Add a ContainerLayer attribute FrameMetrics that stores relevant information from layout/ on the root layer

Comment 87

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 3: Have presshell manage whether it wants to ignore viewport scrolling during painting

Comment 88

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 4: Keep ContainerLayer type info around in a few places

Comment 89

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 90

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: "Rollup" v3

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

Comment 91

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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!

Comment 92

[Benjamin Stover (:stechz)]

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)

Comment 93

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: "Rollup" v4

<browser> should be clipped properly now.

Comment 94

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: "Rollup" v5

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

Comment 95

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 96

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: 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]

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

Comment 97

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: 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

Comment 98

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 99

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 6: Add a window.setResolution(xres, yres) API to control the number of backing pixels allocated to scalable content

Comment 100

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 7: Add APIs for recording the resolution at which ThebesLayers were painted and for requesting resolution-scaled drawing for basic layers

Comment 101

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 8: Implement resolution-scaled drawing for basic layers

Comment 102

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: 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

Comment 103

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 10: Implement <browser>-configured viewport drawing for shadow layers

Comment 104

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part -1: Clip <browser> shadow layers properly

Comment 105

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: "Rollup" v6

Fixes the correctness issues I can repro on desktop.

Comment 106

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(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 107

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 108

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 109

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 110

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+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 111

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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 112

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 113

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(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?

Comment 114

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(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!

Comment 115

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(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");

;).

Comment 116

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(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.

Comment 117

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(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.

Comment 118

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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.

Comment 119

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(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 :).

Comment 120

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Rollup v7, plugs ThebesLayerBuffer surface leak

Comment 121

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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 122

[Timothy Nikkel (:tnikkel)]

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 123

[Timothy Nikkel (:tnikkel)]

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.

Comment 124

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(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.

Comment 125

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: 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

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?

Comment 126

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 6: Add a window.setResolution(xres, yres) API to control the number of backing pixels allocated to scalable content

Comment 127

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 8: Implement resolution-scaled drawing for basic layers

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

Comment 128

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: 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

Added more comments.

Comment 129

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 10: Implement <browser>-configured viewport drawing for shadow layers

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

Comment 130

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 131

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 132

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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 133

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 134

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(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.

Comment 135

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(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 136

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: 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

Comment 137

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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+.

Comment 138

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: "Rollup" 7.1

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

Comment 139

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(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.

Comment 140

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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?

Comment 141

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(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 142

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 143

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.