Bug 28037 - SocketStreamHandle interface for WebSocket API
: SocketStreamHandle interface for WebSocket API
Status: RESOLVED FIXED
: WebKit
WebCore Misc.
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2009-08-05 22:34 PST by
Modified: 2009-09-09 19:40 PST (History)


Attachments
Add SocketStreamHandle interface (36.14 KB, patch)
2009-08-11 02:42 PST, Fumitoshi Ukai
no flags Review Patch | Details | Formatted Diff | Diff
Add SocketStreamHandle interface (36.57 KB, patch)
2009-08-12 02:02 PST, Fumitoshi Ukai
no flags Review Patch | Details | Formatted Diff | Diff
Add SocketStreamHandle interface (36.58 KB, patch)
2009-08-12 02:09 PST, Fumitoshi Ukai
no flags Review Patch | Details | Formatted Diff | Diff
Add SocketStreamHandle interface (44.28 KB, patch)
2009-08-19 04:35 PST, Fumitoshi Ukai
no flags Review Patch | Details | Formatted Diff | Diff
Add SocketStreamHandle interface (43.80 KB, patch)
2009-08-19 22:54 PST, Fumitoshi Ukai
no flags Review Patch | Details | Formatted Diff | Diff
Add SocketStreamHandle interface (43.69 KB, patch)
2009-08-23 19:50 PST, Fumitoshi Ukai
no flags Review Patch | Details | Formatted Diff | Diff
Add SocketStreamHandle interface (43.71 KB, patch)
2009-08-24 19:43 PST, Fumitoshi Ukai
no flags Review Patch | Details | Formatted Diff | Diff
Add SocketStreamHandle interface (43.71 KB, patch)
2009-08-24 22:51 PST, Fumitoshi Ukai
no flags Review Patch | Details | Formatted Diff | Diff
Add SocketStreamHandle interface (43.72 KB, patch)
2009-08-25 19:41 PST, Fumitoshi Ukai
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2009-08-05 22:34:58 PST
SocketStreamHandle will be used for backend of WebSocket API.
These will be added in WebCore/platform/network
------- Comment #1 From 2009-08-11 02:42:20 PST -------
Created an attachment (id=34545) [details]
Add SocketStreamHandle interface


---
 13 files changed, 736 insertions(+), 1 deletions(-)
------- Comment #2 From 2009-08-11 08:38:47 PST -------
(From update of attachment 34545 [details])
Simply a style pass. I looked at this and saw a few style issues to clean up.

> diff --git a/WebCore/platform/network/SocketStreamErrorBase.h b/WebCore/platform/network/SocketStreamErrorBase.h
> +    class SocketStreamErrorBase {
> +      public:

public:, private:, protected: should be aligned with "class". There are multiple instances but I'm only commenting on this one.

> +        SocketStreamErrorBase()
> +                : m_errorCode(0)
> +                , m_isNull(true)

These should only be indented 4 spaces (not 8). There are multiple instances but I'm only commenting on this one.


> diff --git a/WebCore/platform/network/SocketStreamHandle.h b/WebCore/platform/network/SocketStreamHandle.h
> +        virtual int platformSend(const char*, int);

Please include parameter names for this method.
Parameter names are omitted when they don't add information.  In this case, they would add information. 


> diff --git a/WebCore/platform/network/SocketStreamHandleBase.cpp b/WebCore/platform/network/SocketStreamHandleBase.cpp
> +bool SocketStreamHandleBase::send(const char* buf, int size)
s/buf/buffer/
Use full words for variables.

> +    int num_written = 0;

s/num_written/bytesWritten/

bad casing.

> +void SocketStreamHandleBase::setClient(SocketStreamHandleClient* client)
> +{

Consider adding asserts here.
Can this be called in any state?
Can it be called after a client has already been set?


> +bool SocketStreamHandleBase::sendPendingData()
> +{
> +    if (m_state != Open)
> +        return false;
> +    if (m_buffer.isEmpty())
> +        return false;
> +    int num_written = platformSend(m_buffer.data(), m_buffer.size());

s/num_written/bytesWritten/

bad casing.

> +    Vector<char> remaining_data;

s/remaining_data/remainingData/

bad casing.

> +    remaining_data.append(m_buffer.data() + num_written,
> +                           m_buffer.size() - num_written);

Indentation mistake.


> diff --git a/WebCore/platform/network/SocketStreamHandleBase.h b/WebCore/platform/network/SocketStreamHandleBase.h
> +        virtual ~SocketStreamHandleBase() {}
s/{}/{ }/
Add a space in the {}


> +        bool send(const char*, int);

Please include parameter names for this method.

> +        virtual int platformSend(const char*, int) = 0;

Please include parameter names for this method.

> diff --git a/WebCore/platform/network/SocketStreamHandleClient.h b/WebCore/platform/network/SocketStreamHandleClient.h
> +        virtual void willSendData(SocketStreamHandle*, const char*, int) { }

Please include parameter names for the char* and int.

> +        virtual void didReceivedData(SocketStreamHandle*, const char*, int) { }

Please include parameter names for the char* and int.


> diff --git a/WebCore/platform/network/mac/SocketStreamError.h b/WebCore/platform/network/mac/SocketStreamError.h
> +        SocketStreamError() {}
s/{}/{ }/

> diff --git a/WebCore/platform/network/mac/SocketStreamHandleMac.mm b/WebCore/platform/network/mac/SocketStreamHandleMac.mm
> +    SocketStreamHandleInternal() {}
> +    ~SocketStreamHandleInternal() {}

s/{}/{ }/


> diff --git a/WebCore/platform/network/soup/SocketStreamHandleSoup.cpp b/WebCore/platform/network/soup/SocketStreamHandleSoup.cpp
> +class SocketStreamHandleInternal {
> +public:
> +    SocketStreamHandleInternal() {}
> +    ~SocketStreamHandleInternal() {}

s/{}/{ }/
------- Comment #3 From 2009-08-11 09:03:49 PST -------
(From update of attachment 34545 [details])
Two more style comments:

> + /* static */
WebKit doesn't do this.

> + int SocketStreamHandle::platformSend(const char *, int)
* goes near the type
------- Comment #4 From 2009-08-11 09:08:11 PST -------
(From update of attachment 34545 [details])
> +    if (num_written < size)
> +        m_buffer.append(buf + num_written, size - num_written);

I think that we need a (large) upper limit on buffer size - it's too easy to send data faster than network permits, exhausting system memory and making the system unresponsive. Certainly, there are many other ways to exhaust memory via JavaScript, so it would be just a safety net, not a real security measure.

> +    remaining_data.append(m_buffer.data() + num_written,
> +                           m_buffer.size() - num_written);

As David pointed out, this is an indentation mistake - but there's no need to split the line anyway, it's quite short.

Looks good to me in general. I'm not sure if I'm not stepping on Dave's toes here, but r- to address the comments.
------- Comment #5 From 2009-08-11 09:17:30 PST -------
>  I'm not sure if I'm not stepping on Dave's toes...
Not at all.  I simply noticed some styling issues and thought I'd point them out (in case he was able to get to them before the "real" review).  I should have made this more clear.
------- Comment #6 From 2009-08-12 02:02:07 PST -------
Created an attachment (id=34645) [details]
Add SocketStreamHandle interface


---
 13 files changed, 748 insertions(+), 1 deletions(-)
------- Comment #7 From 2009-08-12 02:03:35 PST -------
Thanks for review.
I believe I fixed all comments.
Could you take a look again, please?
Thanks.
------- Comment #8 From 2009-08-12 02:09:44 PST -------
Created an attachment (id=34646) [details]
Add SocketStreamHandle interface


---
 13 files changed, 748 insertions(+), 1 deletions(-)
------- Comment #9 From 2009-08-12 10:24:41 PST -------
(From update of attachment 34646 [details])
+        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.
------- Comment #10 From 2009-08-19 04:35:40 PST -------
Created an attachment (id=35115) [details]
Add SocketStreamHandle interface


---
 14 files changed, 870 insertions(+), 0 deletions(-)
------- Comment #11 From 2009-08-19 04:37:06 PST -------
Thanks for review. 
Sorry for late response since I was on vacation.
I've tried to address all of your comments.
Could you take a look again, please?
------- Comment #12 From 2009-08-19 17:25:07 PST -------
(From update of attachment 35115 [details])
> diff --git a/WebCore/platform/network/cf/SocketStreamHandle.h b/WebCore/platform/network/cf/SocketStreamHandle.h
<...>
> +        KURL m_url;
> +
> +        // platform specific data
> +        friend class SocketStreamHandleInternal;
> +        OwnPtr<SocketStreamHandleInternal> internal;

All data in this file is platform specific, cross-platform data goes to SocketStreamHandleBase. So, there is no need for SocketStreamHandleInternal any more.

Everything else looks good to me, but this is something that's best to fix before landing, so r-.
------- Comment #13 From 2009-08-19 22:54:19 PST -------
Created an attachment (id=35185) [details]
Add SocketStreamHandle interface


---
 14 files changed, 844 insertions(+), 0 deletions(-)
------- Comment #14 From 2009-08-20 22:56:15 PST -------
(From update of attachment 35185 [details])
Bugzilla has experienced amnesia today, please upload the latest patch again.
------- Comment #15 From 2009-08-23 19:50:14 PST -------
Created an attachment (id=38464) [details]
Add SocketStreamHandle interface


---
 14 files changed, 839 insertions(+), 0 deletions(-)
------- Comment #16 From 2009-08-24 09:01:55 PST -------
(From update of attachment 38464 [details])
r=me
------- Comment #17 From 2009-08-24 11:26:55 PST -------
(From update of attachment 38464 [details])
Fumitoshi doesn't have commit bit, so marking cq+
------- Comment #18 From 2009-08-24 11:36:18 PST -------
(From update of attachment 38464 [details])
Rejecting patch 38464 from commit-queue.  This patch will require manual commit.

WebKitTools/Scripts/build-webkit failed with exit code 1
------- Comment #19 From 2009-08-24 11:44:28 PST -------
Since this fails release build on the Mac, the patch will likely need more modification.
------- Comment #20 From 2009-08-24 19:43:00 PST -------
Created an attachment (id=38520) [details]
Add SocketStreamHandle interface


---
 14 files changed, 839 insertions(+), 0 deletions(-)
------- Comment #21 From 2009-08-24 19:44:01 PST -------
(In reply to comment #19)
> Since this fails release build on the Mac, the patch will likely need more
> modification.

Fixed build failure on the Mac.
Comment unused parameters, and make bufferSize unsigned.
------- Comment #22 From 2009-08-24 22:14:04 PST -------
(From update of attachment 38520 [details])
r=me
------- Comment #23 From 2009-08-24 22:37:48 PST -------
(In reply to comment #22)
> (From update of attachment 38520 [details] [details])
> r=me

Thanks for review.
Anyway, do I need to fix typo didReceived* here as well that you mentioned in bug 28038?
------- Comment #24 From 2009-08-24 22:51:53 PST -------
Created an attachment (id=38527) [details]
Add SocketStreamHandle interface


---
 14 files changed, 839 insertions(+), 0 deletions(-)
------- Comment #25 From 2009-08-25 09:21:45 PST -------
(From update of attachment 38527 [details])
Rejecting patch 38527 from commit-queue.  This patch will require manual commit.

['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1'] failed with exit code 1
------- Comment #26 From 2009-08-25 10:08:53 PST -------
media/video-played-ranges-1.html -> failed

is the failure.  I assume that it's just a flakey media test.  I'll add this back to the commit queue.
------- Comment #27 From 2009-08-25 10:16:19 PST -------
(From update of attachment 38527 [details])
Rejecting patch 38527 from commit-queue.  This patch will require manual commit.

Failed to run "['git', 'svn', 'dcommit']"  exit_code: 1  cwd: None
------- Comment #28 From 2009-08-25 13:07:18 PST -------
(From update of attachment 38527 [details])
Sigh.  commit-queue bug.
Merge conflict during commit: Your file or directory 'WebCore/ChangeLog' is probably out-of-date: resource out of date; try updating at /usr/local/libexec/git-core//git-svn line 469

I'll try again, if this fails I'll just do it manually.
------- Comment #29 From 2009-08-25 13:40:45 PST -------
(From update of attachment 38527 [details])
Rejecting patch 38527 from commit-queue.  This patch will require manual commit.

Patch https://bugs.webkit.org/attachment.cgi?id=38527 from bug 28037 failed to download and apply.
------- Comment #30 From 2009-08-25 13:41:28 PST -------
patching file WebCore/WebCore.xcodeproj/project.pbxproj
Hunk #1 succeeded at 893 (offset 61 lines).
Hunk #2 succeeded at 981 (offset 61 lines).
Hunk #3 succeeded at 6028 (offset 121 lines).
Hunk #4 succeeded at 6123 (offset 121 lines).
Hunk #5 succeeded at 10668 (offset 154 lines).
Hunk #6 succeeded at 13876 (offset 178 lines).
Hunk #7 FAILED at 17532.
Hunk #8 FAILED at 19605.
2 out of 8 hunks FAILED -- saving rejects to file WebCore/WebCore.xcodeproj/project.pbxproj.rej
patch -p0 "WebCore/WebCore.xcodeproj/project.pbxproj" returned 1.  Pass --force to ignore patch failures.
------- Comment #31 From 2009-08-25 14:07:42 PST -------
I attempted to land this manually.  However svn-apply bails out after the first failure, so it left only a partial patch applied.

I tried using "curl --location 'https://bugs.webkit.org/attachment.cgi?id=38527' | patch -p1" and that failed to apply the ChangeLogs it seems.

So I think we're left with you updating this patch.  Alternatively someone could sync to the revision you made the patch from, apply it there, and then update.
------- Comment #32 From 2009-08-25 19:41:39 PST -------
Created an attachment (id=38588) [details]
Add SocketStreamHandle interface


---
 14 files changed, 839 insertions(+), 0 deletions(-)
------- Comment #33 From 2009-08-25 19:44:01 PST -------
(In reply to comment #31)
> I attempted to land this manually.  However svn-apply bails out after the first
> failure, so it left only a partial patch applied.
> 
> I tried using "curl --location
> 'https://bugs.webkit.org/attachment.cgi?id=38527' | patch -p1" and that failed
> to apply the ChangeLogs it seems.
> 
> So I think we're left with you updating this patch.  Alternatively someone
> could sync to the revision you made the patch from, apply it there, and then
> update.

Update the patch from revision 47769.
------- Comment #34 From 2009-08-26 09:16:44 PST -------
(From update of attachment 38588 [details])
> I attempted to land this manually.  However svn-apply bails out after the first
> failure, so it left only a partial patch applied.

Is this a recent regression? Sounds like a serious problem to me.
------- Comment #35 From 2009-08-26 11:16:55 PST -------
(From update of attachment 38588 [details])
Rejecting patch 38588 from commit-queue.  This patch will require manual commit.

['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1'] failed with exit code 1
------- Comment #36 From 2009-08-26 12:32:47 PST -------
(From update of attachment 38588 [details])
accessibility/nochildren-elements.html -> crashed

Seems unrelated.  Maybe it's a flakey test.
------- Comment #37 From 2009-08-26 13:05:55 PST -------
(From update of attachment 38588 [details])
Clearing flags on attachment: 38588

Committed r47788: <http://trac.webkit.org/changeset/47788>
------- Comment #38 From 2009-08-26 13:06:00 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #39 From 2009-09-09 19:40:20 PST -------
(In reply to comment #26)
> media/video-played-ranges-1.html -> failed
> 
> is the failure.  I assume that it's just a flakey media test.  I'll add this
> back to the commit queue.

This was likely one of the first instances of bug 28845.