Add support for Google Maps v3 API

[ahocevar]

The attached patch introduces a version split for Layer.Google, and adds version 3 support. This works fine, except for panning, because we don't have the getDragObject method any more. I added a GMaps API feature request for that, see  http://code.google.com/p/gmaps-api-issues/issues/detail?id=2189.

The patch comes with an example and the same tests we have for v2. Tests pass in FF3.5. Please review

[ahocevar]

To be more precise: panning does work, but it looks a bit ugly when mouse-panning quickly.

[bartvde]

Feature so bumping to 2.10.

[ahocevar]

How to review openlayers-2493.2.patch :

• There are no functional changes to the current Layer.Google, but the code has been split up into Layer/Google.js and Layer/Google/v2.js. Layer/Google.js now only contains shared code that can also be used by Layer/Google/v3.js.
• Layer/Google.js now mixes in Layer/Google/v2.js or Layer/Google/v3.js, depending on the GMaps api version used. This is done in Layer.Google::initialize.
• The v3 API does not work without sphericalMercator set to true. This is because we don't have the getDragObject method any more (see  http://code.google.com/p/gmaps-api-issues/issues/detail?id=2189). So I decided to add a DEFAULTS constant, which sets sphericalMercator to true, along with all required projection, maxExtent and so on settings for v3. This is essentially the same that happens in Layer.OSM, and adds convenience.
• Fast dragging of the map feels a bit weird. This is because of  http://code.google.com/p/gmaps-api-issues/issues/detail?id=2346, which we are hoping to get fixed soon. Please star that issue, to raise its priority.

The patch comes with tests, and tests pass in Chromium 6, Firefox 3.6 and Internet Explorer 8.

[ahocevar]

adds default options, including sphericalMercator: true, because the v3 API does not work without it any more. Also pulls up recent changes from trunk.

[tschaub]

Good news:  http://code.google.com/p/gmaps-api-issues/issues/detail?id=2346#c7

[crschmidt]

Is there a timeline on the v2 going away? I think that tying GMaps v3 to OL v3 would be a reasonable thing to do if we can commit to a v3 of OL before v2 of GMaps is EOL.

That way, we can not worry about silly backwards compatibility concerns here.

(If people think OL v2 being a requirement for users will outlive GMaps v2, I suppose that it might be worth pulling this into 2.x.)

[pwr]

Google have now deprecated v2, so according to their terms  http://code.google.com/apis/maps/terms.html#section_4_4 "For a period of 3 years after an announcement (the “Deprecation Period”), Google will use commercially reasonable efforts to continue to operate the Deprecated Version of the Service and to respond to problems with the Deprecated Version of the Service deemed by Google in its discretion to be critical."

[ahocevar]

Incorporating google's fix right now.

[ahocevar]

panning fixed (thanks to google)

[ahocevar]

Updated patch to make use of non-animated panning. If people are not satisfied with the animated zoom, please vote for  http://code.google.com/p/gmaps-api-issues/issues/detail?id=2011

[tschaub]

Replying to crschmidt:

Is there a timeline on the v2 going away? I think that tying GMaps v3 to OL v3 would be a reasonable thing to do if we can commit to a v3 of OL before v2 of GMaps is EOL. That way, we can not worry about silly backwards compatibility concerns here. (If people think OL v2 being a requirement for users will outlive GMaps v2, I suppose that it might be worth pulling this into 2.x.)

I think it is worth pulling anything into 2.x that we think would benefit from some exercise before 3.0. In this case, the v3 layer also brings a huge benefit in that clients don't have to maintain API keys.

Oh, perhaps I misread, are there backwards compatibility issues introduced here?

[tschaub]

This thing is fantastic!! It's the first I've tried it since the animated panning fix. For me, it's a huge benefit to not have to enter an API key. I also don't get animated zooming currently (google.maps.version reports "3.1.6", Mac OS X, Firefox 3.6.6).

I'll review more closely now, but unless there are any real backwards incompatibilities introduced, I don't see why this shouldn't go in immediately.

[tschaub]

A few small issues I see:

1. If anybody was extending the old Google layer (and calling methods on the prototype), they need to change their code.

2. If anybody calls layer.maxExtent.extend (or add), they are modifying the defaults on the mixin.

3. If anybody was building a single file build with just the Google layer, their build config would have to be modified to pull in v2.

4. IE6 has issues with the layer if the cache is empty (my IE8 vm crashed during an upgrade so I need to fix that up before debugging).

I think it is fine to advertise the first one (risks with extending the Google layer) as a known issue for 2.10. The second and third can be addressed in the patch. I'll report more on the IE issue when I dig into it.

[tschaub]

Ok, as far as I can tell, the terms of use element is not appended to the dom until the layer has fully loaded. The map "idle" event is triggered too early. Unfortunately, the map "tilesloaded" event is also triggered too early. Not always. But I'm seeing this cross-browser on crappy (virtual) machines. The result is that the mapObject.getDiv().firstChild.childNodes.length is less than 2 and our code assumes there are at least two elements. Looking at a fully loaded map, there are three child nodes in said element. While it sucks, unless we are given another event that corresponds to the proper elements being ready, I think we have to set timeouts until we see what we expect.

The updated patch does this. With the updated patch, issues 2-4 listed above are addressed. Tests pass and examples work in IE 6, FF 3.6, Safari 5.0.

However, we still have issues. If you add a non-Google layer as the first layer in the map, and then change the visibility of one of the Google layers, you see the map size issues we saw before with v2. So, I think a bit more work is needed here.

[tschaub]

Chris, please comment if you see other backwards compatibility issues. I only see the one mentioned above about people who were extending the old Google layer and calling methods on the prototype. I think this is a fringe case that we can list in the compatibility issues for the next release.

Otherwise, I think this is very close. The map size issue needs addressing (add an OSM layer as the first layer to the google-v3.html example to see it in action). And I think it would be great to see this in 2.10 (barring compatibility issues that others may bring up).

[crschmidt]

Haven't read anything further here; I was just commenting that it might be nice to make a nice clean break, but it's not important to me either way. Feel free to do what makes most sense.

[tschaub]

Actually, the current Google layer fails in the same way (add OSM layer as first layer to the map). I'll check to see if this has been broken since our Google overhaul.

[tschaub]

Replying to tschaub:

Actually, the current Google layer fails in the same way (add OSM layer as first layer to the map). I'll check to see if this has been broken since our Google overhaul.

Scratch that. Bit by sphericalMercator.

[tschaub]

In the event that I don't get this kicked before leaving (shortly), here's the example that demonstrates the problem:

<!DOCTYPE html>
<html>
  <head>
    <title>OpenLayers OSM and Google Example</title>
    <link rel="stylesheet" href="../theme/default/style.css" type="text/css" />
    <link rel="stylesheet" href="../theme/default/google.css" type="text/css" />
    <link rel="stylesheet" href="style.css" type="text/css" />
    <script src="http://maps.google.com/maps/api/js?sensor=false"></script>
    <script src="../lib/OpenLayers.js"></script>
    <script type="text/javascript">
        var map;

        function init() {
            map = new OpenLayers.Map({
                div: "map",
                projection: new OpenLayers.Projection("EPSG:900913"),
                units: "m",
                maxResolution: 156543.0339,
                maxExtent: new OpenLayers.Bounds(
                    -20037508, -20037508, 20037508, 20037508.34
                )
            });
            
            var osm = new OpenLayers.Layer.OSM();
            
            var gmap = new OpenLayers.Layer.Google("Google Streets");

            map.addLayers([osm, gmap]);

            map.addControl(new OpenLayers.Control.LayerSwitcher());

            map.setCenter(
                new OpenLayers.LonLat(10.2, 48.9).transform(
                    new OpenLayers.Projection("EPSG:4326"),
                    map.getProjectionObject()
                ), 
                5
            );
        }
    </script>
  </head>
  <body onload="init()">
    <h1 id="title">OSM and Google Together</h1>

    <div id="tags"></div>

    <p id="shortdesc">
        Demonstrate use of an OSM layer and a Google layer as base layers.
    </p>

    <div id="map" class="smallmap"></div>

    <div id="docs">
     <p>
       In order to position the Google attribution div in the default location,
       you must include the extra theme/default/google.css stylesheet.</p>
    </div>
  </body>
</html>

[crschmidt]

I would argue that this is the corner case that we just don't support anyway; switching map projections on the fly is painful at best.

[tschaub]

Yeah, I'm not talking about switching projections. I'm talking about having a different layer than the Google layer visible at first. We need to support it over here.

The issue can be solved by triggering "resize" after the "tilesloaded" listener sequence is done. However, this looks ugly because the tiles become partially visible and then get rendered again after a quick flash. There is likely a better place to let the map object know that it's size should be calculated.

[tschaub]

Oh, and if you didn't see already, the Google v3 layer is spherical mercator by default (which is why I said not switching projections).

[crschmidt]

So, we're changing the meaning of "OpenLayers.Layer.Google" to mean "spherical mercator by default"? I take back my earlier statement, and say that that is a significant API change that I don't feel comfortable with. I'll evaluate more closely as soon as I can, but I don't think we can change the meaning of that class so seriously in a .x release.

[ahocevar]

Replying to crschmidt:

So, we're changing the meaning of "OpenLayers.Layer.Google" to mean "spherical mercator by default"?

No, we do this only for OpenLayers.Layer.Google.v3. If you use the v2 API, the default is still 4326.

[tschaub]

add Google v3 layer

[tschaub]

Ok, I think this is good to go.

Turns out there were a few issues. The most significant was that both the "idle" and "tilesloaded" GMap events can be fired before the OL layer is visible. At this point, the map object cannot correctly determine its size. The new patch waits until the "center_changed" event is triggered (these guys are worse with their event naming conventions than we are) to reposition the terms of use and powered by elements. As with the earlier patch, care is taken to wait until these elements are actually appended (with slower connections the listener gets can get called before both elements are appended). These elements are not cached until the layer is first made visible - so some changes were needed to the tests as well.

In any case, the thing now works well on IE 6, FF 3.6, and Safari 5 - that is, all examples work and tests pass. I think we can do some serious simplification to this layer in the future as the calls to the map object for translation between OL coords and GMap coords are no longer needed (since we're in spherical mercator) - but we can easily save that for 3.0.

Thanks for the great work Andreas. This is a really nice enhancement. Please commit (assuming there are no objections).

[ahocevar]

(In [10474]) add support for GMaps v3. Thanks tschaub for the big improvements to my original patch. p=tschaub,me, r=tschaub (closes #2493)

[fredj]

There an issue with Layer/Google/v2.js, Layer/Google/v3.js and tests/Layer/Google/v3.html: the files have twice the same content.

[tschaub]

(In [10476]) Removing stuff that was added twice in r10474 (see #2493).

[tschaub]

I've found some additional issues with this in applications with allOverlays true. I'm tempted to call them separate issues and close this one.

[tschaub]

(In [10477]) In practice, this allOverlays example displays the terms of service element. In this test, it appears that the google script is hiding it again after we make it visible - perhaps because it is not actually displayed. And this is not always consistent. I can get this test to pass by making the timeout shorter. In any case, a simpler test is to confirm that repositionMapElements is getting called (see #2493).

[ahocevar]

Replying to tschaub:

3. If anybody was building a single file build with just the Google layer, their build config would have to be modified to pull in v2.

Right now, the full build (and the google-v3.html example on the demo site) breaks because of a circular dependency: Layer.Google requires Layer.Google.v2, and Layer.Google.v2 requires Layer.Google. Removing the Layer.Google dependency from Layer.Google.v2 causes Layer.Google.v2 to be included in the build before Layer.Google.

So IMHO the only way to get around this is to remove the dependency for Layer.Google.v2 again from Layer.Google, and add a note in the 2.10 release note that a build profile change is required.

Or can anybody come up with a better solution?

[tschaub]

(In [10478]) As with r10477, this change simplifies the setGMapVisibility tests to make sure that repositionMapElements is getting called. These tests were periodically failing depending upon the order in which they run. This likely indicates we've got to do some more cleaning up in map.destroy or elsewhere (see #2493).

[tschaub]

Replying to ahocevar:

Replying to tschaub:

3. If anybody was building a single file build with just the Google layer, their build config would have to be modified to pull in v2.

Right now, the full build (and the google-v3.html example on the demo site) breaks because of a circular dependency: Layer.Google requires Layer.Google.v2, and Layer.Google.v2 requires Layer.Google. Removing the Layer.Google dependency from Layer.Google.v2 causes Layer.Google.v2 to be included in the build before Layer.Google. So IMHO the only way to get around this is to remove the dependency for Layer.Google.v2 again from Layer.Google, and add a note in the 2.10 release note that a build profile change is required. Or can anybody come up with a better solution?

My bad. I forgot that toposort.py failed on circular dependencies. We could replace it with a much simpler topological sorting that doesn't fail in this case, but we'd have to add some checks at the head of Google.js to save the v2 mixin if it was already defined and then reset it after defining Google. Ugly.

The problem with only having v2.js require Google.js is that we'll break all custom builds that only include Google.js. This is a pretty large issue in my mind.

A quick solution is to define OpenLayers.Google.v2 in the Google.js file.

[tschaub]

I've taken this circular dependency issue to #2729 (as I've developed a pet peeve for reopened tickets).

[tschaub]

The allOverlays issue mentioned above is addressed in #2730.