[webkit-reviews] review granted: [Bug 70659] EventTarget.h shouldn't need to know about every feature and ifdef : [Attachment 112104] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Sun Oct 23 21:01:43 PDT 2011
Darin Adler <darin at apple.com> has granted Adam Barth <abarth at webkit.org>'s
request for review:
Bug 70659: EventTarget.h shouldn't need to know about every feature and ifdef
https://bugs.webkit.org/show_bug.cgi?id=70659
Attachment 112104: Patch
https://bugs.webkit.org/attachment.cgi?id=112104&action=review
------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=112104&action=review
review+ but note that this broke both Qt and EFL so it could have been a
"review- because it breaks the build"
> Source/WebCore/bindings/js/JSEventTarget.cpp:119
> + AtomicString desiredInterface = target->interfaceName();
This can be const AtomicString& because there is no need to increment/decrement
the reference count here.
> Source/WebCore/bindings/js/WorkerScriptController.cpp:89
> - m_workerContextWrapper.set(*m_globalData,
JSDedicatedWorkerContext::create(*m_globalData, structure,
m_workerContext->toDedicatedWorkerContext()));
> + m_workerContextWrapper.set(*m_globalData,
JSDedicatedWorkerContext::create(*m_globalData, structure,
static_cast<DedicatedWorkerContext*>(m_workerContext)));
Would be nice to have the cast checked in debug builds.
> Source/WebCore/bindings/v8/V8DOMWrapper.cpp:346
> + AtomicString desiredInterface = target->interfaceName();
Ditto.
> Source/WebCore/dom/EventTargetFactory.in:4
> +namespace="EventTarget"
> +
> +EventSource
> +MessagePort
Since these files are often in alphabetical order, I’d like to see a comment
explaining what the order is and why.
More information about the webkit-reviews
mailing list