1258450 - [geckoview] Update GeckoView classycle definitions

Status: RESOLVED FIXED

(Core Graveyard :: Embedding: GRE Core, defect)

Description

[Nick Alexander :nalexander [he/him]]

This ticket tracks updating the GeckoView classycle definitions to reflect more than a year of development.

In addition, I'm going to use this ticket to track some simple manipulations to reduce the compile-time dependency of GeckoView on Fennec.

Comment 1

[Nick Alexander :nalexander [he/him]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=75f8ed742f56

Comment 2

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 1258450 - Update classycle definitions. r=me

To use this, uncomment the line in geckoview.ddf.  Then, after a
build, run

./mach gradle jarLocalDebugClasses

and then

java -cp mobile/android/build/classycle/classycle-1.4.1.jar classycle.dependency.DependencyChecker -mergeInnerClasses -dependencies=@mobile/android/base/geckoview.ddf $OBJDIR/gradle/build/mobile/android/app/intermediates/packaged/local/debug/classes.jar

Review commit: https://reviewboard.mozilla.org/r/41463/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/41463/

Comment 3

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 1258450 - Don't use resources in SwipeDismissListViewTouchListener. r?mcomella

This is just a small simplification to allow us to not depend on
org.mozilla.gecko.R.

Review commit: https://reviewboard.mozilla.org/r/41465/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/41465/

Comment 4

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 1258450 - Remove unused GeckoInterface.getPromptService(). r?snorp

Review commit: https://reviewboard.mozilla.org/r/41467/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/41467/

Comment 5

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 1258450 - Add GeckoInterface.createShortcut. r?snorp

This moves some Fennec-specific home-screen icon manipulations out of
GeckoAppShell.  A GeckoView interface can follow.

Review commit: https://reviewboard.mozilla.org/r/41469/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/41469/

Comment 6

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 1258450 - Move GlobalHistory queries to GeckoInterface. r?snorp

GlobalHistory is Fennec-specific: it accesses the Fennec history data
store, and collects Telemetry.  This allows other consumers to
implement their own store as appropriate.

Review commit: https://reviewboard.mozilla.org/r/41471/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/41471/

Comment 7

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 1258450 - Move references to SnackbarHelper out of GeckoAppShell. r?snorp,sebastian

The existing code assumes an Activity, not just a Context, but doesn't
statically guarantee it.  This patch is safe because it dynamically
type-checks, but it would be better to declare the member to be an
Activity.

Review commit: https://reviewboard.mozilla.org/r/41473/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/41473/

Comment 8

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 1258450 - Route setAccessibilityEnabled through GeckoInterface. r?snorp,jchen

Review commit: https://reviewboard.mozilla.org/r/41475/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/41475/

Comment 9

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 1258450 - Automated (mostly) refactorings moving GeckoAppShell static methods into IntentHelper. r?snorp

Review commit: https://reviewboard.mozilla.org/r/41477/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/41477/

Comment 10

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 1258450 - Delegate Intent handling from GeckoAppShell to GeckoInterface. r?snorp,mcomella

Review commit: https://reviewboard.mozilla.org/r/41479/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/41479/

Comment 11

[Nick Alexander :nalexander [he/him]]

snorp, I flagged you for review on all patches because I want to have continuity across all these little changes.

After these refactorings, I get:

nalexander@chocho ~/M/gecko> java -cp mobile/android/build/classycle/classycle-1.4.1.jar classycle.dependency.DependencyChecker -mergeInnerClasses -dependencies=@mobile/android/base/geckoview.ddf objdir-droid/gradle/build/mobile/android/app/intermediates/packaged/local/debug/classes.jar
show onlyShortestPaths allResults
Set [lib] has 133 classes.
Set [app] has 715 classes.
check [lib] independentOf [app]
  Unexpected dependencies found:
  org.mozilla.gecko.gfx.BitmapUtils
    -> org.mozilla.gecko.R
    -> org.mozilla.gecko.Tab
    -> org.mozilla.gecko.Tabs
    -> org.mozilla.gecko.ThumbnailHelper
  org.mozilla.gecko.GeckoProfile
    -> org.mozilla.gecko.GeckoApp
    -> org.mozilla.gecko.db.BrowserDB
    -> org.mozilla.gecko.distribution.Distribution
    -> org.mozilla.gecko.GuestSession
    -> org.mozilla.gecko.db.StubBrowserDB
    -> org.mozilla.gecko.TelemetryContract
    -> org.mozilla.gecko.Telemetry
    -> org.mozilla.gecko.db.LocalBrowserDB
    -> org.mozilla.gecko.preferences.DistroSharedPrefsImport
  org.mozilla.gecko.GeckoView
    -> org.mozilla.gecko.GeckoActivity
    -> org.mozilla.gecko.Tab
    -> org.mozilla.gecko.Tabs
    -> org.mozilla.gecko.R
  org.mozilla.gecko.gfx.LayerView
    -> org.mozilla.gecko.Tabs
    -> org.mozilla.gecko.Tab
  org.mozilla.gecko.gfx.JavaPanZoomController
    -> org.mozilla.gecko.Tab
    -> org.mozilla.gecko.Tabs
  org.mozilla.gecko.gfx.GeckoLayerClient
    -> org.mozilla.gecko.Tab
    -> org.mozilla.gecko.Tabs
  org.mozilla.gecko.gfx.TouchEventHandler
    -> org.mozilla.gecko.Tabs
    -> org.mozilla.gecko.Tab
  org.mozilla.gecko.gfx.LayerRenderer
    -> org.mozilla.gecko.Tabs
    -> org.mozilla.gecko.Tab

This is remarkably close to being separated.  More tickets to follow!

Comment 12

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

Comment on attachment 8732954 [details]
MozReview Request: Bug 1258450 - Route setAccessibilityEnabled through GeckoInterface. r?snorp,jchen

https://reviewboard.mozilla.org/r/41475/#review37947

Comment 13

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8732953 [details]
MozReview Request: Bug 1258450 - Move references to SnackbarHelper out of GeckoAppShell. r?snorp,sebastian

https://reviewboard.mozilla.org/r/41473/#review38119

::: mobile/android/base/java/org/mozilla/gecko/widget/GeckoActionProvider.java:302
(Diff revision 1)
> +            // We should be, but currently aren't, statically guaranteed an Activity context.
> +            // Try our best.
> +            if (mContext instanceof Activity) {

Technically this is always an activity - An ActionProvider only makes sense in the context of an Activity.

I wonder if we should go ahead and make this an Activity instead of just a Context (and then go up the stack trace). But we could look at this in a follow-up bug.

Comment 14

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

Comment on attachment 8732949 [details]
MozReview Request: Bug 1258450 - Don't use resources in SwipeDismissListViewTouchListener. r?mcomella

https://reviewboard.mozilla.org/r/41465/#review38657

::: mobile/android/base/java/org/mozilla/gecko/widget/SwipeDismissListViewTouchListener.java:73
(Diff revision 1)
>   * see {@link SwipeDismissTouchListener}.</p>
>   *
>   * @see SwipeDismissTouchListener
>   */
>  public class SwipeDismissListViewTouchListener implements View.OnTouchListener {
> +    private static final int TAG_ORIGINAL_HEIGHT = SwipeDismissListViewTouchListener.class.hashCode();

I was skeptical that `hashCode` was a good metric to set this tag as because we could (in theory) conflict with other Objects, but considering we have no control on what values others use as tags for `Views`, `hashCode` is as good as any!

Comment 15

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

Comment on attachment 8732956 [details]
MozReview Request: Bug 1258450 - Delegate Intent handling from GeckoAppShell to GeckoInterface. r?snorp,mcomella

https://reviewboard.mozilla.org/r/41479/#review38659

::: mobile/android/base/java/org/mozilla/gecko/GeckoApp.java:2541
(Diff revision 1)
> +    @Override
>      public void setAccessibilityEnabled(boolean enabled) {
>      }
>  
> +    @Override
> +    public boolean openUriExternal(String targetURI, String mimeType, String packageName, String className, String action, String title) {

Two concerns:

1) I'm hesitant to add more methods to the `Activity` – while I don't have enough time writing new Android apps to really understand the downsides, I suspect it's an anti-pattern because it allows the methods to access any value from the already-god object `Context`/`Activity`, which can lead to issues like memory leaks and difficult to follow state mutation. Also, there's generally a better place to add the code (e.g. as a separate component) and it's difficult to conceptualize large files.

2) Assuming the code stays here, I'd prefer if this method was static – it doesn't mutate any code in `GeckoApp` which makes it easier to reason about.

(perhaps as a follow-up because I know you're just trying to get this working) Could we separate this code from `GeckoApp` somehow? I don't really understand the implications of separating the libraries so I'm afraid I can't see many good solutions -- just passing an additional interface into GeckoAppShell, or delegating the call from GeckoApp to a static function in another class.

Comment 16

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

Comment on attachment 8732950 [details]
MozReview Request: Bug 1258450 - Remove unused GeckoInterface.getPromptService(). r?snorp

https://reviewboard.mozilla.org/r/41467/#review39253

Comment 17

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

Comment on attachment 8732951 [details]
MozReview Request: Bug 1258450 - Add GeckoInterface.createShortcut. r?snorp

https://reviewboard.mozilla.org/r/41469/#review39255

Comment 18

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

Comment on attachment 8732952 [details]
MozReview Request: Bug 1258450 - Move GlobalHistory queries to GeckoInterface. r?snorp

https://reviewboard.mozilla.org/r/41471/#review39257

::: mobile/android/base/java/org/mozilla/gecko/GeckoAppShell.java:2280
(Diff revision 1)
>      }
>  
>      @WrapForJNI(stubName = "CheckURIVisited")
>      static void checkUriVisited(String uri) {
> -        GlobalHistory.getInstance().checkUriVisited(uri);
> +        final GeckoInterface geckoInterface = getGeckoInterface();
> +        if (geckoInterface == null) {

I'm starting to think it would be nice to always return some instance here so we can avoid the null check.

Comment 19

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

Comment on attachment 8732953 [details]
MozReview Request: Bug 1258450 - Move references to SnackbarHelper out of GeckoAppShell. r?snorp,sebastian

https://reviewboard.mozilla.org/r/41473/#review39259

Comment 20

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

Comment on attachment 8732954 [details]
MozReview Request: Bug 1258450 - Route setAccessibilityEnabled through GeckoInterface. r?snorp,jchen

https://reviewboard.mozilla.org/r/41475/#review39261

Comment 21

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

Comment on attachment 8732955 [details]
MozReview Request: Bug 1258450 - Automated (mostly) refactorings moving GeckoAppShell static methods into IntentHelper. r?snorp

https://reviewboard.mozilla.org/r/41477/#review39263

Comment 22

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

Comment on attachment 8732956 [details]
MozReview Request: Bug 1258450 - Delegate Intent handling from GeckoAppShell to GeckoInterface. r?snorp,mcomella

https://reviewboard.mozilla.org/r/41479/#review39265

Since this is the last patch in the series and mcomella already commented about this:

I'm not thrilled with moving everything into the Activity either, but I do think it's a step forward from where we are now. Having everything in an interface will also allow us to reason a little more about what the gecko/fennec interaction looks like from a higher level. I don't believe Nick thinks this is a final solution at all, but a stepping stone to a nicer future.

Comment 23

[Nick Alexander :nalexander [he/him]]

https://hg.mozilla.org/integration/fx-team/rev/ccaff6721da0582775cdc27229d0b969d5d156da
Bug 1258450 - Update classycle definitions. r=me

https://hg.mozilla.org/integration/fx-team/rev/33e39362305beb355699cbd88cefe4a37354136c
Bug 1258450 - Don't use resources in SwipeDismissListViewTouchListener. r=mcomella

https://hg.mozilla.org/integration/fx-team/rev/0fa1a5081a6092771841f255871808b84ab890b3
Bug 1258450 - Remove unused GeckoInterface.getPromptService(). r=snorp

https://hg.mozilla.org/integration/fx-team/rev/f39f05fa8df3717c57452ac44a16e4a86d9cbd2c
Bug 1258450 - Add GeckoInterface.createShortcut. r=snorp

https://hg.mozilla.org/integration/fx-team/rev/476cdaa0ce08b0808c3892d3d5ee55f701504666
Bug 1258450 - Move GlobalHistory queries to GeckoInterface. r=snorp

https://hg.mozilla.org/integration/fx-team/rev/0476a0d0575b4b07c404eb15bc5a943ae04d0289
Bug 1258450 - Move references to SnackbarHelper out of GeckoAppShell. r=snorp,sebastian

https://hg.mozilla.org/integration/fx-team/rev/7a434b18855a9439a98480a2261509ab5596f315
Bug 1258450 - Route setAccessibilityEnabled through GeckoInterface. r=snorp,jchen

https://hg.mozilla.org/integration/fx-team/rev/462e9e4314eff25e00e65e75359274ec09807eee
Bug 1258450 - Automated (mostly) refactorings moving GeckoAppShell static methods into IntentHelper. r=snorp

https://hg.mozilla.org/integration/fx-team/rev/e5ec40672f6ee91471a420e797e653e3e322bba4
Bug 1258450 - Delegate Intent handling from GeckoAppShell to GeckoInterface. r=snorp,mcomella

Comment 24

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/fx-team/rev/7f7a84327477
https://hg.mozilla.org/integration/fx-team/rev/d085c3aaf69d
https://hg.mozilla.org/integration/fx-team/rev/353d5e668129
https://hg.mozilla.org/integration/fx-team/rev/e2787bb704a7
https://hg.mozilla.org/integration/fx-team/rev/990d098bcaba
https://hg.mozilla.org/integration/fx-team/rev/2973c93dcb79
https://hg.mozilla.org/integration/fx-team/rev/624f831dddda
https://hg.mozilla.org/integration/fx-team/rev/1994ad414e8e
https://hg.mozilla.org/integration/fx-team/rev/5a2629dce8a7

Comment 25

[Carsten Book [:Tomcat]]

sorry had to back this out since this or the other push caused https://treeherder.mozilla.org/logviewer.html#?job_id=8313401&repo=fx-team

Comment 26

[Nick Alexander :nalexander [he/him]]

https://hg.mozilla.org/integration/fx-team/rev/537e8f7a4950944c6835d67f76e8e899682209b3
Bug 1258450 - Don't use resources in SwipeDismissListViewTouchListener. r=mcomella

https://hg.mozilla.org/integration/fx-team/rev/311d7535f303544fc4941464692389b59277ada9
Bug 1258450 - Remove unused GeckoInterface.getPromptService(). r=snorp

https://hg.mozilla.org/integration/fx-team/rev/ca6fcc658b21b559dc4eb25a34d267dc57baf521
Bug 1258450 - Add GeckoInterface.createShortcut. r=snorp

https://hg.mozilla.org/integration/fx-team/rev/99c18eae6af0993af8e505f1d97a778cc336e6f7
Bug 1258450 - Move GlobalHistory queries to GeckoInterface. r=snorp

https://hg.mozilla.org/integration/fx-team/rev/53b8d9973481a0795c30e6daeac2fae25f8b51ae
Bug 1258450 - Move references to SnackbarHelper out of GeckoAppShell. r=snorp,sebastian

https://hg.mozilla.org/integration/fx-team/rev/46a922adb14f8fdbcf2a7539dcafeb3784465313
Bug 1258450 - Route setAccessibilityEnabled through GeckoInterface. r=snorp,jchen

https://hg.mozilla.org/integration/fx-team/rev/7eaf58b44b29144de2844056f4578e2603b6a06d
Bug 1258450 - Automated (mostly) refactorings moving GeckoAppShell static methods into IntentHelper. r=snorp

https://hg.mozilla.org/integration/fx-team/rev/6bc6f4e79300e05c8c3288411fb54c50bd24fb6d
Bug 1258450 - Delegate Intent handling from GeckoAppShell to GeckoInterface. r=snorp,mcomella

Comment 27

[Nick Alexander :nalexander [he/him]]

Relanded.  Thanks, Carsten!

Comment 28

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

https://hg.mozilla.org/mozilla-central/rev/537e8f7a4950
https://hg.mozilla.org/mozilla-central/rev/311d7535f303
https://hg.mozilla.org/mozilla-central/rev/ca6fcc658b21
https://hg.mozilla.org/mozilla-central/rev/99c18eae6af0
https://hg.mozilla.org/mozilla-central/rev/53b8d9973481
https://hg.mozilla.org/mozilla-central/rev/46a922adb14f
https://hg.mozilla.org/mozilla-central/rev/7eaf58b44b29
https://hg.mozilla.org/mozilla-central/rev/6bc6f4e79300

Comment 29

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

https://hg.mozilla.org/mozilla-central/rev/537e8f7a4950 got backed out in bug 1271175.