Last Comment Bug 767327 - Add box.com to file link services
: Add box.com to file link services
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: FileLink (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Thunderbird 17.0
Assigned To: Brian King [:kinger]
:
:
Mentors:
Depends on: 782788 783277 782947 783267
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-22 05:05 PDT by Brian King [:kinger]
Modified: 2014-09-03 06:07 PDT (History)
7 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
fixed


Attachments
Patch v1 [incomplete] (51.61 KB, patch)
2012-07-13 14:57 PDT, Brian King [:kinger]
mconley: review-
Details | Diff | Splinter Review
Loading image (2.55 KB, image/gif)
2012-07-13 14:59 PDT, Brian King [:kinger]
no flags Details
Box logo (766 bytes, image/png)
2012-07-13 15:01 PDT, Brian King [:kinger]
no flags Details
Patch v2 [incomplete] (60.48 KB, patch)
2012-07-15 14:16 PDT, Brian King [:kinger]
no flags Details | Diff | Splinter Review
Disabled Box support patch [checked-in] (59.57 KB, patch)
2012-07-16 11:29 PDT, Mike Conley (:mconley)
no flags Details | Diff | Splinter Review
Upload / Delete patch (19.05 KB, patch)
2012-07-20 15:34 PDT, Brian King [:kinger]
no flags Details | Diff | Splinter Review
RC1 Patch (29.57 KB, patch)
2012-07-23 22:57 PDT, Brian King [:kinger]
mconley: review-
Details | Diff | Splinter Review
RC2 Patch (43.97 KB, patch)
2012-07-31 07:19 PDT, Brian King [:kinger]
mconley: review-
Details | Diff | Splinter Review
New progress load icon (1.81 KB, image/gif)
2012-08-06 07:13 PDT, Brian King [:kinger]
no flags Details
RC3 Patch (48.37 KB, patch)
2012-08-06 07:17 PDT, Brian King [:kinger]
mconley: review-
Details | Diff | Splinter Review
RC4 Patch (49.12 KB, patch)
2012-08-13 14:45 PDT, Brian King [:kinger]
mconley: review+
Details | Diff | Splinter Review
RC5 Patch - ready for check in (50.74 KB, patch)
2012-08-14 14:23 PDT, Brian King [:kinger]
mconley: review+
bwinton: approval‑comm‑aurora+
Details | Diff | Splinter Review

Description Brian King [:kinger] 2012-06-22 05:05:14 PDT
Add box.com to file link services, integrated by default into Thunderbird.
Comment 1 Mike Conley (:mconley) 2012-07-09 07:12:51 PDT
Brian:

Can you post whatever you've already got so I can start poking at your patch, and giving you early feedback?

Thanks,

-Mike
Comment 2 Brian King [:kinger] 2012-07-10 10:48:33 PDT
I'm scrambling to make a patch after holidays and a very busy Mozilla Reps Camp. Apologies for the delay.
Comment 3 Jb Piacentino 2012-07-11 05:54:33 PDT
Brian, time is running out. Box's code needs to be reviewed before getting a chance of landing for TB 16... and we're merging on Monday :(
Comment 4 Mike Conley (:mconley) 2012-07-13 11:59:03 PDT
Brian:

Please post whatever you already have, regardless of its state. I'd like to get familiar with it.

-Mike
Comment 5 Brian King [:kinger] 2012-07-13 14:57:28 PDT
Created attachment 642071 [details] [diff] [review]
Patch v1 [incomplete]

Mike,

Here is my first version of the patch, incomplete.

I would split this into 3 features:
1. Authorization
2. Management/Settings
3. Upload

1 + 2 are done mostly, upload not yet.

WRT 2, there is one bug I have currently where you need to log in twice for the settings to load. This goes for after initial login, and also when the TB Prefs window opens.

There are also a few strings hard-coded. What would be the deadline to get new strings in ... same as the merge?
Comment 6 Brian King [:kinger] 2012-07-13 14:59:58 PDT
Created attachment 642073 [details]
Loading image

The rather crude loading image I show when the page is waiting to load in the Auth window. If you have a more elegant solution, let me know.

If the patch doesn't apply and insert this image, it does in components/cloudfile/content/Box/
Comment 7 Brian King [:kinger] 2012-07-13 15:01:48 PDT
Created attachment 642074 [details]
Box logo

The Box logo icon to be used.

If the patch doesn't apply and insert this image, it goes in the mail/icons/ folder of each theme.
Comment 8 Mike Conley (:mconley) 2012-07-14 12:48:29 PDT
Comment on attachment 642071 [details] [diff] [review]
Patch v1 [incomplete]

Review of attachment 642071 [details] [diff] [review]:
-----------------------------------------------------------------

Brian:

Most of what you're doing seems pretty reasonable. I know this is a WIP, so I haven't commented too much on style / blocks of commented out code, etc.

There are some glaring missing pieces, however:

1) The uploader does not appear to be implemented, and this is obviously critical.
2) The ability to delete files does not appear to be implemented. We'll be needing that too.

I also noticed that it doesn't seem like you're caching the auth token, so each interaction with Box.com requires us to do the auth dance again.

Anyhow, thanks for putting this up early though - it's good to get familiar before I do a full review of what you want to land. Please continue to update this patch with your work as it gets done.

I've made a few passing remarks below. Let me know if you have any questions,

-Mike

::: mail/components/cloudfile/cloudFileComponents.manifest
@@ +5,5 @@
>  component {9a44742b-a7b1-4f44-919e-6f3b28902c1a} nsUbuntuOne.js
>  contract @mozilla.org/mail/ubuntuone;1 {9a44742b-a7b1-4f44-919e-6f3b28902c1a}
>  category cloud-files UbuntuOne @mozilla.org/mail/ubuntuone;1
> +
> +component {B65F80EA-514A-4018-A009-128F627BEFC8} nsBox.js

Please re-generate this UUID using lowercase characters.

::: mail/components/cloudfile/content/Box/auth.js
@@ +61,5 @@
> +                              wpl.STATE_IS_INSECURE |
> +                              wpl.STATE_SECURE_HIGH |
> +                              wpl.STATE_SECURE_MED |
> +                              wpl.STATE_SECURE_LOW;
> +    var browser = document.getElementById("requestFrame");

We prefer 'let' over 'var'.

@@ +106,5 @@
> +                               loadRequestedUrl(authUrl);
> +                             },
> +                             function (aReq) {
> +                               alert("get_ticket failed - status = " + aReq.status);
> +                               // XX TODO Handle this some way in the auth window

If you can get the auth window to re-request the ticket, you might as well keep it open and handle it in there.

That'd probably be better than kicking out back to the previous dialog, since it requires less user intervention.

@@ +147,5 @@
> +}
> +
> +var nsBoxAuth = {
> +
> +  kApiKey : "exs8m0agj1fa5728lxvn288ymz01dnzn",

This key, along with the box.com URLs in this file, should all be declared as consts, and put somewhere near the top of the file.

@@ +180,5 @@
> +                      this.log.info("Auth ticket = " + ticket);
> +                      successCallback(ticket);
> +                    }
> +                    else {
> +                      // failureCallback("", aResponseText, aRequest); ??

We should indeed fail in this case.

::: mail/components/cloudfile/content/Box/auth.xul
@@ +13,5 @@
> +        xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
> +        buttons=","
> +        onload="onLoad()"
> +        onclose="reportUserClosed()"
> +        title="Box Authentication"

This needs to be l10n-able.

::: mail/components/cloudfile/content/Box/management.xhtml
@@ +43,5 @@
> +        </div>
> +        <div id="upgrade"><a href="https://www.box.com/pricing/">&cloudfileMgmt.upgradeOffer;</a></div>
> +      </div>
> +      </div>
> +    <div id="provider-account-settings"><a href="https://www.box.com/settings">View my account settings on box.com</a></div>

This string will need to be l10n-able.

::: mail/components/cloudfile/content/Box/settings.js
@@ +3,5 @@
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/*function extraArgs() {
> +  var usernameValue = document.getElementById("username").value;
> +  return {

This function can be excised.

@@ +10,5 @@
> +}*/
> +
> +Cu.import("resource://gre/modules/Services.jsm");
> +
> +function setupAccount() {

From what I can tell, this function doesn't seem to be called - so it can be removed, along with the import.

::: mail/components/cloudfile/content/Box/settings.xhtml
@@ +13,5 @@
> +        href="chrome://messenger/skin/cloudfile/addAccountDialog.css" />
> +  </head>
> +  <body id="provider-settings">
> +    <div id="learn-more">
> +      <a href="https://www.box.com/register">Get a Box account...</a>

This string will need to be l10n-able.

::: mail/components/cloudfile/nsBox.js
@@ +84,5 @@
> +  init: function nsBox_init(aAccountKey) {
> +    this._accountKey = aAccountKey;
> +    this._prefBranch = Services.prefs.getBranch("mail.cloud_files.accounts." +
> +                                                aAccountKey + ".");
> +    //this._loggedIn = this._cachedAuthToken != "";

Why is this commented out?

The auth system, with this patch, is slightly busted in that it seems I need to auth *each time* I try to interact with my account. Is the auth token not being cached?

@@ +117,5 @@
> +      this._uploader = null;
> +  },
> +
> +  /**
> +   * Attempt to upload a file to YouSendIt's servers.

should be Box.com, not YSI.

@@ +214,5 @@
> +   * @param failureCallback a callback fired if retrieving profile information
> +   *                        fails.
> +   */
> +  _getUserInfo: function nsBox__getUserInfo(successCallback, failureCallback) {
> +    this.log.info("Getting User Info 2");

Why "2"?

@@ +241,5 @@
> +                function(aResponseText, aRequest) {
> +                  this.log.info("get_account_info request response = " + aResponseText);
> +
> +                  let docResponse = aRequest.responseXML.documentElement;
> +                  if (docResponse && docResponse.nodeName == "response") {

I forget - does this account for the case where what we get back from the server is not valid XML?

@@ +242,5 @@
> +                  this.log.info("get_account_info request response = " + aResponseText);
> +
> +                  let docResponse = aRequest.responseXML.documentElement;
> +                  if (docResponse && docResponse.nodeName == "response") {
> +                    let docStatus = docResponse.firstChild.firstChild.nodeValue;

Do we account for the case where the XML is not structured in a way that we expect? Where the first child of the document has no children?

@@ +254,5 @@
> +
> +                    /*<user>
> +                      <login>example@example.com</login>
> +                      <email>example@example.com</email>
> +                      <access_id>123456789</access_id>

This block should be removed, or if it's important, given some additional commentary to explain its usefulness.

@@ +327,5 @@
> +   * @param aCallback an nsIRequestObserver for observing the starting and
> +   *                  ending states of the request.
> +   */
> +  refreshUserInfo: function nsBox_refreshUserInfo(aWithUI, aCallback) {
> +    this.log.info("Getting User Info 1 : " + this._loggedIn);

Why "1"?

@@ +405,5 @@
> +   * @param aCallback an nsIRequestObserver for monitoring the start and stop
> +   *                  states of the delete procedure.
> +   */
> +  deleteFile: function nsBox_deleteFile(aFile, aCallback) {
> +    return Cr.NS_ERROR_NOT_IMPLEMENTED;

Deleting files is not possible, or have you just not reached that point yet?

@@ +408,5 @@
> +  deleteFile: function nsBox_deleteFile(aFile, aCallback) {
> +    return Cr.NS_ERROR_NOT_IMPLEMENTED;
> +  },
> +
> +  _getUrlParameter: function nsBox_getAuthParameter(aUrl, aName)

It's a bit unnerving that you have to implement this kind of function yourself. Are you certain that there's no internal XPCOM or Javascript function to do this for you?

@@ +544,5 @@
> +  this.callback = aCallback;
> +  this.requestObserver = aRequestObserver;
> +}
> +
> +nsBoxFileUploader.prototype = {

This object definitely needs finishing before it can land.
Comment 9 Brian King [:kinger] 2012-07-15 14:16:05 PDT
Created attachment 642420 [details] [diff] [review]
Patch v2 [incomplete]

Mike,

Here is a new patch, with most of your review comments fixed. I'll follow up on what is not.

Note file deletion and folder creaation (files are to be uploaded to a Thunderbird/ folder) are untested.

Actual uploading, moving of the bits, is in the works.
Comment 10 Brian King [:kinger] 2012-07-15 14:21:09 PDT
(In reply to Mike Conley (:mconley) from comment #8)
> @@ +106,5 @@
> > +                               loadRequestedUrl(authUrl);
> > +                             },
> > +                             function (aReq) {
> > +                               alert("get_ticket failed - status = " + aReq.status);
> > +                               // XX TODO Handle this some way in the auth window
> 
> If you can get the auth window to re-request the ticket, you might as well
> keep it open and handle it in there.
> 
> That'd probably be better than kicking out back to the previous dialog,
> since it requires less user intervention.

Agreed. This is still TODO.

> @@ +408,5 @@
> > +  deleteFile: function nsBox_deleteFile(aFile, aCallback) {
> > +    return Cr.NS_ERROR_NOT_IMPLEMENTED;
> > +  },
> > +
> > +  _getUrlParameter: function nsBox_getAuthParameter(aUrl, aName)
> 
> It's a bit unnerving that you have to implement this kind of function
> yourself. Are you certain that there's no internal XPCOM or Javascript
> function to do this for you?

There are some XPCOM components that work with urls (nsIURI, nsIURLFixup, ...) but I didn't find anything that did this particular task.
Comment 11 Brian King [:kinger] 2012-07-15 14:23:45 PDT
BTW, I am traveling all day tomorrow (Europe -> Portland) so if I don't make it for the rest of the patch please feel free to run wiht it if it will still be possible to make the deadline.
Comment 12 Mike Conley (:mconley) 2012-07-16 11:29:08 PDT
Created attachment 642657 [details] [diff] [review]
Disabled Box support patch [checked-in]

So, the news from upstairs is that we want to land this on comm-central as is, so that once the merge is done, we have the *possibility* of merging fixes from TB 17 into TB 16 to make this component functional.

In the meantime, I've removed the component registration code, so Box support is disabled by default.
Comment 13 Mike Conley (:mconley) 2012-07-16 11:39:28 PDT
The disabled Box support patch has landed in comm-central as https://hg.mozilla.org/comm-central/rev/afea59d023bc.

Leaving this bug open, because there's quite a bit of stuff left to do on this.
Comment 14 Brian King [:kinger] 2012-07-20 15:34:06 PDT
Created attachment 644503 [details] [diff] [review]
Upload / Delete patch

Mike,

Here is a patch with upload and delete working, meaning this is now feature complete. There is still some cleanup to do, as well as the error handling in the Auth window, but I am throwing this up now for feedback.

I'd like to do the tests in a separate patch, if that is ok with you.
Comment 15 Brian King [:kinger] 2012-07-23 22:57:36 PDT
Created attachment 645200 [details] [diff] [review]
RC1 Patch

This patch tidies up all the remaining nits for me, especially around the API calls and how failures are handled. 

So this would bring the feature up to RC1 level, pending review.
Comment 16 Mike Conley (:mconley) 2012-07-24 09:25:37 PDT
(In reply to Brian King (Briks) [:kinger] from comment #14)
> Created attachment 644503 [details] [diff] [review]
> Upload / Delete patch
> 
> Mike,
> 
> Here is a patch with upload and delete working, meaning this is now feature
> complete. There is still some cleanup to do, as well as the error handling
> in the Auth window, but I am throwing this up now for feedback.
> 
> I'd like to do the tests in a separate patch, if that is ok with you.

Thanks Brian, I'll hopefully have some time to look at this in the next few days.

-Mike
Comment 17 Mike Conley (:mconley) 2012-07-26 13:48:37 PDT
Comment on attachment 645200 [details] [diff] [review]
RC1 Patch

Review of attachment 645200 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Brian - here's my first review.

I'm going to do a post-commit review of your first patch as well.

-Mike

::: mail/components/cloudfile/content/Box/auth.js
@@ +119,1 @@
>    nsBoxAuth.getSessionTicket(function(aTicket) {

I think this would be more readable if the functions were defined before passing them in to getSessionTicket.

@@ +120,5 @@
>                                 var authUrl = "https://www.box.com/api/1.0/auth/" + aTicket;
>                                 loadRequestedUrl(authUrl);
>                               },
>                               function (aReq) {
> +                               log.info("get_ticket failed - status = " + aReq.status);

Use log.error

@@ +171,5 @@
>        // Request to get the ticket
>        doXHRequest(requestUrl, 
>                    null,
>                    null,
>                    function(aResponseText, aRequest) {

Define these outside doXHRequest, and then pass in.

@@ +177,3 @@
>                      let doc = aRequest.responseXML;
>                      let docResponse = doc.documentElement;
>                      if (docResponse && docResponse.nodeName == "response") {

Wrap the contents of this block in a try/catch in case the structure of the XML that we get back is not what we expect.

@@ +188,4 @@
>                        successCallback(ticket);
>                      }
>                      else {
>                        failureCallback("", aResponseText, aRequest);

Put some logging in here too with log.error

@@ +191,5 @@
>                        failureCallback("", aResponseText, aRequest);
>                      }
>                  }.bind(this),
>                  function(aException, aResponseText, aRequest) {
> +                  log.info("Failed to acquire a ticket:" + aResponseText);

Use log.error

::: mail/components/cloudfile/nsBox.js
@@ +401,5 @@
>      if (Services.io.offline)
>        throw Ci.nsIMsgCloudFileProvider.offlineErr;
>  
>        let args = "?action=create_folder&api_key=" + kApiKey
>                    + "&auth_token=" + this._cachedAuthToken

Nit: the + signs should be lined up vertically with the " on line 404.

@@ +412,4 @@
>        doXHRequest(requestUrl, 
>                    null,
>                    null,
>                    function(aResponseText, aRequest) {

Please define the function outside of doXHRequest, and then pass them in.

@@ +434,4 @@
>                      }
>                  }.bind(this),
>                  function(aException, aResponseText, aRequest) {
> +                  this.log.info("Failed to create a new folder: " + aRequest.status);

Use this.log.error here.

@@ +517,5 @@
> +    // Request to delete a file
> +    doXHRequest(requestUrl, 
> +                null,
> +                null,
> +                function(aResponseText, aRequest) {

This could be made a lot more readable if you defined these functions before the call to doXHRequest, and then just pass the function names into doXHRequest. Same in uploadFile.

@@ +523,5 @@
>  
> +                  let doc = aRequest.responseXML;
> +                  let docResponse = doc.documentElement;
> +                  if (docResponse && docResponse.nodeName == "response") {
> +                    let docStatus = doc.getElementsByTagName("status")[0].firstChild.nodeValue;

This should be wrapped in a try / catch in the event that the XML we get back does not have the structure that we expect.

@@ +538,5 @@
> +                    aCallback.onStopRequest(null, null, Cr.NS_ERROR_FAILURE);
> +                  }
> +              }.bind(this),
> +              function(aException, aResponseText, aRequest) {
> +                this.log.info("Failed to delete file:" + aResponseText);

You should use this.log.error for things like this.

@@ +709,2 @@
>     */
> +  uploadFile: function nsBox_uploadFile() {

Some commentary in this function would really help illustrate the individual bits that its doing / defining.

@@ +709,3 @@
>     */
> +  uploadFile: function nsBox_uploadFile() {
> +    let args = this.box._cachedAuthToken + "/" + this.box._folderId + "?new_copy=1&share=1";

Before you do this, you should call the onStartRequest method of the callback with null, null in order to inform the UI that the upload is beginning.

@@ +719,5 @@
> +    let curDate = Date.now().toString();
> +    this.log.info("upload url = " + requestUrl);
> +    this.request = req;
> +    req.open("POST", requestUrl, true);
> +    req.onload = function() {

I think a newline before this rather complex block would help readability.

@@ +768,5 @@
> +    let boundary = "------" + curDate;
> +    let contentType = "multipart/form-data; boundary="+ boundary;
> +    req.setRequestHeader("Content-Type", contentType);
> +
> +    /*let fileContents = "--" + boundary +

This fragment can be excised.

@@ +772,5 @@
> +    /*let fileContents = "--" + boundary +
> +      "\r\nContent-Disposition: form-data; name=\"bid\"\r\n\r\n" +
> +       this._urlInfo.fileId;*/
> +
> +    let fileName = /^[\040-\176]+$/.test(this.file.leafName) 

This fragment is because YouSendIt fails to grok non-ASCII characters correctly on the server side. Does Box really have this same problem?

@@ +859,4 @@
>     */
> +  getTempFile: function nsBox_getTempFile(leafName) {
> +    let tempfile = Cc["@mozilla.org/file/directory_service;1"]
> +      .getService(Ci.nsIProperties).get("TmpD", Ci.nsIFile);

Nit: Indent so that the . in .getService appears below the [ in Cc[

@@ +860,5 @@
> +  getTempFile: function nsBox_getTempFile(leafName) {
> +    let tempfile = Cc["@mozilla.org/file/directory_service;1"]
> +      .getService(Ci.nsIProperties).get("TmpD", Ci.nsIFile);
> +    tempfile.append(leafName)
> +    tempfile.createUnique(Components.interfaces.nsIFile.NORMAL_FILE_TYPE, parseInt("0666", 8));

Just use Ci instead of Components.interfaces.
Comment 18 Mike Conley (:mconley) 2012-07-26 14:04:24 PDT
Comment on attachment 642657 [details] [diff] [review]
Disabled Box support patch [checked-in]

Review of attachment 642657 [details] [diff] [review]:
-----------------------------------------------------------------

I've left out comments on the areas that I believe you took care of or modified (or I already reviewed) in your follow-up patch.

I'm most concerned about the l10n problems I found... we'll have to make some decisions on that stuff soon.

::: mail/components/cloudfile/content/Box/auth.js
@@ +21,5 @@
> +    delete this.securityButton;
> +    return this.securityButton = document.getElementById("security-button");
> +  },
> +
> +  QueryInterface: function(aIID) {

Instead of doing this manually, you can use XPCOMUtils.generateQI (see https://developer.mozilla.org/en/XPCOMUtils.jsm)

@@ +65,5 @@
> +                              wpl.STATE_SECURE_HIGH |
> +                              wpl.STATE_SECURE_MED |
> +                              wpl.STATE_SECURE_LOW;
> +    let browser = document.getElementById("requestFrame");
> +    var level;

Use let instead of var

@@ +99,5 @@
> +  //let account = request.account;
> +  
> +  // headerImage does not exist in the XUL. I wonder if the original intention was security-button?
> +  // I wonder, should we set the url-bar-type url holder to have the service icon?
> +  //if (request.iconURI != "")

Remove this commented out code.

@@ +133,5 @@
> +{
> +  let request = window.arguments[0].wrappedJSObject;
> +  /*document.getElementById("headerMessage").textContent = request.promptText;
> +  let account = request.account;
> +  if (request.iconURI != "")

Remove commented out code.

@@ +136,5 @@
> +  let account = request.account;
> +  if (request.iconURI != "")
> +    document.getElementById("headerImage").src = request.iconURI;*/
> +
> +  var browser = document.getElementById("requestFrame");

Use let instead of var.

@@ +138,5 @@
> +    document.getElementById("headerImage").src = request.iconURI;*/
> +
> +  var browser = document.getElementById("requestFrame");
> +  browser.addProgressListener(reporterListener,
> +                              Components.interfaces.nsIWebProgress.NOTIFY_ALL);

Use Ci instead of Components.interfaces

::: mail/components/cloudfile/nsBox.js
@@ +50,5 @@
> +  _folderId: "",
> +  _loggedIn: false,
> +  _userInfo: null,
> +  _file : null,
> +  _requestDate: null,

_requestDate, _successCallback, _lastErrorStatus and _request are not used. _lastErrorText also isn't used, so you might as well just update the getter on line 42 to return the empty string.

@@ +467,5 @@
> +   *
> +   * @param aError an error to get the URL for.
> +   */
> +  providerUrlForError: function nsBox_providerUrlForError(aError) {
> +    // XX TODO Identify other errors we need to worry about, and the resulting error page

The full list of errors can be found here: http://mxr.mozilla.org/comm-central/source/mail/components/cloudfile/nsIMsgCloudFileProvider.idl#132

@@ +581,5 @@
> +    if (!aWithUI && !authToken.length) {
> +      failureCallback();
> +      return;
> +    }
> +    //let aSuccessCallback = successCallback

Remove commented out code.

@@ +584,5 @@
> +    }
> +    //let aSuccessCallback = successCallback
> +
> +    this._browserRequest = { 
> +        promptText : "box auth prompt",

This string is not l10n friendly. We'll need to steal some other string, or let this be blank, in the patch to Aurora.

@@ +596,5 @@
> +            return;
> +          this.account.log.info("auth cancelled");
> +          this._finishLogonRequest();
> +          this.failureCallback();
> +          //this.account.connectFailureCallback();

Remove commented out code.

@@ +617,5 @@
> +                return;
> +
> +              let ticket = this._parent._getUrlParameter(aURL, "ticket");
> +              this._parent._cachedAuthToken = this._parent._getUrlParameter(aURL, "auth_token");
> +              this._parent.log.info("Box auth redirect : " + aURL + "| ticket = " + ticket + " | Auth token = " + this._parent._cachedAuthToken);

This line > 80 chars, and should be broken up over two or more lines.

@@ +648,5 @@
> +    };
> +    this.wrappedJSObject = this._browserRequest;
> +
> +    Services.ww.openWindow(null,
> +                          "chrome://messenger/content/cloudfile/Box/auth.xul",

Nit - line up lines 652 and 653 with the n in null.

::: mail/locales/en-US/chrome/messenger/cloudfile/Box/auth.dtd
@@ +1,5 @@
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this file,
> +   - You can obtain one at http://mozilla.org/MPL/2.0/.  -->
> +
> +<!ENTITY auth.title   "Box Authentication">

Hm - we may need to update this documentation, or inform the localizers that Box is a brand, as opposed to the noun "box".

::: mail/locales/en-US/chrome/messenger/cloudfile/Box/settings.dtd
@@ +1,5 @@
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this file,
> +   - You can obtain one at http://mozilla.org/MPL/2.0/.  -->
> +
> +<!ENTITY boxSettings.needAnAccount "Get a Box account…">

Hm - we may need to update this documentation, or inform the localizers that Box is a brand, as opposed to the noun "box".
Comment 19 Mike Conley (:mconley) 2012-07-31 06:57:49 PDT
Brian:

Any update on this? How are the fixups coming?

-Mike
Comment 20 Brian King [:kinger] 2012-07-31 07:18:34 PDT
Mike,

Minds collide ... I've been working on it! I have a new patch ready that I will post momentarily that covers your review items, but first to address some of your points...

(In reply to Mike Conley (:mconley) from comment #18)
> I'm most concerned about the l10n problems I found... we'll have to make
> some decisions on that stuff soon.

...

> @@ +584,5 @@
> > +    }
> > +    //let aSuccessCallback = successCallback
> > +
> > +    this._browserRequest = { 
> > +        promptText : "box auth prompt",
> 
> This string is not l10n friendly. We'll need to steal some other string, or
> let this be blank, in the patch to Aurora.

I just set it to 'Box' which does not need to be localized. If nothing is passed in, there is a visible jump in the holder for the url in the auth browser window. I notice this issue exists also in the generic browserRequest window used by oauth, so to fix this we should file a separate bug.
 
> ::: mail/locales/en-US/chrome/messenger/cloudfile/Box/auth.dtd
> @@ +1,5 @@
> > +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> > +   - License, v. 2.0. If a copy of the MPL was not distributed with this file,
> > +   - You can obtain one at http://mozilla.org/MPL/2.0/.  -->
> > +
> > +<!ENTITY auth.title   "Box Authentication">
> 
> Hm - we may need to update this documentation, or inform the localizers that
> Box is a brand, as opposed to the noun "box".
> 
> ::: mail/locales/en-US/chrome/messenger/cloudfile/Box/settings.dtd
> @@ +1,5 @@
> > +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> > +   - License, v. 2.0. If a copy of the MPL was not distributed with this file,
> > +   - You can obtain one at http://mozilla.org/MPL/2.0/.  -->
> > +
> > +<!ENTITY boxSettings.needAnAccount "Get a Box account…">
> 
> Hm - we may need to update this documentation, or inform the localizers that
> Box is a brand, as opposed to the noun "box".

I've just added localization notes to translators for both of these, which is pretty standard practice.
Comment 21 Brian King [:kinger] 2012-07-31 07:19:59 PDT
Created attachment 647522 [details] [diff] [review]
RC2 Patch
Comment 22 Mike Conley (:mconley) 2012-08-03 10:20:50 PDT
Comment on attachment 647522 [details] [diff] [review]
RC2 Patch

Review of attachment 647522 [details] [diff] [review]:
-----------------------------------------------------------------

Brian:

Sorry it takes me so long to get to this - IM bugs on beta is really taking up my time.

This looks mostly good. Just a few more issues, and corner cases to consider.

There are still a few bugs here:
1) The auth window spinner looks very strange with the small white background of the spinner against the dark background.
2) Closing the auth window without completing auth does not stop the spinner in the Set up Filelink dialog - and on TB restart, a bogus "ghostly" Filelink account is listed.
3) The auth window does not seem to fit its contents - the window needs to either become wider, or Box.com's log-in page needs to get thinner.

Those 3 can probably be fixed in follow-up bugs once this patch lands, but I wanted to mention them to you so you can check them out.

-Mike

::: mail/components/cloudfile/content/Box/auth.js
@@ +26,5 @@
>      delete this.securityButton;
>      return this.securityButton = document.getElementById("security-button");
>    },
>  
> +  QueryInterface: XPCOMUtils.generateQI([Ci.nsIWebProgressListener, Ci.nsISupportsWeakReference, Ci.nsISupports]),

> 80 chars, should be broken up

@@ +111,5 @@
> +
> +function setupTicketRequest()
> +{
> +  let successCallback = function(aTicket) {
> +                          var authUrl = "https://www.box.com/api/1.0/auth/" + aTicket;

This URL prefix should probably be a const declared earlier in the file.

@@ +165,5 @@
>        let args = "?action=get_ticket&api_key=" + kApiKey;
>        let requestUrl = kServerUrl + args;
>  
> +      let ticketSuccess = function(aResponseText, aRequest) {
> +                            log.info("get_ticket request response = " + aResponseText);

This block is indented too far in - it should only be two spaces in from line 168.

@@ +173,5 @@
> +                              if (docResponse && docResponse.nodeName == "response") {
> +                                let docStatus = doc.getElementsByTagName("status")[0].firstChild.nodeValue;
> +                                log.info("status = " + docStatus);
> +                                if (docStatus != "get_ticket_ok") {
> +                                  failureCallback(null, aResponseText, aRequest);

The failureCallback (defined in setupTicketRequest) only takes a single argument, whereas each time you call it here, you're passing in 3...

@@ +193,5 @@
> +                              failureCallback("", aResponseText, aRequest);
> +                            }
> +      }.bind(this);
> +      let ticketFailure = function(aException, aResponseText, aRequest) {
> +                            log.error("Ticket acquisition error: " + aResponseText);

Same indentation problem as above.

::: mail/components/cloudfile/nsBox.js
@@ +97,1 @@
>      

Anywhere you see these pink blocks, means that there's trailing whitespace that needs to be removed.

@@ +280,5 @@
>              .onStopRequest(null, null, Ci.nsIMsgCloudFileProvider.authErr);
>        }.bind(this);
>  
> +    let accountInfoSuccess = function(aResponseText, aRequest) {
> +                               this.log.info("get_account_info request response = " + aResponseText);

Same indentation problem in this, and the function below.

@@ +293,5 @@
> +                                   return;
> +                                 }
> +                                 this._userInfo = aResponseText;
> +
> +                                 this._totalStorage = doc.getElementsByTagName("space_amount")[0].firstChild.nodeValue;

We need to protect against bogus XML sent back from the service - this should be wrapped in a try catch.

@@ +410,3 @@
>  
> +      let createSuccess = function(aResponseText, aRequest) {
> +                            this.log.info("create_folder request response = " + aResponseText);

Indentation problem here too, and in the function below.

@@ +412,5 @@
> +                            this.log.info("create_folder request response = " + aResponseText);
> +
> +                            let doc = aRequest.responseXML;
> +                            let docResponse = doc.documentElement;
> +                            if (docResponse && docResponse.nodeName == "response") {

We need to protect against a bogus response - we should wrap this in a try catch

@@ +519,3 @@
>  
> +    let deleteSuccess = function(aResponseText, aRequest) {
> +                          this.log.info("delete request response = " + aResponseText);

Same indentation problem. I'll stop mentioning this now.

@@ +791,5 @@
> +    let contentType = "multipart/form-data; boundary="+ boundary;
> +    req.setRequestHeader("Content-Type", contentType);
> +
> +    /*let fileName = /^[\040-\176]+$/.test(this.file.leafName) 
> +        ? this.file.leafName 

Remove this dead code
Comment 23 Brian King [:kinger] 2012-08-05 14:44:47 PDT
(In reply to Mike Conley (:mconley) from comment #22)
> 1) The auth window spinner looks very strange with the small white
> background of the spinner against the dark background.

Yes this was meant only to be a placeholder. Suggestions welcome on how I can show page load activity in the auth window.
Comment 24 Brian King [:kinger] 2012-08-06 07:13:23 PDT
Created attachment 649262 [details]
New progress load icon

Replace loader.gif in cloudfile/content/Box/ with this.

> There are still a few bugs here:
> 1) The auth window spinner looks very strange with the small white
> background of the spinner against the dark background.
> 2) Closing the auth window without completing auth does not stop the spinner
> in the Set up Filelink dialog - and on TB restart, a bogus "ghostly"
> Filelink account is listed.
> 3) The auth window does not seem to fit its contents - the window needs to
> either become wider, or Box.com's log-in page needs to get thinner.

These are all fixed. For #1 I have gone for a more subtle load indicator in the url bar at the top. Replace the current loader.gif with this one.
Comment 25 Brian King [:kinger] 2012-08-06 07:17:57 PDT
Created attachment 649263 [details] [diff] [review]
RC3 Patch

New patch with latest review nits and bugs fixed.
Comment 26 Mike Conley (:mconley) 2012-08-13 09:18:11 PDT
Comment on attachment 649263 [details] [diff] [review]
RC3 Patch

Review of attachment 649263 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Brian - just a few more fixups...getting closer.

::: mail/components/cloudfile/content/Box/auth.js
@@ +114,5 @@
> +}
> +
> +function setupTicketRequest()
> +{
> +  let successCallback = function(aTicket) {

So, what I was asking for before was for indentation like this:


let successCallback = function(aTicket) {
  let authUrl = kAuthUrl + aTicket;
  loadRequestedUrl(authUrl);
};

let failureCallback = function(aReq) {
  log.error("get_ticket failed - status = " + aReq.status);
  // retry
  setupTicketRequest();
};

@@ +115,5 @@
> +
> +function setupTicketRequest()
> +{
> +  let successCallback = function(aTicket) {
> +                          var authUrl = kAuthUrl + aTicket;

We prefer let over var.

@@ +118,5 @@
> +  let successCallback = function(aTicket) {
> +                          var authUrl = kAuthUrl + aTicket;
> +                          loadRequestedUrl(authUrl);
> +                        };
> +  let failureCallback = function (aReq) {

Nit - no space between "function" and "("

@@ +121,5 @@
> +                        };
> +  let failureCallback = function (aReq) {
> +                          log.error("get_ticket failed - status = " + aReq.status);
> +                          // retry
> +                          setupTicketRequest();

Hm - I think there's a path where we can infinitely call setupTicketRequest(); if getting the session ticket repeatedly fails. We should only retry a limited number of times before bailing out completely.

@@ +171,2 @@
>  
> +    let ticketSuccess = function(aResponseText, aRequest) {

See above for the indentation formatting I was looking for.

@@ +180,5 @@
> +                              if (docStatus != "get_ticket_ok") {
> +                                failureCallback(aRequest);
> +                                return;
> +                              }
> +                              var ticket = doc.getElementsByTagName("ticket")[0].firstChild.nodeValue;

We prefer let over var

@@ +202,5 @@
> +                          failureCallback(aRequest);
> +    }.bind(this)
> +
> +    // Request to get the ticket
> +    doXHRequest(requestUrl, 

Nit: Trailing whitespace

::: mail/components/cloudfile/nsBox.js
@@ +87,5 @@
>    },
>  
>    /**
> +   * Private function for assigning the folder id from a cached version
> +   * If the folder doesn't it exist, set in motion the creation

Typo: remove "it"

@@ +261,5 @@
>     * @param failureCallback a callback fired if retrieving profile information
>     *                        fails.
>     */
>    _getUserInfo: function nsBox__getUserInfo(successCallback, failureCallback) {
>      let args = "?action=get_account_info&api_key=" + kApiKey 

Nit: trailing whitespace

@@ +280,5 @@
>              .onStopRequest(null, null, Ci.nsIMsgCloudFileProvider.authErr);
> +    }.bind(this);
> +
> +    let accountInfoSuccess = function(aResponseText, aRequest) {
> +                               this.log.info("get_account_info request response = " + aResponseText);

This function should be formatted like this:

let accountInfoSuccess = function(aResponseText, aRequest) {
  this.log.info("get_account_info request respones = " + aResponseText);
  ...

}.bind(this);

@@ +313,5 @@
> +                                 this.log.error("Account info response: " + aResponseText);
> +                                 failureCallback();
> +                               }
> +    }.bind(this);
> +    let accountInfoFailure = function(aException, aResponseText, aRequest) {

Same as above wrt formatting

@@ +418,3 @@
>  
> +    let createSuccess = function(aResponseText, aRequest) {
> +                          this.log.info("create_folder request response = " + aResponseText);

Same as above re: function indentation

@@ +533,3 @@
>  
> +    let deleteSuccess = function(aResponseText, aRequest) {
> +                          this.log.info("delete request response = " + aResponseText);

Same as above wrt function indentation

@@ +560,5 @@
> +                          }
> +    }.bind(this);
> +    let deleteFailure = function(aException, aResponseText, aRequest) {
> +                          this.log.error("Failed to delete file:" + aResponseText);
> +                          aCallback.onStopRequest(null, null, Cr.NS_ERROR_FAILURE);

Same as above.

@@ +564,5 @@
> +                          aCallback.onStopRequest(null, null, Cr.NS_ERROR_FAILURE);
> +    }.bind(this);
> +
> +    // Request to delete a file
> +    doXHRequest(requestUrl, 

Nit - trailing whitespace

@@ +643,5 @@
>                  return;
>  
>                let ticket = this._parent._getUrlParameter(aURL, "ticket");
>                this._parent._cachedAuthToken = this._parent._getUrlParameter(aURL, "auth_token");
> +              this._parent.log.info("Box auth redirect : " 

Trailing whitespace in these new lines.
Comment 27 Mike Conley (:mconley) 2012-08-13 09:25:19 PDT
Also, two bugs I just discovered while playing with this patch:

1) The spinner that you've put into the auth window location bar is vertically stretched.
2) Trying to upload a file named <!--.png resulted in an error message.
Comment 28 Brian King [:kinger] 2012-08-13 14:45:48 PDT
Created attachment 651537 [details] [diff] [review]
RC4 Patch

Updated patch with latest review comments, as well as the 2 bugs identified, fixed. I found some other indentation issues as well and fixed them.

Bonus: loader.gif bundled with Box is not removed. I found a loader icon existing in chrome, indeed it is used in clousfile addAccountDialog.
Comment 29 Mike Conley (:mconley) 2012-08-14 13:10:55 PDT
Comment on attachment 651537 [details] [diff] [review]
RC4 Patch

Review of attachment 651537 [details] [diff] [review]:
-----------------------------------------------------------------

Brian:

Ok, I'm pretty happy with this, and I think it's OK to land, once the issues I've found below are fixed.

Also, you'll need to remove loader.gif from mail/components/cloudfile/jar.mn, or else building fails.

Great work,

-Mike

::: mail/components/cloudfile/content/Box/auth.js
@@ +28,5 @@
>      return this.securityButton = document.getElementById("security-button");
>    },
>  
> +  QueryInterface: XPCOMUtils.generateQI([Ci.nsIWebProgressListener,
> +                                        Ci.nsISupportsWeakReference,

Nit - indent this and line 33 just one more space to line up with the Ci.nsIWebProgressListener above it.

@@ +102,5 @@
> + * - The user logs in, and our opener listens for a redirect url
> + * - The redirect url contains the ticket and the auth token
> + * - The opener, when satisfied it has what it needs, closes this window
> + * - If the ticket call produces an error, we try again
> +*/

Nit - indent one space

@@ +113,1 @@
>  

We should set numTries to 0 here so that we can retry again if we failed in the past, or even if we've just tried authenticating more than 3 times (since numTries is incremented regardless of outcome).

::: mail/components/cloudfile/nsBox.js
@@ +283,5 @@
> +    let accountInfoSuccess = function(aResponseText, aRequest) {
> +      this.log.info("get_account_info request response = " + aResponseText);
> +
> +      try {
> +       let doc = aRequest.responseXML;

The contents of this try block needs to be indented 1 more space.

@@ +307,5 @@
> +         failureCallback();
> +       }
> +      }
> +      catch(e) {
> +       // most likely bad XML

This catch block also needs its contents indented 1 more space.

@@ +623,5 @@
> +          return;
> +        this.account.log.info("auth cancelled");
> +        this.account._finishLogonRequest();
> +        this.failureCallback();
> +      // ### auth cancelled.

I don't think this comment adds much - we can remove it.

@@ +644,5 @@
>  
> +            let ticket = this._parent._getUrlParameter(aURL, "ticket");
> +            this._parent._cachedAuthToken = this._parent._getUrlParameter(aURL, "auth_token");
> +            this._parent.log.info("Box auth redirect : "
> +                                 + aURL

Lines 648 - 652 should be indented 1 more space.

@@ +787,5 @@
> +          this.callback(this.requestObserver,
> +                      Ci.nsIMsgCloudFileProvider.uploadErr);
> +        }
> +      }
> +      else

This else block should be wrapped in curly-braces.
Comment 30 Brian King [:kinger] 2012-08-14 14:23:53 PDT
Created attachment 651888 [details] [diff] [review]
RC5 Patch - ready for check in

Mike,

Here is the ready for checkin patch. Thank you for your very thorough reviews.

When this is checked in, please let us know when and where we can get builds to try out.
Comment 31 Mike Conley (:mconley) 2012-08-14 14:31:17 PDT
Brian:

I've pushed your patch to our try server to generate some builds.

I'll post a link here to the directory with the installers once the builds have completed.

-Mike
Comment 32 Ryan VanderMeulen [:RyanVM] 2012-08-14 16:25:03 PDT
https://hg.mozilla.org/comm-central/rev/01e1b7a15528

Thanks for the patch, Brian! One request - to make life easier for those checking in on your behalf, please make sure that your hg is configured to generate patches with all of the needed metadata. Thanks!
https://developer.mozilla.org/en-US/docs/Creating_a_patch_that_can_be_checked_in
Comment 34 Lukas Blakk [:lsblakk] use ?needinfo 2012-08-15 10:09:00 PDT
Comment on attachment 651888 [details] [diff] [review]
RC5 Patch - ready for check in

Unless I'm missing something, I think you mean to nominate this for approval-comm-aurora.
Comment 35 Mike Conley (:mconley) 2012-08-15 10:09:38 PDT
(In reply to Lukas Blakk [:lsblakk] from comment #34)
> Comment on attachment 651888 [details] [diff] [review]
> RC5 Patch - ready for check in
> 
> Unless I'm missing something, I think you mean to nominate this for
> approval-comm-aurora.

You're absolutely correct - sorry about that.
Comment 36 Mike Conley (:mconley) 2012-08-15 10:10:11 PDT
Comment on attachment 651888 [details] [diff] [review]
RC5 Patch - ready for check in

We want this for TB 16, as per Jb's arrangements.
Comment 37 Blake Winton (:bwinton) (:☕️) 2012-08-27 06:23:46 PDT
Comment on attachment 651888 [details] [diff] [review]
RC5 Patch - ready for check in

a=me, as per Jb's arrangements.
Comment 38 Mike Conley (:mconley) 2012-08-27 06:44:39 PDT
comm-aurora: https://hg.mozilla.org/releases/comm-aurora/rev/3e81254c8f0c
Comment 39 Brian King [:kinger] 2012-08-27 07:23:24 PDT
Great!

So if I understand correctly (from https://wiki.mozilla.org/Releases), this will go Beta today and Release in 6 weeks?
Comment 40 Mike Conley (:mconley) 2012-08-27 07:35:21 PDT
(In reply to Brian King (Briks) [:kinger] from comment #39)
> Great!
> 
> So if I understand correctly (from https://wiki.mozilla.org/Releases), this
> will go Beta today and Release in 6 weeks?

Pretty much, yep, assuming all goes well!

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