Closed
Bug 1257315
Opened 8 years ago
Closed 8 years ago
Add support for visualizing visibility to the APZ minimap
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: seth, Assigned: seth)
References
(Blocks 1 open bug)
Details
Attachments
(2 files, 6 obsolete files)
31.96 KB,
patch
|
Details | Diff | Splinter Review | |
10.73 KB,
patch
|
Details | Diff | Splinter Review |
The APZ minimap gives us a useful way to visualize information about the overall page, outside the region we can see in the viewport. This is perfect for visualizing visibility information. Right now the visibility I'm talking about is the image visibility code implemented in nsPresShell.cpp. However, bug 1157546 will expand that code to be able to track any kind of frame. To both ease review of that code and make it easier to verify its correctness, I think it's important that we be able to visualize what that code is doing. This bug will add an additional pref that, when enabled, will visualize this visibility information in the APZ minimap.
Assignee | ||
Comment 1•8 years ago
|
||
Here's the patch. There's a fair amount of code here, but most of it just exists to route the information from the content process to the compositor process. The 'real work' happens almost exclusively in nsPresShell.cpp and ContainerLayerComposite.cpp. I'm requesting review from two people here: - Botond, because I think you're the most familiar with the coordinate spaces and APZ / scrollable stuff that's used here, and after all this patch is implemented according to the recommendations you gave me on IRC. =) - Matt, because I think you're more familiar with the compositor code, and it'd be good to have a second pair of eyes on that stuff.
Attachment #8731402 -
Flags: review?(matt.woodrow)
Attachment #8731402 -
Flags: review?(botond)
Comment 2•8 years ago
|
||
Comment on attachment 8731402 [details] [diff] [review] Add a visualization of visibility tracking to the APZ minimap. Review of attachment 8731402 [details] [diff] [review]: ----------------------------------------------------------------- r+ for most things. A few things in particular where I'm deferring to Matt: - The use of TransformRect() in AddFrameToVisibleRegions(). It looks correct to me, but it doesn't hurt to have an extra pair of eyes on it. - The use of the PCompositor protocol. - Scheduling of composites. I'm not sure how ScheduleComposite() works, but we want to make sure that e.g. receiving an update message for 10 different view IDs doesn't actually schedule 10 composites. ::: gfx/layers/composite/LayerManagerComposite.h @@ +219,5 @@ > { > mInvalidRegion.Or(mInvalidRegion, aRegion); > } > > + void UpdateApproximatlyVisibleRegion(uint64_t aLayersId, typo (Approximatly) @@ +374,5 @@ > gfx::IntRect mTargetBounds; > > nsIntRegion mInvalidRegion; > + > + struct VisibleRegionsKey Another option would be to reuse ScrollableLayerGuid from gfx/layers/FrameMetrics.h, with mPresShellId set to 0. ::: gfx/layers/ipc/PCompositor.ipdl @@ +179,5 @@ > */ > async RequestNotifyAfterRemotePaint(); > > + // The child sends a region containing rects associated with the provided > + // layers ID and view ID that the child considers 'approximately visible'. - Should the word "image" appear somewhere in this description (and perhaps the method name), or is the "visibility" being represented here not specific to images? - It would be more efficient to send all the regions in one message, but as this is a debug aid only, we probably don't care. ::: layout/base/nsPresShell.cpp @@ +5535,5 @@ > + // Retrieve the view ID for this frame (which we obtain from the enclosing > + // scrollable frame). > + nsIScrollableFrame* scrollableFrame = > + nsLayoutUtils::GetNearestScrollableFrame(aFrame, > + nsLayoutUtils::SCROLLABLE_ONLY_ASYNC_SCROLLABLE); You may want to pass in SCROLLABLE_ALWAYS_MATCH_ROOT as well, if you want to see the visible regions on a page that's not scrollable. @@ +5549,5 @@ > + return; > + } > + > + ViewID viewID; > + nsLayoutUtils::FindIDFor(scrollableFrameContent, &viewID); Check the return value of FindIDFor(), returning early if it returns false. ::: layout/base/nsPresShell.h @@ +71,5 @@ > public nsISelectionController, > public nsIObserver, > public nsSupportsWeakReference > { > + template <typename T> using Maybe = mozilla::Maybe<T>; Does "using mozilla::Maybe" not work in this context?
Attachment #8731402 -
Flags: review?(botond) → review+
Assignee | ||
Comment 3•8 years ago
|
||
Thanks for the review! (In reply to Botond Ballo [:botond] from comment #2) > Another option would be to reuse ScrollableLayerGuid from > gfx/layers/FrameMetrics.h, with mPresShellId set to 0. I considered this, but I hesitated exactly because I didn't know what a good value for mPresShellId should be and I wasn't sure that picking an arbitrary value was a good idea. Matt, any opinion here? > - Should the word "image" appear somewhere in this description (and perhaps > the method name), or is the "visibility" being represented here not > specific > to images? It won't be as of the very next patch I'm working on. (Bug 1157546, which this blocks.) > - It would be more efficient to send all the regions in one message, but as > this is a debug aid only, we probably don't care. Yeah, I know. =) It seemed like this would substantially increase the complexity for what is, as you say, a debug-only feature, because AFAICT I'd have to write custom serialization/deserialization code. > Does "using mozilla::Maybe" not work in this context? I didn't think it did, because mozilla::Maybe is not a type by itself, but I have to admit I didn't try it. I'll give it a shot.
Comment 4•8 years ago
|
||
(In reply to Seth Fowler [:seth] [:s2h] from comment #3) > > Does "using mozilla::Maybe" not work in this context? > > I didn't think it did, because mozilla::Maybe is not a type by itself, but I > have to admit I didn't try it. I'll give it a shot. You're right, it doesn't work, although the reason isn't that Maybe is a template; the reason is that using-declarations at class scope are restricted to referring to members of base classes. Not sure why that restriction is in place, but there you go.
Assignee | ||
Comment 5•8 years ago
|
||
(In reply to Seth Fowler [:seth] [:s2h] from comment #3) > I didn't think it did, because mozilla::Maybe is not a type by itself, but I > have to admit I didn't try it. I'll give it a shot. Yeah, it doesn't work. A template alias is necessary.
Assignee | ||
Comment 6•8 years ago
|
||
Here's the updated version of the patch. All of the review comments so far should be addressed except where I noted otherwise in comment 3.
Attachment #8731478 -
Flags: review?(matt.woodrow)
Assignee | ||
Updated•8 years ago
|
Attachment #8731402 -
Attachment is obsolete: true
Attachment #8731402 -
Flags: review?(matt.woodrow)
Comment 7•8 years ago
|
||
(In reply to Seth Fowler [:seth] [:s2h] from comment #3) > (In reply to Botond Ballo [:botond] from comment #2) > > Another option would be to reuse ScrollableLayerGuid from > > gfx/layers/FrameMetrics.h, with mPresShellId set to 0. > > I considered this, but I hesitated exactly because I didn't know what a good > value for mPresShellId should be and I wasn't sure that picking an arbitrary > value was a good idea. I believe that incorporating the pres shell id in ScrollableLayerGuid has been superfluous for a while. I filed bug 1257383 to remove it, which would make ScrollableLayerGuid essentially equivalent to your class (modulo the hashing support, which you'd have to (and be welcome to) add). > > - It would be more efficient to send all the regions in one message, but as > > this is a debug aid only, we probably don't care. > > Yeah, I know. =) It seemed like this would substantially increase the > complexity for what is, as you say, a debug-only feature, because AFAICT I'd > have to write custom serialization/deserialization code. I'm not sure about ns*Hashtable, but at least std::map supports IPC out of the box. (I'm mentioning this for general reference, not suggesting a change.)
Comment 8•8 years ago
|
||
Comment on attachment 8731478 [details] [diff] [review] Add a visualization of visibility tracking to the APZ minimap. Review of attachment 8731478 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/composite/LayerManagerComposite.h @@ +224,5 @@ > + uint64_t aViewId, > + const CSSIntRegion& aRegion) > + { > + CSSIntRegion* regionForLayerAndView = > + mVisibleRegions.LookupOrAdd(VisibleRegionsKey(aLayersId, aViewId)); Nothing ever removes these, right? It might be worth trying to flush them out sometimes to stop them building up. Not a huge deal though, since it should only happen with the pref on. @@ +235,5 @@ > + uint64_t aViewId) > + { > + CSSIntRegion* regionForLayerAndView = > + mVisibleRegions.Get(VisibleRegionsKey(aLayersId, aViewId)); > + return regionForLayerAndView; Might as well just return this immediately instead of using a temp var. ::: layout/base/nsPresShell.cpp @@ +5520,5 @@ > } > } > > +static void > +AddFrameToVisibleRegions(nsIFrame* aFrame, Trailing whitespace.
Attachment #8731478 -
Flags: review?(matt.woodrow) → review+
Assignee | ||
Comment 9•8 years ago
|
||
Thanks for the review! (In reply to Matt Woodrow (:mattwoodrow) from comment #8) > Comment on attachment 8731478 [details] [diff] [review] > Add a visualization of visibility tracking to the APZ minimap. > > Review of attachment 8731478 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: gfx/layers/composite/LayerManagerComposite.h > @@ +224,5 @@ > > + uint64_t aViewId, > > + const CSSIntRegion& aRegion) > > + { > > + CSSIntRegion* regionForLayerAndView = > > + mVisibleRegions.LookupOrAdd(VisibleRegionsKey(aLayersId, aViewId)); > > Nothing ever removes these, right? > > It might be worth trying to flush them out sometimes to stop them building > up. > > Not a huge deal though, since it should only happen with the pref on. Yeah, that is a good point. I'd like to fix that if possible. Presumably when we're sure that a layers ID is dead, we could walk through mVisibleRegions and remove every entry with that layers ID. The thing that's unclear to me is, when can we be sure that a layers ID won't be used anymore?
Flags: needinfo?(matt.woodrow)
Comment 10•8 years ago
|
||
(In reply to Seth Fowler [:seth] [:s2h] from comment #9) > Presumably when we're sure that a layers ID is dead, we could walk through > mVisibleRegions and remove every entry with that layers ID. The thing that's > unclear to me is, when can we be sure that a layers ID won't be used anymore? I don't know the answer to that (Matt probably does), but I think that a layers ID can remain alive for a long time, while an arbitrary number of pages are loaded in it (and the scroll ID keeps going up), so I don't know how much cleaning up after a layers ID goes away will buy us. One advantage of sending all regions for a given layers ID at once, would be that we could just overwrite all the old entries for that layers ID on the compositor side.
Comment 11•8 years ago
|
||
EraseLayerState is where we tear down other data for a given layers ID. We might also want to handle RecvAdoptChild, since that's when a tab gets moved to a new window. I like botonds idea too :)
Flags: needinfo?(matt.woodrow)
Assignee | ||
Comment 12•8 years ago
|
||
So after considering the problem I have come up with the following plan, given that I do want to solve this problem, but I don't want to make things too complex considering this is debug-only: (1) I'll send an IPC message before sending the new contents of the hash table for a given layers ID that will make the CompositorParent remove all the old entries for that layers ID. (2) I'll clean things up in EraseLayerState. I think this should solve the problem nicely without requiring too much additional code. I'll upload a part 2 patch to add these things.
Assignee | ||
Comment 13•8 years ago
|
||
This updated version of the original patch (now renumbered "part 1") addresses Matt's review comments, except for the issue of freeing the visible regions, which for ease of reviewing will be handled in a new part 2.
Assignee | ||
Updated•8 years ago
|
Attachment #8731478 -
Attachment is obsolete: true
Assignee | ||
Comment 14•8 years ago
|
||
This patch implements the plan in comment 12. One thing in particular to pay attention to: I call RecvClearApproximatelyVisibleRegions() with the lock held in EraseLayerState(), because I wasn't sure if it was safe to assume that the CompositorParent pointer would still be valid after calling |sIndirectLayerTrees.erase()|. It seems like it might be, but I wasn't 100% sure, so this seemed safer. If it *is* safe, though, I can move that code after the call to |sIndirectLayerTrees.erase()| and hence outside the lock.
Attachment #8732000 -
Flags: review?(matt.woodrow)
Comment 15•8 years ago
|
||
Comment on attachment 8732000 [details] [diff] [review] (Part 2) - Release old visible regions info when new info is available or a layers ID is no longer used. Review of attachment 8732000 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsPresShell.cpp @@ +5689,5 @@ > return; > } > > + // Clear the old approximately visible regions associated with this layers ID. > + compositorChild->SendClearApproximatelyVisibleRegions(layersId); This function is called by RebuildImageVisibility(), which looks like it calls itself (via MarkImagesInSubtreeVisible()) when descending into a subdocument. Doesn't that mean that we can get into a situation where we'll clear the visible regions we've just set for the parent document, before sending over the ones for a child document?
Assignee | ||
Comment 16•8 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #15) > This function is called by RebuildImageVisibility(), which looks like it > calls itself (via MarkImagesInSubtreeVisible()) when descending into a > subdocument. > > Doesn't that mean that we can get into a situation where we'll clear the > visible regions we've just set for the parent document, before sending over > the ones for a child document? Yes, I suppose so, if the layers ID is not per-document. Still trying to internalize the meaning of this stuff. =) I'll upload a slightly modified version of the patch to solve this problem.
Assignee | ||
Comment 17•8 years ago
|
||
I've updated part 1 to use ScrollableLayerGuid to identify the approximately visible regions in the hashtable, since it turns out that we want to store the pres shell id as well. This is because in part 2, to make sure that we only clear entries associated with the current document when updating the hash table, we need to send over not only the layers ID but also the pres shell ID. Since the hash table key now contains the layers ID, pres shell ID, and view ID, it's identical to a ScrollableLayerGuid and it makes sense to just reuse that.
Attachment #8732451 -
Flags: review?(botond)
Assignee | ||
Updated•8 years ago
|
Attachment #8731998 -
Attachment is obsolete: true
Assignee | ||
Comment 18•8 years ago
|
||
This version of part 2 uses the same strategy as the previous version, except that when clearing the old visible regions in NotifyCompositorOfVisibleRegionsChange(), we send over both the layers ID and the pres shell ID, ensuring that we only clear visible regions associated with the pres shell we're updating visibility for, rather than every pres shell with the same layers ID.
Attachment #8732452 -
Flags: review?(botond)
Assignee | ||
Updated•8 years ago
|
Attachment #8732000 -
Attachment is obsolete: true
Attachment #8732000 -
Flags: review?(matt.woodrow)
Comment 19•8 years ago
|
||
Comment on attachment 8732451 [details] [diff] [review] (Part 1) - Add a visualization of visibility tracking to the APZ minimap. Review of attachment 8732451 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/composite/LayerManagerComposite.h @@ +222,5 @@ > > + void UpdateApproximatelyVisibleRegion(const ScrollableLayerGuid& aGuid, > + const CSSIntRegion& aRegion) > + { > + CSSIntRegion* regionForLayerAndView = mVisibleRegions.LookupOrAdd(aGuid); It makes sense to call this regionForScrollFrame, as that's what a ScrollableLayerGuid identifies.
Attachment #8732451 -
Flags: review?(botond) → review+
Comment 20•8 years ago
|
||
Comment on attachment 8732452 [details] [diff] [review] (Part 2) - Release old visible regions info when new info is available or a layers ID is no longer used. Review of attachment 8732452 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/ipc/CompositorParent.cpp @@ +1784,5 @@ > EraseLayerState(uint64_t aId) > { > MonitorAutoLock lock(*sIndirectLayerTreesLock); > + > + CompositorParent* parent = sIndirectLayerTrees[aId].mParent; This is really nitpicking, but we're doing two lookups in the map (operator[] and erase()) when we could just do one. Here's the way of doing just one: auto iter = sIndirectLayerTrees.find(aId); if (iter != sIndirectLayerTrees.end()) { CompositorParent* parent = iter->second.mParent; if (parent) { parent->Clear...(); } sIndirectLayerTrees.erase(iter); }
Attachment #8732452 -
Flags: review?(botond) → review+
Assignee | ||
Comment 21•8 years ago
|
||
Thanks for the quick review, Botond! This updated version of part 1 addresses the review comments.
Assignee | ||
Updated•8 years ago
|
Attachment #8732451 -
Attachment is obsolete: true
Assignee | ||
Comment 22•8 years ago
|
||
Review comments addressed here too.
Assignee | ||
Updated•8 years ago
|
Attachment #8732452 -
Attachment is obsolete: true
Assignee | ||
Comment 23•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/70a45b0e52860c9deab399bdfb9d59492ccecd24 Bug 1257315 (Part 1) - Add a visualization of visibility tracking to the APZ minimap. r=botond,mattwoodrow https://hg.mozilla.org/integration/mozilla-inbound/rev/f727b5efa34ac5d0bd499f977221a39927dabbc7 Bug 1257315 (Part 2) - Release old visible regions info when new info is available or a layers ID is no longer used. r=botond
Assignee | ||
Comment 24•8 years ago
|
||
Matt, since the current version should be safe I went ahead and landed it, but do you know the answer to the question in comment 14 about the lifetime of the CompositorParent in EraseLayerState()? It'd be nice to move the new code outside of the lock if we want, and if it's safe, I'll do it in a followup bug.
Flags: needinfo?(matt.woodrow)
Comment 25•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/70a45b0e5286 https://hg.mozilla.org/mozilla-central/rev/f727b5efa34a
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Comment 26•8 years ago
|
||
Drive-by comment: these patches send the layers id from the child process to the parent process, which is a security risk because a compromised child process can then start affecting other child processes' layers trees. This was mostly relevant to b2g but will be relevant for desktop some day when we have multiple child processes (or if you manually enable that behavior). Also it's not critical because this is debugging code behind a pref but if you do touch this code again please modify it so that the parent doesn't assume a layers id produced by a child is valid. In general the parent side should be able to either already know the layers id (if using PBrowser) or should be able to verify that the layers id exists in the process that is communicating with it.
Assignee | ||
Comment 27•8 years ago
|
||
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #26) > Drive-by comment: these patches send the layers id from the child process to > the parent process, which is a security risk because a compromised child > process can then start affecting other child processes' layers trees. This > was mostly relevant to b2g but will be relevant for desktop some day when we > have multiple child processes (or if you manually enable that behavior). > Also it's not critical because this is debugging code behind a pref but if > you do touch this code again please modify it so that the parent doesn't > assume a layers id produced by a child is valid. In general the parent side > should be able to either already know the layers id (if using PBrowser) or > should be able to verify that the layers id exists in the process that is > communicating with it. Thanks for letting me know about this issue. I'll file a followup bug and get it fixed.
Comment 28•8 years ago
|
||
(In reply to Seth Fowler [:seth] [:s2h] from comment #14) > Created attachment 8732000 [details] [diff] [review] > (Part 2) - Release old visible regions info when new info is available or a > layers ID is no longer used. > > This patch implements the plan in comment 12. > > One thing in particular to pay attention to: I call > RecvClearApproximatelyVisibleRegions() with the lock held in > EraseLayerState(), > because I wasn't sure if it was safe to assume that the CompositorParent > pointer > would still be valid after calling |sIndirectLayerTrees.erase()|. It seems > like > it might be, but I wasn't 100% sure, so this seemed safer. If it *is* safe, > though, I can move that code after the call to |sIndirectLayerTrees.erase()| > and > hence outside the lock. It should be safe to do so. We can only destroy the CompositorParent from the compositor ipdl thread, which is what you're already on.
Flags: needinfo?(matt.woodrow)
Comment 29•8 years ago
|
||
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #26) > Drive-by comment: these patches send the layers id from the child process to > the parent process, which is a security risk because a compromised child > process can then start affecting other child processes' layers trees. This > was mostly relevant to b2g but will be relevant for desktop some day when we > have multiple child processes (or if you manually enable that behavior). > Also it's not critical because this is debugging code behind a pref but if > you do touch this code again please modify it so that the parent doesn't > assume a layers id produced by a child is valid. In general the parent side > should be able to either already know the layers id (if using PBrowser) or > should be able to verify that the layers id exists in the process that is > communicating with it. Good catch! Seth, it'd be nice to add a clear comment to your fix so that it's obvious for future additions that we need to take this into account.
You need to log in
before you can comment on or make changes to this bug.
Description
•