Closed
Bug 695143
Opened 12 years ago
Closed 10 years ago
provide way to associate high-level BrowserWindow/Tab objects with low-level equivalents
Categories
(Add-on SDK Graveyard :: General, enhancement, P2)
Add-on SDK Graveyard
General
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: myk, Unassigned)
References
Details
Attachments
(7 files)
We should provide a way to associate the high-level API objects BrowserWindow and Tab with their low-level (i.e. native) equivalents, for use cases that mix high- and low-level APIs. My first thought would be to give windows and tabs unique IDs (if they don't already have them), expose those IDs via BrowserWindow and Tab properties, and let you look up the native objects by ID via the WindowTracker and TabTracker objects exposed by the window-utils.js and tab-browser.js low-level modules, respectively. Since the IDs are useless without low-level module access, this seems safe. I'm not sure it's the optimal solution, however.
Reporter | ||
Updated•12 years ago
|
Severity: normal → enhancement
IIRC, xul tab elements do have unique ID attributes.
Reporter | ||
Comment 2•12 years ago
|
||
Alternately, give WindowTracker a secret key that it provides to high-level Window objects, which keep it secret. Then add a `getNativeForWindow` method to WindowTracker that takes a high-level Window object and returns a native object, retrieving it from the Window object by passing it the secret key in a valueOf call. Do the equivalent with TabTracker. That seems like the better option, as it doesn't require us to add a public property to the high-level objects whose only purpose is to be used in conjunction with low-level APIs.
Comment 3•12 years ago
|
||
It may be tricky to actually keep that secret: if someone hands their own object (not actually a Window) to WindowTracker.getNativeForWindow(), with a valueOf() method designed to capture the argument, a naive tracker will reveal the secret to it in the process of trying to prove knowledge of that secret. There are some other solutions floating around, but they aren't coming to me right now. I think one involves giving all of the high-level objects a reference to some (shared) internal mutable slot of the Tracker, and then getNativeForWindow() tells the supposed Window object to place a reference to the native object inside that slot. A real Window is both able and willing to put it there, a fake Window is unable to.
Comment 4•12 years ago
|
||
In latest shipped Firefox we do have WeakMap's that's a value to value map that gc can also claim, which sounds like perfect fit here: var map = new WeakMap; map.set(highLevelWindow, lowLevelWindow); // .... var lw = map.get(highLevelWindow);
Comment 5•12 years ago
|
||
oh, yeah, WeakMaps are the way to go. The hidden-slot trick is one of the workarounds for when you don't have WeakMaps.
Priority: -- → P2
Comment 7•11 years ago
|
||
This is was done for Fennec, it was defined two namespaces that can be used to obtain, from a SDK Tab reference, a XUL Tab reference. The same namespace is for Firefox as well but is not currently populated. We should fix this, because it will be useful in several context. For instance in Unit Test: https://github.com/ZER0/addon-sdk/blob/selection/bug803027/test/test-selection.js#L31-50 Or for create 3rd party module like a notificationbox: https://groups.google.com/d/topic/mozilla-labs-jetpack/QJtKUccLJcU/discussion
Comment 9•11 years ago
|
||
Pointer to Github pull-request
Updated•11 years ago
|
Attachment #727371 -
Flags: feedback?(rFobic)
Updated•11 years ago
|
Attachment #727371 -
Flags: feedback?(rFobic)
Comment 11•11 years ago
|
||
(In reply to Dave Townsend (:Mossop) from comment #9) > Created attachment 727371 [details] > Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/880 > > Pointer to Github pull-request is this ready to be reviewed?
Comment 12•11 years ago
|
||
(In reply to Erik Vold [:erikvold] [:ztatic] from comment #11) > (In reply to Dave Townsend (:Mossop) from comment #9) > > Created attachment 727371 [details] > > Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/880 > > > > Pointer to Github pull-request > > is this ready to be reviewed? No, it doesn't work due to issues with traits.
Comment 13•11 years ago
|
||
If don't want to block this bug until the de-traitification, we can probably use the current namespaces we have. For instance, we have already a method that returns the chrome window from a sdk window, it's just not exposed: https://github.com/mozilla/addon-sdk/blob/master/lib/sdk/windows/firefox.js#L258-L260 Similar story for tab (although they can be implemented by a simple loop with id check: https://github.com/mozilla/addon-sdk/blob/master/lib/sdk/tabs/tab-fennec.js#L8 Unfortunately this namespace is present only in fennec, should be in both. Also, it seems to me that the PR only implements this stuff for Firefox, what about Fennec? Notice also that for the UI Work I need this methods, I just used a temporary workaround waiting for the proper implementations – I guess we can also land them with that workaround, that's why I didn't set this bug as blocker yet.
Comment 14•11 years ago
|
||
(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #13) > If don't want to block this bug until the de-traitification, we can probably > use the current namespaces we have. For instance, we have already a method > that returns the chrome window from a sdk window, it's just not exposed: > > https://github.com/mozilla/addon-sdk/blob/master/lib/sdk/windows/firefox. > js#L258-L260 > > Similar story for tab (although they can be implemented by a simple loop > with id check: > > https://github.com/mozilla/addon-sdk/blob/master/lib/sdk/tabs/tab-fennec. > js#L8 > > Unfortunately this namespace is present only in fennec, should be in both. > > Also, it seems to me that the PR only implements this stuff for Firefox, > what about Fennec? > > Notice also that for the UI Work I need this methods, I just used a > temporary workaround waiting for the proper implementations – I guess we can > also land them with that workaround, that's why I didn't set this bug as > blocker yet. I say let's get these methods landed using whatever method works for now, we can always fix them to use something nicer later
Comment 15•11 years ago
|
||
(In reply to Dave Townsend (:Mossop) from comment #14) > I say let's get these methods landed using whatever method works for now, we > can always fix them to use something nicer later In that case, I post on the ML some temporary methods could be used. For the Tab, we can simply have something like this: const { getTabs, getTabId } = require('sdk/tabs/utils'); function getChromeTab(sdkTab) { for (let tab of getTabs()) if (sdkTab.id === getTabId(tab)) return tab; return null; } For the window, we could have: const { BrowserWindow } = require('sdk/windows'); const { windows } = require('sdk/window/utils'); function getChromeWindow(sdkWindow) { // to include private window use the as second argument // { includePrivate: true } for (let window of windows('navigator:browser')) if (BrowserWindow({window: window}) === sdkWindow) return window; return null; } For this method we should probably implement a better version; currently it seems we have a fennec implementation that is too different from firefox – we have two different `BrowserWindow` and only one is populating the `windowNS` namespace. We should start to remove code duplication and use only one `BrowserWindow` instead of two – probably, the fennec one should be used, it's more recent and its populating the namespace. If we want to have a quick implementation, we could just populate the `windowNS` (`sdk/window/namespace`) on the `BrowserWindow` for Firefox too. Once we do that, the `getChromeWindow` will be simply looks like: const { windowNS } = require('sdk/window/namespace'); function getChromeWindow(sdkWindow) windowNS(sdkWindow).window;
Comment 16•11 years ago
|
||
More I take a look to the windows code, and more I think we should uniform a bit the code between firefox and fennec: they basically do the same thing in a totally different way, when it's not necessary needed. For example, `windows/firefox` is using an internal `viewNS` instead of the `windowNS` used by `windows/fennec`. In my UX Work I now replace `viewNS` with `windowNS`, and I implemented the `getChromeWindow` (locally) as above. I think this is probably the quickest way at the moment to have the `getChromeTab` and `getChromeWindow` implemented.
Updated•11 years ago
|
Assignee: nobody → evold
Comment 17•11 years ago
|
||
Pointer to Github pull-request
Updated•11 years ago
|
Attachment #767984 -
Flags: review?(dtownsend+bugmail)
Comment 18•11 years ago
|
||
Comment on attachment 767984 [details] Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/1054 Looks good to me, would like to spin it past Irakli to make sure he is happy with where they're living
Attachment #767984 -
Flags: review?(rFobic)
Attachment #767984 -
Flags: review?(dtownsend+bugmail)
Attachment #767984 -
Flags: review+
Comment 19•11 years ago
|
||
Pointer to Github pull-request
Updated•11 years ago
|
Attachment #768057 -
Flags: review?(rFobic)
Comment 20•11 years ago
|
||
Comment on attachment 768057 [details] Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/1056 Erik thanks for taking this over, it looks good, just a small changes I'd like you to do before we can land.
Attachment #768057 -
Flags: review?(rFobic) → review-
Comment 21•11 years ago
|
||
Comment on attachment 767984 [details] Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/1054 Same as in other pull request. Also I think it would make sense to just have a `getDOMView` method that both tab and window will implement rather than have similar thing going on in two diff places.
Attachment #767984 -
Flags: review?(rFobic) → review-
Updated•11 years ago
|
Assignee: evold → nobody
Updated•10 years ago
|
Assignee: nobody → evold
Comment 23•10 years ago
|
||
Pointer to Github pull-request
Comment 24•10 years ago
|
||
Comment on attachment 791962 [details] Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/1199 Irakli I've made the API changes that you requested, the rest of what you requested I think should wait until we have de-traitified the window and tabs modules otherwise we'll delay this bug even longer and give ourselves more work for little gain in the end. Also I don't think having set(DOM|SDK|*)(Window|Tab) and unset(DOM|SDK|*)(Window|Tab) for every little internal is a good idea.
Attachment #791962 -
Flags: review?(rFobic)
Comment 25•10 years ago
|
||
Comment on attachment 791962 [details] Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/1199 Hey Erik I'm about to finish rewrite of tabs & windows in https://github.com/mozilla/addon-sdk/pull/1207 that unifies mobile & desktop implementations and also solves this problem. I would really prefer if we would hold this on for little longer and just land that patch instead of spending time on review and trying to make our approaches compatible. If you disagree please re-submit review request or needinfo me or speak up on IRC. Thanks & I'm sorry I have to put down your work here.
Attachment #791962 -
Flags: review?(rFobic)
Updated•10 years ago
|
Assignee: evold → nobody
Comment 26•10 years ago
|
||
Pointer to Github pull-request
Updated•10 years ago
|
Attachment #8335660 -
Flags: review?(zer0)
Updated•10 years ago
|
Attachment #8335660 -
Flags: review?(zer0) → review+
Comment 27•10 years ago
|
||
Commit pushed to master at https://github.com/mozilla/addon-sdk https://github.com/mozilla/addon-sdk/commit/c0d0f6b4b8e20906e3190039c864ec8a11f3d5c2 Merge pull request #1299 from Gozala/bug/getView@695143 Bug 695143 - Implement `getView` function for SDK tabs. r=@ZER0
Comment 28•10 years ago
|
||
Pointer to Github pull-request
Updated•10 years ago
|
Attachment #8335730 -
Flags: review?(zer0)
Updated•10 years ago
|
Attachment #8335730 -
Flags: review?(zer0) → review+
Comment 29•10 years ago
|
||
Commit pushed to master at https://github.com/mozilla/addon-sdk https://github.com/mozilla/addon-sdk/commit/7b5f80a40e79506293c58ba1c337c93be45423b2 Merge pull request #1301 from Gozala/bug/window-getView@695143 Bug 695143 - Implement viewFor(window) support. r=@ZER0
Comment 31•10 years ago
|
||
Pointer to Github pull-request
Updated•10 years ago
|
Attachment #8339756 -
Flags: review?(zer0)
Comment 32•10 years ago
|
||
This will be fixed once last patch I just attached is fixed.
Flags: needinfo?(rFobic)
Comment 33•10 years ago
|
||
Comment on attachment 8339756 [details] Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/1310 Looks good to me, r+, just minor nits (see the comments on PR)
Attachment #8339756 -
Flags: review?(zer0) → review+
Comment 34•10 years ago
|
||
This seemed like it was really super close, so I wanted to check in on the progress and see if we could get the pull request merged since there were only minor nits on the pull request.
Comment 35•10 years ago
|
||
This is now the 2nd most voted up issue. It would be really great to get this patch committed https://bugzilla.mozilla.org/buglist.cgi?component=General&list_id=9904038&product=Add-on%20SDK&query_format=advanced&resolution=---&query_based_on=&columnlist=product%2Ccomponent%2Cassigned_to%2Cbug_status%2Cresolution%2Cshort_desc%2Cchangeddate%2Cvotes
Comment 36•10 years ago
|
||
Correction. This is the most voted issue - tied with one other
Comment 38•10 years ago
|
||
(In reply to Dave Townsend [:mossop] from comment #37) > What's the status here Irakli? State is that we have implemented support for tabs and windows. I don't expect that we're going to implement this for widgets as we deprecated them. Story with new UI components is more complicated, as it's not one to one relation as one button maybe represented by `n` number of DOM nodes (one per window), although we still have API in place that would get you a node associated with high level API instance if for a specific browser window. I think we can close this issue.
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: needinfo?(rFobic)
Resolution: --- → FIXED
Comment 39•10 years ago
|
||
Thanks for the update Irakli. Did you get a chance to update the documentation? I'm really curious how to call the new code for windows and what version of FF it will be appearing in. You had an open pull request for this (https://github.com/mozilla/addon-sdk/pull/1310). Should we leave it open or should we close it as well as already fixed and implemented?
Comment 40•10 years ago
|
||
Commit pushed to master at https://github.com/mozilla/addon-sdk https://github.com/mozilla/addon-sdk/commit/ba6e7ea5243d3621aa2e81ebc4ac381100aae258 Merge pull request #1310 from Gozala/bug/modelFor@695143 Bug 695143 - Implement `modelFor(view)` for getting SDK models for underlaynig platform tab/window. r=@ZER0
Comment 41•10 years ago
|
||
Commit pushed to master at https://github.com/mozilla/addon-sdk https://github.com/mozilla/addon-sdk/commit/98b249183546380ccbaf781644a89ba40506fe74 Merge pull request #1465 from Gozala/bug/modelFor@695143 Bug 695143 - Fix failing tests introduced by ba6e7ea r=@Mossop
Comment 42•10 years ago
|
||
Thanks again so much for the patch here. What version of FF should we look for it in?
Comment 43•10 years ago
|
||
Barring complications it will be in Firefox 31.
Comment 44•10 years ago
|
||
thanks!
Comment 45•10 years ago
|
||
Ben, there are now some simple docs here: https://developer.mozilla.org/en-US/Add-ons/SDK/High-Level_APIs/windows#Converting_to_DOM_windows https://developer.mozilla.org/en-US/Add-ons/SDK/High-Level_APIs/tabs#Converting_to_XUL_tabs Hope it's clear, just ask if it isn't.
Comment 46•10 years ago
|
||
Great, thanks!
Comment 47•9 years ago
|
||
I've been trying these new methods and they're pretty nifty, except for one thing... sdk/windows is still unable to listen for the opening of anything other than BrowserWindows, and on its part, window/utils apparently cannot listen for anything period. This means that even with these methods, it's impossible for certain use-cases to prescind deprecated/window-utils (WindowTracker) For example, I'm making a SDK-addon, I use XUL template for many dialogs, open them as such with window.openDialog and listen for their "open" event to populate them and add relevant data from within my sdk code. This is in-fact not possible currently without WindowTracker. I was actually attempting to make a new version of my code to check for FireFox version and, if > 31, use sdk/windows and window/utils instead of deprecated/window-utils. However, I have noticed that the non-deprecaed apis are just not up to this task...
You need to log in
before you can comment on or make changes to this bug.
Description
•