Closed Bug 893848 Opened 6 years ago Closed 6 years ago

Manifest properties aren't updated when installing apps multiple times

Categories

(DevTools :: WebIDE, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 26

People

(Reporter: ochameau, Assigned: ochameau)

References

Details

(Whiteboard: [needs-coverage])

Attachments

(1 file, 5 obsolete files)

When using the webapps actor to push an app multiple times,
all information like app name, descriptions, ... aren't updated after the first install. You have to reboot your phone or uninstall and reinstall the app to get these data updated.

We already fixed that in the simulator, it is only about uplifting the fix and get it tested.
I think that webapp actor tests have been disabled with the removal of the big xpcshell tests ini file :(
I say that because I had to modify the promise library import to make it work locally on b2g desktop tests.

https://tbpl.mozilla.org/?tree=Try&rev=b1a08be46fc5
(In reply to Alexandre Poirot (:ochameau) from comment #1)
> I think that webapp actor tests have been disabled with the removal of the
> big xpcshell tests ini file :(
> I say that because I had to modify the promise library import to make it
> work locally on b2g desktop tests.

I believe you are right, the latest fx-team build in tbpl doesn't contain apps unit tests. I don't see however how the change you made to promises will be useful. I made this change in bug 885318 and now everything in devtools imports the sdk promise library using a lowercase promise variable identifier. Why is reversing this change necessary?
(In reply to Panos Astithas [:past] from comment #2)
> I don't see however how the change you made to promises
> will be useful. I made this change in bug 885318 and now everything in
> devtools imports the sdk promise library using a lowercase promise variable
> identifier. Why is reversing this change necessary?

TBH, I have no clear idea why I had to modify the promise requirement. I'm used to see weird issues on b2g cu.import behavior, so I just made a random modification of import statement and that one works. Would it be the only one test run on b2g?
Or may be my build is somehow broken and I need to clobber?
Looks like it is broken on b2g, I'm seeing this exception on a master build:
[JavaScript Error: "ReferenceError: promise is not defined wa_actorGetAll@resource://gre/modules/devtools/dbg-server.jsm -> resource://gre/modules/devtools/server/actors/webapps.js:302
Assignee: nobody → poirot.alex
The issue was really simple, the bad thing, we don't have unit test covering b2g remote debugging :(
Attachment #775704 - Attachment is obsolete: true
New patch, with only the fix.
I moved the tests to another patch as it is bigger than exception.
I ended up being stuck in xpcshell, there was no way to test it sanely with just a docshell.
So I converted piece of xpcshell to mochitest and added test for this (app updating)
and also a better test for redirects.

https://tbpl.mozilla.org/?tree=Try&rev=72f17cce96c6
Attachment #779180 - Flags: review?(past)
Comment on attachment 779180 [details] [diff] [review]
Fix promise related exception in webapps actor.

Review of attachment 779180 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!
Attachment #779180 - Flags: review?(past) → review+
Blocks: 896695
Component: Developer Tools → Developer Tools: App Manager
Blocks: appmgr_v1
Attached patch Uplift simulator patch to m-c (obsolete) — Splinter Review
Attachment #779180 - Attachment is obsolete: true
Attachment #779181 - Attachment is obsolete: true
Attachment #779183 - Attachment is obsolete: true
Attachment #802272 - Flags: review?(fabrice)
Tests are about to be added in bug 914604.
Comment on attachment 802272 [details] [diff] [review]
Uplift simulator patch to m-c

Review of attachment 802272 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/devtools/server/actors/webapps.js
@@ +173,5 @@
>        reg._saveApps(function() {
>          aApp.manifest = manifest;
> +
> +        // Needed to evict manifest cache on content side
> +        // (has to be dispatched first, otherwise other messages lik

nit: messages like
Attachment #802272 - Flags: review?(fabrice) → review+
Attached patch Fixed nitSplinter Review
Attachment #802272 - Attachment is obsolete: true
Attachment #802321 - Flags: review+
Keywords: checkin-needed
Whiteboard: [land-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/511d7aa49409
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
Whiteboard: [needs-coverage]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.