[webkit-reviews] review denied: [Bug 14997] Support for server-sent DOM events : [Attachment 30050] proposed patch

[bugzilla-daemon at webkit.org]

Maciej Stachowiak <mjs at apple.com> has denied Adam Bergkvist
<adam.bergkvist at ericsson.com>'s request for review:
Bug 14997: Support for server-sent DOM events
https://bugs.webkit.org/show_bug.cgi?id=14997

Attachment 30050: proposed patch
https://bugs.webkit.org/attachment.cgi?id=30050&action=review

------- Additional Comments from Maciej Stachowiak <mjs at apple.com>
Thanks for the patch! It will be good to finally have this feature. A few
things need to be addressed first:

1) I think it would be ok to enable this by default for testing, unless there
is some critical problem with it.

2) It would be nice if constructors (real ones like this, not fake ones like
Element) could be autogenerated. I guess that is not a new issue with your
patch, so you don't have to fix this right away, just noticing.

3) might be good to file a follow-up bug on this: // FIXME: should support
cross-origin requests

4) EventSource::parseEventStream looks a little large for a single function. Is
there a way to break it up into smaller functions for understandability?

5) A couple of times the code removes UChars from the beginning of the
m_receiveBuf vector:  m_receiveBuf.remove(0, removeLen);

Removing from the start of a Vector is very inefficient. It copies almost all
the contents of the vector every time. I would consider using a different data
structure here. One possibility is to keep each incoming chunk of text as a
separate Vector or String, and just track where you are in the current segment,
and drop it once the whole thing has been processed. Another possibility is to
use a Deque, which allows for relatively efficient prepend or remove from
start.

6) This patch needs regression tests. I think it would be good to add http
tests including both normal Window versions and Worker versions. Tests should
ideally cover both normal event delivery, and handling of malformed EventSource
streams, with particular attention to edge cases.

The tests are the most critical piece of feedback here. We definitely need
regression tests to cover this new feature.

r- for test cases and other feedback. Please resubmit once these issues have
been addressed.