Closed Bug 402147 Opened 14 years ago Closed 12 years ago

tabbrowser event handling cleanup

Categories

(Firefox :: Tabbed Browser, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 3.7a2

People

(Reporter: dao, Assigned: dao)

References

Details

Attachments

(1 file, 5 obsolete files)

Attached patch patch (obsolete) — Splinter Review
No description provided.
Attachment #287035 - Flags: review?(gavin.sharp)
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?
(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.
Attached patch patch v2 (obsolete) — Splinter Review
Hrm, ok ...
Attachment #287035 - Attachment is obsolete: true
Attachment #287080 - Flags: review?(gavin.sharp)
Attachment #287035 - Flags: review?(gavin.sharp)
Depends on: 473745, 489628
Depends on: 502574
Attached patch updated to trunk (obsolete) — Splinter Review
Attachment #287080 - Attachment is obsolete: true
Attachment #387182 - Flags: review?(gavin.sharp)
Attachment #287080 - Flags: review?(gavin.sharp)
Just a heads up that another event listener could be introduced in bug 420605 which would need integrating with this.
Attached patch patch v3 (obsolete) — Splinter Review
Attachment #387182 - Attachment is obsolete: true
Attachment #405711 - Flags: review?(gavin.sharp)
Attachment #387182 - Flags: review?(gavin.sharp)
Attached patch patch v4 (obsolete) — Splinter Review
missed a code sharing opportunity
Attachment #405711 - Attachment is obsolete: true
Attachment #405759 - Flags: review?(mano)
Attachment #405711 - Flags: review?(gavin.sharp)
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.
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.
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.
Attachment #405759 - Flags: review?(vladimir)
Attachment #405759 - Flags: review?(vladimir)
Attachment #405759 - Flags: review?(mconnor)
Attachment #405759 - Flags: review?(mano)
Comment on attachment 405759 [details] [diff] [review]
patch v4

I am no longer actively working on Firefox, please find another reviewer.
Attachment #405759 - Flags: review?(mconnor)
Attachment #405759 - Attachment is obsolete: true
Attachment #426890 - Flags: review?(gavin.sharp)
Blocks: 347930
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).
Attachment #426890 - Flags: review?(gavin.sharp) → review+
(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.
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.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.7a2
Attachment #426890 - Attachment description: patch v4 → patch v4 [Checked in: See comment 15]
Blocks: 504870
You need to log in before you can comment on or make changes to this bug.