Closed Bug 910466 Opened 6 years ago Closed 6 years ago

Refactor ipc app state updating

Categories

(Core Graveyard :: DOM: Apps, defect)

x86_64
Linux
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla27

People

(Reporter: fabrice, Assigned: ferjm)

References

Details

Attachments

(1 file, 6 obsolete files)

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.
Blocks: 910469
Assignee: nobody → ferjmoreno
Attached patch wip (obsolete) — Splinter Review
Attached patch WIP (obsolete) — Splinter Review
Attachment #800262 - Attachment is obsolete: true
Attachment #801531 - Flags: feedback?(fabrice)
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.
Attachment #801531 - Flags: feedback?(fabrice)
Attached patch WIP (obsolete) — Splinter Review
Attachment #801531 - Attachment is obsolete: true
Attached patch v1 (obsolete) — Splinter Review
This patch should address your previous feedback comments.

Mochitests and chrome tests are green locally and I've just sent a try build.
Attachment #813480 - Attachment is obsolete: true
Attachment #814408 - Flags: review?(fabrice)
There is another listener for those messages in browser/modules/webappsUI.jsm
You are right! Thanks Marco!

There is also one missing piece in the devtools code.
Comment on attachment 814408 [details] [diff] [review]
v1

Clearing the r? flag until I fix the comment above
Attachment #814408 - Flags: review?(fabrice)
Attached patch v1 (obsolete) — Splinter Review
The try build looks better this way https://tbpl.mozilla.org/?tree=Try&rev=804b9e3387ff
Attachment #814408 - Attachment is obsolete: true
Attachment #815806 - Flags: review?(fabrice)
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.
Attachment #815806 - Flags: review?(fabrice) → feedback+
Duplicate of this bug: 924796
(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.
(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.
Attached patch v2 (obsolete) — Splinter Review
Attachment #815806 - Attachment is obsolete: true
Attachment #817014 - Flags: review?(fabrice)
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.
Attachment #817014 - Flags: review?(fabrice) → feedback+
(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
Or maybe we could have a higher MIN_PROGRESS_EVENT_DELAY for downloads with unknown size
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.
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)
Oh, I misread your comment, you are right, we need to display the amount of that *already* downloaded. Got it!
Attached patch v3Splinter Review
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.
Attachment #817014 - Attachment is obsolete: true
Attachment #817928 - Flags: review?(fabrice)
Attachment #817928 - Flags: review?(fabrice) → review+
https://hg.mozilla.org/mozilla-central/rev/f7e452886b4f
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Depends on: 1005048
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.