[webkit-reviews] review denied: [Bug 65214] REGRESSION (Safari 5.1): JavaScript dialogs not usable with VoiceOver : [Attachment 102068] patch

[bugzilla-daemon at webkit.org]

Anders Carlsson <andersca at apple.com> has denied chris fleizach
<cfleizach at apple.com>'s request for review:
Bug 65214: REGRESSION (Safari 5.1): JavaScript dialogs not usable with
VoiceOver
https://bugs.webkit.org/show_bug.cgi?id=65214

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

------- Additional Comments from Anders Carlsson <andersca at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=102068&action=review


Overall, I really like this patch; this is pretty much how I'd have implemented
it myself. Great job, Chris! I'll r- it so you can upload a new patch with the
suggested changes.

> Source/WebKit2/Platform/CoreIPC/Connection.cpp:477
> +#if PLATFORM(MAC)
> +	       RunLoop::current()->runForDuration(.25);
> +	       timeout = currentTime() >= absoluteTime;
>  #endif

It looks like this will do a busy-wait and wake up the CPU 4 times a second
when we're in this state.

Fixing this is tricky - we'd need a way to know when a nested run loop is
running so we can quit it when we time out or receive a response.
I think for now you could just run the run loop forever and add a FIXME to fix
this (since this is only used with messages that have an infinite timeout).

>> Source/WebKit2/Platform/CoreIPC/Connection.h:212
>> +	PassOwnPtr<ArgumentDecoder> sendSyncMessage(MessageID, uint64_t
syncRequestID, PassOwnPtr<ArgumentEncoder>, double timeout, bool
continueRunLoop = false);
> 
> Local variables should never be PassOwnPtr (see
http://webkit.org/coding/RefPtr.html).	[readability/pass_ptr] [5]

Instead of using a boolean here, it would be better to add a flags enum,
similar to the MessageSendFlags enum we have for async messages. You could call
it SyncMessageSendFlags and use an unsigned flags = 0 instead of the boolean
parameter.