1038578 - [Collection App] Icons are not updated after installing hosted apps from marketplace

Status: RESOLVED FIXED

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

Description

[Cristian Rodriguez (:crdlc)]

STR:

1) Go to Marketplace
2) Install Penguin Pop game for instances
3) Go to vertical home after installing this game
4) Wait some seconds

Expected:

The "Games" SC icon should have been updated

Result:

The icons is pinned properly but the icon is not updated in the vertical home

Comment 1

[Cristian Rodriguez (:crdlc)]

Attachment: Github pull request

I would like to have your feedback before spending a lot of time with marionette

Comment 2

[Kevin Grandon :kgrandon]

Comment on attachment 8456029 [details]
Github pull request

It makes me really sad that the collection object can't just recover from any state during a save or render() call to get everything we need. It seems like we have a lot of duplicate code in various places that is pretty fragile.

Since we're in a rush I'd say this is fine for 2.0 though - let's see what Amir says.

Comment 3

[Amir Nissim (:amirn)]

Crisitan, can you explain what caused the bug?

NativeInfo started out as a small, standalone file and I think we should try and keep it that way.

I would prefer having the icon rendering logic in synchroize.js and avoid adding code and dependencies to native_info.js

Comment 4

[Cristian Rodriguez (:crdlc)]

There are a couple of bugs:

1) To render the icon properly we need to fetch the background and web results like add-to-collection does

2) The iconDirty was implemented wrongly

* before.background.src is null if the smart collection has not been opened previously
* this condition (if (first.length !== before.length)) was wrong, it is oldFirst.length instead of before.length

I don't follow your suggestion when the nativeInfo is the only piece which knows the matches. I cannot fetch collection from synchronize lib because I don't know the matches.

BTW if you know how to do that, please feel free to take this bug, no problem from my side


(In reply to Amir Nissim (:amirn) from comment #3)
> Crisitan, can you explain what caused the bug?
> 
> NativeInfo started out as a small, standalone file and I think we should try
> and keep it that way.
> 
> I would prefer having the icon rendering logic in synchroize.js and avoid
> adding code and dependencies to native_info.js

Comment 5

[Amir Nissim (:amirn)]

taking, thanks Cristian :)

Comment 6

[Amir Nissim (:amirn)]

Attachment: Pull Request

this is a pretty big patch, so f? :kgrandon for another pair of eyes.

thanks for all your great work Cristian! it really helped me getting this done quickly :)

Comment 7

[Kevin Grandon :kgrandon]

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

Looks like a good approach to me, thanks!

Comment 8

[Cristian Rodriguez (:crdlc)]

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

LGTM. I realized, although maybe it is not related to this bug, that installing Cut The Rope, Pacman or Zombies I can read in traces that there are matches but they don't appear in the SC

evme NativeInfo 1 matches for games ["http://pacman.platzh1rsch.ch/pacman-canvas.webapp"]

evme NativeInfo 1 matches for games ["http://zombie-apocalypse.softgames.de/zombie-apocalypse/mozilla/manifest.webapp"]

Any idea? Thanks

Comment 9

[Kevin Grandon :kgrandon]

Blocking .next until this is landed or blocking status.

Comment 10

[Amir Nissim (:amirn)]

Attachment: Part 2

Thanks for your input Crisitian. While investigating I realized I did not handle offline mode. This patch fixes that.

As for the bug you mentioned:

Looks like this happens only for packaged apps, for which the icon is a local asset. The 'install' event fires too early:
https://github.com/mozilla-b2g/gaia/blob/master/apps/collection/js/synchronize.js#L33
at this point the app hasn't finished downloading and retrieving the icon fails.

I'm gonna file a separate bug for that and changing the scope of this bug.

Thanks.

Comment 11

[Cristian Rodriguez (:crdlc)]

Comment on attachment 8459295 [details]
Part 2

LGTM, thanks a lot

Comment 12

[Amir Nissim (:amirn)]

I wanna land this, but the marionette tests fail:
https://tbpl.mozilla.org/?rev=63aba504f5f5cb86d20073bb0363ad631c4779cb&tree=Gaia-Try

run failing tests locally and everything is green.
$ TEST_FILES="apps/verticalhome/test/marionette/collection_pinned_edit_mode_test.js apps/verticalhome/test/marionette/collection_create_test.js apps/verticalhome/test/marionette/collection_pin_result_uninstall_test.js apps/verticalhome/test/marionette/collection_pin_result_test.js apps/verticalhome/test/marionette/collection_bookmark_pinned_test.js apps/verticalhome/test/marionette/collection_pin_dragdrop_test.js apps/verticalhome/test/marionette/collection_uninstall_test.js apps/verticalhome/test/marionette/collection_localization_test.js" ./bin/gaia-marionette
The Radicale CalDAV server is not installed in the virtualenv, we will skip the CalDAV tests.
If you want to run the CalDAV tests, please run |make caldav-server-install| first.
make: `node_modules' is up to date.
DEBUG=* ./node_modules/.bin/mozilla-download  \
		--verbose \
		--product b2g \
		--channel tinderbox \
		--branch mozilla-central b2g

  ․․․․․․․․

  8 passing (2m)

not really sure what's going on.
Kevin, can you help?

Comment 13

[Kevin Grandon :kgrandon]

I have the tests failing locally with your patch and I'm seeing this error:

[marionette error] app://collection.gaiamobile.org/js/objects.js:62 TypeError: props.pinned is undefined

I'm not sure if it's a problem with general creation, or if our mock e.me server is not properly returning results. (If you guys *always* return the pinned property, and our mock server does not, then we can include it.)

Not sure why you're not seeing it fail though... something fishy is going on.

Comment 14

[Amir Nissim (:amirn)]

Attachment: Pull Request (updated)

Comment 15

[Johan Lorenzo [:jlorenzo]]

Jacqueline, would UX block on that bug?

Comment 16

[Amir Nissim (:amirn)]

ok, down to 1 failing test! yay! :(


the error is: 
[marionette error] dummy file:65 Synchronous XMLHttpRequest on the main thread is deprecated because of its detrimental effects to the end user's experience. For more help http://xhr.spec.whatwg.org/

cause by this line:
https://github.com/EverythingMe/gaia/blob/1038578-sync/apps/collection/js/common.js#L65

tbpl log:
https://tbpl.mozilla.org/php/getParsedLog.php?id=44279210&tree=Gaia-Try&full=1#error0


not sure how to fix that. Kevin, can you help?

Comment 17

[Kevin Grandon :kgrandon]

Oh, we *definitely* want true as the last argument to xhr.open to use async XHR. Not sure how this wasn't caught before, but I guess we're surfacing more paths in our tests now?

Comment 18

[Amir Nissim (:amirn)]

failing test fixed in bug 1041607.

landed in master: https://github.com/mozilla-b2g/gaia/commit/954d736670e1a0886b3dcabae4310f9cc7a84f37

Comment 19

[Amir Nissim (:amirn)]

Comment on attachment 8459585 [details] [review]
Pull Request (updated)

[Bug caused by] (feature/regressing bug #): feature
[User impact] if declined: see description
[Testing completed]: Yes
[Risk to taking this patch] (and alternatives if risky): Low

Comment 20

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

The landing seems to cause perma-red of a keyboard test, see Bug 1041993.

Amir,

do you what might be the cause for that test failure?
Please note that test is involved with keyboard app installation and un-installation. 

Thanks.

Comment 21

[Kevin Grandon :kgrandon]

(In reply to Rudy Lu [:rudyl] from comment #20)
> The landing seems to cause perma-red of a keyboard test, see Bug 1041993.
> 
> Amir,
> 
> do you what might be the cause for that test failure?
> Please note that test is involved with keyboard app installation and
> un-installation. 
> 
> Thanks.

Seems iffy that this would break anything related to keyboard. Are you sure it doesn't coincide with a gecko update that broke it?

Comment 22

[Kevin Grandon :kgrandon]

I am verifying by using this pull request to revert the commit: https://github.com/mozilla-b2g/gaia/pull/22029

If tests pass we broke it, if not it's a gecko regression. We need to turn these tests on asap so we can prevent these gecko regressions.

Comment 23

[Amir Nissim (:amirn)]

Rudy is correct, the keyboard tests timeouts with the collection app geolocation prompt open :/
Reverted the patch locally and the keyboard tests pass.

Comment 24

[Kevin Grandon :kgrandon]

Submitted a fix to bug 1041993.

Comment 25

[Preeti Raghunath(:Preeti)]

Comment on attachment 8459585 [details] [review]
Pull Request (updated)

It is too late to take a new feature into release 2.0 and hence -

Comment 26

[Jason Smith [:jsmith]]

(In reply to Preeti Raghunath(:Preeti) from comment #25)
> Comment on attachment 8459585 [details] [review]
> Pull Request (updated)
> 
> It is too late to take a new feature into release 2.0 and hence -

Preeti - To my understanding, I do not think this is a feature request. This is fixing a bug in which someone installs a hosted app to the vertical homescreen, which results in a failure to update the collections icon on the homescreen if the app applies to that relevant collection. I do not think it's a blocking issue (visual impact only, no user impact).

Comment 27

[Jason Smith [:jsmith]]

(In reply to Jason Smith [:jsmith] from comment #26)
> (In reply to Preeti Raghunath(:Preeti) from comment #25)
> > Comment on attachment 8459585 [details] [review]
> > Pull Request (updated)
> > 
> > It is too late to take a new feature into release 2.0 and hence -
> 
> Preeti - To my understanding, I do not think this is a feature request. This
> is fixing a bug in which someone installs a hosted app to the vertical
> homescreen, which results in a failure to update the collections icon on the
> homescreen if the app applies to that relevant collection. I do not think
> it's a blocking issue (visual impact only, no user impact).

I'm adding a needinfo here to ensure that you understand this information for your approval request decision here.

Comment 28

[Jason Smith [:jsmith]]

Clarified this in IRC with Preeti - we're sticking with a- here.

Comment 29

[Gregor Wagner [:gwagner]]

Blocking another blocker.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing for v2.0 uplift.

Comment 31

[Jacqueline Savory [:jsavory] UX]

Removing flag since bug was fixed

Comment 32

[Kevin Grandon :kgrandon]

I think this is too risky to uplift. We need bug 1016221 uplfited first.

Comment 33

[Amir Nissim (:amirn)]

Can't rebase, the dependencies must be uplifted for this to land.

Comment 34

[Kevin Grandon :kgrandon]

2.0: https://github.com/mozilla-b2g/gaia/commit/9bb159afe7b4f506a7abebb7090ee8980bc6a31d