1016221 - [Collection App] Mozilla CDN Backgrounds for Collection icons

Status: RESOLVED FIXED

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

Comment 1

[Stephany Wilkes]

Patryk, please attach app backgrounds to this bug (from the email last night) so that we can get this reassigned to a dev. Thanks!

Comment 2

[Patryk Adamczyk [:patryk] UX]

https://mozilla.app.box.com/files/0/f/1997312277

Comment 4

[Johan Lorenzo [:jlorenzo]]

https://moztrap.mozilla.org/manage/case/13010/

Comment 5

[Kevin Grandon :kgrandon]

The assets we want to use are too big for the device that are needed to support this. We are working on solving that, but there is no guaranteed timeline. Because of this I'm unblocking the collection-app bug, and blocking bug 1017954 with this so we can focus on what's remaining.

We can still take this in 2.0 if it's critical, but let's only do so if it's blocking status. Please let me know if you have any concerns.

Comment 6

[Amir Nissim (:amirn)]

changed the scope of the bug to system background only so we can land basic support sooner with bug 1024387

Comment 7

[Jason Smith [:jsmith]]

So I see a feature-b2g 2.0 flag, but I see this marked as a VH next.

Peter - Can you make a call if this is needed for 2.0 or not?

Comment 8

[Peter Dolanjski [:pdol]]

Sorry, it's not clear: what is the implication of not having this in 2.0?

Comment 9

[Amir Nissim (:amirn)]

(In reply to Peter Dolanjski [:pdol] from comment #8)
> Sorry, it's not clear: what is the implication of not having this in 2.0?

Not having this would mean that pre-installed collections will not have the pre-defined background images provided by UX. It will have backgrounds provided by E.me (like any other collections do).

Comment 10

[Amir Nissim (:amirn)]

Peter, we are still blocked. Is this required for 2.0?
Don't think we can get it done by 6/20

Comment 11

[Peter Dolanjski [:pdol]]

Patryk, this one is your call.  What do you think?

Comment 12

[Patryk Adamczyk [:patryk] UX]

We should try to get it in, this would impact the overall homescreen experience & marketing shots/billboards. Basically the smart collection icons on the homescreen use these wallpapers.

I would rate this bug 3/5 in terms of impact / criticality.
However the blocking bug is definitely 5/5, we're still swapping out assets in 2.0.

Comment 13

[Peter Dolanjski [:pdol]]

Ran, can we provide the image assets to you guys to come from the server (at least for FxOS devices)?

My understanding is that the main issue with getting this done is the build size.  
Why isn't that also a problem for the standard e.me assets?

Comment 14

[Ran Ben Aharon [:ranbena] EverythingMe]

(In reply to Peter Dolanjski [:pdol] from comment #13)
> Ran, can we provide the image assets to you guys to come from the server (at
> least for FxOS devices)?

I'm not certain this is currently possible in our servers. Let me get back to you on this.

> 
> My understanding is that the main issue with getting this done is the build
> size.  
> Why isn't that also a problem for the standard e.me assets?

E.me assets are mostly js and css files. Almost no images.
The new bg images include high resolution multicolor images which are much heftier.

Comment 15

[Peter Dolanjski [:pdol]]

Thanks Ran.

So, what do the backgrounds from e.me look like with the new design?

Comment 16

[Ran Ben Aharon [:ranbena] EverythingMe]

Well basically, you can take the latest 2.0 or master, add the 10 items from the Smart Collection list and see for yourself :)

I can't currently seem to take screenshots in my Flame :/

Comment 17

[Peter La]

(In reply to Ran Ben Aharon (Everything.me) from comment #16)
> Well basically, you can take the latest 2.0 or master, add the 10 items from
> the Smart Collection list and see for yourself :)
> 
> I can't currently seem to take screenshots in my Flame :/

Hey Ran - FYI there is a new shortcut to take screenshots.  Down volume + power button. :)

Comment 18

[Peter Dolanjski [:pdol]]

Right now it seems that if you add a Collection while offline it just doesn't show on the homescreen at all.  I'm guessing that's another bug?

Comment 19

[Ran Ben Aharon [:ranbena] EverythingMe]

Yes. Opened Bug #1029040.

Comment 20

[Ran Ben Aharon [:ranbena] EverythingMe]

(In reply to Peter La from comment #17)
> Hey Ran - FYI there is a new shortcut to take screenshots.  Down volume +
> power button. :)

Works! Do you need the screenshots? Or can you manage?

Comment 21

[Peter Dolanjski [:pdol]]

(In reply to Ran Ben Aharon (Everything.me) from comment #20)
> 
> Works! Do you need the screenshots? Or can you manage?

No, just a clarification (since the bug above is preventing me from testing):
If I add a collection while offline, what background is used for the icon?  Is it some default one for all icons or are some (smaller) assets included that would be used?

Comment 22

[Peter Dolanjski [:pdol]]

I tested this further.  It looks like the default 4 collections we currently use in the build are using the proper, new assets.  So, is this bug to enable the other 14 predefined collections to use those new assets (which is why we're running into build size issues)?

Comment 23

[Ran Ben Aharon [:ranbena] EverythingMe]

(In reply to Peter Dolanjski [:pdol] from comment #22)
> I tested this further.  It looks like the default 4 collections we currently
> use in the build are using the proper, new assets.  So, is this bug to
> enable the other 14 predefined collections to use those new assets (which is
> why we're running into build size issues)?

Not exactly. The 4 preinstalled Collections get their icon from the manifest (we simply updated the preliminary icon images in apps/collection/collections) but their internal bg image is still being fetched from Eme servers which then overwrites the homescreen icon.

Comment 24

[Ran Ben Aharon [:ranbena] EverythingMe]

Btw, there are 4 preinstalled and another 6 as optionals.

Comment 25

[Jason Smith [:jsmith]]

Keeping this blocking until product indicates that we no longer need this for 2.0.

Comment 26

[Kevin Grandon :kgrandon]

One thing we need to do here regardless, and that might satisfy marketing/billboard uses is update the default icons in these folders: https://github.com/mozilla-b2g/gaia/tree/master/apps/collection/collections . These assets can be found in an attachment in bug 965711.

Without the server-side piece in place though, this would mean that collection icons would be outdated and replaced when the user launches the collection.

Comment 28

[Kevin Grandon :kgrandon]

Unblocking on bug 960720 as I believe we are changing our approach for 2.0.

In 2.0 we should load background images for pre-defined collections from a server. CDN support for this was recently added in bug 1034656, and we will get the collection backgrounds uploaded there soon.

I think we can use this bug to track the work for replacing the default icons, and implementing some logic to override the background image URL for the pre-defined collections.

Comment 29

[Kevin Grandon :kgrandon]

Attachment: Assets - Collection Backgrounds

Attaching the collection backgrounds from bug 965711.

Comment 30

[Kevin Grandon :kgrandon]

Attachment: Script - Change background names to be based on ID

Comment 31

[Kevin Grandon :kgrandon]

Attachment: Assets - Collection Backgrounds by ID

This is a set of all of the collection backgrounds, keyed by ID and @1.5x, @2x nomenclature which gaia is used to using.

I think these should be the set of assets that we upload to the CDN.

Comment 32

[Kevin Grandon :kgrandon]

Amir - glad to have you back :) The images for the ten collections in gaia have been uploaded to our CDN. I was wondering if you could help us implement this? You can access the URLs via our CDN at:

http://fxos.cdn.mozilla.net/collection/background/142.jpg
http://fxos.cdn.mozilla.net/collection/background/[e-mail]

These follow the same convention that we use in gaia (.jpg, @1.5x.jpg, @2x.jpg, etc). I assume that you can just pick the right path based on devicePixelRatio.

Comment 33

[Amir Nissim (:amirn)]

Kevin, I am wondering how to implement this.

E.me server provides a base64 image data which we store locally after fetching once.
The Moz CDN provides binary jpg data (correct?) so I wonder if we should (a) make an XHR and convert to base64 and store the same way as E.me backgrounds, or (b) set 'img.src = cdnURL' store nothing, and leverage caching.

WDYT?

Comment 34

[Kevin Grandon :kgrandon]

(In reply to Amir Nissim (:amirn) from comment #33)
> Kevin, I am wondering how to implement this.
> 
> E.me server provides a base64 image data which we store locally after
> fetching once.
> The Moz CDN provides binary jpg data (correct?) so I wonder if we should (a)
> make an XHR and convert to base64 and store the same way as E.me
> backgrounds, or (b) set 'img.src = cdnURL' store nothing, and leverage
> caching.
> 
> WDYT?

My preference would be to simply set img.src = cdnURL for 2.0 initially, it seems easier and we are way-late on this. If this is going to cause problems with icon generation (don't want icon backgrounds disappearing if you go offline), I believe we should store the results of a canvas.toBlob() call (instead of base64 encoded).

Comment 35

[Amir Nissim (:amirn)]

Attachment: Pull Request

Patch is working but need some help with the tests.
Thanks.

Comment 36

[Kevin Grandon :kgrandon]

Comment on attachment 8453096 [details] [review]
Pull Request

Hey Amir - nice work, I left a few comments on github. I think the only thing that really needs to be done before landing is adding the common.js file to synchronize. I couldn't really test icon generation as I think there's some server bug with native info currently, (but I think that is being tracked in another bug).

Comment 38

[Amir Nissim (:amirn)]

landed: https://github.com/mozilla-b2g/gaia/commit/9096ff7386da9a5dcf637a50e67d2828a8bbcb87

Comment 41

[Amir Nissim (:amirn)]

Comment on attachment 8453096 [details] [review]
Pull Request

Blocks 1029977 which is a v2.0 blocker

[Bug caused by] (feature/regressing bug #): Never Implemented
[User impact] if declined: Will not show Mozilla backgrounds
[Testing completed]: Yes
[Risk to taking this patch] (and alternatives if risky): Low
[String changes made]: Implemented new feature

Comment 42

[Ryan VanderMeulen [:RyanVM]]

[Blocking Requested - why for this release]: The approval request doesn't seem to be going anywhere and this blocks another blocker (bug 1029977)

Comment 43

[Preeti Raghunath(:Preeti)]

Ryan

I've reviewed this approval request and seems like a feature request. I've alerted Lawrence to help with the same and hence changing the ni to lmandel instead of me.

Comment 44

[Preeti Raghunath(:Preeti)]

Lawrence,

Please review this approval request.

Comment 45

[Jason Smith [:jsmith]]

Triage is concerned about the fact that this is a feature being asked to land 2.0 late in the cycle.

I'll discuss this with Gregor offline.

Comment 46

[Jason Smith [:jsmith]]

Peter & I discussed this in IRC.

The impact of not taking this is that smart collection icon backgrounds will get overwritten by what's on e.me's servers (instead of what's defined by UX). Peter mentions that he needs to check with Kevin here because he did have a fallback plan previously that may mitigate the issue (something that's already landed).

I'm leaving needinfo on Peter here to respond back when he finds out if we can use the fallback plan or if we have to take this patch from a product perspective.

Comment 47

[Lawrence Mandel [:lmandel] (use needinfo)]

As Jason noted, we discussed this in triage. Clearing ni on me.

Comment 48

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8453096 [details] [review]
Pull Request

This bug should have received a response earlier on the approval request. At this point I'm going to clear the approval request as the bug is 2.0? and being reviewed by triage.

Comment 49

[Gregor Wagner [:gwagner]]

Kevin, do we have a backup in place here or do we need this patch?

Comment 50

[Kevin Grandon :kgrandon]

(In reply to Gregor Wagner [:gwagner] from comment #49)
> Kevin, do we have a backup in place here or do we need this patch?

The backup is to use the fairly ugly icons that shipped in 1.4. I think we need this patch for the visual refresh work - as the homescreen is a visual refresh project, and these icons are front and center on the vertical homescreen.

Comment 51

[Gregor Wagner [:gwagner]]

Lawrence, based on the comment above, I don't think we can ship without the patch here.

Comment 52

[Peter Dolanjski [:pdol]]

Given that this does indeed block the visual refresh (these icons are literally going to be right in the middle of stickers that are adhered to the screens of dummy units in stores in many markets), I'd agree with Gregor here.

Comment 54

[Lawrence Mandel [:lmandel] (use needinfo)]

(In reply to Kevin Grandon :kgrandon from comment #50)
> The backup is to use the fairly ugly icons that shipped in 1.4. I think we
> need this patch for the visual refresh work - as the homescreen is a visual
> refresh project, and these icons are front and center on the vertical
> homescreen.

As an alternative to using the 'ugly icons', can we ask e.me to update the icons that they serve for 2.0?

Comment 55

[Kevin Grandon :kgrandon]

(In reply to Lawrence Mandel [:lmandel] from comment #54)
> As an alternative to using the 'ugly icons', can we ask e.me to update the
> icons that they serve for 2.0?

This was the route we initially tried to take but it was a non-starter. Even if we did want to go that route we would still need to uplift a patch to update the default icon set on the device.

Comment 56

[Kevin Grandon :kgrandon]

This now blocks blockers and needs to be uplifted. Will setup some time with QA to hammer on smart collections after uplifts.

2.0: https://github.com/mozilla-b2g/gaia/commit/505f8aacc7c2fcc91235de54ef49d650c0b9647f

Comment 57

[Gregor Wagner [:gwagner]]

I discussed the pros and cons of this bug with RM and we agreed to take this bug in order to fix other blockers.