[webkit-reviews] review requested: [Bug 14997] Support for server-sent DOM events : [Attachment 30778] updated patch

[bugzilla-daemon at webkit.org]

Adam Bergkvist <adam.bergkvist at ericsson.com> has asked	for review:
Bug 14997: Support for server-sent DOM events
https://bugs.webkit.org/show_bug.cgi?id=14997

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

------- Additional Comments from Adam Bergkvist <adam.bergkvist at ericsson.com>
(In reply to comment #20)

Thanks for your feedback.

> 1) I think it would be ok to enable this by default for testing, unless there

> is some critical problem with it.
Done. Note that I have only updated the GTK build system since it is the
platform I am testing on.

> 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.
I am not sure how to automatically generate constructors that take arguments.
This could perhaps be fixed later.

> 3) might be good to file a follow-up bug on this: // FIXME: should support
> cross-origin requests
Sure, will do that as soon as this patch is landed.

> 4) EventSource::parseEventStream looks a little large for a single function.
Is
> there a way to break it up into smaller functions for understandability?
Line parsing in EventSource::parseEventStream has been extracted to a separate
function (EventSource::parseEventStreamLine).

> 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.
Removing from the beginning of the Vector is now reduced to being done when all
lines in one chunk has been parsed.

> 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.
Added various tests.