516027 - Tools->Downloads does not restore a minimized download window

Status: VERIFIED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Jesse Ruderman]

Steps to reproduce:
1. Open the downloads window.
2. Minimize the downloads window.
3. Tools->Downloads or Cmd+J.

Result: nothing happens.

Expected: restore (un-minimize) and focus the download window.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

I think this is a dupe of bug 510221.

Comment 2

[Kevin Brosnan]

I agree. Jesse if you still see this on the trunk please reopen.

Comment 3

[Jesse Ruderman]

I still see this on trunk (rev 1aaedcad1634+).

Comment 4

[Håkan Waara]

My fix only covered the Window menu, but I guess the problem exists in the Tools menu as well. Maybe it should not be the chrome's responsibility to un-minimize a window? 

I thought initially about putting the fix in the widget layer; IIRC you could let the nsCocoaWindow::SetFocus() un-minimize a window if you're trying to focus something minimized, but it's a much more "risky" fix.

Comment 5

[Shawn Wilsher :sdwilsh]

I'm pretty sure the code that would try to open this is just this:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/downloads/src/nsDownloadManagerUI.js#61

If we can add the right code in there, we can fix all cases for the download manager.

Comment 6

[Barry K. Nathan]

FWIW I'm able to reproduce this on Firefox 3.6 RC1, but not on Firefox 3.5.7. (I'm running 3.6 RC1 on PowerPC, but 3.5.7 on Intel; I hope that's not relevant. 10.5 Leopard in both cases BTW.) So, from my POV at least, this looks like a regression.

Comment 7

[Barry K. Nathan]

Now I've tried both 3.5.7 and 3.6 RC1 on the same computer, with the same profile (on PowerPC). It's still the case that I can reproduce this bug with 3.6RC1 but not with 3.5.7.

Comment 8

[Barry K. Nathan]

Ok, some more info. This bug:
-does not happen on Firefox 3.5.0
-does happen on Namoroka 3.6a1
-does happen on the latest trunk nightly as of a few hours ago (rev 537a710037d4)

I doubt I will have time to further narrow down when the regression actually happened. So that's why I'm posting what I've found so far, in case it helps anyone.

Comment 9

[Barry K. Nathan]

(In reply to comment #8)
> I doubt I will have time to further narrow down when the regression actually
> happened. So that's why I'm posting what I've found so far, in case it helps
> anyone.

Narrowing it down much further took far less time than I expected:

nightly build 2009-06-16-03-mozilla-central (rev ca8799e74642): cannot reproduce bug

nightly build 2009-06-17-03-mozilla-central (rev 92b6868ef3f4): bug is 100% reproducible

I don't expect to be able to narrow it down any further today (if I did, I would wait before posting another comment).

Comment 10

[Barry K. Nathan]

(In reply to comment #9)
> I don't expect to be able to narrow it down any further today (if I did, I
> would wait before posting another comment).

Once again I was wrong...

hg bisect eventually gave me this output:

The first bad revision is: 
changeset:   29273:ec9034099c13                                                 
user:        Neil Deakin <[e-mail]>
date:        Tue Jun 16 14:05:16 2009 -0400
summary:     Bug 497565, only make the window key when visible, r=josh

Comment 11

[:Gavin Sharp [email: [e-mail]]]

Thanks for the regression range, Barry!

I suppose we could fix this using the code from attachment 394397 [details] [diff] [review], but I'm not sure whether it's expected that bug 497565 changed window.focus() behavior for minimized windows on Mac.

Comment 12

[Barry K. Nathan]

(In reply to comment #11)
> Thanks for the regression range, Barry!

You're welcome.

> I suppose we could fix this using the code from attachment 394397 [details] [diff] [review], but I'm not
> sure whether it's expected that bug 497565 changed window.focus() behavior for
> minimized windows on Mac.

I think attachment 394397 [details] [diff] [review] is already in the trunk, which still has this bug. (If I misunderstood your comment, then never mind.)

Comment 13

[:Gavin Sharp [email: [e-mail]]]

I meant that we could make the same change that that patch is making, but to the code that's relevant to this case (pointed out by Shawn in comment 5).

Comment 14

[Barry K. Nathan]

Attachment: patch v1

This is a patch that implements the suggestion in comments 11 and 13. In the back of my mind I have doubts about whether this is really the correct fix, but I'm pretty unfamiliar with the Mozilla code so those doubts may be unfounded.

Anyway, I tested this against 3.6 RC1 on PowerPC and it seems to fix the problem. (I'll test against the trunk later today.)

Comment 15

[Barry K. Nathan]

Attachment: an alternate patch

I had wanted to compile and test this patch before uploading it, but a situation suddenly arose regarding my build machine, so I might be able to get around to it for another day or two.

This patch tries to fix the problem a different way, under the assumption that the real problem was introduced by the change that made bug 516027 show up in the first place. That assumption may not be correct; this patch could cause bug 497565 to regress if I've misunderstood what's going on; and as I've mentioned, this patch hasn't even been compile-tested yet. Nonetheless, this could be worth trying.

Comment 16

[Barry K. Nathan]

(Oops, that should have been "might *not* be able to get around to it...")

Well, I was able to compile & test both patches (applied to a trunk checkout from earlier today), and both patches fix this bug. I have not yet tried to do any testing with respect to bug 497565.

Comment 17

[Barry K. Nathan]

When I revert attachment 383281 [details] [diff] [review], I can reproduce bug 497565 on demand. If I reapply attachment 383281 [details] [diff] [review], then bug 497565 goes away (of course). If I apply attachment 421299 [details] [diff] [review], then bug 497565 still does not occur.

In fewer words, attachment 421299 [details] [diff] [review] (my alternate patch) does not cause a regression WRT bug 497565.

AFAICT the next step is to figure out which of the two patches is better.

Comment 18

[Barry K. Nathan]

Quoting from widget/src/windows/nsWindow.cpp, starting around line 1658 on
the trunk (around line 1590 on 1.9.2):
>     // Uniconify, if necessary
>     HWND toplevelWnd = GetTopLevelHWND(mWnd);
>     if (aRaise && ::IsIconic(toplevelWnd)) {
>       ::ShowWindow(toplevelWnd, SW_RESTORE);
>     }

Now I feel pretty sure that my second ("alternate") patch uses the correct approach. (If it's not, then the code in widget/src/windows/nsWindow.cpp is wrong...)

Comment 19

[Barry K. Nathan]

(In reply to comment #4)
> My fix only covered the Window menu, but I guess the problem exists in the
> Tools menu as well. Maybe it should not be the chrome's responsibility to
> un-minimize a window? 
> 
> I thought initially about putting the fix in the widget layer; IIRC you could
> let the nsCocoaWindow::SetFocus() un-minimize a window if you're trying to
> focus something minimized, but it's a much more "risky" fix.


(I just re-read this comment, and now it means more to me, since I've read more
of the code.)

As I mentioned in comment 18, the Windows nsWindow::SetFocus()
un-minimizes the window. I just tried downloading and running both a trunk
nightly and Firefox 3.6 RC2 on Fedora 12, and this bug did not occur, so
nsWindow::SetFocus() seems to un-minimize the window on Linux too.

Is there any documentation that specifies what nsWindow::SetFocus() should do
in the case of a minimized window? If there is, I have failed to find it.

Comment 20

[Neil Deakin]

(In reply to comment #19)

> Is there any documentation that specifies what nsWindow::SetFocus() should do
> in the case of a minimized window? If there is, I have failed to find it.

SetFocus should unminimize the window, but only if aRaise is true.

Comment 21

[Barry K. Nathan]

(In reply to comment #20)
> SetFocus should unminimize the window, but only if aRaise is true.

I believe attachment 421299 [details] [diff] [review] is the correct fix, then.

Comment 22

[Barry K. Nathan]

Comment on attachment 421280 [details] [diff] [review]
patch v1

I'm marking my original patch as obsolete because: (a) it doesn't fix the root cause (b) my newer ("alternate") patch also fixes bug 525759 (a.k.a. bug 542308).

Comment 23

[:Gavin Sharp [email: [e-mail]]]

Barry: you should ask "joshmoz@gmail" to review your patch (by setting the "review" flag to "?" and entering his address in the requestee field).

Comment 24

[Josh Aas]

Comment on attachment 421299 [details] [diff] [review]
an alternate patch

I don't think the comment is necessary, it just repeats what is quite obvious from the code already.

Comment 25

[Barry K. Nathan]

Attachment: nsCocoaWindow::SetFocus fix v1.1 (comment now removed)

I'm attaching a new version of the patch which removes the comment.

Comment 26

[Dão Gottwald [::dao]]

http://hg.mozilla.org/mozilla-central/rev/32d0d4b37447

Comment 27

[:Gavin Sharp [email: [e-mail]]]

Can we back out the fix for bug 510221 now?

Comment 28

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #27)
> Can we back out the fix for bug 510221 now?

Barry, do you know the answer to this question?

Comment 29

[David Ascher (:davida)]

Can we port this back to 1.9.2 please?  Would help Thunderbird 3.1, as per bug 534814.

Comment 30

[Mike Beltzner [:beltzner, not reading bugmail]]

Sure, we're happy to take the backport. Right now we're trying to keep the 1.9.2 trees quiet as we're hoping to kick of Firefox 3.6.4 build 2 in the next few hours. Once that build is ready to go, I think we'll start processing some 1.9.2.5 approvals (for landing on mozilla-1.9.2 default) and then you guys can build off of that version.

If you want to build Thunderbird 3.1 off of mozilla-1.9.2's 1.9.2.4 relbranch, please let me know ASAP; that might be trickier.

Comment 31

[LegNeato]

Comment on attachment 424358 [details] [diff] [review]
nsCocoaWindow::SetFocus fix v1.1 (comment now removed)

We will not be taking this for 3.6.6. Moving approval request forward.

Comment 32

[LegNeato]

Actually, ignore comment 31. I am going to approve this as it looks safe and TB wants it. Please land on mozilla-1.9.2 default, *not the relbranch*.

Code freeze is Friday (tomorrow night) @ 11:59 pm PST, apologies for this getting lost in the jumble.

Comment 33

[:ehsan akhgari]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/8686a1a62d3e

Comment 34

[Tanay Gavankar [:tgavankar]]

VERIFIED FIXED on Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2.7) Gecko/20100701 Firefox 3.6.7

Comment 35

[Tanay Gavankar [:tgavankar]]

Err, my mistake - I meant VERI FIXED on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.2.7) Geck/20100701 Firefox/3.6.7