605618 - Scroll iframes in parent process

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Benjamin Stover (:stechz)]

Given a point on the browser, find a unique ID that corresponds to an iframe in the child process.

Comment 1

[Benjamin Stover (:stechz)]

Attachment: Part 1: nsIContent can generate unique IDs

Comment 2

[Benjamin Stover (:stechz)]

Attachment: Part 2: Tag layers with scrollable information

Comment 3

[Benjamin Stover (:stechz)]

Attachment: Part 3: Build shadow display list

Comment 4

[Benjamin Stover (:stechz)]

Attachment: Part 4: Hit test API for frontend

Comment 5

[Benjamin Stover (:stechz)]

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.

Comment 6

[Timothy Nikkel (:tnikkel)]

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 7

[Timothy Nikkel (:tnikkel)]

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 8

[Timothy Nikkel (:tnikkel)]

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 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 10

[Benjamin Stover (:stechz)]

(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.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  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?

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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.

Comment 13

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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 14

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 15

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 16

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 17

[Benjamin Stover (:stechz)]

> 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.

Comment 18

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(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.

Comment 19

[Benjamin Stover (:stechz)]

Attachment: Part 1: nsIContent can generate unique IDs

Comment 20

[Benjamin Stover (:stechz)]

Attachment: Part 2: Tag layers with scrollable information

Comment 21

[Benjamin Stover (:stechz)]

Attachment: Part 3: Build shadow display list

Comment 22

[Benjamin Stover (:stechz)]

Attachment: Part 4: Viewport API for frontend

Comment 23

[Benjamin Stover (:stechz)]

Attachment: Part 5: Hashtable for storing viewports

Comment 24

[Benjamin Stover (:stechz)]

Attachment: Part 5: Use viewports to allow scrolling iframes in parent process

Comment 25

[Benjamin Stover (:stechz)]

Attachment: Part 6: Support displayport for iframes

Comment 26

[Benjamin Stover (:stechz)]

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.

Comment 27

[Benjamin Stover (:stechz)]

Attachment: Part 1: Tag layers with scrollable information

Comment 28

[Benjamin Stover (:stechz)]

Attachment: Part 2: Build shadow display list

Comment 29

[Benjamin Stover (:stechz)]

Attachment: Part 3: Viewport API for frontend

Comment 30

[Benjamin Stover (:stechz)]

Attachment: Part 4: Table for storing viewports

Comment 31

[Benjamin Stover (:stechz)]

Attachment: Part 5: Support displayport for iframes

Comment 32

[Benjamin Stover (:stechz)]

Attachment: Part 6: Use viewports to allow scrolling iframes in parent process

Comment 33

[Benjamin Stover (:stechz)]

Attachment: Part 1: Tag layers with scrollable information

Comment 34

[Benjamin Stover (:stechz)]

Attachment: Part 2: Infrastructure for building shadow display list

Comment 35

[Benjamin Stover (:stechz)]

Attachment: Part 4: Table for storing viewports

Comment 36

[Benjamin Stover (:stechz)]

Attachment: Part 5: Support displayport for iframes

Comment 37

[Benjamin Stover (:stechz)]

Attachment: Part 6: Use viewports to allow scrolling and build display lists

Comment 38

[Benjamin Stover (:stechz)]

Attachment: Part 3: Viewport API for frontend

Comment 39

[Benjamin Stover (:stechz)]

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 40

[Timothy Nikkel (:tnikkel)]

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 41

[Timothy Nikkel (:tnikkel)]

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?

Comment 42

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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".

Comment 43

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 44

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+class nsDisplayRemoteShadow : public nsDisplayItem

Add a comment explaining what this class is for

+    nsTArray<PRUint64>* mShadows;

And a comment here explaining what this means

Comment 45

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 46

[Benjamin Stover (:stechz)]

(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.

Comment 47

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 48

[Robert O'Callahan (:roc) (email my personal email if necessary)]

So we can't use "Remote" and we can't use "Scroll" :-)

nsIContentPortal?

Comment 49

[Oleg Romashin (:romaxa)]

Attachment: fennec changes - something wrong

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?

Comment 50

[Benjamin Stover (:stechz)]

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).

Comment 51

[Mark Finkle (:mfinkle) (use needinfo?)]

(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

Comment 52

[Benjamin Stover (:stechz)]

> >--- 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?

Comment 53

[Benjamin Stover (:stechz)]

Attachment: Part 1: Tag layers with scrollable information

Comment 54

[Benjamin Stover (:stechz)]

Attachment: Part 2: Infrastructure for building shadow display list

Comment 55

[Benjamin Stover (:stechz)]

Attachment: Part 3: Viewport API for frontend

Comment 56

[Benjamin Stover (:stechz)]

Attachment: Part 4: Table for storing viewports

Comment 57

[Benjamin Stover (:stechz)]

Attachment: Part 5: Support displayport for iframes

Comment 58

[Benjamin Stover (:stechz)]

Attachment: Part 6: Use viewports to allow scrolling and build display lists

Comment 59

[Benjamin Stover (:stechz)]

Attachment: Part 7: Include viewport and content size in API

Comment 60

[Benjamin Stover (:stechz)]

Attachment: Part 8: Implement a scrollable interface on the content side

Comment 61

[Benjamin Stover (:stechz)]

Attachment: Mobile: Use new interfaces

Comment 62

[Benjamin Stover (:stechz)]

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.

Comment 63

[Oleg Romashin (:romaxa)]

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.

Comment 64

[Oleg Romashin (:romaxa)]

Attachment: Mobile: Use new interfaces part2

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)

Comment 65

[Oleg Romashin (:romaxa)]

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.

Comment 66

[Oleg Romashin (:romaxa)]

Attachment: Mobile: Use new interfaces part2. r2. Fixed scroll/zoom and id compare

Comment 67

[Oleg Romashin (:romaxa)]

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

Comment 68

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(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.

Comment 69

[Benjamin Stover (:stechz)]

Attachment: Part 5: Support displayport for iframes

Comment 70

[Benjamin Stover (:stechz)]

Attachment: Part 8: Implement a scrollable interface on the content side

Comment 71

[Benjamin Stover (:stechz)]

Attachment: Mobile: Use new interfaces

Comment 72

[Benjamin Stover (:stechz)]

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).

Comment 73

[Benjamin Stover (:stechz)]

Attachment: Part 8: Implement a scrollable interface on the content side

Comment 74

[Benjamin Stover (:stechz)]

Attachment: Mobile: Use new interfaces

Comment 75

[Benjamin Stover (:stechz)]

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

Comment 76

[Benjamin Stover (:stechz)]

Attachment: Part 1: Tag layers with scrollable information

Comment 77

[Benjamin Stover (:stechz)]

Attachment: Part 2: Infrastructure for building shadow display list

Comment 78

[Benjamin Stover (:stechz)]

Attachment: Part 3: Viewport API for frontend

Comment 79

[Benjamin Stover (:stechz)]

Attachment: Part 4: Table for storing viewports

Comment 80

[Benjamin Stover (:stechz)]

Attachment: Part 5: Support displayport for iframes

Comment 81

[Benjamin Stover (:stechz)]

Attachment: Part 6: Use viewports to allow scrolling and build display lists

Comment 82

[Benjamin Stover (:stechz)]

Attachment: Part 7: Include viewport and content size in API

Comment 83

[Benjamin Stover (:stechz)]

Attachment: Part 8: Implement a scrollable interface on the content side

Comment 84

[Benjamin Stover (:stechz)]

Attachment: Mobile: Use new interfaces

Comment 85

[Benjamin Stover (:stechz)]

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

Comment 86

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 87

[Oleg Romashin (:romaxa)]

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 88

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 89

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 90

[Benjamin Stover (:stechz)]

> >-    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.

Comment 92

[Oleg Romashin (:romaxa)]

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

Comment 93

[Benjamin Stover (:stechz)]

(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.

Comment 94

[Benjamin Stover (:stechz)]

Attachment: Part 1: Tag layers with scrollable information

Comment 95

[Benjamin Stover (:stechz)]

Attachment: Part 2: Infrastructure for building shadow display list

Comment 96

[Benjamin Stover (:stechz)]

Attachment: Part 3: Viewport API for frontend

Comment 97

[Benjamin Stover (:stechz)]

Attachment: Part 4: Map for storing viewports

Comment 98

[Benjamin Stover (:stechz)]

Attachment: Part 5: Support displayport for iframes

Comment 99

[Benjamin Stover (:stechz)]

Attachment: Part 6: Use viewports to allow scrolling and build display lists

Comment 100

[Benjamin Stover (:stechz)]

Attachment: Part 7: Include viewport and content size in API

Comment 101

[Benjamin Stover (:stechz)]

Attachment: Part 8: Implement a scrollable interface on the content side

Comment 102

[Benjamin Stover (:stechz)]

Attachment: Mobile: Use new interfaces

Comment 103

[Benjamin Stover (:stechz)]

> 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.

Comment 104

[Benjamin Stover (:stechz)]

Attachment: Part 4: Map for storing views

Comment 105

[Benjamin Stover (:stechz)]

Attachment: Part 5: Support displayport for iframes

Comment 106

[Benjamin Stover (:stechz)]

Attachment: Part 6: Use viewports to allow scrolling and build display lists

Comment 107

[Benjamin Stover (:stechz)]

Attachment: Part 7: Include viewport and content size in API

Comment 108

[Benjamin Stover (:stechz)]

Attachment: Part 8: Implement a scrollable interface on the content side

Comment 109

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(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.

Comment 110

[Benjamin Stover (:stechz)]

> 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 111

[Timothy Nikkel (:tnikkel)]

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.

Comment 112

[Timothy Nikkel (:tnikkel)]

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 113

[Timothy Nikkel (:tnikkel)]

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?

Comment 114

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(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 115

[Oleg Romashin (:romaxa)]

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();
>-          ]]>

Comment 116

[Benjamin Stover (:stechz)]

> 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?

Comment 117

[Benjamin Stover (:stechz)]

> 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!

Comment 118

[Benjamin Stover (:stechz)]

> @@ -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.

Comment 119

[Benjamin Stover (:stechz)]

> 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.

Comment 120

[Benjamin Stover (:stechz)]

(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.

Comment 121

[Benjamin Stover (:stechz)]

Attachment: Part 1: Tag layers with scrollable information

Comment 122

[Benjamin Stover (:stechz)]

Attachment: Part 2: Infrastructure for building shadow display list

Comment 123

[Benjamin Stover (:stechz)]

Attachment: Part 3: Viewport API for frontend

Comment 124

[Benjamin Stover (:stechz)]

Attachment: Part 4: Map for storing views

Comment 125

[Benjamin Stover (:stechz)]

Attachment: Part 5: Support displayport for iframes

Comment 126

[Benjamin Stover (:stechz)]

Attachment: Part 6: Use viewports to allow scrolling and build display lists

Comment 127

[Benjamin Stover (:stechz)]

Attachment: Part 7: Include viewport and content size in API

Comment 128

[Benjamin Stover (:stechz)]

Attachment: Part 8: Implement a scrollable interface on the content side

Comment 129

[Timothy Nikkel (:tnikkel)]

(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.

Comment 130

[Timothy Nikkel (:tnikkel)]

(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?

Comment 131

[Timothy Nikkel (:tnikkel)]

(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.

Comment 132

[Benjamin Stover (:stechz)]

(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.

Comment 133

[Benjamin Stover (:stechz)]

> 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.

Comment 134

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I know of bug 593243.  It's not terribly hard to fix, tn and I discussed it.

Comment 135

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 136

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Oh, my fault on missed sr.  I didn't know we had naming conventions for numerical types.

Comment 137

[Benjamin Stover (:stechz)]

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.

Comment 138

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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.

Comment 139

[Benjamin Stover (:stechz)]

Attachment: Part 3: Viewport API for frontend

Comment 140

[Benjamin Stover (:stechz)]

Attachment: Part 4: Map for storing views

Comment 141

[Benjamin Stover (:stechz)]

Attachment: Part 5: Support displayport for iframes

Comment 142

[Benjamin Stover (:stechz)]

Attachment: Part 7: Include viewport and content size in API

Comment 143

[Benjamin Stover (:stechz)]

Attachment: Mobile: Use new interfaces

Comment 144

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 145

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 {

Comment 146

[Robert O'Callahan (:roc) (email my personal email if necessary)]

BTW I think this is going in the right direction overall.

Comment 147

[Benjamin Stover (:stechz)]

Attachment: Part 1: Tag layers with scrollable information

Comment 148

[Benjamin Stover (:stechz)]

Attachment: Part 1: Tag layers with scrollable information

Comment 149

[Benjamin Stover (:stechz)]

Attachment: Part 2: Infrastructure for building shadow display list

Comment 150

[Benjamin Stover (:stechz)]

Attachment: Part 3: Viewport API for frontend

Comment 151

[Benjamin Stover (:stechz)]

Attachment: Part 4: Map for storing views

Comment 152

[Benjamin Stover (:stechz)]

Attachment: Part 5: Support displayport for iframes

Comment 153

[Benjamin Stover (:stechz)]

Attachment: Part 6: Use viewports to allow scrolling and build display lists

Comment 154

[Benjamin Stover (:stechz)]

Attachment: Part 7: Include viewport and content size in API

Comment 155

[Benjamin Stover (:stechz)]

Attachment: Part 8: Implement a scrollable interface on the content side

Comment 156

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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!

Comment 157

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 158

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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.

Comment 159

[Benjamin Stover (:stechz)]

(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.

Comment 160

[Benjamin Stover (:stechz)]

(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?

Comment 161

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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 };

Comment 162

[Benjamin Stover (:stechz)]

> 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?

Comment 163

[Robert O'Callahan (:roc) (email my personal email if necessary)]

To make sure the compiler makes the enum at least 32 bits wide.

Comment 164

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 165

[Benjamin Stover (:stechz)]

Attachment: Part 1: Tag layers with scrollable information

Comment 166

[Benjamin Stover (:stechz)]

Attachment: Part 2: Infrastructure for building shadow display list

Comment 167

[Benjamin Stover (:stechz)]

Attachment: Part 3: Viewport API for frontend

Comment 168

[Benjamin Stover (:stechz)]

Attachment: Part 4: Map for storing views

Comment 169

[Benjamin Stover (:stechz)]

Attachment: Part 5: Support displayport for iframes

Comment 170

[Benjamin Stover (:stechz)]

Attachment: Part 6: Use viewports to allow scrolling and build display lists

Comment 171

[Benjamin Stover (:stechz)]

Attachment: Part 7: Include viewport and content size in API

Comment 172

[Benjamin Stover (:stechz)]

Attachment: Part 8: Implement a scrollable interface on the content side

Comment 173

[Benjamin Stover (:stechz)]

Attachment: Mobile: Use new interfaces

Comment 174

[Benjamin Stover (:stechz)]

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 175

[Oleg Romashin (:romaxa)]

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.

Comment 176

[Oleg Romashin (:romaxa)]

Attachment: additional patch for mobile browser

stechz: can you check this patch too... it is fixing some browser.getRootContentView - null js errors , and fixing pinch zoom

Comment 177

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 178

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 179

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(Giving up for the night, will review the other patches tomorrow.)

Comment 180

[Benjamin Stover (:stechz)]

Attachment: Part 3: Viewport API for frontend

Comment 181

[Benjamin Stover (:stechz)]

Attachment: Part 4: Map for storing views

Comment 182

[Benjamin Stover (:stechz)]

Attachment: Part 6: Use viewports to allow scrolling and build display lists

Comment 183

[Benjamin Stover (:stechz)]

Attachment: Part 7: Include viewport and content size in API

Comment 184

[Benjamin Stover (:stechz)]

> 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
> >+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 :).

The current frontend patch actually can already do this. Suppose the following sequence of events:
1) frontend grabs and keeps a reference of an iframe view.
2) iframe goes out of displayport by scrolling
3) iframe comes back into view

We end up with a view that still exists but isn't in the hash table. When the view is removed from the hash table, we must set mOwnerContent to null since it may change afterwards when we don't have any way to find the view again. Also, since RenderFrameParent no longer knows about this old view, frontend continues to update it with no appreciable change in scrolling.

The bool result lets frontend know that the content view is dead.

I would be *much* happier if a view was never invalid, and when the iframe came back into view it managed to find the reference frontend was hanging on to. Can you think of a way to do this? Would you be opposed to looking at the reference count for a view in the hash table, only removing it if the hash table has the only reference?

The return result is documented in the IDL.

> >+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?

The scale is initially set to 1 so it ends up not being necessary. Maybe we should default this to the resolution in metrics? Either way, we should document this.

> 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 :).

We can certainly consider this later :) It's nice from a panning point of view for the frontend--now, no matter what, the frontend knows exactly how to scroll things in the units it cares about without having to know what scales and resolutions are set for every view in the hierarchy up to the root.

Of course, for scroll syncing we still need to do this calculation. It may be nice to provide converters on the view itself? It's not necessary in Fennec right now considering we only ever change the scale for one view (the root).

Comment 185

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #184)
> The current frontend patch actually can already do this. Suppose the following
> sequence of events:
> 1) frontend grabs and keeps a reference of an iframe view.
> 2) iframe goes out of displayport by scrolling
> 3) iframe comes back into view
> 
> We end up with a view that still exists but isn't in the hash table. When the
> view is removed from the hash table, we must set mOwnerContent to null since it
> may change afterwards when we don't have any way to find the view again. Also,
> since RenderFrameParent no longer knows about this old view, frontend continues
> to update it with no appreciable change in scrolling.
> 

The situation you're describing sounds like a bug, the frontend holding on to a view too long.  It's hit testing on each pan, right?  It's not clear to why the frontend would try to scroll an invalid view.  If I'm not misunderstanding something, then I think this validity check would be better converted to an NS_ERROR_NOT_AVAILABLE return to throw an exception in the frontend.

> > >+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?
> 
> The scale is initially set to 1 so it ends up not being necessary. Maybe we
> should default this to the resolution in metrics? Either way, we should
> document this.
> 

OK, I understand now, thanks.  I think this is worth noting here.

Comment 186

[Benjamin Stover (:stechz)]

> The situation you're describing sounds like a bug, the frontend holding on to a
> view too long.  It's hit testing on each pan, right?  It's not clear to why the
> frontend would try to scroll an invalid view.  If I'm not misunderstanding
> something, then I think this validity check would be better converted to an
> NS_ERROR_NOT_AVAILABLE return to throw an exception in the frontend.

Currently the code holds on to the view for a little longer than one pan (it's cached based on timeouts in browser.xml, not pans). This can be changed though I'm not sure it's strictly bad behavior.

In my ideal world it shouldn't even be possible to have different views with the same ID (one valid, the rest not), but I think if that's not possible an error makes the most sense.

Comment 187

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Yeah, cache can be discussed later or in another bug.  Not sure timeout is what we want.

It doesn't make sense to hold onto Views forever in the platform (even within a single loaded page) because in the current system, the chrome process has no way to distinguish between a View that goes away because its layer became invisible, vs. one that goes away because the underlying content DOM object was deleted.

Comment 188

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 495928 [details] [diff] [review]
Part 3: Viewport API for frontend

>+  boolean scrollTo(in float xPx, in float yPx);
>+  boolean scrollBy(in float dxPx, in float dyPx);
>+
>+  boolean setScale(in float xScale, in float yScale);
>+

Per discussion, s/boolean/void/ and throw when they're expired,
+updated comment.


>+nsresult
>+nsContentView::Update(const ViewConfig& aConfig, PRBool *aResult)
>+{
>+  *aResult = PRBool(mOwnerContent);

Probably want, 

  if (!mOwnerContent)
    return NS_ERROR_NOT_AVAILABLE;

here

r=me with those changes.

Comment 189

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 495929 [details] [diff] [review]
Part 4: Map for storing views

(In reply to comment #178)
> 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] [details]
> > 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.
> 

Sigh, *still* lots of vestigial "viewport".

r=me with that fixed.

Comment 190

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #188)
> Probably want, 
> 
>   if (!mOwnerContent)
>     return NS_ERROR_NOT_AVAILABLE;

Sorry, should also clarify that

if (foo) {
  return blah;
}

is prevalent style (braced return stmt).

Comment 191

[Benjamin Stover (:stechz)]

Oops, grepped for "viewport" but not "Viewport".

Comment 192

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 495930 [details] [diff] [review]
Part 6: Use viewports to allow scrolling and build display lists

>+struct Transform {

This isn't a generic transform, but instead represents just the
transforms we allow for views.  Suggest the name |ViewTransform|, with
a comment noting the qualified use of "transform".
 
>+static FrameMetrics
>+GetFrameMetrics(Layer* aLayer)

This helper would be a bit simpler and more efficient written as

  static const FrameMetrics*
  GetFrameMetrics()

and returning null or &container->GetFrameMetrics().  (That method
returns a const reference, so this is OK.)

>+// Use shadow layer tree to build display list for the browser's frame.
> static void
>-UpdateShadowSubtree(Layer* aSubtreeRoot)
>+BuildListForLayer(Layer* aLayer,
>+                  nsFrameLoader* aFrameLoader,
>+                  gfx3DMatrix aTransform,
>+                  nsDisplayListBuilder* aBuilder,
>+                  nsDisplayList& aShadowTree,
>+                  nsIFrame* aFrame)
> {
>+    // Calculate transform for this layer.
>+    nsContentView* view =
>+      aFrameLoader->GetCurrentRemoteFrame()->GetContentView(scrollId);
>+    gfx3DMatrix applyTransform = ComputeShadowTreeTransform(
>+      aFrame, metrics, view->GetViewConfig(), aBuilder,
>+      1 / aTransform._11, 1 / aTransform._22);

gfx3dMatrix helpers plz.

>+    transform = applyTransform * aLayer->GetTransform() * aTransform;
>+
>+    // If this is the root layer, then applyTransform contains the offset
>+    // translation for aFrameLoader, but aTransform does not.
>+    if (view->IsRoot()) {
>+      nsIntPoint frameOffset = GetRootFrameOffset(aFrame, aBuilder);
>+
>+      // Column 4 contains the translations.
>+      aTransform._41 += frameOffset.x;
>+      aTransform._42 += frameOffset.y;

gfx3dMatrix helpers plz.

>+    }
>+
>+    // Calculate rect for this layer based on transform and aTransform.
>+    nsRect bounds;
>+    {
>+      nscoord auPerDevPixel = aFrame->PresContext()->AppUnitsPerDevPixel();
>+      bounds = metrics.mViewport.ToAppUnits(auPerDevPixel);
>+
>+      // Column 4 contains translations
>+      // Diagonal entries (1, 1) and (2, 2) contains the scale
>+      bounds.x = bounds.x * transform._11 + aTransform._41 * auPerDevPixel;
>+      bounds.y = bounds.y * transform._22 + aTransform._42 * auPerDevPixel;

gfx3dMatrix helpers plz.

>+TransformShadowTree(nsDisplayListBuilder* aBuilder, nsFrameLoader* aFrameLoader,
>+                    nsIFrame* aFrame, Layer* aLayer,
>+                    float aXScale = 1, float aYScale = 1)
> {
>+  const FrameMetrics& metrics = GetFrameMetrics(aLayer);

This is a reference to deallocated stack space.

>+  const ViewID scrollId = metrics.mScrollId;
>+
>+  gfx3DMatrix shadowTransform;
>+
>+  if (scrollId) {

if (metrics && metrics->IsScrollable())

>+    const nsContentView* view =
>+      aFrameLoader->GetCurrentRemoteFrame()->GetContentView(scrollId);
>+    NS_ASSERTION(view, "Array of views should be consistent with layer tree");
>+

ABORT_IF_FALSE

>+  aXScale *= shadowTransform._11;
>+  aYScale *= shadowTransform._22;
>+

gfx3dMatrix helpers plz.

> void
>@@ -465,19 +576,19 @@ RenderFrameParent::BuildDisplayList(nsDi
> {
>   // We're the subdoc for <browser remote="true"> and it has
>   // painted content.  Display its shadow layer tree.
>   nsDisplayList shadowTree;
>   shadowTree.AppendToTop(
>     new (aBuilder) nsDisplayRemote(aBuilder, aFrame, this));
> 

This nsDisplayRemote is going to be deadweight here for hit tests,
right?  Should this instead be,

>   if (aBuilder->IsForEventDelivery()) {
>-    nscoord auPerDevPixel = aFrame->PresContext()->AppUnitsPerDevPixel();
>     nsRect bounds = aFrame->EnsureInnerView()->GetBounds();
>-    // XXX build display list based on shadow tree
>+    BuildListForLayer(GetRootLayer(), mFrameLoader, gfx3DMatrix(),
>+                      aBuilder, shadowTree, aFrame);
>   }
> 

 else {
    shadowTree.AppendToTop(
      new (aBuilder) nsDisplayRemote(aBuilder, aFrame, this));
 }

r=me conditional on the above fixed and tn's r+, he needs to review the
display-list construction.

Comment 193

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 495931 [details] [diff] [review]
Part 7: Include viewport and content size in API

>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
>   /**
>+   * Dimensions of the viewport in chrome-document CSS pixels.
>+   */
>+  readonly attribute float viewportWidth;
>+  readonly attribute float viewportHeight;
>+

I don't see mViewportSize being set to non-default anywhere in this
patch, so I don't know to what it's supposed to correspond.  Seems
potentially confusing.

>+  /**
>+   * Dimensions of scrolled content in chrome-document CSS pixels.
>+   */
>+  readonly attribute float contentWidth;
>+  readonly attribute float contentHeight;

Seems like we could be more specific than "content" ...  Maybe
"scrollable"?

>diff --git a/gfx/src/nsSize.h b/gfx/src/nsSize.h
>--- a/gfx/src/nsSize.h
>+++ b/gfx/src/nsSize.h
>+  inline nsIntSize ToNearestPixels(nscoord aAppUnitsPerPixel) const;

I looked into adding this same helper to an older patch and was told
not to, since it's not an operation that generally makes sense in
layout, which only snaps rects.  See ThebesLayerBuffer.cpp.

The rest of implementation here looks OK to me.

Comment 194

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 495672 [details] [diff] [review]
Part 8: Implement a scrollable interface on the content side

I don't think going through the frame tree to look up scrollable things is the right approach here.  The scroll ID is a content/DOM thing, so I think we're going to need to start from content/DOM to dig those out.  The frame tree can go away at arbitrary times, and that would imply an undue burden on the frontend to queue these operations.  (Or ignoring them, bad UX.)

Is there a reason the frontend can't use DOM APIs to look these up, all in JS, maybe using getElementsByTagName() with a backstage-pass check for this magic ID?  If that's too slow, I'm not against a special interface for this, but I'd like to not add unnecessary APIs.

Comment 195

[Benjamin Stover (:stechz)]

> I don't think going through the frame tree to look up scrollable things is the
> right approach here.  The scroll ID is a content/DOM thing, so I think we're
> going to need to start from content/DOM to dig those out.  The frame tree can
> go away at arbitrary times, and that would imply an undue burden on the
> frontend to queue these operations.  (Or ignoring them, bad UX.)

Could you give a use case where the frame tree would go away while the user is panning? If it goes away, so does the layer and all of its metrics information. I don't see any realistically bad UX here.

> Is there a reason the frontend can't use DOM APIs to look these up, all in JS,
> maybe using getElementsByTagName() with a backstage-pass check for this magic
> ID?  If that's too slow, I'm not against a special interface for this, but I'd
> like to not add unnecessary APIs.

It's a little forward thinking at this point, but keep in mind we'd have to recursively iterate through all documents looking for every iframe. And soon, looking for every element (since almost any element could be overflow scroll). We'll need a new place to hang display port off anyways (it could be something you could QI from the element once you have it I guess).

And we'll be doing this search every time displayport is updated? Maybe we could cache it for some time? This really seems like it should be supported by an API by me, but I don't have any hard data to back it up for performance reasons.

Comment 196

[Benjamin Stover (:stechz)]

> I don't see mViewportSize being set to non-default anywhere in this
> patch, so I don't know to what it's supposed to correspond.  Seems
> potentially confusing.

It's set when it is generated in BuildViewMap.

https://bugzilla.mozilla.org/attachment.cgi?id=495931&action=diff#a/layout/ipc/RenderFrameParent.cpp_sec1

> 
> >+  /**
> >+   * Dimensions of scrolled content in chrome-document CSS pixels.
> >+   */
> >+  readonly attribute float contentWidth;
> >+  readonly attribute float contentHeight;
> 
> Seems like we could be more specific than "content" ...  Maybe
> "scrollable"?

That seems confusing. Earlier I was referring to scrollables as things that can be scrolled, ie the viewport, not the content that is being viewed inside the viewport.

scrolledWidth/Height? scrolledContentWidth/Height? I hate naming things. :)

Comment 197

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #195)
> > I don't think going through the frame tree to look up scrollable things is the
> > right approach here.  The scroll ID is a content/DOM thing, so I think we're
> > going to need to start from content/DOM to dig those out.  The frame tree can
> > go away at arbitrary times, and that would imply an undue burden on the
> > frontend to queue these operations.  (Or ignoring them, bad UX.)
> 
> Could you give a use case where the frame tree would go away while the user is
> panning?

Nope!  But that's what I'm told.  tn or roc could answer this better.

> If it goes away, so does the layer and all of its metrics information.

All that comes back anew the next time the layer tree is reconstructed, with no loss of information.

Note we have the same problem with attaching a ViewConfig to the <browser> in the chrome process.  Instead of sticking it on the browser's primary frame, we stick it on its frame loader and access it from layout when it's needed.  So the view config persists across frame tree reconstruction.

> I don't see any realistically bad UX here.

Don't follow.

> > Is there a reason the frontend can't use DOM APIs to look these up, all in JS,
> > maybe using getElementsByTagName() with a backstage-pass check for this magic
> > ID?  If that's too slow, I'm not against a special interface for this, but I'd
> > like to not add unnecessary APIs.
> 
> It's a little forward thinking at this point, but keep in mind we'd have to
> recursively iterate through all documents looking for every iframe. And soon,
> looking for every element (since almost any element could be overflow scroll).
> We'll need a new place to hang display port off anyways (it could be something
> you could QI from the element once you have it I guess).
> 

I wouldn't be surprised if getElementsByTagName() is already optimized, but I don't know.  Note that a content+DOM impl of what this patch does would probably end up looking a lot like getElementsByTagName().

> And we'll be doing this search every time displayport is updated? Maybe we
> could cache it for some time? This really seems like it should be supported by
> an API by me, but I don't have any hard data to back it up for performance
> reasons.

Cache could help.  We should get perf data before adding possibly unnecessary new APIs.

Comment 198

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #196)
> > I don't see mViewportSize being set to non-default anywhere in this
> > patch, so I don't know to what it's supposed to correspond.  Seems
> > potentially confusing.
> 
> It's set when it is generated in BuildViewMap.
> 
> https://bugzilla.mozilla.org/attachment.cgi?id=495931&action=diff#a/layout/ipc/RenderFrameParent.cpp_sec1
> 

It's set to metrics.mViewportSize, but I don't see that ever being set.  It appears like it will always be <0, 0>.

> > 
> > >+  /**
> > >+   * Dimensions of scrolled content in chrome-document CSS pixels.
> > >+   */
> > >+  readonly attribute float contentWidth;
> > >+  readonly attribute float contentHeight;
> > 
> > Seems like we could be more specific than "content" ...  Maybe
> > "scrollable"?
> 
> That seems confusing. Earlier I was referring to scrollables as things that can
> be scrolled, ie the viewport, not the content that is being viewed inside the
> viewport.
> 
> scrolledWidth/Height? scrolledContentWidth/Height? I hate naming things. :)

I defer to roc or tn here.

Comment 199

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #197)
> Note we have the same problem with attaching a ViewConfig to the <browser> in
> the chrome process.  Instead of sticking it on the browser's primary frame, we
> stick it on its frame loader and access it from layout when it's needed.  So
> the view config persists across frame tree reconstruction.

... and it can be updated when there's no frame tree, which is what I was angling at.

Comment 200

[Benjamin Stover (:stechz)]

> It's set to metrics.mViewportSize, but I don't see that ever being set.  It
> appears like it will always be <0, 0>.

It's set to metrics mViewport, which was changed from mViewportSize in an earlier patch due to roc's suggestion in part 5. Maybe I'm misunderstanding something silly here.

Comment 201

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Nope, I missed that.  OK.

Comment 202

[Robert O'Callahan (:roc) (email my personal email if necessary)]

> +    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.

Seems like this didn't get addressed.

This is very good work but it's a bit disconcerting when comments don't get addressed.

Comment 203

[Benjamin Stover (:stechz)]

Attachment: Part 1: Tag layers with scrollable information

Comment 204

[Benjamin Stover (:stechz)]

Attachment: Part 2: Infrastructure for building shadow display list

Comment 205

[Benjamin Stover (:stechz)]

Attachment: Part 3: Viewport API for frontend

Comment 206

[Benjamin Stover (:stechz)]

Attachment: Part 4: Map for storing views

Comment 207

[Benjamin Stover (:stechz)]

Attachment: Part 5: Support displayport for iframes

Comment 208

[Benjamin Stover (:stechz)]

Attachment: Part 6: Use viewports to allow scrolling and build display lists

Comment 209

[Benjamin Stover (:stechz)]

Attachment: Part 7: Include viewport and content size in API

Comment 210

[Benjamin Stover (:stechz)]

Attachment: Part 8: Implement a scrollable interface on the content side

Comment 211

[Benjamin Stover (:stechz)]

Attachment: Mobile: Use new interfaces

Comment 212

[Benjamin Stover (:stechz)]

Attachment: Part 5: Support displayport for iframes

Comment 213

[Benjamin Stover (:stechz)]

Attachment: Mobile: Use new interfaces

Comment 214

[Benjamin Stover (:stechz)]

OK, displayport patch now uses UserData so that script can access remote ID, and mobile patch is updated to use it.

I've marked the last patch as obsolete but we can always revive it if performance problems occur.

Comment 215

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 496334 [details] [diff] [review]
Part 5: Support displayport for iframes

What changed that I need to re-review this?

Comment 216

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 496300 [details] [diff] [review]
Part 7: Include viewport and content size in API

>   nsIScrollableFrame* rootScrollableFrame =
>     presShell->GetRootScrollFrameAsScrollable();
>   if (rootScrollableFrame) {
>+    nsSize contentSize = 
>+      rootScrollableFrame->GetScrollRange().Size() +
>+      rootScrollableFrame->GetScrollPortRect().Size();
>+    metrics.mContentSize = nsIntSize(NSAppUnitsToIntPixels(contentSize.width, auPerCSSPixel),
>+                                     NSAppUnitsToIntPixels(contentSize.height, auPerCSSPixel));

>+  view->mViewportSize = nsSize(
>+    NSIntPixelsToAppUnits(metrics.mViewport.width, auPerDevPixel) * aXScale,
>+    NSIntPixelsToAppUnits(metrics.mViewport.height, auPerDevPixel) * aYScale);
>+  view->mContentSize = nsSize(
>+    NSIntPixelsToAppUnits(metrics.mContentSize.width, auPerDevPixel) * aXScale,
>+    NSIntPixelsToAppUnits(metrics.mContentSize.height, auPerDevPixel) * aYScale);

I think this suffers from the same CSS/dev pixel conversion bug I
originally made with scrollOffset, but this is using the same
conversion and we're not being bitten yet, so I'm OK with fixing all
these together when needed.

I'm still not a fan of |contentSize|.  Maybe roc has a better idea,
and he needs to sr this.

Comment 217

[Benjamin Stover (:stechz)]

(In reply to comment #215)
> Comment on attachment 496334 [details] [diff] [review]
> Part 5: Support displayport for iframes
> 
> What changed that I need to re-review this?

SubtractFromVisibleRegion is gone and using SetUserData to store IDs. I can switch to sr+ if you don't need to check these changes.

Comment 218

[Robert O'Callahan (:roc) (email my personal email if necessary)]

nsINode::SetUserData says
   * Should only be used to implement the DOM Level 3 UserData API.

Shouldn't you be using nsINode::SetProperty instead? Then you don't need to use an nsIVariant, you can just do 'new PRUint64'.

Comment 219

[Benjamin Stover (:stechz)]

(In reply to comment #218)
> nsINode::SetUserData says
>    * Should only be used to implement the DOM Level 3 UserData API.
> 
> Shouldn't you be using nsINode::SetProperty instead? Then you don't need to use
> an nsIVariant, you can just do 'new PRUint64'.

I was told to use SetUserData because script could not access GetProperty. Is this not true?

Comment 220

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I guess you'll have to create some scriptable way to get the property. I don't think using SetUserData is an option.

Comment 221

[Benjamin Stover (:stechz)]

I need some alternate proposals then. I'm a big fan of just adding a new interface like part 8 did :)

Comment 222

[Robert O'Callahan (:roc) (email my personal email if necessary)]

You don't need a new interface class just to get the viewID property. Just add a method to DOMWindowUtils?

Comment 223

[Benjamin Stover (:stechz)]

(In reply to comment #222)
> You don't need a new interface class just to get the viewID property. Just add
> a method to DOMWindowUtils?

Sure, DOMWindowUtils_GECKO2.0 or whatever it is?

So would the method find the ID and return the element similar to the previous incarnation, or would you pass in an element for checking?

Comment 224

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The former seems better, it'll be a lot faster.

Comment 225

[Benjamin Stover (:stechz)]

Attachment: Part 1: Tag layers with scrollable information

Comment 226

[Benjamin Stover (:stechz)]

Attachment: Part 2: Infrastructure for building shadow display list

Comment 227

[Benjamin Stover (:stechz)]

Attachment: Part 3: Viewport API for frontend

Comment 228

[Benjamin Stover (:stechz)]

Attachment: Part 4: Map for storing views

Comment 229

[Benjamin Stover (:stechz)]

Attachment: Part 5: Support displayport for iframes

Comment 230

[Benjamin Stover (:stechz)]

Attachment: Part 6: Use viewports to allow scrolling and build display lists

Comment 231

[Benjamin Stover (:stechz)]

Attachment: Part 7: Include viewport and content size in API

Comment 232

[Benjamin Stover (:stechz)]

Attachment: Part 8: Content process map from view IDs to content elements

Comment 233

[Benjamin Stover (:stechz)]

Attachment: Part 9: Fix test-ipcbrowser and crash

Comment 234

[Benjamin Stover (:stechz)]

Attachment: Mobile: Use new interfaces

Comment 235

[Benjamin Stover (:stechz)]

Part 8 moves sScrollIdCounter and the generation of content view IDs to Layers.cpp, all away from the prying eyes of others. A map from view IDs to content elements is also added to Layers.cpp.

Part 9 fixes a crash and test-ipcbrowser.

Comment 236

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 496950 [details] [diff] [review]
Part 8: Content process map from view IDs to content elements

The code looks mostly OK to me on first blush, but I don't think the organization is right.  gfx/layers is two levels removed from nsIContent, need to be clear of it down there.  I would have expected this map to live in content/ somewhere, but having this kind of logic in layout/base passed review already, so maybe we should keep the map in there.  (feedback?ing tn/roc so that they can confirm.)

Comment 237

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 496950 [details] [diff] [review]
Part 8: Content process map from view IDs to content elements

Yeah I think the map should live in content or layout.

Comment 238

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 496951 [details] [diff] [review]
Part 9: Fix test-ipcbrowser and crash

>diff --git a/layout/ipc/RenderFrameParent.cpp b/layout/ipc/RenderFrameParent.cpp
>--- a/layout/ipc/RenderFrameParent.cpp
>+++ b/layout/ipc/RenderFrameParent.cpp
>@@ -553,17 +553,17 @@ RenderFrameParent::DeallocPLayers(PLayer
>   delete aLayers;
>   return true;
> }
> 
> void
> RenderFrameParent::BuildViewMap()
> {
>   ViewMap newContentViews;
>-  if (GetRootLayer()) {
>+  if (GetRootLayer() && mFrameLoader->GetPrimaryFrameOfOwningContent()) {

It's not obvious to me what problem this is fixing.  Add a comment and
repost?

>diff --git a/layout/ipc/test-ipcbrowser-chrome.js b/layout/ipc/test-ipcbrowser-chrome.js

Don't need review here :).

Comment 239

[Timothy Nikkel (:tnikkel)]

Comment on attachment 496948 [details] [diff] [review]
Part 6: Use viewports to allow scrolling and build display lists

+  // |aConfig.mScrollOffset|           What our user expects, or wants, the
+  //                                   frame scroll offset to be in chrome
+  //                                   document CSS pixels.

+  nsIntPoint scrollOffset =
+    aConfig.mScrollOffset.ToNearestPixels(auPerDevPixel);

So it's actually in chrome document app units, not CSS pixels, right?

+nsDisplayScrollLayer::BuildLayer(nsDisplayListBuilder* aBuilder,
+  RecordFrameMetrics(mFrame, layer, mVisibleRect, mClip, scrollId);

mVisibleRect doesn't take into account the display port, so it's wrong, isn't it?

@@ -5996,17 +5996,52 @@ void PresShell::SetRenderingState(const 
+  nsCOMPtr<nsISupports> container = mPresContext->GetContainer();
+  if (!container) {
+    container = do_QueryReferent(mForwardingContainer);
+  }
+
+  nsCOMPtr<nsIDocShellTreeItem> treeItem = do_QueryInterface(container);
+  if (treeItem) {
+    nsCOMPtr<nsIDocShellTreeItem> rootItem;
+    treeItem->GetSameTypeRootTreeItem(getter_AddRefs(rootItem));
+    nsCOMPtr<nsIDocShell> rootDocShell = do_QueryInterface(rootItem);
+    if (rootDocShell) {
+      nsCOMPtr<nsIPresShell> presShell;
+      rootDocShell->GetPresShell(getter_AddRefs(presShell));
+      if (presShell && presShell->FrameManager()) {
+        rootFrame = presShell->FrameManager()->GetRootFrame();
+      }
+    }
+  }
+

I think you want to just get the root pres context here and get it's root frame.

+nsDisplayScrollLayer::ComputeVisibility(nsDisplayListBuilder* aBuilder,
+    nsRegion childVisibleRegion = presShell->GetDisplayPort() + ToReferenceFrame();

The display port is relative to the root frame? The underlying frame for nsDisplayScrollLayer is the root scrollable frame, so ToReferenceFrame is the offset from the root scrollable frame to the reference frame. So the offset here may not be correct, although that may not make a difference.

Comment 240

[Timothy Nikkel (:tnikkel)]

Comment on attachment 496950 [details] [diff] [review]
Part 8: Content process map from view IDs to content elements

Do I need to look at this or wait for a revised patch?

Comment 241

[Benjamin Stover (:stechz)]

(In reply to comment #240)
> Comment on attachment 496950 [details] [diff] [review]
> Part 8: Content process map from view IDs to content elements
> 
> Do I need to look at this or wait for a revised patch?

No need to look, just parts 5 and 6 please.

Comment 242

[Benjamin Stover (:stechz)]

> Yeah I think the map should live in content or layout.

I put it in nsLayoutUtils.

> It's not obvious to me what problem this is fixing.  Add a comment and
> repost?

Check.

> +  nsIntPoint scrollOffset =
> +    aConfig.mScrollOffset.ToNearestPixels(auPerDevPixel);
> 
> So it's actually in chrome document app units, not CSS pixels, right?

Yes, check.

> +nsDisplayScrollLayer::BuildLayer(nsDisplayListBuilder* aBuilder,
> +  RecordFrameMetrics(mFrame, layer, mVisibleRect, mClip, scrollId);
> 
> mVisibleRect doesn't take into account the display port, so it's wrong, isn't
> it?

ComputeVisibilityForSublist sets mVisibleRect, which does account for display port.

> I think you want to just get the root pres context here and get it's root
> frame.

Check.

> +nsDisplayScrollLayer::ComputeVisibility(nsDisplayListBuilder* aBuilder,
> +    nsRegion childVisibleRegion = presShell->GetDisplayPort() +
> ToReferenceFrame();
> 
> The display port is relative to the root frame? The underlying frame for
> nsDisplayScrollLayer is the root scrollable frame, so ToReferenceFrame is the
> offset from the root scrollable frame to the reference frame. So the offset
> here may not be correct, although that may not make a difference.

Should I be calling ToReferenceFrame for the viewport frame instead of the scrolled content frame? Could you define a case where it would make a difference? I've tried various test pages with different frame scroll offsets, and it seems to work fine.

Comment 243

[Timothy Nikkel (:tnikkel)]

(In reply to comment #242)
> > +nsDisplayScrollLayer::BuildLayer(nsDisplayListBuilder* aBuilder,
> > +  RecordFrameMetrics(mFrame, layer, mVisibleRect, mClip, scrollId);
> > 
> > mVisibleRect doesn't take into account the display port, so it's wrong, isn't
> > it?
> 
> ComputeVisibilityForSublist sets mVisibleRect, which does account for display
> port.

Which ComputeVisibilityForSublist call? The one on the sublist containing the nsDisplayScrollLayer item or the call in nsDisplayScrollLayer::ComputeVisibility? The latter sets the mVisibleRect on the mList member of the nsDisplayScrollLayer object, not the mVisibleRect member of nsDisplayScrollLayer. For the former its not clear to me how the display port rect gets to that call.

> Should I be calling ToReferenceFrame for the viewport frame instead of the
> scrolled content frame? Could you define a case where it would make a
> difference? I've tried various test pages with different frame scroll offsets,
> and it seems to work fine.

If its correct, and its not hard to make it do the correct thing, is there a reason we shouldn't do it that way?

Comment 244

[Benjamin Stover (:stechz)]

> Which ComputeVisibilityForSublist call? The one on the sublist containing the
> nsDisplayScrollLayer item or the call in
> nsDisplayScrollLayer::ComputeVisibility? The latter sets the mVisibleRect on
> the mList member of the nsDisplayScrollLayer object, not the mVisibleRect
> member of nsDisplayScrollLayer. For the former its not clear to me how the
> display port rect gets to that call.

You're right. Fixed locally.

> If its correct, and its not hard to make it do the correct thing, is there a
> reason we shouldn't do it that way?

Looks good with this change. Fixed locally.

Comment 245

[Timothy Nikkel (:tnikkel)]

Comment on attachment 496948 [details] [diff] [review]
Part 6: Use viewports to allow scrolling and build display lists

>+      bounds = metrics->mViewport.ToAppUnits(auPerDevPixel);

Where did mViewport get added to FrameMetrics? I don't see it added anywhere in this bug and its not on m-c. Where is its value set?

Comment 246

[Timothy Nikkel (:tnikkel)]

Comment on attachment 496948 [details] [diff] [review]
Part 6: Use viewports to allow scrolling and build display lists

+  // |aConfig.mScrollOffset|           What our user expects, or wants, the
+  //                                   frame scroll offset to be in chrome
+  //                                   document CSS pixels.
   nscoord auPerDevPixel = aContainerFrame->PresContext()->AppUnitsPerDevPixel();
+    aConfig.mScrollOffset.ToNearestPixels(auPerDevPixel);

So it looks like mScrollOffset is in app units.


 ComputeShadowTreeTransform(nsIFrame* aContainerFrame,
+  nsIntPoint shadowTranslation(0, 0);
+  if (aMetrics->IsRootScrollable())
+    shadowTranslation = GetRootFrameOffset(aContainerFrame, aBuilder);

+BuildListForLayer(Layer* aLayer,
+    gfx3DMatrix applyTransform = ComputeShadowTreeTransform(
+      aFrame, metrics, view->GetViewConfig(), aBuilder,
+      1 / GetXScale(aTransform), 1 / GetYScale(aTransform));
+    transform = applyTransform * aLayer->GetTransform() * aTransform;
+
+    // If this is the root layer, then applyTransform contains the offset
+    // translation for aFrameLoader, but aTransform does not. We need this
+    // for bounds calculation.
+    if (view->IsRoot()) {
+      Translate(aTransform, GetRootFrameOffset(aFrame, aBuilder));
+    }

Can we Translate aTransform by GetRootFrameOffset before the call to ComputeShadowTreeTransform and then remove the GetRootFrameOffset call in ComputeShadowTreeTransform?

In fact, do we not want to make the same change to aTransform if we take the else branch of the "if (metrics && metrics->IsScrollable())"? If so, then can we factor this out to happen in RenderFrameParent::BuildDisplayList where we call BuildListForLayer?

+BuildListForLayer(Layer* aLayer,
+                  nsFrameLoader* aFrameLoader,
+                  gfx3DMatrix aTransform,
+                  nsDisplayListBuilder* aBuilder,
+                  nsDisplayList& aShadowTree,
+                  nsIFrame* aFrame)

The names aFrame and aFrameLoader can be confusing here. They never change, they are always the frame and frameloader for the toplevel content document (the top level remote document). aRootContentLoader? aRootContentFrame (although it's actually the subdocument frame containing the root content document)? or something better.

Similarly in part 2 where you define the class nsDisplayRemoteShadow you should add a comment about what the argument aFrame actually is to the constructor.

And having more than one item that has the same type (nsDisplayRemoteShadow) with the same underlying frame could break assumptions, but nsDisplayRemoteShadow isn't used in those situations. So I think you should override GetPerFrameKey in nsDisplayRemoteShadow and assert that it is never called (but otherwise the same), and assert in the constructor for nsDisplayRemoteShadow that the display list builder is in event delivery mode.

-  if (aBuilder->IsForEventDelivery()) {
-    nscoord auPerDevPixel = aFrame->PresContext()->AppUnitsPerDevPixel();
-    nsRect bounds = aFrame->EnsureInnerView()->GetBounds();
-    // XXX build display list based on shadow tree
-  }

If we are just going to remove this here, lets just not add it in the first place. I feel like I said this before somewhere else.

Comment 247

[Benjamin Stover (:stechz)]

(In reply to comment #245)
> Comment on attachment 496948 [details] [diff] [review]
> Part 6: Use viewports to allow scrolling and build display lists
> 
> >+      bounds = metrics->mViewport.ToAppUnits(auPerDevPixel);
> 
> Where did mViewport get added to FrameMetrics? I don't see it added anywhere in
> this bug and its not on m-c. Where is its value set?

mViewportSize has been changed to mViewport in part 1.

(In reply to comment #246)
> Comment on attachment 496948 [details] [diff] [review]
> Part 6: Use viewports to allow scrolling and build display lists
> 
> +  // |aConfig.mScrollOffset|           What our user expects, or wants, the
> +  //                                   frame scroll offset to be in chrome
> +  //                                   document CSS pixels.
>    nscoord auPerDevPixel =
> aContainerFrame->PresContext()->AppUnitsPerDevPixel();
> +    aConfig.mScrollOffset.ToNearestPixels(auPerDevPixel);
> 
> So it looks like mScrollOffset is in app units.
> 

Like I said above, fixed in local queue.

> 
>  ComputeShadowTreeTransform(nsIFrame* aContainerFrame,
> +  nsIntPoint shadowTranslation(0, 0);
> +  if (aMetrics->IsRootScrollable())
> +    shadowTranslation = GetRootFrameOffset(aContainerFrame, aBuilder);
> 
> +BuildListForLayer(Layer* aLayer,
> +    gfx3DMatrix applyTransform = ComputeShadowTreeTransform(
> +      aFrame, metrics, view->GetViewConfig(), aBuilder,
> +      1 / GetXScale(aTransform), 1 / GetYScale(aTransform));
> +    transform = applyTransform * aLayer->GetTransform() * aTransform;
> +
> +    // If this is the root layer, then applyTransform contains the offset
> +    // translation for aFrameLoader, but aTransform does not. We need this
> +    // for bounds calculation.
> +    if (view->IsRoot()) {
> +      Translate(aTransform, GetRootFrameOffset(aFrame, aBuilder));
> +    }
> 
> Can we Translate aTransform by GetRootFrameOffset before the call to
> ComputeShadowTreeTransform and then remove the GetRootFrameOffset call in
> ComputeShadowTreeTransform?

Done.

> In fact, do we not want to make the same change to aTransform if we take the
> else branch of the "if (metrics && metrics->IsScrollable())"? If so, then can
> we factor this out to happen in RenderFrameParent::BuildDisplayList where we
> call BuildListForLayer?

There's an implied precondition here that if there is a shadow tree, it will always contain one root scroll layer that is the ascendant of any other scroll layers. I'm having a hard time of thinking how to make this assertion explicit (the same idea is used in TransformShadowTree).

> 
> +BuildListForLayer(Layer* aLayer,
> +                  nsFrameLoader* aFrameLoader,
> +                  gfx3DMatrix aTransform,
> +                  nsDisplayListBuilder* aBuilder,
> +                  nsDisplayList& aShadowTree,
> +                  nsIFrame* aFrame)
> 
> The names aFrame and aFrameLoader can be confusing here. They never change,
> they are always the frame and frameloader for the toplevel content document
> (the top level remote document). aRootContentLoader? aRootContentFrame
> (although it's actually the subdocument frame containing the root content
> document)? or something better.

Done.

> Similarly in part 2 where you define the class nsDisplayRemoteShadow you should
> add a comment about what the argument aFrame actually is to the constructor.

Done.

> And having more than one item that has the same type (nsDisplayRemoteShadow)
> with the same underlying frame could break assumptions, but
> nsDisplayRemoteShadow isn't used in those situations. So I think you should
> override GetPerFrameKey in nsDisplayRemoteShadow and assert that it is never
> called (but otherwise the same), and assert in the constructor for
> nsDisplayRemoteShadow that the display list builder is in event delivery mode.
> 

Done.

> -  if (aBuilder->IsForEventDelivery()) {
> -    nscoord auPerDevPixel = aFrame->PresContext()->AppUnitsPerDevPixel();
> -    nsRect bounds = aFrame->EnsureInnerView()->GetBounds();
> -    // XXX build display list based on shadow tree
> -  }
> 
> If we are just going to remove this here, lets just not add it in the first
> place. I feel like I said this before somewhere else.

It is just a placeholder...but ok.

Comment 248

[Benjamin Stover (:stechz)]

Attachment: Part 1: Tag layers with scrollable information

Comment 249

[Benjamin Stover (:stechz)]

Attachment: Part 2: Infrastructure for building shadow display list

Comment 250

[Benjamin Stover (:stechz)]

Attachment: Part 3: Viewport API for frontend

Comment 251

[Benjamin Stover (:stechz)]

Attachment: Part 4: Map for storing views

Comment 252

[Benjamin Stover (:stechz)]

Attachment: Part 5: Support displayport for iframes

Comment 253

[Benjamin Stover (:stechz)]

Attachment: Part 6: Use viewports to allow scrolling and build display lists

Comment 254

[Benjamin Stover (:stechz)]

Attachment: Part 7: Include viewport and content size in API

Comment 255

[Benjamin Stover (:stechz)]

Attachment: Part 8: Content process map from view IDs to content elements

Comment 256

[Benjamin Stover (:stechz)]

Attachment: Part 9: Fix test-ipcbrowser and crash

Comment 257

[Benjamin Stover (:stechz)]

Attachment: Mobile: Use new interfaces

Comment 258

[Timothy Nikkel (:tnikkel)]

(In reply to comment #247)
> mViewportSize has been changed to mViewport in part 1.

Sorry, my mistake, I missed this somehow.

> Like I said above, fixed in local queue.

Sorry, I'm getting a little confused.

> > In fact, do we not want to make the same change to aTransform if we take the
> > else branch of the "if (metrics && metrics->IsScrollable())"? If so, then can
> > we factor this out to happen in RenderFrameParent::BuildDisplayList where we
> > call BuildListForLayer?
> 
> There's an implied precondition here that if there is a shadow tree, it will
> always contain one root scroll layer that is the ascendant of any other scroll
> layers. I'm having a hard time of thinking how to make this assertion explicit
> (the same idea is used in TransformShadowTree).

I'm not quite sure what I was saying here. I think what you have now is fine.

Comment 259

[Timothy Nikkel (:tnikkel)]

Comment on attachment 501091 [details] [diff] [review]
Part 6: Use viewports to allow scrolling and build display lists

+nsDisplayScrollLayer::BuildLayer(nsDisplayListBuilder* aBuilder,
+    mViewportFrame->GetRect() + aBuilder->ToReferenceFrame(mViewportFrame),

GetRect is relative to the parent frame, so this should be GetRect - GetPosition.

+BuildListForLayer(Layer* aLayer,
+      nscoord auPerDevPixel = aSubdocFrame->PresContext()->AppUnitsPerDevPixel();
+      bounds = metrics->mViewport.ToAppUnits(auPerDevPixel);

From part 1:
+static void RecordFrameMetrics(nsIFrame* aForFrame,
+  PRInt32 auPerCSSPixel = nsPresContext::AppUnitsPerCSSPixel();
+  metrics.mViewport = aViewport.ToNearestPixels(auPerCSSPixel);

So mViewport in a FrameMetrics is in CSS pixels of its document (from looking at the call sites of RecordFrameMetrics). But in part 6 we convert it using the app units per dev pixel ratio of the chrome document.

Comment 260

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 501093 [details] [diff] [review]
Part 8: Content process map from view IDs to content elements

>diff --git a/gfx/layers/Layers.cpp b/gfx/layers/Layers.cpp
>--- a/gfx/layers/Layers.cpp
>+++ b/gfx/layers/Layers.cpp
>diff --git a/gfx/layers/Layers.h b/gfx/layers/Layers.h
>--- a/gfx/layers/Layers.h
>+++ b/gfx/layers/Layers.h

The above changes look vestigial.  Please remove them if so.

>diff --git a/layout/base/nsLayoutUtils.cpp b/layout/base/nsLayoutUtils.cpp
>--- a/layout/base/nsLayoutUtils.cpp
>+++ b/layout/base/nsLayoutUtils.cpp
>+static ViewID sScrollIdCounter = FrameMetrics::START_SCROLL_ID;
>+
>+typedef map<ViewID, nsWeakPtr> ContentMap;
>+static ContentMap sContentMap;

This needs to use a lazy initializer, because std::map has a nontrivial constructor and those negatively affect startup time.  Instead:

  static ContentMap* sContentMap;
  ContentMap& GetContentMap() {
    if (!sContentMap) {
      sContentMap = new ...;
    }
    return *sContentMap;
  }

IIRC there's a LayoutUtils::Shutdown() or something similar where this can be deleted.

>+ViewID
>+nsLayoutUtils::FindIDFor(nsIContent* aContent)
>+{
>+  ViewID scrollId;
>+
>+  void* scrollIdProperty = aContent->GetProperty(nsGkAtoms::RemoteId);
>+  if (scrollIdProperty) {
>+    scrollId = *static_cast<ViewID*>(scrollIdProperty);
>+  } else {
>+    scrollId = sScrollIdCounter++;
>+    aContent->SetProperty(nsGkAtoms::RemoteId, new ViewID(scrollId),

I didn't review the original patch here, but is there a reason this ViewID needs to be heap allocated instead of being set as intptr_t(scrollId)?

Looks good!  Would like to see one more rev.  v2 will need sr.

Comment 261

[Benjamin Stover (:stechz)]

> +nsDisplayScrollLayer::BuildLayer(nsDisplayListBuilder* aBuilder,
> +    mViewportFrame->GetRect() + aBuilder->ToReferenceFrame(mViewportFrame),
> 
> GetRect is relative to the parent frame, so this should be GetRect -
> GetPosition.

Done, assuming you mean to keep the ToReferenceFrame (it seems to work anyway :)).

> 
> +BuildListForLayer(Layer* aLayer,
> +      nscoord auPerDevPixel =
> aSubdocFrame->PresContext()->AppUnitsPerDevPixel();
> +      bounds = metrics->mViewport.ToAppUnits(auPerDevPixel);
> 
> From part 1:
> +static void RecordFrameMetrics(nsIFrame* aForFrame,
> +  PRInt32 auPerCSSPixel = nsPresContext::AppUnitsPerCSSPixel();
> +  metrics.mViewport = aViewport.ToNearestPixels(auPerCSSPixel);
> 
> So mViewport in a FrameMetrics is in CSS pixels of its document (from looking
> at the call sites of RecordFrameMetrics). But in part 6 we convert it using the
> app units per dev pixel ratio of the chrome document.

I'm not sure how to fix this.

(In reply to comment #260)
> Comment on attachment 501093 [details] [diff] [review]
> Part 8: Content process map from view IDs to content elements
> 
> >diff --git a/gfx/layers/Layers.cpp b/gfx/layers/Layers.cpp
> >--- a/gfx/layers/Layers.cpp
> >+++ b/gfx/layers/Layers.cpp
> >diff --git a/gfx/layers/Layers.h b/gfx/layers/Layers.h
> >--- a/gfx/layers/Layers.h
> >+++ b/gfx/layers/Layers.h
> 
> The above changes look vestigial.  Please remove them if so.

Done.

> 
> >diff --git a/layout/base/nsLayoutUtils.cpp b/layout/base/nsLayoutUtils.cpp
> >--- a/layout/base/nsLayoutUtils.cpp
> >+++ b/layout/base/nsLayoutUtils.cpp
> >+static ViewID sScrollIdCounter = FrameMetrics::START_SCROLL_ID;
> >+
> >+typedef map<ViewID, nsWeakPtr> ContentMap;
> >+static ContentMap sContentMap;
> 
> This needs to use a lazy initializer, because std::map has a nontrivial
> constructor and those negatively affect startup time.  Instead:
> 
>   static ContentMap* sContentMap;
>   ContentMap& GetContentMap() {
>     if (!sContentMap) {
>       sContentMap = new ...;
>     }
>     return *sContentMap;
>   }
> 
> IIRC there's a LayoutUtils::Shutdown() or something similar where this can be
> deleted.
> 

Nope. I added one though.

> >+ViewID
> >+nsLayoutUtils::FindIDFor(nsIContent* aContent)
> >+{
> >+  ViewID scrollId;
> >+
> >+  void* scrollIdProperty = aContent->GetProperty(nsGkAtoms::RemoteId);
> >+  if (scrollIdProperty) {
> >+    scrollId = *static_cast<ViewID*>(scrollIdProperty);
> >+  } else {
> >+    scrollId = sScrollIdCounter++;
> >+    aContent->SetProperty(nsGkAtoms::RemoteId, new ViewID(scrollId),
> 
> I didn't review the original patch here, but is there a reason this ViewID
> needs to be heap allocated instead of being set as intptr_t(scrollId)?
> 
> Looks good!  Would like to see one more rev.  v2 will need sr.

Isn't intptr_t going to sometimes be 32-bits? ViewIDs are 64-bit.

Comment 262

[Benjamin Stover (:stechz)]

Attachment: Part 5: Support displayport for iframes

Comment 263

[Benjamin Stover (:stechz)]

Attachment: Part 8: Content process map from view IDs to content elements

Comment 264

[Benjamin Stover (:stechz)]

Attachment: Part 8: Content process map from view IDs to content elements

Comment 265

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #261)
> > I didn't review the original patch here, but is there a reason this ViewID
> > needs to be heap allocated instead of being set as intptr_t(scrollId)?
> >
> Isn't intptr_t going to sometimes be 32-bits? ViewIDs are 64-bit.

Right-o.

Comment 266

[Benjamin Stover (:stechz)]

Attachment: Part 1: Tag layers with scrollable information

Comment 267

[Benjamin Stover (:stechz)]

Attachment: Part 7: Include viewport and content size in API

Comment 268

[Benjamin Stover (:stechz)]

Timothy: I changed everything to use dev pixels since this is cross process, though the change doesn't affect any code in the patch you are reviewing. I hope this makes sense.

Comment 269

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  nsIDOMElement FindElementWithViewId(in nsViewID aId);

lowercase 'find'

I'd prefer you used nsDataHashtable than <map> here.

+    nsWeakPtr ref = do_GetWeakReference(aContent);
+    NS_ABORT_IF_FALSE(ref, "Could not make weak reference!");
+    GetContentMap().insert(ContentMap::value_type(scrollId, ref));

Why do we need to use weak references here? Why not just an nsIContent*? If the object is destroyed, the property will be cleared in DestroyViewID.

Otherwise looks good.

Comment 270

[Benjamin Stover (:stechz)]

> lowercase 'find'

Done.

> I'd prefer you used nsDataHashtable than <map> here.

Done.

> Why do we need to use weak references here? Why not just an nsIContent*? If the
> object is destroyed, the property will be cleared in DestroyViewID.

No reason. Done.

Comment 271

[Benjamin Stover (:stechz)]

Attachment: Part 8: Content process map from view IDs to content elements

Comment 272

[Benjamin Stover (:stechz)]

Attachment: Mobile: Use new interfaces

Comment 273

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 502929 [details] [diff] [review]
Part 8: Content process map from view IDs to content elements

FindContentFor should just return nsIContent* surely?

Use nsnull instead of NULL in layout.

r+ with that.

Comment 274

[Benjamin Stover (:stechz)]

Attachment: Basic mobile frontend patch

Comment 275

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 503520 [details] [diff] [review]
Basic mobile frontend patch

I definitely have some stylistics issues (no surprise) but this looks good enough to me to test iframe panning.

Comment 276

[Benjamin Stover (:stechz)]

m-c push: http://hg.mozilla.org/mozilla-central/rev/b0c723496180
m-b push: http://hg.mozilla.org/mobile-browser/rev/6a4dc526029c

Leaving bug open for remaining parts of frontend patch.

Comment 277

[Benjamin Stover (:stechz)]

Just to be clear, the mobile patch just updates frontend to use the new API for the root view. Panning iframes is now available to frontend, but the frontend isn't using it.

Comment 278

[Benjamin Stover (:stechz)]

Attachment: Scroll iframes asynchronously in frontend

Comment 279

[Benjamin Stover (:stechz)]

Comment on attachment 503531 [details] [diff] [review]
Scroll iframes asynchronously in frontend

Here's the original patch with some cleanup. Mark, this gets iframe panning to work. It does change the way the size of the displayport using an algorithm that I put in bug 624451 as well.

We should file followup bugs if we land this.

Comment 280

[Benjamin Stover (:stechz)]

Followup pushes on m-c:
http://hg.mozilla.org/mozilla-central/rev/bfb895f3cc9c
http://hg.mozilla.org/mozilla-central/rev/3fd96738579e

Comment 281

[Timothy Nikkel (:tnikkel)]

The automated talos emails tagged this as causing a regression in Firefox.
http://groups.google.com/group/mozilla.dev.tree-management/browse_thread/thread/d427821d3874550a#
Given that this shouldn't have affected Firefox can you take a look to see if this is a real regression?

Comment 282

[Benjamin Stover (:stechz)]

Hm, I think an ID is recorded for root scroll frames now, as well as some scrolling information, even if we don't need it. Maybe we could avoid this.

Comment 283

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

And this was a sunspider yay!!!11??!!11!?

Comment 284

[Benjamin Stover (:stechz)]

The only thing that was really added to metrics was mContentSize and mViewportSize => mViewport. The ID isn't generated and stored for root views, and iframes only generate and store when nsDisplayScrollLayer is used (only in the content process). I can't imagine these would affect performance.

Comment 285

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #283)
> And this was a sunspider yay!!!11??!!11!?

sunspider *win*, oops.  Significant omission.

Comment 286

[Benjamin Stover (:stechz)]

Looks like noise to me.

Dromaeo (DOM) regression
http://graphs.mozilla.org/graph.html#tests=[[73,1,433],[73,1,434],[73,1,435],[73,1,436],[73,1,437],[73,1,438],[73,1,439],[73,1,440],[73,1,441],[73,1,442],[73,1,443],[73,1,444],[73,1,445],[73,1,446],[73,1,447],[73,1,448],[73,1,449],[73,1,450],[73,1,688],[73,1,689],[73,1,690],[73,1,691],[73,1,692],[73,1,693],[73,1,694],[73,1,695],[73,1,696],[73,1,697],[73,1,698],[73,1,699],[73,1,700],[73,1,701],[73,1,702],[73,1,703],[73,1,704],[73,1,705],[73,1,706],[73,1,707],[73,1,708],[73,1,709],[73,1,710],[73,1,711],[73,1,712],[73,1,713],[73,1,714],[73,1,715],[73,1,716],[73,1,717],[73,1,895],[73,1,896],[73,1,897],[73,1,898],[73,1,899]]&sel=1294857425,1295030225

Dromaeo (Sunspider) win
http://graphs.mozilla.org/graph.html#tests=[[75,1,393],[75,1,394],[75,1,395],[75,1,396],[75,1,397],[75,1,398],[75,1,399],[75,1,400],[75,1,401],[75,1,402],[75,1,403],[75,1,404],[75,1,405],[75,1,406],[75,1,407],[75,1,408],[75,1,409],[75,1,410],[75,1,523],[75,1,524],[75,1,525],[75,1,526],[75,1,527],[75,1,528],[75,1,529],[75,1,541],[75,1,542],[75,1,543],[75,1,544],[75,1,545],[75,1,546],[75,1,547],[75,1,548],[75,1,549],[75,1,550],[75,1,551],[75,1,552],[75,1,553],[75,1,668],[75,1,669],[75,1,670],[75,1,671],[75,1,672],[75,1,673],[75,1,674],[75,1,675],[75,1,676],[75,1,677],[75,1,889],[75,1,890],[75,1,891]]

Comment 287

[Timothy Nikkel (:tnikkel)]

For the sunspider win I looked back 10 pushes before this bug. They were all in the 720's or under, every result in a <= 10 unit range. I looked at every push since this bug that has results (6 of them), every one was in the 760s. That does not sounds like noise to me.

Comment 288

[Timothy Nikkel (:tnikkel)]

(graphs.m.o has never been useful tool for me every time I've tried it)

Comment 289

[Timothy Nikkel (:tnikkel)]

(In reply to comment #284)
> The only thing that was really added to metrics was mContentSize and
> mViewportSize => mViewport. The ID isn't generated and stored for root views,
> and iframes only generate and store when nsDisplayScrollLayer is used (only in
> the content process). I can't imagine these would affect performance.

The reason we have performance tests is because we can't always imagine everything that will affect perf. :)

Comment 290

[Timothy Nikkel (:tnikkel)]

The DOM regression does look to be random.

Comment 291

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 503531 [details] [diff] [review]
Scroll iframes asynchronously in frontend


>diff --git a/chrome/content/AnimatedZoom.js b/chrome/content/AnimatedZoom.js

>-    getBrowser()._contentViewManager.rootContentView.setScale(zoomLevel, zoomLevel);
>-    getBrowser()._contentViewManager.rootContentView.scrollTo(nextRect.left * zoomRatio, nextRect.top * zoomRatio);
>+    let contentView = getBrowser()._contentViewManager.rootContentView;
>+    contentView.setScale(zoomLevel, zoomLevel);
>+    contentView.scrollTo(nextRect.left * zoomRatio, nextRect.top * zoomRatio);

nit: if browser._contentViewManager is public, then no leading "_"

>diff --git a/chrome/content/bindings/browser.xml b/chrome/content/bindings/browser.xml
>--- a/chrome/content/bindings/browser.xml
>+++ b/chrome/content/bindings/browser.xml
>@@ -135,18 +135,21 @@
>                 }
>                 break;
> 
>               case "MozScrolledAreaChanged":
>                 self._contentDocumentWidth = aMessage.json.width;
>                 self._contentDocumentHeight = aMessage.json.height;
> 
>                 // Recalculate whether the visible area is actually in bounds
>-                self.scrollBy(0, 0);
>-                self._updateCacheViewport();
>+                let view = self.getRootView();
>+                if (view) {
>+                  view.scrollBy(0, 0);
>+                  view._updateCacheViewport();

browser.getRootView() doesn't seem to return null. I mean, there is a code path in _getViews() that could return null, but I don't think browser.getRootView() allows that code path. You do this check a lot but not everywhere.

nit: if _updateCacheViewport is public, then no leading "_", unless you think it should only be used from within browser XBL

>       <method name="_updateCSSViewport">
>         <body>
>           <![CDATA[
>-            let rootView = this._contentViewManager.rootContentView;
>+            let contentViewManager = this._contentViewManager;
>+            let contentView = contentViewManager.rootContentView;

nit: Why bother?

>+      <field name="_contentView"><![CDATA[

>+          scrollBy: function(x, y) {
>+            let self = this.self;
>+            let position = this.getPosition();
>+            x = Math.floor(Math.max(0, Math.min(self.contentDocumentWidth,  position.x+x))-position.x);
>+            y = Math.floor(Math.max(0, Math.min(self.contentDocumentHeight, position.y+y))-position.y);

nit: Use spaces around the  "+" and "-"

>+      <!-- Keep a temporary store of content views. -->
>+      <field name="_contentViews">({})</field>

Managing this map is a pain, but I guess it's a burden we should carry.

>+      <field name="_contentViewPrototype"><![CDATA[

>+          kDieTime: 3000,

Should be a pref?

>+          _getViewportSize: function() {
>+            let self = this.self;
>+            if (this.isRoot())
>+              return self.getBoundingClientRect();
>+            else
>+              return { width: this._contentView.viewportWidth, height: this._contentView.viewportHeight };
>+          },

Not exactly the same objects, but OK for private use.

>+      <method name="_getView">
>+        <parameter name="contentView"/>
>+        <body>
>+          <![CDATA[
>+            if (!contentView) return null;

Note: the only way to get a null view. getRootView always passes in a contentView.

r+, but I'd like to drop the getRootView checks if you agree they are redundant. And fix the nits

Comment 293

[Benjamin Stover (:stechz)]

> nit: if browser._contentViewManager is public, then no leading "_"

I was hoping for it to be considered private. There is some code in zoom that breaks this rule, but I want to fix that (I believe a comment says it's bad too).

> browser.getRootView() doesn't seem to return null. I mean, there is a code path
> in _getViews() that could return null, but I don't think browser.getRootView()
> allows that code path. You do this check a lot but not everywhere.

Fixed, sort of. There is unfortunately a time before a page has begun loading where it will not have a root frame, so it will not have a root view. I added a _contentNoop with a detailed comment for this case. We may consider a follow-up for guaranteeing a root view in the platform?

> nit: if _updateCacheViewport is public, then no leading "_", unless you think
> it should only be used from within browser XBL

It is internal, yes. I really don't want people to care about it.

> nit: Why bother?

Fixed.

> nit: Use spaces around the  "+" and "-"

Fixed.

> Managing this map is a pain, but I guess it's a burden we should carry.

Yeah :(

> 
> >+      <field name="_contentViewPrototype"><![CDATA[
> 
> >+          kDieTime: 3000,
> 
> Should be a pref?
> 

Fixed.

> >+          _getViewportSize: function() {
> >+            let self = this.self;
> >+            if (this.isRoot())
> >+              return self.getBoundingClientRect();
> >+            else
> >+              return { width: this._contentView.viewportWidth, height: this._contentView.viewportHeight };
> >+          },
> 
> Not exactly the same objects, but OK for private use.

Fixed.

Comment 294

[Benjamin Stover (:stechz)]

Pushed: http://hg.mozilla.org/mobile-browser/rev/8d776bc3c135

Comment 295

[Matt Brubeck (:mbrubeck)]

Attachment: followup

Bug 609940 added back a couple of browser.getPosition calls that weren't included in the mobile-browser patch here.

This will impact add-on authors too; should we make browser.getPosition a shorthand for browser.getRootView.getPosition?

Comment 296

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #295)
> Created attachment 506234 [details] [diff] [review]
> followup
> 
> Bug 609940 added back a couple of browser.getPosition calls that weren't
> included in the mobile-browser patch here.

Let's land this for beta 4

Comment 297

[Matt Brubeck (:mbrubeck)]

Pushed followup: http://hg.mozilla.org/mobile-browser/rev/d49b851fe2fc

Comment 298

[Eric Shepherd [:sheppy]]

I'm starting documentation for this bug. Is it just the new API in the frame loader interfaces, or is there more to it than that?

Comment 299

[Benjamin Stover (:stechz)]

There's also this:
http://mxr.mozilla.org/mozilla-central/source/dom/interfaces/base/nsIDOMWindowUtils.idl?force=1#834

Comment 300

[Eric Shepherd [:sheppy]]

I think this is now documented. Please let me know if there's anything missing here:

https://developer.mozilla.org/en/Multi-Process_Architecture/Working_with_content_views
https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIContentViewManager
https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIContentView

And added the branch stuff from the end of nsIDOMWindowUtils.idl to:

https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIDOMWindowUtils -- in particular: https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIDOMWindowUtils#findElementWithViewId%28%29