1039311 - [Collection] HomeIcons Singleton (refactor)

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Everything.me, defect)

Description

[Amir Nissim (:amirn)]

Having a single instance of HomeIcons will make things much simpler and less bug prone

Comment 1

[Amir Nissim (:amirn)]

Attachment: Pull Request

Comment 2

[Amir Nissim (:amirn)]

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

This patch also add missing async_storage.js in add_to_collection.html and synchronize.html.

Comment 3

[Amir Nissim (:amirn)]

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

One review should be enough, but you are all welcome to take quick look :)
Thanks.

Comment 4

[Dale Harvey (:daleharvey)]

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

Will defer review to Christian or Kevin being more familiar, but this looks much improved.

We do very much need test for this stuff, would really prefer them particularly in refactor patches, but defered to review

Comment 5

[Kevin Grandon :kgrandon]

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

Took a look and I think this is fine. Assuming we have integration tests that still pass I think our coverage is good enough. Though we do need some more integration tests around app installing/uninstalling - we can get on that for 2.1 though.

Clearing Cristian's review as I think that Dale's F+ and my R+ should suffice. Cristian - feel free to weigh in though. Thanks!

Comment 6

[Amir Nissim (:amirn)]

landed: https://github.com/mozilla-b2g/gaia/commit/0240d66

tbpl: https://tbpl.mozilla.org/?rev=b14ba9d76aaf57906ba9a0c051b8a89ed5b0e1ca&tree=Gaia-Try

Comment 7

[Amir Nissim (:amirn)]

This patch is required for bug 1029977 which is a 2.0 blocker

Comment 8

[Preeti Raghunath(:Preeti)]

Peter,

Do we need this for 2.0?

Comment 9

[Jason Smith [:jsmith]]

(In reply to Preeti Raghunath(:Preeti) from comment #8)
> Peter,
> 
> Do we need this for 2.0?

To add to this comment - the confusion point we have in triage is that this patch looks like internal refactoring. Why can't we just provide a branch-specific patch for bug 1029977 to fix the blocking issue & avoid taking regression risk with a refactoring?

Comment 10

[Kevin Grandon :kgrandon]

(In reply to Jason Smith [:jsmith] from comment #9)
> (In reply to Preeti Raghunath(:Preeti) from comment #8)
> > Peter,
> > 
> > Do we need this for 2.0?
> 
> To add to this comment - the confusion point we have in triage is that this
> patch looks like internal refactoring. Why can't we just provide a
> branch-specific patch for bug 1029977 to fix the blocking issue & avoid
> taking regression risk with a refactoring?

The branch specific patch would just be a squashed patch of these two bugs. We should just uplift this, it's less risky as it's been baking on master.

Comment 11

[Peter Dolanjski [:pdol]]

I can't comment on if this is the right solution, but if this is needed to fix a blocker, then I assume we need to block on it.  (particularly since Kevin says that the branch-specific patch would basically do the same thing)

Comment 12

[Preeti Raghunath(:Preeti)]

Taking in 2.0 and hence 2.0+ for taking bug 1029977

Comment 13

[Kevin Grandon :kgrandon]

2.0: https://github.com/mozilla-b2g/gaia/commit/6cbcf697a7849fa74009a57dbb1b265f2454bdef