361383 - Patch for bug 348183 can be backed out now that the patch for bug 348357 went in

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Martijn Wargers (dead)]

Patch for bug 348183 can be backed out now that the patch for bug 348357 went in.
The patch for bug 348183 was more or less a work-around.

Comment 1

[Martijn Wargers (dead)]

Attachment: patch

Like this.

Comment 2

[(not reading, please use [e-mail] instead)]

backing out my fix seems reasonable, but given that nsGlobalWindow::Focus() could still return NS_ERROR_SOMETHING (if widget->SetFocus() returns an error, for example [1]) maybe keeping the try / catch isn't a bad idea? 

[1] http://lxr.mozilla.org/seamonkey/source/dom/src/base/nsGlobalWindow.cpp#3624

I'll defer to gavin.

Comment 3

[(not reading, please use [e-mail] instead)]

Comment on attachment 246145 [details] [diff] [review]
patch

while it was a work around, since .focus() could still fail, I don't think keeping the try/catch is a bad idea.  but if you get r=gavin, then I'm ok with it.

Comment 4

[Martijn Wargers (dead)]

(In reply to comment #2)
> backing out my fix seems reasonable, but given that nsGlobalWindow::Focus()
> could still return NS_ERROR_SOMETHING (if widget->SetFocus() returns an error,
> for example [1]) maybe keeping the try / catch isn't a bad idea? 

When looking at the functions:
http://lxr.mozilla.org/seamonkey/source/widget/src/gtk2/nsWindow.cpp#696
http://lxr.mozilla.org/seamonkey/source/widget/src/os2/nsWindow.cpp#1513
http://lxr.mozilla.org/seamonkey/source/widget/src/gtk/nsWindow.cpp#1347
http://lxr.mozilla.org/seamonkey/source/widget/src/mac/nsWindow.cpp#582
http://lxr.mozilla.org/seamonkey/source/widget/src/beos/nsWindow.cpp#1168
http://lxr.mozilla.org/seamonkey/source/widget/src/windows/nsWindow.cpp#2204
I see that gtk2 and windows indeed can return failure.
But it seems to me that nsGlobalWindow::Focus() should never be able to return an error, should it?

Comment 5

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

Comment on attachment 246145 [details] [diff] [review]
patch

Those failure cases look to me like they're pretty exceptional (mostly OOM and being called from a non-GUI thread), so I think it's a good idea to remove this. There are many other places where we assume that .focus() on a window won't throw, and it largely hasn't in the past, modulo the relatively uncommon case of bug 348357.

Comment 6

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

(In reply to comment #5)
> modulo the relatively uncommon case of bug 348357.

Er, I meant bug 331457 and bug 348183, of course.

Comment 7

[(not reading, please use [e-mail] instead)]

Comment on attachment 246145 [details] [diff] [review]
patch

r=sspitzer, all hail G#!

Comment 8

[Martijn Wargers (dead)]

Checking in tabbrowser.xml;
/cvsroot/mozilla/toolkit/content/widgets/tabbrowser.xml,v  <--  tabbrowser.xml
new revision: 1.215; previous revision: 1.214
done

Checked in on trunk.