Closed Bug 516027 Opened 12 years ago Closed 11 years ago

Tools->Downloads does not restore a minimized download window

Categories

(Core :: Widget: Cocoa, defect)

All
macOS
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla1.9.3a1
Tracking Status
status1.9.2 --- .7-fixed

People

(Reporter: jruderman, Assigned: barryn)

References

Details

(Keywords: regression)

Attachments

(1 file, 2 obsolete files)

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.
I think this is a dupe of bug 510221.
Depends on: 510221
I agree. Jesse if you still see this on the trunk please reopen.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 510221
I still see this on trunk (rev 1aaedcad1634+).
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
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'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.
No longer depends on: 510221
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.
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.
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.
(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).
(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 <neil@mozilla.com>
date:        Tue Jun 16 14:05:16 2009 -0400
summary:     Bug 497565, only make the window key when visible, r=josh
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.
(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.)
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).
Attached patch patch v1 (obsolete) — Splinter Review
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.)
Attached patch an alternate patch (obsolete) — Splinter Review
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.
(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.
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.
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...)
(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.
(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.
(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 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).
Attachment #421280 - Attachment is obsolete: true
Barry: you should ask "joshmoz@gmail" to review your patch (by setting the "review" flag to "?" and entering his address in the requestee field).
Attachment #421299 - Flags: review?(joshmoz)
Assignee: nobody → barryn
Hardware: x86 → All
Attachment #421299 - Flags: review?(joshmoz) → review+
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.
I'm attaching a new version of the patch which removes the comment.
Attachment #421299 - Attachment is obsolete: true
Attachment #424358 - Flags: review?(joshmoz)
Attachment #424358 - Flags: review?(joshmoz) → review+
Component: Download Manager → Widget: Cocoa
Product: Toolkit → Core
QA Contact: download.manager → cocoa
http://hg.mozilla.org/mozilla-central/rev/32d0d4b37447
Status: REOPENED → RESOLVED
Closed: 12 years ago11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
Can we back out the fix for bug 510221 now?
(In reply to comment #27)
> Can we back out the fix for bug 510221 now?

Barry, do you know the answer to this question?
Can we port this back to 1.9.2 please?  Would help Thunderbird 3.1, as per bug 534814.
status1.9.2: --- → ?
Blocks: 534814
Attachment #424358 - Flags: approval1.9.2.5?
Attachment #424358 - Flags: approval1.9.2.4?
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.
Attachment #424358 - Flags: approval1.9.2.4? → approval1.9.2.4-
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.
Attachment #424358 - Flags: approval1.9.2.7?
Attachment #424358 - Flags: approval1.9.2.6-
Attachment #424358 - Flags: approval1.9.2.5?
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.
Attachment #424358 - Flags: approval1.9.2.7?
Attachment #424358 - Flags: approval1.9.2.6-
Attachment #424358 - Flags: approval1.9.2.6+
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
Status: RESOLVED → VERIFIED
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
You need to log in before you can comment on or make changes to this bug.