930072 - Provide example app using GeckoView library in the tree

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

Attachment: geckoview_example.patch

Comment 1

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

Comment on attachment 821092 [details] [diff] [review]
geckoview_example.patch

># HG changeset patch
># User Brad Lassey <[e-mail]>
># Date 1382546234 -7200
># Node ID 9e423b9a0d0ad037a10f884641629be017ef3f23
># Parent  caa250c374332f3e930e17fc1baac953bfffef18
>[mq]: embedding_makefile
>
>diff --git a/embedding/android/geckoview_example/AndroidManifest.xml b/embedding/android/geckoview_example/AndroidManifest.xml
>new file mode 100644
>--- /dev/null
>+++ b/embedding/android/geckoview_example/AndroidManifest.xml
>@@ -0,0 +1,25 @@
>+<?xml version="1.0" encoding="utf-8"?>
>+<manifest xmlns:android="http://schemas.android.com/apk/res/android"
>+      package="org.mozilla.geckoviewexample"
>+      android:versionCode="1"
>+      android:versionName="1.0">

nit: You align the attributes to the first attribute in the main line in the rest of this file, like this:

<manifest xmlns:android="http://schemas.android.com/apk/res/android"
          package="org.mozilla.geckoviewexample"


>+        </activity>
>+
>+
>+
>+    </application>

nit: Remove the blank lines

>diff --git a/embedding/android/geckoview_example/GeckoViewExample.java b/embedding/android/geckoview_example/GeckoViewExample.java

>+public class GeckoViewExample extends Activity
>+{

>+    public void onCreate(Bundle savedInstanceState)
>+    {

nit: Do we use { at the end of the previous line in Fennec code?

>+        //AttributeSet attrs = null; //new AttributeSet();
>+        //org.mozilla.gecko.GeckoView view = new org.mozilla.gecko.GeckoView(this, attrs);

Can you remove these?

>diff --git a/embedding/android/geckoview_example/Makefile.in b/embedding/android/geckoview_example/Makefile.in

>+GARBAGE = \
>+	AndroidManifest.xml \
>+  proguard-project.txt \
>+  project.properties \
>+	ant.properties \
>+	build.xml \
>+	local.properties \
>+	$(NULL)

indent issue

>+ANDROID=$(ANDROID_SDK)/../../tools/android

I assume a build peer will look at this too, but is this a good way to get the ANDROID binary? Do we already define a variable to it?

>diff --git a/embedding/android/geckoview_example/main.xml b/embedding/android/geckoview_example/main.xml

>+<LinearLayout xmlns:android="http://schemas.android.com/apk/res/android"
>+              xmlns:gecko="http://schemas.android.com/apk/res-auto"
>+    android:orientation="vertical"
>+    android:layout_width="fill_parent"
>+    android:layout_height="fill_parent"

nit: indent to "xmlns"

>+<TextView
>+    android:layout_width="fill_parent"

nit: pop this up to the above line

>+    android:layout_height="wrap_content"
>+    android:text="Hello World, GeckoViewExample 2"

indent these lines to the previous "popped up" attribute

>+<org.mozilla.gecko.GeckoView 
>+      android:id="@+id/gecko_view"

nit: pop this up to the above line

>+      android:layout_width="fill_parent"
>+      android:layout_height="fill_parent"

indent these lines to the previous "popped up" attribute

>+      gecko:url="http://mit.edu/"/>

Before we start using this app for tests, we need to use a local URL. We aren't allowed to use external URLs in tests.

>diff --git a/embedding/android/geckoview_example/moz.build b/embedding/android/geckoview_example/moz.build

>+MODULE = "geckoview_example"
>\ No newline at end of file

Add a new line?

Maybe grab Nick Alexander to review the moz.build and Makefile bits?

r+ with nits on the rest from me

Comment 2

[Nick Alexander :nalexander [he/him]]

Comment on attachment 821092 [details] [diff] [review]
geckoview_example.patch

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

This doesn't appear to include the relevant ANT files, nor the referenced Android resources.

On a more serious note, I'm confused as to why this would live in embedding/.  I see no precedent for platform-specific libraries in embedding, and it doesn't agree with my understanding of what embedding was -- but I wasn't around when embedding on Android was real.  I think this should live under mobile/android/; in future we should build an APK for this project using the build system; and we should run a tiny instrumentation or Robotium test suite against that APK on buildbots.  Doing this in embedding/ is, of course, possible -- but makes less sense to me.

::: embedding/android/geckoview_example/AndroidManifest.xml
@@ +1,3 @@
> +<?xml version="1.0" encoding="utf-8"?>
> +<manifest xmlns:android="http://schemas.android.com/apk/res/android"
> +      package="org.mozilla.geckoviewexample"

org.mozilla.geckoview.example?  Seems likely we'll end up with code in org.mozilla.geckoview.* on trunk.  Your call.

@@ +6,5 @@
> +    <uses-sdk android:minSdkVersion="8"
> +              android:targetSdkVersion="16"/>
> +    <uses-permission android:name="android.permission.ACCESS_NETWORK_STATE"/>
> +    <uses-permission android:name="android.permission.INTERNET"/>
> +    <uses-feature android:glEsVersion="0x00020000" android:required="true" />

Why?  We don't require this in Fennec's manifest (or I can't find it.)

@@ +7,5 @@
> +              android:targetSdkVersion="16"/>
> +    <uses-permission android:name="android.permission.ACCESS_NETWORK_STATE"/>
> +    <uses-permission android:name="android.permission.INTERNET"/>
> +    <uses-feature android:glEsVersion="0x00020000" android:required="true" />
> +    <application android:label="@string/app_name" 

nit: trailing ws.

@@ +17,5 @@
> +                <action android:name="android.intent.action.MAIN" />
> +                <category android:name="android.intent.category.LAUNCHER" />
> +            </intent-filter>
> +        </activity>
> +

nit: ws.

::: embedding/android/geckoview_example/GeckoViewExample.java
@@ +10,5 @@
> +    @Override
> +    public void onCreate(Bundle savedInstanceState)
> +    {
> +        super.onCreate(savedInstanceState);
> +        //AttributeSet attrs = null; //new AttributeSet();

Kill or explain.

::: embedding/android/geckoview_example/Makefile.in
@@ +1,4 @@
> +include $(topsrcdir)/config/rules.mk
> +
> +GARBAGE = \
> +	AndroidManifest.xml \

nit: indentation.

@@ +9,5 @@
> +	local.properties \
> +	$(NULL)
> +
> +GARBAGE_DIRS = \
> +	assets 	\

nit: indentation.  Two spaces!

@@ +10,5 @@
> +	$(NULL)
> +
> +GARBAGE_DIRS = \
> +	assets 	\
> +	geckoview_library	\

and weird spaces all over.

@@ +20,5 @@
> +	$(NULL)
> +
> +ANDROID=$(ANDROID_SDK)/../../tools/android
> +build.xml:
> +	$(ANDROID)  create project --name GeckoViewExample --target android-18 --path `pwd` --activity GeckoViewExample --package org.mozilla.geckoviewexample

Save shell invocations: use $CURDIR instead of `pwd`.

@@ +23,5 @@
> +build.xml:
> +	$(ANDROID)  create project --name GeckoViewExample --target android-18 --path `pwd` --activity GeckoViewExample --package org.mozilla.geckoviewexample
> +	$(ANDROID) update project --target android-18 --path `pwd` --library $(DEPTH)/mobile/android/geckoview_library
> +	$(UNZIP) -o $(DIST)/geckoview_library/geckoview_assets.zip
> +	$(NSINSTALL) $(abspath $(srcdir))/main.xml res/layout/

why are these abspath calls necessary?

::: embedding/android/geckoview_example/main.xml
@@ +1,3 @@
> +<?xml version="1.0" encoding="utf-8"?>
> +<LinearLayout xmlns:android="http://schemas.android.com/apk/res/android"
> +              xmlns:gecko="http://schemas.android.com/apk/res-auto"

nit: indentation.

@@ +7,5 @@
> +    >
> +<TextView
> +    android:layout_width="fill_parent"
> +    android:layout_height="wrap_content"
> +    android:text="Hello World, GeckoViewExample 2"

2?

@@ +9,5 @@
> +    android:layout_width="fill_parent"
> +    android:layout_height="wrap_content"
> +    android:text="Hello World, GeckoViewExample 2"
> +    />
> +<org.mozilla.gecko.GeckoView 

nit: trailing ws.

@@ +13,5 @@
> +<org.mozilla.gecko.GeckoView 
> +      android:id="@+id/gecko_view"
> +      android:layout_width="fill_parent"
> +      android:layout_height="fill_parent"
> +      gecko:url="http://mit.edu/"/>

I know this makes no difference, but why are we sending people to MIT?  Shouldn't we be sending folks to some neat Mozilla property?

::: embedding/android/geckoview_library/Makefile.in
@@ +1,4 @@
> +include $(topsrcdir)/config/rules.mk
> +
> +build.xml:
> +	$(ANDROID) $(ANDROID_SDK)/../../tools/android create lib-project --name GeckoView --target android-18 --path `pwd` --package org.mozilla.geckoview

You defined $(ANDROID) in a different makefile, but not here.  And why are you specifying android-18 instead of 16?

@@ +1,5 @@
> +include $(topsrcdir)/config/rules.mk
> +
> +build.xml:
> +	$(ANDROID) $(ANDROID_SDK)/../../tools/android create lib-project --name GeckoView --target android-18 --path `pwd` --package org.mozilla.geckoview
> +	mv res/values/strings.xml res/values/strings2.xml

Whatever this is, it's not a good idea.

@@ +3,5 @@
> +build.xml:
> +	$(ANDROID) $(ANDROID_SDK)/../../tools/android create lib-project --name GeckoView --target android-18 --path `pwd` --package org.mozilla.geckoview
> +	mv res/values/strings.xml res/values/strings2.xml
> +	$(UNZIP) -o $(DIST)/geckoview_library/geckoview_library.zip
> +	cp -r geckoview_library/libs/* libs/

Is it the case that the |create lib-project| above always creates libs/ and res/?  If not, guard with mkdir_deps.  Or NSINSTALL -D.

Comment 3

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

Attachment: geckoview_example.patch.patch

Comment 4

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

(In reply to Nick Alexander :nalexander from comment #2)
> Comment on attachment 821092 [details] [diff] [review]
> geckoview_example.patch
> 
> Review of attachment 821092 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This doesn't appear to include the relevant ANT files, nor the referenced
> Android resources.
All of that is generated by the "android create project..." line in the Makefile
> 
> On a more serious note, I'm confused as to why this would live in
> embedding/.  I see no precedent for platform-specific libraries in
> embedding, and it doesn't agree with my understanding of what embedding was
> -- but I wasn't around when embedding on Android was real.  I think this
> should live under mobile/android/; in future we should build an APK for this
> project using the build system; and we should run a tiny instrumentation or
> Robotium test suite against that APK on buildbots.  Doing this in embedding/
> is, of course, possible -- but makes less sense to me.

This is exactly what the embedding directory has been used for traditionally. This is very analogous to WinEmbed, a stand alone app at embeds gecko.

It very specifically should not be in mobile/android in that mobile/ is an application directory.

I would like this to create the basic geckoview test for bug 921664, but that is decidedly step-next

> 
> ::: embedding/android/geckoview_example/AndroidManifest.xml
> @@ +1,3 @@
> > +<?xml version="1.0" encoding="utf-8"?>
> > +<manifest xmlns:android="http://schemas.android.com/apk/res/android"
> > +      package="org.mozilla.geckoviewexample"
> 
> org.mozilla.geckoview.example?  Seems likely we'll end up with code in
> org.mozilla.geckoview.* on trunk.  Your call.
huh?

 
> @@ +6,5 @@
> > +    <uses-sdk android:minSdkVersion="8"
> > +              android:targetSdkVersion="16"/>
> > +    <uses-permission android:name="android.permission.ACCESS_NETWORK_STATE"/>
> > +    <uses-permission android:name="android.permission.INTERNET"/>
> > +    <uses-feature android:glEsVersion="0x00020000" android:required="true" />
> 
> Why?  We don't require this in Fennec's manifest (or I can't find it.)
https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/AndroidManifest.xml.in#73


> @@ +1,5 @@
> > +include $(topsrcdir)/config/rules.mk
> > +
> > +build.xml:
> > +	$(ANDROID) $(ANDROID_SDK)/../../tools/android create lib-project --name GeckoView --target android-18 --path `pwd` --package org.mozilla.geckoview
> > +	mv res/values/strings.xml res/values/strings2.xml
> 
> Whatever this is, it's not a good idea.
this might not be needed anymore
> 
> @@ +3,5 @@
> > +build.xml:
> > +	$(ANDROID) $(ANDROID_SDK)/../../tools/android create lib-project --name GeckoView --target android-18 --path `pwd` --package org.mozilla.geckoview
> > +	mv res/values/strings.xml res/values/strings2.xml
> > +	$(UNZIP) -o $(DIST)/geckoview_library/geckoview_library.zip
> > +	cp -r geckoview_library/libs/* libs/
> 
> Is it the case that the |create lib-project| above always creates libs/ and
> res/?  If not, guard with mkdir_deps.  Or NSINSTALL -D.

it does

Comment 5

[Nick Alexander :nalexander [he/him]]

Comment on attachment 828860 [details] [diff] [review]
geckoview_example.patch.patch

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

r+ with nits addressed.  Sorry for the slow review, I processed your comments and looked at the code and forgot to finish it off.

::: embedding/android/geckoview_example/AndroidManifest.xml
@@ +7,5 @@
> +              android:targetSdkVersion="16"/>
> +    <uses-permission android:name="android.permission.ACCESS_NETWORK_STATE"/>
> +    <uses-permission android:name="android.permission.INTERNET"/>
> +    <uses-feature android:glEsVersion="0x00020000" android:required="true" />
> +    <application android:label="@string/app_name" 

nit: trailing whitespace.

::: embedding/android/geckoview_example/Makefile.in
@@ +21,5 @@
> +
> +ANDROID=$(ANDROID_SDK)/../../tools/android
> +
> +build.xml:
> +  $(ANDROID)  create project --name GeckoViewExample --target android-18 --path `pwd` --activity GeckoViewExample --package org.mozilla.geckoviewexample

This looks like two spaces (a little hard to tell in splinter) but should be leading tabs.  Also, nit two spaces between $(ANDROID)  create.

@@ +22,5 @@
> +ANDROID=$(ANDROID_SDK)/../../tools/android
> +
> +build.xml:
> +  $(ANDROID)  create project --name GeckoViewExample --target android-18 --path `pwd` --activity GeckoViewExample --package org.mozilla.geckoviewexample
> +  $(ANDROID) update project --target android-18 --path `pwd` --library $(DEPTH)/mobile/android/geckoview_library

$CURDIR instead of `pwd`.

@@ +24,5 @@
> +build.xml:
> +  $(ANDROID)  create project --name GeckoViewExample --target android-18 --path `pwd` --activity GeckoViewExample --package org.mozilla.geckoviewexample
> +  $(ANDROID) update project --target android-18 --path `pwd` --library $(DEPTH)/mobile/android/geckoview_library
> +  $(UNZIP) -o $(DIST)/geckoview_library/geckoview_assets.zip
> +  $(NSINSTALL) $(abspath $(srcdir))/main.xml res/layout/

$(DEPTH) instead of $(abspath $(srcdir))?

@@ +27,5 @@
> +  $(UNZIP) -o $(DIST)/geckoview_library/geckoview_assets.zip
> +  $(NSINSTALL) $(abspath $(srcdir))/main.xml res/layout/
> +  $(NSINSTALL) $(abspath $(srcdir))/AndroidManifest.xml .
> +  $(NSINSTALL) $(abspath $(srcdir))/GeckoViewExample.java src/org/mozilla/geckoviewexample/
> +  echo jar.libs.dir=libs >> project.properties

If the following is not true, please respond: this works because "android create project" recreates project.properties.

::: embedding/android/geckoview_library/Makefile.in
@@ +1,5 @@
> +include $(topsrcdir)/config/rules.mk
> +
> +build.xml:
> +	$(ANDROID) $(ANDROID_SDK)/../../tools/android create lib-project --name GeckoView --target android-18 --path `pwd` --package org.mozilla.geckoview
> +	mv res/values/strings.xml res/values/strings2.xml

Kill this.

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/371a2267b7a8