977677 - GeckoView can't access some resources it needs

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

Attachment: resource_fallback.patch

This patch catches these exceptions and provides fallbacks

Comment 1

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8383134 [details] [diff] [review]
resource_fallback.patch

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

Why are we missing these things?

Comment 2

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8383134 [details] [diff] [review]
resource_fallback.patch

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

I don't know much about how GeckoView is currently architected, but it is my belief that .gfx is thoroughly part of it.  We should be striving to make GeckoView re-usable, and therefore .gfx as well.

The way forward here is not to build a *runtime* dependency on the enclosing app defining |R.color.background_normal| -- and, I'll note, to not document that fact -- but to address the fact that consumers of GeckoView will want to customize certain things, and to build a framework that addresses this customization.

I interpret that to mean we define some kind of callback or delegate to provide customization of this background color, and we select a sensible default if necessary.

::: mobile/android/base/gfx/LayerRenderer.java
@@ +138,5 @@
>          mView = view;
> +        try {
> +            mOverscrollColor = view.getContext().getResources().getColor(R.color.background_normal);
> +        } catch (Resources.NotFoundException nfe) { mOverscrollColor = Color.BLACK; }
> +        

nit: trailing whitespace.

Comment 3

[Nick Alexander :nalexander [he/him]]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #1)
> Comment on attachment 8383134 [details] [diff] [review]
> resource_fallback.patch
> 
> Review of attachment 8383134 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Why are we missing these things?

I'm well aware that I'm parachuting into a field of land mines that I don't fully understand, but I think it's because nobody -- including myself -- has articulated how we intend to build and deliver GeckoView to consumers.  I can explain why our current approach will fail, and I have some thoughts on how to achieve some of the goals I think we have, but this is not the right forum for that.

Comment 4

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

(In reply to Nick Alexander :nalexander from comment #3)
> (In reply to James Willcox (:snorp) ([e-mail]) from comment #1)
> > Comment on attachment 8383134 [details] [diff] [review]
> > resource_fallback.patch
> > 
> > Review of attachment 8383134 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Why are we missing these things?
> 
> I'm well aware that I'm parachuting into a field of land mines that I don't
> fully understand, but I think it's because nobody -- including myself -- has
> articulated how we intend to build and deliver GeckoView to consumers.  I
> can explain why our current approach will fail, and I have some thoughts on
> how to achieve some of the goals I think we have, but this is not the right
> forum for that.

Given that this worked not that long ago, i don't think that that is entirely accurate. I believe this has something to do with how you changed how we build resources.

Comment 5

[Nick Alexander :nalexander [he/him]]

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #4)
> (In reply to Nick Alexander :nalexander from comment #3)
> > (In reply to James Willcox (:snorp) ([e-mail]) from comment #1)
> > > Comment on attachment 8383134 [details] [diff] [review]
> > > resource_fallback.patch
> > > 
> > > Review of attachment 8383134 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > Why are we missing these things?
> > 
> > I'm well aware that I'm parachuting into a field of land mines that I don't
> > fully understand, but I think it's because nobody -- including myself -- has
> > articulated how we intend to build and deliver GeckoView to consumers.  I
> > can explain why our current approach will fail, and I have some thoughts on
> > how to achieve some of the goals I think we have, but this is not the right
> > forum for that.
> 
> Given that this worked not that long ago, i don't think that that is
> entirely accurate. I believe this has something to do with how you changed
> how we build resources.

I have an explanation of how this might have worked, once:

It is my understanding that we ship the JARs we build for Fennec.  Consumers are expected to include those JARs, and all of Fennec's resources, into their build.  Those JARs include org.mozilla.gecko.R, which hard-codes resource IDs, like:

public final class R {
    public static final class anim {
        public static int grow_fade_in=0x7f040000;

At this point, the distributing APK's resources *must conform to the layout that GeckoView expects*.  (By distributing APK, I mean Fennec, or geckoview_example, or whatever.)  Of course, there is no guarantee that this happens, since the distributing APK does not have code to generate org.mozilla.gecko.R.

Now, how could this ever work?  Android's aapt tool sorts resources in a consistent (but undefined, I believe) way.  For some initial subset of resources, GeckoView's sort and the distributing APK's sort will co-incide.  For those resources, we get lucky.  (In my experiments with Eclipse, this can go a long way...)  Of course, as soon as the distributing APK ships enough resources to dramatically alter the sort, this fails entirely.  Given that geckoview_example includes only a few resources, it's very likely we were just getting lucky.

I know how to address this -- in essence, I have done so for Eclipse -- but it's not trivial.  It's the content of a build system in its own right!

Comment 6

[Nick Alexander :nalexander [he/him]]

> > Given that this worked not that long ago, i don't think that that is
> > entirely accurate. I believe this has something to do with how you changed
> > how we build resources.
> 
> I have an explanation of how this might have worked, once:

I should emphasize that my explanation for why this works is independent of the SRCDIR resource changes I implemented.  The issue I outline above, about APK resource layout, has always been the problem I don't understand how you and mfinkle intended to address.

Comment 7

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8383134 [details] [diff] [review]
resource_fallback.patch

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

::: mobile/android/base/gfx/LayerRenderer.java
@@ +137,5 @@
>      public LayerRenderer(LayerView view) {
>          mView = view;
> +        try {
> +            mOverscrollColor = view.getContext().getResources().getColor(R.color.background_normal);
> +        } catch (Resources.NotFoundException nfe) { mOverscrollColor = Color.BLACK; }

If the code is left this way (in opposition to nalexander's comment), I'd at least add a comment explaining why we expect this to fail in some cases (i.e. in geckoview); same comment for the other use.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a253b70ac4cb