[webkit-reviews] review denied: [Bug 55959] REGRESSION (WebKit2): VoiceOver focus no longer follows keyboard focus : [Attachment 85250] patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied chris fleizach <cfleizach at apple.com>'s
request for review:
Bug 55959: REGRESSION (WebKit2): VoiceOver focus no longer follows keyboard
focus
https://bugs.webkit.org/show_bug.cgi?id=55959

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

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

> Source/WebKit2/WebProcess/WebProcess.cpp:465
> +#if PLATFORM(MAC)
> +    Vector<RefPtr<WebPage> > pages;
> +    copyValuesToVector(m_pageMap, pages);
> +    for (size_t i = 0; i < pages.size(); ++i)
> +	   if (pages[i]->windowIsFocused())
> +	       return pages[i].get();
> +#endif
> +    return 0;

Is always zero correct for non-Mac platforms? Or is this just not implemented
for those platforms? Can’t we just compile this function on all platforms? If
we are having some problem compiling it on some of those other platforms where
it’s also not called, we could just put the #if around the entire function
definition instead, which I think would be clearer.

Do we really need to copy the map to a vector? Why can’t we just iterate the
map? Can the windowIsFocused function change the contents of the map?

Need braces around the body of the for loop.

> Source/WebKit2/WebProcess/mac/NSApplicationOverride.mm:32
> + at implementation NSApplication (NSApplicationOverride)

Defining a category is not a reliable way to override the implementation of a
method in an AppKit class even though it often seems to work. The reliable
method involves method_setImplementation and is done in source files such as
WebPluginController.mm, WebHTMLView.mm, and WebView.mm in WebKit.

> Source/WebKit2/WebProcess/mac/NSApplicationOverride.mm:39
> +    if (!WebKit::WebProcess::shared().isSeparateProcess())

It’s normally better to put using namespace WebKit at the start of a file
rather than doing WebKit:: everywhere.