1440587 - Move calProviderUtils to the cal.provider namespace

Status: RESOLVED FIXED

(Calendar :: Internal Components, enhancement)

Comment 1

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Move rfc3339 functions - manual changes - v1

Comment 2

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Move rfc3339 functions - automatic changes - v1

Comment 3

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Move provider functions - manual changes - v1

Comment 4

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Move provider functions - automatic changes - v1

Comment 5

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Successful try run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=6bf2037ffe4e067c007a0e4fe0dd6eb94c986c84&selectedJob=164056900


(note that try may look like it doesn't have all bugs, but there was a previous failed try run, so the newest one isn't showing all changesets. See https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=ae663d89c9caf8d2f0917f5c83168d8540100ea1&selectedJob=163954434 for the previous run)

Comment 6

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Move provider functions - manual changes - v2

Made calUtils import lazy to avoid potential cyclic references.

Comment 7

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Move provider functions - manual changes - v3

Comment 8

[[:MakeMyDay]]

Comment on attachment 8957526 [details] [diff] [review]
Move provider functions - manual changes - v3

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

r=me with comments considered (mostly documentation)

::: calendar/base/modules/calProviderUtils.jsm
@@ +29,5 @@
> +     *                                     latter case the string will be converted
> +     *                                     to an input stream.
> +     * @param aContentType               Value for Content-Type header, if any
> +     * @param aNotificationCallbacks     Calendar using channel
> +     * @param aExisting                  An existing channel to modify (optional)

Can you add the return value here?

@@ +78,5 @@
> +        aStreamLoader.init(aListener);
> +        aChannel.asyncOpen(aStreamLoader, aChannel);
> +    },
> +
> +    createStreamLoader: function() {

Please add a doc block here.

@@ +83,5 @@
> +        return Components.classes["@mozilla.org/network/stream-loader;1"]
> +                         .createInstance(Components.interfaces.nsIStreamLoader);
> +    },
> +
> +    convertByteArray: function(aResult, aResultLength, aCharset, aThrow) {

And here.

@@ +114,5 @@
> +     * will be appended to the realm. If you need that feature disabled, see the
> +     * capabilities section of calICalendar.idl
> +     *
> +     * @param aIID      The interface ID to return
> +     */

Please add the return value

@@ +156,5 @@
> +            this.thisProvider = thisProvider;
> +            this.timer = null;
> +        }
> +
> +        notifyCertProblem(socketInfo, status, targetSite) {

Please add a doc block

@@ +223,4 @@
>  
> +    /**
> +     * Gets the iTIP/iMIP transport if the passed calendar has configured email.
> +     */

Please add param and return value to the doc block

@@ +304,2 @@
>  
> +    promptOverwrite: function(aMode, aItem) {

Please add a doc block

@@ +320,4 @@
>  
> +    /**
> +     * Gets the calendar directory, defaults to <profile-dir>/calendar-data
> +     */

Please add the return value

@@ +418,4 @@
>  
> +            // takeover lightning calendar visibility from 0.5:
> +            takeOverIfNotPresent("lightning-main-in-composite", "calendar-main-in-composite");
> +            takeOverIfNotPresent("lightning-main-default", "calendar-main-default");

Do we still need this migration code? I suggest to at least add a comment that it will be subject to removal, if we get rid of it now.

@@ +496,2 @@
>  
> +        notifyPureOperationComplete(aListener, aStatus, aOperationType, aId, aDetail) {

Please add a doc block here

@@ +503,5 @@
>                  }
>              }
>          }
>  
> +        notifyOperationComplete(aListener, aStatus, aOperationType, aId, aDetail, aExtraMessage) {

and here.

@@ +525,5 @@
>              }
>          }
>  
> +        // for convenience also callable with just an exception
> +        notifyError(aErrNo, aMessage) {

Please add a doc block

@@ +539,5 @@
> +            this.setProperty("currentStatus", aErrNo);
> +            this.observers.notify("onError", [this.superCalendar, aErrNo, aMessage]);
> +        }
> +
> +        // nsIVariant getProperty(in AUTF8String aName);

can you format this in doc block style?

@@ +600,4 @@
>                  }
> +                this.mProperties[aName] = ret;
> +            }
> +    //         cal.LOG("getProperty(\"" + aName + "\"): " + ret);

please remove this line.

@@ +603,5 @@
> +    //         cal.LOG("getProperty(\"" + aName + "\"): " + ret);
> +            return ret;
> +        }
> +
> +        // void setProperty(in AUTF8String aName, in nsIVariant aValue);

Please transform this and below occurrences of this pattern to doc block style.

@@ +698,4 @@
>              return false;
>          }
>  
> +        getInvitedAttendee(aItem) {

Please add a doc block

@@ +717,3 @@
>          }
>  
> +        canNotify(aMethod, aItem) {

and finally, here, too.

::: calendar/base/modules/calUtils.jsm
@@ +141,5 @@
>              Components.utils.reportError(string);
>          }
>      },
>  
> +    generateClassQI: function(aGlobal, aIID, aInterfaces) {

Please add a doc block here.

Comment 9

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

(In reply to [:MakeMyDay] from comment #8)

> @@ +156,5 @@
> > +            this.thisProvider = thisProvider;
> > +            this.timer = null;
> > +        }
> > +
> > +        notifyCertProblem(socketInfo, status, targetSite) {
> 
> Please add a doc block
This is a method from nsIBadCertHandler, I'd rather avoid duplicating documentation from interface methods.


> @@ +418,4 @@
> >  
> > +            // takeover lightning calendar visibility from 0.5:
> > +            takeOverIfNotPresent("lightning-main-in-composite", "calendar-main-in-composite");
> > +            takeOverIfNotPresent("lightning-main-default", "calendar-main-default");
> 
> Do we still need this migration code? I suggest to at least add a comment
> that it will be subject to removal, if we get rid of it now.
I think it is safe to remove, doing that.


> @@ +539,5 @@
> > +            this.setProperty("currentStatus", aErrNo);
> > +            this.observers.notify("onError", [this.superCalendar, aErrNo, aMessage]);
> > +        }
> > +
> > +        // nsIVariant getProperty(in AUTF8String aName);
> 
> can you format this in doc block style?
These are all methods from the interface again, and as mentioned I'd like to avoid duplicate documentation. I've documented all the methods that are not from the interface though and made sure the interface methods have the signature comment. the eslint rules are happy for now.

Comment 10

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Move provider functions - manual changes - v4

Comment 11

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 8967717 [details] [diff] [review]
Move provider functions - manual changes - v4

The automatic changes also have approval and need to be pushed, skipping the flag just like r+.

Comment 12

[Jorg K (CEST = GMT+2)]

No indication as to the landing order here, also, the beta approvals seem inconsistent. May you want to do the landing yourself and I can uplift in landing order later.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/bd7d95197814
Move calProviderUtils to the cal.provider namespace - rfc3339 manual changes. r=MakeMyDay
https://hg.mozilla.org/comm-central/rev/0f768b93b1a7
Move calProviderUtils to the cal.provider namespace - rfc3339 automatic changes. r=MakeMyDay
https://hg.mozilla.org/comm-central/rev/e48e853cdc1a
Move calProviderUtils to the cal.provider namespace - manual provider changes. r=MakeMyDay
https://hg.mozilla.org/comm-central/rev/ba90a7aa8c22
Move calProviderUtils to the cal.provider namespace - automatic provider changes. r=MakeMyDay

Comment 14

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

https://hg.mozilla.org/releases/comm-beta/533050294814
Bug 1440587 - Move calProviderUtils to the cal.provider namespace - rfc3339 manual changes. r=MakeMyDay
https://hg.mozilla.org/releases/comm-beta/54bc3fdbf248
Bug 1440587 - Move calProviderUtils to the cal.provider namespace - rfc3339 automatic changes. r=MakeMyDay
https://hg.mozilla.org/releases/comm-beta/96aefde64d3b
Bug 1440587 - Move calProviderUtils to the cal.provider namespace - manual provider changes. r=MakeMyDay
https://hg.mozilla.org/releases/comm-beta/7f803e181256
Bug 1440587 - Move calProviderUtils to the cal.provider namespace - automatic provider changes. r=MakeMyDay

Comment 15

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/releases/comm-beta/rev/533050294814
Bug 1440587 - Move calProviderUtils to the cal.provider namespace - rfc3339 manual changes. r=MakeMyDay
https://hg.mozilla.org/releases/comm-beta/rev/54bc3fdbf248
Bug 1440587 - Move calProviderUtils to the cal.provider namespace - rfc3339 automatic changes. r=MakeMyDay
https://hg.mozilla.org/releases/comm-beta/rev/96aefde64d3b
Bug 1440587 - Move calProviderUtils to the cal.provider namespace - manual provider changes. r=MakeMyDay
https://hg.mozilla.org/releases/comm-beta/rev/7f803e181256
Bug 1440587 - Move calProviderUtils to the cal.provider namespace - automatic provider changes. r=MakeMyDay

Comment 16

[Stefan Sitter [:ssitter]]

Maybe the message that is logged in Lightning 6.2b4 should be reworked? Instead of X use X?
DEPRECATION WARNING: calProviderUtils' cal.provider.BaseClass() has changed to cal.provider.BaseClass()

Comment 17

[Martin Schröder [:mschroeder]]

Attachment: Patch v1 - Follow up: Corrected wording in deprecation messages

Addresses comment#16.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/aaebcb45064c
Follow-up: Corrected wording in deprecation messages. r=philipp

Comment 19

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/releases/comm-beta/rev/b8485e3925a068ee34b3fbae019f5c8ae14a94ef
Follow-up: Corrected wording in deprecation messages. r=philipp a=philipp

Comment 20

[[e-mail]]

Comment on attachment 8967717 [details] [diff] [review]
Move provider functions - manual changes - v4

>+    BadCertHandler: class {
>+        QueryInterface(iid) {
>+            return cal.generateClassQI(this, iid, [Components.interfaces.nsIBadCertListener2]);
>+        }

>+    FreeBusyInterval: class {
>+        QueryInterface(iid) {
>+            return cal.generateClassQI(this, iid, [Components.interfaces.calIFreeBusyInterval]);
>         }

These don't actually work; `generateClassQI` (since superseded by `generateQI`, but the same issue applies) returns a function which needs to be called, not returned. There are two approaches; the first is to change this into a getter, the second is to set `.prototype.QueryInterface` manually.

Comment 21

[Jorg K (CEST = GMT+2)]

Neil, no one will listen here, please file a new bug.

Comment 22

[[e-mail]]

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

Neil, no one will listen here, please file a new bug.

Don't worry, I was wrong anyway; cal.generateClassQI is subtly different from XPCOMUtils.generateClassQI.