var grandchild is undefined

[ahocevar]

The following fails:

<description><![CDATA[...]]>
</description>

Also:

<description>
<![CDATA[...]]></description>

The following works:

<description>
<![CDATA[...]]>
</description>

Also:

<description><![CDATA[...]]></description>

The proposed patch should fix the issue. Tests still pass in FF3.5. Please review

[bartvde]

Looks good Andreas, please commit. I can confirm tests pass in IE6 as well.

[ahocevar]

(In [10138]) fixed KML parsing for cases where a line break is only before or after a CDATA block. r=bartvde (closes #2398)

[Zed]

Code now breaks at line 905 - KML data fails to display:

if(grandchild.nodeType == 3 grandchild.nodeType == 4)

fails with var "grandchild" is "undefined". Var grandchild is declared twice (along with a few other variables) in the code, which may be the problem? Line 900 also has a redundant fall through to the default statement. It may also be good to change some of the comparisions from == to === and != to !==

I would change the code but I don't feel fully qualified enough to do so.

[bartvde]

Zed, do you have an example page to reproduce your problem? Also which browser are you using?

[Zed]

firefox 3.6.2 with firebug and IE 7.0.5730.11 (both fail)

This page is an example:  http://www.bigyak.net.au/dev/bikeflows.htm

You need to be centred on Melbourne or Sydney, Australia (it should default to Melbourne). Turn off the overlay labelled "Edit Layer" before clicking on map for any reason, as clicks are designed to do different stuff that may confuse. The overlays labeled "Empty Layer: x" should say something else when bug is fixed.

Let me know if I can help further.

[elemoine]

I'm wondering whether

if(grandchildren.length >= 1 || grandchildren.length <= 3) {

shouldn't be:

if(grandchildren.length >= 1 && grandchildren.length <= 3) {

?

[bartvde]

I remember at one point in the review I had the same thought.

[ahocevar]

Replying to bartvde:

I remember at one point in the review I had the same thought.

I guess you are both right. Must have missed that.

[ahocevar]

With the above patch, Zed's data will be parsed in Safari, but Firefox chokes on the too long CDATA blocks. Anyway, the patch fixes the regression. Please review.

[ahocevar]

Oh, after uploading the patch in Safari I see that Firefox has finished parsing the KML and it also works. Just takes ages.

[bartvde]

Looks good, please commit in trunk and I will pull this up to the 2.9 branch later on.

[ahocevar]

r10200

[bartvde]

Pulled up in r10205

[bartvde]

Zed, are you able to confirm that this fix (r10205) works for you? Do I need to get you a new OpenLayers.js file for you to test, or do you build yourself? If you need a new build, please let me know your e-mail address.

[bartvde]

Zed, I've put up a new version of OpenLayers with this fix included on my website, can you please verify that the problem you reported has been fixed. TIA!

 http://www.osgis.nl/openlayers/OpenLayers.js

[Zed]

No - I can't test it, although the fix makes sense. Please send: ancestors .. bigyak"net'au and I'll try it. Thanks for your work.

[Zed]

OK - using script on your server - problem appears to be fixed - great. To review you need to be looking at Sydney, Australia - see pull down list at bottom left of screen.

 http://www.bigyak.net.au/dev/bikeflows.html

However the layer switcher and the attribution and lat/lon strings are all in the top left of the screen now (FireFox & IE)! Which is perhaps a new bug (nothing do to do with the KML) or the update requires CSS changes?

It doesn't happen at this url (url has no trailing l as seen in the above url)

 http://www.bigyak.net.au/dev/bikeflows.htm

which is using the release candidate:

http://openlayers.org/api/2.9-rc2/OpenLayers.js

Any ideas?

[bartvde]

Hi Zed, that's a different issue, because I don't have the OpenLayers style css files on my webserver. So it is a configuration issue, nothing to worry about. Thanks for your test and report back.