Closed Bug 1038578 Opened 5 years ago Closed 5 years ago
[Collection App] Icons are not updated after installing hosted apps from marketplace
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
Status: NEW → ASSIGNED
I would like to have your feedback before spending a lot of time with marionette
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.
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
taking, thanks Cristian :)
Assignee: crdlc → amirn
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 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.
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
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?
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.
Jacqueline, would UX block on that bug?
QA Whiteboard: [VH-FL-blocking-][VH-FC-blocking-] → [VH-FL-blocking-][VH-FC-blocking?]
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?
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?
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.
(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.
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-]
(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.
Blocking another blocker.
blocking-b2g: --- → 2.0+
Needs rebasing for v2.0 uplift.
Removing flag since bug was fixed
Can't rebase, the dependencies must be uplifted for this to land.
You need to log in before you can comment on or make changes to this bug.