Closed Bug 1159638 Opened 9 years ago Closed 9 years ago

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

Categories

(Calendar :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: aleth, Assigned: kepta, Mentored)

Details

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

Attachments

(1 file, 3 obsolete files)

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
Whiteboard: [good first bug][lang=js]
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
Attached patch bug1159638.patch (obsolete) β€” β€” Splinter Review
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+
Attached patch bug1159638.patch (obsolete) β€” β€” Splinter Review
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+
Attached patch bug1159638.patch (obsolete) β€” β€” Splinter Review
Is this fine?
Attachment #8601416 - Attachment is obsolete: true
Attachment #8602020 - Attachment is obsolete: true
Attachment #8602169 - Flags: checkin?(philipp)
Attached patch bug1159638.patch β€” β€” Splinter Review
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+
Pushed to comm-central changeset 88068e33345b
Status: ASSIGNED → RESOLVED
Closed: 9 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.