Last Comment Bug 711085 - Attachment tests are never run as part of mozmilltests.list, and are broken
: Attachment tests are never run as part of mozmilltests.list, and are broken
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: Testing Infrastructure (show other bugs)
: Trunk
: x86 All
: -- normal (vote)
: Thunderbird 12.0
Assigned To: Mike Conley (:mconley) - (needinfo me!)
:
Mentors:
Depends on: 715488
Blocks: 715490
  Show dependency treegraph
 
Reported: 2011-12-15 08:27 PST by Mike Conley (:mconley) - (needinfo me!)
Modified: 2012-01-05 07:12 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch v1 (3.68 KB, patch)
2011-12-30 10:12 PST, Mike Conley (:mconley) - (needinfo me!)
squibblyflabbetydoo: review+
Details | Diff | Splinter Review
Patch v2 (r+'d by squib) (4.76 KB, patch)
2012-01-04 08:10 PST, Mike Conley (:mconley) - (needinfo me!)
no flags Details | Diff | Splinter Review

Description Mike Conley (:mconley) - (needinfo me!) 2011-12-15 08:27:57 PST
Funny story:  it looks like we've never run the attachment tests as part of mozmilltests.list.

That's not good.

Even worse, when I ran the attachment tests, a bunch of them in test-attachment-menus.js failed.

I've got a fix for this - I'll post it up soon.
Comment 1 Mike Conley (:mconley) - (needinfo me!) 2011-12-30 10:12:36 PST
Created attachment 584984 [details] [diff] [review]
Patch v1

Here's my first run at a fix.  Mozmill tests being run on try here:  http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=448128102300
Comment 2 Mike Conley (:mconley) - (needinfo me!) 2011-12-30 10:14:46 PST
Also, note the hackery in order to get the context menu to appear correctly in test-attachment-menus.js... are we sure we only want the icon / filename to have the context menu when right clicking?  Because we currently seem to ignore right-clicking on the file size.
Comment 3 Jim Porter (:squib) 2011-12-30 14:19:45 PST
Comment on attachment 584984 [details] [diff] [review]
Patch v1

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

Good catch on this! Assuming the tests pass now, r=me with the following nit about the comment.

::: mail/test/mozmill/attachment/test-attachment-menus.js
@@ +329,5 @@
> +
> +  // So this next bit is a little ugly.  The attachment names are currently
> +  // the only items that, when right clicking, bring up the context menus...
> +  // so, in order to open these up, we have to drag out the attachment name
> +  // by querying for it via getAnonymousElementByAttribute.

This part isn't quite true: what's really happening is that the <attachmentitem> ignores pointer events on Windows and Linux, but the icon and nameselection (a wrapper around the name) don't ignore them. (This is to allow click-through to work when clicking on whitespace).

I think it'd be more accurate to say "Attachment items themselves ignore pointer events, but the name always responds to them, so get the element for the name first."
Comment 4 Jim Porter (:squib) 2011-12-30 14:21:56 PST
(In reply to Mike Conley (:mconley) from comment #2)
> Also, note the hackery in order to get the context menu to appear correctly
> in test-attachment-menus.js... are we sure we only want the icon / filename
> to have the context menu when right clicking?  Because we currently seem to
> ignore right-clicking on the file size.

We could add CSS to make the file size be clickable too. I don't have any strong opinions there. We might also want to make some other changes, e.g. giving the attachment list Aero theming.
Comment 5 Mike Conley (:mconley) - (needinfo me!) 2012-01-04 08:10:55 PST
Created attachment 585762 [details] [diff] [review]
Patch v2 (r+'d by squib)

Thanks for the review, squib!  I've corrected that comment.
Comment 6 Mike Conley (:mconley) - (needinfo me!) 2012-01-04 08:16:07 PST
Committed to comm-central as http://hg.mozilla.org/comm-central/rev/7b724a79de0c

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