1432235 - Put public GeckoView types in org.mozilla.geckoview

Status: RESOLVED FIXED

(GeckoView :: General, enhancement)

Description

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

It's the right place for it and it makes it easy to generate public documentation, because we can ignore everything in org.mozilla.gecko.*

Comment 1

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1432235-1.0-Move-GeckoView-API-classes-to-org.mo.patch

Move GeckoSession, GeckoView and GeckoSessionSettings to o.m.geckoview package.

Jim, could making all these classes public have performance implications?

Comment 2

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

Comment on attachment 8952139 [details] [diff] [review]
0001-Bug-1432235-1.0-Move-GeckoView-API-classes-to-org.mo.patch

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

Should be okay, but several other API classes (e.g. TextInputController) and classes that are tightly tied to GeckoSession (e.g. GeckoSessionHandler) should be moved as well.

::: mobile/android/base/moz.build
@@ +112,5 @@
>  with Files('../app/src/*/res/menu/browsersearch_contextmenu.xml'):
>      BUG_COMPONENT = ('Firefox for Android', 'Awesomescreen')
>  
> +with Files('java/org/mozilla/geckoview/**'):
> +    BUG_COMPONENT = ('Firefox for Android', 'GeckoView')

Not needed because this is already covered by [1] (although the "Audio/Video" lines after that are wrong)

[1] https://searchfox.org/mozilla-central/rev/5536f71c3833018c4f4e2c73f37eae635aab63ff/mobile/android/moz.build#20

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoSessionHandler.java
@@ +15,3 @@
>  import android.util.Log;
>  
> +public abstract class GeckoSessionHandler<Listener>

I would move this to o.m.geckoview

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoSessionState.java
@@ +4,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +package org.mozilla.gecko;
> +
> +public enum GeckoSessionState implements NativeQueue.State {

I would keep this in GeckoSession. It had package-private access, so we're not exposing it to consumers.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoSessionWindow.java
@@ +18,5 @@
> +import android.os.IInterface;
> +import android.os.SystemClock;
> +import android.util.Log;
> +
> +public final class GeckoSessionWindow extends JNIObject implements IInterface {

I would keep this in GeckoSession, and change it to package-private access, so we're not exposing it to consumers.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/TextInputController.java
@@ +96,5 @@
>      private final GeckoEditable mEditable = new GeckoEditable();
>      private final GeckoEditableChild mEditableChild = new GeckoEditableChild(mEditable);
>      private Delegate mInputConnection;
>  
> +    public TextInputController(final @NonNull GeckoSession session,

Public API; move to o.m.geckoview

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/TrackingProtection.java
@@ +15,2 @@
>  
> +public class TrackingProtection {

Public API; move to o.m.geckoview

::: mobile/android/geckoview_example/src/main/java/org/mozilla/geckoview_example/GeckoViewActivity.java
@@ +27,5 @@
>  
>  public class GeckoViewActivity extends Activity {
>      private static final String LOGTAG = "GeckoViewActivity";
> +    // private static final String DEFAULT_URL = "https://itisatrap.org/firefox/its-a-tracker.html";
> +    private static final String DEFAULT_URL = "www.html-kit.com/tools/cookietester";

Shouldn't be in this patch

Comment 3

[Eugen Sawin [:esawin]]

(In reply to Jim Chen [:jchen] [:darchons] from comment #2)
> Should be okay, but several other API classes (e.g. TextInputController) and
> classes that are tightly tied to GeckoSession (e.g. GeckoSessionHandler)
> should be moved as well.

> mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoSessionHandler.
> java
> @@ +15,3 @@
> >  import android.util.Log;
> >  
> > +public abstract class GeckoSessionHandler<Listener>
> 
> I would move this to o.m.geckoview

I was under the impression that we only want to move the public API to o.m.gv and GeckoSessionHandler is an internal class without public exposure.

> mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoSessionState.
> java
> @@ +4,5 @@
> > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> > +
> > +package org.mozilla.gecko;
> > +
> > +public enum GeckoSessionState implements NativeQueue.State {
> 
> I would keep this in GeckoSession. It had package-private access, so we're
> not exposing it to consumers.

The state is used in GeckoSession (o.m.gv) and GeckoSessionHandler (o.m.g) which means we have to move it out of GeckoSession to avoid exposing internal state.


> 
> :::
> mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoSessionWindow.
> java
> @@ +18,5 @@
> > +import android.os.IInterface;
> > +import android.os.SystemClock;
> > +import android.util.Log;
> > +
> > +public final class GeckoSessionWindow extends JNIObject implements IInterface {
> 
> I would keep this in GeckoSession, and change it to package-private access,
> so we're not exposing it to consumers.

The window is used in GeckoSession (o.m.gv) and TextInputController (o.m.g), so I had to move it out. If TextInputController becomes public, I can move it back in, although there is no real benefit of making a shared class nested.

> mobile/android/geckoview/src/main/java/org/mozilla/gecko/TextInputController.
> java
> @@ +96,5 @@
> >      private final GeckoEditable mEditable = new GeckoEditable();
> >      private final GeckoEditableChild mEditableChild = new GeckoEditableChild(mEditable);
> >      private Delegate mInputConnection;
> >  
> > +    public TextInputController(final @NonNull GeckoSession session,
> 
> Public API; move to o.m.geckoview

OK. Are there any other public classes that I've missed?

> mobile/android/geckoview/src/main/java/org/mozilla/gecko/TrackingProtection.
> java
> @@ +15,2 @@
> >  
> > +public class TrackingProtection {
> 
> Public API; move to o.m.geckoview

TrackingProtection is an internal class, the public TP API consists of TrackingProtectionDelegate and two methods in GeckoSession.

> mobile/android/geckoview_example/src/main/java/org/mozilla/geckoview_example/
> GeckoViewActivity.java
> @@ +27,5 @@
> >  
> >  public class GeckoViewActivity extends Activity {
> >      private static final String LOGTAG = "GeckoViewActivity";
> > +    // private static final String DEFAULT_URL = "https://itisatrap.org/firefox/its-a-tracker.html";
> > +    private static final String DEFAULT_URL = "www.html-kit.com/tools/cookietester";
> 
> Shouldn't be in this patch

Oops.

Thanks!

Comment 4

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

(In reply to Eugen Sawin [:esawin] from comment #3)
> (In reply to Jim Chen [:jchen] [:darchons] from comment #2)
>
> I was under the impression that we only want to move the public API to
> o.m.gv and GeckoSessionHandler is an internal class without public exposure.

IMO it's coupled with GeckoSession enough to warrant moving to o.m.geckoview. I don't think o.m.geckoview should strictly contain public classes. It seems unreasonable to split GeckoSession and GeckoSessionHandler into different packages just because one is public and one is not.

The long term vision is for a multiprocess model where GeckoView runs in the app process and Gecko runs in a service process. With that in mind, everything in the app process logically belongs in the o.m.geckoview package, and everything in the service process logically belongs in the o.m.gecko package. GeckoSessionHandler will certainly run in the app process, so it's logical to have it in the o.m.geckoview package.

> > I would keep this in GeckoSession. It had package-private access, so we're
> > not exposing it to consumers.
> 
> The state is used in GeckoSession (o.m.gv) and GeckoSessionHandler (o.m.g)
> which means we have to move it out of GeckoSession to avoid exposing
> internal state.

This shouldn't be a problem with GeckoSessionHandler in o.m.gv.

> The window is used in GeckoSession (o.m.gv) and TextInputController (o.m.g),
> so I had to move it out. If TextInputController becomes public, I can move
> it back in, although there is no real benefit of making a shared class
> nested.

This shouldn't be a problem either with TextInputController in o.m.gv

> OK. Are there any other public classes that I've missed?

I think that's it.

> TrackingProtection is an internal class, the public TP API consists of
> TrackingProtectionDelegate and two methods in GeckoSession.

I'm partial to exposing TrackingProtection, and an API similar to `session.getTrackingProtection.enable/disable/getDelegate/setDelegate`, but that's outside of this bug.

Comment 5

[Eugen Sawin [:esawin]]

(In reply to Jim Chen [:jchen] [:darchons] from comment #4)
> The long term vision is for a multiprocess model where GeckoView runs in the
> app process and Gecko runs in a service process. With that in mind,
> everything in the app process logically belongs in the o.m.geckoview
> package, and everything in the service process logically belongs in the
> o.m.gecko package. GeckoSessionHandler will certainly run in the app
> process, so it's logical to have it in the o.m.geckoview package.

That makes sense and I'm OK with either approach, but we need to make sure we do this consistently.

Separating the packages based on the process model was not the original intention of this bug, the idea was to have a clear separation of the public API from internal details for documentation.

snorp, would you prefer process-based separation over the documentation-oriented one?

Comment 6

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

Comment on attachment 8952139 [details] [diff] [review]
0001-Bug-1432235-1.0-Move-GeckoView-API-classes-to-org.mo.patch

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

Jim's comments look good to me. I think we should go ahead with the javadoc changes to restrict what we generate as well.

Comment 7

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1432235-1.1-Move-GeckoView-API-classes-to-org.mo.patch

Addressed comments.

Comment 8

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

Comment on attachment 8952501 [details] [diff] [review]
0001-Bug-1432235-1.1-Move-GeckoView-API-classes-to-org.mo.patch

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

Thanks!

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSessionHandler.java
@@ +31,1 @@
>                          final String[] events) {

Alignment

@@ +36,1 @@
>                          final String[] events, final boolean alwaysListen) {

Alignment

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/TextInputController.java
@@ +104,1 @@
>                                        final @NonNull NativeQueue queue) {

Alignment

Comment 9

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1432235-1.2-Move-GeckoView-API-classes-to-org.mo.patch

Addressed comments and rebased.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/60e0b03bcb63
[1.2] Move GeckoView API classes to org.mozilla.geckoview. r=snorp,jchen

Comment 11

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/60e0b03bcb63