Provide new RenderDocument flags for separate painting fixed and not fixed layout frames

NEW
Assigned to

Status

()

Core
Layout
9 years ago
8 years ago

People

(Reporter: romaxa, Assigned: romaxa)

Tracking

Trunk
Other
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 4 obsolete attachments)

(Assignee)

Description

9 years ago
Created attachment 394180 [details] [diff] [review]
New flags for RenderDocument, and usage of these flags in nsViewPortFrame 

For browser rendering implementation with virtual layout surface (canvas for fennec) existing one problem with fixed positioned frames.
Problem appear when we scrolling virtual surface and fixed frames are scrolled too...
Repainting of fixed frames for each scroll event make scrolling speed dependent from content, without synchronous repainting those frames jumping or following to the scrolled virtual surface (depends how often we are updating scroll port position).

There is 2 possible ways to fix this problem:
1) Hide fixed frames before scrolling, and restore it back when scrolling finished.
2) Use option 1) + paint and remember fixed frames temporary layer, and during scrolling paint that layer on top of main surface (which does not contain painted fixed frames).

Here is the working patch which allow to paint layout:
1) Fixed frames only 
2) Without fixed frames
3) both - default

Still not sure about right names for flags and member variables...
Attachment #394180 - Flags: review?(roc)
(Assignee)

Comment 1

9 years ago
Created attachment 394181 [details] [diff] [review]
New PresShell API for Invalidating fixed frames only
Assignee: nobody → romaxa
Attachment #394181 - Flags: review?(roc)
This patch doesn't match the approach described in comment #0.
(Assignee)

Comment 3

8 years ago
(In reply to comment #2)
> This patch doesn't match the approach described in comment #0.

Why it doesn't match? 
First patch providing flag for separate rendering fixed frames and layout without fixed frames... Before scrolling we will paint layout without fixed frames, and after scrolling return fixed frames back by requesting for paint in normal mode.

Second patch will help to repaint only fixed frames regions, to avoid extra painting and make it cheaper.
Oops, I didn't noticed you'd added two patches.

For the first patch, you don't need to add flags to nsViewportFrame. In nsDisplayListBuilder I think it's clearer if the two flags have the same sense --- i.e., they should both indicate frames to paint, or both indicate frames to ignore. In this case I think it's better to make them indicate frames to ignore, so call them mIgnoreFixedFrames and mIgnoreNonFixedFrames.

I don't know why you need the patch in comment #1. Why is it useful to invalidate fixed-pos frames?
(Assignee)

Comment 5

8 years ago
> I don't know why you need the patch in comment #1. Why is it useful to
> invalidate fixed-pos frames?

It is just for fast re-painting of fixed frames area.

On N900, when we start scrolling I'm requesting to repaint facebook toolbar area only, and when scrolling is finished I repaint that area again.

This is important because before scrolling started we have to repaint that area as possible faster, otherwise toolbar moving together with cached offscreen area.
(Assignee)

Comment 6

8 years ago
Comment on attachment 394181 [details] [diff] [review]
New PresShell API for Invalidating fixed frames only

Probably it would be flexible also to have possibility to get region of fixed frames position...
(Assignee)

Comment 7

8 years ago
> For the first patch, you don't need to add flags to nsViewportFrame. In

I need to know value of that flags in "ViewportFrame::GetChildList", how I can get nsDisplayListBuilder in that function?

> ignore. In this case I think it's better to make them indicate frames to
> ignore, so call them mIgnoreFixedFrames and mIgnoreNonFixedFrames.

Ye I can do that
(Assignee)

Comment 8

8 years ago
Created attachment 407848 [details] [diff] [review]
Updated patch

Updated according to comment
Attachment #394180 - Attachment is obsolete: true
Attachment #407848 - Flags: review?(roc)
Attachment #394180 - Flags: review?(roc)
+  NS_IMETHOD InvalidateFixedFrames(void) = 0;

Would it make more sense to not invalidate, just return the region occupied by the fixed frames? Or something like that? InvalidateFixedFrames is a bit weird since it does a NOTIFY_ONLY invalidate, so it's not actually invalidating anything, just firing a MozAfterPaint event.

+  if (!ignoreFixedFrames)
+    aBuilder->MarkFramesForDisplayList(this, mFixedContainer.GetChildList(), aDirtyRect);

{}

+  nsIFrame* kid = ignoreNonFixedFrames ?
+      mFixedContainer.GetChildList().FirstChild() :
+      mFrames.FirstChild();

This will actually cause subtle problems. Normally we paint fixed-pos frames through their placeholders. This guarantees that stuff like
  <div style="opacity:0.5">
     <div style="position:fixed; ..."></div>
  </div>
works properly. By painting the fixed-pos frames directly in this case, you'd break those pages.

I'm not sure what to do here. Possibly you need to go through the display list after it's been built and remove all leaf display items associated with frames that aren't inside one of the fixed-pos frames of this nsViewportFrame.
(Assignee)

Comment 10

8 years ago
What do you mean about problem with this testcase?:
<body bgcolor=blue>
<div style="opacity:0.5">
     <div style="position:fixed;"><h1>XYZ</h1></div>
     <h1>ABC</h1>
</div>
<h1>123</h1>
<body>

Now it painting in this way:
Only Fixed frames: - transparent "XYZ"
Only Normal frames: - transparent "ABC", not opaque "123"

Is it wrong behavior or what?
> Only Fixed frames: - transparent "XYZ"

How does that work? It seems to me that the text should not be transparent, with your patch. Can you dump the display list and paste it here?
(Assignee)

Comment 12

8 years ago
Yep, you right, I was wrong and did some mistake.
How can I detect "items associated with frames that aren't inside" - ?
Every leaf display item has an associated frame --- GetUnderlyingFrame(). Call it, and then scan the ancestor chain of that frame to see if it's under a fixed-pos frame.
(Assignee)

Comment 14

8 years ago
Created attachment 412475 [details] [diff] [review]
New API for getting fixed frames area
Attachment #394181 - Attachment is obsolete: true
Attachment #412475 - Flags: review?(roc)
Attachment #394181 - Flags: review?(roc)
You need to rev IID on nsIPresShell.

I don't think this should return the screenrect. Can't it just return coordinates relative to the presshell, i.e. the union of the frame rects?

And I think it should return nsIntRegion, not an nsIRegion. Does that work for you?
(Assignee)

Comment 16

8 years ago
> You need to rev IID on nsIPresShell.
Ok,
> 
> I don't think this should return the screenrect. Can't it just return
> coordinates relative to the presshell, i.e. the union of the frame rects?
Which function should I call to get that union?

It would be nice to have the same coordinates as in MozAfterPaint event.

For painting these rects I'm using the same function which is painting MozAfterPaint events


> 
> And I think it should return nsIntRegion, not an nsIRegion. Does that work for
> you?
I think I can use nsIntRegion, no problem.
(In reply to comment #16)
> > I don't think this should return the screenrect. Can't it just return
> > coordinates relative to the presshell, i.e. the union of the frame rects?
> Which function should I call to get that union?

Just call GetRect instead of GetScreenRect. Or call GetRect and then convert to CSS pixels.

> It would be nice to have the same coordinates as in MozAfterPaint event.

That's in CSS pixels relative to the presshell, i.e. not screen coordinates.
(Assignee)

Comment 18

8 years ago
(In reply to comment #13)
> Every leaf display item has an associated frame --- GetUnderlyingFrame(). Call
> it, and then scan the ancestor chain of that frame to see if it's under a
> fixed-pos frame.

In this case I should get normal list with all frames at first... and then run recursive function (similar to ComputeVisibility) and recreate DisplayList by keeping fixed frames and frames which are containing fixed-pos frames...

But this I think a bit expensive...

Another way is to hack BuildDisplayList functions and don't add frames which are not containing fixed frames.

This should be a cheaper solution...
(Assignee)

Comment 19

8 years ago
Created attachment 412595 [details] [diff] [review]
Remove nonFixedFrames implementation
Attachment #407848 - Attachment is obsolete: true
Attachment #412595 - Flags: review?(roc)
Attachment #407848 - Flags: review?(roc)
(Assignee)

Comment 20

8 years ago
> > you?
> I think I can use nsIntRegion, no problem.

Hmm... 1.9.2 does not have nsIntRegion... :(.
I will create one version for trunk, based on nsIntRegion...
And another for 1.9.2 branch based on nsRegion.
(Assignee)

Comment 21

8 years ago
Oh, and I not able to use nsRegion outside of layout... 
I'm getting this:
 undefined symbol: _ZN8nsRegion4InitEv
....
(Assignee)

Comment 22

8 years ago
Created attachment 412615 [details] [diff] [review]
Updated IID, GetScreenRect -> GetRect + conversion
Attachment #412475 - Attachment is obsolete: true
Attachment #412615 - Flags: review?(roc)
Attachment #412475 - Flags: review?(roc)
+  PRBool IsIgnoreFixedFrames(void) { return mIgnoreFixedFrames; }
+  PRBool IsIgnoreNonFixedFrames(void) { return mIgnoreNonFixedFrames; }

IsIgnoring

We don't need IsIgnoreNonFixedFrames. PresShell::RenderDocument can just test its aFlags directly.

+  return Count() > 0 ? PR_TRUE : PR_FALSE;

return Count() > 0;

+   * Find and keep only fixed frames and their parents in the list.
+   * @return TRUE if list is not empty
+   */
+  PRBool RemoveNonFixedItems(nsDisplayListBuilder* aBuilder, nsIFrame *aFixed);

Need to document what aFixed means. Actually I suspect what you want to do here is pass in the nsViewportFrame whose fixed-pos frames you want to keep. Then for each GetUnderlyingFrame, find its ancestor whose parent is that nsViewportFrame, and check if that ancestor is the viewport frame's normal child (GetChildList(nsnull)).

+  if (!aBuilder->IsIgnoreFixedFrames()) {
+    aBuilder->MarkFramesForDisplayList(this, mFixedContainer.GetChildList(),
+                                       aDirtyRect);
+  }

This is actually incorrect. It will skip fixed-pos frames in IFRAMEs. You actually want to only ignore fixed frames of the top-level document you're rendering.
+  NS_IMETHOD GetFixedFramesArea(nsIRegion **aOutRegion) = 0;

Need some documentation.

+    outRegion->Union(nsPresContext::AppUnitsToIntCSSPixels(rect.x),
+                     nsPresContext::AppUnitsToIntCSSPixels(rect.y),
+                     nsPresContext::AppUnitsToIntCSSPixels(rect.width),
+                     nsPresContext::AppUnitsToIntCSSPixels(rect.height));

You probably want some specific rounding here, but I'm not sure what. What do you want to return when the edges of the fixed-pos frame aren't aligned with CSS pixels? Note that due to zooming they might be aligned to device pixels but not CSS pixels.

Also, do you want to include the overflow areas of fixed-pos content?
Comment on attachment 412595 [details] [diff] [review]
Remove nonFixedFrames implementation

see comments #23 and #24
Attachment #412595 - Flags: review?(roc) → review-
Comment on attachment 412615 [details] [diff] [review]
Updated IID, GetScreenRect -> GetRect + conversion

see comments #23 and #24
Attachment #412615 - Flags: review?(roc) → review-
You need to log in before you can comment on or make changes to this bug.