[webkit-reviews] review granted: [Bug 89590] showModalDialog message handling is flaky in WebKit2 : [Attachment 148621] Patch v1 - Proposed fix

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Brady Eidson <beidson at apple.com>'s
request for review:
Bug 89590: showModalDialog message handling is flaky in WebKit2
https://bugs.webkit.org/show_bug.cgi?id=89590

Attachment 148621: Patch v1 - Proposed fix
https://bugs.webkit.org/attachment.cgi?id=148621&action=review

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


> Source/WebCore/platform/RunLoop.cpp:59
>  void RunLoop::performWork()
>  {

This function needs a why comment explaining the importance of only fetching
one function from the queue at a time.

The change log does say that this is needed to fix the bug, but the why comment
could say something that specifically talks about the fact that during one of
the functions we may make another call to RunLoop::performWork and that has to
pick up where we left off.

>> Source/WebCore/platform/RunLoop.cpp:63
>> +	    {
> 
> This { should be at the end of the previous line  [whitespace/braces] [4]

This is a stylebot bug and should be reported to the stylebot folks.

>> Source/WebCore/platform/RunLoop.h:36
>> +#include <wtf/Deque.h>
> 
> Alphabetical sorting problem.  [build/include_order] [4]

Please sort this correctly.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:3625
> +    process()->connection()->wakeUpRunLoop();

This needs a why comment. Even the change log doesn’t say why, and in fact, I
don’t know why! I’m sure it’s easy to explain.