Last Comment Bug 1095119 - Remove Promise.defer usage
: Remove Promise.defer usage
Status: RESOLVED FIXED
[good first bug][lang=js]
:
Product: Calendar
Classification: Client Software
Component: Internal Components (show other bugs)
: Trunk
: All All
-- normal (vote)
: 4.0.0.1
Assigned To: Sparsh Paliwal [:discoman]
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2014-11-06 13:29 PST by Philipp Kewisch [:Fallen]
Modified: 2015-01-31 04:39 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
bug-1095119.diff (5.61 KB, patch)
2015-01-29 20:29 PST, Sparsh Paliwal [:discoman]
philipp: review+
Details | Diff | Splinter Review
bug-1095119.diff (5.64 KB, patch)
2015-01-30 01:59 PST, Sparsh Paliwal [:discoman]
sparshpaliwal123: review+
Details | Diff | Splinter Review
fix test failure (1.51 KB, patch)
2015-01-30 15:55 PST, :aceman
no flags Details | Diff | Splinter Review
fix test failure v2 (1.51 KB, patch)
2015-01-30 18:17 PST, :aceman
no flags Details | Diff | Splinter Review

Description User image Philipp Kewisch [:Fallen] 2014-11-06 13:29:27 PST
I totally liked Promise.defer, but this is not a feature of DOM promises, which will supersede Promise.jsm at some point and will be removed in bug 1034599. We need to replace them with DOM promises in calendar code or use PromiseUtils from bug 1093021.
Comment 1 User image Sparsh Paliwal [:discoman] 2015-01-29 20:29:19 PST
Created attachment 8556857 [details] [diff] [review]
bug-1095119.diff

https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/Promise.jsm/Deferred

Here, I realized that implementation of new Promise() as deferred object would require a helper function first and so it's better to look out for places where new Promise can be implemented easily and I found it was in only test_deleted_items.js, every where else our beloved deferred object is used to resolve promise in different parts of code,so I implemented new Promise() here. Would request a review and help in case I have done something wrong.
Comment 2 User image Philipp Kewisch [:Fallen] 2015-01-30 00:55:51 PST
Comment on attachment 8556857 [details] [diff] [review]
bug-1095119.diff

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

You are right, we surprisingly don't use promises that much. Code looks great, thanks for the patch! Let me know if you need more issues to work on. r=philipp.

For checkin, could you change the patch message from "Remove Promise.defer" to something like "Bug 1095119 - Remove Promise.defer usage. r=philipp", upload that as a new attachment and set the checkin-needed keyword? You can directly set review+ on the new patch since I've already +'d this one.
Comment 3 User image Sparsh Paliwal [:discoman] 2015-01-30 01:59:24 PST
Created attachment 8557016 [details] [diff] [review]
bug-1095119.diff

Thank you very much for the help,I never submitted a patch so quickly. If something is wrong with this one we will get back to it. Meanwhile which of these you think i should do next-
Bug-366680
Bug-396515
Bug-842383
Bug-472630
Comment 4 User image Philipp Kewisch [:Fallen] 2015-01-30 02:12:31 PST
I think bug 472630 would be nice, if you want another simple one then bug 842383 is also a good candidate.
Comment 5 User image :aceman 2015-01-30 15:55:30 PST
Created attachment 8557388 [details] [diff] [review]
fix test failure

The patch caused a failure in the test it is touching:
TEST-UNEXPECTED-FAIL | xpcshell-icaljs.ini:calendar/test/unit/test_deleted_items.js | xpcshell return code: 0
SyntaxError: expected expression, got ')' at /builds/slave/test/build/tests/xpcshell/tests/calendar/test/unit/test_deleted_items.js:31
ReferenceError: run_test is not defined at /builds/slave/test/build/tests/xpcshell/head.js:504

I try to fix the syntax error in this patch.

Aryx, please push to try server.
Comment 6 User image Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) 2015-01-30 16:10:38 PST
Pushed to Thunderbird-Try https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=805e9cd652fa
Comment 7 User image :aceman 2015-01-30 18:17:32 PST
Created attachment 8557426 [details] [diff] [review]
fix test failure v2

Didn't work :)
Comment 8 User image Philip Chee 2015-01-31 00:56:57 PST
Comment on attachment 8557016 [details] [diff] [review]
bug-1095119.diff

http://hg.mozilla.org/comm-central/rev/613cf9f167f3
Comment 9 User image Philipp Kewisch [:Fallen] 2015-01-31 04:11:12 PST
Pushed to comm-central changeset 12201ec2a310
Comment 10 User image Philipp Kewisch [:Fallen] 2015-01-31 04:11:50 PST
Sorry about the missing push comment, my python script had an error. I'll update the other bugs in case they haven't already been.
Comment 11 User image Philipp Kewisch [:Fallen] 2015-01-31 04:39:59 PST
Comment on attachment 8557426 [details] [diff] [review]
fix test failure v2

Oops I totally missed you already uploaded a patch, sorry about that. The patch I pushed does about the same thing and also fixes the order of setting the handleCompletion function and calling aFunc().

Note You need to log in before you can comment on or make changes to this bug.