[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]

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 16003: Patch which implements server-sent DOM events
http://bugs.webkit.org/attachment.cgi?id=16003&action=edit

------- 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
m_loader.

+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&
fieldValue)

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
m_target->ownerDocument()->getElementById(fieldValue.substring(1,
fieldValue.length()-1));
+    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>&
fields)

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") :
"message";

+    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
thing).

+    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
over.

+    EventTargetNode* relatedTarget;
+    if (fields.contains("relatedTarget")) {
+	 String relatedTargetStr = fields.get("relatedTarget");
+	 RefPtr<EventTargetNode> relatedTargetRef =
getNodeFromFieldValue(relatedTargetStr);
+	 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
implemented

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)
   break;

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 =
getNodeFromFieldValue(target);
+		 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") +
fieldContent));

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
types.

+	 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.