Closed Bug 695143 Opened 8 years ago Closed 6 years ago

provide way to associate high-level BrowserWindow/Tab objects with low-level equivalents

Categories

(Add-on SDK Graveyard :: General, enhancement, P2)

enhancement

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.
Severity: normal → enhancement
IIRC, xul tab elements do have unique ID attributes.
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.
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.
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);
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
Duplicate of this bug: 803375
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
Duplicate of this bug: 838430
Attachment #727371 - Flags: feedback?(rFobic)
Attachment #727371 - Flags: feedback?(rFobic)
Duplicate of this bug: 877694
(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?
(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.
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.
(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
(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;
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.
Assignee: nobody → evold
Attachment #767984 - Flags: review?(dtownsend+bugmail)
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 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 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-
Assignee: evold → nobody
Duplicate of this bug: 838429
Assignee: nobody → evold
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 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)
Assignee: evold → nobody
Attachment #8335660 - Flags: review?(zer0) → review+
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
Attachment #8335730 - Flags: review?(zer0) → review+
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
Is this fixed now?
Flags: needinfo?(rFobic)
This will be fixed once last patch I just attached is fixed.
Flags: needinfo?(rFobic)
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+
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.
Correction. This is the most voted issue - tied with one other
What's the status here Irakli?
Flags: needinfo?(rFobic)
(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: 6 years ago
Flags: needinfo?(rFobic)
Resolution: --- → FIXED
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?
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
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
Thanks again so much for the patch here. What version of FF should we look for it in?
Barring complications it will be in Firefox 31.
thanks!
Great, thanks!
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.