Scroll iframes in parent process

RESOLVED FIXED

Status

()

Core
IPC
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: stechz, Assigned: stechz)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

unspecified
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(fennec2.0b4+)

Details

Attachments

(12 attachments, 128 obsolete attachments)

14.36 KB, patch
stechz
: review+
stechz
: superreview+
Details | Diff | Splinter Review
28.72 KB, patch
stechz
: review+
Details | Diff | Splinter Review
15.47 KB, patch
stechz
: review+
Details | Diff | Splinter Review
15.76 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
5.71 KB, patch
cjones
: review+
Details | Diff | Splinter Review
17.53 KB, patch
stechz
: review+
stechz
: superreview+
Details | Diff | Splinter Review
9.68 KB, patch
stechz
: review+
stechz
: superreview+
Details | Diff | Splinter Review
8.05 KB, patch
stechz
: review+
stechz
: superreview+
Details | Diff | Splinter Review
10.82 KB, patch
stechz
: review+
Details | Diff | Splinter Review
12.49 KB, patch
mfinkle
: review+
Details | Diff | Splinter Review
31.95 KB, patch
mfinkle
: review+
Details | Diff | Splinter Review
1.27 KB, patch
mfinkle
: review+
Details | Diff | Splinter Review
(Assignee)

Description

7 years ago
Given a point on the browser, find a unique ID that corresponds to an iframe in the child process.
(Assignee)

Comment 1

7 years ago
Created attachment 484489 [details] [diff] [review]
Part 1: nsIContent can generate unique IDs
(Assignee)

Comment 2

7 years ago
Created attachment 484490 [details] [diff] [review]
Part 2: Tag layers with scrollable information
(Assignee)

Comment 3

7 years ago
Created attachment 484491 [details] [diff] [review]
Part 3: Build shadow display list
(Assignee)

Comment 4

7 years ago
Created attachment 484492 [details] [diff] [review]
Part 4: Hit test API for frontend
(Assignee)

Comment 5

7 years ago
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.
(Assignee)

Updated

7 years ago
Attachment #484489 - Flags: feedback?(roc)
(Assignee)

Updated

7 years ago
Attachment #484490 - Flags: feedback?(jones.chris.g)
(Assignee)

Updated

7 years ago
Attachment #484491 - Flags: feedback?(jones.chris.g)
(Assignee)

Updated

7 years ago
Attachment #484492 - Flags: feedback?(jones.chris.g)
(Assignee)

Updated

7 years ago
Attachment #484490 - Flags: feedback?(roc)
(Assignee)

Updated

7 years ago
Blocks: 586288
Comment on attachment 484490 [details] [diff] [review]
Part 2: Tag layers with scrollable information

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

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

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

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

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

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

>+  nsRect mRegion;

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

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

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

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

Can't you just add an ID to IFRAME elements?
Attachment #484489 - Flags: feedback?(roc) → feedback-
(Assignee)

Comment 10

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

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

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

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

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

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

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

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

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

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

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

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

Approach looks OK.

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

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

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

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

Nice.

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

Don't understand this change.

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

Don't understand what this comment refers to.

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

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

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

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

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

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

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

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

I'm not a fan of this API.  I think it's going to be difficult to maintain and hard to interpret.  Let's try to hide more implementation details under a richer API that's easier for the frontend to use.
Attachment #484492 - Flags: feedback?(jones.chris.g) → feedback-
(Assignee)

Comment 17

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

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

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

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

Use CSS selectors?  *waves hands*

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

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

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

Sure.
(Assignee)

Updated

7 years ago
Attachment #484489 - Attachment is obsolete: true
(Assignee)

Comment 19

7 years ago
Created attachment 488234 [details] [diff] [review]
Part 1: nsIContent can generate unique IDs
(Assignee)

Updated

7 years ago
Attachment #484490 - Attachment is obsolete: true
Attachment #484490 - Flags: feedback?(roc)
(Assignee)

Comment 20

7 years ago
Created attachment 488235 [details] [diff] [review]
Part 2: Tag layers with scrollable information
(Assignee)

Updated

7 years ago
Attachment #484491 - Attachment is obsolete: true
(Assignee)

Comment 21

7 years ago
Created attachment 488236 [details] [diff] [review]
Part 3: Build shadow display list
(Assignee)

Updated

7 years ago
Attachment #484492 - Attachment is obsolete: true
(Assignee)

Comment 22

7 years ago
Created attachment 488238 [details] [diff] [review]
Part 4: Viewport API for frontend
(Assignee)

Comment 23

7 years ago
Created attachment 488239 [details] [diff] [review]
Part 5: Hashtable for storing viewports
(Assignee)

Comment 24

7 years ago
Created attachment 488240 [details] [diff] [review]
Part 5: Use viewports to allow scrolling iframes in parent process
(Assignee)

Comment 25

7 years ago
Created attachment 488241 [details] [diff] [review]
Part 6: Support displayport for iframes
(Assignee)

Updated

7 years ago
Summary: Implement API for finding content iframes in parent process → Scroll iframes in parent process
(Assignee)

Comment 26

7 years ago
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.
(Assignee)

Updated

7 years ago
Attachment #488235 - Attachment is obsolete: true
(Assignee)

Comment 27

7 years ago
Created attachment 488967 [details] [diff] [review]
Part 1: Tag layers with scrollable information
(Assignee)

Updated

7 years ago
Attachment #488236 - Attachment is obsolete: true
(Assignee)

Comment 28

7 years ago
Created attachment 488968 [details] [diff] [review]
Part 2: Build shadow display list
(Assignee)

Updated

7 years ago
Attachment #488238 - Attachment is obsolete: true
(Assignee)

Comment 29

7 years ago
Created attachment 488969 [details] [diff] [review]
Part 3: Viewport API for frontend
(Assignee)

Updated

7 years ago
Attachment #488239 - Attachment is obsolete: true
(Assignee)

Comment 30

7 years ago
Created attachment 488970 [details] [diff] [review]
Part 4: Table for storing viewports
(Assignee)

Updated

7 years ago
Attachment #488241 - Attachment is obsolete: true
(Assignee)

Comment 31

7 years ago
Created attachment 488971 [details] [diff] [review]
Part 5: Support displayport for iframes
(Assignee)

Updated

7 years ago
Attachment #488240 - Attachment is obsolete: true
(Assignee)

Comment 32

7 years ago
Created attachment 488972 [details] [diff] [review]
Part 6: Use viewports to allow scrolling iframes in parent process
(Assignee)

Updated

7 years ago
Attachment #488234 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Attachment #488967 - Attachment is obsolete: true
(Assignee)

Comment 33

7 years ago
Created attachment 489021 [details] [diff] [review]
Part 1: Tag layers with scrollable information
(Assignee)

Updated

7 years ago
Attachment #488968 - Attachment is obsolete: true
(Assignee)

Comment 34

7 years ago
Created attachment 489022 [details] [diff] [review]
Part 2: Infrastructure for building shadow display list
(Assignee)

Updated

7 years ago
Attachment #488970 - Attachment is obsolete: true
(Assignee)

Comment 35

7 years ago
Created attachment 489023 [details] [diff] [review]
Part 4: Table for storing viewports
(Assignee)

Updated

7 years ago
Attachment #488971 - Attachment is obsolete: true
(Assignee)

Comment 36

7 years ago
Created attachment 489024 [details] [diff] [review]
Part 5: Support displayport for iframes
(Assignee)

Updated

7 years ago
Attachment #488972 - Attachment is obsolete: true
(Assignee)

Comment 37

7 years ago
Created attachment 489026 [details] [diff] [review]
Part 6: Use viewports to allow scrolling and build display lists
(Assignee)

Updated

7 years ago
Attachment #488969 - Attachment is obsolete: true
(Assignee)

Comment 38

7 years ago
Created attachment 489028 [details] [diff] [review]
Part 3: Viewport API for frontend
(Assignee)

Comment 39

7 years ago
OK, all comments hopefully addressed, so we're ready to start getting reviews for these patches! I'm not sure who to ask for everything, but here's a quick summary.

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

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

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

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

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

Does nsIPresShell::GetRootScrollFrame do what you need here with less fuss?
(Assignee)

Updated

7 years ago
Attachment #489021 - Flags: review?(jones.chris.g)
(Assignee)

Updated

7 years ago
Attachment #489022 - Flags: review?(tnikkel)
(Assignee)

Updated

7 years ago
Attachment #489023 - Flags: review?(roc)
Attachment #489023 - Flags: review?(jones.chris.g)
(Assignee)

Updated

7 years ago
Attachment #489023 - Flags: review?(roc)
(Assignee)

Updated

7 years ago
Attachment #489028 - Flags: review?(roc)
Attachment #489028 - Flags: review?(jones.chris.g)
(Assignee)

Updated

7 years ago
Attachment #489026 - Flags: review?(jones.chris.g)
(Assignee)

Updated

7 years ago
Attachment #489024 - Flags: review?(tnikkel)
Comment on attachment 489021 [details] [diff] [review]
Part 1: Tag layers with scrollable information

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

s.AppendInt(n);

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

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

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

Wait what?  Vestige?

>+static void AddFrameMetricsForViewportFrame(nsIFrame* aForFrame,

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

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

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

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

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

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

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

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

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

Same here.

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

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

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

I'm not qualified to review these changes.

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

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

Add a comment explaining what this class is for

+    nsTArray<PRUint64>* mShadows;

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

nsIRemoteScroll or something like that, maybe?
(Assignee)

Comment 46

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

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

nsIContentPortal?
Assignee: nobody → ben
Status: NEW → ASSIGNED
tracking-fennec: --- → ?
tracking-fennec: ? → 2.0b3+
Created attachment 492071 [details] [diff] [review]
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?
(Assignee)

Comment 50

7 years ago
So I've been working on a frontend patch for this and I found that I needed a few more things (specifically, the size of the scrolled content area for each scrollable and a way to set a display port given an ID).

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

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

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

I think it's too risky to take the day before code freeze as well
(Assignee)

Comment 52

7 years ago
> >--- 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?
(Assignee)

Updated

7 years ago
Attachment #489021 - Attachment is obsolete: true
(Assignee)

Comment 53

7 years ago
Created attachment 492122 [details] [diff] [review]
Part 1: Tag layers with scrollable information
(Assignee)

Updated

7 years ago
Attachment #489022 - Attachment is obsolete: true
Attachment #489022 - Flags: superreview?(roc)
Attachment #489022 - Flags: review?(tnikkel)
(Assignee)

Comment 54

7 years ago
Created attachment 492123 [details] [diff] [review]
Part 2: Infrastructure for building shadow display list
(Assignee)

Updated

7 years ago
Attachment #489028 - Attachment is obsolete: true
Attachment #489028 - Flags: review?(roc)
(Assignee)

Comment 55

7 years ago
Created attachment 492124 [details] [diff] [review]
Part 3: Viewport API for frontend
(Assignee)

Updated

7 years ago
Attachment #489023 - Attachment is obsolete: true
Attachment #489023 - Flags: review?(jones.chris.g)
(Assignee)

Comment 56

7 years ago
Created attachment 492125 [details] [diff] [review]
Part 4: Table for storing viewports
(Assignee)

Updated

7 years ago
Attachment #489024 - Attachment is obsolete: true
Attachment #489024 - Flags: superreview?(roc)
Attachment #489024 - Flags: review?(tnikkel)
(Assignee)

Comment 57

7 years ago
Created attachment 492126 [details] [diff] [review]
Part 5: Support displayport for iframes
(Assignee)

Updated

7 years ago
Attachment #489026 - Attachment is obsolete: true
Attachment #489026 - Flags: review?(jones.chris.g)
(Assignee)

Comment 58

7 years ago
Created attachment 492127 [details] [diff] [review]
Part 6: Use viewports to allow scrolling and build display lists
(Assignee)

Comment 59

7 years ago
Created attachment 492128 [details] [diff] [review]
Part 7: Include viewport and content size in API
(Assignee)

Comment 60

7 years ago
Created attachment 492129 [details] [diff] [review]
Part 8: Implement a scrollable interface on the content side
(Assignee)

Comment 61

7 years ago
Created attachment 492130 [details] [diff] [review]
Mobile: Use new interfaces
(Assignee)

Comment 62

7 years ago
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.
(Assignee)

Updated

7 years ago
Attachment #492122 - Flags: review?(jones.chris.g)
(Assignee)

Updated

7 years ago
Attachment #492126 - Flags: review?(tnikkel)
(Assignee)

Updated

7 years ago
Attachment #492123 - Flags: review?(tnikkel)
(Assignee)

Updated

7 years ago
Attachment #492124 - Flags: review?(jones.chris.g)
(Assignee)

Updated

7 years ago
Attachment #492125 - Flags: review?(jones.chris.g)
Great! gmail scrolling almost works..
I found 3 problems here:
1) after scrolling iframe it's content not updated immediately, I  found that I need to take rootScrollable and call scrollBy(0, -1) scrollBy(0, 1) in order to update child scrollable content. I think it can be solved by calling rootViewport.update(), after child.scrollBy.

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

3) this.contentWindow - is always null, and debug build complaining about browser.xml:525  contentWindow is null et.c.
Created attachment 492166 [details] [diff] [review]
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)
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.
Created attachment 492200 [details] [diff] [review]
Mobile: Use new interfaces part2. r2. Fixed scroll/zoom and id compare
Attachment #492166 - Attachment is obsolete: true
Also what about scrollable DIV's are we going to create scrollable frame for them?
Some pages like www.orkut.com are working like one big scrollable DIV
Attachment #492126 - Flags: superreview?(roc)
Attachment #492123 - Flags: superreview?(roc)
(In reply to comment #52)
> > >--- a/content/base/src/nsFrameLoader.cpp
> > >+++ b/content/base/src/nsFrameLoader.cpp
> > >+NS_IMETHODIMP
> > >+nsFrameLoader::GetScrollable(float aXpx, float aYpx, nsIViewport** aScrollable)
> > >+{
> > >+  nsViewport* viewport = GetScrollable();
> > >+  CallQueryInterface(viewport, aScrollable);
> > 
> > Should just need |*aScrollable = GetScrollable()|.
> 
> No, that won't work. XPCOM out params like this need to addref, and
> GetScrollable does not do this.
> 

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

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

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

We don't support generic 2d transforms, just scales and offsets.  The original config struct or a new one with just those is fine with me.
(Assignee)

Updated

7 years ago
Attachment #492126 - Attachment is obsolete: true
Attachment #492126 - Flags: superreview?(roc)
Attachment #492126 - Flags: review?(tnikkel)
(Assignee)

Comment 69

7 years ago
Created attachment 492418 [details] [diff] [review]
Part 5: Support displayport for iframes
(Assignee)

Updated

7 years ago
Attachment #492129 - Attachment is obsolete: true
(Assignee)

Comment 70

7 years ago
Created attachment 492419 [details] [diff] [review]
Part 8: Implement a scrollable interface on the content side
(Assignee)

Updated

7 years ago
Attachment #492130 - Attachment is obsolete: true
(Assignee)

Comment 71

7 years ago
Created attachment 492420 [details] [diff] [review]
Mobile: Use new interfaces
(Assignee)

Comment 72

7 years ago
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).
(Assignee)

Updated

7 years ago
Attachment #492419 - Attachment is obsolete: true
(Assignee)

Comment 73

7 years ago
Created attachment 492493 [details] [diff] [review]
Part 8: Implement a scrollable interface on the content side
(Assignee)

Updated

7 years ago
Attachment #492420 - Attachment is obsolete: true
(Assignee)

Comment 74

7 years ago
Created attachment 492494 [details] [diff] [review]
Mobile: Use new interfaces
(Assignee)

Comment 75

7 years ago
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

Updated

7 years ago
tracking-fennec: 2.0b3+ → 2.0b4+
(Assignee)

Updated

7 years ago
Attachment #492122 - Attachment is obsolete: true
Attachment #492122 - Flags: review?(jones.chris.g)
(Assignee)

Comment 76

7 years ago
Created attachment 492807 [details] [diff] [review]
Part 1: Tag layers with scrollable information
(Assignee)

Updated

7 years ago
Attachment #492123 - Attachment is obsolete: true
Attachment #492123 - Flags: superreview?(roc)
Attachment #492123 - Flags: review?(tnikkel)
(Assignee)

Comment 77

7 years ago
Created attachment 492808 [details] [diff] [review]
Part 2: Infrastructure for building shadow display list
(Assignee)

Updated

7 years ago
Attachment #492124 - Attachment is obsolete: true
Attachment #492124 - Flags: review?(jones.chris.g)
(Assignee)

Comment 78

7 years ago
Created attachment 492809 [details] [diff] [review]
Part 3: Viewport API for frontend
(Assignee)

Updated

7 years ago
Attachment #492125 - Attachment is obsolete: true
Attachment #492125 - Flags: review?(jones.chris.g)
(Assignee)

Comment 79

7 years ago
Created attachment 492811 [details] [diff] [review]
Part 4: Table for storing viewports
(Assignee)

Updated

7 years ago
Attachment #492418 - Attachment is obsolete: true
(Assignee)

Comment 80

7 years ago
Created attachment 492812 [details] [diff] [review]
Part 5: Support displayport for iframes
(Assignee)

Updated

7 years ago
Attachment #492127 - Attachment is obsolete: true
(Assignee)

Comment 81

7 years ago
Created attachment 492813 [details] [diff] [review]
Part 6: Use viewports to allow scrolling and build display lists
(Assignee)

Updated

7 years ago
Attachment #492128 - Attachment is obsolete: true
(Assignee)

Comment 82

7 years ago
Created attachment 492815 [details] [diff] [review]
Part 7: Include viewport and content size in API
(Assignee)

Updated

7 years ago
Attachment #492493 - Attachment is obsolete: true
(Assignee)

Comment 83

7 years ago
Created attachment 492817 [details] [diff] [review]
Part 8: Implement a scrollable interface on the content side
(Assignee)

Updated

7 years ago
Attachment #492494 - Attachment is obsolete: true
(Assignee)

Comment 84

7 years ago
Created attachment 492818 [details] [diff] [review]
Mobile: Use new interfaces
(Assignee)

Updated

7 years ago
Attachment #492807 - Flags: review?(jones.chris.g)
(Assignee)

Updated

7 years ago
Attachment #492808 - Flags: review?(tnikkel)
(Assignee)

Updated

7 years ago
Attachment #492809 - Flags: review?(jones.chris.g)
(Assignee)

Updated

7 years ago
Attachment #492812 - Flags: review?(tnikkel)
(Assignee)

Updated

7 years ago
Attachment #492811 - Flags: review?(jones.chris.g)
(Assignee)

Comment 85

7 years ago
Changes:
* frontend fix for a bug where we would scroll to the wrong place when displayport is updated
* unbitrot
* rename nsIScrollable to nsIContentView (and other renamings with that)
* use new interfaces instead of nsIDOMWindowUtils and nsIFrameLoader
* code review in #68
Attachment #492200 - Attachment is obsolete: true
Attachment #492071 - Attachment is obsolete: true
Comment on attachment 492807 [details] [diff] [review]
Part 1: Tag layers with scrollable information

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

Instead of directly PRUint64, add a

  typedef PRUint64 scrollid_t;

here and then use scrollid_t.

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

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

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

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

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

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

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

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

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

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

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

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

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

Update() is private.

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

More ugmo ;).  Use ViewConfig.

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

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

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

Don't understand this change.

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

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

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

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

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

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

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

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

Use NULL_SCROLL_ID.

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

Make this ABORT_IF_FALSE.

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

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

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

ABORT_IF_FALSE here too.

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

Yuck.  Just add an inline getter for this.

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

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

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

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

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

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

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

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

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

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

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

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

>+  void ContentChanged(nsIContent* aContent);

Nit: call this OwnerContentChanged().

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

Nit: s/Rebuild/Build/.

>+  ViewportArray mScrollables;

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

This is looking pretty good.
Attachment #492811 - Flags: review?(jones.chris.g)
Attachment #492808 - Flags: superreview?(roc)
Attachment #492812 - Flags: superreview?(roc)
(Assignee)

Updated

7 years ago
Blocks: 615368
(Assignee)

Comment 90

7 years ago
> >-    nsCOMPtr<nsFrameLoader> frameloader = loaderOwner->GetFrameLoader();
> >+    nsRefPtr<nsFrameLoader> frameloader = loaderOwner->GetFrameLoader();
> 
> Don't understand this change.

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

>   if (NS_SUCCEEDED(rv)) {
>+    nsIScrollableFrame* scrollFrame = presShell->GetRootScrollFrameAsScrollable();
>+    nsRect scrollRange = scrollFrame->GetScrollRange();
this might crash, for example on about:about page
(Assignee)

Comment 93

7 years ago
(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.
(Assignee)

Updated

7 years ago
Attachment #492807 - Attachment is obsolete: true
(Assignee)

Comment 94

7 years ago
Created attachment 494121 [details] [diff] [review]
Part 1: Tag layers with scrollable information
(Assignee)

Updated

7 years ago
Attachment #492808 - Attachment is obsolete: true
Attachment #492808 - Flags: superreview?(roc)
Attachment #492808 - Flags: review?(tnikkel)
(Assignee)

Comment 95

7 years ago
Created attachment 494122 [details] [diff] [review]
Part 2: Infrastructure for building shadow display list
(Assignee)

Updated

7 years ago
Attachment #492809 - Attachment is obsolete: true
(Assignee)

Comment 96

7 years ago
Created attachment 494123 [details] [diff] [review]
Part 3: Viewport API for frontend
(Assignee)

Updated

7 years ago
Attachment #492811 - Attachment is obsolete: true
(Assignee)

Comment 97

7 years ago
Created attachment 494124 [details] [diff] [review]
Part 4: Map for storing viewports
(Assignee)

Updated

7 years ago
Attachment #492812 - Attachment is obsolete: true
Attachment #492812 - Flags: superreview?(roc)
Attachment #492812 - Flags: review?(tnikkel)
(Assignee)

Comment 98

7 years ago
Created attachment 494125 [details] [diff] [review]
Part 5: Support displayport for iframes
(Assignee)

Updated

7 years ago
Attachment #492813 - Attachment is obsolete: true
(Assignee)

Comment 99

7 years ago
Created attachment 494126 [details] [diff] [review]
Part 6: Use viewports to allow scrolling and build display lists
(Assignee)

Updated

7 years ago
Attachment #492815 - Attachment is obsolete: true
(Assignee)

Comment 100

7 years ago
Created attachment 494127 [details] [diff] [review]
Part 7: Include viewport and content size in API
(Assignee)

Updated

7 years ago
Attachment #492817 - Attachment is obsolete: true
(Assignee)

Comment 101

7 years ago
Created attachment 494128 [details] [diff] [review]
Part 8: Implement a scrollable interface on the content side
(Assignee)

Updated

7 years ago
Attachment #492818 - Attachment is obsolete: true
(Assignee)

Comment 102

7 years ago
Created attachment 494129 [details] [diff] [review]
Mobile: Use new interfaces
(Assignee)

Updated

7 years ago
Attachment #494121 - Flags: review+
(Assignee)

Updated

7 years ago
Attachment #494122 - Flags: superreview?(roc)
Attachment #494122 - Flags: review?(tnikkel)
(Assignee)

Updated

7 years ago
Attachment #494123 - Flags: review?(jones.chris.g)
(Assignee)

Updated

7 years ago
Attachment #494124 - Flags: review?(jones.chris.g)
(Assignee)

Updated

7 years ago
Attachment #494125 - Flags: superreview?(roc)
Attachment #494125 - Flags: review?(tnikkel)
(Assignee)

Updated

7 years ago
Attachment #494126 - Flags: review?(jones.chris.g)
(Assignee)

Comment 103

7 years ago
> 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.
(Assignee)

Updated

7 years ago
Attachment #494124 - Attachment is obsolete: true
Attachment #494124 - Flags: review?(jones.chris.g)
(Assignee)

Comment 104

7 years ago
Created attachment 494177 [details] [diff] [review]
Part 4: Map for storing views
(Assignee)

Updated

7 years ago
Attachment #494125 - Attachment is obsolete: true
Attachment #494125 - Flags: superreview?(roc)
Attachment #494125 - Flags: review?(tnikkel)
(Assignee)

Comment 105

7 years ago
Created attachment 494179 [details] [diff] [review]
Part 5: Support displayport for iframes
(Assignee)

Updated

7 years ago
Attachment #494126 - Attachment is obsolete: true
Attachment #494126 - Flags: review?(jones.chris.g)
(Assignee)

Comment 106

7 years ago
Created attachment 494180 [details] [diff] [review]
Part 6: Use viewports to allow scrolling and build display lists
(Assignee)

Updated

7 years ago
Attachment #494127 - Attachment is obsolete: true
(Assignee)

Comment 107

7 years ago
Created attachment 494182 [details] [diff] [review]
Part 7: Include viewport and content size in API
(Assignee)

Updated

7 years ago
Attachment #494128 - Attachment is obsolete: true
(Assignee)

Comment 108

7 years ago
Created attachment 494183 [details] [diff] [review]
Part 8: Implement a scrollable interface on the content side
(Assignee)

Updated

7 years ago
Attachment #494177 - Flags: review?(jones.chris.g)
(Assignee)

Updated

7 years ago
Attachment #494179 - Flags: superreview?(roc)
Attachment #494179 - Flags: review?(tnikkel)
(Assignee)

Updated

7 years ago
Attachment #494180 - Flags: review?(jones.chris.g)
(In reply to comment #103)
> > You can remove the need for this function by storing a map from ID ->
> > < view, layer >, then replacing this with a lookup of the layer.  Note
> > however that the memory management is a bit subtle; you'll need to
> > keep a weak ref to the layer, and document the fact that
> > BuildViewBlah() *must* be called on ShadowLayersUpdated(), and that in
> > BuildViewBlah(), the entries in the old map can be touched IFF a layer
> > with their scroll ID exists in the new layer tree.  Other entries in
> > the old map might have pointers to dead layers.
> 
> Let's not pre-optimize this, given the subtle memory management issues you
> mentioned and how fast diving through layers is.
> 

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

Comment 110

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

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

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

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

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

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

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

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

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

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

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

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

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

I don't understand why this change is needed.

+class nsDisplayScrollLayer : public nsDisplayOwnLayer
+  nsRect mClip;

Why do you need this rect?

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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


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

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

>-            if (Math.abs(this._pendingPixelsX) > Math.max(0, this._pendingThresholdX - bcr.width / 2) ||
>-                Math.abs(this._pendingPixelsY) > Math.max(0, this._pendingThresholdY - bcr.height / 2))
>-              this._updateCacheViewport();
>-          ]]>
(Assignee)

Comment 116

7 years ago
> 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?
(Assignee)

Comment 117

7 years ago
> 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!
(Assignee)

Comment 118

7 years ago
> @@ -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.
(Assignee)

Comment 119

7 years ago
> 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.
(Assignee)

Comment 120

7 years ago
(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.
(Assignee)

Updated

7 years ago
Attachment #494121 - Attachment is obsolete: true
(Assignee)

Comment 121

7 years ago
Created attachment 494541 [details] [diff] [review]
Part 1: Tag layers with scrollable information
(Assignee)

Updated

7 years ago
Attachment #494122 - Attachment is obsolete: true
Attachment #494122 - Flags: superreview?(roc)
(Assignee)

Comment 122

7 years ago
Created attachment 494542 [details] [diff] [review]
Part 2: Infrastructure for building shadow display list
(Assignee)

Updated

7 years ago
Attachment #494123 - Attachment is obsolete: true
Attachment #494123 - Flags: review?(jones.chris.g)
(Assignee)

Comment 123

7 years ago
Created attachment 494543 [details] [diff] [review]
Part 3: Viewport API for frontend
(Assignee)

Updated

7 years ago
Attachment #494177 - Attachment is obsolete: true
Attachment #494177 - Flags: review?(jones.chris.g)
(Assignee)

Comment 124

7 years ago
Created attachment 494544 [details] [diff] [review]
Part 4: Map for storing views
(Assignee)

Updated

7 years ago
Attachment #494179 - Attachment is obsolete: true
Attachment #494179 - Flags: superreview?(roc)
Attachment #494179 - Flags: review?(tnikkel)
(Assignee)

Comment 125

7 years ago
Created attachment 494545 [details] [diff] [review]
Part 5: Support displayport for iframes
(Assignee)

Updated

7 years ago
Attachment #494180 - Attachment is obsolete: true
Attachment #494180 - Flags: review?(jones.chris.g)
(Assignee)

Comment 126

7 years ago
Created attachment 494546 [details] [diff] [review]
Part 6: Use viewports to allow scrolling and build display lists
(Assignee)

Updated

7 years ago
Attachment #494182 - Attachment is obsolete: true
(Assignee)

Comment 127

7 years ago
Created attachment 494547 [details] [diff] [review]
Part 7: Include viewport and content size in API
(Assignee)

Updated

7 years ago
Attachment #494183 - Attachment is obsolete: true
(Assignee)

Comment 128

7 years ago
Created attachment 494549 [details] [diff] [review]
Part 8: Implement a scrollable interface on the content side
(Assignee)

Updated

7 years ago
Attachment #494541 - Flags: review+
(Assignee)

Updated

7 years ago
Attachment #494542 - Flags: review+
(Assignee)

Updated

7 years ago
Attachment #494542 - Flags: superreview?(roc)
Attachment #494542 - Flags: review+
(Assignee)

Updated

7 years ago
Attachment #494543 - Flags: review?(jones.chris.g)
(Assignee)

Updated

7 years ago
Attachment #494544 - Flags: review?(jones.chris.g)
(Assignee)

Updated

7 years ago
Attachment #494545 - Flags: superreview?(roc)
Attachment #494545 - Flags: review?(tnikkel)
(Assignee)

Updated

7 years ago
Attachment #494546 - Flags: review?(jones.chris.g)
(Assignee)

Updated

7 years ago
Attachment #494542 - Flags: review+
(In reply to comment #116)
> (In reply to comment #112)
> > Comment on attachment 494121 [details] [diff] [review] [details]
> > Part 1: Tag layers with scrollable information
> > 
> > >@@ -429,44 +461,24 @@ void nsDisplayList::PaintForFrame(nsDisp
> > >+  RecordFrameMetrics(aForFrame, root, mVisibleRect,
> > >+                     FrameMetrics::SCROLL_ROOT_ID);
> > 
> > nsDisplayList::PaintForFrame can be called for more than just the root frame,
> > so I think you need to check some conditions before doing this.
> 
> This was just a refactoring though... This code was unconditionally being
> called before. Is this a bug?

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

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

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

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

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

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

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

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

What I'm suggesting is something like

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

I think that does the correct thing.
(Assignee)

Comment 132

7 years ago
(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.
(Assignee)

Comment 133

7 years ago
> 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.
(Assignee)

Updated

7 years ago
Attachment #494547 - Flags: review?(jones.chris.g)
(Assignee)

Updated

7 years ago
Attachment #494549 - Flags: review?(jones.chris.g)
I know of bug 593243.  It's not terribly hard to fix, tn and I discussed it.
Comment on attachment 494541 [details] [diff] [review]
Part 1: Tag layers with scrollable information

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

Also, we need documentation here.

This patch needed sr.
Attachment #494541 - Flags: superreview-
Oh, my fault on missed sr.  I didn't know we had naming conventions for numerical types.
(Assignee)

Comment 137

7 years ago
FYI, I have noticed that it is possible for dangling pointers to occur. I never set mOwnerContent to NULL for those content views that did not make it into the new hashtable.

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

I don't know why numerical types would be special.
(Assignee)

Updated

7 years ago
Attachment #494543 - Attachment is obsolete: true
Attachment #494543 - Flags: review?(jones.chris.g)
(Assignee)

Comment 139

7 years ago
Created attachment 494877 [details] [diff] [review]
Part 3: Viewport API for frontend
(Assignee)

Updated

7 years ago
Attachment #494544 - Attachment is obsolete: true
Attachment #494544 - Flags: review?(jones.chris.g)
(Assignee)

Comment 140

7 years ago
Created attachment 494878 [details] [diff] [review]
Part 4: Map for storing views
(Assignee)

Updated

7 years ago
Attachment #494545 - Attachment is obsolete: true
Attachment #494545 - Flags: superreview?(roc)
Attachment #494545 - Flags: review?(tnikkel)
(Assignee)

Comment 141

7 years ago
Created attachment 494879 [details] [diff] [review]
Part 5: Support displayport for iframes
(Assignee)

Updated

7 years ago
Attachment #494547 - Attachment is obsolete: true
Attachment #494547 - Flags: review?(jones.chris.g)
(Assignee)

Comment 142

7 years ago
Created attachment 494880 [details] [diff] [review]
Part 7: Include viewport and content size in API
(Assignee)

Updated

7 years ago
Attachment #494129 - Attachment is obsolete: true
(Assignee)

Comment 143

7 years ago
Created attachment 494881 [details] [diff] [review]
Mobile: Use new interfaces
(Assignee)

Updated

7 years ago
Attachment #494877 - Flags: review?(jones.chris.g)
(Assignee)

Updated

7 years ago
Attachment #494878 - Flags: review?(jones.chris.g)
(Assignee)

Updated

7 years ago
Attachment #494879 - Flags: superreview?(roc)
Attachment #494879 - Flags: review?(tnikkel)
(Assignee)

Updated

7 years ago
Attachment #494880 - Flags: review?(jones.chris.g)
Comment on attachment 494542 [details] [diff] [review]
Part 2: Infrastructure for building shadow display list

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

{} around the body

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

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

const nsRect&

nsDisplayScrollLayer needs comments explaining what it does.

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

presShell->GetDisplayPort() + ToReferenceFrame()

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

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

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

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

Can you explain the problem here in more detail?

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

What's an example?

+    }
+    else {

} else {
BTW I think this is going in the right direction overall.
(Assignee)

Updated

7 years ago
Attachment #494541 - Attachment is obsolete: true
(Assignee)

Comment 147

7 years ago
Created attachment 495071 [details] [diff] [review]
Part 1: Tag layers with scrollable information
(Assignee)

Updated

7 years ago
Attachment #495071 - Attachment is obsolete: true
(Assignee)

Comment 148

7 years ago
Created attachment 495104 [details] [diff] [review]
Part 1: Tag layers with scrollable information
(Assignee)

Updated

7 years ago
Attachment #494542 - Attachment is obsolete: true
Attachment #494542 - Flags: superreview?(roc)
(Assignee)

Comment 149

7 years ago
Created attachment 495105 [details] [diff] [review]
Part 2: Infrastructure for building shadow display list
(Assignee)

Updated

7 years ago
Attachment #494877 - Attachment is obsolete: true
Attachment #494877 - Flags: review?(jones.chris.g)
(Assignee)

Comment 150

7 years ago
Created attachment 495106 [details] [diff] [review]
Part 3: Viewport API for frontend
(Assignee)

Updated

7 years ago
Attachment #494878 - Attachment is obsolete: true
Attachment #494878 - Flags: review?(jones.chris.g)
(Assignee)

Comment 151

7 years ago
Created attachment 495110 [details] [diff] [review]
Part 4: Map for storing views
(Assignee)

Updated

7 years ago
Attachment #494879 - Attachment is obsolete: true
Attachment #494879 - Flags: superreview?(roc)
Attachment #494879 - Flags: review?(tnikkel)
(Assignee)

Comment 152

7 years ago
Created attachment 495112 [details] [diff] [review]
Part 5: Support displayport for iframes
(Assignee)

Updated

7 years ago
Attachment #494546 - Attachment is obsolete: true
Attachment #494546 - Flags: review?(jones.chris.g)
(Assignee)

Comment 153

7 years ago
Created attachment 495113 [details] [diff] [review]
Part 6: Use viewports to allow scrolling and build display lists
(Assignee)

Updated

7 years ago
Attachment #494880 - Attachment is obsolete: true
Attachment #494880 - Flags: review?(jones.chris.g)
(Assignee)

Comment 154

7 years ago
Created attachment 495114 [details] [diff] [review]
Part 7: Include viewport and content size in API
(Assignee)

Updated

7 years ago
Attachment #494549 - Attachment is obsolete: true
Attachment #494549 - Flags: review?(jones.chris.g)
(Assignee)

Comment 155

7 years ago
Created attachment 495115 [details] [diff] [review]
Part 8: Implement a scrollable interface on the content side
(Assignee)

Updated

7 years ago
Attachment #495104 - Flags: superreview?(roc)
Attachment #495104 - Flags: review+
(Assignee)

Updated

7 years ago
Attachment #495105 - Flags: superreview?(roc)
Attachment #495105 - Flags: review+
(Assignee)

Updated

7 years ago
Attachment #495106 - Flags: review?(jones.chris.g)
(Assignee)

Updated

7 years ago
Attachment #495110 - Flags: review?(jones.chris.g)
(Assignee)

Updated

7 years ago
Attachment #495112 - Flags: superreview?(roc)
Attachment #495112 - Flags: review?(tnikkel)
(Assignee)

Updated

7 years ago
Attachment #495113 - Flags: review?(jones.chris.g)
(Assignee)

Updated

7 years ago
Attachment #495114 - Flags: review?(jones.chris.g)
(Assignee)

Updated

7 years ago
Attachment #495115 - Flags: review?(jones.chris.g)
Comment on attachment 495104 [details] [diff] [review]
Part 1: Tag layers with scrollable information

Please add comments to document the ViewID stuff.

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

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

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

You haven't addressed this.

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

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

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

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

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

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

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

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

You haven't addressed this.

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

Or this.

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

Or this.
(Assignee)

Comment 159

7 years ago
(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.
(Assignee)

Comment 160

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

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

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

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

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

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

Comment 162

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

I dig it. So, why are you defining a MAX ID?
To make sure the compiler makes the enum at least 32 bits wide.
Or hmm, you probably need 64 bits, and I don't know if 64-bit enums are portable. Maybe just go with the other approach.
(Assignee)

Updated

7 years ago
Attachment #495104 - Attachment is obsolete: true
(Assignee)

Comment 165

7 years ago
Created attachment 495664 [details] [diff] [review]
Part 1: Tag layers with scrollable information
(Assignee)

Updated

7 years ago
Attachment #495105 - Attachment is obsolete: true
(Assignee)

Comment 166

7 years ago
Created attachment 495665 [details] [diff] [review]
Part 2: Infrastructure for building shadow display list
(Assignee)

Updated

7 years ago
Attachment #495106 - Attachment is obsolete: true
Attachment #495106 - Flags: review?(jones.chris.g)
(Assignee)

Comment 167

7 years ago
Created attachment 495666 [details] [diff] [review]
Part 3: Viewport API for frontend
(Assignee)

Updated

7 years ago
Attachment #495110 - Attachment is obsolete: true
Attachment #495110 - Flags: review?(jones.chris.g)
(Assignee)

Comment 168

7 years ago
Created attachment 495667 [details] [diff] [review]
Part 4: Map for storing views
(Assignee)

Updated

7 years ago
Attachment #495112 - Attachment is obsolete: true
Attachment #495112 - Flags: superreview?(roc)
Attachment #495112 - Flags: review?(tnikkel)
(Assignee)

Comment 169

7 years ago
Created attachment 495668 [details] [diff] [review]
Part 5: Support displayport for iframes
(Assignee)

Updated

7 years ago
Attachment #495113 - Attachment is obsolete: true
Attachment #495113 - Flags: review?(jones.chris.g)
(Assignee)

Comment 170

7 years ago
Created attachment 495669 [details] [diff] [review]
Part 6: Use viewports to allow scrolling and build display lists
(Assignee)

Updated

7 years ago
Attachment #495114 - Attachment is obsolete: true
Attachment #495114 - Flags: review?(jones.chris.g)
(Assignee)

Comment 171

7 years ago
Created attachment 495671 [details] [diff] [review]
Part 7: Include viewport and content size in API
(Assignee)

Updated

7 years ago
Attachment #495115 - Attachment is obsolete: true
Attachment #495115 - Flags: review?(jones.chris.g)
(Assignee)

Comment 172

7 years ago
Created attachment 495672 [details] [diff] [review]
Part 8: Implement a scrollable interface on the content side
(Assignee)

Updated

7 years ago
Attachment #494881 - Attachment is obsolete: true
(Assignee)

Comment 173

7 years ago
Created attachment 495673 [details] [diff] [review]
Mobile: Use new interfaces
(Assignee)

Updated

7 years ago
Attachment #495664 - Flags: superreview+
Attachment #495664 - Flags: review+
(Assignee)

Updated

7 years ago
Attachment #495665 - Flags: superreview+
Attachment #495665 - Flags: review+
(Assignee)

Updated

7 years ago
Attachment #495666 - Flags: review?(jones.chris.g)
(Assignee)

Updated

7 years ago
Attachment #495667 - Flags: review?(jones.chris.g)
(Assignee)

Updated

7 years ago
Attachment #495668 - Flags: superreview?(roc)
Attachment #495668 - Flags: review?(tnikkel)
(Assignee)

Updated

7 years ago
Attachment #495669 - Flags: review?(jones.chris.g)
(Assignee)

Updated

7 years ago
Attachment #495671 - Flags: review?(jones.chris.g)
(Assignee)

Updated

7 years ago
Attachment #495672 - Flags: review?(jones.chris.g)
(Assignee)

Comment 174

7 years ago
As roc suggested, the API can now specify a rectangle for finding IDs. As cjones suggested, the API now returns an array of IDs.
Comment on attachment 495673 [details] [diff] [review]
Mobile: Use new interfaces

>+            if (Math.abs(this._pixelsPannedSinceRefresh.x) > 20 ||
>+                Math.abs(this._pixelsPannedSinceRefresh.y) > 20)
>+              this._updateCacheViewport();
this is still cause too many updates for main scrollable content view...  I think we should do it less aggressive, and update viewport 1 time only when it is really necessary.
Created attachment 495780 [details] [diff] [review]
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 on attachment 495666 [details] [diff] [review]
Part 3: Viewport API for frontend

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

Didn't do this.

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

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

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

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

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

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

At any rate, need docs :).

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

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

This wasn't fixed.

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

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

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

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

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

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

Again looking good.  I'd like to see the patch with these issues
fixed.
Attachment #495667 - Flags: review?(jones.chris.g)
(Giving up for the night, will review the other patches tomorrow.)
(Assignee)

Updated

7 years ago
Attachment #495666 - Attachment is obsolete: true
(Assignee)

Comment 180

7 years ago
Created attachment 495928 [details] [diff] [review]
Part 3: Viewport API for frontend
(Assignee)

Updated

7 years ago
Attachment #495667 - Attachment is obsolete: true
(Assignee)

Comment 181

7 years ago
Created attachment 495929 [details] [diff] [review]
Part 4: Map for storing views
(Assignee)

Updated

7 years ago
Attachment #495669 - Attachment is obsolete: true
Attachment #495669 - Flags: review?(jones.chris.g)
(Assignee)

Comment 182

7 years ago
Created attachment 495930 [details] [diff] [review]
Part 6: Use viewports to allow scrolling and build display lists
(Assignee)

Updated

7 years ago
Attachment #495671 - Attachment is obsolete: true
Attachment #495671 - Flags: review?(jones.chris.g)
(Assignee)

Comment 183

7 years ago
Created attachment 495931 [details] [diff] [review]
Part 7: Include viewport and content size in API
(Assignee)

Updated

7 years ago
Attachment #495928 - Flags: review?(jones.chris.g)
(Assignee)

Updated

7 years ago
Attachment #495929 - Flags: review?(jones.chris.g)
(Assignee)

Updated

7 years ago
Attachment #495930 - Flags: review?(jones.chris.g)
(Assignee)

Updated

7 years ago
Attachment #495931 - Flags: review?(jones.chris.g)
(Assignee)

Comment 184

7 years ago
> 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).
(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.
(Assignee)

Comment 186

7 years ago
> 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.
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 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.
Attachment #495928 - Flags: review?(jones.chris.g) → review+
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.
Attachment #495929 - Flags: review?(jones.chris.g) → review+
(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).
(Assignee)

Comment 191

7 years ago
Oops, grepped for "viewport" but not "Viewport".
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.
Attachment #495930 - Flags: review?(tnikkel)
Attachment #495930 - Flags: review?(jones.chris.g)
Attachment #495930 - Flags: review+
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.
Attachment #495931 - Flags: review?(jones.chris.g)
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.
Attachment #495672 - Flags: review?(jones.chris.g)
(Assignee)

Comment 195

7 years ago
> 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.
(Assignee)

Comment 196

7 years ago
> 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. :)
(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.
(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.
(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.
(Assignee)

Comment 200

7 years ago
> 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.
Nope, I missed that.  OK.
> +    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.
(Assignee)

Updated

7 years ago
Attachment #495664 - Attachment is obsolete: true
(Assignee)

Comment 203

7 years ago
Created attachment 496291 [details] [diff] [review]
Part 1: Tag layers with scrollable information
(Assignee)

Updated

7 years ago
Attachment #495665 - Attachment is obsolete: true
(Assignee)

Comment 204

7 years ago
Created attachment 496293 [details] [diff] [review]
Part 2: Infrastructure for building shadow display list
(Assignee)

Updated

7 years ago
Attachment #495928 - Attachment is obsolete: true
(Assignee)

Comment 205

7 years ago
Created attachment 496294 [details] [diff] [review]
Part 3: Viewport API for frontend
(Assignee)

Updated

7 years ago
Attachment #495929 - Attachment is obsolete: true
(Assignee)

Comment 206

7 years ago
Created attachment 496295 [details] [diff] [review]
Part 4: Map for storing views
(Assignee)

Updated

7 years ago
Attachment #495668 - Attachment is obsolete: true
Attachment #495668 - Flags: superreview?(roc)
Attachment #495668 - Flags: review?(tnikkel)
(Assignee)

Comment 207

7 years ago
Created attachment 496296 [details] [diff] [review]
Part 5: Support displayport for iframes
(Assignee)

Updated

7 years ago
Attachment #495930 - Attachment is obsolete: true
Attachment #495930 - Flags: review?(tnikkel)
(Assignee)

Comment 208

7 years ago
Created attachment 496299 [details] [diff] [review]
Part 6: Use viewports to allow scrolling and build display lists
(Assignee)

Updated

7 years ago
Attachment #495931 - Attachment is obsolete: true
(Assignee)

Comment 209

7 years ago
Created attachment 496300 [details] [diff] [review]
Part 7: Include viewport and content size in API
(Assignee)

Updated

7 years ago
Attachment #495672 - Attachment is obsolete: true
(Assignee)

Comment 210

7 years ago
Created attachment 496301 [details] [diff] [review]
Part 8: Implement a scrollable interface on the content side
(Assignee)

Updated

7 years ago
Attachment #495673 - Attachment is obsolete: true
(Assignee)

Comment 211

7 years ago
Created attachment 496302 [details] [diff] [review]
Mobile: Use new interfaces
(Assignee)

Updated

7 years ago
Attachment #496291 - Flags: superreview+
Attachment #496291 - Flags: review+
(Assignee)

Updated

7 years ago
Attachment #496293 - Flags: superreview+
Attachment #496293 - Flags: review+
(Assignee)

Updated

7 years ago
Attachment #496294 - Flags: review+
(Assignee)

Updated

7 years ago
Attachment #496295 - Flags: review+
(Assignee)

Updated

7 years ago
Attachment #496296 - Flags: superreview?(roc)
Attachment #496296 - Flags: review?(tnikkel)
(Assignee)

Updated

7 years ago
Attachment #496299 - Flags: review?(tnikkel)
(Assignee)

Updated

7 years ago
Attachment #496300 - Flags: review?(jones.chris.g)
Attachment #496296 - Flags: superreview?(roc) → superreview+
(Assignee)

Updated

7 years ago
Attachment #496296 - Attachment is obsolete: true
Attachment #496296 - Flags: review?(tnikkel)
(Assignee)

Comment 212

7 years ago
Created attachment 496334 [details] [diff] [review]
Part 5: Support displayport for iframes
(Assignee)

Updated

7 years ago
Attachment #496302 - Attachment is obsolete: true
(Assignee)

Comment 213

7 years ago
Created attachment 496335 [details] [diff] [review]
Mobile: Use new interfaces
(Assignee)

Updated

7 years ago
Attachment #496301 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Attachment #496334 - Flags: superreview?(roc)
Attachment #496334 - Flags: review?(tnikkel)
(Assignee)

Comment 214

7 years ago
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 on attachment 496334 [details] [diff] [review]
Part 5: Support displayport for iframes

What changed that I need to re-review this?
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.
Attachment #496300 - Flags: superreview?(roc)
Attachment #496300 - Flags: review?(jones.chris.g)
Attachment #496300 - Flags: review+
(Assignee)

Comment 217

7 years ago
(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.
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'.
(Assignee)

Comment 219

7 years ago
(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?
I guess you'll have to create some scriptable way to get the property. I don't think using SetUserData is an option.
(Assignee)

Comment 221

7 years ago
I need some alternate proposals then. I'm a big fan of just adding a new interface like part 8 did :)
You don't need a new interface class just to get the viewID property. Just add a method to DOMWindowUtils?
(Assignee)

Comment 223

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