Closed Bug 605618 Opened 9 years ago Closed 9 years ago

Scroll iframes in parent process

Categories

(Core :: IPC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
fennec 2.0b4+ ---

People

(Reporter: stechz, Assigned: stechz)

References

Details

(Keywords: dev-doc-complete)

Attachments

(12 files, 128 obsolete files)

14.36 KB, patch
stechz
: review+
stechz
: superreview+
Details | Diff | Splinter Review
28.72 KB, patch
stechz
: review+
Details | Diff | Splinter Review
15.47 KB, patch
stechz
: review+
Details | Diff | Splinter Review
15.76 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
5.71 KB, patch
cjones
: review+
Details | Diff | Splinter Review
17.53 KB, patch
stechz
: review+
stechz
: superreview+
Details | Diff | Splinter Review
9.68 KB, patch
stechz
: review+
stechz
: superreview+
Details | Diff | Splinter Review
8.05 KB, patch
stechz
: review+
stechz
: superreview+
Details | Diff | Splinter Review
10.82 KB, patch
stechz
: review+
Details | Diff | Splinter Review
12.49 KB, patch
mfinkle
: review+
Details | Diff | Splinter Review
31.95 KB, patch
mfinkle
: review+
Details | Diff | Splinter Review
1.27 KB, patch
mfinkle
: review+
Details | Diff | Splinter Review
Given a point on the browser, find a unique ID that corresponds to an iframe in the child process.
I'm new to almost everything I touched, so I need a lot of feedback on this part. A few notes:

1. I use a PRUint64 to generate handles for any content element I want. These get passed over to the parent process in the layer tree. I would prefer this to be some sort of e10s handle, but I don't think we've implemented them yet.

2. I hacked nsDisplayList to do hit testing for shadowed items using HitTestState. This isn't ideal, but it is minimal code impact. I'm pretty sure we want to use display lists for hit testing remote content just like everything else. This could potentially be useful for other content hit tests that need to have immediate visual feedback.
Attachment #484489 - Flags: feedback?(roc)
Attachment #484490 - Flags: feedback?(jones.chris.g)
Attachment #484491 - Flags: feedback?(jones.chris.g)
Attachment #484492 - Flags: feedback?(jones.chris.g)
Attachment #484490 - Flags: feedback?(roc)
Blocks: 586288
Comment on attachment 484490 [details] [diff] [review]
Part 2: Tag layers with scrollable information

>   nsDisplayOwnLayer(nsDisplayListBuilder* aBuilder, nsIFrame* aFrame,
>-                    nsDisplayList* aList);
>+                    nsDisplayList* aList, bool aScrollable = false);

I would think we want to subclass nsDisplayOwnLayer instead of adding these things onto it.

>@@ -410,22 +410,22 @@ nsSubDocumentFrame::BuildDisplayList(nsD
>-    } else if (presContext->IsRootContentDocument()) {
>+    } else if (PR_GetEnv("MOZ_LAYER_IFRAMES")) {

This is going to break regular old desktop Firefox.
Comment on attachment 484491 [details] [diff] [review]
Part 3: Build shadow display list

>+class nsDisplayRemoteShadow : public nsDisplayItem
>+  NS_DISPLAY_DECL_NAME("Remote", TYPE_REMOTE)

You need a different display item type here, it is actually important. Don't forget to add it to layout/base/nsDisplayItemTypes.h.

>+  nsRect mRegion;

Best to call a rect a rect as we also have nsRegion's.
Comment on attachment 484492 [details] [diff] [review]
Part 4: Hit test API for frontend

>+nsLayoutUtils::GetRemoteContentId(nsIFrame* aFrame,
>+  builder.IgnorePaintSuppression();

Why do we want to ignore paint suppression by default? If the user "clicks" on something during paint suppression I don't think they expect to have that click targeted at something they can't see.
Comment on attachment 484489 [details] [diff] [review]
Part 1: nsIContent can generate unique IDs

You are so not going to add 8 bytes to every DOM node for this :-)

Can't you just add an ID to IFRAME elements?
Attachment #484489 - Flags: feedback?(roc) → feedback-
(In reply to comment #9)
> Comment on attachment 484489 [details] [diff] [review]
> Part 1: nsIContent can generate unique IDs
> 
> You are so not going to add 8 bytes to every DOM node for this :-)
> 
> Can't you just add an ID to IFRAME elements?

We could, but we'll just need to do something different when we support scrolling divs.

The right way to do this is probably e10s handles.
+  AddFrameMetricsForViewportFrame(aForFrame, root, mVisibleRect, 1);You don't want to just pass 1 here. nsDisplayList::PaintForFrame can be called recursively during display list construction.Instead of storing mScrollable in nsDisplayOwnLayer, why not just check if mFrame is an nsSubDocumentFrame?
(In reply to comment #10)
> (In reply to comment #9)
> > Can't you just add an ID to IFRAME elements?
> 
> We could, but we'll just need to do something different when we support
> scrolling divs.

OK, use nsINode::SetProperty etc instead.
I have some higher-level comments from bug 586288 comment 10 that are probably better raised here.

I think we all agree on the building blocks
 - scrollable sub-frames in their own layers (let's call them "scrollable layers")
 - content process attaches FrameMetrics to scrollable layers
 - UI process can apply a ViewportConfig to shadow scrollable layers
 - frontend can read (at least) ViewportConfig for shadow scrollable layers
 - frontend can get a token from a shadow scrollable layer that it can use to look up a DOM node in the content process

I think we also can agree that
 - the token shared across content/UI should not affect memory management of frames or DOM nodes (like window ID does not)

This design should be implementable for both <browser remote=true> and remote=false.

(In reply to comment #10)
> Modified, more elaborated plan:
> 
> Part 1 (Bug 605618)
> 1. Put all iframes in their own container layer and tag them with a unique
> scrollable ID that crosses the process boundary.
> 2. Augment display lists for finding cross-process content by allowing a
> HitTest that returns an ID, that will eventually be redeemable in the content
> process.
> 3. Use layers from content process to add shadow iframes to display lists in
> parent process.
> 4. API that can do hit test and return ID in parent process on nsFrameLoader.
> 
> Part 2 (No bug yet)
> 1. Use hashtable to store ViewportConfigs for nsFrameLoader that are indexable
> by the scroll ID.
> 2. Add nsFrameLoader API that, given a scroll ID, can scroll the shadow layer
> represented by that scroll ID.
> 

I'm not at all fond of adding a lot of platform APIs built around magic IDs.  I'd also like to not add new APIs unless necessary.  Here's what I propose; let's say that the frontend wants to scroll <x, y>
 - frontend asks for a scrollable thing at <x, y>; say, browser.getScrollableAt(x, y) or whatever
 - frontend always gets back a scrollable thing, which may represent a root or a sub-frame
 - the scrollable thing has a first-class IDL API which offers
   * viewport config API (get/set) that currently lives on nsIFrameLoader
   * getter for cross-process token
 - frontend fires off a "real" scroll request to content process using token

In the content process,
 - no new API for looking up token --- iterate over iframes with getElementsByTagName or whatever, compare with the property set by platform
 - the element referred to by the token might have disappeared; if so, fall back on root scrollable
 - frontend-code-in-content does whatever from there on with found element

This keeps the token opaque from the frontend's perspective, and limits its use to a single lookup in content and chrome, after which APIs on real objects are used.

How does this sound?  I'm going to look over these patches mostly with the building blocks above in mind.
Comment on attachment 484490 [details] [diff] [review]
Part 2: Tag layers with scrollable information

Approach looks OK.

This patch doesn't require pulling dom/ipc stuff into layout/base, and I don't think we should do that.  Also, the |ifdef MOZ_IPC| guards aren't needed on setting frame metrics, because (i) it's not IPC related, per se; (ii) setting the frame metrics is cheap; (iii) all our Tier Is always build with -DMOZ_IPC anyway.
Attachment #484490 - Flags: feedback?(jones.chris.g) → feedback+
Comment on attachment 484491 [details] [diff] [review]
Part 3: Build shadow display list

>diff --git a/layout/base/nsDisplayList.h b/layout/base/nsDisplayList.h
>     nsAutoTArray<nsDisplayItem*, 100> mItemBuffer;
>+    nsTArray<PRUint64>* mShadows;

You'll want "real" review from a layout person, but it doesn't feel right to fragment the class like this.  The shadow-layer hit test is conceptually the same as any other, you'll just get back a different display item.

>diff --git a/layout/generic/nsSubDocumentFrame.cpp b/layout/generic/nsSubDocumentFrame.cpp
>@@ -277,29 +277,17 @@ nsSubDocumentFrame::BuildDisplayList(nsD
>+      return rfp->BuildDisplayList(aBuilder, this, aDirtyRect, aLists);

Nice.

>diff --git a/layout/ipc/RenderFrameParent.cpp b/layout/ipc/RenderFrameParent.cpp
> ComputeShadowTreeTransform(nsIFrame* aContainerFrame,
>                            const FrameMetrics& aMetrics,
>-                           const ViewportConfig& aConfig,
>+                           const ViewportConfig aConfig,

Don't understand this change.

>+static gfx3DMatrix
>+CalculateShadowTransform(ContainerLayer* aLayer, const nsIntPoint& aTranslation,
>+                         float aXScale, float aYScale)
>+{
>+  // XXX why were we using shadow transform

Don't understand what this comment refers to.

>+  // NS_ABORT_IF_FALSE(aRoot->GetTransform() == shadow->GetShadowTransform(),

Please leave this assertion in TransformShadowTreeTo, it checks that the transform attribute was synced correctly.

>+static void
>+BuildListForLayer(Layer* aLayer,
>+{
>+  // XXX hack: if layer has a child, then it must be a container layer, so
>+  // once we get to a leaf node there is definitely no more shadow display
>+  // items to build.
>+  if (!aLayer->GetFirstChild())

We might want to fix the TYPE_SHADOW issue forcing this hack for a real patch, but I guess that can be a followup.

A layout person is going to need to comment on the approach of this function in concert with BuildDisplayList below; I'm not familiar enough with display lists to give useful feedback.  I will say that this algorithm doesn't quite feel right, because container tranforms are additive down their subtrees.  I would expect a recursive algorithm here.

>+NS_IMETHODIMP
>+RenderFrameParent::BuildDisplayList(nsDisplayListBuilder* aBuilder,
>+    UpdateShadowSubtree(GetRootLayer());

Sprinkling these calls around worries me, since forgetting one will have bad results.  But I don't have any better ideas right now.  Always doing this at ShadowLayersUpdated doesn't fix things.

Looks OK to me as far as I'm qualified to give feedback.
Attachment #484491 - Flags: feedback?(jones.chris.g) → feedback+
Comment on attachment 484492 [details] [diff] [review]
Part 4: Hit test API for frontend

I'm not a fan of this API.  I think it's going to be difficult to maintain and hard to interpret.  Let's try to hide more implementation details under a richer API that's easier for the frontend to use.
Attachment #484492 - Flags: feedback?(jones.chris.g) → feedback-
> I'm not at all fond of adding a lot of platform APIs built around magic IDs. 
> I'd also like to not add new APIs unless necessary.  Here's what I propose;
> let's say that the frontend wants to scroll <x, y>
>  - frontend asks for a scrollable thing at <x, y>; say,
> browser.getScrollableAt(x, y) or whatever
>  - frontend always gets back a scrollable thing, which may represent a root or
> a sub-frame
>  - the scrollable thing has a first-class IDL API which offers
>    * viewport config API (get/set) that currently lives on nsIFrameLoader
>    * getter for cross-process token
>  - frontend fires off a "real" scroll request to content process using token

This is a great plan. Why didn't I think of that? :)

> In the content process,
>  - no new API for looking up token --- iterate over iframes with
> getElementsByTagName or whatever, compare with the property set by platform
>  - the element referred to by the token might have disappeared; if so, fall
> back on root scrollable
>  - frontend-code-in-content does whatever from there on with found element

What about overflow divs or other scrollable things? I have a feeling that we need a similar scrolling API in the content process. I don't think we can "fall back" to the root scroll frame either--we will be trying to sync scroll coordinates for a specific frame. We would probably just ignore any ID that doesn't resolve to a scrollable.
(In reply to comment #17)
> > In the content process,
> >  - no new API for looking up token --- iterate over iframes with
> > getElementsByTagName or whatever, compare with the property set by platform
> >  - the element referred to by the token might have disappeared; if so, fall
> > back on root scrollable
> >  - frontend-code-in-content does whatever from there on with found element
> 
> What about overflow divs or other scrollable things?

Use CSS selectors?  *waves hands*

> I have a feeling that we
> need a similar scrolling API in the content process.

I don't think we /need/ one, but may want to add a specialized interface for perf.  I'd prefer to try non-specialized first (unless there's something I'm missing that precludes that).

> I don't think we can "fall
> back" to the root scroll frame either--we will be trying to sync scroll
> coordinates for a specific frame. We would probably just ignore any ID that
> doesn't resolve to a scrollable.

Sure.
Attachment #484489 - Attachment is obsolete: true
Attachment #484490 - Attachment is obsolete: true
Attachment #484490 - Flags: feedback?(roc)
Attachment #484491 - Attachment is obsolete: true
Attachment #484492 - Attachment is obsolete: true
Summary: Implement API for finding content iframes in parent process → Scroll iframes in parent process
This gets iframe panning working with a custom version of Fennec I have. I've incorporated Chris's idea of having a new viewport interface that can be fetched from the frame loader. Ideally we will get this to work for non-remote tabs as well (but I don't know if it will be in this bug). Here's what I need to do still:
1) General clean up of patches.
2) Have a way to rebuild hashtables whenever shadow layers are updated. Since the hashtable will only contain iframes in the current displayport, we might actually be better off with an array.
3) Displayport for iframes are currently not relative to the viewport like the root scrollframe is.
4) I'm not very confident about the way I'm building a display item for my iframe. I think this may need some review help and a few iterations. This part took a lot of time :)
5) All the feedback review given above.

Then we might be ready for some serious review. I changed the scope of this bug to iframe panning. Note that there's still some work to do to generalize displayport for anything scrollable.
Attachment #488235 - Attachment is obsolete: true
Attachment #488236 - Attachment is obsolete: true
Attachment #488238 - Attachment is obsolete: true
Attachment #488239 - Attachment is obsolete: true
Attachment #488241 - Attachment is obsolete: true
Attachment #488240 - Attachment is obsolete: true
Attachment #488234 - Attachment is obsolete: true
Attachment #488967 - Attachment is obsolete: true
Attachment #488968 - Attachment is obsolete: true
Attachment #488970 - Attachment is obsolete: true
Attachment #488971 - Attachment is obsolete: true
Attachment #488972 - Attachment is obsolete: true
Attachment #488969 - Attachment is obsolete: true
OK, all comments hopefully addressed, so we're ready to start getting reviews for these patches! I'm not sure who to ask for everything, but here's a quick summary.

* Part 1: Adds new scroll attribute to layers. roc or cjones probably.
* Part 2: A way to do hit testing for remote content. Not sure, tn?
* Part 3: API for new scrollables. roc and cjones.
* Part 4: Store viewport configs for iframes with RenderFrameParent. cjones probably.
* Part 5: Displayport support for iframes. Not sure, lots of layout things in here.
* Part 6: Scrolling and building display lists based on layers. cjones or roc.
Comment on attachment 489022 [details] [diff] [review]
Part 2: Infrastructure for building shadow display list

>+class nsDisplayRemoteShadow : public nsDisplayItem
>+  NS_DISPLAY_DECL_NAME("Remote", TYPE_REMOTE)

You should create a new display item type instead of re-using TYPE_REMOTE.
Comment on attachment 489024 [details] [diff] [review]
Part 5: Support displayport for iframes

>@@ -398,16 +398,29 @@ nsSubDocumentFrame::BuildDisplayList(nsD
>     if (subdocRootFrame && parentAPD != subdocAPD) {
>       nsDisplayZoom* zoomItem =
>         new (aBuilder) nsDisplayZoom(aBuilder, subdocRootFrame, &childItems,
>                                      subdocAPD, parentAPD);
>       childItems.AppendToTop(zoomItem);
>+    } else if (subdocRootFrame && presShell->UsingDisplayPort()) {
>+      // Scroll layer needs a child frame inside the scroll frame with content
>+      nsDisplayItem* item = childItems.GetBottom();
>+      while (item && (!item->GetUnderlyingFrame() || !item->GetUnderlyingFrame()->GetContent())) {
>+        nsDisplayList* list = item->GetList();
>+        item = list ? list->GetBottom() : NULL;
>+      }
>+
>+      if (item) {
>+        nsDisplayScrollLayer* layerItem = new (aBuilder) nsDisplayScrollLayer(
>+          aBuilder, &childItems, this, item->GetUnderlyingFrame(), subdocBoundsInParentUnits);
>+        childItems.AppendToTop(layerItem);
>+      }
>     } else if (presContext->IsRootContentDocument()) {

If both UsingDisplayPort and IsRootContentDocument are true then I don't think this does the right thing. Similarly in the zoom case; I think we still need the zoom item _and_ to do the scroll layer stuff. I think the UsingDisplayPort case is probably independent from this if statement.

Does nsIPresShell::GetRootScrollFrame do what you need here with less fuss?
Attachment #489021 - Flags: review?(jones.chris.g)
Attachment #489022 - Flags: review?(tnikkel)
Attachment #489023 - Flags: review?(roc)
Attachment #489023 - Flags: review?(jones.chris.g)
Attachment #489023 - Flags: review?(roc)
Attachment #489028 - Flags: review?(roc)
Attachment #489028 - Flags: review?(jones.chris.g)
Attachment #489026 - Flags: review?(jones.chris.g)
Attachment #489024 - Flags: review?(tnikkel)
Comment on attachment 489021 [details] [diff] [review]
Part 1: Tag layers with scrollable information

> nsACString&
>+AppendToString(nsACString& s, PRUint64 n,
>+               const char* pfx="", const char* sfx="")
>+{
>+  s += pfx;
>+  s += nsPrintfCString( 128, "%lld", n);

s.AppendInt(n);

> struct FrameMetrics {
>   FrameMetrics()
>+    , mScrollId(0)

Is "0" a sentinel value that means "not an ID"?  (We need such a thing.)  If so, please doc, and if you use need a test for "has a scroll id" please add it to FrameMetrics rather than comparing against "0" elsewhere.

>+ifdef MOZ_IPC
>+LOCAL_INCLUDES += \
>+		-I$(srcdir)/../../dom/ipc \
>+		$(NULL)
>+endif
>+

Wait what?  Vestige?

>+static void AddFrameMetricsForViewportFrame(nsIFrame* aForFrame,

My somewhat limited understanding of the frame tree is that there's exactly one viewport frame, though there may be many scrollable frames.  I assume you refactored this in preparation for using it elsewhere on non-viewport frames, so I'd call this something more like just "RecordFrameMetrics".
Attachment #489021 - Flags: review?(jones.chris.g)
Comment on attachment 489028 [details] [diff] [review]
Part 3: Viewport API for frontend

I forgot that desktop FF is sort-of-API-frozen, so we can't touch any
existing interfaces unless we exhaust all other options >:(.  The
scroll APIs need to go into a new interface implemented by
nsIFrameLoader, and we keep the old APIs but make them throwing
no-ops.

Throughout this code, "viewport" is being used to mean "scrollable
thing", which is just going to lead to confusion.  Let's use a name
closer to "scrollable thing".

>+   nsIViewport getScrollable(in float xPx, in float yPx);

Hmmmm ... I wonder if it would be better to return a stack of
scrollables under a given point, for more flexibility in the future in
deciding which to move (could be a pref or something).  (For now it
would obviously just return either [ root ] or [ iframe, root ]).

I'm OK with this kind of interface, but let's call it
"getTopScrollableAt()" or something for clarity.

>diff --git a/content/base/src/nsFrameLoader.cpp b/content/base/src/nsFrameLoader.cpp
>--- a/content/base/src/nsFrameLoader.cpp
>+++ b/content/base/src/nsFrameLoader.cpp
>+NS_IMETHODIMP
>+nsFrameLoader::GetScrollable(float aXpx, float aYpx, nsIViewport** aScrollable)
>+{
>+  nsViewport* viewport = GetScrollable();
>+  CallQueryInterface(viewport, aScrollable);

Should just need |*aScrollable = GetScrollable()|.

>+NS_IMETHODIMP
>+nsFrameLoader::GetRootScrollable(nsIViewport** aScrollable)
>+{
>+  CallQueryInterface(GetScrollable(), aScrollable);

Same here.

>diff --git a/content/base/src/nsFrameLoader.h b/content/base/src/nsFrameLoader.h
>--- a/content/base/src/nsFrameLoader.h
>+++ b/content/base/src/nsFrameLoader.h
>@@ -75,16 +75,82 @@ class RenderFrameParent;
> #ifdef MOZ_WIDGET_GTK2
> typedef struct _GtkWidget GtkWidget;
> #endif
> #ifdef MOZ_WIDGET_QT
> class QX11EmbedContainer;
> #endif
> #endif
> 
>+#define ROOT_SCROLLABLE_ID 1
>+
>+/**
>+ * Defines a target configuration for this <browser>'s content
>+ * document's viewport.  If the content document's actual viewport
>+ * doesn't match a desired ViewportConfig, then on paints its pixels
>+ * are transformed to compensate for the difference.
>+ *
>+ * Used to support asynchronous re-paints of content pixels; see
>+ * nsIFrameLoader.scrollViewport* and viewportScale.
>+ */
>+class nsViewport : public nsIViewport

I'd prefer to keep the current |struct ViewportConfig| intact (but
probably rename it to ScrollableConfig or something).  It's just a bag
of data that can be compared == and so forth.  Your nsScrollable or
whatever here would just have an inline ScrollableConfig member.

>diff --git a/layout/base/nsLayoutUtils.cpp b/layout/base/nsLayoutUtils.cpp
>diff --git a/layout/base/nsLayoutUtils.h b/layout/base/nsLayoutUtils.h

I'm not qualified to review these changes.

>diff --git a/layout/ipc/RenderFrameParent.cpp b/layout/ipc/RenderFrameParent.cpp
>--- a/layout/ipc/RenderFrameParent.cpp
>+++ b/layout/ipc/RenderFrameParent.cpp
>@@ -44,17 +44,16 @@
> #include "LayerManagerOGL.h"
> #include "RenderFrameParent.h"
> 
> #include "gfx3DMatrix.h"
> #include "nsFrameLoader.h"
> #include "nsViewportFrame.h"
> #include "nsSubDocumentFrame.h"
> 
>-typedef nsFrameLoader::ViewportConfig ViewportConfig;
> using namespace mozilla::layers;
> 
> namespace mozilla {
> namespace layout {
> 
> static void
> AssertInTopLevelChromeDoc(ContainerLayer* aContainer,
>                           nsIFrame* aContainedFrame)
>@@ -80,43 +79,43 @@ AssertValidContainerOfShadowTree(Contain
> // Compute the transform of the shadow tree contained by
> // |aContainerFrame| to widget space.  We transform because the
> // subprocess layer manager renders to a different top-left than where
> // the shadow tree is drawn here and because a scale can be set on the
> // shadow tree.
> static void
> ComputeShadowTreeTransform(nsIFrame* aContainerFrame,
>                            const FrameMetrics& aMetrics,
>-                           const ViewportConfig& aConfig,
>+                           nsViewport* aConfig,

I really prefer having the input to this computation be a chunk of
data rather the scrollable-thing impl.
Attachment #489028 - Flags: review?(jones.chris.g)
Attachment #489022 - Flags: superreview?(roc)
Attachment #489024 - Flags: superreview?(roc)
+class nsDisplayRemoteShadow : public nsDisplayItem

Add a comment explaining what this class is for

+    nsTArray<PRUint64>* mShadows;

And a comment here explaining what this means
I wonder if we should rename nsIViewport. "viewport" is already a very overloaded term in browsers. It will also be confusing when we apply this interface to any scrollable element.

nsIRemoteScroll or something like that, maybe?
(In reply to comment #45)
> I wonder if we should rename nsIViewport. "viewport" is already a very
> overloaded term in browsers. It will also be confusing when we apply this
> interface to any scrollable element.
> 
> nsIRemoteScroll or something like that, maybe?

Keep in mind it supports scaling as well, and this interface *might* be nice to have on regular scrollboxes as well. One common way to scroll things in frontend code == very happy Ben.
This is another interface that we'll want an in-process impl of probably, eventually, so I'm not a fan of "Remote" in the name.  Anything near nsIScrollThing (where Thing != Frame) seems ok to me.
So we can't use "Remote" and we can't use "Scroll" :-)

nsIContentPortal?
Assignee: nobody → ben
Status: NEW → ASSIGNED
tracking-fennec: --- → ?
tracking-fennec: ? → 2.0b3+
Attached patch fennec changes - something wrong (obsolete) — Splinter Review
A bit confused about how it supposed to work...
I made here small patch which should make scrolling works, but don't understand why loader.getScrollable always return the same scrollable as root?
So I've been working on a frontend patch for this and I found that I needed a few more things (specifically, the size of the scrolled content area for each scrollable and a way to set a display port given an ID).

I will upload the new patches (including frontend) this weekend but I've started finding a lot of problems once the displayport moves too far away from the scroll viewport. :( I want to spend a little more time debugging.

At this point I don't think there's any way this will be ready in time for b3 (unless I figure out all the problems this weekend and we blast through reviews on Monday).
(In reply to comment #50)

> At this point I don't think there's any way this will be ready in time for b3
> (unless I figure out all the problems this weekend and we blast through reviews
> on Monday).

I think it's too risky to take the day before code freeze as well
> >--- a/content/base/src/nsFrameLoader.cpp
> >+++ b/content/base/src/nsFrameLoader.cpp
> >+NS_IMETHODIMP
> >+nsFrameLoader::GetScrollable(float aXpx, float aYpx, nsIViewport** aScrollable)
> >+{
> >+  nsViewport* viewport = GetScrollable();
> >+  CallQueryInterface(viewport, aScrollable);
> 
> Should just need |*aScrollable = GetScrollable()|.

No, that won't work. XPCOM out params like this need to addref, and GetScrollable does not do this.

> I really prefer having the input to this computation be a chunk of
> data rather the scrollable-thing impl.

Hmm, I refactored this to be a matrix since that chunk of data was just scale and offset (just a transformation). I could use a struct with the offset and scale instead?
Attachment #489021 - Attachment is obsolete: true
Attachment #489022 - Attachment is obsolete: true
Attachment #489022 - Flags: superreview?(roc)
Attachment #489022 - Flags: review?(tnikkel)
Attachment #489028 - Attachment is obsolete: true
Attachment #489028 - Flags: review?(roc)
Attachment #489023 - Attachment is obsolete: true
Attachment #489023 - Flags: review?(jones.chris.g)
Attachment #489024 - Attachment is obsolete: true
Attachment #489024 - Flags: superreview?(roc)
Attachment #489024 - Flags: review?(tnikkel)
Attachment #489026 - Attachment is obsolete: true
Attachment #489026 - Flags: review?(jones.chris.g)
Attached patch Mobile: Use new interfaces (obsolete) — Splinter Review
New patches uploaded with frontend! Things seem to be working! :)

These patches seem to work for bugzilla and my test iframe page, but no extensive testing has been done. The test iframe page is at:
http://people.mozilla.org/~bstover/iframe.html

I am having trouble keeping track of all the code reviews, so please forgive me because I know I missed things.
Attachment #492122 - Flags: review?(jones.chris.g)
Attachment #492126 - Flags: review?(tnikkel)
Attachment #492123 - Flags: review?(tnikkel)
Attachment #492124 - Flags: review?(jones.chris.g)
Attachment #492125 - Flags: review?(jones.chris.g)
Great! gmail scrolling almost works..
I found 3 problems here:
1) after scrolling iframe it's content not updated immediately, I  found that I need to take rootScrollable and call scrollBy(0, -1) scrollBy(0, 1) in order to update child scrollable content. I think it can be solved by calling rootViewport.update(), after child.scrollBy.

2) minor problem with mouse event coordinates (they are coming to wrong scrollOffset)

3) this.contentWindow - is always null, and debug build complaining about browser.xml:525  contentWindow is null et.c.
Attached patch Mobile: Use new interfaces part2 (obsolete) — Splinter Review
Additional patch for fennec UI:
- Fixed mouse events (click/taps)
- highlight position
- Refresh for rootScrollable after child scrolled (hack)
- some random JS errors workarounds (this.contentWindow = null)
Also something need to be done with elementFromPoint functionality... and it seems we need to change scrollPort for domDocument in order to get that functionality returning right values.
Also what about scrollable DIV's are we going to create scrollable frame for them?
Some pages like www.orkut.com are working like one big scrollable DIV
Attachment #492126 - Flags: superreview?(roc)
Attachment #492123 - Flags: superreview?(roc)
(In reply to comment #52)
> > >--- a/content/base/src/nsFrameLoader.cpp
> > >+++ b/content/base/src/nsFrameLoader.cpp
> > >+NS_IMETHODIMP
> > >+nsFrameLoader::GetScrollable(float aXpx, float aYpx, nsIViewport** aScrollable)
> > >+{
> > >+  nsViewport* viewport = GetScrollable();
> > >+  CallQueryInterface(viewport, aScrollable);
> > 
> > Should just need |*aScrollable = GetScrollable()|.
> 
> No, that won't work. XPCOM out params like this need to addref, and
> GetScrollable does not do this.
> 

  *aScrollable = nsRefPtr<nsIBlah>(GetScrollable()).forget();

or break it up if that's too long.  Don't use CallQueryInterface() just to add a ref.

> > I really prefer having the input to this computation be a chunk of
> > data rather the scrollable-thing impl.
> 
> Hmm, I refactored this to be a matrix since that chunk of data was just scale
> and offset (just a transformation). I could use a struct with the offset and
> scale instead?

We don't support generic 2d transforms, just scales and offsets.  The original config struct or a new one with just those is fine with me.
Attachment #492126 - Attachment is obsolete: true
Attachment #492126 - Flags: superreview?(roc)
Attachment #492126 - Flags: review?(tnikkel)
Attachment #492129 - Attachment is obsolete: true
Attachment #492130 - Attachment is obsolete: true
Attached patch Mobile: Use new interfaces (obsolete) — Splinter Review
These patches fix most of Oleg's issues and some problems with the iframe itself being too far away from the displayport.

One thing I haven't fixed yet is elementFromPoint. There's also another problem. STR:
1. Go to http://people.mozilla.com/~bstover/iframe3.html
2. Scroll down to iframe and scroll iframe contents around (far away from where you started)
3. Scroll root content up and scroll back down
4. The iframe area will be blank until you pan, and then the iframe will be back at its original scroll position (not where you scrolled it to)

This is because nsScrollable's scrollX and scrollY are never synced to the content process, so once this information is destroyed (when iframe is no longer in root displayport), the scroll information goes with it.

elementFromPoint suffers from a similar problem.

The most obvious thing to do here is to use iframe.scrollBy instead of changing the displayport, committing scrollX/Y changes often. I think this will have some rendering issues though (see bug 612599).
Attachment #492419 - Attachment is obsolete: true
Attachment #492420 - Attachment is obsolete: true
Attached patch Mobile: Use new interfaces (obsolete) — Splinter Review
Like mentioned above, this calls iframeWindow.scrollBy/scrollTo to sync scrollX/Y changes. Link highlighting seems to be working great now, and no more reset scroll changes when the displayport goes away.

I'm not noticing any rendering issues either.

Things I'm aware that these patches need:
* a name for scrollables (I think cjones and mfinkle liked getContentViewAt)
* separate interfaces for everything (because of API freeze, sigh)
* review notes in #68
tracking-fennec: 2.0b3+ → 2.0b4+
Attachment #492122 - Attachment is obsolete: true
Attachment #492122 - Flags: review?(jones.chris.g)
Attachment #492123 - Attachment is obsolete: true
Attachment #492123 - Flags: superreview?(roc)
Attachment #492123 - Flags: review?(tnikkel)
Attachment #492124 - Attachment is obsolete: true
Attachment #492124 - Flags: review?(jones.chris.g)
Attachment #492125 - Attachment is obsolete: true
Attachment #492125 - Flags: review?(jones.chris.g)
Attachment #492418 - Attachment is obsolete: true
Attachment #492127 - Attachment is obsolete: true
Attachment #492128 - Attachment is obsolete: true
Attachment #492493 - Attachment is obsolete: true
Attachment #492494 - Attachment is obsolete: true
Attached patch Mobile: Use new interfaces (obsolete) — Splinter Review
Attachment #492807 - Flags: review?(jones.chris.g)
Attachment #492808 - Flags: review?(tnikkel)
Attachment #492809 - Flags: review?(jones.chris.g)
Attachment #492812 - Flags: review?(tnikkel)
Attachment #492811 - Flags: review?(jones.chris.g)
Changes:
* frontend fix for a bug where we would scroll to the wrong place when displayport is updated
* unbitrot
* rename nsIScrollable to nsIContentView (and other renamings with that)
* use new interfaces instead of nsIDOMWindowUtils and nsIFrameLoader
* code review in #68
Attachment #492200 - Attachment is obsolete: true
Attachment #492071 - Attachment is obsolete: true
Comment on attachment 492807 [details] [diff] [review]
Part 1: Tag layers with scrollable information

>diff --git a/gfx/layers/Layers.h b/gfx/layers/Layers.h
>--- a/gfx/layers/Layers.h
>+++ b/gfx/layers/Layers.h
>@@ -82,38 +82,55 @@ class SpecificLayerAttributes;
> 
> /**
>  * The viewport and displayport metrics for the painted frame at the
>  * time of a layer-tree transaction.  These metrics are especially
>  * useful for shadow layers, because the metrics values are updated
>  * atomically with new pixels.
>  */

Instead of directly PRUint64, add a

  typedef PRUint64 scrollid_t;

here and then use scrollid_t.

> struct FrameMetrics {
>+public:
>+  static const PRUint64 NO_SCROLL_ID = 0;
>+  static const PRUint64 SCROLL_ROOT_ID = 1;

Nits: s/NO_SCROLL_ID/NULL_SCROLL_ID/, s/SCROLL_ROOT_ID/ROOT_SCROLL_ID/.

r=me with those fixed.
Attachment #492807 - Flags: review?(jones.chris.g) → review+
Have tested this stuff a bit... and it looks like with latest patches update gmail is not scrollable to the bottom anymore... it stuck in the middle.

with first version it was better
Comment on attachment 492809 [details] [diff] [review]
Part 3: Viewport API for frontend

>diff --git a/content/base/public/nsIFrameLoader.idl b/content/base/public/nsIFrameLoader.idl
>--- a/content/base/public/nsIFrameLoader.idl
>+++ b/content/base/public/nsIFrameLoader.idl

Need to update comments in here for s/viewport/view/.

>   /**
>+   * DEPRECATED. Please QI for nsIContentViewManager.
>    */

Also add a FIXME to remove in 2.next with a bug #, and s/QI for/QI
to/.

>diff --git a/content/base/src/nsFrameLoader.cpp b/content/base/src/nsFrameLoader.cpp
>--- a/content/base/src/nsFrameLoader.cpp
>+++ b/content/base/src/nsFrameLoader.cpp
>@@ -134,16 +134,105 @@ public:
>     if (base_win) {
>       base_win->Destroy();
>     }
>     return NS_OK;
>   }
>   nsRefPtr<nsIDocShell> mDocShell;
> };
> 
>+NS_IMPL_ISUPPORTS1(nsContentView, nsIContentView)
>+
>+nsresult
>+nsContentView::Update()
>+{

As noted in comment 43, I think the current impl with a ViewportConfig
type (rename to ViewConfig?) is the right way to write this update
code.  We need before+after values to compute invalidations etc.,
which the old code had and this new code drops.  It's much cleaner to
have a struct containing all these values than trying to fiddle with
them individually.

>diff --git a/content/base/src/nsFrameLoader.h b/content/base/src/nsFrameLoader.h
>--- a/content/base/src/nsFrameLoader.h
>+++ b/content/base/src/nsFrameLoader.h
>@@ -75,17 +75,83 @@ class RenderFrameParent;
> #ifdef MOZ_WIDGET_GTK2
> typedef struct _GtkWidget GtkWidget;
> #endif
> #ifdef MOZ_WIDGET_QT
> class QX11EmbedContainer;
> #endif
> #endif
> 
>-class nsFrameLoader : public nsIFrameLoader
>+#define ROOT_SCROLLABLE_ID 1

Reuse the const in Layers.h.  No need to inline IsRoot(), so can move
that into the .cpp and avoid pulling Layers.h into this header.

>+
>+/**
>+ * Defines a target configuration for this <browser>'s content
>+ * document's viewport.  If the content document's actual viewport
>+ * doesn't match this nsIContentView, then on paints its pixels
>+ * are transformed to compensate for the difference.
>+ *
>+ * Used to support asynchronous re-paints of content pixels; see
>+ * nsIContentView.
>+ */
>+class nsContentView : public nsIContentView
>+{
>+public:
>+  NS_DECL_ISUPPORTS
>+  NS_DECL_NSICONTENTVIEW
>+
>+  nsContentView(nsIContent* aOwnerContent, PRUint64 aScrollId,
>+             nsPoint aScrollOffset = nsPoint(0, 0))
>+    : mOwnerContent(aOwnerContent)
>+    , mScrollOffset(aScrollOffset)
>+    , mXScale(1.0)
>+    , mYScale(1.0)
>+    , mScrollId(aScrollId)
>+    {}
>+
>+  // Default copy ctor and operator= are fine
>+  PRBool operator==(const nsContentView& aOther) const
>+  {
>+    return (mScrollOffset == aOther.mScrollOffset &&
>+            mXScale == aOther.mXScale &&
>+            mYScale == aOther.mYScale &&
>+            mScrollId == aOther.mScrollId);
>+  }
>+
>+  nsresult Update();

Update() is private.

>+
>+  nsPoint GetScrollOffset() const { return mScrollOffset; }
>+  float GetXScale() const { return mXScale; }
>+  float GetYScale() const { return mYScale; }
>+  PRUint64 GetId() const { return mScrollId; }
>+  

More ugmo ;).  Use ViewConfig.

>+class nsFrameLoader : public nsIFrameLoader, public nsIContentViewManager
> {
>+    , mViewportConfig(new nsContentView(aOwner, ROOT_SCROLLABLE_ID))

This isn't a viewportconfig anymore, it's a scrollable view thing.

>diff --git a/content/canvas/src/nsCanvasRenderingContext2D.cpp b/content/canvas/src/nsCanvasRenderingContext2D.cpp
>--- a/content/canvas/src/nsCanvasRenderingContext2D.cpp
>+++ b/content/canvas/src/nsCanvasRenderingContext2D.cpp
>@@ -3668,17 +3668,17 @@ nsCanvasRenderingContext2D::AsyncDrawXUL
>         // XXX ERRMSG we need to report an error to developers here! (bug 329026)
>         return NS_ERROR_DOM_SECURITY_ERR;
>     }
> 
>     nsCOMPtr<nsIFrameLoaderOwner> loaderOwner = do_QueryInterface(aElem);
>     if (!loaderOwner)
>         return NS_ERROR_FAILURE;
> 
>-    nsCOMPtr<nsFrameLoader> frameloader = loaderOwner->GetFrameLoader();
>+    nsRefPtr<nsFrameLoader> frameloader = loaderOwner->GetFrameLoader();

Don't understand this change.

>diff --git a/layout/ipc/RenderFrameParent.cpp b/layout/ipc/RenderFrameParent.cpp
>--- a/layout/ipc/RenderFrameParent.cpp
>+++ b/layout/ipc/RenderFrameParent.cpp
>@@ -80,43 +79,43 @@ AssertValidContainerOfShadowTree(Contain
> // Compute the transform of the shadow tree contained by
> // |aContainerFrame| to widget space.  We transform because the
> // subprocess layer manager renders to a different top-left than where
> // the shadow tree is drawn here and because a scale can be set on the
> // shadow tree.
> static void
> ComputeShadowTreeTransform(nsIFrame* aContainerFrame,
>                            const FrameMetrics& aMetrics,
>-                           const ViewportConfig& aConfig,
>+                           nsContentView* aConfig,

This function doesn't need the XPCOM-y view thing, and is yuckier now
for having it.  Use ViewConfig.
Attachment #492809 - Flags: review?(jones.chris.g)
Comment on attachment 492811 [details] [diff] [review]
Part 4: Table for storing viewports

Lots of vestigial "viewport" use in this patch.  Not calling them out
individually for brevity's sake.

>diff --git a/content/base/src/nsFrameLoader.cpp b/content/base/src/nsFrameLoader.cpp
> void
> nsFrameLoader::SetOwnerContent(nsIContent* aContent)
> {
>   mOwnerContent = aContent;
>+  if (GetCurrentRemoteFrame())
>+    GetCurrentRemoteFrame()->ContentChanged(aContent);

if (RenderFrameParent* rfp = GetCurrentRemoteFrame()) {
  rfp->Frob();
}

> NS_IMETHODIMP
> nsFrameLoader::GetContentViewAt(float aXpx, float aYpx, nsIContentView** aContentView)
> {
>-  nsRefPtr<nsIContentView>(GetContentView()).forget(aContentView);
>+  nscoord x = nsPresContext::CSSPixelsToAppUnits(aXpx);
>+  nscoord y = nsPresContext::CSSPixelsToAppUnits(aYpx);
>+  nsPoint pt(x, y);
>+
>+  nsIFrame* frame = GetPrimaryFrameOfOwningContent();
>+
>+  PRUint64 id = nsLayoutUtils::GetRemoteContentId(frame, pt, true);

I don't think it's a good idea to alias these IDs as
scroll/view/content ID; we should pick one and be consistent with it,
otherwise newcomers to this code will be über confused.

>+  // XXX needs to be not a zero check
>+  if (id == 0 || !GetCurrentRemoteFrame()) {

Use NULL_SCROLL_ID.

>+  nsIContentView* viewport = GetCurrentRemoteFrame()->GetContentView(id);
>+  NS_ASSERTION(viewport, "Retrieved ID from RenderFrameParent, it should be valid!");

Make this ABORT_IF_FALSE.

> NS_IMETHODIMP
> nsFrameLoader::GetRootContentView(nsIContentView** aContentView)
> {
>-  nsRefPtr<nsIContentView>(GetContentView()).forget(aContentView);
>+  if (GetCurrentRemoteFrame() == NULL) {

Save that to a local, use if (![local]), and re-use it below.

>+    *aContentView = nsnull;
>+    return NS_OK;
>+  }
>+
>+  nsContentView* viewport = GetCurrentRemoteFrame()->GetContentView();
>+  NS_ASSERTION(viewport, "Should always be able to create root scrollable!");

ABORT_IF_FALSE here too.

>diff --git a/content/base/src/nsFrameLoader.h b/content/base/src/nsFrameLoader.h
>--- a/content/base/src/nsFrameLoader.h
>+++ b/content/base/src/nsFrameLoader.h
>   nsCOMPtr<nsIDocShell> mDocShell;
>   nsCOMPtr<nsIURI> mURIToLoad;
>+public:
>   nsIContent *mOwnerContent; // WEAK

Yuck.  Just add an inline getter for this.

>diff --git a/layout/ipc/RenderFrameParent.cpp b/layout/ipc/RenderFrameParent.cpp
>--- a/layout/ipc/RenderFrameParent.cpp
>+++ b/layout/ipc/RenderFrameParent.cpp
>+static nsContentView*
>+FindViewportForId(const ViewportArray& aArray, nsContentViewId aId)

Shouldn't need this function, see comment about hashtable.

>@@ -167,40 +180,112 @@ ShadowRootOf(ContainerLayer* aContainer)
> // widget, and hence in non-retained mode.
> static PRBool
> IsTempLayerManager(LayerManager* aManager)
> {
>   return (LayerManager::LAYERS_BASIC == aManager->GetBackendType() &&
>           !static_cast<BasicLayerManager*>(aManager)->IsRetained());
> }
> 
>+// Goes down layer tree and looks for any scrollable layers with aScrollId.
>+static ContainerLayer*
>+FindLayerWithScrollId(PRUint64 aScrollId, Layer* aLayer)

You can remove the need for this function by storing a map from ID ->
< view, layer >, then replacing this with a lookup of the layer.  Note
however that the memory management is a bit subtle; you'll need to
keep a weak ref to the layer, and document the fact that
BuildViewBlah() *must* be called on ShadowLayersUpdated(), and that in
BuildViewBlah(), the entries in the old map can be touched IFF a layer
with their scroll ID exists in the new layer tree.  Other entries in
the old map might have pointers to dead layers.

>+// Recursively create a new array of scrollables, preserving any scrollables
>+// that are still in the layer tree.
>+static void
>+RebuildViewportArray(ViewportArray& oldScrollables, ViewportArray& newScrollables,
>+                     nsFrameLoader* aFrameLoader, Layer* aLayer,
>+                     float aXScale = 1, float aYScale = 1)

There are several things going on at once in this function, and I have
to admit I don't understand all that's happening (and there aren't
comments).  It looks like you're trying to propagate information down
the layer tree that doesn't need to be propagated: during painting,
container's transforms accumulate down the tree.  The target
ContentView scales should be relative the containers, as they are in
the layer tree, and so should start out as identity.  It also appears
that this code changes the old model in which the ViewConfig starts
out with an offset <0, 0> and it's the user's responsibility to update
it.  I think this new impl is fine, but need to document this model in
nsIContentView.

> void
>+RenderFrameParent::ContentChanged(nsIContent* aContent)
>+{

Add an NS_ABORT_IF_FALSE(mFrameLoader->OwnerContent() == aContent).

>+void
>+RenderFrameParent::RebuildViewportArray()
>+{
>+  ViewportArray newScrollables;
>+  if (GetRootLayer()) {
>+    mozilla::layout::RebuildViewportArray(mScrollables, newScrollables, mFrameLoader, GetRootLayer());

I think the compiler should be able to disambiguate this call based on
the args, so I don't think you need the explict qualification.  I'm
not 100% sure though.

>diff --git a/layout/ipc/RenderFrameParent.h b/layout/ipc/RenderFrameParent.h
>--- a/layout/ipc/RenderFrameParent.h
>+++ b/layout/ipc/RenderFrameParent.h
>@@ -38,19 +38,24 @@
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
> #ifndef mozilla_layout_RenderFrameParent_h
> #define mozilla_layout_RenderFrameParent_h
> 
> #include "mozilla/layout/PRenderFrameParent.h"
> 
>+#include "nsInterfaceHashtable.h"
> #include "nsDisplayList.h"
> 
>-class nsFrameLoader;
>+#ifndef ROOT_SCROLLABLE_ID
>+#define ROOT_SCROLLABLE_ID 1
>+#endif

Don't spread this all around.  We already have the constants in
Layers.h, let's use them.

>+  void ContentChanged(nsIContent* aContent);

Nit: call this OwnerContentChanged().

>+
> protected:
>   NS_OVERRIDE void ActorDestroy(ActorDestroyReason why);
> 
>   NS_OVERRIDE virtual PLayersParent* AllocPLayers();
>   NS_OVERRIDE virtual bool DeallocPLayers(PLayersParent* aLayers);
> 
> private:
>+  void RebuildViewportArray();

Nit: s/Rebuild/Build/.

>+  ViewportArray mScrollables;

Looks like you started to use a hashtable for these and then got
scared off by the API? ;) Hashtable is correct here and will save some
code to boot.

This is looking pretty good.
Attachment #492811 - Flags: review?(jones.chris.g)
Attachment #492808 - Flags: superreview?(roc)
Attachment #492812 - Flags: superreview?(roc)
Blocks: 615368
> >-    nsCOMPtr<nsFrameLoader> frameloader = loaderOwner->GetFrameLoader();
> >+    nsRefPtr<nsFrameLoader> frameloader = loaderOwner->GetFrameLoader();
> 
> Don't understand this change.

This stems from nsFrameLoader now having multiple interfaces. nsRefPtr doesn't produce ambiguous interface errors, nsCOMPtr does. I don't understand it much more than that.
Duplicate of this bug: 615520
Comment on attachment 492812 [details] [diff] [review]
Part 5: Support displayport for iframes

>   if (NS_SUCCEEDED(rv)) {
>+    nsIScrollableFrame* scrollFrame = presShell->GetRootScrollFrameAsScrollable();
>+    nsRect scrollRange = scrollFrame->GetScrollRange();
this might crash, for example on about:about page
(In reply to comment #92)
> Comment on attachment 492812 [details] [diff] [review]
> Part 5: Support displayport for iframes
> 
> >   if (NS_SUCCEEDED(rv)) {
> >+    nsIScrollableFrame* scrollFrame = presShell->GetRootScrollFrameAsScrollable();
> >+    nsRect scrollRange = scrollFrame->GetScrollRange();
> this might crash, for example on about:about page

Yes, this is fixed in my patch queue. New patches up soon.
Attachment #492807 - Attachment is obsolete: true
Attachment #492808 - Attachment is obsolete: true
Attachment #492808 - Flags: superreview?(roc)
Attachment #492808 - Flags: review?(tnikkel)
Attachment #492809 - Attachment is obsolete: true
Attachment #492811 - Attachment is obsolete: true
Attachment #492812 - Attachment is obsolete: true
Attachment #492812 - Flags: superreview?(roc)
Attachment #492812 - Flags: review?(tnikkel)
Attachment #492813 - Attachment is obsolete: true
Attachment #492815 - Attachment is obsolete: true
Attachment #492817 - Attachment is obsolete: true
Attachment #492818 - Attachment is obsolete: true
Attached patch Mobile: Use new interfaces (obsolete) — Splinter Review
Attachment #494121 - Flags: review+
Attachment #494122 - Flags: superreview?(roc)
Attachment #494122 - Flags: review?(tnikkel)
Attachment #494123 - Flags: review?(jones.chris.g)
Attachment #494124 - Flags: review?(jones.chris.g)
Attachment #494125 - Flags: superreview?(roc)
Attachment #494125 - Flags: review?(tnikkel)
Attachment #494126 - Flags: review?(jones.chris.g)
> Shouldn't need this function, see comment about hashtable.

Leaving as a function because of how stupid the STL is (see patch).

> You can remove the need for this function by storing a map from ID ->
> < view, layer >, then replacing this with a lookup of the layer.  Note
> however that the memory management is a bit subtle; you'll need to
> keep a weak ref to the layer, and document the fact that
> BuildViewBlah() *must* be called on ShadowLayersUpdated(), and that in
> BuildViewBlah(), the entries in the old map can be touched IFF a layer
> with their scroll ID exists in the new layer tree.  Other entries in
> the old map might have pointers to dead layers.

Let's not pre-optimize this, given the subtle memory management issues you mentioned and how fast diving through layers is.

> >+// Recursively create a new array of scrollables, preserving any scrollables
> >+// that are still in the layer tree.
> >+static void
> >+RebuildViewportArray(ViewportArray& oldScrollables, ViewportArray& newScrollables,
> >+                     nsFrameLoader* aFrameLoader, Layer* aLayer,
> >+                     float aXScale = 1, float aYScale = 1)
> 
> There are several things going on at once in this function, and I have
> to admit I don't understand all that's happening (and there aren't
> comments).  It looks like you're trying to propagate information down
> the layer tree that doesn't need to be propagated: during painting,
> container's transforms accumulate down the tree.  The target
> ContentView scales should be relative the containers, as they are in
> the layer tree, and so should start out as identity.  It also appears
> that this code changes the old model in which the ViewConfig starts
> out with an offset <0, 0> and it's the user's responsibility to update
> it.  I think this new impl is fine, but need to document this model in
> nsIContentView.

You are correct: the behavior of the default value of offset has changed. Unfortunately it is because of this that we have to propagate the scale down. I've commented this now.

> 
> >+void
> >+RenderFrameParent::RebuildViewportArray()
> >+{
> >+  ViewportArray newScrollables;
> >+  if (GetRootLayer()) {
> >+    mozilla::layout::RebuildViewportArray(mScrollables, newScrollables, mFrameLoader, GetRootLayer());
> 
> I think the compiler should be able to disambiguate this call based on
> the args, so I don't think you need the explict qualification.  I'm
> not 100% sure though.

Nope.
Attachment #494124 - Attachment is obsolete: true
Attachment #494124 - Flags: review?(jones.chris.g)
Attached patch Part 4: Map for storing views (obsolete) — Splinter Review
Attachment #494125 - Attachment is obsolete: true
Attachment #494125 - Flags: superreview?(roc)
Attachment #494125 - Flags: review?(tnikkel)
Attachment #494126 - Attachment is obsolete: true
Attachment #494126 - Flags: review?(jones.chris.g)
Attachment #494127 - Attachment is obsolete: true
Attachment #494128 - Attachment is obsolete: true
Attachment #494177 - Flags: review?(jones.chris.g)
Attachment #494179 - Flags: superreview?(roc)
Attachment #494179 - Flags: review?(tnikkel)
Attachment #494180 - Flags: review?(jones.chris.g)
(In reply to comment #103)
> > You can remove the need for this function by storing a map from ID ->
> > < view, layer >, then replacing this with a lookup of the layer.  Note
> > however that the memory management is a bit subtle; you'll need to
> > keep a weak ref to the layer, and document the fact that
> > BuildViewBlah() *must* be called on ShadowLayersUpdated(), and that in
> > BuildViewBlah(), the entries in the old map can be touched IFF a layer
> > with their scroll ID exists in the new layer tree.  Other entries in
> > the old map might have pointers to dead layers.
> 
> Let's not pre-optimize this, given the subtle memory management issues you
> mentioned and how fast diving through layers is.
> 

Don't care about optimizing it, this function is just extraneous code.  It duplicates your tree-recursing logic when instead you could just build the map, have a common search function for ID -> mapped pair (view/layer), and grab which of the pair you need.
> Don't care about optimizing it, this function is just extraneous code.  It
> duplicates your tree-recursing logic when instead you could just build the map,
> have a common search function for ID -> mapped pair (view/layer), and grab
> which of the pair you need.

Hm, but just because the scroll ID is in the new tree doesn't mean the layer is not a dangling pointer, right? Besides, I'd rather have an extra straight-forward layer walking function than keep dangling pointers about.

Would a templatized or macro'd layer walker make this feel better to you?
Comment on attachment 494122 [details] [diff] [review]
Part 2: Infrastructure for building shadow display list

I was looking for the definition of nsContentViewId (used in parts 1 and 2) and I found it in part 3. Is it too much trouble to order things so that it compiles after each part so bisecting works?

>+public:
>+  nsDisplayRemoteShadow(nsDisplayListBuilder* aBuilder,
>+                        nsRect aRect,
>+                        nsContentViewId aId)
>+    : nsDisplayItem(aBuilder, nsnull)
>+    , mRect(aRect)
>+    , mId(aId)

We have code that assumes non-clip display items have a non-null underlying frame, so if we can avoid it easily (and it looks like we can) we should pass a frame to the nsDisplayItem constructor to use as an underlying frame.

+   * Find an ID corresponding to some content element in the child process.

This isn't really informative. Add something to say what kind of content element it looks at.
Attachment #494122 - Flags: review?(tnikkel) → review+
Comment on attachment 494121 [details] [diff] [review]
Part 1: Tag layers with scrollable information

>@@ -429,44 +461,24 @@ void nsDisplayList::PaintForFrame(nsDisp
>+  RecordFrameMetrics(aForFrame, root, mVisibleRect,
>+                     FrameMetrics::SCROLL_ROOT_ID);

nsDisplayList::PaintForFrame can be called for more than just the root frame, so I think you need to check some conditions before doing this.
Comment on attachment 494179 [details] [diff] [review]
Part 5: Support displayport for iframes

+  template <class T> static void Destroy(void* aObject, nsIAtom* aPropertyName,

Should probably call this DestroyProperty or something, so it doesn't get confused with a nsINode destroy function.

@@ -5995,17 +5995,42 @@ void PresShell::SetRenderingState(const 

I don't understand why this change is needed.

+class nsDisplayScrollLayer : public nsDisplayOwnLayer
+  nsRect mClip;

Why do you need this rect?

+#ifdef NS_BUILD_REFCNT_LOGGING
+nsDisplayScrollLayer::~nsDisplayScrollLayer()
+{
+  MOZ_COUNT_DTOR(nsDisplayScrollLayer);
+}
+#endif

There wasn't a MOZ_COUNT_CTOR in the constructor, or am I missing something?

I want to think about what nsDisplayScrollLayer::ComputeVisibility should be doing more.

@@ -337,20 +337,31 @@ nsSubDocumentFrame::BuildDisplayList(nsD
+    if (presShell->UsingDisplayPort()) {
+      dirty = presShell->GetDisplayPort();
+
+      // The visual overflow rect of our viewport frame unfortunately may not
+      // intersect with the displayport of that frame. We have to force the
+      // frame to have a little faith and build a display list anyway.
+      presShell->GetRootScrollFrame()->AddStateBits(
+        NS_FRAME_FORCE_DISPLAY_LIST_DESCEND_INTO);

I don't know if making the dirty rect be the entire display port and using the force descend bit is the right approach. I want to think about it more.

@@ -337,20 +337,31 @@ nsSubDocumentFrame::BuildDisplayList(nsD
+      presShell->GetRootScrollFrame()->AddStateBits(
+        NS_FRAME_FORCE_DISPLAY_LIST_DESCEND_INTO);

I think you need to null check the result of GetRootScrollFrame here.

@@ -393,16 +404,31 @@ nsSubDocumentFrame::BuildDisplayList(nsD
   if (NS_SUCCEEDED(rv)) {
+    nsIScrollableFrame* scrollFrame = presShell->GetRootScrollFrameAsScrollable();
+    nsRect scrollRange = scrollFrame ?
+      scrollFrame->GetScrollRange() : nsRect(0, 0, 0, 0);
+
+    if (subdocRootFrame &&
+        (scrollRange.width != 0 || scrollRange.height != 0)) {
+      nsDisplayScrollLayer* layerItem = new (aBuilder) nsDisplayScrollLayer(
+        aBuilder,
+        &childItems,
+        presShell->GetRootScrollFrame(),
+        subdocBoundsInParentUnits
+      );
+      childItems.AppendToTop(layerItem);
+    }

Why don't we just make this block conditional on scrollFrame being non-null? I think it would simplify the logic. And scrollFrame can only be non-null if subdocRootFrame is.

We also want to make this block conditional on something, we only want to add a scroll layer item if we are in a content process. Is checking for the use of a display port on the presshell what we need to do?

After this we then may wrap childItems in a display zoom. Anything inside a display zoom should be child app units, whereas subdocBoundsInParentUnits was used on the scroll layer item.

I think we should change the "else if (presContext->IsRootContentDocument()) {" here to a seperate if and make it only execute if we didn't already add an item that creates a layer (a scroll layer item or a zoom item).

There shouldn't be a case where we end up adding a scroll layer item and a zoom item, but if we do then we have the overhead of an extra container layer. So we should assert or warn if that happens.

Why did you make the root scroll frame the underlying frame for the scroll layer item?
(In reply to comment #110)
> > Don't care about optimizing it, this function is just extraneous code.  It
> > duplicates your tree-recursing logic when instead you could just build the map,
> > have a common search function for ID -> mapped pair (view/layer), and grab
> > which of the pair you need.
> 

I realized yesterday that I gave you bad intel on this: shadow layers are only destroyed after transactions, and this map-building code runs in the same event as transactions.  Shadow layers can only be destroyed in later events, and by that time they're out of the layer tree so should also be out of this map.  So there's no dangling-pointer hazard.

> Hm, but just because the scroll ID is in the new tree doesn't mean the layer is
> not a dangling pointer, right? 

I don't quite follow this question, but we have the strong invariants that in a consistent map (after Build and before the next shadow-layer transaction), (i) mapped layers aren't dangling; and (ii) mapped layers are in the shadow-layer tree.

> Besides, I'd rather have an extra
> straight-forward layer walking function than keep dangling pointers about.
> 
> Would a templatized or macro'd layer walker make this feel better to you?

I still prefer the simpler solution.  We can hold strong refs to the layers in the map as well, it won't change their lifetime.
Comment on attachment 494129 [details] [diff] [review]
Mobile: Use new interfaces


>+            x = Math.floor(Math.max(0, Math.min(scrollRangeX, contentView.scrollX + x)) - contentView.scrollX);
>+            y = Math.floor(Math.max(0, Math.min(scrollRangeY, contentView.scrollY + y)) - contentView.scrollY);
>+
>+            if (x == 0 && y == 0)
>+              return;
>+
>+            contentView.scrollBy(x, y);
>+            this._updateCacheViewport();

sounds like with this change we are going to overload chrome + content with cachecViewport calls, and that makes scrolling of main content much slower

>-            if (Math.abs(this._pendingPixelsX) > Math.max(0, this._pendingThresholdX - bcr.width / 2) ||
>-                Math.abs(this._pendingPixelsY) > Math.max(0, this._pendingThresholdY - bcr.height / 2))
>-              this._updateCacheViewport();
>-          ]]>
> I was looking for the definition of nsContentViewId (used in parts 1 and 2) and
> I found it in part 3. Is it too much trouble to order things so that it
> compiles after each part so bisecting works?

My mistake, sorry! Yes, the goal is to have every step compile. During review cycles I don't always compile every step of the way, so I missed this.

(In reply to comment #112)
> Comment on attachment 494121 [details] [diff] [review]
> Part 1: Tag layers with scrollable information
> 
> >@@ -429,44 +461,24 @@ void nsDisplayList::PaintForFrame(nsDisp
> >+  RecordFrameMetrics(aForFrame, root, mVisibleRect,
> >+                     FrameMetrics::SCROLL_ROOT_ID);
> 
> nsDisplayList::PaintForFrame can be called for more than just the root frame,
> so I think you need to check some conditions before doing this.

This was just a refactoring though... This code was unconditionally being called before. Is this a bug?
> I realized yesterday that I gave you bad intel on this: shadow layers are only
> destroyed after transactions, and this map-building code runs in the same event
> as transactions.  Shadow layers can only be destroyed in later events, and by
> that time they're out of the layer tree so should also be out of this map.  So
> there's no dangling-pointer hazard.

OK, sounds fine to me!
> @@ -5995,17 +5995,42 @@ void PresShell::SetRenderingState(const 
> 
> I don't understand why this change is needed.

I tried to explain this in the XXX comment. Sometimes, content will not be rerendered after calling SetDisplayPort on an iframe that is not within the visible area of content (but it *is* visible in the displayport).

This code invalidates the root layer instead of the iframe layer, guaranteeing that content will be rerendered.

> 
> +class nsDisplayScrollLayer : public nsDisplayOwnLayer
> +  nsRect mClip;
> 
> Why do you need this rect?

We set the visible region to this rect. Since we base the remote shadow region on the iframe's visible area in the scrolling patch, we need the visible region to be the size of the iframe.

> I want to think about what nsDisplayScrollLayer::ComputeVisibility should be
> doing more.

Yes, this is the part I'm worried about.

> 
> @@ -337,20 +337,31 @@ nsSubDocumentFrame::BuildDisplayList(nsD
> +    if (presShell->UsingDisplayPort()) {
> +      dirty = presShell->GetDisplayPort();
> +
> +      // The visual overflow rect of our viewport frame unfortunately may not
> +      // intersect with the displayport of that frame. We have to force the
> +      // frame to have a little faith and build a display list anyway.
> +      presShell->GetRootScrollFrame()->AddStateBits(
> +        NS_FRAME_FORCE_DISPLAY_LIST_DESCEND_INTO);
> 
> I don't know if making the dirty rect be the entire display port and using the
> force descend bit is the right approach. I want to think about it more.

I'm very open to how to do this better. If you can't think of anything it can always be a followup bug.

> There shouldn't be a case where we end up adding a scroll layer item and a zoom
> item, but if we do then we have the overhead of an extra container layer. So we
> should assert or warn if that happens.
> 

> Why did you make the root scroll frame the underlying frame for the scroll
> layer item?

This will be used more generally for all scroll frames eventually.
> We also want to make this block conditional on something, we only want to add a
> scroll layer item if we are in a content process. Is checking for the use of a
> display port on the presshell what we need to do?

No, we need even non-displayport iframes to be layers so we know if we can move them around in the parent process. We can check the process type.

> I think we should change the "else if (presContext->IsRootContentDocument()) {"
> here to a seperate if and make it only execute if we didn't already add an item
> that creates a layer (a scroll layer item or a zoom item).
> 

Wouldn't this break zoomed iframe pages?? nsDisplayZoom has hit test and computevisibility logic that nsDisplayScrollLayer does not have.
(In reply to comment #117)
> > I realized yesterday that I gave you bad intel on this: shadow layers are only
> > destroyed after transactions, and this map-building code runs in the same event
> > as transactions.  Shadow layers can only be destroyed in later events, and by
> > that time they're out of the layer tree so should also be out of this map.  So
> > there's no dangling-pointer hazard.

As it turns out, this code is no longer needed so I have just removed it altogether.
Attachment #494121 - Attachment is obsolete: true
Attachment #494122 - Attachment is obsolete: true
Attachment #494122 - Flags: superreview?(roc)
Attachment #494123 - Attachment is obsolete: true
Attachment #494123 - Flags: review?(jones.chris.g)
Attachment #494177 - Attachment is obsolete: true
Attachment #494177 - Flags: review?(jones.chris.g)
Attached patch Part 4: Map for storing views (obsolete) — Splinter Review
Attachment #494179 - Attachment is obsolete: true
Attachment #494179 - Flags: superreview?(roc)
Attachment #494179 - Flags: review?(tnikkel)
Attachment #494180 - Attachment is obsolete: true
Attachment #494180 - Flags: review?(jones.chris.g)
Attachment #494182 - Attachment is obsolete: true
Attachment #494183 - Attachment is obsolete: true
Attachment #494541 - Flags: review+
Attachment #494542 - Flags: review+
Attachment #494542 - Flags: superreview?(roc)
Attachment #494542 - Flags: review+
Attachment #494543 - Flags: review?(jones.chris.g)
Attachment #494544 - Flags: review?(jones.chris.g)
Attachment #494545 - Flags: superreview?(roc)
Attachment #494545 - Flags: review?(tnikkel)
Attachment #494546 - Flags: review?(jones.chris.g)
Attachment #494542 - Flags: review+
(In reply to comment #116)
> (In reply to comment #112)
> > Comment on attachment 494121 [details] [diff] [review] [details]
> > Part 1: Tag layers with scrollable information
> > 
> > >@@ -429,44 +461,24 @@ void nsDisplayList::PaintForFrame(nsDisp
> > >+  RecordFrameMetrics(aForFrame, root, mVisibleRect,
> > >+                     FrameMetrics::SCROLL_ROOT_ID);
> > 
> > nsDisplayList::PaintForFrame can be called for more than just the root frame,
> > so I think you need to check some conditions before doing this.
> 
> This was just a refactoring though... This code was unconditionally being
> called before. Is this a bug?

If I understand the code correctly, then yes I think so.
(In reply to comment #118)
> I tried to explain this in the XXX comment. Sometimes, content will not be
> rerendered after calling SetDisplayPort on an iframe that is not within the
> visible area of content (but it *is* visible in the displayport).
> 
> This code invalidates the root layer instead of the iframe layer, guaranteeing
> that content will be rerendered.

So it sounds like a hack to work around the display port not being considered somewhere. We should really fix that, but maybe it's hard enough that we should take a hack.

> We set the visible region to this rect. Since we base the remote shadow region
> on the iframe's visible area in the scrolling patch, we need the visible region
> to be the size of the iframe.

Why can't we just use the mVisibleRect that is set on the item after compute visibility? That also has the benefit that if the scrollable area is covered by something we don't include part of what covers it in the area we consider scrollable.

> > @@ -337,20 +337,31 @@ nsSubDocumentFrame::BuildDisplayList(nsD
> > +    if (presShell->UsingDisplayPort()) {
> > +      dirty = presShell->GetDisplayPort();
> > +
> > +      // The visual overflow rect of our viewport frame unfortunately may not
> > +      // intersect with the displayport of that frame. We have to force the
> > +      // frame to have a little faith and build a display list anyway.
> > +      presShell->GetRootScrollFrame()->AddStateBits(
> > +        NS_FRAME_FORCE_DISPLAY_LIST_DESCEND_INTO);
> > 
> > I don't know if making the dirty rect be the entire display port and using the
> > force descend bit is the right approach. I want to think about it more.
> 
> I'm very open to how to do this better. If you can't think of anything it can
> always be a followup bug.

So this seems like another hack where we should be considering the display port but aren't.

> > Why did you make the root scroll frame the underlying frame for the scroll
> > layer item?
> 
> This will be used more generally for all scroll frames eventually.

So for that we will need similar code where we build display lists for scroll frames? Are we going to duplicate this code or move this code there?
(In reply to comment #119)
> > I think we should change the "else if (presContext->IsRootContentDocument()) {"
> > here to a seperate if and make it only execute if we didn't already add an item
> > that creates a layer (a scroll layer item or a zoom item).
> 
> Wouldn't this break zoomed iframe pages?? nsDisplayZoom has hit test and
> computevisibility logic that nsDisplayScrollLayer does not have.

What I'm suggesting is something like

if (scroll layer item is needed)
  layerAdded = true
  // add scroll layer item
if (zoom layer item is needed)
  if (layerAdded) warn("two container layers added, performance will probably suffer")
  layerAdded = true
  // add zoom layer item)
if (!layerAdded && IsRootContentDocument)
  // add a own layer item

I think that does the correct thing.
(In reply to comment #130)
> So it sounds like a hack to work around the display port not being considered
> somewhere. We should really fix that, but maybe it's hard enough that we should
> take a hack.

We could file a followup?

> Why can't we just use the mVisibleRect that is set on the item after compute
> visibility? That also has the benefit that if the scrollable area is covered by
> something we don't include part of what covers it in the area we consider
> scrollable.

We do set mVisibleRect to mClip. If this is not done, mVisibleRect by default will be set to the entire displayport.

> So this seems like another hack where we should be considering the display port
> but aren't.

There is a short-circuit in nsFrame.cpp here:
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#1650

This descend bit is currently used for overflow frames to guarantee that overflow pixels are painted, which in a sense is like displayport. Even if the dirty rect doesn't intersect the visual overflow rect, we still want to generate those display lists. It seems like this flag was in fact made for situations like the display port.

Otherwise, nsFrame would have to know about displayport I guess? That is fine with me, but IMO it seems cleaner for nsFrame to have no knowledge of displayport-fu.

> So for that we will need similar code where we build display lists for scroll
> frames? Are we going to duplicate this code or move this code there?

My gut says the code will be moved. It seems to make the most sense to me.

> > Wouldn't this break zoomed iframe pages?? nsDisplayZoom has hit test and
> > computevisibility logic that nsDisplayScrollLayer does not have.
> 
> What I'm suggesting is something like
> 
> if (scroll layer item is needed)
>   layerAdded = true
>   // add scroll layer item
> if (zoom layer item is needed)
>   if (layerAdded) warn("two container layers added, performance will probably
> suffer")
>   layerAdded = true
>   // add zoom layer item)
> if (!layerAdded && IsRootContentDocument)
>   // add a own layer item
> 
> I think that does the correct thing.

Ah, I see.
> So it sounds like a hack to work around the display port not being considered
> somewhere. We should really fix that, but maybe it's hard enough that we should
> take a hack.

Actually, I think it's been known for a while that things in the displayport but not in the visible area do not always repaint when they are supposed to. Maybe Chris knows if there's a bug already filed.
Attachment #494547 - Flags: review?(jones.chris.g)
Attachment #494549 - Flags: review?(jones.chris.g)
I know of bug 593243.  It's not terribly hard to fix, tn and I discussed it.
Comment on attachment 494541 [details] [diff] [review]
Part 1: Tag layers with scrollable information

Why isn't viewid_t ViewID, as per our naming conventions?

Also, we need documentation here.

This patch needed sr.
Attachment #494541 - Flags: superreview-
Oh, my fault on missed sr.  I didn't know we had naming conventions for numerical types.
FYI, I have noticed that it is possible for dangling pointers to occur. I never set mOwnerContent to NULL for those content views that did not make it into the new hashtable.

This affects parts 3 and 4. Chris, I will be refreshing those patches, but if you need to see a diff I can provide that to you.
(In reply to comment #136)
> Oh, my fault on missed sr.  I didn't know we had naming conventions for
> numerical types.

I don't know why numerical types would be special.
Attachment #494543 - Attachment is obsolete: true
Attachment #494543 - Flags: review?(jones.chris.g)
Attachment #494544 - Attachment is obsolete: true
Attachment #494544 - Flags: review?(jones.chris.g)
Attached patch Part 4: Map for storing views (obsolete) — Splinter Review
Attachment #494545 - Attachment is obsolete: true
Attachment #494545 - Flags: superreview?(roc)
Attachment #494545 - Flags: review?(tnikkel)
Attachment #494547 - Attachment is obsolete: true
Attachment #494547 - Flags: review?(jones.chris.g)
Attachment #494129 - Attachment is obsolete: true
Attached patch Mobile: Use new interfaces (obsolete) — Splinter Review
Attachment #494877 - Flags: review?(jones.chris.g)
Attachment #494878 - Flags: review?(jones.chris.g)
Attachment #494879 - Flags: superreview?(roc)
Attachment #494879 - Flags: review?(tnikkel)
Attachment #494880 - Flags: review?(jones.chris.g)
Comment on attachment 494542 [details] [diff] [review]
Part 2: Infrastructure for building shadow display list

+  if (aState->mShadows)
+    aState->mShadows->AppendElement(mId);

{} around the body

You haven't addressed comment #44.
Comment on attachment 494879 [details] [diff] [review]
Part 5: Support displayport for iframes

+  nsDisplayScrollLayer(nsDisplayListBuilder* aBuilder, nsDisplayList* aList,
+                       nsIFrame* aForFrame, nsRect aClip);

const nsRect&

nsDisplayScrollLayer needs comments explaining what it does.

+    nsRegion visibleRegion = presShell->GetDisplayPort();
+    visibleRegion.MoveBy(ToReferenceFrame());

presShell->GetDisplayPort() + ToReferenceFrame()

+    mVisibleRect = mClip;
+    
+    if (visible) {
+      aBuilder->SubtractFromVisibleRegion(aVisibleRegion, visibleRegion);
+    }

I'm not sure what this is trying to do. What's the different between mClip and the displayport?

It looks like you're trying to render the entire contents of the IFRAME even if it's covered by opaque stuff, so we can scroll them into view in the compositor process without ending up with missing pieces?

+  // XXX
+  // It doesn't look like invalidated content pays any attention to
+  // the displayport, so we can't invalidate our root frame. We have
+  // to invalidate the root presshell's root frame.

Can you explain the problem here in more detail?

+      // The visual overflow rect of our viewport frame unfortunately may not
+      // intersect with the displayport of that frame.

What's an example?

+    }
+    else {

} else {
BTW I think this is going in the right direction overall.
Attachment #494541 - Attachment is obsolete: true
Attachment #495071 - Attachment is obsolete: true
Attachment #494542 - Attachment is obsolete: true
Attachment #494542 - Flags: superreview?(roc)
Attachment #494877 - Attachment is obsolete: true
Attachment #494877 - Flags: review?(jones.chris.g)
Attachment #494878 - Attachment is obsolete: true
Attachment #494878 - Flags: review?(jones.chris.g)
Attached patch Part 4: Map for storing views (obsolete) — Splinter Review
Attachment #494879 - Attachment is obsolete: true
Attachment #494879 - Flags: superreview?(roc)
Attachment #494879 - Flags: review?(tnikkel)
Attachment #494546 - Attachment is obsolete: true
Attachment #494546 - Flags: review?(jones.chris.g)
Attachment #494880 - Attachment is obsolete: true
Attachment #494880 - Flags: review?(jones.chris.g)
Attachment #494549 - Attachment is obsolete: true
Attachment #494549 - Flags: review?(jones.chris.g)
Attachment #495104 - Flags: superreview?(roc)
Attachment #495104 - Flags: review+
Attachment #495105 - Flags: superreview?(roc)
Attachment #495105 - Flags: review+
Attachment #495106 - Flags: review?(jones.chris.g)
Attachment #495110 - Flags: review?(jones.chris.g)
Attachment #495112 - Flags: superreview?(roc)
Attachment #495112 - Flags: review?(tnikkel)
Attachment #495113 - Flags: review?(jones.chris.g)
Attachment #495114 - Flags: review?(jones.chris.g)
Attachment #495115 - Flags: review?(jones.chris.g)
Comment on attachment 495104 [details] [diff] [review]
Part 1: Tag layers with scrollable information

Please add comments to document the ViewID stuff.

+  static const ViewID NULL_SCROLL_ID = 0;
+  static const ViewID ROOT_SCROLL_ID = 1;

Does this actually build on non-gcc? I guess we'll see!
Attachment #495104 - Flags: superreview?(roc) → superreview+
Comment on attachment 495105 [details] [diff] [review]
Part 2: Infrastructure for building shadow display list

One question: why are we passing a point to GetRemoteContentId instead of a rect? Don't we want the "fat-finger" hit-testing to work for remote content too?
Attachment #495105 - Flags: superreview?(roc) → superreview+
(In reply to comment #145)
> Comment on attachment 494879 [details] [diff] [review]
> Part 5: Support displayport for iframes
> 
> +  nsDisplayScrollLayer(nsDisplayListBuilder* aBuilder, nsDisplayList* aList,
> +                       nsIFrame* aForFrame, nsRect aClip);
> 
> const nsRect&

You haven't addressed this.

+// Root scroll ID is always 1, so start scroll counter at 2.
+static ViewID sScrollIdCounter = 2;

How about a ViewID_MAX or something like that which we can start at here?

+    // Since aForFrame normally determines the visible region for the layer,
What does aForFrame refer to?

+    // We set the visible region to the viewport so that the compositor process
+    // can use the visible region to do shadow hit testing.

I think it would be better to pass an explicit hit-test region through the layer tree instead of overloading visible regions like this.

+    if (visible) {
+      aBuilder->SubtractFromVisibleRegion(aVisibleRegion, childVisibleRegion);
+    }

I still don't know why you're doing this. Only opaque stuff should be subtracted from aVisibleRegion. Since the scrolled stuff might be moved asynchronously, we should probably remove nothing from aVisibleRegion even if the scrolled stuff is opaque, since we have to render what's under it.

> +  // XXX
> +  // It doesn't look like invalidated content pays any attention to
> +  // the displayport, so we can't invalidate our root frame. We have
> +  // to invalidate the root presshell's root frame.
> 
> Can you explain the problem here in more detail?

You haven't addressed this.

> +      // The visual overflow rect of our viewport frame unfortunately may not
> +      // intersect with the displayport of that frame.
> 
> What's an example?

Or this.

> +    }
> +    else {
> 
> } else {

Or this.
(In reply to comment #157)
> Comment on attachment 495105 [details] [diff] [review]
> Part 2: Infrastructure for building shadow display list
> 
> One question: why are we passing a point to GetRemoteContentId instead of a
> rect? Don't we want the "fat-finger" hit-testing to work for remote content
> too?

Hm, do you think we need this? I was sort of assuming any scrollable area would be pretty easy for a finger to hit.
(In reply to comment #156)
> Comment on attachment 495104 [details] [diff] [review]
> Part 1: Tag layers with scrollable information
> 
> Please add comments to document the ViewID stuff.
> 
> +  static const ViewID NULL_SCROLL_ID = 0;
> +  static const ViewID ROOT_SCROLL_ID = 1;
> 
> Does this actually build on non-gcc? I guess we'll see!

What should this be then?
(In reply to comment #159)
> (In reply to comment #157)
> > Comment on attachment 495105 [details] [diff] [review] [details]
> > Part 2: Infrastructure for building shadow display list
> > 
> > One question: why are we passing a point to GetRemoteContentId instead of a
> > rect? Don't we want the "fat-finger" hit-testing to work for remote content
> > too?
> 
> Hm, do you think we need this? I was sort of assuming any scrollable area would
> be pretty easy for a finger to hit.

I don't know! But it might make sense for the API to be consistent with the rest of the HitTest API.

(In reply to comment #160)
> (In reply to comment #156)
> > Comment on attachment 495104 [details] [diff] [review] [details]
> > Part 1: Tag layers with scrollable information
> > 
> > Please add comments to document the ViewID stuff.
> > 
> > +  static const ViewID NULL_SCROLL_ID = 0;
> > +  static const ViewID ROOT_SCROLL_ID = 1;
> > 
> > Does this actually build on non-gcc? I guess we'll see!
> 
> What should this be then?

Initialize the constant values in the .cpp file where they're defined.

Another approach would be to use an enum:
  enum ViewID { NULL_SCROLL_ID, ROOT_SCROLL_ID, FIRST_DYNAMIC_ID,
    MAX_ID = 0x7FFFFFFF };
> Another approach would be to use an enum:
>   enum ViewID { NULL_SCROLL_ID, ROOT_SCROLL_ID, FIRST_DYNAMIC_ID,
>     MAX_ID = 0x7FFFFFFF };

I dig it. So, why are you defining a MAX ID?
To make sure the compiler makes the enum at least 32 bits wide.
Or hmm, you probably need 64 bits, and I don't know if 64-bit enums are portable. Maybe just go with the other approach.
Attachment #495104 - Attachment is obsolete: true
Attachment #495105 - Attachment is obsolete: true
Attachment #495106 - Attachment is obsolete: true
Attachment #495106 - Flags: review?(jones.chris.g)
Attachment #495110 - Attachment is obsolete: true
Attachment #495110 - Flags: review?(jones.chris.g)
Attached patch Part 4: Map for storing views (obsolete) — Splinter Review
Attachment #495112 - Attachment is obsolete: true
Attachment #495112 - Flags: superreview?(roc)
Attachment #495112 - Flags: review?(tnikkel)
Attachment #495113 - Attachment is obsolete: true
Attachment #495113 - Flags: review?(jones.chris.g)
Attachment #495114 - Attachment is obsolete: true
Attachment #495114 - Flags: review?(jones.chris.g)
Attachment #495115 - Attachment is obsolete: true
Attachment #495115 - Flags: review?(jones.chris.g)
Attachment #494881 - Attachment is obsolete: true
Attached patch Mobile: Use new interfaces (obsolete) — Splinter Review
Attachment #495664 - Flags: superreview+
Attachment #495664 - Flags: review+
Attachment #495665 - Flags: superreview+
Attachment #495665 - Flags: review+
Attachment #495666 - Flags: review?(jones.chris.g)
Attachment #495667 - Flags: review?(jones.chris.g)
Attachment #495668 - Flags: superreview?(roc)
Attachment #495668 - Flags: review?(tnikkel)
Attachment #495669 - Flags: review?(jones.chris.g)
Attachment #495671 - Flags: review?(jones.chris.g)
Attachment #495672 - Flags: review?(jones.chris.g)
As roc suggested, the API can now specify a rectangle for finding IDs. As cjones suggested, the API now returns an array of IDs.
Comment on attachment 495673 [details] [diff] [review]
Mobile: Use new interfaces

>+            if (Math.abs(this._pixelsPannedSinceRefresh.x) > 20 ||
>+                Math.abs(this._pixelsPannedSinceRefresh.y) > 20)
>+              this._updateCacheViewport();
this is still cause too many updates for main scrollable content view...  I think we should do it less aggressive, and update viewport 1 time only when it is really necessary.
stechz: can you check this patch too... it is fixing some browser.getRootContentView - null js errors , and fixing pinch zoom
Comment on attachment 495666 [details] [diff] [review]
Part 3: Viewport API for frontend

(In reply to comment #88)
> Comment on attachment 492809 [details] [diff] [review]
> Part 3: Viewport API for frontend
> 
> >diff --git a/content/base/public/nsIFrameLoader.idl b/content/base/public/nsIFrameLoader.idl
> >--- a/content/base/public/nsIFrameLoader.idl
> >+++ b/content/base/public/nsIFrameLoader.idl
> 
> Need to update comments in here for s/viewport/view/.
> 

Didn't do this.

getContentViewsAt() needs a new comment.  It's probably better named getContentViewsIn() now that it's looking in a rect.

Why does this use <centerX,centerY,leftSize,rightSize,bottomSize,topSize> instead of the equivalent <topX,leftY,width,height>?

>diff --git a/content/base/src/Makefile.in b/content/base/src/Makefile.in
>--- a/content/base/src/Makefile.in
>+++ b/content/base/src/Makefile.in
>@@ -188,15 +188,16 @@ INCLUDES	+= \
> 		-I$(srcdir)/../../xul/content/src \
> 		-I$(srcdir)/../../html/content/src \
> 		-I$(srcdir)/../../base/src \
> 		-I$(srcdir)/../../xbl/src \
> 		-I$(srcdir)/../../../layout/generic \
> 		-I$(srcdir)/../../../layout/style \
> 		-I$(srcdir)/../../../dom/base \
> 		-I$(srcdir)/../../xml/document/src \
>+		-I$(topsrcdir)/gfx/layers \
> 		-I$(topsrcdir)/xpcom/io \
> 		-I$(topsrcdir)/dom/ipc \
> 		-I$(topsrcdir)/js/src/xpconnect/src \
> 		-I$(topsrcdir)/caps/include \
> 		$(NULL)
> 

Layers.h is exported, you shouldn't need this change I don't think.

>diff --git a/content/base/src/nsFrameLoader.cpp b/content/base/src/nsFrameLoader.cpp
>--- a/content/base/src/nsFrameLoader.cpp
>+++ b/content/base/src/nsFrameLoader.cpp
>+nsresult
>+nsContentView::Update(const ViewConfig& aConfig, PRBool *aResult)
>+{
>+  *aResult = PRBool(mOwnerContent);
>+

This bool-return API makes me a bit nervous; in which situations will
the frontend legitimately grab or retain views for which this is true?
(I genuinely don't know.)  I don't see any place in this patch where
mOwnerContent is nulled for a live nsContentView, so I wonder if it
would instead be better to fail to create a view.  Maybe nulling is 
coming in a later patch.  (Depending on your answer here, other
solutions may make more sense, or the bool return might be right.)

At any rate, need docs :).

This is looking good.  Ready to r+ after answers to the questions above.
Attachment #495666 - Flags: review?(jones.chris.g)
Comment on attachment 495667 [details] [diff] [review]
Part 4: Map for storing views

(In reply to comment #89)
> Comment on attachment 492811 [details] [diff] [review]
> Part 4: Table for storing viewports
> 
> Lots of vestigial "viewport" use in this patch.  Not calling them out
> individually for brevity's sake.
> 

This wasn't fixed.

>diff --git a/layout/ipc/RenderFrameParent.cpp b/layout/ipc/RenderFrameParent.cpp
>--- a/layout/ipc/RenderFrameParent.cpp
>+++ b/layout/ipc/RenderFrameParent.cpp
>+static void
>+BuildViewportMap(ViewportMap& oldContentViews, ViewportMap& newContentViews,
>+                   nsFrameLoader* aFrameLoader, Layer* aLayer,
>+                   float aXScale = 1, float aYScale = 1)
>+{
>+  if (view) {
>+    // View already exists. Be sure to propagate scales for any values
>+    // that need to be calculated something in chrome-doc CSS pixels.
>+    ViewConfig config = view->GetViewConfig();
>+    aXScale *= config.mXScale;
>+    aYScale *= config.mYScale;
>+    view->mOwnerContent = aFrameLoader->GetOwnerContent();
>+  } else {
>+    // View doesn't exist, so generate one. We start the view scroll offset at
>+    // the same position as the framemetric's scroll offset from the layer.
>+    ViewConfig config;
>+    config.mScrollOffset = nsPoint(
>+      NSIntPixelsToAppUnits(metrics.mViewportScrollOffset.x, auPerDevPixel) * aXScale,
>+      NSIntPixelsToAppUnits(metrics.mViewportScrollOffset.y, auPerDevPixel) * aYScale);
>+    view = new nsContentView(aFrameLoader->GetOwnerContent(), scrollId, config);

Doesn't the scale need to be propagated down in this case too?

I'm a little sad we went with this semantics instead of fixing the
frontend way back when, but ah well.  Not an ff4 blocker :).

> void
> RenderFrameParent::ShadowLayersUpdated()
> {
>   mFrameLoader->SetCurrentRemoteFrame(this);
> 
>   nsIFrame* docFrame = mFrameLoader->GetPrimaryFrameOfOwningContent();
>   if (!docFrame) {
>     // Bad, but nothing we can do about it (XXX/cjones: or is there?
>     // maybe bug 589337?).  When the new frame is created, we'll
>     // probably still be the current render frame and will get to draw
>     // our content then.  Or, we're shutting down and this update goes
>     // to /dev/null.
>     return;
>   }
> 
>+  BuildViewportMap();
>+

Big non-nit: you need to rebuild the view map after *all*
shadow-layers updates, not just when we have a frame.  Otherwise
you'll prevent dead shadow layers from being deleted.  So this call
needs to be right after |SetCurrentRemoteFrame()| above.

Need to note here the invariant we discussed: the view map always only
contains layers that are in the layer tree.  This implementation
breaks that invariant.

Again looking good.  I'd like to see the patch with these issues
fixed.
Attachment #495667 - Flags: review?(jones.chris.g)
(Giving up for the night, will review the other patches tomorrow.)
Attachment #495666 - Attachment is obsolete: true
Attachment #495667 - Attachment is obsolete: true
Attached patch Part 4: Map for storing views (obsolete) — Splinter Review
Attachment #495669 - Attachment is obsolete: true
Attachment #495669 - Flags: review?(jones.chris.g)
Attachment #495671 - Attachment is obsolete: true
Attachment #495671 - Flags: review?(jones.chris.g)
Attachment #495928 - Flags: review?(jones.chris.g)
Attachment #495929 - Flags: review?(jones.chris.g)
Attachment #495930 - Flags: review?(jones.chris.g)
Attachment #495931 - Flags: review?(jones.chris.g)
> Why does this use <centerX,centerY,leftSize,rightSize,bottomSize,topSize>
> instead of the equivalent <topX,leftY,width,height>?

To compliment the nsIDOMWindowUtils GetFramesForRect. I assumed the API looks this way so that it is possible to specify a hit region with dimensions 1 app unit x 1 app unit.

> 
> >diff --git a/content/base/src/nsFrameLoader.cpp b/content/base/src/nsFrameLoader.cpp
> >--- a/content/base/src/nsFrameLoader.cpp
> >+++ b/content/base/src/nsFrameLoader.cpp
>