leaving hidden while we work out some details...

start of a component category system for registering cloud file providers. My plan is to flesh out the YouSendIt component just to start somewhere, and see how the prototyping goes.

this fixes the component registration and category handling for the cloud file providers, and gets the yousendit component loading. The component implements the getting the auth token part of login, though it's untested. It's also not completely hooked up to the UI correctly yet, which is why it's not tested.

finally figured out how to do the dynamic menus, and try to do a logon to yousendit. The logon fails, so I have to figure out why that is next.

this gets us all the way to sending a login request to yousendit, but yousendit complains that the signature doesn't match what it expects. I'm going to have to sit down and debug their sample code to see what's different about what we're sending.

This gets us to the uploadFile part of the yousendit process - it fails complaining about an invalid content type; I'm not sure if this is because necko is adding charset=utf8 to my content-type string. I had to disable the normal attaching of files because my xul fu is not good enough to stop my dynamic menus from executing the command for the button...

Irving, the patch has a sample manifest file for a js component.

This adds a bunch of code to do oauth (the authentication part of dropbox's api). It doesn't quite work yet, since I haven't gotten the UI part working, where the browser window gets the redirect and tells the oauth listener that the app has been authorized. I stopped working on yousendit for a few days since their test infrastructure was down last week, though it was working again by Friday evening.

oauth is still broken (I don't seem to be getting a redirect url as per the spec), but yousendit is returning a url for an uploaded attachment. Now I need to figure out what the compose UI should do with the url. The natural approach would be to use nsMsgCompfields.attachments, and augment the nsIMsgAttachment interface to know about cloud files. This would mean the backend c++ code would need to know about cloud files, to some extent. An alternative is to try to put almost all the smarts for cloud files in the compose window js UI code. That, however, would make it extremely difficult to try to incorporate uploading big files into the send in background framework, since there's no compose window by the time we get to send in background. And we need to support things like saving as draft, and sending, which means we need to be able to persist the big file upload state. The more ambitious we are with changing the whole compose and send infrastructure to make the big file upload process as transparent as possible, the greater the schedule risk, of course. And the more the backend processes need to know about big files, keeping in mind that we don't know the download url until the upload has finished. Save as draft/send in background all assume the message has been completely constructed, including the url for downloading a big file, so things are much easier if we keep things simple and simply hide the compose window while we finish the upload to the cloud. But I've had a lot of resistance to that approach in the past, so that may not be an option.

In order to make editing drafts with big files work correctly, I believe I'm going to have to make big files their own attachments - I can't just stick a div in the html and have the draft code parse the html and recreate the attachment info, I don't think. In theory, the compose window could grovel through the html in the editor, but that feels fragile, if we change the html we generate. But I'm not sure how other e-mail clients are going to handle displaying multiple html parts - we might need to append the big file info to the main message body *and* generate separate html parts. I think most clients should handle multiple inline html parts just fine, however, so it's probably worth checking the major e-mail, web mail, and mobile clients.

nsMsgSendPart::Write() is where we write out attachments to the message file, before sending it. This may be where we want to check for cloud attachments, and/or we may want to do something in PreProcessAttachments to avoid turning the attached file into a temp file, and we may want to munge the part into an html part.

At this point, I'm just trying to propagate the download url and the fact that the attachment is really just a link to the cloud to the code that should do different things based on that. I haven't gotten to the part where we actually insert a link into the e-mail yet.

Turns out that we figure out all the interesting information about file attachments when we fetch them (content type, etc). Since we're not fetching cloud files, we can't easily decorate the big file part with the same interesting info as we do for real attachments. It's beginning to look like it's just going to be a simple url, at least in the first go-round.

Would it be possible to create a fake/unused attachment part that stored that data, so that we could fetch it later to display? (My other thought is that if we added a unique id to the attachment part, we could duplicate that in the editor, and it could help us grovel through the html we generate to more easily find the attachment bits in a draft, maybe…)

(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #13) > Would it be possible to create a fake/unused attachment part that stored > that data, so that we could fetch it later to display? We don't have that information because it's determined by the actual fetching of the attachment to construct the message. And we're not doing that for cloud files. It's probably not an insurmountable problem, but I don't have time for it. I'm talking about things like the content type for an attachment. > > (My other thought is that if we added a unique id to the attachment part, we > could duplicate that in the editor, and it could help us grovel through the > html we generate to more easily find the attachment bits in a draft, maybe…) I would just use the download url for this. Stick that in a header in the attachment part, and then grovel through the html for the same url. I suppose the editor ui is going to need to learn that when editing a draft, if the user removes a big file from the attachment pane (or converts it to a normal attachment), then we need to remove url from the html. Non-html compose will have to be handled a bit differently, in that we'll need to grovel specifically for the url in the plain text. Or we could simply disable big files for people who don't use html compose :-)

Looking into it a bit more, I'm starting to think the compose window front end should handle inserting the url into the message body. I had hoped that doing it in the backend would allow us to do more things in the background, as part of the send in background process, but I don't think that's at all doable with the current schedule. The other thing is that the link(s) need to be inserted before the signature, if any, and the backend code doesn't know anything about signatures. I'll have a whack at it, and whoever does the front end code can fix whatever I do wrong.

just an FYI, the urls returned by yousendit don't and won't work until we switch from using the development test server to the production server. So we can't test this end to end until we switch to the production server, unfortunately.

the gets oauth working (step 1 of the 7 or so for getting dropbox to work)

CCing a couple of needed folks. On my end, the plan is to get bug 708982 finished up first, since that will make the "detect large attachments as they're attached" feature less invasive in the code. After that, maybe we should pull out all the front-end changes here into a separate patch, and I can work on that and improve it. That should minimize Bienvenu and I tripping over each other. That said, I can't guarantee that I'll get the front-end stuff done by the 20th, since I've only got so much time (somewhat less now that the holidays are approaching).

squib: If there's anything you need me to tackle to help you with this, let me know. Want me to have a go at bug 708982? -Mike

(In reply to Mike Conley (:mconley) from comment #19) > If there's anything you need me to tackle to help you with this, let me > know. Want me to have a go at bug 708982? I've got a mostly-finished patch for that already, but you're welcome to review it when I post it (hopefully this evening).

Here's some code for the notification that pops up when you attach a big file. None of the buttons do anything yet, but the rest of it's done. I picked 1 MB as the threshold for notifying users, but that's just a pref, so it's easily changed.

(In reply to David :Bienvenu from comment #22) > Created attachment 581172 [details] [diff] [review] > more work on dropbox...fix http issue with yousendit Would it be possible for you to split out the UI parts of this patch? I'd do it myself, but I figure I'd end up bitrotting you.

(In reply to Jim Porter (:squib) from comment #23) > > Would it be possible for you to split out the UI parts of this patch? I'd do > it myself, but I figure I'd end up bitrotting you. Yes, I can do that - there are probably still changes I'm going to need to make to MsgComposeCommands.js, but they should be relatively small. I suppose I can just attach those as diffs to the current UI patch. I'll be including the oauth browser files in the UI patch since that UI needs to be cleaned up a lot - I just hacked something together to be able to make progress. I'm not sure what to do with the Makefiles and jar files and such - I don't think I'll be changing those, but I'm not sure. You're probably more likely to be adding new files.

I seem to have broken oauth trying to get dropbox uploading working - not sure what's going on there. So I'm going to back up to a previous working version and see if I can figure it out. But checkpointing this in the meantime. Jim, I'll try to break out the patch into UI later today - I'd like to have a few things (or anything) working first.

I went back to a previous patch where oauth was working and cleaned it up a little, so I could compare it to what the patch where oauth doesn't work. Just saving it here for posterity.

I seem to have completely horked my dropbox account, so I'll just checkpoint the code I have so far and work on separating the patch into UI and backend.

I think the line endings are screwed up in the dtd file for some reason, sorry about that.

Hmm, I'm getting a build failure with the backend patch: creating mail/components/cloudfile/Makefile /thunderbird/comm-central/mozilla/build/autoconf/make-makefile: Cannot read /thunderbird/comm-central/mozilla/mail/components/cloudfile/Makefile.in: No such file or directory

You'll need to force a configure to happen which will rebuild those makefiles. Unfortunately that's a build system issue.

(In reply to Jim Porter (:squib) from comment #30) > Hmm, I'm getting a build failure with the backend patch: > > creating mail/components/cloudfile/Makefile > /thunderbird/comm-central/mozilla/build/autoconf/make-makefile: Cannot read > /thunderbird/comm-central/mozilla/mail/components/cloudfile/Makefile.in: No > such file or directory I've also found that I need to explicitly make in objdir/mail/components/cloudfile in order to pick up changes to the dropbox/yousendit implementations. I haven't had time to figure out why, unfortunately.

Jim, the biggest change from your p.o.v. is that I've removed the debug code that hardcoded the fact that we have accounts. Now you either need to tell the cloud provider that it has an account (provider.hasAccount = true;) or set the bool pref manually - mail.yousendit.hasAccount and mail.dropbox.hasAccount. Ideally, we'd try to figure out if the user has a dropbox/yousendit account on their machine, but obviously we don't have time to do that for 1.0 I need to remove the hard-coded use of Sifry's account for YouSendIt. I also need to implement the yousendit api that creates a new account. This also gets the dropbox impl a bit further though it's still pretty much completely broken.

Ah, I remember now that I did implement createNewAccount for YouSendIt though it's completely untested.

An other note - though uploading is starting to work for text files, binary files won't work at all. Figuring out how to get js and xpcom to work with binary data might be beyond my paltry js skills, but I'll give it a try. It looks like dropbox wants binary data uploaded but perhaps I'm not reading between the lines of the documentation correctly. I'd hate to have to start over in c++.

this gets some binary file uploads working with dropbox - note to self, dropbox seems very unhappy with the filename "$(compile_pdbfile).pdb" so I need to figure out why exactly. Haven't figured out how to do binary files with YouSendIt - I can't use the same tricks I used with dropbox because I need to mix some text content with the file body - I suspect I'm going to have to do some sort of text encoding, which means we'll lose the nice speedup we get uploading binary files w/o encoding.

Here are some updates to the UI patch. Nothing much works, but I merged in my notification UI, fixed some entity issues, and cleaned up a few things here and there.

I've been banging my head against yousendit all day - I think the errors are because yousendit servers are not happy right now, but I don't know for sure - this isn't one of the usual failures I've been seeing. In any case, this checkpoint adds the ability to upload binary files to yousendit (or, at least it tries; I don't know if it's right or not). I've also fixed some of the xml parsing so finally I can figure out the available storage for the user, along with the max upload size. Currently, I only populate those values if the frontend calls uploadFile, which isn't terribly useful the first time the user tries to use bigFiles in a session. I could kick off a userInfo command if the front end asks for the available storage/max upload size, but that would be a potentially async request, which complicates the interface. I suppose I could check the values in the backend, and return an error to the front end, though it's cleaner if the frontend can preflight this and not have to decipher whatever error I return. And however I do it is probably how the other providers will have to do it, so I could be making them do more work. I'm also not refreshing the values after the user uploads files...I could make the front end call an async userInfo method before every upload, and after that, we'd guarantee that we know the available storage and whatever other stuff is interesting to know about the user's privileges.

OK, yousendit problem is that they don't like Content-Transfer-Encoding binary. And they don't appear to like binary files w/o that header either. So I have to figure out how to give them binary files.

this gets binary files uploading on yousendit - I can't tell if the integrity of the files is maintained because their servers are quarantining the files checking for viruses for a long time (I suspect something is horked since it's taking a lot longer than it used to..). But for now, I'll just pretend it's all working fine. There's a ton more to do, like handling user names and passwords for yousendit, handling all sorts of error cases that we blow off right now, handling multiple file uploads (I think the providers will probably have to do the queuing, unless the front end volunteers to do the serialization. I also haven't gotten very far w/ adding an html part for big file attachments, though maybe that can slide for 1.0 since we need the url in the message body anyway. I haven't done anything about propagating expiration info, since my understanding is that JB is going to make expiration go away. But I might add a way for the front end to get the expiration string for a given upload file just in case.

This adds a bit of password prompt handling to yousendit. And it fixes the remember password aspect of YouSendIt to be per-username. I also added code to handle expired drop box auth tokens, though I don't know to expire an auth token. Jim, please note that I changed MsgComposeCommands.js so that it does *not* pass a password into nsIMsgCloudFileProvider.init(). The usernames are still hard-coded because the frontend code is going to need to pass in the correct username to nsIMsgCloudProvider.init since only the front end knows which account the user picked. It might still be useful to have a password parameter for init to handle the initial account setup case where the user gives us a password. I'm using the password manager to store the password, by remembering the password as https://<uri encoded username>@<providerUrl>. I wasn't sure where we landed on multiple user accounts per provider, since Blake pointed out that providers would most likely see that as an attempt to circumvent quotas. But the UI spec has them, so I guess that's what we're doing. The way I see this all working is that the frontend creates a cloud provider object for every single file upload, with the corresponding username (a provider per file upload means the providers don't have to worry about serializing upload requests). The cloud provider will handle prompting for and caching the password, if appropriate, for that user. The DropBox provider won't because Thunderbird never actually sees the Dropbox password, since that's entered on the DropBox web page as part of oauth. But the drop box provider will remember the user's auth token in prefs.js. Jim/Mike, I don't know how you want to handle accounts. I'd recommend that the code that provisions accounts store them in preferences, similar to the what we do for mail server accounts. I'm storing user auth tokens/secrets for dropbox in the prefs as mail.dropbox.<username>.authToken and authSecret so something similar for other per-user prefs would be consistent (I'm not sure what characters are illegal for pref names, but we probably need to actually store the usernames as some sort of encoded pref value for our international users). Whatever scheme you decide on, I can fix the backend code to use it. As far as provider account settings are concerned, there aren't any settings that the backend cares about that I can see for either Dropbox or YouSendit, other than the username and password. The YouSendIt mockups talk about proxy servers, but my extremely limited understanding of Thunderbird's proxy support is that it's handled in our proxy ui settings, options, advanced settings, network and disk space, settings... button

Here are some more fixes to the UI patch. I fixed that issue where two file prompts showed up (one to attach via Dropbox and one to attach regular-style) as well as some more cleanup to the code. Some questions/comments: * Should the "Attach via Cloud" submenu have an item to add a new account? The design doc doesn't describe how a user would set this up except when the little notification bar prompts them. What about the case where a user knows they want to set this up and doesn't want to wait until they've attached a sufficiently-large file? Maybe this should just be in settings somewhere? * What happens / what should happen when a user attaches a file to the cloud but then decides to delete that attachment or just not send the mail? * As far as multiple accounts per service go, I think we definitely need them, though maybe not for Dropbox/Yousendit. I'd certainly expect to be able to have multiple SFTP accounts, for instance. * The configuration code should probably be per-provider, since I can't imagine there's a whole lot of overlap. It sounds like Dropbox needs a mini-browser (for OAuth), but Yousendit just needs a regular login dialog. The pages to tweak your settings are probably equally disjoint. * As for the whole "we need a modal dialog for initial setup", I'm not sure that's actually necessary, since we could just open the settings tab and have the compose window listen for some notifications that indicate successful setup.

Jim, can you incorporate the changes I made to MsgComposeCommands.js into your patch, in particular not hard-coding the passwords? Or I can do it to your latest patch, and attach it here, if you'll carry it forward.

I can do that; I'll update the patch this evening. While I'm here, I'm going to list some of the subtleties I know I'll have to deal with, so I don't forget about them: * Drag and drop cloud files from composer windows to other composer windows (currently, the "cloud-ness" will be lost, I think) * Override icons in the attachment bucket to show the cloud host (not sure how best to handle this yet; maybe return an array of the newly-added rows in the bucket?)

Re dropbox filename handling, I'm calling encodeURIComponent on the filename, but even that's not quite sufficient because DropBox expects the single quote character to be escaped, and encodeURIComponent doesn't do that. I'll try to get some information about exactly what characters they expect to be encoded since it wouldn't surprise me if there are others.

One issue I encountered but forgot to mention: in nsIMsgCloudFileProvider.idl, the constants at the bottom are out of range for longs and the IDL compiler throws a fit (for the time being I just made the values smaller).

Here's an updated patch to remove the hardcoded password. Unfortunately, I'm having build issues with the backend patch (it can't find nsinstall), so I haven't done much else with this yet...

Bienvenu, about the storage of account info: it seems to me that this should be handled in the backend. Right now, it seems like the frontend would set "hasAccount" to true, but is then responsible for handing off the account info to the backend every time a file gets attached to the cloud (at least for Yousendit; I'm not sure where the OAuth token gets stored for Dropbox). I think what I'd like is something like nsIMsgCloudFileProvider.registerAccount and .unregisterAccount. (Or add/removeAccount if we're allowing multiple accounts, which I think we should at least provide the basic structure for ASAP. SFTP will want it for sure.) In most cases, registerAccount would present the UI for login/signup, as appropriate for the provider; we could also provide an optional nsISupports argument for doing this without UI (e.g. registering an SFTP account programmatically). Ideally, when a provider has been set up, the only thing I should need to call from the UI is uploadFile. Granted, some of this might be stuff you have in mind already, and it's just not there because it's not done, but I thought I'd lay out what would be most useful from my perspective. :)

To clarify the above: I'd really like it if the cloud provider accounts worked like mail accounts, in that I could just enumerate all the existing accounts on the "backend", pick one out, and use it. That is, not having to call the init function as we currently do. Right now, most of the work would have to happen elsewhere, except for the presence of hasAccount (if the "front end" is storing all the account info, how would the backend know whether there are accounts set up or not?)

This cleans things up a bit more and adds some more UI code (notably, cloud attachments' icons will be the favicon of the provider, as per the design doc).

(In reply to Jim Porter (:squib) from comment #50) > To clarify the above: I'd really like it if the cloud provider accounts > worked like mail accounts, in that I could just enumerate all the existing > accounts on the "backend", pick one out, and use it. That is, not having to > call the init function as we currently do. Right now, most of the work would > have to happen elsewhere, except for the presence of hasAccount (if the > "front end" is storing all the account info, how would the backend know > whether there are accounts set up or not?) When I first started implementing the provider objects, we didn't have a UI design, and I didn't think we were going to have multiple accounts per providers, so that's why the provider instance could implement hasAccount (that, and I really just needed some way to hack up UI to drive the backend). So hasAccount can go away as soon as we have some other account mechanism. I can write some js code to implement a simple pref-based account system for cloud providers. I don't plan on doing a full-on xpcom interface for it like the mail backend.

(In reply to Jim Porter (:squib) from comment #49) > Bienvenu, about the storage of account info: it seems to me that this should > be handled in the backend. Right now, it seems like the frontend would set > "hasAccount" to true, but is then responsible for handing off the account > info to the backend every time a file gets attached to the cloud (at least > for Yousendit; I'm not sure where the OAuth token gets stored for Dropbox). They get stored in prefs, per comment 41. > > I think what I'd like is something like > nsIMsgCloudFileProvider.registerAccount and .unregisterAccount. (Or > add/removeAccount if we're allowing multiple accounts, which I think we > should at least provide the basic structure for ASAP. That would mean each cloud provider would need to implement its own account system. I'd much rather have a simple central pref-based account system that gets you the cloud provider, the username, and whatever per-provider prefs the provider wants. I can implement that as a js module. I don't want to do a full blown xpcom interface and all the hassle that entails. SFTP will want it for > sure.) In most cases, registerAccount would present the UI for login/signup, I don't want the provider backend code to have to do UI. If that's better for you, we can make it a method on the provider, but I'm not the person to implement it. I realize that getting the account info out of the provider after setup is specific to each provider, so I can see why it makes sense to be part of the provider. We have a similar issue with provider account settings - I don't know how to shoehorn the UI for that into the backend provider. I suspect we'll just have to have xul and js files per provider for that, like we do for extending the account manager settings. But for yousendit and dropbox, all we need now is usernames, which are e-mail addresses. > as appropriate for the provider; we could also provide an optional > nsISupports argument for doing this without UI (e.g. registering an SFTP > account programmatically). I have a method already in the interface for defining accounts without a UI. If a UI is required, I provide a url the frontend can use to get to the UI. > > Ideally, when a provider has been set up, the only thing I should need to > call from the UI is uploadFile. That's basically the case, once the account has been set up, and you've initialized the provider instance. Though you also need to get the icon for the provider, check the max attachment size, user quota, etc, all of which are (imperfectly) exposed by the backend. And I suspect we'll want to something about users cancelling messages with cloud attachments (the cloud providers have methods for deleting files, though I haven't exposed or implemented them yet). So we'll still need to init the provider instance with the username, unless we add a username parameter to all those methods.

(In reply to David :Bienvenu from comment #53) > > I think what I'd like is something like > > nsIMsgCloudFileProvider.registerAccount and .unregisterAccount. (Or > > add/removeAccount if we're allowing multiple accounts, which I think we > > should at least provide the basic structure for ASAP. > > That would mean each cloud provider would need to implement its own account > system. I'd much rather have a simple central pref-based account system that > gets you the cloud provider, the username, and whatever per-provider prefs > the provider wants. If we only allow one account of each type for now, couldn't we do this by setting some prefs from the provider (like how hasAccount works now)? In the longer term, when (if?) we want to support multiple accounts per provider type, I was thinking something like: mail.cloud_providers.account1.type = dropbox mail.cloud_providers.account1.login_info = {oauth_key: ...} mail.cloud_providers.account1.type = yousendit mail.cloud_providers.account1.login_info = {username: ..., password: ...} The login info doesn't necessarily have to be a JSON object, of course. It could be anything (or any number of prefs). > SFTP will want it for > > sure.) In most cases, registerAccount would present the UI for login/signup, > > I don't want the provider backend code to have to do UI. If that's better > for you, we can make it a method on the provider, but I'm not the person to > implement it. I've got no problems implementing it; I just want to make sure this is the correct route to go. > We have a similar issue with provider account settings - I don't know how to > shoehorn the UI for that into the backend provider. I suspect we'll just > have to have xul and js files per provider for that, like we do for > extending the account manager settings. But for yousendit and dropbox, all > we need now is usernames, which are e-mail addresses. I thought Dropbox's authentication didn't require a username; isn't everything handled with the OAuth key? (I know the username is used as a key for storing prefs, but in theory, that value could be anything, as long as it's unique.)

(In reply to Jim Porter (:squib) from comment #54) > If we only allow one account of each type for now, couldn't we do this by > setting some prefs from the provider (like how hasAccount works now)? If we're ever going to allow multiple accounts per provider, it's far better to do it from the beginning to avoid migration issues. > > In the longer term, when (if?) we want to support multiple accounts per > provider type, I was thinking something like: > > mail.cloud_providers.account1.type = dropbox > mail.cloud_providers.account1.login_info = {oauth_key: ...} > mail.cloud_providers.account1.type = yousendit > mail.cloud_providers.account1.login_info = {username: ..., password: ...} > > The login info doesn't necessarily have to be a JSON object, of course. It > could be anything (or any number of prefs). Yeah, that's along the lines of what I was thinking. Except the second one would be mail.cloud_providers.account2 :-) We probably don't need a separate pref to list the accounts, since we can enumerate the prefs. If ordering is an issue, we can add an ordinal pref to the accounts. > > I've got no problems implementing it; I just want to make sure this is the > correct route to go. It's kinda weird to me when the backend has to do frontend stuff, but that's the world we're in. > > I thought Dropbox's authentication didn't require a username; isn't > everything handled with the OAuth key? (I know the username is used as a key > for storing prefs, but in theory, that value could be anything, as long as > it's unique.) Yes, that's correct, but I think using the username is the right way to go - if we ever allow multiple drop box accounts in a profile, we need to present the key to the user, so it might as well be something the user is familiar with :-) I don't have strong feelings about it, but I don't see any big disadvantage to knowing the dropbox username. If we only allow a single dropbox account, I suppose we could just make up a username...

(In reply to David :Bienvenu from comment #55) > (In reply to Jim Porter (:squib) from comment #54) > > > If we only allow one account of each type for now, couldn't we do this by > > setting some prefs from the provider (like how hasAccount works now)? > > If we're ever going to allow multiple accounts per provider, it's far better > to do it from the beginning to avoid migration issues. > > > > In the longer term, when (if?) we want to support multiple accounts per > > provider type, I was thinking something like: > > > > mail.cloud_providers.account1.type = dropbox > > mail.cloud_providers.account1.login_info = {oauth_key: ...} > > mail.cloud_providers.account1.type = yousendit > > mail.cloud_providers.account1.login_info = {username: ..., password: ...} > > > > The login info doesn't necessarily have to be a JSON object, of course. It > > could be anything (or any number of prefs). > > Yeah, that's along the lines of what I was thinking. Except the second one > would be mail.cloud_providers.account2 :-) We probably don't need a separate > pref to list the accounts, since we can enumerate the prefs. If ordering is > an issue, we can add an ordinal pref to the accounts. I think from here, all we'd need is a function to create the appropriate provider object based on a particular pref branch. So, I'd ask for "account1" (or something like that) and it would check mail.cloud_providers.account1.type, make a new nsDropbox object, pass the auth info to it, and then return it to me. I could probably do this in MsgComposeCommands.js, or we could add something like nsIMsgCloudFileProviderManager.idl that would have some code to iterate over the available accounts based on the prefs above. This would also be a good spot to handle the default account, assuming we're still doing that (the design doc mentions it in one place). > > I've got no problems implementing it; I just want to make sure this is the > > correct route to go. > > It's kinda weird to me when the backend has to do frontend stuff, but that's > the world we're in. Well, maybe "backend" isn't quite the right word for it. It seems like the providers are stuck in the middle, in that they have some front-end stuff and some back-end stuff. > Yes, that's correct, but I think using the username is the right way to go - > if we ever allow multiple drop box accounts in a profile, we need to present > the key to the user, so it might as well be something the user is familiar > with :-) I don't have strong feelings about it, but I don't see any big > disadvantage to knowing the dropbox username. If we only allow a single > dropbox account, I suppose we could just make up a username... The design doc shows a field for the user to provide a display name for the account; we could use that. I think it would be nice if we could avoid storing the Dropbox username, since it's not really necessary, and the less data leakage like that, the better. People used to how OAuth works might be concerned if we ask them for their username anyway, since that's not a usual step. If we stored the username as a key, I think we'd end up requiring the user to enter their username twice: once on the OAuth webpage and once somewhere in XUL so we can access it. It doesn't look like the OAuth API gives us access to that information. I guess we could grab the username from the page before it's submitted, but that's probably pretty brittle.

(In reply to David :Bienvenu from comment #55) > (In reply to Jim Porter (:squib) from comment #54) > > mail.cloud_providers.account1.type = dropbox > > mail.cloud_providers.account1.login_info = {oauth_key: ...} > > mail.cloud_providers.account1.type = yousendit > > mail.cloud_providers.account1.login_info = {username: ..., password: ...} > > > > The login info doesn't necessarily have to be a JSON object, of course. It > > could be anything (or any number of prefs). > > Yeah, that's along the lines of what I was thinking. Except the second one > would be mail.cloud_providers.account2 :-) We probably don't need a separate > pref to list the accounts, since we can enumerate the prefs. If ordering is > an issue, we can add an ordinal pref to the accounts. So, could we please not have an ordinal, and instead just re-number the prefs to be in the order the user chooses? Having account15 when I only have 5 email accounts is a real pain… :( Also, why not say that we're using JSON, and wrap up the type into the login_info, giving us: mail.cloud_providers.account1 = {type: dropbox, oauth_key: ...} mail.cloud_providers.account2 = {type: yousendit, username: ..., password: ...} And I thought the default account would be account1, and the user could re-order them if they wanted a different default. (To be fair, that wouldn't allow us to have different defaults per email account, which some people might want.) Thanks, Blake.

(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #57) > Also, why not say that we're using JSON, and wrap up the type into the > login_info, giving us: > mail.cloud_providers.account1 = {type: dropbox, oauth_key: ...} > mail.cloud_providers.account2 = {type: yousendit, username: ..., password: > ...} I'm not really sure we want JSON at all. It's probably easier to have provider-specific prefs, especially since Dropbox and Yousendit only have one piece of data each (the password for Yousendit would actually be store in the password manager).

(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #57) > So, could we please not have an ordinal, and instead just re-number the > prefs to be in the order the user chooses? Having account15 when I only > have 5 email accounts is a real pain… :( I was talking about an ordinal pref, not hiding an ordinal value in the pref name, the way mail accounts do it. So, pref("mail.cloud_providers.account1.ordinal", 1); If the user changes the order, we update the ordinals for all the accounts. but now that I read your comment again, perhaps you are complaining about the way we pick account names, not the way we could do ordering. One reason not to change account names is that invalidates any code that might actually persist the account names, and I doubt people will be adding/removing accounts. The prefname is pretty unimportant. It doesn't have to be a number at all :-)

(In reply to Jim Porter (:squib) from comment #58) > I'm not really sure we want JSON at all. It's probably easier to have > provider-specific prefs, especially since Dropbox and Yousendit only have > one piece of data each (the password for Yousendit would actually be store > in the password manager). Is stored :-) Yeah, I'm not crazy about storing the json in prefs but I'll keep an open mind about it.

(In reply to Jim Porter (:squib) from comment #56) > I could probably do this in MsgComposeCommands.js, or we could add something > like nsIMsgCloudFileProviderManager.idl that would have some code to iterate > over the available accounts based on the prefs above. This would also be a > good spot to handle the default account, assuming we're still doing that > (the design doc mentions it in one place). I'm thinking a js module would be lighter weight. It would only have to export a few symbols. I'm happy to do that, if that sounds OK.

(In reply to David :Bienvenu from comment #61) > I'm thinking a js module would be lighter weight. It would only have to > export a few symbols. I'm happy to do that, if that sounds OK. That's fine with me. How much harder is it to set up an XPCOM service in JS? (We might want to do this in the future if we want to handle any of this in C++). The iterateCloudProviders function I added might be useful in some way here (though I'm sure it'll need to be modified): https://bugzilla.mozilla.org/attachment.cgi?id=583090&action=diff#a/mail/components/compose/content/MsgComposeCommands.js_sec2

(In reply to David :Bienvenu from comment #59) > but now that I read your comment again, perhaps you are complaining about > the way we pick account names, not the way we could do ordering. One reason > not to change account names is that invalidates any code that might actually > persist the account names, and I doubt people will be adding/removing > accounts. The prefname is pretty unimportant. It doesn't have to be a number > at all :-) I agree with this. Really, the "accountN" substring could be a UUID for all it matters. It does feel a little messy when you have "account15" and you only have 3 active accounts, but that could easily be avoided by using non-sequential IDs, e.g. mail.cloud_files.accounts.a67deb02.type = "dropbox" This isn't much different from the random ID used for Mozilla profiles, really.

(In reply to Jim Porter (:squib) from comment #62) > (In reply to David :Bienvenu from comment #61) > > I'm thinking a js module would be lighter weight. It would only have to > > export a few symbols. I'm happy to do that, if that sounds OK. > > That's fine with me. How much harder is it to set up an XPCOM service in JS? > (We might want to do this in the future if we want to handle any of this in > C++). It means adding an idl file and revving that whenever we change anything, and getting two separate code reviews :-) - in the unlikely event we move from js to c++, then we can add an interface. We're really actively discouraging c++ extensions, so I don't think a c++ extension is going to want to get at our cloud file accounts. I can see providers that for some reason want to be in c++, but they shouldn't need to know about the account structure. They might want to know about the pref branch, though. I don't want to overdesign this and end up with something like the mess we have with mail accounts.

That's probably ok. I guess it just seems a bit odd for some of the API to be defined in IDL and some of it not, but I'm sure I'll get over it.

This implements the disk space available and max file size for Dropbox, and fixes the handling of expired auth tokens from dropbox (tested by editing the cached tokens stored in prefs by hand to make them bad).

Thanks - that's fixed Linux builds for me.

This fixes the encoding of filenames for dropbox, basically adapt encodeURIComponent so that it's x-www-form-urlencoded compliant, or at least enough to make dropbox happy. It also cleans up our temp file cleanup for yousendit, by adding a helper method.

Bienvenu, did you ever end up writing the cloud file account manager module mentioned in comment 61? I need that to make more progress on the configuration UI.

(In reply to Jim Porter (:squib) from comment #72) > Bienvenu, did you ever end up writing the cloud file account manager module > mentioned in comment 61? I need that to make more progress on the > configuration UI. No, not yet, I've mostly been busy dealing with the pluggable store landing/fallout. I can try to do something over the next couple days, though I'm pretty busy with holiday stuff until Tuesday.

I've added a possible account system in cloudFileAccounts.js with a few methods: "AddCloudFileAccount", "RemoveCloudFileAccount", "GetCloudFileAccounts", "SaveAccounts" This is all completely untested, but the idea is that accounts are js objects, and that we persist the array of accounts as a json file. This allows the different accounts to have arbitrary attributes. As I said, completely untested, and I haven't changed the two existing providers to use the account system (though I'm not sure they have to, yet, anyway...) since the front end would be driving the accounts, and use the accounts module instead of asking the providers if they have accounts. I may not have done the module thing right; it's not my usual area...

this changes the providers to do the quota and file size limit checking on every upload, and return specific errors to the front end (it's up to the front end to tell the user how to do the upgrade, after getting these errors).

I restructured the dropbox provider code quite a bit to allow the front end to spew uploadFile requests synchronously. I need to do the same thing to the yousendit provider; I hope that goes more smoothly.

this makes youSendIt queue upload requests.

Just an update on where I'm at (hopefully I'll be uploading patches in the next day or two). I've finally gotten everything recognizing the existence or non-existence of cloud accounts, with the various widgets showing/hiding themselves as appropriate. I've also reworked the cloud account manager to store stuff in prefs, since that's easier for me than using a JSON file (and makes it easier to tweak this stuff while testing). I've also been simplifying a lot of the UI code, so it's less error-prone now. Here's what I still need to do: * Hook up account creation * Make the notification bar do things * Convert between regular and cloud attachments * Error handling, testing, more cleanup, etc

This patch is meant to be applied to the backend half; it's not totally working yet, but I reworked the account manager and added a "createExistingAccount" method to the provider. I also changed how providers work so that they're instances rather than services (so you can have multiple Dropbox accounts). It's still a bit messy, but I think this is pretty close to what I need for the frontend.

This UI patch should go on top of the intermediate patch and takes advantage of its various features. I changed strings, simplified some code, made the notification bar do things, and started hooking stuff up to convert between regular and cloud attachments. With all this, things should be pretty close to working, but not quite. If anyone wants to take a look at this, it would be much appreciated. :)

> With all this, things should be pretty close to working, but not quite. If anyone > wants to take a look at this, it would be much appreciated. :) By "take a look at this", what do you mean? Do you need someone to test, review, or polish the code? Or do you need someone to work on a portion of it? Let me know, and I'll be all over it. :)

Well, anything really. :) You could try applying all the patches and seeing if the UI works as expected. (You'll need to, at minimum, set mail.cloud_files.account1.type to "DropBox" or "YouSendIt"*.) If there's stuff that doesn't work, that would be helpful to know. Patches for specific bits are definitely welcome. Some things that I know could use more work include account creation and converting between regular and cloud attachments. I also think I broke YouSendIt at the moment.

I should clarify: I think YouSendIt needs to persist username and password in prefs, and add a UI for entering those data.

(In reply to Jim Porter (:squib) from comment #83) > I should clarify: I think YouSendIt needs to persist username and password > in prefs, and add a UI for entering those data. The backend for you sendit will prompt for a password, and store it in the password manager, but the front end account setup needs to provide a username and store it in the account prefs.

Right. Would it make sense to store the username in the password manager too? I think that's what NNTP does. I'm guessing the answer is "probably not".

(In reply to Jim Porter (:squib) from comment #85) > Right. Would it make sense to store the username in the password manager > too? I think that's what NNTP does. I'm guessing the answer is "probably > not". I'd much rather store it in the prefs for the account - I'll just say that what NNTP does is unlike anything else mail does and leave it like that :-)

I'm building with your patches now - with the changes to the api, the yousendit-specific account setup code will need to agree with the backend code as to which pref to store the username in...and perhaps agree on what the pref branch is - I haven't seen how that works yet...

I'm thinking we should have nsYouSendIt.js handle the setup UI as well, since that will make things the simplest for adding new provider types. It does mean that nsYouSendIt.js isn't totally "backend", but short of adding nsYouSendItUI.js, I don't know that there's much we can do while keeping things extensible. I'll handle the UI bits, though.

(In reply to Jim Porter (:squib) from comment #88) > I'm thinking we should have nsYouSendIt.js handle the setup UI as well, > since that will make things the simplest for adding new provider types. It > does mean that nsYouSendIt.js isn't totally "backend", but short of adding > nsYouSendItUI.js, I don't know that there's much we can do while keeping > things extensible. I'll handle the UI bits, though. Right, so we'll need a new method on the interface, I guess - what should it look like? We'll need something similar for editing the account settings as well, I would think.

(In reply to David :Bienvenu from comment #89) > Right, so we'll need a new method on the interface, I guess - what should it > look like? We'll need something similar for editing the account settings as > well, I would think. For the time being, I'm going with "createExistingAccount", which doesn't address the editing UI, but would at least get things working. In the long run, I think what I'd really like is for nsYouSendIt.js and friends to return a document that I can insert wherever I like with a seamless iframe. Unfortunately, those don't exist yet. I imagine the Right Way will become clearer once we have everything working and start iterating on the UI.

For some providers, we may need to limit the availability of the provider to certain locales - this would have to be in the account provisioning aspect of the feature.

This might be a matter of having a method on the provider interface that returns the list of supported locales, perhaps with an option of just turning it on for all locales.

this does the following: fold squib's intermediate patch into the backend patch switch to mconley's dropbox credentials (mconley@mozilla.com, ThunderbirdsAreGo) Fix yousendit auth - to enable yousendit, add these two prefs: mail.cloud_files.account2.username, "david@sifry.com" mail.cloud_files.account2.type, "YouSendIt" password is "thunder" (password prompting was broken; I fixed that)

Just a note that attachment 587859 [details] [diff] [review] did not apply cleanly for me for mail/components/compose/content/MsgComposeCommands.js

Ah, nevermind - the order of the patches has changed. It is now necessary to apply the UI patch first, and then the backend patch.

(In reply to Mike Conley (:mconley) from comment #95) > Ah, nevermind - the order of the patches has changed. It is now necessary > to apply the UI patch first, and then the backend patch. As a warning, the next frontend patch I post will probably reverse the order again, since I really just need to resolve a line of bitrot in the patches.

Comment on attachment 587859 [details] [diff] [review] update backend to incorporate intermediate patch and fix yousendit One obvious thing is that more code could be shared in nsYouSendIt.js - I'd like to do that at some point, though it will break something, I'm sure.

Comment on attachment 587859 [details] [diff] [review] update backend to incorporate intermediate patch and fix yousendit Bienvenu: for simplicity, could you remove the change in this patch in MsgComposeCommands.js? I'll add it to the front-end patch so we don't have to worry about touching the same files.

(In reply to Jim Porter (:squib) from comment #98) > Bienvenu: for simplicity, could you remove the change in this patch in > MsgComposeCommands.js? I'll add it to the front-end patch so we don't have > to worry about touching the same files. Yes, I thought I had, but apparently not. I'll do that today.

you'll need to add Components.utils.import("resource:///modules/cloudFileAccounts.js"); to MsgComposeCommands.js to get things working again.

I don't want to bitrot squib, but fwiw, here's a new ui patch with the extra include.

This patch adds a loading throbber to the attachment in the list while we're uploading the file. Based on the comments, I'm not sure if "done" really means the file's uploaded or if we just got the URL for it, but it still takes a few seconds, and helps make things less confusing especially if you have to sign in to the cloud provider.

Comment on attachment 588771 [details] [diff] [review] be patch without MsgComposeCommands.js Review of attachment 588771 [details] [diff] [review]: ----------------------------------------------------------------- I'm unsure if I was supposed to review http.jsm and oauth.jsm - they seem like outside dependencies that we're bringing it, like jQuery. And I don't recall us reviewing jQuery when it came in. Regardless, I've given them a read in this review. Disregard if we don't want to change http.jsm and oauth.jsm. ::: mail/base/modules/http.jsm @@ +37,5 @@ > + > +var EXPORTED_SYMBOLS = ["doXHRequest"]; > + > +/* > + TODO We should probably wrap this in a proper comment block, like: /** * Comment */ @@ +41,5 @@ > + TODO > + replace doXHRequest with a more generic 'HTTP' object > +*/ > + > +const {classes: Cc, interfaces: Ci, utils: Cu} = Components; That's a neat construct. @@ +88,5 @@ > + } > + }; > + > + if (aHeaders) { > + aHeaders.forEach(function(header) { I believe the more common iteration idiom we tend to use in our Javascript is: for (let [, header] in Iterator(aHeaders)) { ... } ::: mail/base/modules/oauth.jsm @@ +19,5 @@ > + * Portions created by the Initial Developer are Copyright (C) 2012 > + * the Initial Developer. All Rights Reserved. > + * > + * Contributor(s): > + David Bienvenu <dbienvenu@mozilla.com> Missing a * at the beginning of this line. @@ +59,5 @@ > + this._tokenSecret = aAuthTokenSecret; > + this.consumerKey = aAppKey; > + this.consumerSecret = aAppSecret; > +} > +OAuth.prototype = { For readability, I'd prefer a newline before the OAuth.prototype line, and a removal of the newline after it. @@ +65,5 @@ > + consumerKey: "", > + consumerSecret: "", > + completionURI: "http://oauthcallback.local/", > + baseURI: "", > + get token() this._token, This getter / setter for token doesn't seem to be doing anything special. Why's wrong with just having a token property? @@ +67,5 @@ > + completionURI: "http://oauthcallback.local/", > + baseURI: "", > + get token() this._token, > + set token(token) {this._token = token;}, > + get tokenSecret() this._tokenSecret, Same as above, re this getter/setter. @@ +71,5 @@ > + get tokenSecret() this._tokenSecret, > + set tokenSecret(token) {this._tokenSecret = token;}, > + _token: "", > + _tokenSecret: "", > + connect: function(aSuccess, aFailure) { I'd prefer a newline before each function, for readability. Also, we should probably document these functions. @@ +79,5 @@ > + this._enabled = true; > + > + // Get a new token if needed... > + if (!this.token || !this.tokenSecret) { > + dump("don't think we have a token or token secret\n"); We should drop these dump statements and do some logging instead. @@ +84,5 @@ > + this.requestToken(aSuccess, aFailure); > + return; > + } > + > + dump("Connecting using existing token"); Remove this dump statement, and use log4moz or something similar. @@ +88,5 @@ > + dump("Connecting using existing token"); > + aSuccess(); > + }, > + > + signAndSend: function(aUrl, aHeaders, aMethod, aPOSTData, aOnLoad, aOnError, aThis, This function seems to be the one most used, and should probably be documented. @@ +90,5 @@ > + }, > + > + signAndSend: function(aUrl, aHeaders, aMethod, aPOSTData, aOnLoad, aOnError, aThis, > + aOAuthParams) { > + dump("sign and send aUrl = " + aUrl); Same as above, re: dump. I'm going to stop mentioning dump statements now - I think you get the drift. @@ +91,5 @@ > + > + signAndSend: function(aUrl, aHeaders, aMethod, aPOSTData, aOnLoad, aOnError, aThis, > + aOAuthParams) { > + dump("sign and send aUrl = " + aUrl); > + const chars = Shouldn't this const be named, prefixed with a k? Like kChars? @@ +93,5 @@ > + aOAuthParams) { > + dump("sign and send aUrl = " + aUrl); > + const chars = > + "0123456789ABCDEFGHIJKLMNOPQRSTUVWXTZabcdefghiklmnopqrstuvwxyz"; > + const nonceLength = 6; Same as above; kNonceLength? @@ +98,5 @@ > + let nonce = ""; > + for (let i = 0; i < nonceLength; ++i) > + nonce += chars[Math.floor(Math.random() * chars.length)]; > + > + let params = (aOAuthParams || []).concat([ The magic strings "HMAC-SHA1" and "1.0" should probably be turned into consts. @@ +107,5 @@ > + ["oauth_timestamp", Math.floor(((new Date()).getTime()) / 1000)], > + ["oauth_version", "1.0"] > + ]); > + > + function percentEncode(aString) Holy smokes, this is a bit terse. At the very least, we should document how this function is later used (via the map function) @@ +122,5 @@ > + .map(function(p) p.split("=").map(percentEncode)); > + } > + dump("in sign and send url = " + url + "\nurlSpec = " + urlSpec + "\n"); > + dump("dataParams = " + dataParams); > + let signatureKey = this.consumerSecret + "&" + this.tokenSecret; I, personally, would prefer [this.consumerSecret, this.tokenSecret].join("&"); @@ +125,5 @@ > + dump("dataParams = " + dataParams); > + let signatureKey = this.consumerSecret + "&" + this.tokenSecret; > + let signatureBase = > + aMethod + "&" + encodeURIComponent(urlSpec) + "&" + > + params.concat(dataParams) I think these next couple of lines warrant some documentation to explain what's going on. @@ +151,5 @@ > + return doXHRequest(url, headers, aMethod, aPOSTData, aOnLoad, aOnError, aThis); > + }, > + _parseURLData: function(aData) { > + let result = {}; > + aData.split("&").forEach(function (aParam) { See above, re: for (let [, aParam] in Iterator(aData)) @@ +164,5 @@ > + _receivedLength: 0, > + onDataAvailable: function(aRequest) { > + let text = aRequest.target.responseText; > + let newText = this._pendingData + text.slice(this._receivedLength); > + DEBUG("Received data: " + newText); Why this usage of DEBUG here, but nowhere else? And where is that function defined? @@ +263,5 @@ > + onAuthorizationReceived: function(aData) { > + dump("authorization received\n"); > + this.requestAccessToken(null /* data.oauth_verifier */); > + }, > + requestAccessToken: function(aTokenVerifier) { aTokenVerifier never seems to be used... @@ +289,5 @@ > + if (this._streamingRequest) { > + this._streamingRequest.abort(); > + delete this._streamingRequest; > + } > +// delete this.token; Why are these commented out? Should they just be removed? @@ +292,5 @@ > + } > +// delete this.token; > +// delete this.tokenSecret; > + }, > + gotDisconnected: function(aError, aErrorMessage) { aError and aErrorMessage never seem to be used. @@ +303,5 @@ > + }, > + unInit: function() { > + this.cleanUp(); > + }, > + onError: function(aException) { What are onError and onRequestedInfoReceived for? As far as I can tell, you're not stubbing out an interface...so why do we need them?

thx for the review comments, Mike. http.jsm and oauth.jsm need to be reviewed. They originally came from Instantbird, but I changed a bit, especially oauth, because twitter's use of oauth is different from dropbox and we're in such a hurry that I didn't have the time to unify them.

(In reply to Mike Conley (:mconley) from comment #103) > Comment on attachment 588771 [details] [diff] [review] > be patch without MsgComposeCommands.js > > Review of attachment 588771 [details] [diff] [review]: > ----------------------------------------------------------------- > > I'm unsure if I was supposed to review http.jsm and oauth.jsm - they seem > like outside dependencies that we're bringing it, like jQuery. And I don't > recall us reviewing jQuery when it came in. > > Regardless, I've given them a read in this review. Disregard if we don't > want to change http.jsm and oauth.jsm. I wrote a significant part of these files (thanks for reading them!) for Instantbird's twitter support, but some of your comments are for things David has edited. If you want to compare with the original files from Instantbird, they are: - http://lxr.instantbird.org/instantbird/source/chat/modules/http.jsm - http://lxr.instantbird.org/instantbird/source/chat/protocols/twitter/twitter.js It seems most of the dump calls in David's version were originally LOG calls. Instantbird has a very easy to use logging system with DEBUG/LOG/WARN/ERROR functions. This logging system is implemented in http://lxr.instantbird.org/instantbird/source/chat/modules/imXPCOMUtils.jsm Advantages compared to log4moz: - all messages are reported to the error console with location information, so that one can just click a link in the error console and have the line that produced the message immediately visible. - It's dead simple :). (I've never fully understood how one should use log4moz, which seems to support lots of things I'm unlikely to ever have any use case for). Of course if you rely on some special features of log4moz, log4moz is probably better (+ consistency is important). Some of your comments are things I would happily take in Instantbird's version, for example the constants clear naming, more documentation (some of this was internal to the twitter implementation at the time it was created, hence the total lack of documentation in some places). Some are things I would rather avoid: - replacing a simple Array.forEach call with |for (let [, header] in Iterator(aHeaders)) {| adds complexity (from my point of view at least) for no apparent benefit. - [this.consumerSecret, this.tokenSecret].join("&") also seems to me like adding complexity, but it's more a matter of personal preference in this case :). > > + function percentEncode(aString) > > Holy smokes, this is a bit terse. At the very least, we should document how > this function is later used (via the map function) It's just encodeURIComponent but also encoding these characters which need to be percent encoded for OAuth: !*'()

Comment on attachment 588771 [details] [diff] [review] be patch without MsgComposeCommands.js Review of attachment 588771 [details] [diff] [review]: ----------------------------------------------------------------- Ok, just finished going through the rest of these. A lot of my nits are just stylistic preferences - if you've got good reasons for your particular styles, I'm willing to back off. All in all, however, this looks pretty good. So, feedback+. Keep up the good work. :) ::: mail/components/cloudfile/cloudFileAccounts.js @@ +1,2 @@ > +/* ***** BEGIN LICENSE BLOCK ***** > + * Version: MPL 1.1/GPL 2.0/LGPL 2.1 We should probably use the new MPL 2. @@ +44,5 @@ > + > +Cu.import("resource://gre/modules/Services.jsm"); > +Cu.import("resource://gre/modules/XPCOMUtils.jsm"); > + > +function GetCloudFileAccountKeys() Just a general comment that we'll probably want to document these functions. @@ +52,5 @@ > + let children = branch.getChildList("", {}); > + for (let [,child] in Iterator(children)) { > + let dot = child.indexOf("."); > + let subbranch = dot == -1 ? child : child.slice(0, dot); > + accountKeySet[subbranch] = 1; Just curious - why are we setting this to 1, and then just stripping out the keys from this Object? @@ +66,5 @@ > + // Pick a unique account key (TODO: this is a dumb way to do it, probably) > + let n = 1; > + let existingKeys = GetCloudFileAccountKeys(); > + while (true) { > + if (existingKeys.indexOf("account"+n) == -1) Spaces on either side of + @@ +70,5 @@ > + if (existingKeys.indexOf("account"+n) == -1) > + break; > + n++; > + } > + return "account"+n; Spaces on either side of + @@ +77,5 @@ > +function GetCloudFileAccounts() > +{ > + let accounts = []; > + > + const CATEGORY = "cloud-files"; CATEGORY, or kCategory? Does it matter? @@ +81,5 @@ > + const CATEGORY = "cloud-files"; > + let catMan = Cc["@mozilla.org/categorymanager;1"] > + .getService(Ci.nsICategoryManager); > + for (let [,accountKey] in Iterator(GetCloudFileAccountKeys())) { > + let branch = Services.prefs.getBranch("mail.cloud_files."+accountKey+"."); Spaces on either side of the +'s @@ +94,5 @@ > +} > + > +function AddCloudFileAccount(aType) > +{ > + const CATEGORY = "cloud-files"; This is the second time this const is used. Should it be pulled up a scope level? @@ +99,5 @@ > + let catMan = Cc["@mozilla.org/categorymanager;1"] > + .getService(Ci.nsICategoryManager); > + > + let accountKey = CreateUniqueAccountKey(); > + let branch = Services.prefs.getBranch("mail.cloud_files."+accountKey+"."); Spaces on either side of the +'s. @@ +101,5 @@ > + > + let accountKey = CreateUniqueAccountKey(); > + let branch = Services.prefs.getBranch("mail.cloud_files."+accountKey+"."); > + > + let className = catMan.getCategoryEntry(CATEGORY, aType) The code below is extremely similar to the code in the GetCloudFileAccounts loop. Can it be factored out into a separate function? ::: mail/components/cloudfile/nsDropBox.js @@ +1,2 @@ > +/* ***** BEGIN LICENSE BLOCK ***** > + * Version: MPL 1.1/GPL 2.0/LGPL 2.1 We're going to want MPL 2. @@ +82,5 @@ > + _uploadingFile : null, > + _uploader : null, > + _lastErrorStatus : 0, > + _lastErrorText : "", > + _maxFileSize : 157286400, // according to Dropbox, this is a fixed limit. This should probably then be a const, declared outside of the scope of this prototype object. @@ +93,5 @@ > + this._accountKey = aAccountKey; > + this._prefBranch = Services.prefs.getBranch("mail.cloud_files." + > + aAccountKey + "."); > + }, > + uploaderCallback : function(aThis, aRequestObserver, aStatus) { Newline before this function definition @@ +99,5 @@ > + aThis._uploadingFile = null; > + aThis._uploads.shift(); > + if (aThis._uploads.length > 0) { > + let nextUpload = aThis._uploads[0]; > + dump("chaining upload, file = " + nextUpload.file.leafName); We should switch from using dump to using log4moz or similar. @@ +107,5 @@ > + } > + else > + aThis._uploader = null; > + }, > + uploadFile: function nsDropBox_uploadFile(aFile, aRecipients, aCallback) { Newline before this function def'n. @@ +111,5 @@ > + uploadFile: function nsDropBox_uploadFile(aFile, aRecipients, aCallback) { > + if (Services.io.offline) > + return Ci.nsIMsgCloudFileProvider.offlineErr; > + dump("uploading " + aFile.leafName + "\n"); > + var me = this; let instead of var @@ +121,5 @@ > + } > + this._file = aFile; > + this._uploadingFile = aFile; > + if (!this._loggedIn) > + return this.logon(function () {me._getUserInfo(function() {me.finishUpload(aFile, aRecipients, aCallback)})}); This line makes my brain hurt a little. :) And I think it goes over the 80 char limit. Any way to make it clearer? At least some documentation to tell future devs what states these functions transition us to. @@ +127,5 @@ > + if (!this._userInfo) > + return this._getUserInfo(function () {me.finishUpload(aFile, aRecipients, aCallback)}); > + this.finishUpload(aFile, aRecipients, aCallback); > + }, > + finishUpload: function(aFile, aRecipients, aCallback) Newline before def'n. Also, I think this function name is a bit misleading. finishUpload suggests a function that is called once an upload has completed. Perhaps something like "beginUpload" or "enqueueUpload" would be more accurate. @@ +131,5 @@ > + finishUpload: function(aFile, aRecipients, aCallback) > + { > + if (aFile.fileSize > this._maxFileSize) > + return aCallback.onStopRequest(null, null, > + Ci.nsIMsgCloudFileProvider.uploadExceedsFileLimit); I think this line should be lined up with the first n in the first null, and then broken up by the periods. @@ +134,5 @@ > + return aCallback.onStopRequest(null, null, > + Ci.nsIMsgCloudFileProvider.uploadExceedsFileLimit); > + if (aFile.fileSize > this._availableStorage) > + return aCallback.onStopRequest(null, null, > + Ci.nsIMsgCloudFileProvider.uploadWouldExceedQuota); Same as above, re line up with the null and break up by periods. @@ +137,5 @@ > + return aCallback.onStopRequest(null, null, > + Ci.nsIMsgCloudFileProvider.uploadWouldExceedQuota); > + delete this._userInfo; // force us to update userInfo on every upload. > + if (!this._uploader) { > + this._uploader = new nsDropBoxFileUploader(this, aFile, aRecipients, this.uploaderCallback, aCallback); Is this over the 80 char limit? @@ +144,5 @@ > + > + this._uploadingFile = aFile; > + this._uploader.uploadFile(); > + }, > + _getUserInfo: function nsDropBox_userInfo(successCallback) { Newline before function def'n @@ +145,5 @@ > + this._uploadingFile = aFile; > + this._uploader.uploadFile(); > + }, > + _getUserInfo: function nsDropBox_userInfo(successCallback) { > + var me = this; let instead of var @@ +147,5 @@ > + }, > + _getUserInfo: function nsDropBox_userInfo(successCallback) { > + var me = this; > + me._successCallback = successCallback; > + this._connection.signAndSend(gServerUrl + "account/info", "", I'm a bit vary of this account/info magic string. It should probably be defined as a const. @@ +170,5 @@ > + me._successCallback(); > + return; > + } > + dump("user info failed, status = " + aRequest.status); > + me._requestObserver.onStopRequest(null, null, Ci.nsIMsgCloudFileProvider.authErr); I think this line is over the 80 char limit. @@ +173,5 @@ > + dump("user info failed, status = " + aRequest.status); > + me._requestObserver.onStopRequest(null, null, Ci.nsIMsgCloudFileProvider.authErr); > + }, this); > + }, > + urlForFile: function nsDropBox_urlForFile(aFile) {return this._urlsForFiles[aFile];}, Despite the fact that this function is a one-liner, I think we should space it normally on several lines, for readability's sake. @@ +174,5 @@ > + me._requestObserver.onStopRequest(null, null, Ci.nsIMsgCloudFileProvider.authErr); > + }, this); > + }, > + urlForFile: function nsDropBox_urlForFile(aFile) {return this._urlsForFiles[aFile];}, > + createNewAccount: function nsDropBox_createNewAccount(aEmailAddress, aPassword, aFirstName, aLastName) { > 80 chars @@ +175,5 @@ > + }, this); > + }, > + urlForFile: function nsDropBox_urlForFile(aFile) {return this._urlsForFiles[aFile];}, > + createNewAccount: function nsDropBox_createNewAccount(aEmailAddress, aPassword, aFirstName, aLastName) { > + return Components.results.NS_ERROR_NOT_IMPLEMENTED;}, Please put the } on the next line. @@ +186,5 @@ > + * If the provider doesn't have an API for creating an account, perhaps > + * there's a url we can load in a content tab that will allow the user > + * to create an account. > + */ > + get createNewAccountUrl() {return "";}, Whatever this URL is, it should probably be a const, and I think this line should be broken up like: get createNewAccountUrl() { return kNewAccountUrl; }, @@ +191,5 @@ > + /** > + * If we don't know the limit, this will return -1. > + */ > + > + get fileUploadSizeLimit() {return this._maxFileSize;}, Same as above, re: breaking up one-liners @@ +193,5 @@ > + */ > + > + get fileUploadSizeLimit() {return this._maxFileSize;}, > + /// -1 if we don't have this info > + get remainingFileSpace() {this._availableStorage;}, Same as above, re: breaking up one-liners @@ +206,5 @@ > + > + var me = this; > + me._requestObserver = aCallback; > + let path = this.wwwFormUrlEncoded(uploadInfo.path); > + let url = gServerUrl + "fileops/delete/" + "?root=sandbox" + "&path=" + fileops/delete/ and ?root=sandbox are probably ripe for const'ing. @@ +209,5 @@ > + let path = this.wwwFormUrlEncoded(uploadInfo.path); > + let url = gServerUrl + "fileops/delete/" + "?root=sandbox" + "&path=" + > + uploadInfo.path; > + let oauthParams = > + [["root", "sandbox"], ["path", path]]; We'll probably want to const "sandbox", since I believe we'll be switching that to "dropbox" when we go live. @@ +230,5 @@ > + > + logon: function (successCallback) { > + let authToken = this.getCachedAuthToken(); > + let authSecret = this.getCachedAuthSecret(); > + var me = this; let instead of var @@ +233,5 @@ > + let authSecret = this.getCachedAuthSecret(); > + var me = this; > + me._successCallback = successCallback; > + this._connection = new OAuth(this.displayName, gServerUrl, gAuthUrl, authToken, authSecret, > + "7xkhuze09iqkghm", "3i5kwjkt74rkkjc"); These keys should definitely be consts. Also, since these keys identify Thunderbird uniquely as a DropBox client application, how do we keep these private? @@ +234,5 @@ > + var me = this; > + me._successCallback = successCallback; > + this._connection = new OAuth(this.displayName, gServerUrl, gAuthUrl, authToken, authSecret, > + "7xkhuze09iqkghm", "3i5kwjkt74rkkjc"); > + this._connection.connect(function () {dump("success connecting\n"); I'd put this dump on the next line, and indent two spaces after the "function" of this line. I'd also line up the other lines below. @@ +239,5 @@ > + me._loggedIn = true; > + me.setCachedAuthToken(me._connection.token); > + me.setCachedAuthSecret(me._connection.tokenSecret); > + me._successCallback();}, > + function () {dump("failed connecting\n"); Same as above, re: breakup and lineup. @@ +260,5 @@ > + setCachedAuthSecret: function(aAuthSecret) { > + this._prefBranch.setCharPref("authSecret", aAuthSecret); > + }, > + wwwFormUrlEncoded : function(str) { > + return encodeURIComponent(str).replace(/!/g, '%21').replace(/'/g, '%27').replace(/\(/g, '%28'). It blows my mind that there's no better, built-in way of doing this. @@ +280,5 @@ > + file : null, > + callback : null, > + recipients : null, > + uploadFile : function() { > + var thisDropBox = this.dropBox; let instead of var @@ +281,5 @@ > + callback : null, > + recipients : null, > + uploadFile : function() { > + var thisDropBox = this.dropBox; > + var thisUploader = this; let instead of var @@ +283,5 @@ > + uploadFile : function() { > + var thisDropBox = this.dropBox; > + var thisUploader = this; > + dump("ready to upload file " + thisDropBox.wwwFormUrlEncoded(this.file.leafName) + "\n"); > + let url = gContentUrl + "files_put/sandbox/" + const for that files_put/sandbox/ string. @@ +311,5 @@ > + }, this, oauthParams); > + }, > + _getShareUrl : function(aFile, aCallback) { > + dump("in get shareUrl\n"); > + let url = gServerUrl + "shares/sandbox/" + this.dropBox.wwwFormUrlEncoded(aFile.leafName); const for shares/sandbox/ @@ +312,5 @@ > + }, > + _getShareUrl : function(aFile, aCallback) { > + dump("in get shareUrl\n"); > + let url = gServerUrl + "shares/sandbox/" + this.dropBox.wwwFormUrlEncoded(aFile.leafName); > + var thisDropBox = this.dropBox; let instead of var @@ +313,5 @@ > + _getShareUrl : function(aFile, aCallback) { > + dump("in get shareUrl\n"); > + let url = gServerUrl + "shares/sandbox/" + this.dropBox.wwwFormUrlEncoded(aFile.leafName); > + var thisDropBox = this.dropBox; > + var thisUploader = this; let instead of var ::: mail/components/cloudfile/nsIMsgCloudFileProvider.idl @@ +1,3 @@ > +/* -*- Mode: I*DL; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ > +/* ***** BEGIN LICENSE BLOCK ***** > + * Version: MPL 1.1/GPL 2.0/LGPL 2.1 MPL 2. @@ +104,5 @@ > + /// -1 if we don't have this info > + readonly attribute long long remainingFileSpace; > + > + /// This is used by our test harness to override the urls the provider uses. > + void overrideUrls(in long aNumUrls, [array, size_is(aNumUrls)] in string aUrls); Just a note - I had to make aNumUrls type PRUint32 in order to call the function from Javascript. Otherwise, we fail an assertion. @@ +109,5 @@ > + > + // Error handling > + // If the cloud provider gets textual errors back from the server, > + // they can be retrieved here. > + readonly attribute AString lastError; Hm. Does this mean we only ever store the last error message? What about a stack of error messages? ::: mail/components/cloudfile/nsYouSendIt.js @@ +1,2 @@ > +/* ***** BEGIN LICENSE BLOCK ***** > + * Version: MPL 1.1/GPL 2.0/LGPL 2.1 MPL 2 @@ +48,5 @@ > +Components.utils.import("resource://gre/modules/XPCOMUtils.jsm"); > +Components.utils.import("resource://gre/modules/Services.jsm"); > + > +// Production url: var gServerUrl = "https://developer-api.yousendit.com"; > +var gServerUrl = "https://test-api.yousendit.com"; let instead of var @@ +50,5 @@ > + > +// Production url: var gServerUrl = "https://developer-api.yousendit.com"; > +var gServerUrl = "https://test-api.yousendit.com"; > + > +var me; let instead of var @@ +62,5 @@ > + classID: Components.ID("{32fd439f-9eb6-4907-ac0b-2c88eb14d98d}"), > + > + get displayName() {return "YouSendIt";}, > + get iconClass() {return null;}, > + get accountKey() {return this._accountKey;}, As stated in some of my other comments, I think I prefer these to be written like: get displayName() { return "YouSendIt"; }, Using consts instead of magic strings where applicable. @@ +92,5 @@ > + }, > + > + init: function nsYouSendIt_init(aAccountKey) { > + this._accountKey = aAccountKey; > + this._prefBranch = Services.prefs.getBranch("mail.cloud_files." + "mail.cloud_files" should be a const @@ +97,5 @@ > + aAccountKey + "."); > + this._userName = this._prefBranch.getCharPref("username"); > + }, > + > + uploaderCallback : function(aThis, aRequestObserver, aStatus) { no space after uploaderCallback. Also, this seems very similar to the DropBox implementation. Would it be advantageous to abstract this function out to a base class? @@ +111,5 @@ > + } > + else > + aThis._uploader = null; > + }, > + uploadFile: function nsYouSendIt_uploadFile(aFile, aRecipients, aCallback) { newline after def'n. @@ +125,5 @@ > + this._uploadingFile = aFile; > + this._recipients = aRecipients; > + dump("recipients = " + aRecipients + "\n"); > + this._urlListener = aCallback; > + var me = this; let instead of var @@ +128,5 @@ > + this._urlListener = aCallback; > + var me = this; > + if (!this._loggedIn) > + return this.logon( > + function () {me._getUserInfo( I think I'd prefer: (function() { me._getUserInfo( function() { me.finishUpload(aFile, aRecipients, aCallback); }, function() { me._urlListener.onStopRequest(blah, blah, blah); }) }, function() { me._urlListener.onStopRequest(blah, blah, blah, blah); }) @@ +134,5 @@ > + function () {me._urlListener.onStopRequest(null, null, Ci.nsIMsgCloudFileProvider.authErr);});}, > + function() {me._urlListener.onStopRequest(null, null, Ci.nsIMsgCloudFileProvider.authErr);}); > + dump("getting user info\n"); > + if (!this._userInfo) > + return this._getUserInfo(function () {me.finishUpload(aFile, aRecipients, aCallback)}, Same as above, re line-breaking and spacing @@ +139,5 @@ > + function () {me._urlListener.onStopRequest(null, null, Ci.nsIMsgCloudFileProvider.authErr);}); > + > + this.finishUpload(aFile, aRecipients, aCallback); > + }, > + finishUpload: function(aFile, aRecipients, aCallback) Same comment as in nsDropBox - I think the name of this function is misleading. @@ +161,5 @@ > + > + }, > + > + _getUserInfo: function nsYouSendIt_userInfo(successCallback, failureCallback) { > + let resource = "/v2/user"; const for this magic string @@ +167,5 @@ > + let curDate = Date.now().toString(); > + > + let signature = this.signature("GET", resource, curDate); > + dump("signature = " + signature + " resource = " + resource + "\n"); > + let req = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"] Is there a reason why we roll our own XmlHTTPRequest, instead of using http.jsm? @@ +173,5 @@ > + this._request = req; > + this._successCallback = successCallback; > + this._failureCallback = failureCallback; > + req.open("GET", gServerUrl + "/v2/user" + args, true); > + var me = this; let instead of var @@ +178,5 @@ > + req.onload = function() { > + if (me._request.status >= 200 && me._request.status < 300) { > + dump("request status = " + me._request.status + " response = " + me._request.responseText + "\n"); > + let response = me._request.responseText.replace(/<\?xml[^>]*\?>/, ""); > + let docResponse = new XML(response); What happens if parsing the XML fails? @@ +204,5 @@ > + // in the log, and crashes if there aren't two spaces. > + req.setRequestHeader("Authorization", this._authToken + " "); > + req.send(); > + }, > + urlForFile: function nsYouSendIt_urlForFile(aFile) {return this._urlsForFiles[aFile];}, Break up into several lines, newline before def'n. @@ +205,5 @@ > + req.setRequestHeader("Authorization", this._authToken + " "); > + req.send(); > + }, > + urlForFile: function nsYouSendIt_urlForFile(aFile) {return this._urlsForFiles[aFile];}, > + createNewAccount: function nsYouSendIt_createNewAccount(aEmailAddress, aPassword, newline before def'n. @@ +211,5 @@ > + aRequestObserver) { > + if (Services.io.offline) > + return Ci.nsIMsgCloudFileProvider.offlineErr; > + > + let resource = "/v2/user"; This is the second function that uses this string - it should probably be declared as a const in a higher scope. @@ +218,5 @@ > + let curDate = Date.now().toString(); > + > + let signature = this.signature("POST", resource, curDate); > + dump("signature = " + signature + " resource = " + resource + "\n"); > + let req = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"] Again, is there a particular reason we avoid using http.jsm? @@ +222,5 @@ > + let req = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"] > + .createInstance(Ci.nsIXMLHttpRequest); > + this._request = req; > + this._requestObserver = aRequestObserver; > + req.open("POST", gServerUrl + "/v2/user" + args, true); We should use the const here. @@ +223,5 @@ > + .createInstance(Ci.nsIXMLHttpRequest); > + this._request = req; > + this._requestObserver = aRequestObserver; > + req.open("POST", gServerUrl + "/v2/user" + args, true); > + var me = this; let instead of var @@ +256,5 @@ > + * If the provider doesn't have an API for creating an account, perhaps > + * there's a url we can load in a content tab that will allow the user > + * to create an account. > + */ > + get createNewAccountUrl() {return "";}, Break up into several lines @@ +261,5 @@ > + > + /** > + * If we don't know the limit, this will return -1. > + */ > + get fileUploadSizeLimit() {return this._maxFileSize;}, Break these two functions up over several lines. @@ +273,5 @@ > + let uploadInfo = this._uploadInfo[aFile]; > + if (!uploadInfo) > + throw Components.results.NS_ERROR_FAILURE; > + > + var me = this; let instead of var @@ +275,5 @@ > + throw Components.results.NS_ERROR_FAILURE; > + > + var me = this; > + me._requestObserver = aCallback; > + let req = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"] Same question regarding using this instead of http.jsm @@ +278,5 @@ > + me._requestObserver = aCallback; > + let req = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"] > + .createInstance(Ci.nsIXMLHttpRequest); > + this._request = req; > + var me = this; We're redeclaring the var me - that shouldn't work, I don't think. @@ +279,5 @@ > + let req = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"] > + .createInstance(Ci.nsIXMLHttpRequest); > + this._request = req; > + var me = this; > + let resource = "/v2/item/delete/" + uploadInfo.itemID; const for "/v2/item/delete" @@ +312,5 @@ > + }, > + // returns "" if no remembered password; otherwise, the password > + // stored in the password manager. > + getPassword: function(aUsername) { > + let passwordURI = gServerUrl; Why alias this global? @@ +337,5 @@ > + serverUrl = gServerUrl.substr(0, userPos) + userNamePart + gServerUrl.substr(userPos); > + let messengerBundle = Services.strings.createBundle( > + "chrome://messenger/locale/messenger.properties"); > + if (authPrompter.promptPassword(this.displayName, > + messengerBundle.formatStringFromName("passwordPrompt", [this._userName, this.displayName], 2), This line is > 80 chars @@ +357,5 @@ > + let passwordURI = gServerUrl; > + login.init(passwordURI, null, passwordURI, username, password, "", ""); > + let logins = lm.findLogins({}, passwordURI, null, passwordURI); > + if (logins.some(function(l) login.matches(l, true))) > + lm.modifyLogin(l, logon); Is l actually in scope here? Also, what does logon refer to? I think it's undefined here. @@ +363,5 @@ > + lm.addLogin(login); > + }, > + > + logon: function (successCallback, failureCallback) { > + let resource = "/v2/auth"; const for this. @@ +372,5 @@ > + > + let signature = this.signature("GET", resource, curDate); > + dump("signature = " + signature + " resource = " + resource + "\n"); > + let req = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"] > + .createInstance(Ci.nsIXMLHttpRequest); Same question as before - why do we use this instead of http.jsm? @@ +376,5 @@ > + .createInstance(Ci.nsIXMLHttpRequest); > + this._request = req; > + this._successCallback = successCallback; > + this._failureCallback = failureCallback; > + req.open("GET", gServerUrl + "/v2/auth" + args, true); Should use the const instead of "/v2/auth" literal @@ +377,5 @@ > + this._request = req; > + this._successCallback = successCallback; > + this._failureCallback = failureCallback; > + req.open("GET", gServerUrl + "/v2/auth" + args, true); > + var me = this; let instead of var @@ +378,5 @@ > + this._successCallback = successCallback; > + this._failureCallback = failureCallback; > + req.open("GET", gServerUrl + "/v2/auth" + args, true); > + var me = this; > + req.onerror = function() {dump("logon failure\n"); me._failureCallback();} Break this up over several lines. @@ +408,5 @@ > + httpVerb + "\n" + contentMD5 + "\n" + contentType + "\n" + date + "\n" > + + (this._authToken == null ? "" : this._authToken) + "\n" + resource; > + > + dump("string to sign = " + stringToSign + "\n"); > + let utf8NotSoSecretAccessKey = "Thbird"; These should be consts. @@ +419,5 @@ > + let keyFactory = Cc["@mozilla.org/security/keyobjectfactory;1"] > + .getService(Ci.nsIKeyObjectFactory); > + > + let keyObject = keyFactory.keyFromString(Ci.nsIKeyObject.HMAC, > + utf8NotSoSecretAccessKey); Nit: Needs one more space of indentation to be lined up. @@ +433,5 @@ > + return "YSI " + apiKey + ":" + hash; > + } > + catch (ex) > + { > + dump(ex); Hm - we'll probably want to return something here as well. Maybe null. @@ +438,5 @@ > + } > + }, > +}; > + > +function nsYouSendItFileUploader(aYouSendIt, aFile, aRecipients, aCallback, aRequestObserver) { I think this is > 80 chars. @@ +457,5 @@ > + var thisYouSendIt = this.youSendIt; > + var thisUploader = this; > + let curDate = Date.now().toString(); > + > + return this._prepareToSend(function () {thisUploader.uploadFile()}, Again, I don't prefer the one-liner function def'ns. If you could break them up over more lines, that'd be preferable, I think. @@ +458,5 @@ > + var thisUploader = this; > + let curDate = Date.now().toString(); > + > + return this._prepareToSend(function () {thisUploader.uploadFile()}, > + function () {thisUploader.callback(thisYouSendIt, thisUploader.requestObserver, > 80 chars. @@ +462,5 @@ > + function () {thisUploader.callback(thisYouSendIt, thisUploader.requestObserver, > + Ci.nsIMsgCloudFileProvider.uploadErr);}); > + }, > + _prepareToSend: function (successCallback, failureCallback) { > + let resource = "/v2/item/send"; const @@ +468,5 @@ > + "&secureUrl=true"; > + let curDate = Date.now().toString(); > + > + let signature = this.youSendIt.signature("POST", resource, curDate); > + let req = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"] This vs. http.jsm? @@ +473,5 @@ > + .createInstance(Ci.nsIXMLHttpRequest); > + this._request = req; > + this._successCallback = successCallback; > + this._failureCallback = failureCallback; > + req.open("POST", gServerUrl + "/v2/item/send" + args, true); Use the const defined above @@ +476,5 @@ > + this._failureCallback = failureCallback; > + req.open("POST", gServerUrl + "/v2/item/send" + args, true); > + var thisUploader = this; > + var thisYouSendIt = this.youSendIt; > + req.onerror = function() {me._failureCallback();} Break up over several lines please. @@ +482,5 @@ > + let response = thisUploader._request.responseText; > + if (thisUploader._request.status >= 200 && > + thisUploader._request.status < 300) { > + let response = thisUploader._request.responseText.replace(/<\?xml[^>]*\?>/, ""); > + thisUploader._urlInfo = new XML(response); What happens if the XML is invalid? @@ +504,5 @@ > + req.setRequestHeader("Authorization", this.youSendIt._authToken + " "); > + req.setRequestHeader("Content-Type", "application/x-www-form-urlencoded"); > + req.send(); > + }, > + uploadFile : function() Space before function def'n. @@ +527,5 @@ > + dump("\nysi error = " + ysiError); > + let errorCode = docResponse['error-code']; > + dump("\nerror code = " + errorCode); > + thisUploader._commitSend(thisUploader.requestObserver); > + } catch (ex) {dump(ex);} Break up over several lines @@ +540,5 @@ > + thisUploader.callback(thisYouSendIt, thisUploader.requestObserver, > + Ci.nsIMsgCloudFileProvider.uploadErr); > + }; > + req.setRequestHeader("Date", curDate); > + let boundary = "------" + curDate; const for the magic string "------" @@ +541,5 @@ > + Ci.nsIMsgCloudFileProvider.uploadErr); > + }; > + req.setRequestHeader("Date", curDate); > + let boundary = "------" + curDate; > + let contentType = "multipart/form-data; boundary="+ boundary; Probably a const for the multipart/form-data string - also, a space before the +. @@ +543,5 @@ > + req.setRequestHeader("Date", curDate); > + let boundary = "------" + curDate; > + let contentType = "multipart/form-data; boundary="+ boundary; > + req.setRequestHeader("Content-Type", contentType); > + let fileContents = "--" + boundary +"\r\nContent-Disposition: form-data; name=\"bid\"\r\n\r\n" + Space after the + @@ +554,5 @@ > + // a temp file consisting of the message preamble, the file contents, and > + // the post script, and pass a stream based on that file to > + // nsIXMLHttpRequest.send(). > + > + try { There's some inconsistency in this file regarding where curly braces go (on the same opening line, or the next line unindented). We should probably just pick one and stick with it. @@ +557,5 @@ > + > + try { > + thisUploader._tempFile = thisUploader.getTempFile(this.file.leafName); > + let ostream = Cc["@mozilla.org/network/file-output-stream;1"] > + .createInstance(Ci.nsIFileOutputStream); Nit: I think this needs to be indented a single space more. @@ +562,5 @@ > + ostream.init(this._tempFile, -1, -1, 0); > + ostream.write(fileContents, fileContents.length); > + > + this._fstream = Cc["@mozilla.org/network/file-input-stream;1"] > + .createInstance(Ci.nsIFileInputStream); Same nit as above - please indent a single space more. @@ +570,5 @@ > + sstream.init(this._fstream); > + > + // This blocks the UI which is less than ideal. But it's a local > + // file operations so probably not the end of the world. > + while (sstream.available() > 0) { Could we use NetUtils.asyncCopy or asyncFetch to take this off of the main thread instead? @@ +591,5 @@ > + this._fstream = Cc["@mozilla.org/network/file-input-stream;1"] > + .createInstance(Ci.nsIFileInputStream); > + this._fstream.init(this._tempFile, -1, 0, 0); > + this._bufStream = Cc["@mozilla.org/network/buffered-input-stream;1"]. > + createInstance(Ci.nsIBufferedInputStream); Nit - period for this should be on the same line, and below and indented two spaces after the Cc. @@ +602,5 @@ > + dump(ex); > + throw ex; > + } > + }, > + _commitSend : function (urlListener) { Newline before function def'n. Some documentation on what this internal function is responsible for would also be good. @@ +606,5 @@ > + _commitSend : function (urlListener) { > + let req = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"] > + .createInstance(Ci.nsIXMLHttpRequest); > + this._request = req; > + var thisUploader = this; let instead of var @@ +607,5 @@ > + let req = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"] > + .createInstance(Ci.nsIXMLHttpRequest); > + this._request = req; > + var thisUploader = this; > + var thisYouSendIt = this.youSendIt; let instead of var @@ +608,5 @@ > + .createInstance(Ci.nsIXMLHttpRequest); > + this._request = req; > + var thisUploader = this; > + var thisYouSendIt = this.youSendIt; > + let resource = "/v2/item/commit/" + this._urlInfo.itemID; const for "/v2/item/commit/" @@ +609,5 @@ > + this._request = req; > + var thisUploader = this; > + var thisYouSendIt = this.youSendIt; > + let resource = "/v2/item/commit/" + this._urlInfo.itemID; > + let args = "?sendEmailNotifications=false"; const for this arg too. @@ +641,5 @@ > + req.setRequestHeader("Content-Type", "application/x-www-form-urlencoded"); > + req.setRequestHeader("X-YSI-Signature", signature); > + req.send(); > + }, > + getTempFile : function(leafName) { Newline before this function def'n. Also, no space before the :. @@ +643,5 @@ > + req.send(); > + }, > + getTempFile : function(leafName) { > + let tempfile = Cc["@mozilla.org/file/directory_service;1"] > + .getService(Ci.nsIProperties).get("TmpD", Ci.nsIFile); Nit: please line up the .getService two spaces indented below the Cc. @@ +649,5 @@ > + tempfile.createUnique(Components.interfaces.nsIFile.NORMAL_FILE_TYPE, 0666); > + // do whatever you need to the created file > + return tempfile.clone() > + }, > + cleanupTempFile: function () { Newline before this function def'n. ::: mailnews/compose/src/nsMsgAttachment.cpp @@ -108,0 +109,8 @@ > > +NS_IMETHODIMP nsMsgAttachment::GetSendViaCloud(bool *aSendViaCloud) > > +{ > > + NS_ENSURE_ARG_POINTER(aSendViaCloud); > > + NaN more ... Newline before this function def'n. @@ -108,0 +109,20 @@ > > +NS_IMETHODIMP nsMsgAttachment::GetSendViaCloud(bool *aSendViaCloud) > > +{ > > + NS_ENSURE_ARG_POINTER(aSendViaCloud); > > + NaN more ... It's been a while since I munged about in Moz's C++ - but isn't there something we should be doing to verify that &aAnnotation is what we think it is? ::: mailnews/compose/src/nsMsgAttachmentHandler.h @@ -176,3 +176,4 @@ > > bool mMHTMLPart; // This is true if its an MHTML part, otherwise, false > > bool mPartUserOmissionOverride; // This is true if the user send send the email without this part > > bool mMainBody; // True if this is a main body. > > + // true if this should be sent as a link to a file. To make it clearer which line this comment refers to, can you put it after the bool def'n, on the same line, like the others above?

(In reply to Mike Conley (:mconley) from comment #106) > Comment on attachment 588771 [details] [diff] [review] > be patch without MsgComposeCommands.js > > Review of attachment 588771 [details] [diff] [review]: > ----------------------------------------------------------------- > > + get displayName() {return "YouSendIt";}, > > As stated in some of my other comments, I think I prefer these to be written > like: > > get displayName() { > return "YouSendIt"; > }, Out of curiosity, any reason why you don't recommend: get displayName() "YouSendIt", ? This is what I would recommend if I was doing the review. Less typing, and more readable from my point of view (it's almost as simple as a property without custom getter/setter). I also use this form for all trivial functions only returning a value and fitting in 80 chars. > @@ +193,5 @@ > > + */ > > + > > + get fileUploadSizeLimit() {return this._maxFileSize;}, > > + /// -1 if we don't have this info > > + get remainingFileSpace() {this._availableStorage;}, > > Same as above, re: breaking up one-liners In this instance it's not just a matter of stylistic preference, the "return" keyword is missing, so that getter probably doesn't work. > @@ +260,5 @@ > > + setCachedAuthSecret: function(aAuthSecret) { > > + this._prefBranch.setCharPref("authSecret", aAuthSecret); > > + }, > > + wwwFormUrlEncoded : function(str) { > > + return encodeURIComponent(str).replace(/!/g, '%21').replace(/'/g, '%27').replace(/\(/g, '%28'). > > It blows my mind that there's no better, built-in way of doing this. Isn't this doing exactly what percentEncode from oauth.jsm does?

(In reply to Florian Quèze from comment #107) > (In reply to Mike Conley (:mconley) from comment #106) > > Comment on attachment 588771 [details] [diff] [review] > > be patch without MsgComposeCommands.js > > > > Review of attachment 588771 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > + get displayName() {return "YouSendIt";}, > > > > As stated in some of my other comments, I think I prefer these to be written > > like: > > > > get displayName() { > > return "YouSendIt"; > > }, > > Out of curiosity, any reason why you don't recommend: > get displayName() "YouSendIt", > ? > This is what I would recommend if I was doing the review. Less typing, and > more readable from my point of view (it's almost as simple as a property > without custom getter/setter). I also use this form for all trivial > functions only returning a value and fitting in 80 chars. Oh, yes, that's nice. I like that. I think I'm siding with Florian on this one. > > > @@ +193,5 @@ > > > + */ > > > + > > > + get fileUploadSizeLimit() {return this._maxFileSize;}, > > > + /// -1 if we don't have this info > > > + get remainingFileSpace() {this._availableStorage;}, > > > > Same as above, re: breaking up one-liners > > In this instance it's not just a matter of stylistic preference, the > "return" keyword is missing, so that getter probably doesn't work. Good catch!

(In reply to Jim Porter (:squib) from comment #102) > Created attachment 588776 [details] [diff] [review] > Add loading throbber while uploading cloud files > > This patch adds a loading throbber to the attachment in the list while we're > uploading the file. Based on the comments, I'm not sure if "done" really > means the file's uploaded or if we just got the URL for it We can't get the URL for it until after the upload completes, so yeah, we're really done.

UI note - convert to cloud file in the attachment pane is enabled even if there are no accounts provisioned, and puts up an empty sub-menu. Yousendit has reworked their api's, so I'm going to have to redo quite a bit of that code.

(In reply to David :Bienvenu from comment #110) > UI note - convert to cloud file in the attachment pane is enabled even if > there are no accounts provisioned, and puts up an empty sub-menu. Right. I just hadn't gotten to that yet, since the UI for that is wrong anyway (based on the design doc). Speaking of which: is it possible for us to change the content-disposition of an attachment? The design doc says that the "Convert to Cloud File" menu should have options for attaching inline and as an attachment. UI-wise, this is easy, of course, but if the backend doesn't support it (and if we don't have time to make it work), we'll have to come up with an alternative.

(In reply to Jim Porter (:squib) from comment #111) > > Right. I just hadn't gotten to that yet, since the UI for that is wrong > anyway (based on the design doc). Speaking of which: is it possible for us > to change the content-disposition of an attachment? The design doc says that > the "Convert to Cloud File" menu should have options for attaching inline > and as an attachment. UI-wise, this is easy, of course, but if the backend > doesn't support it (and if we don't have time to make it work), we'll have > to come up with an alternative. Since Thunderbird basically ignores the content-disposition when displaying received messages (the theory being that the recipient should get to decide how to display attachments, not the sender), I'm not sure what we expect these options to do. What problem are we trying to solve?

(Adding David Sifry, cause why wasn't he on this bug from the beginning?!? ;)

Just wanted to check in with you guys about when you feel that this code will be in a place that we can send it off to some other cloud providers that we're negotiating with. Will it be easily packagable so that they can understand what they need to do to bild their backend APIs and add themselves to the frontend code?

(In reply to David Sifry from comment #114) > Just wanted to check in with you guys about when you feel that this code > will be in a place that we can send it off to some other cloud providers > that we're negotiating with. Will it be easily packagable so that they can > understand what they need to do to bild their backend APIs and add > themselves to the frontend code? I've already cc'ed the Silver Oak folks on this bug, so they can see the code. Yes, by looking at the UI and Backend patches in this bug, they can figure out what they need to do to add themselves to the front end.

This gets us working with v2 of the yousendit api. I still need to rework some of the error handling because the way errors are returned has changed, but attaching works again.

Quick question: earlier, we discussed the idea of the "order" of cloud file accounts. I think the suggested solution was to add an "ordinal" pref to the branch for the account. How about something like this: mail.cloud_files.account1.type = Dropbox mail.cloud_files.account2.type = YouSendIt mail.cloud_files.account_order = account1,account2 This would make it easier to get a list of all the account keys (just get the account_order pref and split on ","), and we wouldn't have to worry about what to do when two accounts have the same ordinal.

(In reply to David :Bienvenu from comment #112) > Since Thunderbird basically ignores the content-disposition when displaying > received messages (the theory being that the recipient should get to decide > how to display attachments, not the sender), I'm not sure what we expect > these options to do. What problem are we trying to solve? Well, I'm not really a fan of how Thunderbird does this; I think "Display attachments inline" should be a tri-state defaulting to "follow the advice of the message". Really though, I'm just trying to follow the design doc. Maybe handling Content-Disposition is a thing for later.

Another piece that would be useful for the front-end: it would be helpful if nsIMsgAttachment had a property that indicated which cloud account the file was using (I need this to be able to show the current cloud account when showing the "Convert to Cloud File" submenu).

More questions/comments for the UI side of things: Stuff from the design doc: * What does it mean to be the "default" provider and how does that get set? * Where should the config UI live? The design doc talks about opening the config UI from the notification bar in the compose window, but what about when people want to configure additional accounts? * For the time being, can we have separate dialogs for configuring each cloud account rather than putting them on one page? Putting them all on one page will be fairly tough without seamless iframes. * I think we should hide the "Tell me more" / "What's this?" button in the notification if the user already has a cloud account set up in Thunderbird. At that point, they probably won't need that button. * Relatedly, instead of saying "Yes, I would!" / "Convert" when there are no accounts set up, maybe the button should say "Create an account" or something like that. Perhaps we could roll this in with the above point somehow and have the "Create an account" button also serve as the "What's this?" button. * nsIMsgCloudFileProvider should probably have two display name properties: 1) a display name for the provider type (e.g. "Dropbox"), and 2) a display name for the individual account (e.g. "Dropbox for bob@example.com"). Other stuff: * For upload errors, why not have a separate function to call (i.e. something other than onStopRequest)? * Should it be possible to drag/drop cloud attachments between compose windows? If so, how do we handle deletion of cloud attachments? When there's only one compose window, it's easy (just delete the file on the server), but if there are multiple windows, we probably have to do some kind of reference counting. * nsIMsgCloudFileProvider.uploadFile only seems to accept local files; shouldn't it accept anything file-like (e.g. attachments from other messages)? Maybe this is a v2 thing...

(In reply to Jim Porter (:squib) from comment #120) > More questions/comments for the UI side of things: I'll let Blake respond to the UI questions. > > * nsIMsgCloudFileProvider should probably have two display name properties: > 1) a display name for the provider type (e.g. "Dropbox"), and 2) a display > name for the individual account (e.g. "Dropbox for bob@example.com"). I think the second property should belong to the account object, not the cloud provider interface. > > Other stuff: > * For upload errors, why not have a separate function to call (i.e. something > other than onStopRequest)? Why? That would mean defining a new interface since we're doing this with xpcom. Is it hard to check the return status to see if it failed? My intent was that if you wanted more details about the failure, you could ask the cloud provider for lastError immediately upon getting notified of the failure, though Blake might have some qualms about displaying server error messages to the user. > * Should it be possible to drag/drop cloud attachments between compose > windows? Yeah, that definitely sounds like a v2 thing. > * nsIMsgCloudFileProvider.uploadFile only seems to accept local files; > shouldn't > it accept anything file-like (e.g. attachments from other messages)? Maybe > this is a v2 thing... You can always create a file first. Big Files is pretty file-based!

(In reply to Jim Porter (:squib) from comment #119) > Another piece that would be useful for the front-end: it would be helpful if > nsIMsgAttachment had a property that indicated which cloud account the file > was using (I need this to be able to show the current cloud account when > showing the "Convert to Cloud File" submenu). Since you know this yourself, you could always use a hash table to map between the attachment and the cloud provider. I could add an attribute, of course, but until then...

(In reply to Jim Porter (:squib) from comment #118) > Well, I'm not really a fan of how Thunderbird does this; I think "Display > attachments inline" should be a tri-state defaulting to "follow the advice > of the message". Really though, I'm just trying to follow the design doc. > Maybe handling Content-Disposition is a thing for later. Just as a note, the design doc was done by someone who had no idea what the code would look like. ;) If it's getting in your way, or if, after trying to use the feature, you notice that it would work better another way, please go ahead and code it up the way you think it should work, and throw me some email telling me what you did and why. Thanks, Blake.

(In reply to Jim Porter (:squib) from comment #119) > Another piece that would be useful for the front-end: it would be helpful if > nsIMsgAttachment had a property that indicated which cloud account the file > was using (I need this to be able to show the current cloud account when > showing the "Convert to Cloud File" submenu). Also, when deciding if something should be in the front end or backend (like the smarts to figure out how to convert arbitrary data sources into files), bear in mind that there's one front end and a number of different cloud providers, and we don't want to make it too hard to implement a provider.

(In reply to David :Bienvenu from comment #121) > > * nsIMsgCloudFileProvider should probably have two display name properties: > > 1) a display name for the provider type (e.g. "Dropbox"), and 2) a display > > name for the individual account (e.g. "Dropbox for bob@example.com"). > > I think the second property should belong to the account object, not the > cloud provider interface. The way the interface works now, an nsIMsgCloudFileProvider implementation is the provider, and an instance of that is the account. So displayName would essentially be a static member and accountDisplayName would be a regular instance member. > > * For upload errors, why not have a separate function to call (i.e. something > > other than onStopRequest)? > Why? That would mean defining a new interface since we're doing this with > xpcom. Is it hard to check the return status to see if it failed? My intent > was that if you wanted more details about the failure, you could ask the > cloud provider for lastError immediately upon getting notified of the > failure, though Blake might have some qualms about displaying server error > messages to the user. I thought it would be easier than trying to get the lastError, which worries me a little bit (we might end up reading a stale error if there's a bug somewhere). Maybe there's a way we can pass the error details to onStopRequest? Or maybe I won't need a whole lot in the way of details for the time being... > > * Should it be possible to drag/drop cloud attachments between compose > > windows? > Yeah, that definitely sounds like a v2 thing. Well, I'll have to explicitly disable that then, since it should work just fine up until the point where you try to delete cloud attachments. > > * nsIMsgCloudFileProvider.uploadFile only seems to accept local files; > > shouldn't > > it accept anything file-like (e.g. attachments from other messages)? Maybe > > this is a v2 thing... > > You can always create a file first. Big Files is pretty file-based! I suppose. I might just disallow this for now, since I don't really want to deal with all the edge cases, like what happens when the attachment hasn't been downloaded yet.

(In reply to David :Bienvenu from comment #124) > (In reply to Jim Porter (:squib) from comment #119) > > Another piece that would be useful for the front-end: it would be helpful if > > nsIMsgAttachment had a property that indicated which cloud account the file > > was using (I need this to be able to show the current cloud account when > > showing the "Convert to Cloud File" submenu). > > Also, when deciding if something should be in the front end or backend (like > the smarts to figure out how to convert arbitrary data sources into files), > bear in mind that there's one front end and a number of different cloud > providers, and we don't want to make it too hard to implement a provider. Well, the only "backend" bit here would be a property on nsIMsgAttachment. All the manipulation of it would probably happen in the UI code.

(In reply to Jim Porter (:squib) from comment #126) > > Also, when deciding if something should be in the front end or backend (like > > the smarts to figure out how to convert arbitrary data sources into files), > > bear in mind that there's one front end and a number of different cloud > > providers, and we don't want to make it too hard to implement a provider. > > Well, the only "backend" bit here would be a property on nsIMsgAttachment. > All the manipulation of it would probably happen in the UI code. I was specifically referring to your suggestion that the uploadFiles method should take an arbitrary parameter that could be converted to an nsIFile, like a message url. Sorry if that wasn't clear.

(In reply to David :Bienvenu from comment #127) > (In reply to Jim Porter (:squib) from comment #126) > > > > Also, when deciding if something should be in the front end or backend (like > > > the smarts to figure out how to convert arbitrary data sources into files), > > > bear in mind that there's one front end and a number of different cloud > > > providers, and we don't want to make it too hard to implement a provider. > > > > Well, the only "backend" bit here would be a property on nsIMsgAttachment. > > All the manipulation of it would probably happen in the UI code. > > I was specifically referring to your suggestion that the uploadFiles method > should take an arbitrary parameter that could be converted to an nsIFile, > like a message url. Sorry if that wasn't clear. Theoretically speaking, would it be possible to make attachments in received emails have an nsIFile subclass to get at their data? This would also make it easier for nsIMsgCopyService to support copying .eml attachments to mail folders.

(In reply to Jim Porter (:squib) from comment #128) > Theoretically speaking, would it be possible to make attachments in received > emails have an nsIFile subclass to get at their data? This would also make > it easier for nsIMsgCopyService to support copying .eml attachments to mail > folders. It's much easier to stream the attachment to a file using libmime. In a pluggable store world (or even, in an imap world), there is not necessarily a file at all.

(In reply to David :Bienvenu from comment #129) > It's much easier to stream the attachment to a file using libmime. In a > pluggable store world (or even, in an imap world), there is not necessarily > a file at all. What I'm really looking for is an arbitrary "stream" object that represents a file-like object (think FUSE or C++ I/O streams). Maybe that's something to grouse about in the future. It's not really essential at the moment.

squib: Just a note that your front-end patch has bitrotted since we landed the patch for bug 526998. -Mike

Comment on attachment 588776 [details] [diff] [review] Add loading throbber while uploading cloud files Review of attachment 588776 [details] [diff] [review]: ----------------------------------------------------------------- Hey - just noticed this after pulling the latest patches. ::: mail/components/compose/content/MsgComposeCommands.js @@ +989,5 @@ > +function convertToCloudAttachment(attachments, provider) > +{ > + let items = []; > + for (let [,attachment] in Iterator(attachments)) { > + let item = findItemForAttachment(aAttachment); This is broken - aAttachment isn't defined in this context. This should probably be findItemForAttachment(attachment);

I've got a patch that I'll try to upload today that should fix pretty much everything, and make conversion between cloud and regular attachments work. Deletion of cloud attachments is still causing me some issues, though. What do people think of these options for handling deletion? (Note that deletion includes converting from a cloud attachment back to a regular attachment.) 1) When you delete a cloud attachment from the attachment bucket, it gets deleted from cloud storage iff the attachment was added during the current composition session. (That way, deleting an attachment if you edit a previously-sent message won't invalidate your attachment.) 2) When deleting a cloud attachment, ask the user if they want to delete it from the server as well. 3) A combination of (1) and (2), i.e. do (1) when in the current session, or (2) in a separate session.

I think #1 is the way to go. I don't think asking the user whether they actually wanted to delete something that they just asked us to delete is a good idea. ;) The tricky part would be handling it when they save as a draft, I suspect.

For me, when I delete an attachment in Thunderbird, I think of it as a way of removing the binary file contents from my disk to save space. I don't assume that it deletes the attachment from the other recipients. So, if we'd like to persist that mental model, I agree with Blake - #1 sounds like the right choice.

In that case, if you add a cloud attachment, save the message as a draft, close the compose window, and come back to it later, deleting the cloud attachment will only remove it from the message, not delete it from the server. There's no reliable way for me to tell if that message has been sent or not, so we should probably play it safe. If the user really wants to be sure that their attachments are deleted from the server, they can always open their Dropbox (or what-have-you) and delete it there. This behavior should also make it easier to support dragging cloud attachments between compose windows.

> If the user really wants to be sure that their attachments are deleted from the > server, they can always open their Dropbox (or what-have-you) and delete it there. Yes, that sounds like the right course of action.

Comment on attachment 590520 [details] [diff] [review] support new yousendit api, address some review comments Review of attachment 590520 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/components/cloudfile/nsDropBox.js @@ +240,5 @@ > + me.setCachedAuthToken(me._connection.token); > + me.setCachedAuthSecret(me._connection.tokenSecret); > + me._successCallback();}, > + function () {dump("failed connecting\n"); > + me._requestObserver.onStopRequest(null, null, Ci.nsIMsgCloudFileProvider.authErr);}) Just noticed that _requestObserver is only defined in deleteFile. So if we fail to connect during file upload, Javascript complains that me._requestObserver is undefined.

(In reply to Jim Porter (:squib) from comment #120) > More questions/comments for the UI side of things: > > Stuff from the design doc: > * What does it mean to be the "default" provider and how does that get set? The default provider is the one that we upload files to when the user clicks the "Convert" button. > * Where should the config UI live? The design doc talks about opening the > config UI from the notification bar in the compose window, but what about > when people want to configure additional accounts? I thought it would be very similar to a Blog or News account, appearing in the Account Settings, and in the "File » New" menu. (Perhaps it's more like Outgoing Servers, though…) > * For the time being, can we have separate dialogs for configuring each cloud > account rather than putting them on one page? Putting them all on one page > will be fairly tough without seamless iframes. I tried to take a look at what you currently have, but couldn't find a way to configure my account (I think I might have run into the Auth Token limit)… Is there any way you (or mconley, maybe) could record a screencast of how you do various things in the feature as implemented? > * I think we should hide the "Tell me more" / "What's this?" button in the > notification if the user already has a cloud account set up in Thunderbird. > At that point, they probably won't need that button. I agree. We also need some real text for that dialog. > * Relatedly, instead of saying "Yes, I would!" / "Convert" when there are no > accounts set up, maybe the button should say "Create an account" or > something like that. Perhaps we could roll this in with the above point > somehow and have the "Create an account" button also serve as the "What's > this?" button. So, this is something I noticed that I think we need to change. When I first convert a file to a Cloud File, it shows a dialog and asks "Do you want to create a new DropBox account?", and gives me the options of Cancel and OK, neither of which are answers to the question. And neither of which are what I want to do, which is to connect my current DropBox account. I would suggest something more like: +-----------------------------------------------------+ | Set up Cloud Files | +-----------------------------------------------------+ | To convert this attachment to a link, | | we need a place to upload it. | | | | (Cancel) (Connect to DropBox) | +-----------------------------------------------------+ > * nsIMsgCloudFileProvider should probably have two display name properties: > 1) a display name for the provider type (e.g. "Dropbox"), and 2) a display > name for the individual account (e.g. "Dropbox for bob@example.com"). And the user can set that second name to whatever they want? ;) > Other stuff: > * Should it be possible to drag/drop cloud attachments between compose > windows? I think so… Does that answer most of your questions? I'ld love to see how it looks live, or even via a screencast, so that I could provide more feedback. (Or maybe we can get my account added to the DropBox auth key somehow?) Thanks, Blake.

Here are the first series of tests worth posting / looking at. Currently, I just test the following: 1) That the notification bar appears in the compose window when attaching files of a size larger than the threshold. 2) That uploading a file to DropBox, or requesting URLs for an uploaded file sends an appropriate HTTP request to a DropBox server 3) That a series of attached files can be queued for upload (Bienvenu's upload chaining) This patch depends on ObservationRecorder - a tool I developed in patch 715961, which has yet to land.

Here's a second intermediate patch which cleans up the API for cloudFileAccounts.js. Mostly, I made it look more like nsIMsgAccountManager (in a good way). There's more work to do with it, but the API should be nailed down now.

This patch makes the conversion between regular and cloud attachments work. I also added a Dropbox icon for UI testing purposes (shamelessly stolen from a random Google image search result). We probably want an "official" icon, as well as one for YouSendIt, but this is sufficient for now.

First, to get a Dropbox account set up, the easiest way is to attach a >1MB file, click "Convert", say "Ok" when asked about creating a Dropbox account, and then log in with mconley's info he emailed to everyone. Obviously, this needs some work so that account creation isn't totally insane, but I've been focusing on getting the attachment UI working first. (In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #139) > > * What does it mean to be the "default" provider and how does that get set? > > The default provider is the one that we upload files to when the user clicks > the "Convert" button. How should it be set? Just the first account? > > * Where should the config UI live? The design doc talks about opening the > > config UI from the notification bar in the compose window, but what about > > when people want to configure additional accounts? > > I thought it would be very similar to a Blog or News account, appearing in > the Account Settings, and in the "File » New" menu. (Perhaps it's more like > Outgoing Servers, though…) That might be nice. I'm not sure how easy it will be to fit this into the account settings UI, though. (Again, seamless iframes would help out a lot here.) > I tried to take a look at what you currently have, but couldn't find a way > to configure my account (I think I might have run into the Auth Token > limit)… Is there any way you (or mconley, maybe) could record a screencast > of how you do various things in the feature as implemented? There's no config UI yet; right now, the only concession I've made is the "Do you want to make a new Dropbox account?" prompt, which sets things up for the first time. To do anything else, you'd need to manually set prefs. > > * I think we should hide the "Tell me more" / "What's this?" button in the > > notification if the user already has a cloud account set up in Thunderbird. > > At that point, they probably won't need that button. > > I agree. We also need some real text for that dialog. Right. I'm also going to try to make the dialog work like a doorhanger, since I think that will let us use some slightly richer markup (e.g. a link to a webpage with more details). > So, this is something I noticed that I think we need to change. > > When I first convert a file to a Cloud File, it shows a dialog and asks "Do > you want to create a new DropBox account?", and gives me the options of > Cancel and OK, neither of which are answers to the question. And neither of > which are what I want to do, which is to connect my current DropBox account. > I would suggest something more like: [snip] The current UI is all very, very temporary. I just wanted to hack something together so that people could at least test out Dropbox without going into about:config. This will need to change, so that people can create YouSendIt accounts, or any other available account type. > > * nsIMsgCloudFileProvider should probably have two display name properties: > > 1) a display name for the provider type (e.g. "Dropbox"), and 2) a display > > name for the individual account (e.g. "Dropbox for bob@example.com"). > > And the user can set that second name to whatever they want? ;) Right. > > Other stuff: > > * Should it be possible to drag/drop cloud attachments between compose > > windows? > > I think so… Depending on how much time I have, this might go in v2, but we'll see. Based on the above discussion of how deletion should work, I think I know how to handle most of this. > Does that answer most of your questions? I'ld love to see how it looks > live, or even via a screencast, so that I could provide more feedback. (Or > maybe we can get my account added to the DropBox auth key somehow?) Assuming my instructions at the top of this comment don't help you out, I'll see if I can at least post screenshots.

cloud-ui now needs to be applied after cloud-be - I'll try to fix that right now...

(In reply to Jim Porter (:squib) from comment #143) > First, to get a Dropbox account set up, the easiest way is to attach a >1MB > file, click "Convert", say "Ok" when asked about creating a Dropbox account, > and then log in with mconley's info he emailed to everyone. Obviously, this > needs some work so that account creation isn't totally insane, but I've been > focusing on getting the attachment UI working first. Seems reasonable. > (In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #139) > > > * What does it mean to be the "default" provider and how does that get set? > > The default provider is the one that we upload files to when the user clicks > > the "Convert" button. > How should it be set? Just the first account? That was the original design, but since we're probably not doing the crazy settings dialog that it talked about, I think we might need a different way to set the default account. Something like the "Set Default" button for Outgoing Accounts would be a reasonable compromise. > > > * Where should the config UI live? > > I thought it would be very similar to a Blog or News account, appearing in > > the Account Settings, and in the "File » New" menu. > That might be nice. I'm not sure how easy it will be to fit this into the > account settings UI, though. (Again, seamless iframes would help out a lot > here.) Yeah, I agree. I could see putting it in "Preferences » Composition", or "Preferences » Cloud Files", if that would be any easier… > > I tried to take a look at what you currently have, but couldn't find a way > > to configure my account (I think I might have run into the Auth Token > > limit)… Is there any way you (or mconley, maybe) could record a screencast > > of how you do various things in the feature as implemented? > There's no config UI yet; right now, the only concession I've made is the > "Do you want to make a new Dropbox account?" prompt, which sets things up > for the first time. To do anything else, you'd need to manually set prefs. Right, let me know when you want me to take a look at that. > The current UI is all very, very temporary. I just wanted to hack something > together so that people could at least test out Dropbox without going into > about:config. This will need to change, so that people can create YouSendIt > accounts, or any other available account type. Okay. > > Does that answer most of your questions? I'ld love to see how it looks > > live, or even via a screencast, so that I could provide more feedback. (Or > > maybe we can get my account added to the DropBox auth key somehow?) > Assuming my instructions at the top of this comment don't help you out, I'll > see if I can at least post screenshots. Since it's all about mconley's account, I might just drop by his desk unexpected, and ask for a demo… ;) Thanks, Blake.

Jim, I needed to make these changes to get things working on windows (added dropbox.png to qute theme), and for the backend patch to apply...

This incorporates Jim's intermediate patch, and moves the dropbox icon change from the ui patch to the backend patch.

Jim, unless you object, I'm going to make a mozilla hg repo where you and I and Andreas can work together without having to pass patches around. I'll create the repo, and land the latest patches, and then we can land at will on the repo. It'll make it easier to collaborate, though there will be a bit of pain syncing with the trunk, and a fair chunk of pain at the end when we get ready to land on the trunk.

(In reply to David :Bienvenu from comment #148) > Jim, unless you object, I'm going to make a mozilla hg repo where you and I > and Andreas can work together without having to pass patches around. I fully support this plan.

(In reply to Mike Conley (:mconley) from comment #149) > (In reply to David :Bienvenu from comment #148) > > Jim, unless you object, I'm going to make a mozilla hg repo where you and I > > and Andreas can work together without having to pass patches around. > > I fully support this plan. Oh, and Mike can work there too :-) I should have this up and going by this afternoon.

Here's a repo we can share - http://hg.mozilla.org/users/bienvenu_nventure.com/big-files I pushed the latest versions of the front and back-end patches. I had to do the dropbox.png files for the themes by hand; not sure why that was, something might be wrong there. I'm just building now to see if it all still builds and works.

(In reply to David :Bienvenu from comment #151) > Here's a repo we can share - > > http://hg.mozilla.org/users/bienvenu_nventure.com/big-files Build from this repo seems to be working reasonably well.

(In reply to Jim Porter (:squib) from comment #125) > I thought it would be easier than trying to get the lastError, which worries > me a little bit (we might end up reading a stale error if there's a bug > somewhere). Maybe there's a way we can pass the error details to > onStopRequest? Or maybe I won't need a whole lot in the way of details for > the time being... I'm passing an error code in onStopRequest that's somewhat specific (auth error, offline, upload error, etc). The specific error text from the server may not be something we want to display; for one thing, it'll most likely be in English, and we have no control over it. But, in any case, if you ask for the error text when you get the onStopRequest call, you're guaranteed that it's up to date, since we've only got one UI thread, and it's not preempted. > > > Well, I'll have to explicitly disable that then, since it should work just > fine up until the point where you try to delete cloud attachments. Well, I believe what drag drop of a cloud attachment should do is essentially copy the url with some attachment wrapper, like the provider, etc. I suppose you'll need the nsIFile info as well so that you can delete the file from the cloud from the target as well, or convert back to a normal attachment. If you need more from the backend to make this work, let me know. > > I suppose. I might just disallow this for now, since I don't really want to > deal with all the edge cases, like what happens when the attachment hasn't > been downloaded yet. I think in the compose window case, the attachments are always downloaded (as opposed to the message display case).

To people using the shared repo, please let me know if I need to sync back with the comm-central trunk - this often happens when a mozilla-central change requires comm-central changes, because if you do python client.py checkout, you'll get the latest mozilla-central with the current shared repo comm-central files. I'll try to merge with the comm-central trunk every few days.

I landed some changes to the dropbox files, to address mconley's review comments, and fix some of the error handling with oauth/dropbox. I switched some dumps to log4moz - log4moz may not be ideal, but we've definitely standardized on it. I've left the for each's the way they are, and I agree with Florian about the suggested join usage; that's quite a bit less readable. I've still got to go through mconley's second round of comments.

(In reply to David :Bienvenu from comment #154) > To people using the shared repo, please let me know if I need to sync back > with the comm-central trunk - this often happens when a mozilla-central > change requires comm-central changes, because if you do python client.py > checkout, you'll get the latest mozilla-central with the current shared repo > comm-central files. I'll try to merge with the comm-central trunk every few > days. Heh, that didn't take long to happen. If you update the mozilla-central part of your local repo, you'll need to update comm-central as well - I merged back with the trunk and pushed a few minutes ago.

If the trunk gets out of sync with the big files repo in important ways, and I'm not around, here's how to do a merge with the comm-central trunk: hg pull https://hg.mozilla.org/comm-central/ hg merge (and hope that there were no problems in the merge, otherwise correct them) hg commit hg push

mconley, re account info/quota info, from what I can tell, YouSendIt will tell you how much room you have left, but not how much you've used up. Dropbox tells you how much your total allocation is, and how much you've used in shared space and non-shared space. In general, to deal with different providers, you'll need to be tolerant of not getting certain values (I've indicated in the interface that you'll get -1 for values that the providers don't implement).

bienvenu: That's fine. The way I figure it (and basing it off of squibs advice), the page that displays things like remaining space / space used, will be an iframe embedded in the manager tab, render XHTML that is customizable from provider to provider. I think we'll be fine. Thanks, -Mike

mconley, I landed support for refreshUserInfo and fileSpaceUsed (for Dropbox). I've also converted most of the dump statements in YouSendIt and DropBox to use log4moz log modules (log modules have the obvious names). I haven't test refreshUserInfo calls yet, but I thought I'd land what I have. I'll try to test them locally in a little bit.

I've landed my first round of tests on our branch.

I did some hand-testing of refreshUserInfo and landed some fixes.

I've started improving the error handling, and fixed an issue where attempting to upload a file to Dropbox with bad tokens would fail silently. I also simplified some of the Dropbox code so that we hopefully won't run into this again.

I also fixed conversion from cloud attachments back to local attachments. Still to do in this area: * Convert between different cloud providers * Delete the cloud attachment when converting from cloud storage * Disallow conversion while an upload is happening (also add a "stop upload" command?) * Fix (or disallow) drag-and-drop between compose windows

One thing I noticed: uploadListener.onStartRequest is never called. Should we modify the providers to call this before we start the upload process? I want to put some code in there to consolidate things like the loading throbber.

(In reply to Jim Porter (:squib) from comment #165) > One thing I noticed: uploadListener.onStartRequest is never called. Should > we modify the providers to call this before we start the upload process? I > want to put some code in there to consolidate things like the loading > throbber. It depends on what you want to show when the file is pending upload, but the upload hasn't started (e.g., you select 3 files to upload from the attach dialog). If you'll have some sort of throbber/icon that indicates a pending upload, then yes, we can call onStartRequest when the upload starts. If you want to just start the throbber when you've queued the upload, then there's no need to call onStartRequest.

I've added a settingsURL read-only attribute to nsIMsgCloudFileProvider. This attribute allows the provider to specify a chrome URL for settings content that gets loaded into an iframe in the Preferences > Attachments > Outgoing pane.

(In reply to Mike Conley (:mconley) from comment #167) > I've added a settingsURL read-only attribute to nsIMsgCloudFileProvider. > This attribute allows the provider to specify a chrome URL for settings > content that gets loaded into an iframe in the Preferences > Attachments > > Outgoing pane. We may also want a way to get to the provider UI that allows the user to manage the storage of files in their cloud account (e.g., delete old files to get some space back), though of course, that will break any messages that were sent with the link. Maybe settings will be the same url, or close enough. Blake, what do you think?

Were you thinking of something like http://dl.dropbox.com/u/2301433/Screenshots/DropBoxTab.png ? Or just opening that url in the user's default browser? Or something more complicated, like shell integration that opens the actual directory on the user's computer? (Also, while I'm here, why is our app named "BigFiles" instead of "Thunderbird"? Can we change that?) Thanks, Blake.

(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #169) > Were you thinking of something like > http://dl.dropbox.com/u/2301433/Screenshots/DropBoxTab.png ? Yeah, opening that in a TB tab, I think. > > (Also, while I'm here, why is our app named "BigFiles" instead of > "Thunderbird"? Can we change that?) Presumably, when mconley signed up for an app key, he gave the app name as BigFiles. I don't know if we can change that w/o signing up for a new app key, but when we go official, we'll get a new key and call the app Thunderbird.

mconley: looks like you forgot to add mail/components/cloudfile/content/addAccountDialog.js...

squib: Good catch! Thanks, committed. -Mike

I added providerUrlForError(aError) to allow us to optionally open provider content in a web tab for certain errors.

I did some cleanup for the account creation dialog since I ran into a couple of sharp edges (e.g. if you start creating a Dropbox account and then back out, you have to dig into about:config and delete the half-created account's prefs in order to try again). I also improved the notification bar in the compose window. Next up for me: * Make sure we use the display name for cloud accounts wherever appropriate * Add "Upgrade" buttons to the appropriate error dialogs

Just as a note, the try builds I started yesterday afternoon are done, and live at https://ftp.mozilla.org/pub/mozilla.org/thunderbird/try-builds/bwinton@latte.ca-6b4c1c58b211/ If any of you feel like you've added or fixed anything important since then, let me know, and I'll start another set of builds on Sunday evening… Thanks, Blake.

It looks, I've commented in the wrong bug. Please see Bug 721381 comment 4

(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #175) > Just as a note, the try builds I started yesterday afternoon are done, and > live at > https://ftp.mozilla.org/pub/mozilla.org/thunderbird/try-builds/bwinton@latte. > ca-6b4c1c58b211/ If any of you feel like you've added or fixed anything > important since then, let me know, and I'll start another set of builds on > Sunday evening… I've pushed some more style tweaks to the preferences pane, so if you want to kick off another build that would be great.

Kicked off. If I understand the try server, I think builds should appear at https://ftp.mozilla.org/pub/mozilla.org/thunderbird/try-builds/bwinton@latte.ca-a0210471f8e1/ sometime soon. :)

andreasn: I replaced the dummy space chart with a real Protovis chart. Feel free to tweak it to your liking.

mconley: regarding changeset 343cb4b25e18, I think it would be preferable if we used the nsIMsgCloudFileProvider.iconClass property for the icons in the richlist. That way, it'll be easier for add-ons to make everything just work (i.e. they won't have to define their own CSS in addition to everything else).

Hey all, I just wanted to get an idea of what's left for us to do. I can think of the following: * s/DropBox/Dropbox/g (I'll do this over the weekend to minimize clashes) * Handle deletion of cloud attachments where possible (I'll take this too) * Disable drag-and-drop between compose windows (Also me; I'll re-enable this once I refactor the drag-and-drop code to be more extensible) * Theming/prettiness * Go over wording, add localizations wherever necessary * Make links in the management UI work * Tests Anything else?

Hey Jim, we've been tracking this stuff in the Feature Page, at https://wiki.mozilla.org/Features/Thunderbird/BigFiles#Team_status_notes Would you mind adding these things there, and hopefully the dates you think they can be done by? Thanks, Blake.

I added "disallow send while upload is still in progress" - we can't start the send until all the uploads are finished, so we should ask the user if they want to cancel the upload and send or wait until the upload is finished and then do the send.

I synced back with the trunk this morning and landed it. Now I'm working on trying to make insertion of links happen after the last non-quoted text, on send/send later. Any hints on how to do stuff with the editor appreciated - it's not obvious how to go from a dom node to a cursor position in the editor. I'll probably look at the spell checker code to figure it out.

Milestone 1 try builds are being spun up here: https://ftp.mozilla.org/pub/mozilla.org/thunderbird/try-builds/mconley@mozilla.com-60e014150094/

Dropbox is failing for me, so I can't really test this code very well, but I've landed some code to try to insert urls after the last top level non-empty text node. In order to debug it, I've made it insert the url at actual upload time, so I can play with it, *and* at send time, so we can see if that works. I pass in the attachments instead of just the url, so we can put in some nice html with the file name and the provider, once we decide what that should look like. I also insert a url on failure, since I can't get a provider to work...I'll remove that, and the insertion of a url at upload time once we think the code that figures out where to put the urls is doing approximately the right thing.

I've added a cancelFileUpload(aFile) method to the cloud file provider interface, which should cancel a current or pending upload, and ignore an upload that's already completed. It's completely untested, so, squib, when you have code that calls it, and it breaks, let me know and I'll fix it :-)

I should have the UI hooked up to the cancel code on the backend. I tweaked some of the nsDropbox code to make it work right, but I'm not sure the way I handled it is the right way. I also changed how the loading throbbers work, partly because the onStartRequest gets called about a second after we click "ok", and I wanted to show the "connecting" state. This does mean that we have the Firefox-style connecting/loading icons, so maybe we can use those elsewhere (Aero already uses the loading icon, I think). Finally, I fixed a bug where the notification bar popped up even if you initially clicked "attach to cloud". I'm going to go in and start bullet-proofing some of the code so that I don't regress this stuff as much (and start looking into tests).

(In reply to Jim Porter (:squib) from comment #188) > I also changed how the loading throbbers work, partly because the > onStartRequest gets called about a second after we click "ok", and I wanted > to show the "connecting" state. This does mean that we have the > Firefox-style connecting/loading icons, so maybe we can use those elsewhere > (Aero already uses the loading icon, I think). There's an other state, which is "waiting for previous uploads to finish". If you select several files from the attach dialog, the first file will get uploaded and the others will get queued. Those other files should be in some sort of pending state.

(In reply to David :Bienvenu from comment #189) > There's an other state, which is "waiting for previous uploads to finish". > If you select several files from the attach dialog, the first file will get > uploaded and the others will get queued. Those other files should be in some > sort of pending state. I guess there's really no way for you to distinguish between the connecting and waiting to upload states...I think they can be merged (but probably not called "connecting...", perhaps, pending.)

(In reply to David :Bienvenu from comment #190) > I guess there's really no way for you to distinguish between the connecting > and waiting to upload states...I think they can be merged (but probably not > called "connecting...", perhaps, pending.) Right. I'm just saying "connecting" here because that's the filename of the image from Firefox. I'll probably add some more-appropriate string for it in the UI, though.

if you're encountering build bustage, I've merged with the comm-central trunk again and pushed - it should build now.

(In reply to Jim Porter (:squib) from comment #188) > I should have the UI hooked up to the cancel code on the backend. I tweaked > some of the nsDropbox code to make it work right, but I'm not sure the way I > handled it is the right way. > > I also changed how the loading throbbers work, partly because the > onStartRequest gets called about a second after we click "ok", and I wanted > to show the "connecting" state. This does mean that we have the > Firefox-style connecting/loading icons, so maybe we can use those elsewhere > (Aero already uses the loading icon, I think). Pinstripe and Gnomestripe has these too, so it should easy to use those.

I've pushed a change that starts the process of getting cloud files to save the info for drafts in a real mime part - it's just a start, because finding the appropriate code to change was a challenge. We can also generate an html part that describes the annotation, once mconley gets that part written...

(In reply to Andreas Nilsson (:andreasn) from comment #193) > (In reply to Jim Porter (:squib) from comment #188) > > I should have the UI hooked up to the cancel code on the backend. I tweaked > > some of the nsDropbox code to make it work right, but I'm not sure the way I > > handled it is the right way. > > > > I also changed how the loading throbbers work, partly because the > > onStartRequest gets called about a second after we click "ok", and I wanted > > to show the "connecting" state. This does mean that we have the > > Firefox-style connecting/loading icons, so maybe we can use those elsewhere > > (Aero already uses the loading icon, I think). > > Pinstripe and Gnomestripe has these too, so it should easy to use those. Right. I had to copy them from Firefox though, since everything but Aero currently uses the Toolkit loading throbber, whereas Aero uses the Firefox loading throbber.

Two updates (one good, one bad): Good: It turns out that the plain text editor is also an nsIHTMLEditor, so we can insert some HTML to make it easier to find and destroy the attachment links in the body Bad: If you have two compose windows open, all the attachment events are fired on all the windows, so the background window ends up responding to bogus events. Any ideas on how to fix this? (I was thinking we use regular DOM events instead of the observer service.)

Ok, the latest commit handles insertion/removal of cloud links in real-time (plain-text only). I've only lightly tested it, and it might need some cleanup, but it seems to work for the obvious cases anyway. I also put it in its own file, since the code is totally isolated from the rest of the composer. If that's wrong, feel free to change it. Obviously, this code will need to change a bit when we figure out a better way to handle the notifications.

(In reply to Jim Porter (:squib) from comment #197) > Ok, the latest commit handles insertion/removal of cloud links in real-time > (plain-text only). HTML composition works now too, though it needs some style work, and it needs to be inserted in the right spot (I just put it at the end of the body for now, which is probably wrong if you have sigs or quotes-on-bottom). It should be straightforward to pretty things up based on the existing code.

Question for Blake/Mike - now that we're inserting the links at attach time, are we going to keep them together when the files are attached at different times? I.e., I attach a cloud file, we insert a link at some reasonable place, then the user types some more, e.g., types some text after the inserted links, and then the user attaches an other file. Do we try to keep the links together? Or do we try to put each link near where the user is typing at the time?

I think it might be simplest to keep the links together. It looks like this is what squib's code attempts to do already.

Just a heads-up, the access keys in this changeset will conflict with existing access keys: http://hg.mozilla.org/users/bienvenu_nventure.com/big-files/rev/42be07b427cc ("Subject" and "Add to To", respectively). Also, we should probably either say "Share" and "Attach" or make those strings pluralizable, since "Attach it" doesn't make sense when you have multiple attachments to act on.

I believe I fixed the mixed attachments bug, and added the ability to save messages with cloud attachment as drafts and reload them. I pushed that to our repo, and requested a try server build - http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=e9ed20b05ffa - since I'm the only person who has tested it, I won't suggest we use it for m2, but it would be great if folks could try it (or just build the tip of the repo themselves) and let me know if the mixed-type attachment issues are fixed for them.

All: I pushed a few changes today that change the prefs locations for account info, and moves auth tokens and secrets into nsILoginManager. As such, your cloudfiles accounts will need to be re-setup in TB. Just thought I'd give a heads up, -Mike