Comment on attachment 821092 [details] [diff] [review] geckoview_example.patch ># HG changeset patch ># User Brad Lassey <blassey@mozilla.com> ># 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 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.

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