Closed
Bug 920481
Opened 7 years ago
Closed 7 years ago
[app manager] if the app is running when we click "update", the app is not killed
Categories
(DevTools Graveyard :: WebIDE, defect, P1)
Tracking
(blocking-b2g:koi+, firefox27 wontfix, firefox28 verified, b2g-v1.2 fixed, b2g-v1.3 fixed)
People
(Reporter: paul, Assigned: ochameau)
References
Details
(Whiteboard: [needs-coverage])
Attachments
(2 files, 3 obsolete files)
3.86 KB,
patch
|
ochameau
:
review+
|
Details | Diff | Splinter Review |
3.56 KB,
patch
|
paul
:
review+
jryans
:
feedback+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Updated•7 years ago
|
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 922707
Reporter | ||
Updated•7 years ago
|
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Is stopping and starting the app after install what we want here? Or is the some kind of faster "refresh" we can offer to reload the app?
Assignee: nobody → jryans
Status: REOPENED → ASSIGNED
Flags: needinfo?(paul)
Reporter | ||
Comment 3•7 years ago
|
||
(In reply to J. Ryan Stinnett [:jryans] from comment #2) > Is stopping and starting the app after install what we want here? Yes.
Flags: needinfo?(paul)
Assignee | ||
Comment 4•7 years ago
|
||
That's a poor UX. Seeing the app close/open doesn't look very important/usefull and will take sigificant amount of time due to the animation. Also closing/opening apps are complex operations where we still don't really know exactly when the app is done closing/opening. So I'm expecting such behavior would be harder to implement right. I think we should try to do what simulator was doing, i.e. refreshing the frame. The simulator was having the bad practice to call gaia codebase, it was calling this method: https://mxr.mozilla.org/gaia/source/apps/system/js/window.js#143 But we can simply fetch the frame from the actor and call frame.reload(true).
Alex, I've attempted your plan of calling frame.reload(true), but it doesn't seem to work too well for me. The page flashes briefly while reloading, but it reappears with the old content. Is there some additional cache clearing I need to do? I am able to see the new content by stopping and starting the app after install.
Attachment #818167 -
Flags: feedback?(poirot.alex)
Assignee | ||
Comment 6•7 years ago
|
||
I've been looking at this and for me it seems to work only once, that's a really weird behavior. Luca identified that we are missing a fix we had in the old simulator app actor, where we were flushing the jar: http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.jsm#1408 I tried to also fire this flush notification, but it doesn't fix this... I'll look into this.
Assignee: jryans → poirot.alex
Assignee | ||
Comment 7•7 years ago
|
||
Here is the UI patch, that just call the existing reload method of BrowserTabActor that does a window.location.reload()
Attachment #818167 -
Attachment is obsolete: true
Attachment #818167 -
Flags: feedback?(poirot.alex)
Assignee | ||
Comment 8•7 years ago
|
||
Here is a device fix, that's an uplift from the old simulator codebase. We need to flush the jar cache when updating the app zip package. But it is a bit more complex on device, with apps running OOP. We have to flush the cache in the app process itself.
Assignee | ||
Updated•7 years ago
|
Attachment #818574 -
Flags: review?(paul)
Attachment #818574 -
Flags: review?(jryans)
Assignee | ||
Updated•7 years ago
|
Attachment #818576 -
Flags: review?(paul)
Attachment #818576 -
Flags: review?(jryans)
Comment on attachment 818574 [details] [diff] [review] Call BrowserTabActor.reload when the app is updated to refresh the app r=paul Review of attachment 818574 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/app-manager/content/projects.js @@ +187,5 @@ > + }); > + }, > + > + reload: function (project) { > + getTargetForApp(this.connection.client, This whole function could be moved to app-actor-front.js or a similar file. I was liking the trend of separating UI logic from the calls to the debugger API that we got from having things in app-actor-front.js, so it would be nice to continue that. That could return a promise you can use to log "App reloaded". Hopefully "soon" we can refactor this file a bit, as it is quite a mess...
Attachment #818574 -
Flags: review?(jryans) → feedback+
Comment on attachment 818576 [details] [diff] [review] Ensure flushing jar cache when updating an app r=paul Review of attachment 818576 [details] [diff] [review]: ----------------------------------------------------------------- Looks good overall! It's quite a nice UX to see it reload in place! :D ::: toolkit/devtools/server/actors/webapps.js @@ +422,5 @@ > + // http://hg.mozilla.org/mozilla-central/annotate/aaefec5d34f8/dom/apps/src/Webapps.jsm#l1125 > + // Do it in parent process for the simulator > + let jar = installDir.clone(); > + jar.append("application.zip"); > + Services.obs.notifyObservers(jar, "flush-cache-entry", null); Is there any harm to running this on device? I guess it will just be ignored safely...?
Attachment #818576 -
Flags: review?(jryans) → feedback+
Reporter | ||
Updated•7 years ago
|
Attachment #818576 -
Flags: review?(paul) → review+
Reporter | ||
Updated•7 years ago
|
Attachment #818574 -
Flags: review?(paul) → review+
Assignee | ||
Comment 11•7 years ago
|
||
Attachment #818574 -
Attachment is obsolete: true
Assignee | ||
Comment 12•7 years ago
|
||
(In reply to J. Ryan Stinnett [:jryans] from comment #10) > ::: toolkit/devtools/server/actors/webapps.js > @@ +422,5 @@ > > + // http://hg.mozilla.org/mozilla-central/annotate/aaefec5d34f8/dom/apps/src/Webapps.jsm#l1125 > > + // Do it in parent process for the simulator > > + let jar = installDir.clone(); > > + jar.append("application.zip"); > > + Services.obs.notifyObservers(jar, "flush-cache-entry", null); > > Is there any harm to running this on device? I guess it will just be > ignored safely...? Yes, especially since we give the precise application jar path. The jar channel will only prune cache for this particular jar.
Assignee | ||
Updated•7 years ago
|
Attachment #819595 -
Flags: review+
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Reporter | ||
Updated•7 years ago
|
Whiteboard: [needed-in-aurora-26]
Assignee | ||
Comment 13•7 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/54da01870611 https://hg.mozilla.org/integration/fx-team/rev/1864e27d5ee4
Keywords: checkin-needed
Whiteboard: [needed-in-aurora-26] → [needed-in-aurora-26][fixed-in-fx-team]
Comment 14•7 years ago
|
||
Backed out for test_webapps_actor.html timeouts: https://tbpl.mozilla.org/php/getParsedLog.php?id=29550901&tree=Fx-Team https://tbpl.mozilla.org/php/getParsedLog.php?id=29551027&tree=Fx-Team remote: https://hg.mozilla.org/integration/fx-team/rev/dad5e53c31a9 remote: https://hg.mozilla.org/integration/fx-team/rev/987ac1967cbe
Assignee | ||
Comment 15•7 years ago
|
||
Attachment #818576 -
Attachment is obsolete: true
Assignee | ||
Comment 16•7 years ago
|
||
Comment on attachment 821060 [details] [diff] [review] Ensure flushing jar cache when updating an app. [fixed desktop exception] Review of attachment 821060 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/devtools/server/actors/webapps.js @@ +768,5 @@ > _appFrames: function () { > + // For now, we only support app frames on b2g > + if (Services.appinfo.ID != "{3c2e2abc-06d4-11e1-ac3b-374f68613e61}") { > + return; > + } I had to add this to prevent test exceptions on desktop.
Attachment #821060 -
Flags: review?(paul)
Attachment #821060 -
Flags: feedback?(jryans)
Comment on attachment 821060 [details] [diff] [review] Ensure flushing jar cache when updating an app. [fixed desktop exception] Review of attachment 821060 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. Would not have guessed that was needed, but good to know for the future!
Attachment #821060 -
Flags: feedback?(jryans) → feedback+
Assignee | ||
Comment 18•7 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=3004cda61bb9
Reporter | ||
Updated•7 years ago
|
Whiteboard: [needed-in-aurora-26][fixed-in-fx-team] → [needed-in-aurora-26]
Reporter | ||
Updated•7 years ago
|
Attachment #821060 -
Flags: review?(paul) → review+
Reporter | ||
Updated•7 years ago
|
Keywords: checkin-needed
Reporter | ||
Updated•7 years ago
|
Whiteboard: [needed-in-aurora-26]
Assignee | ||
Comment 19•7 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/8bc8a56998dd https://hg.mozilla.org/integration/fx-team/rev/874714520261
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 20•7 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/8bc8a56998dd https://hg.mozilla.org/mozilla-central/rev/874714520261
Status: ASSIGNED → RESOLVED
Closed: 7 years ago → 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 28
Reporter | ||
Comment 21•7 years ago
|
||
Comment on attachment 821060 [details] [diff] [review] Ensure flushing jar cache when updating an app. [fixed desktop exception] NOTE: This flag is now for security issues only. 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 #): new feature (app manager) User impact if declined: repushing an app via the app manager doesn't update the app, the user has to kill the app manually Testing completed: works with FxOS 1.3 Risk to taking this patch (and alternatives if risky): low String or UUID changes made by this patch: none
Attachment #821060 -
Flags: approval-mozilla-b2g26?
Reporter | ||
Comment 22•7 years ago
|
||
Alex, can we uplift that to the Simulator in the meantime?
Flags: needinfo?(poirot.alex)
Comment 23•7 years ago
|
||
I have this problem with the 1.2 simulator and it is quite annoying. Please uplift ASAP?
Assignee | ||
Comment 24•7 years ago
|
||
We no longer build a custom b2g runtime, nor have a custom copy of webapps actor. So we can't really uplift it as-is on the simulator without uplifting this patch on m-c. Otherwise, we should ensure that approval-mozilla-b2g26 is still tracked...
Flags: needinfo?(poirot.alex)
Reporter | ||
Comment 25•7 years ago
|
||
(In reply to Garrett Robinson [:grobinson] from comment #23) > I have this problem with the 1.2 simulator and it is quite annoying. Please > uplift ASAP? Can you use version 1.3 in the meantime?
Reporter | ||
Updated•7 years ago
|
blocking-b2g: --- → koi?
Comment 27•7 years ago
|
||
Go ahead & land this - this is critical for dev tools experience with apps.
blocking-b2g: koi? → koi+
Reporter | ||
Comment 28•7 years ago
|
||
(In reply to Jason Smith [:jsmith] from comment #27) > Go ahead & land this - this is critical for dev tools experience with apps. I'm not sure how to land this. What's the repo?
Flags: needinfo?(jsmith)
Updated•7 years ago
|
Attachment #821060 -
Flags: approval-mozilla-b2g26?
Comment 29•7 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/7224ee49d76a https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/7d6aae88ff50
status-b2g-v1.2:
--- → fixed
status-b2g-v1.3:
--- → fixed
status-firefox27:
--- → wontfix
status-firefox28:
--- → fixed
Flags: needinfo?(jsmith)
Comment 30•7 years ago
|
||
This is awesome! Just downloaded the latest 1.3 simulator and clicking "Update" worked perfectly. Thanks everyone! (Now I can continue working on adding OTR support to https://github.com/waalt/loqui with minimal frustration)
Whiteboard: [needs-coverage]
Comment 31•7 years ago
|
||
Verified as fixed on Firefox 28 beta 6 under Win 7 64-bit and Ubuntu 13.04 64-bit using 1.3 Simulator and an inagi device having: Gaia 49312fb89b90eca48a8011b94a1a1a751daf32d2 Gecko https://hg.mozilla.org/mozilla-central/rev/1507f021ac93 BuildID 20140225160202 Version 30.0a1 ro.build.version.incremental=eng.cltbld.20140225.192300 ro.build.date=Tue Feb 25 19:48:26 EST 2014
Updated•3 years ago
|
Product: Firefox → DevTools
Updated•1 year ago
|
Product: DevTools → DevTools Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•