[webkit-reviews] review canceled: [Bug 68833] Discard event data not followed by an empty line before eof when parsing an event-stream : [Attachment 108722] patch

[bugzilla-daemon at webkit.org]

Per-Erik Brodin <per-erik.brodin at ericsson.com> has canceled Per-Erik Brodin
<per-erik.brodin at ericsson.com>'s request for review:
Bug 68833: Discard event data not followed by an empty line before eof when
parsing an event-stream
https://bugs.webkit.org/show_bug.cgi?id=68833

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

------- Additional Comments from Per-Erik Brodin <per-erik.brodin at ericsson.com>
(In reply to comment #6)
> (From update of attachment 108722 [details])
> View in context:
https://bugs.webkit.org/attachment.cgi?id=108722&action=review
> 
> I see, that explains why we need two member variables.
> 
> Would "m_currentlyParsedEventId" and "m_latestCompleteEventId" work? You
could also null out m_currentlyParsedEventId after transferring its value to
m_latestCompleteEventId.

I would really like to leave m_lastEventId as it is since that's a name used in
the spec and the lastEventId property on MessageEvent and the Last-Event-ID
request header are set from it, but I've renamed m_eventId to
m_currentlyParsedEventId to better reflect how it's used.
 
> > Source/WebCore/page/EventSource.cpp:319
> > +		 if (!m_eventId.isNull())
> > +		     m_lastEventId = m_eventId;
> 
> What will break if this if() check is removed?

If the EventSource reconnects and data is discarded, and the first event after
reconnecting doesn't contain an 'id' field, then m_lastEventId will be
incorrectly overwritten without that if() check. I've slightly updated the
eventsource-reconnect test so that this case is covered by the tests.