[Webkit-unassigned] [Bug 28037] SocketStreamHandle interface for WebSocket API

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=28037


Alexey Proskuryakov <ap at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #34646|review?                     |review-
               Flag|                            |




--- Comment #9 from Alexey Proskuryakov <ap at webkit.org>  2009-08-12 10:24:41 PDT ---
(From update of attachment 34646)
+        No new tests. (OOPS!)

Please don't say OOPS in ChangeLog - pre-commit script will block such a patch
from landing. I think it would be better to say something positive anyway, like
"tests will be landed once this code is complete and functional).

     WebCore/bindings/js/JSWebSocketCustom.cpp \
     WebCore/websockets/WebSocket.cpp \
-    WebCore/websockets/WebSocket.h
+    WebCore/websockets/WebSocket.h \
+    WebCore/platform/network/SocketStreamErrorBase.cpp \

Looks like this list is supposed to be sorted.

             children = (
+                510D4A391031665F0049EA54 /* SocketStreamHandleMac.mm */,
+                510D4A2D103165EE0049EA54 /* SocketStreamErrorBase.cpp */,
+                510D4A2E103165EE0049EA54 /* SocketStreamErrorBase.h */,
+                510D4A2F103165EE0049EA54 /* SocketStreamHandle.h */,
+                510D4A30103165EE0049EA54 /* SocketStreamHandleBase.cpp */,
+                510D4A31103165EE0049EA54 /* SocketStreamHandleBase.h */,
+                510D4A32103165EE0049EA54 /* SocketStreamHandleClient.h */,
                 B2F34FE70E82F81700F627CD /* cf */,
                 656B84E70AEA1DAE00A095B4 /* mac */,

Shouldn't SocketStreamHandleMac be in mac folder? And sorry for not realizing
this before, but I would actually prefer to have "cf" stubs for SocketStream,
not mac ones.

+ * Copyright (C) 2009 Google Inc. All rights reserved.
<...>
+ * THIS SOFTWARE IS PROVIDED BY APPLE COMPUTER, INC. ``AS IS'' AND ANY
+ * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE

I'm not sure what template is right for files originally added by Google, but
"Apple Computer, Inc." is not even the correct name for Apple any more. You've
previously used a different header in WebSocket.h.

+const int maxBufferAmount = 524288;

I've been thinking of a much larger buffer, something like 100MB (for desktop
platforms at least). The idea is to prevent the browser from bringing the whole
system to its knees too easily, not to close the connection as soon as it seems
that data is sent too quickly.

"Buffer amount" doesn't sound like proper English to me - should be either
"buffer size" or "maximum buffered amount".

+            // FIXME: set error code?

There is no error code in SocketStreamHandleBase, so I don't understand what
this FIXME is about. Could you re-phrase the comment to make it clearer what
the concern/bug is?

Perhaps an error should just be logged to console via a client didFail ()
callback.

+        int bufferedAmount() const { return m_buffer.size(); }
+
+        SocketStreamHandleClient* client() const;

It's surprising that one is inline, and the other is not. Not really a problem
but was there a reason for this difference?

> diff --git a/WebCore/platform/network/SocketStreamHandle.h b/WebCore/platform/network/SocketStreamHandle.h

Why is SocketStreamHandle.h in cross-platform directory? The model that e.g.
ResourceRequest.h uses seems superior to me - cross-platform parts go into
Base, and platforms have both headers and cpp files of their own.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.