Bug 28037 - SocketStreamHandle interface for WebSocket API
Summary: SocketStreamHandle interface for WebSocket API
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-08-05 22:34 PDT by Fumitoshi Ukai
Modified: 2009-09-09 19:40 PDT (History)
3 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Fumitoshi Ukai 2009-08-05 22:34:58 PDT
SocketStreamHandle will be used for backend of WebSocket API.
These will be added in WebCore/platform/network
Comment 1 Fumitoshi Ukai 2009-08-11 02:42:20 PDT
Created attachment 34545 [details]
Add SocketStreamHandle interface


---
 13 files changed, 736 insertions(+), 1 deletions(-)
Comment 2 David Levin 2009-08-11 08:38:47 PDT
Comment on attachment 34545 [details]
Add SocketStreamHandle interface

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 David Levin 2009-08-11 09:03:49 PDT
Comment on attachment 34545 [details]
Add SocketStreamHandle interface

Two more style comments:

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

> + int SocketStreamHandle::platformSend(const char *, int)
* goes near the type
Comment 4 Alexey Proskuryakov 2009-08-11 09:08:11 PDT
Comment on attachment 34545 [details]
Add SocketStreamHandle interface

> +    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 David Levin 2009-08-11 09:17:30 PDT
>  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 Fumitoshi Ukai 2009-08-12 02:02:07 PDT
Created attachment 34645 [details]
Add SocketStreamHandle interface


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


---
 13 files changed, 748 insertions(+), 1 deletions(-)
Comment 9 Alexey Proskuryakov 2009-08-12 10:24:41 PDT
Comment on attachment 34646 [details]
Add SocketStreamHandle interface

+        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 Fumitoshi Ukai 2009-08-19 04:35:40 PDT
Created attachment 35115 [details]
Add SocketStreamHandle interface


---
 14 files changed, 870 insertions(+), 0 deletions(-)
Comment 11 Fumitoshi Ukai 2009-08-19 04:37:06 PDT
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 Alexey Proskuryakov 2009-08-19 17:25:07 PDT
Comment on attachment 35115 [details]
Add SocketStreamHandle interface

> 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 Fumitoshi Ukai 2009-08-19 22:54:19 PDT
Created attachment 35185 [details]
Add SocketStreamHandle interface


---
 14 files changed, 844 insertions(+), 0 deletions(-)
Comment 14 Alexey Proskuryakov 2009-08-20 22:56:15 PDT
Comment on attachment 35185 [details]
Add SocketStreamHandle interface

Bugzilla has experienced amnesia today, please upload the latest patch again.
Comment 15 Fumitoshi Ukai 2009-08-23 19:50:14 PDT
Created attachment 38464 [details]
Add SocketStreamHandle interface


---
 14 files changed, 839 insertions(+), 0 deletions(-)
Comment 16 Alexey Proskuryakov 2009-08-24 09:01:55 PDT
Comment on attachment 38464 [details]
Add SocketStreamHandle interface

r=me
Comment 17 Eric Seidel (no email) 2009-08-24 11:26:55 PDT
Comment on attachment 38464 [details]
Add SocketStreamHandle interface

Fumitoshi doesn't have commit bit, so marking cq+
Comment 18 Eric Seidel (no email) 2009-08-24 11:36:18 PDT
Comment on attachment 38464 [details]
Add SocketStreamHandle interface

Rejecting patch 38464 from commit-queue.  This patch will require manual commit.

WebKitTools/Scripts/build-webkit failed with exit code 1
Comment 19 Eric Seidel (no email) 2009-08-24 11:44:28 PDT
Since this fails release build on the Mac, the patch will likely need more modification.
Comment 20 Fumitoshi Ukai 2009-08-24 19:43:00 PDT
Created attachment 38520 [details]
Add SocketStreamHandle interface


---
 14 files changed, 839 insertions(+), 0 deletions(-)
Comment 21 Fumitoshi Ukai 2009-08-24 19:44:01 PDT
(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 Alexey Proskuryakov 2009-08-24 22:14:04 PDT
Comment on attachment 38520 [details]
Add SocketStreamHandle interface

r=me
Comment 23 Fumitoshi Ukai 2009-08-24 22:37:48 PDT
(In reply to comment #22)
> (From update of attachment 38520 [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 Fumitoshi Ukai 2009-08-24 22:51:53 PDT
Created attachment 38527 [details]
Add SocketStreamHandle interface


---
 14 files changed, 839 insertions(+), 0 deletions(-)
Comment 25 Eric Seidel (no email) 2009-08-25 09:21:45 PDT
Comment on attachment 38527 [details]
Add SocketStreamHandle interface

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 Eric Seidel (no email) 2009-08-25 10:08:53 PDT
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 Eric Seidel (no email) 2009-08-25 10:16:19 PDT
Comment on attachment 38527 [details]
Add SocketStreamHandle interface

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 Eric Seidel (no email) 2009-08-25 13:07:18 PDT
Comment on attachment 38527 [details]
Add SocketStreamHandle interface

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 Eric Seidel (no email) 2009-08-25 13:40:45 PDT
Comment on attachment 38527 [details]
Add SocketStreamHandle interface

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 Eric Seidel (no email) 2009-08-25 13:41:28 PDT
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 Eric Seidel (no email) 2009-08-25 14:07:42 PDT
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 Fumitoshi Ukai 2009-08-25 19:41:39 PDT
Created attachment 38588 [details]
Add SocketStreamHandle interface


---
 14 files changed, 839 insertions(+), 0 deletions(-)
Comment 33 Fumitoshi Ukai 2009-08-25 19:44:01 PDT
(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 Alexey Proskuryakov 2009-08-26 09:16:44 PDT
Comment on attachment 38588 [details]
Add SocketStreamHandle interface

> 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 Eric Seidel (no email) 2009-08-26 11:16:55 PDT
Comment on attachment 38588 [details]
Add SocketStreamHandle interface

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 Eric Seidel (no email) 2009-08-26 12:32:47 PDT
Comment on attachment 38588 [details]
Add SocketStreamHandle interface

accessibility/nochildren-elements.html -> crashed

Seems unrelated.  Maybe it's a flakey test.
Comment 37 Eric Seidel (no email) 2009-08-26 13:05:55 PDT
Comment on attachment 38588 [details]
Add SocketStreamHandle interface

Clearing flags on attachment: 38588

Committed r47788: <http://trac.webkit.org/changeset/47788>
Comment 38 Eric Seidel (no email) 2009-08-26 13:06:00 PDT
All reviewed patches have been landed.  Closing bug.
Comment 39 Eric Seidel (no email) 2009-09-09 19:40:20 PDT
(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.