Closed Bug 1440587 Opened 2 years ago Closed 2 years ago

Move calProviderUtils to the cal.provider namespace

Categories

(Calendar :: Internal Components, enhancement)

Lightning 6.2
enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: Fallen, Assigned: Fallen)

References

Details

Attachments

(5 files, 3 obsolete files)

No description provided.
Attachment #8953381 - Flags: review?(makemyday)
Attachment #8953379 - Flags: review?(makemyday)
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)
Made calUtils import lazy to avoid potential cyclic references.
Attachment #8953381 - Attachment is obsolete: true
Attachment #8953381 - Flags: review?(makemyday)
Attachment #8953760 - Flags: review?(makemyday)
Attachment #8953760 - Attachment is obsolete: true
Attachment #8953760 - Flags: review?(makemyday)
Attachment #8957526 - Flags: review?(makemyday)
Attachment #8953379 - Flags: review?(makemyday) → review+
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.
Attachment #8957526 - Flags: review?(makemyday) → review+
(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.
Attachment #8957526 - Attachment is obsolete: true
Attachment #8967717 - Flags: review+
Attachment #8953379 - Flags: approval-calendar-beta+
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+.
Attachment #8967717 - Flags: approval-calendar-beta+
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.
Pushed by mozilla@kewis.ch:
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
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
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
Target Milestone: --- → 6.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
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()
Addresses comment#16.
Attachment #8971019 - Flags: review?(philipp)
Attachment #8971019 - Flags: approval-calendar-beta?(philipp)
Attachment #8971019 - Flags: review?(philipp)
Attachment #8971019 - Flags: review+
Attachment #8971019 - Flags: approval-calendar-beta?(philipp)
Attachment #8971019 - Flags: approval-calendar-beta+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/aaebcb45064c
Follow-up: Corrected wording in deprecation messages. r=philipp
Keywords: checkin-needed
https://hg.mozilla.org/releases/comm-beta/rev/b8485e3925a068ee34b3fbae019f5c8ae14a94ef
Follow-up: Corrected wording in deprecation messages. r=philipp a=philipp
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.

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

(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.

You need to log in before you can comment on or make changes to this bug.