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

Status: RESOLVED FIXED

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

Description

[Myk Melez [:myk] [@mykmelez]]

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.

Comment 1

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

IIRC, xul tab elements do have unique ID attributes.

Comment 2

[Myk Melez [:myk] [@mykmelez]]

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

[Brian Warner [:warner :bwarner]]

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

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

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

[Brian Warner [:warner :bwarner]]

oh, yeah, WeakMaps are the way to go. The hidden-slot trick is one of the workarounds for when you don't have WeakMaps.

Comment 7

[Matteo Ferretti [:zer0] [:matteo]]

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

[Dave Townsend [:mossop]]

Attachment: Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/880

Pointer to Github pull-request

Comment 11

[Erik Vold [:erikvold] (please needinfo? me)]

(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

[Dave Townsend [:mossop]]

(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

[Matteo Ferretti [:zer0] [:matteo]]

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

[Dave Townsend [:mossop]]

(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

[Matteo Ferretti [:zer0] [:matteo]]

(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

[Matteo Ferretti [:zer0] [:matteo]]

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.

Comment 17

[Erik Vold [:erikvold] (please needinfo? me)]

Attachment: Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/1054

Pointer to Github pull-request

Comment 18

[Dave Townsend [:mossop]]

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

Comment 19

[Erik Vold [:erikvold] (please needinfo? me)]

Attachment: Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/1056

Pointer to Github pull-request

Comment 20

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

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.

Comment 21

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

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.

Comment 23

[Erik Vold [:erikvold] (please needinfo? me)]

Attachment: Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/1199

Pointer to Github pull-request

Comment 24

[Erik Vold [:erikvold] (please needinfo? me)]

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.

Comment 25

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

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.

Comment 26

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Attachment: Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/1299/files

Pointer to Github pull-request

Comment 27

[[github robot]]

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

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Attachment: Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/1301

Pointer to Github pull-request

Comment 29

[[github robot]]

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 30

[Dave Townsend [:mossop]]

Is this fixed now?

Comment 31

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Attachment: Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/1310

Pointer to Github pull-request

Comment 32

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

This will be fixed once last patch I just attached is fixed.

Comment 33

[Matteo Ferretti [:zer0] [:matteo]]

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)

Comment 34

[Ben McCann]

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

[Ben McCann]

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

[Ben McCann]

Correction. This is the most voted issue - tied with one other

Comment 37

[Dave Townsend [:mossop]]

What's the status here Irakli?

Comment 38

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

(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.

Comment 39

[Ben McCann]

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

[[github robot]]

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

[[github robot]]

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

[Ben McCann]

Thanks again so much for the patch here. What version of FF should we look for it in?

Comment 43

[Dave Townsend [:mossop]]

Barring complications it will be in Firefox 31.

Comment 44

[Ben McCann]

thanks!

Comment 45

[Will Bamberg [:wbamberg]]

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

[Ben McCann]

Great, thanks!

Comment 47

[g.litenstein]

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...