Closed Bug 937688 Opened 6 years ago Closed 6 years ago

Ensure all users of APZCTreeManager pass in fully populated ScrollableLayerGuid instances

Categories

(Core :: Panning and Zooming, defect)

All
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: kats, Assigned: kats)

References

Details

Attachments

(10 files, 10 obsolete files)

2.75 KB, patch
kats
: review+
Details | Diff | Splinter Review
2.22 KB, patch
kats
: review+
Details | Diff | Splinter Review
2.96 KB, patch
kats
: review+
Details | Diff | Splinter Review
6.96 KB, patch
kats
: review+
Details | Diff | Splinter Review
9.49 KB, patch
kats
: review+
Details | Diff | Splinter Review
5.12 KB, patch
botond
: review+
Details | Diff | Splinter Review
5.01 KB, patch
botond
: review+
Details | Diff | Splinter Review
18.96 KB, patch
kats
: review+
Details | Diff | Splinter Review
13.51 KB, patch
kats
: review+
Details | Diff | Splinter Review
6.72 KB, patch
kats
: review+
Details | Diff | Splinter Review
Currently RenderFrameParent uses APZCTreeManager but only populates some of the fields in the ScrollableLayerGuid. This is bad because messages might end up at the wrong APZC instance or get dropped entirely. We should fix that up. The win condition here I think is if we can get rid of all the constructors of ScrollableLayerGuid that use hard-coded values for any of the fields.
Assignee: nobody → bugmail.mozilla
There's still UpdateCompositionBounds and ContentReceivedTouch to deal with which are a little more complicated.
Attachment #830972 - Flags: review?(botond)
Attachment #830968 - Flags: review?(botond) → review+
Comment on attachment 830969 [details] [diff] [review]
Part 2 - Extract a helper to get the scroll identifiers

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

::: widget/xpwidgets/APZCCallbackHelper.cpp
@@ +135,5 @@
>  
> +bool
> +APZCCallbackHelper::GetScrollIdentifiers(nsIContent* aContent,
> +                                         uint32_t* aPresShellIdOut,
> +                                         FrameMetrics::ViewID* aViewIdOut)

Do we have a style guideline for when to use pointers vs. references for out parameters? For example, APZCTreeManager::GetInputTransforms() uses references, but nsLayoutUtils::FindIDFor() uses a pointer.

If we don't have an existing style guideline, I wouldn't mind following one for new code we write anyways. My preference would be to use a reference in cases where the function unconditionally writes to the out parameter (so that nullptr wouldn't be a valid input for a pointer), and a pointer in cases where the function checks for nullptr and only writes to it if it's not null. (Moreover, I'd avoid the second form unless there was a good reason to use it.)

@@ +144,5 @@
> +    nsCOMPtr<nsIDOMWindowUtils> utils = GetDOMWindowUtils(aContent);
> +    if (!utils || utils->GetPresShellId(aPresShellIdOut) != NS_OK) {
> +        return false;
> +    }
> +    return true;

The last few lines can be written as:

return utils && utils->GetPresShellId(aPresShellIdOut) == NS_OK;

(In fact, the whole thing could be written as:

nsCOMPtr<nsIDOMWindowUtils> utils;
return nsLayoutUtils::FindIDFor(aContent, aViewIdOut)
       && (utils = GetDOMWindowUtils(aContent))
       && utils->GetPresShellId(aPresShellIdOut) == NS_OK;

though I'm not sure that's a win overall.)

Up to you which style you like, of course.
Attachment #830969 - Flags: review?(botond) → review+
Comment on attachment 830970 [details] [diff] [review]
Part 3 - Extract TabChild::GetDocument()

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

::: dom/ipc/TabChild.h
@@ +451,5 @@
>  
>      // Get the DOMWindowUtils for the top-level window in this tab.
>      already_AddRefed<nsIDOMWindowUtils> GetDOMWindowUtils();
> +    // Get the Document for the top-level window in this tab.
> +    already_AddRefed<nsIDocument> GetDocument();

Alternatively, this could be a static nonmember function in TabChild.cpp, and then other .cpp files that include TabChild.h don't have to be recompiled. No strong preference about this though.
Attachment #830970 - Flags: review?(botond) → review+
Comment on attachment 830971 [details] [diff] [review]
Part 4 - Add identifiers in UpdateZoomConstraints

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

::: dom/ipc/PBrowser.ipdl
@@ +287,5 @@
>       */
>      ContentReceivedTouch(bool aPreventDefault);
>  
>      /**
>       * Updates any zoom constraints on the parent and anything tied to it. This

Can we update this comment? It doesn't sound like this function updates zoom constraints "on the parent and anything tied to it" any more.

::: dom/ipc/TabChild.cpp
@@ +534,5 @@
> +  uint32_t presShellId;
> +  ViewID viewId;
> +  if (!APZCCallbackHelper::GetScrollIdentifiers(document->GetDocumentElement(),
> +                                                &presShellId, &viewId)) {
> +    return;

Do we expect this to fail sometimes? If not, should we add a warning here?
Attachment #830971 - Flags: review?(botond) → review+
Attachment #830972 - Flags: review?(botond) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #5)
> There's still UpdateCompositionBounds and ContentReceivedTouch to deal with
> which are a little more complicated.

Note that we may be able to remove UpdateCompositionBounds now that we're updating the composition bounds in NotifyLayersUpdated (see https://bugzilla.mozilla.org/show_bug.cgi?id=912657#c10).
(In reply to Botond Ballo [:botond] from comment #6)
> Do we have a style guideline for when to use pointers vs. references for out
> parameters? For example, APZCTreeManager::GetInputTransforms() uses
> references, but nsLayoutUtils::FindIDFor() uses a pointer.

https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style says to use pointers (which is surprising to me since most mozcode I've seen uses references). I prefer using pointers because when reading code at the call site it's more obvious that the function can modify those parameters.

> If we don't have an existing style guideline, I wouldn't mind following one
> for new code we write anyways. My preference would be to use a reference in
> cases where the function unconditionally writes to the out parameter (so
> that nullptr wouldn't be a valid input for a pointer), and a pointer in
> cases where the function checks for nullptr and only writes to it if it's
> not null. (Moreover, I'd avoid the second form unless there was a good
> reason to use it.)

So nsDOMWindowUtils::GetPresShellId assumes a non-null pointer input and unconditionally writes to it. I assume that if it's ok for that code to do it then it's ok for my wrapper function to do it :) In general I do like what you stated above but it conflicts with existing and the coding style, so maybe instead of trying to figure out a globally valid rule we just have to settle for deciding on a per-use basis.

> The last few lines can be written as:
> 
> return utils && utils->GetPresShellId(aPresShellIdOut) == NS_OK;

Yeah good point, I'll update that. I don't like the overly-compressed version :)

(In reply to Botond Ballo [:botond] from comment #7)
> Alternatively, this could be a static nonmember function in TabChild.cpp,
> and then other .cpp files that include TabChild.h don't have to be
> recompiled. No strong preference about this though.

The function uses mWebNav so I can't make it static.(In reply to Botond Ballo [:botond] from comment #8)
> >      /**
> >       * Updates any zoom constraints on the parent and anything tied to it. This
> 
> Can we update this comment? It doesn't sound like this function updates zoom
> constraints "on the parent and anything tied to it" any more.

Will do.

> > +  if (!APZCCallbackHelper::GetScrollIdentifiers(document->GetDocumentElement(),
> > +                                                &presShellId, &viewId)) {
> > +    return;
> 
> Do we expect this to fail sometimes? If not, should we add a warning here?

I was debating doing that but frankly there's so much stuff getting logged that I don't think I would ever see the warning. Even if it got printed I'd probably still end up debugging the problem from scratch. I can still add the warning if you want though.

(In reply to Botond Ballo [:botond] from comment #9)
> Note that we may be able to remove UpdateCompositionBounds now that we're
> updating the composition bounds in NotifyLayersUpdated (see
> https://bugzilla.mozilla.org/show_bug.cgi?id=912657#c10).

As per our discussion we probably can't remove UpdateCompositionBounds since it is needed for "immediate" update of the zoom on rotation, whereas the NotifyLayersUpdated call comes in much later.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #10)
> (In reply to Botond Ballo [:botond] from comment #6)
> > Do we have a style guideline for when to use pointers vs. references for out
> > parameters? For example, APZCTreeManager::GetInputTransforms() uses
> > references, but nsLayoutUtils::FindIDFor() uses a pointer.
> 
> https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style says
> to use pointers (which is surprising to me since most mozcode I've seen uses
> references). I prefer using pointers because when reading code at the call
> site it's more obvious that the function can modify those parameters.
> 
> > If we don't have an existing style guideline, I wouldn't mind following one
> > for new code we write anyways. My preference would be to use a reference in
> > cases where the function unconditionally writes to the out parameter (so
> > that nullptr wouldn't be a valid input for a pointer), and a pointer in
> > cases where the function checks for nullptr and only writes to it if it's
> > not null. (Moreover, I'd avoid the second form unless there was a good
> > reason to use it.)
> 
> So nsDOMWindowUtils::GetPresShellId assumes a non-null pointer input and
> unconditionally writes to it. I assume that if it's ok for that code to do
> it then it's ok for my wrapper function to do it :) 

Definitely :)

> In general I do like
> what you stated above but it conflicts with existing and the coding style,
> so maybe instead of trying to figure out a globally valid rule we just have
> to settle for deciding on a per-use basis.

Since the coding style suggests pointers, how about we use those in new code except where it conflicts with existing practice in the file/module/similar functions? (For this patch, that would mean leaving it as it is.)

> (In reply to Botond Ballo [:botond] from comment #7)
> > Alternatively, this could be a static nonmember function in TabChild.cpp,
> > and then other .cpp files that include TabChild.h don't have to be
> > recompiled. No strong preference about this though.
> 
> The function uses mWebNav so I can't make it static.

Ah, good point. (The real solution for getting things to cause minimal recompilation is modules, which we'll get to use about 7 years from now if we're lucky :)).

> > > +  if (!APZCCallbackHelper::GetScrollIdentifiers(document->GetDocumentElement(),
> > > +                                                &presShellId, &viewId)) {
> > > +    return;
> > 
> > Do we expect this to fail sometimes? If not, should we add a warning here?
> 
> I was debating doing that but frankly there's so much stuff getting logged
> that I don't think I would ever see the warning. Even if it got printed I'd
> probably still end up debugging the problem from scratch. I can still add
> the warning if you want though.

No, that's all right.
This seemed to be the better approach since we know (for now) it will only ever get used for the root APZC in a layer tree
Attachment #831060 - Flags: review?(botond)
Comment on attachment 831060 [details] [diff] [review]
Part 6 - Get rid of ScrollableLayerGuid for UpdateCompositionBounds

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

::: gfx/layers/composite/APZCTreeManager.cpp
@@ +729,5 @@
>    return nullptr;
>  }
>  
> +AsyncPanZoomController*
> +APZCTreeManager::FindRootAPZC(AsyncPanZoomController* aApzc, const uint64_t& aLayersId) {

To be consistent with our treatment of recursive tree-walking helper functions in bug 937130, can we add an 'mTreeLock.AssertCurrentThreadOwns();' here?
Attachment #831060 - Flags: review?(botond) → review+
Rebased, carrying r+
Attachment #830968 - Attachment is obsolete: true
Attachment #831209 - Flags: review+
Updated for review comments and rebased, carrying r+
Attachment #830969 - Attachment is obsolete: true
Attachment #831210 - Flags: review+
Rebased, carrying r+
Attachment #830970 - Attachment is obsolete: true
Attachment #831212 - Flags: review+
Updated the comment, rebased. Re-requesting review for the comment change to be safe.
Attachment #830971 - Attachment is obsolete: true
Attachment #831213 - Flags: review?(botond)
Rebased, carrying r+
Attachment #830972 - Attachment is obsolete: true
Attachment #831217 - Flags: review+
Updated with comment (good catch! I didn't have the locking patch applied on my working branch so I missed the assert) and rebased. Carrying r+
Attachment #831060 - Attachment is obsolete: true
Attachment #831218 - Flags: review+
So that people don't have to include APZCTreeManager.h to get at it. It itself doesn't require including anything else (other than FrameMetrics::ViewID which it has access to) so this seems like a good move.
Attachment #831220 - Flags: review?(botond)
Comment on attachment 831213 [details] [diff] [review]
Part 4 - Add identifiers in UpdateZoomConstraints

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

::: dom/ipc/PBrowser.ipdl
@@ +287,5 @@
>       */
>      ContentReceivedTouch(bool aPreventDefault);
>  
>      /**
> +     * Updates the parent's zoom constraints for this tab. The zoom controller

I was about to suggest that we say instead "update the parent's zoom constraints for a particular scroll frame for this tab", but then I realized that right now only one scroll frame - the root one - is scrollable in a tab. Given that, can we do the same thing that we did for UpdateCompositionBounds and have the APZCTM function take just a layers id and the find the root APZC for it?
Attachment #831220 - Flags: review?(botond) → review+
Comment on attachment 831221 [details] [diff] [review]
Part 8 - Pass the ScrollableLayerGuid gotten from ReceiveInputEvent into ContentReceivedTouch

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

Looks good. Just one observation: the functions to which this patch adds a ScrollableLayerGuid* out parameter don't have a return value, so we could return the guid from them instead of using an out parameter. Up to you if you want to make this change or not.

If we decide to stick with the out parameters, could we add a comment to those functions saying that it's valid for the argument to be nullptr? Currently I had to follow the chain of calls all the way to AsyncPanZoomController::GetGuid() to determine this.
Attachment #831221 - Flags: review?(botond) → review+
(In reply to Botond Ballo [:botond] from comment #23)
> Given that, can we do the same thing that we did for
> UpdateCompositionBounds and have the APZCTM function take just a layers id
> and the find the root APZC for it?

Good point. Updated part 4 to only pass the layers id.
Attachment #831213 - Attachment is obsolete: true
Attachment #831213 - Flags: review?(botond)
Attachment #831595 - Flags: review?(botond)
(In reply to Botond Ballo [:botond] from comment #24)
> Looks good. Just one observation: the functions to which this patch adds a
> ScrollableLayerGuid* out parameter don't have a return value, so we could
> return the guid from them instead of using an out parameter. Up to you if
> you want to make this change or not.

While that's true, the APZCTreeManager::ReceiveInputEvent function does have a return value (which is currently ignored in RenderFrameParent, but may not always be). I would rather keep the out-param for consistency and in case we start using that return value in the future.

> If we decide to stick with the out parameters, could we add a comment to
> those functions saying that it's valid for the argument to be nullptr?
> Currently I had to follow the chain of calls all the way to
> AsyncPanZoomController::GetGuid() to determine this.

Yup. Annoyingly the aOutEvent parameter must be non-null. I've addd comments to RenderFrameParent.h and TabParent.h to indicate this. Eventually I'm going to get rid of aOutEvent anyway and just do the untransform in-place on aEvent so I'm not too bothered by the inconsistency here.

Carrying r+
Attachment #831598 - Flags: review+
Attachment #831595 - Flags: review?(botond) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #25)
> Created attachment 831595 [details] [diff] [review]
> Part 4 - Add identifiers in UpdateZoomConstraints (v2)
> 
> (In reply to Botond Ballo [:botond] from comment #23)
> > Given that, can we do the same thing that we did for
> > UpdateCompositionBounds and have the APZCTM function take just a layers id
> > and the find the root APZC for it?
> 
> Good point. Updated part 4 to only pass the layers id.

I just realized we can't really do this for UpdateZoomConstraints. On Metro for example different tabs can have different zoom constraints but they all have the same layers id. We should probably use a full ScrollableLayerGuid for this. Whoops.
Whiteboard: [leave open]
As you pointed out on IRC we need special behaviour for UpdateCompositionBounds as well.
Attachment #832290 - Flags: review?(botond)
Comment on attachment 832251 [details] [diff] [review]
Part 9 - Switch UpdateZoomConstraints back to using a ScrollableLayerGuid

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

::: dom/ipc/PBrowser.ipdl
@@ +288,5 @@
>       */
>      ContentReceivedTouch(ScrollableLayerGuid aGuid, bool aPreventDefault);
>  
>      /**
>       * Updates the parent's zoom constraints for this tab. The zoom controller

Now that we're passing in a presShellId/viewId pair, can we say "update the parent's zoom constraints for a scrollable frame in this tab"?

Alternatively, could we keep the original signature (i.e. not pass presShellId/viewId) at the PBrowser level, and only add them back in APZCTM? I think when we use PBrowser each tab will have its own layers id, and so we would have just one root scroll frame per layers id. (And then the comment "update zoom constraints for this tab" would be accurate.)

r=me with one of those changes.
Attachment #832251 - Flags: review?(botond) → review+
Comment on attachment 832290 [details] [diff] [review]
Part 10 - Notify all root APZCs in UpdateCompositionBounds

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

::: gfx/layers/composite/APZCTreeManager.cpp
@@ +656,5 @@
>  }
>  
> +void
> +APZCTreeManager::GetRootAPZCsFor(const uint64_t& aLayersId,
> +                                 nsTArray< nsRefPtr<AsyncPanZoomController> >* aRootApzcs)

aOutRootApzcs?
Attachment #832290 - Flags: review?(botond) → review+
(In reply to Botond Ballo [:botond] from comment #31)
> Now that we're passing in a presShellId/viewId pair, can we say "update the
> parent's zoom constraints for a scrollable frame in this tab"?
> 
> Alternatively, could we keep the original signature (i.e. not pass
> presShellId/viewId) at the PBrowser level, and only add them back in APZCTM?
> I think when we use PBrowser each tab will have its own layers id, and so we
> would have just one root scroll frame per layers id. (And then the comment
> "update zoom constraints for this tab" would be accurate.)

I'll update the comment. If we get rid of the parameters in the PBrowser interface then TabParent/RenderFrameParent will have nowhere to get them from to pass them into APZCTM.

(In reply to Botond Ballo [:botond] from comment #32)
> > +void
> > +APZCTreeManager::GetRootAPZCsFor(const uint64_t& aLayersId,
> > +                                 nsTArray< nsRefPtr<AsyncPanZoomController> >* aRootApzcs)
> 
> aOutRootApzcs?

Sure, will do.
Update as per review comment, carrying r+
Attachment #832251 - Attachment is obsolete: true
Attachment #832326 - Flags: review+
Updated as per review comments, carrying r+
Attachment #832290 - Attachment is obsolete: true
Attachment #832327 - Flags: review+
You need to log in before you can comment on or make changes to this bug.