Closed Bug 1040443 Opened 10 years ago Closed 10 years ago

[Lint] Found bitmap drawable res/drawable/phone.png in densityless folder

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 34

People

(Reporter: rnewman, Assigned: aymen, Mentored)

References

Details

(Whiteboard: lint, [good first bug])

Attachments

(1 file, 1 obsolete file)

Found bitmap drawable res/drawable/phone.png in densityless folder

Issue: Ensures that images are not defined in the density-independent drawable folder
Id: IconLocation

The res/drawable folder is intended for density-independent graphics such as shapes defined in XML. For bitmaps, move it to drawable-mdpi and consider providing higher and lower resolution versions in drawable-ldpi, drawable-hdpi and drawable-xhdpi. If the icon really is density independent (for example a solid color) you can place it in drawable-nodpi.

[http://developer.android.com/guide/practices/screens_support.html]
I'll work on this - looks very simple, especially since I've had lots of android experience in the past.

I just need to finish setting up my development environment.
Comment on attachment 8461552 [details] [diff] [review]
1040443_1.patch Moved phone.png to resolution-specific drawable-mdpi directory

This drawable isn't actually used.
Attachment #8461552 - Flags: review?(blassey.bugs)
Comment on attachment 8461552 [details] [diff] [review]
1040443_1.patch Moved phone.png to resolution-specific drawable-mdpi directory

if you're moving a file, use "hg move" rather than just moving it on the file system
Attachment #8461552 - Flags: review?(blassey.bugs)
Attachment #8461552 - Flags: review-
Attachment #8461552 - Flags: feedback+
Attached patch 1040443_2.patchSplinter Review
Sorry, used HG move this time.
Attachment #8461552 - Attachment is obsolete: true
Attachment #8461612 - Flags: review?(blassey.bugs)
(In reply to Aymen Qader from comment #3)

> This drawable isn't actually used.

It is:

mobile/android/chrome/content/SelectionHandler.js
641:      icon: "drawable://phone",
(In reply to Richard Newman [:rnewman] from comment #6)
> (In reply to Aymen Qader from comment #3)
> 
> > This drawable isn't actually used.
> 
> It is:
> 
> mobile/android/chrome/content/SelectionHandler.js
> 641:      icon: "drawable://phone",

Apologies, I was told that it wasn't used (and I couldn't find it using MXR)
(In reply to Aymen Qader from comment #7)
> (In reply to Richard Newman [:rnewman] from comment #6)
> > (In reply to Aymen Qader from comment #3)
> > 
> > > This drawable isn't actually used.
> > 
> > It is:
> > 
> > mobile/android/chrome/content/SelectionHandler.js
> > 641:      icon: "drawable://phone",
> 
> Apologies, I was told that it wasn't used (and I couldn't find it using MXR)

That was misinformation from me. Sorry. Using Java resources from JS makes them hard to find, especially with names like "phone".
Comment on attachment 8461612 [details] [diff] [review]
1040443_2.patch

moving the review to rnewman. I don't know how we handle dpi-dependant icons from js
Attachment #8461612 - Flags: review?(blassey.bugs) → review?(rnewman)
It *should* be seamless, because we do this already, delegating to the usual Android search mechanism. E.g.,

mobile/android/chrome/content/browser.js
569:      icon: "drawable://ic_menu_share",

mobile/android/base/resources/drawable-hdpi/ic_menu_share.png
mobile/android/base/resources/drawable-mdpi/ic_menu_share.png
mobile/android/base/resources/drawable-xhdpi/ic_menu_share.png

A quick scan suggests that this is the code that does the processing:

        if (data.startsWith("drawable://")) {
            final Uri imageUri = Uri.parse(data);
            final int id = getResource(imageUri, R.drawable.ic_status_logo);
            final Drawable d = context.getResources().getDrawable(id);

            runOnBitmapFoundOnUiThread(loader, d);
            return;
        }

so it should do the right thing. Needs testing to verify.
Assignee: nobody → aymen
Mentor: blassey.bugs → rnewman
Status: NEW → ASSIGNED
Comment on attachment 8461612 [details] [diff] [review]
1040443_2.patch

Tested locally on an xhdpi device. I'll land this with a correct commit message and User tag.
Attachment #8461612 - Flags: review?(rnewman) → review+
Ignore the fx-team landing; the tree was closed, but my hg extension posted to Bugzilla regardless.
https://hg.mozilla.org/mozilla-central/rev/16c16c68ed5f
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.