[webkit-reviews] review granted: [Bug 132111] Text caret disappears in Mail after returning from another application : [Attachment 230059] Fixes the bug

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 132111: Text caret disappears in Mail after returning from another
application
https://bugs.webkit.org/show_bug.cgi?id=132111

Attachment 230059: Fixes the bug
https://bugs.webkit.org/attachment.cgi?id=230059&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=230059&action=review


I’m saying review+ but I am not sure about a few aspects of the patch.

> Source/WebKit/mac/WebView/WebView.mm:5227
> +	   [[NSNotificationCenter defaultCenter] addObserver:self
selector:@selector(_windowKeyStateChanged:)
> +	       name:NSWindowDidBecomeKeyNotification object:nil];
> +	   [[NSNotificationCenter defaultCenter] addObserver:self
selector:@selector(_windowKeyStateChanged:)
> +	       name:NSWindowDidResignKeyNotification object:nil];

This says we want a notification any time *any* window in this app becomes or
resigns key, since we pass an object of nil. Is that really what we want? I
think we want to pass object:window here, but then we’ll have to retest to be
sure this actually fixes the bug.

> Source/WebKit/mac/WebView/WebView.mm:-5335
> -- (void)_windowChangedKeyState

I’m not sure exactly why I overrode this internal AppKit method in 2007 rather
than using the notifications. I am concerned that this might have significantly
different behavior from the old code in ways that go beyond fixing this bug.

I think that in 2007 this exactly matched how AppKit itself kept controls up to
date, so maybe that’s why I did it that way.

> Source/WebKit/mac/WebView/WebView.mm:5342
> +- (void)_windowKeyStateChanged:(NSNotification *)notification

I know WebKit does this all over the place, but it’s dangerous to use an
underscore prefix because this could easily conflict with a same-named method
inside a future version of AppKit. Given that names without prefixes and names
with an underscore prefix are both fair game for both AppKit and for apps who
might subclass WebView, we should probably use some kind of crazy wk_ prefix on
our internal methods. Sorry, not really specific to this patch, and really ugly
so maybe others on the project won’t agree.