Closed Bug 1557504 Opened 5 years ago Closed 5 years ago

remove [array] use in xpidl from calendar/

Categories

(Calendar :: General, task, P2)

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 73.0

People

(Reporter: mkmelin, Assigned: khushil324)

References

Details

Attachments

(12 files, 20 obsolete files)

(deleted), patch
pmorris
: review+
mkmelin
: feedback+
Details | Diff | Splinter Review
(deleted), patch
pmorris
: review+
Details | Diff | Splinter Review
(deleted), patch
khushil324
: review+
Details | Diff | Splinter Review
(deleted), patch
khushil324
: review+
Details | Diff | Splinter Review
(deleted), patch
khushil324
: review+
Details | Diff | Splinter Review
(deleted), patch
khushil324
: review+
Details | Diff | Splinter Review
(deleted), patch
khushil324
: review+
Details | Diff | Splinter Review
(deleted), patch
pmorris
: review+
Details | Diff | Splinter Review
(deleted), patch
pmorris
: review+
Details | Diff | Splinter Review
(deleted), patch
khushil324
: review+
Details | Diff | Splinter Review
(deleted), patch
khushil324
: review+
Details | Diff | Splinter Review
(deleted), patch
pmorris
: review+
Details | Diff | Splinter Review
Summary: remove [array] use from → remove [array] use from calendar/
Summary: remove [array] use from calendar/ → remove [array] use in xpidl from calendar/

Rodger that

Probably best to do one patch per interface.

Attached file bug-1557504-callImportExport.idl (obsolete) (deleted) —

one sec I'm a goof

Attachment #9079734 - Flags: feedback?(mkmelin+mozilla)
Attached patch bug-1557504-callImportExport.patch (obsolete) (deleted) — Splinter Review

Renamed the patch

Attachment #9079734 - Attachment is obsolete: true
Attachment #9079734 - Flags: feedback?(mkmelin+mozilla)
Attached patch bug-1557504-calIItipItem.patch (obsolete) (deleted) — Splinter Review

Second patch, for calIItipItem.idl

Attached patch bug-1557504-calCalendarACLManager.patch (obsolete) (deleted) — Splinter Review
Attached patch bug-1557504-calCalendarSearchProvider.patch (obsolete) (deleted) — Splinter Review
Attached patch bug-1557504-calItemBase.patch (obsolete) (deleted) — Splinter Review
Attached patch bug-1557504-calItemBase_js.patch (obsolete) (deleted) — Splinter Review
Attached patch bug-1557504-callPrintFormatter.patch (obsolete) (deleted) — Splinter Review
Attached patch bug-1557504-calIcsSerializer.patch (obsolete) (deleted) — Splinter Review
Attached patch bug-1557504-calCalendarViewController.patch (obsolete) (deleted) — Splinter Review

I think that's all of them.

Status: NEW → ASSIGNED

Benjamin, you should ask for review. Like it is now, nothing will happen.

You should not just change the idl, you also need to go through all the callers of the functions you change.

Attachment #9080035 - Attachment is obsolete: true
Attachment #9080036 - Attachment is obsolete: true
Attachment #9080042 - Attachment is obsolete: true
Attachment #9080043 - Attachment is obsolete: true
Attachment #9080046 - Attachment is obsolete: true
Attachment #9080047 - Attachment is obsolete: true
Attachment #9080049 - Attachment is obsolete: true
Attachment #9080050 - Attachment is obsolete: true
Attachment #9080051 - Attachment is obsolete: true

Also each patch needs to have the proper commit message properly formatted.
I'd suggest doing one or two interfaces first completely and then continue on with the next.

Ben, please take over here since Benjamin is no longer working for Thunderbird.

Assignee: benjamin → benc
Status: ASSIGNED → NEW
Assignee: benc → khushil324
Priority: -- → P2
Attachment #9108316 - Attachment description: Bug-1557504_remove-array-calIImportExport.patch → Bug-1557504_remove-array-calIImportExport-0.patch
Attachment #9108469 - Flags: review?(paul)
Attachment #9108469 - Flags: feedback?(mkmelin+mozilla)
Attachment #9108342 - Attachment is patch: true
Comment on attachment 9108469 [details] [diff] [review] Bug-1557504_remove-array-calIItemBase-calIAlarm-0.patch Review of attachment 9108469 [details] [diff] [review]: ----------------------------------------------------------------- Changes look good. I haven't tested, but the try run covers that in this case.
Attachment #9108469 - Flags: review?(paul) → review+

calIAlarm.idl
calICalendar.idl
calICalendarACLManager.idl
calICalendarManager.idl
calICalendarSearchProvider.idl
calICalendarView.idl
calICalendarViewController.idl
calIChangeLog.idl
calIDecoratedView.idl
calIIcsParser.idl
calIIcsSerializer.idl
calIICSService.idl
calIImportExport.idl
calIItemBase.idl
calIItipItem.idl
calIItipTransport.idl
calIPrintFormatter.idl
calIRecurrenceInfo.idl
calIRecurrenceItem.idl
calIRecurrenceRule.idl

Attachment #9108524 - Flags: review?(paul)
Attachment #9108524 - Flags: feedback?(mkmelin+mozilla)
Attachment #9108524 - Attachment is patch: true
Status: NEW → ASSIGNED
Attachment #9108533 - Flags: review?(paul)
Attachment #9108469 - Flags: feedback?(mkmelin+mozilla)

Can you tell me what you want checked in? Attachment 9108469 [details] [diff] which is the only one with r+ so far? Have you looked at your own try run?
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=9cb5c0e7b0e90d36df874f00e92e6254ef622c07
That looks pretty terrible to me. Firstly, you haven't rebased for a while, so you're getting errors you shouldn't be getting and second, the mochitests (bct) which include the calendar tests are pretty bad, for example:
TEST-UNEXPECTED-FAIL | comm/calendar/test/browser/preferences/browser_categoryColors.js | Uncaught exception - at resource://testing-common/mozmill

Also, for try runs, just do Linux and Mac opt, since you're messing with JS only, we don't need debug builds and we don't need Windows either.

Comment on attachment 9108469 [details] [diff] [review] Bug-1557504_remove-array-calIItemBase-calIAlarm-0.patch Test failures :-(
Attachment #9108469 - Flags: feedback-

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

Test failures :-(

Let me update the patch and submit it again.

Attachment #9108316 - Flags: feedback?(mkmelin+mozilla) → feedback+
Comment on attachment 9108316 [details] [diff] [review] Bug-1557504_remove-array-calIImportExport-0.patch Review of attachment 9108316 [details] [diff] [review]: ----------------------------------------------------------------- Changes look good overall. I have one question though. ::: calendar/import-export/calIcsImportExport.js @@ +35,3 @@ > let parser = Cc["@mozilla.org/calendar/ics-parser;1"].createInstance(Ci.calIIcsParser); > parser.parseFromStream(aStream, null); > + return parser.getItems({}); Hm, can you explain what the story is with this getItems call? Looks like it takes an integer not an object ("uint32_t"). https://searchfox.org/comm-central/source/calendar/base/public/calIIcsParser.idl#77
Comment on attachment 9108342 [details] [diff] [review] Bug-1557504_remove-array-calIItipItem-0.patch Review of attachment 9108342 [details] [diff] [review]: ----------------------------------------------------------------- Changes look good. r+
Attachment #9108342 - Flags: review?(paul) → review+

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

Hm, can you explain what the story is with this getItems call? Looks like
it takes an integer not an object ("uint32_t").
https://searchfox.org/comm-central/source/calendar/base/public/calIIcsParser.
idl#77

It will be used like this: https://searchfox.org/comm-central/source/calendar/base/src/calIcsParser.js#170
And anyway, it will get removed and the related patch is also submitted.

Khushil, can you attach the new version of Bug-1557504_remove-array-calIItemBase-calIAlarm-0.patch. Looks like that can land together with Bug-1557504_remove-array-calIItipItem-0.patch.

Yes, wait. There are three patches that can go together. I am just updating them in sequence.

Comment on attachment 9108524 [details] [diff] [review] Bug-1557504_remove-array-calICalendarManager-calICalendar-0.patch Review of attachment 9108524 [details] [diff] [review]: ----------------------------------------------------------------- Looks good overall. A couple of questions / things to address, and we should check with Magnus about the aArgs thing. ::: calendar/providers/composite/calCompositeCalendar.js @@ +512,5 @@ > if (this.mMaxItems && this.mItemsReceived >= this.mMaxItems) { > return; > } > > + let aCount = aItems.length; "aCount" is not the name you need here. The "a" is to indicate an argument passed to the function. You could use "count" but that's vague, so I'd go with "itemsCount" or "itemsLength", or maybe best is just skip the separate variable and use "aItems.length". ::: calendar/providers/ics/calICSCalendar.js @@ +366,5 @@ > self.forceRefresh(); > } > }, > + onGetResult: function(aCalendar, aStatus, aItemType, aDetail, aItems) { > + this.serializer.addItems(aItems, {}); Instead of `{}`, seems like we'd want `aItems.length` for the second argument here? @@ +528,5 @@ > this.mAction = action; > } > modListener.prototype = { > QueryInterface: ChromeUtils.generateQI([Ci.calIOperationListener]), > + onGetResult: function(aCalendar, aStatus, aItemType, aDetail, aItems) {}, Based on Magnus' code reviews I've been avoiding using the aArgs naming convention for function arguments. (See bug 1533520.) So ideally we'd avoid them in this patch, and move away from them where we are making changes to function arguments. That said, this is already such a big set of changes, and you've already done them, so I'm not sure if we want to add that to it now. We should ask Magnus.
Attachment #9108524 - Flags: review?(paul) → feedback+
Comment on attachment 9108316 [details] [diff] [review] Bug-1557504_remove-array-calIImportExport-0.patch Review of attachment 9108316 [details] [diff] [review]: ----------------------------------------------------------------- r+ Thanks for the explanation.
Attachment #9108316 - Flags: review?(paul) → review+
Comment on attachment 9108533 [details] [diff] [review] Bug-1557504_remove-array-calIPrintFormatter-calIItipTransport-0.patch Review of attachment 9108533 [details] [diff] [review]: ----------------------------------------------------------------- Changes look good. ::: calendar/import-export/calListFormatter.js @@ +23,4 @@ > let htmlexporter = Cc["@mozilla.org/calendar/export;1?type=htmllist"].createInstance( > Ci.calIExporter > ); > + htmlexporter.exportToStream(aStream, {}, aItems, aTitle); I take it the `{}` instead of `aCount` here is similar to those in the other patch, where it doesn't actually need to be a number, and possibly will be removed by another patch. Looks like this is the interface for it: https://searchfox.org/comm-central/source/calendar/base/public/calIImportExport.idl#61
Attachment #9108533 - Flags: review?(paul) → review+
Comment on attachment 9108641 [details] [diff] [review] Bug-1557504_remove-array-calICalendarSearchProvider-calICalendarViewController-0.patch Review of attachment 9108641 [details] [diff] [review]: ----------------------------------------------------------------- Changes look good here too. Will be great to have all of these updated to the newer/better approach!
Attachment #9108641 - Flags: review?(paul) → review+
Attachment #9108469 - Attachment is obsolete: true
Attachment #9108729 - Flags: review+
Attachment #9108641 - Attachment description: Bug-1557504_remove-array-calICalendarSearchProvider-calICalendarViewController.patch → Bug-1557504_remove-array-calICalendarSearchProvider-calICalendarViewController-0.patch

Order:
Bug-1557504_remove-array-calIImportExport.patch(base)
Bug-1557504_remove-array-calIItipItem.patch
Bug-1557504_remove-array-calIItemBase-calIAlarm.patch
Bug-1557504_remove-array-calIPrintFormatter-calIItipTransport.patch
Bug-1557504_remove-array-calICalendarSearchProvider-calICalendarViewController.patch(tip)

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=1a988642b3dfb70c9b2696a81c74bf28dc12424f
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=30acc4bc0e80c467000de9774801f1bdad396624

Attachment #9108342 - Flags: feedback?(mkmelin+mozilla)

Fixes for ESLint error.

Attachment #9108641 - Attachment is obsolete: true
Attachment #9108757 - Flags: review+
Attachment #9108729 - Attachment is patch: true
Attachment #9108727 - Attachment is patch: true

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/1ce2ab429d0b
remove [array] use in xpidl from calIImportExport.idl. r=pmorris
https://hg.mozilla.org/comm-central/rev/64e5700c2428
remove [array] use in xpidl from calIItipItem.idl. r=pmorris
https://hg.mozilla.org/comm-central/rev/110ee34fe561
remove [array] use in xpidl from calIItemBase.idl and calIAlarm.idl. r=pmorris

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Attachment #9108757 - Attachment is patch: true

I'll land
Bug-1557504_remove-array-calIPrintFormatter-calIItipTransport.patch
Bug-1557504_remove-array-calICalendarSearchProvider-calICalendarViewController.patch(tip)
later.

Target Milestone: --- → 72
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/bfb3d45e856f
remove [array] use in xpidl from calIItipTransport.idl and calIPrintFormatter.idl. r=pmorris
https://hg.mozilla.org/comm-central/rev/d876748e86f5
remove [array] use in xpidl from calICalendarSearchProvider.idl and calICalendarViewController.idl. r=pmorris

Comment on attachment 9108524 [details] [diff] [review] Bug-1557504_remove-array-calICalendarManager-calICalendar-0.patch Review of attachment 9108524 [details] [diff] [review]: ----------------------------------------------------------------- ::: calendar/providers/ics/calICSCalendar.js @@ +366,5 @@ > self.forceRefresh(); > } > }, > + onGetResult: function(aCalendar, aStatus, aItemType, aDetail, aItems) { > + this.serializer.addItems(aItems, {}); I think there should be no second parameter, once calIIcsSerializer is changed. But it's not so far, so seems like a wrong change to me @@ +528,5 @@ > this.mAction = action; > } > modListener.prototype = { > QueryInterface: ChromeUtils.generateQI([Ci.calIOperationListener]), > + onGetResult: function(aCalendar, aStatus, aItemType, aDetail, aItems) {}, I think we can keep aNaming changes out of this patch.
Attachment #9108524 - Flags: feedback?(mkmelin+mozilla)
Attachment #9108524 - Attachment is obsolete: true
Attachment #9109116 - Flags: review?(paul)
Comment on attachment 9109116 [details] [diff] [review] Bug-1557504_remove-array-calICalendarManager-calICalendar-1.patch Hm, I have no idea why, but there's no "diff" or "splinter review" link for this patch. I did a manual diff of the two patches. I see some aArg conversions which seems fine to me. Also some functions have the args (like {}) removed that we're removing in this bug. I assume that's because other patches in this bug have now landed, so we're updating these calls to match. For the things in my previous review, the aCount -> itemCount thing looks good. On `this.serializer.addItems(aItems, {});` it's unchanged from earlier patch, and Magnus made a comment about it in comment 49. r+ if you can help me understand what the story is for that second argument there.
Attachment #9109116 - Flags: review?(paul) → review+
Comment on attachment 9109116 [details] [diff] [review] Bug-1557504_remove-array-calICalendarManager-calICalendar-1.patch Oh, I see the checkbox for "patch" is not checked for this patch. Let's see if checking it gets the diff and review links to appear. Edit: yep, that worked.
Attachment #9109116 - Attachment is patch: true

So this is the last patch that needs to land here? Bug-1557504_remove-array-calICalendarManager-calICalendar-1.patch

Any try run?

Keywords: leave-open

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

So this is the last patch that needs to land here? Bug-1557504_remove-array-calICalendarManager-calICalendar-1.patch
No, there are 10 more files to take care.

Any try run?

No try runs yet.

Keywords: leave-open
Attachment #9109744 - Attachment is patch: true

Hmm, linting error.

Umm, what about the linting error?

Attachment #9108757 - Attachment is obsolete: false
Attachment #9109116 - Attachment is obsolete: true
Attachment #9109744 - Attachment is obsolete: true
Attachment #9109790 - Flags: review+
Attachment #9109790 - Attachment is patch: true

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/2a0f5703a010
remove [array] use in xpidl from calICalendarManager.idl and calICalendar.idl. r=pmorris

Attachment #9109931 - Attachment is patch: true
Comment on attachment 9109931 [details] [diff] [review] Bug-1557504_remove-array-calICalendarACLManager-calICalendarView-calIDecoratedView-0.patch Review of attachment 9109931 [details] [diff] [review]: ----------------------------------------------------------------- Changes look good. For future reference, in cases like this, you could even just change one function per patch. That makes review simpler, keeps similar changes together in the history, etc.
Attachment #9109931 - Flags: review?(paul) → review+

Please rebase Bug-1557504_remove-array-calICalendarACLManager-calICalendarView-calIDecoratedView-0.patch and add a bug number to the commit message:
$ hg qpush
applying Bug-1557504_remove-array-calICalendarACLManager-calICalendarView-calIDecoratedView.patch
patching file calendar/base/content/calendar-views-utils.js
Hunk #3 FAILED at 531
1 out of 3 hunks FAILED -- saving rejects to file calendar/base/content/calendar-views-utils.js.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh Bug-1557504_remove-array-calICalendarACLManager-calICalendarView-calIDecoratedView.patch

Maybe you'll need to wait until my push is done to see the conflict.

Attachment #9110231 - Attachment is patch: true
Attachment #9110231 - Flags: review+

Updated the commit message.

Attachment #9110231 - Attachment is obsolete: true
Attachment #9110279 - Flags: review+
Attachment #9110284 - Attachment filename: Bug-1557504_remove-array-calIChangeLog.patch → Bug-1557504_remove-array-calIChangeLog-0.patch
Attachment #9110284 - Attachment description: Bug-1557504_remove-array-calIChangeLog.patch → Bug-1557504_remove-array-calIChangeLog-0.patch
Comment on attachment 9110284 [details] [diff] [review] Bug-1557504_remove-array-calIChangeLog-0.patch Review of attachment 9110284 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. A few minor comments to address. ::: calendar/providers/caldav/calDavCalendar.js @@ +302,5 @@ > > fetchCachedMetaData: function() { > cal.LOG("CalDAV: Retrieving server info from cache for " + this.name); > + let cacheIds = this.mOfflineStorage.getAllMetaDataIds(); > + let cacheValues = this.mOfflineStorage.getAllMetaDataValues(); Ahhhh, so much nicer! ::: calendar/providers/storage/calStorageCalendar.js @@ +2516,2 @@ > } catch (e) { > this.logError("Error getting all metadata!", e); The error messages here and below need to be updated to match the changed functions. @@ +2534,5 @@ > + } finally { > + query.reset(); > + } > + return values; > + }, Since these two functions are basically the same except for the id/value parts, you could merge them into a single "internal" function, something like this: _getAllMetaDataResults: function(key) { let query = this.mSelectAllMetaData; let results = []; try { this.prepareStatement(query); while (query.executeStep()) { results.push(query.row[key]); } } catch (e) { this.logError(`Error getting all metadata ${key == "item_id" ? "IDs" : "values"} ` + , e); } finally { query.reset(); } return results; } getAllMetaDataIds: this._getAllMetaDataResults.bind(null, "item_id"), getAllMetaDataValues: this._getAllMetaDataResults.bind(null, "value"), ::: calendar/test/unit/test_providers.js @@ +337,5 @@ > equal(aCalendar.getMetaData("item2"), "meta2"); > > + let ids, values; > + ids = aCalendar.getAllMetaDataIds(); > + values = aCalendar.getAllMetaDataValues(); I would just do: let ids = ... let values = ... No need for separate let declaration. @@ +338,5 @@ > > + let ids, values; > + ids = aCalendar.getAllMetaDataIds(); > + values = aCalendar.getAllMetaDataValues(); > + equal(values.length, 2); For good measure, I'd also add: equal(ids.length, 2); Here and in similar cases below. @@ +360,5 @@ > > aCalendar.deleteMetaData("item2"); > equal(aCalendar.getMetaData("item2"), null); > + values = aCalendar.getAllMetaDataValues(); > + equal(values.length, 0); Might as well check ids as well, here and below.
Attachment #9110284 - Flags: review?(paul) → feedback+
Comment on attachment 9110289 [details] [diff] [review] Bug-1557504_remove-array-calIIcsSerializer-0.patch Review of attachment 9110289 [details] [diff] [review]: ----------------------------------------------------------------- LGTM from inspecting the changes.
Attachment #9110289 - Flags: review?(paul) → review+

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/8dc56944ca5a
remove [array] use in xpidl from calICalendarACLManager.idl, calICalendarView.idl and calIDecoratedView.idl. r=pmorris
https://hg.mozilla.org/comm-central/rev/dd38ba65ea98
remove [array] use in xpidl from calIIcsSerializer.idl. r=pmorris

Attachment #9110284 - Attachment is obsolete: true
Comment on attachment 9110294 [details] [diff] [review] Bug-1557504_remove-array-calIIcsParser-calIICSService-0.patch Review of attachment 9110294 [details] [diff] [review]: ----------------------------------------------------------------- LGTM from reading the changes.
Attachment #9110294 - Flags: review?(paul) → review+
Comment on attachment 9110569 [details] [diff] [review] Bug-1557504_remove-array-calIChangeLog-1.patch Review of attachment 9110569 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, thanks. r+
Attachment #9110569 - Flags: review?(paul) → review+

The Try runs for both of the latest patches went wrong. More work to be done.

Attachment #9111094 - Attachment is patch: true

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/4f4eb9541c61
remove [array] use in xpidl from calIChangeLog.idl. r=pmorris

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/8ec25287b881
remove [array] use in xpidl from calIIcsParser.idl. r=pmorris

Comment on attachment 9111364 [details] [diff] [review] Bug-1557504_remove-array-calIWcapCalendar-0.patch Review of attachment 9111364 [details] [diff] [review]: ----------------------------------------------------------------- LGTM, from reading the changes.
Attachment #9111364 - Flags: review?(paul) → review+

Khushil please update your comm-central tree to a known good revision before doing a Try run. We shouldn't have to sort through the failures to work out which ones might have been caused by the patch being tested.

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/cffc48bec697
remove [array] use in xpidl from calIWcapCalendar.idl. r=pmorris DONTBUILD

Depends on: 1602422
Depends on: 1602423
Depends on: 1602424
Depends on: 1602425

Looks like done now,

Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: 72 → 73
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: