Last Comment Bug 715961 - Attachment events in compose window need to be tested
: Attachment events in compose window need to be tested
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: Message Compose Window (show other bugs)
: Trunk
: x86 All
: -- normal (vote)
: Thunderbird 12.0
Assigned To: Mike Conley (:mconley) - (Away until June 29th)
:
Mentors:
: 715490 (view as bug list)
Depends on: 708982
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-06 10:49 PST by Mike Conley (:mconley) - (Away until June 29th)
Modified: 2012-01-23 14:55 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch v1 (35.08 KB, patch)
2012-01-06 10:52 PST, Mike Conley (:mconley) - (Away until June 29th)
no flags Details | Diff | Review
Patch v2 (22.09 KB, patch)
2012-01-09 09:34 PST, Mike Conley (:mconley) - (Away until June 29th)
no flags Details | Diff | Review
Patch v3 (26.28 KB, patch)
2012-01-10 09:16 PST, Mike Conley (:mconley) - (Away until June 29th)
no flags Details | Diff | Review
Patch v4 (24.96 KB, patch)
2012-01-19 13:03 PST, Mike Conley (:mconley) - (Away until June 29th)
no flags Details | Diff | Review
Patch v5 (26.33 KB, patch)
2012-01-23 08:57 PST, Mike Conley (:mconley) - (Away until June 29th)
sid.bugzilla: review+
Details | Diff | Review

Description Mike Conley (:mconley) - (Away until June 29th) 2012-01-06 10:49:22 PST
We'd like some Mozmill tests to ensure that the events introduced in bug 708982 actually do what we want them to do.
Comment 1 Mike Conley (:mconley) - (Away until June 29th) 2012-01-06 10:52:29 PST
Created attachment 586482 [details] [diff] [review]
Patch v1
Comment 2 Mike Conley (:mconley) - (Away until June 29th) 2012-01-06 10:55:08 PST
Comment on attachment 586482 [details] [diff] [review]
Patch v1

Hey squib,

I know your swamped, so I'm not asking for a full review from you, but could you give my tests a quick peek to make sure I'm poking at the right things?

Also, if you can think of more interesting tests I should try, let me know.

Thanks,

-Mike
Comment 3 Mike Conley (:mconley) - (Away until June 29th) 2012-01-06 12:05:05 PST
*** Bug 715490 has been marked as a duplicate of this bug. ***
Comment 4 Jim Porter (:squib) 2012-01-06 23:49:56 PST
I haven't had a chance to look at all of this, but for simplicity, you don't need to attach a real file. It should be possible to add a fake attachment via add_attachment[1]. You also don't need to synthesize mouse events to select attachments in the list; all the XUL listbox methods, like selectItemRange(), are available.

[1] http://mxr.mozilla.org/comm-central/source/mail/test/mozmill/shared-modules/test-compose-helpers.js#256
Comment 5 Mike Conley (:mconley) - (Away until June 29th) 2012-01-09 09:34:11 PST
Created attachment 587021 [details] [diff] [review]
Patch v2

Thanks for the feedback squib!  Very useful - I've been able to simplify my patch immensely.
Comment 6 Mike Conley (:mconley) - (Away until June 29th) 2012-01-10 09:16:28 PST
Created attachment 587344 [details] [diff] [review]
Patch v3

Whoops - forgot a pretty critical file.

http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=33bdc74bedb3
Comment 7 Mike Conley (:mconley) - (Away until June 29th) 2012-01-13 07:53:47 PST
Comment on attachment 587344 [details] [diff] [review]
Patch v3

Hey Sid - have any cycles to check this out?
Comment 8 Siddharth Agarwal [:sid0] (inactive) 2012-01-16 12:05:33 PST
Comment on attachment 587344 [details] [diff] [review]
Patch v3

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

::: mail/test/mozmill/attachment/test-attachment-events.js
@@ +1,2 @@
> +/* ***** BEGIN LICENSE BLOCK *****
> + * Version: MPL 1.1/GPL 2.0/LGPL 2.1

We use MPL 2.0 now, which means much simpler headers! http://www.mozilla.org/MPL/headers/

@@ +282,5 @@
> +  let removedAttachmentItems = select_attachments(cw, 0, 2);
> +
> +  let removedAttachmentUrls = removedAttachmentItems.map(function(aAttachment) {
> +    return aAttachment.attachment.url;
> +  });

function (aAttachment) aAttachment.attachment.url

@@ +374,5 @@
> +  assert_true(obs.didSee(kAttachmentRenamed));
> +  // Ensure that the event mentions the right attachment
> +  let renamedAttachment1 = obs.subject[kAttachmentRenamed][0];
> +  assert_true(renamedAttachment1 instanceof Ci.nsIMsgAttachment);
> +  assert_equals(kRenameTo1, renamedAttachment1.name);

Does mail:attachmentRenamed not tell you what the original name was? That seems like an oversight.

@@ +379,5 @@
> +  assert_true(renamedAttachment1.url.indexOf("http://www.example.com/1") != -1);
> +
> +  // Ok, let's try renaming the same attachment.
> +  gMockPromptService.reset();
> +  gMockPromptService.inoutValue = kRenameTo2

;

@@ +393,5 @@
> +  assert_true(renamedAttachment2.url.indexOf("http://www.example.com/1") != -1);
> +
> +  // Ok, let's rename another attachment
> +  gMockPromptService.reset();
> +  gMockPromptService.inoutValue = kRenameTo3

;

@@ +415,5 @@
> +}
> +
> +/**
> + * Test that the mail:attachmentsRenamed event is not fired if we set the
> + * filename to be blank.

Why does this behaviour exist?

::: mail/test/mozmill/shared-modules/test-observer-helpers.js
@@ +1,2 @@
> +/* ***** BEGIN LICENSE BLOCK *****
> + * Version: MPL 1.1/GPL 2.0/LGPL 2.1

MPL 2.0

@@ +34,5 @@
> + * the terms of any one of the MPL, the GPL or the LGPL.
> + *
> + * ***** END LICENSE BLOCK ***** */
> +
> +const MODULE_NAME = "observer-helpers";

This is going to be really, really useful. Thanks for doing this.

@@ +50,5 @@
> +function ObservationRecorder() {
> +  this._topics = [];
> +  this.data = {};
> +  this.subject = {};
> +  this.saw = {};

I'd suggest calling reset instead.

@@ +57,5 @@
> +ObservationRecorder.prototype = {
> +  /**
> +   * Called by the Observer Service when an event is fired.
> +   */
> +  observe: function(aSubject, aTopic, aData) {

Please name this and all the other functions.

@@ +79,5 @@
> +
> +  /**
> +   * Puts the observer back into its starting state.
> +   */
> +  reset: function() {

Might a resetTopic function make sense?

@@ +90,5 @@
> +  /**
> +   * Gets the AttachmentObserver ready to observe events.  Must be called
> +   * before any recording can be done.
> +   *
> +   * @param aTopics A string representing the topic that the AttachmentObserver

that the ObservationRecorder

@@ +107,5 @@
> +
> +  /**
> +   * Returns true of a particular topic was observed at least once.
> +   *
> +   * @param aTopic the topic to check if the AttachmentObserver saw.

the ObservationRecorder saw
Comment 9 Jim Porter (:squib) 2012-01-16 14:45:45 PST
(In reply to Siddharth Agarwal [:sid0] from comment #8)
> @@ +415,5 @@
> > +}
> > +
> > +/**
> > + * Test that the mail:attachmentsRenamed event is not fired if we set the
> > + * filename to be blank.
> 
> Why does this behaviour exist?

If you try to rename an attachment to the empty string, it will bail out and refuse to actually rename it. Thus, no event gets fired.
Comment 10 Mike Conley (:mconley) - (Away until June 29th) 2012-01-19 13:03:40 PST
Created attachment 589959 [details] [diff] [review]
Patch v4

Thanks Sid - I've made the corrections you've suggested.

Awesome how short the MPL 2 header is.

> Does mail:attachmentRenamed not tell you what the original name was? That
> seems like an oversight.

squib:  what's your take on that?

-Mike
Comment 11 Jim Porter (:squib) 2012-01-19 18:07:26 PST
(In reply to Mike Conley (:mconley) from comment #10)
> > Does mail:attachmentRenamed not tell you what the original name was? That
> > seems like an oversight.
> 
> squib:  what's your take on that?

Seems reasonable. There's a wstring argument to notifyObservers that we could use.
Comment 12 Mike Conley (:mconley) - (Away until June 29th) 2012-01-23 08:57:52 PST
Created attachment 590734 [details] [diff] [review]
Patch v5

Ok, I've also added Sid's suggestion of notifying observers of a renamed attachments original name.
Comment 13 Mike Conley (:mconley) - (Away until June 29th) 2012-01-23 08:59:37 PST
Comment on attachment 590734 [details] [diff] [review]
Patch v5

Ok, we're notifying observers about the original names of renamed attachments now, as per your suggestion.
Comment 14 Siddharth Agarwal [:sid0] (inactive) 2012-01-23 14:51:41 PST
Comment on attachment 590734 [details] [diff] [review]
Patch v5

Thanks.
Comment 15 Mike Conley (:mconley) - (Away until June 29th) 2012-01-23 14:55:45 PST
Committed to comm-central as http://hg.mozilla.org/comm-central/rev/99470a5f7a8b

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