Bug 33210 - EventSource should support receiving empty events and events with newline characters only
Summary: EventSource should support receiving empty events and events with newline cha...
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: 2010-01-05 07:49 PST by Adam Bergkvist
Modified: 2010-03-15 21:59 PDT (History)
3 users (show)

See Also:


Attachments
patch (4.55 KB, patch)
2010-01-05 07:57 PST, Adam Bergkvist
eric: review-
Details | Formatted Diff | Diff
updated patch (5.45 KB, patch)
2010-01-07 08:34 PST, Adam Bergkvist
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Bergkvist 2010-01-05 07:49:06 PST
According to the updated specification, EventSource should support receiving empty
events and events with newline characters only.
Comment 1 Adam Bergkvist 2010-01-05 07:57:17 PST
Created attachment 45891 [details]
patch

Proposed patch.
Comment 2 WebKit Review Bot 2010-01-05 08:00:32 PST
style-queue ran check-webkit-style on attachment 45891 [details] without any errors.
Comment 3 Darin Adler 2010-01-05 08:34:37 PST
Comment on attachment 45891 [details]
patch

Are the two code changes here separate fixes? How do the code changes relate to the desired behavior? What is the removeLast for?

I think the comments in the change log are too terse. They assume that it's obvious what is wrong here and why this fixes it. Let those things are not clear to me.
Comment 4 Eric Seidel (no email) 2010-01-05 12:25:01 PST
Comment on attachment 45891 [details]
patch

I agree with Darin, this change is unreviewable.  It's not clear from the ChagneLog or from the code comments what you're trying to do here.
Comment 5 Adam Bergkvist 2010-01-07 08:34:28 PST
Created attachment 46054 [details]
updated patch

I agree that the ChangeLog message was insufficient. Here's an updated patch with more descriptive message in the ChangeLog.

The diff of the specification change in question:
http://html5.org/tools/web-apps-tracker?from=4078&to=4079
Comment 6 WebKit Review Bot 2010-01-07 08:38:37 PST
style-queue ran check-webkit-style on attachment 46054 [details] without any errors.
Comment 7 Eric Seidel (no email) 2010-02-17 15:12:18 PST
You mention the "updated specification" in the ChangeLog, but a link would be much more helpful.  Especially one which points to the paragraph in question.

I don't even know what specification is under discussion here.
Comment 8 Adam Barth 2010-03-08 10:39:22 PST
Comment on attachment 46054 [details]
updated patch

The code looks reasonable.  I haven't validated that this behavior actually matches what the spec requires, but I'm trusting Adam here.
Comment 9 WebKit Commit Bot 2010-03-15 21:59:03 PDT
Comment on attachment 46054 [details]
updated patch

Clearing flags on attachment: 46054

Committed r56035: <http://trac.webkit.org/changeset/56035>
Comment 10 WebKit Commit Bot 2010-03-15 21:59:09 PDT
All reviewed patches have been landed.  Closing bug.