[webkit-reviews] review granted: [Bug 47864] Convert WebNSUserDefaultsExtras.m to .mm : [Attachment 71107] proposed patch

[bugzilla-daemon at webkit.org]

David Kilzer (ddkilzer) <ddkilzer at webkit.org> has granted Alexey Proskuryakov
<ap at webkit.org>'s request for review:
Bug 47864: Convert WebNSUserDefaultsExtras.m to .mm
https://bugs.webkit.org/show_bug.cgi?id=47864

Attachment 71107: proposed patch
https://bugs.webkit.org/attachment.cgi?id=71107&action=review

------- Additional Comments from David Kilzer (ddkilzer) <ddkilzer at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=71107&action=review

r=me, but consider the suggestions below.

> WebKit/ChangeLog:5
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   Need a short description and bug URL (OOPS!)

Need bug link, bug title and description here.

> WebKit/mac/ChangeLog:10
> +	   - Fixed notification center observer to actually work (previsouly,
it only picked up changes

Typo: "previsouly" should be "previously".

> WebKit/mac/Misc/WebNSUserDefaultsExtras.mm:55
> +    NSMutableString *result = [[lowercaseLanguageCode mutableCopy]
autorelease];
>      [result replaceCharactersInRange:NSMakeRange(2, 1) withString:@"-"];
> -    return [result autorelease];
> +    return [result copy];

Returning a non-mutable copy of an autoreleased, mutable copy seems to create
unnecessary churn.  Why can't you just return the autoreleased, mutable object?
  I've never heard of any issue returning an NSMutableString* when the method
signature expects NSString*.

> WebKit/mac/Misc/WebNSUserDefaultsExtras.mm:61
> +static BOOL languageChangeObserverAdded = NO;

I'm pretty sure we try to use a 'bool' C++ type when possible, so there's no
need to use 'BOOL' here.

I think this variable could also be moved inside the static method instead of
leaving it out here.  More suggestions below.

> WebKit/mac/Misc/WebNSUserDefaultsExtras.mm:85
> +	   if ([languages count] == 0)

Optional (I seem to recall a discussion on webkit-dev about this, but I had no
strong opinions either way):

    if (![language count])

> WebKit/mac/Misc/WebNSUserDefaultsExtras.mm:94
> +	   addLanguageChangeObserver();

Why not just inline addLanguageChangeObserver()?  It only contains a single
method call.

Or better yet, move the static languageChangeObserverAdded variable here an
initialize it by calling addLanguageChangeObserver(), and change that method to
return a bool:

    static bool languageChangeObserverAdded = addLanguageChangeObserver();

Then you get the one-time initialization for "free" thanks to C++.  The only
thing to worry about is the compiler eliminating this as "dead code", which I
don't think it should do.