Fix NSString conversions to treat a null NSString as a string of length 0, in...

Fix NSString conversions to treat a null NSString as a string of length 0, instead of crashing. This allows Cocoa to use null objects as empties, as is its wont, and we only run a check when needed.

This CL also removes the now-superfluous checks for null NSStrings from BugReportWindowController.  A cursory look through the code shows that there are many places where a check for null precedes a call to an NSString conversion; filed another bug against myself to go through and fix all of these (http://code.google.com/p/chromium/issues/detail?id=27055).  Also filed a bug to expand unit tests for NSString conversion methods (http://code.google.com/p/chromium/issues/detail?id=27059).

BUG= none, but see comments in http://codereview.chromium.org/371012/show
TEST= none, but unit tests are included that test changed functionality.

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

[Miranda Callahan, 2009-11-08 00:25:13]

The unit test for the NSStringToUTFxxx methods should probably have lots more in
it, since it hasn't been subjected to any unit tests at all before now.  I'd
like to file a bug on this and extend it after the current push.

[John Grabowski, 2009-11-08 01:35:32]

On 2009/11/08 00:25:13, Miranda Callahan wrote:
> The unit test for the NSStringToUTFxxx methods should probably have lots more
in
> it, since it hasn't been subjected to any unit tests at all before now.  I'd
> like to file a bug on this and extend it after the current push.

Perhaps add bug numbers to this CL description.

[John Grabowski, 2009-11-08 01:36:14]

LGTM

Thanks for the follow-through here.

+cc pink as FYI

http://codereview.chromium.org/371057/diff/4002/4005
File base/sys_string_conversions_mac_unittest.mm (right):

http://codereview.chromium.org/371057/diff/4002/4005#newcode9
Line 9: EXPECT_EQ("Hello, world!", base::SysNSStringToUTF8(@"Hello, world!"));
EXPECT_STREQ (and below)
std:;string happens to have an operator==(char*,std::string), so this works
as-is, but STREQ is more clear.

[John Grabowski, 2009-11-08 01:37:05]

Still LGTM but...

http://codereview.chromium.org/371057/diff/4002/4005
File base/sys_string_conversions_mac_unittest.mm (right):

http://codereview.chromium.org/371057/diff/4002/4005#newcode1
Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
--> 2009

[Miranda Callahan, 2009-11-08 01:48:09]

Thanks for the Saturday afternoon review!

http://codereview.chromium.org/371057/diff/4002/4005
File base/sys_string_conversions_mac_unittest.mm (right):

http://codereview.chromium.org/371057/diff/4002/4005#newcode1
Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
On 2009/11/08 01:37:05, John Grabowski wrote:
> --> 2009

Done.

http://codereview.chromium.org/371057/diff/4002/4005#newcode9
Line 9: EXPECT_EQ("Hello, world!", base::SysNSStringToUTF8(@"Hello, world!"));
On 2009/11/08 01:36:14, John Grabowski wrote:
> EXPECT_STREQ (and below)
> std:;string happens to have an operator==(char*,std::string), so this works
> as-is, but STREQ is more clear.

Done.

[Mark Mentovai, 2009-11-08 04:12:50]

http://codereview.chromium.org/371057/diff/7001/3008
File base/sys_string_conversions_mac.mm (right):

http://codereview.chromium.org/371057/diff/7001/3008#newcode26
Line 26: // cfstring could have been initialized with a NULL NSString, so check
before
Sigh.

Please don't do this in this function.  I asked that this be done in the
SysNSStringTo* wrappers.  I don't think that we should null-check for CFStrings.
 Thanks.

[mirandac, 2009-11-08 04:36:27]

On Sat, Nov 7, 2009 at 8:12 PM,  <mark@chromium.org> wrote:
>
> http://codereview.chromium.org/371057/diff/7001/3008
> File base/sys_string_conversions_mac.mm (right):
>
> http://codereview.chromium.org/371057/diff/7001/3008#newcode26
> Line 26: // cfstring could have been initialized with a NULL NSString,
> so check before
> Sigh.
>
> Please don't do this in this function. =A0I asked that this be done in th=
e
> SysNSStringTo* wrappers. =A0I don't think that we should null-check for
> CFStrings. =A0Thanks.
>
> http://codereview.chromium.org/371057
>

Yes, you specifically did, explaining the entire CF/NS interface, and
I managed to read the entire discussion and forget it by the time I
did the fix this morning.  Double-sigh.  I'll make it right first
thing tomorrow.

[Miranda Callahan, 2009-11-08 17:28:19]

Moved null check to the SysNSStringTo wrappers.

http://codereview.chromium.org/371057/diff/7001/3008
File base/sys_string_conversions_mac.mm (right):

http://codereview.chromium.org/371057/diff/7001/3008#newcode26
Line 26: // cfstring could have been initialized with a NULL NSString, so check
before
On 2009/11/08 04:12:50, Mark Mentovai wrote:
> Sigh.
> 
> Please don't do this in this function.  I asked that this be done in the
> SysNSStringTo* wrappers.  I don't think that we should null-check for
CFStrings.
>  Thanks.

Done.

[Mark Mentovai, 2009-11-08 17:34:30]

LG otherwise

http://codereview.chromium.org/371057/diff/2011/2012
File base/base.gyp (right):

http://codereview.chromium.org/371057/diff/2011/2012#newcode623
Line 623: 'sys_string_conversions_mac_unittest.mm',
Sort nit: _mac before _unittest.

http://codereview.chromium.org/371057/diff/2011/2013
File base/sys_string_conversions_mac.mm (right):

http://codereview.chromium.org/371057/diff/2011/2013#newcode188
Line 188: return SysCFStringRefToUTF8(reinterpret_cast<CFStringRef>(
I would suggest:

  if (!nsstring)
    return std::string();
  return SysCFStringRefToUTF8(reinterpret_cast<CFStringRef>(nsstring));

to avoid doing busywork when it's known that the return value will just be an
empty string.

http://codereview.chromium.org/371057/diff/2011/2014
File base/sys_string_conversions_mac_unittest.mm (right):

http://codereview.chromium.org/371057/diff/2011/2014#newcode5
Line 5: #include "base/sys_string_conversions.h"
This file needs #import <Foundation/Foundation.h>

http://codereview.chromium.org/371057/diff/2011/2014#newcode13
Line 13: EXPECT_STREQ("", base::SysNSStringToUTF8((NSString*)nil).c_str());
Surprised you needed an NSString* cast.

[Mark Mentovai, 2009-11-08 17:49:24]

http://codereview.chromium.org/371057/diff/2011/2013
File base/sys_string_conversions_mac.mm (right):

http://codereview.chromium.org/371057/diff/2011/2013#newcode186
Line 186: // Check for nil NSString* to avoid initializing a NULL CFStringRef.
Also, any comment that you're inclined to put outside a function body is
probably an interface comment, and those belong in headers.  You might want to
put this into sys_string_conversions.h, because callers might care about this
without really caring about the internal implementation.

[Miranda Callahan, 2009-11-08 18:21:48]

Addressed comments and added unit tests for each of the wrapper functions
individually.

http://codereview.chromium.org/371057/diff/2011/2012
File base/base.gyp (right):

http://codereview.chromium.org/371057/diff/2011/2012#newcode623
Line 623: 'sys_string_conversions_mac_unittest.mm',
On 2009/11/08 17:34:30, Mark Mentovai wrote:
> Sort nit: _mac before _unittest.

Done.

http://codereview.chromium.org/371057/diff/2011/2013
File base/sys_string_conversions_mac.mm (right):

http://codereview.chromium.org/371057/diff/2011/2013#newcode186
Line 186: // Check for nil NSString* to avoid initializing a NULL CFStringRef.
On 2009/11/08 17:49:24, Mark Mentovai wrote:
> Also, any comment that you're inclined to put outside a function body is
> probably an interface comment, and those belong in headers.  You might want to
> put this into sys_string_conversions.h, because callers might care about this
> without really caring about the internal implementation.

Done, and made comment more appropriate.

http://codereview.chromium.org/371057/diff/2011/2013#newcode188
Line 188: return SysCFStringRefToUTF8(reinterpret_cast<CFStringRef>(
On 2009/11/08 17:34:30, Mark Mentovai wrote:
> I would suggest:
> 
>   if (!nsstring)
>     return std::string();
>   return SysCFStringRefToUTF8(reinterpret_cast<CFStringRef>(nsstring));
> 
> to avoid doing busywork when it's known that the return value will just be an
> empty string.

Done.

http://codereview.chromium.org/371057/diff/2011/2014
File base/sys_string_conversions_mac_unittest.mm (right):

http://codereview.chromium.org/371057/diff/2011/2014#newcode5
Line 5: #include "base/sys_string_conversions.h"
On 2009/11/08 17:34:30, Mark Mentovai wrote:
> This file needs #import <Foundation/Foundation.h>

Done.

http://codereview.chromium.org/371057/diff/2011/2014#newcode13
Line 13: EXPECT_STREQ("", base::SysNSStringToUTF8((NSString*)nil).c_str());
On 2009/11/08 17:34:30, Mark Mentovai wrote:
> Surprised you needed an NSString* cast.

You're right, none was required.

[Mark Mentovai, 2009-11-08 19:01:42]

LGTM!