twooten [twooten]

// Set lat/long/start_zoom to some defaults
$center_latitude = 19.9733;
$center_longitude = 0.7031;
$start_zoom = 1;

If you're going to have global you need to respect name space rules. Better not to have them at all.

// setup some javascript variable for use in the map
drupal_add_js("var centerLatitude = " . drupal_to_js($center_latitude) . ";", 'inline');    
drupal_add_js("var centerLongitude = " . drupal_to_js($center_longitude) . ";", 'inline');    
drupal_add_js("var startZoom = " . drupal_to_js($start_zoom) . ";", 'inline');

same with javascript variables. You should use 'setting' rather than 'inline' in the drupal_add_js function. See http://api.drupal.org/api/function/drupal_add_js/6

// private function for map page content
function _visitorinfo_get_map() {

/* SNIP */
  $map_div = "<h3>Last " . $count . " visitor locations.</h3>";
  $map_div .= "<div id='map' style='width: 100%; height: 400px; border: 5px solid #666666;'></div>";
  return $map_div;
}

You should provided a theme function ot template, so that users of your module can override the default html output of the map. Also, I don't see any use of t() to allow user interface text to be translated.

function visitorinfo_help($path, $arg) {
  if ($path == 'admin/help#visitorinfo') {
    $txt = '<p>The Visitorinfo module captures the IP address of your visitors, '

  /* SNIP */

    return '<p>'. t($txt, $replace) .'</p>';
  }
}

You're nesting <p> tags. It's a good idea to break up text passed through t() into smaller bites too, and keep markup for blocks (p, div, etc) out of text to be translated.

  // convert the PHP $visitors array to a javascript array
  drupal_add_js("var visitors = " . drupal_to_js($visitors) . ";", 'inline');
  // print out all the js map stuff
  drupal_add_js('
    var map;
    var descrip;
    function addMarker(latitude, longitude, description) {
      var marker = new GMarker(new GLatLng(latitude, longitude));
      GEvent.addListener(marker, \'click\',
	     function() {
		 marker.openInfoWindowHtml(description);
	     }
	);
	map.addOverlay(marker);

/* SNIP */

    window.onload = init;
    window.onunload = GUnload;

Rather than doing this inline, it's preferable to put this in an external js file. As a bare minimum you *must* ensure you are not going to have name collisions with other modules. Drupal.behaviors.yourmodulesnamespace= function (context) {} can be used for functions to be run on windows loading after the DOM is setup. Drupal.yourmodule = {} can be used for just about anything you want, and you get your own javascript namespace that way. Look at the way some other modules are using javascript.

function visitorinfo_schema() {
$schema['visitorinfo'] = array(
  'description' => t('Store data on site visitors'),

Schema descriptors don't go through t().

function visitorinfo_init() {
  // get the visitors ip address
  $ip = $_SERVER['REMOTE_ADDR'];
  // and see if it is already in the db

hook_init is not called for cached pages, therefore anonymous users will not be logged if the admin has caching enabled, which would defeat the point of this module. You might try hook_boot.

You also need to include the a LICENSE.txt with the GPL2 license.

(I'm not an 'official' reviewer, just helping out while I'm waiting on my own CVS request)

You also need to include the a LICENSE.txt with the GPL2 license.

The license file is added from the packaging script, and it is not allowed to commit that file in CVS.

function visitorinfo_help($path, $arg) {
  if ($path == 'admin/help#visitorinfo') {
    $txt = '<p>The Visitorinfo module captures the IP address of your visitors, '

  /* SNIP */

    return '<p>'. t($txt, $replace) .'</p>';
  }
}

The first argument of t() is a literal string, not a dynamic value; differently, the script to extract the strings to translate will not extract the string.

function theme_visitorinfo_map($count) {
  $output = '<div>' . t("Last ") . $count . t(" Visitor Locations.") . '</div>';
  $output .= '<div id="map"></div>';
  return $output;
}

Use a placeholder.

Strings used in user interface should have the first word in capital case, and the other words in lower case (with the exception of proper nouns, adjectives derived from proper nouns, and acronyms).

1.   $sql = "SELECT * FROM {visitorinfo} order by csid desc";
     // put a cap on number of markers to show, default to 200
     // this can be changed on the admin page
     $num_markers = variable_get('visitorinfo_markers', '200');
     $result = db_query_range(db_rewrite_sql($sql), 0, $num_markers);
   

   db_rewrite_sql() uses the following default arguments $primary_table = 'n', $primary_field = 'nid', $args = array(), which are the same arguments used for the call to the function made from the code I reported. Considering that node.module uses the following implementation of hook_db_rewrite_sql()

   function node_db_rewrite_sql($query, $primary_table, $primary_field) {
     if ($primary_field == 'nid' && !node_access_view_all_nodes()) {
       $return['join'] = _node_access_join_sql($primary_table);
       $return['where'] = _node_access_where_sql();
       $return['distinct'] = 1;
       return $return;
     }
   }
   

   it is probable that the query would be altered in a not appropriate mode.

2.   drupal_add_js("var visitors = " . drupal_to_js($visitors) . ";", 'inline');
   

   Also that line should be using setting.

3. /**
    * Implementation of hook_boot().
    */
   function visitorinfo_boot() {
     // get the visitors ip address
     $ip = $_SERVER['REMOTE_ADDR'];
     // and see if it is already in the db
     //$visitorinfo_ip = "SELECT ip FROM {visitorinfo} WHERE ip = '". db_escape_string($ip) ."'";
     $result = db_query("SELECT ip FROM {visitorinfo} v WHERE v.ip = '%s' ", $ip);
     // db_affected_rows will be > than 0 if the IP is already in the db
     if (db_affected_rows($result) == 0) {
       _visitorinfo_get_location($ip);
     }
     else {
       // not sure about this return...
       return TRUE;
     }
   }
   

   hook_boot() is called when Drupal is still bootstrapping. The function you are calling is probably not yet available; see drupal_bootstrap() for more details.
   hook_boot() is then not supposed to return any values.

4.   $items['admin/reports/visitorinfo'] = array(
       'title' => 'Visitors Location Map',
       'description' => 'Listing of visitors.',
       'page callback' => '_visitorinfo_get_map',
       'access arguments' => array('access visitorinfo'),
       'type' => MENU_NORMAL_ITEM,
     );
     $items['admin/settings/visitorinfo'] = array(
       'title' => 'Visitor Info',
       'description' => 'Settings for visitorinfo.',
       'position' => 'left',
       'page callback' => 'drupal_get_form',
       'page arguments' => array('visitorinfo_settings'),
       'access arguments' => array('administer site configuration'),
       'block callback' => 'visitorinfo_settings',
       'type' => MENU_NORMAL_ITEM,    
     );
   

   See comment #11.

var map;
var descrip;

/* snip */

window.onload = init;
window.onunload = GUnload;

You need to play nice with other javascript libraries, specifically jQuery which is core to Drupal. Overwriting window.onload is a good way to break things.

You should use:

Drupal.behaviors.myModuleBehavior = function (context) {
  var map;
  var descrip;
  // contents of your init() function
  function addMarker(latitude, longitude, description) {  .....  }
};

or

$(document).ready(function(){
  // same as before
}); 

As well as playing nice with jQuery, this puts symbols in their own scope. The way you have it, your variables and function names are in the global scope, so if any other js file defines those symbols, things break. The above two constructions solve that problem.

I'm not sure, but I think you can use this jQuery snippet for the unload:

$(window).unload( function () { GUnload(); } );

The CVS account has been already approved. The CVS application is not anymore necessary.