There's a lot happening offscreen with APZ, it would be handy to build a minimap of the page to show the current viewport, the state of the tiles, the current viewport.

The second "viewport" in comment 0 is meant to be "displayport", I believe.

Yes, I want to show both.

I'm trying to find the right spot to get started. I want to know where to do the drawing: http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/ContainerLayerComposite.cpp#399 . This looks like a good spot. But I'm not sure how to properly handle container-less APZ. I only want to show one minimap for the entire APZ 'group'. I figured I should have a plan for this before I get too far along in the wrong path.

The layer borders drawing code for scrollable layers (which you linked to) will currently draw the border in the same position for every layer associated with the scroll frame (so we're drawing the border redundantly). Assuming you want to avoid that for the minimaps, you could keep track of which scroll frames had their minimaps drawn already during this composite, and only draw the minimap for the first layer you encounter that's associated with that scroll frame. (It doesn't matter which layer you do it for, as you would draw it based on the FrameMetrics which is the same for all layers associated with the scroll frame.) You can uniquely identify scroll frames by their ScrollableLayerGuid, which you can get via layer->GetAsyncPanZoomController()->GetGuid().

Sounds good!

Just making sure I understand the screenshot - the thing on the left is the minimap, with the green box being the displayport and the pink box inside being the visible area?

- Outer box is the page. This will keep the aspect ratio of the page and will be restricted to the left-most 100px. - Green box is the display port (fixed the offset issue after the screenshot) - Pink is the viewport. Does anyone feel strongly about critical displayport? Right now I'm learning towards leaving it out not to overload the view but I could be easily convinced. I'll be tweaking the color, adding contrast. I also want to look at the feasibility of showing where the content is -currently- painting but that might be too difficult since it would need to by-pass IPDL.

With the default 12 or so tiles across on the full screen retina, you may want to have those 100px a configurable number :)

(In reply to Benoit Girard (:BenWa) from comment #8) > Does anyone feel strongly about critical displayport? Right now I'm learning > towards leaving it out not to overload the view but I could be easily > convinced. I'm fine with this. Really it depends on what you want to use the minimap for.

Bug 1186159 - Add an APZ minimap. r=kats

I've punted on the issue of multi-layer cases because I didn't encounter any while testing locally. Feel free to push back when reviewing if you think it's important. Otherwise we can address it when it comes up. If I understand correctly we're just going to draw the minimap over itself in those cases. BTW the typed units made this a lot easier to write :D

Comment on attachment 8637444 [details] MozReview Request: Bug 1186159 - Add an APZ minimap. r=kats https://reviewboard.mozilla.org/r/13867/#review12455 Looks pretty good. Some issues though, would like to see an updated version. I'm also not entirely confident I can review the Matrix4x4 computation you have but I assume you know what you're doing for that. ::: gfx/layers/composite/ContainerLayerComposite.cpp:339 (Diff revision 1) > + if (aLayer->GetFrameMetricsCount() >= 1) { I'd prefer inverting this condition and doing an early return. It will reduce indentation of the rest of the code and anybody reading this code doesn't have to keep the mental overhead of "what happens after this condition ends?" ::: gfx/layers/composite/ContainerLayerComposite.cpp:341 (Diff revision 1) > + if (controller) { Ditto here ::: gfx/layers/composite/ContainerLayerComposite.cpp:360 (Diff revision 1) > + LayerRect viewRect = fm.GetViewport() * fm.LayersPixelsPerCSSPixel() + scrollOffset / LayerToParentLayerScale((1)); fm.GetViewport() returns the CSS viewport which is not really what you want here (although in practice this will probably work most of the time). The more correct thing is: LayerRect viewRect = ParentLayerRect(scrollOffset, fm.GetCompositionBounds().Size()) / LayerToParentLayerScale(1); ::: gfx/layers/composite/ContainerLayerComposite.cpp:359 (Diff revision 1) > + const CSSRect& scrollRect = fm.GetScrollableRect(); Everywhere you use scrollRect you're multiplying by fm.LayersPixelsPerCSSPixel. It's better to just turn it into a LayerRect here once. ::: gfx/layers/composite/ContainerLayerComposite.cpp:377 (Diff revision 1) > + clipRect = transform.TransformBounds((scrollRect * fm.LayersPixelsPerCSSPixel()).ToUnknownRect()); Combine this into the previous line: Rect clipRect = ...; ::: gfx/layers/composite/ContainerLayerComposite.cpp:383 (Diff revision 1) > + r = transform.TransformBounds((scrollRect * fm.LayersPixelsPerCSSPixel()).ToUnknownRect()); Combine this into the previous line ::: gfx/layers/composite/ContainerLayerComposite.cpp:390 (Diff revision 1) > + for (int x = layerScrollRect.x; x < layerScrollRect.x + layerScrollRect.width; x += tileW) { Use layerScrollRect.XMost() ::: gfx/layers/composite/ContainerLayerComposite.cpp:391 (Diff revision 1) > + for (int y = layerScrollRect.y; y < layerScrollRect.y + layerScrollRect.height; y += tileH) { Ditto YMost() ::: gfx/layers/composite/ContainerLayerComposite.cpp:361 (Diff revision 1) > + CSSRect dp = fm.GetDisplayPort(); Convert this as a LayerRect here also

Comment on attachment 8637444 [details] MozReview Request: Bug 1186159 - Add an APZ minimap. r=kats Bug 1186159 - Add an APZ minimap. r=kats

Comment on attachment 8637444 [details] MozReview Request: Bug 1186159 - Add an APZ minimap. r=kats https://reviewboard.mozilla.org/r/13867/#review12481 r+ with comments addressed ::: gfx/layers/composite/ContainerLayerComposite.cpp:374 (Diff revision 2) > + scaleFactorX = 100.f / (scrollRect.width); You can remove the parens around scrollRect.width here and scrollRect.height on the next line ::: gfx/layers/Compositor.cpp:381 (Diff revision 2) > + Stray newline ::: gfx/layers/apz/src/AsyncPanZoomController.cpp (Diff revision 2) > -# define APZC_LOG(...) Stray change snuck in from somewhere? Looks like you have some other patch under this one ::: modules/libpref/init/all.js:550 (Diff revision 2) > +pref("apz.minimap.enabled", true); Default this to false

Comment on attachment 8637444 [details] MozReview Request: Bug 1186159 - Add an APZ minimap. r=kats Bug 1186159 - Add an APZ minimap. r=kats

Waiting on try

I'd rename the DrawRect and FillRect methods - right now, the "todo" is that it's possibly slow, but only used for debugging, so that's OK. It would be useful to have that reflected in the name (SlowDrawRect, SlowFillRect?) so that it's obvious to somebody looking for a function to use some time from now.

Comment on attachment 8637444 [details] MozReview Request: Bug 1186159 - Add an APZ minimap. r=kats Bug 1186159 - Add an APZ minimap. r=kats

I made the change for DrawRect only since it's the only slow one, the other is a simple wrapper. r?milan since you had comments on the patch.

Comment on attachment 8637444 [details] MozReview Request: Bug 1186159 - Add an APZ minimap. r=kats https://reviewboard.mozilla.org/r/13867/#review12535 Nice

url: https://hg.mozilla.org/integration/mozilla-inbound/rev/38b3f4ab32a55fac48913189b9856a0cb2bf70f4 changeset: 38b3f4ab32a55fac48913189b9856a0cb2bf70f4 user: Benoit Girard <b56girard@gmail.com> date: Thu Jul 23 11:29:22 2015 -0400 description: Bug 1186159 - Add an APZ minimap. r=kats