Julien Wajsberg [:julienw] Assignee
	Description • 11 years ago

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.

	Julien Wajsberg [:julienw] Assignee
	Comment 1 • 11 years ago

asking tef? here per bug 860681 comment 28.

	Jason Smith [:jsmith]
	Comment 2 • 11 years ago

This is a dupe. Let me find it...

	Jason Smith [:jsmith]
	Comment 3 • 11 years ago

Found it.

	Jason Smith [:jsmith]
	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.

	Alex Keybl [:akeybl]
	Comment 5 • 11 years ago

Not user critical, not a blocker.

	Jason Smith [:jsmith]
	Comment 6 • 11 years ago

Not a regression - this has been known since the app updates implementation landed.

	Julien Wajsberg [:julienw] 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.

	Wayne Chang [:wchang]
	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?

	Julien Wajsberg [:julienw] 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.

	Julien Wajsberg [:julienw] 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.

	Lukas Blakk [:lsblakk] use ?needinfo
	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.

	Julien Wajsberg [:julienw] 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?

	[:fabrice] Fabrice Desré
	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.

	[:fabrice] Fabrice Desré
	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.

	Julien Wajsberg [:julienw] 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.

	Jason Smith [:jsmith]
	Comment 16 • 11 years ago

Julien - Can you get a test here?

	[:fabrice] Fabrice Desré
	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/

	Julien Wajsberg [:julienw] Assignee
	Comment 18 • 11 years ago

Well yes, the second patch obsoletes the first one...

Ok, I'll add a test.

	[:fabrice] Fabrice Desré
	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?

	Julien Wajsberg [:julienw] 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.

	Julien Wajsberg [:julienw] 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/ ?

	Jason Smith [:jsmith]
	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.

	Julien Wajsberg [:julienw] 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.

	Jason Smith [:jsmith]
	Comment 24 • 11 years ago

Julien - Do you have commit level 1? That's what needed to issue a try build.

	Julien Wajsberg [:julienw] Assignee
	Comment 25 • 11 years ago

Jason, I should, I reopened Bug 853369 to see what happens.

	Fernando Jiménez Moreno [:ferjm]
	Comment 26 • 11 years ago

Try build https://tbpl.mozilla.org/?tree=Try&rev=74220a0845cb

	Fernando Jiménez Moreno [:ferjm]
	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.

	Julien Wajsberg [:julienw] Assignee
	Comment 28 • 11 years ago

carrying r=fabrice,ferjm

fixed the test nits

thanks !

	Julien Wajsberg [:julienw] 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

	Ryan VanderMeulen [:RyanVM]
	Comment 30 • 11 years ago

https://hg.mozilla.org/projects/birch/rev/7ee10964f9e7

	Ryan VanderMeulen [:RyanVM]
	Comment 31 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/7ee10964f9e7

	Ryan VanderMeulen [:RyanVM]
	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.

	Julien Wajsberg [:julienw] Assignee
	Comment 33 • 11 years ago

Filed bug 877630 for this.