910466 - Refactor ipc app state updating

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

[[:fabrice] Fabrice Desré]

Currently we use two ipc messages: Webapps:OfflineCache and Webapps:PackageEvent to update the state of DOM objects and keep them synchronized with the backend.

They have very similar roles, but also trigger events in some situations. We should rather have a clear distinction between updating the app state and firing an event on the app object.

So we would still have two messages: Webapps:UpdateAppState and Webapps:FireAppEvent. That would help a lot with improving Webapps.jsm code by making clear how to trigger events in the front-end.

Comment 1

[Fernando Jiménez Moreno [:ferjm]]

Attachment: wip

Comment 2

[Fernando Jiménez Moreno [:ferjm]]

Attachment: WIP

Comment 3

[[:fabrice] Fabrice Desré]

Comment on attachment 801531 [details] [diff] [review]
WIP

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

Nothing major, but a bunch of small fixes. Also, you could rename Webapps:FireAppEvent to Webapps:FireEvent and Webapps:UpdateAppState to Webapps:UpdateState.

Also, I'd like to understand why you removed the "isCanceling" logic.

::: dom/apps/src/Webapps.js
@@ +362,5 @@
>  
>    get updateManifest() {
> +    return this.updateManifest = this._updateManifest ?
> +                                 ObjectWrapper.wrap(this._updateManifest,
> +                                                    this._window) : null;

I'd really like to keep ? and : aligned, but this line keep being too looooonng

@@ +484,5 @@
>      }
>    },
>  
> +  _updateAppState: function(aApp, aProgress, aError, aManifest) {
> +    if (aApp) {

I think we must allow sending partial updates to the app state, so doing something like would work:
for (let prop in aApp) {
  this[prop] = aApp.prop;
}

@@ +498,5 @@
> +    }
> +
> +    // During apps download, we are likely receiving multiple requests for
> +    // updating the app object when only the progress value has changed.
> +    if (aProgress) {

We don't need that anymore if we support partial updates.

@@ +510,5 @@
> +    }
> +
> +    if (aManifest) {
> +      dump("UPDATE MANIFEST*****\n");
> +      this._manifest = aManifest;

If you change the manifest, you need to invalidate the cache.

@@ +579,5 @@
>  
> +        // Update app details before firing any event.
> +        this._updateAppState(msg.app, msg.progress, msg.error, msg.manifest);
> +
> +        if (msg.invalidateCache) {

I don't think we need anything an explicit flag to invalidate the cache, but feel free to convince me otherwise

@@ +599,5 @@
> +              this._fireEvent(eventType, this._ondownloadapplied);
> +              break;
> +            case "downloadavailable":
> +              dump("Firing " + eventType + "\n");
> +              this._fireEvent(eventType, this._ondownloadavailable); 

nit: ws

@@ +605,5 @@
> +            case "downloaderror":
> +              dump("Firing " + eventType + "\n");
> +              this._fireEvent(eventType, this._ondownloaderror);
> +              break;
> +            case "downloadprogress":

rename this event to 'progress'

@@ +620,2 @@
>            }
> +        }).bind(this));

can we rewrite this switch()/case with something like:

if ("_on" + eventType in this) {
  this._fireEvent(eventType); // and let fireEvent find the method to call.
} else {
  debug("Unsupported event type: " + eventType);
}

@@ +629,4 @@
>  
>          // Set app values according to parent process results.
> +        if (msg.app || msg.error || msg.manifest) {
> +          this._updateAppState(msg.app, msg.progress, msg.error, msg.manifest);

You can just pass msg as parameter.

::: dom/apps/src/Webapps.jsm
@@ +1067,5 @@
> +        eventType: "downloaderror",
> +        manifestURL: app.manifestURL,
> +        app: app,
> +        error: error
> +      });

We should have 2 messages here (with Webapps:UpdateAppState first) and no app object in Webapps:FireAppEvent

@@ +1088,5 @@
>      }
>  
>      // If the caller is trying to start a download but we have nothing to
>      // download, send an error.
> +    if (!app.downloadAvailable) { 

nit: trailing ws.

@@ +1092,5 @@
> +    if (!app.downloadAvailable) { 
> +      this.broadcastMessage("Webapps:FireAppEvent", {
> +        eventType: "downloaderror",
> +        manifestURL: app.manifestURL,
> +        app: app,

remove that.

@@ +1137,5 @@
> +              manifestURL: aManifestURL,
> +              app: app,
> +              manifest: jsonManifest,
> +              invalidateCache: true
> +            });

split in 2 messages.

@@ +1181,5 @@
> +            DOMApplicationRegistry.broadcastMessage("Webapps:FireAppEvent",{
> +              eventType: "downloadsuccess",
> +              manifestURL: aManifestURL,
> +              app: app
> +            });

here also.

@@ +1305,5 @@
>      aApp.progress = 0;
>      DOMApplicationRegistry._saveApps((function() {
> +      DOMApplicationRegistry.broadcastMessage("Webapps:UpdateAppState", {
> +        manifestURL: aApp.manifestURL,
> +        app: aApp

It's ok for now, but we should only send the modified properties of the app object.

@@ +1493,5 @@
>        // Update the registry.
>        this.webapps[id] = app;
>        this._saveApps(function() {
>          let reg = DOMApplicationRegistry;
> +        

Nit: ws

@@ +1989,5 @@
> +            manifestURL: appObject.manifestURL,
> +            app: app,
> +            manifest: aManifest,
> +            invalidateCache: true
> +          });

split this one in 2 messages also.

@@ +2422,5 @@
> +                eventType: ["downloadsuccess", "downloadapplied"],
> +                manifestURL: aApp.manifestURL,
> +                app: app,
> +                invalidateCache: true
> +              });

split this one in two messages.

@@ +2423,5 @@
> +                manifestURL: aApp.manifestURL,
> +                app: app,
> +                invalidateCache: true
> +              });
> +              

ws.

@@ +3111,5 @@
> +      DOMApplicationRegistry.broadcastMessage("Webapps:FireAppEvent", {
> +        eventType: ["downloadsuccess", "downloadapplied"],
> +        manifestURL: app.manifestURL,
> +        app: app
> +      });

split this one also.

@@ +3124,5 @@
> +        eventType: "downloaderror",
> +        manifestURL: app.manifestURL,
> +        app: app,
> +        error: error
> +      });

and here too.

Comment 4

[Fernando Jiménez Moreno [:ferjm]]

Attachment: WIP

Comment 5

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v1

This patch should address your previous feedback comments.

Mochitests and chrome tests are green locally and I've just sent a try build.

Comment 6

[Fernando Jiménez Moreno [:ferjm]]

https://tbpl.mozilla.org/?tree=Try&rev=c6429ec88a92

Comment 7

[Marco Castelluccio [:marco]]

There is another listener for those messages in browser/modules/webappsUI.jsm

Comment 8

[Fernando Jiménez Moreno [:ferjm]]

You are right! Thanks Marco!

There is also one missing piece in the devtools code.

Comment 9

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 814408 [details] [diff] [review]
v1

Clearing the r? flag until I fix the comment above

Comment 10

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v1

The try build looks better this way https://tbpl.mozilla.org/?tree=Try&rev=804b9e3387ff

Comment 11

[[:fabrice] Fabrice Desré]

Comment on attachment 815806 [details] [diff] [review]
v1

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

::: dom/apps/src/Webapps.js
@@ +585,5 @@
>          break;
>        case "Webapps:CheckForUpdate:Return:KO":
>          Services.DOMRequest.fireError(req, msg.error);
>          break;
>        case "Webapps:CheckForUpdate:Return:OK":

I think we should kill this one and also use FireEvent and UpdateState instead.

::: dom/apps/src/Webapps.jsm
@@ +42,5 @@
>    return Services.prefs.getBoolPref("dom.sysmsg.enabled");
>  }
>  
>  // Minimum delay between two progress events while downloading, in ms.
> +const MIN_PROGRESS_EVENT_DELAY = 1500;

Why did you change that?

@@ +1133,5 @@
>        mmRef.mm.sendAsyncMessage(aMsgName, aContent);
>      });
>    },
>  
> +  _getAppDir: function _getAppDir(aId) {

no need to name functions anymore. If anything, we should remove all the useless ones.

Comment 13

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Fabrice Desré [:fabrice] from comment #11)
> Comment on attachment 815806 [details] [diff] [review]
> v1
> 
> Review of attachment 815806 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/apps/src/Webapps.js
> @@ +585,5 @@
> >          break;
> >        case "Webapps:CheckForUpdate:Return:KO":
> >          Services.DOMRequest.fireError(req, msg.error);
> >          break;
> >        case "Webapps:CheckForUpdate:Return:OK":
> 
> I think we should kill this one and also use FireEvent and UpdateState
> instead.
> 

New try build getting rid of Webapps:CheckForUpdate:Return:OK

https://tbpl.mozilla.org/?tree=Try&rev=0212da706604

> ::: dom/apps/src/Webapps.jsm
> @@ +42,5 @@
> >    return Services.prefs.getBoolPref("dom.sysmsg.enabled");
> >  }
> >  
> >  // Minimum delay between two progress events while downloading, in ms.
> > +const MIN_PROGRESS_EVENT_DELAY = 1500;
> 
> Why did you change that?
> 

We are sending double the amount of IPC messages than we used to send, so I thought about sending them less often so the performance isn't affected. Once we do what you proposed in https://bugzilla.mozilla.org/show_bug.cgi?id=903291#c32 we should not be worried about this, but until then, we might end up with a bad performance during a download if a big number of apps is installed in the device (we are broadcasting progress events).

> @@ +1133,5 @@
> >        mmRef.mm.sendAsyncMessage(aMsgName, aContent);
> >      });
> >    },
> >  
> > +  _getAppDir: function _getAppDir(aId) {
> 
> no need to name functions anymore. If anything, we should remove all the
> useless ones.

Ok, I'll write a second part for getting rid of the function names.

Comment 14

[[:fabrice] Fabrice Desré]

(In reply to Fernando Jiménez Moreno [:ferjm] (not reading bugmail until 14th October) from comment #13)

> > 
> > no need to name functions anymore. If anything, we should remove all the
> > useless ones.
> 
> Ok, I'll write a second part for getting rid of the function names.

No need to do that in this bug. Just don't add new ones.

Comment 15

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v2

Comment 16

[[:fabrice] Fabrice Desré]

Comment on attachment 817014 [details] [diff] [review]
v2

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

Can you also post a try build?

::: dom/apps/src/Webapps.jsm
@@ -3370,5 @@
>          break;
>        case Ci.nsIOfflineCacheUpdateObserver.STATE_DOWNLOADING:
>        case Ci.nsIOfflineCacheUpdateObserver.STATE_ITEMSTARTED:
> -        setStatus(this.startStatus, aUpdate.byteProgress);
> -        break;

I'm not sure we want to remove that. I think it's useful when there's a long delay before download start and the next progress.

::: toolkit/devtools/server/actors/webapps.js
@@ +179,5 @@
> +        reg.broadcastMessage("Webapps:UpdateState", {
> +          app: aApp,
> +          manifest: manifest,
> +          manifestURL: aApp.manifestURL
> +        });

You also need to fire downloadsuccess and downloadapplied.

Comment 17

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Fabrice Desré [:fabrice] from comment #16)
> Comment on attachment 817014 [details] [diff] [review]
> v2
> 
> Review of attachment 817014 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Can you also post a try build?
> 

That would be the URL in comment 13 :) I should have added again when uploading the patch. Sorry about that.

> ::: dom/apps/src/Webapps.jsm
> @@ -3370,5 @@
> >          break;
> >        case Ci.nsIOfflineCacheUpdateObserver.STATE_DOWNLOADING:
> >        case Ci.nsIOfflineCacheUpdateObserver.STATE_ITEMSTARTED:
> > -        setStatus(this.startStatus, aUpdate.byteProgress);
> > -        break;
> 
> I'm not sure we want to remove that. I think it's useful when there's a long
> delay before download start and the next progress.
>

We get STATE_DOWNLOADING only once when we start downloading the appcache, so I am ok sending a progress event there even if we already send one when we create the AppcacheObserver [1]. But we receive STATE_ITEMSTARTED for each item in the appcache manifest [2], which can produce way too much IPC messages. So I would prefer not to send an event with each STATE_ITEMSTARTED, but every MIN_PROGRESS_EVENT_DELAY instead.

Actually, I would even like to get rid of sending a progress event with each STATE_ITEMPROGRESS or STATE_ITEMSTARTED. If I am not wrong, we don't know the size of the offline cache, so the current UI for appcache downloads is an infinite progress bar or a spinner and I don't think that will change. IMHO it doesn't really make any difference the number of progress events that we send for the appcache download case since the UI is not being updated accordingly with each progress event.

[1] https://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.jsm#3309
[2] https://mxr.mozilla.org/mozilla-central/source/uriloader/prefetch/nsOfflineCacheUpdate.cpp#1934

Comment 18

[Fernando Jiménez Moreno [:ferjm]]

Or maybe we could have a higher MIN_PROGRESS_EVENT_DELAY for downloads with unknown size

Comment 19

[[:fabrice] Fabrice Desré]

We still need progress event for downloads with unknown size since we want to be able to display the amount of data that has been downloaded. And I'm not convinced that it's doing that much IPC either. 2 messages every 1.5 seconds is not a lot.

Comment 20

[Fernando Jiménez Moreno [:ferjm]]

I don't think that we need a progress event for that, but only the amount of downloaded data in the final download* event.
Also, if I am not wrong, it is not only 2 messages every 1.5 sec, but 2 messages broadcasted to every mozIDOMApplication object every 1.5 sec, which could be a big number of receivers (that would not be an issue after we do what you suggests in https://bugzilla.mozilla.org/show_bug.cgi?id=903291#c32 though)

Comment 21

[Fernando Jiménez Moreno [:ferjm]]

Oh, I misread your comment, you are right, we need to display the amount of that *already* downloaded. Got it!

Comment 22

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v3

The changes since the latest patch shouldn't make any difference, but I prefer to stay in the safe side, so another ( and hopefully last :) ) try build

https://tbpl.mozilla.org/?tree=Try&rev=c53485681e1d

I finally moved STATE_ITEMSTARTED so we handle this case sending progress events every 1.5 seconds.

Comment 23

[Fernando Jiménez Moreno [:ferjm]]

Thanks Fabrice!

https://hg.mozilla.org/integration/mozilla-inbound/rev/f7e452886b4f

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f7e452886b4f