402147 - tabbrowser event handling cleanup

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Dão Gottwald [:dao]]

Attachment: patch

Comment 1

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

I vaguely remember Neil disliking reliance on JS prototype chain lookup in XBL when similar changes were proposed elsewhere, but I don't quite recall why... performance, probably?

Comment 2

[[e-mail]]

(In reply to comment #1)
>I vaguely remember Neil disliking reliance on JS prototype chain lookup in XBL
>when similar changes were proposed elsewhere, but I don't quite recall why...
>performance, probably?
It's not JS proto chain lookup, it's worse. Basically when you write "foo();" in a XUL event listener, then foo is resolved by looking on the element and all of its ancestors up to the window, so it's better if you can avoid it.
Contrast this with HTML, where foo is only resolved on the element, its form (if it has one), its document and window.
For Mozilla2 I think I might want to persuade jst to revisit this.

Comment 3

[Dão Gottwald [:dao]]

Attachment: patch v2

Hrm, ok ...

Comment 4

[Dão Gottwald [:dao]]

Attachment: updated to trunk

Comment 5

[John P Baker]

Just a heads up that another event listener could be introduced in bug 420605 which would need integrating with this.

Comment 6

[Dão Gottwald [:dao]]

Attachment: patch v3

Comment 7

[Dão Gottwald [:dao]]

Attachment: patch v4

missed a code sharing opportunity

Comment 8

[Dão Gottwald [:dao]]

Comment on attachment 405759 [details] [diff] [review]
patch v4

>-            // There's only one browser left. If a window is being
>-            // closed and the window is *not* the window in the
>-            // browser that's still around, prevent the event's default
>-            // action to prevent closing a window that's being closed
>-            // already.
>-            if (this.getBrowserAtIndex(0).contentWindow != event.target)
>-              event.preventDefault();

This was "Aviary branch landing" code, hence no bug and no further details. I don't see how the DOMWindowClose event could reach tabbrowser if that window isn't in tabbrowser anymore. SeaMonkey also doesn't do this.

Comment 9

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

Aviary branch history can be found by checking the relevant branch history (using e.g. http://mxr.mozilla.org/aviarybranch/ which leads to http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/content/widgets/tabbrowser.xml&rev=AVIARY_1_0_1_20050124_BRANCH ) - in this case the relevant bugs are bug 263844 and the regressions it caused, bug 265790 / bug 265962.

Comment 10

[Dão Gottwald [:dao]]

Thanks. So this was added in bug 263844 for bug 265790.

I set browser.link.open_newwindow.restriction to 0 and opened the testcases from all three bugs, they all worked with this patch.

Comment 11

[Mike Connor [:mconnor]]

Comment on attachment 405759 [details] [diff] [review]
patch v4

I am no longer actively working on Firefox, please find another reviewer.

Comment 12

[Dão Gottwald [:dao]]

Attachment: patch v4 [Checked in: See comment 15]

Comment 13

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

Comment on attachment 426890 [details] [diff] [review]
patch v4
[Checked in: See comment 15]

>diff --git a/browser/base/content/tabbrowser.xml b/browser/base/content/tabbrowser.xml

>       <method name="getBrowserIndexForDocument">

>+            var tab = this._getTabForContentWindow(aDocument.defaultView);
>+            return tab ? tab._tPos : -1;

Reliance on keeping _tPos in sync with actual document position has always bothered me, but I suppose we're already committed to that in other places...

>+      <method name="_getTabForContentWindow">

>+          Array.some(this.mTabs, function (t)
>+            t.linkedBrowser.contentWindow == aWindow && !!(tab = t)
>+          );

This is a confusing pattern, I think I'd actually prefer an explicit loop.

>       <method name="enterTabbedMode">

>             if (XULBrowserWindow.isBusy) {
>               this.mCurrentTab.setAttribute("busy", "true");
>               this.mIsBusy = true;
>               this.setTabTitleLoading(this.mCurrentTab);
>               this.updateIcon(this.mCurrentTab);
>             } else {
>-              this.setTabTitle(this.mCurrentTab);
>               this.setIcon(this.mCurrentTab, this.mCurrentBrowser.mIconURL);
>             }

Can you explain this removal?

>+      <method name="_keyEventHandler">

Would prefer _handleKeyEvent, I think.

>       <handler event="DOMWindowClose" phase="capturing">

>+          this.removeTab(this._getTabForContentWindow(event.target));
>+          event.preventDefault();

>       <handler event="DOMWillOpenModalDialog" phase="capturing">

>           // We're about to open a modal dialog, make sure the opening
>           // tab is brought to the front.
>+          this.selectedTab = this._getTabForContentWindow(event.target.top);

These will now fail if _getTabForContentWindow returns null, where they didn't before. That might be OK for DOMWillOpenModalDialog (because it uses .top), but I'm not sure that it's OK for DOMWindowClose. I'd be more comfortable with some null checks added.

Looks good otherwise, r=me with the setTabTitle removal explained (or undone).

Comment 14

[Dão Gottwald [:dao]]

(In reply to comment #13)
> >       <method name="enterTabbedMode">
> 
> >             if (XULBrowserWindow.isBusy) {
> >               this.mCurrentTab.setAttribute("busy", "true");
> >               this.mIsBusy = true;
> >               this.setTabTitleLoading(this.mCurrentTab);
> >               this.updateIcon(this.mCurrentTab);
> >             } else {
> >-              this.setTabTitle(this.mCurrentTab);
> >               this.setIcon(this.mCurrentTab, this.mCurrentBrowser.mIconURL);
> >             }
> 
> Can you explain this removal?

The title should already be up to date, since DOMTitleChanged is handled globally rather than individually for each browser.

Comment 15

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/0c6e5108494e

(In reply to comment #13)
> These will now fail if _getTabForContentWindow returns null, where they didn't
> before. That might be OK for DOMWillOpenModalDialog (because it uses .top), but
> I'm not sure that it's OK for DOMWindowClose. I'd be more comfortable with some
> null checks added.

Added the null check for DOMWindowClose. Since it shouldn't be null in DOMWillOpenModalDialog and we'd want to know about it if it was, I didn't add the check there.