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

Status: RESOLVED FIXED

(Calendar :: General, defect)

Description

[aleth [:aleth]]

JavaScript error: chrome://calendar/content/calendar-migration-dialog.js, line 462: NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIProperties.get]

Comment 1

[Philipp Kewisch [:Fallen][📱📆]]

This one I believe is *NOT* fixed with bug 1049591 :-) Thanks for filing these bugs!

Comment 2

[Kushan Joshi [:kepta]]

philipp,
Can I please work on this bug ?

Comment 3

[Philipp Kewisch [:Fallen][📱📆]]

Sure thing. Let me know if you need help getting started.

Comment 4

[Kushan Joshi [:kepta]]

Attachment: bug1159638.patch

Please review the patch.

Comment 5

[Philipp Kewisch [:Fallen][📱📆]]

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.

Comment 6

[Kushan Joshi [:kepta]]

Attachment: bug1159638.patch

That was silly, I forgot about the scope of the let statement.
Please review this patch.

Comment 7

[Philipp Kewisch [:Fallen][📱📆]]

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?

Comment 8

[Kushan Joshi [:kepta]]

Attachment: bug1159638.patch

Is this fine?

Comment 9

[Kushan Joshi [:kepta]]

Attachment: bug1159638.patch

Sorry, I uploaded the wrong file earlier.

Comment 10

[Philipp Kewisch [:Fallen][📱📆]]

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 :-)

Comment 11

[Philipp Kewisch [:Fallen][📱📆]]

Pushed to comm-central changeset 88068e33345b