Closed Bug 920481 Opened 6 years ago Closed 6 years ago

[app manager] if the app is running when we click "update", the app is not killed

Categories

(DevTools Graveyard :: WebIDE, defect, P1)

x86
All
defect

Tracking

(blocking-b2g:koi+, firefox27 wontfix, firefox28 verified, b2g-v1.2 fixed, b2g-v1.3 fixed)

VERIFIED FIXED
Firefox 28
blocking-b2g koi+
Tracking Status
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)

No description provided.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 922707
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)
(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)
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).
Attached patch Reload app frame after update (obsolete) — Splinter Review
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)
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
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)
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.
Attachment #818574 - Flags: review?(paul)
Attachment #818574 - Flags: review?(jryans)
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+
Attachment #818576 - Flags: review?(paul) → review+
Attachment #818574 - Flags: review?(paul) → review+
(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.
Attachment #819595 - Flags: review+
Keywords: checkin-needed
Whiteboard: [needed-in-aurora-26]
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 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+
Whiteboard: [needed-in-aurora-26][fixed-in-fx-team] → [needed-in-aurora-26]
Attachment #821060 - Flags: review?(paul) → review+
Keywords: checkin-needed
Whiteboard: [needed-in-aurora-26]
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8bc8a56998dd
https://hg.mozilla.org/mozilla-central/rev/874714520261
Status: ASSIGNED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 28
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?
Blocks: 947514
Alex, can we uplift that to the Simulator in the meantime?
Flags: needinfo?(poirot.alex)
I have this problem with the 1.2 simulator and it is quite annoying. Please uplift ASAP?
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)
(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?
blocking-b2g: --- → koi?
Duplicate of this bug: 947514
Go ahead & land this - this is critical for dev tools experience with apps.
blocking-b2g: koi? → koi+
(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)
Attachment #821060 - Flags: approval-mozilla-b2g26?
Keywords: verifyme
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)
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
Status: RESOLVED → VERIFIED
Keywords: verifyme
Product: Firefox → DevTools
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.