Closed
Bug 863337
Opened 11 years ago
Closed 11 years ago
if an icon is changed in an update, it's not updated before the next reboot
Categories
(Core Graveyard :: DOM: Apps, defect)
Tracking
(blocking-b2g:-, firefox22 wontfix, firefox23 wontfix, firefox24 fixed, b2g18+ fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 wontfix)
People
(Reporter: julienw, Assigned: julienw)
References
Details
(Whiteboard: u=fx-os-user c=scravag-sprint p=1)
Attachments
(1 file, 3 obsolete files)
8.95 KB,
patch
|
julienw
:
review+
lsblakk
:
approval-mozilla-b2g18+
|
Details | Diff | Splinter Review |
STR: * install an app whose you control the manifest * change the icon in that manifest * check for updates (the update will be automatically applied if the app is hosted without an appcache) Expected: * the new icon is there Actual: * the old icon is displayed until the next reboot adding qawanted to see if that happens with other types of app too.
Assignee | ||
Comment 1•11 years ago
|
||
asking tef? here per bug 860681 comment 28.
blocking-b2g: leo? → tef?
Comment 3•11 years ago
|
||
Found it.
Status: NEW → RESOLVED
blocking-b2g: tef? → ---
Closed: 11 years ago
Resolution: --- → DUPLICATE
Whiteboard: DUPEME
Comment 4•11 years ago
|
||
I'm closing out bug 822870 - it's a multi-definition bug. Reopening this one to track fixing this for the homescreen only.
blocking-b2g: --- → tef?
Updated•11 years ago
|
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Updated•11 years ago
|
Blocks: b2g-apps-v1-next
Comment 5•11 years ago
|
||
Not user critical, not a blocker.
Comment 6•11 years ago
|
||
Not a regression - this has been known since the app updates implementation landed.
Keywords: regression
Assignee | ||
Comment 7•11 years ago
|
||
As I explained in bug 860681 comment 28, I'd like this bug to be tef+ instead of Bug 860681. That's why I nominated here.
blocking-b2g: - → tef?
Comment 8•11 years ago
|
||
Hi Julien, How do you propose for this to be fixed and once that is done, how does it not cover the entirety of bug 860681?
Flags: needinfo?(felash)
Assignee | ||
Comment 9•11 years ago
|
||
So this is a gecko problem after all. The main problem here (I think) is that we don't evict the manifest cache. patch is coming. What I don't understand yet is that when updating the apps, for one line of log in Webapps.jsm I get several line of logs in Webapps.js. Does that mean we keep some app objets around ? Fabrice would you know ? Wayne> I ask for leo? here instead of tef? after all. I'll fix Bug 860681 correctly for tef.
Assignee: nobody → felash
blocking-b2g: tef? → leo?
Component: Gaia::Homescreen → DOM: Apps
Flags: needinfo?(felash)
Product: Boot2Gecko → Core
Assignee | ||
Updated•11 years ago
|
Summary: [homescreen] if an icon is changed in an update, it's not updated before the next reboot → if an icon is changed in an update, it's not updated before the next reboot
Assignee | ||
Comment 10•11 years ago
|
||
I _think_ I saw this working at one moment, but now it doesn't. So this is a WIP for now as I have to work on other bugs first.
Comment 11•11 years ago
|
||
The user isn't expecting an icon change, it does get picked up on reboot, this is not critical or a blocker so we'll leave it as tracking and please go ahead with uplift nomination if this is tested & low risk.
blocking-b2g: leo? → -
Assignee | ||
Comment 12•11 years ago
|
||
Comment on attachment 740250 [details] [diff] [review] patch v1 Seems to always work for me now, so r=fabrice?
Attachment #740250 -
Attachment description: WIP patch v1 → patch v1
Attachment #740250 -
Flags: review?(fabrice)
Comment 13•11 years ago
|
||
Comment on attachment 740250 [details] [diff] [review] patch v1 Review of attachment 740250 [details] [diff] [review]: ----------------------------------------------------------------- r=me with nits fixed ::: dom/apps/src/Webapps.js @@ +375,5 @@ > function WebappsApplication() { > this.wrappedJSObject = this; > } > > +var hiddenProps = ['manifest', 'updateManifest']; s/var/let, and use double quotes @@ +376,5 @@ > this.wrappedJSObject = this; > } > > +var hiddenProps = ['manifest', 'updateManifest']; > +var updatableProps = ['installOrigin', 'installTime', 'installState', idem @@ +379,5 @@ > +var hiddenProps = ['manifest', 'updateManifest']; > +var updatableProps = ['installOrigin', 'installTime', 'installState', > + 'lastUpdateCheck', 'updateTime', 'progress', 'downloadAvailable', > + 'downloading', 'readyToApplyDownload', 'downloadSize']; > +// props that we don't update: origin, receipts, manifestURL, removable Please start the comment with a Capital and end with a full stop. @@ +568,5 @@ > + }, this); > + > + hiddenProps.forEach(function(prop) { > + if (msg.app[prop]) { > + this['_' + prop] = msg.app[prop]; nit: double quotes in this file.
Comment 14•11 years ago
|
||
Comment on attachment 740250 [details] [diff] [review] patch v1 Review of attachment 740250 [details] [diff] [review]: ----------------------------------------------------------------- r=me with nits fixed ::: dom/apps/src/Webapps.js @@ +375,5 @@ > function WebappsApplication() { > this.wrappedJSObject = this; > } > > +var hiddenProps = ['manifest', 'updateManifest']; s/var/let, and use double quotes @@ +376,5 @@ > this.wrappedJSObject = this; > } > > +var hiddenProps = ['manifest', 'updateManifest']; > +var updatableProps = ['installOrigin', 'installTime', 'installState', idem @@ +379,5 @@ > +var hiddenProps = ['manifest', 'updateManifest']; > +var updatableProps = ['installOrigin', 'installTime', 'installState', > + 'lastUpdateCheck', 'updateTime', 'progress', 'downloadAvailable', > + 'downloading', 'readyToApplyDownload', 'downloadSize']; > +// props that we don't update: origin, receipts, manifestURL, removable Please start the comment with a Capital and end with a full stop. @@ +568,5 @@ > + }, this); > + > + hiddenProps.forEach(function(prop) { > + if (msg.app[prop]) { > + this['_' + prop] = msg.app[prop]; nit: double quotes in this file.
Attachment #740250 -
Flags: review?(fabrice) → review+
Assignee | ||
Comment 15•11 years ago
|
||
Finally found why it was not working sometimes: because for hosted app without appcache, we didn't change the app.updateTime property, and therefore homescreen skipped the icon retrieval. So requesting review again for this change.
Attachment #740250 -
Attachment is obsolete: true
Attachment #750977 -
Flags: review?(fabrice)
Comment 16•11 years ago
|
||
Julien - Can you get a test here?
Comment 17•11 years ago
|
||
Did you really mean to obsolete the first patch? Also, yes, adding a test would be nice. We already have hosted apps install/update tests in https://mxr.mozilla.org/mozilla-central/source/dom/apps/tests/
Assignee | ||
Comment 18•11 years ago
|
||
Well yes, the second patch obsoletes the first one... Ok, I'll add a test.
Comment 19•11 years ago
|
||
Comment on attachment 750977 [details] [diff] [review] patch v2 Review of attachment 750977 [details] [diff] [review]: ----------------------------------------------------------------- r=me with nits fixed. ::: dom/apps/src/Webapps.js @@ +379,5 @@ > +let hiddenProps = ["manifest", "updateManifest"]; > +let updatableProps = ["installOrigin", "installTime", "installState", > + "lastUpdateCheck", "updateTime", "progress", "downloadAvailable", > + "downloading", "readyToApplyDownload", "downloadSize"]; > +// Props that we don't update: origin, receipts, manifestURL, removable. Since you use these only in the Webapps:CheckForUpdate:Return:OK case, can you move them in the 'case' block?
Attachment #750977 -
Flags: review?(fabrice) → review+
Updated•11 years ago
|
Whiteboard: u=fx-os-user c=scravag-sprint-may-20-31 p=1
Updated•11 years ago
|
Whiteboard: u=fx-os-user c=scravag-sprint-may-20-31 p=1 → u=fx-os-user c=scravag-sprint p=1
Assignee | ||
Comment 20•11 years ago
|
||
(In reply to Fabrice Desré [:fabrice] from comment #19) > Comment on attachment 750977 [details] [diff] [review] > patch v2 > > Review of attachment 750977 [details] [diff] [review]: > ----------------------------------------------------------------- > > r=me with nits fixed. > > ::: dom/apps/src/Webapps.js > @@ +379,5 @@ > > +let hiddenProps = ["manifest", "updateManifest"]; > > +let updatableProps = ["installOrigin", "installTime", "installState", > > + "lastUpdateCheck", "updateTime", "progress", "downloadAvailable", > > + "downloading", "readyToApplyDownload", "downloadSize"]; > > +// Props that we don't update: origin, receipts, manifestURL, removable. > > Since you use these only in the Webapps:CheckForUpdate:Return:OK case, can > you move them in the 'case' block? I was kind of hoping that we could reuse these vars for other cases too. But ok, I moved them. Now I'm trying to write some tests.
Assignee | ||
Comment 21•11 years ago
|
||
(In reply to Fabrice Desré [:fabrice] from comment #17) > We already have hosted apps install/update tests in > https://mxr.mozilla.org/mozilla-central/source/dom/apps/tests/ What about the tests in https://mxr.mozilla.org/mozilla-central/source/dom/tests/mochitest/webapps/ ?
Comment 22•11 years ago
|
||
(In reply to Julien Wajsberg [:julienw] from comment #21) > (In reply to Fabrice Desré [:fabrice] from comment #17) > > We already have hosted apps install/update tests in > > https://mxr.mozilla.org/mozilla-central/source/dom/apps/tests/ > > What about the tests in > https://mxr.mozilla.org/mozilla-central/source/dom/tests/mochitest/webapps/ ? Most of those tests were originally targeted for desktop only (the mochitest chrome tests specifically). You'll probably want to build off the app update tests in the dom/apps/tests directory. We do need to merge those two test directories into one, however.
Assignee | ||
Comment 23•11 years ago
|
||
asking review from :ferjm for the tests part, as this is my first mochitest. Plus my try access does not work so I can't trigger a try build.
Attachment #750977 -
Attachment is obsolete: true
Attachment #754810 -
Flags: review?(ferjmoreno)
Comment 24•11 years ago
|
||
Julien - Do you have commit level 1? That's what needed to issue a try build.
Assignee | ||
Comment 25•11 years ago
|
||
Jason, I should, I reopened Bug 853369 to see what happens.
Comment 26•11 years ago
|
||
Try build https://tbpl.mozilla.org/?tree=Try&rev=74220a0845cb
Comment 27•11 years ago
|
||
Comment on attachment 754810 [details] [diff] [review] patch v3 Review of attachment 754810 [details] [diff] [review]: ----------------------------------------------------------------- Thanks Julien! Looks good. r=me ::: dom/apps/tests/file_packaged_app.template.webapp @@ +8,5 @@ > "developer": { > "name": "DEVELOPERTOKEN", > "url": "DEVELOPERURLTOKEN" > }, > + "icons": { No need to add this to the packaged app manifest template. ::: dom/apps/tests/test_app_update.html @@ +233,5 @@ > cb(); > } > } > + > + // this event is triggered when the app calls "alert" nit: comments start with capital letter and ends with full stop.
Attachment #754810 -
Flags: review?(ferjmoreno) → review+
Assignee | ||
Comment 28•11 years ago
|
||
carrying r=fabrice,ferjm fixed the test nits thanks !
Attachment #754810 -
Attachment is obsolete: true
Attachment #755304 -
Flags: review+
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 29•11 years ago
|
||
Comment on attachment 755304 [details] [diff] [review] patch v4 NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings. [Approval Request Comment] Bug caused by (feature/regressing bug #): - User impact if declined: Some properties of apps (especially the icons) are not updated before a reboot Testing completed: yes + unit tests covering this case Risk to taking this patch (and alternatives if risky): low String or UUID changes made by this patch: none
Attachment #755304 -
Flags: approval-mozilla-b2g18?
Updated•11 years ago
|
Attachment #755304 -
Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+
Comment 30•11 years ago
|
||
https://hg.mozilla.org/projects/birch/rev/7ee10964f9e7
Flags: in-testsuite+
Keywords: checkin-needed
Comment 31•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/7ee10964f9e7
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Comment 32•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g18/rev/09ac1fd2959c Landed w/o the test changes per IRC conversation with Julien. He will file a follow-up for adding them on b2g18.
status-b2g18:
--- → fixed
status-b2g18-v1.0.0:
--- → wontfix
status-b2g18-v1.0.1:
--- → wontfix
status-firefox22:
--- → wontfix
status-firefox23:
--- → wontfix
status-firefox24:
--- → fixed
Assignee | ||
Comment 33•11 years ago
|
||
Filed bug 877630 for this.
Updated•11 years ago
|
No longer blocks: b2g-apps-v1-next
Updated•7 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•