callback for close button on popup

just a suggestion for making the patch a little simpler

Thanks euzuro for the patch. I cooked up a new patch based on euzuro's with an example and tests included. Tests pass on FF, except BaseTypes/test_Element.html and Control/test_PanZoom.html which actually also fail without my patch! Did not run test on IE.

new patch with an example and tests

slightly modified version of eric's latest patch

your new patch is great, eric. the only thing i'd want to change is the overriding of the OpenLayers.Function.bindAsEventListener. I see that you're doing that so that you can test if the observer is the same. The only problem is that maybe overriding 'bindAsEventListener()' will cause problems in other tests in other parts of the code.

Generally, we try to avoid overriding basic functionality in the tests, but when we do, we always make sure to re-assign the value at the end. So in this case we'd do something like this:

var tempBind = OpenLayers.Function.bindAsEventListener;
OpenLayers.Function.bindAsEventListener = function(func, obj) { return func };

and then at the end of the test function:

OpenLayers.Function.bindAsEventListener = tempBind;

In this case, however, I dont think that it's strictly necessary to override the function. I believe we acheive the same test by simply *calling* the observer, and then in the observer function's code adding a t.ok(true). If it doesn't get called, we won't have the right number of tests passing. This is a technique I have used in the past.

Additionally, I removed the map creation/ addpopup call as all we really need to do is take the 2nd child of the popup's groupDiv. This makes the tests' overhead a little less... and since they're getting pretty damn slow these days, I think we should keep an eye out for ways to make them run faster.

Let me know what you think of the patch. Like anything, it's only a suggestion on my part.

note that my patch passes all tests in ff and ie6... (and for the record, so did eric's... the ones that were failing in ff must have been a timing thing or something)

Replying to euzuro:

your new patch is great, eric. the only thing i'd want to change is the overriding of the OpenLayers.Function.bindAsEventListener. I see that you're doing that so that you can test if the observer is the same. The only problem is that maybe overriding 'bindAsEventListener()' will cause problems in other tests in other parts of the code. Generally, we try to avoid overriding basic functionality in the tests, but when we do, we always make sure to re-assign the value at the end. So in this case we'd do something like this:

var tempBind = OpenLayers.Function.bindAsEventListener;
OpenLayers.Function.bindAsEventListener = function(func, obj) { return func };

and then at the end of the test function:

OpenLayers.Function.bindAsEventListener = tempBind;

Good catch Erik, I indeed forgot to do that.

In this case, however, I dont think that it's strictly necessary to override the function. I believe we acheive the same test by simply *calling* the observer, and then in the observer function's code adding a t.ok(true). If it doesn't get called, we won't have the right number of tests passing. This is a technique I have used in the past. Additionally, I removed the map creation/ addpopup call as all we really need to do is take the 2nd child of the popup's groupDiv. This makes the tests' overhead a little less... and since they're getting pretty damn slow these days, I think we should keep an eye out for ways to make them run faster. Let me know what you think of the patch. Like anything, it's only a suggestion on my part.

I think your patch is great. I agree with all the changes your're proposing.

We can plan to commit this for 2.6.

-- Eric

ok we are both in agreement with this final patch, and all tests are passing. please commit!

(In [4916]) callback for close button on popup (closes #1061)