Closed Bug 1171946 Opened 4 years ago Closed 4 years ago

Consider removing mdpi drawable resources

Categories

(Firefox for Android :: General, defect)

35 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 42
Tracking Status
firefox42 --- fixed
fennec 42+ ---

People

(Reporter: Margaret, Assigned: mhaigh)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(1 file)

We've been talking about doing this to reduce our APK size, so let's have a bug to investigate further.
Good call. Thanks for filing this.

Let's investigate the costs and benefits in here when you have some time Martyn :)
Is this not a dupe/morph of Bug 959203?
(In reply to Richard Newman [:rnewman] from comment #2)
> Is this not a dupe/morph of Bug 959203?

I suppose it is... although bug 959203 sounds more extreme than this one. I feel like we should try this smaller-scoped approach here, then we can follow up in bug 959203 with a more extreme change if we think that's necessary.
Let's do some basic analysis before we get going:

Running a diff between mdpi and hdpi gives us this: https://etherpad.mozilla.org/0vqrTL8FDL

Of those results, we aren't worried about the files which are in both directories - this is expected.  Removing those files leaves us with a much shorter list: https://etherpad.mozilla.org/9GTeClhV5H.  Most of these files are only in the drawable-hdpi folder, lending weight to our thinking that we can survive in a post mdpi world.  Worryingly there are a few files which are present in mdpi and not hdpi:

Only in drawable-mdpi: autocomplete_list_bg.9.png
- This is a plain stretchable background, we should move this to the drawables folder as upscaling this could result in unwanted visual artifacts.

Only in drawable-mdpi: fxaccount_sync_error.png
- This is bad, we only have this defined in mdpi. antlam: can we have new assets for this?

Only in drawable-mdpi: scrollbar.png
- This doesn't seem to be used anywhere
Flags: needinfo?(alam)
Blocks: 1172831
Just a note to say that all files defined in drawable-mdpi-v11 are also defined in drawable-hdpi-v11.

As an aside - all files defined in drawable-mdpi-v11 are also defined in drawable-xhdpi-v11, but very few have made it to drawable-xxhdpi-v11
(In reply to Martyn Haigh (:mhaigh) from comment #5)
> As an aside - all files defined in drawable-mdpi-v11 are also defined in
> drawable-xhdpi-v11, but very few have made it to drawable-xxhdpi-v11

Historical context: iirc, xxhdpi only came out part way through Ian's tenure, so we're missing many of the xxhdpi assets from before that time. Also, I don't think we regularly used xxhpdi until I talked to antlam about it during the new tablet refresh, so we might be missing assets before then too.
Assignee: mhaigh → nobody
Let's get an assignee.
tracking-fennec: --- → ?
Martyn, I have a 2.3 mdpi device - is there a patch I can test here?
Flags: needinfo?(mhaigh)
Hm, I can't find this icon/asset in my design. I had originally handed this off to Ryan and John so maybe they know where this asset is being used?

CC'ing Ryan here for some help.

Ryan, where are using this (http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/drawable-mdpi/fxaccount_sync_error.png) in the current Firefox Accounts flow? I can't find this in my designs anywhere but maybe you know where this surfaces itself in the UI?
Flags: needinfo?(alam) → needinfo?(rfeeley)
tracking-fennec: ? → 42+
(In reply to Anthony Lam (:antlam) from comment #9)
> Ryan, where are using this
> (http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/
> drawable-mdpi/fxaccount_sync_error.png) in the current Firefox Accounts
> flow? I can't find this in my designs anywhere but maybe you know where this
> surfaces itself in the UI?

I don't see it in our Sketch assets. I believe it appears on one of the Android notifications. Definitely not used on web or desktop.
Flags: needinfo?(rfeeley)
(In reply to Anthony Lam (:antlam) from comment #9)
> Hm, I can't find this icon/asset in my design. I had originally handed this
> off to Ryan and John so maybe they know where this asset is being used?
> 
> CC'ing Ryan here for some help.
> 
> Ryan, where are using this
> (http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/
> drawable-mdpi/fxaccount_sync_error.png) in the current Firefox Accounts
> flow? I can't find this in my designs anywhere but maybe you know where this
> surfaces itself in the UI?

Maybe Nick has an idea?
Flags: needinfo?(nalexander)
(In reply to Mark Finkle (:mfinkle) from comment #11)
> (In reply to Anthony Lam (:antlam) from comment #9)
> > Hm, I can't find this icon/asset in my design. I had originally handed this
> > off to Ryan and John so maybe they know where this asset is being used?
> > 
> > CC'ing Ryan here for some help.
> > 
> > Ryan, where are using this
> > (http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/
> > drawable-mdpi/fxaccount_sync_error.png) in the current Firefox Accounts
> > flow? I can't find this in my designs anywhere but maybe you know where this
> > surfaces itself in the UI?
> 
> Maybe Nick has an idea?

https://dxr.mozilla.org/mozilla-central/search?tree=mozilla-central&q=fxaccount_sync_error&redirect=true

It's used in the Firefox Account status activity on Android, screenshot here: https://people.mozilla.org/~nalexander/screenshots/Sync.Migration.Status.Activity.png

It is in no way special; update it as needed.  I'd be happy to have it as SVG; or as hdpi and xxhdpi or whatever minimal set we identify.  (Which I think is hdpi (for <11) and xxhpdi (for 11+) builds.)
Flags: needinfo?(nalexander)
(In reply to Martyn Haigh (:mhaigh) from comment #13)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=0302c34038e1

I installed and tested this build on my DroidPro (I think it's MDPI) running Android 2.3.3

I didn't notice any missing or poorly rendered images. I looked at:
* URL Bar: magnify glass, stop, reader, tab-count
* Edit URL: close, mic
* Tabs Tray: plus, close, normal tabs and private tabs
* Home pages: switch-to-tab, empty reading list, empty recent tabs, empty history

Lets land this soon (beginning of cycle) and get more Nightly users to try it out.
Depends on: 1178804
From comment 4, there were three files which we needed to do something about:

autocomplete_list_bg.9.png
- This has been moved in to the drawables folder but bug 1178813 is tracking as we should either define as xml or make a much smaller version of this asset.

fxaccount_sync_error.png
- Still waiting on antlam for new assets, I've put the existing asset in the hdpi folder for now.  Bug 1178804 filed to track this issue.

scrollbar.png
I was incorrect in my previous statement - this is used in LayerView.  It doesn't seem to need a scaled version at any point (I tried putting this version in drawable and the hdpi folders with no visible difference), so I've decided to move it to the drawables folder.
Flags: needinfo?(mhaigh)
Attachment #8627745 - Flags: review?(s.kaspari)
Depends on: 1178813
Comment on attachment 8627745 [details] [diff] [review]
Remove mdpi drawable resources

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

LGTM.

As we removed files in drawable-mdpi and drawable-mdpi-v11 maybe we should also check dialogs and notification icons on an mdpi device because the style changed a lot in the past here (dark/bright colors, gingerbread vs. holo) and there's a chance that we might pick a resource from the wrong bucket now.
Attachment #8627745 - Flags: review?(s.kaspari) → review+
https://hg.mozilla.org/mozilla-central/rev/a03382da40d5
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Assignee: nobody → mhaigh
Martyn, there are a few more mdpi resources in not mobile/android/base/resources [1] – is there any reason these were not removed?

[1]: http://mxr.mozilla.org/mozilla-central/find?text=&string=mdpi
Flags: needinfo?(mhaigh)
Not intentional (aside from the xml files added in bug 1179758).  Let's get them removed!
Flags: needinfo?(mhaigh)
Depends on: 1194205
You need to log in before you can comment on or make changes to this bug.