Closed Bug 470163 Opened 11 years ago Closed 11 years ago

FUEL: pass BrowserTab object as event data for Tab* events

Categories

(Firefox :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: mfinkle, Assigned: mfinkle)

References

Details

(Keywords: fixed1.9.1)

Attachments

(1 file)

When redirecting Tab* events, FUEL currently passes an empty string for the event data. Passing the BrowserTab corresponding to the target of the event is preferable.

http://mxr.mozilla.org/mozilla-central/source/browser/fuel/src/fuelApplication.js#142
ubiquity could use this. currently there's no way to (for example) get the newly opened tab from a TabOpen event.
OS: Mac OS X → All
Hardware: PC → All
Attached patch patchSplinter Review
This patch sends a BrowserTab object with the "Tab*" events of the Window object and the "load" event of the BrowserTab object.

Tests included. Also, adds some missing methods to the nsIWebProgressListener impl - errors were showing up in the console.
Assignee: nobody → mark.finkle
Attachment #353703 - Flags: review?(gavin.sharp)
Flags: wanted-firefox3.1?
Attachment #353703 - Flags: review?(gavin.sharp) → review+
Comment on attachment 353703 [details] [diff] [review]
patch

>diff --git a/browser/fuel/src/fuelApplication.js b/browser/fuel/src/fuelApplication.js

>+    this._events.dispatch(aEvent.type, new BrowserTab(this._window, aEvent.originalTarget.linkedBrowser));

Technically you should be using tabbrowser.getBrowserForTab, I think, but I bet we couldn't get away with changing how the browser is stored at this point anyways, so probably doesn't matter.

>diff --git a/browser/fuel/test/browser_Browser.js b/browser/fuel/test/browser_Browser.js

Perhaps worth adding a test that the tab object's "window" is correct, for the TabOpen case?
(In reply to comment #3)
> (From update of attachment 353703 [details] [diff] [review])
> >diff --git a/browser/fuel/src/fuelApplication.js b/browser/fuel/src/fuelApplication.js
> 
> >+    this._events.dispatch(aEvent.type, new BrowserTab(this._window, aEvent.originalTarget.linkedBrowser));
> 
> Technically you should be using tabbrowser.getBrowserForTab, I think, but I bet
> we couldn't get away with changing how the browser is stored at this point
> anyways, so probably doesn't matter.

True, but we do that in more places, so a new "cleanup" bug might be best. I'll file a bug.

> Perhaps worth adding a test that the tab object's "window" is correct, for the
> TabOpen case?

The objects returned from Application.activeWindow and gPageA(B).window are not the same object as the event.data.window and I don't have a handy alternative to use as a test, like I do with BrowserTab.uri.spec
http://hg.mozilla.org/mozilla-central/rev/09ac7c3e27e7
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Beltzner, this could be a nice feature for Ubiquity users. No public API is changed by the patch. The only public change is the data passed to an event handler now has a reference to the BrowserTab object that spawned the event, instead of null.
Just ask for approval on the patch?
Attachment #353703 - Flags: approval1.9.1?
Attachment #353703 - Flags: approval1.9.1? → approval1.9.1+
Flags: wanted-firefox3.5?
You need to log in before you can comment on or make changes to this bug.