Closed Bug 784908 Opened 12 years ago Closed 12 years ago

Fix terminology and coordinate spaces in FrameMetrics

Categories

(Core :: Graphics: Layers, defect)

Product:
Core
Component:
Graphics: Layers
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla18

People

(Reporter: drs, Assigned: drs)

References

Details

Attachments

(5 files, 25 obsolete files)

Part 3: Distinguish resolution from a new zoom field on FrameMetrics.
23.48 KB, patch
drs
: review+
Details | Diff | Splinter Review
Part 2: Add a call to UpdateDimensions when a tab is first created.
4.00 KB, patch
drs
: review+
Details | Diff | Splinter Review
Part 0: Make this test actually test session history instead of scroll port clamping
773 bytes, patch
cjones
: review+
Details | Diff | Splinter Review
Part 4: Temporarily backout the coordinate space changes to FrameMetrics::mScrollOffset
14.98 KB, patch
Details | Diff | Splinter Review
Part 1: Change names of FrameMetrics variables to be more descriptive, add documentation, change some coordinate spaces.
68.05 KB, patch
drs
: review+
Details | Diff | Splinter Review
The terminology used in FrameMetrics and coordinate spaces/variable precision are wrong in some places. Refer to: https://bugzilla.mozilla.org/show_bug.cgi?id=750974#c18 The comments here aren't entirely correct since FrameMetrics has grown in use cases since that comment was made. The main things I want to do are: 1) Document FrameMetrics correctly. 2) Fix coordinate spaces. 3) Clear up the viewport vs. widget bounds confusion once and for all. 4) Add a way to distinguish zoom and resolution.
Also note that it would be nice if we could combine the scroll offset and zoom into a single transform, but this is a pretty big undertaking and I'd prefer to do it in a follow-up.
roc: setting you for review, let me know if you can't/don't want to do it. There are some mistakes leftover after this patch which are corrected in followups posted in this thread. For example, we are grossly abusing the mCSSViewport field on FrameMetrics to mean what it doesn't. I added the mWidgetBounds field here, but don't actually use it until a later patch.
Assignee: nobody → bugzilla
Attachment #654560 - Flags: review?(roc)
This properly distinguishes the widget bounds and CSS viewport, which are different concepts entirely but have unfortunately been muddled a bit.
Attachment #654561 - Flags: review?(roc)
This was previously working because we were muddling the CSS viewport and widget bounds. The CSS viewport "happened" to be the widget bounds, so when we first got it, it would be correct by accident. This patch makes |mWidgetBounds| get properly initialized.
Attachment #654562 - Flags: review?(jones.chris.g)
This is mostly a "nice to have," in that it doesn't do anything useful yet. Eventually, we will probably want to be able to paint at a lower resolution than we were zoomed into since doing this allows us to repaint faster while we're scrolling very fast.
Attachment #654564 - Flags: review?(roc)
It might be a good idea to fold the first two patches together.
Comment on attachment 654560 [details] [diff] [review] Change names of FrameMetrics variables to be more descriptive, add documentation, change some coordinate spaces. Review of attachment 654560 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/FrameMetrics.h @@ +72,5 @@ > + // --------------------------------------------------------------------------- > + // The following metrics are all in widget space/device pixels. > + // > + > + // The width/height of the surface we draw to through a glContext. We can't refer to glContext here since this will be used for non-GL backends. What does it mean for non-root layers? @@ +109,4 @@ > ViewID mScrollId; > > + // The bounds of a frame. This is determined by reflow. Relative to "something > + // else". For the top-level |window|, "something else" isn't all that helpful :-). These comments could all use some improvement. Most of them need to be clarified for non-root layers.
Folded version of "attachment 654560 [details] [diff] [review]: Change names of FrameMetrics variables to be more descriptive, add documentation, change some coordinate spaces." and "attachment 654561 [details] [diff] [review]: Clear up confusion between viewport and widget bounds", including fixes for code review comments.
Attachment #654560 - Attachment is obsolete: true
Attachment #654561 - Attachment is obsolete: true
Attachment #654560 - Flags: review?(roc)
Attachment #654561 - Flags: review?(roc)
Attachment #654904 - Flags: review?(roc)
Rebased.
Attachment #654562 - Attachment is obsolete: true
Attachment #654562 - Flags: review?(jones.chris.g)
Attachment #654905 - Flags: review?(jones.chris.g)
Rebased. Also a few bug fixes, mostly in the NotifyLayersUpdated function.
Attachment #654564 - Attachment is obsolete: true
Attachment #654564 - Flags: review?(roc)
Attachment #654906 - Flags: review?(roc)
I'm not pushing this new round to try. Nothing changed other than rebasing, comments, and stuff that doesn't get tested anyways.
Comment on attachment 654904 [details] [diff] [review] Change names of FrameMetrics variables to be more descriptive, add documentation, change some coordinate spaces. Review of attachment 654904 [details] [diff] [review]: ----------------------------------------------------------------- Sorry, but this is still very confusing. ::: gfx/layers/FrameMetrics.h @@ +75,5 @@ > + // > + > + // The width/height of the surface we draw to. > + // So { 0, 0, [surface.width], [surface.height] }.} > + // In the case of the root layer, this will generally just be the screen size. You mean the window size? @@ +77,5 @@ > + // The width/height of the surface we draw to. > + // So { 0, 0, [surface.width], [surface.height] }.} > + // In the case of the root layer, this will generally just be the screen size. > + // In the case of non-root layers, this will be however large the frame > + // appears on the screen. This doesn't really make sense. Non-root layers can be transformed, so what does mWidgetBounds mean then? Should we just say that it's undefined and not used for non-root layers? @@ +82,5 @@ > + nsIntRect mWidgetBounds; > + > + // The CSS content rect, stored in device pixels. > + // XXX: Is this really necessary? Can it not be calculated from > + // |mCSSContentRect| whenever it's needed? Same, root layers only? And what exactly does "CSS content rect" mean? @@ +87,4 @@ > nsIntRect mContentRect; > + > + // --------------------------------------------------------------------------- > + // The following metrics are all in layer space/CSS pixels. "layer space" and CSS pixels aren't the same thing. @@ +107,5 @@ > + // impacted by the <meta name="viewport"> tag. > + // This is means essentially the same thing on the root layer and non-root > + // layers. The CSS viewport may or may not be smaller on non-root layers than > + // on the root layer. > + gfx::Rect mCSSViewport; I don't know what the "CSS viewport" means for non-root layers. @@ +115,5 @@ > + // content rect. > + // In the case of non-root layers, this is still relative to the top-left of > + // the CSS content rect, but for this frame, not for any parent frame. In > + // essence, this metric is isolated from other frames. > + gfx::Point mCSSScrollOffset; Isn't this redundant with the other values? @@ +125,5 @@ > + // The bounds of a frame. This is determined by reflow. For the top-level > + // |window|, > + // { x = window.scrollX, y = window.scrollY, // could be 0, 0 > + // width = window.innerWidth, height = window.innerHeight } > + // Drawing is always clipped to this rect (and the clip accumulates). What does "the clip accumulates" mean? Drawing isn't always clipped to this rect, right? It's clipped to the displayport instead. I think you need to clarify that mCSSContentRect is in the coordinates of the layer (if that's true!). @@ +133,4 @@ > gfx::Rect mCSSContentRect; > > // This represents the resolution at which the associated layer > // will been rendered. This could be better documented too.
Attachment #654905 - Flags: review?(jones.chris.g) → review+
Blocks: 785929
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #13) > Comment on attachment 654904 [details] [diff] [review] > Change names of FrameMetrics variables to be more descriptive, add > documentation, change some coordinate spaces. > > Review of attachment 654904 [details] [diff] [review]: > ----------------------------------------------------------------- > > Sorry, but this is still very confusing. Yeah, I was really messing up mobile and desktop, as well as writing about things we don't yet support. Hopefully this new patch will clear things up, minus some nits I expect you'll have. My main question for you still is: do iframes within documents get their own layer tree, or _can_ they be part of another layer tree? I assumed the latter for all of my comments. > ::: gfx/layers/FrameMetrics.h > @@ +75,5 @@ > > + // > > + > > + // The width/height of the surface we draw to. > > + // So { 0, 0, [surface.width], [surface.height] }.} > > + // In the case of the root layer, this will generally just be the screen size. > > You mean the window size? Yes, that's right. I renamed this to mCompositionBounds since that's more accurate, and the comment should explain how it could be useful for non-root layers (assuming iframes can be in the layer tree). > > @@ +77,5 @@ > > + // The width/height of the surface we draw to. > > + // So { 0, 0, [surface.width], [surface.height] }.} > > + // In the case of the root layer, this will generally just be the screen size. > > + // In the case of non-root layers, this will be however large the frame > > + // appears on the screen. > > This doesn't really make sense. Non-root layers can be transformed, so what > does mWidgetBounds mean then? Should we just say that it's undefined and not > used for non-root layers? That's correct too, at least for now. > @@ +82,5 @@ > > + nsIntRect mWidgetBounds; > > + > > + // The CSS content rect, stored in device pixels. > > + // XXX: Is this really necessary? Can it not be calculated from > > + // |mCSSContentRect| whenever it's needed? > > Same, root layers only? > > And what exactly does "CSS content rect" mean? I think it's more accurate to say it's valid whenever mCSSContentRect is, which addresses both of these. > @@ +107,5 @@ > > + // impacted by the <meta name="viewport"> tag. > > + // This is means essentially the same thing on the root layer and non-root > > + // layers. The CSS viewport may or may not be smaller on non-root layers than > > + // on the root layer. > > + gfx::Rect mCSSViewport; > > I don't know what the "CSS viewport" means for non-root layers. Assuming iframes can be inside the layer tree, then it would be their viewport. > @@ +115,5 @@ > > + // content rect. > > + // In the case of non-root layers, this is still relative to the top-left of > > + // the CSS content rect, but for this frame, not for any parent frame. In > > + // essence, this metric is isolated from other frames. > > + gfx::Point mCSSScrollOffset; > > Isn't this redundant with the other values? I don't see how. Are you saying that we should store the scroll offset on |mCSSViewport|'s x and y offsets? If so, isn't that confusing since the viewport and document should move relative to one another? Also, I believe that will eventually make it more difficult to combine the zoom and scroll into one transform matrix, which is something I want to do eventually. Maybe I'm just misunderstanding you here; would appreciate a clarification. > > @@ +125,5 @@ > > + // The bounds of a frame. This is determined by reflow. For the top-level > > + // |window|, > > + // { x = window.scrollX, y = window.scrollY, // could be 0, 0 > > + // width = window.innerWidth, height = window.innerHeight } > > + // Drawing is always clipped to this rect (and the clip accumulates). > > What does "the clip accumulates" mean? > > Drawing isn't always clipped to this rect, right? It's clipped to the > displayport instead. > > I think you need to clarify that mCSSContentRect is in the coordinates of > the layer (if that's true!). Yeah, good point, and mCSSContentRect is not in layer coordinates. > @@ +133,4 @@ > > gfx::Rect mCSSContentRect; > > > > // This represents the resolution at which the associated layer > > // will been rendered. > > This could be better documented too. This is commented better in the zoom vs. resolution patch.
Attachment #654904 - Attachment is obsolete: true
Attachment #654904 - Flags: review?(roc)
Attachment #655687 - Flags: review?(roc)
Rebased.
Attachment #654906 - Attachment is obsolete: true
Attachment #654906 - Flags: review?(roc)
Attachment #655688 - Flags: review?(roc)
Bug fix/add ctor for mZoom.
Attachment #655688 - Attachment is obsolete: true
Attachment #655688 - Flags: review?(roc)
Attachment #656006 - Flags: review?(roc)
(In reply to Doug Sherk (:drs) (:dRdR) from comment #14) > My main question for you still is: do iframes within documents get their own > layer tree, or _can_ they be part of another layer tree? I assumed the > latter for all of my comments. They can be part of another layer tree. > > I don't know what the "CSS viewport" means for non-root layers. > > Assuming iframes can be inside the layer tree, then it would be their > viewport. What if I set a displayport on a scrollable DIV element? > > @@ +133,4 @@ > > > gfx::Rect mCSSContentRect; > > > > > > // This represents the resolution at which the associated layer > > > // will been rendered. > > > > This could be better documented too. > > This is commented better in the zoom vs. resolution patch. Which patch is that?
Comment on attachment 655687 [details] [diff] [review] Change names of FrameMetrics variables to be more descriptive, add documentation, change some coordinate spaces. Review of attachment 655687 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/FrameMetrics.h @@ +85,5 @@ > + // of it prerendered. > + // > + // On desktop, this is not used, but theoretically it can be said that: > + // |mCompositionBounds == mCSSViewport == mCSSDisplayPort| after unit > + // conversions. How can they be the same if they have different units? @@ +88,5 @@ > + // |mCompositionBounds == mCSSViewport == mCSSDisplayPort| after unit > + // conversions. > + // > + // This is only valid on the root layer. Nested iframes do not need this > + // metric as they do not have a displayport. See bug 775452. They could have a displayport. But I think it's still true that this is only relevant on the root layer. @@ +89,5 @@ > + // conversions. > + // > + // This is only valid on the root layer. Nested iframes do not need this > + // metric as they do not have a displayport. See bug 775452. > + nsIntRect mCompositionBounds; This is the area within the widget that we're compositing to. Right? @@ +104,4 @@ > nsIntRect mContentRect; > + > + // --------------------------------------------------------------------------- > + // The following metrics are all in CSS space/CSS pixels. Really? Why not layer pixels? The layer system doesn't deal with CSS pixels anywhere else. @@ +152,5 @@ > + // width = window.innerWidth, height = window.innerHeight } > + // > + // On desktop, drawing is always clipped to this rect. > + // On mobile, drawing is clipped to |mCSSDisplayPort| instead, but > + // |mCSSDisplayPort| is clipped to this rect. I don't think we should be talking about "on desktop, we do this, on mobile we do that". FrameMetrics is part of the layers API, and we should explain it in those terms.
Blocks: 786808
Blocks: 746502
No longer blocks: 746502
Blocks: 746502
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #18) > Comment on attachment 655687 [details] [diff] [review] > Change names of FrameMetrics variables to be more descriptive, add > documentation, change some coordinate spaces. > > Review of attachment 655687 [details] [diff] [review]: > ----------------------------------------------------------------- > > This is the area within the widget that we're compositing to. Right? Yes. I like your explanation better so I'm going to use it instead. > @@ +104,4 @@ > > nsIntRect mContentRect; > > + > > + // --------------------------------------------------------------------------- > > + // The following metrics are all in CSS space/CSS pixels. > > Really? Why not layer pixels? The layer system doesn't deal with CSS pixels > anywhere else. FrameMetrics is built by layout/base/nsDisplayList.cpp, and it's used mostly within layout code. To me it seems like it's an interface between gfx and layout, and it makes more sense to provide it in the coordinate space it will be used in (otherwise we will need a second redundant interface). > @@ +152,5 @@ > > + // width = window.innerWidth, height = window.innerHeight } > > + // > > + // On desktop, drawing is always clipped to this rect. > > + // On mobile, drawing is clipped to |mCSSDisplayPort| instead, but > > + // |mCSSDisplayPort| is clipped to this rect. > > I don't think we should be talking about "on desktop, we do this, on mobile > we do that". FrameMetrics is part of the layers API, and we should explain > it in those terms. Unfortunately it's not that simple. =/ I agree in this case and I'm removing that comment. In the other cases, however, I think it's useful information. Perhaps I could explain it in more general terms; for example, instead of saying "on desktop, X; on mobile, Y" I could say "if the viewport and displayport are not the same, this is the interaction that will take place." Does that sound better to you?
Ok, this should generalize the terminology a bit so we're not referring to desktop/mobile a lot. I left one reference to it because I think it's important to at least allude to this distinction.
Attachment #655687 - Attachment is obsolete: true
Attachment #655687 - Flags: review?(roc)
Attachment #656858 - Flags: review?(roc)
Rebased, and better comments in FrameMetrics.h explaining things more from the layers perspective.
Attachment #656006 - Attachment is obsolete: true
Attachment #656006 - Flags: review?(roc)
Attachment #656859 - Flags: review?(roc)
(In reply to Doug Sherk (:drs) (:dRdR) from comment #19) > FrameMetrics is built by layout/base/nsDisplayList.cpp, and it's used mostly > within layout code. To me it seems like it's an interface between gfx and > layout, and it makes more sense to provide it in the coordinate space it > will be used in (otherwise we will need a second redundant interface). I don't understand. The consumers of the information in FrameMetrics should all be layer-related and working with layer pixels. > Unfortunately it's not that simple. =/ I agree in this case and I'm removing > that comment. In the other cases, however, I think it's useful information. > > Perhaps I could explain it in more general terms; for example, instead of > saying "on desktop, X; on mobile, Y" I could say "if the viewport and > displayport are not the same, this is the interaction that will take place." > Does that sound better to you? Yes.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #22) > (In reply to Doug Sherk (:drs) (:dRdR) from comment #19) > > FrameMetrics is built by layout/base/nsDisplayList.cpp, and it's used mostly > > within layout code. To me it seems like it's an interface between gfx and > > layout, and it makes more sense to provide it in the coordinate space it > > will be used in (otherwise we will need a second redundant interface). > > I don't understand. The consumers of the information in FrameMetrics should > all be layer-related and working with layer pixels. > I wanted it to be this way too, but it turns out in practice that basically none of the consumers want layer pixels. It's been simpler and less error prone to compute in float CSS pixels, and then let gecko handle the pixel snapping on the content side.
The very first FrameMetrics consumer I looked at, in CompensateForContentScrollOffset, has gfx3DMatrix m(aContainer->GetTransform()); m.Translate(gfxPoint3D(-fm.mViewportScrollOffset.x, -fm.mViewportScrollOffset.y, 0)); i.e. it's assuming mViewportScrollOffset is in layer pixels.
I was very careful not to say *all* the consumers.
OK, let me put it this way. I think all consumers of FrameMetrics data should be using layer pixels. Can you point me to some that must use CSS pixels?
As per IRC discussion, I'm going to add a new metric to FrameMetrics which defines the current ratio of CSS pixels to device pixels, so that consumers that want to convert a frame's CSS pixel metrics into device pixels or layers pixels can do so.
(In reply to Doug Sherk (:drs) (:dRdR) from comment #27) > As per IRC discussion, I'm going to add a new metric to FrameMetrics which > defines the current ratio of CSS pixels to device pixels, so that consumers > that want to convert a frame's CSS pixel metrics into device pixels or > layers pixels can do so. Also note as a result of this I'll probably be folding bug 786808 into this one.
Dealt with code review comments.
Attachment #656858 - Attachment is obsolete: true
Attachment #656858 - Flags: review?(roc)
Attachment #659113 - Flags: review?(roc)
Rebased, small bug fixes.
Attachment #656859 - Attachment is obsolete: true
Attachment #656859 - Flags: review?(roc)
Attachment #659114 - Flags: review?(roc)
Comment on attachment 659113 [details] [diff] [review] Change names of FrameMetrics variables to be more descriptive, add documentation, change some coordinate spaces. Review of attachment 659113 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/FrameMetrics.h @@ +70,5 @@ > { > return mScrollId != NULL_SCROLL_ID; > } > > + float LayersPixelsPerCSSPixel() const We need to distinguish horizontal from vertical since they can be different (e.g. for elements stretched by CSS transforms). @@ +94,5 @@ > + // instead. > + // > + // This is only valid on the root layer. Nested iframes do not need this > + // metric as they do not have a displayport set. See bug 775452. > + nsIntRect mCompositionBounds; In device pixels? @@ +101,5 @@ > + // > + // This is valid on any layer where |mCSSContentRect| is, though it may be > + // more lazily maintained than |mCSSContentRect|. That is, when > + // |mCSSContentRect| is updated, this may lag. For this reason, it's better to > + // use |mCSSContentRect| for any control logic. This sounds scary. It sounds like "like mCSSContentRect, but you can't use it". Why not just get rid of it? @@ +138,5 @@ > + // > + // This is mainly useful on the root layer, however nested iframes can have > + // their own viewport, which will just be the size of the window of the > + // iframe. > + gfx::Rect mCSSViewport; I think there's some redundancy here, and these different rectangles are still quite confusing. For layers that correspond to the rendering of a document, does mCSSViewport tell us where the CSS viewport of the document is, in this layer? If so, then for layers that don't correspond to the rendering of a document, what does it mean?
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #32) > Comment on attachment 659113 [details] [diff] [review] > Change names of FrameMetrics variables to be more descriptive, add > documentation, change some coordinate spaces. > > Review of attachment 659113 [details] [diff] [review]: > ----------------------------------------------------------------- > > @@ +101,5 @@ > > + // > > + // This is valid on any layer where |mCSSContentRect| is, though it may be > > + // more lazily maintained than |mCSSContentRect|. That is, when > > + // |mCSSContentRect| is updated, this may lag. For this reason, it's better to > > + // use |mCSSContentRect| for any control logic. > > This sounds scary. It sounds like "like mCSSContentRect, but you can't use > it". Why not just get rid of it? I'd rather do that in a followup. This patch doesn't touch this metric, other than commenting it better. I filed bug 785929 a while ago to do this. > @@ +138,5 @@ > > + // > > + // This is mainly useful on the root layer, however nested iframes can have > > + // their own viewport, which will just be the size of the window of the > > + // iframe. > > + gfx::Rect mCSSViewport; > > I think there's some redundancy here, and these different rectangles are > still quite confusing. I don't see how, could you elaborate on this? > For layers that correspond to the rendering of a document, does mCSSViewport > tell us where the CSS viewport of the document is, in this layer? No, it tells us the dimensions of the viewport, which are not the same as the composition bounds. |mCSSViewport| can be set by arbitrary web content whereas |mCompositionBounds| is always the area, in layers pixels, that we're compositing to. > If so, then for layers that don't correspond to the rendering of a document, > what does it mean? You're right, this is a big hole I left out. I'll comment this more extensively. To answer your question, for layers that don't correspond to a document, the metric is meaningless and invalid.
Dealt with code review comments.
Attachment #659113 - Attachment is obsolete: true
Attachment #659113 - Flags: review?(roc)
Attachment #659132 - Flags: review?(roc)
(In reply to Doug Sherk (:drs) (:dRdR) from comment #33) > I'd rather do that in a followup. This patch doesn't touch this metric, > other than commenting it better. I filed bug 785929 a while ago to do this. Then change the comment to "OBSOLETE. DO NOT USE." > > For layers that correspond to the rendering of a document, does mCSSViewport > > tell us where the CSS viewport of the document is, in this layer? > > No, it tells us the dimensions of the viewport, which are not the same as > the composition bounds. |mCSSViewport| can be set by arbitrary web content > whereas |mCompositionBounds| is always the area, in layers pixels, that > we're compositing to. I don't understand this. mCSSViewport is a rectangle, so it doesn't just contain dimensions. If it doesn't represent the CSS viewport, perhaps it should have a different name?
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #35) > > > For layers that correspond to the rendering of a document, does mCSSViewport > > > tell us where the CSS viewport of the document is, in this layer? > > > > No, it tells us the dimensions of the viewport, which are not the same as > > the composition bounds. |mCSSViewport| can be set by arbitrary web content > > whereas |mCompositionBounds| is always the area, in layers pixels, that > > we're compositing to. > > I don't understand this. mCSSViewport is a rectangle, so it doesn't just > contain dimensions. If it doesn't represent the CSS viewport, perhaps it > should have a different name? I believe that in a bunch of places that we use rects instead of sizes where the origin of the rect is usually 0,0 to provide support for RTL pages. I don't know much about this so I can't comment. Perhaps it would be ok if the viewport was a size instead, and maybe the composition bounds too.
I talked with roc again on IRC and here's the list of stuff we came up with that need to be done for the next iteration: * mCSSViewport should use, as its offset, the position of a frame relative to its parent. This is useful in the case that we're looking at the FrameMetrics of an iframe (or really any element) in a parent document and scrolling is not implemented by setting the scroll offset on its document, but rather by transforming it in some way and leaving the scroll offset at 0,0. roc says that, here: https://mxr.mozilla.org/mozilla-central/source/layout/base/nsDisplayList.cpp#1031 "yeah, but mVisibleRect isn't the right thing to use for the viewport, since it takes account of clipping and occlusion for example... it needs to use aForFrame's bounds, plus aForFrame->GetOffsetTo(referenceFrame) or something like that" * mCSSContentRect should be renamed to mCSSScrollableRect, since this makes it more clear that this rect is not just a rectangle created by finding the bounds of all content, but more specifically the region that is actually scrollable. * It should be documented that it is required that the rectangle (mCSSScrollOffset.x/y, mCSSViewportRect.width/height) is contained in mCSSScrollableRect (except while you're allowing overscrolling). * Note that mCSSScrollableRect and mCSSScrollOffset are in the same coordinate space, but that is a *different* coordinate space to mCSSViewport/mCSSDisplayPort. * Remove mCompositionBounds and instead pass mVisibleRegion into APZC::SampleContentTransformForFrame(), which can call CalculatePendingDisplayPort() using these bounds. * Take the CSS tag off of everything since everything is in CSS pixels now and it's a useless distinction. Also of note, remove mContentRect, though that'll be done in a followup (bug 785929).
Addressed all code review comments. I didn't remove mCompositionBounds because we need this information off the compositor thread. I don't see an easy way of accessing this information without storing it on FrameMetrics. I might revisit this eventually.
Attachment #659132 - Attachment is obsolete: true
Attachment #659132 - Flags: review?(roc)
Attachment #661415 - Flags: review?(roc)
Rebased/small bug fixes.
Attachment #659114 - Attachment is obsolete: true
Attachment #659114 - Flags: review?(roc)
Attachment #661416 - Flags: review?(roc)
(In reply to Doug Sherk (:drs) (:dRdR) from comment #38) > Addressed all code review comments. I didn't remove mCompositionBounds > because we need this information off the compositor thread. I don't see an > easy way of accessing this information without storing it on FrameMetrics. I > might revisit this eventually. I don't quite understand? How would you have access to FrameMetrics but not the layer itself?
Comment on attachment 661415 [details] [diff] [review] Change names of FrameMetrics variables to be more descriptive, add documentation, change some coordinate spaces. Review of attachment 661415 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/FrameMetrics.h @@ +93,5 @@ > + // viewport, there is no need for this and we can just use the viewport > + // instead. > + // > + // This is only valid on the root layer. Nested iframes do not need this > + // metric as they do not have a displayport set. See bug 775452. Can we define this as the bounds of the root layer's visible region? @@ +126,5 @@ > + // width = window.innerWidth + 100, height = window.innerHeight + 100 } > + // > + // This is only valid on the root layer. Nested iframes do not have a > + // displayport set on them. See bug 775452. > + gfx::Rect mDisplayPortRect; This can just be mDisplayPort? Didn't we agree to define this as the CSS display port, in CSS pixels, relative to the top-left of the layer? If so, this comment should say so. @@ +139,5 @@ > + // This is mainly useful on the root layer, however nested iframes can have > + // their own viewport, which will just be the size of the window of the > + // iframe. For layers that don't correspond to a document, this metric is > + // meaningless and invalid. > + gfx::Rect mViewportRect; This can just be mViewport. Didn't we agree to define mViewport as the CSS viewport, in CSS pixels, relative to the top-left of the layer? If so, this comment should say so. @@ +156,5 @@ > + // Be within |mScrollableRect|. > + // > + // This is valid for any layer, but is always relative to this frame and > + // not any parents, regardless of parent transforms. > + gfx::Point mScrollOffset; I think for this you should say that it's in CSS pixels, and that it's in the coordinate system understood by window.scrollTo. @@ +167,5 @@ > + // For the top-level |window|, > + // { x = window.scrollX, y = window.scrollY, // could be 0, 0 > + // width = window.innerWidth, height = window.innerHeight } > + // Drawing is always clipped to this rect. That is to say, we never try to > + // draw outside it. This doesn't have to be true, does it? Layout would draw outside this rect if the displayport is set outside it. Maybe just take these two sentences out. @@ +174,5 @@ > + // |mScrollOffset|, but a different coordinate space than |mViewportRect| and > + // |mDisplayPortRect|. > + // > + // On non-root layers the clip accumulates (i.e. a parent's |mScrollableRect| > + // will clip a child's depending on where the child is positioned). I'm not sure what this means. I don't think mScrollableRect (or any other parameters in FrameMetrics) should affect rendering at all.
Dealt with comments.
Attachment #661415 - Attachment is obsolete: true
Attachment #661415 - Flags: review?(roc)
Attachment #662253 - Flags: review?(roc)
Comment on attachment 662253 [details] [diff] [review] Change names of FrameMetrics variables to be more descriptive, add documentation, change some coordinate spaces. Review of attachment 662253 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/FrameMetrics.h @@ +95,5 @@ > + // instead. > + // > + // This is only valid on the root layer. Nested iframes do not need this > + // metric as they do not have a displayport set. See bug 775452. > + nsIntRect mCompositionBounds; I still think we should add something saying that this is the bounds of the root layer's visible region. @@ +114,5 @@ > + // space, so each is explained separately. > + // > + > + // The area of a frame's contents that has been painted, relative to the > + // top-left of the layer. It is in the same coordinate space as |mViewport|. Didn't we agree to make mDisplayPort relative to the viewport's top-left, i.e. if mDisplayPort's top-left is 0,0, then it's in the same place as the viewport top-left in the layer, and at the same place as the scroll offset in the document? @@ +144,5 @@ > + // This is mainly useful on the root layer, however nested iframes can have > + // their own viewport, which will just be the size of the window of the > + // iframe. For layers that don't correspond to a document, this metric is > + // meaningless and invalid. > + gfx::Rect mViewport; I still think we should say that this is the CSS viewport.
> ::: gfx/layers/FrameMetrics.h > @@ +95,5 @@ > > + // instead. > > + // > > + // This is only valid on the root layer. Nested iframes do not need this > > + // metric as they do not have a displayport set. See bug 775452. > > + nsIntRect mCompositionBounds; > > I still think we should add something saying that this is the bounds of the > root layer's visible region. Dealt with everything except this. I included this information already at the top of the comments for this field.
Attachment #662253 - Attachment is obsolete: true
Attachment #662253 - Flags: review?(roc)
Attachment #663508 - Flags: review?(roc)
Blocks: 786267
Comment on attachment 663508 [details] [diff] [review] Change names of FrameMetrics variables to be more descriptive, add documentation, change some coordinate spaces. Review of attachment 663508 [details] [diff] [review]: ----------------------------------------------------------------- Looking good! ::: gfx/layers/Layers.cpp @@ +149,5 @@ > +{ > + s += pfx; > + s += nsPrintfCString( > + "(x=%f, y=%f, w=%f, h=%f)", > + r.x, r.y, r.width, r.height); s.AppendPrintf ::: gfx/layers/ipc/AsyncPanZoomController.cpp @@ +681,5 @@ > gfx::Rect pageSize = aCSSPageRect; > float scale = mFrameMetrics.mResolution.width; > > // The page rect is the css page rect scaled by the current zoom. > pageSize.ScaleRoundOut(1 / scale); Call ScaleInverseRoundOut @@ +736,3 @@ > float scale = mFrameMetrics.mResolution.width; > + nsIntRect compositionBounds = mFrameMetrics.mCompositionBounds; > + compositionBounds.ScaleRoundIn(1 / scale); Add BaseRect::ScaleInverseRoundIn and call it here. @@ +885,5 @@ > > if (frame.IsScrollable()) { > + metricsScrollOffset = > + gfx::Point(frame.mScrollOffset.x * frame.LayersPixelsPerCSSPixel().width, > + frame.mScrollOffset.y * frame.LayersPixelsPerCSSPixel().height); Add FrameMetrics::GetScrollOffsetInLayerPixels and call it here and in CompositorParent.cpp (twice) @@ +1011,5 @@ > // size. > if (zoomToRect.IsEmpty()) { > + // composition bounds in CSS coordinates > + nsIntRect cssCompositionBounds = compositionBounds; > + cssCompositionBounds.ScaleRoundIn(1 / mFrameMetrics.mResolution.width); Add BaseRect::ScaleInverseRoundIn and call it here. ::: gfx/layers/ipc/Axis.cpp @@ +274,5 @@ > + nsIntRect compositionBounds = metrics.mCompositionBounds; > + gfx::Rect scaledCompositionBounds = > + gfx::Rect(compositionBounds.x, compositionBounds.y, > + compositionBounds.width, compositionBounds.height); > + scaledCompositionBounds.ScaleRoundIn(1 / (currentScale * aScale)); ScaleInverseRoundIn ::: layout/base/nsDisplayList.cpp @@ +589,5 @@ > + metrics.mDisplayPort = mozilla::gfx::Rect( > + NSAppUnitsToDoublePixels(aDisplayPort->x, auPerDevPixel) * aContainerParameters.mXScale, > + NSAppUnitsToDoublePixels(aDisplayPort->y, auPerDevPixel) * aContainerParameters.mYScale, > + NSAppUnitsToDoublePixels(aDisplayPort->width, auPerDevPixel) * aContainerParameters.mXScale, > + NSAppUnitsToDoublePixels(aDisplayPort->height, auPerDevPixel) * aContainerParameters.mYScale); displayport and viewport are in CSS pixels now, so I don't understand why we still need to multiply by aContainerParameters.mXScale/mYScale. Can't we just convert from appunits to CSS pixels and be done? @@ +1057,5 @@ > } > > + nsRect viewport = aForFrame->GetRect() - > + aForFrame->GetPosition() + > + aBuilder->ToReferenceFrame(aForFrame); nsRect viewport(aBuilder->ToReferenceFrame(aForFrame), aForFrame->GetSize()); ::: layout/ipc/RenderFrameParent.cpp @@ +159,5 @@ > aConfig.mScrollOffset.ToNearestPixels(auPerDevPixel); > // metricsScrollOffset is in layer coordinates. > + gfx::Point metricsScrollOffset = > + gfx::Point(aMetrics->mScrollOffset.x * aMetrics->LayersPixelsPerCSSPixel().width, > + aMetrics->mScrollOffset.y * aMetrics->LayersPixelsPerCSSPixel().height); Another use for FrameMetrics::GetScrollOffsetInLayerPixels()
Dealt with code review comments.
Attachment #663508 - Attachment is obsolete: true
Attachment #663508 - Flags: review?(roc)
Attachment #665115 - Flags: review?(roc)
Rebased, r+ carried.
Attachment #654905 - Attachment is obsolete: true
Attachment #665116 - Flags: review+
Rebased.
Attachment #661416 - Attachment is obsolete: true
Attachment #661416 - Flags: review?(roc)
Attachment #665117 - Flags: review?(roc)
Comment on attachment 665115 [details] [diff] [review] Change names of FrameMetrics variables to be more descriptive, add documentation, change some coordinate spaces. Review of attachment 665115 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/ipc/TabChild.cpp @@ +863,5 @@ > data += nsPrintfCString(", \"cssPageRect\" : "); > + data += nsPrintfCString("{ \"x\" : %f", aFrameMetrics.mScrollableRect.x); > + data += nsPrintfCString(", \"y\" : %f", aFrameMetrics.mScrollableRect.y); > + data += nsPrintfCString(", \"width\" : %f", aFrameMetrics.mScrollableRect.width); > + data += nsPrintfCString(", \"height\" : %f", aFrameMetrics.mScrollableRect.height); These should all be converted to AppendPrintf, but that can happen later. ::: gfx/layers/FrameMetrics.h @@ +75,5 @@ > + { > + return mResolution * mDevPixelsPerCSSPixel; > + } > + > + gfx::Point GetScrollOffsetInLayersPixels() const Call it GetScrollOffsetInLayerPixels() (no "s")
Attachment #665115 - Flags: review?(roc) → review+
Comment on attachment 665117 [details] [diff] [review] Distinguish resolution from a new zoom field on FrameMetrics Review of attachment 665117 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/browser-element/BrowserElementScrolling.js @@ +205,5 @@ > _recvViewportChange: function(data) { > let metrics = data.json; > let displayPort = metrics.displayPort; > > + dump(JSON.stringify(metrics)); Remove this ::: gfx/layers/ipc/AsyncPanZoomController.cpp @@ +1112,5 @@ > } > > +void AsyncPanZoomController::SetZoomAndResolution(float aScale) { > + mFrameMetrics.mResolution.width = mFrameMetrics.mResolution.height = > + mFrameMetrics.mZoom.width = mFrameMetrics.mZoom.height = aScale; Add AssertCurrentThreadIn on the monitor.
Attachment #665117 - Flags: review?(roc) → review+
Dealt with code review comments, r+ carried.
Attachment #665115 - Attachment is obsolete: true
Attachment #665257 - Flags: review+
Rebased, r+ carried.
Attachment #665116 - Attachment is obsolete: true
Attachment #665258 - Flags: review+
Dealt with code review comments, r+ carried.
Attachment #665117 - Attachment is obsolete: true
Attachment #665260 - Flags: review+
Fixed -Werror problem, r+ carried.
Attachment #665258 - Attachment is obsolete: true
Attachment #665499 - Flags: review+
There's a reftest failure on Android M3 that I'm dealing with which is blocking me from landing this: 46 ERROR TEST-UNEXPECTED-FAIL | /tests/docshell/test/test_bug590573.html | test 8 - got 191, expected 200 Bug 593144 - debug test mochitests-2/5: Intermittent timeout in docshell/test/test_bug590573.html 48 ERROR TEST-UNEXPECTED-FAIL | /tests/docshell/test/test_bug590573.html | test 10 - got 191, expected 200 Bug 593144 - debug test mochitests-2/5: Intermittent timeout in docshell/test/test_bug590573.html https://tbpl.mozilla.org/?tree=Try&rev=3051421f164a The other failures seem to be intermittents. I'm still working on it.
I get 12 ERROR TEST-UNEXPECTED-FAIL | /tests/docshell/test/test_bug590573.html | test 8 - got 0, expected 200 13 INFO TEST-PASS | /tests/docshell/test/test_bug590573.html | test 9 - 150 should equal 150 14 ERROR TEST-UNEXPECTED-FAIL | /tests/docshell/test/test_bug590573.html | test 10 - got 0, expected 200 locally, which I guess is close enough for government work.
So even without your patch applied, I still get 12 ERROR TEST-UNEXPECTED-FAIL | /tests/docshell/test/test_bug590573.html | test 8 - got 199, expected 200 13 INFO TEST-PASS | /tests/docshell/test/test_bug590573.html | test 9 - 150 should equal 150 14 ERROR TEST-UNEXPECTED-FAIL | /tests/docshell/test/test_bug590573.html | test 10 - got 199, expected 200 I'll poke at this for a little bit but I'm very close to calling "bad test".
Not too surprising, but part 1 is at fault.
I can no longer reproduce the near-pass without your patch applied, so I'm very suspicious of results so far. Afraid I'm seeing a different intermittent failure.
Same result without these patches after a clobber, uninstall on device, reboot, and new run. Sigh.
If I make the size of the window 1 div 10000px, the bug goes away. I wonder if this has something to do with fennec autofit.
It seems pretty clear that part 1 changed a rounding computation somewhere, but it's not clear whether that's actually a problem yet. I wasn't able to spot it after looking over the patch several times. Anyway, this patch restores the original intent of the test here. https://tbpl.mozilla.org/?tree=Try&rev=6e3301701c03
Attachment #665844 - Flags: review+
This patch didn't make the failures go away, although I had brief hope with a green M3 on try.
Hm, now seeing a new and exciting result locally 12 ERROR TEST-UNEXPECTED-FAIL | /tests/docshell/test/test_bug590573.html | test 8 - got 408, expected 200 13 INFO TEST-PASS | /tests/docshell/test/test_bug590573.html | test 9 - 150 should equal 150 14 ERROR TEST-UNEXPECTED-FAIL | /tests/docshell/test/test_bug590573.html | test 10 - got 408, expected 200
I/GeckoApp(19813): Got message: Tab:ViewportMetadata I/GeckoApp(19813): Return D/GeckoFavicons(19813): Could not get URI for favicon I/GeckoApp(19813): Got message: Content:StateChange I/GeckoApp(19813): State - 786448 I/GeckoApp(19813): Got a document stop I/GeckoApp(19813): Return I/GeckoToolbar(19813): zerdatime 13772326 - Throbber stop I/GeckoScreenshot(19813): rect: 0.000000, 0.000000, 0.000000, 0.000000 I/Gecko (19813): -- ScrollFrameInner::ScrollTo -- <x=0, y=408> So what happens is, after the first round of tests runs, the frontend gets ViewportMetadata, the screenshotting stuff kicks in, and something presumably triggered by that ends up scrolling to y=408. This then becomes the shistory state, instead of 200 as the test expects. Argh!
I/Gecko (22595): -- WU::SetScrollPosClamping -- <w=980, h=1408.75> I/Gecko (22595): -- ScrollFrameInner::ScrollTo -- <x=0, y=408> I don't really know what this means anymore.
I tried browsing around in a fennec build and there are definitely some serious issues. I think we're not translating some rect into the space that the java code expects.
When it passes, I get I/Gecko (25216): -- WU::SetScrollPosClamping -- <w=980, h=1408.75> I/GeckoApp(25216): Got message: Tab:ViewportMetadata I/GeckoApp(25216): Return I/GeckoApp(25216): Got message: Content:StateChange I/GeckoApp(25216): State - 786448 I/GeckoApp(25216): Got a document stop I/GeckoToolbar(25216): zerdatime 16484206 - Throbber stop I/GeckoApp(25216): Return I/Gecko (25216): -- WU::SetScrollPosClamping -- <w=980, h=1408.75> there's no ScrollTo() call.
When the test passes, CompositorParent::SyncViewportInfo sees |aScrollOffset|s of # .scrollTop = 300 I/Gecko (28849): -- CP::SyncViewportInfo -- <x=0, y=147> # .scrollTop = 200 I/Gecko (28849): -- CP::SyncViewportInfo -- <x=0, y=98> # .scrollTop = 150 I/Gecko (28849): -- CP::SyncViewportInfo -- <x=0, y=73>
When it fails, the offsets are back in CSS pixels # .scrollTop = 200 I/Gecko (30959): -- ScrollFrameInner::ScrollTo -- <x=0, y=200> # .scrollTop = 150 I/Gecko (30959): -- ScrollFrameInner::ScrollTo -- <x=0, y=150>
Here's where things go haywire: we run through the first series of scroll()s without the frontend being involved. Then in subsequent event-loop iterations, we get I/Gecko (12660): -- CP::SetFirstPaintViewport -- offset(?) = <x=0, y=200> I/Gecko (12660): -- CP::SyncViewportInfo -- scroll = <x=0, y=0>, res = 0.489796 I/Gecko (12660): dp = <x=0, y=0, w=980, h=3136> I/Gecko (12660): got back scroll offset <x=0, y=200> I/Gecko (12660): -- WU::SetScrollPosClamping -- <w=980, h=1408.75> I/Gecko (12660): -- ScrollFrameInner::ScrollTo -- <x=0, y=408> which means that something in the frontend is returning |offset / aDisplayResolution|. Will see if there's a place to hack ...
Comment on attachment 666126 [details] [diff] [review] FOLDME INTO PART 1: Convert to device pixels a little harder. Will fold this in and land.
Attachment #666126 - Flags: review?(bugzilla) → review+
Folded device pixel conversion.
Attachment #665257 - Attachment is obsolete: true
Attachment #666126 - Attachment is obsolete: true
Attachment #666129 - Flags: review+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: