815491 - [Settings] Settings app assumes data uris when displaying icons in app permissions

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Settings, defect)

Description

[Jason Smith [:jsmith]]

Build:

Device - Unagi

Hashes

  <project name="gaia" path="gaia" remote="b2g" revision="176aea9cba4965594350f282751758b14d2624fc"/>
  <project name="releases/gecko" path="gecko" remote="mozillaorg" revision="4eaacd3fad7b6da220de206c0013dc0f307455c9"/>

Steps:

1. Install an app from testmanifest.com
2. Go to settings --> app permissions

Expected:

The app icon should be seen.

Actual:

No app icon is seen.

Comment 1

[Patryk Adamczyk [:patryk] UX]

Is this even a visual design issue? Shouldn't the icon be in the manifest?

Comment 3

[Jason Smith [:jsmith]]

(In reply to Patryk Adamczyk [:patryk] UX from comment #1)
> Is this even a visual design issue? Shouldn't the icon be in the manifest?

Probably isn't. It's a functional issue where we don't show the icon at all.

Marking for tracking, but not blocking.

Comment 4

[Dale Harvey (:daleharvey)]

As far as I remember the icons in permissions page were linked directly so if you specified a url in your manifest and you are offline, icons would not display, it was something that worried me before (not great for performance and will break offline)

Will verify shortly

Comment 6

[Dale Harvey (:daleharvey)]

Attachment: Only add origin to relative urls.

Comment 7

[Dale Harvey (:daleharvey)]

My diagnoses was wrong, this was simply not expect http urls for icons, the patch fixes it, but also filed a follow up 

https://bugzilla.mozilla.org/show_bug.cgi?id=869969

Comment 9

[Daniel Coloma:dcoloma]

This is just polish, not blocker in my opinion.

Comment 10

[Fabien Cazenave [:kaze]]

Comment on attachment 746975 [details] [diff] [review]
Only add origin to relative urls.

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

Looks good to me — I just have a suggestion regarding the URI scheme detection.

::: apps/settings/js/apps.js
@@ +169,3 @@
>  
> +        // Adding origin if it is a relative URL
> +        if (['http', 'https', 'data'].indexOf(protocol) === -1) {

Nit: maybe we could use a regexp instead?

    if (/^(http|https|data):/.test(iconURL))

Comment 11

[Lukas Blakk [:lsblakk] use ?needinfo]

Need-info? on the blocking request, at this point all new blocker require a partner champion and a milestone by which they must be resolved or risk slipping ship dates.  Can you please state the timeline expectation here if this is to become a blocker?

Comment 12

[Dale Harvey (:daleharvey)]

Attachment: Only add origin to relative urls

Updated to fix nit

Comment 13

[Dale Harvey (:daleharvey)]

Merged in: https://github.com/mozilla-b2g/gaia/commit/80f707869d6a031e7bce9ac8ba63270102173be1

Clearing the needinfo too

Comment 14

[Dale Harvey (:daleharvey)]

Sorry forgot to resolve

Comment 15

[ying.tan]

Sand Trap and Zombie Lines are fixed,,But same issue appeared on hosted apps:accuweather,facebook and notes!!

Comment 16

[Jason Smith [:jsmith]]

From the QA perspective - polish fix, not a blocker. We should consider nominating this for uplift though.

Comment 17

[Alex Keybl [:akeybl]]

No response to needinfo, given comment comment 9 and comment 16 not blocking.

Comment 18

[Deepa Subramanian]

This issue is still seen in Leo device. 

I enabled data connection and downloaded few apps (Airbnb, Accuweather, Facebook etc.,)from the Marketplace. 
Apps found in Settings App Permissions has few missed icons (the same apps were missing icon when wi-fi is also enabled). 

The user can although see that the icons are trying to load on the App permission list during enabled data connection. Please see screenshots.

Comment 19

[Deepa Subramanian]

Here are the environmental variables used for testing
Leo Build ID: 
Build ID: 20130625070217
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/29933d1937db
Gaia: 1436e2778b90bd74635b0b94d1cf8ccb0d71b60c
Platform Version: 18.1
RIL Version: 01.01.00.019.138

Comment 20

[Deepa Subramanian]

Attachment: Screenshot

Comment 21

[John Hammink]

Tested on latest nightly.  

Gecko  http://hg.mozilla.org/releases/mozilla-b2g18/rev/282b5c37cf8d
Gaia   e2ef782119b7e79fc62c48d36f0c36909d982988
BuildID 20130712070210
Version 18.0

Still NOT fixed.  Reopening.

Comment 22

[Jason Smith [:jsmith]]

(In reply to John Hammink from comment #21)
> Tested on latest nightly.  
> 
> Gecko  http://hg.mozilla.org/releases/mozilla-b2g18/rev/282b5c37cf8d
> Gaia   e2ef782119b7e79fc62c48d36f0c36909d982988
> BuildID 20130712070210
> Version 18.0
> 
> Still NOT fixed.  Reopening.

The fix here has only landed on master, not v1-train, so it's expected that this is still reproducing on that branch.

Comment 24

[hyuna.cho]

Please land in v1-train.

Comment 26

[lecky]

hi,jason

Has this issue been landed in V1.1HD branch?