[webkit-reviews] review denied: [Bug 38171] WebSocket needs to suspend/resume as Active DOM object. : [Attachment 54390] Patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Fumitoshi Ukai
<ukai at chromium.org>'s request for review:
Bug 38171: WebSocket needs to suspend/resume as Active DOM object.
https://bugs.webkit.org/show_bug.cgi?id=38171

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

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
+    , m_suspend(false)

Probably should be m_suspended.

+bool WebSocket::canSuspend() const
+{
+    return true;
+}

There are currently three uses for suspend/resume that I know of:

1. Suspend behind modal dialogs and alerts, so that program state wouldn't
change during an alert() call. WebSocket definitely needs that.

2. Suspend when a page goes into back/forward cache. One can't practically
suspend a socket for that long, so open WebSocket objects should prevent a page
from going to b/f cache. Fo this, canSuspend should return false - see how it's
done for XMLHttpRequest.

3. Suspend during a pause in debugger. I don't know much about how that works.

So in short, canSuspend() should return !m_channel.

+void WebSocket::suspend()
+{
+    m_suspend = true;
+}

I don't think it's sufficient. We need to make sure that readyState and other
properties don't change during an alert or modalDialog() calls.

-    dispatchEvent(Event::create(eventNames().openEvent, false, false));
+    enqueueEvent(Event::create(eventNames().openEvent, false, false));

The enqueueEvent() name makes it sound like some kind of deferred dispatch,
like "post a task to fire a simple event" in HTML5 parlance. But that's not how
it generally behaves.

+	 Deque<RefPtr<Event> > m_pendingEvents;

Deque is not a great container to use here. We don't do random append/remove in
normal case - we fill the whole container first, and then empty it. See similar
patterns in Document and HTMLMediaElement.

Of these comments, the most difficult to address is of course insufficiency of
a boolean flag. I don't know how to best fix that, perhaps one needs to rely of
platform specific code in SocketStreamHandle, similarly to what we do for
XMLHttpRequest.