[webkit-reviews] review denied: [Bug 14997] Support for server-sent DOM events : [Attachment 16038] Updated Patch

[bugzilla-daemon at webkit.org]

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
http://bugs.webkit.org/show_bug.cgi?id=14997

Attachment 16038: Updated Patch
http://bugs.webkit.org/attachment.cgi?id=16038&action=edit

------- Additional Comments from Adam Roben <aroben at apple.com>
Sorry it's taken so long for me to get back to this patch. This is looking a
lot nicer!

A couple more stylistic comments:

+static HashSet<String>& nonBubblingEventTypes()
+static HashSet<String>& nonCancelableEventTypes()

The return types should be const.

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

The * should be next to EventTargetNode, not target.

+PassRefPtr<EventTargetNode> DOMEventStreamLoader::getNodeFromFieldValue(const
String& fieldValue)

I was wrong before in saying that this should return a PassRefPtr. Since we're
not transferring ownership, the correct return type is EventTargetNode* (as you
had it originally).

+	 /* Not supported in WebCore yet....
+	 else if (eventType == DOMElementNamedChangedEvent || eventType ==
DOMAttributeNameChangeEvent)
+	     event = new MutationNameEvent; */

We don't like to have commented-out code checked in to our tree. I think it
would be more appropriate to have a FIXME comment here linking to a bug about
supporting these event types.

+    return event;

(at the end of createEventFromFields) To be slightly more efficient, you can
do:

return event.releaseRef();

+    if (loader != m_loader) return;
+    if (len < 1) return;

Please put the return on its own line, here and elsewhere.

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

In my previous comment I forgot to say that this should be a const method.

+    private:
+    
+	 String getStringFieldValue(const HashMap<String, String>&, const
String&, const String& defaultValue = "");
+	 int getIntFieldValue(const HashMap<String, String>&, const String&,
const int defaultValue = 0);
+	 unsigned getUnsignedFieldValue(const HashMap<String, String>&, const
String&, const unsigned defaultValue = 0);
+	 unsigned short getUnsignedShortFieldValue(const HashMap<String,
String>&, const String&, const unsigned short defaultValue = 0);
+	 bool getBoolFieldValue(const HashMap<String, String>&, const String&,
const bool defaultValue = false);
+	 float getFloatFieldValue(const HashMap<String, String>&, const
String&, const float defaultValue = 0.0);
+	 EventTargetNode* getEventTargetNodeFieldValue(const HashMap<String,
String>&, const String&, EventTargetNode* defaultValue = 0);
+	 
+	 PassRefPtr<EventTargetNode> getNodeFromFieldValue(const String&
fieldValue);
+	 PassRefPtr<Event> createEventFromFields(const HashMap<String, String>&
fields);

I think these can all be static helper functions in the .cpp file, rather than
being private instance methods.

+    HashMap<String, EventSourceEntry>* m_eventSources;

You could put this in an OwnPtr if you want so that you can avoid having to
remember to delete it.

+#include <wtf/Forward.h>

(in RemoteEventTarget.h) I don't think this include is necessary.

I don't think this is quite ready to go in yet, but we're making nice progress!