Closed
Bug 740956
Opened 12 years ago
Closed 12 years ago
Cancelling uploads to Dropbox or YouSendIt does not work, and is unavailable after first attempt
Categories
(Thunderbird :: Message Compose Window, defect)
Tracking
(thunderbird13 fixed)
RESOLVED
FIXED
Thunderbird 14.0
Tracking | Status | |
---|---|---|
thunderbird13 | --- | fixed |
People
(Reporter: mconley, Assigned: Bienvenu)
References
Details
Attachments
(4 files, 5 obsolete files)
8.85 KB,
image/png
|
Details | |
1.99 KB,
patch
|
Bienvenu
:
review+
Bienvenu
:
approval-comm-aurora+
|
Details | Diff | Splinter Review |
7.61 KB,
patch
|
mconley
:
review+
Bienvenu
:
approval-comm-aurora+
|
Details | Diff | Splinter Review |
31.61 KB,
patch
|
Bienvenu
:
review+
Bienvenu
:
approval-comm-aurora+
|
Details | Diff | Splinter Review |
Using Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0) Gecko/20120330 Thunderbird/13.0a2 Steps to reproduce for Dropbox: 1) Create both a Dropbox Filelink account. 2) Start uploading a large file to Dropbox, and then cancel the upload. What happens? The icon reverts back to the non-uploading icon, but the upload continues in the background, and the URL is inserted upon completion. What's expected? The upload should have been cancelled. Steps to reproduce for YouSendIt: 1) Create a YouSendIt Filelink account 2) Upload a large file to YouSendIt, and right-click on the attachment item to cancel it What happens? There's not Cancel option available for the uploading file. What's expected? There should be a cancel menuitem there...
Reporter | ||
Updated•12 years ago
|
status-thunderbird13:
--- → affected
Assignee | ||
Comment 1•12 years ago
|
||
presumably item.uploading isn't set for yousendit uploads?
Assignee | ||
Comment 2•12 years ago
|
||
cancel is available for yousendit (I tried it here). It doesn't seem to work; I believe that's because the yousendit uploader doesn't implement cancel. I can fix that.
Assignee | ||
Comment 3•12 years ago
|
||
I think cancel isn't enabled after the first attempt; it doesn't have to do with yousendit per se, I don't think. I have a fix for that, I believe. I'm still working on the cancel stuff.
Assignee | ||
Comment 4•12 years ago
|
||
this gets cancel basically working, at least to the extent that we cancel the channel. The compose window still seems to think that the upload succeeded, but I'm pretty sure we're cancelling the channel now.
Assignee: nobody → dbienvenu
Reporter | ||
Comment 5•12 years ago
|
||
David: I just noticed that for both YouSendIt and Dropbox, we use: req.channel.cancel(); Passing zero arguments. Should we not pass some error status, like "NS_BINDING_ABORTED"? (http://mxr.mozilla.org/comm-central/source/mozilla/netwerk/base/public/nsIRequest.idl#77) -Mike
Assignee | ||
Comment 6•12 years ago
|
||
(In reply to Mike Conley (:mconley) from comment #5) > Passing zero arguments. Should we not pass some error status, like > "NS_BINDING_ABORTED"? thx, that might be why the compose window thinks the upload succeeded. I'll add that to the patch.
Assignee | ||
Comment 7•12 years ago
|
||
this should get it working all the way through, and stops the front end from telling you that there was an error, if you explicitly cancel.
Attachment #611092 -
Attachment is obsolete: true
Attachment #611507 -
Flags: review?(mconley)
Reporter | ||
Comment 8•12 years ago
|
||
Fixes the UI to allow us to re-upload a file that has been cancelled.
Reporter | ||
Comment 9•12 years ago
|
||
And tests! Huzzah!
Reporter | ||
Comment 10•12 years ago
|
||
Comment on attachment 611507 [details] [diff] [review] get cancel, and don't alert the user Review of attachment 611507 [details] [diff] [review]: ----------------------------------------------------------------- One little problem I noticed - we don't seem to do any cancellation when cancelling one of a group of uploads. STR: 1) Upload a series of files in one shot. 2) Try cancelling any of the files that are NOT the first to be uploaded It's possible that the channels are actually being cancelled and the UI is not responding properly. Can you confirm? ::: mail/components/cloudfile/nsDropbox.js @@ +199,5 @@ > this._uploader.cancel(); > } > else { > for (let i = 0; i < this._uploads.length; i++) > + if (this._uploads[i].file.equals(aFile)) { I don't see any cancellation happening in this else clause - and manual testing seems to indicate that when we have multiple files being uploaded, we can only cancel the first one. Same goes for nsYouSendIt.
Assignee | ||
Comment 11•12 years ago
|
||
Comment on attachment 611801 [details] [diff] [review] Make cancelling and uploading repeatable makes sense.
Attachment #611801 -
Flags: review+
Assignee | ||
Comment 12•12 years ago
|
||
(In reply to Mike Conley (:mconley) from comment #10) > > STR: > > 1) Upload a series of files in one shot. > 2) Try cancelling any of the files that are NOT the first to be uploaded uploads are serialized, so the code simply removes the upload request if it hasn't been started. We most likely need to update the UI by telling it that the request was cancelled, even though it hasn't started. > > It's possible that the channels are actually being cancelled there shouldn't be any channels for uploads that haven't started yet.
Reporter | ||
Comment 13•12 years ago
|
||
(In reply to David :Bienvenu from comment #12) > (In reply to Mike Conley (:mconley) from comment #10) > > > > there shouldn't be any channels for uploads that haven't started yet. Ah, of course. > uploads are serialized, so the code simply removes the upload request if it > hasn't been started. We most likely need to update the UI by telling it that > the request was cancelled, even though it hasn't started. Cool. I can take care of that, if you'd like. I'll write some tests while I'm at it.
Reporter | ||
Comment 14•12 years ago
|
||
Comment on attachment 611507 [details] [diff] [review] get cancel, and don't alert the user I'm happy with this.
Attachment #611507 -
Flags: review?(mconley) → review+
Assignee | ||
Comment 15•12 years ago
|
||
same as 611507, w/ addition of notifying front end of cancels for uploads that haven't started.
Attachment #611507 -
Attachment is obsolete: true
Attachment #611830 -
Flags: review?(mconley)
Reporter | ||
Comment 16•12 years ago
|
||
Wow, that took forever, sorry about that. Before I ask for review, going to push this to try and let it back overnight. I'm adding some timer-y things, and I want some assurance that I haven't introduced some random oranges.
Attachment #611802 -
Attachment is obsolete: true
Reporter | ||
Comment 17•12 years ago
|
||
Disentangled from my patch for bug 738299.
Attachment #611953 -
Attachment is obsolete: true
Reporter | ||
Comment 18•12 years ago
|
||
Comment on attachment 611830 [details] [diff] [review] fix for cancelling uploads that haven't started Review of attachment 611830 [details] [diff] [review]: ----------------------------------------------------------------- This looks reasonable to me. ::: mail/components/compose/content/MsgComposeCommands.js @@ +727,5 @@ > } > > let bucket = document.getElementById("attachmentBucket"); > for (let [,item] in Iterator(bucket.selectedItems)) { > + if (item && item.uploading) I'm just curious - in what cases is item not defined or null?
Attachment #611830 -
Flags: review?(mconley) → review+
Assignee | ||
Comment 19•12 years ago
|
||
(In reply to Mike Conley (:mconley) from comment #18) > > I'm just curious - in what cases is item not defined or null? After we do the first cancel, then the code failed, presumably because item.uploading threw an exception. Seems like selectedItems shouldn't return something bogus, but I didn't drill down on it. patch landed on trunk - http://hg.mozilla.org/comm-central/rev/6d73f1858352
Reporter | ||
Comment 20•12 years ago
|
||
Attachment 611801 [details] [diff] committed to comm-central as http://hg.mozilla.org/comm-central/rev/76d613340388
Reporter | ||
Comment 21•12 years ago
|
||
Comment on attachment 611801 [details] [diff] [review] Make cancelling and uploading repeatable We certainly want to be able to cancel uploads, so a? for TB 13.
Attachment #611801 -
Flags: approval-comm-aurora?
Reporter | ||
Comment 22•12 years ago
|
||
Comment on attachment 611830 [details] [diff] [review] fix for cancelling uploads that haven't started Same as above.
Attachment #611830 -
Flags: approval-comm-aurora?
Assignee | ||
Updated•12 years ago
|
Attachment #611830 -
Flags: approval-comm-aurora? → approval-comm-aurora+
Assignee | ||
Updated•12 years ago
|
Attachment #611801 -
Flags: approval-comm-aurora? → approval-comm-aurora+
Reporter | ||
Comment 23•12 years ago
|
||
Attachment 611830 [details] [diff] committed to comm-aurora as http://hg.mozilla.org/releases/comm-aurora/rev/45bc09d75c4f Attachment 611803 [details] committed to comm-aurora as http://hg.mozilla.org/releases/comm-aurora/rev/e7c41caf1f08
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 14.0
Reporter | ||
Comment 24•12 years ago
|
||
David: Ok, I've cleaned these up a fair bit. Can you take them for a spin on your Windows box? I'm having troubles getting my Windows build going, over here... Thanks, -Mike
Attachment #611958 -
Attachment is obsolete: true
Attachment #612197 -
Flags: review?(dbienvenu)
Assignee | ||
Comment 25•12 years ago
|
||
Comment on attachment 612197 [details] [diff] [review] Tests v4 good job! I'm more than a little worried about intermittent oranges due to machine speed differences so we'll have to keep an eye out for that. Great to have tests for this, though.
Attachment #612197 -
Flags: review?(dbienvenu) → review+
Reporter | ||
Comment 26•12 years ago
|
||
Comment on attachment 612197 [details] [diff] [review] Tests v4 Pretty safe for Aurora. The only risk is introducing intermittent oranges.
Attachment #612197 -
Flags: approval-comm-aurora?
Assignee | ||
Comment 27•12 years ago
|
||
Comment on attachment 612197 [details] [diff] [review] Tests v4 I'll approve for aurora, but I would love 24 hours of cycling on tinderbox trunk before landing this on aurora.
Attachment #612197 -
Flags: approval-comm-aurora? → approval-comm-aurora+
Reporter | ||
Comment 28•12 years ago
|
||
(In reply to David :Bienvenu from comment #27) > Comment on attachment 612197 [details] [diff] [review] > Tests v4 > > I'll approve for aurora, but I would love 24 hours of cycling on tinderbox > trunk before landing this on aurora. I think that's wise. Agreed.
Reporter | ||
Comment 29•12 years ago
|
||
David: I haven't seen any oranges from these tests pop up. In fact, I'm reasonably certain this patch fixed a random orange we were seeing on Windows (bug 739986). Am I OK to push to comm-aurora? -Mike
Assignee | ||
Comment 30•12 years ago
|
||
(In reply to Mike Conley (:mconley) from comment #29) > Am I OK to push to comm-aurora? yes, definitely, thx!
Reporter | ||
Comment 31•12 years ago
|
||
Ok, Attachment 612197 [details] [diff] pushed to comm-aurora as http://hg.mozilla.org/releases/comm-aurora/rev/a4075036955b.
Assignee | ||
Updated•12 years ago
|
Summary: Cancelling uploads to Dropbox does not work, and is unavailable for YouSendIt → Cancelling uploads to Dropbox or YouSendIt does not work, and is unavailable after first attempt
You need to log in
before you can comment on or make changes to this bug.
Description
•