[Webkit-unassigned] [Bug 14997] Support for server-sent DOM events

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


aroben at apple.com changed:

           What    |Removed                     |Added
  Attachment #16003|review?                     |review-
               Flag|                            |

------- Comment #5 from aroben at apple.com  2007-08-18 01:04 PDT -------
(From update of attachment 16003)
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.

Configure bugmail: http://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

More information about the webkit-unassigned mailing list