Last Comment Bug 742725 - Filelink offer reappears after converting a large file back to a regular attachment
: Filelink offer reappears after converting a large file back to a regular atta...
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: Message Compose Window (show other bugs)
: Trunk
: x86 All
: -- normal (vote)
: Thunderbird 14.0
Assigned To: Mike Conley (:mconley) - (needinfo me!)
:
:
Mentors:
Depends on: 738299
Blocks: BigFiles
  Show dependency treegraph
 
Reported: 2012-04-05 07:56 PDT by Mike Conley (:mconley) - (needinfo me!)
Modified: 2012-04-16 11:57 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
fixed


Attachments
Patch v1 (6.58 KB, patch)
2012-04-05 11:43 PDT, Mike Conley (:mconley) - (needinfo me!)
squibblyflabbetydoo: review+
Details | Diff | Splinter Review
Patch v2 (carrying over r+ from squib) (6.58 KB, patch)
2012-04-16 08:40 PDT, Mike Conley (:mconley) - (needinfo me!)
no flags Details | Diff | Splinter Review
Patch v3 (carrying over r+ from squib) (6.79 KB, patch)
2012-04-16 08:42 PDT, Mike Conley (:mconley) - (needinfo me!)
mozilla: approval‑comm‑aurora+
Details | Diff | Splinter Review

Description Mike Conley (:mconley) - (needinfo me!) 2012-04-05 07:56:22 PDT
STR:

1)  Attach a large file in a compose window, causing the Filelink offer to appear
2)  Convert the file to a Filelink
3)  Convert the file back to a regular attachment

What happens?

The Filelink offer reappears.

What's expected?

Since I already know about Filelink, already used it, and decided not to, I really shouldn't hear about it again.
Comment 1 Mike Conley (:mconley) - (needinfo me!) 2012-04-05 11:43:29 PDT
Created attachment 612628 [details] [diff] [review]
Patch v1

It looks like we were calling the attachmentsAdded function inside attachmentsConverted.

Fixes that, cleans bigFileObserver.js a little bit, and adds a test.

-Mike
Comment 2 Mike Conley (:mconley) - (needinfo me!) 2012-04-05 11:46:20 PDT
squib:

Just a note that the patch for bug 738299 needs to be applied before this one.

-Mike
Comment 3 Jim Porter (:squib) 2012-04-15 14:20:33 PDT
Comment on attachment 612628 [details] [diff] [review]
Patch v1

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

This looks good to me by inspection. Assuming the tests pass, r=me.

::: mail/test/mozmill/cloudfile/test-cloudfile-notifications.js
@@ +8,5 @@
>  
>  let Cu = Components.utils;
>  let Cc = Components.classes;
>  let Ci = Components.interfaces;
> +let Cr = Components.results;

While we're here, maybe switch these to const? (Not that it really matters, but that's how I usually see these.)
Comment 4 Mike Conley (:mconley) - (needinfo me!) 2012-04-16 08:40:13 PDT
Created attachment 615344 [details] [diff] [review]
Patch v2 (carrying over r+ from squib)

Unbitrotting due to 744004 landing.
Comment 5 Mike Conley (:mconley) - (needinfo me!) 2012-04-16 08:42:05 PDT
(In reply to Jim Porter (:squib) from comment #3)
> Comment on attachment 612628 [details] [diff] [review]
> Patch v1
> 
> Review of attachment 612628 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good to me by inspection. Assuming the tests pass, r=me.
> 
> ::: mail/test/mozmill/cloudfile/test-cloudfile-notifications.js
> @@ +8,5 @@
> >  
> >  let Cu = Components.utils;
> >  let Cc = Components.classes;
> >  let Ci = Components.interfaces;
> > +let Cr = Components.results;
> 
> While we're here, maybe switch these to const? (Not that it really matters,
> but that's how I usually see these.)

Funnily enough, it turns out that these are all predefined in the global scope.  I'll excise them.
Comment 6 Mike Conley (:mconley) - (needinfo me!) 2012-04-16 08:42:52 PDT
Created attachment 615345 [details] [diff] [review]
Patch v3 (carrying over r+ from squib)

Removing Cc/Ci/Cu/Cr definitions.
Comment 7 Mike Conley (:mconley) - (needinfo me!) 2012-04-16 09:58:31 PDT
Committed to comm-central as http://hg.mozilla.org/comm-central/rev/88c29b17c96d
Comment 8 Mike Conley (:mconley) - (needinfo me!) 2012-04-16 11:57:11 PDT
Committed to comm-aurora as http://hg.mozilla.org/releases/comm-aurora/rev/0a45d6df193b

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