Closed Bug 976699 Opened 8 years ago Closed 7 years ago

Place architecture-specific assets in a different subdirectory

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 36

People

(Reporter: toonetown, Assigned: nalexander)

References

Details

Attachments

(2 files)

Attached patch arch-assets.diffSplinter Review
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_9_1) AppleWebKit/537.73.11 (KHTML, like Gecko) Version/7.0.1 Safari/537.73.11

Steps to reproduce:

It would be nice to have architecture-specific assets placed within the apk at assets/{architecture} - thus facilitating the ability to create a single apk for multiple platforms.



Expected results:

The attached patch allows you to specify ANDROID_ASSET_ARCH in a mozconfig in order to place the asset libraries in that subdirectory.  For example, after applying the patch, you can add:
export ANDROID_ASSET_ARCH=armeabi-v7a
and asset libraries will be placed within (and loaded from) the apk at assets/armeabi-v7a.  The location of the omni.ja file is unchanged.
Comment on attachment 8381592 [details] [diff] [review]
arch-assets.diff

Not sure if you wanted this looked at or actually reviewed, feel free to flip flag on the patch to review.
Attachment #8381592 - Attachment is patch: true
Attachment #8381592 - Attachment mime type: text/x-patch → text/plain
Attachment #8381592 - Flags: feedback?(mh+mozilla)
This is just a change that I implemented after a (quick) discussion on the #mobile IRC channel.  I submitted the bug in the spirit of helping out...

I don't know if it will be valuable or helpful to others - it's more of a "feature request" than a "bug".  I just wasn't sure what the process for submitting patches back upstream was, so I figured I'd log it here.  Feel free to review/give feedback/close/delete as makes sense for the overall project.
Comment on attachment 8381592 [details] [diff] [review]
arch-assets.diff

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

Sorry for the long delay, I was heads down on something.
Seems to me ANDROID_ASSET_ARCH should always have the same value as ANDROID_CPU_ARCH, and that there is no value in having this being conditional, but I'd rather leave this for nalexander to tell.
Attachment #8381592 - Flags: feedback?(mh+mozilla) → feedback?(nalexander)
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Mac OS X → Android
Hardware: x86 → All
Comment on attachment 8381592 [details] [diff] [review]
arch-assets.diff

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

I like this idea, and think we should do something like it.  This could be valuable for GeckoView consumers; it's possible they'll want to ship multi-arch APKs.  But I would like to see a slightly different approach.

1) Let's drop ANDROID_ASSET_ARCH in favour of ANDROID_CPU_ARCH, and always use it.  So we get

assets/omni.ja
assets/armeabi/libnss.so
...

The fewer conditionals in the packager, the better.

2) Rather than bake the arch into APKOpen, let's put the assets/$ARCH computation into the APKOpen consumer.  One less path assumption in the C++ layer.  And I have toyed with the idea of loading these libraries from other locations (/data/data, a different APK entirely): see Bug 974462.
Attachment #8381592 - Flags: feedback?(nalexander) → feedback+
> 2) Rather than bake the arch into APKOpen, let's put the assets/$ARCH
> computation into the APKOpen consumer.  One less path assumption in the C++
> layer.  And I have toyed with the idea of loading these libraries from other
> locations (/data/data, a different APK entirely): see Bug 974462.

Further to this -- this computation should be dynamic, and the Java code has easy arch inspection.  (I imagine the C++ layer does too.)
Depends on: 1093242
Works locally; I'll push to try in a moment.
Attachment #8516385 - Flags: review?(mh+mozilla)
Attachment #8516385 - Flags: review?(mh+mozilla) → review+
Assignee: nobody → nalexander
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/25449ca505b4
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.