support GetMap-Request via HTTP-POST

[ingo]

I want to make GetMap-Requests possible via HTTP-POST. This would allow longer filter and sld in such a request (at the moment the length is limited to the URL length).

To implement this feature, the GetMap-Request is sent through a <form> element with an iframe as target. Each tile is displayed in an own iframe. To support mouse events, I will have to put a <div> over the iframe, because mouse events in an iframe aren't triggered to the outer html.

OpenLayers.WMS will get a config option for the request-method (GET or POST). Depending on the request method of the WMS-layer, the GetMap-request will instantiate Tile.Image or the new class, creating iframes.

[bartvde]

Hi, have you looked into the following approach?

http://openlayers.org/pipermail/dev/2007-November/001932.html

[ingo]

The problem of loading images via the XmlHttpRequest-object is 'same origin policy'. The GetMap-request would need a proxy like WFS.

[crschmidt]

Due to the significant differences between this and the standard WMS layer, I would recommend making this a layer subclass, rathr than complicating layer.wms with an option.

Additionally, I'll advise that this seems likely to be difficult to make work, but perhaps I'm overly cautious in this regard.

Best of luck.

[tschaub]

Yeah, I agree that a new layer type would probably be easier all around - particularly if this introduces any browser specific oddities we can't anticipate now. The WMS layer is a staple of many applications and I think we should be cautious messing with it.

I'll be interested to see how this goes.

[ingo]

I added a patch with the implementation of the described feature and UnitTests. One Problem occured while developing that patch: Opera seems not to be able to show transparent images in dynamically created iFrames. So, if you try to create a transparent OpenLayers.Layer.WMS.Post in Opera, you will get an OpenLayers.Layer.WMS. Opera supports a nearly unlimited length of characters in an URL, so there won't be a problem when sending large data via HTTP-GET.

[ingo]

adapted patch

[ingo]

After talking to ahocevar, who has reviewn my patch the last days, I made some changes. The new tile class now inherits from Tile.Image which avoids a lot of duplicated code. It got a new property OpenLayers.Tile.IFrame.layerAlphaHack to check, if the current browser is Opera.

OpenLayers.Layer.WMS.Post got a new property as well - 'tileClass', which is to check, if the user is running an Opera, and if the layer should be transparent. In that case, we have to create a Layer.WMS instead of Layer.WMS.Post. The OverlayDiv, which have been created only one time for all WMS.Post-layer in the first approach, works in the current patch similar to the EventPane used in Google-layers. Every WMS.Post-layer has its own OverlayDiv container, which handles mouse-events, and makes the hack in OpenLayers.Handler.MouseWheel needless.

[ahocevar]

added new scripts to OpenLayers.js; removed unused function from Util; made tileClass handling more efficient; removed id for form; removed moveend handler for pane; made createRequestForm more efficient

[ahocevar]

Thanks ingo for your efforts on this. This is great work!

I uploaded a new patch with a few changes:

• added the new scripts to OpenLayers.js.
• fixed include and inheritance comments in Tile/Image/IFrame.js.
• removed a leftover from Util.js.
• store the class in tileClass instead of a string.
• removed the id from the form. The id could have caused problems because it was not unique, and having to fetch the form with getElementById is unefficient.
• removed the unneeded moveend handler to reposition the pane.
• made the createRequestForm method more efficient by adding BBOX, WIDTH and HEIGHT to the params that get processed by the for loop.

There are still some issues with your patch:

• have you tried zooming a WMS.Post layer? For me this resulted in getting the same tiles repeated all over. I suspect this is either a caching issue or has to do with some non-unique id that I have overlooked (removing form.id did not help).
• at least in Firefox, dragging a map with WMS.Post layers causes unpleasant flicker of screen areas that have invisible tiles underneath. Maybe we have to live with that, but maybe the effect can be reduced by making tiles outside the visible extent either hidden or invisible.
• Without having investigated this in detail, I doubt that error handling ("pink tiles") works the same way as in Tile.Image.

Feel free to contact me on IRC if you have questions, and thanks again for the great work!

[ingo]

Thanks Andreas for your work on this patch.

Changes in this patch:

* clear issues mentioned above

* modify and improve the unit tests for both classes Layer.WMS.Post and Tile.Image.IFrame

* add a simple example, creating a WMS.Post-layer with a big inline sld, which won't work in IE6 with Layer.WMS

[ingo]

Adjusted text of the example

[ahocevar]

same as previous one, but with duplicated test files removed

[ahocevar]

No thorough review yet, but I tried to set transitionEffect: "resize" on the Post layer in the example, and I got an error. But I think instead of fixing this error, Layer.WMS.Post should have SUPPORTED_TRANSITIONS = [], because I am not aware of a way to stretch the content of an iframe from a different origin.

Anyway, this patch is great work. Thanks Ingo for the efforts. I'll continue my review and post comments here.

[ahocevar]

Here is the rest of my review:

• OL coding style requires to always use a block with curly braces after an if statement. I think there may be some more, but Tile.Image.IFrame::positionImage (L108-109) is one example.
• in Tile.Image.IFrame::clone, the last statement (setting obj.imgDiv to null) is redundant, because this is already done in the superclass.
• in Tile.Image.IFrame::initImgDiv, the checks for this.isBackBuffer can be removed when setting zIndex.
• in tests/Tile/Image/IFrame.html, L247, the function should be named test_Tile_Image_IFrame_hide (it is _show, overwriting the _show function above)
• The example tells about a "Single WMS", but you are actually using two WMS. Please change the wording, and you could also use Layer.WMS instead of Layer.WMS.Post for the 1st WMS, because that might distract users from what is shown in the example. Or just remove the 1st WMS.

But apart from these minor things, this is great work, and it has no negative impact on existing OpenLayers functionality, because it does not modify existing code.

Because I mentored Ingo a bit when he made this contribution, I would like to have another pair of eyes review this patch before it goes in.

[ingo]

removed existing issues regarding needless code and coding style

[ingo]

Thanks Andreas for your first review and mentoring this contribution.

I attached a new patch regarding following things you mentioned above:

• removing needless code
• improving the code regarding the coding style
• setting the SUPPORTED_TRANSITIONS = [] in Layer.WMS.Post, because it is not possible to resize the content (the image) of the IFrames. So, the transition demo works again with that empty array.
• Renamed test function test_Tile_Image_IFrame_show to function test_Tile_Image_IFrame_hide
• changed text in WMSPost example

The patch should be ready for being reviewed now, I think.

[ahocevar]

Simplified Tile.Image.IFrame

[ahocevar]

Thanks for your latest patch Ingo! I could not resist to further simplify Tile.Image.IFrame (and modify the tests to reflect the changes). tile.frame and tile.imgDiv now have the same position in the DOM as with the superclass (should make the code more clear), and inside tile.imgDiv I created the iFrame and an eventPane div. The result is a more reader-friendly code, and more than 50 lines of code less.

Ingo, please report if these changes work for you. Thanks again for your great work!

Other developers: Again, one more (at least quick) review would be in order before this goes in. I think this is good now to commit.

[elemoine]

This is indeed great work! My review:

• Tile/Image/Iframe.js, createRequestForm(): I'm not a big fan of the way the image URL is obtained. I think the Layer.HttpRequest::getFullRequestString method could be slightly changed so it doesn't always take the layer parameters into account. Something like that:

   getFullRequestString(newParams, altUrl, noLayerParams) {
       ...
       var allParams = {};
       if(!noLayerParams) {
           allParams = OpenLayers.Util.extend(allParams, this.params);
       }
       ...
    },

• Tile/Image/Iframe.js, destroy(), I don't think this one should be marked as an API method, as it should not be called from outside the library.
• OpenLayers.Tile.IFrame must replaced by OpenLayers.Tile.Image.IFrame in the docs, the mistake occurs multiple times, both in Tile/Image/Iframe.js and Layer/WMS/Post.js

The tests pass in FF3, Chromium, and IE8. The example works in FF3, Chromium, and IE8, but I've noticed these two issues:

• the browser's back button doesn't work as expected anymore
• the browser's viewport shakes when panning the layer in FF3

Sorry, I have no time for investigating this at the moment. And I don't quite know if the above issues are show stoppers, what others think?

[ingo]

Thanks for your review!

I think the two issues could be solved pretty easy.

• to avoid the back-button problem, the Tile.Image.IFrame::clear method should remove its iframe, so that it doesn't get any new content and the history will not be changed.
• Layer.WMS.Post gets a property that makes it possible to use Tile.Image tiles instead of Tile.Image.IFrame, because Firefox is able to handle GET-requests with 'nearly unlimited' length.

I will work on this.

[ingo]

The attached patch fixes following issues:

• back-button issue, by removing tile's iframe in Tile.Image.IFrame::clear method
• viewport-shaking, the user is able to set a property, which has the effect, that the layer instantiates Tile.Image instead of Tile.Image.IFrame tiles
• marked Tile.Image.IFrame::destroy as method, not as API method
• Renamed OpenLayers.Tile.IFrame to OpenLayers.Tile.Image.IFrame in different places

[ingo]

Removed issues discovered by a review from elemoine

[elemoine]

I can't review this today but I'm happy to do it on monday.

[ingo]

Changed name of a property, added new method for creating the request url

[ingo]

In the attached patch, I have changed the name of the property, which is used to avoid the ugly effect of viewport-shaking in some Firefox browsers. Furthermore, I have added a new method to create the request-url, which does not take the layer parameters into account.

[ingo]

After Andreas having reviewed the patch, I changed a few things.

• Handling problems in Opera & Firefox changed. I removed the two properties for telling OpenLayers, that Opera and/or Firefox should instantiate Tile.Image tiles and added a new array property where the user can insert the browser which should instantiate Tile.Image tiles. This makes the configuration easier for users and saved lines of code.

• The request-url is created with the method OpenLayers.Util.urlAppend which have been added by Andreas in Ticket #2297. This means, that the patch, I have added depends on his patch.

[ingo]

Depends on Ticket #2297 (removed some small bugs)

[ahocevar]

(In [9734]) added Layer.WMS.Post for WMS request params exceeding the maximum url length for GET requests. Thanks ingo for this excellent work! It is an honor to commit such a high quality patch. p=ingo, r=elemoine,me (closes #2224)