809199 - Eliminate ViewportMetrics.java in favor of ImmutableViewportMetrics

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Toolbar, defect)

Description

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

Attachment: Part 1 - Move interpolation code to ImmutableViewportMetrics

Since we added ImmutableViewportMetrics, the old ViewportMetrics class just sort of gets in the way. It's mutable so harder to reason about, and creates a bunch of internal RectF objects which adds to java heap stress. I'd like to get rid of the old ViewportMetrics and switch all of the code over to using ImmutableViewportMetrics.

The patches do the job in pieces for (hopefully) easier reviewing. Intermediate patches may introduce some "new ViewportMetrics(immutableVersion)" and "new ImmutableViewportMetrics(mutableVersion)" calls but these temporary conversions get taken out in subsequent patches as more of the code gets converted.

Comment 1

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

Attachment: Part 2 - Modify PanZoomTarget methods

Comment 2

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

Attachment: Part 3 - Add an offsetViewportBy function

Comment 3

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

Attachment: Part 4 - Remove ViewportMetrics from PZC

Comment 4

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

Attachment: Part 5 - Update createViewportEvent

Comment 5

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

Attachment: Part 6 - Switch mGeckoViewport to be an IVM

Comment 6

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

Attachment: Part 7 - Update JNI code

Comment 7

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

Attachment: Part 8 - Delete ViewportMetrics.java

Comment 8

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

Also, try run is green: https://tbpl.mozilla.org/?tree=Try&rev=54f62c8017da

Comment 9

[Chris Lord [:cwiiis]]

Comment on attachment 678881 [details] [diff] [review]
Part 1 - Move interpolation code to ImmutableViewportMetrics

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

Looks fine to me.

Comment 10

[Chris Lord [:cwiiis]]

Comment on attachment 678883 [details] [diff] [review]
Part 2 - Modify PanZoomTarget methods

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

Looks good to me.

::: mobile/android/base/ui/PanZoomController.java
@@ +259,5 @@
>      public void pageRectUpdated() {
>          if (mState == PanZoomState.NOTHING) {
>              synchronized (mTarget.getLock()) {
> +                ImmutableViewportMetrics validated = getValidViewportMetrics();
> +                if (! getMetrics().fuzzyEquals(validated)) {

Do we normally put spaces after the ! ? Might be a good opportunity to get rid of it.

Comment 11

[Chris Lord [:cwiiis]]

Comment on attachment 678884 [details] [diff] [review]
Part 3 - Add an offsetViewportBy function

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

LGTM.

::: mobile/android/base/ui/PanZoomController.java
@@ +486,5 @@
>      }
>  
>      private void scrollBy(PointF point) {
> +        ImmutableViewportMetrics scrolled = getMetrics().offsetViewportBy(point);
> +        mTarget.setViewportMetrics(scrolled);

This is much nicer... Not sure why we didn't add an offset convenience function before...

Comment 12

[Chris Lord [:cwiiis]]

Comment on attachment 678885 [details] [diff] [review]
Part 4 - Remove ViewportMetrics from PZC

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

Fine, but would like to see comments addressed if they aren't already in a later patch.

::: mobile/android/base/gfx/ImmutableViewportMetrics.java
@@ +154,2 @@
>      public ImmutableViewportMetrics offsetViewportBy(PointF delta) {
> +        return setViewportOrigin(new PointF(viewportRectLeft + delta.x, viewportRectTop + delta.y));

Wasn't part of the point of this to stop creating objects so much... But you're replacing the code you added here with a new PointF?

@@ +178,5 @@
> +            origin.x, origin.y, origin.x + getWidth(), origin.y + getHeight(),
> +            newZoomFactor);
> +    }
> +
> +    /** Clamps the viewport to the page size. */

nit, would rather this said something along the lines of "Clamps the viewport to reamain within the page rect"

::: mobile/android/base/ui/PanZoomController.java
@@ +727,3 @@
>      }
>  
> +    private ImmutableViewportMetrics getValidViewportMetrics(ImmutableViewportMetrics viewportMetrics) {

This now creates two sets of ViewportMetrics. It would be nice to be able to do this with just creating the returned metrics.

@@ +1036,5 @@
> +        ImmutableViewportMetrics finalMetrics = getMetrics();
> +        finalMetrics = finalMetrics.setViewportOrigin(
> +                new PointF(zoomToRect.left * finalMetrics.zoomFactor,
> +                           zoomToRect.top * finalMetrics.zoomFactor));
> +        finalMetrics = finalMetrics.scaleTo(finalZoom, new PointF(0.0f, 0.0f));

Same here.

Comment 13

[Chris Lord [:cwiiis]]

Comment on attachment 678886 [details] [diff] [review]
Part 5 - Update createViewportEvent

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

Yup.

Comment 14

[Chris Lord [:cwiiis]]

Comment on attachment 678887 [details] [diff] [review]
Part 6 - Switch mGeckoViewport to be an IVM

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

mmhmm.

Comment 15

[Chris Lord [:cwiiis]]

Comment on attachment 678888 [details] [diff] [review]
Part 7 - Update JNI code

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

I assume that after all these patches, the methods line up for the JNI change here to be correct.

Comment 16

[Chris Lord [:cwiiis]]

Comment on attachment 678890 [details] [diff] [review]
Part 8 - Delete ViewportMetrics.java

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

Great :)

My last comment, the set* functions are a little confusingly named now as they don't set anything... But then, the class has 'Immutable' in the name, so I guess that ought to be obvious. Of course, now we only have ImmutableViewportMetrics, you could argue it should be renamed to ViewportMetrics, but I think that argument would, on the whole, be wrong.

It's a shame that there is still an amount of copying going on here, but it's likely a lot less than before, so great! The code reads much nicer now too, so well done :)

Comment 17

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

(In reply to Chris Lord [:cwiiis] from comment #10)
> Do we normally put spaces after the ! ? Might be a good opportunity to get
> rid of it.

Fixed. That space was probably my fault - it's my non-Mozilla coding style leaking through :)

(In reply to Chris Lord [:cwiiis] from comment #12)
> Wasn't part of the point of this to stop creating objects so much... But
> you're replacing the code you added here with a new PointF?

Good point (pun!). I've modified the setViewportOrigin method to now take two floats instead of a PointF, which eliminates the unnecessary PointF object creation from both places it is called.

> > +    /** Clamps the viewport to the page size. */
> 
> nit, would rather this said something along the lines of "Clamps the
> viewport to reamain within the page rect"

Done.

> > +    private ImmutableViewportMetrics getValidViewportMetrics(ImmutableViewportMetrics viewportMetrics) {
> 
> This now creates two sets of ViewportMetrics. It would be nice to be able to
> do this with just creating the returned metrics.
> 
> @@ +1036,5 @@
> > +        ImmutableViewportMetrics finalMetrics = getMetrics();
> > +        finalMetrics = finalMetrics.setViewportOrigin(
> > +                new PointF(zoomToRect.left * finalMetrics.zoomFactor,
> > +                           zoomToRect.top * finalMetrics.zoomFactor));
> > +        finalMetrics = finalMetrics.scaleTo(finalZoom, new PointF(0.0f, 0.0f));
> 
> Same here.

Agreed. I did think about that but it seemed like it would be pretty ugly since I would need a method on IVM that does both transformations, so I figured I would leave it like this for now and have another patch at the end that tries to remove as many of these extra object creations as possible. As it is this version creates fewer objects than the old one; they just happen to be IVMs instead of RectF and PointFs.

Comment 18

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

Attachment: Part 9 - Update setViewportSize to not use PointFs

Comment 19

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

Attachment: Part 10 - Update offsetViewportBy to not take a PointF

I would argue that this even makes better sense semantically since a PointF shouldn't be used to carry around a delta :)

I can see a few more changes like these but a lot of the remaining functions are used throughout LayerRenderer, and I would like to clean up that code before tackling them, so I'll do that in a separate bug.

Comment 20

[Chris Lord [:cwiiis]]

Comment on attachment 679150 [details] [diff] [review]
Part 9 - Update setViewportSize to not use PointFs

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

Great :)

Comment 21

[Chris Lord [:cwiiis]]

Comment on attachment 679151 [details] [diff] [review]
Part 10 - Update offsetViewportBy to not take a PointF

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

Great!

Comment 22

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/2dec880e1ca3
https://hg.mozilla.org/integration/mozilla-inbound/rev/13d206cabd9a
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ef3e0bf0fe6
https://hg.mozilla.org/integration/mozilla-inbound/rev/d25bb9a159b9
https://hg.mozilla.org/integration/mozilla-inbound/rev/cfcde7d4bb32
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ba6780ecdb5
https://hg.mozilla.org/integration/mozilla-inbound/rev/548c33b6cc08
https://hg.mozilla.org/integration/mozilla-inbound/rev/a571a90b645b
https://hg.mozilla.org/integration/mozilla-inbound/rev/e21daa6b90b7
https://hg.mozilla.org/integration/mozilla-inbound/rev/d6a48ba051bc

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2dec880e1ca3
https://hg.mozilla.org/mozilla-central/rev/13d206cabd9a
https://hg.mozilla.org/mozilla-central/rev/7ef3e0bf0fe6
https://hg.mozilla.org/mozilla-central/rev/d25bb9a159b9
https://hg.mozilla.org/mozilla-central/rev/cfcde7d4bb32
https://hg.mozilla.org/mozilla-central/rev/1ba6780ecdb5
https://hg.mozilla.org/mozilla-central/rev/548c33b6cc08
https://hg.mozilla.org/mozilla-central/rev/a571a90b645b
https://hg.mozilla.org/mozilla-central/rev/e21daa6b90b7
https://hg.mozilla.org/mozilla-central/rev/d6a48ba051bc

Comment 24

[Aaron Train [:aaronmt]]

This has triggered a major regression in ability to scroll pages; see bug 810933. Recommending back-out here.

Comment 25

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

Pushed a backout try to make sure it still builds and nothing else depends on it: https://tbpl.mozilla.org/?tree=Try&rev=c32396a03323. If it's green I'll push to inbound. Looking into why this breakage happened at all.

Comment 26

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

(In reply to Kartikaya Gupta (:kats) from comment #25)
> Pushed a backout try to make sure it still builds and nothing else depends
> on it: https://tbpl.mozilla.org/?tree=Try&rev=c32396a03323. If it's green
> I'll push to inbound. Looking into why this breakage happened at all.

I found the problem and have a patch on bug 801933. Will leave this closed and won't back it out.