Ajax handler update

We need to reconcile OpenLayers.Util.getParameters with the prototype getParametersObject -- possibly getParameters should just use it, but something needs to be done.

Once again, I realize that I don't know the API enough.

The reason I wrote the OpenLayers.Util.getParametersObject was to handle the toQueryParams() String prototype method of prototypejs. But I didn't need it because OpenLayers.Util.getParameters does exactly the job I would use it for.

New patch now affects only the Ajax.js code.

The register method is defined twice here (instead of unregister I'm guessing). In the second register (should be unregister?), you call OpenLayers.Util.removeItem with one argument. If this argument were an array, all of its undefined items would be spliced out. Is this the intention?

Good catch. Method is "unregister", and arguments were wrong. I was probably a distracted.

Attached patch fixes this.

In the OpenLayers.Ajax.Base class, there's a ND comment for method setOptions, while this method doesn't exist.

Without this patch I often get exceptions when trying to abort Ajax requests. With this patch, aborting requests works great! For that reason, I'm in favor for this patch.

However, knowing close to nothing about prototypejs, the review of this patch is a tough exercise.

Not knowing too much about this either: does this patch also provide an option for a request to be cancelled?

Replying to elemoine:

In the OpenLayers.Ajax.Base class, there's a ND comment for method setOptions, while this method doesn't exist.

You are right, the setOptions method is replaced by the initialize method (constructor).

Without this patch I often get exceptions when trying to abort Ajax requests. With this patch, aborting requests works great! For that reason, I'm in favor for this patch. However, knowing close to nothing about prototypejs, the review of this patch is a tough exercise.

You could have compared the code with the code in prototypejs 1.6.0. It is very similar.

Replying to rdewit:

Not knowing too much about this either: does this patch also provide an option for a request to be cancelled?

This was one of the main purpose of this patch. Though you could do it with the old code, it should now behave in a more proper way.

You just have to call : request.transport.abort();

All looks good. I don't want to hold you guys up if this is blocking something - but it would be awfully nice to have at least one manual acceptance test that did something with this code. I set up tests/manual as a place for things like this. Would it be possible to get an page in there that showed some of this code working?

I'll have a look though I don't have an example on how to build those acceptance tests. I would be pleased if you can point me to some.

One other note about this, it seems like the problem of prototypejs updates has already been raised in the past : http://trac.openlayers.org/ticket/829.

pgiraud and I just discussed this possible acceptance test for the Ajax patch. It seems to us that it'd be a bit complex to set up. Let's say we want an acceptance to check that aborting a request works. To actually check that abort succeeds one needs to be guaranteed that the request hasn't complete when abort() is actually triggered. How to get such a guarantee? I personnaly don't know. Any suggestion?

The only acceptance tests that I'm interested in are regression-based: specifically, "Does it do the same thing it did before?" Our normal tests should catch this implicitly, but I'd prefer to have an explicit "We can do a synchronous request and get the result back" kind of thing as well.

So, creating a simple HTML page that you can press a button on and it loads a file in the ../xml/ directory or something both synchronously and aynch, and that the behavior doesn't change with/without the patch, would be good in my eyes.

Make sure this also fix #1146

Replying to crschmidt:

The only acceptance tests that I'm interested in are regression-based: specifically, "Does it do the same thing it did before?" Our normal tests should catch this implicitly, but I'd prefer to have an explicit "We can do a synchronous request and get the result back" kind of thing as well. So, creating a simple HTML page that you can press a button on and it loads a file in the ../xml/ directory or something both synchronously and aynch, and that the behavior doesn't change with/without the patch, would be good in my eyes.

Yup, this is what I meant as well. A fancy test page could have a cancel button to see that in effect (with a largish file to load and a random query string). But at least just a synchronous test and an asynchronous test would be nice. Load button, and loading/loaded indicator.

Attached is a new patch including a new manual test. In this test, you'll find 3 buttons that checks for asynchronous, synchronous request and transport abortion.

Only one comment:

311	328	            if (this.options.asynchronous) { 
312	 	                setTimeout(OpenLayers.Function.bind( 
313	 	                    (function() {this.respondToReadyState(1);}),this), 10 
314	 	                ); 
 	329	                window.setTimeout( 
 	330	                    OpenLayers.Function.bind(this.respondToReadyState, this), 
 	331	                    1000); 
315	332	            } 
316	333	             

why the change from 10 -> 1000? is that coming directly from prototype?

From http://svn.rubyonrails.org/rails/spinoffs/prototype/trunk/src/ (ajax.js and base.js) it looks like they call respondToReadystate via function.defer(1). Function.prototype.defer is delay.curry(0.01) and Function.prototype.delay does a setTimeout arguments[0] * 1000. So basically, respondToReadyState gets called in 10 miliseconds with a single argument, 1 (this maps to "Loading" in the Events array).

So, since the third arg to window.setTimeout isn't cross browser, I think you need to bind the 1 to respondToReadyState.

I'll put in a patch and see what you think.

(It seems like Tim has been quicker)

Great catch, my mistake.

Here is what comes from prototype :

if (this.options.asynchronous) this.respondToReadyState.bind(this).defer(1);

And defer is written like following :

Function.prototype.defer = Function.prototype.delay.curry(0.01);

And delay is given an argument multiplied by 1000 to set the timeout.

Function.prototype.defer = Function.prototype.delay.curry(0.01);

I missed the 0.01 multiplication.

Tim is right, the '1' argument is obviously to be passed to respondToReadyState callback with a 10 ms delay. I may have been lost in the several links between methods in the prototypejs code.

Yeah, curry sets the first arg (0.01) and gives you a function that you can specify additional args to (it doesn't multiply).

Anyway, the ajax.patch addresses that issue.

If you watch respondToReadyState, you see that it gets: 1, 2, 3, 4. With the previous patch, it was getting 2, 3, 4, and after 1 second, -5.

The whole point of this is to get cross browser xhr. I've mentioned my distaste for this prototype.js code before. The fact that we've got all this stuff around that we have trouble interpreting bugs me. I know it didn't get any traction ("we've already hooked up our cart to their donkey" or some such), but if we want cross-browser xhr: http://code.google.com/p/xmlhttprequest/

If all we are relying on is loadUrl() and OL.Ajax.Request(), those aren't that hard to write by wrapping XMLHttpRequest.

Can this be discussed and worked on (if we have a volunteer) after the patch is committed ? Though, I agree with the fact that it shouldn't have any side effect.

ajax prototype.js style

(In [5535]) Serious rewrite in the Ajax namespace by pgiraud to keep up with prototype.js 1.6. Thanks for the effort and for suffering numerous revisions. Now, let's cross our fingers... r=crschmidt,me (closes #1170)

Thanks for dealing with this Pierre. I was just grumbling in that last comment because I'm uncomfortable not completely following the code. Anyway, I appreciate your efforts on this.