Bug 47864 - Convert WebNSUserDefaultsExtras.m to .mm
Summary: Convert WebNSUserDefaultsExtras.m to .mm
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh All
: P2 Normal
Assignee: Alexey Proskuryakov
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-10-18 17:36 PDT by Alexey Proskuryakov
Modified: 2010-10-19 11:00 PDT (History)
1 user (show)

See Also:


Attachments
proposed patch (21.42 KB, patch)
2010-10-18 17:45 PDT, Alexey Proskuryakov
ddkilzer: review+
ddkilzer: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Proskuryakov 2010-10-18 17:36:31 PDT
Patch forthcoming
Comment 1 Alexey Proskuryakov 2010-10-18 17:45:16 PDT
Created attachment 71107 [details]
proposed patch
Comment 2 David Kilzer (:ddkilzer) 2010-10-19 10:36:32 PDT
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.
Comment 3 David Kilzer (:ddkilzer) 2010-10-19 10:38:16 PDT
(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);
Comment 4 Alexey Proskuryakov 2010-10-19 11:00:11 PDT
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.