Last Comment Bug 763008 - update to YSI v3 API
: update to YSI v3 API
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: FileLink (show other bugs)
: unspecified
: x86_64 Windows 7
: -- normal with 1 vote (vote)
: Thunderbird 16.0
Assigned To: Maxim Baz
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-08 12:03 PDT by David :Bienvenu
Modified: 2013-10-13 04:34 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
fixed
fixed


Attachments
proposed fix (48.80 KB, patch)
2012-06-08 12:03 PDT, David :Bienvenu
no flags Details | Diff | Splinter Review
proposed fix corrected (61.82 KB, patch)
2012-06-12 22:44 PDT, Maxim Baz
no flags Details | Diff | Splinter Review
proposed fix corrected 2 (58.97 KB, patch)
2012-06-13 09:43 PDT, Maxim Baz
mozilla: review-
Details | Diff | Splinter Review
proposed fix corrected 3 (75.96 KB, patch)
2012-06-13 23:14 PDT, Maxim Baz
mozilla: review-
Details | Diff | Splinter Review
proposed fix corrected 4 (91.47 KB, patch)
2012-06-14 23:16 PDT, Maxim Baz
no flags Details | Diff | Splinter Review
de-bitrotted patch (73.97 KB, patch)
2012-06-15 07:54 PDT, David :Bienvenu
mozilla: review-
Details | Diff | Splinter Review
proposed fix corrected 5 (93.87 KB, patch)
2012-06-15 13:45 PDT, Maxim Baz
no flags Details | Diff | Splinter Review
de-bitrotted patch (75.33 KB, patch)
2012-06-15 16:03 PDT, David :Bienvenu
mozilla: review+
mozilla: ui‑review+
Details | Diff | Splinter Review
show how to tell the front end about the file size limit exceeded (827 bytes, patch)
2012-06-15 17:02 PDT, David :Bienvenu
no flags Details | Diff | Splinter Review
patch for old TB (47.00 KB, patch)
2012-06-15 17:55 PDT, Maxim Baz
mozilla: review+
Details | Diff | Splinter Review
patch with blank lines fixed (35.64 KB, patch)
2012-06-19 08:34 PDT, David :Bienvenu
mozilla: review+
Details | Diff | Splinter Review
same patch for new TB with fixed one string line (75.32 KB, patch)
2012-06-19 14:42 PDT, Maxim Baz
no flags Details | Diff | Splinter Review
Patch to revert to old API (76.97 KB, patch)
2012-07-06 07:54 PDT, Mike Conley (:mconley) - (needinfo me!)
no flags Details | Diff | Splinter Review
Backout patch for comm-beta (46.99 KB, patch)
2012-07-06 09:45 PDT, Mike Conley (:mconley) - (needinfo me!)
no flags Details | Diff | Splinter Review

Description David :Bienvenu 2012-06-08 12:03:30 PDT
Created attachment 631492 [details] [diff] [review]
proposed fix

YSI has a new api for folder mgmt
Comment 1 David :Bienvenu 2012-06-11 12:27:28 PDT
I've run with this patch - basic link creation seems to work fine, and the options ui looks good too. What's broken is our mozmill tests. This may be because our ysi mozmill fake server needs to be updated to deal with the new api. mconley can probably help with that if you have questions.

to see the mozmill failures, you can do the following:

cd <objdir>
make SOLO_FILE=cloudfile/test-cloudfile-backend-yousendit.js mozmill-one

we're getting the following error:

{"key":null,"id":null,"type":"BAS","policy":null,"version":"v3","password":null,
"role":null,"email":"john@example.com","firstname":"John","lastname":"User","cre
ated":null,"account":{"passwordProtect":"Pay-per-use","returnReceipt":"Pay-per-u
se","availableStorage":"2147483648","billingPlan":null,"controlExpirationDate":n
ull,"dropboxUrl":null,"knowledgeBase":"Yes","maxDownloadBWpermonth":"1073741824"
,"maxFileDownloads":"100","maxFileSize":"104857600","premiumDelivery":null,"veri
fyRecipientIdentity":"Included"},"storage":{"currentUsage":1045,"storageQuota":2
147483648},"status":null,"errorStatus":null}
2012-06-11 12:14:03     YouSendIt       INFO    user info response parsed = [obj
ect Object]
2012-06-11 12:14:03     YouSendIt       INFO    available storage = 2147482603 m
ax file size = 52428800
2012-06-11 12:14:03     YouSendIt       INFO    checkFolderID
************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'[JavaScript Error: "JSON.parse: unexpected character" {file: "fi
le:///c:/builds/tbirdhq/objdir-tb/mozilla/dist/bin/components/nsYouSendIt.js" li
ne: 543}]' when calling method: [nsIDOMEventListener::handleEvent]"  nsresult: "
0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "native fram
e :: <unknown filename> :: <TOP_LEVEL> :: line 0"  data: yes]
************************************************************
Test Failure: waitFor: Timeout exceeded for 'function () requestObserver.success

perhaps because the test code doesn't know about the new folderId command. You may want to look at mail\test\mozmill\shared-modules\test-cloudfile-yousendit-helpers.js

Also, side note, if you look at the diff, you'll see a few warnings about files not ending with a new line. You should fix those in your next patch, thx!
Comment 2 Maxim Baz 2012-06-12 22:44:01 PDT
Created attachment 632554 [details] [diff] [review]
proposed fix corrected
Comment 3 Maxim Baz 2012-06-12 22:48:19 PDT
Hello David,

Thanks for your comments. I fixed tests and new line endings, and attached new diff. Please take a look.

Thanks!

(In reply to Maxim Baz from comment #2)
> Created attachment 632554 [details] [diff] [review]
> proposed fix corrected
Comment 4 David :Bienvenu 2012-06-13 07:34:08 PDT
this patch doesn't quite apply against the trunk - the chunk starting with @@ -905,48 +1101,98 @@ nsYouSendItFileUploader.prototype = {

fails.
Comment 5 Maxim Baz 2012-06-13 09:43:17 PDT
Created attachment 632739 [details] [diff] [review]
proposed fix corrected 2

Sorry, that was my mistake. 
I attached new patch, I checked - it's applying without errors.
Comment 6 David :Bienvenu 2012-06-13 17:09:09 PDT
(In reply to Maxim Baz from comment #5)
> Created attachment 632739 [details] [diff] [review]
> proposed fix corrected 2
> 
> Sorry, that was my mistake. 
> I attached new patch, I checked - it's applying without errors.

thx, yes, that applied, and the tests pass now, thx!
Comment 7 David :Bienvenu 2012-06-13 17:22:01 PDT
Comment on attachment 632739 [details] [diff] [review]
proposed fix corrected 2

I tried attaching a file > 50MB, and I got the following error on the console:

Request] response = {"name":"Apps","id":"15625805","type":"private","size":"0","
revision":"1339342730950","folders":{"folder":[{"name":"Mozilla Thunderbird","id
":"15626766","type":"private","size":"20016","revision":"1339633107853","folders
":{"folder":[]},"parentid":"15625805","ownedByStorage":null,"currentUsage":null,
"storageQuota":null,"createdOn":"2012-06-10T08:38:51-07:00","files":{"file":[]},
"fileCount":null,"folderCount":null,"ownedByUser":null,"readable":"true","update
dOn":"2012-06-13T17:18:27-07:00","writeable":"true","status":null,"errorStatus":
null}]},"parentid":"0","ownedByStorage":null,"currentUsage":null,"storageQuota":
null,"createdOn":"2012-06-10T08:38:50-07:00","files":null,"fileCount":"0","folde
rCount":"1","ownedByUser":null,"readable":"true","updatedOn":"2012-06-10T08:38:5
0-07:00","writeable":"true","status":null,"errorStatus":null}
2012-06-13 17:21:33     YouSendIt       INFO    Looking for a child folder
2012-06-13 17:21:33     YouSendIt       INFO    saveFolderId
2012-06-13 17:21:33     YouSendIt       INFO    in cancel upload
************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'[JavaScript Error: "this._uploader is null" {file: "file:///C:/b
uilds/tbirdhq/objdir-tb/mozilla/dist/bin/components/nsYouSendIt.js" line: 288}]'
 when calling method: [nsIDOMEventListener::handleEvent]"  nsresult: "0x80570021
 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "native frame :: <unkn
own filename> :: <TOP_LEVEL> :: line 0"  data: yes]
************************************************************

so I couldn't see the upgrade UI...
Comment 8 Maxim Baz 2012-06-13 23:14:38 PDT
Created attachment 633039 [details] [diff] [review]
proposed fix corrected 3

It's strange, anyway sorry for that!
I checked that part of code more carefully...and attached new fix, please review it.
Comment 9 David :Bienvenu 2012-06-14 16:31:09 PDT
Maxim, I tried attaching a 63 MB file with the new patch - now I don't see any error on the console, but it doesn't seem to be uploading, and the user doesn't see any error either.
Comment 10 David :Bienvenu 2012-06-14 16:33:30 PDT
smaller uploads fail as well, in the same way.
Comment 11 David :Bienvenu 2012-06-14 16:34:34 PDT
Comment on attachment 633039 [details] [diff] [review]
proposed fix corrected 3

minus based on upload not working for me...
Comment 12 Maxim Baz 2012-06-14 23:16:34 PDT
Created attachment 633408 [details] [diff] [review]
proposed fix corrected 4

I updated jar.mn files, now everything should be OK.
Thank you, David, for helping with this issue.
Comment 13 David :Bienvenu 2012-06-15 07:54:29 PDT
Created attachment 633526 [details] [diff] [review]
de-bitrotted patch

Maxim's patch had bit-rotted slightly. Building with this patch now.
Comment 14 David :Bienvenu 2012-06-15 12:39:54 PDT
Comment on attachment 633526 [details] [diff] [review]
de-bitrotted patch

cool, this patch works for me...requesting ui review.
Comment 15 David :Bienvenu 2012-06-15 13:07:34 PDT
Comment on attachment 633526 [details] [diff] [review]
de-bitrotted patch

you can use let instead of var here:
+  if ("wrappedJSObject" in window.arguments[0]) {
+    var storage = parseInt(window.arguments[0].wrappedJSObject.storage);
+    storage = (storage / 1024 / 1024 / 1024).toFixed(2);
+    var currentStorage = document.getElementById('currentStorage');
+    currentStorage.textContent = currentStorage.textContent.replace('#XXX', storage);


You don't need the braces here (mozilla style is mostly not to include unneeded braces)
+    if (aFile.fileSize > 2147483648) {
+      return this._fileExceedsLimit(aCallback, '2GB', 0);
+    }
+    if (aFile.fileSize > this._maxFileSize) {
+      return this._fileExceedsLimit(aCallback, 'Limit', 0);
+    }
+    if (aFile.fileSize > this._availableStorage) {
+      return this._fileExceedsLimit(aCallback, 'Quota', 
+                                    aFile.fileSize + this._fileSpaceUsed);
+    }

let here as well:

+    var args = {storage: aStorageSize};
+    args.wrappedJSObject = args;

is the + 0 really needed here?

+          this._fileSpaceUsed = parseInt(storage.currentUsage) + 0;
+          this._availableStorage = parseInt(storage.storageQuota) - this._fileSpaceUsed;

this doesn't look right - either the indentation is wrong, or the braces are wrong:
+      if (req.status >= 200 && req.status < 400) {
+        this.log.info("request status = " + req + " response = " +
+                      req.responseText);
+		
+      if(aFoundCallback && docResponse.folders)
+        aFoundCallback(docResponse.folders);
+      }

same thing here:
+      if (req.status >= 200 && req.status < 400) {
+        this.log.info("request status = " + req + " response = " +
+                      req.responseText);
+		
+      if(aSuccessCallback)
+        aSuccessCallback(docResponse.id)
+      }
+

you can use the ? operator here instead of if else:

+        if (this.file.leafName.length > 120) 
+          this.callback(this.requestObserver, Ci.nsIMsgCloudFileProvider.uploadExceedsFileNameLimit);
+        else
+          this.callback(this.requestObserver, Ci.nsIMsgCloudFileProvider.uploadErr);

space after for here:
+    for(let i in this._foldersId) {
+      this._server.registerPathHandler(kFolderPath + this._foldersId[i],
+                                       null);
+    }

all tests pass, and this is looking fairly good. But minusing for the above comments.
Comment 16 Maxim Baz 2012-06-15 13:45:25 PDT
Created attachment 633656 [details] [diff] [review]
proposed fix corrected 5

Thanks, David. 
I fixed all problems you told me and some similar problems as well.
Can you review new patch?
Thanks!
Comment 17 David :Bienvenu 2012-06-15 16:03:07 PDT
Created attachment 633712 [details] [diff] [review]
de-bitrotted patch

de-bitrotted patch - Maxim, you should update your mercurial repo to pick up the latest changes, so your patches will apply cleanly going forward.
Comment 18 David :Bienvenu 2012-06-15 17:02:29 PDT
Created attachment 633725 [details] [diff] [review]
show how to tell the front end about the file size limit exceeded

Maxim, this just shows you how to tell the compose window front end about the file size limit exceeded error and have it put up its own message. It's not a useful patch, but I said I'd put it in the bug, so I did. Per my e-mail, this is what you'd do:

   The way it works is that the YSI file link module would return either uploadWouldExceedQuota or uploadExceedsFileLimit errors. And, it would put the appropriate urls for those errors in the method providerUrlForError. If you do that, Thunderbird will put up a prompt asking the user if they want to upgrade, and if the user says yes, we'll open the default browser with the url you provide.
Comment 19 David :Bienvenu 2012-06-15 17:13:06 PDT
Comment on attachment 633712 [details] [diff] [review]
de-bitrotted patch

this looks ok to me; sending over to Blake for UI review.

One thought is that we might want to use this prettier UI in a more general way, if possible. This would entail invoking it from MsgComposeCommands.js instead of nsYouSendIt.js. But I'll let you and Blake talk about that...you're certainly free to do your own UI.
Comment 20 Maxim Baz 2012-06-15 17:55:32 PDT
Created attachment 633741 [details] [diff] [review]
patch for old TB

I attached patch for old TB. 
Removed all unnecessary files.

By the way, I don't know what "de-bitrotted" means, but I always pull changes and update repo before creating patch. And when I created previous 2 patches, there were no updates in repo...
Comment 21 David :Bienvenu 2012-06-18 17:33:03 PDT
Comment on attachment 633741 [details] [diff] [review]
patch for old TB

getting on my review radar
Comment 22 David :Bienvenu 2012-06-19 08:33:28 PDT
Comment on attachment 633741 [details] [diff] [review]
patch for old TB

this seems to work fine. There are a couple blanks lines with spaces on them. I'll upload a version with that fixed in a minute.
Comment 23 David :Bienvenu 2012-06-19 08:34:31 PDT
Created attachment 634439 [details] [diff] [review]
patch with blank lines fixed

this is ready to land, though I'd like to land the trunk patch first just to get a little more testing, if possible.
Comment 24 David :Bienvenu 2012-06-19 13:06:23 PDT
Comment on attachment 633712 [details] [diff] [review]
de-bitrotted patch

Blake says he likes it.
Comment 25 David :Bienvenu 2012-06-19 13:40:48 PDT
landed on trunk - http://hg.mozilla.org/comm-central/rev/ee9bf9095b6e
landed on beta - http://hg.mozilla.org/releases/comm-beta/rev/40dbdb482fac

I'll land on Aurora next.
Comment 26 David :Bienvenu 2012-06-19 13:55:16 PDT
aurora - http://hg.mozilla.org/releases/comm-aurora/rev/b6fc291dba94
Comment 27 Hiroyuki Ikezoe (:hiro) 2012-06-19 14:20:01 PDT
Got an error on Linux:
RuntimeError: File "mail/cloudfile/YouSendIt/check.png" not found in /home/zoe/hg/comm-central/mail/themes/gnomestripe, /home/zoe/hg/comm-central/objdir-thunderbird/mail/themes/gnomestripe
Comment 28 David :Bienvenu 2012-06-19 14:20:50 PDT
landed missing check.png on trunk - http://hg.mozilla.org/comm-central/rev/4468b7bdbd88
Comment 29 Hiroyuki Ikezoe (:hiro) 2012-06-19 14:22:10 PDT
Thanks, now the file is coming.
Comment 30 Maxim Baz 2012-06-19 14:42:09 PDT
Created attachment 634598 [details] [diff] [review]
same patch for new TB with fixed one string line

I attached the same patch for new TB, which already passed review, with change in single text line.
Comment 31 David :Bienvenu 2012-06-19 14:59:40 PDT
http://hg.mozilla.org/comm-central/rev/c22b706b16e2 landed for string change. Since this has only been checked in for an hour, I didn't change the string id, which normally we do when the meaning of a string changes.
Comment 32 Théo Chevalier [:tchevalier] 2012-06-20 03:43:50 PDT
Is there an error in "errorCloudFileNameLimit.message=Uploading %2$S to %1$S contains has more than 120 characters in its name." ? 

"%2$S contains more than 120 characters in its name." seems to have more sense, but maybe I just don't understand the original sentence.
Comment 33 Mike Conley (:mconley) - (needinfo me!) 2012-07-06 07:54:54 PDT
Created attachment 639670 [details] [diff] [review]
Patch to revert to old API

In the event that the v3 API bugs (bug 771073, bug 771131, bug 771132) cannot be resolved, we'll need to back this out.  This patch will reverse these changes.
Comment 34 Mike Conley (:mconley) - (needinfo me!) 2012-07-06 07:58:17 PDT
Comment on attachment 639670 [details] [diff] [review]
Patch to revert to old API

Hrm, so this doesn't apply on comm-beta - give me a sec to re-tool this.
Comment 35 Mike Conley (:mconley) - (needinfo me!) 2012-07-06 09:45:07 PDT
Created attachment 639704 [details] [diff] [review]
Backout patch for comm-beta

This patch applies cleanly on top of comm-beta, and will revert us back to the old YSI API.

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