1179758 - Missing icon for 'Add to reading list' button under custom menu

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Flaviu Cos]

Attachment: missing icon.png

Environment: 
Device: Asus Transformer Pad (Android 4.2.1);
Build: Nightly 42.0a1 (2015-07-02);

Steps to reproduce:
1. Launch Fennec;
2. Tap on menu button;

Expected result:
The 'Add to reading list' button is correctly displayed in the custom menu.

Actual result:
The 'Add to reading list' button have no icon. 

Notes:
Please check the attached screenshot.

Regression window:
Last good build: 2015-07-01;
First bad build: 2015-07-02;
Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=079b6f1ae1c3&tochange=f5e3bacfb60e

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

Martyn - MDPI affected?

Comment 2

[u421692]

Regressed from bug 1172831 ?

Comment 3

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

(In reply to Mihai Pop from comment #2)
> Regressed from bug 1172831 ?

Don't see anything here.

My best guess is in bug 1171946 where drawable-mdpi-v11/ic_menu_reader* was removed. We mirrored that value in drawable/ic_menu_reader*.xml [1] which are empty values as to not break API 9 builds (which strip v11+ resources). I'm guessing mdpi devices better match drawable/ now than drawable-hdpi-v11/.

[1]: http://hg.mozilla.org/mozilla-central/annotate/fa9f4d788144/mobile/android/base/resources/drawable/ic_menu_reader_add.xml

Comment 4

[Martyn Haigh (:mhaigh)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=78780d2d017f

Comment 5

[Martyn Haigh (:mhaigh)]

Attachment: MozReview Request: Bug 1179758 - Missing icon for 'Add to reading list' button under custom menu; r?mcomella

Bug 1179758 - Missing icon for 'Add to reading list' button under custom menu; r?mcomella

Icons have been removed from the v11 specific directories as it seems that the drawable folder was being matched on an mdpi device on >v9, over the drawable-hdpi-v11 folder.  Have inserted logic to prevent GB devices from displaying the icon.

Comment 6

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

Comment on attachment 8629340 [details]
MozReview Request: Bug 1179758 - Missing icon for 'Add to reading list' button under custom menu; r?mcomella

https://reviewboard.mozilla.org/r/12567/#review11177

I don't think this is the best way to do this - because the files were moved from v11 to non-version specific folders, API 9 builds (which need the space savings the most) will now ship all of the resources they don't use.

Perhaps you can use a [resource alias](http://developer.android.com/guide/topics/resources/providing-resources.html#AliasResources) to use the hdpi icon in the mdpi folder? e.g. put the hdpi icon in `drawable-v11/`, and create an alias to it from both `drawable-mdpi-v11/` and `drawable-hdpi-v11/`.

If that doesn't work, I'd prefer shipping the mdpi asset to v11 devices over shipping all assets to v9 (though maybe there's another solution?).

::: mobile/android/base/BrowserApp.java:3363
(Diff revision 1)
> +        assert Build.VERSION.SDK_INT > Build.VERSION_CODES.GINGERBREAD;

I don't think we actually run assertions (and supposedly, java assertions aren't great - I think that was from Kitching).

However, you can use the Assert class: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/Assert.java

That works, afaik.

::: mobile/android/base/BrowserApp.java:3208
(Diff revision 1)
> +        if(Build.VERSION.SDK_INT > Build.VERSION_CODES.GINGERBREAD) {

nit: `if (`. Here and below.

::: mobile/android/base/BrowserApp.java:3408
(Diff revision 1)
> +                    if(Build.VERSION.SDK_INT > Build.VERSION_CODES.GINGERBREAD) {

You should be using `AppConstants.Versions.feature11Plus`. I think most of the code imports `...AppConstants.Versions` so you can use `Versions` directly though.

Comment 7

[Martyn Haigh (:mhaigh)]

mcomella: We only currently ship hdpi files to GB devices (now that the mdpi folder has been removed) so you don't have to worry that we're shipping all versions to GB devices.  Also - there are quite a few GB tablets out there, so the question here is really should we be supporting them or limiting support for them?

Comment 8

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

(In reply to Martyn Haigh (:mhaigh) from comment #7)
> mcomella: We only currently ship hdpi files to GB devices (now that the mdpi
> folder has been removed) so you don't have to worry that we're shipping all
> versions to GB devices.

Fair enough, though we'd still be shipping one more asset than we need to. One thing I like about the resource alias approach is that it saves us the resource on API 9 devices *and* provides us the opportunity to comment and explain wtf is going on (inside the aliases), rather than having these v11+ files inconsistently in a non-API specific folder.

Is there a reason you don't like this approach?

> Also - there are quite a few GB tablets out there,
> so the question here is really should we be supporting them or limiting
> support for them?

I don't think we actively support them – have you ever tested on a GB tablet? I'd wager it's completely broken, given our drawable-large*v11/ directory hierarchy – better to raise this discussion in another bug, if we're interested in it rather than making a baby step here.

Comment 9

[Martyn Haigh (:mhaigh)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ed91fcac6ecd

Comment 10

[Martyn Haigh (:mhaigh)]

Comment on attachment 8629340 [details]
MozReview Request: Bug 1179758 - Missing icon for 'Add to reading list' button under custom menu; r?mcomella

Bug 1179758 - Missing icon for 'Add to reading list' button under custom menu; r?mcomella

Have taken your advice and reworked - turns out this way was simplier although it introduces back an mdpi folder (which I believe I was getting hung up on the first time around with this bug, having just removed it!)

Comment 11

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

Comment on attachment 8629340 [details]
MozReview Request: Bug 1179758 - Missing icon for 'Add to reading list' button under custom menu; r?mcomella

https://reviewboard.mozilla.org/r/12567/#review13631

If this works, wfm! :)

I'm actually surprised a resource alias from mdpi to hdpi works (I'd expect the asset would have to be in `drawable-v11/`) but Android: I'd believe it.

::: mobile/android/base/resources/drawable-mdpi-v11/ic_menu_reader_add.xml:6
(Diff revision 2)
> +<bitmap xmlns:android="http://schemas.android.com/apk/res/android"

Add a comment here explaining what's going on.

::: mobile/android/base/resources/drawable-mdpi-v11/ic_menu_reader_remove.xml:6
(Diff revision 2)
> +<bitmap xmlns:android="http://schemas.android.com/apk/res/android"

Ditto.

Comment 12

[Martyn Haigh (:mhaigh)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fdcec3b938f9

Comment 13

[Martyn Haigh (:mhaigh)]

url:        https://hg.mozilla.org/integration/fx-team/rev/8f092bb3b7498e6c91ca25db9f1f697f31b798ce
changeset:  8f092bb3b7498e6c91ca25db9f1f697f31b798ce
user:       Martyn Haigh <[e-mail]>
date:       Mon Aug 10 14:36:27 2015 +0100
description:
Bug 1179758 - Missing icon for 'Add to reading list' button under custom menu; r=mcomella

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8f092bb3b749

Comment 15

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

NI for uplift.

Comment 16

[Martyn Haigh (:mhaigh)]

Comment on attachment 8629340 [details]
MozReview Request: Bug 1179758 - Missing icon for 'Add to reading list' button under custom menu; r?mcomella

Approval Request Comment
[Feature/regressing bug #]:1171946
[User impact if declined]:some users won't see the reading list icon in the menu, instead it'll be an empty, although functional, area. 
[Describe test coverage new/current, TreeHerder]: Passed tests on try
[Risks and why]: We change some files for other device configurations, so there's a risk of further devices with specific screen sizes from also seeing this issue.
[String/UUID change made/needed]: NA

Comment 17

[Flaviu Cos]

Verified as fixed in build 43.0a1 2015-08-16;
Device: Asus Transformer Pad (Android 4.2.1).

Comment 18

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8629340 [details]
MozReview Request: Bug 1179758 - Missing icon for 'Add to reading list' button under custom menu; r?mcomella

Fix a visual issue, taking it.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f20639a7ee40

Comment 20

[Flaviu Cos]

Verified as fixed in build 42.0a2 2015-08-19;
Device: Asus Transformer Pad (Android 4.2.1).