Last Comment Bug 723151 - No drawable resources defined for eclair
: No drawable resources defined for eclair
Status: RESOLVED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: Firefox 13
Assigned To: Brad Lassey [:blassey] (use needinfo?)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-02-01 09:25 PST by Brad Lassey [:blassey] (use needinfo?)
Modified: 2012-02-06 19:28 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed
fixed
11+


Attachments
patch (212 bytes, patch)
2012-02-01 09:25 PST, Brad Lassey [:blassey] (use needinfo?)
no flags Details | Diff | Splinter Review
patch (41.55 KB, patch)
2012-02-01 11:35 PST, Brad Lassey [:blassey] (use needinfo?)
mark.finkle: review+
sriram.mozilla: feedback+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Brad Lassey [:blassey] (use needinfo?) 2012-02-01 09:25:00 PST
Created attachment 593485 [details] [diff] [review]
patch

all our drawables are drawalbe-[m/h/xh/]dpi-v[8/11/14]. Its that last part, specifying versions, that excludes eclair. The "right" thing to do according to the android docs is to have a complete set of your base resources in res/drawable, res/layout, res/strings etc etc. We're certainly violating that for drawables, not sure about the rest of them.

With this patch I can run fennec on my N1 running eclair
Comment 1 Mark Finkle (:mfinkle) (use needinfo?) 2012-02-01 10:51:16 PST
Patch is partly missing
Comment 2 Brad Lassey [:blassey] (use needinfo?) 2012-02-01 11:35:50 PST
Created attachment 593539 [details] [diff] [review]
patch

not sure what went wrong there
Comment 3 Mark Finkle (:mfinkle) (use needinfo?) 2012-02-01 12:03:21 PST
Comment on attachment 593539 [details] [diff] [review]
patch

Looks good, but I want Sriram to be aware of this too and look for any issues
Comment 4 Sriram Ramasubramanian [:sriram] 2012-02-01 12:11:44 PST
Comment on attachment 593539 [details] [diff] [review]
patch

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

The v8 was added thinking that we wouldn't be supporting eclair. I am happy with this change :)
I am not sure if we want LDPI version. If I remember correctly we don't support LDPI.
Comment 5 Brad Lassey [:blassey] (use needinfo?) 2012-02-01 12:20:33 PST
(In reply to Sriram Ramasubramanian [:sriram] from comment #4)
> Comment on attachment 593539 [details] [diff] [review]
> patch
> 
> Review of attachment 593539 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The v8 was added thinking that we wouldn't be supporting eclair. I am happy
> with this change :)
> I am not sure if we want LDPI version. If I remember correctly we don't
> support LDPI.

we don't, but sync has an LDPI resource.
Comment 6 Sriram Ramasubramanian [:sriram] 2012-02-01 12:22:29 PST
Probably we need to ask Sync team if they really want those resources (I believe they use eclipse and eclipse created the ldpi folder by default). If they don't want, we can remove it some day. If they want it, we should think of copying low resolution images into drawable-ldpi to make us LDPI devices ready.
Comment 7 Ed Morley [:emorley] 2012-02-02 07:16:39 PST
https://hg.mozilla.org/mozilla-central/rev/f7a88134a0f7
Comment 8 Brad Lassey [:blassey] (use needinfo?) 2012-02-02 14:18:44 PST
Comment on attachment 593539 [details] [diff] [review]
patch

[Approval Request Comment]
Regression caused by (bug #): 
User impact if declined: 
crash on startup on eclair
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky):
String changes made by this patch:
Comment 9 Cristian Nicolae (:xti) 2012-02-03 09:20:53 PST
Verified fixed on:

Mozilla/5.0 (Android;Linux armv7l;rv:13.0a1)Gecko/20120203
Firefox/13.0a1 Fennec/13.0a1
Device: Samsung Galaxy S
OS: Android 2.1-update1
Comment 10 Cristian Nicolae (:xti) 2012-02-03 09:24:43 PST
Aurora nom?
Comment 11 Aaron Train [:aaronmt] 2012-02-03 09:25:43 PST
(In reply to Cristian Nicolae (:xti) from comment #10)
> Aurora nom?

See the patch above. It's question marked already.
Comment 12 Cristian Nicolae (:xti) 2012-02-03 09:33:10 PST
(In reply to Aaron Train [:aaronmt] from comment #11)
> (In reply to Cristian Nicolae (:xti) from comment #10)
> > Aurora nom?
> 
> See the patch above. It's question marked already.

Sorry, I just missed it. Thanks
Comment 13 Alex Keybl [:akeybl] 2012-02-05 13:44:59 PST
Comment on attachment 593539 [details] [diff] [review]
patch

[Triage Comment]
Mobile only - approved for Aurora 12 and Beta 11.
Comment 14 Mark Finkle (:mfinkle) (use needinfo?) 2012-02-06 09:22:08 PST
https://hg.mozilla.org/releases/mozilla-aurora/rev/7c4652b0f3b0
Comment 15 Brad Lassey [:blassey] (use needinfo?) 2012-02-06 19:28:41 PST
https://hg.mozilla.org/releases/mozilla-beta/rev/02d946fad4c7

Note You need to log in before you can comment on or make changes to this bug.