1056159 - APZCTreeManager::UpdatePanZoomControllerTree has too many arguments

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

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

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.

Comment 1

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

Attachment: Part 1 - Add the TreeBuildingState helper struct

Comment 2

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

Attachment: Part 2 - Extract a helper function

Comment 3

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

Attachment: Part 2 (diff ignoring whitespace)

Comment 4

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

Try push including these patches: https://tbpl.mozilla.org/?tree=Try&rev=ca10ab9d4739

Comment 5

[Botond Ballo [:botond]]

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.

Comment 6

[Botond Ballo [:botond]]

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

Comment 7

[Botond Ballo [:botond]]

Comment on attachment 8475971 [details] [diff] [review]
Part 2 - Extract a helper function

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

See ignoring-whitespace patch.

Comment 8

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

Updated per comments above.

remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/a84e852880b9
remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/1f79d92c1c7c

Comment 9

[Botond Ballo [: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.

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a84e852880b9
https://hg.mozilla.org/mozilla-central/rev/1f79d92c1c7c