Add support for geolocation wifi scanning on OSX 10.6...

Add support for geolocation wifi scanning on OSX 10.6

BUG=http://crbug.com/37198
TEST=Run browser with --enable-geolocation on OSX 10.6, open maps.google.com


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=40891

[joth, 2010-03-04 21:33:31]

Jeremy, Mike

Would one of you be able to take a look over this change for me? It's my first
objective C change, but I got some help some help from some non-Chrome folks
here in London so hopefully it won't be tooooo painful :)

The code's a little tricky as it needs to run on both 10.5 and 10.6 using
different APIs, and build on both SDKs (where one API only exists officially in
the 10.6 SDK). Do let me know if there are more formulaic ways to achieve this
sort of thing in Chrome.

Cheers
Joth

[jorlow, 2010-03-05 11:20:07]

Non .mm parts LGTM.  Pink, can you take a look at the .mm parts?

[pink (ping after 24hrs), 2010-03-05 14:07:07]

Will do this morning, just getting through a few things for SoC.

On Fri, Mar 5, 2010 at 6:20 AM,  <jorlow@chromium.org> wrote:
> Non .mm parts LGTM.  Pink, can you take a look at the .mm parts?
>
> http://codereview.chromium.org/669083
>



-- 
Mike Pinkerton
Mac Weenie
pinkerton@google.com

[pink (ping after 24hrs), 2010-03-05 15:05:46]

Pretty good, but a few changes requested. I'll take one more pass after you make
them.

http://codereview.chromium.org/669083/diff/14/1009
File chrome/browser/geolocation/wifi_data_provider_corewlan_mac.mm (right):

http://codereview.chromium.org/669083/diff/14/1009#newcode73
chrome/browser/geolocation/wifi_data_provider_corewlan_mac.mm:73: bool Init();
part of the public interface and not inherited, needs a comment.

http://codereview.chromium.org/669083/diff/14/1009#newcode79
chrome/browser/geolocation/wifi_data_provider_corewlan_mac.mm:79:
NSAutoreleasePool* pool_;
if these are owned pointers, use scoped_nsobject for all 3

http://codereview.chromium.org/669083/diff/14/1009#newcode91
chrome/browser/geolocation/wifi_data_provider_corewlan_mac.mm:91: bundle_ = nil;
do these in the initializer declaration.

http://codereview.chromium.org/669083/diff/14/1009#newcode123
chrome/browser/geolocation/wifi_data_provider_corewlan_mac.mm:123: [NSDictionary
dictionaryWithObject:[NSNumber numberWithBool:false]
NO, not false.

http://codereview.chromium.org/669083/diff/14/1009#newcode137
chrome/browser/geolocation/wifi_data_provider_corewlan_mac.mm:137: CWNetwork*
network = [scan objectAtIndex:i];
use fast enumeration instead of objectAtIndex:

http://codereview.chromium.org/669083/diff/14/1009#newcode152
chrome/browser/geolocation/wifi_data_provider_corewlan_mac.mm:152: DLOG(INFO) <<
"  AP " << i
do you want to wrap this in an ifdef so everyone running debug builds isn't
subject to this console spew? Or just remove it?

http://codereview.chromium.org/669083/diff/14/1010
File chrome/browser/geolocation/wifi_data_provider_mac.cc (right):

http://codereview.chromium.org/669083/diff/14/1010#newcode164
chrome/browser/geolocation/wifi_data_provider_mac.cc:164: // engineered iStumber
code.
iStumbler

http://codereview.chromium.org/669083/diff/14/1011
File chrome/browser/geolocation/wifi_data_provider_mac.h (right):

http://codereview.chromium.org/669083/diff/14/1011#newcode10
chrome/browser/geolocation/wifi_data_provider_mac.h:10: class
OsxWifiDataProvider : public WifiDataProviderCommon {
Use "Mac", we don't ever use "Osx" as a prefix or suffix.

http://codereview.chromium.org/669083/diff/14/1011#newcode10
chrome/browser/geolocation/wifi_data_provider_mac.h:10: class
OsxWifiDataProvider : public WifiDataProviderCommon {
this class needs a class-level comment.

[joth, 2010-03-05 17:56:49]

Many thanks! All comments addresses & new snapshot uploaded. Another look?

On 5 March 2010 15:05, <pinkerton@chromium.org> wrote:

> Pretty good, but a few changes requested. I'll take one more pass after you
> make
> them.
>
>
> http://codereview.chromium.org/669083/diff/14/1009
> File chrome/browser/geolocation/wifi_data_provider_corewlan_mac.mm
> (right):
>
> http://codereview.chromium.org/669083/diff/14/1009#newcode73
> chrome/browser/geolocation/wifi_data_provider_corewlan_mac.mm:73: bool
> Init();
> part of the public interface and not inherited, needs a comment.
>
> http://codereview.chromium.org/669083/diff/14/1009#newcode79
> chrome/browser/geolocation/wifi_data_provider_corewlan_mac.mm:79:
> NSAutoreleasePool* pool_;
> if these are owned pointers, use scoped_nsobject for all 3
>
> Done, except I now use ScopedNSAutoreleasePool in the individual functions
in place of pool_


> http://codereview.chromium.org/669083/diff/14/1009#newcode91
> chrome/browser/geolocation/wifi_data_provider_corewlan_mac.mm:91:
> bundle_ = nil;
> do these in the initializer declaration.
>
> obsolete with scoped_nsobject


> http://codereview.chromium.org/669083/diff/14/1009#newcode123
> chrome/browser/geolocation/wifi_data_provider_corewlan_mac.mm:123:
> [NSDictionary dictionaryWithObject:[NSNumber numberWithBool:false]
> NO, not false.
>
> http://codereview.chromium.org/669083/diff/14/1009#newcode137
> chrome/browser/geolocation/wifi_data_provider_corewlan_mac.mm:137:
> CWNetwork* network = [scan objectAtIndex:i];
> use fast enumeration instead of objectAtIndex:
>
> nice. thanks for the tip.


> http://codereview.chromium.org/669083/diff/14/1009#newcode152
> chrome/browser/geolocation/wifi_data_provider_corewlan_mac.mm:152:
> DLOG(INFO) << "  AP " << i
> do you want to wrap this in an ifdef so everyone running debug builds
> isn't subject to this console spew? Or just remove it?
>
> http://codereview.chromium.org/669083/diff/14/1010
> File chrome/browser/geolocation/wifi_data_provider_mac.cc (right):
>
> http://codereview.chromium.org/669083/diff/14/1010#newcode164
> chrome/browser/geolocation/wifi_data_provider_mac.cc:164: // engineered
> iStumber code.
> iStumbler
>
> http://codereview.chromium.org/669083/diff/14/1011
> File chrome/browser/geolocation/wifi_data_provider_mac.h (right):
>
> http://codereview.chromium.org/669083/diff/14/1011#newcode10
> chrome/browser/geolocation/wifi_data_provider_mac.h:10: class
> OsxWifiDataProvider : public WifiDataProviderCommon {
> Use "Mac", we don't ever use "Osx" as a prefix or suffix.
>
> http://codereview.chromium.org/669083/diff/14/1011#newcode10
> chrome/browser/geolocation/wifi_data_provider_mac.h:10: class
> OsxWifiDataProvider : public WifiDataProviderCommon {
> this class needs a class-level comment.
>
>
(I also removed a couple TODOs about wifi 'age' members, which are obsolete
as they're covered by a bug to use Time instead)



>
> http://codereview.chromium.org/669083
>

[pink (ping after 24hrs), 2010-03-05 20:12:05]

LGTM

http://codereview.chromium.org/669083/diff/1023/1025
File chrome/browser/geolocation/wifi_data_provider_mac.cc (right):

http://codereview.chromium.org/669083/diff/1023/1025#newcode25
chrome/browser/geolocation/wifi_data_provider_mac.cc:25: class OsxWlanApi :
public WifiDataProviderCommon::WlanApiInterface {
Fix this name as well (WlanApiMac)

http://codereview.chromium.org/669083/diff/1023/1025#newcode30
chrome/browser/geolocation/wifi_data_provider_mac.cc:30: bool Init();
public method needs a comment

http://codereview.chromium.org/669083/diff/1023/1026
File chrome/browser/geolocation/wifi_data_provider_mac.h (right):

http://codereview.chromium.org/669083/diff/1023/1026#newcode13
chrome/browser/geolocation/wifi_data_provider_mac.h:13: class
MacWifiDataProvider : public WifiDataProviderCommon {
personally, I'd prefer to see WifiDataProviderMac, but I won't stab you for
sticking with what you have.

[joth, 2010-03-08 10:57:40]

On 2010/03/05 20:12:05, pink wrote:
> LGTM
> 
> http://codereview.chromium.org/669083/diff/1023/1025
> File chrome/browser/geolocation/wifi_data_provider_mac.cc (right):
> 
> http://codereview.chromium.org/669083/diff/1023/1025#newcode25
> chrome/browser/geolocation/wifi_data_provider_mac.cc:25: class OsxWlanApi :
> public WifiDataProviderCommon::WlanApiInterface {
> Fix this name as well (WlanApiMac)
> 
Good point.
I've gone with Apple80211Api as this is the name of the framework that it uses,
making it the counterpart of the new CoreWlanApi. I think this makes the
trailing Mac spurious, let me know if you still like to see that added.


> http://codereview.chromium.org/669083/diff/1023/1025#newcode30
> chrome/browser/geolocation/wifi_data_provider_mac.cc:30: bool Init();
> public method needs a comment
> 
> http://codereview.chromium.org/669083/diff/1023/1026
> File chrome/browser/geolocation/wifi_data_provider_mac.h (right):
> 
> http://codereview.chromium.org/669083/diff/1023/1026#newcode13
> chrome/browser/geolocation/wifi_data_provider_mac.h:13: class
> MacWifiDataProvider : public WifiDataProviderCommon {
> personally, I'd prefer to see WifiDataProviderMac, but I won't stab you for
> sticking with what you have.

I'm slowly converting all these classes (windows, linux & mac)  from gears to
chrome, gradually bringing them all into line with chrome naming conventions.
I'll do this renaming in another patch to avoid pulling more stuff into this
one, if that's OK.