[webkit-reviews] review granted: [Bug 130767] Connection::dispatchOneMessage() can be re-entered while handling Cmd-key menu equivalents, ASSERT(!_data->_keyDownEventBeingResent) : [Attachment 227831] proposed patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Alexey Proskuryakov <ap at webkit.org>'s
request for review:
Bug 130767: Connection::dispatchOneMessage() can be re-entered while handling
Cmd-key menu equivalents, ASSERT(!_data->_keyDownEventBeingResent)
https://bugs.webkit.org/show_bug.cgi?id=130767

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

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


Seems OK. Tricky to decide which run loop modes we want to omit. In the past,
it was important to include NSEventTrackingRunLoopMode so that web page loading
didn’t stop any time the mouse was down in another window, but it’s always been
a tricky tradeoff.

> Source/WTF/wtf/RunLoop.h:149
> +    RetainPtr<CFMutableSetRef> m_additionalTimerModes;

Seems a little over-general to use a set for this given that it’s always either
null or just a set with a single entry, NSModalPanelRunLoopMode. I suppose it’s
nice to be general purpose, but maybe supporting just a single additional mode
is OK. I can even imagine hard-coding this one mode and just using a boolean.

> Source/WebKit2/Shared/WebKit2Initialize.cpp:44
> +#if PLATFORM(MAC)
> +extern "C" CFStringRef NSModalPanelRunLoopMode;
> +#endif

I don’t think this declaration is a good idea. Giving the linker the wrong type
and depending on toll-free bridging is unnecessarily ugly. We should do this in
a .mm file where we can access the real constant in NSApplication.h.

Might be worth considering a helper function, maybe somewhere in
WebCore/platform, that people can use to do this from cpp files.

> Source/WebKit2/Shared/WebKit2Initialize.cpp:62
>      RunLoop::initializeMainRunLoop();
> +#if PLATFORM(MAC)
> +    RunLoop::main().addModeForWakeUpAndTimers(NSModalPanelRunLoopMode);
> +#endif

I suggest exporting a function that does these two things together.

> Source/WebKit/mac/Carbon/CarbonWindowAdapter.mm:276
>      JSC::initializeThreading();
>      WTF::initializeMainThreadToProcessMainThread();
>      RunLoop::initializeMainRunLoop();
> +   
RunLoop::main().addModeForWakeUpAndTimers((CFStringRef)NSModalPanelRunLoopMode)
;

I think this whole sequence should be a single helper function rather than
putting up with having the code copied and pasted into 20 different classes.