[webkit-reviews] review denied: [Bug 14997] Support for server-sent DOM events : [Attachment 16003] Patch which implements server-sent DOM events

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sat Aug 18 01:04:36 PDT 2007

Adam Roben <aroben at apple.com> has denied Henry Mason <hmason at mac.com>'s request
for review:
Bug 14997: Support for server-sent DOM events

Attachment 16003: Patch which implements server-sent DOM events

------- Additional Comments from Adam Roben <aroben at apple.com>
It's exciting to see an implementation of this part of HTML5!

Thanks for adding the new files to all 3 build systems!

Your patch has a mixture of LGPL and BSD licenses for the new files. Would you
be willing to make them all BSD?

-				EnablePREfast="$(EnablePREfast)"

Please remove this change from your patch in the 3 places it occurs.

+static HashSet<String> *NonBubblingEventTypes()
+static HashSet<String> *NonCancelableEventTypes()

These functions should return a const HashSet<String>&. Their names should have
a lowercase first letter. Any *s in this patch should be next to the type name,
not the variable name.

+DOMEventStreamLoader::DOMEventStreamLoader(EventTargetNode *target, KURL& url)

The second parameter should be a const KURL&.

+void DOMEventStreamLoader::open()
+    if (!m_isOpen) {

Please reverse the condition of this if and make it an early return. This
avoids needless indentation.

+	 RefPtr<SubresourceLoader> loader =
SubresourceLoader::create(m_target->ownerDocument()->frame(), this, request,
true, true);
+	 m_loader = loader.release();

There's no need for the local loader variable. You can assign straight into

+void DOMEventStreamLoader::close()
+    if (m_isOpen) {

Please make this an early return.

+void DOMEventStreamLoader::handleEvent(Event* event, bool isWindowEvent)

Please remove the unused parameter name (release builds will break otherwise).
You also don't need braces around the body of the if.

+RefPtr<EventTargetNode> DOMEventStreamLoader::getNodeFromFieldValue(String&

The return type should be a PassRefPtr (see
<http://webkit.org/coding/RefPtr.html> for an explanation of the uses of RefPtr
vs. PassRefPtr). The parameter should be a const String&.

+    if (fieldValue.isEmpty())
+	 return 0;
+    else if (fieldValue[0] == '#')
+	 return
+    else if (fieldValue == "Document")
+	 return m_target->ownerDocument();
+    else
+	 return 0;

You can remove all instances of the else keyword from these lines, since they
all follow return statements. You also don't need to pass a second argument to
String::substring (the default argument will do the same thing for you).

+Event* DOMEventStreamLoader::createEventFromFields(HashMap<String, String>&

The return type should be a PassRefPtr<Event>. The parameter should be a const
HashMap<String, String>&.

+    String eventType = fields.contains("Event") ? fields.get("Event") :

+    bool bubbles, cancelable;

Please declare these on separate lines (preferably right above the if-else
where each is initialized).

+    Event *event;
+    PassRefPtr<WebCore::Event> eventRefPtr;

There's no need for two variables here. You should just use a single
RefPtr<Event> event. (If event is not a RefPtr, you will crash!)

+	 else if (eventType == messageEvent)
+	     event = new MessageEvent;

There's no need for this branch (the else below will accomplish the same

+    String data = fields.contains("data") ? fields.get("data") : "";
+    int detail = fields.contains("detail") ? fields.get("detail").toInt() : 0;

+    bool ctrlKey = fields.contains("ctrlKey") ? (fields.get("ctrlKey") !=
"false") : false;

You should make helper functions like getStringFieldValue, getIntFieldValue,
getBooleanFieldValue instead of repeating these ternary operators over and

+    EventTargetNode* relatedTarget;
+    if (fields.contains("relatedTarget")) {
+	 String relatedTargetStr = fields.get("relatedTarget");
+	 RefPtr<EventTargetNode> relatedTargetRef =
+	 relatedTarget = relatedTargetRef ? relatedTargetRef.get() : 0;
+    } else 
+	 relatedTarget = 0;

relatedTarget should be a RefPtr. Once you change that you won't need the
relatedTargetRef local variable. You also don't need the relatedTargetStr local
variable. If you initialize relatedTarget to 0 you won't need the else at all.
These comments also apply to the relatedNode block below it.

+    // TODO: These fields should be used in cross-domain messaging when it is

Should be a FIXME (we don't use TODO generally).

+    Document* source(0);

We would normally use = 0 instead of the constructor syntax here.

+	 (static_cast<UIEvent*>(event))->initUIEvent(eventType, bubbles,
cancelable, view, detail);

Don't need the parentheses around the static_cast.

+	 (static_cast<SVGZoomEvent*>(event))->setNewScale(newScale);
+	 (static_cast<SVGZoomEvent*>(event))->setPreviousScale(previousScale);

I think a local variable is preferable to casting twice.

+void DOMEventStreamLoader::willSendRequest(SubresourceLoader* loader,
ResourceRequest& request, const ResourceResponse& redirectResponse)

Please remove the unused request parameter name.

+    if (loader != m_loader) return;

Put the return on the next line. (This happens elsewhere as well)

+	 /* FIXME:
+	    There is a rare unhandled case here. If we get the first byte of a
CRLF ("\r\n")
+	    new line in one data chunk, when we get the second chunk with an
'\n' byte at the
+	    start, we will wrongly parse an empty event. 
+	 */

Please use C++ style comments.

+	 int newLinePos, nextParseStartPos;

+	 } else {
+	     // No newline chars, need more data!
+	     break;
+	 }

I think it would be good to do:

if (lfPos < 0 && crPos < 0)

right after the calls to find(). Then this else won't be needed.

+	     RefPtr<EventTargetNode> eventTarget;
+	     if (fields.contains("Target")) {
+		 String target = fields.get("Target");
+		 RefPtr<EventTargetNode> otherTarget =
+		 eventTarget = otherTarget ? otherTarget : m_target;
+	     } else
+		 eventTarget = m_target;

Would be clearer/simpler as:

RefPtr<EventTargetNode> eventTarget = 0;
if (fields.contains("Target"))
    eventTarget = getNodeFromFieldValue(fields.get("Target"));

if (!eventTarget)
    eventTarget = m_target;

+	     Event* event = createEventFromFields(fields);

event needs to be a RefPtr.

+	     // Dispatch completed (or not), clear the fields and start parsing
the next event

I don't think this comment adds any information the code doesn't already say.

+	 } else if ((m_unparsedData[parseStartPos] != ';') &&
(m_unparsedData[parseStartPos] != ':')) {

You can reverse this and turn it into an early continue.

+	     String fieldName, fieldContent;

Please declare these on separate lines.

+		 fields.set(fieldName, (fields.get(fieldName) + String("\n") +

It seems a shame to keep appending all these strings as we go along. There must
be a more efficient way to do this.

+    m_reloadTimer.startOneShot(12.0);

The 12.0 constant should go in a static const double at the top of the file.

+#include "EventListener.h"
+#include "EventTargetNode.h"
+#include "KURL.h"
+#include "SubresourceLoaderClient.h"
+#include "TextResourceDecoder.h"
+#include "Timer.h"
+#ifndef DOMEventStreamLoader_h
+#define DOMEventStreamLoader_h

The header guards should go above the #includes. You don't need to #include 
EventTargetNode.h, you can just forward declare EventTargetNode.

+    class DOMEventStreamLoader : public EventListener, private
SubresourceLoaderClient {

"private" is not needed.

+	 DOMEventStreamLoader(EventTargetNode *target, KURL& url);

Please remove the parameter names as they don't add any information beyond the

+	 void setTarget(PassRefPtr<EventTargetNode> target) { m_target =
target; }

The parameter type should be an EventTargetNode*.

+	 KURL *URL() { return &m_url; }

This should be:

const KURL& url() { return m_url; }

+} // namespace

Please name the namespace here.

+void EventTargetNode::addEventSource(String& src)
+    if (m_eventSources.contains(src)) {

You've got an extra newline above the if. The parameter should be a const
String& (ditto for removeEventSource).

+    if (m_eventSources.contains(src)) {

Please reverse this and make it an early return.

 #include "EventTarget.h"
+#include "RemoteEventTarget.h"
 #include "Node.h"
+#include "DOMEventStreamLoader.h"

Please keep the #includes lexically sorted. You should also be able to
forward-declare DOMEventStreamLoader instead of #including the header.

+ * This file is part of the DOM implementation for KDE.

Please remove this line from any new files.

+ * Copyright (C) 2001 Peter Kelly (pmk at post.com)
+ * Copyright (C) 2001 Tobias Anton (anton at stud.fbi.fh-darmstadt.de)
+ * Copyright (C) 2006 Samuel Weinig (sam.weinig at gmail.com)
+ * Copyright (C) 2007 Henry Mason (hmason at mac.com)
+ * Copyright (C) 2003, 2005, 2006 Apple Computer, Inc.

I don't think all these old copyrights apply to your new MessageEvent.cpp file.

+    protected:
+    private:	 

The protected: is not needed since there's nothing in it.

+	 virtual void addEventSource(String& src);
+	 virtual void removeEventSource(String& src);

Perhaps these should be pure virtual functions?

+    DOMEventStreamLoader *m_loader;

Please make this an OwnPtr<DOMEventStreamLoader>.

+    m_loader = new DOMEventStreamLoader((EventTargetNode*)this, kurl);

Use static_cast instead of a C-style case.

+#include "EventTargetNode.h"

I don't see why this is needed in HTTPParsers.h.

Index: LayoutTests/http/tests/domeventsource/event-source.html

It would be great to add tests of invalid values for the various fields defined
in HTML5.

Index: LayoutTests/http/tests/domeventsource/remote-event-target-expected.txt

This should go inside a "resources" directory. Then run-webkit-tests will know
that it's not a test itself and won't generate results for it.

\ No newline at end of file

Please add a newline.

r- so that the style can be cleaned up and the RefPtr problems can be
addressed. I'm sure other people will have more comments on this patch as well.

More information about the webkit-reviews mailing list