Bug 47864 – Convert WebNSUserDefaultsExtras.m to .mm

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Alexey Proskuryakov]

Patch forthcoming

[Alexey Proskuryakov]

Created attachment 71107 [details]
proposed patch

[David Kilzer (:ddkilzer)]

Comment on attachment 71107 [details]
proposed patch

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.

[David Kilzer (:ddkilzer)]

(In reply to comment #2)
> 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.

And add an assert for debug builds:

    ASSERT(languageChangeObserverAdded);

[Alexey Proskuryakov]

Committed <http://trac.webkit.org/changeset/70069>. Made most suggested changes, but not the deeper refactoring of how the observer is registered. This is going to be redone soon for WebKit2 anyway.