Kartikaya Gupta (email:kats@mozilla.staktrace.com) Reporter
	Description • 11 years ago

After some discussion with BenWa and botond we came to the conclusion that in order to best solve our displayport woes in bug 947523 for 1.3+ we should change the displayport representation in layout.

Currently the displayport is set as a CSS-pixel rect that is relative to the scroll offset. Instead, we want it to be specified in layer pixels, and to be a set of margins that should be applied to the visible region of the frame.

The assumption we have here is one that tn and I discussed on IRC a few days ago. The assumption is that the user's maximum panning speed is defined in layer pixels / second, which means the displayport margin should be roughly constant in layer pixels irrespective of what zoom level is being used.

We considered a couple of other approaches, such as doing the displayport recomputation more frequently in the APZC code, but that leads to the possibility of race conditions. Storing the displayport as a percentage margin is also not as good because content scripts could arbitrarily change how much of the iframe is visible and result in unexpected changes to the displayport region.

The one downside of storing the displayport as layerpixel margin values is that as more of the iframe becomes visible the displayport will change shape. Enabling tiling should mitigate this problem.

	Ben Kelly [:bkelly, not reviewing]
	Comment 1 • 11 years ago

NOMing this for 1.3? since it addresses the b2g OOM originally identified in bug 947523.  The fix in bug 947523 only addressed v1.2, so we need this to solve the issue for v1.3+.

	Mike Lee [:mlee]
	Comment 2 • 11 years ago

Milan can you have someone on your team look into this?

	Timothy Nikkel (:tnikkel) Assignee
	Comment 3 • 11 years ago

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #0)
> Instead, we want it to be specified in layer pixels, and to
> be a set of margins that should be applied to the visible region of the
> frame.

We'll need to define exactly what we mean by "visible region" at some point.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Reporter
	Comment 4 • 11 years ago

(In reply to Timothy Nikkel (:tn) from comment #3)
> We'll need to define exactly what we mean by "visible region" at some point.

Yeah. In the context of this description I was referring to the region visible to the user. That is, it takes into account the clips of parent scroll frames. BenWa was saying that layout already computes this because without APZC/displayports that's what it ends up painting.

	Benoit Girard (:BenWa)
	Comment 5 • 11 years ago

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #0)
> which means the displayport margin should be roughly
> constant in layer pixels irrespective of what zoom level is being used.

and constant in memory usage.

> 
> Storing the displayport as a percentage
> margin is also not as good because content scripts could arbitrarily change
> how much of the iframe is visible and result in unexpected changes to the
> displayport region.

i.e. make the code fundamentally race-y.


(In reply to Timothy Nikkel (:tn) from comment #3)
> We'll need to define exactly what we mean by "visible region" at some point.

And change layers' visible region to retain region which is really what they are :D

	Timothy Nikkel (:tnikkel) Assignee
	Comment 6 • 11 years ago

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #4)
> (In reply to Timothy Nikkel (:tn) from comment #3)
> > We'll need to define exactly what we mean by "visible region" at some point.
> 
> Yeah. In the context of this description I was referring to the region
> visible to the user. That is, it takes into account the clips of parent
> scroll frames. BenWa was saying that layout already computes this because
> without APZC/displayports that's what it ends up painting.

Layout computes it if displayports aren't set, but if they are then we won't take into account the root scroll frame's clip for example, so anything in the display port will be included in this definition of visible.

	Milan Sreckovic [:milan] (needinfo for best results)
	Comment 7 • 11 years ago

Jet, can you send us somebody to help with this?

	Markus Stange [:mstange]
	Comment 8 • 11 years ago

Jet asked me to add my thought on this, so here they are:

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #4)
> (In reply to Timothy Nikkel (:tn) from comment #3)
> > We'll need to define exactly what we mean by "visible region" at some point.
> 
> Yeah. In the context of this description I was referring to the region
> visible to the user. That is, it takes into account the clips of parent
> scroll frames. BenWa was saying that layout already computes this because
> without APZC/displayports that's what it ends up painting.

What layout computes is actually a bit more than that. For example, when an opaque element covers parts of the scrollable subframe, that part will also be removed from the visible region. So the visible region can have a more complex shape than just a single rect (which is what you'd get if you only did the ancestor scrollframe clipping).

(In reply to Timothy Nikkel (:tn) from comment #6)
> Layout computes it if displayports aren't set, but if they are then we won't
> take into account the root scroll frame's clip for example, so anything in
> the display port will be included in this definition of visible.

So then this is the code we'd change, right? Instead of overriding the visible region with a display port, we'd take the computed visible region, maybe simplify it to its bounding rect, then add the layerpixel margins, and propagate that result to the scrolled contents. And in case we have tiling, we'd also do the expand-to-tiles calculation here. At the moment that happens just before APZCCallbackHelper sets the display port.

Two more questions come to mind:
1. What if the visible region of a scrollable subframe is empty, for example because the it is scrolled out of view or completely covered? I think we should keep the display port empty in that case, too. Then we won't waste any memory on invisible scroll frames, which is definitely desirable.
2. When determining whether nested scrollable frames are visible, should we consult its actual visible region or the one that has already been subjected to the margin expansion by the outer scroll frame? I'd argue for the latter: If the outer frame's prerender margins include parts of a scrollable subframe, that should get its contents prerendered, too, so that if you pan to it asynchronously you can start scrolling it right away without waiting for the main thread to catch up. It also maps to the code in a much more natural way.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Reporter
	Comment 9 • 11 years ago

(In reply to Markus Stange [:mstange] from comment #8)
> What layout computes is actually a bit more than that. For example, when an
> opaque element covers parts of the scrollable subframe, that part will also
> be removed from the visible region. So the visible region can have a more
> complex shape than just a single rect (which is what you'd get if you only
> did the ancestor scrollframe clipping).

Ah, I didn't know that. Thanks for the info! Everything else you said makes perfect sense to me as well. I might bug you a bit more later for pointers to code as to where these changes need to be made.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Reporter
	Comment 10 • 11 years ago

I'm going to start looking at this bug although I might have to hand it off to somebody in layout later.

	Doug Sherk (:drs) (inactive)
	Comment 11 • 11 years ago

This is a temporary fix which makes the browser and other things usable for me again on Hamachi. This should not be pushed to the tree unless all else fails, and I won't be pushing it following r+. kats has better plans for dealing with this which we should go with instead.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Reporter
	Comment 12 • 11 years ago

Comment on attachment 8361851 [details] [diff] [review]
Shrink APZC margins to reduce OOMing on B2G

Review of attachment 8361851 [details] [diff] [review]:
-----------------------------------------------------------------

I'd rather change this in the b2g prefs file if it comes to that. That way metro won't be affected.

	Timothy Nikkel (:tnikkel) Assignee
	Comment 13 • 11 years ago

Talked to kats about this today some. For the rect to apply the margins to I think we want to use the intersection of the passed in clip to ScrollFrameHelper::BuildDisplayList, with the dirty rect, and the scrollport rect. If we were to use the visible rect of the display item or the visible region of the layer that would be more complex (the visible rect/region depend on the geometry of display items, but determining the displayport determines the geometry of some of the display items, so it is sort of a cycle dependency, it could probably be resolved, but I'm not sure the added complexity is worth it). I don't think opaque items covering part of a scroll frame affecting the displayport is going to be an important case. If it turns out to be we can always go with the more complicated solution later.

We should store this rect somewhere so all users of displayport can access it. If we only need it during display list processing/construction we can store it on the display item. If we need it outside of those times we would probably have to store it as a property. Since display lists are build for more than just painting to the screen (drawwindow, hit testing) we'd want to make sure the stored rect from one type doesn't affect the painting to the screen display list.

	Timothy Nikkel (:tnikkel) Assignee
	Comment 14 • 11 years ago

(In reply to Timothy Nikkel (:tn) from comment #13)
> For the rect to apply the margins to I
> think we want to use the intersection of the passed in clip to
> ScrollFrameHelper::BuildDisplayList, with the dirty rect, and the scrollport
> rect.

And this might be different from root scroll frames of root content documents. In those we might want to use the scroll position clamping scroll port size (we should probably rename this, it's being use for more than just that one purpose now) instead of the scrollport.

	Timothy Nikkel (:tnikkel) Assignee
	Comment 15 • 11 years ago

Looks like IsRectNearlyVisible (via UpdateImageVisibilityForFrame) need the display port, and that needs to work completely outside of anything display list based, so we'll need to store the property outside of the display item. On the frame, or perhaps on the content (alongside where we will store the displayport margins?).

	Timothy Nikkel (:tnikkel) Assignee
	Comment 16 • 11 years ago

IsRectNearlyVisible needs to work on pages that haven't painted (background tabs), so we will either need to give it a way to compute the displayport from the displayport margins, or base it off something it can compute easily.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Reporter
	Comment 17 • 11 years ago

For IsRectNearlyVisible, can we not just tack on the margins to the mScrollPort there?

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Reporter
	Comment 22 • 11 years ago

There are some problems in this patch but it's what I started with

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Reporter
	Comment 23 • 11 years ago

I have no idea what the proper changes for these bits will look like.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Reporter
	Comment 24 • 11 years ago

Comment on attachment 8364675 [details] [diff] [review]
Part 5 (WIP) - Updates to layout

I'm sure there's stuff in here that's obviously wrong to you. Please let me know what it is.

	Timothy Nikkel (:tnikkel) Assignee
	Comment 25 • 11 years ago

Comment on attachment 8364675 [details] [diff] [review]
Part 5 (WIP) - Updates to layout

So the resulting displayport computing in the nsDisplayList.cpp changes is based off mVisibleRect, which is the result of ComputeVisibility, ie opaquely covered areas are removed from it. So we will get a different result from when we compute the displayport in for example nsGfxScrollFrame.cpp. So I think we will need to store the base rect that we use to expand to create the display port. That works for scroll layer items. The PaintForFrame case is a little different. If we could use the dirty rect from it's caller, nsLayoutUtils::PaintFrame, that might work.

Otherwise looks good.

	Peter Dolanjski [:pdol]
	Comment 26 • 11 years ago

What is the user impact of not fixing this in 1.3?  We're hitting our deadlines, so just trying to ascertain if this is vital to fix.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Reporter
	Comment 27 • 11 years ago

Probably the main user-visible impact is bug 947523. That was fixed for 1.2 but this bug should fix it properly for 1.3 and up. There are also other less user-visible impacts, such as more efficient use of memory and possibly less checkerboarding with APZ panning.

	Jason Smith [:jsmith]
	Comment 28 • 11 years ago

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #27)
> Probably the main user-visible impact is bug 947523. That was fixed for 1.2
> but this bug should fix it properly for 1.3 and up. There are also other
> less user-visible impacts, such as more efficient use of memory and possibly
> less checkerboarding with APZ panning.

I'm curious to see where we stand actually with 1.3 with that bug to see if the impact is still the same.

QA Wanted - Can someone retest the STR on bug 947523 & indicate what happens here on 1.3?

	Peter Bylenga [:PBylenga]
	Comment 29 • 11 years ago

I restart OS crashed after reproducing the STRs on bug 947523 on the latest nightly b2g28 branch of v1.3 which is identical to the actual results of that issue.

v1.3 Environmental Variables:
Device: Buri v1.3 MOZ
BuildID: 20140203181708
Gaia: bb36b4164d3e51ca04b612e507e1178f80bf240d
Gecko: 9731b0b7fa78
Version: 28.0
Firmware Version: v1.2-device.cfg

	Kai-Chih Hu [:khu]
	Comment 30 • 11 years ago

Can we get ETA to fix this bug? Thank you.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Reporter
	Comment 38 • 11 years ago

The mochitest in here let me test this code on desktop much more easily than I could test it on a device, so I'm including it here.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Reporter
	Comment 39 • 11 years ago

Handing this off to :tn for now since I've been too busy with other APZ things to make much progress here. Plus it's really slow going for me because I get don't understand this code as much, so hopefully tn can make more progress here.

The attached patches are my latest WIPs. Part 5 I'm not really confident is right but it was the best I could come up with; part 6 is just a hack because I don't know what the right rect to be using at the call sites there is so I just commented it out. Part 7 I think is mostly correct but might need some debugging. I was using the mochitest + printfs in part 8 to verify that the displayport was getting propagated properly and expanded correctly in various places. If bug 961289 lands then the stuff in part 8 could actually be turned into a proper test and landed.

	Jason Smith [:jsmith]
	Comment 40 • 11 years ago

The size of the work involved here looks really risky for this point in the release, so I think we need to re-evaluate the blocking decision here. We know that we could consider porting bug 947523 to 1.3 to resolve bug 965945, so that could resolve that blocker. bug 942750 is also more of a meta bug, so unblocking this allows that bug to be finished after QA verification of the current state of checkboarding.

	Jason Smith [:jsmith]
	Comment 41 • 11 years ago

An offline discussion with Milan concluded that this is too risky to take to 1.3 at this point, so I'm punting this to 1.4 triage.

	Chris Lord [:cwiiis]
	Comment 42 • 11 years ago

If APZC is being turned on in 1.3 but we don't have this, we likely want to add a temporary max displayport size, based on screen resolution perhaps. Otherwise we risk having apps crash after a user zooms in (when zooming is enabled, of course).

	Botond Ballo [:botond]
	Comment 43 • 11 years ago

(In reply to Chris Lord [:cwiiis] from comment #42)
> If APZC is being turned on in 1.3 but we don't have this, we likely want to
> add a temporary max displayport size, based on screen resolution perhaps.
> Otherwise we risk having apps crash after a user zooms in (when zooming is
> enabled, of course).

I think Kats already did this in bug 965945, and then did it better (no crashes) in bug 975033.

	Chris Lord [:cwiiis]
	Comment 44 • 11 years ago

(In reply to Botond Ballo [:botond] from comment #43)
> (In reply to Chris Lord [:cwiiis] from comment #42)
> > If APZC is being turned on in 1.3 but we don't have this, we likely want to
> > add a temporary max displayport size, based on screen resolution perhaps.
> > Otherwise we risk having apps crash after a user zooms in (when zooming is
> > enabled, of course).
> 
> I think Kats already did this in bug 965945, and then did it better (no
> crashes) in bug 975033.

hmm, not quite... That disallows displayports bigger than 4096x4096, but a displayport of that size could require as much as 128mb of video memory if double-buffered, which is likely more than is going to be available on any device we're shipping on for a good while yet (the Peak, for example, has 128mb of ram total available for video memory).

I think it'd be better to have a lower limit and rather than just remove the displayport in that case, make sure it stays within those bounds. At least for the tiles case anyway.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Reporter
	Comment 45 • 11 years ago

For bug posterity, Chris filed bug 979084 for the issue in comments 42-44.

	Timothy Nikkel (:tnikkel) Assignee
	Comment 46 • 10 years ago

Just using layer margins won't solve this bug. I outlined why in bug 982596, comment 0, which is the bug I filed with patches to do the other piece of work we'll need to not OOM.

	Timothy Nikkel (:tnikkel) Assignee
	Comment 47 • 10 years ago

Summary of this bug: there is a problem when a scrollable element other than the root has a display port, if the user pinch zooms the page the resolution will be increased. Since displayports are set in css pixels this means the displayport size for the scrollable elements increase by the same factor, and this can easily lead to displayports that are very large in actual pixel size. Leading to OOM.

There is another related problem with the way APZC controller calculates displayports. It does so as a multiplier based on the composition bounds. For scrollable elements that is the amount of content that it will allow visible on the screen at any one time. However scrollable elements can be arbitrarily tall, so this calculation just isn't reasonable, and needs to be changed.

We have some hacks that hardcode max sizes and a few other things to try to prevent these OOMs from occuring but they don't fix all the cases. kats is the author of these and would know how prevalent these problems still are with these hacks in place.

	Chris Lord [:cwiiis]
	Comment 48 • 10 years ago

(In reply to Timothy Nikkel (:tn) from comment #47)
> There is another related problem with the way APZC controller calculates
> displayports. It does so as a multiplier based on the composition bounds.
> For scrollable elements that is the amount of content that it will allow
> visible on the screen at any one time. However scrollable elements can be
> arbitrarily tall, so this calculation just isn't reasonable, and needs to be
> changed.

This part isn't true - the composition bounds are the amount of content that is visible on the screen, and are bound by the widget size, which is eventually bound by the screen size. The values we've chosen as multipliers work ok based on the screen/memory sizes of the devices we have at the moment.

	Timothy Nikkel (:tnikkel) Assignee
	Comment 49 • 10 years ago

(In reply to Chris Lord [:cwiiis] from comment #48)
> (In reply to Timothy Nikkel (:tn) from comment #47)
> > There is another related problem with the way APZC controller calculates
> > displayports. It does so as a multiplier based on the composition bounds.
> > For scrollable elements that is the amount of content that it will allow
> > visible on the screen at any one time. However scrollable elements can be
> > arbitrarily tall, so this calculation just isn't reasonable, and needs to be
> > changed.
> 
> This part isn't true - the composition bounds are the amount of content that
> is visible on the screen, and are bound by the widget size, which is
> eventually bound by the screen size. The values we've chosen as multipliers
> work ok based on the screen/memory sizes of the devices we have at the
> moment.

For the composition bounds for the root scroll frame what you say is true, but for composition bounds for any other element they are just the size of the frame, which is controlled purely via css, so it can be arbitrarily big. This is why the problem only applies to other scrollable elements, and not the root.

	Chris Lord [:cwiiis]
	Comment 50 • 10 years ago

(In reply to Timothy Nikkel (:tn) from comment #49)
> (In reply to Chris Lord [:cwiiis] from comment #48)
> > (In reply to Timothy Nikkel (:tn) from comment #47)
> > > There is another related problem with the way APZC controller calculates
> > > displayports. It does so as a multiplier based on the composition bounds.
> > > For scrollable elements that is the amount of content that it will allow
> > > visible on the screen at any one time. However scrollable elements can be
> > > arbitrarily tall, so this calculation just isn't reasonable, and needs to be
> > > changed.
> > 
> > This part isn't true - the composition bounds are the amount of content that
> > is visible on the screen, and are bound by the widget size, which is
> > eventually bound by the screen size. The values we've chosen as multipliers
> > work ok based on the screen/memory sizes of the devices we have at the
> > moment.
> 
> For the composition bounds for the root scroll frame what you say is true,
> but for composition bounds for any other element they are just the size of
> the frame, which is controlled purely via css, so it can be arbitrarily big.
> This is why the problem only applies to other scrollable elements, and not
> the root.

Oh, interesting - what I said is, as I understand it, how it was intended to be. I guess that would still be problematic when you involve transforms, but that's certainly a problem then.

	Timothy Nikkel (:tnikkel) Assignee
	Comment 51 • 10 years ago

(In reply to Chris Lord [:cwiiis] from comment #50)
> Oh, interesting - what I said is, as I understand it, how it was intended to
> be. I guess that would still be problematic when you involve transforms, but
> that's certainly a problem then.

You don't even need transforms: <div style="height: 100000px; overflow: scroll;"> is enough.

	Chris Lord [:cwiiis]
	Comment 52 • 10 years ago

(In reply to Timothy Nikkel (:tn) from comment #51)
> (In reply to Chris Lord [:cwiiis] from comment #50)
> > Oh, interesting - what I said is, as I understand it, how it was intended to
> > be. I guess that would still be problematic when you involve transforms, but
> > that's certainly a problem then.
> 
> You don't even need transforms: <div style="height: 100000px; overflow:
> scroll;"> is enough.

I meant if it worked as it was intended (recursing and clipping on parents' composition bounds), you could still trigger this with transforms.

	Chris Lord [:cwiiis]
	Comment 53 • 10 years ago

(In reply to Chris Lord [:cwiiis] from comment #52)
> I meant if it worked as it was intended (recursing and clipping on parents'
> composition bounds), you could still trigger this with transforms.

This is easily fixed by taking the min of the composition bounds and the client size though.

	Timothy Nikkel (:tnikkel) Assignee
	Comment 54 • 10 years ago

Anyways, what you describe as the intended approach is what I basically did in bug 982596, by leaving the composition bounds alone and adding a new field. However that approach got rebuffed in that bug.

	Timothy Nikkel (:tnikkel) Assignee
	Comment 55 • 10 years ago

Current plan:
1) change how APZC calculates displayport. cap the maximize size of composition bounds we use to the composition bounds of the root APZC. This is the maximum amount of content that we can scroll into view quickly. But since the display port calculation has been tuned for root scroll frames (it scales based on the screen size only essentially) this just basically decides on layer margins using only the current velocity and screen size using the already tuned code extended logically to non-root scrollables. If we want to throw out our display port calculation code and start anew we can do that as well, and can easily be done separately once we have something that works reasonable.

2) Convert representation of displayports to layer margins + base rect. I think the api needs to be able to provide a display port rect to consumers, it doesn't make sense for every consumer to try to pull a base rect out of somewhere to apply margins to, it's too inconvenient. So APZC can set an initial base rect to the composition bounds (or something) just so there is some base rect, and then layout will update it properly every time it paints (the "visible" rect). So I don't have to update every display port user I'll add this margin api in parallel with the rect one, making the getting prefer the stored margin value if both are set. We can remove the old api when all uses are converted.

You may all now proceed to tear this proposal apart. :)

	Markus Stange [:mstange]
	Comment 56 • 10 years ago

Your proposal sounds very reasonable to me, fwiw.

	Timothy Nikkel (:tnikkel) Assignee
	Comment 57 • 10 years ago

Filed bug 986413 for part 1) of comment 55.

	Timothy Nikkel (:tnikkel) Assignee
	Comment 60 • 10 years ago

This also contains a bunch of odds and ends that should be in other patches.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Reporter
	Comment 61 • 10 years ago

Comment on attachment 8396316 [details] [diff] [review]
the layer margins api

Review of attachment 8396316 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsDOMWindowUtils.cpp
@@ +312,5 @@
>      static_cast<DisplayPortPropertyData*>(aPropertyValue);
>    delete data;
>  }
>  
> +static void DestroyDisplayPortMarginsPropertyData(void* aObject, nsIAtom* aPropertyName,

Bug 983285 just landed on inbound this morning when you pick that up you can replace this function with using the template function.

::: layout/base/nsLayoutUtils.cpp
@@ +684,5 @@
> +      }
> +
> +      nsIFrame* frame = aContent->GetPrimaryFrame();
> +      if (frame) {
> +        // first conver the base rect to layer pixels

s/conver/convert/

@@ +706,5 @@
> +            marginsData->mAlignment * floor(rect.y / marginsData->mAlignment);
> +          float right =
> +            marginsData->mAlignment * floor(rect.XMost() / marginsData->mAlignment);
> +          float bottom =
> +            marginsData->mAlignment * floor(rect.YMost() / marginsData->mAlignment);

Probably want s/floor/ceil/ for the right and bottom values?

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Reporter
	Comment 62 • 10 years ago

Comment on attachment 8396317 [details] [diff] [review]
set the display port base rect

Review of attachment 8396317 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsLayoutUtils.cpp
@@ +724,5 @@
>    return true;
>  }
>  
> +static void DestroyNsRect(void* aObject, nsIAtom* aPropertyName,
> +                          void* aPropertyValue, void* aData)

Won't need this function either with bug 983285.

@@ +2311,5 @@
>      }
>    }
>  
> +  if (usingDisplayPort && (aFlags & PAINT_WIDGET_LAYERS)) {
> +    visibleRegion = displayport;    

nit: trailing whitespace

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Reporter
	Comment 63 • 10 years ago

Comment on attachment 8396319 [details] [diff] [review]
set the display port in layer margins

Review of attachment 8396319 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsSubDocumentFrame.cpp
@@ +386,5 @@
>        haveDisplayPort = true;
> +      // for root content documents we want the base to be the composition bounds
> +      nsLayoutUtils::SetDisplayPortBase(rootScrollFrame->GetContent(),
> +        presContext->IsRootContentDocument() ? 
> +          CalculateCompositionSizeForFrame(nsRect(nsPoint(0,0), rootScrollFrame) :

missed qref? CalculateCompositionSizeForFrame is in nsLayoutUtils and has a different signature.

::: widget/xpwidgets/APZCCallbackHelper.cpp
@@ +93,5 @@
> +          aFrameMetrics.LayersPixelsPerCSSPixel();
> +      LayerMargin margins = aFrameMetrics.GetDisplayPortMargins();
> +      margins.left -= shift.x;
> +      margins.right += shift.x;
> +      margins.top -= shift.y;

I don't know if this makes sense. The margins are relative anyway so I don't think they need to be shifted at all. If anything it's the base rect that would need to be shifted?

@@ +244,5 @@
> +        nsRect base(baseCSS.x * nsPresContext::AppUnitsPerCSSPixel(),
> +                    baseCSS.y * nsPresContext::AppUnitsPerCSSPixel(),
> +                    baseCSS.width * nsPresContext::AppUnitsPerCSSPixel(),
> +                    baseCSS.height * nsPresContext::AppUnitsPerCSSPixel());
> +        nsLayoutUtils::SetDisplayPortBase(content, base);

Might be better to change SetDisplayPortBase to take CSS pixels

	Timothy Nikkel (:tnikkel) Assignee
	Comment 64 • 10 years ago

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #63)
> ::: widget/xpwidgets/APZCCallbackHelper.cpp
> @@ +93,5 @@
> > +          aFrameMetrics.LayersPixelsPerCSSPixel();
> > +      LayerMargin margins = aFrameMetrics.GetDisplayPortMargins();
> > +      margins.left -= shift.x;
> > +      margins.right += shift.x;
> > +      margins.top -= shift.y;
> 
> I don't know if this makes sense. The margins are relative anyway so I don't
> think they need to be shifted at all. If anything it's the base rect that
> would need to be shifted?

I think it's mathematically the same thing as what we do with rect representation of display port. For example if we have a 100x100 scrollable element (all of it visible) scrolled 1000 pixels down and let's say the difference between scroll offsets is 10, and we have a display port that has 100 extra pixels above and below.

With a rect display port we start with (0,-100)x(100,300) ((x,y)x(w,h)), shift by 10 to get (0,-90)x(100,300).

With margins we have (100,0,100,0) ((t,r,b,l)) which applied to a 100x100 base rect give the same before rect. Shift by 10 the margins are now (90,0,110,0), applied to the same base rect we now get (0,-90)x(100,300).

I don't think it makes sense to shift the base rect. The base rect is the visible part of the scrollable element, and it doesn't change when the element is scrolled.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Reporter
	Comment 65 • 10 years ago

Ok, makes sense

	Timothy Nikkel (:tnikkel) Assignee
	Comment 66 • 10 years ago

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #63)
> Might be better to change SetDisplayPortBase to take CSS pixels

Several other call sites have app units handy. So it's a tossup.

Fixed all your other comments.

	Timothy Nikkel (:tnikkel) Assignee
	Comment 67 • 10 years ago

These will be used later, but I separated them out.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Reporter
	Comment 71 • 10 years ago

Comment on attachment 8397156 [details] [diff] [review]
Part 2, add and implement new layer margins api for display port.

Review of attachment 8397156 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsDOMWindowUtils.cpp
@@ +423,5 @@
> +  // LayerMargin constructor.
> +  LayerMargin displayportMargins(aTopMargin,
> +                                 aRightMargin,
> +                                 aBottomMargin,
> +                                 aLeftMargin);

Doesn't really matter but you can move this down to just before it's used.

@@ +451,5 @@
> +                       nsINode::DeleteProperty<DisplayPortMarginsPropertyData>);
> +
> +  nsIFrame* rootScrollFrame = presShell->GetRootScrollFrame();
> +  if (rootScrollFrame) {
> +    if (content == rootScrollFrame->GetContent()) {

Combine the two if conditions with &&

::: layout/base/nsLayoutUtils.cpp
@@ +699,5 @@
> +        gfxSize res = presContext->PresShell()->GetCumulativeResolution();
> +        gfxSize parentRes = res;
> +        if (isRoot) {
> +          gfxSize localRes = presContext->PresShell()->GetResolution();
> +          parentRes.width /= localRes.width;

Add a comment as to why this needs to be divided out.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Reporter
	Comment 72 • 10 years ago

Comment on attachment 8397183 [details] [diff] [review]
Part 4, compute and set display ports in layer margins.

Review of attachment 8397183 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/xpwidgets/APZCCallbackHelper.cpp
@@ +95,5 @@
> +      margins.left -= shift.x;
> +      margins.right += shift.x;
> +      margins.top -= shift.y;
> +      margins.bottom += shift.y;
> +      aFrameMetrics.SetDisplayPortMargins(margins);     

nit: trailing ws

@@ +109,5 @@
> +        aFrameMetrics.mDisplayPort.y = (compositionBounds.height - aFrameMetrics.mDisplayPort.height) / 2;
> +    } else {
> +        LayerMargin margins = aFrameMetrics.GetDisplayPortMargins();
> +        margins.right = margins.left = margins.LeftRight()/2;
> +        margins.top = margins.bottom = margins.TopBottom()/2;

nit: spaces around '/'

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Reporter
	Comment 73 • 10 years ago
try

I pushed the changes from this bug (with my nits fixed) and bug 986413 to try:

https://tbpl.mozilla.org/?tree=Try&rev=0992f3e840bb

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Reporter
	Comment 74 • 10 years ago
try

Now with build fix for Android (goes in part 1 of this bug's patches):

https://tbpl.mozilla.org/?tree=Try&rev=2fec4ac13ac7

	Timothy Nikkel (:tnikkel) Assignee
	Comment 75 • 10 years ago

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #71)
> > +  // LayerMargin constructor.
> > +  LayerMargin displayportMargins(aTopMargin,
> > +                                 aRightMargin,
> > +                                 aBottomMargin,
> > +                                 aLeftMargin);
> 
> Doesn't really matter but you can move this down to just before it's used.
> 
> @@ +451,5 @@
> > +                       nsINode::DeleteProperty<DisplayPortMarginsPropertyData>);
> > +
> > +  nsIFrame* rootScrollFrame = presShell->GetRootScrollFrame();
> > +  if (rootScrollFrame) {
> > +    if (content == rootScrollFrame->GetContent()) {
> 
> Combine the two if conditions with &&

The regular displayport rect setter has these same problems, which is why I didn't fix them, just kept them in sync. It would have made the patch less clear. A separate cleanup patch would be the ideal way. I'll post a part 2.1.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 77 • 10 years ago

Comment on attachment 8397179 [details] [diff] [review]
Part 3, set display port base rect in layout when painting.

Review of attachment 8397179 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsLayoutUtils.cpp
@@ +763,5 @@
> +void
> +nsLayoutUtils::SetDisplayPortBase(nsIContent* aContent, const nsRect& aBase)
> +{
> +  aContent->SetProperty(nsGkAtoms::DisplayPortBase, new nsRect(aBase),
> +                        nsINode::DeleteProperty<nsRect>); 

trailing whitespace

::: layout/generic/nsSubDocumentFrame.cpp
@@ +386,4 @@
>        haveDisplayPort = true;
> +      // for root content documents we want the base to be the composition bounds
> +      nsLayoutUtils::SetDisplayPortBase(rootScrollFrame->GetContent(),
> +        presContext->IsRootContentDocument() ? 

trailing whitespace

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Reporter
	Comment 78 • 10 years ago

Landed on inbound:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/40651e2d3cbc
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8b0da7b8afc6
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/4a75c46797d1
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f9aee922c83b
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/77ca7f1eda03

	Timothy Nikkel (:tnikkel) Assignee
	Comment 79 • 10 years ago

Added the comment kats asked for
https://hg.mozilla.org/integration/mozilla-inbound/rev/b0dbbb5d770a

	Timothy Nikkel (:tnikkel) Assignee
	Comment 80 • 10 years ago

And apparently we can get a null frame in GetDisplayPort, which seems a little odd, so I'm going to look into it more, but for now I pushed a null check to fix Cipc
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed1595ba411b

	Carsten Book [:Tomcat]
	Comment 81 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/40651e2d3cbc
https://hg.mozilla.org/mozilla-central/rev/8b0da7b8afc6
https://hg.mozilla.org/mozilla-central/rev/4a75c46797d1
https://hg.mozilla.org/mozilla-central/rev/f9aee922c83b
https://hg.mozilla.org/mozilla-central/rev/77ca7f1eda03
https://hg.mozilla.org/mozilla-central/rev/b0dbbb5d770a
https://hg.mozilla.org/mozilla-central/rev/ed1595ba411b

	Timothy Nikkel (:tnikkel) Assignee
	Comment 83 • 10 years ago

(In reply to Timothy Nikkel (:tn) from comment #80)
> And apparently we can get a null frame in GetDisplayPort, which seems a
> little odd, so I'm going to look into it more, but for now I pushed a null
> check to fix Cipc
> https://hg.mozilla.org/integration/mozilla-inbound/rev/ed1595ba411b

This was a unique corner case. Normally we'd construct a scroll frame (with content the root element), then a canvas frame as a child (with content the root element), and then finally the primary frame of the root element, whether it's a block or other. If the root element is display none or an invalid root svg node then we don't construct this last frame. Leaving us with two frames for the root element, none of hem being the primary frame for it. The fix I landed should be sufficient for this case.

	Milan Sreckovic [:milan] (needinfo for best results)
	Comment 84 • 10 years ago

No-Jun, can you test this on the "second" B2G build today, it should have it.

	Ryan VanderMeulen [:RyanVM]
	Comment 85 • 10 years ago

This uplift requires more rebasing for Aurora than I'm comfortable doing.

	Timothy Nikkel (:tnikkel) Assignee
	Comment 86 • 10 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/80984c5e9b3a
https://hg.mozilla.org/releases/mozilla-aurora/rev/156c60792f97
https://hg.mozilla.org/releases/mozilla-aurora/rev/b730b23743d3
https://hg.mozilla.org/releases/mozilla-aurora/rev/acfec8a966e3
https://hg.mozilla.org/releases/mozilla-aurora/rev/1e6ec67b5d99

	No-Jun Park [:njpark]
	Comment 87 • 10 years ago

I was testing following master branch on buri, and tried following STR for 947523, and I got "Well this is embarrasing" message of "the page is not responding" from firefox browser. Not sure whether this is related to this fix though. I visited other sites with iframes and scrollable divs such as groupon.com and http://nunzioweb.com/iframes-example.htm, but did not see any painting issues.

Gaia      4cf7ec04b82320a1ff1f825220204f454f951a87
Gecko     https://hg.mozilla.org/mozilla-central/rev/1c23863d8dda
BuildID   20140330160201
Version   31.0a1

Repro Steps:
1)  Updated Buri to Build ID: 20131206040203
2)  Tap on the "Browser" icon.
3)  Navigate to https://etherpad.mozilla.org
4)  Tap on "Create new public pad".
5)  Keep zooming in on the webpage as far as the user can go. If the crash doesn't occur keeping zooming in on the webpage while changing the phone's orientation.

Logcat attached below

	No-Jun Park [:njpark]
	Comment 88 • 10 years ago

When the error message is shown on the phone, the logcat shows following message:

I/Gecko   (  136): ###!!! [Parent][MessageChannel] Error: Channel error: cannot send/recv
I/Gecko   (  136):

	No-Jun Park [:njpark]
	Comment 89 • 10 years ago

(continue from comment 88)
I/Gecko   (  136): ###!!! [Parent][MessageChannel] Error: Channel error: cannot send/recv
I/Gecko   (  136): 
E/OomLogger(  136): [Kill]: select 977 (Browser), adj 2, size 21334, to kill
E/OomLogger(  136): [Kill]: send sigkill to 977 (Browser), adj 2, size 21334
E/QCALOG  (  194): [MessageQ] ProcessNewMessage: [XTWiFi-PE] unknown deliver target [OS-Agent]
E/QCALOG  (  194): [MessageQ] ProcessNewMessage: [XTWWAN-PE] unknown deliver target [OS-Agent]
E/QCALOG  (  194): [MessageQ] ProcessNewMessage: [XT-CS] unknown deliver target [OS-Agent]
I/GonkMemoryPressure(  989): Checking to see if memory pressure is over.
I/GonkMemoryPressure(  989): Memory pressure is over.
I/GonkMemoryPressure( 1051): Checking to see if memory pressure is over.
I/GonkMemoryPressure( 1051): Memory pressure is over.
I/GonkMemoryPressure(  136): Checking to see if memory pressure is over.
I/GonkMemoryPressure(  136): Memory pressure is over.
I/Diag_Lib(  139): rpc_handle_rpc_call: for Xid: 2fe, Prog: 31000000, Vers: d17ed9ea, Proc: 00000014
I/ONCRPC  (  139): oncrpc_proxy_handle_cmd_rpc_call: Dispatching xid: 2fe
I/Diag_Lib(  139): rpc_handle_rpc_call: Find Status: 0 Xid: 2fe
D/QCRIL_RPC(  139): Enter qcril_cm_srvsys_event_callback
D/QCRIL_RPC(  139): Exit qcril_cm_srvsys_event_callback
I/ONCRPC  (  139): oncrpc_xdr_reply_msg_start: Prog: 00000000, Ver: 00000000, Proc: 00000000 Xid: 000002fe
I/ONCRPC  (  139): oncrpc_msg_reply: Prog: 00000000, Ver: 00000000, Proc: 00000000 Xid: 000002fe
I/ONCRPC  (  139): oncrpc_proxy_handle_cmd_rpc_call: Dispatch returned for xid: 2fe
I/Gecko   (  136): 
I/Gecko   (  136): ###!!! [Parent][MessageChannel] Error: Channel error: cannot send/recv
I/Gecko   (  136): 
E/QCALOG  (  194): [MessageQ] ProcessNewMessage: [XTWiFi-PE] unknown deliver target [OS-Agent]
E/QCALOG  (  194): [MessageQ] ProcessNewMessage: [XT-CS] unknown deliver target [OS-Agent]
E/QCALOG  (  194): [MessageQ] ProcessNewMessage: [XTWWAN-PE] unknown deliver target [OS-Agent]
I/Gonk    (  136): Setting nice for pid 1051 to 18
I/Gonk    (  136): Changed nice for pid 1051 from 18 to 18.
I/Gecko   ( 1051): ###################################### forms.js loaded
I/Gecko   ( 1051): ############################### browserElementPanning.js loaded
I/Gecko   ( 1051): ######################## BrowserElementChildPreload.js loaded
E/memalloc(  136): /dev/pmem: No more pmem available
W/memalloc(  136): Falling back to ashmem
D/skia    ( 1051): START /proc/cpuinfo:
D/skia    ( 1051): Processor	: ARMv7 Processor rev 1 (v7l)
D/skia    ( 1051): processor	: 0
D/skia    ( 1051): BogoMIPS	: 501.64
D/skia    ( 1051): 
D/skia    ( 1051): Features	: swp half thumb fastmult vfp edsp thumbee neon vfpv3 tls vfpv4 
D/skia    ( 1051): CPU implementer	: 0x41
D/skia    ( 1051): CPU architecture: 7
D/skia    ( 1051): CPU variant	: 0x0
D/skia    ( 1051): CPU part	: 0xc05
D/skia    ( 1051): CPU revision	: 1
D/skia    ( 1051): 
D/skia    ( 1051): Hardware	: QCT MSM7x27a FFA
D/skia    ( 1051): Revision	: 0000
D/skia    ( 1051): Serial		: 0000000000000000
D/skia    ( 1051): 
D/skia    ( 1051): END /proc/cpuinfo
D/skia    ( 1051): Device supports ARM NEON instructions!
I/Gecko   (  136): 
I/Gecko   (  136): ###!!! [Parent][MessageChannel] Error: Channel error: cannot send/recv
I/Gecko   (  136): 
E/msm7627a.hwcomposer(  136): drawLayerUsingCopybit: greater than max supported size dsdx=0.003125 dtdy=320.000000 scaleLimitMax=16.000000 scaleLimitMin=0.062500
E/msm7627a.hwcomposer(  136): hwc_set: Copybit layer draw failed!
E/msm7627a.hwcomposer(  136): hwc_set: Unable to render by hwc due to non-pmem memory
E/msm7627a.hwcomposer(  136): drawLayerUsingCopybit: greater than max supported size dsdx=0.003125 dtdy=320.000000 scaleLimitMax=16.000000 scaleLimitMin=0.062500
E/msm7627a.hwcomposer(  136): hwc_set: Copybit layer draw failed!
E/msm7627a.hwcomposer(  136): hwc_set: Unable to render by hwc due to non-pmem memory
E/msm7627a.hwcomposer(  136): drawLayerUsingCopybit: greater than max supported size dsdx=0.003125 dtdy=320.000000 scaleLimitMax=16.000000 scaleLimitMin=0.062500
E/msm7627a.hwcomposer(  136): hwc_set: Copybit layer draw failed!
E/msm7627a.hwcomposer(  136): hwc_set: Unable to render by hwc due to non-pmem memory
E/msm7627a.hwcomposer(  136): drawLayerUsingCopybit: greater than max supported size dsdx=0.003125 dtdy=320.000000 scaleLimitMax=16.000000 scaleLimitMin=0.062500
E/msm7627a.hwcomposer(  136): hwc_set: Copybit layer draw failed!
E/msm7627a.hwcomposer(  136): hwc_set: Unable to render by hwc due to non-pmem memory
E/msm7627a.hwcomposer(  136): drawLayerUsingCopybit: greater than max supported size dsdx=0.003125 dtdy=320.000000 scaleLimitMax=16.000000 scaleLimitMin=0.062500
E/msm7627a.hwcomposer(  136): hwc_set: Copybit layer draw failed!
E/Profiler(  136): BPUnw: [1 total] thread_unregister_for_profiling(me=0x1cea2d0)  (NOT REGISTERED) 
E/memalloc(  136): /dev/pmem: No more pmem available
W/memalloc(  136): Falling back to ashmem
E/memalloc(  136): /dev/pmem: No more pmem available
W/memalloc(  136): Falling back to ashmem
E/memalloc(  136): /dev/pmem: No more pmem available
W/memalloc(  136): Falling back to ashmem
E/memalloc(  136): /dev/pmem: No more pmem available
W/memalloc(  136): Falling back to ashmem
E/memalloc(  136): /dev/pmem: No more pmem available
W/memalloc(  136): Falling back to ashmem

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Reporter
	Comment 90 • 10 years ago

tn, can you see if you can repro this and figure out what's going on? Probably worth forking that to a separate bug.

	Timothy Nikkel (:tnikkel) Assignee
	Comment 91 • 10 years ago

I can repro, I filed bug 989897.