830903 - OS Doesn't properly use privacy_sprite.png in Settings

Status: RESOLVED FIXED

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

Description

[Peter La]

We're replacing the branded and unbranded assets in:

gaia/shared/resources/branding

and there are two pngs that are not being used by Settings to display its icons in Setting -> Device Information -> Privacy:
official/privacy_sprite.png
unofficial/privacy_sprite.png

The assets will be checked in with Bug 828251, so I've noted the dependency.

Comment 1

[Sam Joch [:samjoch]]

Attachment: patch

This patch use the privacy_sprite.png in shared folder.

Comment 2

[Peter La]

Hey Sam,

I looked at your fix, and I believe this is not the correct way to fix it.  You can't just reference one image, you have to reference the correct version, either branded (official) or unbranded (unofficial).  There are two directories under gaia/shared/resources/branding, each with its own copy of privacy_sprite.png:

gaia/shared/resources/branding/official -> image contains the firefox logo
gaia/shared/resources/branding/unofficial -> image contains a generic blue globe

I checked in new images for these yestereday with:
https://github.com/gordonbrander/gaia/pull/92

Can you take a look at this again and make the proper changes?

Thanks!
Peter

Comment 3

[Sam Joch [:samjoch]]

Attachment: render official/unofficial icons in privacy list

Peter, 

Actually there is a flag MOZILLA_OFFICIAL=1 that you can use to build gaia using official resources or not.

That's why in HTML/CSS we use paths without the official/unofficial part.

Here is the command you should try to build gaia on the device: MOZILLA_OFFICIAL=1 make production

Can you test with that and let me know if i doing something wrong ?

Comment 4

[Peter La]

Hi Sam,

Thanks for clarifying.  As long as the graphics in those official/unofficial directories are still being used based on the flag.  I was just worried that you moved the graphics to the directory above. :)

Peter.

Comment 5

[Sam Joch [:samjoch]]

Landed in uxbranch: https://github.com/gordonbrander/gaia/commit/081e4e75e15d50754cc0dda652bd6ddacc397d6a

Comment 6

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Sweet.  qa-verified

Comment 7

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

https://github.com/mozilla-b2g/gaia/commit/eb803368dfd558fc03aac5fb89ca3376d1f90497

Comment 8

[Julien Wajsberg [:julienw]]

The specific master commit for this app is 9aff5a6 .

Should this go to v1 ?

Comment 9

[Lukas Blakk [:lsblakk] use ?needinfo]

If the UX team is making a case for this in v1.1 go ahead with nomination for uplift.

Comment 10

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Not fixed on master : 
https://github.com/mozilla-b2g/gaia/blob/master/apps/settings/style/icons.css#L351

nor v1 train :
https://github.com/mozilla-b2g/gaia/blob/v1-train/apps/settings/style/icons.css#L318

I think someone overwrote the changes to the master one and it probably never got pushed to v1 train.

Comment 11

[Julien Wajsberg [:julienw]]

(In reply to Naoki Hirata :nhirata from comment #10)
> Not fixed on master : 
> https://github.com/mozilla-b2g/gaia/blob/master/apps/settings/style/icons.
> css#L351

actually this line was not in the original patch, it was introduced later.

> 
> nor v1 train :
> https://github.com/mozilla-b2g/gaia/blob/v1-train/apps/settings/style/icons.
> css#L318

It was never uplifted to v1-train because nobody cared enough.

> 
> I think someone overwrote the changes to the master one and it probably
> never got pushed to v1 train.

Yep, it seems that was Bug 830644.
see https://github.com/mozilla-b2g/gaia/commit/0f3a6c729085e322502c05b5dc932bf27ea0139c#L4L323

But in that patch, the good image was actually put in settings and I actually just verified that it works as expected in master. Could you check again ?

Comment 12

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

I can't check from a visual stand point even though the code looks right.  see bug 858722

Comment 13

[Sam Joch [:samjoch]]

Attachment: Regression fix RP

Comment 14

[Julien Wajsberg [:julienw]]

Comment on attachment 738987 [details]
Regression fix RP

added a comment in the PR

sorry about the delay, I was busy with tef+ stuff.

Comment 15

[Julien Wajsberg [:julienw]]

still waiting for the follow up

Comment 16

[Sam Joch [:samjoch]]

Just updated the branch.

Comment 17

[Julien Wajsberg [:julienw]]

Comment on attachment 738987 [details]
Regression fix RP

r=me
thanks Sam !

Comment 18

[Julien Wajsberg [:julienw]]

Sam, could you also follow that it works as expected on the v1-train branch ? Like requesting approval on both patches, and resolving conflicts ?

Comment 19

[Sam Joch [:samjoch]]

Follow up merge in master: https://github.com/mozilla-b2g/gaia/commit/dbe4a990391a8045dc1caf0241ad3926126b0ad8

Comment 20

[Sam Joch [:samjoch]]

Actually the first commit https://github.com/mozilla-b2g/gaia/commit/9aff5a63a7f3e4609e939f90847640274a77501a hasn't any conflicts with v1-train. 

Should i just land this one in v1-train ?

Because the follow up depends on: https://github.com/mozilla-b2g/gaia/commit/0f3a6c729085e322502c05b5dc932bf27ea0139c

Comment 21

[Julien Wajsberg [:julienw]]

we need to ask approvals first ;)