Closed Bug 978587 Opened 7 years ago Closed 7 years ago

Fix geckoview_library resource packaging

Categories

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

All
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla30

People

(Reporter: nalexander, Assigned: nalexander)

References

Details

Attachments

(3 files)

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.
See Also: → 977677
This agrees with Fennec (and BrowserInstrumentationTests).
Attachment #8384994 - Flags: review?(blassey.bugs)
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).
Attachment #8384995 - Flags: review?(blassey.bugs)
Attachment #8384993 - Flags: review?(blassey.bugs) → review+
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))
Attachment #8384994 - Flags: review?(blassey.bugs) → review-
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         }
NI for an opinion on my suggestion in comment 5
Flags: needinfo?(nalexander)
(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.
Flags: needinfo?(nalexander)
(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 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
Attachment #8384994 - Flags: review- → review+
Attachment #8384995 - Flags: review?(blassey.bugs) → review+
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.