APZCTreeManager::UpdatePanZoomControllerTree has too many arguments

RESOLVED FIXED in mozilla34

Status

()

Core
Panning and Zooming
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: kats, Assigned: kats)

Tracking

unspecified
mozilla34
All
Gonk (Firefox OS)
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

See http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/APZCTreeManager.cpp?rev=413e0473a1d8#190. It's getting a little out of hand. As part of my work for bug 1055760 I cleaned this up a bit. I'd like to land the refactoring even if we don't end up doing bug 1055760 so I'm splitting it into a separate bug.
Created attachment 8475970 [details] [diff] [review]
Part 1 - Add the TreeBuildingState helper struct
Attachment #8475970 - Flags: review?(botond)
Created attachment 8475971 [details] [diff] [review]
Part 2 - Extract a helper function
Attachment #8475971 - Flags: review?(botond)
Created attachment 8475975 [details] [diff] [review]
Part 2 (diff ignoring whitespace)
(Assignee)

Comment 4

4 years ago
try
Try push including these patches: https://tbpl.mozilla.org/?tree=Try&rev=ca10ab9d4739
Comment on attachment 8475970 [details] [diff] [review]
Part 1 - Add the TreeBuildingState helper struct

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

::: gfx/layers/apz/src/APZCTreeManager.cpp
@@ +52,5 @@
> +  const bool mIsFirstPaint;
> +  const uint64_t mOriginatingLayersId;
> +
> +  // State that is updated as we perform the tree build
> +  APZPaintLogHelper mPaintLogger;

The LogTestData() methods of APZPaintLogHelper are 'const', so this field can be made const and put into the "state that doesn't change" category.
Attachment #8475970 - Flags: review?(botond) → review+
Comment on attachment 8475975 [details] [diff] [review]
Part 2 (diff ignoring whitespace)

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

::: gfx/layers/apz/src/APZCTreeManager.h
@@ +366,5 @@
>                                          const ZoomConstraints& aConstraints);
>  
> +  AsyncPanZoomController* PrepareAPZCForLayer(const Layer* aLayer,
> +                                              const FrameMetrics& aMetrics,
> +                                              const uint64_t& aLayersId,

Let's remain consistent with UPZCT in passing uint64_t by value.

@@ +370,5 @@
> +                                              const uint64_t& aLayersId,
> +                                              const gfx::Matrix4x4& aAncestorTransform,
> +                                              const nsIntRegion& aObscured,
> +                                              AsyncPanZoomController*& aParent,
> +                                              AsyncPanZoomController*& aNextSibling,

aOutParent, aOutNextSibling
Attachment #8475975 - Flags: review+
Comment on attachment 8475971 [details] [diff] [review]
Part 2 - Extract a helper function

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

See ignoring-whitespace patch.
Attachment #8475971 - Flags: review?(botond)
(In reply to Botond Ballo [:botond] from comment #6)
> @@ +370,5 @@
> > +                                              const uint64_t& aLayersId,
> > +                                              const gfx::Matrix4x4& aAncestorTransform,
> > +                                              const nsIntRegion& aObscured,
> > +                                              AsyncPanZoomController*& aParent,
> > +                                              AsyncPanZoomController*& aNextSibling,
> 
> aOutParent, aOutNextSibling

On closer look, we don't actually use aOutNextSibling as an 'out' parameter. We could have just passed the pointer by value.
https://hg.mozilla.org/mozilla-central/rev/a84e852880b9
https://hg.mozilla.org/mozilla-central/rev/1f79d92c1c7c
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.