Patch forthcoming
Created attachment 71107 [details] proposed patch
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.
(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);
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.