978587 - Fix geckoview_library resource packaging

Status: RESOLVED FIXED

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

Description

[Nick Alexander :nalexander [he/him]]

At the moment, we declare the geckoview_library package to be org.mozilla.geckoview.  The geckoview_library shipped .jar files include org.mozilla.gecko.R.  Both of these are wrong.

We need to not ship org.mozilla.gecko.R (that is provided by the consuming application), and we need to declare the geckoview_library package to be org.mozilla.gecko (so that the consuming application generates org.mozilla.gecko.R).

With these tweaks, we have a geckoview_library that addresses https://bugzilla.mozilla.org/show_bug.cgi?id=977677#c5.

Comment 1

[Nick Alexander :nalexander [he/him]]

Attachment: Pre: Let Java JARs contain either sources or generated sources. r=blassey

Comment 2

[Nick Alexander :nalexander [he/him]]

Attachment: Pre: Make geckoview_library and BackgroundInstrumentationTests target android-16. r=blassey

This agrees with Fennec (and BrowserInstrumentationTests).

Comment 3

[Nick Alexander :nalexander [he/him]]

Attachment: Make GeckoView package org.mozilla.gecko; don't include org.mozilla.gecko.R. r=blassey

This builds a new Java JAR containing only org.mozilla.gecko.R.  This
new JAR file is included in Fennec, but not included in
geckoview_library.  (Usually ant, gradle, or Eclipse arranges to produce
org.mozilla.gecko.R but not include it in the classes.jar output as part
of a library project.  This mimics that.)

Changing the GeckoView package to org.mozilla.gecko declares to
consuming applications that they should produce org.mozilla.gecko.R,
replacing what was removed above (but with correct resource IDs).

Comment 4

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

Comment on attachment 8384994 [details] [diff] [review]
Pre: Make geckoview_library and BackgroundInstrumentationTests target android-16. r=blassey

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

::: mobile/android/geckoview_library/project.properties
@@ +10,5 @@
>  # To enable ProGuard to shrink and obfuscate your code, uncomment this (available properties: sdk.dir, user.home):
>  #proguard.config=${sdk.dir}/tools/proguard/proguard-android.txt:proguard-project.txt
>  
>  # Project target.
> +target=android-16

can we make this a preprocessed file? I'd hate to have to keep the SDK level up to date across multiple files. We'll be bumping our build to SDK 17 soon-ish in bug 933189.

::: mobile/android/tests/background/junit3/AndroidManifest.xml.in
@@ +6,5 @@
>      android:versionCode="1"
>      android:versionName="1.0" >
>  
>      <uses-sdk android:minSdkVersion="8"
> +              android:targetSdkVersion="16" />

This is already a preprocessed file, so let's keep the SDK level current. You can get a Makefile variable assigned to the SDK level with:

TARGET= $(notdir $(ANDROID_SDK))

Comment 5

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

Comment on attachment 8384995 [details] [diff] [review]
Make GeckoView package org.mozilla.gecko; don't include org.mozilla.gecko.R. r=blassey

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

So the expectation here is that embedders of GeckoView will have to provide matching resource IDs?

I'm wondering now if we should package gecko resources and have a wrapper to do some fallback. For example from https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/ThumbnailHelper.java#57, rather than: 
57         try {
58             mPendingWidth = new AtomicInteger((int)GeckoAppShell.getContext().getResources().getDimension(R.dimen.tab_thumbnail_width));
59         } catch (Resources.NotFoundException nfe) { mPendingWidth = new AtomicInteger(0); }

we could have 
57         try {
58             mPendingWidth = new AtomicInteger((int)GeckoAppShell.getContext().getResources().getDimension(R.dimen.tab_thumbnail_width));
59         } catch (Resources.NotFoundException nfe) {
60             mPendingWidth = new AtomicInteger((int)GeckoAppShell.getContext().getResources().getDimension(org.mozilla.gecko.R.dimen.tab_thumbnail_width));
61         }

Comment 6

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

NI for an opinion on my suggestion in comment 5

Comment 7

[Nick Alexander :nalexander [he/him]]

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #4)
> Comment on attachment 8384994 [details] [diff] [review]
> Pre: Make geckoview_library and BackgroundInstrumentationTests target
> android-16. r=blassey
> 
> Review of attachment 8384994 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/geckoview_library/project.properties
> @@ +10,5 @@
> >  # To enable ProGuard to shrink and obfuscate your code, uncomment this (available properties: sdk.dir, user.home):
> >  #proguard.config=${sdk.dir}/tools/proguard/proguard-android.txt:proguard-project.txt
> >  
> >  # Project target.
> > +target=android-16
> 
> can we make this a preprocessed file? I'd hate to have to keep the SDK level
> up to date across multiple files. We'll be bumping our build to SDK 17
> soon-ish in bug 933189.
> 
> ::: mobile/android/tests/background/junit3/AndroidManifest.xml.in
> @@ +6,5 @@
> >      android:versionCode="1"
> >      android:versionName="1.0" >
> >  
> >      <uses-sdk android:minSdkVersion="8"
> > +              android:targetSdkVersion="16" />
> 
> This is already a preprocessed file, so let's keep the SDK level current.
> You can get a Makefile variable assigned to the SDK level with:
> 
> TARGET= $(notdir $(ANDROID_SDK))

I thought about doing this, 'cuz I had the same concern about bumping target versions as you, and decided not to after a little searching.  But I've filed Bug 979438 to track.  I'd prefer to not block on that, but if you feel very strongly, we can.

Comment 8

[Nick Alexander :nalexander [he/him]]

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #5)
> Comment on attachment 8384995 [details] [diff] [review]
> Make GeckoView package org.mozilla.gecko; don't include org.mozilla.gecko.R.
> r=blassey
> 
> Review of attachment 8384995 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> So the expectation here is that embedders of GeckoView will have to provide
> matching resource IDs?

Not exactly.  Embedders of GeckoView must include all of the resources that GeckoView (now Fennec) depends on, lest bad things happen.  This is a non-negotiable part of using an Android library.

But what you propose is not necessary.  Both the R's referenced in GeckoView will correspond to the Java package org.mozilla.gecko.R.  The point is that the org.mozilla.gecko.R referenced needs to be generated and provided by the embedder (which ant already arranges for, providing that you ask it correctly, which is the name change in the geckoview_library manifest).  It can't be provided by the library as compiled code (and this is what the JAR juggling removes).

So there are no additional requirements on embedders other than to include the GeckoView resource set.

Comment 9

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

Comment on attachment 8384994 [details] [diff] [review]
Pre: Make geckoview_library and BackgroundInstrumentationTests target android-16. r=blassey

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

I'm happy with a follow up

Comment 10

[Nick Alexander :nalexander [he/him]]

https://hg.mozilla.org/integration/fx-team/rev/aae39725a68d
https://hg.mozilla.org/integration/fx-team/rev/53cfd01ff0d3
https://hg.mozilla.org/integration/fx-team/rev/519c9181051a

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/aae39725a68d
https://hg.mozilla.org/mozilla-central/rev/53cfd01ff0d3
https://hg.mozilla.org/mozilla-central/rev/519c9181051a