Last Comment Bug 744037 - Add UbuntuOne support for Filelink
: Add UbuntuOne support for Filelink
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: FileLink (show other bugs)
: Trunk
: x86 All
: -- normal with 3 votes (vote)
: Thunderbird 15.0
Assigned To: James Henstridge
:
Mentors:
Depends on: 764312 785062
Blocks: BigFiles
  Show dependency treegraph
 
Reported: 2012-04-10 09:23 PDT by Mike Conley (:mconley) - (Away until June 29th)
Modified: 2012-08-23 06:56 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
fixed
fixed


Attachments
initial patch from Ubuntu (44.95 KB, patch)
2012-04-19 08:35 PDT, David :Bienvenu
no flags Details | Diff | Review
fix a few issues (42.21 KB, patch)
2012-04-20 15:27 PDT, David :Bienvenu
no flags Details | Diff | Review
The latest version of my work (64.21 KB, patch)
2012-04-21 07:57 PDT, James Henstridge
no flags Details | Diff | Review
Fix offline handling in _uploaderCallback (64.48 KB, patch)
2012-04-21 19:02 PDT, James Henstridge
mozilla: review+
bwinton: ui‑review+
Details | Diff | Review
Updated patch based on review comments (64.40 KB, patch)
2012-05-22 01:30 PDT, James Henstridge
no flags Details | Diff | Review
Add referral codes to one.ubuntu.com links (3.77 KB, patch)
2012-05-24 01:39 PDT, James Henstridge
mozilla: review+
Details | Diff | Review
cloudfiles-ubuntuone-aurora.patch (62.54 KB, patch)
2012-05-24 23:43 PDT, James Henstridge
no flags Details | Diff | Review
de-bitrotted patch for beta (62.48 KB, patch)
2012-06-12 09:20 PDT, David :Bienvenu
mozilla: review+
mozilla: approval‑comm‑beta+
Details | Diff | Review
Fix the typo in _acquireToken() that Florian pointed out (812 bytes, patch)
2012-06-17 20:05 PDT, James Henstridge
mozilla: review+
standard8: approval‑comm‑aurora+
standard8: approval‑comm‑beta+
Details | Diff | Review

Description Mike Conley (:mconley) - (Away until June 29th) 2012-04-10 09:23:41 PDT
We're aiming to support UbuntuOne with the Filelink feature.
Comment 1 David :Bienvenu 2012-04-19 08:35:52 PDT
Created attachment 616588 [details] [diff] [review]
initial patch from Ubuntu
Comment 2 David :Bienvenu 2012-04-20 15:27:20 PDT
Created attachment 617120 [details] [diff] [review]
fix a few issues

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.
Comment 3 James Henstridge 2012-04-21 07:57:51 PDT
Created attachment 617224 [details] [diff] [review]
The latest version of my work

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.
Comment 4 James Henstridge 2012-04-21 19:02:29 PDT
Created attachment 617270 [details] [diff] [review]
Fix offline handling in _uploaderCallback

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.
Comment 5 David :Bienvenu 2012-05-15 15:37:17 PDT
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 David :Bienvenu 2012-05-17 08:06:21 PDT
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.
Comment 7 David :Bienvenu 2012-05-17 08:51:34 PDT
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.
Comment 8 Blake Winton (:bwinton) (:☕️) (PTO 'til London. Find me there for quick answers!) 2012-05-17 12:55:52 PDT
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.
Comment 9 Mike Conley (:mconley) - (Away until June 29th) 2012-05-18 06:54:48 PDT
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.
Comment 10 James Henstridge 2012-05-22 01:30:56 PDT
Created attachment 625926 [details] [diff] [review]
Updated patch based on review comments

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.
Comment 11 James Henstridge 2012-05-22 02:35:31 PDT
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 David :Bienvenu 2012-05-22 10:18:20 PDT
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.
Comment 13 David :Bienvenu 2012-05-22 11:01:43 PDT
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?
Comment 14 Mark Banner (:standard8) 2012-05-23 04:11:53 PDT
(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 David :Bienvenu 2012-05-23 07:21:57 PDT
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!
Comment 16 James Henstridge 2012-05-24 01:39:28 PDT
Created attachment 626731 [details] [diff] [review]
Add referral codes to one.ubuntu.com links

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 David :Bienvenu 2012-05-24 08:11:18 PDT
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.
Comment 18 David :Bienvenu 2012-05-24 14:30:20 PDT
referral codes landed on trunk - http://hg.mozilla.org/comm-central/rev/0bc9fd424556
Comment 19 James Henstridge 2012-05-24 23:43:13 PDT
Created attachment 627112 [details] [diff] [review]
cloudfiles-ubuntuone-aurora.patch

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.
Comment 20 David :Bienvenu 2012-06-12 09:20:03 PDT
Created attachment 632287 [details] [diff] [review]
de-bitrotted patch for beta
Comment 21 David :Bienvenu 2012-06-12 10:23:22 PDT
pushed to beta - http://hg.mozilla.org/releases/comm-beta/rev/7e5fd53cc408
Comment 22 David :Bienvenu 2012-06-14 16:28:55 PDT
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  […]
Comment 23 James Henstridge 2012-06-17 20:05:24 PDT
Created attachment 633940 [details] [diff] [review]
Fix the typo in _acquireToken() that Florian pointed out

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().
Comment 24 Mark Banner (:standard8) 2012-06-19 13:13:19 PDT
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.

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