1258470 - [geckoview] Make org.mozilla.gecko.gfx not depend on Fennec's Tab/Tabs

Status: RESOLVED FIXED

(GeckoView :: General, defect)

Description

[Nick Alexander :nalexander [he/him]]

After Bug 1258450, we have:

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.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
<snip>

This ticket tracks removing the compile-time dependence on Tab/Tabs.  There are two parts.  First, finding a way for BitmapUtils to not depend on Tab/Tabs.  There's a getDrawable() helper that handles a thumbnail: pseudo-protocol at https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/gfx/BitmapUtils.java#66.  A cursory search suggest that thumbnail: is only used at https://dxr.mozilla.org/mozilla-central/source/mobile/android/components/TabSource.js#50, but perhaps it's also used by the tab tray.  In any case, we should be able to split this Tab stuff out of the GeckoView part.

The second part tracks not using Tab/Tabs directly on org.mozilla.gecko.gfx.  All these uses want to get events when the current tab display settings change: background color, viewport constraints, RTL, etc.  Can we expose that data but keep the Tab/Tabs munging on the Fennec side?

Comment 1

[Nick Alexander :nalexander [he/him]]

jchen: you seem the best person to actually get this done.  Can you make this happen?

Comment 2

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

A lot of it is in gfx code that I don't feel very comfortable touching. I think snorp, kats, or rbarker would be better.

Comment 3

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

Some of this should be easier in 49, since we're planning to nuke the java layer system then, but it looks like we'll still have some issues here.

I think for the most part it makes sense to just factor out the tab-specific logic into some other Fennec place. For example, TouchEventHandler uses Tab.getHasTouchListeners() to set mWaitForTouchListeners according to the current tab. We could just have GeckoView.setWaitForTouchListener() that Fennec would manage instead.

Comment 4

[Benjamin Smedberg]

Mass change of bugs in the "Embedding: GRE Core" component in preparation for archiving it. I believe that these are no longer relevant; but if they are, they should be reopened and moved into a component relevant to the code in question.

Comment 5

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1258470 - Part 1: Assume MOZ_ANDROID_APZ. DO NOT LAND.

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

Comment 6

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1258470 - Part 2: MOZ_ANDROID_APZ implies Tabs.VIEWPORT_CHANGED event is no longer fired.

See https://dxr.mozilla.org/mozilla-central/source/widget/android/nsWindow.cpp#3604-3627.

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

Comment 7

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1258470 - Part 3: Remove zoom constraints and RTL flag.

The previous patch removed the only setter of mZoomConstraints and
mIsRTL.  It's not clear to me how RTL is handled in this tangled code,
but this should work for now.

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

Comment 8

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1258470 - Part 4: MOZ_ANDROID_APZ implies no need for functional getDisplayPort.

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

Comment 9

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1258470 - Part 5: Remove use of Tab for background color.

I can't justify this: I'm just doing it to get Tab and Tabs out of the
omg.gfx package.  I think this will change behaviour, since I can't
find an explanation for why it wouldn't.

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

Comment 10

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1258470 - Part 6: Update LayerView in response to Tab.SELECTED from outside omg.gfx.

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

Comment 11

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1258470 - Part 7: Remove thumbnail: support.

This changes behaviour: the list of tabs to stream will no longer have
icons.

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

Comment 12

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1258470 - Part 8: Move some constants that should be Fennec-specific into GeckoAppShell.

This is just expedient: these things are almost certainly
App-specific, but re-working the code to support that is hard work.

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

Comment 13

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1258470 - Part 9: Move some of GuestSession into GuestProfile.

Again, this needs a dependency-injection hook of some kind to let the
App handle guest sessions.

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

Comment 14

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1258470 - Part 9b: Rework some GeckoProfile bits, removing Distribution and default bookmark support.

This needs a hook into the App in some way.

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

Comment 15

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1258470 - Part 11a: Move BrowserDB access out of GeckoProfile into GeckoApplication.

This is the first half of the equation -- updating the callers.  This
was automated using IntelliJ's "Replace Structurally ..." operation, with
the template:

$Instance$.getDB() -> GeckoApplication.getDB($Instance$)

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

Comment 16

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1258470 - Part 11b: Inline BrowserDB factory into GeckoApplication.

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

Comment 17

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1258470 - Part 12a: Move UnusedResourcesUtil out of omg.util.

This file isn't actually part of the build; it's just in place to
placate Gradle's Lint integration.

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

Comment 18

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1258470 - Part 12b: Move Experiments out of omg.util.

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

Comment 19

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1258470 - Part 13: Remove FormAssistPopup from GeckoView.

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

Comment 20

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1258470 - Part 14: Add geckoview, still using org.mozilla.gecko.

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

Comment 21

[Nick Alexander :nalexander [he/him]]

Attachment: !Bug 1258470 - Part 15: Hacks to make stand-alone :geckoview Gradle project compile.

These will need to be worked around for real.

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

Comment 22

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1258470 - Part 16: Add a geckoview_example Gradle project.

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

Comment 23

[Nick Alexander :nalexander [he/him]]

snorp, jchen: the attached long series of commits shows one (the?) path forward toward a stand-alone GeckoView AAR and a consumer.  After the series (and Bug 1281890), I have a functioning moz.build Fennec (well, I removed some small things), and a new Gradle-based geckoview_example App that correctly renders a page.

There's a lot going on in the series and more work to be done but it shows that the approach can work and could provide the technical foundation for a Fennec-independent Android embedding story.

Comment 24

[Nick Alexander :nalexander [he/him]]

In case it's not clear -- this isn't ready to review or to land.  I just want to get it out and hopefully have a motivated platform person start cherry-picking the re-factorings through-out the series.

Comment 25

[Nick Alexander :nalexander [he/him]]

kats, blassey -- just wanted you two to be aware of this work.  I hope it pushes the removal of the JPZC up the priority stack, since that's a critical part of separating GV from browser.js.

Comment 26

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

I'll be happy to start removing JPZC code once 48 is in release. We can start writing the patches before then, certainly.

Comment 27

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

Comment on attachment 8764781 [details]
Bug 1258470 - Part 16: Add a geckoview_example Gradle project.

https://reviewboard.mozilla.org/r/60480/#review58200

::: mobile/android/geckoview_example/src/main/java/org/mozilla/geckoview_example/GeckoViewActivity.java:28
(Diff revision 1)
> +import org.mozilla.gecko.util.NativeEventListener;
> +
> +import java.io.File;
> +
> +public class GeckoViewActivity extends Activity {
> +	private static final String LOGTAG = "GeckoViewActivity";

Indent

::: mobile/android/geckoview_example/src/main/java/org/mozilla/geckoview_example/GeckoViewActivity.java:46
(Diff revision 1)
> +        final GeckoProfile profile = GeckoProfile.get(getApplicationContext());
> +
> +        EventDispatcher.getInstance().registerGeckoThreadListener((GeckoEventListener) mGeckoView,
> +                "Gecko:Ready",
> +                "Accessibility:Event",
> +                "Content:StateChange",
> +                "Content:LoadError",
> +                "Content:PageShow",
> +                "DOMTitleChanged",
> +                "Link:Favicon",
> +                "Prompt:Show",
> +                "Prompt:ShowTop");
> +
> +        EventDispatcher.getInstance().registerGeckoThreadListener((NativeEventListener) mGeckoView,
> +                "Accessibility:Ready",
> +                "GeckoView:Message");
> +
> +        // GeckoProfile profile = GeckoProfile.get(this, "default", getApplicationContext().getFilesDir()).forceCreate();
> +        GeckoThread.init(profile, "chrome://browser/content/headless.xul", /* action */ null, /* debugging */ false);
> +
> +//    if (url != null) {
> +//        GeckoAppShell.sendEventToGecko(GeckoEvent.createURILoadEvent(url));
> +//    }
> +
> +        if (GeckoThread.launch()) {
> +            // This is the first launch, so finish initialization and go.
> +             // GeckoProfile profile = GeckoProfile.get(this, "default", getApplicationContext().getFilesDir()).forceCreate();
> +        } else if (GeckoThread.isRunning()) {
> +            // If Gecko is already running, that means the Activity was
> +            // destroyed, so we need to re-attach Gecko to this GeckoView.
> +            mGeckoView.connectToGecko();
> +        }

Why are these here? The GeckoView client shouldn't have to deal with profiles, event listeners, and Gecko thread management.

Comment 28

[Nick Alexander :nalexander [he/him]]

(In reply to Jim Chen [:jchen] [:darchons] from comment #27)
> Comment on attachment 8764781 [details]
> Bug 1258470 - Part 16: Add a geckoview_example Gradle project.
> 
> https://reviewboard.mozilla.org/r/60480/#review58200
> 
> :::
> mobile/android/geckoview_example/src/main/java/org/mozilla/geckoview_example/
> GeckoViewActivity.java:28
> (Diff revision 1)
> > +import org.mozilla.gecko.util.NativeEventListener;
> > +
> > +import java.io.File;
> > +
> > +public class GeckoViewActivity extends Activity {
> > +	private static final String LOGTAG = "GeckoViewActivity";
> 
> Indent
> 
> :::
> mobile/android/geckoview_example/src/main/java/org/mozilla/geckoview_example/
> GeckoViewActivity.java:46
> (Diff revision 1)
> > +        final GeckoProfile profile = GeckoProfile.get(getApplicationContext());
> > +
> > +        EventDispatcher.getInstance().registerGeckoThreadListener((GeckoEventListener) mGeckoView,
> > +                "Gecko:Ready",
> > +                "Accessibility:Event",
> > +                "Content:StateChange",
> > +                "Content:LoadError",
> > +                "Content:PageShow",
> > +                "DOMTitleChanged",
> > +                "Link:Favicon",
> > +                "Prompt:Show",
> > +                "Prompt:ShowTop");
> > +
> > +        EventDispatcher.getInstance().registerGeckoThreadListener((NativeEventListener) mGeckoView,
> > +                "Accessibility:Ready",
> > +                "GeckoView:Message");
> > +
> > +        // GeckoProfile profile = GeckoProfile.get(this, "default", getApplicationContext().getFilesDir()).forceCreate();
> > +        GeckoThread.init(profile, "chrome://browser/content/headless.xul", /* action */ null, /* debugging */ false);
> > +
> > +//    if (url != null) {
> > +//        GeckoAppShell.sendEventToGecko(GeckoEvent.createURILoadEvent(url));
> > +//    }
> > +
> > +        if (GeckoThread.launch()) {
> > +            // This is the first launch, so finish initialization and go.
> > +             // GeckoProfile profile = GeckoProfile.get(this, "default", getApplicationContext().getFilesDir()).forceCreate();
> > +        } else if (GeckoThread.isRunning()) {
> > +            // If Gecko is already running, that means the Activity was
> > +            // destroyed, so we need to re-attach Gecko to this GeckoView.
> > +            mGeckoView.connectToGecko();
> > +        }
> 
> Why are these here? The GeckoView client shouldn't have to deal with
> profiles, event listeners, and Gecko thread management.

Oh, I absolutely concur.  I'm sorry I wasn't clear in https://bugzilla.mozilla.org/show_bug.cgi?id=1258470#c23 and #c24 -- this isn't really ready for review, I just wanted you and snorp to see the approach I took.  You can see what I *needed* to do to connect the activity to Gecko (not much!), not what we *should* do.

Comment 29

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

Ah I see. Carry on!

Comment 30

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

Comment on attachment 8764781 [details]
Bug 1258470 - Part 16: Add a geckoview_example Gradle project.

https://reviewboard.mozilla.org/r/60480/#review59098

Nice!

Comment 31

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

https://reviewboard.mozilla.org/r/60480/#review59102

Comment 32

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

I have some patches ready for review that should take Tab/Tabs out of gfx. A lot of them are adapted from Nick's patches above.

Comment 33

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

Attachment: 1. Remove RTL and ZoomConstraints variables from gfx (v1)

We no longer send viewport metadata, so we don't actually make use of the RTL
and ZoomConstraints variables we keep in Java. There is a single usage of an
RTL flag in ImmutableViewportMetrics.offsetViewportByAndClamp, but I think the
two branches are equivalent, so the RTL flag is not needed there either. The
patch gets rid of the RTL flag in ImmutableViewportMetrics entirely.

Comment 34

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

Attachment: 2. Move thumbnail code out of BitmapUtils (v1)

Move the "thumbnail:" handler out of BitmapUtils and into
ThumbnailHelper and PromptListItem.

The patch adds two overloads of the getAndProcessThumbnailFor method in
ThumbnailHelper, which handle the tasks of getting a thumbnail for a
specific tab and calling a given BitmapLoader.

Because only PromptListItem makes use of the "thumbnail:" convention,
the actual handling of "thumbnail:" is moved to PromptListItem, which
calls ThumbnailHelper to get the thumbnail.

Comment 35

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

Attachment: 3. Small optimizations in ThumbnailHelper (v1)

This patch includes a small memory optimization of using ArrayList and
`volatile int` for storing the pending thumbnails list and pending
width, instead of using LinkedList and AtomicInteger, respectively.

The patch also fixes a possible race condition due to calling
processNextThumbnail outside of a lock. Now it must be called inside a
lock and its name is changed to reflect that.

Comment 36

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8789556 [details] [diff] [review]
2. Move thumbnail code out of BitmapUtils (v1)

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

sebastian is PTO for a while, so I'll review this.  lgtm!

Comment 37

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8789557 [details] [diff] [review]
3. Small optimizations in ThumbnailHelper (v1)

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

The JNI stuff looks like a step in the wrong direction.  Explain?

::: mobile/android/base/java/org/mozilla/gecko/ThumbnailHelper.java
@@ +59,5 @@
>      }
>  
>      // instance stuff
>  
> +    private final ArrayList<Tab> mPendingThumbnails;    // synchronized access only

Say what it should be synchronized on.

::: widget/android/GeneratedJNIWrappers.h
@@ +4028,5 @@
>                  bool,
>                  bool> Args;
>          static constexpr char name[] = "notifyThumbnail";
>          static constexpr char signature[] =
> +                "(Ljava/nio/ByteBuffer;Lorg/mozilla/gecko/Tab;ZZ)V";

This seems like worst possible: now we have JNI referring to Tab.  I know you want Fennec to provide JNI code, but this seems like we're making it *harder* to separate Fennec from GV.

Can you explain your longterm plan here?

Comment 38

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

(In reply to Nick Alexander :nalexander from comment #37)
> Comment on attachment 8789557 [details] [diff] [review]
> 3. Small optimizations in ThumbnailHelper (v1)
> 
> Review of attachment 8789557 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/android/GeneratedJNIWrappers.h
> @@ +4028,5 @@
> >                  bool,
> >                  bool> Args;
> >          static constexpr char name[] = "notifyThumbnail";
> >          static constexpr char signature[] =
> > +                "(Ljava/nio/ByteBuffer;Lorg/mozilla/gecko/Tab;ZZ)V";
> 
> This seems like worst possible: now we have JNI referring to Tab.  I know
> you want Fennec to provide JNI code, but this seems like we're making it
> *harder* to separate Fennec from GV.
> 
> Can you explain your longterm plan here?

ThumbnailHelper is part of Fennec just like Tab, so it's okay for it to refer to Tab in its JNI call.

In bug 1291375, I'm working on splitting our JNI bindings. We'll have one set of JNI bindings for geckoview and a separate, independent set of JNI bindings for Fennec. In the future, we can optionally compile the Fennec bindings on the C++ side if we're building Fennec, and not compile them if we're building geckoview.

Comment 39

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8789557 [details] [diff] [review]
3. Small optimizations in ThumbnailHelper (v1)

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

OK, then I'm fine with this.

Comment 40

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e4e13f55217
1. Remove RTL and ZoomConstraints variables from gfx; r=rbarker
https://hg.mozilla.org/integration/mozilla-inbound/rev/ead253ed6f22
2. Move thumbnail code out of BitmapUtils; r=nalexander
https://hg.mozilla.org/integration/mozilla-inbound/rev/74422ceb5af2
3. Small optimizations in ThumbnailHelper; r=nalexander

Comment 41

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9e4e13f55217
https://hg.mozilla.org/mozilla-central/rev/ead253ed6f22
https://hg.mozilla.org/mozilla-central/rev/74422ceb5af2