Closed Bug 1258470 Opened 4 years ago Closed 3 years ago

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

Categories

(GeckoView :: General, defect)

defect
Not set

Tracking

(firefox51 fixed)

RESOLVED FIXED
Tracking Status
firefox51 --- fixed

People

(Reporter: nalexander, Assigned: jchen)

References

Details

Attachments

(21 files)

58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
snorp
: review+
Details
29.39 KB, patch
rbarker
: review+
Details | Diff | Splinter Review
8.42 KB, patch
nalexander
: review+
Details | Diff | Splinter Review
17.10 KB, patch
nalexander
: review+
Details | Diff | Splinter Review
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?
jchen: you seem the best person to actually get this done.  Can you make this happen?
Flags: needinfo?(nchen)
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.
Flags: needinfo?(nchen) → needinfo?(snorp)
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.
Flags: needinfo?(snorp)
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.
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → INCOMPLETE
Product: Core → Core Graveyard
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/
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/
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/
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/
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/
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/
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/
Status: RESOLVED → REOPENED
Component: Embedding: GRE Core → GeckoView
Product: Core Graveyard → Firefox for Android
Resolution: INCOMPLETE → ---
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.
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.
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.
I'll be happy to start removing JPZC code once 48 is in release. We can start writing the patches before then, certainly.
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.
Attachment #8764781 - Flags: review?(nchen)
(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.
Ah I see. Carry on!
Comment on attachment 8764781 [details]
Bug 1258470 - Part 16: Add a geckoview_example Gradle project.

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

Nice!
Attachment #8764781 - Flags: review?(snorp) → review+
Depends on: 1291373
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.
Assignee: nobody → nchen
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.
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.
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.
Attachment #8789555 - Flags: review?(rbarker)
Attachment #8789556 - Flags: review?(s.kaspari)
Attachment #8789557 - Flags: review?(s.kaspari)
Attachment #8789555 - Flags: review?(rbarker) → review+
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!
Attachment #8789556 - Flags: review?(s.kaspari) → review+
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?
Attachment #8789557 - Flags: feedback-
(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 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.
Attachment #8789557 - Flags: review?(s.kaspari)
Attachment #8789557 - Flags: review+
Attachment #8789557 - Flags: feedback-
Pushed by nchen@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/9e4e13f55217
https://hg.mozilla.org/mozilla-central/rev/ead253ed6f22
https://hg.mozilla.org/mozilla-central/rev/74422ceb5af2
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Product: Firefox for Android → GeckoView
Target Milestone: Firefox 51 → ---
You need to log in before you can comment on or make changes to this bug.