Last Comment Bug 738078 - Make use of BigFiles/FileLink to attach files in the event dialog
: Make use of BigFiles/FileLink to attach files in the event dialog
Status: RESOLVED FIXED
:
Product: Calendar
Classification: Client Software
Component: Dialogs (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: 1.8
Assigned To: Philipp Kewisch [:Fallen]
:
Mentors:
Depends on: 738079 738081 785733 792884
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-21 17:00 PDT by Philipp Kewisch [:Fallen]
Modified: 2012-09-20 09:44 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Fix - v1 (16.91 KB, patch)
2012-07-21 05:16 PDT, Philipp Kewisch [:Fallen]
matthew.mecca: review-
Details | Diff | Review
Fix - v2 (17.76 KB, patch)
2012-08-25 03:07 PDT, Philipp Kewisch [:Fallen]
matthew.mecca: review+
Details | Diff | Review

Description Philipp Kewisch [:Fallen] 2012-03-21 17:00:21 PDT
Now that Thunderbird supports BigFiles/FileLink, we could very well make use of it to offer makeshift file attachments.

This is actually quite easy, I have a work in progress patch that implements the basic features. I just have to clean up error reporting, finish the UI work and some improvements on how the async operations are handled.

Something I feel quite uncomfortable with is saving the cloud provider account key and original filename on the attachment object. It will be serialized as:

ATTACH;X-MOZ-CLOUD-PROVIDER=account1;X-MOZ-CLOUD-FILENAME=08sim.pdf:http:/
 /db.tt/FUsxqHrR

I could likely get rid of the provider parameter, but how would I get the filename?

Also, deleting attachments directly on the remote store only works within one event dialog session, as there is no method to delete a file using its public URL.
Comment 1 Philipp Kewisch [:Fallen] 2012-07-15 15:32:52 PDT
I've decided to push the strings for this bug in comm-central changeset dd788105a464 before the merge, this has been work in progress long enough. Lets get this show on the road, I promise^H^H^H^H^H^H^Hthink I'll fix the patch shortly after the merge.
Comment 2 Philipp Kewisch [:Fallen] 2012-07-21 05:16:03 PDT
Created attachment 644626 [details] [diff] [review]
Fix - v1

There we go, here is the patch for comm-central. Please test thoroughly.

There is one issue remaining, I use the FILENAME attribute from http://tools.ietf.org/html/draft-daboo-caldav-attachments-01 but I haven't yet found a good solution for the security issue. Theoretically there could be evil crafting something like this:

ATTACH;FILENAME=your_bill.pdf:http://example.com/evil.scr 

to trick the user into downloading the file. I do have the tooltip set to the full uri, but I don't know if thats enough or how else we can prevent this.
Comment 3 Matthew Mecca [:mmecca] 2012-07-29 07:29:05 PDT
Comment on attachment 644626 [details] [diff] [review]
Fix - v1

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

Overall it works well, but there are a few issues:

- The image in the attachment list doesn't show up for me until the file upload is completed, the loading icon doesn't ever appear.

- When the upload fails, I don't see the error image either, the file isn't removed from the attachment list, and the following appears in the error console:

Error: TypeError: documentLink.removeItem is not a function
Source file: chrome://calendar/content/calendar-event-dialog.js
Line: 1819

- With Storage calendar items, the file URL appears in the attachment list instead of the file name after restarting Thunderbird.

::: calendar/base/content/dialogs/calendar-event-dialog.js
@@ +1782,4 @@
>      return name;
>  }
>  
> +function uploadCloudAttachment(attachment, cloudProvider, item) {

Using "item" as the argument name is a little confusing here - when it is used in other functions in the file it usually refers to the calendar item, while in this case it refers to a list item element. A function comment might be useful here also.
Comment 4 Philipp Kewisch [:Fallen] 2012-08-25 03:07:46 PDT
Created attachment 655288 [details] [diff] [review]
Fix - v2

(In reply to Matthew Mecca [:mmecca] from comment #3)

Fixed all issues, except for:

> - With Storage calendar items, the file URL appears in the attachment list
> instead of the file name after restarting Thunderbird.
> 
This is very unfortunate. Fixing the problem makes me want to fix so many other things though:

* the prop is lost, because the storage calendar doesn't save it
* this makes me want to change the cal_attachments table to save the whole icalString, but there is no setter for this in calIIcalProperty
* this makes me want to add a such setter, but that means changing our C++ code
* this makes me want to get rid of our C++ code
* it also makes me want to rewrite the storage provider so the whole item is in one blob and only indexed values are saved in extra fields

so you see, I think its easier to just accept this problem for now :)


> Using "item" as the argument name is a little confusing here - when it is
> used in other functions in the file it usually refers to the calendar item,
> while in this case it refers to a list item element. A function comment
> might be useful here also.
Changed to listItem and added a function comment.

Do you think you could review this in the next few days? I'd like to push this down to beta since we also have the strings there.
Comment 5 Matthew Mecca [:mmecca] 2012-08-25 11:10:51 PDT
Comment on attachment 655288 [details] [diff] [review]
Fix - v2

r=mmecca
Comment 6 Philipp Kewisch [:Fallen] 2012-08-25 15:22:06 PDT
Pushed to comm-central changeset 58cd3591ca84
Comment 7 Philipp Kewisch [:Fallen] 2012-08-25 15:25:19 PDT
Backported to releases/comm-aurora changeset 4ab2340c2d04
Comment 8 Philipp Kewisch [:Fallen] 2012-08-25 15:25:43 PDT
The comm-aurora patch obviously doesn't have the string changes.

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