Closed
Bug 744037
Opened 12 years ago
Closed 12 years ago
Add UbuntuOne support for Filelink
Categories
(Thunderbird :: FileLink, defect)
Tracking
(thunderbird14 fixed, thunderbird15 fixed)
RESOLVED
FIXED
Thunderbird 15.0
People
(Reporter: mconley, Assigned: james)
References
Details
Attachments
(4 files, 5 obsolete files)
64.40 KB,
patch
|
Details | Diff | Splinter Review | |
3.77 KB,
patch
|
Bienvenu
:
review+
|
Details | Diff | Splinter Review |
62.48 KB,
patch
|
Bienvenu
:
review+
Bienvenu
:
approval-comm-beta+
|
Details | Diff | Splinter Review |
812 bytes,
patch
|
Bienvenu
:
review+
standard8
:
approval-comm-aurora+
standard8
:
approval-comm-beta+
|
Details | Diff | Splinter Review |
We're aiming to support UbuntuOne with the Filelink feature.
Comment 1•12 years ago
|
||
Attachment #616588 -
Flags: review?(dbienvenu)
Comment 2•12 years ago
|
||
This fixes a bit rot issue with the previous patch, and makes uploading files that we don't know the mime type of work (it was failing because the code wasn't catching the exception, so I've fixed that). I've also fixed the offline error to use throw. With those changes, I was able to sign up for a UbuntuOne account and send a filelink, so the basics seem to work.
Attachment #616588 -
Attachment is obsolete: true
Attachment #616588 -
Flags: review?(dbienvenu)
Attachment #617120 -
Flags: review?(dbienvenu)
Assignee | ||
Comment 3•12 years ago
|
||
Here is my latest work on the Ubuntu One backend, squashed down to a single changeset and rebased on tip. I'm just rebuilding to test the backend, so if I discover any issues I'll post an updated patch. I hope I've incorporated the offlineErr changes from David's update correctly. This version also includes mozmill tests, giving a similar level of coverage as the other backends. One thing missing from the patch at the moment is the referral codes in the links. Once we've sorted out what they should be, I'll post an update.
Assignee | ||
Comment 4•12 years ago
|
||
I noticed that the try/catch hadn't been added to _uploaderCallback() as part of the offline error handling, so I've added that. This seems to have all of David's changes, so this is the patch to review. As with the last patch, it still needs the referral codes in some of the links, but that shouldn't prevent the code itself from being reviewed.
Attachment #617120 -
Attachment is obsolete: true
Attachment #617224 -
Attachment is obsolete: true
Attachment #617120 -
Flags: review?(dbienvenu)
Attachment #617270 -
Flags: review?(dbienvenu)
Updated•12 years ago
|
Assignee: nobody → james
Status: NEW → ASSIGNED
See Also: → https://launchpad.net/bugs/991033
Comment 5•12 years ago
|
||
Comment on attachment 617270 [details] [diff] [review] Fix offline handling in _uploaderCallback overall, this is looking pretty good. I'll work on reviewing it over the next couple days.
Comment 6•12 years ago
|
||
Comment on attachment 617270 [details] [diff] [review] Fix offline handling in _uploaderCallback sorry for the delay in getting to this: these don't seem to be used: + _authToken: "", + _lastErrorStatus : 0, + _lastErrorText : "", I tried uploading a 43MB file - it never seemed to finish, even after waiting several minutes. A 300K message finished in a few seconds. A 9MB attachment failed after about 3 minutes. This might be something with my network, but it worked with other providers in about 20 seconds. The code looks good, in general. I tried changing my password on the UbuntuOne site, removed my auth token from the password manager, and my next attempt to upload caused a password prompt in TB, so that's good. Changing my password maybe should invalidate my auth token, but that's something to worry about on the server side... I'll run through the mozmill tests in a minute. You'll want a ui review from Blake. The one thing I noticed is that during account setup, once I enter an e-mail address, the "get new account link" mostly disappears, but not quite, so it looks a bit off. I notice you have new strings in management.dtd and settings.dtd. We're way past string freeze for TB 14, so we'll need to think about what to do there.
Attachment #617270 -
Flags: ui-review?(bwinton)
Comment 7•12 years ago
|
||
Comment on attachment 617270 [details] [diff] [review] Fix offline handling in _uploaderCallback self doesn't seem to be used here either: + aOptions = this._overrideDefault(kDefaultReturnHeader, aOptions); + let self = this; or + let subjectString = Cc["@mozilla.org/supports-string;1"] + .createInstance(Ci.nsISupportsString); + subjectString.data = aString; but other than that and the previous comment, this looks OK. The failure to upload large files worries me, so we should figure that out.
Attachment #617270 -
Flags: review?(dbienvenu) → review+
Comment 8•12 years ago
|
||
Comment on attachment 617270 [details] [diff] [review] Fix offline handling in _uploaderCallback For the first screen, I would prefer things to line up with themselves and with the other elements on the screen, similarly to the way they do in http://is.gd/AjY10y (That also adds a "Learn More…" link which you can redirect to an Ubuntu One page.) Other than that, I'm pretty happy with it, so ui-r=me with those fixed. Thanks, Blake.
Attachment #617270 -
Flags: ui-review?(bwinton) → ui-review+
Reporter | ||
Comment 9•12 years ago
|
||
I believe this patch suffers from the same problem that our YouSendIt implementation did with forcing auth prompts to spawn from compose windows (see https://bugzilla.mozilla.org/show_bug.cgi?id=756239) The same fix in that bug should be applied to this patch.
Assignee | ||
Comment 10•12 years ago
|
||
Here is an updated patch based on the review comments: - I've updated to use the settings form to use addAccountDialog.css so that it matches the design Blake requested. - I've included the fix for the password dialog parent window from bug 756239. - I've removed the unused variables in the main code and tests. These sections of code were adapted from the Dropbox backend, so it could probably make the same changes. As far as the string freeze goes, only the &ubuntuOneMgmt.viewSettings; string ("View my account settings on one.ubuntu.com") is specific to Ubuntu One. The settings ones could probably reuse the YouSendIt strings if needed: while we ask for an email address, people would probably know what was required if we asked for a user name. If anyone has any better suggestions, I'm open to them.
Attachment #617270 -
Attachment is obsolete: true
Attachment #625926 -
Flags: ui-review?(bwinton)
Attachment #625926 -
Flags: review?(dbienvenu)
Assignee | ||
Comment 11•12 years ago
|
||
I forgot to add: we do have some engineers looking into the problem with the larger file uploads through the REST API this backend is using. They have identified the problem, so it should hopefully be fixed soon.
Comment 12•12 years ago
|
||
Comment on attachment 625926 [details] [diff] [review] Updated patch based on review comments You don't usually need to request a new review if a review has been granted with just small comments needing to be addressed. I'll try to land this on the trunk in the next day or so, and we'll try to figure out what to do about the strings for the alpha branch.
Attachment #625926 -
Flags: ui-review?(bwinton)
Attachment #625926 -
Flags: review?(dbienvenu)
Comment 13•12 years ago
|
||
pushed to trunk, thx, James! - http://hg.mozilla.org/comm-central/rev/436d34c9eba1 Now we have to figure out what to do about the strings. Standard8, is it crazy to do late l10n on the couple new strings and land this for aurora? Or should we steal some strings and hack up an aurora-only patch?
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 15.0
Comment 14•12 years ago
|
||
(In reply to David :Bienvenu from comment #13) > pushed to trunk, thx, James! - > http://hg.mozilla.org/comm-central/rev/436d34c9eba1 > > Now we have to figure out what to do about the strings. Standard8, is it > crazy to do late l10n on the couple new strings and land this for aurora? Or > should we steal some strings and hack up an aurora-only patch? Yes, we can't do l10n changes on aurora. So if we can find other strings then we'll have to go with that.
Comment 15•12 years ago
|
||
OK, James, then you should pull a comm-aurora repository (hg.mozilla.org/comm-aurora), apply your patch, minus the new strings, and try embedding the existing yousendit strings, or whatever other strings you can steal, and propose a patch for aurora (alpha). Thx!
Assignee | ||
Comment 16•12 years ago
|
||
This is another patch for trunk: It updates the links to https://one.ubuntu.com/ to include a referral code so that we can track the subscriptions that come from Thunderbird users. I'll have the Aurora patch ready shortly, which will include this change too.
Comment 17•12 years ago
|
||
Comment on attachment 626731 [details] [diff] [review] Add referral codes to one.ubuntu.com links this looks reasonable. I can land this on the trunk later today.
Attachment #626731 -
Flags: review+
Comment 18•12 years ago
|
||
referral codes landed on trunk - http://hg.mozilla.org/comm-central/rev/0bc9fd424556
Assignee | ||
Comment 19•12 years ago
|
||
Here is a patch against comm-aurora, containing the changes from the two revisions from trunk. The Aurora specific changes include: * For the management panel, use the "View settings for this account" string from msgAccountCentral.dtd in place of "View my account settings on one.ubuntu.com". * For the account setup panel, use the "Learn more" and "Need an account?" strings from the YouSendIt backend, and the "Email address:" string from accountCreation.dtd I believe I've maintained the intent of these borrowed strings, so the translations should make sense in this new context.
Assignee | ||
Updated•12 years ago
|
Attachment #627112 -
Flags: review?(dbienvenu)
Comment 20•12 years ago
|
||
Attachment #627112 -
Attachment is obsolete: true
Attachment #627112 -
Flags: review?(dbienvenu)
Attachment #632287 -
Flags: review?(dbienvenu)
Updated•12 years ago
|
Attachment #632287 -
Flags: review?(dbienvenu)
Attachment #632287 -
Flags: review+
Attachment #632287 -
Flags: approval-comm-beta+
Comment 21•12 years ago
|
||
pushed to beta - http://hg.mozilla.org/releases/comm-beta/rev/7e5fd53cc408
Comment 22•12 years ago
|
||
James, as pointed out by Florian, this looks like a typo: http://mxr.mozilla.org/comm-central/source/mail/components/cloudfile/nsUbuntuOne.js#502 4:26:17 PM - florian: ".bind(this). this)" 4:26:54 PM - florian: I would expect a comma there; and I don't understand how this can be […]
Assignee | ||
Comment 23•12 years ago
|
||
Here is a patch to fix the problem Florian discovered. I guess the typo would have resulted in me passing "undefined" as the error callback to doXHRequest().
Attachment #633940 -
Flags: review?(dbienvenu)
Updated•12 years ago
|
Assignee: james → nobody
Component: Message Compose Window → FileLink
QA Contact: message-compose → filelink
Updated•12 years ago
|
Assignee: nobody → james
Updated•12 years ago
|
Attachment #633940 -
Flags: review?(dbienvenu) → review+
Comment 24•12 years ago
|
||
Comment on attachment 633940 [details] [diff] [review] Fix the typo in _acquireToken() that Florian pointed out Checked in: https://hg.mozilla.org/comm-central/rev/13c4be7e7d31 We'll also want this on aurora and beta as that's where the rest of the code has landed.
Attachment #633940 -
Flags: approval-comm-beta+
Attachment #633940 -
Flags: approval-comm-aurora+
Comment 25•12 years ago
|
||
Also in aurora and beta: https://hg.mozilla.org/releases/comm-aurora/rev/25662480aa1e https://hg.mozilla.org/releases/comm-beta/rev/c7243ee102fc
Updated•12 years ago
|
status-thunderbird14:
--- → fixed
status-thunderbird15:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•