Closed Bug 1258450 Opened 8 years ago Closed 8 years ago

[geckoview] Update GeckoView classycle definitions

Categories

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

defect
Not set
normal

Tracking

(firefox48 fixed)

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: nalexander, Assigned: nalexander)

References

Details

Attachments

(9 files)

58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
mcomella
: review+
Details
58 bytes, text/x-review-board-request
snorp
: review+
Details
58 bytes, text/x-review-board-request
snorp
: review+
Details
58 bytes, text/x-review-board-request
snorp
: review+
Details
58 bytes, text/x-review-board-request
sebastian
: review+
snorp
: review+
Details
58 bytes, text/x-review-board-request
jchen
: review+
snorp
: review+
Details
58 bytes, text/x-review-board-request
snorp
: review+
Details
58 bytes, text/x-review-board-request
mcomella
: review+
snorp
: review+
Details
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.
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/
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/
Attachment #8732949 - Flags: review?(michael.l.comella)
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/
Attachment #8732951 - Flags: review?(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/
Attachment #8732952 - Flags: review?(snorp)
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/
Attachment #8732953 - Flags: review?(snorp)
Attachment #8732953 - Flags: review?(s.kaspari)
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!
Blocks: 1258464
Blocks: 1258470
Blocks: 1258472
Comment on attachment 8732954 [details]
MozReview Request: Bug 1258450 - Route setAccessibilityEnabled through GeckoInterface. r?snorp,jchen

https://reviewboard.mozilla.org/r/41475/#review37947
Attachment #8732954 - Flags: review?(nchen) → review+
Attachment #8732953 - Flags: review?(s.kaspari) → review+
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.
Attachment #8732949 - Flags: review?(michael.l.comella) → review+
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 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.
Attachment #8732956 - Flags: review?(michael.l.comella) → review+
Comment on attachment 8732950 [details]
MozReview Request: Bug 1258450 - Remove unused GeckoInterface.getPromptService(). r?snorp

https://reviewboard.mozilla.org/r/41467/#review39253
Attachment #8732950 - Flags: review?(snorp) → review+
Comment on attachment 8732951 [details]
MozReview Request: Bug 1258450 - Add GeckoInterface.createShortcut. r?snorp

https://reviewboard.mozilla.org/r/41469/#review39255
Attachment #8732951 - Flags: review?(snorp) → review+
Attachment #8732952 - Flags: review?(snorp) → review+
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 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
Attachment #8732953 - Flags: review?(snorp) → review+
Comment on attachment 8732954 [details]
MozReview Request: Bug 1258450 - Route setAccessibilityEnabled through GeckoInterface. r?snorp,jchen

https://reviewboard.mozilla.org/r/41475/#review39261
Attachment #8732954 - Flags: review?(snorp) → review+
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
Attachment #8732955 - Flags: review?(snorp) → review+
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.
Attachment #8732956 - Flags: review?(snorp) → review+
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
Status: NEW → ASSIGNED
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
Flags: needinfo?(nalexander)
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
Relanded.  Thanks, Carsten!
Flags: needinfo?(nalexander)
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.