Bug 698925 (BigFiles)

add support for sending attachments via the cloud

RESOLVED FIXED in Thunderbird 12.0

Status

defect
RESOLVED FIXED
8 years ago
5 years ago

People

(Reporter: Bienvenu, Unassigned)

Tracking

(Depends on 6 bugs, Blocks 1 bug, {dev-doc-needed})

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(9 attachments, 47 obsolete attachments)

81.46 KB, patch
Details | Diff | Splinter Review
16.87 KB, patch
Details | Diff | Splinter Review
52.57 KB, patch
Details | Diff | Splinter Review
54.39 KB, patch
Details | Diff | Splinter Review
74.81 KB, patch
Details | Diff | Splinter Review
2.64 KB, patch
Bienvenu
: feedback+
Details | Diff | Splinter Review
37.10 KB, patch
Bienvenu
: review+
standard8
: superreview+
Details | Diff | Splinter Review
88.88 KB, patch
mconley
: review+
standard8
: superreview+
Details | Diff | Splinter Review
374.82 KB, patch
Bienvenu
: review+
mconley
: ui-review+
squib
: feedback+
Details | Diff | Splinter Review
(Reporter)

Description

8 years ago
leaving hidden while we work out some details...
(Reporter)

Comment 1

8 years ago
Posted patch initial skeleton (obsolete) — Splinter Review
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.
(Reporter)

Comment 2

8 years ago
Posted file start work on yousendit (obsolete) —
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.
Attachment #571176 - Attachment is obsolete: true
(Reporter)

Comment 3

8 years ago
Posted patch got attach menu stuff working (obsolete) — Splinter Review
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.
Attachment #572591 - Attachment is obsolete: true
(Reporter)

Comment 4

8 years ago
Posted patch try to logon to yousendit (obsolete) — Splinter Review
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.
Attachment #573668 - Attachment is obsolete: true
(Reporter)

Comment 5

8 years ago
Posted patch get to uploading file part (obsolete) — Splinter Review
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...
Attachment #574369 - Attachment is obsolete: true
(Reporter)

Comment 6

8 years ago
Irving, the patch has a sample manifest file for a js component.
(Reporter)

Comment 7

8 years ago
Posted patch start work on oauth (obsolete) — Splinter Review
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.
Attachment #574767 - Attachment is obsolete: true
(Reporter)

Comment 8

8 years ago
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.
Attachment #579056 - Attachment is obsolete: true
(Reporter)

Comment 9

8 years ago
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.
(Reporter)

Comment 10

8 years ago
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.
(Reporter)

Comment 11

8 years ago
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.
Attachment #579162 - Attachment is obsolete: true
(Reporter)

Comment 12

8 years ago
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…)
(Reporter)

Comment 14

8 years ago
(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 :-)
(Reporter)

Comment 15

8 years ago
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.
(Reporter)

Comment 16

8 years ago
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.
(Reporter)

Comment 17

8 years ago
Posted patch get oauth working (obsolete) — Splinter Review
the gets oauth working (step 1 of the 7 or so for getting dropbox to work)
Attachment #579834 - Attachment is obsolete: true
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.
(Reporter)

Comment 22

8 years ago
Attachment #580529 - Attachment is obsolete: true
(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.
(Reporter)

Comment 24

8 years ago
(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.
(Reporter)

Comment 25

8 years ago
Posted patch checkpointing work (obsolete) — Splinter Review
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.
(Reporter)

Comment 26

8 years ago
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.
(Reporter)

Comment 27

8 years ago
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.
Attachment #581309 - Attachment is obsolete: true
(Reporter)

Comment 28

8 years ago
Posted patch UI work (obsolete) — Splinter Review
I think the line endings are screwed up in the dtd file for some reason, sorry about that.
(Reporter)

Comment 29

8 years ago
Posted patch backend work (obsolete) — Splinter Review
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.
(Reporter)

Comment 32

8 years ago
(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.
(Reporter)

Comment 33

8 years ago
Posted patch checkpoint backend work (obsolete) — Splinter Review
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.
Attachment #581496 - Attachment is obsolete: true
(Reporter)

Comment 34

8 years ago
Ah, I remember now that I did implement createNewAccount for YouSendIt though it's completely untested.
(Reporter)

Comment 35

8 years ago
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++.
(Reporter)

Comment 36

8 years ago
Posted patch checkpoint backend work (obsolete) — Splinter Review
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.
Attachment #581692 - Attachment is obsolete: true
Posted patch Update UI patch (obsolete) — Splinter Review
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.
Attachment #581152 - Attachment is obsolete: true
Attachment #581495 - Attachment is obsolete: true
(Reporter)

Comment 38

8 years ago
Posted patch BE - more work on yousendit (obsolete) — Splinter Review
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.
Attachment #581782 - Attachment is obsolete: true
(Reporter)

Comment 39

8 years ago
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.
(Reporter)

Comment 40

8 years ago
Posted patch binary files for yousendit (obsolete) — Splinter Review
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.
Attachment #582148 - Attachment is obsolete: true
(Reporter)

Comment 41

8 years ago
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
Attachment #582317 - Attachment is obsolete: true
Posted patch More updates to UI patch (obsolete) — Splinter Review
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.
Attachment #581877 - Attachment is obsolete: true
(Reporter)

Comment 43

7 years ago
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?)
(Reporter)

Comment 45

7 years ago
Attachment #581172 - Attachment is obsolete: true
Attachment #581416 - Attachment is obsolete: true
Attachment #582401 - Attachment is obsolete: true
(Reporter)

Comment 46

7 years ago
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).
Posted patch Remove hardcoded password (obsolete) — Splinter Review
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...
Attachment #582761 - Attachment is obsolete: true
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?)
Posted patch Further UI updates (obsolete) — Splinter Review
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).
Attachment #583078 - Attachment is obsolete: true
(Reporter)

Comment 52

7 years ago
(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.
(Reporter)

Comment 53

7 years ago
(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.)
(Reporter)

Comment 55

7 years ago
(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).
(Reporter)

Comment 59

7 years ago
(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 :-)
(Reporter)

Comment 60

7 years ago
(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.
(Reporter)

Comment 61

7 years ago
(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.
(Reporter)

Comment 64

7 years ago
(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.
(Reporter)

Comment 66

7 years ago
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).
Attachment #582998 - Attachment is obsolete: true
(Reporter)

Comment 67

7 years ago
Attachment #583534 - Attachment is obsolete: true
(Reporter)

Comment 68

7 years ago
Attachment #583557 - Attachment is obsolete: true
Thanks - that's fixed Linux builds for me.
(Reporter)

Comment 70

7 years ago
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.
Attachment #583564 - Attachment is obsolete: true
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.
(Reporter)

Comment 73

7 years ago
(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.
(Reporter)

Comment 74

7 years ago
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...
Attachment #583933 - Attachment is obsolete: true
(Reporter)

Comment 75

7 years ago
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).
Attachment #585224 - Attachment is obsolete: true
(Reporter)

Comment 76

7 years ago
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.
Attachment #585456 - Attachment is obsolete: true
(Reporter)

Comment 77

7 years ago
this makes youSendIt queue upload requests.
Attachment #585902 - Attachment is obsolete: true
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
Posted patch Intermediate patch (obsolete) — Splinter Review
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.
Posted patch UI patch v5(?) (obsolete) — Splinter Review
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. :)

Updated

7 years ago
Attachment #583090 - Attachment is obsolete: true
> 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.
(Reporter)

Comment 84

7 years ago
(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".
(Reporter)

Comment 86

7 years ago
(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 :-)
(Reporter)

Comment 87

7 years ago
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.
(Reporter)

Comment 89

7 years ago
(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.
(Reporter)

Comment 91

7 years ago
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.
(Reporter)

Comment 92

7 years ago
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.
(Reporter)

Comment 93

7 years ago
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)
Attachment #586229 - Attachment is obsolete: true
Attachment #587575 - Attachment is obsolete: true
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.
(Reporter)

Comment 97

7 years ago
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.
Attachment #587859 - Flags: feedback?(mconley)
Attachment #587859 - Flags: feedback?(mbanner)
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.
(Reporter)

Comment 99

7 years ago
(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.
(Reporter)

Comment 100

7 years ago
you'll need to add

Components.utils.import("resource:///modules/cloudFileAccounts.js");

to MsgComposeCommands.js to get things working again.
Attachment #587859 - Attachment is obsolete: true
Attachment #587859 - Flags: feedback?(mconley)
Attachment #587859 - Flags: feedback?(mbanner)
(Reporter)

Comment 101

7 years ago
Posted patch ui patch with added include (obsolete) — Splinter Review
I don't want to bitrot squib, but fwiw, here's a new ui patch with the extra include.
(Reporter)

Updated

7 years ago
Attachment #588771 - Flags: feedback?(mconley)
Attachment #588771 - Flags: feedback?(mbanner)
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.
Attachment #588772 - Attachment is obsolete: true
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?
(Reporter)

Comment 104

7 years ago
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?
Attachment #588771 - Flags: feedback?(mconley) → feedback+
(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!
(Reporter)

Comment 109

7 years ago
(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.
(Reporter)

Comment 110

7 years ago
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.
(Reporter)

Comment 112

7 years ago
(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?!? ;)

Comment 114

7 years ago
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?
(Reporter)

Comment 115

7 years ago
(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.
(Reporter)

Comment 116

7 years ago
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.
Attachment #588771 - Attachment is obsolete: true
Attachment #588771 - Flags: feedback?(mbanner)

Updated

7 years ago
Attachment #587577 - Attachment is obsolete: true
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...
(Reporter)

Comment 121

7 years ago
(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!
(Reporter)

Comment 122

7 years ago
(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.
(Reporter)

Comment 124

7 years ago
(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.
(Reporter)

Comment 127

7 years ago
(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.
(Reporter)

Comment 129

7 years ago
(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.
Posted patch Intermediate patch v2 (obsolete) — Splinter Review
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.
Attachment #588776 - Attachment is obsolete: true
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.
(Reporter)

Comment 144

7 years ago
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.
(Reporter)

Comment 146

7 years ago
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...
(Reporter)

Comment 147

7 years ago
This incorporates Jim's intermediate patch, and moves the dropbox icon change from the ui patch to the backend patch.
Attachment #590520 - Attachment is obsolete: true
Attachment #591013 - Attachment is obsolete: true
(Reporter)

Comment 148

7 years ago
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.
(Reporter)

Comment 150

7 years ago
(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.
(Reporter)

Comment 151

7 years ago
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.
(Reporter)

Updated

7 years ago
Group: mozilla-confidential, mozilla-messaging-confidential
(Reporter)

Comment 152

7 years ago
(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.
(Reporter)

Comment 153

7 years ago
(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).
(Reporter)

Comment 154

7 years ago
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.
(Reporter)

Comment 155

7 years ago
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.
(Reporter)

Comment 156

7 years ago
(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.
(Reporter)

Comment 157

7 years 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
(Reporter)

Comment 158

7 years ago
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
(Reporter)

Comment 160

7 years ago
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.
(Reporter)

Comment 162

7 years ago
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.
(Reporter)

Comment 166

7 years ago
(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.
(Reporter)

Comment 168

7 years ago
(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.
(Reporter)

Comment 170

7 years ago
(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
Version: unspecified → Trunk
(Reporter)

Comment 173

7 years ago
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.
(Reporter)

Comment 183

7 years ago
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.
(Reporter)

Comment 184

7 years ago
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/
Target Milestone: --- → Thunderbird 12.0
(Reporter)

Comment 186

7 years ago
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.
(Reporter)

Comment 187

7 years ago
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).
(Reporter)

Comment 189

7 years ago
(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.
(Reporter)

Comment 190

7 years ago
(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.
(Reporter)

Comment 192

7 years ago
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.
(Reporter)

Comment 194

7 years ago
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.
(Reporter)

Comment 199

7 years ago
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.

Updated

7 years ago
Depends on: 731932
(Reporter)

Comment 202

7 years ago
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
David:

So here's what I did to nsMsgCompose.cpp.  It seems to successfully wrap both signatures and the quote prefixes in spans - however, I'm a bit confused about whether or not I'm checking for the right things and catching all of the cases...

Is there ever a case, for example, where the editor is *not* an HTML editor?  And my changes are within a block that's executing of aQuoted is passed as "true" to ConvertAndLoadComposeWindow...under what circumstances is aQuoted passed as false, and do we need to account for that?

Thanks,

-Mike
Attachment #604112 - Flags: feedback?(dbienvenu)
(Reporter)

Comment 205

7 years ago
(In reply to Mike Conley (:mconley) from comment #204)
> Created attachment 604112 [details] [diff] [review]
> Wrap plaintext signatures and quote prefixes in spans
> 
> David:
> 
> So here's what I did to nsMsgCompose.cpp.  It seems to successfully wrap
> both signatures and the quote prefixes in spans - however, I'm a bit
> confused about whether or not I'm checking for the right things and catching
> all of the cases...
> 
> Is there ever a case, for example, where the editor is *not* an HTML editor?

I don't think so. If plain text and html compose both have an html editor, then we're ok. The import code creates a fake editor which is not an html editor, iirc, but I don't think we'll hit your code in that case. We certainly don't add sigs when we import :-)

> And my changes are within a block that's executing of aQuoted is passed as
> "true" to ConvertAndLoadComposeWindow...under what circumstances is aQuoted
> passed as false, and do we need to account for that?

turns out this is what you'd expect - all types of replies pass in aQuoted as true, everything else (new message, forward as attachment, edit message as new, etc) pass in aQuoted as false. Not sure about forward inline; I would expect aQuoted to be true there, but the code doesn't look that way. In any case, you do need to "handle" the non quoted case, because that's the "write new message" case.

You don't need to initialize comptrs to null; they do that for free.
(Reporter)

Comment 206

7 years ago
Comment on attachment 604112 [details] [diff] [review]
Wrap plaintext signatures and quote prefixes in spans

feedback+, modulo my comments.

Sigs need to be handled for both quoted and non-quoted case. There's no citation handling for non-quoted case, I don't think.

Does the "-----Original Message----" line when doing a forward inline cause us any grief? If so, you might want to handle that as well. It's inserted by mimedrft.cpp mime_insert_all_headers() with this code like this:

    NS_MsgSACopy(&(newBody), "<HTML><BODY><BR><BR>");

    NS_MsgSACat(&newBody, MimeGetNamedString(MIME_FORWARDED_MESSAGE_HTML_USER_WROTE));
    NS_MsgSACat(&newBody, MIME_HEADER_TABLE);
Attachment #604112 - Flags: feedback?(dbienvenu) → feedback+
(Reporter)

Comment 207

7 years ago
Posted patch backend changes for big files (obsolete) — Splinter Review
These backend changes can be reviewed on their own, and I'm trying to make the final patch a bit smaller and get the review process going so we can hope to land the whole feature by Tuesday.
Attachment #604567 - Flags: superreview?(mbanner)
Attachment #604567 - Flags: review?(squibblyflabbetydoo)
Comment on attachment 604567 [details] [diff] [review]
backend changes for big files

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

I think this looks ok, modulo the comments below. I'm not 100% confident in my ability to comprehend the sending code, though.

::: mailnews/compose/src/nsMsgSend.cpp
@@ +2096,5 @@
> +        // Check to see if this is a file URL, if so, don't retrieve
> +        // like a remote URL...
> +        bool sendViaCloud = false;
> +        attachment->GetSendViaCloud(&sendViaCloud);
> +          if (nsMsgIsLocalFile(url.get()) /* && !sendViaCloud */)

We should either undo this change entirely or uncomment the "&& !sendViaCloud" part.

::: mailnews/local/src/nsPop3Protocol.cpp
@@ -3935,5 @@
>        break;
>  
> -    case POP3_OBTAIN_PASSWORD_BEFORE_USERNAME:
> -      status = -1;
> -      break;

Did this file sneak in here? I thought I remembered seeing this change get checked into comm-central to fix some bug with the password prompt...

::: mailnews/mime/src/mimedrft.cpp
@@ +227,5 @@
>            attachment->SetContentType(curAttachment->m_realType.get());
>            attachment->SetMacType(curAttachment->m_xMacType.get());
>            attachment->SetMacCreator(curAttachment->m_xMacCreator.get());
>            attachment->SetSize(curAttachment->m_size);
> +          nsCString cloudPartInfo;

This variable appears to be unused.
Attachment #604567 - Flags: review?(squibblyflabbetydoo) → review+
Just a heads-up in case I get assigned any reviews: I should have some time on Monday afternoon to look at things, but I won't be free during Tuesday. So, if there's anything you guys would like me to look at, the sooner the better.

(Yay, we're almost done!)
(Reporter)

Comment 210

7 years ago
asking for sr for the interface changes
Attachment #604567 - Attachment is obsolete: true
Attachment #604968 - Flags: superreview?(mbanner)
Attachment #604968 - Flags: review+
Attachment #604567 - Flags: superreview?(mbanner)
(Reporter)

Comment 211

7 years ago
This breaks out the providers and provider interface for r/sr. I've included the oauth code as well.

This patch isn't meant to be standalone - to test it, you'd need to run the big files repo build. I can attach a cumulative diff as well in a bit, if that's easier.  Some of the makefile changes include more than that just the providers; ths is broken out strictly for sanity of review, not for landing. For landing, we'll land the cumulative patch once its all reviewed.
Attachment #604971 - Flags: superreview?(mbanner)
Attachment #604971 - Flags: review?(mconley)
(Reporter)

Comment 212

7 years ago
Attachment #604975 - Flags: review?(dbienvenu)
Attachment #604975 - Flags: feedback?(squibblyflabbetydoo)
(Reporter)

Comment 213

7 years ago
Comment on attachment 604975 [details] [diff] [review]
front end diffs for review

I can review most of the js changes (MsgComposeCommands.js, the account handler code, the link manager code, the unit tests, etc). Squib, I was hoping you could look at mconley's account settings code; mconley, I was hoping you could look at the remaining stuff, the other xul, dtd, and css code, etc. I know everything depends on everything else, and when in doubt it's better to have review overlap than gaps.

It would be great if Blake could look at some UI code, but he might be busy with IM.
Attachment #604975 - Flags: ui-review?(mconley)
Comment on attachment 604975 [details] [diff] [review]
front end diffs for review

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

::: mail/locales/en-US/chrome/messenger/cloudfile/addAccountDialog.properties
@@ +1,1 @@
> +# The following are used by the Add an Account dialog for

This file needs removing as it isn't used.

::: mail/locales/en-US/chrome/messenger/messengercompose/composeMsgs.properties
@@ +372,5 @@
> +# See: http://developer.mozilla.org/en/Localization_and_Plurals
> +# #1 number of big attached files
> +bigFileDescription=This is a large file. It might be better to use Filelink instead.;These are large files. It might be better to use Filelink instead.
> +bigFileShare.label=Link
> +bigFileShare.accesskey=h

Shouldn't this access key be a letter in the .label?

@@ +374,5 @@
> +bigFileDescription=This is a large file. It might be better to use Filelink instead.;These are large files. It might be better to use Filelink instead.
> +bigFileShare.label=Link
> +bigFileShare.accesskey=h
> +bigFileAttach.label=Ignore
> +bigFileAttach.accesskey=A

Shouldn't this access key be a letter in the .label?

@@ +413,5 @@
>  ## will be replaced with the name of the folder the message is being saved to.
>  errorSavingMsg=There was an error saving the message to %S. Retry?
> +
> +errorCloudFileAuth.title=Authentication Error
> +errorCloudFileAuth.message=Unable to authenticate to %1$S.

errorCloudFile* need localization notes as to what %1$S and %2$S actually are.

::: mail/locales/en-US/chrome/messenger/preferences/applications.properties
@@ +1,2 @@
> +# LOCALIZATION NOTE
> +# in dialog_removeAccount, %S will be replaced with the user-defined name

This should be:

LOCALIZATION NOTE (dialog_removeAccount): %S will be...
Comment on attachment 604971 [details] [diff] [review]
provider backend diffs

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

Here are the issues I found.  I'm going to take care of a bunch of them, so don't address these yet.  If I have questions, I'll ping you (bienvenu) in IRC.

::: mail/base/content/browserRequest.js
@@ +1,1 @@
> +/* ***** BEGIN LICENSE BLOCK *****

I'm not sure what to do in the case of code brought in from other projects, but I believe new code should be MPL2'd.

@@ +51,5 @@
> +        aIID.equals(Components.interfaces.nsISupports))
> +      return this;
> +    throw Components.results.NS_NOINTERFACE;
> +  },
> +  onStateChange: function(/*in nsIWebProgress*/ aWebProgress,

Newline before onStateChange.

::: mail/base/content/browserRequest.xul
@@ +1,2 @@
> +<?xml version="1.0"?>
> +<!-- ***** BEGIN LICENSE BLOCK *****

MPL2.

@@ +52,5 @@
> +
> +  <script type="application/javascript" src="chrome://messenger/content/browserRequest.js"/>
> +
> +  <keyset id="mainKeyset">
> +    <key id="key_close"   key="w" modifiers="accel" oncommand="cancelRequest()"/>

Remove the extra spacing before key for lines 56 and 57

::: mail/base/modules/http.jsm
@@ +1,1 @@
> +/* ***** BEGIN LICENSE BLOCK *****

MPL2

::: mail/base/modules/oauth.jsm
@@ +1,1 @@
> +/* ***** BEGIN LICENSE BLOCK *****

MPL2.

::: mail/components/cloudfile/Makefile.in
@@ +1,1 @@
> +# ***** BEGIN LICENSE BLOCK *****

MPL2.

::: mail/components/cloudfile/nsDropbox.js
@@ +1,1 @@
> +/* ***** BEGIN LICENSE BLOCK *****

MPL2.

@@ +93,5 @@
> +  _uploadingFile : null,
> +  _uploader : null,
> +  _lastErrorStatus : 0,
> +  _lastErrorText : "",
> +  _maxFileSize : 157286400, // according to Dropbox, this is a fixed limit.

This should be a const defined at the beginning of the file.

@@ +100,5 @@
> +  _uploads: [],
> +  _urlsForFiles : [],
> +  _uploadInfo : [], // upload info keyed on aFiles.
> +
> +  init: function nsDropbox_init(aAccountKey) {

Each of these functions needs to be separated by newlines, and, more importantly, needs documentation.

@@ +105,5 @@
> +    this._accountKey = aAccountKey;
> +    this._prefBranch = Services.prefs.getBranch("mail.cloud_files.accounts." + 
> +                                                aAccountKey + ".");
> +  },
> +  uploaderCallback : function(aThis, aRequestObserver, aStatus) {

This function, and several others in this file, need names, like nsDropbox_uploaderCallback.

Also, with proper function binding, I don't think we need to pass a "aThis" variable.

@@ +121,5 @@
> +      aThis._uploader = null;
> +  },
> +  uploadFile: function nsDropbox_uploadFile(aFile, aRecipients, aCallback) {
> +    if (Services.io.offline)
> +      return Ci.nsIMsgCloudFileProvider.offlineErr;

Newline after this if-block, for readability.

@@ +122,5 @@
> +  },
> +  uploadFile: function nsDropbox_uploadFile(aFile, aRecipients, aCallback) {
> +    if (Services.io.offline)
> +      return Ci.nsIMsgCloudFileProvider.offlineErr;
> +    this.log.info("uploading " + aFile.leafName + "\n");

I don't think log messages needs carriage returns at the end of them.

@@ +126,5 @@
> +    this.log.info("uploading " + aFile.leafName + "\n");
> +    this.requestObserver = aCallback;
> +    // if we're uploading a file, queue this request.
> +    if (this._uploadingFile && this._uploadingFile != aFile) {
> +      let uploader = new nsDropboxFileUploader(this, aFile, aRecipients, this.uploaderCallback, aCallback);

I believe this line is greater than 80 chars and should be broken up over two lines.

@@ +141,5 @@
> +    if (!this._userInfo)
> +      return this._getUserInfo(successCallback);
> +    successCallback();
> +  },
> +  finishUpload: function(aFile, aRecipients, aCallback) 

There seems to be some inconsistency in this file, with the curly-brace beting on the same line as the function def'n, and other times, on the next line.

We should pick one, and be consistent within the file.

@@ +145,5 @@
> +  finishUpload: function(aFile, aRecipients, aCallback) 
> +  {
> +    if (aFile.fileSize > this._maxFileSize)
> +      return aCallback.onStopRequest(null, null,
> +                              Ci.nsIMsgCloudFileProvider.uploadExceedsFileLimit);

The indentation of this line seems arbitrary.

We should do one of the following:

1)  Indent the Ci.nIMsgCloudFileProvider lines by two spaces.
2)  Alias Ci.nsIMsgCloudFileProvider.upload* to some shorter local variables, and use those instead

@@ +155,5 @@
> +      this._uploader = new nsDropboxFileUploader(this, aFile, aRecipients, this.uploaderCallback, aCallback);
> +      this._uploads.unshift(this._uploader);
> +    }
> +
> +    this._uploadingFile = aFile;

Could we ever get into a case where aFile does not map to the file that this._uploader belongs to?

@@ +171,5 @@
> +        }
> +    }
> +  },
> +  _getUserInfo: function nsDropbox_userInfo(successCallback, failureCallback) {
> +    var me = this;

With the changes I mention below, we should no longer need the "me" variable.

@@ +174,5 @@
> +  _getUserInfo: function nsDropbox_userInfo(successCallback, failureCallback) {
> +    var me = this;
> +
> +    if (successCallback)
> +      me._successCallback = successCallback;

We don't need to stash successCallback into me (this) - the callback functions defined below can just refer to successCallback and failureCallback - they'll have access to them via the magic / power of closures.

See nsYouSendIt's implementation of this same function for an example.

@@ +182,5 @@
> +                                         me._loggedIn ? Cr.NS_OK : Ci.nsIMsgCloudFileProvider.authErr);
> +      };
> +
> +    if (failureCallback)
> +      me._failureCallback = failureCallback;

Same as above, re: me._successCallback.

@@ +189,5 @@
> +        me.requestObserver.onStopRequest(null, null, Ci.nsIMsgCloudFileProvider.authErr);
> +      };
> +
> +    this._connection.signAndSend(
> +      gServerUrl + "account/info", "", "GET", [],

account/info should be a string const defined at the beginning of the file.

@@ +197,5 @@
> +        let quota = me._userInfo.quota_info;
> +        me._availableStorage = quota.quota;
> +        me._fileSpaceUsed = quota.normal + quota.shared;
> +        me.log.info("avail storage = " + me._availableStorage);
> +        me._successCallback();

Here's where you can just use successCallback();.

@@ +198,5 @@
> +        me._availableStorage = quota.quota;
> +        me._fileSpaceUsed = quota.normal + quota.shared;
> +        me.log.info("avail storage = " + me._availableStorage);
> +        me._successCallback();
> +      },

After this curly brace, put:

.bind(this),

And that will allow us to use "this" within this function and refer to this instance of nsDropBox, as opposed to using "me".

@@ +209,5 @@
> +          me.log.info("got bad token");
> +          me._loggedIn = false;
> +          me.setCachedAuthToken("");
> +          me.setCachedAuthSecret("");
> +          me._successCallback();

Just use successCallback(); here.

@@ +215,5 @@
> +        }
> +        me.log.error("user info failed, status = " + aRequest.status);
> +        me.log.error("response text = " + aResponseText);
> +        me.log.error("exception = " + aException);
> +        me._failureCallback();

Just use failureCallback(); here.

@@ +216,5 @@
> +        me.log.error("user info failed, status = " + aRequest.status);
> +        me.log.error("response text = " + aResponseText);
> +        me.log.error("exception = " + aException);
> +        me._failureCallback();
> +      }, this);

Like above, put .bind(this) at the end of this curly brace, and replace "me" with "this" within the function.

@@ +219,5 @@
> +        me._failureCallback();
> +      }, this);
> +  },
> +  _logonAndGetUserInfo: function nsDropbox_logonAndGetUserInfo(aSuccessCallback, aFailureCallback, aWithUI) {
> +    let me = this;

With the changes suggested below, we won't need a "me" variable.

@@ +223,5 @@
> +    let me = this;
> +    if (!aFailureCallback)
> +      aFailureCallback = function () {
> +        me.requestObserver.onStopRequest(null, null, Ci.nsIMsgCloudFileProvider.authErr);
> +      };

Put .bind(this); at the end of the function, and replace "me" with "this" within the function def'n.

@@ +226,5 @@
> +        me.requestObserver.onStopRequest(null, null, Ci.nsIMsgCloudFileProvider.authErr);
> +      };
> +    return this.logon(function() {
> +      me._getUserInfo(aSuccessCallback, aFailureCallback);
> +    }, aFailureCallback, aWithUI);

Same as above, with the bind, and replacing "me".

@@ +229,5 @@
> +      me._getUserInfo(aSuccessCallback, aFailureCallback);
> +    }, aFailureCallback, aWithUI);
> +  },
> +
> +  urlForFile: function nsDropbox_urlForFile(aFile) {return this._urlsForFiles[aFile];},

I think I'd prefer the split up over several lines, for readability.

@@ +244,5 @@
> +    return this._userInfo;
> +  },
> +
> +
> +  createNewAccount: function nsDropbox_createNewAccount(aEmailAddress, aPassword, aFirstName, aLastName) {

Should be split out among several lines.

@@ +248,5 @@
> +  createNewAccount: function nsDropbox_createNewAccount(aEmailAddress, aPassword, aFirstName, aLastName) {
> +    return Cr.NS_ERROR_NOT_IMPLEMENTED;},
> +
> +  createExistingAccount: function nsDropbox_createExistingAccount(aRequestObserver) {
> +     // XXX: replace this with a better function

Do we have a better function in mind?

@@ +266,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 "";},

We can can just use

get createNewAccountUrl() "",

@@ +290,5 @@
> +    let uploadInfo = this._uploadInfo[aFile.path];
> +    if (!uploadInfo)
> +      throw Cr.NS_ERROR_FAILURE;
> +
> +    var me = this;

With proper function binding, we can drop this "me", like elsewhere in this file.

@@ +291,5 @@
> +    if (!uploadInfo)
> +      throw Cr.NS_ERROR_FAILURE;
> +
> +    var me = this;
> +    me.requestObserver = aCallback;

If we're just assigning this to requestObserver to refer to it from within the functions passed to signAndSend, we can just use "aCallback" instead.

@@ +293,5 @@
> +
> +    var me = this;
> +    me.requestObserver = aCallback;
> +    let path = this.wwwFormUrlEncoded(uploadInfo.path);
> +    let url = gServerUrl + "fileops/delete/" + "?root=sandbox" + "&path=" +

These magic strings should be moved to consts.

@@ +300,5 @@
> +    let oauthParams =
> +      [["root", "sandbox"], ["path", path]];
> +    this._connection.signAndSend(url, "", "POST", null,
> +      function(aResponseText, aRequest) {
> +        dump("success deleting file; response = " + aResponseText + "\n");

We should use this.log.info instead of dump.

@@ +301,5 @@
> +      [["root", "sandbox"], ["path", path]];
> +    this._connection.signAndSend(url, "", "POST", null,
> +      function(aResponseText, aRequest) {
> +        dump("success deleting file; response = " + aResponseText + "\n");
> +        me.requestObserver.onStopRequest(null, null, Cr.NS_OK);

We can just use aCallback instead of me.requestObserver.

@@ +302,5 @@
> +    this._connection.signAndSend(url, "", "POST", null,
> +      function(aResponseText, aRequest) {
> +        dump("success deleting file; response = " + aResponseText + "\n");
> +        me.requestObserver.onStopRequest(null, null, Cr.NS_OK);
> +      },

We should bind this function using .bind(this)

@@ +304,5 @@
> +        dump("success deleting file; response = " + aResponseText + "\n");
> +        me.requestObserver.onStopRequest(null, null, Cr.NS_OK);
> +      },
> +      function(aException, aResponseText, aRequest) {
> +        dump("failed deleting file; response = " + aResponseText + "\n");

Same as above, re binding, and using aCallback, and logging.

@@ +331,5 @@
> +    if (!aWithUI && (!authToken.length || !authSecret.length)) {
> +      failureCallback();
> +      return;
> +    }
> +    var me = this;

Cut the me.

@@ +332,5 @@
> +      failureCallback();
> +      return;
> +    }
> +    var me = this;
> +    me._successCallback = successCallback;

No need to do this - just use successCallback / failureCallback in the functions.

@@ +336,5 @@
> +    me._successCallback = successCallback;
> +    me._failureCallback = failureCallback;
> +
> +    this._connection = new OAuth(this.displayName, gServerUrl, gAuthUrl, authToken, authSecret,
> +                                 "7xkhuze09iqkghm", "3i5kwjkt74rkkjc");

These magic token strings should be consts.

@@ +371,5 @@
> +    return authSecret;
> +  },
> +
> +  setCachedAuthToken: function(aAuthToken) {
> +    dump("aAuthToken = " + aAuthToken);

Should log instead of dump.

@@ +382,5 @@
> +    cloudFileAccounts.setSecretValue(this.accountKey,
> +                                     kAuthSecretRealm,
> +                                     aAuthSecret);
> +  },
> +  wwwFormUrlEncoded : function(str) {

Maybe instead of making this function part of the nsDropBox object, we can just have it be a helper function defined in this file, seeing as how it's used by both nsDropBox and nsDropboxFileUploader.

@@ +389,5 @@
> +  }
> +};
> +
> +function nsDropboxFileUploader(aDropbox, aFile, aRecipients, aCallback, aRequestObserver) {
> +  dump("new nsDropboxFileUploader file = " + aFile.leafName + "\n");

Should log instead of dump.

@@ +403,5 @@
> +  file : null,
> +  callback : null,
> +  recipients : null,
> +  request : null,
> +  uploadFile : function() {

The functions in this object should be separated with newlines, and need documentation.

@@ +404,5 @@
> +  callback : null,
> +  recipients : null,
> +  request : null,
> +  uploadFile : function() {
> +    var thisDropbox = this.dropbox;

Like the "me" comments above, thisDropbox and thisUploader isn't really necessary with proper function binding.

@@ +447,5 @@
> +      this.request = null;
> +    }
> +  },
> +  _getShareUrl : function(aFile, aCallback) {
> +    let url = gServerUrl + "shares/sandbox/" + this.dropbox.wwwFormUrlEncoded(aFile.leafName);

Magic string.

::: mail/components/cloudfile/nsIMsgCloudFileProvider.idl
@@ +1,1 @@
> +/* -*- Mode: I*DL; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

MPL2.

@@ +48,5 @@
> +  // Unlike the type, the displayName is purely for rendering the name of
> +  // a storage service provider.
> +  readonly attribute ACString displayName;
> +  // A link to the homepage of the service, if applicable.
> +  readonly attribute AString serviceURL;

Why do we use ACString in some cases, and AString in other cases?

@@ +63,5 @@
> +
> +  /**
> +   * upload the file to the cloud provider. The callback's OnStopRequest
> +   * method will be called when finished, with success or an error code.
> +

Nit: missing an asterisk

@@ +89,5 @@
> +   *                page. If false, we won't, and will fail if auth is required.
> +   * @param aCallback callback when finished.
> +   */
> +  void refreshUserInfo(in boolean aWithUI, in nsIRequestObserver aCallback);
> +  /**

Nit: newline needed

@@ +112,5 @@
> +   * otherwise, an error occurred which is * probably specific to the provider.
> +   * If the request fails completely, onStopRequest will get called with
> +   * Components.results.NS_ERROR_FAILURE
> +   */
> +  void createNewAccount(in ACString aEmailAddress, in ACString aPassword,

Neither of our instances make use of this function.  In both cases, we simply forward the user to the service's sign-up page.  Do we still need this function?  Where in the UI is this function called?

::: mail/components/cloudfile/nsYouSendIt.js
@@ +1,1 @@
> +/* ***** BEGIN LICENSE BLOCK *****

MPL2.

@@ +48,5 @@
> +Cu.import("resource://gre/modules/Services.jsm");
> +Cu.import("resource:///modules/gloda/log4moz.js");
> +Cu.import("resource:///modules/cloudFileAccounts.js");
> +
> +// Production url: var gServerUrl = "https://dpi.yousendit.com";

When do we switch to this url?  Should this be a pref instead of a hard-coded string?

@@ +108,5 @@
> +    this._userName = this._prefBranch.getCharPref("username");
> +    this._loggedIn = this._cachedAuthToken != "";
> +  },
> +
> +  uploaderCallback : function(aRequestObserver, aStatus) {

Some of these functions need names, like nsYouSendIt_uploaderCallback.

@@ +145,5 @@
> +    this._uploadingFile = aFile;
> +    this._recipients = aRecipients;
> +    this._urlListener = aCallback;
> +
> +    var me = this;

It doesn't look like this "me" is being used.  Let's remove it.

@@ +223,5 @@
> +  },
> +
> +  _getUserInfo: function nsYouSendIt_userInfo(successCallback, failureCallback) {
> +    this.log.info("getting user info");
> +    let resource = "/dpi/v1/user";

This /dpi/* string should probably be a const.

@@ +233,5 @@
> +
> +    req.onload = function() {
> +      if (req.status >= 200 && req.status < 400) {
> +        this.log.info("request status = " + req.status +
> +                    " response = " + req.responseText + "\n");

Nit - misaligned, and no need for carriage return.

@@ +282,5 @@
> +  urlForFile: function nsYouSendIt_urlForFile(aFile) {
> +    return this._urlsForFiles[aFile];
> +  },
> +
> +  refreshUserInfo: function nsYouSendIt_refreshUserInfo(aWithUI, aListener) {

I think this function needs some internal documentation, to help figure out what's going on.

@@ +367,5 @@
> +    req.send();
> +  },
> +
> +  createExistingAccount: function nsYouSendIt_createExistingAccount(aRequestObserver) {
> +     // XXX: replace this with a better function

Do we have a better function in mind?

@@ +386,5 @@
> +      return "http://www.yousendit.com";
> +    return "";
> +  },
> +
> +

Remove the extra newline.

@@ +419,5 @@
> +    }
> +
> +    let req = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"]
> +                .createInstance(Ci.nsIXMLHttpRequest);
> +    let resource = "/dpi/v1/item/" + uploadInfo.itemId;

/dpi/v1/item and /dpi/v1/user need to be defined as constants within this file, and all instances need to be replaced by those constants.

@@ +498,5 @@
> +    let passwordURI = gServerUrl;
> +    let loginManager = Cc["@mozilla.org/login-manager;1"].getService(Ci.nsILoginManager);
> +    let logins = loginManager.findLogins({}, passwordURI, null, passwordURI);
> +    for each (let loginInfo in logins)
> +    {

We need some internal consistency with our opening braces - whether or not to put them on the same line as the function def'n or on the next line.

@@ +630,5 @@
> +
> +    return this._prepareToSend(onSuccess, onFailure);
> +  },
> +
> +  _prepareToSend: function (successCallback, failureCallback) {

Nit: no space after "function"

@@ +631,5 @@
> +    return this._prepareToSend(onSuccess, onFailure);
> +  },
> +
> +  _prepareToSend: function (successCallback, failureCallback) {
> +    let resource = "/dpi/v1/item/send";

Should be a const

@@ +639,5 @@
> +
> +    let req = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"]
> +                .createInstance(Ci.nsIXMLHttpRequest);
> +
> +    req.open("POST", gServerUrl + "/dpi/v1/item/send" + args, true);

Same, re: const.

@@ +678,5 @@
> +    let req = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"]
> +                .createInstance(Ci.nsIXMLHttpRequest);
> +
> +    let curDate = Date.now().toString();
> +    dump("upload url = " + this._urlInfo.uploadUrl);

Log instead of dump.
(Reporter)

Comment 216

7 years ago
converting a cloud attachment to a regular attachment of a message loaded from the drafts folder fails:

Timestamp: 3/12/2012 10:17:22 AM
Error: An error occurred executing the cmd_convertAttachment command: [Exception... "'[JavaScript Error: "this._connection is null" {file: "file:///C:/builds/bigfiles/objdir-tb/mozilla/dist/bin/components/nsDropbox.js" line: 302}]' when calling method: [nsIMsgCloudFileProvider::deleteFile]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: chrome://messenger/content/messengercompose/MsgComposeCommands.js :: convertListItemsToRegularAttachment :: line 1288"  data: yes]
Source File: chrome://global/content/globalOverlay.js
Line: 100

I believe the draft is doing the right thing w.r.t. the original url, so I think the issue might have to do with the deleteFile implementation, or the caller. I'll have to debug this, which might delay the rest of the review.
(Reporter)

Comment 217

7 years ago
ah, yes, deleteFile assumes the file has been uploaded in the current compose window, which isn't true in the loaded draft case. We don't save the info required to delete the draft at a later point (essentially, we'd need to save some provider-specific info in the draft for each attachment). In the near term, we could allow the conversion to happen to a regular attachment, but leave the cloud file on the provider, or we could disable this command when editing a draft.
(Reporter)

Comment 218

7 years ago
converting a cloud file attachment to a regular attachment doesn't remove the html annotation, but deleting the cloud file attachment does. I'll see if I can fix this.
(Reporter)

Comment 219

7 years ago
looking at the tests, three mozmill tests are failing: (I'll investigate)

SUMMARY-UNEXPECTED-FAIL | c:\builds\bigfiles\mail\test\mozmill\cloudfile\test-cl
oudfile-manager.js | test-cloudfile-manager.js::test_load_accounts_and_properly_
order
  EXCEPTION: Should be displaying 4 accounts: '4' != '0'.
    at: test-folder-display-helpers.js line 2855
       assert_true(false,"Should be displaying 4 accounts: '4' != '0'.") test-fo
lder-display-helpers.js 2855
       assert_equals(4,0,"Should be displaying 4 accounts") test-folder-display-
helpers.js 2842
            test-cloudfile-manager.js 99
            test-cloudfile-manager.js 85
       waitForPaneLoad([object Object]) test-pref-window-helpers.js 93
            frame.js 552
       WindowWatcher_notify([object XPCWrappedNative_NoHelper]) test-window-help
ers.js 365
SUMMARY-PASS | test-cloudfile-manager.js::teardownModule
SUMMARY-PASS | test-cloudfile-notifications.js::setupModule
SUMMARY-PASS | test-cloudfile-notifications.js::test_no_notification_for_small_f
ile
SUMMARY-UNEXPECTED-FAIL | c:\builds\bigfiles\mail\test\mozmill\cloudfile\test-cl
oudfile-notifications.js | test-cloudfile-notifications.js::test_notification_fo
r_big_files
  EXCEPTION: Expected the notification to be shown: 'false' != 'true'.
    at: test-folder-display-helpers.js line 2855
       assert_true(false,"Expected the notification to be shown: 'false' != 'tru
e'.") test-folder-display-helpers.js 2855
       assert_equals(false,true,"Expected the notification to be shown") test-fo
lder-display-helpers.js 2842
       assert_cloudfile_notification_displayed([object Object],true) test-cloudf
ile-notifications.js 52
       test_notification_for_big_files() test-cloudfile-notifications.js 73
            frame.js 557
            frame.js 626
            frame.js 669
            frame.js 497
            frame.js 675
            server.js 179
            server.js 183
SUMMARY-UNEXPECTED-FAIL | c:\builds\bigfiles\mail\test\mozmill\cloudfile\test-cl
oudfile-notifications.js | test-cloudfile-notifications.js::test_graduate_to_not
ification
  EXCEPTION: Expected the notification to be shown: 'false' != 'true'.
    at: test-folder-display-helpers.js line 2855
       assert_true(false,"Expected the notification to be shown: 'false' != 'tru
e'.") test-folder-display-helpers.js 2855
       assert_equals(false,true,"Expected the notification to be shown") test-fo
lder-display-helpers.js 2842
       assert_cloudfile_notification_displayed([object Object],true) test-cloudf
ile-notifications.js 52
       test_graduate_to_notification() test-cloudfile-notifications.js 94
            frame.js 557
            frame.js 626
            frame.js 669
            frame.js 497
            frame.js 675
            server.js 179
            server.js 183
Comment on attachment 604975 [details] [diff] [review]
front end diffs for review

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

I have some comments below, but overall this looks good. One thing I did notice is that the "Outgoing Attachments" tab in the prefs dialog is too big for the dialog under Linux, so the buttons are off-screen.

::: mail/components/cloudfile/content/YouSendIt/settings.js
@@ +1,1 @@
> +function extraArgs() {

Add a license block to the beginning of this file.

::: mail/components/cloudfile/content/YouSendIt/settings.xhtml
@@ +1,1 @@
> +<?xml version="1.0" encoding="UTF-8"?>

Add a license block to the beginning of this file too.

::: mail/locales/en-US/chrome/messenger/messengercompose/composeMsgs.properties
@@ +429,5 @@
> +errorCloudFileUpgrade.label=Upgrade
> +
> +## LOCALIZATION NOTE(cloudAttachmentListHeader): "Filelink" is the name we're
> +## using for the cloud attachment feature.
> +cloudAttachmentListHeader=Filelink Attachments:

We should probably remove this (I believe it's the plain-text header) and use cloudAttachmentCountHeader in plain-text as well.

::: mail/test/mozmill/cloudfile/test-cloudfile-attachment-urls.js
@@ +63,5 @@
> +
> +  let aFiles = ['./data/testFile1', './data/testFile2'];
> +  gMockFilePicker.returnFiles = collectFiles(aFiles);
> +  cw.window.attachToCloud(provider);
> +  // TODO - this is just an example test - these need to get fleshed out.

Should we just remove this test from the list since it doesn't do much of anything yet?

::: mail/themes/gnomestripe/mail/compose/messengercompose.css
@@ +723,5 @@
>    position: relative;
>    z-index: 10;
>  }
> +
> +/* XXX: remove this probably? */

We could either remove this comment, or remove the following CSS, depending on whether we want to show the cloud provider icons in the Attach menu on Linux.
Attachment #604975 - Flags: feedback?(squibblyflabbetydoo) → feedback+
(Reporter)

Comment 221

7 years ago
OK, I've fixed converting an attachment to regular from cloud to clean up the html in the message body, and addressed the issues that I found in the front end js. I still have to look at the test code and failures.
I've addressed and committed most of the issues I found in nsDropbox.js, nsYouSendIt.js and nsIMsgCloudFileProvider.idl.  The things I didn't address, couldn't address, or didn't know the answers to, are listed below.


(In reply to Mike Conley (:mconley) from comment #215)
> Comment on attachment 604971 [details] [diff] [review]
> provider backend diffs
> 
> Review of attachment 604971 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Here are the issues I found.  I'm going to take care of a bunch of them, so
> don't address these yet.  If I have questions, I'll ping you (bienvenu) in
> IRC.
> 
> ::: mail/base/content/browserRequest.js
> @@ +1,1 @@
> > +/* ***** BEGIN LICENSE BLOCK *****
> 
> I'm not sure what to do in the case of code brought in from other projects,
> but I believe new code should be MPL2'd.
> 
> @@ +51,5 @@
> > +        aIID.equals(Components.interfaces.nsISupports))
> > +      return this;
> > +    throw Components.results.NS_NOINTERFACE;
> > +  },
> > +  onStateChange: function(/*in nsIWebProgress*/ aWebProgress,
> 
> Newline before onStateChange.
> 
> ::: mail/base/content/browserRequest.xul
> @@ +1,2 @@
> > +<?xml version="1.0"?>
> > +<!-- ***** BEGIN LICENSE BLOCK *****
> 
> MPL2.
> 
> @@ +52,5 @@
> > +
> > +  <script type="application/javascript" src="chrome://messenger/content/browserRequest.js"/>
> > +
> > +  <keyset id="mainKeyset">
> > +    <key id="key_close"   key="w" modifiers="accel" oncommand="cancelRequest()"/>
> 
> Remove the extra spacing before key for lines 56 and 57
> 
> ::: mail/base/modules/http.jsm
> @@ +1,1 @@
> > +/* ***** BEGIN LICENSE BLOCK *****
> 
> MPL2
> 
> ::: mail/base/modules/oauth.jsm
> @@ +1,1 @@
> > +/* ***** BEGIN LICENSE BLOCK *****
> 
> MPL2.
> 
> ::: mail/components/cloudfile/Makefile.in
> @@ +1,1 @@
> > +# ***** BEGIN LICENSE BLOCK *****
> 
> MPL2.
> 
> ::: mail/components/cloudfile/nsDropbox.js

> @@ +155,5 @@
> > +      this._uploader = new nsDropboxFileUploader(this, aFile, aRecipients, this.uploaderCallback, aCallback);
> > +      this._uploads.unshift(this._uploader);
> > +    }
> > +
> > +    this._uploadingFile = aFile;
> 
> Could we ever get into a case where aFile does not map to the file that
> this._uploader belongs to?
> 

> @@ +248,5 @@
> > +  createNewAccount: function nsDropbox_createNewAccount(aEmailAddress, aPassword, aFirstName, aLastName) {
> > +    return Cr.NS_ERROR_NOT_IMPLEMENTED;},
> > +
> > +  createExistingAccount: function nsDropbox_createExistingAccount(aRequestObserver) {
> > +     // XXX: replace this with a better function
> 
> Do we have a better function in mind?


> ::: mail/components/cloudfile/nsIMsgCloudFileProvider.idl

> @@ +112,5 @@
> > +   * otherwise, an error occurred which is * probably specific to the provider.
> > +   * If the request fails completely, onStopRequest will get called with
> > +   * Components.results.NS_ERROR_FAILURE
> > +   */
> > +  void createNewAccount(in ACString aEmailAddress, in ACString aPassword,
> 
> Neither of our instances make use of this function.  In both cases, we
> simply forward the user to the service's sign-up page.  Do we still need
> this function?  Where in the UI is this function called?
> 
> ::: mail/components/cloudfile/nsYouSendIt.js

> @@ +48,5 @@
> > +Cu.import("resource://gre/modules/Services.jsm");
> > +Cu.import("resource:///modules/gloda/log4moz.js");
> > +Cu.import("resource:///modules/cloudFileAccounts.js");
> > +
> > +// Production url: var gServerUrl = "https://dpi.yousendit.com";
> 
> When do we switch to this url?  Should this be a pref instead of a
> hard-coded string?
> 
> 
> @@ +367,5 @@
> > +    req.send();
> > +  },
> > +
> > +  createExistingAccount: function nsYouSendIt_createExistingAccount(aRequestObserver) {
> > +     // XXX: replace this with a better function
> 
> Do we have a better function in mind?
>
(Reporter)

Comment 223

7 years ago
I don't think this is my comment so I can't say:

> > +  createExistingAccount: function nsDropbox_createExistingAccount(aRequestObserver) {
> > +     // XXX: replace this with a better function
> 
> Do we have a better function in mind?

> When do we switch to this url?  Should this be a pref instead of a
> hard-coded string?

When we get switched to production status by yousendit. I can ask when they think that should be. I don't think it should be a pref, no, since it has to match the code, I bet. I.e., when newer versions of the api are developed, they'll need to fix the code.
Thanks squib - fixes pushed.

(In reply to Jim Porter (:squib) from comment #220)
> Comment on attachment 604975 [details] [diff] [review]
> front end diffs for review
> 
> Review of attachment 604975 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I have some comments below, but overall this looks good. One thing I did
> notice is that the "Outgoing Attachments" tab in the prefs dialog is too big
> for the dialog under Linux, so the buttons are off-screen.
> 
> ::: mail/components/cloudfile/content/YouSendIt/settings.js
> @@ +1,1 @@
> > +function extraArgs() {
> 
> Add a license block to the beginning of this file.
> 

Done.

> ::: mail/components/cloudfile/content/YouSendIt/settings.xhtml
> @@ +1,1 @@
> > +<?xml version="1.0" encoding="UTF-8"?>
> 
> Add a license block to the beginning of this file too.
> 

Done.

> :::
> mail/locales/en-US/chrome/messenger/messengercompose/composeMsgs.properties
> @@ +429,5 @@
> > +errorCloudFileUpgrade.label=Upgrade
> > +
> > +## LOCALIZATION NOTE(cloudAttachmentListHeader): "Filelink" is the name we're
> > +## using for the cloud attachment feature.
> > +cloudAttachmentListHeader=Filelink Attachments:
> 
> We should probably remove this (I believe it's the plain-text header) and
> use cloudAttachmentCountHeader in plain-text as well.
> 

Done.

> ::: mail/test/mozmill/cloudfile/test-cloudfile-attachment-urls.js
> @@ +63,5 @@
> > +
> > +  let aFiles = ['./data/testFile1', './data/testFile2'];
> > +  gMockFilePicker.returnFiles = collectFiles(aFiles);
> > +  cw.window.attachToCloud(provider);
> > +  // TODO - this is just an example test - these need to get fleshed out.
> 
> Should we just remove this test from the list since it doesn't do much of
> anything yet?
> 

Sure.  Done.

> ::: mail/themes/gnomestripe/mail/compose/messengercompose.css
> @@ +723,5 @@
> >    position: relative;
> >    z-index: 10;
> >  }
> > +
> > +/* XXX: remove this probably? */
> 
> We could either remove this comment, or remove the following CSS, depending
> on whether we want to show the cloud provider icons in the Attach menu on
> Linux.

Done.
(Reporter)

Comment 225

7 years ago
(In reply to Mike Conley (:mconley) from comment #222)
> > > +  void createNewAccount(in ACString aEmailAddress, in ACString aPassword,
> > 
> > Neither of our instances make use of this function.  In both cases, we
> > simply forward the user to the service's sign-up page.  Do we still need
> > this function?  Where in the UI is this function called?

We're going to have other providers, and they may have an api for this. Yousendit has an API, in fact, but we don't have the privileges for it. So I'd like to leave this in for now.
Attachment #604968 - Flags: superreview?(mbanner) → superreview+
Thanks Mark, fixes pushed to our working branch.

(In reply to Mark Banner (:standard8) from comment #214)
> Comment on attachment 604975 [details] [diff] [review]
> front end diffs for review
> 
> Review of attachment 604975 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mail/locales/en-US/chrome/messenger/cloudfile/addAccountDialog.properties
> @@ +1,1 @@
> > +# The following are used by the Add an Account dialog for
> 
> This file needs removing as it isn't used.
> 

Done.

> :::
> mail/locales/en-US/chrome/messenger/messengercompose/composeMsgs.properties
> @@ +372,5 @@
> > +# See: http://developer.mozilla.org/en/Localization_and_Plurals
> > +# #1 number of big attached files
> > +bigFileDescription=This is a large file. It might be better to use Filelink instead.;These are large files. It might be better to use Filelink instead.
> > +bigFileShare.label=Link
> > +bigFileShare.accesskey=h
> 
> Shouldn't this access key be a letter in the .label?

Done.

> 
> @@ +374,5 @@
> > +bigFileDescription=This is a large file. It might be better to use Filelink instead.;These are large files. It might be better to use Filelink instead.
> > +bigFileShare.label=Link
> > +bigFileShare.accesskey=h
> > +bigFileAttach.label=Ignore
> > +bigFileAttach.accesskey=A
> 
> Shouldn't this access key be a letter in the .label?

Done

> 
> @@ +413,5 @@
> >  ## will be replaced with the name of the folder the message is being saved to.
> >  errorSavingMsg=There was an error saving the message to %S. Retry?
> > +
> > +errorCloudFileAuth.title=Authentication Error
> > +errorCloudFileAuth.message=Unable to authenticate to %1$S.
> 
> errorCloudFile* need localization notes as to what %1$S and %2$S actually
> are.

Done.

> 
> ::: mail/locales/en-US/chrome/messenger/preferences/applications.properties
> @@ +1,2 @@
> > +# LOCALIZATION NOTE
> > +# in dialog_removeAccount, %S will be replaced with the user-defined name
> 
> This should be:
> 
> LOCALIZATION NOTE (dialog_removeAccount): %S will be...

Done.
Comment on attachment 604971 [details] [diff] [review]
provider backend diffs

Assuming all of the issues I found were fixed, r=me.
Attachment #604971 - Flags: review?(mconley) → review+
(Reporter)

Comment 228

7 years ago
Comment on attachment 604975 [details] [diff] [review]
front end diffs for review

I've landed fixes for all the issues I came across in the reviews of the js code and unit tests
Attachment #604975 - Flags: review?(dbienvenu) → review+
Attachment #604975 - Flags: ui-review?(mconley) → ui-review+
Comment on attachment 604971 [details] [diff] [review]
provider backend diffs

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

sr=me for the revised interface I saw at http://hg.mozilla.org/users/bienvenu_nventure.com/big-files/file/3e017a2cd232/mail/components/cloudfile/nsIMsgCloudFileProvider.idl
Attachment #604971 - Flags: superreview?(mbanner) → superreview+
(Reporter)

Comment 231

7 years ago
I messed up transferring the binary files between the repos - these pushes should rectify that:
http://hg.mozilla.org/comm-central/rev/cccf1c19d8a2
http://hg.mozilla.org/comm-central/rev/0c0248775ff8
http://hg.mozilla.org/comm-central/rev/ac761966deb9
Alias: BigFiles
OS: Windows 7 → All
QA Contact: dbienvenu → message-compose
Hardware: x86_64 → All

Updated

7 years ago
Depends on: 735240
Depends on: 735219

Updated

7 years ago
Depends on: 736789

Comment 233

7 years ago
I'm trying to integrate Spideroak's storage service into BigFiles, but have run into issues with the way that OAuth is handled. Currently, oauth.jsm seems to have a lot of magic strings in place to handle URI definition, signature method, etc. Is there any way that the handler could be re-written or extended in a more general way to handle other providers?

Our API docs are presently (temporarily) held at https://dhain.dev.spideroak.com:888/apis/bigfiles/docs/api.html . This is a developer sandbox, so expect SSL errors. I've also attached an early patch so you can see where the process breaks down.

https://spideroak.com/browse/share/so_tom/2FZ2ihz0WdR6agFUkVJ8p2DQjELOnfmDqaIEukbE

Thanks,
Tom ThompsonSoftware Engineer - Spideroak
(Reporter)

Comment 234

7 years ago
(In reply to Tom Thompson from comment #233)
> I'm trying to integrate Spideroak's storage service into BigFiles, but have
> run into issues with the way that OAuth is handled. Currently, oauth.jsm
> seems to have a lot of magic strings in place to handle URI definition,
> signature method, etc.

You can propose a patch to add more params to the OAuth constructor, or add an other method to override some of the default behavior, e.g.,

overrideOAuthParams : function(params)

and implement it such that the oauth instance has a params member that's initialized with the defaults and your method can change the defaults. That way the other callers don't have to be changed.

Was there something other than the signature method that we've hard-coded? Or are there extra things you need to pass in?

Comment 235

7 years ago
The URIs necessary for grabbing tokens are also hard-coded and not defined by the OAuth spec, as such almost all providers have a unique set of URIs. There may be one or two other things, but the three URIs and the signature method are all I'm sure of off-hand.
(Reporter)

Comment 236

7 years ago
by URI, do you mean things like oauth/request_token?

Comment 237

7 years ago
yes

Updated

7 years ago
Blocks: 737347
(Reporter)

Comment 238

7 years ago
(In reply to Tom Thompson from comment #237)
> yes

It seems to me you could unblock yourself by doing your own oauth code; or you could propose a patch to the core code to customize the various hard-coded things.
No longer blocks: 737349
Depends on: 737349
(Reporter)

Updated

7 years ago
No longer depends on: 736789

Comment 240

7 years ago
Here is the patch for oauth to be compatible with current providers as well as SpiderOak. Please let me know if there are any issues with it, and I will address them promptly.

I will send in the patch for SpiderOak in bigfiles after we move it to a production environment.

There are also a few issues I would like to address before submitting for approval, but could use some help on.
What is the process for requisitioning an appropriate UID for cloudFileComponents.manifest?
I've noticed that Thunderbird will remember other providers after restarting but not my new provider. Where is this controlled?

Regards,
Tom Thompson
Software Engineer - SpiderOak
(Reporter)

Comment 241

7 years ago
Hi Tom,
  the command line program uuidgen will generate a uuid for you.

  The list of providers in the attach menu is really the list of file link accounts, not providers per se - are you creating an account? In particular, you need to create the following prefs:

mail.cloud_files.accounts.<accountnum>.type
mail.cloud_files.accounts.<accountnum>.displayName
(Reporter)

Comment 242

7 years ago
Comment on attachment 611641 [details] [diff] [review]
oauth compatibility enhancements

thx, I'll try this out later today.
(Reporter)

Updated

7 years ago
Attachment #611641 - Flags: review?(dbienvenu)

Comment 243

7 years ago
Here is the patch to integrate Spideroak into BigFiles/FileLink. It requires the oauth patch I provided earlier. Let me know if there are any issues with this patch, and I will address them promptly.

I'll see about getting a real app key/secret sometime today. Let me know if there is anything else we need to do to move this forward. We are excited to be a part of this initiative.

Regards,
Tom Thompson - Software Engineer, Spideroak

Comment 244

7 years ago
Posted image small spideroak icon for use in UI (obsolete) —
I believe this needs to be added for my recent patch to:
mail/themes/gnomestripe/mail/icons
mail/themes/pinstripe/mail/icons
mail/themes/qute/mail/icons
(Reporter)

Comment 245

7 years ago
(In reply to Tom Thompson from comment #243)
> Created attachment 613294 [details] [diff] [review]
> spideroak support for BigFiles(FileLink?)
> 
> Here is the patch to integrate Spideroak into BigFiles/FileLink. It requires
> the oauth patch I provided earlier. Let me know if there are any issues with
> this patch, and I will address them promptly.
> 
> I'll see about getting a real app key/secret sometime today. Let me know if
> there is anything else we need to do to move this forward. We are excited to
> be a part of this initiative.
> 
> Regards,
> Tom Thompson - Software Engineer, Spideroak

Cool, glad you got it working.

We found several issues in the cancel process, which means the cancel code you copied from the other providers is not quite right. The two main things are that you should send an OnStopRequest to the callback like this: http://mxr.mozilla.org/comm-central/source/mail/components/cloudfile/nsYouSendIt.js#212

or else the spinners won't stop spinning in the compose window UI, 

and you should cancel the channel, not the request, with a binding aborted, like this:

http://mxr.mozilla.org/comm-central/source/mail/components/cloudfile/nsYouSendIt.js#871

And, we should have unit tests for the spider oak provider. I believe these can be copied fairly easily from the dropbox tests, since your interface is similar. In particular, see http://mxr.mozilla.org/comm-central/source/mail/test/mozmill/cloudfile/test-cloudfile-backend-dropbox.js and http://mxr.mozilla.org/comm-central/source/mail/test/mozmill/shared-modules/test-cloudfile-dropbox-helpers.js - mconley can probably help with the details.
(Reporter)

Comment 246

7 years ago
Comment on attachment 611641 [details] [diff] [review]
oauth compatibility enhancements

this patch has bit-rotted (you must not have updated to the tip before generating your diff). I've tried to fix it myself and I'll attach an updated patch in a bit.
Attachment #611641 - Flags: review?(dbienvenu) → review-
(Reporter)

Comment 247

7 years ago
Posted patch de-bitrotted patch (obsolete) — Splinter Review
I'll try to test this today.
Attachment #611641 - Attachment is obsolete: true
Attachment #613585 - Flags: review?(dbienvenu)
(Reporter)

Updated

7 years ago
Depends on: 744004
Comment on attachment 613294 [details] [diff] [review]
spideroak support for BigFiles(FileLink?)

This file and associated work is being moved to bug 744035.
Attachment #613294 - Attachment is obsolete: true
Comment on attachment 613585 [details] [diff] [review]
de-bitrotted patch

This file and associated work is being moved to bug 744035.
Attachment #613585 - Attachment is obsolete: true
Attachment #613585 - Flags: review?(dbienvenu)
Attachment #613296 - Attachment is obsolete: true

Updated

7 years ago
Depends on: 743181

Comment 250

7 years ago
(In reply to Mike Conley (:mconley) from comment #204)
> Created attachment 604112 [details] [diff] [review]
> Wrap plaintext signatures and quote prefixes in spans

In InsertDivWrappedTextAtSelection(), you have added an extra line break after the DIV-Tag: <http://hg.mozilla.org/comm-central/rev/43b791fd9ea4?revcount=100#l8.45>

Is that necessary?

In plaintext messages this causes empty lines after the signature.

Could this be omitted?
Assignee: dbienvenu → nobody
Component: Message Compose Window → FileLink
QA Contact: message-compose → filelink
This feature has been shipped - I'm going to close this one unless anybody has a problem with that.
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED

Updated

7 years ago
Duplicate of this bug: 462083
Blocks: 462083

Updated

5 years ago
Duplicate of this bug: 361561
You need to log in before you can comment on or make changes to this bug.