900092 - Get rid of FrameMetrics::ROOT_SCROLL_ID

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

The ROOT_SCROLL_ID does not uniquely identify anything, because it is set on FrameMetrics that correspond to root content documents, of which there could be many. It would be better to get rid of ROOT_SCROLL_ID entirely and just use a unique id from the incrementing counter.

Comment 1

[Botond Ballo [:botond]]

I think this could be done by adding a new boolean field isRoot to FrameMetrics, and use that to carry around the information of whether or not the frame is the root scroll frame, rather than packing that information into the scroll id. Then the scroll id for the root scroll frame could be a unique integer just like it is for everything else, and we can avoid issues like https://bugzilla.mozilla.org/show_bug.cgi?id=909525#c13.

Comment 2

[Botond Ballo [:botond]]

I will implement the approach described in comment #1.

Comment 3

[Botond Ballo [:botond]]

Some of the uses of ROOT_SCROLL_ID are in the interface nsIContentView and its implementation, and related code. I have proposed removing this interface (though not its implementation for now) in bug 936690. That would make fixing this bug easier, but it's not a hard dependency.

Comment 4

[Botond Ballo [:botond]]

Attachment: Patch

First attempt. Will look at try results before flagging for review.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=3b165609a299

Comment 5

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #4)
> Created attachment 829629 [details] [diff] [review]
> Patch
> 
> First attempt. Will look at try results before flagging for review.
> 
> Try push: https://tbpl.mozilla.org/?tree=Try&rev=3b165609a299

There are some mochitest failures on metro in that try push that look like they might be caused by this patch.

Comment 6

[Botond Ballo [:botond]]

Attachment: bug900092.patch

Updated patch in accordance with some recently changes (particularly, thanks to kats' work in bug 937688, the ScrollableLayerGuid::ScrollableLayerGuid(aLayersId) constructor could finally be removed, and this patch does so).

The metro issue that was causing the failures mentioned in the previous comment is fixed by jimm's bug 937750 patch.

One more try push: https://tbpl.mozilla.org/?tree=Try&rev=f0d4d0b5c0bb

Comment 7

[Botond Ballo [:botond]]

Comment on attachment 8334109 [details] [diff] [review]
bug900092.patch

Try run looks clean. Flagging for review.

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8334109 [details] [diff] [review]
bug900092.patch

Review of attachment 8334109 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comments addressed. However I'd like tn to also review.

::: layout/base/nsDisplayList.cpp
@@ +1229,5 @@
>    root->SetPostScale(1.0f/containerParameters.mXScale,
>                       1.0f/containerParameters.mYScale);
>  
> +  ViewID id = FrameMetrics::NULL_SCROLL_ID;
> +  bool isRoot = false;

Should this be set to presContext->IsRootContentDocument() here? I think it still makes sense to have isRoot = true in that case even if the ViewID is NULL_SCROLL_ID (because we fail to find a rootScrollFrame or associated content below).

@@ +1245,5 @@
>  
> +      // If this is the root content document, we want it to have a scroll id.
> +      if (presContext->IsRootContentDocument()) {
> +        id = nsLayoutUtils::FindOrCreateIDFor(content);
> +        isRoot = true;

If we set isRoot to presContext->IsRootContentDocument() above then this condition can be simplified to if (isRoot) { id = ...; }

::: layout/ipc/RenderFrameParent.cpp
@@ +1051,5 @@
>                                           const CSSToScreenScale& aMinZoom,
>                                           const CSSToScreenScale& aMaxZoom)
>  {
> +  // TODO(botond): Fix this up.
> +  if (mContentController/* && aViewId == FrameMetrics::ROOT_SCROLL_ID*/) {

do you want to fix this before landing? You can add a aIsRoot parameter here and the call site in TabChild.cpp can just always pass in true.

Comment 9

[Botond Ballo [:botond]]

(In reply to (PTO Nov21-Dec01) Kartikaya Gupta (email:[e-mail]) from comment #8)
> Comment on attachment 8334109 [details] [diff] [review]
> bug900092.patch
> 
> Review of attachment 8334109 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with comments addressed. However I'd like tn to also review.
> 
> ::: layout/base/nsDisplayList.cpp
> @@ +1229,5 @@
> >    root->SetPostScale(1.0f/containerParameters.mXScale,
> >                       1.0f/containerParameters.mYScale);
> >  
> > +  ViewID id = FrameMetrics::NULL_SCROLL_ID;
> > +  bool isRoot = false;
> 
> Should this be set to presContext->IsRootContentDocument() here? I think it
> still makes sense to have isRoot = true in that case even if the ViewID is
> NULL_SCROLL_ID (because we fail to find a rootScrollFrame or associated
> content below).

Agreed, I'll change that.

> ::: layout/ipc/RenderFrameParent.cpp
> @@ +1051,5 @@
> >                                           const CSSToScreenScale& aMinZoom,
> >                                           const CSSToScreenScale& aMaxZoom)
> >  {
> > +  // TODO(botond): Fix this up.
> > +  if (mContentController/* && aViewId == FrameMetrics::ROOT_SCROLL_ID*/) {
> 
> do you want to fix this before landing? You can add a aIsRoot parameter here
> and the call site in TabChild.cpp can just always pass in true.

Yeah, thanks for catching that! I like your suggested fix.

Comment 10

[Botond Ballo [:botond]]

Attachment: bug900092.patch

Updated patch to address Kats' review comments.

Comment 11

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8334760 [details] [diff] [review]
bug900092.patch

Get we get a clear and precise definition of "root" into the code in at least one place? There are several concepts of root that could be confused: root scroll frame in any document (even if it is in a content iframe in a content document), root scroll frame in the root content document, and root scroll frame in the root document in the process. In this case I think it's the second one.

Comment 12

[Botond Ballo [:botond]]

Attachment: bug900092a.patch

Added comment clarifying that FrameMetrics::mIsRoot is set for the root scroll frame of the root content document, not any root scroll frame.

Carrying r+.

Comment 13

[Botond Ballo [:botond]]

Attachment: bug900092.patch

Rebased patch to apply cleanly to trunk. Carrying r+.

Comment 14

[Botond Ballo [:botond]]

https://hg.mozilla.org/integration/b2g-inbound/rev/fd925f15d8bc

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fd925f15d8bc