Closed Bug 1469459 Opened 6 years ago Closed 6 years ago

ESLint fixes in calendar/

Categories

(Calendar :: General, enhancement)

Product:
Calendar
Component:
General
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: darktrojan, Assigned: darktrojan)

References

Details

Attachments

(3 files)

Bug 1469459 - ESLint fixes in calendar/
59 bytes, text/x-review-board-request
Fallen
: review+
Fallen
: approval-calendar-beta+
Details
Bug 1469459 - ESLint fixes in calendar/
59 bytes, text/x-review-board-request
Fallen
: review+
Fallen
: approval-calendar-beta+
Details
1469459-eslint3.diff
2.16 KB, patch
Fallen
: review+
Fallen
: approval-calendar-beta+
Details | Diff | Splinter Review 
There's dozens of warnings about generateQI, plus I discovered we're ignoring two files because of preprocessing, that we don't preprocess any more.
Attachment #8986080 - Flags: review?(philipp)
Geoff, please rebase this after I finally landed bug 1457087. There are also follow-up bugs: bug 1469499 and bug 1469501.
Comment on attachment 8986081 [details]
Bug 1469459 - ESLint fixes in calendar/

https://reviewboard.mozilla.org/r/251534/#review258470

::: calendar/base/content/agenda-listbox.js:598
(Diff revision 2)
>   * @param aCalendar     (optional) If specified, the single calendar from
>   *                                   which the refresh will occur.
>   */
>  agendaListbox.refreshCalendarQuery = function(aStart, aEnd, aCalendar) {
>      let refreshJob = {
> -        QueryInterface: XPCOMUtils.generateQI([Components.interfaces.calIOperationListener]),
> +        QueryInterface: ChromeUtils.generateQI([Components.interfaces.calIOperationListener]),

Might as well switch to Ci here, but you could also have that done in bug 1458367.

::: calendar/base/content/agenda-listbox.js:871
(Diff revision 2)
>   * @see calIObserver
>   * @see calICompositeObserver
>   */
>  agendaListbox.calendarObserver = { agendaListbox: agendaListbox };
>  
> -agendaListbox.calendarObserver.QueryInterface = function(aIID) {
> +agendaListbox.calendarObserver.QueryInterface = ChromeUtils.generateQI([

If there is more than one interface, you need to use cal.generateQI, otherwise agendaListbox.calendarObserver will not automatically QI between calIObserver and calICompositeObserver.

Also, you can remove nsISupports here.
Attachment #8986081 - Flags: review?(philipp) → review+
Comment on attachment 8986080 [details]
Bug 1469459 - ESLint fixes in calendar/

https://reviewboard.mozilla.org/r/251532/#review258474

I was pretty sure I fixed most of these in what was pushed as https://hg.mozilla.org/comm-central/rev/02830077b19b
As Jörg mentioned, makes sense to rebase :) r- for now, please upload again once you've rebased.
Attachment #8986080 - Flags: review?(philipp) → review-
That's what I thought too, but there's still dozens of them, and the patch applies cleanly.
(In reply to Philipp Kewisch [:Fallen]  from comment #6)
> If there is more than one interface, you need to use cal.generateQI,
> otherwise agendaListbox.calendarObserver will not automatically QI between
> calIObserver and calICompositeObserver.

Does this apply everywhere? Why is it a thing?
Comment on attachment 8986080 [details]
Bug 1469459 - ESLint fixes in calendar/

(In reply to Geoff Lankow (:darktrojan) from comment #9)
> (In reply to Philipp Kewisch [:Fallen]  from comment #6)
> > If there is more than one interface, you need to use cal.generateQI,
> > otherwise agendaListbox.calendarObserver will not automatically QI between
> > calIObserver and calICompositeObserver.
> 
> Does this apply everywhere? Why is it a thing?

Yes, needs to happen everywhere you are using more than one interface. The alternative is to make sure that all callers QI to the right interface before they make use of methods, and/or know if they got a wrapped instance QI'd to the right interface. ChromeUtils.generateQI doesn't fill in the class info, which is necessary to make objects that listen to all interfaces. I asked Kris about this in the bug and he was not interested in providing that. Essentially, things that are javascript only should not be using XPCOM, and things that are C++ will not be using ChromeUtils.

I'll take a look at the patch again soon, but maybe in the meanwhile you can adjust the cal.generateQI thing.
Attachment #8986080 - Flags: review- → review?(philipp)
If with "everywhere" you mean Thunderbird, it seems Thunderbird never relied on the classInfo, so probably only in calendar.
Okay, I'd better go through the patch and repost it anyway.
Blocks: 1458367
Product: Thunderbird → Calendar
Please add this to the list :-( - Maybe in another bug:
TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/comm/mail/components/extensions/child/ext-mail.js:7:1 | 'extensions' is not defined. (no-undef)
TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/comm/mail/components/extensions/child/ext-tabs.js:7:27 | 'ExtensionAPI' is not defined. (no-undef)
TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/comm/mail/components/extensions/child/ext-tabs.js:11:9 | Expected method shorthand. (object-shorthand)
TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/comm/mail/components/extensions/child/ext-tabs.js:26:9 | Expected method shorthand. (object-shorthand)
TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/comm/mail/components/extensions/child/ext-tabs.js:29:13 | Expected property shorthand. (object-shorthand)
TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/comm/mail/components/extensions/parent/ext-mail.js:11:5 | 'ExtensionUtils' is not defined. (no-undef)
TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/comm/mail/components/extensions/parent/ext-mail.js:26:14 | 'ExtensionUtils' is not defined. (no-undef)
TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/comm/mail/components/extensions/parent/ext-mail.js:27:14 | 'ExtensionUtils' is not defined. (no-undef)
TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/comm/mail/components/extensions/parent/ext-mail.js:40:1 | 'global' is not defined. (no-undef)
TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/comm/mail/components/extensions/parent/ext-mail.js:42:1 | 'global' is not defined. (no-undef)
TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/comm/mail/components/extensions/parent/ext-mail.js:70:1 | 'global' is not defined. (no-undef)
TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/comm/mail/components/extensions/parent/ext-mail.js:76:29 | 'WindowTrackerBase' is not defined. (no-undef)
TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/comm/mail/components/extensions/parent/ext-mail.js:119:12 | 'Services' is not defined. (no-undef)
TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/comm/mail/components/extensions/parent/ext-mail.js:131:9 | 'AppConstants' is not defined. (no-undef)
TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/comm/mail/components/extensions/parent/ext-mail.js:132:24 | 'Services' is not defined. (no-undef)
TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/comm/mail/components/extensions/parent/ext-mail.js:147:13 | 'Services' is not defined. (no-undef)
TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/comm/mail/components/extensions/parent/ext-mail.js:152:26 | 'Services' is not defined. (no-undef)
TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/comm/mail/components/extensions/parent/ext-mail.js:182:1 | 'global' is not defined. (no-undef)
TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/comm/mail/components/extensions/parent/ext-mail.js:182:43 | 'EventManager' is not defined. (no-undef)
TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/comm/mail/components/extensions/parent/ext-mail.js:199:26 | 'TabTrackerBase' is not defined. (no-undef)
TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/comm/mail/components/extensions/parent/ext-mail.js:513:15 | 'global' is not defined. (no-undef)
TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/comm/mail/components/extensions/parent/ext-mail.js:518:19 | 'TabBase' is not defined. (no-undef)
TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/comm/mail/components/extensions/parent/ext-mail.js:646:22 | 'WindowBase' is not defined. (no-undef)
TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/comm/mail/components/extensions/parent/ext-mail.js:851:14 | 'tabManager' is not defined. (no-undef)
TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/comm/mail/components/extensions/parent/ext-mail.js:857:15 | 'global' is not defined. (no-undef)
Can I have the remaining review on this soon please? Other errors are starting to slip in because I'm ignoring all of them.
Comment on attachment 8986080 [details]
Bug 1469459 - ESLint fixes in calendar/

https://reviewboard.mozilla.org/r/251532/#review260972

Thanks for the update, and sorry for the delay!
Attachment #8986080 - Flags: review?(philipp) → review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/1ee3d20fcf06
ESLint fixes in calendar/. r=philipp
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Landed merged patch (since both had the same commit message). I guess this wants to go to beta as well, right? Please apply.
Target Milestone: --- → 6.5
https://hg.mozilla.org/comm-central/rev/1ee3d20fcf06d9f4101ad2271030a6e9652b7576#l42.13
+        QueryInterface: ChromeUtils.generateQI([
+            Ci.calICalendarSearchProvider,
+            Ci.calIOperation
+        ]),
wasn't that meant to be cal.generateQI() if it's more than one?
Attachment #8986080 - Flags: approval-calendar-beta?(philipp)
Attachment #8986081 - Flags: approval-calendar-beta?(philipp)
Rubber stamp, please!
Attachment #8989143 - Flags: review?(philipp)
Comment on attachment 8989143 [details] [diff] [review]
1469459-eslint3.diff

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

Stamps and approvals as you need them :) Not actually sure how far back this needs to go off hand.
Attachment #8989143 - Flags: review?(philipp)
Attachment #8989143 - Flags: review+
Attachment #8989143 - Flags: approval-calendar-esr+
Attachment #8989143 - Flags: approval-calendar-beta+
Attachment #8986081 - Flags: approval-calendar-beta?(philipp) → approval-calendar-beta+
Attachment #8986080 - Flags: approval-calendar-beta?(philipp) → approval-calendar-beta+
Comment on attachment 8989143 [details] [diff] [review]
1469459-eslint3.diff

Please remove the ESR flag again. This most certainly won't go onto the 60/6.2 ESR.

And, as requested before, grant me permission to set/reset those flags for administrative purposes :-(
Flags: needinfo?(philipp)
Flags: needinfo?(philipp)
Attachment #8989143 - Flags: approval-calendar-esr+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/a025a148f193
Follow-up - more eslint fixes in calendar/. r=philipp
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: