Closed Bug 1016221 Opened 6 years ago Closed 6 years ago

[Collection App] Mozilla CDN Backgrounds for Collection icons

Categories

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

x86
macOS
defect
Not set

Tracking

(blocking-b2g:2.0+, feature-b2g:2.0, b2g-v2.0 fixed, b2g-v2.1 fixed)

RESOLVED FIXED
2.0 S6 (18july)
blocking-b2g 2.0+
feature-b2g 2.0
Tracking Status
b2g-v2.0 --- fixed
b2g-v2.1 --- fixed

People

(Reporter: amirn, Assigned: amirn)

References

Details

(Whiteboard: [systemsfe])

User Story

Collections backgrounds should use images from the Mozilla CDN when available, and use E.me images otherwise.

Attachments

(3 files, 1 obsolete file)

No description provided.
Depends on: 960720
Summary: [Collections App] Backgrounds for Collection icons → [Collection App] Backgrounds for Collection icons
blocking-b2g: --- → backlog
feature-b2g: --- → 2.0
Whiteboard: [systemsfe]
Patryk, please attach app backgrounds to this bug (from the email last night) so that we can get this reassigned to a dev. Thanks!
Flags: needinfo?(padamczyk)
Assignee: nobody → amirn
Duplicate of this bug: 994236
Depends on: 1016225
Target Milestone: --- → 2.0 S3 (6june)
Target Milestone: 2.0 S3 (6june) → 2.0 S4 (20june)
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.
Blocks: vertical-home-next
No longer blocks: collection-app
Summary: [Collection App] Backgrounds for Collection icons → [Collection App] System Backgrounds for Collection icons
changed the scope of the bug to system background only so we can land basic support sooner with bug 1024387
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?
Flags: needinfo?(pdolanjski)
Sorry, it's not clear: what is the implication of not having this in 2.0?
Flags: needinfo?(pdolanjski) → needinfo?(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).
Flags: needinfo?(amirn)
QA Whiteboard: [VH-FL-blocking?][VH-FC-blocking?]
Peter, we are still blocked. Is this required for 2.0?
Don't think we can get it done by 6/20
Assignee: amirn → nobody
Flags: needinfo?(pdolanjski)
Patryk, this one is your call.  What do you think?
Flags: needinfo?(pdolanjski) → needinfo?(padamczyk)
User Story: (updated)
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.
Flags: needinfo?(padamczyk)
Component: Gaia → Gaia::Homescreen
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?
Flags: needinfo?(ran)
(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.
Flags: needinfo?(ran)
Thanks Ran.

So, what do the backgrounds from e.me look like with the new design?
Flags: needinfo?(ran)
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 :/
Flags: needinfo?(ran)
(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. :)
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?
Flags: needinfo?(ran)
Yes. Opened Bug #1029040.
Flags: needinfo?(ran)
(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?
(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?
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)?
Flags: needinfo?(ran)
Target Milestone: 2.0 S4 (20june) → 2.0 S5 (4july)
(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.
Flags: needinfo?(ran)
Btw, there are 4 preinstalled and another 6 as optionals.
QA Whiteboard: [VH-FL-blocking?][VH-FC-blocking?] → [VH-FL-blocking?][VH-FC-blocking+]
Blocks: vertical-homescreen
No longer blocks: vertical-home-next
Keeping this blocking until product indicates that we no longer need this for 2.0.
QA Whiteboard: [VH-FL-blocking?][VH-FC-blocking+] → [VH-FL-blocking+][VH-FC-blocking+]
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.
Target Milestone: 2.0 S5 (4july) → 2.0 S6 (18july)
Duplicate of this bug: 1033502
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.
No longer depends on: 960720
See Also: → 1034656
User Story: (updated)
Attached file Assets - Collection Backgrounds (obsolete) —
Attaching the collection backgrounds from bug 965711.
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.
Attachment #8451299 - Attachment is obsolete: true
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/142@1.5x.jpg

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.
Flags: needinfo?(amirn)
Assignee: nobody → amirn
Flags: needinfo?(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?
Flags: needinfo?(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).
Flags: needinfo?(kgrandon)
User Story: (updated)
Summary: [Collection App] System Backgrounds for Collection icons → [Collection App] Mozilla CDN Backgrounds for Collection icons
Attached file Pull Request
Patch is working but need some help with the tests.
Thanks.
Attachment #8453096 - Flags: review?(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).
Attachment #8453096 - Flags: review?(kgrandon) → review+
Duplicate of this bug: 1037026
landed: https://github.com/mozilla-b2g/gaia/commit/9096ff7386da9a5dcf637a50e67d2828a8bbcb87
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Duplicate of this bug: 960720
Duplicate of this bug: 1020134
Blocks: 1029977
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
Attachment #8453096 - Flags: approval-gaia-v2.0?
[Blocking Requested - why for this release]: The approval request doesn't seem to be going anywhere and this blocks another blocker (bug 1029977)
blocking-b2g: backlog → 2.0?
Flags: needinfo?(praghunath)
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.
Flags: needinfo?(praghunath)
Lawrence,

Please review this approval request.
Flags: needinfo?(lmandel)
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.
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.
Flags: needinfo?(pdolanjski)
As Jason noted, we discussed this in triage. Clearing ni on me.
Flags: needinfo?(lmandel)
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.
Attachment #8453096 - Flags: approval-gaia-v2.0?
Kevin, do we have a backup in place here or do we need this patch?
Flags: needinfo?(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.
Flags: needinfo?(kgrandon)
Lawrence, based on the comment above, I don't think we can ship without the patch here.
Flags: needinfo?(lmandel)
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.
Flags: needinfo?(pdolanjski)
Duplicate of this bug: 1044134
(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?
Flags: needinfo?(lmandel) → needinfo?(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.
Flags: needinfo?(kgrandon)
Blocks: 1038578
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
blocking-b2g: 2.0? → ---
I discussed the pros and cons of this bug with RM and we agreed to take this bug in order to fix other blockers.
blocking-b2g: --- → 2.0+
You need to log in before you can comment on or make changes to this bug.