The attachment events added in bug 708982 are too global and get sent to all compose windows, which causes strange behavior. We should probably use DOM events instead. There are a couple of options: 1) Create the events in MsgComposeCommands.js like they are now 2) Create the events in the XBL binding 3) Fire one event for each attachment added/removed 4) Fire one event for all attachments added/removed at once I think I prefer (1) and (4) respectively. (1) because add/remove events don't make much sense in the reader, and (4) just because that's how it is now, and it reduces the number of events. Another question is what node the events get fired on. I'm thinking the attachmentBucket for add/remove, and the individual <attachmentitem> for rename. I'm open to suggestions for any of these.

I concur, regarding not mucking around in the XBL binding. Let's create them in MsgComposeCommands.js. I also concur on 4, since we might as well stick as close to what we're doing as possible. In fact, I agree with everything you're saying - including where to fire the events. It all sounds reasonable. So, thumbs up for me.

Here's a patch for comm-central to fix this. Not asking for review yet, since it's totally untested (I don't have a comm-central build at the moment, since my VM doesn't have space for two full builds). Try build incoming, though: <http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=b421305d3fb5>. Assuming that passes, this can be reviewd.

This should fix everything for the comm-central part.

Also, here's a patch for the big-files repo, which updates all the interfaces to use the new way.

Comment on attachment 604696 [details] [diff] [review] Patch for comm-central (v2) Review of attachment 604696 [details] [diff] [review]: ----------------------------------------------------------------- This looks good - just one nit, and one note. -Mike ::: mail/components/compose/content/MsgComposeCommands.js @@ +3806,5 @@ > > gContentChanged = true; > + > + let event = document.createEvent("CustomEvent"); > + event.initCustomEvent("attachment-renamed", true, true, originalName); Hm. At first glance, it seems that we don't pass back the renamed attachment to event handlers. However, clearly, we do - since the attachment-renamed tests are still passing. I guess what I'm saying is that we'll certainly want dev-doc for this, since it's not immediately clear how to get the original attachment by just looking at the code. ::: mail/test/mozmill/attachment/test-attachment-events.js @@ +304,4 @@ > } > > /** > + * Test that we don't see the attachments_removed event if no attachments Nit - attachments-removed instead of attachments_removed

Comment on attachment 604697 [details] [diff] [review] Patch for big-files Review of attachment 604697 [details] [diff] [review]: ----------------------------------------------------------------- I know you haven't asked for review on this yet, but I just noticed this, and thought I'd point it out. Cheers, -Mike ::: mail/components/compose/content/bigFileObserver.js @@ +75,5 @@ > if (this.hidden) > return; > > + const callbacks = { > + "attachments-added": this.attachmentsAdded, this.attachmentsAdded, this.attachmentsRemoved and this.attachmentsConverted do not exist. Instead, it's this.attachmentAdded, this.attachmentRemoved, this.attachmentConverted.

(In reply to Mike Conley (:mconley) from comment #5) > Comment on attachment 604696 [details] [diff] [review] > Patch for comm-central (v2) > > Review of attachment 604696 [details] [diff] [review]: > ----------------------------------------------------------------- > > This looks good - just one nit, and one note. > > -Mike > > ::: mail/components/compose/content/MsgComposeCommands.js > @@ +3806,5 @@ > > > > gContentChanged = true; > > + > > + let event = document.createEvent("CustomEvent"); > > + event.initCustomEvent("attachment-renamed", true, true, originalName); > > Hm. At first glance, it seems that we don't pass back the renamed > attachment to event handlers. However, clearly, we do - since the > attachment-renamed tests are still passing. That's because of the next line, which dispatches the event to the specific attachment item being renamed: > + item.dispatchEvent(event); > I guess what I'm saying is that we'll certainly want dev-doc for this, since > it's not immediately clear how to get the original attachment by just > looking at the code. It would probably useful to add some documentation about all of these events so that extension authors can use these hooks. > > + * Test that we don't see the attachments_removed event if no attachments > > Nit - attachments-removed instead of attachments_removed Fixed.

Comment on attachment 604696 [details] [diff] [review] Patch for comm-central (v2) Should we take this on aurora/beta? The non-test changes are tiny, no one uses these events (yet), and I'd prefer to get out quickly so that extension authors can start using them, instead of having to wait until 13 for the new API in this patch.

Checked into central: http://hg.mozilla.org/comm-central/rev/862863b8547f

(In reply to Jim Porter (:squib) from comment #8) > Comment on attachment 604696 [details] [diff] [review] > Patch for comm-central (v2) > > Should we take this on aurora/beta? The non-test changes are tiny, no one > uses these events (yet), and I'd prefer to get out quickly so that extension > authors can start using them, instead of having to wait until 13 for the new > API in this patch. Except you're also removing APIs that extensions may already rely on. So we should give them the maximum amount of time to adopt.