1024023 - Add File Transfer Support for JS-XMPP

Status: NEW

(Chat Core :: XMPP, defect)

Description

[Saurabh Anand [:sawrubh]]

This bug will track my work to implement file transfer in XMPP using protocol support. It will be broken in two parts:
* Create an interface for file transfers
* Implement that in xmpp

Comment 2

[Atul Jangra [:atuljangra]]

I think the initial code which I wrote was in working state, as in the file transfer was working perfectly.
So feel free to reuse that part of the code. That would save student's time and you guys can spend more time in the interface.
Also, Patrick, Saurabh, if you need any help from me, feel free to ping me.

Comment 3

[Saurabh Anand [:sawrubh]]

Attachment: Backend code for implementing XEP-0096

What this patch does:
* Implements XEP-0096 (which involved implementing XEP-0095 and XEP-0047)

What's left:
* We need to add a mechanism to signal the UI about a file transfer offer.
* We need to implement Entity Capabilities (XEP-0115)

Comment 4

[Saurabh Anand [:sawrubh]]

Attachment: Adds empty implementation to purplexpcom

This basically adds empty (NS_ERROR_NOT_IMPLEMENTED) implementations so that js-xmpp implementation compiles.

Comment 5

[Patrick Cloke [:clokep]]

Comment on attachment 8442064 [details] [diff] [review]
Backend code for implementing XEP-0096

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

I didn't get quite through all of this, but I wanted to get you some feedback ASAP. Although there's a bunch of nits in here, more importantly we need to finish the architecture.

::: chat/components/public/prplIConversation.idl
@@ +45,5 @@
>    /* Send a message in the conversation */
>    void sendMsg(in AUTF8String aMsg);
>  
> +  /* Send a file in the conversation */
> +  void sendFile(in nsIFile aFile);

My inclination is that we want to return a prplIFileTransfer object (and the same thing from an acceptFile method) that allows a prpl and UI to communicate easily. We should talk to Florian about this.

::: chat/protocols/xmpp/moz.build
@@ +9,5 @@
>  ]
>  
>  EXTRA_JS_MODULES += [
>      'xmpp-authmechs.jsm',
> +    'xmpp-ft.jsm',

Can we please use file-transfer, there's no reason to try to shorten this.

::: chat/protocols/xmpp/xmpp-ft.jsm
@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +const EXPORTED_SYMBOLS = ["XMPPFileTransfer", "InBandBytestreams"];
> +
> +function ArrayBufferToString(aData) {

This is in ArrayBufferUtils, use it from there.

@@ +8,5 @@
> +  let view = new Uint8Array(aData);
> +  return [String.fromCharCode(view[i]) for (i in view)].join("");
> +}
> +
> +/* Array of bytes to base64 string decoding */

What is this doing? This sentence doesn't seem to make much sense. Include what the input and output of this function is. Nit: no space after the function name.

@@ +21,5 @@
> +      62
> +    : nChr === 47 ?
> +      63
> +    :
> +      0;

This code is formatted very poorly and definitely needs comments.

@@ +24,5 @@
> +    :
> +      0;
> +}
> +
> +function base64DecToArr (sBase64, nBlocksSize) {

This needs a comment above it saying what it does and what the inputs are. Also nit: no space after the function name.

@@ +29,5 @@
> +  let sB64Enc = sBase64.replace(/[^A-Za-z0-9\+\/]/g, ""),
> +      nInLen = sB64Enc.length,
> +      nOutLen = nBlocksSize ? Math.ceil((nInLen * 3 + 1 >> 2) / nBlocksSize) * nBlocksSize
> +                            : nInLen * 3 + 1 >> 2,
> +      taBytes = new Uint8Array(nOutLen);

Nit: We usually use let on each line.

@@ +32,5 @@
> +                            : nInLen * 3 + 1 >> 2,
> +      taBytes = new Uint8Array(nOutLen);
> +
> +  for (let nMod3, nMod4, nUint24 = 0, nOutIdx = 0, nInIdx = 0; nInIdx < nInLen;
> +       nInIdx++) {

Nit: ++i, not i++. Please add comments about what this is doing.

@@ +38,5 @@
> +    nUint24 |= b64ToUint6(sB64Enc.charCodeAt(nInIdx)) << 18 - 6 * nMod4;
> +    if (nMod4 === 3 || nInLen - nInIdx === 1) {
> +      for (nMod3 = 0; nMod3 < 3 && nOutIdx < nOutLen; nMod3++, nOutIdx++) {
> +        taBytes[nOutIdx] = nUint24 >>> (16 >>> nMod3 & 24) & 255;
> +      }

Nit: No { } around a single line.

@@ +46,5 @@
> +  return taBytes;
> +}
> +
> +/**
> + * Handles the file transfer using XEP-0096.

Is XEP-0096 the IBB? Can you include that here? ("XEP-0096, In-Band Bytetransfer" or however it's formatted).

@@ +59,5 @@
> +  // if it's false, then it's a download.
> +  if (aIsUpload)
> +    this._init(aConv, aFile, aIsUpload);
> +  else
> +    this._init(aConv, aFile, aIsUpload, aSessionID);

It looks like the stuff inside of _init can just be in the constructor.

@@ +63,5 @@
> +    this._init(aConv, aFile, aIsUpload, aSessionID);
> +}
> +XMPPFileTransfer.prototype =  {
> +  // InBandBytestreams instance associated with this transfer.
> +  ibb: null,

Why is this a separate object? It almost seems to me like you really want a File transfer object that InBandBytestreams subclasses from. But it's a bit hard to figure out without knowing how different mechanisms are chosen.

@@ +80,5 @@
> +
> +  // This function is used to send a stream initiator request to the receiver.
> +  sendInitiatorRequest: function(aFile) {
> +    // Generate a unique one, but for testing, any will do.
> +    let sessionID = new Date().getTime() + "";

Sounds like this is a TODO. I have a feeling we want some mechanism on the account that will generate unique IDs of some sort.

@@ +85,5 @@
> +
> +    // Add this object to the account map, with session ID as the key.
> +    // Generate a new session ID if we already have this one.
> +    while (this.conv._account._fileTransferMap.has(sessionID))
> +      sessionID = sessionID + "1";

What is this trying to do? Ensure that this is unique for the map?

@@ +120,5 @@
> +  onReceiverResponse: function(aStanza) {
> +    let from = aStanza.attributes["from"];
> +    let type = aStanza.attributes["type"];
> +    if (type == "error") {
> +      Cu.reportError("File transfer not supported by the remote client " + from );

This needs to be displayed in the UI somewhere. Add a TODO comment.

@@ +132,5 @@
> +      this.ibb = new InBandBytestreams(this.conv, this.file, this.sessionID);
> +      this.ibb.fileSize = this.file.fileSize;
> +      this.ibb.fileName = this.file.leafName;
> +      this.ibb.initiateIbbSession();
> +      return;

This return is unnecessary.

@@ +133,5 @@
> +      this.ibb.fileSize = this.file.fileSize;
> +      this.ibb.fileName = this.file.leafName;
> +      this.ibb.initiateIbbSession();
> +      return;
> +    }

What happens if it's a different method?

@@ +190,5 @@
> +      if (findError.getChildren("not-acceptable").length != 0) {
> +        Cu.reportError("User rejected your file transfer offer");
> +        return;
> +      }
> +      Cu.reportError("IBB not supported by the remote client " + from );

Again, all these reportErrors need to be UI level things. Just mark them with TODOs until it's done.

@@ +200,5 @@
> +      function opened(file) {
> +        self._file = file;
> +        self.sendChunk();
> +        return file;
> +      },

You can probably use fat arrow functions if you want. It could clean up the code. Nit: use aFile, not file. (And similarly for all functions everywhere.)

@@ +203,5 @@
> +        return file;
> +      },
> +      function openError() {
> +        Cu.reportError("Error while opening the file");
> +      });

Can you just provide Cu.reportError instead of creating a new function?

@@ +209,5 @@
> +
> +  // Read from this._file and send iq data stanza.
> +  // Should be called only when this._file is resolved.
> +  sendChunk: function() {
> +    Cu.reportError("fileSize: " + this.fileSize + "fileCounter: " + this.fileCounter + "blockSize: " + this.blockSize);

Move this into a real logging statement?

@@ +212,5 @@
> +  sendChunk: function() {
> +    Cu.reportError("fileSize: " + this.fileSize + "fileCounter: " + this.fileCounter + "blockSize: " + this.blockSize);
> +    let difference = this.fileSize - this.fileCounter;
> +    // If less than blockSize bytes remains to be read, read that much, else read blockSize bytes
> +    let sizeToRead = difference < this.blockSize ? difference : this.blockSize;

min(difference, this.blockSize)

@@ +219,5 @@
> +        if (array.length != 0) {
> +          // Encode the read data as base 64.
> +          // To encode we need a string, but we have read the data as
> +          // ArrayBufferView, thus doing proper conversion.
> +          let end = btoa(ArrayBufferToString(array.buffer));

"end" seems like a weird variable name, also can't you just call toString() directly on this.

@@ +225,5 @@
> +          let s = Stanza.iq("set", null, this._conv.to,
> +                          Stanza.node("data", Stanza.NS.ibb,
> +                                      {"seq": this.seqID,
> +                                       "sid": this._sessionID}, chunk));
> +          this._conv._account._connection.sendStanza(s, this.onAckReceived, this);

If we're often referencing the account, it (or connection) it might make sense to have a getter that does the this._conv._account part for you.

@@ +226,5 @@
> +                          Stanza.node("data", Stanza.NS.ibb,
> +                                      {"seq": this.seqID,
> +                                       "sid": this._sessionID}, chunk));
> +          this._conv._account._connection.sendStanza(s, this.onAckReceived, this);
> +          this.seqID = this.seqID + 1;

Use ++

@@ +227,5 @@
> +                                      {"seq": this.seqID,
> +                                       "sid": this._sessionID}, chunk));
> +          this._conv._account._connection.sendStanza(s, this.onAckReceived, this);
> +          this.seqID = this.seqID + 1;
> +          this.fileCounter = this.fileCounter + array.length;

+= array.length

@@ +240,5 @@
> +            function failure(aError) {
> +              Cu.reportError("Error while closing file " + aError);
> +            }
> +          )
> +          Cu.reportError("Ending: data with seqID " + this.seqID);

Change all this to logging code.

::: chat/protocols/xmpp/xmpp.jsm
@@ +627,5 @@
>      this._conv = {};
>      this._buddies = {};
>      this._mucs = {};
> +    this._fileTransferMap = new Map();
> +    this._pendingFileTransfers = [];

Can you please add this above to the prototype with a comment about why it is necessary.

@@ +1341,5 @@
>  
>      for each (let muc in this._mucs)
>        muc.left = true;
>  
> +    for each (let offer in this._pendingFileTransfers)

for each in is deprecated, use for of. (Or you can just use a map call here:
this._pendingFileTransfers.map(offer => offer.cancel()); Doesn't this also have to clean up _fileTransferMap?

Comment 6

[Patrick Cloke [:clokep]]

Comment on attachment 8442070 [details] [diff] [review]
Adds empty implementation to purplexpcom

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

::: purplexpcom/src/purpleConvChat.h
@@ +61,5 @@
>      return purpleConversation::RemoveObserver(aObserver);
>    }
> +  NS_IMETHODIMP SendFile(nsIFile *aFile) {
> +    return purpleConversation::SendFile(aFile);
> +  }

Nit: you seem to have deleted a blank line.

Comment 7

[Saurabh Anand [:sawrubh]]

Attachment: Backend code for implementing XEP-0096 (v2)

Addresses comments. I didn't see any point of returning a prplIFileTransfer object from sendFile. I simply now pass a prplIFileTransfer object between the backend and UI using notifyObservers.

Comment 8

[Saurabh Anand [:sawrubh]]

Attachment: Adds empty implementation to purplexpcom (v2)

Comment 9

[Saurabh Anand [:sawrubh]]

Attachment: UI for drag and drop and accept/reject file transfer offer (v1)

Comment 10

[Saurabh Anand [:sawrubh]]

Attachment: Backend code for implementing XEP-0096 (v3)

* Moves the accept/deny choice from IBB initiation to Stream Initiation.
* Tried to implement showing the error in the UI, added the ClassInfo to XMPPFileTransfer but it isn't working. I'm getting that prpl-jabber error which normally comes because of some syntax error.

Comment 11

[Saurabh Anand [:sawrubh]]

Attachment: UI for drag and drop and accept/reject file transfer offer (v2)

* Better formatting of the file size shown in the accept/deny notification bar using DownloadUtil's convertByteUnits.
* Tried to show the errors in the UI but it's not working.

Comment 12

[Saurabh Anand [:sawrubh]]

Attachment: Backend code for implementing XEP-0096 (v4)

Everything works in this.

Comment 13

[Saurabh Anand [:sawrubh]]

Attachment: UI for drag and drop and accept/reject file transfer offer (v3)

Everything works.

Comment 14

[Patrick Cloke [:clokep]]

Comment on attachment 8448123 [details] [diff] [review]
Backend code for implementing XEP-0096 (v4)

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

Overall this looks like a great start! :) Can't wait to see this in action. I made a bunch of comments, some of them are nits. Please remember that nits apply EVERYWHERE. So re-audit your own code and check our the nits I left and take care of 'em!

I think you should re-read about how promises work, you seem to be nesting callbacks, which is funky and what Promises are meant to AVOID!

We can have some discussions about API changes over IRC or something if you'd like, but when you put up new patches...please let me know what of my feedback you DID not take into account. If you don't comment about something in particular I'm going to assume you took care of it (and won't be happy if I need to comment on the same issue again).

Keep up the good work!

::: chat/components/public/prplIConversation.idl
@@ +45,5 @@
>    /* Send a message in the conversation */
>    void sendMsg(in AUTF8String aMsg);
>  
> +  /* Send a file in the conversation */
> +  void sendFile(in nsIFile aFile);

Are we purposefully adding this for both ConvIM and ConvChat?

::: chat/components/public/prplIFileTransfer.idl
@@ +21,5 @@
> +  const short ERROR_SEND_CLOSE = 7;
> +  const short ERROR_ACK = 8;
> +  const short ERROR_WRITE = 9;
> +  const short ERROR_WRITE_CLOSE = 10;
> +  const short ERROR_WRITE_OPEN = 11;

These all need comments describing what they are and should not include XMPP specific ideas in them. (E.g. SI and IBB.)

@@ +27,5 @@
> +  /* Name of the file sender. */
> +  readonly attribute AUTF8String sender;
> +
> +  /* Name of the file corresponding to this transfer. */
> +  readonly attribute AUTF8String fileName;

How about just giving the nsIFile?

@@ +35,5 @@
> +
> +  readonly attribute AUTF8String error;
> +
> +  void accept();
> +  void reject();

More comments! Remember the idl is the base level of documentation needed. Try to be super clear in what all of these properties and methods are.

::: chat/protocols/xmpp/xmpp-file-transfer.jsm
@@ +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/. */
> +
> +const {classes: Cc, interfaces: Ci, utils: Cu} = Components;

Nit: You don't seem to use Cc, please remove it.

@@ +11,5 @@
> +
> +const EXPORTED_SYMBOLS = ["XMPPFileTransfer", "InBandBytestreams"];
> +
> +/* Convert given base64 encoded character to it's UTF-8 equivalent. */
> +function b64ToUint6(nChr) {

I think I asked for you to include a comment saying where these are from and to reformat them into the "proper" formatting. Sames goes for base64DecToArr.

@@ +54,5 @@
> + * @param aFile      nsIFile associated with the transfer.
> + * @param aIsUpload  Boolean to indicate if it's a download or upload.
> + * @param aSessionID Unique SessionID to identify this transfer.
> + */
> +function XMPPFileTransfer(aConv, aFile, aIsUpload, aSessionID) {

We should swap aIsUpload and aSessionId. That way we can just leave aIsUpload off in some situations. Also, please use "aSessionI*d*". (This goes for...everywhere, I think.)

@@ +89,5 @@
> +    // TODO Check if this needs to be changed.
> +    let mime = 'text/plain';
> +
> +    // To ask the receiver which stream should we continue with,
> +    // however right now we only support IBB

This comment doesn't really make sense to me, can you reword?

@@ +106,5 @@
> +                    Stanza.node("feature", Stanza.NS.feature, null, child)];
> +    let iqArg = [Stanza.node("si", Stanza.NS.si,
> +                             {id: sessionID, "mime-type": mime,
> +                              profile: Stanza.NS.file_transfer_profile},
> +                              children)];

Can you explain a bit more (line by line) what is being configured here?

@@ +126,5 @@
> +      return;
> +    }
> +    // Find which transfer type is supported. This will be present in the received stanza.
> +    let method = aStanza.getElement(["si", "feature"])
> +                .getElement(["x", "field"]).getElement(["value"]);

Nit" Line up the .s in each of these.

@@ +133,5 @@
> +      this.ibb = new InBandBytestreams(this.conv, this.file, this.sessionID);
> +      this.ibb.fileSize = this.file.fileSize;
> +      this.ibb.fileName = this.file.leafName;
> +      this.ibb.initiateIbbSession();
> +    }

Else...what? What happens here? I think we should at least through an error or something.

@@ +137,5 @@
> +    }
> +  }
> +};
> +
> +function InBandBytestreams(aConv, aFile, aSessionID) {

What is this? :) (AKA: Add a comment.)

@@ +138,5 @@
> +  }
> +};
> +
> +function InBandBytestreams(aConv, aFile, aSessionID) {
> +  this._conv = aConv;

I find it odd that this is _conv and above it's conv. What's the deal?

@@ +167,5 @@
> +  // Promise for open file while sending/receiving.
> +  _file: null,
> +
> +  // Current pointer in the file for keeping track of how much we've read/written.
> +  fileCounter: 0,

I dislike this name. I think something more like "fileByteCounter" might be better. "fileCounter" makes me think you're counting multiple files.

@@ +169,5 @@
> +
> +  // Current pointer in the file for keeping track of how much we've read/written.
> +  fileCounter: 0,
> +
> +  // This is the method which initiates ibb session with the receiver.

This comment doesn't add TOO much beyond the name of the function. What is this really doing?

@@ +181,5 @@
> +
> +  // This is called when we have received the response to the ibb initiate
> +  // stanza. We analyse the received stanza, and then start sending the data
> +  // if everything is okay.
> +  onIBBInitiateReceive: function(aStanza) {

Please use proper camelcase everywhere: "onIbbInitiateReceive".

@@ +212,5 @@
> +  // Should be called only when this._file is resolved.
> +  sendChunk: function() {
> +    let difference = this.fileSize - this.fileCounter;
> +    // If less than blockSize bytes remains to be read, read that much, else read blockSize bytes
> +    let sizeToRead = Math.min(difference, this.blockSize);

You can inline the difference here, that's clear enough.

@@ +214,5 @@
> +    let difference = this.fileSize - this.fileCounter;
> +    // If less than blockSize bytes remains to be read, read that much, else read blockSize bytes
> +    let sizeToRead = Math.min(difference, this.blockSize);
> +    this._file.read(sizeToRead).then(
> +      (array) => {

Nit: Parameters should be like aFoo, i.e. aArray here.

@@ +215,5 @@
> +    // If less than blockSize bytes remains to be read, read that much, else read blockSize bytes
> +    let sizeToRead = Math.min(difference, this.blockSize);
> +    this._file.read(sizeToRead).then(
> +      (array) => {
> +        if (array.length != 0) {

Can't you avoid this check by seeing if sizeToRead is 0 outside of this function so we don't have to have as many indent levels?

@@ +219,5 @@
> +        if (array.length != 0) {
> +          // Encode the read data as base 64.
> +          // To encode we need a string, but we have read the data as
> +          // ArrayBufferView, thus doing proper conversion.
> +          let chunk = btoa(ArrayBufferToString(array.buffer)).toString();

Is the toString() really necessary here?

@@ +225,5 @@
> +                          Stanza.node("data", Stanza.NS.ibb,
> +                                      {"seq": this.seqID,
> +                                       "sid": this._sessionID}, chunk));
> +          this._conv._account._connection.sendStanza(s, this.onAckReceived, this);
> +          this.seqID++;

Nit: ++this.seqId, please.

@@ +232,5 @@
> +        else {
> +          // Close the file and end the file transfer.
> +          this._file.close(
> +            () => {
> +              this._conv.systemMessage("File transfer complete");

Add a TODO comment here.

@@ +250,5 @@
> +    );
> +  },
> +
> +  // This functions receives the stanza sent by the receiver,
> +  // analyses it and then send the next block of data.

Do we usually use American or British spellings? I'd do this as "analyzes" :). Also, this comment looks wrapped incorrectly.

@@ +273,5 @@
> +    let s = Stanza.iq("set", null, this._conv.to,
> +                      Stanza.node("close", Stanza.NS.ibb,
> +                                  {"sid": this._sessionID}, null));
> +
> +    // Currently not listening to this. But maybe we want to.

Maybe we want to...for what purpose?

@@ +279,5 @@
> +    this._conv._account._fileTransferMap.delete(this._sessionID);
> +  },
> +
> +  // This is called by the user when we are receiving.
> +  // This functions writes to the file.

"called by the user"!? That doesn't sound right at all. Also, these comments are a bit funky sounding, please reword.

@@ +283,5 @@
> +  // This functions writes to the file.
> +  writeChunk: function(aChunk, aStanza) {
> +    let dataToWrite = base64DecToArr(aChunk);
> +    // Opening the file and writing the chunk.
> +    this._file = OS.File.open(this._targetFile.path, {write: true});

So are we closing this file each time? Does that make sense to do?

@@ +284,5 @@
> +  writeChunk: function(aChunk, aStanza) {
> +    let dataToWrite = base64DecToArr(aChunk);
> +    // Opening the file and writing the chunk.
> +    this._file = OS.File.open(this._targetFile.path, {write: true});
> +    let self = this;

Why do you need this is if you're using fat-arrow functions below?

@@ +292,5 @@
> +          (bytes) => {
> +            aFile.close().then(
> +              // successfully written chunk to disk.
> +              () => {
> +              },

Nit: Put {} on the same line.

::: chat/protocols/xmpp/xmpp.jsm
@@ +237,5 @@
> +      this.sendErrorMessage(Ci.prplIFileTransfer.ERROR_NOT_POSSIBLE, false);
> +      return;
> +    }
> +    let transfer = new XMPPFileTransfer(this, aFile, true);
> +  },

I think it would most likely make sense for this to return a reference to a prplIFileTransfer object, this object could (potentially) already be in an error state.

@@ +247,5 @@
>        to += "/" + this._targetResource;
>      return to;
>    },
>  
> +  sendErrorMessage: function(aError, toFormat, aData) {

It looks to me like this should be in the UI code, not in the backend code.

@@ +668,5 @@
>  
>    _jid: null, // parsed Jabber ID: node, domain, resource
>    _connection: null, // XMPP Connection
>    authMechanisms: null, // hook to let prpls tweak the list of auth mechanisms
> +  _fileTransferMap: null, // A map of all the ongoing file transfers.

Just call this _fileTransfers.

@@ +882,5 @@
>        }
> +
> +      // Received a Stream Initiation offer.
> +      // This tell us that the sender wants to start a file transfer.
> +      if (aStanza.getChildren("si").length != 0) {

Nit: We don't usually do != 0 for length checks. (This applies everywhere.)

@@ +890,5 @@
> +        let fileSize = fileStanza.attributes["size"];
> +
> +        // Sending the notification to UI.
> +        let self = this;
> +        self.stanza = aStanza;

I'm not sure if this is doing what you expect it to. If you have multiple requests at once this will overwrite each other. You should be saving this on the fileOffer somewhere. (Or just referencing it from this scope.)

@@ +891,5 @@
> +
> +        // Sending the notification to UI.
> +        let self = this;
> +        self.stanza = aStanza;
> +        let fileOffer = {

Is this essentially an XMPPFileTransfer object from above? Why is this one "custom" instead of using your class from above?

@@ +909,5 @@
> +                              Stanza.node("error", null, {type: "error"},
> +                                          Stanza.node("forbidden", Stanza.NS.stanzas)));
> +            self._connection.sendStanza(s);
> +          },
> +          QueryInterface: XPCOMUtils.generateQI([Ci.prplIFileTransfer])

ClassInfo?

@@ +925,5 @@
> +      }
> +
> +      // Data sent by sender through IBB method.
> +      if (aStanza.getChildren("data").length != 0) {
> +        // Get the sid to associate it with the correct file.

What is "sid"? Use the full term here.

@@ +930,5 @@
> +        let sID = aStanza.getElement(["data"]).attributes["sid"];
> +
> +        // Get the sequence ID associated with this data.
> +        let seqID = aStanza.getElement(["data"]).attributes["seq"];
> +        if (!this._fileTransferMap.has(sID)) {

Put a comment about what's happening here.

@@ +942,5 @@
> +          this._conv[from].sendErrorMessage("fileTransferError.data", true, [aStanza.attributes["from"]]);
> +          return;
> +        }
> +
> +        let fileTransfer = this._fileTransferMap.get(sID);

You can just get this and then null check above.

@@ +945,5 @@
> +
> +        let fileTransfer = this._fileTransferMap.get(sID);
> +        let encodedData = aStanza.getElement(["data"]).innerText;
> +
> +        // Writing to the file and /then/ sending ok.

Nit: No / / in comments. I also don't understand this comment, I think you're saying "Write the data to a file and then send that it was received OK."

@@ +953,5 @@
> +      }
> +
> +      // IBB close request received by the sender.
> +      if (aStanza.getChildren("close").length != 0) {
> +        // We want to close the transfer if it exists and close the transfer.

You use "close" a lot in here, what does that really mean? Does that mean the transfer is supposed to be done at this point?

@@ +955,5 @@
> +      // IBB close request received by the sender.
> +      if (aStanza.getChildren("close").length != 0) {
> +        // We want to close the transfer if it exists and close the transfer.
> +        // If there is not such file transfer going on, then we send iq stanza
> +        // clearly stating that.

I don't think this comment matches what's happening below.

@@ +972,5 @@
> +                                Stanza.node("item-not-found",
> +                                            Stanza.NS.stanzas))];
> +        let s = Stanza.iq("error", aStanza.attributes["id"],
> +                          aStanza.attributes["from"], iqArg);
> +        this._connection.sendStanza(s);

Duplicate code warning! Can you somehow abstract this out above these two if blocks?

@@ +985,5 @@
>        }
>      }
>    },
>  
> +  onSIInitiateRequest: function(aStanza) {

Nit: The spacing of this function is wrong. Two space indent.

Please include a block comment about what this function does.

@@ +990,5 @@
> +        let from = this.normalize(aStanza.attributes["from"]);
> +        let fileStanza = aStanza.getElement(["si", "file"]);
> +        let fileName = fileStanza.attributes["name"];
> +        let fileSize = fileStanza.attributes["size"];
> +        // Creating a new file where the data will be stored.

Change the tense of this: "Create a new file where the data will be stored."

@@ +995,5 @@
> +        let file = Services.dirsvc.get("DfltDwnld", Ci.nsIFile);
> +        file.append(fileName);
> +
> +        // We need to have a unique file for file transfer in case a
> +        // file with same name already exists.

"Ensure the file name is unique."

I try not to be "We" in comments, I don't really know why and we don't have a real style guide about it...but I find it leads to shorter comments.

@@ +998,5 @@
> +        // We need to have a unique file for file transfer in case a
> +        // file with same name already exists.
> +        file.createUnique(Ci.nsIFile.NORMAL_FILE_TYPE, FileUtils.PERMS_FILE);
> +
> +        // Getting the session ID from the stanza.

Nit: "Get the session ID..."

@@ +1002,5 @@
> +        // Getting the session ID from the stanza.
> +        let sessionID = aStanza.getElement(["si"]).attributes["id"];
> +        // Create a new IBBFileTransfer instance that will handle the
> +        // file transfer for this user as receiver.
> +        let fileTransfer = new XMPPFileTransfer(this._conv[from], file, false, sessionID);

A new IBBFileTransfer or a new XMPPFileTransfer? :-S Move this underneath the check for whether the session ID exists, I think? You don't seem to use it until after that check.

@@ +1011,5 @@
> +          this._connection.sendStanza(s);
> +          return;
> +        }
> +        fileTransfer.ibb.fileName = fileName;
> +        fileTransfer.ibb.fileSize = fileStanza.attributes["size"]

Nit: Missing a trailing ;.

@@ +1014,5 @@
> +        fileTransfer.ibb.fileName = fileName;
> +        fileTransfer.ibb.fileSize = fileStanza.attributes["size"]
> +
> +        // In future, we also need to know which method the user wants to use.
> +        // Presently sending IBB as we support it.

This comment is a bit funky, let's re-word it.

@@ +1016,5 @@
> +
> +        // In future, we also need to know which method the user wants to use.
> +        // Presently sending IBB as we support it.
> +        let supportedMethod = [Stanza.node("value", null, null,
> +                                           Stanza.NS.ibb)];

I have a feeling this is supposed to be "supportedMethod*s*", but we only support one right now?

Comment 15

[Patrick Cloke [:clokep]]

Comment on attachment 8448124 [details] [diff] [review]
UI for drag and drop and accept/reject file transfer offer (v3)

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

::: im/app/profile/all-instantbird.js
@@ +44,5 @@
>  // Timespan (in seconds) that a MUC nick is marked active after speaking.
>  // -1 = keep active forever
>  pref("messenger.conversations.nickActiveTimespan", 3600);
>  
> +pref("messenger.cloudfile.enabled", false);

This pref name doesn't fully make sense here. :) I'm sure you're trying not to bitrot yourself, but please change it.

::: im/content/conversation.xml
@@ +138,5 @@
> +
> +          // Need to prevent the default handlers for drop.
> +          this.cancelEvent(aEvent);
> +          let file = dataTransfer.mozGetDataAt("application/x-moz-file", 0);
> +          file = file.QueryInterface(Ci.nsIFile);

Nit: Put the QueryInterface on the same statement as the mozGetDataAt.

@@ +544,5 @@
>          ]]>
>          </body>
>        </method>
>  
> +      <method name="fileTransferOffer">

This is received...when we're offered to receive a file? Please put a comment above this.

@@ +549,5 @@
> +        <parameter name="aSubject"/>
> +        <body>
> +        <![CDATA[
> +          Components.utils.import("resource://gre/modules/DownloadUtils.jsm");
> +          aSubject.QueryInterface(Ci.prplIFileTransfer);

Does this line do anything?

@@ +560,5 @@
> +          let acceptButton = {
> +            accessKey: bundle.GetStringFromName("fileTransferOffer.accept.accesskey"),
> +            label: bundle.GetStringFromName("fileTransferOffer.accept.label"),
> +            callback: aSubject.accept
> +            };

Nit: } should line up with let. (Also below.)

@@ +577,5 @@
> +      <method name="fileTransferError">
> +        <parameter name="aSubject"/>
> +        <body>
> +        <![CDATA[
> +          aSubject.QueryInterface(Ci.prplIFileTransfer);

Again, is this necessary?

@@ +589,5 @@
> +              let conv = window.getTabBrowser().selectedConversation;
> +              if (conv) {
> +                let nb = document.getAnonymousElementByAttribute(conv, "anonid", "convNotificationBox");
> +                nb.removeCurrentNotification();
> +              }

You seem to easily get the conversation box earlier (in the "box" variable, above), but here have to get it via getAnon... what's going on here?

@@ +592,5 @@
> +                nb.removeCurrentNotification();
> +              }
> +            }
> +          };
> +          let iconURL = "chrome://browser/skin/Info.png";

Shouldn't this be an error?

::: im/locales/en-US/chrome/instantbird/conversation.properties
@@ +8,5 @@
>  contextCloseConv.accesskey=v
>  contextHideConv.label=Put Conversation on Hold
>  contextHideConv.accesskey=h
> +
> +#LOCALIZATION NOTE

Nit: Space after #.

@@ +11,5 @@
> +
> +#LOCALIZATION NOTE
> +# This is shown when a buddy wants to send a file to the user.
> +# %1$S is the name of the file that is being transferred, %2$S size of the same file
> +# %3$S is the sender's identity

%2$S *is the*

@@ +12,5 @@
> +#LOCALIZATION NOTE
> +# This is shown when a buddy wants to send a file to the user.
> +# %1$S is the name of the file that is being transferred, %2$S size of the same file
> +# %3$S is the sender's identity
> +fileTransferOffer.message=Do you want to accept the file '%1$S' of size %2$S %3$S from %4$S?

This is an awkward message, can we make this better? "<Foo> is trying to transfer the file <XYZ.png> (<n> bytes), accept?"

@@ +21,5 @@
> +
> +fileTransferError.close.label=Close
> +fileTransferError.close.accesskey=C
> +
> +fileTransferError.notPossible=Transfer not possible, ask receiver to send you a Hi!

Send you a hi? What?

@@ +26,5 @@
> +fileTransferError.data=Invalid data received from %1$S
> +fileTransferError.siNotSupported=File transfer is not supported by the remote client %1$S
> +fileTransferError.siDeclined=File transfer was rejected by the receiver %1$S
> +fileTransferError.ibbDeclined=User rejected your file transfer offer
> +fileTransferError.ibbNotSupported=In Band Bytestream not supported by the remote client %1$S

I think I mentioned it somewhere else, but si and ibb don't make sense here, these strings should be protocol agnostic.

@@ +30,5 @@
> +fileTransferError.ibbNotSupported=In Band Bytestream not supported by the remote client %1$S
> +fileTransferError.sendOpen=Error while opening the file
> +fileTransferError.sendClose=Error while closing file %1$S
> +fileTransferError.sendRead=Error while reading
> +fileTransferError.ack=Sendfile error from %1$S

When does this error occur? Also "Sendfile" is not a word. :)

Comment 16

[Saurabh Anand [:sawrubh]]

(In reply to Patrick Cloke [:clokep] from comment #14)
> Are we purposefully adding this for both ConvIM and ConvChat?

I did it here because I saw sendMsg being here.

> How about just giving the nsIFile?

Yeah we could avoid having fileSize and fileName and just simply file.

> 
> I think I asked for you to include a comment saying where these are from and
> to reformat them into the "proper" formatting. Sames goes for base64DecToArr.

I'll get the formatting verified on IRC, it's just a bit confusing with so many nested ternary operations.

> Else...what? What happens here? I think we should at least through an error
> or something.

Ok, I'll throw the error about streaming method being not supported.

> I find it odd that this is _conv and above it's conv. What's the deal?

Will standardize to _conv above and here, since it's private.

> Add a TODO comment here.

About displaying this to the UI like the Download Panel?

> Do we usually use American or British spellings? I'd do this as "analyzes"
> :). Also, this comment looks wrapped incorrectly.

I don't know which spelling we prefer, we'll go with American since no one's British over here. Wrapped incorrectly meaning the comma shouldn't be at the end?

> So are we closing this file each time? Does that make sense to do?

> I think it would most likely make sense for this to return a reference to a
> prplIFileTransfer object, this object could (potentially) already be in an
> error state.

We aren't doing anything with what's returned, does it make a difference what I return?

> It looks to me like this should be in the UI code, not in the backend code.

But the backend code calls this, if I move it to UI (conversation.xml say) then it will become unnecessarily complicated.

> Is this essentially an XMPPFileTransfer object from above? Why is this one
> "custom" instead of using your class from above?

Are you talking about the fileOffer object? It's a prplIFileTransfer object.

> ClassInfo?

I didn't know ClassInfo could be used to pass interfaces. I'll look how to use ClassInfo here.

> You use "close" a lot in here, what does that really mean? Does that mean
> the transfer is supposed to be done at this point?

When you want to end the transfer (either in between or when transfer is complete) you send a close element in an IQ stanza, that's what I meant.

Comment 17

[Saurabh Anand [:sawrubh]]

Attachment: Backend code for implementing XEP-0096 (v5)

Addresses previous comments.

What's not addressed:
* Not using ClassInfo.
* Haven't moved the sendErrorMessage from backend to UI, since I think that can only be possible with a change in the IDL, because in order to move this to conversation.xml, I'll need to call notifyObserver from every place where I right now call sendErrorMessage, now sometimes I pass an array of arguments to construct the error string, in order to pass that to the UI, I'll need to add that field to prplIFileTransfer.idl. 
* Still closing the file every time I write the chunk.

f? both flo and clokep since clokep's on a holiday.

Comment 18

[Saurabh Anand [:sawrubh]]

Attachment: Adds empty implementation to purplexpcom (v3)

Moves sendFile to purpleConvIM.

Comment 19

[Saurabh Anand [:sawrubh]]

Attachment: UI for drag and drop and accept/reject file transfer offer (v4)

Updated preference name, addresses other comments.

Comment 20

[Saurabh Anand [:sawrubh]]

(In reply to Patrick Cloke [:clokep] from comment #15)
> @@ +21,5 @@
> > +
> > +fileTransferError.close.label=Close
> > +fileTransferError.close.accesskey=C
> > +
> > +fileTransferError.notPossible=Transfer not possible, ask receiver to send you a Hi!
> 
> Send you a hi? What?

There needs to be some conversation already with the receiver other I get an error with this._targetResource. This is just to make sure that. I'm not opening a new window or tab or something on the receiver when the sender wants to offer a file.

Also in the previous 'Backend code for implementing XEP-0096 (v5)' patch, ignore the indentation issue in sendItemNotFound, I forgot to fix it, I'll fix it in the next version of the patch.

Comment 21

[Patrick Cloke [:clokep]]

Comment on attachment 8452336 [details] [diff] [review]
Backend code for implementing XEP-0096 (v5)

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

(In reply to Saurabh Anand [:sawrubh] from comment #17)
> Addresses previous comments.
> 
> What's not addressed:
> * Not using ClassInfo.
> * Haven't moved the sendErrorMessage from backend to UI, since I think that
> can only be possible with a change in the IDL, because in order to move this
> to conversation.xml, I'll need to call notifyObserver from every place where
> I right now call sendErrorMessage, now sometimes I pass an array of
> arguments to construct the error string, in order to pass that to the UI,
> I'll need to add that field to prplIFileTransfer.idl. 
> * Still closing the file every time I write the chunk.

It sounds to me like you did not address a bunch of my review comments. Please upload a new patch when you have.

Comment 22

[Patrick Cloke [:clokep]]

(In reply to Saurabh Anand [:sawrubh] from comment #20)
> (In reply to Patrick Cloke [:clokep] from comment #15)
> > @@ +21,5 @@
> > > +
> > > +fileTransferError.close.label=Close
> > > +fileTransferError.close.accesskey=C
> > > +
> > > +fileTransferError.notPossible=Transfer not possible, ask receiver to send you a Hi!
> > 
> > Send you a hi? What?
> 
> There needs to be some conversation already with the receiver other I get an
> error with this._targetResource. This is just to make sure that. I'm not
> opening a new window or tab or something on the receiver when the sender
> wants to offer a file.

This needs some sort of explanatory comment somewhere about what is happening. (And possibly a better message to the user.) Is this something we expect to be able to fix?

Comment 23

[Atul Jangra [:atuljangra]]

I remember I was having a similar error last time. this._targetResource is not set until there is already a conversation going on. You guys can try setting the dependency in such as way that targetResource is set before actually sending the file. Or a hacky way would be to mock a message.

Comment 24

[Saurabh Anand [:sawrubh]]

Attachment: Backend code for implementing XEP-0096 (v6)

Addressed all the comments till now.

f? aleth for the Task part.

Comment 25

[Saurabh Anand [:sawrubh]]

Attachment: UI for drag and drop and accept/reject file transfer offer (v5)

Comment 26

[Patrick Cloke [:clokep]]

Comment on attachment 8452338 [details] [diff] [review]
Adds empty implementation to purplexpcom (v3)

Clearing the flag on the purplexpcom part until we're positive the JS-prpl part is stable.

Comment 27

[Patrick Cloke [:clokep]]

Comment on attachment 8456417 [details] [diff] [review]
Backend code for implementing XEP-0096 (v6)

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

This looks much better than the previous patch! There's a lot of comments below, but many of them are asking for clarifying comments in the code, and some are nits.

::: chat/components/public/prplIFileTransfer.idl
@@ +15,5 @@
> +  /* File transfer declined by receiver. */
> +  const short ERROR_DECLINED = 0;
> +  /* File transfer not supported by receiver. */
> +  const short ERROR_NOT_SUPPORTED = 1;
> +  const short ERROR_NOT_POSSIBLE = 2;

What's the difference between NOT_SUPPORTED and NOT_POSSIBLE?

@@ +24,5 @@
> +  /* Error while closing the file to be sent. */
> +  const short ERROR_SEND_CLOSE = 5;
> +  /* Error while sending, somewhere in between. */
> +  /* TODO specialize the error while sending to handle different cases. */
> +  const short ERROR_ACK = 6;

This error name implies error while waiting for an acknowledgement. What do you mean by "somewhere in between"? I'm also not positive I understand your TODO.

@@ +30,5 @@
> +  const short ERROR_WRITE = 7;
> +  /* Error while closing the file to write to, on the receiver side. */
> +  const short ERROR_WRITE_CLOSE = 8;
> +  /* Error while opening the file to write to, on the receiver side. */
> +  const short ERROR_WRITE_OPEN = 9;

Do we really need WRITE versions of CLOSE and OPEN? We can't just use the ones from above?

@@ +32,5 @@
> +  const short ERROR_WRITE_CLOSE = 8;
> +  /* Error while opening the file to write to, on the receiver side. */
> +  const short ERROR_WRITE_OPEN = 9;
> +  /* Error in the data sent by sender */
> +  const short ERROR_DATA = 10;

Maybe ERROR_CORRUPT_DATA would be clearer? Nit: Please add a period at the end of the comments. (And check all the others too!)

@@ +35,5 @@
> +  /* Error in the data sent by sender */
> +  const short ERROR_DATA = 10;
> +
> +  /* Name of the entity to whom we send or from whom we receive. */
> +  readonly attribute AUTF8String other;

I don't like this name. It seems like we require a buddy in prplIConvIM: https://mxr.mozilla.org/comm-central/source/chat/components/public/prplIConversation.idl#73, which we probably don't want to do here. Maybe something like "remoteName"? Let's talk on IRC about this.

::: chat/protocols/xmpp/xmpp-file-transfer.jsm
@@ +13,5 @@
> +const EXPORTED_SYMBOLS = ["XMPPFileTransfer", "InBandBytestreams"];
> +
> +/* Convert given base64 encoded character to it's UTF-8 equivalent, from
> +   https://developer.mozilla.org/en-US/docs/Web/JavaScript/Base64_encoding_and_decoding
> +*/

Nit: Please format include a " * " in front of all these comments, and indent the last line a space, and don't include text on the first line.

THanks for reformatting these functions, they're much clearer now!

@@ +58,5 @@
> + * @param aSessionId Unique SessionId to identify the transfer.
> + */
> +function XMPPFileTransfer(aConv, aFile, aSessionId) {
> +  // If aIsUpload is true, it means this is a upload,
> +  // if it's false, then it's a download.

aIsUpload doesn't seem to exist. Maybe you mean if aSessionId exists, it is an upload?

@@ +74,5 @@
> +    // Generate a new session ID if we already have this one.
> +    while (this._conv._account._fileTransfers.has(sessionId))
> +      sessionId = sessionId + "1";
> +    this.sessionId = sessionId;
> +    this._conv._account._fileTransfers.set(this.sessionId, this);

This line of code can be moved outside the if block.

@@ +80,5 @@
> +}
> +XMPPFileTransfer.prototype =  {
> +  __proto__: ClassInfo("prplIFileTransfer", "generic file transfer object"),
> +
> +  // InBandBytestreams instance associated with this transfer.

Can you use the same spelling of "In Band Byestreams" everywhere.

@@ +108,5 @@
> +                              profile: Stanza.NS.file_transfer_profile},
> +                              children)];
> +    let s = Stanza.iq("set", null, this._conv.to, iqArg);
> +    this._conv._account._connection.sendStanza(s, this.onReceiverResponse, this);
> +  },

This looks very similar to some code I saw in xmpp.jsm...can they be combined in someway?

@@ +118,5 @@
> +    if (type == "error") {
> +      let errorMsg;
> +      if (aStanza.getChildren("error")[0].getChildren("forbidden").length) {
> +        this.error = Ci.prplIFileTransfer.ERROR_SI_DECLINED;
> +      }

Nit: No { } around single line statements.

Also, I don't think these errors exist anymore.

@@ +169,5 @@
> +  // This is the amount of bytes that are transferred in one IQ stanza.
> +  blockSize: 4096,
> +
> +  // This is the ID of data elements to keep them ordered.
> +  seqID: 0,

Nit: seqId, please.

@@ +223,5 @@
> +  // Should be called only when this._file is resolved.
> +  sendChunk: function() {
> +    // If less than blockSize bytes remains to be read, read that much, else read blockSize bytes
> +    let sizeToRead = Math.min(this.fileSize - this.fileByteCounter, this.blockSize);
> +    if (!sizeToRead) {

Add a comment saying that if the file transfer is done, close the file.

@@ +227,5 @@
> +    if (!sizeToRead) {
> +      // Close the file and end the file transfer.
> +      this._file.close().then(
> +        () => {
> +          // TODO Need to display this in a better way than a system message

Better way "than a system message"? Are we even sending that right now?

@@ +235,5 @@
> +          this.error = Ci.prplIFileTransfer.ERROR_SEND_CLOSE;
> +          this._conv.notifyObservers(this, "file-transfer-error", null);
> +        }
> +      );
> +      this.endTransfer();

Return after this and remove the else clause.

@@ +271,5 @@
> +      // TODO Handle the error as mentioned after http://xmpp.org/extensions/xep-0047.html#example-7
> +      this.endTransfer();
> +      return;
> +    }
> +    // Reading again and sending the data.

By this, you mean "Send the next chunk of data", right?

@@ +288,5 @@
> +  },
> +
> +  // Writes data sent by sender to local file.
> +  writeChunk: function(aChunk, outfile, aStanza) {
> +    let that = this;

Will fat-arryows help here?

@@ +292,5 @@
> +    let that = this;
> +    return Task.spawn(function() {
> +      let dataToWrite = base64DecToArr(aChunk);
> +      yield outfile.write(dataToWrite);
> +      that._conv._account._connection.sendStanza(aStanza);

What is this Stanza you're sending? It should probably be generated here (I think I have a matching comment in the other file that calls this.)

::: chat/protocols/xmpp/xmpp-xml.jsm
@@ +73,5 @@
> +  pubsub_event              : "http://jabber.org/protocol/pubsub#event",
> +
> +  // File transfer
> +  si                        : "http://jabber.org/protocol/si",
> +  file_transfer_profile     : "http://jabber.org/protocol/si/profile/file-transfer",

Any reason for changing the order here?

@@ +74,5 @@
> +
> +  // File transfer
> +  si                        : "http://jabber.org/protocol/si",
> +  file_transfer_profile     : "http://jabber.org/protocol/si/profile/file-transfer",
> +  feature                   : "http://jabber.org/protocol/feature-neg",

It seems like feature_neg would match the pattern better.

::: chat/protocols/xmpp/xmpp.jsm
@@ +858,5 @@
> +          this.onSIInitiateRequest(aStanza);
> +        };
> +        fileTransfer.reject = () => {
> +          // If this file transfer offer is not accepted, then send a stanza
> +          // stating the same.

This sentence is awkward: "Send a stanza stating the file transfer offer was not accepted." is clearer.

@@ +887,5 @@
> +        let fileTransfer = this._fileTransfers.get(sessionId);
> +        // Check if data corresponds to an ongoing transfer.
> +        if (!fileTransfer) {
> +          this.sendItemNotFound(aStanza);
> +          let from = this.normalize(aStanza.attributes["from"]);

This is already defined earlier.

@@ +902,5 @@
> +        Task.spawn(function* () {
> +          if (!fileTransfer.ibb._file)
> +            fileTransfer.ibb._file = yield OS.File.open(fileTransfer.ibb._targetFile.path, {write: true});
> +          fileTransfer.ibb.writeChunk(encodedData, fileTransfer.ibb._file, s);
> +        });

This code seems to me like it should be organized inside of the IBB code.

@@ +927,5 @@
> +        }
> +
> +        // If we didn't have a file transfer going on, we want to tell the
> +        // sender about it. Thus sending a stanza stating that.
> +        this.sendItemNotFound(aStanza);

Do this case first and return early.

@@ +940,5 @@
>        }
>      }
>    },
>  
> +  sendItemNotFound: function(aStanza) {

Please add a comment describing this function.

@@ +949,5 @@
> +                      aStanza.attributes["from"], iqArg);
> +    this._connection.sendStanza(s);
> +  },
> +
> +  /* Callback for when the receiver accepts the file transfer offer. */

I'm a bit confused by this method in general. You created a file transfer object already and then use this as the accept method. But then you re-read data out of the stanza instead of out of the object you already created. Am I totally misunderstanding what is happening here?

@@ +950,5 @@
> +    this._connection.sendStanza(s);
> +  },
> +
> +  /* Callback for when the receiver accepts the file transfer offer. */
> +  onSIInitiateRequest: function(aStanza) {

Please camelcase this title (onSiInitiateRequest).

@@ +961,5 @@
> +    let file = Services.dirsvc.get("DfltDwnld", Ci.nsIFile);
> +    file.append(fileName);
> +
> +    // Ensure the file name is unique.
> +    file.createUnique(Ci.nsIFile.NORMAL_FILE_TYPE, FileUtils.PERMS_FILE);

Does this create the file on disk or just a unique filename?

@@ +963,5 @@
> +
> +    // Ensure the file name is unique.
> +    file.createUnique(Ci.nsIFile.NORMAL_FILE_TYPE, FileUtils.PERMS_FILE);
> +
> +    let fileTransfer = this._fileTransfers.get(sessionId);

When did the file transfer get added to the map?

@@ +978,5 @@
> +    fileTransfer.ibb.fileName = fileName;
> +    fileTransfer.ibb.fileSize = fileStanza.attributes["size"];
> +
> +    // Available streaming methods, possible values can be
> +    // In Band Bytestreams, SOCKS5 Bytestreams

Currently only IBB is supported though, right? I think what you're trying to say is "Currently supported" methods? So don't mention SOCKS5 Bytestreams until we support that. Nit: fully sentences that ends in a . please.

@@ +980,5 @@
> +
> +    // Available streaming methods, possible values can be
> +    // In Band Bytestreams, SOCKS5 Bytestreams
> +    let supportedMethods = [Stanza.node("value", null, null,
> +                                       Stanza.NS.ibb)];

Nit: Spacing.

Comment 28

[Patrick Cloke [:clokep]]

Comment on attachment 8456421 [details] [diff] [review]
UI for drag and drop and accept/reject file transfer offer (v5)

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

::: im/content/conversation.xml
@@ +138,5 @@
> +
> +          // Need to prevent the default handlers for drop.
> +          this.cancelEvent(aEvent);
> +          let file = dataTransfer.mozGetDataAt("application/x-moz-file", 0).QueryInterface(Ci.nsIFile);
> +          this._conv.sendFile(file);

What happens if we drop multiple files?

@@ +543,5 @@
>          ]]>
>          </body>
>        </method>
>  
> +      <!-- This is received when the sender wants to send a file. -->

"the sender wants to send a file"? This is confusing, I think you mean "the person you're talking to wants to send you a file", but that needs to be reworded to be more technical.

@@ +554,5 @@
> +              Services.strings.createBundle("chrome://instantbird/locale/conversation.properties");
> +          let size = DownloadUtils.convertByteUnits(aSubject.fileSize);
> +          let msgString = bundle.formatStringFromName
> +            ("fileTransferOffer.message", [aSubject.other, aSubject.fileName, size[0], size[1]], 4);
> +          let box = this.getElt("convNotificationBox");

Blank line here and a comment about what this is doing.

@@ +571,5 @@
> +        ]]>
> +        </body>
> +      </method>
> +
> +      <!-- This is received when the sender wants to send a file. -->

Copy and paste error, I think!

@@ +622,5 @@
> +            case aSubject.ERROR_WRITE_CLOSE:
> +              errorString = bundle.formatStringFromName("fileTransferError.writeClose", [aSubject.fileName], 1);
> +              break;
> +          }
> +          box.appendNotification(errorString, 'file-transfer-error-bar', null, box.PRIORITY_INFO_HIGH,

Is there a ERROR priority?

Comment 29

[Saurabh Anand [:sawrubh]]

Attachment: Backend code for implementing XEP-0096 (v7)

Changeset:
* Simplified InBandBytestream object (reusing most of the things from the associated XMPPFileTransfer object)
* Displaying a system message while file transfer is complete
* Bunch of other beauty-changes.

Comment 30

[Saurabh Anand [:sawrubh]]

Attachment: UI for drag and drop and accept/reject file transfer offer (v6)

Comment 31

[Patrick Cloke [:clokep]]

Comment on attachment 8457731 [details] [diff] [review]
Backend code for implementing XEP-0096 (v7)

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

Have you tried transferring a file to a different program? (Jitsi? Pidgin?)

This was a pretty quick review since I'm having a busy day, but I think this is pretty close. There are a few things that I don't love (since they don't seem abstract enough), but should be fixed in bug 1035132.

::: chat/protocols/xmpp/xmpp-file-transfer.jsm
@@ +94,5 @@
> +    // TODO Check if this needs to be changed.
> +    let mime = 'text/plain';
> +
> +    // Available streaming methods, possible values can be
> +    // In Band Bytestreams, SOCKS5 Bytestreams

I think I asked you to change this comment. It also looks like it's formatted oddly.

@@ +106,5 @@
> +    let children = [Stanza.node("file", Stanza.NS.file_transfer_profile,
> +                                {name: this.file.leafName, size: this.fileSize}),
> +                    Stanza.node("feature", Stanza.NS.feature_neg, null, child)];
> +    // Generating the IQ stanza for stream initiation
> +    // with the appropriate sessionId, mime-type, profile

As we talked about on IRC. Fix the formatting of these comments to be the full line length. And make them all full sentences that end in a period.

@@ +180,5 @@
> +  onIbbInitiateReceive: function(aStanza) {
> +    let from = aStanza.attributes["from"];
> +    let type = aStanza.attributes["type"];
> +    if (type == "error") {
> +      let findError = aStanza.getElement(["error"]);

What does the "find" mean in this variable?

::: chat/protocols/xmpp/xmpp-xml.jsm
@@ +73,5 @@
> +  pubsub_event              : "http://jabber.org/protocol/pubsub#event",
> +
> +  // File transfer
> +  si                        : "http://jabber.org/protocol/si",
> +  file_transfer_profile     : "http://jabber.org/protocol/si/profile/file-transfer",

I thought we agreed that we'd change this to profile_file_transfer?

::: chat/protocols/xmpp/xmpp.jsm
@@ +879,5 @@
> +        // Get the sessionId to associate it with the correct file.
> +        let sessionId = aStanza.getElement(["data"]).attributes["sid"];
> +
> +        // Get the sequence ID associated with this data.
> +        let seqID = aStanza.getElement(["data"]).attributes["seq"];

Nit: seqId

@@ +895,5 @@
> +        // Write the data to a file and then send that it was received OK.
> +        let s = Stanza.iq("result", aStanza.attributes["id"],
> +                          aStanza.attributes["from"]);
> +        fileTransfer.ibb.writeChunk(encodedData);
> +        this._connection.sendStanza(s);

I dislike that this references ibb directly. I think I'd rather move all of those code into a fileTransfer.writeChunk, but that might be premature abstraction until we support other mechanisms.

@@ +911,5 @@
> +        }
> +
> +        // End the given file transfer.
> +        fileTransfer.ibb._file.close().then(() => {
> +          // TODO Show to the UI that file transfer is complete

Nit: .

Comment 32

[Patrick Cloke [:clokep]]

Comment on attachment 8457733 [details] [diff] [review]
UI for drag and drop and accept/reject file transfer offer (v6)

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

::: im/content/conversation.xml
@@ +138,5 @@
> +
> +          // Need to prevent the default handlers for drop.
> +          this.cancelEvent(aEvent);
> +          for (let i = 0; i < dataTransfer.mozItemCount; ++i) {
> +            let file = dataTransfer.mozGetDataAt("application/x-moz-file", i).QueryInterface(Ci.nsIFile);

This line looks > 80 characters.

@@ +622,5 @@
> +              errorString = bundle.formatStringFromName("fileTransferError.write", [aSubject.fileName], 1);
> +              break;
> +            case aSubject.ERROR_ACK:
> +              errorString = bundle.formatStringFromName("fileTransferError.ack", [aSubject.other], 1);
> +              break;

I'm re-thinking whether we need separate error flags here for each file operation. How do we handle errors like this for connecting accounts? We should have a discussion with aleth and Florian about this.

::: im/locales/en-US/chrome/instantbird/conversation.properties
@@ +23,5 @@
> +
> +fileTransferError.close.label=Close
> +fileTransferError.close.accesskey=C
> +
> +fileTransferError.notPossible=Transfer not possible, ask receiver to send you a Hi!

I don't think you understood my previous comment, please change this to "please ask the received to send you a message."

Comment 33

[Saurabh Anand [:sawrubh]]

Attachment: Backend code for implementing XEP-0096 (v8)

Comment 34

[Saurabh Anand [:sawrubh]]

Attachment: UI for drag and drop and accept/reject file transfer offer (v7)

Comment 35

[Patrick Cloke [:clokep]]

Comment on attachment 8459154 [details] [diff] [review]
Backend code for implementing XEP-0096 (v8)

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

I did not review this entire file since I've still found errors that we've previously discussed. Please self-review all of your code again and ensure it meets the previous guidance that we've given before uploading a new patch.

::: chat/components/public/prplIFileTransfer.idl
@@ +11,5 @@
> + * associated with conversations.
> + */
> +[scriptable, uuid(15b0dead-0fa4-462b-9365-9eb1e2a85c4c)]
> +interface prplIFileTransfer : nsISupports {
> +  /* File transfer declined by receiver. */

We should define a NO_ERROR like https://mxr.mozilla.org/comm-central/source/chat/components/public/imIAccount.idl#151

@@ +14,5 @@
> +interface prplIFileTransfer : nsISupports {
> +  /* File transfer declined by receiver. */
> +  const short ERROR_DECLINED = 0;
> +  /* File transfer not supported by the receiver due to lack of Stream Initiation
> +     or In Band Bytestream implementation. */

This should not refer to anything XMPP specific.

@@ +17,5 @@
> +  /* File transfer not supported by the receiver due to lack of Stream Initiation
> +     or In Band Bytestream implementation. */
> +  const short ERROR_NOT_SUPPORTED = 1;
> +  /* Error when file transfer is not possible since there is not an existing
> +     conversation with the receiver. */

Isn't this an XMPP specific case? And something that is actually a bug in Instantbird?

@@ +23,5 @@
> +  /* File I/O error was encountered. */
> +  const short ERROR_FILE_IO = 3;
> +  /* Error while sending, could be one of the reasons mentioned in
> +     http://xmpp.org/extensions/xep-0047.html#example-7.
> +     TODO Handle the error as mentioned in the above link. */

Again...this shouldn't refer to XMPP.

@@ +28,5 @@
> +  const short ERROR_ACK = 4;
> +  /* Error due to corruption in the data sent by sender. */
> +  const short ERROR_CORRUPT_DATA = 5;
> +
> +  /* Name of the entity to whom we send or from whom we receive. */

Nit: add "data" at the end of this sentence.

@@ +34,5 @@
> +
> +  /* Name of the file being received. */
> +  readonly attribute AUTF8String fileName;
> +
> +  /* Size of the file being received. */

Specify that this is in bytes.

::: chat/protocols/xmpp/xmpp.jsm
@@ +865,5 @@
> +        this._conv.get(from).notifyObservers(fileTransfer, "file-transfer-offer", null);
> +      }
> +
> +      // IBB Initiator request received from the sender. This is used to
> +      // initiate the IBB stream.

This comment is still formatting wrong.

Comment 36

[Patrick Cloke [:clokep]]

Comment on attachment 8459155 [details] [diff] [review]
UI for drag and drop and accept/reject file transfer offer (v7)

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

::: im/locales/en-US/chrome/instantbird/conversation.properties
@@ +23,5 @@
> +
> +fileTransferError.close.label=Close
> +fileTransferError.close.accesskey=C
> +
> +fileTransferError.notPossible=Please ask the receiver to send you a message.

This should really be a more generic message that it isn't possible, not XMPP specific like this. I'm still not entirely convinced about the need for both an unsupported and not possible case.

Nit: Please order these alphabetically unless there's some other order that makes more sense.

@@ +27,5 @@
> +fileTransferError.notPossible=Please ask the receiver to send you a message.
> +fileTransferError.data=Invalid data received from %1$S.
> +fileTransferError.transferNotSupported=File transfer is not supported by the remote client %1$S.
> +fileTransferError.transferDeclined=File transfer was rejected by the receiver %1$S.
> +fileTransferError.fileIO=Error while doing file operations for '%1$S'.

"An error occurred during file operations while transferring '%1%S'." sounds a bit better, I think.

Comment 37

[Saurabh Anand [:sawrubh]]

Attachment: Backend code for implementing XEP-0096 (v9)

Comment 38

[Saurabh Anand [:sawrubh]]

Attachment: UI for drag and drop and accept/reject file transfer offer (v8)

Comment 39

[Patrick Cloke [:clokep]]

Comment on attachment 8459768 [details] [diff] [review]
UI for drag and drop and accept/reject file transfer offer (v8)

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

One nit to fix, you can carry my r+ forward onto the new patch.

::: im/content/conversation.xml
@@ +558,5 @@
> +          let size = DownloadUtils.convertByteUnits(aSubject.fileSize);
> +          let msgString = bundle.formatStringFromName
> +            ("fileTransferOffer.message", [aSubject.remoteName,
> +                                           aSubject.fileName,
> +                                           size[0], size[1]], 4);

Nit: Please put the ( on the line above, you might also want to play with putting the second parameter on the next line if you haven't yet. Just try to make it look pretty. pastebin it to me if you have questions.

@@ +571,5 @@
> +          };
> +          let rejectButton = {
> +            accessKey: bundle.GetStringFromName("fileTransferOffer.reject.accesskey"),
> +            label: bundle.GetStringFromName("fileTransferOffer.reject.label"),
> +            callback: () => aSubject.reject()

Did we ever file that bug or make a reduced test case?

Comment 40

[Patrick Cloke [:clokep]]

Comment on attachment 8459765 [details] [diff] [review]
Backend code for implementing XEP-0096 (v9)

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

I think we're getting very close! I'd appreciate if aleth could take a look of the promises/tasks usage and Florian could look over the interfaces! Thanks.

::: chat/protocols/xmpp/xmpp-file-transfer.jsm
@@ +9,5 @@
> +Cu.import("resource:///modules/ArrayBufferUtils.jsm");
> +Cu.import("resource:///modules/imXPCOMUtils.jsm");
> +Cu.import("resource:///modules/xmpp-xml.jsm");
> +
> +const EXPORTED_SYMBOLS = ["XMPPFileTransfer", "InBandBytestreams"];

Does InBandBystreams actually need to be exported? Where is this used?

@@ +70,5 @@
> +    this.file = aFile;
> +    this.fileSize = this.file.fileSize;
> +    this.fileName = this.file.leafName;
> +    // TODO Generate a unique one, but for testing, any will do.
> +    let sessionId = new Date().getTime() + "";

You probably want to call toString() here?

@@ +192,5 @@
> +        return;
> +      }
> +      this.fileTransfer.error = Ci.prplIFileTransfer.ERROR_NOT_SUPPORTED;
> +      this.fileTransfer.conv.notifyObservers(
> +        this.fileTransfer, "file-transfer-error", null);

Looks like this code has a bit of duplication in it and can be cleaned up by setting the error and then calling the observer only in one place.

@@ +251,5 @@
> +                                      chunk));
> +        this.fileTransfer.conv._account._connection.sendStanza(
> +          s, this.onAckReceived, this);
> +        // Loop the seqId at maximum value of a 16-bit unsigned integer which
> +        // is 65535 (2^15-1)

You don't need the (2^15-1) here. Please end your comment in a ..

@@ +255,5 @@
> +        // is 65535 (2^15-1)
> +        if (this.seqId == 65535)
> +          this.seqId = 0;
> +        else
> +          ++this.seqId;

This looks like duplicated code from xmpp.jsm, can it be shared somehow? (I.e. a calcNextSeqId() function or something).

@@ +268,5 @@
> +    );
> +  },
> +
> +  // This functions receives the stanza sent by the receiver, analyzes it and
> +  // then send the next block of data.

Nit: sends

::: chat/protocols/xmpp/xmpp.jsm
@@ +239,5 @@
> +    fileTransfer.remoteName = this._account.normalize(this.to);
> +    // FIXME _targetResource should not be null to successfully send an iq
> +    // stanza.
> +    if (!this._targetResource) {
> +      Cu.reportError(

Use the account logger here so it shows up in the protocol log!

@@ +856,5 @@
> +        fileTransfer.fileName = fileName;
> +        fileTransfer.fileSize = fileSize;
> +        fileTransfer.accept = () => {
> +          this.onSiInitiateRequest(aStanza);
> +        };

Are the { } necessary? (This is a real question, I don't know the answer.)

@@ +870,5 @@
> +        this._conv.get(from).notifyObservers(
> +          fileTransfer, "file-transfer-offer", null);
> +      }
> +
> +      // IBB Initiator request received from the sender. This is used to 

Nit: Trailing white space.

@@ +892,5 @@
> +        // Check if data corresponds to an ongoing transfer.
> +        if (!fileTransfer) {
> +          this.sendError("cancel", "item-not-found", aStanza.attributes["id"],
> +                         aStanza.attributes["from"]);
> +          fileTransfer.error = Ci.prplIFileTransfer.ERROR_CORRUPT_DATA;

Isn't fileTransfer null in this case? I'm a bit confused on how this is checking for corrupt data. It seems to me like we don't need to display anything in this case.

@@ +895,5 @@
> +                         aStanza.attributes["from"]);
> +          fileTransfer.error = Ci.prplIFileTransfer.ERROR_CORRUPT_DATA;
> +          this._conv.get(from).notifyObservers(fileTransfer,
> +                                               "file-transfer-error", null);
> +          return;

I think we discussed this once before, but I don't fully remember (sorry :-[), when do file transfers get removed from the map if they resolve in an error?

@@ +906,5 @@
> +                         aStanza.attributes["from"]);
> +          return;
> +        }
> +
> +        if (fileTransfer.ibb.seqId == 65535)

I know it's pretty basic, but include a comment that you're wrapping the seq ID to the size of an unsigned integer.

@@ +933,5 @@
> +          return;
> +        }
> +
> +        // End the given file transfer.
> +        fileTransfer.ibb._file.close().then(() => {

Can we move this into the fileTransfer object so we don't touch ibb outside of the FT code?

@@ +963,5 @@
>        }
>      }
>    },
>  
> +  /** 

Nit: Trailing space. And we don't normally do Javadoc style comments (remove the second *).

Comment 41

[aleth [:aleth]]

Comment on attachment 8459765 [details] [diff] [review]
Backend code for implementing XEP-0096 (v9)

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

::: chat/protocols/xmpp/xmpp-file-transfer.jsm
@@ +69,5 @@
> +  else {
> +    this.file = aFile;
> +    this.fileSize = this.file.fileSize;
> +    this.fileName = this.file.leafName;
> +    // TODO Generate a unique one, but for testing, any will do.

How about you and mayanktg get together and decide on a generateId function you add to the account that you can all use, rather than keep inventing new workarounds? You could do this *today* in a separate bug and land it, then you can both use it.

@@ +199,5 @@
> +    this.openFile = OS.File.open(this.fileTransfer.file.path);
> +    this.openFile.then(
> +      // File to be sent, opened successfully.
> +      (aFile) => {
> +        this._file = aFile;

Looks like this._file is not a promise. If so change the comment above the _file definition.

@@ +204,5 @@
> +        this.sendChunk();
> +        return aFile;
> +      },
> +      // Error encountered while opening the file to be sent.
> +      () => {

Please Cu.reportError the details of the failure. Do this also in the other file i/o error handlers. Look at nhnt11's async logs patch for an example of how to do this, or ask him.

@@ +255,5 @@
> +        // is 65535 (2^15-1)
> +        if (this.seqId == 65535)
> +          this.seqId = 0;
> +        else
> +          ++this.seqId;

This should also be in a helper method on the account.

@@ +262,5 @@
> +      // Error while reading the file to be sent.
> +      (error) => {
> +        this.fileTransfer.error = Ci.prplIFileTransfer.ERROR_FILE_IO;
> +        this.fileTransfer.conv.notifyObservers(
> +          this.fileTransfer, "file-transfer-error", null);

Don't you need to abort the file transfer somehow? Let the receiver know there was a problem?

@@ +298,5 @@
> +
> +  // Writes data sent by sender to local file.
> +  writeChunk: function(aChunk) {
> +    let that = this;
> +    return Task.spawn(function* () {

You don't need the that = this trick if you use Task.async, i.e.

writeChunk: Task.async(function* (aChunk) { ...

@@ +310,5 @@
> +      } catch (aError) {
> +        that.fileTransfer.error = Ci.prplIFileTransfer.ERROR_FILE_IO;
> +        that.fileTransfer.conv.notifyObservers(
> +          that.fileTransfer, "file-transfer-error", null);
> +        yield that._file.close();

Again, don't you need to tell the sender to stop sending you stuff?

Comment 42

[Saurabh Anand [:sawrubh]]

Attachment: bug-1024023-3-v9.patch

Carrying forward the r+ from the previous version. No we haven't yet filed a bug for the issue because I guess we haven't been able to make a reduced test case. I'll talk with nhnt11 and see what to do (since I don't understand things completely about the bug there).

Comment 43

[Florian Quèze [:florian]]

Comment on attachment 8459765 [details] [diff] [review]
Backend code for implementing XEP-0096 (v9)

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

I stopped reading at some point (and have mostly skipped xmpp-file-transfer.jsm), too many unaddressed review comments already.

::: chat/components/public/prplIFileTransfer.idl
@@ +22,5 @@
> +  const short ERROR_ACK = 3;
> +  /* Error due to corruption in the data sent by sender. */
> +  const short ERROR_CORRUPT_DATA = 4;
> +
> +  /* Name of the entity to whom we send or from whom we receive data. */

Is this a name intended to be displayed to the user? An internal name (eg. a full JID with a resource)? Something else?

Please improve the comment, and maybe include an example.

@@ +35,5 @@
> +  /* Error during the file transfer. */
> +  readonly attribute short error;
> +
> +  /* Callback after receiver accepts file transfer offer. */
> +  void accept();

When the UI accepts the transfer, should it provide a path of where the received file should be written to disk?

::: chat/protocols/xmpp/xmpp-file-transfer.jsm
@@ +28,5 @@
> +    nChr > 47 && nChr < 58 ? nChr + 4 : nChr === 43 ? 62 : nChr === 47 ? 63 : 0;
> +}
> +
> +/**
> + * Convert base64 encoded string to UTF-8 which can be written to disk, from

Why is UTF8 needed here? Wouldn't we want to write raw data to disk?

::: chat/protocols/xmpp/xmpp.jsm
@@ +16,5 @@
>  Cu.import("resource:///modules/imXPCOMUtils.jsm");
>  Cu.import("resource:///modules/jsProtoHelper.jsm");
>  Cu.import("resource:///modules/NormalizedMap.jsm");
>  Cu.import("resource:///modules/socket.jsm");
> +Cu.import("resource:///modules/xmpp-file-transfer.jsm");

Can this be a lazy import?

@@ +233,5 @@
>        delete this._typingTimer;
>      }
>    },
>  
> +  sendFile: function (aFile) {

nit: no space between "function" and "("

@@ +235,5 @@
>    },
>  
> +  sendFile: function (aFile) {
> +    let fileTransfer = new XMPPFileTransfer(this, aFile);
> +    fileTransfer.remoteName = this._account.normalize(this.to);

The .normalize call here removes the resource; is this intentional?

@@ +238,5 @@
> +    let fileTransfer = new XMPPFileTransfer(this, aFile);
> +    fileTransfer.remoteName = this._account.normalize(this.to);
> +    // FIXME _targetResource should not be null to successfully send an iq
> +    // stanza.
> +    if (!this._targetResource) {

You can do this sanity check at the very beginning before creating the XMPPFileTransfer object.

@@ +240,5 @@
> +    // FIXME _targetResource should not be null to successfully send an iq
> +    // stanza.
> +    if (!this._targetResource) {
> +      Cu.reportError(
> +        "This is bug, we're fixing it! Until then, the workaround is to ask the \

This is _a_ bug

@@ +842,5 @@
> +      let from = this.normalize(aStanza.attributes["from"]);
> +
> +      // Received a Stream Initiation offer.
> +      // This tell us that the sender wants to start a file transfer.
> +      if (aStanza.getChildren("si").length) {

Shouldn't there also be a namespace check?

@@ +844,5 @@
> +      // Received a Stream Initiation offer.
> +      // This tell us that the sender wants to start a file transfer.
> +      if (aStanza.getChildren("si").length) {
> +        let fileStanza = aStanza.getElement(["si", "file"]);
> +        let fileName = fileStanza.attributes["name"];

What if fileStanza is null?

@@ +850,5 @@
> +        let sessionId = aStanza.getElement(["si"]).attributes["id"];
> +
> +        // Sending the notification to UI.
> +        let fileTransfer = new XMPPFileTransfer(
> +          this._conv.get(from), null, sessionId);

nit: The line break here should rather be before "null".

@@ +856,5 @@
> +        fileTransfer.fileName = fileName;
> +        fileTransfer.fileSize = fileSize;
> +        fileTransfer.accept = () => {
> +          this.onSiInitiateRequest(aStanza);
> +        };

If you don't have the { }, you add an implicit 'return'.

@@ +866,5 @@
> +                                        Stanza.node("forbidden",
> +                                                    Stanza.NS.stanzas)));
> +          this._connection.sendStanza(s);
> +        };
> +        this._conv.get(from).notifyObservers(

Can this._conv.get(from) be null here?

@@ +867,5 @@
> +                                                    Stanza.NS.stanzas)));
> +          this._connection.sendStanza(s);
> +        };
> +        this._conv.get(from).notifyObservers(
> +          fileTransfer, "file-transfer-offer", null);

nit: line break at the wrong place.

@@ +872,5 @@
> +      }
> +
> +      // IBB Initiator request received from the sender. This is used to 
> +      // initiate the IBB stream.
> +      if (aStanza.getChildren("open").length) {

Namespace check?

@@ +881,5 @@
> +        this._connection.sendStanza(s);
> +      }
> +
> +      // Data sent by sender through IBB method.
> +      if (aStanza.getChildren("data").length) {

Namespace check?

@@ +886,5 @@
> +        // Get the sessionId to associate it with the correct file.
> +        let sessionId = aStanza.getElement(["data"]).attributes["sid"];
> +
> +        // Get the sequence ID associated with this data.
> +        let seqId = aStanza.getElement(["data"]).attributes["seq"];

Duplicated "aStanza.getElement(["data"])"

@@ +892,5 @@
> +        // Check if data corresponds to an ongoing transfer.
> +        if (!fileTransfer) {
> +          this.sendError("cancel", "item-not-found", aStanza.attributes["id"],
> +                         aStanza.attributes["from"]);
> +          fileTransfer.error = Ci.prplIFileTransfer.ERROR_CORRUPT_DATA;

Yeah... We may want to log to the debug log that we received junk though.

@@ +902,5 @@
> +        if (seqId < fileTransfer.ibb.seqId) {
> +          // Closing the connection because the sequence number has already been
> +          // used.
> +          this.sendError("cancel", "unexpected-request", aStanza.attributes["id"],
> +                         aStanza.attributes["from"]);

Should this mark the transfer as failed?

@@ +906,5 @@
> +                         aStanza.attributes["from"]);
> +          return;
> +        }
> +
> +        if (fileTransfer.ibb.seqId == 65535)

Or better, just do it with the modulo operator:
fileTransfer.ibb.seqId = (fileTransfer.ibb.seqId + 1) % 65536;
You can still add a comment though :-)

@@ +933,5 @@
> +          return;
> +        }
> +
> +        // End the given file transfer.
> +        fileTransfer.ibb._file.close().then(() => {

Yes please, let's do just namespace checks on the stanza in xmpp.jsm, and handle the details of file transfers from the file transfer jsm file.

Comment 44

[Saurabh Anand [:sawrubh]]

Attachment: Backend code for implementing XEP-0096 (v10)

Comment 45

[Saurabh Anand [:sawrubh]]

(In reply to Patrick Cloke [:clokep] from comment #40)
> Does InBandBystreams actually need to be exported? Where is this used?

Yes, it's used in xmpp.jsm (when I reuse the XMPPFileTransfer object I used for sending notification to the UI and the receiver accepted the offer). I create the InBandBytestream object which is then used and hence need to export.

> You probably want to call toString() here?

Now reusing mayanktg's generateId function :)

> This looks like duplicated code from xmpp.jsm, can it be shared somehow?
> (I.e. a calcNextSeqId() function or something).

Went with flo's suggestion to use modulo.

> Use the account logger here so it shows up in the protocol log!

Now using this._account.LOG (which is what I think you meant by account logger).

> Are the { } necessary? (This is a real question, I don't know the answer.)

As flo answered, not putting the braces will add an implied return which we don't want.

> Isn't fileTransfer null in this case? I'm a bit confused on how this is
> checking for corrupt data. It seems to me like we don't need to display
> anything in this case.

Yeah, it was my bad. Now not displaying anything (and hence not having to use the non-existent fileTransfer object), simply sending the request to cancel the transfer.

> I think we discussed this once before, but I don't fully remember (sorry
> :-[), when do file transfers get removed from the map if they resolve in an
> error?

When we encounter this error, we send the request to cancel the transfer, the sender get's this request to cancel, sends the close element, the sender removes the fileTransfer from it's map, when the receiver gets the close element from the sender, it removes the fileTransfer from it's map too (inside where close element is handled in OnIQStanza).

> Can we move this into the fileTransfer object so we don't touch ibb outside
> of the FT code?

Moved to a function called closeFile. BTW we still need to access the seqId inside the ibb from xmpp.jsm sometimes so it's not that we cannot absolutely not touch ibb outside of the FT code.

Comment 46

[Saurabh Anand [:sawrubh]]

(In reply to aleth [:aleth] from comment #41)
> Looks like this._file is not a promise. If so change the comment above the
> _file definition.

Updated comment.

> Please Cu.reportError the details of the failure. Do this also in the other
> file i/o error handlers. Look at nhnt11's async logs patch for an example of
> how to do this, or ask him.

Done.

> Don't you need to abort the file transfer somehow? Let the receiver know
> there was a problem?

Done.

> You don't need the that = this trick if you use Task.async, i.e.

Yeah, using what you suggested now. Thanks, this is much better.

Comment 47

[Saurabh Anand [:sawrubh]]

(In reply to Florian Quèze [:florian] [:flo] from comment #43)
> Is this a name intended to be displayed to the user? An internal name (eg. a
> full JID with a resource)? Something else?

Yes, this will be shown to the user in the notification bar that he gets when a file transfer offer happens. This is the full JID without the resource (in case of XMPP). In other cases, it could be something else. Basically anything which makes it easy for the user to identify from whom the offer is coming. Since in XMPP, I don't think there is a name in jabber accounts.

> Please improve the comment, and maybe include an example.

If I include an example, it'll be protocol specific, because in XMPP, it'll be the jabber id (without the resource) and in some other protocol, it could be something else (like a full name, if available).

> When the UI accepts the transfer, should it provide a path of where the
> received file should be written to disk?

The user anyway doesn't have the option to choose the location where the file is saved, it's saved to the default download directory. I could show the path to that if you want?

> Why is UTF8 needed here? Wouldn't we want to write raw data to disk?

As https://developer.mozilla.org/en-US/docs/Web/JavaScript/Base64_encoding_and_decoding#The_.22Unicode_Problem.22 says there are two solutions to the Unicode problem, I've used the latter since OS.File takes ArrayBufferView. Hence I'm converting to the UTF8 here.

> Can this be a lazy import?

I'll check if it can be lazy imported.

> The .normalize call here removes the resource; is this intentional?

Yes, I want to show the JID (without the resource) to the user, when he gets the notification bar and hence intentionally removing the resource.

> What if fileStanza is null?

I don't think it would be null, since a si element will always come with file element (when the sender sends it, which is what is handled here), but still handling that fileStanza being null case by returning.

> nit: The line break here should rather be before "null".

Is there some practice that one follows when doing line break, I could technically even do a line break after "null" without exceeding 80. Also how do you decide whether to align the parameter on the line break with the parameters before the line break or just two-space indent the parameters on the line break?

> Can this._conv.get(from) be null here?

I don't think so, because that would mean we don't have a reference to the current conversation which is going on, which would be a bug.

> Should this mark the transfer as failed?

Do you mean mark as failed a)in some visible notification bar to the UI b)the account log c)only hint to the sender that we failed?

Comment 48

[Florian Quèze [:florian]]

(In reply to Saurabh Anand [:sawrubh] from comment #47)
> This is the full JID without the resource (in case of XMPP).

"full JID" means _with_ the resource.

> Since in XMPP, I don't think there is a name in jabber
> accounts.

We don't display the raw JID all the time in conversations...

> > When the UI accepts the transfer, should it provide a path of where the
> > received file should be written to disk?
> 
> The user anyway doesn't have the option to choose the location where the
> file is saved, it's saved to the default download directory. I could show
> the path to that if you want?

Ok, maybe this isn't useful, let's not over complicate things :-).

> > Why is UTF8 needed here? Wouldn't we want to write raw data to disk?
> 
> As
> https://developer.mozilla.org/en-US/docs/Web/JavaScript/
> Base64_encoding_and_decoding#The_.22Unicode_Problem.22 says there are two
> solutions to the Unicode problem

This doesn't answer my question. I'm asking why you have Unicode data. If you are transferring a binary file (eg. an image), it's not UTF8 data.

> > What if fileStanza is null?
> 
> I don't think it would be null, since a si element will always come with
> file element

Please never assume that data coming from the network is in the expected format. You could be talking to a broken XMPP client on the other end.

> > nit: The line break here should rather be before "null".
> 
> Is there some practice that one follows when doing line break, I could
> technically even do a line break after "null" without exceeding 80.

Usually: keep as many parameters as possible on the first line. Do a line break when adding another parameter would exceed the 80 chars.

> Also how
> do you decide whether to align the parameter on the line break with the
> parameters before the line break or just two-space indent the parameters on
> the line break?

Align the parameters after the line break with the first parameter, unless the parameters are very long in which case you need to 2-space indent.

> > Can this._conv.get(from) be null here?
> 
> I don't think so, because that would mean we don't have a reference to the
> current conversation which is going on, which would be a bug.

Can the user close the conversation before the end of the file transfer?

> > Should this mark the transfer as failed?
> 
> Do you mean mark as failed a)in some visible notification bar to the UI
> b)the account log c)only hint to the sender that we failed?

I mean send a notification so the UI so that it could remove visible progress UI, and potentially display an error message.

Comment 49

[Patrick Cloke [:clokep]]

Comment on attachment 8464921 [details] [diff] [review]
Backend code for implementing XEP-0096 (v10)

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

::: chat/protocols/xmpp/xmpp-file-transfer.jsm
@@ +29,5 @@
> +}
> +
> +/*
> + * Convert base64 encoded string to UTF-8 which can be written to disk, from
> + * https://developer.mozilla.org/en-US/docs/Web/JavaScript/Base64_encoding_and_decoding

I echo Florian's sentiment of...what does this have to do with UTF-8? Don't you want to convert a base-64 encoded string to binary data? Binary data doesn't have an encoding... If this function truly does convert a base-64 string to an Array Buffer it sounds like we should put it into ArrayBufferUtils + add tests for it. Also, don't we need the opposite situation?

@@ +63,5 @@
> +  this.conv = aConv;
> +  // If aSessionId exists, it is an upload.
> +  if (aSessionId) {
> +    this.sessionId = aSessionId;
> +    this.conv._account._fileTransfers.set(this.sessionId, this);

This is the same line as the last one in the function, you can remove this.

@@ +76,5 @@
> +  // Add this XMPPFileTransfer object to the account map, with session ID as the
> +  // key.
> +  this.conv._account._fileTransfers.set(this.sessionId, this);
> +}
> +

Nit: No blank line here.

@@ +144,5 @@
> +        {system: true});
> +    }, (aError) => {
> +      // Error while closing file on the receiver.
> +      this.error = Ci.prplIFileTransfer.ERROR_FILE_IO;
> +      this.conv.notifyObservers(this, "file-transfer-error", null);

It looks like you want to add a setter to the error field that automatically notifies observers.

@@ +145,5 @@
> +    }, (aError) => {
> +      // Error while closing file on the receiver.
> +      this.error = Ci.prplIFileTransfer.ERROR_FILE_IO;
> +      this.conv.notifyObservers(this, "file-transfer-error", null);
> +      Cu.reportError("Failed to close the file:\n" + aError);

This should probably use the account logger.

@@ +167,5 @@
> +  // This is the ID of data elements to keep them ordered.
> +  seqId: 0,
> +
> +  // Instance of the open file while sending/receiving.
> +  _file: null,

What is the type of _file?

@@ +181,5 @@
> +                                  {"block-size": this.blockSize,
> +                                   "sid": this.fileTransfer.sessionId,
> +                                   "stanza": "iq"}));
> +    this.fileTransfer.conv._account._connection.sendStanza(
> +      s,this.onIbbInitiateReceive, this);

Nit: Space after the comma.

@@ +213,5 @@
> +      (aError) => {
> +        this.fileTransfer.error = Ci.prplIFileTransfer.ERROR_FILE_IO;
> +        this.fileTransfer.conv.notifyObservers(
> +          this.fileTransfer, "file-transfer-error", null);
> +        Cu.reportError("Failed to open the file:\n" + aError);

This should probably log to the account. (This probably goes for everywhere you're using Cu.reportError right now, but we should discuss it with aleth and Florian about whether they think this stuff should be in the log or just the error console.)

@@ +249,5 @@
> +    this._file.read(sizeToRead).then(
> +      (aArray) => {
> +        // Encode the read data as base 64. To encode we need a string, but we
> +        // have read the data as ArrayBufferView, thus doing proper conversion.
> +        let chunk = btoa(ArrayBufferToString(aArray.buffer));

I'm not convinced this is truly what you want to do. What happens if there is a null byte or something else "odd" in the buffer? Does this still work?

@@ +301,5 @@
> +    this.fileTransfer.conv._account._fileTransfers.delete(
> +      this.fileTransfer.sessionId);
> +  },
> +
> +  // Writes data sent by sender to local file.

Can you expand this comment to describe what aChunk is?

@@ +302,5 @@
> +      this.fileTransfer.sessionId);
> +  },
> +
> +  // Writes data sent by sender to local file.
> +  writeChunk: Task.async(function* (aChunk) {

How do we normally format generates? (Where does the " " vs. * go, etc.)

::: chat/protocols/xmpp/xmpp.jsm
@@ +240,5 @@
> +  sendFile: function(aFile) {
> +    // FIXME _targetResource should not be null to successfully send an iq
> +    // stanza.
> +    if (!this._targetResource) {
> +      this._account.LOG(

Log this as an error.

@@ +247,5 @@
> +      return;
> +    }
> +    let fileTransfer = new XMPPFileTransfer(this, aFile);
> +    fileTransfer.remoteName = this._account.normalize(this.to);
> +    fileTransfer.sendInitiatorRequest();

sendInitiatorRequest should probably take a "remote name" that it uses...or actually, isn't remote name known by knowing what "this" is? (Or if not, should this be another input to the constructor? You seem to set it directly after creating an XMPPFileTransfer in both situations.)

@@ +889,5 @@
> +      }
> +
> +      // Data sent by sender through IBB method.
> +      if (aStanza.getChildren("data").length &&
> +          aStanza.getElement(["data"]).uri == Stanza.NS.ibb) {

This block looks very specific to IBB, does it have to be done here or can it be hidden away somewhere?

@@ +990,5 @@
> +    // Create a file on disk.
> +    file.createUnique(Ci.nsIFile.NORMAL_FILE_TYPE, FileUtils.PERMS_FILE);
> +
> +    fileTransfer.file = file;
> +    fileTransfer.ibb = new InBandBytestreams(fileTransfer);

I don't understand why this isn't hidden from the implementation of xmpp.jsm. It shouldn't care about the underlying file transfer mechanism. I think I've asked this a few times, but have not gotten a satisfactory answer.

Comment 50

[Saurabh Anand [:sawrubh]]

I will not be able to take a look at this before December so unassigning myself.