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