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

VERIFIED FIXED in Firefox 42

Status

()

VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: cos_flaviu, Assigned: mhaigh)

Tracking

({regression})

Trunk
Firefox 43
ARM
Android
regression
Points:
---

Firefox Tracking Flags

(firefox42 verified, firefox43 verified, fennec42+)

Details

Attachments

(2 attachments)

(Reporter)

Description

4 years ago
Created attachment 8628793 [details]
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
(Reporter)

Updated

4 years ago
Keywords: regression
Martyn - MDPI affected?
Flags: needinfo?(mhaigh)

Comment 2

4 years ago
Regressed from bug 1172831 ?
(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
(Assignee)

Updated

4 years ago
Assignee: nobody → mhaigh
Flags: needinfo?(mhaigh)
(Assignee)

Comment 5

4 years ago
Created 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

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.
Attachment #8629340 - Flags: review?(michael.l.comella)
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.
(Assignee)

Comment 7

4 years ago
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?
Flags: needinfo?(michael.l.comella)
(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.
Flags: needinfo?(michael.l.comella) → needinfo?(mhaigh)
(Assignee)

Comment 10

4 years ago
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!)
Attachment #8629340 - Flags: review?(michael.l.comella)
(Assignee)

Updated

4 years ago
Flags: needinfo?(mhaigh)
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.
Attachment #8629340 - Flags: review?(michael.l.comella) → review+
(Assignee)

Comment 13

4 years ago
url:        https://hg.mozilla.org/integration/fx-team/rev/8f092bb3b7498e6c91ca25db9f1f697f31b798ce
changeset:  8f092bb3b7498e6c91ca25db9f1f697f31b798ce
user:       Martyn Haigh <mhaigh@mozilla.org>
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
https://hg.mozilla.org/mozilla-central/rev/8f092bb3b749
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox43: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
NI for uplift.
tracking-fennec: --- → 42+
Flags: needinfo?(mhaigh)
(Assignee)

Comment 16

4 years ago
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
Flags: needinfo?(mhaigh)
Attachment #8629340 - Flags: approval-mozilla-aurora?
(Reporter)

Comment 17

4 years ago
Verified as fixed in build 43.0a1 2015-08-16;
Device: Asus Transformer Pad (Android 4.2.1).
status-firefox43: fixed → verified
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.
Attachment #8629340 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Reporter)

Comment 20

4 years ago
Verified as fixed in build 42.0a2 2015-08-19;
Device: Asus Transformer Pad (Android 4.2.1).
Status: RESOLVED → VERIFIED
status-firefox42: fixed → verified
You need to log in before you can comment on or make changes to this bug.