902953 - Application Manager in Settings

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: Gaia::Settings, defect)

Description

[Joe Cheng [:jcheng] (please needinfo)]

Application Manager that allow users to look at sizes of applications on the phone and easily understand which app to remove when storage is running low

Comment 1

[Joe Cheng [:jcheng] (please needinfo)]

Attachment: TCL App Manager for FFOS_Jul31.pdf

Comment 2

[Joe Cheng [:jcheng] (please needinfo)]

Please see the attachment for TCL's proposal on contributing this feature
ni? UX and Product for feedback on this feature for v1.2 and future

Comment 3

[Stephany Wilkes]

Flagging Neo, though it seems like this may be a larger discussion, too. The targeted lock for 1.2 is in about a month, which seems like this would then be for 1.3 for later. Are new 1.2 items still being accepted? Maybe Product can comment on the release planning.

Comment 4

[Peter Dolanjski [:pdol]]

Stephany, if UX and Product are good with the proposed feature, there is no reason it can't go in 1.2.

Could you have someone review it on your end from a UX perspective?

Comment 5

[jachen]

It seems Neo is busy so I will give some initial feedback:

- I would recommend removing the 2 tabs for "system" and "app"- to the user it doesn't make much difference. So just create one long list
- For the list of items, I would not show any apps which should not or cannot be killed- like homescreen and system. If the user cannot take action, then do not provide that as an item in the list- it will be confusion especially because those consume so much more than all the others
- As a bonus, it would be great to provide a visualization (pie or bar chart) to show the data to the user in a way that they can process a bit better

The UX requires an updated flow starting from the UX proposal here:
https://bug853350.bugzilla.mozilla.org/attachment.cgi?id=734288
- The flow will need to change from allowing the user to exit the message by click "ok" to allowing them to go directly to settings

Then following through to the new section in settings based on the feedback above.

Ideally the system would know that it needs to free space and kill the apps accordingly to avoid the situation in the first place.

Please let me know any questions.

Comment 6

[jachen]

We can assign someone from our team to make some wireframes. However, if the creator of this bug wants to update the wireframes and we can review again that is fine too.

Please let me know your preference.

Comment 7

[[:fabrice] Fabrice Desré]

Also note that implementing that involves quite a bit of platform work that is not planned today. That is very much at risk for 1.2

Comment 8

[Joe Cheng [:jcheng] (please needinfo)]

Hi Amelie, can your team provide feedback on the changes you have implemented on the platform? thanks

Comment 9

[Amelie Kong]

HI, Guoqiang, please help provide information needed here.

Comment 10

[buri.blff]

Attachment: app-manager-gecko.patch

The change in gecko for app manager

Comment 11

[buri.blff]

Hi Fabrice,
Would you please review the gecko patch, thanks.

Comment 12

[buri.blff]

(In reply to jachen from comment #5)
> It seems Neo is busy so I will give some initial feedback:
> 
> - I would recommend removing the 2 tabs for "system" and "app"- to the user
> it doesn't make much difference. So just create one long list
> - For the list of items, I would not show any apps which should not or
> cannot be killed- like homescreen and system. If the user cannot take
> action, then do not provide that as an item in the list- it will be
> confusion especially because those consume so much more than all the others
> - As a bonus, it would be great to provide a visualization (pie or bar
> chart) to show the data to the user in a way that they can process a bit
> better
> 
> The UX requires an updated flow starting from the UX proposal here:
> https://bug853350.bugzilla.mozilla.org/attachment.cgi?id=734288
> - The flow will need to change from allowing the user to exit the message by
> click "ok" to allowing them to go directly to settings
> 
> Then following through to the new section in settings based on the feedback
> above.
> 
> Ideally the system would know that it needs to free space and kill the apps
> accordingly to avoid the situation in the first place.
> 
> Please let me know any questions.

Hi jachen,

Thanks for your feedback, the following is some of our ideas:

1. For the "system apps" and "user apps" tab:
Since the system apps cannot be uninstalled and the user apps can, we thought it would be nice for user to filter them.

2. For the "useless" apps like homescreen and system:
We combined "app permissions" to "app manager", and the "app permissions" menu could be hidden later, so all the apps should be shown here.
How do you think about this design?

We sorted the apps list by size, so it might be clear for user to choose which app to uninstall.

Comment 13

[buri.blff]

Another idea:
> As a bonus, it would be great to provide a visualization (pie or bar chart) to
> show the data to the user in a way that they can process a bit better

In settings, there is a menu "app storage" which is quite match your requirement,
how about combine it to "app manager"?

Comment 14

[[:fabrice] Fabrice Desré]

(In reply to buri.blff from comment #11)
> Hi Fabrice,
> Would you please review the gecko patch, thanks.

So, this patch covers many different things, and needs to be broken up in different pieces.

We first need to discuss changes to the API (preferably on the dev-webapi mailing list) and then file bugs for each functionality.

Comment 15

[buri.blff]

Attachment: application manager patch for gecko - master

This patch is for master branch. It is gecko part. 

Hi Fabrice,
Would you please review the gecko patch, thanks.

Comment 16

[buri.blff]

Attachment: Application-manager-in-settings-for-gaia.patch

This patch is for master branch. It is gaia part. 

Hi jachen,
Would you please review the gaia patch, thanks.

Comment 17

[buri.blff]

Attachment: Application-manager-in-settings-for-gaia.patch

Hi jachen,
Sorry, would you please review this gaia patch, thanks!

Comment 18

[Jason Smith [:jsmith]]

(In reply to buri.blff from comment #17)
> Created attachment 829182 [details] [diff] [review]
> Application-manager-in-settings-for-gaia.patch
> 
> Hi jachen,
> Sorry, would you please review this gaia patch, thanks!

For a code review involving settings code - you probably want a developer to review the code. Maybe Kaze or Evelyn?

Comment 19

[[:fabrice] Fabrice Desré]

Comment on attachment 829118 [details] [diff] [review]
application manager patch for gecko - master

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

I only took a look at Webapps.js and Webapps.jsm but there are fundamental issues with this patch.
The way you track running apps and close them is closely coupled to the b2g/gaia implementation, so this will fail on our android and desktop runtimes.
You need to make that pluggable, with an implementation for each runtime (it's ok to only provide the b2g implementation for this bug). The usual way to do this is to use an xpcom service with a known contract id that has a different implementation in each runtime. We can also use a js module instead.
Let me know if you need help getting started on that.

::: dom/apps/src/Webapps.js
@@ +463,5 @@
>      return request;
>    },
>  
> +  stop: function() {
> +    debug(" webapps.js stop app" + this.manifestURL + "\n");

there's no debug() function in Webapps.js. Please add it in this patch since it looks based on some custom code.

@@ +483,5 @@
> +                                                 requestID: this.getRequestId(request) });
> +    return request;
> +  },
> +
> +  getDataSize: function(type){

nit: s/type/aType (do the same for all the other function arguments in this file).

@@ +486,5 @@
> +
> +  getDataSize: function(type){
> +    debug("--webapps.js getSize type"+type+"\n");
> +    let request = this.createRequest();
> +

Check the 'type' argument, and return early if it's unsupported.

@@ +496,5 @@
> +  },
> +
> +  clearAppData: function(clearType) {
> +    debug("webapps.js clear app data " + this.manifestURL + "\n");
> +    let request = this.createRequest();

Check the 'clearType' argument, and return early if it's unsupported.

::: dom/interfaces/apps/nsIDOMApplicationRegistry.idl
@@ +103,5 @@
> +   *
> +   * @returns   : A DOMRequest object, returning the app's running status in |result.status|
> +   *              RUNNING V.S. STOP
> +   */
> +  nsIDOMDOMRequest isRunning();

Does that need to be async? I'd rather have a boolean property on the app object.

@@ +114,5 @@
> +   *          "CACHE": Only clear the app cache of the application
> +   *          "ALL": Clear user data and app cache of the application
> +   * @returns   : A DOMRequest object, onsuccess will be called if data is cleared
> +   */
> +  nsIDOMDOMRequest clearAppData(in string clearType);

Nit: Change the string constants to 'UserData', 'Cache' and 'All'

@@ +127,5 @@
> +   *          "ALL": Calculate the size of app + user data + cache
> +   * @returns   : A DOMRequest object, returning the result in |result|
> +   */
> +  nsIDOMDOMRequest getDataSize(in string type);
> +  

nit: trailing white space, and change the constants to 'App', 'UserData', 'Cache', 'All'. Is the 'Cache' size the Offline cache and/or the http cache?

@@ +189,5 @@
> +   * Return an array of the running applications.
> +   *
> +   * @returns   : A DOMRequest object, returning the app's manifestURL array in |result|
> +   */
> +  nsIDOMDOMRequest getRunningApps();

Remove that if we can use a sync boolean property for isRunning

Comment 20

[buri.blff]

(In reply to Fabrice Desré [:fabrice] from comment #19)
> Comment on attachment 829118 [details] [diff] [review]
> application manager patch for gecko - master
> 
> Review of attachment 829118 [details] [diff] [review]:
> -----------------------------------------------------------------

Dear Fabrice,

Thanks for your review!
Please help us to make sure the following questions.
Is there OK for us to add a readonly attribute runState instead of isRunning() function on ApplicationMgmt object,
and put stop() function with a argument on ApplicationMgmt? 
Or should we add a new interface in file nsIDOMApplicationRegistry.idl?

Comment 21

[[:fabrice] Fabrice Desré]

(In reply to buri.blff from comment #20)
> Please help us to make sure the following questions.
> Is there OK for us to add a readonly attribute runState instead of
> isRunning() function on ApplicationMgmt object,

What would be the values of runState? If it's a boolean, 'isRunning' is a better name. That would be a property of the Application object, not of ApplicationMgmt.

> and put stop() function with a argument on ApplicationMgmt? 

Yes, like we did for uninstall().

> Or should we add a new interface in file nsIDOMApplicationRegistry.idl?

I don't think we need a new interface.

Comment 22

[[:fabrice] Fabrice Desré]

Is there any reason to keep this bug as confidential?

Comment 23

[buri.blff]

(In reply to Fabrice Desré [:fabrice] from comment #21)
> What would be the values of runState? If it's a boolean, 'isRunning' is a
> better name. That would be a property of the Application object, not of
> ApplicationMgmt.
> 
Yes, it is a boolean value.
Thanks for your advice.

Comment 24

[buri.blff]

(In reply to Fabrice Desré [:fabrice] from comment #22)
> Is there any reason to keep this bug as confidential?

Now this bug has no necessary to keep confidential, but I have no permission to change it.

Comment 25

[Fabien Cazenave [:kaze]]

Comment on attachment 829182 [details] [diff] [review]
Application-manager-in-settings-for-gaia.patch

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

This is just a quick feedback regarding the coding style.

I haven’t really looked at the logic yet; I’ll have a much closer look at your patch as soon as the Gecko part is ready.

::: apps/settings/elements/app-manager.html
@@ +1,1 @@
> +<element name="appManager" extends="section">

this file should be named `app_manager.html`: no dashes, no uppercase letters.

@@ +7,5 @@
> +      <a href="#root"><span class="icon icon-back">back</span></a>
> +      <h1 data-l10n-id="appmanager-application-manager">Application Manager</h1>
> +    </header>
> +
> +    <span id="appManager-ul"></span>

Is this really supposed to be an /inline/ container?

@@ +9,5 @@
> +    </header>
> +
> +    <span id="appManager-ul"></span>
> +    <div id="appManager-loading">
> +      <span data-l10n-id="appmanager-loading">loading...</span>

Nit: s/.../…/

@@ +20,5 @@
> +    <link rel="stylesheet" type="text/css" href="shared/style/buttons.css"/>
> +    <link rel="stylesheet" type="text/css" href="style/appmanager.css"/>
> +    <script type="application/javascript" src="shared/js/manifest_helper.js"></script>
> +    <script src="js/hiddenapps.js"></script>
> +    <script src="js/appManager.js"/>

Consistency: please always use the same syntax for <script> tags — include a "type" attribute and close with </script>.

::: apps/settings/elements/app_manager_info.html
@@ +70,5 @@
> +    <link rel="stylesheet" type="text/css" href="shared/style/buttons.css"/>
> +    <link rel="stylesheet" type="text/css" href="style/appmanager.css"/>
> +    <script type="application/javascript" src="shared/js/manifest_helper.js"></script>
> +    <script src="js/hiddenapps.js"></script>
> +    <script src="js/appManager.js"/>

Same remark regarding the consistency of <script> attributes.
Naive question: do we need to re-include these scripts? Aren’t they already loaded by app_manager.html?

::: apps/settings/index.html
@@ +115,5 @@
>      <link rel="import" href="/elements/do_not_track.html">
> +
> +    <link rel="import" href="/elements/app-manager.html">
> +    <link rel="import" href="/elements/app_manager_info.html">
> +

Consistency: please don’t skip lines here.

@@ +268,5 @@
>  
> +    <!-- Device :: AppManager -->
> +    <section is="appManager-info" role="region" id="appManager-info"></section>
> +    <section is="appManager" role="region" id="appManager" data-require-sub-panels="true"></section>
> +

Consistency: please don’t skip this line.

@@ +461,5 @@
> +          <li id="app-manager-section">
> +            <small id="app-manager-desc" class="menu-item-desc"></small>
> +            <a id="menuItem-appManager" class="menu-item" href="#appManager" data-l10n-id="appmanager-application-manager">Application Manager</a>
> +          </li>
> +

Consistency: please don’t skip this line.

::: apps/settings/js/appManager.js
@@ +1,1 @@
> +'use strict';

Consistency: this file should be named `app_manager.js`. No dashes, no camelCase.

Make sure this file passes the linter (gjslint). It probably doesn’t.

@@ +13,5 @@
> +var _ = navigator.mozL10n.get;
> +
> +var appManager = {
> +  _blackList: ['app://system.gaiamobile.org/manifest.webapp', 'app://homescreen.gaiamobile.org/manifest.webapp'],
> +  _blackListforStop:['app://system.gaiamobile.org/manifest.webapp', 'app://homescreen.gaiamobile.org/manifest.webapp','app://communications.gaiamobile.org/manifest.webapp'],

These lines are too long, please split:
 _blackList: [
    'app://system.gaiamobile.org/manifest.webapp',
    'app://homescreen.gaiamobile.org/manifest.webapp'
  ],
 _blackListforStop:[
    'app://system.gaiamobile.org/manifest.webapp',
    'app://homescreen.gaiamobile.org/manifest.webapp',
    'app://communications.gaiamobile.org/manifest.webapp'
  ],

@@ +39,5 @@
> +  permissionsList: document.querySelector('#permissions + ul'),
> +  developerHeader: document.getElementById('developer-header'),
> +  developerList: document.getElementById('developer-infos'),
> +  developerName: document.querySelector('#developer-infos > a'),
> +  developerLink: document.querySelector('#developer-infos > small > a'),

Nit: this would be a good place to skip one line. :-)

@@ +50,5 @@
> +    this._totalApp = 0;
> +    this._readyApp = 0;
> +    this.initButtonEvent();
> +
> +    // load the permission table

please reuse loadJSON in js/utils.js.

@@ +96,5 @@
> +      table.plainPermissions.forEach(function permIterator(perm) {
> +        var isExplicit = mozPerms.isExplicit(perm, app.manifestURL, app.origin, false);
> +        if (isExplicit) {
> +          table.explicitCertifiedPermissions.push(perm);
> +

Nit: don’t skip this line.

@@ +244,5 @@
> +        self._displayApp = self._appsInfo[appId];
> +        self._displayApp.appId = appId;
> +        self.showAppInfoDetail();
> +
> +	      Settings.openDialog('appManager-info');

indentation

::: apps/settings/js/hiddenapps.js
@@ +9,5 @@
> +  'app://settings.gaiamobile.org/manifest.webapp',
> +  'app://ringtones.gaiamobile.org/manifest.webapp',
> +  'app://fl.gaiamobile.org/manifest.webapp',
> +  'app://test-keyboard-app.gaiamobile.org/manifest.webapp'
> +]

this file has been included by mistake in your patch, right?

::: apps/settings/locales/settings.en-US.properties
@@ +759,5 @@
> +appmanager-clear-cache = Clear Cache
> +appmanager-system-apps = System Apps
> +appmanager-running = Running
> +appmanager-stop = Stop
> +appmanager-loading = loading...

s/.../…/

@@ +769,5 @@
> +appmanager-confirm-stop = Confirm stop?
> +appmanager-confirm-uninstall = Confirm uninstall?
> +appmanager-confirm-clear-user-data = Confirm clear user data?
> +appmanager-confirm-clear-cache = Confirm clear cache?
> +appmanager-alert-running = App is running,please stop it first.

space after comma

::: apps/settings/locales/settings.fr.properties
@@ +664,5 @@
> +appmanager-cache=Cache
> +appmanager-system-apps=Applications système
> +appmanager-loading=Chrgmt...
> +appmanager-storage=Mémoire
> +appmanager-application=Application

Please leave the non-English locales unchanged, our localizers will take good care of that.

::: apps/settings/style/appmanager.css
@@ +1,1 @@
> +#appManager-loading {

Consistency: this file should be named `app_manager.css`.

Comment 26

[buri.blff]

(In reply to Fabien Cazenave [:kaze] from comment #25)
> Comment on attachment 829182 [details] [diff] [review]
> Application-manager-in-settings-for-gaia.patch
> 
> Review of attachment 829182 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +7,5 @@
> > +      <a href="#root"><span class="icon icon-back">back</span></a>
> > +      <h1 data-l10n-id="appmanager-application-manager">Application Manager</h1>
> > +    </header>
> > +
> > +    <span id="appManager-ul"></span>
> 
> Is this really supposed to be an /inline/ container?

Dear Kaze,
What about this comment? 
The element <span> here is used for the "loading" show.
    <span id="appManager-ul"></span>    
    <div id="appManager-loading">
      <span data-l10n-id="appmanager-loading">loading…</span>
    </div>

Comment 27

[buri.blff]

Attachment: v2 patch for gecko

Dear Fabrice,

This patch is for master branch. It is gecko part. 
Would you please review the v2 patch, thanks.

Comment 28

[buri.blff]

Attachment: v2 patch for gaia

Dear Kaze,

This patch is for master branch. It is gaia part. 
Would you please review the v2 patch, thanks.

Comment 29

[[:fabrice] Fabrice Desré]

Comment on attachment 8334442 [details] [diff] [review]
v2 patch for gecko

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

I'm giving r- because my comment from https://bugzilla.mozilla.org/show_bug.cgi?id=902953#c19 was ignored: this code stills uses the same tight coupling with gaia, which we don't want. Quoting myself: "You need to make that pluggable, with an implementation for each runtime (it's ok to only provide the b2g implementation for this bug). The usual way to do this is to use an xpcom service with a known contract id that has a different implementation in each runtime. We can also use a js module instead.
Let me know if you need help getting started on that."

Note that I stopped reviewing when I hit the _findFrameByName() function in Webapps.jsm

::: dom/apps/src/Webapps.js
@@ +465,5 @@
>    },
>  
> +  get isRunning() {
> +    debug("check app running status " + this.manifestURL);
> +    return state = cpmm.sendSyncMessage("Webapps:GetIsRunning", { origin: this.origin,

we should not need the origin. Apps are identified by their manifest url.

Also, sending a sync message is not great, especially if you will loop over all the apps. Doing a cache + async update should be better.

@@ +475,5 @@
> +    debug("get app data size for type " + aType);
> +    if (aType !== "App" &&
> +        aType !== "UserData" &&
> +        aType !== "Cache" &&
> +        aType !== "All")

nit: use curly braces even for single line if (...) { } blocks.

You could also revrite this one with something like:

if (["App", "UserData", "Cache", "All"].indexOf(aType) == -1) {
  return;
}

@@ +479,5 @@
> +        aType !== "All")
> +      return;
> +
> +    let request = this.createRequest();
> +    cpmm.sendAsyncMessage("Webapps:GetDataSize", { origin: this.origin,

use the manifest url instead of the origin.

@@ +492,5 @@
> +    if (aClearType !== "App" &&
> +        aClearType !== "UserData" &&
> +        aClearType !== "Cache" &&
> +        aClearType !== "All")
> +      return;

same change as in getDataSize()

@@ +495,5 @@
> +        aClearType !== "All")
> +      return;
> +
> +    let request = this.createRequest();
> +    cpmm.sendAsyncMessage("Webapps:ClearAppData", { origin: this.origin,

use the manifest url instead of the origin.

@@ +773,5 @@
>  
> +  stop: function(aApp, aType) {
> +    debug("stop app " + aApp.manifestURL);
> +    let request = this.createRequest();
> +    cpmm.sendAsyncMessage("Webapps:Stop", { origin: aApp.origin,

remove that.

::: dom/apps/src/Webapps.jsm
@@ +103,4 @@
>    children: [ ],
>    allAppsLaunchable: false,
>  
> +  runningApps: [], //For app manager, recording the running apps list

nit: // For the application manager, keeping track of the running applications list.

Update other comments to have a space after '//', start with a capital and end with a full stop.

@@ +1106,5 @@
>          break;
>        case "child-process-shutdown":
>          this.removeMessageListener(["Webapps:Internal:AllMessages"], mm);
> +
> +        //Remove the shutdown app from the running list for app manager

Note that nothing prevents an app from having multiple instances running in general. Your code will not work in this case.

@@ +1107,5 @@
>        case "child-process-shutdown":
>          this.removeMessageListener(["Webapps:Internal:AllMessages"], mm);
> +
> +        //Remove the shutdown app from the running list for app manager
> +        for (let i=0; i<this.runningApps.length; i++){

nit: spaces around operators: 'i = 0', 'i < this...' here and everywhere else.

@@ +1109,5 @@
> +
> +        //Remove the shutdown app from the running list for app manager
> +        for (let i=0; i<this.runningApps.length; i++){
> +          if (msg.appManifestURL == this.getManifestURLByLocalId(this.runningApps[i].appId)){
> +            this.runningApps.splice(i,1);

nit: splice(i, 1);

Comment 30

[buri.blff]

(In reply to Fabrice Desré [:fabrice] from comment #29)
> Comment on attachment 8334442 [details] [diff] [review]
> v2 patch for gecko
> 
> Review of attachment 8334442 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm giving r- because my comment from
> https://bugzilla.mozilla.org/show_bug.cgi?id=902953#c19 was ignored: this
> code stills uses the same tight coupling with gaia, which we don't want.
> Quoting myself: "You need to make that pluggable, with an implementation for
> each runtime (it's ok to only provide the b2g implementation for this bug).
> The usual way to do this is to use an xpcom service with a known contract id
> that has a different implementation in each runtime. We can also use a js
> module instead.
> Let me know if you need help getting started on that."
> 
> Note that I stopped reviewing when I hit the _findFrameByName() function in
> Webapps.jsm
> 

Dear Fabrice,
I haven't got the real mean of these comment. Did you mean the ways to find 
running apps and to stop an app are not suitable for here? Or what they should be from your opinion?

Maybe stop an app can use function close in the webapps.jsm, do you think so?
  let app = this.getAppByManifestURL(aData.manifestURL);
  this.close(app);

But the browser application is very different with other apps, when kill it, the message "child-process-shutdown" could not send.

Comment 31

[buri.blff]

(In reply to Fabrice Desré [:fabrice] from comment #29)
> Comment on attachment 8334442 [details] [diff] [review]
> v2 patch for gecko
> 
> Review of attachment 8334442 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm giving r- because my comment from
> https://bugzilla.mozilla.org/show_bug.cgi?id=902953#c19 was ignored: this
> code stills uses the same tight coupling with gaia, which we don't want.
> Quoting myself: "You need to make that pluggable, with an implementation for
> each runtime (it's ok to only provide the b2g implementation for this bug).
> The usual way to do this is to use an xpcom service with a known contract id
> that has a different implementation in each runtime. We can also use a js
> module instead.
> Let me know if you need help getting started on that."
> 
> Note that I stopped reviewing when I hit the _findFrameByName() function in
> Webapps.jsm

The code for the added API, such as stop an app and get running apps is already in the js module webapps.jsm. Did you mean we should add a new file which is a jsm other than put them in the webapps.jsm?

Comment 32

[[:fabrice] Fabrice Desré]

(In reply to buri.blff from comment #30)

> Dear Fabrice,
> I haven't got the real mean of these comment. Did you mean the ways to find 
> running apps and to stop an app are not suitable for here? Or what they
> should be from your opinion?

Yes, the way to find apps can't be coded like that in webapps.jsm, because :
1) What you do here is b2g specific. That won't work with our other runtimes (android and desktop).
2) That introduces a tight coupling between dom/ and gaia, which is bad. Anything that is b2g specific has to be under b2g/. That's what we do for the activities and payment api for instance.

> Maybe stop an app can use function close in the webapps.jsm, do you think so?
>   let app = this.getAppByManifestURL(aData.manifestURL);
>   this.close(app);

That will not solve the issue, which is how to implement this.close()...

> But the browser application is very different with other apps, when kill it,
> the message "child-process-shutdown" could not send.

Yes, because it's not running out of process. Here's what we have to do:

For each runtime (b2g, android, desktop), we need an implementation of these functionalities that will be called in by webapps.jsm. Myk & Marco (just cced) started working on that because we also want to improve the install/uninstall process in the same way. For your use case, we need this runtime-specific code to be able to:
- close and app.
- keep track of running apps.
From an implementation point of view, that can be either an xpcom component or (preferably) a js module. This module will communicate with shell.js and gaia's system app. Note that we already have support to stop apps in webapps.jsm (see https://mxr.mozilla.org/mozilla-central/search?string=webapps-close).

Comment 33

[buri.blff]

(In reply to Fabrice Desré [:fabrice] from comment #32)
> (In reply to buri.blff from comment #30)
> 
> For your use case, we need this
> runtime-specific code to be able to:
> - close and app.
> - keep track of running apps.
> From an implementation point of view, that can be either an xpcom component
> or (preferably) a js module. This module will communicate with shell.js and
> gaia's system app. 

Well, my understanding is the following:
              app.isRunning           webapps-isRunning             mozChromeEvent                mozContentEvent
settings app -----------------> DOM --------------------> shell.js -----------------> system app ----------------->

shell.js ---------------------------> DOM ---------------------------> settings app (gaia)
           result: app.isRunning             result: app.isRunning

Does this process meet the requirements?

> Note that we already have support to stop apps in
> webapps.jsm (see
> https://mxr.mozilla.org/mozilla-central/search?string=webapps-close).

Yes, I have noticed that, so for the stop implement, I suggest use close directly:
   let app = this.getAppByManifestURL(aData.manifestURL);
   this.close(app);

And in settings app (for application manager part), we call stop(app) to stop an app.

Comment 34

[buri.blff]

Attachment: flow for app.isRunning

Comment 35

[[:fabrice] Fabrice Desré]

(In reply to buri.blff from comment #34)
> Created attachment 8335799 [details]
> flow for app.isRunning

This flow is basically correct. As always, the devil will be in the details ;)
Some things to keep in mind:
- we don't want any direct dependency from dom/ code on b2g/.
- apps can be killed by various mechanisms: closing the window, calling app.close(), or killed by the LMK. isRunning needs to be correct in all cases.

Comment 36

[buri.blff]

(In reply to Fabrice Desré [:fabrice] from comment #35)
> (In reply to buri.blff from comment #34)
> > Created attachment 8335799 [details]
> > flow for app.isRunning
> 
> This flow is basically correct. As always, the devil will be in the details
> ;)
> Some things to keep in mind:
> - we don't want any direct dependency from dom/ code on b2g/.
> - apps can be killed by various mechanisms: closing the window, calling
> app.close(), or killed by the LMK. isRunning needs to be correct in all
> cases.

Dear Fabrice,

I'm totally confused about how to start it. I think I need your help.

If not consider the browser app, we can implement stop(app) like that:
webapps.js
  stop: function(aApp, aType) {
    debug("stop app " + aApp.manifestURL);
    let request = this.createRequest();
    cpmm.sendAsyncMessage("Webapps:Stop", { manifestURL: aApp.manifestURL,
                                            oid: this._id,
                                            type: aType,
                                            timestamp: Date.now(),
                                            requestID: this.getRequestId(request) });
    return request;
  },

webapps.jsm
  stop: function(aData, aMm) {
    let app = this.getAppByManifestURL(aData.manifestURL);
    if (!app) {
      aData.error = "NO_SUCH_APP";
      aMm.sendAsyncMessage("Webapps:Stop:Return:KO", aData);
      return;
    }
    this.close(app);
    aMm.sendAsyncMessage("Webapps:Stop:Return:OK", aData);
  },

It is not direct dependency from dom/ code on b2g/. Did you think it is OK for your suggest？ 
Or should we make a new jsm file(with a new contract ID) which contains the implementation of stop and isRunning?

If we use the flow as comment 34, how can settings app obtain the attribute isRunning(cache + async like your comment #29) result? Settings app ask to obtain an app's running status, but "isRunning" is a readonly attribute, how to send a async request?
I think we do not need to track the running apps, just obtain an app's running status.

Could you tell me what you would expect the process? Thank you very much!

Comment 37

[buri.blff]

Dear Fabrice,

I really need your help. Could you please give me a suggest how to start it to match your advice?
I am really not familiar that the concepts of each runtime (b2g, android, desktop).

My understanding is that make a new javascript modules(jsm) file which contains the stop and isRunning API's implementation. And in webapps.jsm called the two function to get the app' running status or to stop an app.
But as my comment #36, how can a readonly attribute could send a async request.

And you have mentioned Myk & Marco (just cced) started working on that because we also want to improve the install/uninstall process in the same way. Do they have a clear solution yet?

Comment 38

[[:fabrice] Fabrice Desré]

What you outline in comment #36 is ok for now. We'll refactor later if needed.
Do you mind doing just that for now in this bug, and open a new one for isRunning? This one is way more complex.

Comment 39

[buri.blff]

(In reply to Fabrice Desré [:fabrice] from comment #38)
> What you outline in comment #36 is ok for now. We'll refactor later if
> needed.
> Do you mind doing just that for now in this bug, and open a new one for
> isRunning? This one is way more complex.

Dear Fabrice,

It's OK doing just that for now in this bug. I will attach the patch without isRunning for gecko and gaia later. And open a new bug for isRunning.
Thanks!

Comment 40

[buri.blff]

Attachment: v3 patch for gecko

Dear Fabrice,

This patch is for master branch. It is gecko part. This patch has not included isRunning as your comment #38.
Would you please review the v3 patch, thanks.

Comment 41

[buri.blff]

Dears,

For isRunning I have filed a new bug 944277, FYI.

Comment 42

[[:fabrice] Fabrice Desré]

Comment on attachment 8339656 [details] [diff] [review]
v3 patch for gecko

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

Thanks, that looks much better!
Can you update your patch to address the comments? We also need tests for that code.

::: dom/apps/src/Webapps.js
@@ +695,5 @@
>      { name: "Webapps:Uninstall:Return:OK", strongRef: true },
>      { name: "Webapps:Uninstall:Broadcast:Return:OK", strongRef: true },
>      { name: "Webapps:Uninstall:Return:KO", strongRef: true },
> +    { name: "Webapps:Stop:Return:OK", strongRef: true },
> +    { name: "Webapps:Stop:Return:KO", strongRef: true },

Beware that we are now using strong references by default, so this will need to be updated.

::: dom/apps/src/Webapps.jsm
@@ +3290,5 @@
> +    this.close(app);
> +    aMm.sendAsyncMessage("Webapps:Stop:Return:OK", aData);
> +  },
> +
> +  // XXX The size of user data is now the combination of

nit: remove XXX (also in your other comments)

@@ +3298,5 @@
> +  //
> +  // The user data size calculation is now doing in an async way. (due to the
> +  // design of indexDB manager)
> +  _getUserDataSize: function(aManifestURL, aCallback) {
> +    let app = this.getAppByManifestURL(aManifestURL);

bail out if app is null.

@@ +3310,5 @@
> +    // It must restart the device the webappsstore.sqlite will update.
> +    // So we have not got the local storage when
> +    // calcute the user data size now.
> +
> +    // For indexDB.

nit: indexedDB

@@ +3312,5 @@
> +    // calcute the user data size now.
> +
> +    // For indexDB.
> +    let quotaManager = Cc["@mozilla.org/dom/quota/manager;1"]
> +          .getService(Ci.nsIQuotaManager);

nit: align .getService with ["@mozilla..."]

@@ +3315,5 @@
> +    let quotaManager = Cc["@mozilla.org/dom/quota/manager;1"]
> +          .getService(Ci.nsIQuotaManager);
> +
> +    quotaManager.getUsageForURI(
> +      Services.io.newURI(aManifestURL, null, null), //url.

nit: remove comment

@@ +3328,5 @@
> +  },
> +
> +  // Get the size of app offline cache.
> +  // FIXME: The way we find the active cache is quite ugly,
> +  // must be improved later.

please file a followup bug and add the bug number here.

@@ +3335,5 @@
> +    let uri1;
> +    let result = 0;
> +
> +    let cacheService = Cc["@mozilla.org/network/application-cache-service;1"]
> +          .getService(Ci.nsIApplicationCacheService);

nit: align .getService with ["@moz..."]

@@ +3341,5 @@
> +
> +    groups = cacheService.getGroups();
> +    for (let i = 0; i < groups.length; i++) {
> +      uri1 = Services.io.newURI(groups[i], null, null);
> +      if (uri0.asciiHost == uri1.asciiHost) {

I think you can just do (uri1.spec == aManifestURL)

@@ +3359,5 @@
> +  // at system partition and doesn't consume any data partition.
> +  //
> +  // *The size of |HOST APP| could be neglected also.
> +  // The "size" of the app itself only take into consideration of
> +  // the app manifest file, which is supposed to be very small. ( <10K )

They can be larger when holding many localized descriptions.

@@ +3368,5 @@
> +  // Current implementation is quite in convinient but intuitive,
> +  // go through the app dir and sum the file size up.
> +  _getAppSize: function(aManifestURL) {
> +    let result = 0;
> +    for (let id in this.webapps) {

Why do a loop there, and not use getAppByManifestURL() ?

@@ +3411,5 @@
> +      let size = this._getAppSize(aData.manifestURL);
> +      aData.result = size;
> +      aMm.sendAsyncMessage("Webapps:GetDataSize:Return", aData);
> +    }
> +    else if (aData.type == "UserData") {

nit: } else if (....)

@@ +3431,5 @@
> +        aMm.sendAsyncMessage("Webapps:GetDataSize:Return", aData);
> +      }).bind(this));
> +    }
> +    else {
> +      aData.result = {'size': 0, 'error_msg': "UNSOPPORT"};

should be: aData.result = { size: 0, error_msg: "INVALID_DATA_TYPE" }

::: dom/interfaces/apps/nsIDOMApplicationRegistry.idl
@@ +178,5 @@
> +   *               "normal" :
> +   *               "force":
> +   * @returns   : A DOMRequest object, onsuccess will be triggered if the app is stopped
> +   */
> +  nsIDOMDOMRequest stop(in mozIDOMApplication app, [optional] in DOMString type);

The |type| attribute is not implemented at all, so just remove that.

::: dom/quota/QuotaManager.cpp
@@ +2468,5 @@
>      return NS_OK;
>    }
>  
> +  if (!strcmp(aTopic, TOPIC_WEB_APP_CLEAR_DATA) ||
> +      !strcmp(aTopic, "webapps-clear-data-user")) {

You want to use a constant here too, to be consistent with the coding style.

::: layout/build/nsLayoutModule.cpp
@@ +1288,4 @@
>    { JAVASCRIPT_GLOBAL_STATIC_NAMESET_CATEGORY, "PrivilegeManager", NS_SECURITYNAMESET_CONTRACTID },
>    { "app-startup", "Script Security Manager", "service," NS_SCRIPTSECURITYMANAGER_CONTRACTID },
>    { TOPIC_WEB_APP_CLEAR_DATA, "QuotaManager", "service," QUOTA_MANAGER_CONTRACTID },
> +  { "webapps-clear-data-user", "QuotaManager", "service," QUOTA_MANAGER_CONTRACTID },

use a constant here too.

::: netwerk/cache/nsApplicationCacheService.cpp
@@ +211,5 @@
>      NS_IMETHODIMP
>      Observe(nsISupports *aSubject, const char *aTopic, const PRUnichar *aData)
>      {
> +        MOZ_ASSERT(!nsCRT::strcmp(aTopic, TOPIC_WEB_APP_CLEAR_DATA) ||
> +                   !nsCRT::strcmp(aTopic, "webapps-clear-data-cache"));

Use a constant here too.

::: netwerk/cookie/nsCookieService.cpp
@@ +578,5 @@
>    NS_IMETHODIMP
>    Observe(nsISupports *aSubject, const char *aTopic, const PRUnichar *data)
>    {
> +    MOZ_ASSERT(!nsCRT::strcmp(aTopic, TOPIC_WEB_APP_CLEAR_DATA) ||
> +               !nsCRT::strcmp(aTopic, "webapps-clear-data-user"));

and here...

Comment 43

[buri.blff]

(In reply to Fabrice Desré [:fabrice] from comment #42)
> Comment on attachment 8339656 [details] [diff] [review]
> v3 patch for gecko
> 
> Review of attachment 8339656 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: dom/apps/src/Webapps.js
> @@ +695,5 @@
> >      { name: "Webapps:Uninstall:Return:OK", strongRef: true },
> >      { name: "Webapps:Uninstall:Broadcast:Return:OK", strongRef: true },
> >      { name: "Webapps:Uninstall:Return:KO", strongRef: true },
> > +    { name: "Webapps:Stop:Return:OK", strongRef: true },
> > +    { name: "Webapps:Stop:Return:KO", strongRef: true },
> 
> Beware that we are now using strong references by default, so this will need
> to be updated.

OK, I will update it.

> 
> @@ +3298,5 @@
> > +  //
> > +  // The user data size calculation is now doing in an async way. (due to the
> > +  // design of indexDB manager)
> > +  _getUserDataSize: function(aManifestURL, aCallback) {
> > +    let app = this.getAppByManifestURL(aManifestURL);
> 
> bail out if app is null.

Sorry to not check it.

> @@ +3328,5 @@
> > +  },
> > +
> > +  // Get the size of app offline cache.
> > +  // FIXME: The way we find the active cache is quite ugly,
> > +  // must be improved later.
> 
> please file a followup bug and add the bug number here.

OK, I will follow this suggestion.

> @@ +3341,5 @@
> > +
> > +    groups = cacheService.getGroups();
> > +    for (let i = 0; i < groups.length; i++) {
> > +      uri1 = Services.io.newURI(groups[i], null, null);
> > +      if (uri0.asciiHost == uri1.asciiHost) {
> 
> I think you can just do (uri1.spec == aManifestURL)

I think here can not use (uri1.spec == aManifestURL). Because uri1.spec can not be equal to aManifestURL.
For example:
aManifestURL = https://bits.wikimedia.org/WikipediaMobileFirefoxOS/manifest.webapp?feature_profile=3ebdd4ff37b6.46.3
uri1.spec = https://bits.wikimedia.org/WikipediaMobileFirefoxOS/app.appcache#1001+f

> 
> @@ +3359,5 @@
> > +  // at system partition and doesn't consume any data partition.
> > +  //
> > +  // *The size of |HOST APP| could be neglected also.
> > +  // The "size" of the app itself only take into consideration of
> > +  // the app manifest file, which is supposed to be very small. ( <10K )
> 
> They can be larger when holding many localized descriptions.

This comment should be removed. Because now we calculate the app size for HOST APP also.

> 
> @@ +3368,5 @@
> > +  // Current implementation is quite in convinient but intuitive,
> > +  // go through the app dir and sum the file size up.
> > +  _getAppSize: function(aManifestURL) {
> > +    let result = 0;
> > +    for (let id in this.webapps) {
> 
> Why do a loop there, and not use getAppByManifestURL() ?

Oh, no need to do a loop here, I will update it.

> @@ +3431,5 @@
> > +        aMm.sendAsyncMessage("Webapps:GetDataSize:Return", aData);
> > +      }).bind(this));
> > +    }
> > +    else {
> > +      aData.result = {'size': 0, 'error_msg': "UNSOPPORT"};
> 
> should be: aData.result = { size: 0, error_msg: "INVALID_DATA_TYPE" }
> 

It is better using INVALID_DATA_TYPE, thanks!

> ::: dom/interfaces/apps/nsIDOMApplicationRegistry.idl
> @@ +178,5 @@
> > +   *               "normal" :
> > +   *               "force":
> > +   * @returns   : A DOMRequest object, onsuccess will be triggered if the app is stopped
> > +   */
> > +  nsIDOMDOMRequest stop(in mozIDOMApplication app, [optional] in DOMString type);
> 
> The |type| attribute is not implemented at all, so just remove that.
> 

OK.
Thanks you!

Comment 44

[buri.blff]

Attachment: v4 patch for gecko without tests code

Dear Fabrice,

This patch is a gecko part for master branch. Would you please review the v4 patch?
Because I am not good at the gecko tests, before write the tests code, I must study how to execute the tests code. So do really need us to write these tests code?
And, the master branch is different from v1.1 for Offline Cache. When clear the offline cache for an app, it will not download the offline cache files when we launch the app again (device in network). But I test it in desktop nightly, after I clear the offline cache for a website, it can download the offline cache files when I refresh the website. Do you know if the offline cache mechanism is special for b2g?

Thanks!

Comment 45

[buri.blff]

Attachment: v4 patch for gecko without tests code

Dear Fabrice,

Sorry, please review this new patch.

Comment 46

[[:fabrice] Fabrice Desré]

Comment on attachment 8342983 [details] [diff] [review]
v4 patch for gecko without tests code

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

We're almost there! mostly nits, but the observer removals need to be checked.

::: dom/apps/src/Webapps.js
@@ +347,5 @@
>        { name: "Webapps:Connect:Return:OK", weakRef: true },
>        { name: "Webapps:Connect:Return:KO", weakRef: true },
>        { name: "Webapps:FireEvent", weakRef: true },
> +      { name: "Webapps:GetDataSize:Return", weakRef: true },
> +      { name: "Webapps:ClearAppData:Return", weakRef: true },

These were strong refs in the previous patch. Why did that change?

@@ +457,5 @@
>    },
>  
> +  getDataSize: function(aType) {
> +    if (["App", "UserData", "Cache", "All"].indexOf(aType) == -1) {
> +      return;

You can't just return undefined. You need to return a request and fire an error.

@@ +470,5 @@
> +  },
> +
> +  clearAppData: function(aClearType) {
> +    if (["App", "UserData", "Cache", "All"].indexOf(aClearType) == -1) {
> +      return;

You can't just return undefined. You need to return a request and fire an error.

::: dom/apps/src/Webapps.jsm
@@ +3373,5 @@
> +
> +  // Get the size of app offline cache.
> +  _getCacheSize: function(aManifestURL) {
> +    let groups;
> +    let uri1;

no need to define these here.

@@ +3380,5 @@
> +    let cacheService = Cc["@mozilla.org/network/application-cache-service;1"]
> +                         .getService(Ci.nsIApplicationCacheService);
> +    let uri0 = Services.io.newURI(aManifestURL, null, null);
> +
> +    groups = cacheService.getGroups();

let groups = ...

@@ +3382,5 @@
> +    let uri0 = Services.io.newURI(aManifestURL, null, null);
> +
> +    groups = cacheService.getGroups();
> +    for (let i = 0; i < groups.length; i++) {
> +      uri1 = Services.io.newURI(groups[i], null, null);

let uri = ...

@@ +3383,5 @@
> +
> +    groups = cacheService.getGroups();
> +    for (let i = 0; i < groups.length; i++) {
> +      uri1 = Services.io.newURI(groups[i], null, null);
> +      if (uri0.asciiHost == uri1.asciiHost) {

why asciiHost and not host? Also, you could just keep uri0.host as a local variable instead of uri0.

@@ +3405,5 @@
> +      return;
> +    }
> +
> +    let result = 0;
> +    if (app.basePath == this.getCoreAppsBasePath()){

nit : this.getCoreAppsBasePath()) {

@@ +3423,5 @@
> +      let entry = entries.getNext().QueryInterface(Ci.nsIFile);
> +      if (entry && entry.exists()) {
> +        try {
> +          let file = Cc["@mozilla.org/file/local;1"]
> +                       .createInstance(Ci.nsILocalFile);

s/nsILocalFile/nsIFile

@@ +3431,5 @@
> +        } catch(e) {
> +          debug("file error is " + e + "\n");
> +        }
> +      }
> +    }

We should rather use OS.File for all file access (https://developer.mozilla.org/en-US/docs/JavaScript_OS.File/OS.File_for_the_main_thread)

::: dom/datastore/DataStoreService.js
@@ +55,4 @@
>      }
>  
>      obs.addObserver(this, 'webapps-clear-data', false);
> +    obs.addObserver(this, 'webapps-clear-data-user', false);

Do we ever remove that observer?

::: dom/interfaces/apps/mozIApplicationClearPrivateDataParams.idl
@@ +16,5 @@
>  
>  %{C++
>  #define TOPIC_WEB_APP_CLEAR_DATA "webapps-clear-data"
> +#define TOPIC_WEB_APP_CLEAR_DATA_USER "webapps-clear-data-user"
> +#define TOPIC_WEB_APP_CLEAR_DATA_CACHE "webapps-clear-data-cache"

nit: align all the strings.

::: dom/interfaces/apps/nsIDOMApplicationRegistry.idl
@@ +180,1 @@
>  };

You need to update the uuid of the interface you changed.

::: dom/src/storage/DOMStorageObserver.cpp
@@ +62,4 @@
>    obs->AddObserver(sSelf, "browser:purge-domain-data", true);
>    obs->AddObserver(sSelf, "last-pb-context-exited", true);
>    obs->AddObserver(sSelf, "webapps-clear-data", true);
> +  obs->AddObserver(sSelf, "webapps-clear-data-user", true);

Do we ever remove this observer?

::: netwerk/cache/nsApplicationCacheService.cpp
@@ +243,5 @@
>              = new AppCacheClearDataObserver();
>          observerService->AddObserver(obs, TOPIC_WEB_APP_CLEAR_DATA,
>                                       /*holdsWeak=*/ false);
> +        observerService->AddObserver(obs, TOPIC_WEB_APP_CLEAR_DATA_CACHE,
> +                                     /*holdsWeak=*/ false);

Do we ever remove that observer?

::: netwerk/cookie/nsCookieService.cpp
@@ +647,5 @@
>    nsCOMPtr<AppClearDataObserver> obs = new AppClearDataObserver();
>    observerService->AddObserver(obs, TOPIC_WEB_APP_CLEAR_DATA,
>                                 /* holdsWeak= */ false);
> +  observerService->AddObserver(obs, TOPIC_WEB_APP_CLEAR_DATA_USER,
> +                               /* holdsWeak= */ false);

Do we ever remove that observer?

Comment 47

[[:fabrice] Fabrice Desré]

(In reply to buri.blff from comment #44)
> Created attachment 8342975 [details] [diff] [review]
> v4 patch for gecko without tests code
> 
> Dear Fabrice,
> 
> This patch is a gecko part for master branch. Would you please review the v4
> patch?
> Because I am not good at the gecko tests, before write the tests code, I
> must study how to execute the tests code. So do really need us to write
> these tests code?

Yes, we only land new functionalities with tests. You can use the existing ones in https://mxr.mozilla.org/mozilla-central/source/dom/apps/tests/ as a starting point.

> And, the master branch is different from v1.1 for Offline Cache. When clear
> the offline cache for an app, it will not download the offline cache files
> when we launch the app again (device in network). But I test it in desktop
> nightly, after I clear the offline cache for a website, it can download the
> offline cache files when I refresh the website. Do you know if the offline
> cache mechanism is special for b2g?

No, the offline cache is not special on b2g. When you launch an app, it will fetch the offline cache only if it's declared in the html page (we don't check the manifest appcache_path at this point).

Comment 48

[buri.blff]

(In reply to Fabrice Desré [:fabrice] from comment #46)
> Comment on attachment 8342983 [details] [diff] [review]
> v4 patch for gecko without tests code
> 
> Review of attachment 8342983 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> We're almost there! mostly nits, but the observer removals need to be
> checked.
> 
> ::: dom/apps/src/Webapps.js
> @@ +347,5 @@
> >        { name: "Webapps:Connect:Return:OK", weakRef: true },
> >        { name: "Webapps:Connect:Return:KO", weakRef: true },
> >        { name: "Webapps:FireEvent", weakRef: true },
> > +      { name: "Webapps:GetDataSize:Return", weakRef: true },
> > +      { name: "Webapps:ClearAppData:Return", weakRef: true },
> 
> These were strong refs in the previous patch. Why did that change?
> 

I think you may make a mistake for this. For stop is a strong refs but for GetDataSize and ClearAppData are weak refs in the previous patch.

> @@ +457,5 @@
> >    },
> >  
> > +  getDataSize: function(aType) {
> > +    if (["App", "UserData", "Cache", "All"].indexOf(aType) == -1) {
> > +      return;
> 
> You can't just return undefined. You need to return a request and fire an
> error.
> 

OK, I will change this.

> @@ +3383,5 @@
> > +
> > +    groups = cacheService.getGroups();
> > +    for (let i = 0; i < groups.length; i++) {
> > +      uri1 = Services.io.newURI(groups[i], null, null);
> > +      if (uri0.asciiHost == uri1.asciiHost) {
> 
> why asciiHost and not host?

Here I take a reference like the following:
http://mxr.mozilla.org/mozilla-central/source/browser/components/preferences/advanced.js#476

> @@ +3423,5 @@
> > +      let entry = entries.getNext().QueryInterface(Ci.nsIFile);
> > +      if (entry && entry.exists()) {
> > +        try {
> > +          let file = Cc["@mozilla.org/file/local;1"]
> > +                       .createInstance(Ci.nsILocalFile);
> We should rather use OS.File for all file access

Yes, to get the file size we can use OS.File.stat(somePath).
But it is asynchronous, when we have to sum many files up, it will be slowly and not convenient.

> ::: dom/datastore/DataStoreService.js
> @@ +55,4 @@
> >      }
> >  
> >      obs.addObserver(this, 'webapps-clear-data', false);
> > +    obs.addObserver(this, 'webapps-clear-data-user', false);
> 
> Do we ever remove that observer?
> 

We think there is no need to remove that observer, just like 'webapps-clear-data'.

> ::: dom/interfaces/apps/nsIDOMApplicationRegistry.idl
> @@ +180,1 @@
> >  };
> 
> You need to update the uuid of the interface you changed.

Yes, I will update later.

Thanks!

Comment 49

[buri.blff]

Attachment: v5 patch for gecko without test code

Dear Fabrice,

This patch is a gecko part for master branch. Would you please review the patch?

Could you help me to find why the offline cache files has not been downloaded again after we clear the offline cache? I install the clock app from the marketplace. It has the manifest attribute in the html page and it has the appcache_path too.

Thank you very much!

Comment 50

[buri.blff]

I found the event MozApplicationManifest has not been received in shell.js. So the Offline cache has not been updated. I don't know why it is not been received. Who maybe know?

Comment 51

[[:fabrice] Fabrice Desré]

Once you have cleared the cache, did you run the app again when you checked that the offline cache was populated? We also update the offline cache when updating the apps, so you could do a forced update to trigger that.

But you may also be hitting bug 918880 that we should have a fix for shortly.

Comment 52

[buri.blff]

(In reply to Fabrice Desré [:fabrice] from comment #51)
> Once you have cleared the cache, did you run the app again when you checked
> that the offline cache was populated? 

Yes, I run the app again after cleared the offline cache. But it can not download the offline cache files again.

> We also update the offline cache when
> updating the apps, so you could do a forced update to trigger that.

Yep, this can trigger update check, if user click update in notification, the offline cache files will be downloaded again. But it is not the real Offline cache mechanism, in my opinion.

> But you may also be hitting bug 918880 that we should have a fix for shortly.
Hope it will be fixed soon.

Comment 53

[Fabien Cazenave [:kaze]]

Comment on attachment 8334443 [details] [diff] [review]
v2 patch for gaia

I’m not sure it makes much sense for me to review the Gaia part as long as the Gecko part is not ready… Hence, clearing the review flag for now.

Can you please re-propose a Gaia patch in a github pull request when the Gecko part is ready? We’ll need a PR to get the Travis results anyway.

Comment 54

[[:fabrice] Fabrice Desré]

Comment on attachment 8345677 [details] [diff] [review]
v5 patch for gecko without test code

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

That's pretty good, apart from the file manipulation that really needs to be rewritten. Did you make progress on tests?

::: dom/apps/src/Webapps.jsm
@@ +3345,5 @@
> +  },
> +
> +  // The size of user data is now the combination of localstorage
> +  // and indexedDB. The cookie is not taken into consideration
> +  // in volume calculation. While we would still clear local storage,

nit: s/While/However

@@ +3439,5 @@
> +        } catch(e) {
> +          debug("file error is " + e + "\n");
> +        }
> +      }
> +    }

I'd still like that to use OS.File. With tasks.jsm it's not that bad to loop over entries.

Comment 55

[buri.blff]

Attachment: v6 patch for gecko

Dear Fabrice,

This patch is a gecko part for master branch. Would you please review the patch?

Follow your suggestion in Comment 54, we use OS.File to calculate the app files size.

The tests file are about getDataSize("App") and getDataSize("UserData"), now UserData size is just indexedDB data size, see the comment in code. 

Because we use function close to stop an app, so there is no need to add test code for it. And clearAppData function is also the same as "webapps-clear-data", so it has no test code either.

We are busy in our product work now, sorry for late to answer.

Thank you very much!

Comment 56

[[:fabrice] Fabrice Desré]

Comment on attachment 8357656 [details] [diff] [review]
v6 patch for gecko

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

Thanks for working on the tests!

The issue I have with them is that they only test that the API is returning something, but not the value.
We should get a test that does:
- start an app
- checks that initially the data size is 0.
- adds data in indexedDB.
- check that the amount of data used is correct.
- clears user data.
- checks that the amount of data is then 0.

::: dom/apps/src/Webapps.jsm
@@ +3398,5 @@
> +    // the data stored in webappsstore.sqlite-wal has not change when
> +    // clear data. And the data has not been Synchronized to
> +    // webappsstore.sqlite when local storage has changed.
> +    // So we have not got the local storage when
> +    // calcute the user data size now.

Remove this comment and just add the we can't get localstorage usage until bug 960707 is fixed.

Comment 57

[BMO Automation]

Firefox OS is not being worked on