1291373 - [geckoview] Remove JavaPanZoomController (JPZ) from mobile/android

Status: RESOLVED FIXED

(GeckoView :: General, defect)

Description

[Nick Alexander :nalexander [he/him]]

This is step 0 for the Java changes.  It is absolutely crucial that
this land as soon as possible.  This allows to separate Tab/Tabs from
Fennec (tracked by Bug 1258470) and unblocks almost all of the other
Frontend work.

Comment 1

[Randall Barker [:rbarker]]

Attachment: 0001-Bug-1291373-geckoview-Remove-JavaPanZoomController-JPZ-from-mobile-android-16080416-2777239.patch

Work in progress. Not sure of everything that will need to be cut but this is a start.

Comment 2

[Randall Barker [:rbarker]]

Attachment: 0001-Bug-1291373-geckoview-part-1-Remove-JavaPanZoomController-JPZ-from-mobile-android-16080514-df7f0bc.patch

Updated and rebased.

Comment 3

[Randall Barker [:rbarker]]

Attachment: 0002-Bug-1291373-geckoview-part-2-Clean-up-ActionBarTextSelection-to-remove-unsupported-code-16080514-992cbaa.patch

Comment 4

[Randall Barker [:rbarker]]

Attachment: 0003-Bug-1291373-geckoview-part-3-Remove-MOZ_ANDROID_APZ-16080514-845d588.patch

Comment 5

[Randall Barker [:rbarker]]

Attachment: 0001-Bug-1291373-geckoview-part-1-Remove-JavaPanZoomController-JPZ-from-mobile-android-16080817-a7fb4f8.patch

Comment 6

[Randall Barker [:rbarker]]

Attachment: 0002-Bug-1291373-geckoview-part-2-Clean-up-ActionBarTextSelection-to-remove-unsupported-code-16080817-577269d.patch

Comment 7

[Randall Barker [:rbarker]]

Attachment: 0003-Bug-1291373-geckoview-part-3-Remove-MOZ_ANDROID_APZ-16080817-1aada14.patch

Comment 8

[Randall Barker [:rbarker]]

Attachment: 0004-Bug-1291373-geckoview-part-4-Remove-GeckoEvent-and-AndroidGeckoEvent-16080817-1e2f9d6.patch

Comment 9

[Randall Barker [:rbarker]]

Attachment: 0001-Bug-1291373-geckoview-part-1-Remove-JavaPanZoomController-JPZ-from-mobile-android-r-snorp-16081113-52ef158.patch

Comment 10

[Randall Barker [:rbarker]]

Attachment: 0002-Bug-1291373-geckoview-part-2-Clean-up-ActionBarTextSelection-to-remove-unsupported-code-r-snorp-16081113-00ca943.patch

Comment 11

[Randall Barker [:rbarker]]

Attachment: 0003-Bug-1291373-geckoview-part-3-Remove-MOZ_ANDROID_APZ-r-botond-snorp-16081113-ae74550.patch

Comment 12

[Randall Barker [:rbarker]]

Attachment: 0004-Bug-1291373-geckoview-part-4-Remove-GeckoEvent-and-AndroidGeckoEvent-r-jchen-16081113-ef81d33.patch

Comment 13

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8780259 [details] [diff] [review]
0002-Bug-1291373-geckoview-part-2-Clean-up-ActionBarTextSelection-to-remove-unsupported-code-r-snorp-16081113-00ca943.patch

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

Looks good to me, but I really don't know that code. Mike can you look?

Comment 14

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8780260 [details] [diff] [review]
0003-Bug-1291373-geckoview-part-3-Remove-MOZ_ANDROID_APZ-r-botond-snorp-16081113-ae74550.patch

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

Woo!

Comment 15

[Botond Ballo [:botond]]

Comment on attachment 8780260 [details] [diff] [review]
0003-Bug-1291373-geckoview-part-3-Remove-MOZ_ANDROID_APZ-r-botond-snorp-16081113-ae74550.patch

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

I assume the timing of the JPZ removal (riding the trains in 51) has been discussed with :kats.

Codewise, r=me.

::: mobile/android/chrome/content/browser.js
@@ +3788,5 @@
>          setScrollPositionClampingScrollPortSize(scrollPortWidth, scrollPortHeight);
>    },
>  
>    setViewport: function(aViewport) {
> +    return;

Consider removing these functions altogether as a follow-up?

@@ -4696,5 @@
>      Services.obs.addObserver(this, "Gesture:SingleTap", false);
>      Services.obs.addObserver(this, "Gesture:ClickInZoomedView", false);
>  
> -    if (!AppConstants.MOZ_ANDROID_APZ) {
> -      Services.obs.addObserver(this, "Gesture:CancelTouch", false);

Remove the corresponding cases in handleUserEvent() (and functions they call if those functions have no other callers) as a follow-up?

::: widget/android/nsWindow.cpp
@@ -3715,5 @@
> -                         "  \"maxZoom\": %f }",
> -        aConstraints->mAllowZoom ? "true" : "false",
> -        aConstraints->mAllowDoubleTapZoom ? "true" : "false",
> -        aConstraints->mMinZoom.scale, aConstraints->mMaxZoom.scale);
> -    obsServ->NotifyObservers(doc, "zoom-constraints-updated",

Whatever handles this can probably be removed too. (Feel free to leave all these suggestions to a follow-up bug.)

Comment 16

[Randall Barker [:rbarker]]

(In reply to Botond Ballo [:botond] from comment #15)
> Comment on attachment 8780260 [details] [diff] [review]
> 0003-Bug-1291373-geckoview-part-3-Remove-MOZ_ANDROID_APZ-r-botond-snorp-
> 16081113-ae74550.patch
> 
> Review of attachment 8780260 [details] [diff] [review]:
> -----------------------------------------------------------------
... 
> Whatever handles this can probably be removed too. (Feel free to leave all
> these suggestions to a follow-up bug.)

I'll just add a patch to this bug that cleans all this up since I'm going to wait for :kats to get back to give him the option to look over these patches.

Comment 17

[Randall Barker [:rbarker]]

Attachment: 0005-Bug-1291373-geckoview-part-5-Remove-Layer-and-all-derived-classes-r-snorp-16081215-ea37ade.patch

Feel free to reassign review.

Comment 18

[Randall Barker [:rbarker]]

Attachment: 0006-Bug-1291373-geckoview-part-6-Remove-unused-functions-from-browser.js-r-snorp-16081215-df9008a.patch

Comment 19

[Randall Barker [:rbarker]]

Attachment: 0006-Bug-1291373-geckoview-part-6-Remove-unused-functions-from-browser.js-r-snorp-16081215-34515ac.patch

Removed more code.

Comment 20

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

(In reply to Botond Ballo [:botond] from comment #15)
> I assume the timing of the JPZ removal (riding the trains in 51) has been
> discussed with :kats.

I don't think I've said this explicitly yet but I'm happy to remove it in 51.

Comment 21

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8780729 [details] [diff] [review]
0005-Bug-1291373-geckoview-part-5-Remove-Layer-and-all-derived-classes-r-snorp-16081215-ea37ade.patch

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

::: gfx/layers/opengl/CompositorOGL.cpp
@@ +684,5 @@
>  
>    mPixelsPerFrame = width * height;
>    mPixelsFilled = 0;
>  
> +#ifdef MOZ_WIDGET_ANDROID

Whoops.

Comment 22

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8780732 [details] [diff] [review]
0006-Bug-1291373-geckoview-part-6-Remove-unused-functions-from-browser.js-r-snorp-16081215-34515ac.patch

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

Big improvement

Comment 23

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8780261 [details] [diff] [review]
0004-Bug-1291373-geckoview-part-4-Remove-GeckoEvent-and-AndroidGeckoEvent-r-jchen-16081113-ef81d33.patch

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

::: widget/android/AndroidBridge.h
@@ +527,5 @@
>  }
>  
>  #define NS_ANDROIDBRIDGE_CID \
> +{ 0x1989da17, 0x50be, 0x425a, \
> +  { 0x9e, 0x66, 0xd6, 0xbb, 0xc8, 0xf5, 0x29, 0x8c } }

Don't need to change CID.

::: widget/android/nsAppShell.cpp
@@ -628,5 @@
>      }
>      return nullptr;
>  }
>  
> -class nsAppShell::LegacyGeckoEvent : public Event

Remove LegacyGeckoEvent in nsAppShell.h as well

::: widget/android/nsIAndroidBridge.idl
@@ +31,5 @@
>    nsIBrowserTab getBrowserTab(in int32_t tabId);
>    nsIUITelemetryObserver getUITelemetryObserver();
>  };
>  
> +[scriptable, uuid(d01b89bc-15d5-4988-86ba-1ca18abd9ffd)]

We don't bump IIDs anymore.

Comment 24

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #13)
> Looks good to me, but I really don't know that code. Mike can you look?

fwiw, I don't think anyone currently developing fennec knows this code (I've only made checkstyle changes in this file) but I'll do my best to check for any issues.

Comment 25

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8780259 [details] [diff] [review]
0002-Bug-1291373-geckoview-part-2-Clean-up-ActionBarTextSelection-to-remove-unsupported-code-r-snorp-16081113-00ca943.patch

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

I don't know enough of the underlying code or what the effects of removing the JavaPanZoomController should be to properly review this code. That being said, the changes seem consistent and I see no red flags.

Sorry for blocking.

::: mobile/android/base/java/org/mozilla/gecko/ActionBarTextSelection.java
@@ +9,4 @@
>  import org.mozilla.gecko.menu.GeckoMenu;
>  import org.mozilla.gecko.menu.GeckoMenuItem;
>  import org.mozilla.gecko.text.TextSelection;
> +//import org.mozilla.gecko.util.FloatUtils;

nit: Just rm it

Comment 26

[Randall Barker [:rbarker]]

(In reply to Michael Comella (:mcomella) [not actively working on fennec: expect slow responses] from comment #25)
> Comment on attachment 8780259 [details] [diff] [review]
> 0002-Bug-1291373-geckoview-part-2-Clean-up-ActionBarTextSelection-to-remove-
> unsupported-code-r-snorp-16081113-00ca943.patch
> 
> Review of attachment 8780259 [details] [diff] [review]:
> ----------------------------------------------------------------- 
> ::: mobile/android/base/java/org/mozilla/gecko/ActionBarTextSelection.java
> @@ +9,4 @@
> >  import org.mozilla.gecko.menu.GeckoMenu;
> >  import org.mozilla.gecko.menu.GeckoMenuItem;
> >  import org.mozilla.gecko.text.TextSelection;
> > +//import org.mozilla.gecko.util.FloatUtils;
> 
> nit: Just rm it

Sorry, I caught that after I uploaded the patch but didn't upload the fix patch.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/268f3aea336d
[geckoview] part 1, Remove JavaPanZoomController (JPZ) from mobile/android r=snorp
https://hg.mozilla.org/integration/mozilla-inbound/rev/19222e86fb30
[geckoview] part 2, Clean up ActionBarTextSelection to remove unsupported code r=snorp,mcomella
https://hg.mozilla.org/integration/mozilla-inbound/rev/2986bfd73d46
[geckoview] part 3, Remove MOZ_ANDROID_APZ r=botond,snorp
https://hg.mozilla.org/integration/mozilla-inbound/rev/2fc2a6e9b9fc
[geckoview] part 4, Remove GeckoEvent and AndroidGeckoEvent r=jchen
https://hg.mozilla.org/integration/mozilla-inbound/rev/e25807db3f89
[geckoview] part 5, Remove Layer and all derived classes r=snorp
https://hg.mozilla.org/integration/mozilla-inbound/rev/98a1df2388af
[geckoview] part 6, Remove unused functions from browser.js r=snorp

Comment 28

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

https://hg.mozilla.org/mozilla-central/rev/268f3aea336d
https://hg.mozilla.org/mozilla-central/rev/19222e86fb30
https://hg.mozilla.org/mozilla-central/rev/2986bfd73d46
https://hg.mozilla.org/mozilla-central/rev/2fc2a6e9b9fc
https://hg.mozilla.org/mozilla-central/rev/e25807db3f89
https://hg.mozilla.org/mozilla-central/rev/98a1df2388af