880118 - Package Gecko and GeckoView into an Android library project

Status: RESOLVED FIXED

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

Description

[Mark Finkle (:mfinkle) (use needinfo?)]

Being able to create a standalone JAR package for Gecko and GeckoView will allow other Android developers to use GeckoView in their applications.

Comment 1

[Nick Alexander :nalexander [he/him]]

Bug 873569 is for moving Gecko assets (like libs, and omnijar) into the assets/ folder of the APK.

Comment 2

[Shane Tully (:stully:]

Attachment: tmp-create-library-script.diff

Temporary script for copying built JARs, SOs, omni.ja, and resources to library project directory.

Comment 3

[Shane Tully (:stully:]

Attachment: add-new-tab-option.diff

Temporary patch to add option to GeckoView to open a URL in a new thread

Comment 4

[Shane Tully (:stully:]

Attachment: constant-id.diff

Changes resources to non-constant IDs which is necessary for library projects.

Comment 5

[Shane Tully (:stully:]

Attachment: library-project.diff

WIP patch to adapt GeckoView to run without having control over an Activity

Comment 6

[Shane Tully (:stully:]

Attachment: tmp-create-library-script.diff

Comment 7

[Shane Tully (:stully:]

Attachment: library-project.diff

updated patches for allowing a page to load :)

Comment 8

[Shane Tully (:stully:]

Attachment: constant-id.diff

Change resources to non-static variables which is necessary for a library project.

Comment 9

[Shane Tully (:stully:]

Comment on attachment 787232 [details] [diff] [review]
add-new-tab-option.diff

I know this was patch was rejected in another bug, but it is necessary to get a page to load in GeckoView. Until a proper GeckoView interface is defined, this will at least allow a page to be loaded.

Comment 10

[Chris Peterson [:cpeterson]]

Comment on attachment 787232 [details] [diff] [review]
add-new-tab-option.diff

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

::: mobile/android/base/GeckoView.java
@@ +76,4 @@
>      }
>  
> +    public void loadUrl(String uri, boolean openInNewTab) {
> +        Tabs.getInstance().loadUrl(uri, (openInNewTab ? Tabs.LOADURL_NEW_TAB : Tabs.LOADURL_NONE));

Boolean parameters are often a short-sighted design and obscure the caller code. I would much prefer this new method be named loadUrlInNewTab() or take a LOADURL flags parameter. Taking a flags parameter might be a better design because it emphasizes that this method is simply a pass-through to Tab.getInstance().

Comment 11

[Shane Tully (:stully:]

Attachment: add-new-tab-option.diff

Changed to "loadUrlInNewTab" because changing to expose the flags variable means having to expose the Tabs flags to the 3rd party app using the library. While this is temporary anyway, it seems better to hide this info for now until the GeckoView interface is hashed out.

Comment 12

[Chris Peterson [:cpeterson]]

Comment on attachment 789798 [details] [diff] [review]
add-new-tab-option.diff

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

LGTM!

Comment 13

[Chris Peterson [:cpeterson]]

Comment on attachment 789771 [details] [diff] [review]
constant-id.diff

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

LGTM, but I think you should simplify these long chains of `if/else` statements to just a sequence of `if` statements than each call `return`. You can then add an empty line between each `if` block to make these long methods easier to read.

Comment 14

[Shane Tully (:stully:]

Attachment: constant-id.diff

Comment 15

[Shane Tully (:stully:]

Attachment: constant-id.diff

Comment 16

[Chris Peterson [:cpeterson]]

Comment on attachment 789898 [details] [diff] [review]
constant-id.diff

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

LGTM!

::: mobile/android/base/BrowserApp.java
@@ +1728,2 @@
>  
> +        int itemId = item.getItemId();

Please make this itemId `final` like you did in the other methods.

Comment 17

[Chris Peterson [:cpeterson]]

add-new-tab-option.diff:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d17e4a0cba94

constant-id.diff:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a3cc1c802366

Comment 18

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 789898 [details] [diff] [review]
constant-id.diff

Holy cow. This patch will be very unpopular.

What's the issue here?

Comment 19

[Shane Tully (:stully:]

A library project cannot have its resource IDs in R.java be static which means they can't be used as case labels. Otherwise, all the resources will be null.

Comment 20

[Nick Alexander :nalexander [he/him]]

(In reply to Mark Finkle (:mfinkle) from comment #18)
> Comment on attachment 789898 [details] [diff] [review]
> constant-id.diff
> 
> Holy cow. This patch will be very unpopular.
> 
> What's the issue here?

Reading between the lines, stully is discovering that we strongly enforce having only one set of Android resource IDs and therefore only one R.java.  You probably recall fighting with Sync to get the resources to work (or perhaps rnewman did that all): at that time, there was less support for having multiple resource sets that got merged together.

It is a longer term project to make Fennec build better with multiple resource sets, like what stully is hacking in here.  This shouldn't land as it is but is useful experimentation.

Comment 21

[Shane Tully (:stully:]

> A library project cannot have its resource IDs in R.java be static which means they can't be used as case labels. Otherwise, all the resources will be null.

Correction, they cannot be final; static is okay.

Comment 22

[Shane Tully (:stully:]

> It is a longer term project to make Fennec build better with multiple resource sets, like what stully is hacking in here.  This shouldn't land as it is but is useful experimentation.

The problem isn't multiple resource sets. By default, resource IDs are static final ints. The value of a static final int will be inlined into the bytecode. This means that the memory location this int points to is going to be invalid when the classes are used in a library project. The solution to this is to make the resource IDs non-final at the cost of not being able to use them as case labels.

If you want to keep them as final, every single resource reference in the code will be to be gotten by "getResources().getIdentifier(...)" which is much worse than just making the resource IDs non-final. Given the relatively small number of places in our code that resources are used as case labels, it seemed this was the lesser of two evils.

The only other option is to have two versions of each file that uses resources IDs as case lebels, one with a case structure and one with an if-else structure. This idea, obviously, shouldn't even be entertained.

Comment 23

[Mark Finkle (:mfinkle) (use needinfo?)]

More info:
http://tools.android.com/tips/non-constant-fields

Based on this, I think we'll need to move away from switch statements and use ugly if/else blocks.

BTW, I was able to get Fennec building with multiple resource sets in bug 901803 (the first obsoleted Cast SDK patch).

Comment 24

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/a3cc1c802366

Comment 25

[Chris Peterson [:cpeterson]]

(In reply to Nick Alexander :nalexander from comment #20)
> It is a longer term project to make Fennec build better with multiple
> resource sets, like what stully is hacking in here.  This shouldn't land as
> it is but is useful experimentation.

oops. I already landed the non-final resource ID patch yesterday. Should I back it out?

Comment 26

[Nick Alexander :nalexander [he/him]]

(In reply to Chris Peterson (:cpeterson) from comment #25)
> (In reply to Nick Alexander :nalexander from comment #20)
> > It is a longer term project to make Fennec build better with multiple
> > resource sets, like what stully is hacking in here.  This shouldn't land as
> > it is but is useful experimentation.
> 
> oops. I already landed the non-final resource ID patch yesterday. Should I
> back it out?

Nah, let it ride.  I'm not thrilled about having Makefile.in changes land without build peer review and I'm with blassey on not liking the if/elif chains, but no sense churning for churns sake.

Comment 27

[Shane Tully (:stully:]

Attachment: add-library-project-directory.diff

Everything needed to create zip files ready to be dropped into Eclipse for using GeckoView in a third party app.

Comment 28

[Nick Alexander :nalexander [he/him]]

Comment on attachment 790961 [details] [diff] [review]
add-library-project-directory.diff

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

This is moving in a good direction, but since there's no *build* per se (except moving some files -- keep that in the new Makefile.in), I'd like to see all of the zip/libs/assets logic moved into packager.mk.  There'll be fewer gnarly ../.. chains.  Also, prefer listing explicit files to update the zip with rather than globbing.  Lists are good!  Make sure to think about armv7/armv6/x86 too.

::: mobile/android/geckoview_library/.classpath
@@ +4,5 @@
> +	<classpathentry kind="src" path="gen"/>
> +	<classpathentry kind="con" path="com.android.ide.eclipse.adt.ANDROID_FRAMEWORK"/>
> +	<classpathentry exported="true" kind="con" path="com.android.ide.eclipse.adt.LIBRARIES">
> +		<attributes>
> +			<attribute name="org.eclipse.jdt.launching.CLASSPATH_ATTR_LIBRARY_PATH_ENTRY" value="geckoview_library/libs/armeabi-v7a"/>

x86?  ARMv6?  And how do the Fennec libraries (some of which are native) fit in here?  I'm not thrilled with shipping a .classpath, but I'm not clear on how you intend this to be used.

::: mobile/android/geckoview_library/Makefile.in
@@ +10,5 @@
> +include $(DEPTH)/config/autoconf.mk
> +
> +INSTALL_TARGETS += GECKOVIEW_LIBRARY
> +GECKOVIEW_LIBRARY_DEST = $(CURDIR)
> +GECKOVIEW_LIBRARY_FILES := AndroidManifest.xml \

nit: := \
\tfile1 \
\tfile2 \

@@ +25,5 @@
> +	# Make empty directories to fit an Android project structure
> +	mkdir -p bin gen libs/armeabi-v7a src
> +
> +	# JARs
> +	mv ../base/jars/* libs/

This will bust the previously built parts of the objdir.  You can $(CP), but I think you might want to do this whole business in packager.mk.

@@ +28,5 @@
> +	# JARs
> +	mv ../base/jars/* libs/
> +
> +	# Mozglue (this should be in a directory named 'libs', not 'lib')
> +	cp ../../../dist/bin/libmozglue.so libs/armeabi-v7a/

Again, x86/armv6.  We have fu to deal with this in packager.mk; you might need to extract a better framework.

@@ +40,5 @@
> +	cd ..; \
> +	zip -r ../../dist/geckoview_library/geckoview_library.zip geckoview_library
> +
> +	# The makefiles shouldn't be included in the zip
> +	zip -d ../../../dist/geckoview_library/geckoview_library.zip geckoview_library/backend.mk geckoview_library/Makefile

What is going into this?  Is it just a few static files (listed above) and then some of the jars and libs built above?  If so, we should be doing this all in packager.mk.

@@ +43,5 @@
> +	# The makefiles shouldn't be included in the zip
> +	zip -d ../../../dist/geckoview_library/geckoview_library.zip geckoview_library/backend.mk geckoview_library/Makefile
> +
> +	# Move the JARs and resources back
> +	mv libs/*.jar ../base/jars/

Well, that's good -- but prefer staging + $(CP).  And $(ZIP), $(MKDIR), etc while you're at it.

::: toolkit/mozapps/installer/packager.mk
@@ +344,5 @@
>  INNER_ROBOCOP_PACKAGE=echo 'Testing is disabled - No Robocop for you'
>  endif
>  
> +INNER_MAKE_GECKOVIEW_LIBRARY= \
> +  cd fennec && \

nit: subshell for the scope of the cd.

@@ +345,5 @@
>  endif
>  
> +INNER_MAKE_GECKOVIEW_LIBRARY= \
> +  cd fennec && \
> +  $(ZIP) -r ../geckoview_library/geckoview_assets.zip assets && \

Not clear why we have a separate assets zip.  I'd prefer one zip with all the things.

@@ +346,5 @@
>  
> +INNER_MAKE_GECKOVIEW_LIBRARY= \
> +  cd fennec && \
> +  $(ZIP) -r ../geckoview_library/geckoview_assets.zip assets && \
> +  cd -

Not clear on what cd - does.  subshell is better.

Comment 29

[Shane Tully (:stully:]

Attachment: add-library-project-directory.diff

Comment 30

[Shane Tully (:stully:]

Attachment: library-project.diff

Comment 31

[Shane Tully (:stully:]

Attachment: disable-content-providers.diff

Comment 32

[Chris Peterson [:cpeterson]]

Comment on attachment 792994 [details] [diff] [review]
disable-content-providers.diff

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

Maybe the content providers should be off by default and manually enabled by GeckoApp instead of adding hacks to GeckoView.

::: mobile/android/base/db/BrowserDB.java
@@ +19,5 @@
>  import java.util.List;
>  
>  public class BrowserDB {
>      public static String ABOUT_PAGES_URL_FILTER = "about:%";
> +    private static boolean sDisableContentProviders;

I think a name like `sAreContentProvidersDisabled` (or, flipping the logic, `sAreContentProvidersEnabled`) might be clearer because it describes a state, not an action.

@@ +213,4 @@
>      }
>  
>      public static boolean isReadingListItem(ContentResolver cr, String uri) {
> +        return (sDisableContentProviders ? false : sDb.isReadingListItem(cr, uri));

You can simplify these expressions to something like:

    return (!sDisableContentProviders && sDb.isReadingListItem(cr, uri));

Comment 33

[Chris Peterson [:cpeterson]]

Comment on attachment 792993 [details] [diff] [review]
library-project.diff

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

::: mobile/android/base/GeckoView.java
@@ +129,5 @@
> +    }
> +
> +    public Activity getActivity() {
> +        return (Activity)mContext;
> +    };

remove extra semicolon.

@@ +172,5 @@
> +    public void disableCameraView() {}
> +
> +    public void addAppStateListener(GeckoAppShell.AppStateListener listener) {}
> +
> +    public void removeAppStateListener(GeckoAppShell.AppStateListener listener) {}

Where are all these nop methods called from?

Comment 34

[Shane Tully (:stully:]

Attachment: disable-content-providers.diff

Comment 35

[Shane Tully (:stully:]

Attachment: library-project.diff

Comment 36

[Shane Tully (:stully:]

Attachment: add-library-project-directory.diff

Updated manifest with permissions for GeckoView

Comment 37

[Nick Alexander :nalexander [he/him]]

Comment on attachment 793128 [details] [diff] [review]
add-library-project-directory.diff

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

I'm confused about how this will be consumed.  Specifying .classpath suggests an Eclipse project (but then there should be .project, no)?  The inclusion of Makefile and backend.mk is almost certainly not wanted.  This is a strong f+ -- providing consumer build context might make this an r+.

::: mobile/android/geckoview_library/.classpath
@@ +4,5 @@
> +	<classpathentry kind="src" path="gen"/>
> +	<classpathentry kind="con" path="com.android.ide.eclipse.adt.ANDROID_FRAMEWORK"/>
> +	<classpathentry exported="true" kind="con" path="com.android.ide.eclipse.adt.LIBRARIES">
> +		<attributes>
> +			<attribute name="org.eclipse.jdt.launching.CLASSPATH_ATTR_LIBRARY_PATH_ENTRY" value="geckoview_library/libs/armeabi-v7a"/>

What was the word on all the different ABIs we support?

::: mobile/android/geckoview_library/Makefile.in
@@ +11,5 @@
> +
> +INSTALL_TARGETS += GECKOVIEW_LIBRARY
> +GECKOVIEW_LIBRARY_DEST = $(CURDIR)
> +GECKOVIEW_LIBRARY_FILES := \
> +	AndroidManifest.xml \

nit: sort these.  And two spaces at start rather than \t (Make is crazy).

::: toolkit/mozapps/installer/packager.mk
@@ +343,5 @@
>  else
>  INNER_ROBOCOP_PACKAGE=echo 'Testing is disabled - No Robocop for you'
>  endif
>  
> +INNER_MAKE_GECKOVIEW_LIBRARY= \

Can we get a one-line comment explaining what this does right before the command?  For example:

# Create geckoview_library/geckoview_{assets,library}.zip for third-party GeckoView consumers.

@@ +354,5 @@
> +  cp bin/libmozglue.so bin/lib/libplugin-container.so ../mobile/android/geckoview_library/libs/$(ABI_DIR)/ && \
> +  cp -R ../mobile/android/base/res ../mobile/android/geckoview_library && \
> +  ( cd ../mobile/android && \
> +    $(ZIP) -r ../../dist/geckoview_library/geckoview_library.zip geckoview_library) && \
> +  $(ZIP) -d geckoview_library/geckoview_library.zip geckoview_library/backend.mk geckoview_library/Makefile

I'm a bit confused here.  You almost certainly don't want the processed Makefile or backend.mk included here -- that's going to include a ton of things that third parties won't want.  (For example, includes like rules.mk.)

Comment 38

[Shane Tully (:stully:]

Attachment: add-library-project-directory.diff

> I'm confused about how this will be consumed.  Specifying .classpath suggests an Eclipse project (but then there should be .project, no)?

Yes, it will be an Eclipse project. The .project file should have been included in the patch, but I guess I missed it. The updated patch has it included.

Comment 39

[Chris Peterson [:cpeterson]]

Comment on attachment 793127 [details] [diff] [review]
library-project.diff

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

Brad should probably review these GeckoView changes.

Comment 40

[Chris Peterson [:cpeterson]]

Comment on attachment 793125 [details] [diff] [review]
disable-content-providers.diff

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

Brad should probably review these GeckoView changes.

Comment 41

[Shane Tully (:stully:]

https://tbpl.mozilla.org/?tree=Try&rev=c52d3c0355bf

Comment 42

[Nick Alexander :nalexander [he/him]]

Comment on attachment 793197 [details] [diff] [review]
add-library-project-directory.diff

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

Looks good.

Comment 43

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 793125 [details] [diff] [review]
disable-content-providers.diff

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

::: mobile/android/base/GeckoView.java
@@ +42,5 @@
> +        // If running outside of a GeckoActivity (eg, from a library project),
> +        // load the native code and disable content providers
> +        if (!(context instanceof GeckoActivity)) {
> +            BrowserDB.setEnableContentProviders(false);
> +        }

Just wanted to point out that this code won't get hit in Fennec because it is after the doInit check. This would also be true of any user that set doinit to fale, but they'd already be a little screwed anyway.

Comment 44

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 793127 [details] [diff] [review]
library-project.diff

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

You have the basics here (stubbed out or default implementations of GeckoInterface functions), but the simple cast of the Context to an Activity seems wrong.

::: mobile/android/base/GeckoView.java
@@ +135,5 @@
> +        return null;
> +    }
> +
> +    public Activity getActivity() {
> +        return (Activity)mContext;

not a fan of this. There is a reason that there is a separate getContext() and getActivity(). Also, if this were "the right thing to do", callers of getActivity() could instead call (Activity)getContext().

Instead, let's provide a BaseGeckoInterface class that takes an Activity in the constructor and stubs out or provides default implementations for everything else (like you've done here).

To make things "just work" we can do an instanceof test on the context we get in the GeckoView constructor and construct a BaseGeckoInterface using that.

Comment 45

[Shane Tully (:stully:]

Attachment: library-project.diff

> callers of getActivity() could instead call (Activity)getContext().

AFAIK, views do not have a getActivity() function so the only way to get an activity is to cast the context given in the constructor or returned by getContext() to an activity.

Regardless, other methods in the new BaseGeckoInterface need a context, so its constructor takes a context that is then cast to an activity. GeckoView checks that the context given to BaseGeckoInterface is, in fact, an instance of Activity.

Comment 46

[Nick Alexander :nalexander [he/him]]

Comment on attachment 793197 [details] [diff] [review]
add-library-project-directory.diff

Mike, could you look at the changes to packager.mk?

Context: this series of patches wraps up some of the Android assets and some new project files for consumption by third party developers who want to embed GeckoView on Android.  We create two zip files in the packaging step; third parties would configure an Eclipse (or other) project using the assets and the project files.

1.  It just occurred to me that some of the build artifacts that get zipped in the packaging step might not be present during some of the |make package| invocations on build bots.  If you see anything obvious, can you say?

2.  Eventually we want to distribute these zips via the buildbots.  Is there anything else needed except an UPLOAD_EXTRA_FILES line (like robocop.apk)?

3.  Should we be building these zips only conditionally (say for local and Nightly builds only)?

Thanks!

Comment 47

[Mike Hommey [:glandium]]

Comment on attachment 793197 [details] [diff] [review]
add-library-project-directory.diff

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

::: toolkit/mozapps/installer/packager.mk
@@ +355,5 @@
> +  cp bin/libmozglue.so bin/lib/libplugin-container.so ../mobile/android/geckoview_library/libs/$(ABI_DIR)/ && \
> +  cp -R ../mobile/android/base/res ../mobile/android/geckoview_library && \
> +  ( cd ../mobile/android && \
> +    $(ZIP) -r ../../dist/geckoview_library/geckoview_library.zip geckoview_library) && \
> +  $(ZIP) -d geckoview_library/geckoview_library.zip geckoview_library/backend.mk geckoview_library/Makefile

I'd rather see this as a $(MAKE) -C $(DEPTH)/mobile/android/geckoview_library package

with a package rule in mobile/android/geckoview_library/Makefile.in

That'd be more readable.

Note $(ZIP) has a -x option to exclude files when creating the archive instead of removing them with -d afterwards.

Comment 48

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 793741 [details] [diff] [review]
library-project.diff

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

r=blassey with nits addressed

::: mobile/android/base/BaseGeckoInterface.java
@@ +53,5 @@
> +    }
> +
> +    public SensorEventListener getSensorEventListener() {
> +        return null;
> +    }

file some follow up bugs to add set<>Listener() methods

@@ +55,5 @@
> +    public SensorEventListener getSensorEventListener() {
> +        return null;
> +    }
> +
> +    public void doRestart() {}

file a follow up bug to implement this (and put the bug number in a comment)

@@ +75,5 @@
> +    }
> +
> +    public void addPluginView(final View view, final RectF rect, final boolean isFullScreen) {}
> +
> +    public void removePluginView(final View view, final boolean isFullScreen) {}

guess what? file follow ups for implementations.

Actually, file a follow up for each method that is just stubbed out to get an implementation

::: mobile/android/base/GeckoView.java
@@ +45,5 @@
>  
>          // If running outside of a GeckoActivity (eg, from a library project),
>          // load the native code and disable content providers
>          if (!(context instanceof GeckoActivity)) {
> +            setGeckoInterface(context);

do your instanceof check here and create a BaseGeckoInterface to pass to setGeckoInterface(). Also, do a (getGeckoInterface() != null) check to make sure we're not overwriting the interface that the embedder has already set.

@@ +97,5 @@
>              requestRender();
>          }
>      }
>  
> +    public static void setGeckoInterface(final Context context) {

keep setGeckoInterface(GeckoInterface) because I'd like an implementer to be able to call it

Comment 49

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to Shane Tully (:stully: from comment #45)
> Created attachment 793741 [details] [diff] [review]
> library-project.diff
> 
> > callers of getActivity() could instead call (Activity)getContext().
> 
> AFAIK, views do not have a getActivity() function so the only way to get an
> activity is to cast the context given in the constructor or returned by
> getContext() to an activity.
I was referring to GeckoInterface.getActivity()

Comment 50

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 793741 [details] [diff] [review]
library-project.diff

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

r=blassey with nits addressed

::: mobile/android/base/BaseGeckoInterface.java
@@ +53,5 @@
> +    }
> +
> +    public SensorEventListener getSensorEventListener() {
> +        return null;
> +    }

file some follow up bugs to add set<>Listener() methods

@@ +55,5 @@
> +    public SensorEventListener getSensorEventListener() {
> +        return null;
> +    }
> +
> +    public void doRestart() {}

file a follow up bug to implement this (and put the bug number in a comment)

@@ +75,5 @@
> +    }
> +
> +    public void addPluginView(final View view, final RectF rect, final boolean isFullScreen) {}
> +
> +    public void removePluginView(final View view, final boolean isFullScreen) {}

guess what? file follow ups for implementations.

Actually, file a follow up for each method that is just stubbed out to get an implementation

::: mobile/android/base/GeckoView.java
@@ +45,5 @@
>  
>          // If running outside of a GeckoActivity (eg, from a library project),
>          // load the native code and disable content providers
>          if (!(context instanceof GeckoActivity)) {
> +            setGeckoInterface(context);

do your instanceof check here and create a BaseGeckoInterface to pass to setGeckoInterface(). Also, do a (getGeckoInterface() != null) check to make sure we're not overwriting the interface that the embedder has already set.

@@ +97,5 @@
>              requestRender();
>          }
>      }
>  
> +    public static void setGeckoInterface(final Context context) {

keep setGeckoInterface(GeckoInterface) because I'd like an implementer to be able to call it

Comment 51

[Shane Tully (:stully:]

Attachment: add-library-project-directory.diff

Comment 52

[Shane Tully (:stully:]

Attachment: library-project.diff

Comment 53

[Shane Tully (:stully:]

Documentation: https://wiki.mozilla.org/Mobile/GeckoView

Comment 54

[Mike Hommey [:glandium]]

Comment on attachment 794744 [details] [diff] [review]
add-library-project-directory.diff

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

Almost there. You'll need someone else's review for the xml files, though.

::: mobile/android/geckoview_library/Makefile.in
@@ +21,5 @@
> +include $(topsrcdir)/config/rules.mk
> +
> +package:
> +	# Make directory for the zips
> +	$(MKDIR) -p ../../../dist/geckoview_library

Please use $(DIST) instead of ../../../dist.

@@ +25,5 @@
> +	$(MKDIR) -p ../../../dist/geckoview_library
> +
> +	# Zip the assets
> +	cd ../../../dist/fennec; \
> +	$(ZIP) -r ../geckoview_library/geckoview_assets.zip assets

This is going to break some of the .so that need to *not* be deflated. See SZIP_LIBRARIES in packager.mk.

@@ +29,5 @@
> +	$(ZIP) -r ../geckoview_library/geckoview_assets.zip assets
> +
> +	# Make empty directories to fit an Android project structure
> +	$(MKDIR) -p bin gen libs/$(ABI_DIR) src
> +	

Please remove the various empty tabs in this file.

@@ +41,5 @@
> +	cp -R ../base/res .
> +	
> +	# Zip the directory
> +	cd ..; \
> +	$(ZIP) -r ../../dist/geckoview_library/geckoview_library.zip geckoview_library --exclude geckoview_library/backend.mk geckoview_library/Makefile

Does the zip need to have a root geckoview_library directory?

Comment 55

[Shane Tully (:stully:]

Attachment: add-library-project-directory.diff

Comment 56

[Mike Hommey [:glandium]]

Comment on attachment 794925 [details] [diff] [review]
add-library-project-directory.diff

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

::: mobile/android/geckoview_library/Makefile.in
@@ +25,5 @@
> +	$(MKDIR) -p $(DIST)/geckoview_library
> +
> +	# Zip the assets
> +	cd $(DIST)/fennec; \
> +	$(ZIP) -0 -r ../geckoview_library/geckoview_assets.zip assets

You're effectively making all assets uncompressed.
BTW, since this is not going to be an apk, how is an apk going to be created from this? Because that's actually where that matters most, that some files are kept uncompressed...

::: toolkit/mozapps/installer/packager.mk
@@ +345,5 @@
>  endif
>  
> +# Create geckoview_library/geckoview_{assets,library}.zip for third-party GeckoView consumers.
> +INNER_MAKE_GECKOVIEW_LIBRARY= \
> +  $(MAKE) -C ../mobile/android/geckoview_library package ABI_DIR=$(ABI_DIR) dist=$(DIST)

You don't need to pass DIST.

Comment 57

[Mike Hommey [:glandium]]

Comment on attachment 794925 [details] [diff] [review]
add-library-project-directory.diff

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

::: mobile/android/geckoview_library/Makefile.in
@@ +25,5 @@
> +	$(MKDIR) -p $(DIST)/geckoview_library
> +
> +	# Zip the assets
> +	cd $(DIST)/fennec; \
> +	$(ZIP) -0 -r ../geckoview_library/geckoview_assets.zip assets

As per the discussion on irc, you have my r+ if you remove that -0 and remove the dist=$(DIST) from packager.mk

Comment 58

[Mike Hommey [:glandium]]

Comment on attachment 794786 [details] [diff] [review]
library-project.diff

gah, wrong one

Comment 59

[Shane Tully (:stully:]

Attachment: add-library-project-directory.diff

Comment 60

[Chris Peterson [:cpeterson]]

Comment on attachment 794942 [details] [diff] [review]
add-library-project-directory.diff

Carrying forward r=blassey from comment 50

Comment 61

[Chris Peterson [:cpeterson]]

Comment on attachment 794786 [details] [diff] [review]
library-project.diff

Carrying forward r=glandium from comment 58

Comment 62

[Chris Peterson [:cpeterson]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/39ee92c06d6b
https://hg.mozilla.org/integration/mozilla-inbound/rev/59f669c61da5
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa20cf0458b5

Comment 63

[Mark Finkle (:mfinkle) (use needinfo?)]

I tried a build and package. After, <objdir>/dist/geckoview_library/*.zip files were created.

When I tired to create an Eclipse project (what a nightmare) I ended up with some missing references to a "geckoview_library/bin/geckoview.jar". Any idea what's going on? There is no "bin/geckoview.jar" file in my ZIP files.

Comment 64

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fa20cf0458b5
https://hg.mozilla.org/mozilla-central/rev/59f669c61da5
https://hg.mozilla.org/mozilla-central/rev/39ee92c06d6b

Comment 65

[Mark Finkle (:mfinkle) (use needinfo?)]

This seems to have broken l10n builds (repacks):
https://tbpl.mozilla.org/?showall=1&jobname=l10n%20nightly&rev=416075f77249

Comment 66

[Sina Momken]

(In reply to Mark Finkle (:mfinkle) from comment #63)
> I tried a build and package. After, <objdir>/dist/geckoview_library/*.zip
> files were created.
> 
> When I tired to create an Eclipse project (what a nightmare) I ended up with
> some missing references to a "geckoview_library/bin/geckoview.jar". Any idea
> what's going on? There is no "bin/geckoview.jar" file in my ZIP files.

Did you followed the instructions in https://wiki.mozilla.org/Mobile/GeckoView ?
According to those instructions you have to build 2 eclipse projects: a library project (named GeckoView) which must be imported from unzipped geckoview_library.zip and another eclipse android project (e.g. GeckoViewExample made by stully) which uses the first library project.
If you do all things correctly, after building GeckoViewExample project you will also build GeckoView library project. The "bin/geckoview.jar" is located under GeckoView/bin/geckoview.jar

Comment 67

[Sina Momken]

(In reply to Mark Finkle (:mfinkle) from comment #63)
> I tried a build and package. After, <objdir>/dist/geckoview_library/*.zip
> files were created.
> 
> When I tired to create an Eclipse project (what a nightmare) I ended up with
> some missing references to a "geckoview_library/bin/geckoview.jar". Any idea
> what's going on? There is no "bin/geckoview.jar" file in my ZIP files.

Notice that although in GeckoViewExample project which stully uploaded in https://wiki.mozilla.org/Mobile/GeckoView the library project is called "geckoview_library" but after importing geckoview_library from the zipped file geckoview_library.zip the imported project in eclipse will have the name "GeckoView".
I think this is the source of your problem. The solution is:
Right_click on GeckoViewExample project in eclipse -> properties -> Android -> delete reference to geckoview_library (it must have a red cross beside itself) -> read "GeckoView" instead

Comment 68

[Sina Momken]

I also have a problem too:
I can import, compile and build both GeckoView library project and GeckoViewExample project in eclipse. But when I run the GeckoViewExample app on my Android 4.2.2 device, it issues the error: Unfortunately, GeckoView Example has stopped. -> OK
I can't figure out what can be the problem. Anyone has any idea?

Comment 69

[Chris Peterson [:cpeterson]]

Backed out 39ee92c06d6b because GeckoView breaks Android's l10n repacks:
https://hg.mozilla.org/integration/mozilla-inbound/rev/35d97694d436

Comment 70

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: Pull resources dynamically to avoid R.java reorder

This patch fixes the bug where any drawable used in a GeckoView-based Application would potentially reorder the R.java identifiers and cause lookups in org.mozilla.gecko compiled JARs to fail. The two resources where this happens is: shadow and scrollbar, since they are the only ones hit during typical use of GeckoView.

The problem seems to be that the JARs in geckoview_library are compiled with specific values for the R.type.resource and when the Application that uses geckoview_library mixes in it's resources the R.java values can be changed. This is expected. What's not expected is that geckoview_library does not care about the changed values. It only knows about the original values.

Long story short, we can make it work if we dynamically load the resource identifiers for any *core* GeckoView resource.

99% of the resources in the geckoview_library will never be needed by the Application, so we could remove them in a separate bug.

Comment 71

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Chris Peterson (:cpeterson) from comment #69)
> Backed out 39ee92c06d6b because GeckoView breaks Android's l10n repacks:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/35d97694d436

Link to the bustage:
https://tbpl.mozilla.org/?showall=1&jobname=l10n%20nightly&rev=416075f77249

Comment 72

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: allow-mozconfig-disable

This patch adds a mozconfig option (--disable-geckoview-library) so we can skip any GeckoView work during the packaging.

Comment 73

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: disable-geckoview-l10n

Disable geckoview-library on any l10n builds. We are not ready to make it work there.

Comment 74

[Mike Hommey [:glandium]]

Comment on attachment 797650 [details] [diff] [review]
allow-mozconfig-disable

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

::: configure.in
@@ +5133,5 @@
> +    MOZ_DISABLE_GECKOVIEW=1,
> +    MOZ_DISABLE_GECKOVIEW=)
> +
> +if test -n "$MOZ_DISABLE_GECKOVIEW"; then
> +    AC_DEFINE(MOZ_DISABLE_GECKOVIEW)

You're not using the AC_DEFINE, so no need for it.

(AC_SUBST for things accessed in makefiles, AC_DEFINE for things accessed in code (and even for that, AC_DEFINE is most of the time overkill))

That being said, since this is a l10n releng build specific option, it would be better not to add one to the already long list of configure flags available. Just keep the AC_SUBST, and set MOZ_DISABLE_GECKOVIEW in the l10n mozconfigs.

Comment 75

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: allow-mozconfig-disable v0.2

This patch removes the AC_DEFINE support and just keeps the AC_SUBST support

Comment 76

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: disable-geckoview-l10n v0.2

This patch disables GeckoView for l10n builds by exporting the MOZ_DISABLE_GECKOVIEW flag directly. Tested locally and it does disable the library from being packaged.

Comment 77

[Mike Hommey [:glandium]]

Comment on attachment 799445 [details] [diff] [review]
disable-geckoview-l10n v0.2

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

::: mobile/android/config/mozconfigs/android-armv6/l10n-nightly
@@ +33,5 @@
>  
>  export JAVA_HOME=/tools/jdk6
>  export MOZILLA_OFFICIAL=1
>  export MOZ_PKG_SPECIAL=armv6
> +export MOZ_DISABLE_GECKOVIEW=1

I think you don't need to export, but okay either way.

Comment 78

[Mark Finkle (:mfinkle) (use needinfo?)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9ff14d43346d
https://hg.mozilla.org/integration/mozilla-inbound/rev/965457d9bc1e
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f3ef3de5617

Comment 79

[Mark Finkle (:mfinkle) (use needinfo?)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cfcf5b7edc53

Comment 80

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/9ff14d43346d
https://hg.mozilla.org/mozilla-central/rev/965457d9bc1e
https://hg.mozilla.org/mozilla-central/rev/9f3ef3de5617
https://hg.mozilla.org/mozilla-central/rev/cfcf5b7edc53

Comment 81

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Backed out:

https://hg.mozilla.org/integration/mozilla-inbound/rev/feffc2cf422e
https://hg.mozilla.org/integration/mozilla-inbound/rev/0fa38163e3bf

because https://hg.mozilla.org/mozilla-central/rev/9ff14d43346d didn't seem to have build peer review.

Comment 82

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 794942 [details] [diff] [review]
add-library-project-directory.diff

Actually, Mike already reviewed this, so I'm carrying it forward:
https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=880118&attachment=794925

Comment 83

[Mark Finkle (:mfinkle) (use needinfo?)]

... and relanded:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/cb3a85808d77
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b7396ea8f548
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2a9bde2ff860
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/fff320870b20

Comment 84

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/cb3a85808d77
https://hg.mozilla.org/mozilla-central/rev/b7396ea8f548
https://hg.mozilla.org/mozilla-central/rev/2a9bde2ff860
https://hg.mozilla.org/mozilla-central/rev/fff320870b20

Comment 85

[chrishou118]

I am using geckoview lib. I got an resource not found exception.
I solved this excepiton in a tricky way.
I unpack gecko-browser.jar, rm the R classes and then re-pack to gecko-browser.jar
replace origin gecko-broser.jar.
Then ant debug. install apk to device. This will result java.lang.NoClassDefFoundError: org.mozilla.gecko.R$styleable.
Then copy the geckoview fold in gen/org/mozilla/,rename to gecko. 
Edit the gen/org/mozilla/gecko/R.java, change package name to org.mozilla.gecko
then ant debug, install apk to device.
It works. Because the classloader can load the right R.class which have the right int id value.

And I found your are working on fix the issue.https://bugzilla.mozilla.org/attachment.cgi?id=797273&action=diff
I think there are two methons to avoid R.java reorder.
1. getResources().getIdentifier(), as in the diff
2. Change the R.java's package name to "org.mozilla.geckoview", Do not package R.class into gecko-browser.jar. Actually I only try in a HelloProject, Because I don't known how to change the build script this project. Any suggestion is appreciated

P.S. I tried another method. Change the gecko library project's package name to "org.mozilla.gecko". But I got a so runtime exception. Is gecko library project's package name must be "org.mozilla.geckoview"?

(In reply to Mark Finkle (:mfinkle) from comment #70)
> Created attachment 797273 [details] [diff] [review]
> Pull resources dynamically to avoid R.java reorder
> 
> This patch fixes the bug where any drawable used in a GeckoView-based
> Application would potentially reorder the R.java identifiers and cause
> lookups in org.mozilla.gecko compiled JARs to fail. The two resources where
> this happens is: shadow and scrollbar, since they are the only ones hit
> during typical use of GeckoView.
> 
> The problem seems to be that the JARs in geckoview_library are compiled with
> specific values for the R.type.resource and when the Application that uses
> geckoview_library mixes in it's resources the R.java values can be changed.
> This is expected. What's not expected is that geckoview_library does not
> care about the changed values. It only knows about the original values.
> 
> Long story short, we can make it work if we dynamically load the resource
> identifiers for any *core* GeckoView resource.
> 
> 99% of the resources in the geckoview_library will never be needed by the
> Application, so we could remove them in a separate bug.

Comment 86

[Ali Kasimoglu]

We are implementing GeckoView in our project. But we have a problem with clear cookies.
Can somebody say, what class clear cookies in geckoview? Urgent! Best Regards

Comment 87

[Botond Ballo [:botond]]

(In reply to Ali Kasimoglu from comment #86)
> We are implementing GeckoView in our project. But we have a problem with
> clear cookies.
> Can somebody say, what class clear cookies in geckoview? Urgent! Best Regards

You're more likely to get an answer if you ask on the mobile-firefox-dev mailing list [1], or in the #mobile IRC channel [2].

[1] https://mail.mozilla.org/listinfo/mobile-firefox-dev
[2] https://wiki.mozilla.org/IRC

Comment 88

[Ali Kasimoglu]

(In reply to Botond Ballo [:botond] from comment #87)
> (In reply to Ali Kasimoglu from comment #86)
> > We are implementing GeckoView in our project. But we have a problem with
> > clear cookies.
> > Can somebody say, what class clear cookies in geckoview? Urgent! Best Regards
> 
> You're more likely to get an answer if you ask on the mobile-firefox-dev
> mailing list [1], or in the #mobile IRC channel [2].
> 
> [1] https://mail.mozilla.org/listinfo/mobile-firefox-dev
> [2] https://wiki.mozilla.org/IRC

thank you