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

[bugzilla-daemon at webkit.org]

http://bugs.webkit.org/show_bug.cgi?id=14997


aroben at apple.com changed:

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




------- Comment #8 from aroben at apple.com  2007-09-30 00:00 PDT -------
(From update of attachment 16038)
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!


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