Create a nicer interface for base::mac::GetValueFromDictionary

Create a nicer interface for base::mac::GetValueFromDictionary

The interface now conforms to base::mac::GetValueFromDictionary<>(dictionary, key). The tests written have loosely compared it against the original base::mac::GetValueFromDictionary method for expectations.

TEST=FoundationUtilTest.GetValueFromDictionary


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

[KushalP, 2011-11-11 22:57:33]

Something I'm not too sure about and would appreciate clarification on is the
level of testing. I've gone through and created tests for the method itself
which are positive, negative and then compare against the method which it's
supposed to be emulating.

@mark I've taken on your comments from my previous change in this file and tried
to keep the tests as minimal and clean as possible. However if there are ways of
making it learner, I'm all ears!

[KushalP, 2011-11-11 23:05:37]

I'm also now wondering whether it would be worthwhile to have a "Strict" variant
which DCHECKs (using base::mac::CFCastStrict) ?

[Nico, 2011-11-11 23:08:36]

http://codereview.chromium.org/8540021/diff/5/base/mac/foundation_util.h
File base/mac/foundation_util.h (right):

http://codereview.chromium.org/8540021/diff/5/base/mac/foundation_util.h#newc...
base/mac/foundation_util.h:108: T GetValueFromDictionary(CFDictionaryRef dict,
CFStringRef key);
I don't understand why this is a template function instead of

GetCFStringFromDictionary();
GetCFNumberFromDictionary();

etc.

[KushalP, 2011-11-11 23:13:35]

http://codereview.chromium.org/8540021/diff/5/base/mac/foundation_util.h
File base/mac/foundation_util.h (right):

http://codereview.chromium.org/8540021/diff/5/base/mac/foundation_util.h#newc...
base/mac/foundation_util.h:108: T GetValueFromDictionary(CFDictionaryRef dict,
CFStringRef key);
On 2011/11/11 23:08:36, Nico wrote:
> I don't understand why this is a template function instead of
> 
> GetCFStringFromDictionary();
> GetCFNumberFromDictionary();
> 
> etc.

I would've thought the template function is a cleaner way of using the method.

You can do something similar to your example with:

base::mac::GetValueFromDictionary<CFStringRef>();
base::mac::GetValueFromDictionary<CFNumberRef>();

[Nico, 2011-11-11 23:19:04]

> You can do something similar to your example with:
> 
> base::mac::GetValueFromDictionary<CFStringRef>();
> base::mac::GetValueFromDictionary<CFNumberRef>();

Yeah, but with the template approach, GetValueFromDictionary<Browser>() will
compile fine, just to produce a linker error. Having a template function and
then providing nothing but specializations is fairly unusual for this reason.

(Exporting templates is too, for similar reasons.)

[Mark Mentovai, 2011-11-11 23:20:45]

Here are the options.

This change should be accompanied by one that transitions existing callers.

http://codereview.chromium.org/8540021/diff/1/base/mac/foundation_util.h
File base/mac/foundation_util.h (right):

http://codereview.chromium.org/8540021/diff/1/base/mac/foundation_util.h#newc...
base/mac/foundation_util.h:105: BASE_EXPORT template<class T>
More of a typename than a class. You can fix CFCast and ObjCCast similarly.

http://codereview.chromium.org/8540021/diff/1/base/mac/foundation_util.mm
File base/mac/foundation_util.mm (right):

http://codereview.chromium.org/8540021/diff/1/base/mac/foundation_util.mm#new...
base/mac/foundation_util.mm:351: #define GET_VAL_FROM_DICT_DEFN(TypeCF) \
No reason to abbreviate here, you can write GET_VALUE_FROM_DICTIONARY_DEFN.

http://codereview.chromium.org/8540021/diff/1/base/mac/foundation_util.mm#new...
base/mac/foundation_util.mm:354: CFTypeRef val = CFDictionaryGetValue(dict,
key); \
val and value as names are confusing.

http://codereview.chromium.org/8540021/diff/1/base/mac/foundation_util.mm#new...
base/mac/foundation_util.mm:355: TypeCF##Ref value =
base::mac::CFCast<TypeCF##Ref>(val); \
base::mac:: isn’t necessary, you’re already in that namespace.

http://codereview.chromium.org/8540021/diff/1/base/mac/foundation_util.mm#new...
base/mac/foundation_util.mm:358: if (!value) { \
The contents of this block should be pulled out into its own
anonymous-namespaced helper function, so you’d write:

  if (!value) { \
    DLOG(WARNING) <<
        GetValueFromDictionaryErrorMessage(key, TypeCF##GetTypeID(), val); \
  } \
  return value; \

Why? Avoidance of duplication. You can write the function that produces the
error string in a totally type-independent way. You wind up only emitting one
copy of that code instead of ten, one for each flavor of this function that you
currently output.

http://codereview.chromium.org/8540021/diff/1/base/mac/foundation_util.mm#new...
base/mac/foundation_util.mm:364: << base::SysCFStringRefToUTF8(key) \
The alignment’s all off here.

It’s off in the spot where you copied this from, too. Oops.

http://codereview.chromium.org/8540021/diff/1/base/mac/foundation_util.mm#new...
base/mac/foundation_util.mm:373: }
You could write this entire function without any macros by putting something
like this into the header:

template<typename CFT>
CFT GetValueFromDictionary(CFDictionaryRef dict, CFStringRef key) {
  CFTypeRef value = CFDictionaryGetValue(dict, key);
  CFT value_specific = CFCast<CFT>(value);
  if (value && !value_specific) {
    DLOG(WARNING) << GetValueFromDictionaryErrorMessage(key, value);
  }
  return value_specific;
}

You lose the ability to have the error message show the expected type name, but
I think that’s fine. You can also go to a small bit of extra trouble to
templatize a function that can provide the type name.

http://codereview.chromium.org/8540021/diff/1/base/mac/foundation_util_unitte...
File base/mac/foundation_util_unittest.mm (right):

http://codereview.chromium.org/8540021/diff/1/base/mac/foundation_util_unitte...
base/mac/foundation_util_unittest.mm:281: CFStringRef keys[3] = {
No [3], just [].

http://codereview.chromium.org/8540021/diff/1/base/mac/foundation_util_unitte...
base/mac/foundation_util_unittest.mm:284: CFNumberRef values[3] = {
Also no [3].

http://codereview.chromium.org/8540021/diff/1/base/mac/foundation_util_unitte...
base/mac/foundation_util_unittest.mm:292: (const void**)keys, (const
void**)values, 3,
Use C++ casts. (Do you need the const?)

Use arraysize.

COMPILE_ASSERT that the two arraysizes are equal.

[Mark Mentovai, 2011-11-11 23:25:34]

Nico wrote:
> Yeah, but with the template approach, GetValueFromDictionary<Browser>() will
> compile fine, just to produce a linker error.

I think this is fine. Templates frequently don’t operate on the entire universe
of types as template parameters.

> Having a template function and
> then providing nothing but specializations is fairly unusual for this reason.

I do think the template makes more sense if written as a real template
implementation rather than a template declaration and then a series of
specializations. (I suggested this in my review.)

[KushalP, 2011-11-12 01:24:38]

On 2011/11/11 23:25:34, Mark Mentovai wrote:
> 
> I do think the template makes more sense if written as a real template
> implementation rather than a template declaration and then a series of
> specializations. (I suggested this in my review.)

I like this as well. However the reason I went for the
declaration/specialisation route is because I don't know how to consistently get
the type name of a templated function. I know you could can do things like
typeid(T).name(), but that's assuming RTTI.

[KushalP, 2011-11-12 01:38:30]

Patch Set 3 takes into account most of @mark's recommendations. The only thing
left to do is get the type name of T as a string. See Line 295 in
foundation_util.h.

[Mark Mentovai, 2011-11-13 01:28:21]

http://codereview.chromium.org/8540021/diff/8001/base/mac/foundation_util.h
File base/mac/foundation_util.h (right):

http://codereview.chromium.org/8540021/diff/8001/base/mac/foundation_util.h#n...
base/mac/foundation_util.h:104: CFStringRef expected_type_ref,
Bad alignment on the indent.

http://codereview.chromium.org/8540021/diff/8001/base/mac/foundation_util.h#n...
base/mac/foundation_util.h:115: BASE_EXPORT template<class T>
Remember what I said? typename sounds better than class here.

http://codereview.chromium.org/8540021/diff/8001/base/mac/foundation_util.h#n...
base/mac/foundation_util.h:116: T GetValueFromDictionary(CFDictionaryRef dict,
CFStringRef key);
Why did you declare this here? You can just have the declaration and definition
down below. You can also move the declaration of
GetvalueFromDictionaryErrorMessage down there, so that it’s near the caller that
it’s subservient to. There will be no other callers.

http://codereview.chromium.org/8540021/diff/8001/base/mac/foundation_util.h#n...
base/mac/foundation_util.h:289: if (!value)
Rid this entire function of early returns. Everything can just return
value_specific down below.

Making this change means getting rid of this conditional, changing the next one
to be (value && !value_specific), and getting rid of the |return NULL|.

http://codereview.chromium.org/8540021/diff/8001/base/mac/foundation_util.h#n...
base/mac/foundation_util.h:294: // TODO: Get the string value of the type name
T.
Can you think of a good way to do this?

http://codereview.chromium.org/8540021/diff/8001/base/mac/foundation_util.mm
File base/mac/foundation_util.mm (right):

http://codereview.chromium.org/8540021/diff/8001/base/mac/foundation_util.mm#...
base/mac/foundation_util.mm:207: return "Expected value for key "
http://dev.chromium.org/developers/coding-style#TOC-Code-formatting

“When expressions are wrapped, the operator should be on the end of the broken
line…”

http://codereview.chromium.org/8540021/diff/8001/base/mac/foundation_util.mm#...
base/mac/foundation_util.mm:225: CFCopyTypeIDDescription(expected_type));
Why did you change the correct four-space indent on this line to something else?

http://codereview.chromium.org/8540021/diff/8001/base/mac/foundation_util_uni...
File base/mac/foundation_util_unittest.mm (right):

http://codereview.chromium.org/8540021/diff/8001/base/mac/foundation_util_uni...
base/mac/foundation_util_unittest.mm:291: 
Extra blank line.

[KushalP, 2011-11-13 12:30:27]

Most nits covered in Patch Set 6.

http://codereview.chromium.org/8540021/diff/8001/base/mac/foundation_util.h
File base/mac/foundation_util.h (right):

http://codereview.chromium.org/8540021/diff/8001/base/mac/foundation_util.h#n...
base/mac/foundation_util.h:116: T GetValueFromDictionary(CFDictionaryRef dict,
CFStringRef key);
On 2011/11/13 01:28:22, Mark Mentovai wrote:
> Why did you declare this here? You can just have the declaration and
definition
> down below. You can also move the declaration of
> GetvalueFromDictionaryErrorMessage down there, so that it’s near the caller
that
> it’s subservient to. There will be no other callers.

Thought I'd killed this with the patch set.

http://codereview.chromium.org/8540021/diff/8001/base/mac/foundation_util.h#n...
base/mac/foundation_util.h:294: // TODO: Get the string value of the type name
T.
On 2011/11/13 01:28:22, Mark Mentovai wrote:
> Can you think of a good way to do this?

If I'm honest I don't know "what" I'm looking for. I played around with using
typeid(value_specific).name() but that just seems to stacktrace in interesting
ways.

Now that it's not a specialisation I don't think it should use CFGetTypeID
against value_specific.

I'll look a little deeper myself first and see how far I get.

http://codereview.chromium.org/8540021/diff/8001/base/mac/foundation_util.mm
File base/mac/foundation_util.mm (right):

http://codereview.chromium.org/8540021/diff/8001/base/mac/foundation_util.mm#...
base/mac/foundation_util.mm:225: CFCopyTypeIDDescription(expected_type));
On 2011/11/13 01:28:22, Mark Mentovai wrote:
> Why did you change the correct four-space indent on this line to something
else?

I'm not entirely sure. I must have done it manually for some reason because my
Emacs is set to follow the style guide.

[Mark Mentovai, 2011-11-14 14:12:05]

http://codereview.chromium.org/8540021/diff/8001/base/mac/foundation_util.h
File base/mac/foundation_util.h (right):

http://codereview.chromium.org/8540021/diff/8001/base/mac/foundation_util.h#n...
base/mac/foundation_util.h:294: // TODO: Get the string value of the type name
T.
KushalP wrote:
> On 2011/11/13 01:28:22, Mark Mentovai wrote:
> > Can you think of a good way to do this?
> 
> If I'm honest I don't know "what" I'm looking for. I played around with using
> typeid(value_specific).name() but that just seems to stacktrace in interesting
> ways.
> 
> Now that it's not a specialisation I don't think it should use CFGetTypeID
> against value_specific.
> 
> I'll look a little deeper myself first and see how far I get.

You want a function that can be called like

std::string TypeNameForCFType(CFArrayRef cfo);
std::string TypeNameForCFType(CFDictionaryRef cfo);
std::string TypeNameForCFType(CFStringRef cfo);

And that returns "CFArray", "CFDictionary", and "CFString" in these cases. Does
that help put you on the right track?

[KushalP, 2011-11-14 15:03:29]

On 2011/11/14 14:12:05, Mark Mentovai wrote:
> You want a function that can be called like
> 
> std::string TypeNameForCFType(CFArrayRef cfo);
> std::string TypeNameForCFType(CFDictionaryRef cfo);
> std::string TypeNameForCFType(CFStringRef cfo);
> 
> And that returns "CFArray", "CFDictionary", and "CFString" in these cases.
Does
> that help put you on the right track?

Yup. That makes much more sense. However my first thought to implement such a
function would be to aim for template specialisation.

Is this how you intended it to be implemented? Or is there a cleaner way that
I've missed?

[Mark Mentovai, 2011-11-14 15:12:53]

You can't do it without writing the function body (perhaps by macro) for
each type because there is no function (templatized or overloaded) that
returns the type name for its argument. The function you're looking to
provide fills that void. That's why you can use it from a templatized
function (GetValueFromDictionary) but it can't be templatized itself.

(It can be a macro-generated function body that offers either an overload,
like TypeNameForCFType(CFManyTypes), or it can be a template specialization
for each type, like TypeNameForCFType<CFWhateverRef>(CFWhateverRef). I see
no reason to use the template for this implementation detail.)

[KushalP, 2011-11-14 18:03:24]

This now seems to work as expected and is ready for another review. Thanks for
your help Mark!

[Mark Mentovai, 2011-11-14 18:08:55]

http://codereview.chromium.org/8540021/diff/15002/base/mac/foundation_util.h
File base/mac/foundation_util.h (right):

http://codereview.chromium.org/8540021/diff/15002/base/mac/foundation_util.h#...
base/mac/foundation_util.h:300: ScopedCFTypeRef<CFStringRef>
expected_type_ref(CFStringCreateWithCString(
Why are you turning this into a CFStringRef, when
GetValueFromDictionaryErrorMessage will just turn it back into a straight
string?

http://codereview.chromium.org/8540021/diff/15002/base/mac/foundation_util.mm
File base/mac/foundation_util.mm (right):

http://codereview.chromium.org/8540021/diff/15002/base/mac/foundation_util.mm...
base/mac/foundation_util.mm:205: return type_name + "Ref"; \
The values returned by CFCopyTypeIDDescription don’t have a Ref appended, do
they?

http://codereview.chromium.org/8540021/diff/15002/base/mac/foundation_util.mm...
base/mac/foundation_util.mm:220: CFStringRef expected_type_ref,
Do something about this cheesey alignment.

[KushalP, 2011-11-14 18:47:02]

http://codereview.chromium.org/8540021/diff/15002/base/mac/foundation_util.mm
File base/mac/foundation_util.mm (right):

http://codereview.chromium.org/8540021/diff/15002/base/mac/foundation_util.mm...
base/mac/foundation_util.mm:205: return type_name + "Ref"; \
On 2011/11/14 18:08:55, Mark Mentovai wrote:
> The values returned by CFCopyTypeIDDescription don’t have a Ref appended, do
> they?

No. They do not. Fixed.

[Mark Mentovai, 2011-11-14 18:59:28]

http://codereview.chromium.org/8540021/diff/14003/base/mac/foundation_util.h
File base/mac/foundation_util.h (right):

http://codereview.chromium.org/8540021/diff/14003/base/mac/foundation_util.h#...
base/mac/foundation_util.h:117: std::string expected_type_ref,
This can accept a const&, can’t it?

[Mark Mentovai, 2011-11-14 19:29:14]

http://codereview.chromium.org/8540021/diff/14003/base/mac/foundation_util.h
File base/mac/foundation_util.h (right):

http://codereview.chromium.org/8540021/diff/14003/base/mac/foundation_util.h#...
base/mac/foundation_util.h:101: std::string TypeNameForCFType(TypeCF##Ref cfo);
The argument is unused and thus doesn’t even need to be named. Here and in the
definition in the .mm file.

http://codereview.chromium.org/8540021/diff/14003/base/mac/foundation_util.h#...
base/mac/foundation_util.h:113: 
You should #undef TYPE_NAME_FOR_CF_TYPE_DECL at this point.

See
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Prepro...
.

You can add #undefs for the other macros used the same way in this .h file and
the .mm file.

http://codereview.chromium.org/8540021/diff/14003/base/mac/foundation_util.h#...
base/mac/foundation_util.h:117: std::string expected_type_ref,
This can be const std::string&, right?

There’s nothing “reffy” about it, the variable can just be |expected_type|.

http://codereview.chromium.org/8540021/diff/14003/base/mac/foundation_util.h#...
base/mac/foundation_util.h:122: BASE_EXPORT CFTypeRef
GetValueFromDictionary(CFDictionaryRef dict,
The goal is to entirely replace this function, correct?

If so, mark it as deprecated in this change, and your next change should be a
simple no-contest change to convert all existing callers to the new API.

http://codereview.chromium.org/8540021/diff/14003/base/mac/foundation_util.h#...
base/mac/foundation_util.h:289: // Utility function to pull out a value from a
dictionary, check its type, and
Why is this all the way down here, and not up with the GetValueWithDictionary
that it will replace?

http://codereview.chromium.org/8540021/diff/14003/base/mac/foundation_util.h#...
base/mac/foundation_util.h:291: // Is a cleaner implementation of
base::mac::GetValueFromDictionary() above.
This is a fragment, not a sentence. But if you’re be marking the other one as
deprecated and removing it, you don’t need this line at all.

http://codereview.chromium.org/8540021/diff/14003/base/mac/foundation_util.h#...
base/mac/foundation_util.h:303: return value_specific;
Add a blank line before this because this is conceptually a new “paragraph.”

http://codereview.chromium.org/8540021/diff/14003/base/mac/foundation_util_un...
File base/mac/foundation_util_unittest.mm (right):

http://codereview.chromium.org/8540021/diff/14003/base/mac/foundation_util_un...
base/mac/foundation_util_unittest.mm:286: CFNumberCreate(kCFAllocatorDefault,
kCFNumberIntType, &one),
Looks like a leak to me.

http://codereview.chromium.org/8540021/diff/14003/base/mac/foundation_util_un...
base/mac/foundation_util_unittest.mm:316: CFSTR("four")));
Bad alignment. Line 318 too.

http://codereview.chromium.org/8540021/diff/14003/base/mac/foundation_util_un...
base/mac/foundation_util_unittest.mm:320: //
base::mac::GetValueFromDictionary<>(_, _) should have the same
You will get rid of the three-argument form, so get rid of the rest of this
function from this point forward.

[KushalP, 2011-11-14 20:37:13]

http://codereview.chromium.org/8540021/diff/14003/base/mac/foundation_util.h
File base/mac/foundation_util.h (right):

http://codereview.chromium.org/8540021/diff/14003/base/mac/foundation_util.h#...
base/mac/foundation_util.h:122: BASE_EXPORT CFTypeRef
GetValueFromDictionary(CFDictionaryRef dict,
On 2011/11/14 19:29:14, Mark Mentovai wrote:
> The goal is to entirely replace this function, correct?
> 
> If so, mark it as deprecated in this change, and your next change should be a
> simple no-contest change to convert all existing callers to the new API.

How do I mark as deprecated in this case? Is it just adding __attribute__
((deprecated)) to the end of the method?

http://codereview.chromium.org/8540021/diff/14003/base/mac/foundation_util.h#...
base/mac/foundation_util.h:289: // Utility function to pull out a value from a
dictionary, check its type, and
On 2011/11/14 19:29:14, Mark Mentovai wrote:
> Why is this all the way down here, and not up with the GetValueWithDictionary
> that it will replace?

because it uses CFCast which isn't defined above this point.

http://codereview.chromium.org/8540021/diff/14003/base/mac/foundation_util_un...
File base/mac/foundation_util_unittest.mm (right):

http://codereview.chromium.org/8540021/diff/14003/base/mac/foundation_util_un...
base/mac/foundation_util_unittest.mm:286: CFNumberCreate(kCFAllocatorDefault,
kCFNumberIntType, &one),
On 2011/11/14 19:29:14, Mark Mentovai wrote:
> Looks like a leak to me.

How so? What did I do wrong here?

[Mark Mentovai, 2011-11-14 20:49:21]

http://codereview.chromium.org/8540021/diff/14003/base/mac/foundation_util.h
File base/mac/foundation_util.h (right):

http://codereview.chromium.org/8540021/diff/14003/base/mac/foundation_util.h#...
base/mac/foundation_util.h:122: BASE_EXPORT CFTypeRef
GetValueFromDictionary(CFDictionaryRef dict,
KushalP wrote:
> How do I mark as deprecated in this case? Is it just adding __attribute__
> ((deprecated)) to the end of the method?

Just put a comment in.

And it won’t stay there for long, because your next change will be to convert
all existing callers over to the new API.

[Mark Mentovai, 2011-11-14 20:50:19]

http://codereview.chromium.org/8540021/diff/14003/base/mac/foundation_util_un...
File base/mac/foundation_util_unittest.mm (right):

http://codereview.chromium.org/8540021/diff/14003/base/mac/foundation_util_un...
base/mac/foundation_util_unittest.mm:286: CFNumberCreate(kCFAllocatorDefault,
kCFNumberIntType, &one),
KushalP wrote:
> On 2011/11/14 19:29:14, Mark Mentovai wrote:
> > Looks like a leak to me.
> 
> How so? What did I do wrong here?

Who releases these three CFNumbers?

[KushalP, 2011-11-14 21:04:12]

http://codereview.chromium.org/8540021/diff/14003/base/mac/foundation_util.h
File base/mac/foundation_util.h (right):

http://codereview.chromium.org/8540021/diff/14003/base/mac/foundation_util.h#...
base/mac/foundation_util.h:122: BASE_EXPORT CFTypeRef
GetValueFromDictionary(CFDictionaryRef dict,
On 2011/11/14 20:49:22, Mark Mentovai wrote:
> KushalP wrote:
> > How do I mark as deprecated in this case? Is it just adding __attribute__
> > ((deprecated)) to the end of the method?
> 
> Just put a comment in.
> 
> And it won’t stay there for long, because your next change will be to convert
> all existing callers over to the new API.

Not in one change I hope! That'll be git rebase hell until it's committed.

http://codereview.chromium.org/8540021/diff/14003/base/mac/foundation_util_un...
File base/mac/foundation_util_unittest.mm (right):

http://codereview.chromium.org/8540021/diff/14003/base/mac/foundation_util_un...
base/mac/foundation_util_unittest.mm:286: CFNumberCreate(kCFAllocatorDefault,
kCFNumberIntType, &one),
On 2011/11/14 20:50:19, Mark Mentovai wrote:
>
> Who releases these three CFNumbers?

It's owned by values because of the CF "create rule" and the function has
"Create" in its name.

[Mark Mentovai, 2011-11-14 21:09:01]

http://codereview.chromium.org/8540021/diff/14003/base/mac/foundation_util.h
File base/mac/foundation_util.h (right):

http://codereview.chromium.org/8540021/diff/14003/base/mac/foundation_util.h#...
base/mac/foundation_util.h:122: BASE_EXPORT CFTypeRef
GetValueFromDictionary(CFDictionaryRef dict,
KushalP wrote:
> On 2011/11/14 20:49:22, Mark Mentovai wrote:
> > KushalP wrote:
> > > How do I mark as deprecated in this case? Is it just adding __attribute__
> > > ((deprecated)) to the end of the method?
> > 
> > Just put a comment in.
> > 
> > And it won’t stay there for long, because your next change will be to
convert
> > all existing callers over to the new API.
> 
> Not in one change I hope! That'll be git rebase hell until it's committed.

I don’t know or care about that, but I mean “change” in the sense that other
developers will see, that is, svn commits.

http://codereview.chromium.org/8540021/diff/14003/base/mac/foundation_util_un...
File base/mac/foundation_util_unittest.mm (right):

http://codereview.chromium.org/8540021/diff/14003/base/mac/foundation_util_un...
base/mac/foundation_util_unittest.mm:286: CFNumberCreate(kCFAllocatorDefault,
kCFNumberIntType, &one),
KushalP wrote:
> On 2011/11/14 20:50:19, Mark Mentovai wrote:
> >
> > Who releases these three CFNumbers?
> 
> It's owned by values because of the CF "create rule" and the function has
> "Create" in its name.

values is a simple C array, it doesn’t “own” anything.

[KushalP, 2011-11-14 21:14:39]

http://codereview.chromium.org/8540021/diff/14003/base/mac/foundation_util_un...
File base/mac/foundation_util_unittest.mm (right):

http://codereview.chromium.org/8540021/diff/14003/base/mac/foundation_util_un...
base/mac/foundation_util_unittest.mm:286: CFNumberCreate(kCFAllocatorDefault,
kCFNumberIntType, &one),
On 2011/11/14 21:09:01, Mark Mentovai wrote:
> 
> values is a simple C array, it doesn’t “own” anything.

I'll wrap these in ScopedCFTypeRefs

[KushalP, 2011-11-14 21:46:10]

The most recent change covers your previous comments.

Only thing I'm unsure about is which proper C++ cast you were referring to in
foundation_util_unittest.mm.

[Mark Mentovai, 2011-11-14 21:57:18]

Otherwise this is now fine.

http://codereview.chromium.org/8540021/diff/14008/base/mac/foundation_util.h
File base/mac/foundation_util.h (right):

http://codereview.chromium.org/8540021/diff/14008/base/mac/foundation_util.h#...
base/mac/foundation_util.h:123: // This is now deprecated.
This is now deprecated in favor of the two-argument form below.

File a bug for the cleanup and put a TODO(kushi.p) on it saying when you expect
to have the callers cleaned up by so that the old form may be removed.

http://codereview.chromium.org/8540021/diff/14008/base/mac/foundation_util.h#...
base/mac/foundation_util.h:223: 
You can #undef three macros at this point as well.

[KushalP, 2011-11-14 22:07:45]

Updated again with new nits covered.

I've posted the bug here: crbug.com/104200

[Mark Mentovai, 2011-11-14 22:09:42]

http://codereview.chromium.org/8540021/diff/14008/base/mac/foundation_util_un...
File base/mac/foundation_util_unittest.mm (right):

http://codereview.chromium.org/8540021/diff/14008/base/mac/foundation_util_un...
base/mac/foundation_util_unittest.mm:298: (const void**)keys,
Can’t these both be C++-style casts?

reinterpret_cast<const void**>(keys),
reinterpret_cast<const void**>(values),

The style guide has a very strong preference for explicit C++-style casts over
“anything goes” C-style ones.

See
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Castin...
.

[Mark Mentovai, 2011-11-14 22:11:37]

http://codereview.chromium.org/8540021/diff/14009/base/mac/foundation_util.h
File base/mac/foundation_util.h (right):

http://codereview.chromium.org/8540021/diff/14009/base/mac/foundation_util.h#...
base/mac/foundation_util.h:124: // TODO(kushi.p)
Please finish this comment by referencing your newly-filed bug and saying what
the TODO means.

Reference:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Castin...

Further reference:
https://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thr...

[Mark Mentovai, 2011-11-14 22:13:21]

http://codereview.chromium.org/8540021/diff/14009/base/mac/foundation_util.h
File base/mac/foundation_util.h (right):

http://codereview.chromium.org/8540021/diff/14009/base/mac/foundation_util.h#...
base/mac/foundation_util.h:226: #undef CF_TO_NS_MUTABLE_CAST_DECL
I said three, right?

How about OBJC_CPP_CLASS_DECL?

[KushalP, 2011-11-14 22:20:46]

Updated again.

http://codereview.chromium.org/8540021/diff/14009/base/mac/foundation_util.h
File base/mac/foundation_util.h (right):

http://codereview.chromium.org/8540021/diff/14009/base/mac/foundation_util.h#...
base/mac/foundation_util.h:226: #undef CF_TO_NS_MUTABLE_CAST_DECL
On 2011/11/14 22:13:21, Mark Mentovai wrote:
> I said three, right?
> 
> How about OBJC_CPP_CLASS_DECL?

Ah, thought you just mean this 10 line block here.

[Mark Mentovai, 2011-11-14 22:28:08]

This is now LGTM. I’m hitting the CQ button.

[commit-bot: I haz the power, 2011-11-14 22:28:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kushi.p@gmail.com/8540021/17008

[commit-bot: I haz the power, 2011-11-15 00:03:55]

Change committed as 109985