777075 - Extract an interface for the PanZoomController -> LayerController dependency

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Toolbar, defect)

Description

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

As long as PZC depends directly on LayerController it can't be moved to native code

Comment 1

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

Attachment: Part 1 - Refactor PZC to consolidate code that accesses viewport metrics

Comment 2

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

Attachment: Part 2 - Extract a ZoomConstraints class

Comment 3

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

Attachment: Part 3 - Eliminate getViewport and getZoomFactor

Comment 4

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

Attachment: Part 4 - Extract a PanZoomTarget interface

Comment 5

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

Attachment: Part 5 - Move scrollBy and scaleWithFocus into PZC

Comment 6

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

Attachment: Part 6 - Add a getMutableMetrics helper

Comment 7

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

Attachment: Part 7 - Remove notifyLayerClientOfGeometryChange from the PanZoomTarget interface

Comment 8

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

Comment on attachment 645503 [details] [diff] [review]
Part 1 - Refactor PZC to consolidate code that accesses viewport metrics

mbrubeck, requesting review from you for this patch set since you worked on the zoom-related functions in part 2. The rest of the patches could be reviewed by anybody really since it's just moving code around without making functional changes. Feel free to decline if you don't want to review it.

Comment 9

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

Comment on attachment 645505 [details] [diff] [review]
Part 3 - Eliminate getViewport and getZoomFactor

This one actually has a small correctness fix as well - the change to LayerRenderer uses mFrameMetrics instead of the LayerController's viewport metrics, which is correct from a threading point of view (this code runs on the compositor thread which is supposed to use mFrameMetrics)

Comment 10

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

Comment on attachment 645760 [details] [diff] [review]
Part 7 - Remove notifyLayerClientOfGeometryChange from the PanZoomTarget interface

This one has a side-effect of forcing a redraw after gecko loads, which is probably unnecessary but also pretty harmless.

Comment 11

[Matt Brubeck (:mbrubeck)]

Comment on attachment 645758 [details] [diff] [review]
Part 5 - Move scrollBy and scaleWithFocus into PZC

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

::: mobile/android/base/gfx/LayerController.java
@@ -140,5 @@
> -        viewportMetrics.setOrigin(origin);
> -        mViewportMetrics = new ImmutableViewportMetrics(viewportMetrics);
> -
> -        notifyLayerClientOfGeometryChange();
> -        mView.requestRender();

Just as a note to myself: In the new code, setViewportMetrics is responsible for calling mView.requestRender().

Comment 12

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/8aeda525c094
https://hg.mozilla.org/integration/mozilla-inbound/rev/b52dc1df2fde
https://hg.mozilla.org/integration/mozilla-inbound/rev/65393fe32cce
https://hg.mozilla.org/integration/mozilla-inbound/rev/d10df9bfef60
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b194a7f47d4
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f1fc980f1f1
https://hg.mozilla.org/integration/mozilla-inbound/rev/abc5b9a922dc

Comment 13

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

(Landed these involved some rebasing since the code has changed a bunch since I wrote the patches, but there wasn't anything too significant)

Comment 14

[Ed Morley [:emorley]]

Backed out for native Android R1 failures, eg:
https://tbpl.mozilla.org/php/getParsedLog.php?id=14039325&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=14040037&tree=Mozilla-Inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/24e458030842

Comment 15

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

The R1 failures were apparently caused by the fact that my new patches change the initial value of Tab.getAllowZoom() from false to true. Previously the mAllowZoom was uninitialized in Tab, and java defaulted it to false. In my patches I created a new ZoomConstraints which had it defaulted to true, and that change in behaviour caused the zoom calculations to be wrong, or so it would appear. I'm not entirely sure why that happened, since I thought that the zoom constraints should always be getting set before we use them, but I guess not. I added this additional change: https://hg.mozilla.org/try/rev/77b37e4283e4 and it fixed the R1 failures (try run at https://tbpl.mozilla.org/?tree=Try&rev=06b1f3484b9c). Will rebase and land.

Comment 16

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

Rebased patches landed on inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/2cf97e254d95
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ac3296d73a1
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e86f20539bb
https://hg.mozilla.org/integration/mozilla-inbound/rev/e3e51a7d8f08
https://hg.mozilla.org/integration/mozilla-inbound/rev/a0cc76df8f3b
https://hg.mozilla.org/integration/mozilla-inbound/rev/c7501863b612
https://hg.mozilla.org/integration/mozilla-inbound/rev/f8f959a0eb5f

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2cf97e254d95
https://hg.mozilla.org/mozilla-central/rev/2ac3296d73a1
https://hg.mozilla.org/mozilla-central/rev/0e86f20539bb
https://hg.mozilla.org/mozilla-central/rev/e3e51a7d8f08
https://hg.mozilla.org/mozilla-central/rev/a0cc76df8f3b
https://hg.mozilla.org/mozilla-central/rev/c7501863b612
https://hg.mozilla.org/mozilla-central/rev/f8f959a0eb5f