Closed
Bug 402147
Opened 17 years ago
Closed 15 years ago
tabbrowser event handling cleanup
Categories
(Firefox :: Tabbed Browser, defect)
Firefox
Tabbed Browser
Tracking
()
RESOLVED
FIXED
Firefox 3.7a2
People
(Reporter: dao, Assigned: dao)
References
Details
Attachments
(1 file, 5 obsolete files)
17.61 KB,
patch
|
Gavin
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Attachment #287035 -
Flags: review?(gavin.sharp)
Comment 1•17 years ago
|
||
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•17 years ago
|
||
(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.
Assignee | ||
Comment 3•17 years ago
|
||
Hrm, ok ...
Attachment #287035 -
Attachment is obsolete: true
Attachment #287080 -
Flags: review?(gavin.sharp)
Attachment #287035 -
Flags: review?(gavin.sharp)
Assignee | ||
Updated•16 years ago
|
Assignee | ||
Comment 4•16 years ago
|
||
Attachment #287080 -
Attachment is obsolete: true
Attachment #387182 -
Flags: review?(gavin.sharp)
Attachment #287080 -
Flags: review?(gavin.sharp)
Comment 5•16 years ago
|
||
Just a heads up that another event listener could be introduced in bug 420605 which would need integrating with this.
Assignee | ||
Comment 6•15 years ago
|
||
Attachment #387182 -
Attachment is obsolete: true
Attachment #405711 -
Flags: review?(gavin.sharp)
Attachment #387182 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 7•15 years ago
|
||
missed a code sharing opportunity
Attachment #405711 -
Attachment is obsolete: true
Attachment #405759 -
Flags: review?(mano)
Attachment #405711 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 8•15 years ago
|
||
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•15 years ago
|
||
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.
Assignee | ||
Comment 10•15 years ago
|
||
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.
Assignee | ||
Updated•15 years ago
|
Attachment #405759 -
Flags: review?(vladimir)
Assignee | ||
Updated•15 years ago
|
Attachment #405759 -
Flags: review?(vladimir)
Attachment #405759 -
Flags: review?(mconnor)
Attachment #405759 -
Flags: review?(mano)
Comment 11•15 years ago
|
||
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)
Assignee | ||
Comment 12•15 years ago
|
||
Attachment #405759 -
Attachment is obsolete: true
Attachment #426890 -
Flags: review?(gavin.sharp)
Comment 13•15 years ago
|
||
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+
Assignee | ||
Comment 14•15 years ago
|
||
(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.
Assignee | ||
Comment 15•15 years ago
|
||
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.
Assignee | ||
Updated•15 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.7a2
Updated•14 years ago
|
Attachment #426890 -
Attachment description: patch v4 → patch v4
[Checked in: See comment 15]
You need to log in
before you can comment on or make changes to this bug.
Description
•