First seen: Thu Feb 9, 2017, 7:47:00 https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=450c53f428f5a590658ab880b8cf28d590c59ae9 Last good: M-C: 5e17f9181c6c First bad: M-C: f505911eb333 Range: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=5e17f9181c6c&tochange=f505911eb333 Error according to log: INFO - SUMMARY-PASS | test-blocked-content.js::setupModule INFO - SUMMARY-UNEXPECTED-FAIL | C:\slave\test\build\tests\mozmill\composition\test-blocked-content.js | test-blocked-content.js::test_paste_file_urls INFO - EXCEPTION: Timeout waiting for pasted tmp image to be loaded ok INFO - at: utils.js line 447 INFO - TimeoutError utils.js:447 13 INFO - waitFor utils.js:485 11 INFO - MozMillController.prototype.waitFor controller.js:687 3 INFO - test_paste_file_urls test-blocked-content.js:100 3 INFO - Runner.prototype.wrapper frame.js:585 9 INFO - Runner.prototype._runTestModule frame.js:655 9 INFO - Runner.prototype.runTestModule frame.js:701 3 INFO - Runner.prototype.runTestDirectory frame.js:525 7 INFO - runTestDirectory frame.js:707 3 INFO - Bridge.prototype._execFunction server.js:179 10 INFO - Bridge.prototype.execFunction server.js:183 16 INFO - Session.prototype.receive server.js:283 3 INFO - AsyncRead.prototype.onDataAvailable server.js:88 3
Magnus, your new test is already failing ;-( It's really funny since I copied that section into a test I wrote, well, patch-worked together, and that doesn't fail, but the original does.
Created attachment 8835638 [details] [diff] [review] date.patch since i don't want anymore backouts of my patches ;) here's the fix. maybe it was Bug 1335536, or maybe deprecated lastModifiedDate finally was removed or something. test passes locally.
Thanks, but I don't think this is right at all. Have you tested this after refreshing your local build? File.createFromNsIFile() is async now, so you need something like: https://hg.mozilla.org/mozilla-central/rev/b6a45e1bab4c#l18.15 or perhaps: https://hg.mozilla.org/mozilla-central/rev/b6a45e1bab4c#l17.12 I'm experimenting with a .then right now.
Comment on attachment 8835638 [details] [diff] [review] date.patch ah, of course..
Created attachment 8835662 [details] [diff] [review] WIP: 1338138-create-file-async.patch (v1) This repairs the function in TB, the test still fails since it needs to wait differently now.
Created attachment 8835698 [details] [diff] [review] 1338138-create-file-async.patch (v2). This does the trick. The indentation is *deliberately* wrong for easier review. I'll indent it correctly when landing. I put Aceman as a second reviewer, so whoever comes first. It would be good to get this landed quickly, since the function is currently broken. I testes pasting from MS Word, too, since for that purpose we have all this processing.
Comment on attachment 8835698 [details] [diff] [review] 1338138-create-file-async.patch (v2). Review of attachment 8835698 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/components/compose/content/MsgComposeCommands.js @@ +2269,5 @@ > let html = dataTransfer.getData("text/html"); > let doc = (new DOMParser()).parseFromString(html, "text/html"); > let tmpD = Services.dirsvc.get("TmpD", Components.interfaces.nsIFile); > let pendingConversions = 0; > + let firstTime = true; I don't like the name. It's easier to understand too as a counter like it used to be
(In reply to Magnus Melin from comment #8) > > + let firstTime = true; > I don't like the name. It's easier to understand too as a counter like it > used to be Well, the counter confused me. I was looking whether anyone looked at the value. No one. Can we leave it a bool? What name do you suggest? needToCancelDefault = false; ?
Sorry, needToCancelDefault = true; to start with.
https://hg.mozilla.org/comm-central/rev/e5b59ae458f1c9620edad6c53fca4e9e511cb2d8 I left the bool, but changed the name and gave it a nice comment. The changeset looks terrible due to the changed indentation of a long block. Thanks for the quick review, btw.
but you dropped the lastModifiedDate change, meaning someone will have to come back here.. https://dxr.mozilla.org/comm-central/rev/5e17f9181c6cb0968966280d1c1d96e725702af1/mozilla/dom/webidl/File.webidl#33
https://hg.mozilla.org/comm-central/rev/e5b59ae458f1c9620edad6c53fca4e9e511cb2d8 (comment #11) https://hg.mozilla.org/comm-central/rev/af32a353227ef5633d06be9ffee8fd491af3350d (follow up) Thanks, yes, I saw the deprecation warning but honestly, I couldn't see the other interface existed. Too many fires at once. Can you please point me to it. I landed this in your name since it was your idea in the first place. Thanks again.
It's |lastModified| and is in line 19 of the link in comment 12. It's also in the File spec: https://w3c.github.io/FileAPI/#file
Thanks, I did see that but was confused by the |long long|, ah well, it's just a lot of milliseconds ;-) All done now, one fire less ;-)