Getter fails in calender-migration-dialog on first run after installation

RESOLVED FIXED in 4.2

Status

Calendar
General
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: aleth, Assigned: kepta, Mentored)

Tracking

Trunk

Details

(Whiteboard: [good first bug][lang=js])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

2 years ago
JavaScript error: chrome://calendar/content/calendar-migration-dialog.js, line 462: NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIProperties.get]
This one I believe is *NOT* fixed with bug 1049591 :-) Thanks for filing these bugs!
Mentor: philipp@bugzilla.kewis.ch
Whiteboard: [good first bug][lang=js]
(Assignee)

Comment 2

2 years ago
philipp,
Can I please work on this bug ?
Sure thing. Let me know if you need help getting started.
Assignee: nobody → 0o3ko0
Status: NEW → ASSIGNED
(Assignee)

Comment 4

2 years ago
Created attachment 8601416 [details] [diff] [review]
bug1159638.patch

Please review the patch.
Attachment #8601416 - Flags: review?(philipp)
Comment on attachment 8601416 [details] [diff] [review]
bug1159638.patch

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

I'd appreciate a new patch with the following comment fixed:

::: calendar/base/content/dialogs/calendar-migration-dialog.js
@@ +467,5 @@
> +    		return [];
> +    	}
> +    	else {
> +    		let maildir = this.dirService.get("LocalAppData",
> +												Components.interfaces.nsILocalFile);

This will not work, as you define maildir with let inside a block. The variable will go out of scope as soon as you leave the else block.

That said, you don't need an else block at all. If it goes into the if-part, then it will return anyway. Just define the variable after the if-block as before.
Attachment #8601416 - Flags: review?(philipp) → feedback+
(Assignee)

Comment 6

2 years ago
Created attachment 8602020 [details] [diff] [review]
bug1159638.patch

That was silly, I forgot about the scope of the let statement.
Please review this patch.
Attachment #8602020 - Flags: review?(philipp)
Comment on attachment 8602020 [details] [diff] [review]
bug1159638.patch

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

Looks good to me, r=philipp :)

Sorry I didn't mention this before, but could you upload a patch with a commit message formatted like this?

Bug 1159638 - Getter fails in calender-migration-dialog on first run after installation. r=philipp

If you are using mq you can do it with hg qrefresh -m "the message"

You can then set checkin-needed as a keyword when uploading the patch and set review+ on the patch because you already have review+ from this one.

::: calendar/base/content/dialogs/calendar-migration-dialog.js
@@ +467,5 @@
>              return [];
>          }
> +
> +        let maildir = this.dirService.get("LocalAppData",
> +                                            Components.interfaces.nsILocalFile);

Very minor nit, and I wouldn't have mentioned if you didn't have to upload a new patch anyway, but could you align the Components with the "LocalAppData", i.e. remove two spaces?
Attachment #8602020 - Flags: review?(philipp) → review+
(Assignee)

Comment 8

2 years ago
Created attachment 8602169 [details] [diff] [review]
bug1159638.patch

Is this fine?
Attachment #8601416 - Attachment is obsolete: true
Attachment #8602020 - Attachment is obsolete: true
Attachment #8602169 - Flags: checkin?(philipp)
(Assignee)

Comment 9

2 years ago
Created attachment 8602171 [details] [diff] [review]
bug1159638.patch

Sorry, I uploaded the wrong file earlier.
Attachment #8602169 - Attachment is obsolete: true
Attachment #8602169 - Flags: checkin?(philipp)
Attachment #8602171 - Flags: checkin?(philipp)
Comment on attachment 8602171 [details] [diff] [review]
bug1159638.patch

Looks great, thanks! I'm switching from the checkin flag to the checkin-needed keyword. I know its easy to mix up :-)
Attachment #8602171 - Flags: checkin?(philipp) → review+
Keywords: checkin-needed
Pushed to comm-central changeset 88068e33345b
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 4.2
You need to log in before you can comment on or make changes to this bug.