Closed Bug 1038578 Opened 5 years ago Closed 5 years ago

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

Categories

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

All
Gonk (Firefox OS)
defect
Not set

Tracking

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

RESOLVED FIXED
2.1 S1 (1aug)
blocking-b2g 2.0+
Tracking Status
b2g-v2.0 --- fixed
b2g-v2.1 --- fixed

People

(Reporter: crdlc, Assigned: amirn)

References

Details

(Whiteboard: [systemsfe])

Attachments

(1 file, 3 obsolete files)

46 bytes, text/x-github-pull-request
Details | Review
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
Assignee: nobody → crdlc
Blocks: 1015336
Status: NEW → ASSIGNED
Attached file Github pull request (obsolete) —
I would like to have your feedback before spending a lot of time with marionette
Attachment #8456029 - Flags: feedback?(kgrandon)
Attachment #8456029 - Flags: feedback?(amirn)
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.
Attachment #8456029 - Flags: feedback?(kgrandon)
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
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
Whiteboard: [systemsfe]
taking, thanks Cristian :)
Assignee: crdlc → amirn
Attached file Pull Request (obsolete) —
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 :)
Attachment #8456029 - Attachment is obsolete: true
Attachment #8456029 - Flags: feedback?(amirn)
Attachment #8458000 - Flags: review?(crdlc)
Attachment #8458000 - Flags: feedback?(kgrandon)
Comment on attachment 8458000 [details] [review]
Pull Request

Looks like a good approach to me, thanks!
Attachment #8458000 - Flags: feedback?(kgrandon) → feedback+
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
Attachment #8458000 - Flags: review?(crdlc) → review+
Blocking .next until this is landed or blocking status.
Blocks: vertical-home-next
No longer blocks: 1015336
Attached file Part 2 (obsolete) —
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.
Attachment #8459295 - Flags: review?(crdlc)
Summary: [Collection App] Icons are not updated after installing apps from marketplace → [Collection App] Icons are not updated after installing hosted apps from marketplace
Blocks: 1041295
Comment on attachment 8459295 [details]
Part 2

LGTM, thanks a lot
Attachment #8459295 - Flags: review?(crdlc) → review+
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?
Flags: needinfo?(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.
Flags: needinfo?(kgrandon)
Attached file Pull Request (updated)
Attachment #8458000 - Attachment is obsolete: true
Attachment #8459295 - Attachment is obsolete: true
QA Whiteboard: [VH-FL-blocking-][VH-FC-blocking-]
Jacqueline, would UX block on that bug?
QA Whiteboard: [VH-FL-blocking-][VH-FC-blocking-] → [VH-FL-blocking-][VH-FC-blocking?]
Flags: needinfo?(jsavory)
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?
Flags: needinfo?(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?
Flags: needinfo?(kgrandon)
Depends on: 1041607
failing test fixed in bug 1041607.

landed in master: https://github.com/mozilla-b2g/gaia/commit/954d736670e1a0886b3dcabae4310f9cc7a84f37
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
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
Attachment #8459585 - Flags: approval-gaia-v2.0?
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.
Flags: needinfo?(amirn)
(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?
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.
Rudy is correct, the keyboard tests timeouts with the collection app geolocation prompt open :/
Reverted the patch locally and the keyboard tests pass.
Flags: needinfo?(amirn)
Submitted a fix to bug 1041993.
Comment on attachment 8459585 [details] [review]
Pull Request (updated)

It is too late to take a new feature into release 2.0 and hence -
Attachment #8459585 - Flags: approval-gaia-v2.0? → approval-gaia-v2.0-
(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).
QA Whiteboard: [VH-FL-blocking-][VH-FC-blocking?] → [VH-FL-blocking-][VH-FC-blocking-]
Flags: needinfo?(praghunath)
(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.
Clarified this in IRC with Preeti - we're sticking with a- here.
Flags: needinfo?(praghunath)
Blocks: 1043725
Blocking another blocker.
blocking-b2g: --- → 2.0+
Needs rebasing for v2.0 uplift.
Flags: needinfo?(amirn)
Target Milestone: --- → 2.1 S1 (1aug)
Removing flag since bug was fixed
Flags: needinfo?(jsavory)
I think this is too risky to uplift. We need bug 1016221 uplfited first.
Depends on: 1016221
Can't rebase, the dependencies must be uplifted for this to land.
Flags: needinfo?(amirn)
Depends on: 1039311
You need to log in before you can comment on or make changes to this bug.