Closed Bug 618975 Opened 13 years ago Closed 13 years ago

Pan overflow elements in parent process

Categories

(Core :: IPC, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla2.0
Tracking Status
fennec 2.0+ ---

People

(Reporter: stechz, Assigned: stechz)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-needed, Whiteboard: needs new patch)

Attachments

(4 files, 21 obsolete files)

39.29 KB, patch
stechz
: review+
stechz
: superreview+
Details | Diff | Splinter Review
5.12 KB, patch
stechz
: review+
Details | Diff | Splinter Review
25.90 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
2.75 KB, patch
blassey
: review+
Details | Diff | Splinter Review
      No description provided.
Depends on: 605618
tracking-fennec: --- → ?
Depends on: 586288
Blocks: 586288
No longer depends on: 586288
Attached patch Mobile patch (obsolete) — Splinter Review
This patch gets overflow scrolling working on Fennec. It works on my test page really well, but Google Reader seems to be somewhat broken. I'm not sure what's wrong yet.
Assignee: nobody → ben
Status: NEW → ASSIGNED
Wow. HTML, you win this round.

http://people.mozilla.org/~bstover/scrolling.html

Above is a case I hadn't considered before. Since overflow divs are not a separate document, we cannot just use BuildDisplayListForStackingContext like we could for iframes.

I have a rough patch that gets panning working for overflow elements, but it does not pay nice with scenarios like the URL above. We will need to make sure that we can effectively treat the child's display list as if it is a new stacking context before we make a new layer.
Status: ASSIGNED → NEW
tracking-fennec: ? → 2.0+
Blocks: 559437
Are these ready for review?
Blocks: 572793
Blocks: 616348
Attachment #497380 - Attachment is obsolete: true
Attachment #497381 - Attachment is obsolete: true
Attached patch Mobile patch (obsolete) — Splinter Review
Attachment #515213 - Flags: review?(mark.finkle)
Attachment #515212 - Flags: review?(tnikkel)
Attachment #515212 - Flags: review?(jones.chris.g)
I went ahead and tried to do some trailing space busting. I hope you don't mind for review! :)

Timothy and Chris: if you feel like you need me to split this patch up so it's easier to see what you need to review, let me know.
Comment on attachment 515212 [details] [diff] [review]
Pan overflow elements in parent process

In nsDisplayList.h your editor seems to have removed all trailing spaces making it harder to find what has actually changed.
(In reply to comment #10)
> Comment on attachment 515212 [details] [diff] [review]
> Pan overflow elements in parent process
> 
> In nsDisplayList.h your editor seems to have removed all trailing spaces making
> it harder to find what has actually changed.

I guess that means you do mind trailing space busting. The only change you care about in that file is here:
https://bugzilla.mozilla.org/attachment.cgi?id=515212&action=diff#a/layout/base/nsDisplayList.h_sec25
I don't really care either way, but mixing it in with another patch makes it harder to review.
Attachment #515213 - Flags: review?(mark.finkle) → review+
I would rather you didn't remove trailing whitespace in nsDisplayList.h.  The negative impact on hg blame outweighs whatever benefit removing the whitespace might convey, IMHO.  Also I agree with Timothy re: reviewing.
Comment on attachment 515212 [details] [diff] [review]
Pan overflow elements in parent process

Some initial comments.

   void setDisplayPort(in float aXPx, in float aYPx,
-                      in float aWidthPx, in float aHeightPx);
+                      in float aWidthPx, in float aHeightPx,
+                      in nsIDOMElement aElement);

What's the meaning of this new parameter? is a question the comments above it should probably answer.


 nsDOMWindowUtils::SetDisplayPort(float aXPx, float aYPx,
+      rootFrame->InvalidateFrameSubtree();

You have a patch changing this call. You'll want to make sure that it doesn't get lost in the shuffle. In fact this patch is based on a tree with that patch applied.

 static void RecordFrameMetrics(nsIFrame* aForFrame,
+                               nsIFrame* aViewportFrame,
                                ContainerLayer* aRoot,
                                nsRect aVisibleRect,
                                nsRect aViewport,
+                               nsRect aDisplayPort,
                                ViewID aScrollId) {

+  RecordFrameMetrics(aForFrame, presShell->GetRootScrollFrame(),
+                     root, mVisibleRect, mVisibleRect, displayport, id);

GetRootScrollFrame is never the viewport frame. Looks like nsDisplayScrollLayer has a member named mViewportFrame, and I'm guessing its not a viewport frame either. I must have let that through when I first reviewed this code. Since it's already existing code you can fix this with a seperate patch.

 class nsDisplayScrollLayer : public nsDisplayOwnLayer
+   * @param aDisplayPort Overrides the visibility of the child items if it
+   *                     is not equal to aClip.

What is aClip?

The nsIPresShell.h changes will require an IID bump I think.
Blocks: 635642
> What's the meaning of this new parameter? is a question the comments above it
> should probably answer.

Done.

>  nsDOMWindowUtils::SetDisplayPort(float aXPx, float aYPx,
> +      rootFrame->InvalidateFrameSubtree();
> 
> You have a patch changing this call. You'll want to make sure that it doesn't
> get lost in the shuffle. In fact this patch is based on a tree with that patch
> applied.

Fixed. I'll just apply to trunk and if the other lands I'll update this. :)

>  static void RecordFrameMetrics(nsIFrame* aForFrame,
> +                               nsIFrame* aViewportFrame,
>                                 ContainerLayer* aRoot,
>                                 nsRect aVisibleRect,
>                                 nsRect aViewport,
> +                               nsRect aDisplayPort,
>                                 ViewID aScrollId) {
> 
> +  RecordFrameMetrics(aForFrame, presShell->GetRootScrollFrame(),
> +                     root, mVisibleRect, mVisibleRect, displayport, id);
> 
> GetRootScrollFrame is never the viewport frame. Looks like nsDisplayScrollLayer
> has a member named mViewportFrame, and I'm guessing its not a viewport frame
> either. I must have let that through when I first reviewed this code. Since
> it's already existing code you can fix this with a seperate patch.

Well, I don't think I understand the distinction so I probably just gave this a bad name. What I really want is the scrollable frame that contains the content, whether its the root or an overflow div. I'm open to names.

>  class nsDisplayScrollLayer : public nsDisplayOwnLayer
> +   * @param aDisplayPort Overrides the visibility of the child items if it
> +   *                     is not equal to aClip.
> 
> What is aClip?

Fixed.

> 
> The nsIPresShell.h changes will require an IID bump I think.

Done.
Attachment #515212 - Attachment is obsolete: true
Attachment #515212 - Flags: review?(tnikkel)
Attachment #515212 - Flags: review?(jones.chris.g)
Attachment #515953 - Flags: review?(tnikkel)
Attachment #515953 - Flags: review?(jones.chris.g)
(In reply to comment #15)
> Well, I don't think I understand the distinction so I probably just gave this a
> bad name. What I really want is the scrollable frame that contains the content,
> whether its the root or an overflow div. I'm open to names.

Ok, so where ever you use this you have two frames, the scroll frame and the scrolled frame. How about naming the two frames just that?
Whiteboard: [needs-review (tnikkel, cjones)]
Comment on attachment 515953 [details] [diff] [review]
Pan overflow elements in parent process

+++ b/dom/interfaces/base/nsIDOMWindowUtils.idl
+   * <x, y> is relative to the top-left of the visible area.  This
    * means that the pixels rendered to the displayport take scrolling
    * into account, for example.

Which visible area? The visible area of the element?

+   * area's bounds.  Nothing pixels are rendered outside the
+   * area bounds.

What does that mean exactly?

 enum LayerState {
   LAYER_NONE,
   LAYER_INACTIVE,
-  LAYER_ACTIVE
+  LAYER_ACTIVE,
+  LAYER_ACTIVE_FORCE
 };

Document this new type, ie "force an active layer even if it would otherwise be made an inactive layer for correct rendering inside of rounded rect clips".

+  virtual LayerState GetLayerState(nsDisplayListBuilder* aBuilder,
+                                   LayerManager* aManager)
+  {
+    // Try as hard as possible to make this a layer so we can scroll it
+    // asynchronously.

It's more like "force this layer active even if it causes incorrect rendering so we can...".

 #define NS_IPRESSHELL_IID     \
- { 0xd1978bee, 0x43b9, 0x40de, \
-    { 0x95, 0x47, 0x85, 0x06, 0x5e, 0x02, 0xec, 0xb4 } }
+ { 0x7e88db33, 0x2766, 0x488a, \
+    { 0xaf, 0xce, 0x1e, 0x39, 0x02, 0xf3, 0x2f, 0x0c } }

I don't know when you are planning on landing this, but the presshell IID is frozen right now, so you should be doing this to the mozilla 2.0 branch presshell. If you want to land after the IIDs get unfrozen this is fine (I don't know when or how that will happen).

+PRBool PresShell::UsingDisplayPort() const
+{
+  nsIFrame* rootFrame = GetRootScrollFrame();

Call it rootScrollFrame, it'll reduce confusion. And same for GetDisplayPort.

diff --git a/layout/generic/nsGfxScrollFrame.h b/layout/generic/nsGfxScrollFrame.h
+  bool GetDisplayPort(nsRect* aResult) const;

This seems to be dead code.

diff --git a/layout/generic/nsSubDocumentFrame.cpp b/layout/generic/nsSubDocumentFrame.cpp
-      subdocRootScrollFrame->AddStateBits(
-        NS_FRAME_FORCE_DISPLAY_LIST_DESCEND_INTO);

We don't need this anymore?

@@ -1858,41 +1858,82 @@ nsGfxScrollFrameInner::BuildDisplayList(
+  if (usingDisplayPort)
+    dirtyRect = displayport;

Add comments about why we need this?

@@ -1858,41 +1858,82 @@ nsGfxScrollFrameInner::BuildDisplayList(
+  bool buildingLayer =
+    (XRE_GetProcessType() == GeckoProcessType_Content &&
+     (scrollRange.width >= 20 || scrollRange.height >= 20)) &&
+     (!mIsRoot || !mOuter->PresContext()->IsRootContentDocument());

+  if (buildingLayer) {

+    if (scrollRange.width >= 20 || scrollRange.height >= 20) {

If buildingLayer is true then isn't scrollRange.width >= 20 || scrollRange.height >= 20 also true?

Why 20? Since this is in appunits thats usually 1/3 of a pixel.

@@ -1858,41 +1858,82 @@ nsGfxScrollFrameInner::BuildDisplayList(
+    rv = mScrolledFrame->BuildDisplayListForStackingContext(
+      aBuilder,
+      dirtyRect - mScrolledFrame->GetOffsetTo(mOuter),

dirtyRect + mOuter->GetOffsetTo(mScrolledFrame) is clearer I think.

Using BuildDisplayListForStackingContext instead of BuildDisplayListForChild must break some things, although I'm not sure how serious a problem this is.

And we need to rename the variables called viewportframe, this can be in a patch before or after this patch, whatever is easiest.
Comment on attachment 515953 [details] [diff] [review]
Pan overflow elements in parent process

In addition to tn's comments:

I think these changes mean it's no longer possible to unset a
displayport from the root document, but I also don't think I care.

>diff --git a/dom/base/nsDOMWindowUtils.cpp b/dom/base/nsDOMWindowUtils.cpp
>--- a/dom/base/nsDOMWindowUtils.cpp
>+++ b/dom/base/nsDOMWindowUtils.cpp
> NS_IMETHODIMP
> nsDOMWindowUtils::SetDisplayPort(float aXPx, float aYPx,
>-                                 float aWidthPx, float aHeightPx)
>+                                 float aWidthPx, float aHeightPx,
>+                                 nsIDOMElement* aElement)
>+  nsCOMPtr<nsIContent> content = do_QueryInterface(aElement);

Need to null-check |content|.

>+  nsRect lastDisplayPort;
>+  if (nsLayoutUtils::GetDisplayPort(content, &lastDisplayPort) &&
>+      displayport == lastDisplayPort) {
>+    return NS_OK;
>+  }
>+
>+  content->SetProperty(nsGkAtoms::DisplayPort, new nsRect(displayport),
>+                       DestroyNsRect);
> 
>   presShell->SetIgnoreViewportScrolling(PR_TRUE);

This is a somewhat undefined state; this call could be said to be
wrong if there's not a displayport set for the root document.  But
we'll of course always assume that, so it's probably worth noting in
the docs.

>diff --git a/dom/interfaces/base/nsIDOMWindowUtils.idl b/dom/interfaces/base/nsIDOMWindowUtils.idl
>--- a/dom/interfaces/base/nsIDOMWindowUtils.idl
>+++ b/dom/interfaces/base/nsIDOMWindowUtils.idl

Need an IID bump here.  Probably easier to stick this on the
MOZILLA_2_0_BRANCH interface rather than trying to outfox
mozilla-central.

>@@ -124,43 +124,51 @@ interface nsIDOMWindowUtils : nsISupport
>    * This will trigger reflow.
>    *
>    * The caller of this method must have UniversalXPConnect
>    * privileges.
>    */
>   void setCSSViewport(in float aWidthPx, in float aHeightPx);
> 
>   /**
>-   * Set the "displayport" to be <xPx, yPx, widthPx, heightPx> in
>-   * units of CSS pixels, regardless of the size of the enclosing
>-   * widget/view.  This will *not* trigger reflow.
>+   * For any scrollable element, this allows you to override the
>+   * visible region and draw more than what is visible, which is
>+   * useful for asynchrnous drawing. The "displayport" will be

Typo.

>    *
>-   * The displayport will be used as the window's visible region for
>-   * the purposes of invalidation and painting.  The displayport can
>-   * approximately be thought of as a "persistent" drawWindow()
>-   * (albeit with coordinates relative to the CSS viewport): the
>-   * bounds are remembered by the platform, and layer pixels are
>+   * The displayport will be used as the area's visible region for
>+   * the purposes of invalidation and painting.  The displayport for
>+   * the root element can approximately be thought of as a "persistent"
>+   * drawWindow() (albeit with coordinates relative to the CSS viewport):
>+   * the bounds are remembered by the platform, and layer pixels are
>    * retained and updated inside the viewport bounds.

The CSS viewport is mostly irrelevant for displayports set on <div>s etc.

>    *
>-   * It's legal to set a displayport that extends beyond the CSS
>-   * viewport in any direction (left/right/top/bottom).
>+   * It's legal to set a displayport that extends beyond the scrollable
>+   * area in any direction (left/right/top/bottom).
>    * 
>    * It's also legal to set a displayport that extends beyond the
>-   * document's bounds.  The value of the pixels rendered outside the
>-   * document bounds is not yet defined.
>+   * area's bounds.  Nothing pixels are rendered outside the
>+   * area bounds.

I think the term we want is "overflow area", but I'm not 100% sure.

>diff --git a/layout/base/nsDisplayList.cpp b/layout/base/nsDisplayList.cpp
>--- a/layout/base/nsDisplayList.cpp
>+++ b/layout/base/nsDisplayList.cpp

Just skimmed these changes.

> static void RecordFrameMetrics(nsIFrame* aForFrame,
>+                               nsIFrame* aViewportFrame,
>                                ContainerLayer* aRoot,
>                                nsRect aVisibleRect,
>                                nsRect aViewport,
>+                               nsRect aDisplayPort,
>                                ViewID aScrollId) {
>   nsPresContext* presContext = aForFrame->PresContext();
>-  nsIPresShell* presShell = presContext->GetPresShell();
> 
>   nsIntRect visible = aVisibleRect.ToNearestPixels(presContext->AppUnitsPerDevPixel());
>   aRoot->SetVisibleRegion(nsIntRegion(visible));
> 
>   FrameMetrics metrics;
> 
>   PRInt32 auPerDevPixel = presContext->AppUnitsPerDevPixel();
>   metrics.mViewport = aViewport.ToNearestPixels(auPerDevPixel);

Hm, this "viewport" name is unfortunate now, but can fix in a
low-priority followup.

>diff --git a/layout/base/nsPresShell.cpp b/layout/base/nsPresShell.cpp
>--- a/layout/base/nsPresShell.cpp
>+++ b/layout/base/nsPresShell.cpp
>+nsRect PresShell::GetDisplayPort() const
>+{
>+  nsIFrame* rootFrame = GetRootScrollFrame();
>+  NS_ABORT_IF_FALSE(rootFrame, "no root frame");
>+  nsIContent* content = rootFrame->GetContent();
>+  NS_ABORT_IF_FALSE(content, "no root content");
>+
>+  nsRect result;
>+  NS_ABORT_IF_FALSE(
>+    nsLayoutUtils::GetDisplayPort(content, &result),
>+    "no displayport defined!"
>+  );

NS_ABORT_IF_FALSE() disappears in opt builds.  Don't think that's what
was intended here.  You probably instead want to assert
UsingDisplayPort() in debug builds on entry to this method.

Need to note in the API docs that garbage is returned if
!UsingDisplayPort(), it's a precondition.

>diff --git a/layout/generic/nsGfxScrollFrame.cpp b/layout/generic/nsGfxScrollFrame.cpp
>--- a/layout/generic/nsGfxScrollFrame.cpp
>+++ b/layout/generic/nsGfxScrollFrame.cpp
>diff --git a/layout/generic/nsSubDocumentFrame.cpp b/layout/generic/nsSubDocumentFrame.cpp
>--- a/layout/generic/nsSubDocumentFrame.cpp
>+++ b/layout/generic/nsSubDocumentFrame.cpp

Just skimmed these changes.
Attachment #515953 - Flags: review?(jones.chris.g) → review-
> Which visible area? The visible area of the element?

Yes, fixed.

> 
> +   * area's bounds.  Nothing pixels are rendered outside the
> +   * area bounds.
> 
> What does that mean exactly?

Meant to say "no pixels." Fixed.

>  enum LayerState {
>    LAYER_NONE,
>    LAYER_INACTIVE,
> -  LAYER_ACTIVE
> +  LAYER_ACTIVE,
> +  LAYER_ACTIVE_FORCE
>  };
> 
> Document this new type, ie "force an active layer even if it would otherwise be
> made an inactive layer for correct rendering inside of rounded rect clips".

Done.

> 
> +  virtual LayerState GetLayerState(nsDisplayListBuilder* aBuilder,
> +                                   LayerManager* aManager)
> +  {
> +    // Try as hard as possible to make this a layer so we can scroll it
> +    // asynchronously.
> 
> It's more like "force this layer active even if it causes incorrect rendering
> so we can...".

Done.

> 
>  #define NS_IPRESSHELL_IID     \
> - { 0xd1978bee, 0x43b9, 0x40de, \
> -    { 0x95, 0x47, 0x85, 0x06, 0x5e, 0x02, 0xec, 0xb4 } }
> + { 0x7e88db33, 0x2766, 0x488a, \
> +    { 0xaf, 0xce, 0x1e, 0x39, 0x02, 0xf3, 0x2f, 0x0c } }
> 
> I don't know when you are planning on landing this, but the presshell IID is
> frozen right now, so you should be doing this to the mozilla 2.0 branch
> presshell. If you want to land after the IIDs get unfrozen this is fine (I
> don't know when or how that will happen).

After thinking about this, I believe I would have had to keep the old displayport functions as not virtual and make new names for them. It was weird to have two different places in platform to set the displayport anyway. I removed the presshell stuff altogether and have now made it deprecated. Callers should now use nsLayoutUtils.

Chris: does this sound fine to you?

> 
> +PRBool PresShell::UsingDisplayPort() const
> +{
> +  nsIFrame* rootFrame = GetRootScrollFrame();
> 
> Call it rootScrollFrame, it'll reduce confusion. And same for GetDisplayPort.

Done.

> 
> diff --git a/layout/generic/nsGfxScrollFrame.h
> b/layout/generic/nsGfxScrollFrame.h
> +  bool GetDisplayPort(nsRect* aResult) const;
> 
> This seems to be dead code.

Fixed.

> 
> diff --git a/layout/generic/nsSubDocumentFrame.cpp
> b/layout/generic/nsSubDocumentFrame.cpp
> -      subdocRootScrollFrame->AddStateBits(
> -        NS_FRAME_FORCE_DISPLAY_LIST_DESCEND_INTO);
> 
> We don't need this anymore?

Nope. This was a side effect of putting the displayport display item a few layers outside of the gfx scroll frame.

> 
> @@ -1858,41 +1858,82 @@ nsGfxScrollFrameInner::BuildDisplayList(
> +  if (usingDisplayPort)
> +    dirtyRect = displayport;
> 
> Add comments about why we need this?

Done. I am now wondering: does this mess up small invalidations so that everything in the displayport has to be redrawn? Maybe we need to do better than this. :/

> 
> @@ -1858,41 +1858,82 @@ nsGfxScrollFrameInner::BuildDisplayList(
> +  bool buildingLayer =
> +    (XRE_GetProcessType() == GeckoProcessType_Content &&
> +     (scrollRange.width >= 20 || scrollRange.height >= 20)) &&
> +     (!mIsRoot || !mOuter->PresContext()->IsRootContentDocument());
> 
> +  if (buildingLayer) {
> 
> +    if (scrollRange.width >= 20 || scrollRange.height >= 20) {
> 
> If buildingLayer is true then isn't scrollRange.width >= 20 ||
> scrollRange.height >= 20 also true?
> 
> Why 20? Since this is in appunits thats usually 1/3 of a pixel.

Err oops, I meant this to be in app units.

This is pretty necessary for me. For instance, on reader.google.com, This was sort of a cutoff decision, because when I first wrote this I was seeing a ton of frames with very small scroll ranges that Fennec wouldn't want to scroll asynchronously. 

For instance, on reader.google.com I see lots of scroll boxes with no scroll ranges or just 4 or 5 pixels of scroll.

Are you OK with a hard coded constant at the top of the file? Does 20px sound OK to you? Or do we need something better here?

> 
> @@ -1858,41 +1858,82 @@ nsGfxScrollFrameInner::BuildDisplayList(
> +    rv = mScrolledFrame->BuildDisplayListForStackingContext(
> +      aBuilder,
> +      dirtyRect - mScrolledFrame->GetOffsetTo(mOuter),
> 
> dirtyRect + mOuter->GetOffsetTo(mScrolledFrame) is clearer I think.
> 
> Using BuildDisplayListForStackingContext instead of BuildDisplayListForChild
> must break some things, although I'm not sure how serious a problem this is.

It does; see comment number 4.

> And we need to rename the variables called viewportframe, this can be in a
> patch before or after this patch, whatever is easiest.

See comment for you below about the renaming.

(In reply to comment #21)
> Comment on attachment 515953 [details] [diff] [review]
> Pan overflow elements in parent process
> 
> In addition to tn's comments:
> 
> I think these changes mean it's no longer possible to unset a
> displayport from the root document, but I also don't think I care.

This is true, though it was never possible for frontend. I'm fine with this as well.

> 
> >diff --git a/dom/base/nsDOMWindowUtils.cpp b/dom/base/nsDOMWindowUtils.cpp
> >--- a/dom/base/nsDOMWindowUtils.cpp
> >+++ b/dom/base/nsDOMWindowUtils.cpp
> > NS_IMETHODIMP
> > nsDOMWindowUtils::SetDisplayPort(float aXPx, float aYPx,
> >-                                 float aWidthPx, float aHeightPx)
> >+                                 float aWidthPx, float aHeightPx,
> >+                                 nsIDOMElement* aElement)
> >+  nsCOMPtr<nsIContent> content = do_QueryInterface(aElement);
> 
> Need to null-check |content|.

Done.

> 
> >+  nsRect lastDisplayPort;
> >+  if (nsLayoutUtils::GetDisplayPort(content, &lastDisplayPort) &&
> >+      displayport == lastDisplayPort) {
> >+    return NS_OK;
> >+  }
> >+
> >+  content->SetProperty(nsGkAtoms::DisplayPort, new nsRect(displayport),
> >+                       DestroyNsRect);
> > 
> >   presShell->SetIgnoreViewportScrolling(PR_TRUE);
> 
> This is a somewhat undefined state; this call could be said to be
> wrong if there's not a displayport set for the root document.  But
> we'll of course always assume that, so it's probably worth noting in
> the docs.

Sorry, which docs should we note this in?

> >diff --git a/dom/interfaces/base/nsIDOMWindowUtils.idl b/dom/interfaces/base/nsIDOMWindowUtils.idl
> >--- a/dom/interfaces/base/nsIDOMWindowUtils.idl
> >+++ b/dom/interfaces/base/nsIDOMWindowUtils.idl
> 
> Need an IID bump here.  Probably easier to stick this on the
> MOZILLA_2_0_BRANCH interface rather than trying to outfox
> mozilla-central.

No longer necessary with new patch.

> 
> >@@ -124,43 +124,51 @@ interface nsIDOMWindowUtils : nsISupport
> >    * This will trigger reflow.
> >    *
> >    * The caller of this method must have UniversalXPConnect
> >    * privileges.
> >    */
> >   void setCSSViewport(in float aWidthPx, in float aHeightPx);
> > 
> >   /**
> >-   * Set the "displayport" to be <xPx, yPx, widthPx, heightPx> in
> >-   * units of CSS pixels, regardless of the size of the enclosing
> >-   * widget/view.  This will *not* trigger reflow.
> >+   * For any scrollable element, this allows you to override the
> >+   * visible region and draw more than what is visible, which is
> >+   * useful for asynchrnous drawing. The "displayport" will be
> 
> Typo.

Fixed.

> 
> >    *
> >-   * The displayport will be used as the window's visible region for
> >-   * the purposes of invalidation and painting.  The displayport can
> >-   * approximately be thought of as a "persistent" drawWindow()
> >-   * (albeit with coordinates relative to the CSS viewport): the
> >-   * bounds are remembered by the platform, and layer pixels are
> >+   * The displayport will be used as the area's visible region for
> >+   * the purposes of invalidation and painting.  The displayport for
> >+   * the root element can approximately be thought of as a "persistent"
> >+   * drawWindow() (albeit with coordinates relative to the CSS viewport):
> >+   * the bounds are remembered by the platform, and layer pixels are
> >    * retained and updated inside the viewport bounds.
> 
> The CSS viewport is mostly irrelevant for displayports set on <div>s etc.

That's why I added "for the root element" because this still seems like a useful way to think of things. Should I remove it altogether?

> I think the term we want is "overflow area", but I'm not 100% sure.

That seems right. Changed to overflow.

> Hm, this "viewport" name is unfortunate now, but can fix in a
> low-priority followup.

Timothy, you good with this plan?

> NS_ABORT_IF_FALSE() disappears in opt builds.  Don't think that's what
> was intended here.  You probably instead want to assert
> UsingDisplayPort() in debug builds on entry to this method.

This code has been removed.

> Need to note in the API docs that garbage is returned if
> !UsingDisplayPort(), it's a precondition.

This now does a runtime abort. Let me know what I should use here.
Attachment #515953 - Attachment is obsolete: true
Attachment #515953 - Flags: review?(tnikkel)
Attachment #517647 - Flags: review?(tnikkel)
Attachment #517647 - Flags: review?(jones.chris.g)
(In reply to comment #22)
> > @@ -1858,41 +1858,82 @@ nsGfxScrollFrameInner::BuildDisplayList(
> > +  if (usingDisplayPort)
> > +    dirtyRect = displayport;
> > 
> > Add comments about why we need this?
> 
> Done. I am now wondering: does this mess up small invalidations so that
> everything in the displayport has to be redrawn? Maybe we need to do better
> than this. :/

Oops, I meant to comment about this issue. This is going to cause a lot of extra painting.

> > 
> > @@ -1858,41 +1858,82 @@ nsGfxScrollFrameInner::BuildDisplayList(
> > +  bool buildingLayer =
> > +    (XRE_GetProcessType() == GeckoProcessType_Content &&
> > +     (scrollRange.width >= 20 || scrollRange.height >= 20)) &&
> > +     (!mIsRoot || !mOuter->PresContext()->IsRootContentDocument());
> > 
> > +  if (buildingLayer) {
> > 
> > +    if (scrollRange.width >= 20 || scrollRange.height >= 20) {
> > 
> > If buildingLayer is true then isn't scrollRange.width >= 20 ||
> > scrollRange.height >= 20 also true?
> > 
> > Why 20? Since this is in appunits thats usually 1/3 of a pixel.
> 
> Err oops, I meant this to be in app units.
> 
> This is pretty necessary for me. For instance, on reader.google.com, This was
> sort of a cutoff decision, because when I first wrote this I was seeing a ton
> of frames with very small scroll ranges that Fennec wouldn't want to scroll
> asynchronously. 
> 
> For instance, on reader.google.com I see lots of scroll boxes with no scroll
> ranges or just 4 or 5 pixels of scroll.
> 
> Are you OK with a hard coded constant at the top of the file? Does 20px sound
> OK to you? Or do we need something better here?

That's fine, just put a comment explaining this.

The other point that I was trying to make was that the if (scrollRange.width >= 20 || scrollRange.height >= 20) check was redundant, in that if we get there it will always be true. Maybe I'm misreading the code though.

> > Hm, this "viewport" name is unfortunate now, but can fix in a
> > low-priority followup.
> 
> Timothy, you good with this plan?

Sure, as long as it gets done.
(In reply to comment #22)
> > This is a somewhat undefined state; this call could be said to be
> > wrong if there's not a displayport set for the root document.  But
> > we'll of course always assume that, so it's probably worth noting in
> > the docs.
> 
> Sorry, which docs should we note this in?

SetDisplayPort().

> > The CSS viewport is mostly irrelevant for displayports set on <div>s etc.
> 
> That's why I added "for the root element" because this still seems like a
> useful way to think of things. Should I remove it altogether?

Yeah, I'd drop it.

> > Need to note in the API docs that garbage is returned if
> > !UsingDisplayPort(), it's a precondition.
> 
> This now does a runtime abort. Let me know what I should use here.

NS_ABORT_IF_FALSE() is fine for the precondition check, this doesn't need to be a fatal error in release builds.
Blocks: 639938
> I went ahead and tried to do some trailing space busting.

Please put this sort of thing in separate patches, with no substantive changes, if you decide to do it?
> Oops, I meant to comment about this issue. This is going to cause a lot of
> extra painting.

How so? Roc mentioned that this won't cause issues for invalidations, but it will need mean more painting. Are you saying there's more painting here than there needs to be?

BTW, this is exactly what happens for the root displayport now:
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsLayoutUtils.cpp#1457

> That's fine, just put a comment explaining this.

Done.

> The other point that I was trying to make was that the if (scrollRange.width >=
> 20 || scrollRange.height >= 20) check was redundant, in that if we get there it
> will always be true. Maybe I'm misreading the code though.

What makes you think this? I've outputted the scroll range and many times I see a range of 0, 0. Perhaps there is a deeper issue here.

> Sure, as long as it gets done.

Filed bug 618975.

(In reply to comment #25)
> (In reply to comment #22)
> > > This is a somewhat undefined state; this call could be said to be
> > > wrong if there's not a displayport set for the root document.  But
> > > we'll of course always assume that, so it's probably worth noting in
> > > the docs.
> > 
> > Sorry, which docs should we note this in?
> 
> SetDisplayPort().

Oh, I see what you mean now. Why not just only set this flag for root displayports then? New patch does this now

> Yeah, I'd drop it.

Done.

> NS_ABORT_IF_FALSE() is fine for the precondition check, this doesn't need to be
> a fatal error in release builds.

Done.

> Please put this sort of thing in separate patches, with no substantive changes,
> if you decide to do it?

Will do next time. I gave up for this bug. Maybe I'll try again another time. On a related note, I don't understand the blame argument; it seems much more important that we have consistent clean code than having blame always point to something meaningful. We can build tools for the latter problem.
Attachment #517647 - Attachment is obsolete: true
Attachment #517647 - Flags: review?(tnikkel)
Attachment #517647 - Flags: review?(jones.chris.g)
Attachment #517827 - Flags: review?(tnikkel)
Attachment #517827 - Flags: review?(jones.chris.g)
(In reply to comment #27)
> > The other point that I was trying to make was that the if (scrollRange.width >=
> > 20 || scrollRange.height >= 20) check was redundant, in that if we get there it
> > will always be true. Maybe I'm misreading the code though.
> 
> What makes you think this? I've outputted the scroll range and many times I see
> a range of 0, 0. Perhaps there is a deeper issue here.

Because the "if (scrollRange.width >= 20 || scrollRange.height >= 20)" is inside "if (buildingLayer)" and the definition of buildingLayer has a condition on scrollRange that I think would make "(scrollRange.width >= 20 || scrollRange.height >= 20)" always true when buildingLayer is true. Am I missing something?
Sorry I see what you are saying now! That is indeed a mistake.
Attachment #517827 - Attachment is obsolete: true
Attachment #517827 - Flags: review?(tnikkel)
Attachment #517827 - Flags: review?(jones.chris.g)
Attachment #517868 - Flags: review?(tnikkel)
Attachment #517868 - Flags: review?(jones.chris.g)
(In reply to comment #27)
> > Oops, I meant to comment about this issue. This is going to cause a lot of
> > extra painting.
> 
> How so? Roc mentioned that this won't cause issues for invalidations, but it
> will need mean more painting. Are you saying there's more painting here than
> there needs to be?
> 
> BTW, this is exactly what happens for the root displayport now:
> http://mxr.mozilla.org/mozilla-central/source/layout/base/nsLayoutUtils.cpp#1457

I was correct the first time, I didn't need to add a review comment for that. :) When you mentioned it I didn't see the context and thought it was in Invalidate(). Sorry for the confusion.
Comment on attachment 517868 [details] [diff] [review]
Pan overflow elements in parent process

+  virtual LayerState GetLayerState(nsDisplayListBuilder* aBuilder,
+                                   LayerManager* aManager)
+  {
+    // Force this as a layer so we can scroll asynchronously.
+    return mozilla::LAYER_ACTIVE_FORCE;
+  }

The comment should mention that this can cause incorrect rendering as we are making a trade-off here, we should make it clear in the code.

Now that those methods on nsIPresShell are dead code we should mark them as such so they have a better chance of eventually getting deleted.

> @@ -1858,41 +1858,82 @@ nsGfxScrollFrameInner::BuildDisplayList(
> +    rv = mScrolledFrame->BuildDisplayListForStackingContext(
> +      aBuilder,
> +      dirtyRect - mScrolledFrame->GetOffsetTo(mOuter),
> 
> dirtyRect + mOuter->GetOffsetTo(mScrolledFrame) is clearer I think.

This didn't get addressed.

+  // For some reason, sometimes gfx scroll frames are generated with a scroll
+  // range that is 0, 0.

This makes it sound like a mystery. Its done on purpose: anything that might scroll gets a scroll frame. If it doesn't have enough content it has no scroll range.
Comment on attachment 517868 [details] [diff] [review]
Pan overflow elements in parent process

I'd like roc to sign off on the forcing of a layer and how it is done and the use of BuildDisplayListForStackingContext in nsGfxScrollFrameInner::BuildDisplayList.
Attachment #517868 - Flags: superreview?(roc)
Attachment #517868 - Flags: review?(tnikkel)
Attachment #517868 - Flags: review+
+  bool usingDisplayPort = nsLayoutUtils::GetDisplayPort(mOuter->GetContent(),
+  bool buildingLayer =

PRBool in layout

+  if (usingDisplayPort)
+    dirtyRect = displayport;

{}

+    // Note that using StackingContext breaks z order, so the resulting
+    // rendering can be incorrect for weird edge cases!

Yeah, this isn't good :-(.

How about calling mOuter->BuildDisplayListForChild(aBuilder, mScrolledFrame, dirtyRect, set), then examine the display list set that you get. If all of the display items are in, say, the Content() list (the other lists are empty) then it's safe to do this, otherwise it isn't. I don't really want to just break the rendering here. The question is whether that captures the cases you care about.

If we do that, we should leave in the code in nsSubDocumentFrame since it's always safe to mash things together there.
> The comment should mention that this can cause incorrect rendering as we are
> making a trade-off here, we should make it clear in the code.

Done.

> Now that those methods on nsIPresShell are dead code we should mark them as
> such so they have a better chance of eventually getting deleted.

Done.

> 
> > @@ -1858,41 +1858,82 @@ nsGfxScrollFrameInner::BuildDisplayList(
> > +    rv = mScrolledFrame->BuildDisplayListForStackingContext(
> > +      aBuilder,
> > +      dirtyRect - mScrolledFrame->GetOffsetTo(mOuter),
> > 
> > dirtyRect + mOuter->GetOffsetTo(mScrolledFrame) is clearer I think.
> 
> This didn't get addressed.

Sorry. Done.

> 
> +  // For some reason, sometimes gfx scroll frames are generated with a scroll
> +  // range that is 0, 0.
> 
> This makes it sound like a mystery. Its done on purpose: anything that might
> scroll gets a scroll frame. If it doesn't have enough content it has no scroll
> range.

Rephrased.
> PRBool in layout

Done.

> 
> +  if (usingDisplayPort)
> +    dirtyRect = displayport;
> 
> {}

Done.

> 
> +    // Note that using StackingContext breaks z order, so the resulting
> +    // rendering can be incorrect for weird edge cases!
> 
> Yeah, this isn't good :-(.
> 
> How about calling mOuter->BuildDisplayListForChild(aBuilder, mScrolledFrame,
> dirtyRect, set), then examine the display list set that you get. If all of the
> display items are in, say, the Content() list (the other lists are empty) then
> it's safe to do this, otherwise it isn't. I don't really want to just break the
> rendering here. The question is whether that captures the cases you care about.

Hm, I think there were some cases on reader.google.com that seemed to have stuff not in Content() list, but still rendered fine with this approach. It was images that were position: relative I think, but the elements were not intersecting with anything outside of the scroll area.

I think we'd have to have a more in-depth check than this, that looks for z-order elements outside of the scroll area that intersect with the z-order elements inside. Sounds pretty terrible to do.

> If we do that, we should leave in the code in nsSubDocumentFrame since it's
> always safe to mash things together there.

I'm not sure this is necessary, but we should figure out the above first.
Comment on attachment 517868 [details] [diff] [review]
Pan overflow elements in parent process

Still need an IID rev on nsIDOMWindowUtils, which probably means the new interface needs to go on nsIDOMWindowUtils_MOZILLA_2_0_BRANCH.
Attachment #517868 - Flags: review?(jones.chris.g)
(In reply to comment #38)
> Comment on attachment 517868 [details] [diff] [review]
> Pan overflow elements in parent process
> 
> Still need an IID rev on nsIDOMWindowUtils, which probably means the new
> interface needs to go on nsIDOMWindowUtils_MOZILLA_2_0_BRANCH.

Sorry again. I somehow mixed the nsIDOMWindowUtils discussion with nsIPresShell in my head. Done.
(In reply to comment #37)
> I think we'd have to have a more in-depth check than this, that looks for
> z-order elements outside of the scroll area that intersect with the z-order
> elements inside. Sounds pretty terrible to do.

Yeah. But we can't just start violating CSS here, at least not as a long-term solution. So we need a plan.
Comment on attachment 517868 [details] [diff] [review]
Pan overflow elements in parent process

I'm willing to OK this as a temporary measure but we have to fix this ASAP.
Attachment #517868 - Flags: superreview?(roc) → superreview+
Filed follow-up bug 640048.
Blocks: 640048
The current best idea for fixing this properly is to use nsDisplayWrapList to wrap every child display item or display item list in a different nsDisplayScrollLayer. Then, during display list flattening, track whether we've managed to merge all nsDisplayScrollLayers for a given nsGfxScrollFrame together into a single item. If we haven't, then remove them all to effectively make the scrollable element not async-scrollable.
Attachment #517868 - Attachment is obsolete: true
Attachment #517943 - Flags: superreview+
Attachment #517943 - Flags: review?(jones.chris.g)
Attachment #517943 - Flags: review?(jones.chris.g) → review+
Pushed mobile: http://hg.mozilla.org/mobile-browser/rev/47065e6de864
Pushed m-c: http://hg.mozilla.org/mozilla-central/rev/99f6b3acc464
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
caused a crash/regression:

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1299649970.1299650086.12070.gz&fulltext=1
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [needs-review (tnikkel, cjones)] → needs new patch
Looks like the reftest harness support for testing displayports needs to be updated for the changes in this bug.
Attachment #517943 - Attachment is obsolete: true
Comment on attachment 518543 [details] [diff] [review]
Pan overflow elements in parent process   s

I've fixed the obvious setDisplayPort problems, but there's still problems on try. I can't reproduce them locally because my crashtests crash before the problem, and even crash without the patch.

I don't know what to do next. This is very at risk.
Attachment #518543 - Flags: superreview+
Attachment #518543 - Flags: review+
this runtime abort seems to be because the layer's viewport is unreasonably tall. destBufferDims is {width = 771, height = 17895716} because that is the size aLayer->GetVisibleRegion() returns here: https://mxr.mozilla.org/mozilla-central/source/gfx/layers/ThebesLayerBuffer.cpp#238

The question is, what about this patch is making the visible region so tall?

here's the stack trace:
#0  0x00002abcb1438ba5 in raise (sig=<value optimized out>) at ../nptl/sysdeps/unix/sysv/linux/raise.c:64
#1  0x00002abcb143c6b0 in abort () at abort.c:92
#2  0x00002abcae36ff5e in mozalloc_abort (msg=<value optimized out>) at ../../../memory/mozalloc/mozalloc_abort.cpp:75
#3  0x00002abcaf5cbc66 in Abort (aSeverity=70731680, aStr=<value optimized out>, aExpr=<value optimized out>, aFile=0x2abcafb9351d "../../../gfx/layers/basic/BasicLayers.cpp", 
    aLine=<value optimized out>) at ../../../xpcom/base/nsDebugImpl.cpp:393
#4  NS_DebugBreak_P (aSeverity=70731680, aStr=<value optimized out>, aExpr=<value optimized out>, aFile=0x2abcafb9351d "../../../gfx/layers/basic/BasicLayers.cpp", 
    aLine=<value optimized out>) at ../../../xpcom/base/nsDebugImpl.cpp:380
#5  0x00002abcaf6563b8 in mozilla::layers::BasicShadowableThebesLayer::CreateBuffer (this=0x2abcc1f56400, aType=gfxASurface::CONTENT_COLOR_ALPHA, aSize=...)
    at ../../../gfx/layers/basic/BasicLayers.cpp:1926
#6  0x00002abcaf652c11 in mozilla::layers::BasicThebesLayerBuffer::CreateBuffer (this=<value optimized out>, aType=4425, aSize=...)
    at ../../../gfx/layers/basic/BasicLayers.cpp:674
#7  0x00002abcaf65c6be in mozilla::layers::ThebesLayerBuffer::BeginPaint (this=0x2abcc1f56598, aLayer=<value optimized out>, aContentType=<value optimized out>, 
    aXResolution=<value optimized out>, aYResolution=<value optimized out>, aFlags=<value optimized out>) at ../../../gfx/layers/ThebesLayerBuffer.cpp:366
#8  0x00002abcaf656ede in mozilla::layers::BasicThebesLayer::PaintThebes (this=0x2abcc1f56400, aContext=0x2abcbe349660, 
    aCallback=0x2abcaec6655a <mozilla::FrameLayerBuilder::DrawThebesLayer(mozilla::layers::ThebesLayer*, gfxContext*, nsIntRegion const&, nsIntRegion const&, void*)>, 
    aCallbackData=<value optimized out>, aReadback=<value optimized out>) at ../../../gfx/layers/basic/BasicLayers.cpp:595
#9  0x00002abcaf654011 in mozilla::layers::BasicLayerManager::PaintLayer (this=0x2abcb8e22de0, aLayer=0x2abcc1f56400, aCallback=<value optimized out>, 
    aCallbackData=<value optimized out>, aReadback=<value optimized out>) at ../../../gfx/layers/basic/BasicLayers.cpp:1500
#10 0x00002abcaf65406a in mozilla::layers::BasicLayerManager::PaintLayer (this=0x2abcb8e22de0, aLayer=0x2abcb8e54860, aCallback=<value optimized out>, 
    aCallbackData=<value optimized out>, aReadback=<value optimized out>) at ../../../gfx/layers/basic/BasicLayers.cpp:1513
#11 0x00002abcaf65406a in mozilla::layers::BasicLayerManager::PaintLayer (this=0x2abcb8e22de0, aLayer=0x2abcb8e54350, aCallback=<value optimized out>, 
    aCallbackData=<value optimized out>, aReadback=<value optimized out>) at ../../../gfx/layers/basic/BasicLayers.cpp:1513
#12 0x00002abcaf654c1e in mozilla::layers::BasicLayerManager::EndTransactionInternal (this=0x2abcb8e22de0, 
    aCallback=0x2abcaec6655a <mozilla::FrameLayerBuilder::DrawThebesLayer(mozilla::layers::ThebesLayer*, gfxContext*, nsIntRegion const&, nsIntRegion const&, void*)>, 
    aCallbackData=0x7fff04375b00) at ../../../gfx/layers/basic/BasicLayers.cpp:1365
#13 0x00002abcaf6575a5 in mozilla::layers::BasicShadowLayerManager::EndTransaction (this=0x1149, aCallback=0x1149, aCallbackData=0x6)
    at ../../../gfx/layers/basic/BasicLayers.cpp:2785
#14 0x00002abcaec8b5f1 in nsDisplayList::PaintForFrame (this=<value optimized out>, aBuilder=0x7fff04375b00, aCtx=<value optimized out>, aForFrame=0x2abcc1b2f650, 
    aFlags=<value optimized out>) at ../../../layout/base/nsDisplayList.cpp:553
#15 0x00002abcaec9e02c in nsLayoutUtils::PaintFrame (aRenderingContext=0x0, aFrame=0x2abcc1b2f650, aDirtyRegion=<value optimized out>, aBackstop=<value optimized out>, 
    aFlags=260) at ../../../layout/base/nsLayoutUtils.cpp:1601
#16 0x00002abcaecb00d1 in PresShell::Paint (this=0x2abcbb93b400, aDisplayRoot=<value optimized out>, aViewToPaint=0x2abcc16bf780, aWidgetToPaint=0x2abcbb26aa40, 
    aDirtyRegion=<value optimized out>, aIntDirtyRegion=<value optimized out>, aPaintDefaultBackground=0, aWillSendDidPaint=0) at ../../../layout/base/nsPresShell.cpp:6164
#17 0x00002abcaef7b65f in nsViewManager::RenderViews (this=0x2abcc1b7beb0, aView=0x2abcc16bf780, aWidget=0x2abcbb26aa40, aRegion=<value optimized out>, 
    aIntRegion=<value optimized out>, aPaintDefaultBackground=<value optimized out>, aWillSendDidPaint=0) at ../../../view/src/nsViewManager.cpp:458
#18 0x00002abcaef7b733 in nsViewManager::Refresh (this=0x2abcc1b7beb0, aView=0x2abcc16bf780, aWidget=0x2abcbb26aa40, aRegion=..., aUpdateFlags=1)
    at ../../../view/src/nsViewManager.cpp:424
#19 0x00002abcaef7c94d in nsViewManager::DispatchEvent (this=0x2abcc1b7beb0, aEvent=0x7fff04376460, aView=0x2abcc16bf780, aStatus=<value optimized out>)
    at ../../../view/src/nsViewManager.cpp:925
#20 0x00002abcaef79103 in HandleEvent (aEvent=0x7fff04376460) at ../../../view/src/nsView.cpp:161
#21 0x00002abcaf475480 in mozilla::widget::PuppetWidget::DispatchEvent (this=0x2abcbb26aa40, event=0x7fff04376460, aStatus=@0x7fff0437650c)
    at ../../../../widget/src/xpwidgets/PuppetWidget.cpp:322
#22 0x00002abcaf476219 in mozilla::widget::PuppetWidget::DispatchPaintEvent (this=0x2abcbb26aa40) at ../../../../widget/src/xpwidgets/PuppetWidget.cpp:532
#23 0x00002abcaf4762bc in mozilla::widget::PuppetWidget::PaintTask::Run (this=<value optimized out>) at ../../../../widget/src/xpwidgets/PuppetWidget.cpp:571
#24 0x00002abcaf5c5230 in nsThread::ProcessNextEvent (this=0x2abcb8e0e660, mayWait=0, result=0x7fff043765cc) at ../../../xpcom/threads/nsThread.cpp:633
#25 0x00002abcaf591815 in NS_ProcessNextEvent_P (thread=0x1149, mayWait=4425) at nsThreadUtils.cpp:250
#26 0x00002abcaf51dcc4 in mozilla::ipc::MessagePump::Run (this=0x2abcb8e0f440, aDelegate=0x7fff04377780) at ../../../ipc/glue/MessagePump.cpp:110
#27 0x00002abcaf5f6cee in RunHandler (this=0x1149) at ../../../ipc/chromium/src/base/message_loop.cc:202
#28 MessageLoop::Run (this=0x1149) at ../../../ipc/chromium/src/base/message_loop.cc:176
#29 0x00002abcaf46637f in nsBaseAppShell::Run (this=0x2abcbf1e9970) at ../../../../widget/src/xpwidgets/nsBaseAppShell.cpp:192
#30 0x00002abcaeb42ba0 in XRE_RunAppShell () at ../../../toolkit/xre/nsEmbedFunctions.cpp:678
#31 0x00002abcaf5f6cee in RunHandler (this=0x1149) at ../../../ipc/chromium/src/base/message_loop.cc:202
#32 MessageLoop::Run (this=0x1149) at ../../../ipc/chromium/src/base/message_loop.cc:176
#33 0x00002abcaeb43031 in XRE_InitChildProcess (aArgc=2, aArgv=<value optimized out>, aProcess=<value optimized out>) at ../../../toolkit/xre/nsEmbedFunctions.cpp:515
---Type <return> to continue, or q <return> to quit---
#34 0x0000000000401284 in main (argc=3, argv=<value optimized out>) at ../../../ipc/app/MozillaRuntimeMain.cpp:80
The visible layers should not be bigger than the displayport. Is the displayport unreasonably large?
Attached patch patch (obsolete) — Splinter Review
this change fixes the crash test bustage. However, when I updated my tree to pick up bug 593243 painting during panning breaks.
Attachment #518543 - Attachment is obsolete: true
Attachment #519094 - Flags: review?(jones.chris.g)
Attachment #519094 - Flags: review?(ben)
(In reply to comment #52)
> Created attachment 519094 [details] [diff] [review]
> patch
> 
> this change fixes the crash test bustage. However, when I updated my tree to
> pick up bug 593243 painting during panning breaks.

What was the change in this version?

After bug 593243 you'll need to make some changes to this patch. Basically take the nsGfxScrollFrame changes from bug 593243 and make them aware that any scroll frame can have a display port set.
(In reply to comment #53)
> After bug 593243 you'll need to make some changes to this patch. Basically take
> the nsGfxScrollFrame changes from bug 593243 and make them aware that any
> scroll frame can have a display port set.

Instead of just the root scroll frame being able to have a display port set.
here's a try run with this patch and bug 593243 backed out. Its as green as things get now-a-days
http://tbpl.mozilla.org/?tree=MozillaTry&rev=65a9b2b724ec
(In reply to comment #53)
> What was the change in this version?
the change from the last patch is in nsDisplayScrollLayer::ComputeVisibility, using nsLayoutUtils::GetDisplayPort(mFrame->GetContent(), &displayPort) to get the display port rather than mDisplayPort
(In reply to comment #56)
> (In reply to comment #53)
> > What was the change in this version?
> the change from the last patch is in nsDisplayScrollLayer::ComputeVisibility,
> using nsLayoutUtils::GetDisplayPort(mFrame->GetContent(), &displayPort) to get
> the display port rather than mDisplayPort

Thanks for finding this! I need to think about this change after my brain warms up because I remember there was a reason nsDisplayScrollLayer uses mDisplayPort the way it does.

Working on this and the invalidation dimensions now.
Attachment #515213 - Attachment is obsolete: true
Attached patch Mobile patch (obsolete) — Splinter Review
Fleshed out blassey's idea by removing mDisplayport from nsDisplayScrollLayer and just using the content of the frame.

Currently on Fennec I'm seeing a problem with the displayport and not factoring in scroll offset. The current mobile patch has setScrollOffset commented out. I'm looking into this issue now.

After this, I'll ask for review and re-base on top of the reviewed patch.
Something strange is going on. Even if I never set display port, setting scrollTop for my div seems to have no effect.
Refreshing the page updates the scroll area properly. Must be something with invalidateinternal?
Attachment #519184 - Attachment is obsolete: true
Attached patch Mobile patch (obsolete) — Splinter Review
Attachment #519225 - Attachment is obsolete: true
Attachment #519183 - Attachment description: Pan overflow elements in parent process s → Pan overflow elements in parent process
Attachment #519183 - Flags: superreview+
Attachment #519183 - Flags: review+
Attachment #519226 - Flags: review+
Really sorry, something went wrong with bz export :( Please ignore giant comment.
Comment on attachment 519094 [details] [diff] [review]
patch

I get the feeling that this is some bug in ThebesLayerBuffer that causes the contents of the layer that contains the scroll area to get corrupted. My suspicions come from:
1) InvalidateInternal is getting called just fine for nsGfxScrollFrame, with a sufficient invalidation. Commenting out the intersection with the displayport/scrollframe doesn't fix the issue.
2) nsDisplayScrollLayer is getting constructed for painting appropriately.
3) Invalidating the entire region directly in ThebesLayerBuffer::BeginPaint by using result.mRegionToInvalidate fixes the problem.

Is there an easy way to turn off self-copy completely in ThebesLayerBuffer?
Attachment #519094 - Attachment is obsolete: true
Attachment #519094 - Flags: review?(jones.chris.g)
Attachment #519094 - Flags: review?(ben)
Other important points:
1) NSPR_LOG_MODULES=Layers:5 output seems fine.
2) Making the scrolled layer opaque fixes the problem (I changed OptimalFormatFor in ShadowLayers.cpp).

This sounds similar to the problem Alon was seeing in bug 641538, since the difference between stock Linux desktop MOZ_GFX_OPTIMIZE_MOBILE (or whatever its name is) is an alpha channel.
The good news is that it looks like we are passing on tryserver!
http://tbpl.mozilla.org/?tree=MozillaTry&rev=3be7f216863a

The bad news is we are still having graphics corruption issues with scrolling. This has corruption problems:
http://people.mozilla.org/~bstover/scrolling.html

While this seems to work fine:
http://people.mozilla.org/~bstover/scrolling2.html

I've asked Timothy to look over the followup code, and he didn't find anything that would cause this. ThebesLayerBuffer seems to be working correctly as well. I think my next step is to get a test case that reproduces this problem with test-ipcbrowser so that somebody smarter than me can help!
Attached file Test case (obsolete) —
This is a different problem than what I'm seeing in Fennec, but I think it must be related. This testcase automatically starts scrolling two scroll boxes, and they both behave incorrectly by not rendering the new content. Reproducible with test-ipcbrowser.

Chris, would you be up for importing the platform patches and trying the test-case? I'm not having any luck fixing this.
(In reply to comment #65)
> Created attachment 519227 [details] [diff] [review]
> Followup: bitrot and remove mDisplayport from nsDisplayScrollLayer

This doesn't apply to m-c tip.
Attachment #519227 - Attachment is obsolete: true
Attachment #519183 - Attachment is obsolete: true
Attachment #519286 - Flags: superreview+
Attachment #519286 - Flags: review+
Still applies for me, but re-uploaded just to make sure. Are you applying both patches?
Gah, I think this test-case does expose a bug, though not anything helpful. If no displayport is set, we still let the parent process scroll independently of the child process, which would make it look like content is disappearing. Ideally we should build a layer but not scroll independently if there is no displayport set.

To reproduce the more concerning bug, I'll need to modify test-ipcbrowser and make a sample test case.
This contains a test (chrome://global/content/test-subframescroll.xul) that can reproduce the problem. The timeout for the scroll is necessary, for some unknown reason.
Attachment #519287 - Attachment is obsolete: true
Just to note here, the Linux try-run has finished and part 5 has an error with scrolling that doesn't look like a random orange.
I'm not feeling 100% tonight and haven't got this sorted yet, but I should post what I've found out so far before crashing

 - We're drawing the wrong content into the thebes layer for the <div> in this test.  This log shows where things go wrong http://pastebin.mozilla.org/1163845; it's from the paint after setting div.scrollTop = 30 ("SRC" is the old buffer, "DEST" is the new).  The buffer rect for the layer before the scroll is at y=45, but it's y=15 *after* the scroll.  That's just wrong, and because of that we copy the pixels from the old to new buffer into the wrong y offset.  Not sure what the cause is yet, looks like a coordinate system is changing behind the thebes layer's back.

 - There's no support for drawing a checkerboard behind subframes scrolled out-of-bounds, AFAICT.  That's actually Pretty Hard.  Seems OK though because the frontend apparently doesn't allow panning iframes to undefined content, but this test ends up doing that, so I'm not quite sure what pixels would end up being drawn in the undefined space.
Attachment #519262 - Attachment is obsolete: true
No idea why, but it has something to do with the active scroll layer. In nsGfxScrollFrameInner::ScrollVisual, if I set canScrollWithBlitting to true, then I see no problems.

I'm guessing that there is code that is assuming that the scroll frame is a layer if and only if it is active.
There are several things left to do in the bug. Status of this bug currently:
* Get code review for followup patch that addresses bit-rot and try failures.
** This includes a few changes Timothy spotted, and fixes async scroll for divs with no displayport. This part shouldn't take long.
* There is a try run orange that needs to be looked at: http://tbpl.mozilla.org/?tree=MozillaTry&rev=3be7f216863a. It could be related to issues that will be addressed in code review.
* Fixing the MarkActive bug. The easy fix is to make sure that all gfx's with a displayport defined are marked active during ScrollVisual, but I think there's an underlying offset issue. I think Timothy will know if this fix is OK.

There is still unknowns here. The best case is MarkActive is exactly what we need and code review addresses what I saw in the try run. This could land today in that case.
Just override IsScrollingActive to return true when you want to create a layer for the scrollframe.
This follow-up version has removed the test case and set any scroll frames to be
active if we are building a layer for it. We now also check for a displayport
set in RenderFrameParent. Fennec needs to be more careful about having too many
scroll frames with displayports, so we will likely need a way to unset
displayports in the future, but I do not think that it would need to block.
Attachment #519485 - Flags: review?(tnikkel)
Attachment #519485 - Flags: review?(jones.chris.g)
Attachment #519320 - Attachment is obsolete: true
Comment on attachment 519485 [details] [diff] [review]
Followup: bitrot and remove mDisplayport from nsDisplayScrollLayer

>diff --git a/dom/base/nsDOMWindowUtils.cpp b/dom/base/nsDOMWindowUtils.cpp
>   nsIFrame* rootScrollFrame = presShell->GetRootScrollFrame();
>+

Gratuitous whitespace change.

>+      // We are setting the root displayport for the root document. Therefore,
>+      // it is tied to a widget and the widget needs to know about the
>+      // displayport for invalidation reasons.

"The root document currently has a widget, but we might end up
painting content inside the displayport and outside the widget bounds.
So ensure the document's view honors invalidations within the
displayport."

>diff --git a/layout/generic/nsGfxScrollFrame.cpp b/layout/generic/nsGfxScrollFrame.cpp
>+      nsRect displayport;
>+      PRBool usingDisplayport = nsLayoutUtils::GetDisplayPort(GetContent(),
>+                                                              &displayport);

That function returns |bool|.  Hope this doesn't ding perf ...

>+    PRBool usingDisplayport = nsLayoutUtils::GetDisplayPort(GetContent(),

bool

>+                                                            &displayport);
>+    if (usingDisplayport) {
>+      parentDamage.IntersectRect(damage, displayport);
>     } else {
>       parentDamage.IntersectRect(damage, mInner.mScrollPort);
>     }
> 
>     if (IsScrollingActive()) {
>       // This is the damage rect that we're going to pass up and
>       // only request invalidation of ThebesLayers for.
>       // damage is now in our coordinate system, which means it was
>@@ -1705,16 +1709,20 @@ void nsGfxScrollFrameInner::ScrollVisual
>       MarkInactive();
>     } else {
>       flags |= nsIFrame::INVALIDATE_NO_THEBES_LAYERS;
>     }
>   }
>   if (canScrollWithBlitting) {
>     MarkActive();
>   }
>+
>+  nsRect invalidateRect = mScrollPort;
>+  nsLayoutUtils::GetDisplayPort(mOuter->GetContent(), &invalidateRect);

I don't like this style.  The following is a little more verbose

  nsRect displayport, invalidateRect;
  invalidateRect =
    nsLayoutUtils::GetDisplayPort(mOuter->GetContent(), &displayport) ? displayport : mScrollPort;

but makes me happier stylistically.
 
>+bool
>+nsGfxScrollFrameInner::ShouldBuildLayer() const
>+{
>+  // range of 20 pixels to eliminate many gfx scroll frames from becoming a

Seems pretty random to me, but tn or roc could provide better
heuristics than I could.

r=me with nits picked.
Attachment #519485 - Flags: review?(jones.chris.g) → review+
> >   nsIFrame* rootScrollFrame = presShell->GetRootScrollFrame();
> >+
> 
> Gratuitous whitespace change.

Excess corrected.

> "The root document currently has a widget, but we might end up
> painting content inside the displayport and outside the widget bounds.
> So ensure the document's view honors invalidations within the
> displayport."

Done.

> That function returns |bool|.  Hope this doesn't ding perf ...

Oops. Switched to PRBool.

> I don't like this style.  The following is a little more verbose
> 
>   nsRect displayport, invalidateRect;
>   invalidateRect =
>     nsLayoutUtils::GetDisplayPort(mOuter->GetContent(), &displayport) ?
> displayport : mScrollPort;
> 
> but makes me happier stylistically.

Fixed.

> Seems pretty random to me, but tn or roc could provide better
> heuristics than I could.

Yep, pretty random, and only necessary because the parent process won't know about the scrollboxen otherwise. I'd like to separate this out from layers sometime so that we need not waste so much memory just to know that something is a scrollable area.
Updated patch has cjones review comments addressed and implements roc's IRC suggestion: using GetScrollRange seems to upset GfxScrollFrame if we aren't ready to build a displaylist yet on reftests. I also didn't end up using invalidateRect, oops.
Attachment #519505 - Flags: review?(tnikkel)
Attachment #519485 - Attachment is obsolete: true
Attachment #519485 - Flags: review?(tnikkel)
Attached patch Mobile patchSplinter Review
Attachment #519226 - Attachment is obsolete: true
Attachment #519506 - Flags: review+
Attachment #519505 - Flags: review+
Comment on attachment 519505 [details] [diff] [review]
Followup: bitrot and remove mDisplayport from nsDisplayScrollLayer

+      // We are setting a root displayport for a document.
+      // The pres context needs a special flag set.
       presShell->SetIgnoreViewportScrolling(PR_TRUE);

s/pres context/pres shell/

+      // The root document currently has a widget, but we might end up
+      // painting content inside the displayport but outside the widget
+      // bounds. This ensures the document's view honors invalidations
+      // within the displayport.
+      nsPresContext* presContext = GetPresContext();
+      if (presContext && presContext->IsRoot()) {
+        nsIView* view = rootScrollFrame->GetView();
+        if (view) {
+          view->SetInvalidationDimensions(&displayport);
+        }
+      }

This code is a no-op since scroll frames don't have views. How about: get the root frame (not the root scroll frame) from the pressell, get it's view, and set the invalidation dimensions if that view has a widget.

-                     mDisplayPort, scrollId);
+                     (usingDisplayport ? &displayport : NULL), scrollId);

Are we using NULL now instead of nsnull? I don't mind either way, but we should try to follow whatever convention we are using for our code.

+    // Since we aren't using an nsDisplayListCollection for stacking,
+    // we just use one of its lists. Here, Content() is arbitrarily
+    // picked.

I wouldn't say arbitrary, isn't it the best choice?

Please initialize mShouldBuildLayer somewhere.

+    // Only only asynchronous scrolling if it is enabled and there is a
+    // displayport defined. It is useful to have a scroll layer that is
+    // synchronously scrolled for identifying a scroll area before it is
+    // being actively scrolled.

I think you mean "Only use/do asynchronous scrolling".
(In reply to comment #88)
> Comment on attachment 519505 [details] [diff] [review]
> Followup: bitrot and remove mDisplayport from nsDisplayScrollLayer
> 
> +      // We are setting a root displayport for a document.
> +      // The pres context needs a special flag set.
>        presShell->SetIgnoreViewportScrolling(PR_TRUE);
> 
> s/pres context/pres shell/

Fixed.

> 
> +      // The root document currently has a widget, but we might end up
> +      // painting content inside the displayport but outside the widget
> +      // bounds. This ensures the document's view honors invalidations
> +      // within the displayport.
> +      nsPresContext* presContext = GetPresContext();
> +      if (presContext && presContext->IsRoot()) {
> +        nsIView* view = rootScrollFrame->GetView();
> +        if (view) {
> +          view->SetInvalidationDimensions(&displayport);
> +        }
> +      }
> 
> This code is a no-op since scroll frames don't have views. How about: get the
> root frame (not the root scroll frame) from the pressell, get it's view, and
> set the invalidation dimensions if that view has a widget.

Oh, you're right. I still don't understand why scroll frames don't have views, but your way works!

> 
> -                     mDisplayPort, scrollId);
> +                     (usingDisplayport ? &displayport : NULL), scrollId);
> 
> Are we using NULL now instead of nsnull? I don't mind either way, but we should
> try to follow whatever convention we are using for our code.

Sorry. I try, but the conventions are different for every module and I get mixed up. This becomes more problematic the more modules I touch. Fixed.

> 
> +    // Since we aren't using an nsDisplayListCollection for stacking,
> +    // we just use one of its lists. Here, Content() is arbitrarily
> +    // picked.
> 
> I wouldn't say arbitrary, isn't it the best choice?

Well, I just need *a* list. Any list. I put the list in Content(), and then nsDisplayScrollLayer takes the list over. I could have just as easily used something else besides a collection but it was already there.

This is a little misleading as you see, so I put the comment in. Maybe I should just go ahead use a collection for one branch, and a list for the other?

> 
> Please initialize mShouldBuildLayer somewhere.
> 
> +    // Only only asynchronous scrolling if it is enabled and there is a
> +    // displayport defined. It is useful to have a scroll layer that is
> +    // synchronously scrolled for identifying a scroll area before it is
> +    // being actively scrolled.
> 
> I think you mean "Only use/do asynchronous scrolling".

Fixed. Thanks!
(In reply to comment #89)
> Oh, you're right. I still don't understand why scroll frames don't have views,
> but your way works!

You're thinking about it wrong. You should be thinking "why do any frames have views". :)

> > +    // Since we aren't using an nsDisplayListCollection for stacking,
> > +    // we just use one of its lists. Here, Content() is arbitrarily
> > +    // picked.
> > 
> > I wouldn't say arbitrary, isn't it the best choice?
> 
> Well, I just need *a* list. Any list. I put the list in Content(), and then
> nsDisplayScrollLayer takes the list over. I could have just as easily used
> something else besides a collection but it was already there.

Ok, sure the list doesn't matter there. The thing that matters is where you put the nsDisplayScrollLayer, ie the "set.Content()->AppendNewToTop(layerItem);", this isn't arbitrary.

> This is a little misleading as you see, so I put the comment in. Maybe I should
> just go ahead use a collection for one branch, and a list for the other?

Just create a new temp list, use it, wrap it in a display scroll layer item, and put that item into the right place.
Comments addressed.
Attachment #519541 - Flags: review?(tnikkel)
Attachment #519505 - Attachment is obsolete: true
Attachment #519505 - Flags: review?(tnikkel)
Comment on attachment 519541 [details] [diff] [review]
Followup: bitrot and remove mDisplayport from nsDisplayScrollLayer

+PRBool
+nsGfxScrollFrameInner::ShouldBuildLayer() const
+{
+#ifdef MOZ_IPC
+  return mShouldBuildLayer;
+#else
+  return false;
+#endif
+}

PRBool means PR_FALSE.
Attachment #519541 - Flags: review?(tnikkel) → review+
Fixed. And pushed:
http://hg.mozilla.org/mozilla-central/rev/dea535299cf2
http://hg.mozilla.org/mozilla-central/rev/d66b9df51eff
http://hg.mozilla.org/mobile-browser/rev/a847680012a7

Let's hope it sticks!
Status: REOPENED → RESOLVED
Closed: 13 years ago13 years ago
Resolution: --- → FIXED
Hey ben and company,
Can we get a little help on how we should be testing the fixes for this bug?   the more detailed scenarios, the more we can help.   

Thanks
Depends on: 642047
Would these pushes have caused the current redness on comm-central?
make[6]: *** [nsGfxScrollFrame.o] Error 1
make[5]: *** [generic_libs] Error 2
make[5]: *** Waiting for unfinished jobs....
make[4]: *** [libs_tier_platform] Error 2
make[3]: *** [tier_platform] Error 2
make[2]: *** [default] Error 2
make[1]: *** [default] Error 2
make: *** [build] Error 2

http://tinderbox.mozilla.org/showlog.cgi?log=Thunderbird/1300235621.1300237680.13328.gz
Not sure if this even needs a review, but better safe than sorry.
Attachment #519574 - Flags: review?(blassey.bugs)
Comment on attachment 519574 [details] [diff] [review]
Follow-up: Fix for non-IPC builds and remove warning


>   } else {
>+#endif
>     rv = mOuter->BuildDisplayListForChild(aBuilder, mScrolledFrame, dirtyRect, set);
>+#ifdef MOZ_IPC
>   }
>+#endif


instead, do this:

   } else
>+#endif
  {
     rv = mOuter->BuildDisplayListForChild(aBuilder, mScrolledFrame, dirtyRect, set);
   }
Attachment #519574 - Flags: review?(blassey.bugs) → review+
Depends on: 642205
Depends on: 642345
Target Milestone: --- → mozilla2.0
You need to log in before you can comment on or make changes to this bug.