Closed Bug 744699 Opened 8 years ago Closed 8 years ago

Need to investigate how changes in displayport affect area drawn

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 14
Tracking Status
firefox14 --- fixed
firefox15 --- verified
blocking-fennec1.0 --- +

People

(Reporter: kats, Assigned: kats)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Specifically, if I scroll around a page gently with the displayport size the same as the viewport (gfx.displayport.strategy = 3), I see some small amount of checkerboarding (< 100 pixels) at the edge. In theory enlarging the displayport by that amount while keeping everything else constant should just push the checkerboarded area off-screen and result in no checkerboarding. However when I do that I still get checkerboard. This makes me think that somewhere in the draw pipeline something is getting drawn when it shouldn't - it could be something like rounding errors or improper invalid rect propagation. It could also be related to changing the size of the displayport (for example in the current velocity-bias strategy, we trim the margins on the axis we're not scrolling on, which might trigger unneccessary reallocations and drawing). I would like to get rendertrace-style info for rects that are processed in the draw pipeline to make sure we aren't drawing more than we need to.
So while investigating this I'm finding that presShell->GetRootScrollFrame() is returning null, specifically at http://mxr.mozilla.org/mozilla-central/source/layout/base/nsLayoutUtils.cpp#1558 - this seems to imply that we don't end up using the displayport at all! I don't know enough about this code to know what's going on here, cc'ing roc to hopefully get some answers on this.

Specific questions I have:

- why does the code at http://mxr.mozilla.org/mozilla-central/source/layout/base/nsLayoutUtils.cpp#1561 require a scroll frame? I added some code to dump the tree at that point and was getting trees that looked like this (pre-order traversal with depth printed) on about:buildconfig as a simple example.

04-13 22:23:57.218 I/Gecko   ( 4632): 0 frame is ViewportFrame
04-13 22:23:57.218 I/Gecko   ( 4632): 1 frame is RootFrame
04-13 22:23:57.218 I/Gecko   ( 4632): 2 frame is BoxFrame
04-13 22:23:57.218 I/Gecko   ( 4632): 3 frame is PopupSetFrame
04-13 22:23:57.218 I/Gecko   ( 4632): 3 frame is PlaceholderFrame
04-13 22:23:57.218 I/Gecko   ( 4632): 3 frame is DeckFrame
04-13 22:23:57.218 I/Gecko   ( 4632): 4 frame is subDocumentFrame

Note that there is a RootFrame where the code expects a ScrollFrame.

- What does the block of code at http://mxr.mozilla.org/mozilla-central/source/dom/base/nsDOMWindowUtils.cpp#324 do? Here too the root scroll frame is null which makes me suspect bad things are happening.

- If the code at http://mxr.mozilla.org/mozilla-central/source/layout/base/nsLayoutUtils.cpp#1564 sets usingDisplayPort to false, what happens? Do we still draw the displayport area (maybe there's a check somewhere else?) or do we not do that? I'm seeing the code go into the !usingDisplayPort branch a few lines below that, where it gets the visible region from GetVisualOverflowRectRelativeToSelf().
Assignee: nobody → bugmail.mozilla
blocking-fennec1.0: --- → ?
What element are we trying to set the display port on?

Setting the displayport on the root element of the <browser>'s subdocument should work, because that document is scrollable. Setting the displayport on the root element of the chrome document containing the <browser> will not work because that document isn't scrollable.
We set it on the root element of the content document, which I think is the same as what you mean by the <browser>'s subdocument. The code where we do this is at http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#1656
presShell->GetRootScrollFrame() for the presShell of the <browser>'s subdocument should certainly be returning non-null.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #2)
> What element are we trying to set the display port on?
> 
> Setting the displayport on the root element of the <browser>'s subdocument
> should work, because that document is scrollable. Setting the displayport on
> the root element of the chrome document containing the <browser> will not
> work because that document isn't scrollable.

Ok, so we're setting the displayport on the right element (the root element of the <browser>'s subdocument), but we're doing so by calling SetDisplayPortForElement on the presShell of the *chrome* document. Will this work to set the displayport?

When I change the code to call SetDisplayPortForElement on the presShell of the content document, I see the displayport getting set more correctly, in that it finds the root scroll frame and the block of code at http://mxr.mozilla.org/mozilla-central/source/dom/base/nsDOMWindowUtils.cpp#324 executes.

However, doing that doesn't solve the other problem, which is that at paint-time, usingDisplayPort is still coming out to be false. When we request a paint, we fire a NS_PAINT event which triggers the Paint() function on the chrome document's presShell, and that still returns null from GetRootScrollFrame().
(In reply to Kartikaya Gupta (:kats) from comment #5)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #2)
> > What element are we trying to set the display port on?
> > 
> > Setting the displayport on the root element of the <browser>'s subdocument
> > should work, because that document is scrollable. Setting the displayport on
> > the root element of the chrome document containing the <browser> will not
> > work because that document isn't scrollable.
> 
> Ok, so we're setting the displayport on the right element (the root element
> of the <browser>'s subdocument), but we're doing so by calling
> SetDisplayPortForElement on the presShell of the *chrome* document. Will
> this work to set the displayport?

No! nsDOMWindowUtils::SetDisplayPortForElement should check that the element's document is the same as presShell's document and error out if it isn't. Can you fix that (and the caller)?

> However, doing that doesn't solve the other problem, which is that at
> paint-time, usingDisplayPort is still coming out to be false. When we
> request a paint, we fire a NS_PAINT event which triggers the Paint()
> function on the chrome document's presShell, and that still returns null
> from GetRootScrollFrame().

You are setting a displayport on the <browser>'s child document. That means we should construct layers for that document that cover its displayport. When we scroll inside the <browser> element, those layers should be used. I.e., in nsGfxScrollFrameInner::BuildDisplayList, we should be hitting usingDisplayport sometimes.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #6)
> No! nsDOMWindowUtils::SetDisplayPortForElement should check that the
> element's document is the same as presShell's document and error out if it
> isn't. Can you fix that (and the caller)?

Patch attached.

> You are setting a displayport on the <browser>'s child document. That means
> we should construct layers for that document that cover its displayport.
> When we scroll inside the <browser> element, those layers should be used.
> I.e., in nsGfxScrollFrameInner::BuildDisplayList, we should be hitting
> usingDisplayport sometimes.

Ok, I see the usingDisplayport code in nsGfxScrollFrameInner::BuildDisplayList getting hit (both with and without the patch above). I think this part of it is at least a little clearer to me now, thanks. I'll continue investigating more and ping you if I have further questions.
Attachment #615233 - Flags: review?(roc)
blocking-fennec1.0: ? → +
https://hg.mozilla.org/mozilla-central/rev/7cd93796187b
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
I don't see any checkerboard on the latest Nightly if follow the scenarios mentioned in comment #0. Closing bug as verified fixed on:

Firefox 15.0a1 (2012-05-23)
Device: Galaxy Nexus
OS: Android 4.0.2
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.