Ticket #1565 (closed feature: fixed)

Opened 5 years ago

Last modified 5 years ago

add cross-browser XMLHttpRequest and convenience methods

Reported by: tschaub Owned by:
Priority: minor Milestone: 2.7 Release
Component: Ajax Version: 2.6
Keywords: Cc:
State: Complete

Description

As described in the proposal page?, we can improve how we do XMLHttpRequests. This change deprecates OpenLayers.Ajax.Request and provides an alternative to OpenLayers.loadURL (though the same can still be used).

Attachments

request.patch Download (47.8 KB) - added by tschaub 5 years ago.
add cross-browser XMLHttpRequest and convenience methods

Change History

  Changed 5 years ago by tschaub

  • state set to Review

This change depends on and includes modifications in the patch for #1564. That issue should be closed first.

All tests pass in FF and examples work. I left a few examples using loadURL to show they still work - though the method doesn't need to be used.

follow-up: ↓ 3   Changed 5 years ago by elemoine

I'm in favor for this going into trunk at some point.

Have no time to do a full review right now but here are preliminary questions:

Preliminary question: is the goal to have OpenLayers.Request.XMLHttpRequest just wrap the  http://code.google.com/p/xmlhttprequest/ code and not modify that code? (so that we can just copy&paste upstream code should that upstream code change)

OpenLayers/Request/XMLHttpRequest.js re-defines window.Function.prototype.apply to deal with IE 5.0 not having window.Function.prototype.apply. Do we want this kind of redefinition in OL?

Thank you Tim.

in reply to: ↑ 2   Changed 5 years ago by tschaub

Replying to elemoine:

I'm in favor for this going into trunk at some point. Have no time to do a full review right now but here are preliminary questions: Preliminary question: is the goal to have OpenLayers.Request.XMLHttpRequest just wrap the  http://code.google.com/p/xmlhttprequest/ code and not modify that code? (so that we can just copy&paste upstream code should that upstream code change)

Yeah, I'd like to change as little of the XMLHttpRequest code as possible. Right now, it's just the "export" line that I've modified (to get XMLHttpRequest in the OpenLayers.Request namespace).

I would have grabbed the latest version, but I was left with a bit of license confusion. The main page ( http://code.google.com/p/xmlhttprequest/) links to LGPL, but the source in the zip archive says "You should have received a copy of the GNU General Public License

along with this program" (and the archive doesn't contain the GPL or LGPL license detail).

So, since the old version was Apache 2.0 license, I left that as it was.

In general, I think it makes sense to take advantage of small, single-purpose, well written libraries. I'd rather have us responsible for tracking upgrades to this one small script - instead of trying to merge changes from a larger frameworky thing.

OpenLayers/Request/XMLHttpRequest.js re-defines window.Function.prototype.apply to deal with IE 5.0 not having window.Function.prototype.apply. Do we want this kind of redefinition in OL?

My vote is that this is an acceptable thing to allow in OL. If we get to 3.0 and ditch the old prototype extensions, this would leave us with one extension to native prototypes - and most importantly, it is a standard ECMA method and it is only added where it doesn't already exist. That I can swallow.

Extending prototypes with names that sound good to one author is one thing. Extending ECMA standard names to browsers that don't support them is another.

Thank you Tim.

Thanks for taking a look at it. I'll report on IE tests sometime soon.

Changed 5 years ago by tschaub

add cross-browser XMLHttpRequest and convenience methods

  Changed 5 years ago by tschaub

All tests pass in IE7 (with the fix for #1571).

  Changed 5 years ago by tschaub

All tests pass in IE6 (with the fix for #1571).

  Changed 5 years ago by tschaub

All relevant tests pass Opera (marker layers setOpacity fails and I got bored of waiting for the tests after the scale line control - perhaps one of them hangs opera).

  Changed 5 years ago by tschaub

All tests pass in Safari as well.

  Changed 5 years ago by elemoine

As discussed on IRC, let's add a ND comment above issue(), GET() and friends indicating that the user-provided config object will be modified, so the user shouldn't reuse it.

  Changed 5 years ago by elemoine

  • state changed from Review to Commit

  Changed 5 years ago by tschaub

  • status changed from new to closed
  • state changed from Commit to Complete
  • resolution set to fixed

(In [7335]) Adding cross-browser XMLHttpRequest functionality and convenience methods around it in the OpenLayers.Request namespace. Deprecating OpenLayers.Ajax.Request. Full support sync/async requests using all HTTP verbs now. r=elemoine (closes #1565)

  Changed 5 years ago by tschaub

(In [7340]) Including the change in tests that satisfied IE. (see #1565)

Note: See TracTickets for help on using tickets.