Miller Medeiros [:millermedeiros] Reporter
	Description • 10 years ago

fix JSHint errors for files at the top-level "js" folder, "js/utils" and "js/worker"

	Evan Tseng [:evanxd] Assignee
	Comment 2 • 10 years ago

Comment on attachment 8399038 [details] [review]
Pull request

Hi Miller,

Please help me to review the patch.
Thanks.

	Kevin Grandon :kgrandon
	Comment 3 • 10 years ago

Comment on attachment 8399038 [details] [review]
Pull request

Travis is green, so let's do it. Probably better to have the globals specified per file so we can understand our dependencies a bit better, but this looks good to me.

	Kevin Grandon :kgrandon
	Comment 4 • 10 years ago

Landed: https://github.com/mozilla-b2g/gaia/commit/5ed8fe436345eaf5c9f4f6d8e8cd1170da10ca9d

	Evan Tseng [:evanxd] Assignee
	Comment 5 • 10 years ago

Hi Kevin,

Thanks for your help. :)

	Jason Smith [:jsmith]
	Comment 6 • 10 years ago

Flagging for checkin-needed for a backout for causing bug 991896.

	Kevin Grandon :kgrandon
	Comment 7 • 10 years ago

Darn, I could be wrong but I am fairly sure what's causing this is this line: https://github.com/mozilla-b2g/gaia/pull/17779/files#diff-6bbe7f35ee63a1bab8c2df5cf6907d5fR21

I know we have caldav tests, so I'm wondering how our CI suite didn't catch this. Evan - can you also investigate if we can add a test that would catch this?

	Kevin Grandon :kgrandon
	Comment 8 • 10 years ago

There were a few conflicts backing this out, so I went ahead and took care of it:

https://github.com/mozilla-b2g/gaia/commit/92d4447a9ee72a49a09410143c9cb3068f73cd53

	Evan Tseng [:evanxd] Assignee
	Comment 9 • 10 years ago

Hi Kevin,

I'm investigating that.

	Evan Tseng [:evanxd] Assignee
	Comment 10 • 10 years ago

Hi Kevin,

The root cause of Bug 991896 is that we removed https://github.com/evanxd/gaia/blob/e05b0063fbb6139c2df8910e2b09590fad6b77d9/apps/calendar/js/worker/thread.js#L8-L11 before. After we added these lines back, it could work now.

But we might not write a uni test for that, because we could not let `window` as undefined in our unit test runner. Or we could write a marionette test for it? But the test could not run on the try server(tbpl). How do you think?

And could you help me to review the patch?
Thanks.

	Kevin Grandon :kgrandon
	Comment 11 • 10 years ago

Comment on attachment 8402514 [details] [review]
Pull request

Nice find. I would love to prioritize adding a marionette test for this functionality if possible in the future. I tested the add account flow and everything looks good, so R+ from me.

https://github.com/mozilla-b2g/gaia/commit/07eabe06e407be0411466aeeb6efdb99bb648aaa

	Evan Tseng [:evanxd] Assignee
	Comment 12 • 10 years ago

(In reply to Kevin Grandon :kgrandon from comment #11)
> Comment on attachment 8402514 [details] [review]
> Pull request
> 
> Nice find. I would love to prioritize adding a marionette test for this
> functionality if possible in the future. 
Sure, let's do this in Bug 994017.