1530938 - calendar/resources/content/publish.js relies on asyncOpen taking context as second argument which is defunct since bug 1520868

Status: RESOLVED FIXED

(Calendar :: General, enhancement)

Description

[Magnus Melin [:mkmelin]]

publish.js needs to be updated to not rely in passing a context as second argument to asyncOpen. That version no longer exists since bug 1520868.

https://searchfox.org/comm-central/source/calendar/resources/content/publish.js#175

175: channel.asyncOpen(publishingListener, aProgressDialog);

Comment 1

[Magnus Melin [:mkmelin]]

Paul, can you give this a shot?

I looked briefly, but this code is fairly messed up, and could do with some serious reworking IMHO. The interaction between this and publishDialog.{js,xul} is quite odd passing globals around it seems. Don't know why the linter doesn't complain about the undeclared |self| either (the declaration of which must be coming from some other file - ouch!)

Comment 2

[Magnus Melin [:mkmelin]]

The minimal fix would need to figure out how the error handling will get to the progressdialog object, since that object can no longer be receive via asyncOpen. I.e. change to channel.asyncOpen(publishingListener), and find out how to get the progress dialog in this new scenario.

Comment 3

[Paul Morris [:pmorris]]

Hey Magnus, Yeah, I'll give it a go, thanks for the explanations. Took a quick look and the 'self' here is likely window.self.

Comment 4

[Magnus Melin [:mkmelin]]

Ah - of course.

Comment 5

[Paul Morris [:pmorris]]

Attachment: publish-async-open-0.patch

There may be more to do here, but this solves the asyncOpen issue. Now that there's no context argument, we can provide access to the dialog window by binding it to the callback in the listener object (that gets passed to asyncOpen).

I tested this by publishing a calendar using a bogus URL, and when the request failed, I saw the progress bar stopped indicating progress (as expected). I also used a temporary console.log to see that "onStopUpload" was indeed fired.

Comment 6

[Paul Morris [:pmorris]]

Hmm, I see there are 11 uses of asyncOpen in the calendar code, all passing a second context argument. So maybe a more general solution is in order?

Comment 7

[Magnus Melin [:mkmelin]]

I think searchfox hasn't had the time to reindex yet. Bug 1528681 removed a lot.

Comment 8

[Magnus Melin [:mkmelin]]

Comment on attachment 9047085 [details] [diff] [review]
publish-async-open-0.patch

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

::: calendar/resources/content/publish.js
@@ +199,5 @@
> + * @param aRequest           Request being observed
> + * @param aContext           User defined context (apparently no longer used, see bug 1520868)
> + * @param aStatusCode        Reason for stopping (NS_OK if completed successfully)
> + */
> +function onStopRequestWithDialog(aProgressDialog, aRequest, aContext, aStatusCode) {

as this is used only once, I'd move the implementation there

@@ +229,5 @@
> + * to provide access to the dialog when the request is done.
> + * @param aProgressDialog   Dialog window
> + * @returns {Object}        Listener
> + */
> +function makePublishingListener(aProgressDialog) {

maybe clearer to have this be a constructor for an object, instead of some kind of factory?

@@ +238,5 @@
> +        },
> +
> +        onStopRequest: onStopRequestWithDialog.bind(null, aProgressDialog),
> +
> +        onDataAvailable: function(aRequest, aContext, inStream, sourceOffset, count) {

please don't use the aStyle, and do not mix with that style

Comment 9

[Paul Morris [:pmorris]]

Attachment: publish-async-open-1.patch

as this is used only once, I'd move the implementation there

Okay, I've moved it inline, and switched to capturing the dialog argument via a closure rather than with "bind". (Bind is easier to understand with a separate/named function, and I generally like using bind rather than closures because that's more explicit. But inline with a closure is not bad here.)

maybe clearer to have this be a constructor for an object, instead of some kind of factory?

Okay, I've changed this to a constructor function that uses 'new'. (I tend to prefer factory functions which are more explicit and straightforward to understand than the implicitness of JS's 'new' operator.)

please don't use the aStyle, and do not mix with that style

Ah, yes, good call on not mixing. I've dropped the aStyle throughout this patch. Thanks for additional info on IRC.

Comment 10

[Magnus Melin [:mkmelin]]

Comment on attachment 9049295 [details] [diff] [review]
publish-async-open-1.patch

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

The patch doesn't apply at all anymore for some reason.

I was thinking it could be something like this (untested)

/**
 * @implements {nsIStreamListener}
 */
class PublishingListener {
  constructor(progressDialog) {
    this.progressDialog = progressDialog;
  }

  onStartRequest(request, context) { }
  onStopRequest(request, context, statusCode) {
    this.progressDialog.wrappedJSObject.onStopUpload();

    let channel;
    let requestSucceeded;
    try {
        channel = request.QueryInterface(Ci.nsIHttpChannel);
        requestSucceeded = channel.requestSucceeded;
    } catch (e) {
        // Don't fail if it is not an http channel, will be handled below
    }

    if (channel && !requestSucceeded) {
        let body = cal.l10n.getCalString("httpPutError", [
            channel.responseStatus, channel.responseStatusText
        ]);
        Services.prompt.alert(null, cal.l10n.getCalString("genericErrorTitle"), body);
    } else if (!channel && !Components.isSuccessCode(request.status)) {
        // XXX this should be made human-readable.
        let body = cal.l10n.getCalString("otherPutError", [request.status.toString(16)]);
        Services.prompt.alert(null, cal.l10n.getCalString("genericErrorTitle"), body);
    }
  }
  onDataAvailable(request, context, inStream, sourceOffset, count) { }

  QueryInterface: ChromeUtils.generateQI([Ci.nsIStreamListener])
}

Comment 11

[Paul Morris [:pmorris]]

Attachment: publish-async-open-2.patch

Ah, I see, thanks. Yeah, that works. (I'm still getting used to this JS class syntax business.)

Redone with class syntax and rebased. (Merge conflict was due to changes in bug 1531307.) Still works when I tested it: "onStopUnload" is called and the progress indicator stops indicating progress.

Comment 12

[Magnus Melin [:mkmelin]]

Comment on attachment 9049543 [details] [diff] [review]
publish-async-open-2.patch

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

::: calendar/resources/content/publish.js
@@ +197,5 @@
> + */
> +class PublishingListener {
> +    constructor(progressDialog) {
> +        this.progressDialog = progressDialog;
> +        this.QueryInterface = ChromeUtils.generateQI([Ci.nsIStreamListener]);

why not this one as a method?

Comment 13

[Philipp Kewisch [:Fallen][☁️📆]]

(In reply to Magnus Melin [:mkmelin] from comment #12)

Comment on attachment 9049543 [details] [diff] [review]
publish-async-open-2.patch

Review of attachment 9049543 [details] [diff] [review]:

::: calendar/resources/content/publish.js
@@ +197,5 @@

• */
  +class PublishingListener {
• constructor(progressDialog) {

•    this.progressDialog = progressDialog;
  

•    this.QueryInterface = ChromeUtils.generateQI([Ci.nsIStreamListener]);
  

why not this one as a method?

You can't assign prototype methods from the result of a function call in es6 classes, and you don't want to run generateQI on each get. I created this helper: https://searchfox.org/comm-central/source/calendar/base/modules/calUtils.jsm#152 but assigning it in the constructor might actually be less bulky than I thought.

The best thing you can do is this:

class foo {

}
foo.prototype.QueryInterface = ChromeUtils.generateQI([Ci.nsIStreamListener]);

but then you put the QI after the class, which isn't great from a readability standpoint.

Comment 14

[Paul Morris [:pmorris]]

(In reply to Magnus Melin [:mkmelin] from comment #12)

Comment on attachment 9049543 [details] [diff] [review]
publish-async-open-2.patch

Review of attachment 9049543 [details] [diff] [review]:

::: calendar/resources/content/publish.js
@@ +197,5 @@

• */
  +class PublishingListener {
• constructor(progressDialog) {

•    this.progressDialog = progressDialog;
  

•    this.QueryInterface = ChromeUtils.generateQI([Ci.nsIStreamListener]);
  

why not this one as a method?

I didn't want to call .generateQI on each access/use of QueryInterface as making it a method would need to do. Better to call it once (per instantiation) in the constructor.

As Philipp mentions, better to put it on the prototype, so it's only ever calculated/fetched once, but you can't do that using ES6 class syntax. (The class syntax sugar is a bit of a "leaky abstraction" here...) You have to do this after the class declaration:

foo.prototype.QueryInterface = ChromeUtils.generateQI([Ci.nsIStreamListener]);

I could see doing that, and adding a comment in the class to alert readers. But since we have a helper function for this (that Philipp references), I think I'll use that.

Comment 15

[Paul Morris [:pmorris]]

Attachment: publish-async-open-3.patch

This patch uses the helper function to generate the QueryInterface method. Still works as before when tested: "onStopUnload" is called and the progress indicator stops indicating progress.

One benefit of the helper function is that it is lazy, deferring the call to generateQI until it's needed. Although, that probably doesn't matter in this case (where assigning it in the constructor wouldn't be that much different since the object is a "use once and throw away" object), we might as well use the helper for consistency's sake.

Comment 16

[Paul Morris [:pmorris]]

Attachment: publish-async-open-4.patch

The previous patch accidentally still had "context" parameters for onStopRequest and friends. The context parameters had been removed from that interface, and are no longer there in this patch.

Comment 17

[Paul Morris [:pmorris]]

Attachment: publish-async-open-5.patch

sigh The previous patch unintentionally removed the 3rd (unused) argument "errorMsg" from "onStopRequest". Now it's back, and this is really ready for review this time. (Need to check the diffs more thoroughly before uploading patches.)

Comment 18

[Magnus Melin [:mkmelin]]

Comment on attachment 9050074 [details] [diff] [review]
publish-async-open-5.patch

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

Sweet, r=mkmelin

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/02fa6334f4ed
asyncOpen() no longer takes 'context' as second argument, pass it as property on the listener object. r=mkmelin

Comment 20

[Jorg K (CEST = GMT+2)]

First bug, haha, at least under the new circumstances ;-)

Please provide a meaningful commit message, copying the bug summary is mostly not a good idea.

Comment 21

[Jorg K (CEST = GMT+2)]

Without having read the whole bug, after M-C discontinued the context parameter and before this patch, what wasn't working? IOW, does it need the parameter or could it just have been dropped? There is no test for this functionality?

In hindsight, maybe a Calendar peer should have reviewed this?

Comment 22

[Magnus Melin [:mkmelin]]

(In reply to Paul Morris [:pmorris] from comment #17)

sigh The previous patch unintentionally removed the 3rd (unused) argument
"errorMsg" from "onStopRequest". Now it's back, and this is really ready
for review this time.

Heh, that errorMsg arg should actually have been removed now following bug 1525319

-onStopRequest(request, status, errorMsg) {
+onStopRequest(request, status) {

Comment 23

[Magnus Melin [:mkmelin]]

(In reply to Jorg K (GMT+1) from comment #21)

Without having read the whole bug, after M-C discontinued the context
parameter and before this patch, what wasn't working?

Originally when this bug was filed, you would not have got an error message for publishing failure.
It seems that part was actually "fixed" through https://hg.mozilla.org/comm-central/rev/0022c92d1421
Or well, that fix is wrong and would not work here.

In hindsight, maybe a Calendar peer should have reviewed this?

Theoretically yes, but I think they have their hands full with other changes than adapting to breakage.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/970bdd9dd9bd
Follow-up: remove 'error' parameter from onStopRequest(). r=mkmelin

Comment 25

[Jorg K (CEST = GMT+2)]

Thanks, Magnus. Looking at the changes closer, yes, we need the dialog in onStopRequest(), so I agree with passing it this way now that the context parameter has disappeared. I assume there is no test for the publishing functionality.

Comment 26

[Paul Morris [:pmorris]]

Thanks Jorg, for the push and the follow-up! Good to land my first one under new circumstances. :) Will write a better commit message next time. I just did a search and did not find a test for publishing functionality.