Skip to content
This repository has been archived by the owner on Feb 26, 2022. It is now read-only.

onUntrack should be optional for the WindowTracker constructor in the window-utils module #197

Merged
merged 2 commits into from
Jun 30, 2011
Merged

onUntrack should be optional for the WindowTracker constructor in the window-utils module #197

merged 2 commits into from
Jun 30, 2011

Conversation

erikvold
Copy link
Contributor

No description provided.

@Gozala
Copy link
Contributor

Gozala commented Jun 29, 2011

What is a use case for this ? When do you want to listen for onTrack without onUntrack ?

@erikvold
Copy link
Contributor Author

When I use third-party apis to handle unloaders, example:

https://github.com/erikvold/toolbarbutton-jplib/blob/master/lib/toolbarbutton.js#L123

@Gozala
Copy link
Contributor

Gozala commented Jun 30, 2011

To be honest I don't quite see why do you prefer listener on unload over onUntrack. Also have you seen
api-utils/windows/observer module ? You can see how to us it in api-utils/tabs/observer module. I'd recommend you to use this instead as it's going to replace WindowTracker eventually.

The change itself looks ok, but then one might expect to make onTrack optional as well to listen for onUntrack only.

Let me know if you still want it even though there is windows/observer.

@erikvold
Copy link
Contributor Author

To be honest I don't quite see why do you prefer listener on unload over onUntrack.

If I use onUntrack then I would need to store an array of windows or windowIDs along with arrays of toolbar buttons used on the window, and event listeners used on the window, so that I could remove them when onUntrack is called, which takes longer to implement, seems more error prone, and harder to maintain. Also but I can't tell why onUntrack is called when it is called, which is helpful to know so that I don't do unnecessary work when a window is simply being closed. Lastly onUntrack is called from every window closed, and I have to write code that checks the location to make sure it's not a window that I care about.

I'll replace the standard unload listener with a window ID based implementation soon too.

Also have you seen api-utils/windows/observer module ? You can see how to us it in api-utils/tabs/observer module.

No I hadn't, I think I will write my own module though.

@Gozala
Copy link
Contributor

Gozala commented Jun 30, 2011

In any case change looks, so r+ on it and I'm pulling it in.

No I hadn't, I think I will write my own module though.

I would not recommend going with that path, let's better improve APIs that are in SDK if they are not good enough

Gozala added a commit that referenced this pull request Jun 30, 2011
onUntrack should be optional for the WindowTracker constructor in the window-utils module r=irakli
@Gozala Gozala merged commit a67b7ef into mozilla:master Jun 30, 2011
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants