Manifest properties aren't updated when installing apps multiple times

RESOLVED FIXED in Firefox 26

Status

()

Firefox
Developer Tools: WebIDE
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: ochameau, Assigned: ochameau)

Tracking

unspecified
Firefox 26
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [needs-coverage])

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
Created attachment 775704 [details] [diff] [review]
Uplift simulator's patch with an additional xpcshell test

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?
(Assignee)

Comment 3

5 years ago
(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?
(Assignee)

Comment 4

5 years ago
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)

Updated

5 years ago
Assignee: nobody → poirot.alex
(Assignee)

Comment 5

5 years ago
Created attachment 779180 [details] [diff] [review]
Fix promise related exception in webapps actor.

The issue was really simple, the bad thing, we don't have unit test covering b2g remote debugging :(
Attachment #775704 - Attachment is obsolete: true
(Assignee)

Comment 6

5 years ago
Created attachment 779181 [details] [diff] [review]
Manifest properties aren't updated when installing apps multiple times

New patch, with only the fix.
(Assignee)

Comment 7

5 years ago
Created attachment 779183 [details] [diff] [review]
Convert webapps xpcshell test to mochitest-plain and cover app reinstall and redirects manifest property

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
(Assignee)

Updated

5 years ago
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+

Updated

5 years ago
Blocks: 896695
(Assignee)

Updated

5 years ago
Component: Developer Tools → Developer Tools: App Manager

Updated

5 years ago
Blocks: 912913
(Assignee)

Comment 9

5 years ago
Created attachment 802272 [details] [diff] [review]
Uplift simulator patch to m-c
Attachment #779180 - Attachment is obsolete: true
Attachment #779181 - Attachment is obsolete: true
Attachment #779183 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #802272 - Flags: review?(fabrice)
(Assignee)

Comment 10

5 years ago
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+
(Assignee)

Comment 12

5 years ago
Created attachment 802321 [details] [diff] [review]
Fixed nit
Attachment #802272 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #802321 - Flags: review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
Whiteboard: [land-in-fx-team]
https://hg.mozilla.org/integration/b2g-inbound/rev/511d7aa49409
Keywords: checkin-needed
Whiteboard: [land-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/511d7aa49409
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
Whiteboard: [needs-coverage]
You need to log in before you can comment on or make changes to this bug.