Closed Bug 955474 Opened 11 years ago Closed 10 years ago

Support for File transfer for XMPP

Categories

(Chat Core :: XMPP, defect)

Product:
Chat Core
Component:
XMPP
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED DUPLICATE of bug 1024023

People

(Reporter: atuljangra, Unassigned)

References

(Blocks 1 open bug,
URL
)

Details

Attachments

(1 file, 4 obsolete files)

Backend for ft
24.38 KB, patch
Details | Diff | Splinter Review 
*** Original post on bio 2037 at 2013-07-10 16:58:00 UTC ***

This bug is for tracking the progress of work done on implementing the file transfer in XMPP
*** Original post on bio 2037 at 2013-07-10 16:59:58 UTC ***

Blocking Bug 953460 (bio 9)
Blocks: 953460
*** Original post on bio 2037 as attmnt 2569 at 2013-07-10 17:29:00 UTC ***

I'm sure there will be lot's of nits and naming problem. I'll update everything in the next patch.

Thanks
Attachment #8354337 - Flags: feedback?(clokep)
*** Original post on bio 2037 at 2013-07-10 17:30:31 UTC ***

Assigning.
Assignee: nobody → atuljangra66
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Comment on attachment 8354337 [details] [diff] [review]
Backend for ft implemented without proper error handling

*** Original change on bio 2037 attmnt 2569 at 2013-07-10 17:30:49 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354337 - Flags: feedback?(florian)
Comment on attachment 8354337 [details] [diff] [review]
Backend for ft implemented without proper error handling

*** Original change on bio 2037 attmnt 2569 at 2013-07-10 18:18:30 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354337 - Flags: feedback?(clokep) → feedback+
*** Original post on bio 2037 at 2013-07-10 18:18:30 UTC ***

Comment on attachment 8354337 [details] [diff] [review] (bio-attmnt 2569)
Backend for ft implemented without proper error handling

>diff -r f5160cd0f8c4 -r a13fca14b7a9 chat/components/public/imIAccount.idl
>@@ -277,4 +277,5 @@
>   // imIAccount also implements an observe method but this
>   // observe should only be called by the prplIAccount
>   // implementations to report connection status changes.
>+
> };
Fix this.

>diff -r f5160cd0f8c4 -r a13fca14b7a9 chat/components/public/prplIFileTransfer.idl
I'm not reviewing this file.

>diff -r f5160cd0f8c4 -r a13fca14b7a9 chat/components/src/imConversations.js
>@@ -271,6 +271,7 @@
>   get title() this.target.title,
>   get startDate() this.target.startDate,
>   sendMsg: function (aMsg) { this.target.sendMsg(aMsg); },
>+  sendFile: function (aFileURI) {this.target.sendFile(aFileURI)},
Nit: Space after { and before }.

>diff -r f5160cd0f8c4 -r a13fca14b7a9 chat/modules/jsProtoHelper.jsm
>@@ -11,7 +11,8 @@
>   "GenericMessagePrototype",
>   "GenericProtocolPrototype",
>   "Message",
>-  "TooltipInfo"
>+  "TooltipInfo",
>+  "GenericFileTransferPrototype"
Nit: Put this is alphabetical order. (It looks like they are at least...)

>@@ -761,3 +767,48 @@
>   get classDescription() this.name + " Protocol",
>   get contractID() "@mozilla.org/chat/" + this.normalizedName + ";1"
> };
>+
>+
Nit: There should only be one empty line here.

>+const GenericFileTransferPrototype = {
>+  __proto__: ClassInfo("prplIFileTransfer", "generic filetranfer object"),
"file transfer" as two words, spelled correctly.

>+
>+  __init: function(aConv, aFile, aIsDownload) {
Why two _? This might need a comment above it about what aConv, aFile and aIsDownload are.

>+  addObserver: function(aObserver) {
>+    if (this._observers.indexOf(aObserver) == -1)
>+      this._observers.push(aObserver);
>+  },
>+  
Nit: Empty lines should be empty with no whitespace (this goes for every file).

>+  getCurrentStatus: function() {
>+    return this.currentStatus;
Is currentStatus defined anywhere?

>diff -r f5160cd0f8c4 -r a13fca14b7a9 chat/protocols/xmpp/xmpp-session.jsm
>@@ -506,8 +506,10 @@
>-      else if (name == "iq")
>+      else if (name == "iq") {
>+        this.LOG("PIXIE: "+ aStanza);
>         this._account.onIQStanza(aStanza, handled);
>+      }
>     }
I have a feeling this change can be rolled back.

>diff -r f5160cd0f8c4 -r a13fca14b7a9 chat/protocols/xmpp/xmpp-xml.jsm
>@@ -70,7 +70,16 @@
>   avatar_metadata           : "urn:xmpp:avatar:metadata",
>   avatar_metadata_notify    : "urn:xmpp:avatar:metadata+notify",
>   pubsub                    : "http://jabber.org/protocol/pubsub",
>-  pubsub_event              : "http://jabber.org/protocol/pubsub#event"
>+  pubsub_event              : "http://jabber.org/protocol/pubsub#event",
>+  
>+  // File transfer
>+  si                        : "http://jabber.org/protocol/si",
>+  fileTransferProfile       : "http://jabber.org/protocol/si/profile/file-transfer",
>+  feature                   : "http://jabber.org/protocol/feature-neg",
>+  jabberX                   : "jabber:x:data",
>+  jabberBytestreams         : "http://jabber.org/protocol/bytestreams",
>+  jabberIBB                 : "http://jabber.org/protocol/ibb",
>+  itemNotFound              : "urn:ietf:params:xml:ns:xmpp-stanzas"
The other ones in context here seem to use _ instead of camelcase, I don't have a strong opinion about this, however.

>diff -r f5160cd0f8c4 -r a13fca14b7a9 chat/protocols/xmpp/xmpp.jsm
>@@ -18,6 +19,9 @@
> Cu.import("resource:///modules/socket.jsm");
> Cu.import("resource:///modules/xmpp-xml.jsm");
> Cu.import("resource:///modules/xmpp-session.jsm");
>+Cu.import("resource://gre/modules/osfile.jsm");
>+Cu.import("resource://gre/modules/Task.jsm");
>+Components.utils.import("resource://gre/modules/commonjs/sdk/core/promise.js");
Nit: Please put gre modules above chat modules and use Cu instead of Components.utils. Also, do you really need the promise.js one? (I'd think osfile.jsm is enough.)

>@@ -35,10 +39,25 @@
>+// A decoder.
>+XPCOMUtils.defineLazyGetter(this, "gDecoder", function () {
It decodes what to what? Why gDecoder?

>+function BytesToArrayBuffer(aBytes = []) {
>+function StringToBytes(aString) [aString.charCodeAt(i) for (i in aString)];
We'd obviously want to use the ones from the module here, but that's not committed yet, so this is fine for now.

>@@ -104,6 +123,12 @@
>+  sendFile: function (aFileURI) {
>+    let aFile = aFileURI.QueryInterface(Ci.nsIFileURL).file;
>+    // This function is for the case of Upload
Nit: Please use full sentences with proper grammar.

>+    transfer = new XMPPFileTransfer (this, aFile, 0);
Nit: No space before (. What is the 0 here?
Can you send files to a MUC? Does this make sense?

>@@ -267,6 +292,15 @@
>+  sendFile: function (aFileURI) {
This needs to be cleaned up a bit, I'm ignoring the reportError lines.

>+    var aFile = aFileURI.QueryInterface(Components.interfaces.nsIFileURL).file;
Nit: Use let not var, always.

>+    if (!this._targetResource)
>+      Cu.reportError("Transfer not possible, ask the receiverto send you a little Hi!");
>+      // Don't know why, but _targetResource should not be null to successfully send an iq stanza
Should this be an XXX or TODO comment or something?

>+      var transfer = new XMPPFileTransfer(this, aFile, 0);
>+      Cu.reportError("File dropped here xmppmucconversation " + aFileURI.spec);
Nit: The indentation here looks wrong.

>@@ -608,6 +642,7 @@
>   _jid: null, // parsed Jabber ID: node, domain, resource
>   _connection: null, // XMPP Connection
>   authMechanisms: null, // hook to let prpls tweak the list of auth mechanisms
>+  _currentFileTransferMap: null, // a map of all the ongoing file transfers
Does this really need current in the title?

>@@ -811,8 +847,133 @@
>           this._onRosterItem(item, true);
>         return;
>       }
>+      
>+      // SI initiator request by sender
This comment doesn't tell me much, but maybe I just don't know the specs one. Nit: . at the end please.

>+      if (aStanza.getChildren("si").length != 0) {
>+        let norm = this._normalizeJID(aStanza.attributes["from"]);
>+        var file = aStanza.getElement(["si", "file"]);
>+        var fileName = file.attributes["name"];
>+        var fileSize = file.attributes["size"];
What's with the mixing of let and var here?

>+        // Creating a new file where the data will be stored
>+        var file = Components.classes["@mozilla.org/file/directory_service;1"].
>+                   getService(Components.interfaces.nsIProperties).
>+                   get("DfltDwnld", Components.interfaces.nsIFile);
Nit: I'm guessing that Cc and Ci are defined in this file. Please put . to start the next line.

>+        // create a new XMPPFileTransfer instance that will handle the file receiving
Nit: Full sentence with proper grammar ending in a period, every! I'll stop commenting on this now!

>+        let fileTransfer = new XMPPFileTransfer(this._conv[norm], file, 1, sessionID); 
Nit: Space at end of line? What is the 1?

>+        // FIXME UI is needed to be implemented for this.
>+        // User can accept/deny the file transfer request.
>+        // Send a stanza to the sender to tell them that we are ready to go
>+        // In future, we also need to know which method the user wants to use
>+        // Presently sending IBB as we support it.
>+        let supportedMethod = [];
Should this be a constant somewhere? (Similar to how we do auth methods?)

>+        supportedMethod.push(Stanza.node("value",null,null,Stanza.NS.jabberIBB));
Nit: spaces after ,.

>+        let iqArg =[];
>+        iqArg.push(Stanza.node("x", Stanza.NS.jabberX,{type : "submit"},
>+                               Stanza.node("field", null, {
>+                                "var" : "stream-method"} ,supportedMethod)));
Nit: space after =, and no need to push this in, just put it directly in the array.

>+        let iqData = [];
>+        iqData.push(Stanza.node("si", Stanza.NS.si, null, 
>+                                      Stanza.node("feature", Stanza.NS.feature,
>+                                                  null, iqArg)));
See above.

>+        let s = Stanza.iq("result", aStanza.attributes["id"],
>+                          aStanza.attributes["from"], iqData);
>+        this._connection.sendStanza(s);
>+        return;
Remove this return.

>+        let xmppFileTransferInstance = this._currentFileTransferMap.get(sID);
What if this doesn't exist? Also, please use a shorter variable name.

>+        let decodedData;
>+        //FIXME Error handling needed
>+        try {
>+          decodedData = atob(encodedData);  
I don't think this can ever throw (maybe if you put really weird characters though).

>@@ -820,6 +981,7 @@
>         this._connection.sendStanza(s);
>       }
>     }
>+    
Remove this change.

>@@ -1400,3 +1566,295 @@
>+XMPPFileTransfer.prototype = XMPPFileTransferPrototype;
>+
>+const XMPPFileTransferPrototype = {
Just set the prototype to the object, don't save it separately first.

>+  // Promise for open file while sending/receiving
>+  OSFile: null,
I dislike the name OSFile, also is this "private" it should have a _ in front of it then.

>+  _init: function(aConv, aFile, aIsDownload, aSessionID) {
Can't this just be the constructor that calls this._init?

>+    this.conv = aConv;
>+    this.file = aFile;
>+    GenericFileTransferPrototype.__init.call(aConv, aFile, aIsDownload);
>+    if (aIsDownload == 0) {
Can't we pass true and false in instead of 0 and 1? Nit: No { } around single line statements.

>+  sendInitiatorRequest: function(aFile) {
>+    // We'll be sending file block by block.
>+    var fileSize = aFile.fileSize;
>+    var fileName = aFile.leafName;
Are these variables used?

>+    // 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._currentFileTransferMap.has(sessionID))
>+      sessionID = sessionID+"1";
Nit: spaces around +.

>+    // To ask the receiver which stream should we continue with
>+    let options = [];
>+    options.push(Stanza.node("option",null,null,
>+                             Stanza.node("value",null,null,
>+                                         Stanza.NS.jabberBytestreams)));
>+    options.push(Stanza.node("option",null,null,
>+                             Stanza.node("value",null,null,
>+                                         Stanza.NS.jabberIBB)));
I think this would be more readable if you just built the array instead of pushed.

>+    let children = [];
>+    let child=[];
>+    child.push(Stanza.node ("x", Stanza.NS.jabberX, {type: "form"},
>+                            Stanza.node("field", null, {"var" : "stream-method"
>+                                        , type: "list-single"}, options)));
>+    children.push(Stanza.node("file", Stanza.NS.fileTransferProfile, {name: fileName,
>+                              size:fileSize}));
>+    children.push(Stanza.node("feature", Stanza.NS.feature, null, child));
Same as above. Nit: spaces around =.

>+    let iqArg = [];
>+    iqArg.push(Stanza.node("si", Stanza.NS.si,{ id: sessionID,
>+                           "mime-type": mime, profile :
>+                           Stanza.NS.fileTransferProfile }, children));
Nit: Fix the spacing, the object should be indented properly.

>+  initiateIbbSession: function() {
>+    // FIXME: This should be user adjustable
>+    this.blockSize = Ci.prplIFileTransfer.DEFAULT_BLOCK_SIZE;
>+    this.fileCounter = 0;
>+    this.seqID = 0;
Should this just default to 0 in the prototype?

>+  // read from this.osfile and send iq data stanza
>+  // Should be called only when this.OSFile is resolved
>+  sendChunk: function() {
>+    this.OSFile.read(this.blockSize).then(
>+      function readSuccess(array) {
>+       Cu.reportError("In sendchunk trying to read from the file");
>+       if (array.length != 0) {

>+          let arr = gDecoder.decode(array);
>+          let end = btoa(arr);
>+          let dec = atob(end);
Umm...what? This needs a comment.

>+  // This functions receives the stanza by the receiver,
>+  // analyses it and then send the next block of data 
>+  onDataReceived: function (aStanza) {
Please add comments to each section here about what's happening.

>+function XMPPFileTransfer(aConv, aFile, aIsDownload, aSessionID) {
This should be above the prototype definition.

>diff -r f5160cd0f8c4 -r a13fca14b7a9 instantbird/content/conversation.xml
I did not review this file.

>diff -r f5160cd0f8c4 -r a13fca14b7a9 mozconfig
Please remove these changes.

>diff -r f5160cd0f8c4 -r a13fca14b7a9 purple/purplexpcom/src/purpleConvIM.h
>--- a/purple/purplexpcom/src/purpleConvIM.h	Mon May 27 23:37:26 2013 +0900
>+++ b/purple/purplexpcom/src/purpleConvIM.h	Wed Jul 10 22:54:40 2013 +0530
>@@ -55,11 +55,12 @@
>   NS_IMETHOD RemoveObserver(nsIObserver *aObserver) {
>     return purpleConversation::RemoveObserver(aObserver);
>   }
>-
>+  NS_IMETHODIMP SendFile(nsIURI * aFileURI) {
>+    return purpleConversation::SendFile(aFileURI);
>+  }
>   // Keep the SendMsg and SendTyping methods here.
>   NS_IMETHOD SendMsg(const nsACString & aMsg);
>   NS_IMETHODIMP SendTyping(const nsACString & aString, int32_t *_retval);
>-
Nit: Don't remove the lines arbitrarily, please include an empty line around this new function.
*** Original post on bio 2037 at 2013-07-10 18:50:19 UTC ***

I agree with all of Patrick's comments. Here are a few more:

> >+  sendFile: function (aFileURI) {this.target.sendFile(aFileURI)},
> Nit: Space after { and before }.

Nit: missing semicolon.

> >+  // File transfer
> >+  si                        : "http://jabber.org/protocol/si",
> >+  fileTransferProfile       : "http://jabber.org/protocol/si/profile/file-transfer",
> >+  feature                   : "http://jabber.org/protocol/feature-neg",
> >+  jabberX                   : "jabber:x:data",
> >+  jabberBytestreams         : "http://jabber.org/protocol/bytestreams",
> >+  jabberIBB                 : "http://jabber.org/protocol/ibb",

I don't see any use for the "jabber" prefix here.

> >+  itemNotFound              : "urn:ietf:params:xml:ns:xmpp-stanzas"

Are you sure of this line?

> >+        // Creating a new file where the data will be stored
> >+        var file = Components.classes["@mozilla.org/file/directory_service;1"].
> >+                   getService(Components.interfaces.nsIProperties).
> >+                   get("DfltDwnld", Components.interfaces.nsIFile);
> Nit: I'm guessing that Cc and Ci are defined in this file. Please put . to
> start the next line.

Use Services.dirsvc here.
*** Original post on bio 2037 at 2013-07-10 23:18:28 UTC ***

> 
> >+  getCurrentStatus: function() {
> >+    return this.currentStatus;
> Is currentStatus defined anywhere?

Yes in the idl file. I plan to use it later with observer to get the current status of file transfer

> 
> >@@ -35,10 +39,25 @@
> >+// A decoder.
> >+XPCOMUtils.defineLazyGetter(this, "gDecoder", function () {
> It decodes what to what? Why gDecoder?

https://developer.mozilla.org/en-US/docs/JavaScript_OS.File/OS.File_for_the_main_thread#Example.3A_Read_the_contents_of_a_file_as_text
This is needed while reading from the file to read it as array. I tried doing without it, but it didn't work out. I'm changing the name to textDecoder.

> >+function BytesToArrayBuffer(aBytes = []) {
> >+function StringToBytes(aString) [aString.charCodeAt(i) for (i in aString)];
> We'd obviously want to use the ones from the module here, but that's not
> committed yet, so this is fine for now.
Yes, we can remove them once it is committed.
 
> >+    transfer = new XMPPFileTransfer (this, aFile, 0);
> Nit: No space before (. What is the 0 here?
> Can you send files to a MUC? Does this make sense?
No, I can't. What do we want to do if user drops a file on MUC?
I'm changing this to reporting an error saying this is not allowed.

> 
> >+    if (!this._targetResource)
> >+      Cu.reportError("Transfer not possible, ask the receiverto send you a little Hi!");
> >+      // Don't know why, but _targetResource should not be null to successfully send an iq stanza
> Should this be an XXX or TODO comment or something?
I guess TODO.

> >@@ -608,6 +642,7 @@
> >   _jid: null, // parsed Jabber ID: node, domain, resource
> >   _connection: null, // XMPP Connection
> >   authMechanisms: null, // hook to let prpls tweak the list of auth mechanisms
> >+  _currentFileTransferMap: null, // a map of all the ongoing file transfers
> Does this really need current in the title?
It do contains only "current" file transfer, but no, we can remove it. So fileTransferMap

> 
> >+        let fileTransfer = new XMPPFileTransfer(this._conv[norm], file, 1, sessionID); 
> Nit: Space at end of line? What is the 1?
> 
> >+        // FIXME UI is needed to be implemented for this.
> >+        // User can accept/deny the file transfer request.
> >+        // Send a stanza to the sender to tell them that we are ready to go
> >+        // In future, we also need to know which method the user wants to use
> >+        // Presently sending IBB as we support it.
> >+        let supportedMethod = [];
> Should this be a constant somewhere? (Similar to how we do auth methods?)
I don't exactly understand this. When we will support multiple methods, then we can have the list of supported method and corresponding ui to choose from.


> 
> >+        let xmppFileTransferInstance = this._currentFileTransferMap.get(sID);
> What if this doesn't exist? Also, please use a shorter variable name.
Changed this to send a corresponding stanza. Was going to cover this while handling error

> 
> >+        let decodedData;
> >+        //FIXME Error handling needed
> >+        try {
> >+          decodedData = atob(encodedData);  
> I don't think this can ever throw (maybe if you put really weird characters
> though).
Then how do I handle the case of bad encoding. Currently not addressing this.
*** Original post on bio 2037 as attmnt 2571 at 2013-07-10 23:37:00 UTC ***

Tried my best to address all the comments.
Attachment #8354339 - Flags: feedback?(clokep)
Attachment #8354339 - Flags: feedback?(florian)
*** Original post on bio 2037 at 2013-07-11 02:18:51 UTC ***

(In reply to comment #6)
> > 
> > >+  getCurrentStatus: function() {
> > >+    return this.currentStatus;
> > Is currentStatus defined anywhere?
> 
> Yes in the idl file. I plan to use it later with observer to get the current
> status of file transfer
But the actual object implementing the interface needs to define it too.

> > >@@ -35,10 +39,25 @@
> > >+// A decoder.
> > >+XPCOMUtils.defineLazyGetter(this, "gDecoder", function () {
> > It decodes what to what? Why gDecoder?
> 
> https://developer.mozilla.org/en-US/docs/JavaScript_OS.File/OS.File_for_the_main_thread#Example.3A_Read_the_contents_of_a_file_as_text
> This is needed while reading from the file to read it as array. I tried doing
> without it, but it didn't work out. I'm changing the name to textDecoder.
My point of this comment is that "a decoder" is not a good comment.

> > >+    transfer = new XMPPFileTransfer (this, aFile, 0);
> > Nit: No space before (. What is the 0 here?
> > Can you send files to a MUC? Does this make sense?
> No, I can't. What do we want to do if user drops a file on MUC?
> I'm changing this to reporting an error saying this is not allowed.
I'm not sure, we probably need a way of saying whether this is possible (or just not allow it at all).

> > >@@ -608,6 +642,7 @@
> > >   _jid: null, // parsed Jabber ID: node, domain, resource
> > >   _connection: null, // XMPP Connection
> > >   authMechanisms: null, // hook to let prpls tweak the list of auth mechanisms
> > >+  _currentFileTransferMap: null, // a map of all the ongoing file transfers
> > Does this really need current in the title?
> It do contains only "current" file transfer, but no, we can remove it. So
> fileTransferMap
Yeah but other file transfers shouldn't even be in the Map, they should be removed when they're done.

> > >+        // Presently sending IBB as we support it.
> > >+        let supportedMethod = [];
> > Should this be a constant somewhere? (Similar to how we do auth methods?)
> I don't exactly understand this. When we will support multiple methods, then we
> can have the list of supported method and corresponding ui to choose from.
Umm...no we don't want UI to choose from, the user shouldn't have to choose this. I'm saying why is this defined inside of this function instead of outside of the function as a constant in the file, see http://lxr.instantbird.org/instantbird/source/chat/protocols/xmpp/xmpp-authmechs.jsm#136

> > >+        let decodedData;
> > >+        //FIXME Error handling needed
> > >+        try {
> > >+          decodedData = atob(encodedData);  
> > I don't think this can ever throw (maybe if you put really weird characters
> > though).
> Then how do I handle the case of bad encoding. Currently not addressing this.
What is a "bad" encoding? How do you get one of those? I tried creating an arbitrary string and running atob on it and it worked fine...maybe I didn't try hard enough... I just tried harder not and if you put a crazy character in it will throw, try using e and you should be able to do good error checking.
Comment on attachment 8354337 [details] [diff] [review]
Backend for ft implemented without proper error handling

*** Original change on bio 2037 attmnt 2569 at 2013-07-11 02:19:24 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354337 - Attachment is obsolete: true
Attachment #8354337 - Flags: feedback?(florian)
Comment on attachment 8354339 [details] [diff] [review]
Backend for ft implemented without proper error handling

*** Original change on bio 2037 attmnt 2571 at 2013-07-11 02:55:26 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354339 - Flags: feedback?(clokep) → feedback+
*** Original post on bio 2037 at 2013-07-11 02:55:26 UTC ***

Comment on attachment 8354339 [details] [diff] [review] (bio-attmnt 2571)
Backend for ft implemented without proper error handling

>diff --git a/chat/components/public/prplIFileTransfer.idl b/chat/components/public/prplIFileTransfer.idl
I've ignored this file.

>diff --git a/chat/modules/jsProtoHelper.jsm b/chat/modules/jsProtoHelper.jsm
>@@ -756,8 +762,55 @@ const GenericProtocolPrototype = {
>+const GenericFileTransferPrototype = {
>+  // aConv is the conversation corresponding to this file transfer.
>+  // aFile is the file corresponding to this file transfer.
Why are the types?

>+  // aIsDownload is the attribute which determines if the file transfer is
>+  // upload or download.
This is a boolean, not an attribute.

>+  _init: function(aConv, aFile, aIsDownload) {
>+    this._conv = aConv;
>+    this._targetFile = aFile;
>+    this._isDownload = aIsDownload;
>+  },

>diff --git a/chat/protocols/xmpp/xmpp-xml.jsm b/chat/protocols/xmpp/xmpp-xml.jsm
>+  feature                   : "http://jabber.org/protocol/feature-neg",
>+  jabber_X                  : "jabber:x:data",
>+  jabber_bytestreams        : "http://jabber.org/protocol/bytestreams",
>+  jabber_IBB                : "http://jabber.org/protocol/ibb",
Florian suggested the jabber is redundant here.

>diff --git a/chat/protocols/xmpp/xmpp.jsm b/chat/protocols/xmpp/xmpp.jsm
>@@ -30,20 +32,35 @@ XPCOMUtils.defineLazyGetter(this, "FileU
>+// A decoder.
Again, this comment needs to be expanded.

>@@ -99,16 +116,21 @@ const XMPPMUCConversationPrototype = {

>+  sendFile: function (aFileURI) {
>+    let aFile = aFileURI.QueryInterface(Ci.nsIFileURL).file;
>+    Cu.reportError("File transfer in MUC is not allowed");
Why generate the file here at all? I suspect conversations need a flag about whether sendFile can be used or not. (Otherwise, I guess we can just throw?) We could also make sendFile part of prplIConvIM instead of prplIConversation, but theoretically some protocols could transfer to a MUC (yeah...IRC allows crazy things like that).

>@@ -262,16 +284,27 @@ const XMPPConversationPrototype = {
>+  sendFile: function (aFileURI) {
>+    let aFile = aFileURI.QueryInterface(Ci.nsIFileURL).file;
This should be "file" not "aFile", usually things prefixed with "a" are parameters.

>+    if (!this._targetResource) {
>+      Cu.reportError("Transfer not possible, ask receiver to send you a Hi!");
Is this supposed to be displayed to the user?

>@@ -806,16 +841,146 @@ const XMPPAccountPrototype = {
>+      // Stream initiator request by sender.
>+      // Tells us that sender wants to start a file transfer.
>+      if (aStanza.getChildren("si").length != 0) {
>+        let norm = this._normalizeJID(aStanza.attributes["from"]);
norm? Should this be from?

>+        // FIXME UI is needed to be implemented for this.
>+        // User can accept/deny the file transfer request.
>+        // Currently sending a stanza to the sender to tell them that we
>+        // are ready to receive.
>+        // In future, we also need to know which method the user wants to use
>+        // Presently sending IBB as we support it.
>+        let supportedMethod = [];
>+        supportedMethod.push(Stanza.node("value", null, null,
>+                                         Stanza.NS.jabber_IBB));
This would be a lot clearer as 
let supportedMethod = [Stanza.node("value", null, null,
                                   Stanza.NS.jabber_IBB)];
This is true everywhere in the code, please make this change.

>+        let iqArg =[Stanza.node("x", Stanza.NS.jabber_X,{ type : "submit" },
Nit: space after the = and ,, not after the {, before the : or }. Please follow these rules everywhere, I'm not going to comment on all of them.

>+      // IBB Initiator request received.
>+      // This is used to initiate the IBB stream.
>+      if (aStanza.getChildren("open").length != 0) {
>+        this.LOG("ATUL: IBB init received");
Update this message to something more reasonable.

>+        let norm = this._normalizeJID(aStanza.attributes["from"]);
norm or from?

>+        // Get the sid to associate it with the correct file.
>+        var sid = aStanza.getElement(["open"]).attributes["sid"];
Why var? Look at this everywhere. I highly doubt we should be adding any vars.

>+      // Data sent by sender through IBB method.
>+      if (aStanza.getChildren("data").length != 0) {
...
>+        if (!this._fileTransferMap.has(sID)) {
>+          let iqArg = [Stanza.node("error", null, null,
>+                                   Stanza.node("item-not-found",
>+                                               Stanza.NS.item_not_found))];
>+          let s = Stanza.iq("error", aStanza.attributes["id"],
>+                          aStanza.attributes["from"], iqArg);
>+          this._connection.sendStanza(s);
Should an error be displayed to the user here?

>+        let xmppFTInstance = this._fileTransferMap.get(sID);
Does this really need "xmpp" in front of it?

>@@ -1395,8 +1564,303 @@ const XMPPAccountPrototype = {
>     // Send the vCard only if it has really changed.
>     if (this._userVCard.getXML() != existingVCard)
>       this._connection.sendStanza(Stanza.iq("set", null, null, this._userVCard));
>     else
>       this.LOG("Not sending the vCard because the server stored vCard is identical.");
>   }
> };
>+
>+
Nit: Only one empty line.

>+function XMPPFileTransfer(aConv, aFile, aIsUpload, aSessionID) {
>+  // We need to upload the file.
>+  if (aIsUpload)
>+    this._init(aConv, aFile, aIsUpload);
>+  else
>+    this._init(aConv, aFile, aIsUpload, aSessionID);
Won't aSessionID be undefined in the other case and you can just send it through to _init where it'll be undefined?

>+XMPPFileTransfer.prototype =  {
...
>+  // This is the amount of bytes that are transferred in one IQ stanza.
>+  blockSize: null,
Shouldn't this default to 4096 according to the XEP?

>+  // Current pointer in the file for keeping track of how much we've read/written.
>+  fileCounter: null,
Shouldn't this default to 0, not null?

>+  _init: function(aConv, aFile, aIsUpload, aSessionID) {
>+    this.conv = aConv;
>+    this.file = aFile;
>+    GenericFileTransferPrototype._init.call(aConv, aFile, aIsUpload);
>+    if (aIsUpload)
>+      this.sendInitiatorRequest(aFile);
>+    else {
>+      this.sessionID = aSessionID;
>+      this.conv._account._fileTransferMap.set(aSessionID, this);
>+      let temp = this.conv._account._fileTransferMap.has(aSessionID);
>+      Cu.reportError("Receiver: Starting a new ft session"+
>+                     aSessionID + " | " + temp);
>+    }
>+  },
Please put the contents of this into the constructor. There's no reason to override _init (and then force calling the GenericFileTransferPrototype version).

>+
>+
Nit: Only one empty line.

>+  sendInitiatorRequest: function(aFile) {
>+    // We'll be sending file block by block.
>+    let fileSize = aFile.fileSize;
>+    let fileName = aFile.leafName;
Do these really need to be saved here? They look like they're used only once.

>+
>+    // generate a unique one, but for testing, any will do.
>+    // FIXME sessionID should be generated by current time/date
>+    var sessionID = 'abcd1234!';
Please fix this, it should be only a line or two.

>+    let child = [Stanza.node ("x", Stanza.NS.jabber_X, { type: "form" },
>+                              Stanza.node("field", null,
>+                                          { "var" : "stream-method",
>+                                          type: "list-single" }, options))];
Please no space after the { and before the } in an object, and not before the :, the above should be formatted as:
>+    let child = [Stanza.node ("x", Stanza.NS.jabber_X, {type: "form"},
>+                              Stanza.node("field", null,
>+                                          {"var": "stream-method",
>+                                           type: "list-single"}, options))];
If it is not clear what I've changed, please ask. Please check this throughout this patch, there are other place this is wrong. I will not comment on the other ones.

>+  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 );
Should this be displayed to the user?

>+  initiateIbbSession: function() {
>+    // FIXME: This should be user adjustable.
>+    this.blockSize = Ci.prplIFileTransfer.DEFAULT_BLOCK_SIZE;
I thought this is supposed to be 4096? Does this really need to be user adjustable, can't we "figure it out" ourselves?

>+    this.fileCounter = 0;
Why is this not 0 in the prototype?

>+          let arr = gDecoder.decode(array);
I thought you renamed this? What are we actually decoding here anyway? What does this line do?

>+  // This function ends the filetransfer.
>+  fileTransferred : function() {
Nit: No space before the :.

>+  // This is calledby the user when we are receiving.
Nit: "called by"

>diff --git a/instantbird/content/conversation.xml b/instantbird/content/conversation.xml
I have not looked at this file.

Overall comments:
 - Please fix your styling to match what we normally use. If you have questions about what that is, please ask.
 - I've repeated some of my review comments from my first review. Please make sure to read over all the review comments and to meet them before posting another patch. Thanks.
 - Please fix up all the reportError calls (either use the proper logging methods or remove them). I know you were working on doing this, but just reiterating.
*** Original post on bio 2037 at 2013-07-11 09:41:28 UTC ***


> 
> >diff --git a/chat/modules/jsProtoHelper.jsm b/chat/modules/jsProtoHelper.jsm
> >@@ -756,8 +762,55 @@ const GenericProtocolPrototype = {
> >+const GenericFileTransferPrototype = {
> >+  // aConv is the conversation corresponding to this file transfer.
> >+  // aFile is the file corresponding to this file transfer.
> Why are the types?
Will add the types.
> 
> >+  // aIsDownload is the attribute which determines if the file transfer is
> >+  // upload or download.
> This is a boolean, not an attribute.
Will change it

> >diff --git a/chat/protocols/xmpp/xmpp-xml.jsm b/chat/protocols/xmpp/xmpp-xml.jsm
> >+  feature                   : "http://jabber.org/protocol/feature-neg",
> >+  jabber_X                  : "jabber:x:data",
> >+  jabber_bytestreams        : "http://jabber.org/protocol/bytestreams",
> >+  jabber_IBB                : "http://jabber.org/protocol/ibb",
> Florian suggested the jabber is redundant here.
Yes, but I think, since they are jabber protocol, thus we should include that. Anyway I'll change these to X, bytestreams and IBB. I hope that will be okay. "X" look quite weird though. So what about "x_data"?

> >@@ -99,16 +116,21 @@ const XMPPMUCConversationPrototype = {
> 
> >+  sendFile: function (aFileURI) {
> >+    let aFile = aFileURI.QueryInterface(Ci.nsIFileURL).file;
> >+    Cu.reportError("File transfer in MUC is not allowed");
> Why generate the file here at all? I suspect conversations need a flag about
> whether sendFile can be used or not. (Otherwise, I guess we can just throw?) We
> could also make sendFile part of prplIConvIM instead of prplIConversation, but
> theoretically some protocols could transfer to a MUC (yeah...IRC allows crazy
> things like that).
Okay. I'll remove the file generation. I thought we can support file transfer in MUC. Can we allow this in xmpp also. Something like if I drop a file in muc, it goes to all the users. This will make much more sense one we have implemented the fileLink feature.
> 
> >@@ -262,16 +284,27 @@ const XMPPConversationPrototype = {
> >+  sendFile: function (aFileURI) {
> >+    let aFile = aFileURI.QueryInterface(Ci.nsIFileURL).file;
> This should be "file" not "aFile", usually things prefixed with "a" are
> parameters.
Sorry. Will change.
> 
> >+    if (!this._targetResource) {
> >+      Cu.reportError("Transfer not possible, ask receiver to send you a Hi!");
> Is this supposed to be displayed to the user?
Actually this was one problem which me and flo were discussing the other day. This needs to be fixed. 
> 
> >@@ -806,16 +841,146 @@ const XMPPAccountPrototype = {
> >+      // Stream initiator request by sender.
> >+      // Tells us that sender wants to start a file transfer.
> >+      if (aStanza.getChildren("si").length != 0) {
> >+        let norm = this._normalizeJID(aStanza.attributes["from"]);
> norm? Should this be from?
norm: normalised jid. I'll change it to from.
> >+        // FIXME UI is needed to be implemented for this.
> >+        // User can accept/deny the file transfer request.
> >+        // Currently sending a stanza to the sender to tell them that we
> >+        // are ready to receive.
> >+        // In future, we also need to know which method the user wants to use
> >+        // Presently sending IBB as we support it.
> >+        let supportedMethod = [];
> >+        supportedMethod.push(Stanza.node("value", null, null,
> >+                                         Stanza.NS.jabber_IBB));
> This would be a lot clearer as 
> let supportedMethod = [Stanza.node("value", null, null,
>                                    Stanza.NS.jabber_IBB)];
> This is true everywhere in the code, please make this change.
> 
> >+        let iqArg =[Stanza.node("x", Stanza.NS.jabber_X,{ type : "submit" },
> Nit: space after the = and ,, not after the {, before the : or }. Please follow
> these rules everywhere, I'm not going to comment on all of them.
Sorry will do.
> >+      // IBB Initiator request received.
> >+      // This is used to initiate the IBB stream.
> >+      if (aStanza.getChildren("open").length != 0) {
> >+        this.LOG("ATUL: IBB init received");
> Update this message to something more reasonable.
Removing it.
> >+        let norm = this._normalizeJID(aStanza.attributes["from"]);
> norm or from?
Again will change it to from.
> >+        // Get the sid to associate it with the correct file.
> >+        var sid = aStanza.getElement(["open"]).attributes["sid"];
> Why var? Look at this everywhere. I highly doubt we should be adding any vars.
OKay :)
> >+      // Data sent by sender through IBB method.
> >+      if (aStanza.getChildren("data").length != 0) {
> ...
> >+        if (!this._fileTransferMap.has(sID)) {
> >+          let iqArg = [Stanza.node("error", null, null,
> >+                                   Stanza.node("item-not-found",
> >+                                               Stanza.NS.item_not_found))];
> >+          let s = Stanza.iq("error", aStanza.attributes["id"],
> >+                          aStanza.attributes["from"], iqArg);
> >+          this._connection.sendStanza(s);
> Should an error be displayed to the user here?
Yes,I'll add it.
> 
> >+        let xmppFTInstance = this._fileTransferMap.get(sID);
> Does this really need "xmpp" in front of it?
Okay changing it to FTInstance

> >@@ -1395,8 +1564,303 @@ const XMPPAccountPrototype = {
> >     // Send the vCard only if it has really changed.
> >     if (this._userVCard.getXML() != existingVCard)
> >       this._connection.sendStanza(Stanza.iq("set", null, null, this._userVCard));
> >     else
> >       this.LOG("Not sending the vCard because the server stored vCard is identical.");
> >   }
> > };
> >+
> >+
> Nit: Only one empty line.
> 
> >+function XMPPFileTransfer(aConv, aFile, aIsUpload, aSessionID) {
> >+  // We need to upload the file.
> >+  if (aIsUpload)
> >+    this._init(aConv, aFile, aIsUpload);
> >+  else
> >+    this._init(aConv, aFile, aIsUpload, aSessionID);
> Won't aSessionID be undefined in the other case and you can just send it
> through to _init where it'll be undefined?
Yes. Will change

> >+XMPPFileTransfer.prototype =  {
> ...
> >+  // This is the amount of bytes that are transferred in one IQ stanza.
> >+  blockSize: null,
> Shouldn't this default to 4096 according to the XEP?
Okay I'll keep 4096 as default.
> >+  // Current pointer in the file for keeping track of how much we've read/written.
> >+  fileCounter: null,
> Shouldn't this default to 0, not null?
Defauting to 0
> >+  _init: function(aConv, aFile, aIsUpload, aSessionID) {
> >+    this.conv = aConv;
> >+    this.file = aFile;
> >+    GenericFileTransferPrototype._init.call(aConv, aFile, aIsUpload);
> >+    if (aIsUpload)
> >+      this.sendInitiatorRequest(aFile);
> >+    else {
> >+      this.sessionID = aSessionID;
> >+      this.conv._account._fileTransferMap.set(aSessionID, this);
> >+      let temp = this.conv._account._fileTransferMap.has(aSessionID);
> >+      Cu.reportError("Receiver: Starting a new ft session"+
> >+                     aSessionID + " | " + temp);
> >+    }
> >+  },
> Please put the contents of this into the constructor. There's no reason to
> override _init (and then force calling the GenericFileTransferPrototype
> version).
> 
> >+
> >+
> Nit: Only one empty line.
> 
> >+  sendInitiatorRequest: function(aFile) {
> >+    // We'll be sending file block by block.
> >+    let fileSize = aFile.fileSize;
> >+    let fileName = aFile.leafName;
> Do these really need to be saved here? They look like they're used only once.
Okay. using them directly.
> >+
> >+    // generate a unique one, but for testing, any will do.
> >+    // FIXME sessionID should be generated by current time/date
> >+    var sessionID = 'abcd1234!';
> Please fix this, it should be only a line or two.
> 
Okay :)

> >+    let child = [Stanza.node ("x", Stanza.NS.jabber_X, { type: "form" },
> >+                              Stanza.node("field", null,
> >+                                          { "var" : "stream-method",
> >+                                          type: "list-single" }, options))];
> Please no space after the { and before the } in an object, and not before the
> :, the above should be formatted as:
> >+    let child = [Stanza.node ("x", Stanza.NS.jabber_X, {type: "form"},
> >+                              Stanza.node("field", null,
> >+                                          {"var": "stream-method",
> >+                                           type: "list-single"}, options))];
> If it is not clear what I've changed, please ask. Please check this throughout
> this patch, there are other place this is wrong. I will not comment on the
> other ones.
I somehow thought you wanted to change this with spaces around { } everywhere. I actually changed from nospace to adding spaces. Will revert this change
> 
> >+  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 );
> Should this be displayed to the user?
I'm using it as debug statements, as I mentioned on irc. I'll remove them if you want me to. :)

> >+  initiateIbbSession: function() {
> >+    // FIXME: This should be user adjustable.
> >+    this.blockSize = Ci.prplIFileTransfer.DEFAULT_BLOCK_SIZE;
> I thought this is supposed to be 4096? Does this really need to be user
> adjustable, can't we "figure it out" ourselves?
Yes. I'll change this to be 4096 by default. I just thought of giving user a pref for this. But yes, we can figure this our ourself.  
> >+    this.fileCounter = 0;
> Why is this not 0 in the prototype?
Will change 
> >+          let arr = gDecoder.decode(array);
> I thought you renamed this? What are we actually decoding here anyway? What
> does this line do?
I did rename, I don't know how this is here :(

> Overall comments:
>  - Please fix your styling to match what we normally use. If you have questions
> about what that is, please ask.
Sure. I woke up just few minutes back I'll come on irc later and clear things up.
>  - I've repeated some of my review comments from my first review. Please make
> sure to read over all the review comments and to meet them before posting
> another patch. Thanks.
Seems like some of my changes are not reflected in this patch. I read all the comments twice and make corresponding changes.
>  - Please fix up all the reportError calls (either use the proper logging
> methods or remove them). I know you were working on doing this, but just
> reiterating.
Okay will remove them
*** Original post on bio 2037 at 2013-07-11 09:46:24 UTC ***


> > >+          let arr = gDecoder.decode(array);
> > I thought you renamed this? What are we actually decoding here anyway? What
> > does this line do?
> I did rename, I don't know how this is here :(
> 
This is fine when I see the diff of the patch here on bugzilla.
*** Original post on bio 2037 at 2013-07-11 09:53:24 UTC ***

(In reply to comment #11)
> > > >+          let arr = gDecoder.decode(array);
> > > I thought you renamed this? What are we actually decoding here anyway? What
> > > does this line do?
> > I did rename, I don't know how this is here :(
> > 
> This is fine when I see the diff of the patch here on bugzilla.

There's still one place where the old variable name is used. Just search for gDecoder.
*** Original post on bio 2037 at 2013-07-11 10:09:45 UTC ***

(In reply to comment #10)

> > >+  initiateIbbSession: function() {
> > >+    // FIXME: This should be user adjustable.
> > >+    this.blockSize = Ci.prplIFileTransfer.DEFAULT_BLOCK_SIZE;

Why is this in the prplIFileTransfer interface when it looks like it's XMPP specific?

> > I thought this is supposed to be 4096? Does this really need to be user
> > adjustable, can't we "figure it out" ourselves?
> Yes. I'll change this to be 4096 by default. I just thought of giving user a
> pref for this. But yes, we can figure this our ourself.  

Users don't want to read XEPs before transferring a file. They don't care (or even understand) what the block size is. We already said this several times on IRC.
*** Original post on bio 2037 at 2013-07-11 10:36:40 UTC ***

(In reply to comment #10)
> > >diff --git a/chat/protocols/xmpp/xmpp-xml.jsm b/chat/protocols/xmpp/xmpp-xml.jsm
> > >+  feature                   : "http://jabber.org/protocol/feature-neg",
> > >+  jabber_X                  : "jabber:x:data",
> > >+  jabber_bytestreams        : "http://jabber.org/protocol/bytestreams",
> > >+  jabber_IBB                : "http://jabber.org/protocol/ibb",
> > Florian suggested the jabber is redundant here.
> Yes, but I think, since they are jabber protocol, thus we should include that.
> Anyway I'll change these to X, bytestreams and IBB. I hope that will be okay.
> "X" look quite weird though. So what about "x_data"?
That's fine. Everything in this file is about Jabber/XMPP, the ones that are jabber.org don't mention jabber. I really don't have a strong opinion on this though, so do whatever Florian thinks is best.

> > Why generate the file here at all? I suspect conversations need a flag about
> > whether sendFile can be used or not. (Otherwise, I guess we can just throw?) We
> > could also make sendFile part of prplIConvIM instead of prplIConversation, but
> > theoretically some protocols could transfer to a MUC (yeah...IRC allows crazy
> > things like that).
> Okay. I'll remove the file generation. I thought we can support file transfer
> in MUC. Can we allow this in xmpp also. Something like if I drop a file in muc,
> it goes to all the users. This will make much more sense one we have
> implemented the fileLink feature.
Right, but if we don't currently do that (or know how it is going to be done), don't write half finished code.

> > >+  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 );
> > Should this be displayed to the user?
> I'm using it as debug statements, as I mentioned on irc. I'll remove them if
> you want me to. :)
Right, but you didn't answer my question of whether this should be displayed to the user or not.
*** Original post on bio 2037 at 2013-07-11 10:50:32 UTC ***

(In reply to comment #12)
> (In reply to comment #11)
> > > > >+          let arr = gDecoder.decode(array);
> > > > I thought you renamed this? What are we actually decoding here anyway? What
> > > > does this line do?
> > > I did rename, I don't know how this is here :(
> > > 
> > This is fine when I see the diff of the patch here on bugzilla.
> 
> There's still one place where the old variable name is used. Just search for
> gDecoder.

Sorry missed it. Will do the needful
*** Original post on bio 2037 at 2013-07-11 10:52:53 UTC ***

(In reply to comment #14)
> (In reply to comment #10)
> > > >diff --git a/chat/protocols/xmpp/xmpp-xml.jsm b/chat/protocols/xmpp/xmpp-xml.jsm
> > > >+  feature                   : "http://jabber.org/protocol/feature-neg",
> > > >+  jabber_X                  : "jabber:x:data",
> > > >+  jabber_bytestreams        : "http://jabber.org/protocol/bytestreams",
> > > >+  jabber_IBB                : "http://jabber.org/protocol/ibb",
> > > Florian suggested the jabber is redundant here.
> > Yes, but I think, since they are jabber protocol, thus we should include that.
> > Anyway I'll change these to X, bytestreams and IBB. I hope that will be okay.
> > "X" look quite weird though. So what about "x_data"?
> That's fine. Everything in this file is about Jabber/XMPP, the ones that are
> jabber.org don't mention jabber. I really don't have a strong opinion on this
> though, so do whatever Florian thinks is best.
okay I'll make them without the "jabber". I intentionally didn't change that.
> 
> > > Why generate the file here at all? I suspect conversations need a flag about
> > > whether sendFile can be used or not. (Otherwise, I guess we can just throw?) We
> > > could also make sendFile part of prplIConvIM instead of prplIConversation, but
> > > theoretically some protocols could transfer to a MUC (yeah...IRC allows crazy
> > > things like that).
> > Okay. I'll remove the file generation. I thought we can support file transfer
> > in MUC. Can we allow this in xmpp also. Something like if I drop a file in muc,
> > it goes to all the users. This will make much more sense one we have
> > implemented the fileLink feature.
> Right, but if we don't currently do that (or know how it is going to be done),
> don't write half finished code.

Okay. So I'll just report an error in this case.

> Right, but you didn't answer my question of whether this should be displayed to
> the user or not.

I do want to report this to user. We can even have a ui for this if you want later. But for now, I guess I can just show the error.
*** Original post on bio 2037 at 2013-07-11 10:54:02 UTC ***

(In reply to comment #13)
> (In reply to comment #10)
> 
> > > >+  initiateIbbSession: function() {
> > > >+    // FIXME: This should be user adjustable.
> > > >+    this.blockSize = Ci.prplIFileTransfer.DEFAULT_BLOCK_SIZE;
> 
> Why is this in the prplIFileTransfer interface when it looks like it's XMPP
> specific?
> 
> > > I thought this is supposed to be 4096? Does this really need to be user
> > > adjustable, can't we "figure it out" ourselves?
> > Yes. I'll change this to be 4096 by default. I just thought of giving user a
> > pref for this. But yes, we can figure this our ourself.  
> 
> Users don't want to read XEPs before transferring a file. They don't care (or
> even understand) what the block size is. We already said this several times on
> IRC.

Removing the block size from the interface. And making the default value of block size to be 4096.
*** Original post on bio 2037 at 2013-07-11 10:58:33 UTC ***

(In reply to comment #16)
> > > Okay. I'll remove the file generation. I thought we can support file transfer
> > > in MUC. Can we allow this in xmpp also. Something like if I drop a file in muc,
> > > it goes to all the users. This will make much more sense one we have
> > > implemented the fileLink feature.
> > Right, but if we don't currently do that (or know how it is going to be done),
> > don't write half finished code.
> 
> Okay. So I'll just report an error in this case.
You probably don't want to override this and throw in this case.

> > Right, but you didn't answer my question of whether this should be displayed to
> > the user or not.
> 
> I do want to report this to user. We can even have a ui for this if you want
> later. But for now, I guess I can just show the error.

Every time you think "We can add UI for this!" I want you to take a step back and ask yourself "Do we already have UI that will suit this?" In this case, the answer is "yes!" just display a system/error message in the conversation that the file transfer failed. This should be pretty easy to do.
*** Original post on bio 2037 as attmnt 2572 at 2013-07-11 14:39:00 UTC ***

Tried to address all the comments. Hopyfully did the justice to the comments.
Attachment #8354340 - Flags: feedback?(clokep)
Attachment #8354340 - Flags: feedback?(florian)
Comment on attachment 8354339 [details] [diff] [review]
Backend for ft implemented without proper error handling

*** Original change on bio 2037 attmnt 2571 at 2013-07-11 14:39:29 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354339 - Attachment is obsolete: true
Attachment #8354339 - Flags: feedback?(florian)
Comment on attachment 8354340 [details] [diff] [review]
Backend for ft implemented without proper error handling v3

*** Original change on bio 2037 attmnt 2572 at 2013-07-11 15:10:52 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354340 - Flags: feedback?(clokep) → feedback+
*** Original post on bio 2037 at 2013-07-11 15:10:52 UTC ***

Comment on attachment 8354340 [details] [diff] [review] (bio-attmnt 2572)
Backend for ft implemented without proper error handling v3

>diff --git a/chat/protocols/xmpp/xmpp.jsm b/chat/protocols/xmpp/xmpp.jsm
> const EXPORTED_SYMBOLS = [
>   "XMPPConversationPrototype",
>   "XMPPMUCConversationPrototype",
>   "XMPPAccountBuddyPrototype",
>-  "XMPPAccountPrototype"
>+  "XMPPAccountPrototype",
>+  "XMPPFileTransferPrototype"
This doesn't seem to exist...

>@@ -30,20 +32,35 @@ XPCOMUtils.defineLazyGetter(this, "FileU
> XPCOMUtils.defineLazyGetter(this, "NetUtil", function() {
>   Components.utils.import("resource://gre/modules/NetUtil.jsm");
>   return NetUtil;
> });
> XPCOMUtils.defineLazyServiceGetter(this, "imgTools",
>                                    "@mozilla.org/image/tools;1",
>                                    "imgITools");
> 
>+// A text decoder used for decoding the data read from the file into an array
Nit: Period at the end of all comments, always. I will not repeat this throughout the file. Please check EVERY comment you're adding before uploading a new patch. (Also for proper grammar and starting with a capital letter).
> XPCOMUtils.defineLazyGetter(this, "_", function()
>   l10nHelper("chrome://chat/locale/xmpp.properties")
> );
> 
>+
Nit: Only one blank line between functions, everywhere, always. I will not repeat this.

>@@ -806,16 +835,140 @@ const XMPPAccountPrototype = {
>       for each (let qe in aStanza.getChildren("query")) {
>         if (qe.uri != Stanza.NS.roster)
>           continue;
> 
>         for each (let item in qe.getChildren("item"))
>           this._onRosterItem(item, true);
>         return;
>       }
>+
>+      // Stream initiator request by sender.
>+      // Tells us that sender wants to start a file transfer.
>+      if (aStanza.getChildren("si").length != 0) {
>+        let from = this._normalizeJID(aStanza.attributes["from"]);
>+        let fileStanza = aStanza.getElement(["si", "file"]);
>+
>+        // Sending the notification to UI.
>+        this._conv[from].fileReceived();
>+
>+        // Creating a new file where the data will be stored.
>+        let file = Services.dirsvc.get("DfltDwnld", Ci.nsIFile);
>+        file.append(fileStanza.attributes["name"]);
>+
>+        // We need to have a unique file for File transfer in case where a 
>+        // file with same name already exists.
>+        file.createUnique(Ci.nsIFile.NORMAL_FILE_TYPE, FileUtils.PERMS_FILE);
>+
>+        // Getting the session ID from the stanza.
>+        let sessionID = aStanza.getElement(["si"]).attributes["id"];
>+
>+        // Create a new XMPPFileTransfer instance that will handle the
>+        // file transfer for this user as receiver.
>+        let fileTransfer = new XMPPFileTransfer(this._conv[from], file, false, sessionID); 
>+
>+        // FIXME UI is needed to be implemented for this.
>+        // User can accept/deny the file transfer request.
>+        // Currently sending a stanza to the sender to tell them that we
>+        // are ready to receive.
>+        // 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)];
>+        let iqArg = [Stanza.node("x", Stanza.NS.x_data, {type: "submit"},
>+                                Stanza.node("field", null,
>+                                            {"var": "stream-method"}, supportedMethod))];
Nit: The lines above is missing a space such that Stanza lines up with "x" above it.

>+      // IBB Initiator request received.
>+      // This is used to initiate the IBB stream.
>+      if (aStanza.getChildren("open").length != 0) {
...
>+        // Get the blocksize
>+        let blockSize = aStanza.getElement(["open"]).attributes["block-size"];
This seems unused?

>+      // Data sent by sender through IBB method.
>+      if (aStanza.getChildren("data").length != 0) {
...
>+          let iqArg = [Stanza.node("error", null, null,
>+                                   Stanza.node("item-not-found",
>+                                               Stanza.NS.stanzas))];
>+          let s = Stanza.iq("error", aStanza.attributes["id"],
>+                          aStanza.attributes["from"], iqArg);
Nit: Align aStanza with "error".

>+      // IBB close request received by the sender.
>+      if (aStanza.getChildren("close").length != 0) {
I'm tempted to say !aStanza.getChildren("close").length, but some people dislike that.

>@@ -1395,8 +1552,258 @@ const XMPPAccountPrototype = {
>+function XMPPFileTransfer(aConv, aFile, aIsUpload, aSessionID) {
I thought we decided to rename this IBBFileTransfer?

>+  this._init(aConv, aFile, aIsUpload);
>+  // FIXME Needs a change in interface which will be done in different bug.
>+  this.file = aFile;
>+  this.conv = aConv;
Aren't these two lines handled inside of _init? The abstract class seems to have _conv and _targetFile defined, you should be using those (also I dislike those names...).

...
>+}
>+
>+XMPPFileTransfer.prototype =  {
Nit: No empty line between a class and it's prototype.

>+    let mime = 'text/plain';
Did you verify if this is always correct?

>+    let child = [Stanza.node ("x", Stanza.NS.x_data, {type: "form"},
>+                              Stanza.node("field", null,
>+                                          {"var": "stream-method",
>+                                          type: "list-single"}, options))];
Nit: type should align with "var", and why does one have quotes and the other doesn't?

>+    let iqArg = [Stanza.node("si", Stanza.NS.si,
>+                             {id: sessionID, "mime-type": mime,
>+                             profile: Stanza.NS.file_transfer_profile},
>+                             children)];
Nit: profile should align with id.

>+  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 );
I thought we decided that this should print an error in the conversation? This is trivial, please include it in the next patch.

>+      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"]);
>+    let methodText = method.innerText;
>+    if (methodText == Stanza.NS.ibb) {
>+      this.initiateIbbSession();
>+      return;
>+    }
>+    Cu.reportError("Method not supported " + methodText);
Isn't this the same case as above, essentially? (I.e. the remote client doesn't support file transfer.)

>+  initiateIbbSession: function() {
>+    let s = Stanza.iq("set", null, this.conv.to,
>+                      Stanza.node("open", Stanza.NS.ibb,
>+                                  { "block-size": this.blockSize,
>+                                  "sid": this.sessionID, "stanza": "iq"}));
Nit: Spacing here.

>+  sendChunk: function() {
>+    this._filePromise.read(this.blockSize).then(
>+      function readSuccess(array) {
>+       if (array.length != 0) {
>+          // Decoding the data we just read from file into arr.
>+          let arr = textDecoder.decode(array);
I'm a little bit confused at what we're decoding. Don't we want to treat this data as binary, not as text?

The following looks like code duplication with the above:
>+    // Reading again and sending the data.
>+    this._filePromise.read(this.blockSize).then(
>+      function readSuccess(array) {
...
>+          let s = Stanza.iq("set", null, this.conv.to,
>+                          Stanza.node("data", Stanza.NS.ibb,
>+                                      {"seq": this.seqID,
>+                                      "sid": this.sessionID}, chunk));
Nit: Spacing.

>+  // This is called by the user when we are receiving.
>+  // This functions writes to the file.
>+  writeChunk: function(aChunk, aStanza) {
>+    let dataToWrite = new Uint8Array(BytesToArrayBuffer(StringToBytes(aChunk)));
I doubt you need the Uint8Array here, it probably doesn't want a ArrayBufferView, but an ArrayBuffer.

>\ No newline at end of file
Nit: Please end all files in newlines.

Where are all the other changes? Will they be attached somewhere else?
*** Original post on bio 2037 as attmnt 2577 at 2013-07-12 02:26:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354345 - Flags: feedback?(clokep)
Attachment #8354345 - Flags: feedback?(florian)
Comment on attachment 8354340 [details] [diff] [review]
Backend for ft implemented without proper error handling v3

*** Original change on bio 2037 attmnt 2572 at 2013-07-12 02:26:05 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354340 - Attachment is obsolete: true
Attachment #8354340 - Flags: feedback?(florian)
Comment on attachment 8354345 [details] [diff] [review]
Backend for ft implemention in xmpp

*** Original change on bio 2037 attmnt 2577 at 2013-07-12 12:18:02 UTC ***

>diff --git a/chat/protocols/xmpp/xmpp.jsm b/chat/protocols/xmpp/xmpp.jsm
>@@ -30,20 +31,34 @@ XPCOMUtils.defineLazyGetter(this, "FileU
>+// A text decoder used for decoding the data read from the file into an array.
>+XPCOMUtils.defineLazyGetter(this, "textDecoder", function () {
>+  return new TextDecoder();
>+});

>@@ -806,16 +834,161 @@ const XMPPAccountPrototype = {
>+      // Sender has gives a stream initiator request.
>+      // This tell us that the sender wants to start a file transfer.
This comment doesn't make much sense to me.


>+        // In future, we also need to know which method the user wants to use
Nit: Dot at the end of the sentence.

>+        // Sending the notification to UI.
>+        let self = this;
>+        let fileOffer = {
Should this be in jsProtoHelper at all or no?

>+          fileName: fileTransfer.fileName,
>+          fileSize: fileTransfer.fileSize,
>+          sender: from,
Something I don't like about this is that it won't show the sender's alias if they have one. I'm not sure if we care or not though.

>@@ -1395,8 +1576,239 @@ const XMPPAccountPrototype = {
>+function IBBFileTransfer(aConv, aFile, aIsUpload, aSessionID) {
>+  this._init(aConv, aFile, aIsUpload);
>+  if (aIsUpload)
>+    this.sendInitiatorRequest(aFile);
>+  else {
>+    if (aSessionID)
>+      this.sessionID = aSessionID;
What if there is not a session ID here? Won't the call below fail?

>+    aConv._account._fileTransferMap.set(aSessionID, this);
Shouldn't this be added to the map whether it's an upload or not?

>+IBBFileTransfer.prototype =  {
>+  // This is the name of the received file. This is different from the file to
>+  // which we will be storing the data
Nit: Dot at the end of the sentence.

>+  sendInitiatorRequest: function(aFile) {
Can we get a comment saying what this function does? (And the other ones in here to, like initiateIbbSession, etc.)

>+  initiateIbbSession: function() {
>+    let s = Stanza.iq("set", null, this.conv.to,
>+                      Stanza.node("open", Stanza.NS.ibb,
>+                                  {"block-size": this.blockSize,
>+                                  "sid": this.sessionID, "stanza": "iq"}));
Nit: "sid" needs to line up with "block-size".

Does this patch have the "working" binary stuff in it or no?

Also, an FYI on this patch...Florian and I discussed it and we're not positive we want to land IBB (which is a pretty bad file transfer mechanism and is meant as a fallback for other XMPP FT mechanisms) without supporting at least one of the other "better" ones. We're definitely going to use this for testing, but we might not polish this 100% so we can start working on the FileLink fallback.
Attachment #8354345 - Flags: feedback?(clokep) → feedback+
*** Original post on bio 2037 at 2013-07-12 19:58:29 UTC ***

(In reply to comment #22)
> >+        // Sending the notification to UI.
> >+        let self = this;
> >+        let fileOffer = {
> Should this be in jsProtoHelper at all or no?
I guess not. But again, if this is meant to be in jsProtoHelper, this can be done. OR maybe when we /need/ it there, then we can add it.
> 
> >+          fileName: fileTransfer.fileName,
> >+          fileSize: fileTransfer.fileSize,
> >+          sender: from,
> Something I don't like about this is that it won't show the sender's alias if
> they have one. I'm not sure if we care or not though.
I don't have any preference. I'll do whatever you guys want me to do.
> 
> >@@ -1395,8 +1576,239 @@ const XMPPAccountPrototype = {
> >+function IBBFileTransfer(aConv, aFile, aIsUpload, aSessionID) {
> >+  this._init(aConv, aFile, aIsUpload);
> >+  if (aIsUpload)
> >+    this.sendInitiatorRequest(aFile);
> >+  else {
> >+    if (aSessionID)
> >+      this.sessionID = aSessionID;
> What if there is not a session ID here? Won't the call below fail?
If it's a download we /need/ to pass sessionID. 

> >+    aConv._account._fileTransferMap.set(aSessionID, this);
> Shouldn't this be added to the map whether it's an upload or not?
No. In case of upload sessionID will be generated later, and then add it to the map. While in case of download we get the sessionID from the sender in the stanza. Thus we /need/ session ID for the else condition.
One thing that can be done is to generate the sessionID for upload in the constructor function. And then we can add it to the map in the constructor only. And while we are in the case when aIsDownload is false, we can throw an error to the console stating that session ID is needed in this case, and then returning. Obviously we cannot add it to the map without a sessionID. 


> Does this patch have the "working" binary stuff in it or no?
No.

> Also, an FYI on this patch...Florian and I discussed it and we're not positive
> we want to land IBB (which is a pretty bad file transfer mechanism and is meant
> as a fallback for other XMPP FT mechanisms) without supporting at least one of
> the other "better" ones. We're definitely going to use this for testing, but we
> might not polish this 100% so we can start working on the FileLink fallback.
Okay. I'm initiating the work on fileLink today.
*** Original post on bio 2037 at 2013-07-12 21:54:48 UTC ***

(In reply to comment #23)
> > >@@ -1395,8 +1576,239 @@ const XMPPAccountPrototype = {
> > >+function IBBFileTransfer(aConv, aFile, aIsUpload, aSessionID) {
> > >+  this._init(aConv, aFile, aIsUpload);
> > >+  if (aIsUpload)
> > >+    this.sendInitiatorRequest(aFile);
> > >+  else {
> > >+    if (aSessionID)
> > >+      this.sessionID = aSessionID;
> > What if there is not a session ID here? Won't the call below fail?
> If it's a download we /need/ to pass sessionID. 
Umm...that's not what you're checking at all, you check if it's an upload, then in the download part (the else statement) you check if aSessionID exists, if it doesn't shouldn't we throw an error or something? You just continue on merrily if that's undefined. Why are we even checking it then?
*** Original post on bio 2037 at 2013-07-12 22:01:15 UTC ***

(In reply to comment #24)
> (In reply to comment #23)
> > > >@@ -1395,8 +1576,239 @@ const XMPPAccountPrototype = {
> > > >+function IBBFileTransfer(aConv, aFile, aIsUpload, aSessionID) {
> > > >+  this._init(aConv, aFile, aIsUpload);
> > > >+  if (aIsUpload)
> > > >+    this.sendInitiatorRequest(aFile);
> > > >+  else {
> > > >+    if (aSessionID)
> > > >+      this.sessionID = aSessionID;
> > > What if there is not a session ID here? Won't the call below fail?
> > If it's a download we /need/ to pass sessionID. 
> Umm...that's not what you're checking at all, you check if it's an upload, then
> in the download part (the else statement) you check if aSessionID exists, if it
> doesn't shouldn't we throw an error or something? You just continue on merrily
> if that's undefined. Why are we even checking it then?

Yes. I will throw an error in this case and not add it to the fileTransfermap. I guess that should resolve the whole situation here.
Attached patch Backend for ft — — Splinter Review 
*** Original post on bio 2037 as attmnt 2588 at 2013-07-13 00:24:00 UTC ***

Updated patch
Attachment #8354357 - Flags: feedback?(clokep)
Attachment #8354357 - Flags: feedback?(florian)
Comment on attachment 8354345 [details] [diff] [review]
Backend for ft implemention in xmpp

*** Original change on bio 2037 attmnt 2577 at 2013-07-13 00:24:10 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354345 - Attachment is obsolete: true
Attachment #8354345 - Flags: feedback?(florian)
Comment on attachment 8354357 [details] [diff] [review]
Backend for ft

*** Original change on bio 2037 attmnt 2588 at 2013-09-09 23:58:24 UTC ***

Clearing out my review queue, please re-request f? if you plan to continue to work on this.
Attachment #8354357 - Flags: feedback?(clokep)
Comment on attachment 8354357 [details] [diff] [review]
Backend for ft

I don't think this is in a reviewable state.
Attachment #8354357 - Flags: feedback?(florian)
Assignee: atuljangra66 → nobody
URL: http://xmpp.org/extensions/xep-0096.html
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → DUPLICATE
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: