Summary: | Made AudioNode an EventTarget | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jer Noble <jer.noble> | ||||
Component: | New Bugs | Assignee: | Jer Noble <jer.noble> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | cdumez, commit-queue, eric.carlson, esprehn+autocc, glenn | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Bug Depends on: | |||||||
Bug Blocks: | 116798 | ||||||
Attachments: |
|
Description
Jer Noble
2013-05-28 09:15:56 PDT
Created attachment 203061 [details]
Patch
Attachment 203061 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/webaudio/audionode-expected.txt', u'LayoutTests/webaudio/audionode.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/Modules/webaudio/AudioNode.cpp', u'Source/WebCore/Modules/webaudio/AudioNode.h', u'Source/WebCore/Modules/webaudio/AudioNode.idl', u'Source/WebCore/Modules/webaudio/ScriptProcessorNode.cpp', u'Source/WebCore/Modules/webaudio/ScriptProcessorNode.h', u'Source/WebCore/Modules/webaudio/ScriptProcessorNode.idl', u'Source/WebCore/dom/EventTarget.h', u'Source/WebCore/dom/EventTargetFactory.in']" exit_code: 1
Source/WebCore/dom/EventTarget.h:43: Code inside a namespace should not be indented. [whitespace/indent] [4]
Total errors found: 1 in 10 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 203061 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=203061&action=review > Source/WebCore/Modules/webaudio/AudioNode.h:186 > + // EventTarget > + virtual const AtomicString& interfaceName() const OVERRIDE; > + virtual ScriptExecutionContext* scriptExecutionContext() const OVERRIDE; > + virtual EventTargetData* eventTargetData() OVERRIDE { return &m_eventTargetData; } > + virtual EventTargetData* ensureEventTargetData() OVERRIDE { return &m_eventTargetData; } Could these overrides be private? I can’t think of any reason someone would call these on an AudioNode directly rather than through an EventTarget&, so making them private would be nice. > Source/WebCore/Modules/webaudio/AudioNode.idl:62 > + void addEventListener(DOMString type, > + EventListener listener, > + optional boolean useCapture); > + > + void removeEventListener(DOMString type, > + EventListener listener, > + optional boolean useCapture); > + > + boolean dispatchEvent(Event evt) > + raises(EventException); These read better on single lines, to my taste. And I see no reason to name it “evt” instead of event”. (In reply to comment #3) > (From update of attachment 203061 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=203061&action=review > > > Source/WebCore/Modules/webaudio/AudioNode.h:186 > > + // EventTarget > > + virtual const AtomicString& interfaceName() const OVERRIDE; > > + virtual ScriptExecutionContext* scriptExecutionContext() const OVERRIDE; > > + virtual EventTargetData* eventTargetData() OVERRIDE { return &m_eventTargetData; } > > + virtual EventTargetData* ensureEventTargetData() OVERRIDE { return &m_eventTargetData; } > > Could these overrides be private? I can’t think of any reason someone would call these on an AudioNode directly rather than through an EventTarget&, so making them private would be nice. Yes, these can be moved into private:. > > Source/WebCore/Modules/webaudio/AudioNode.idl:62 > > + void addEventListener(DOMString type, > > + EventListener listener, > > + optional boolean useCapture); > > + > > + void removeEventListener(DOMString type, > > + EventListener listener, > > + optional boolean useCapture); > > + > > + boolean dispatchEvent(Event evt) > > + raises(EventException); > > These read better on single lines, to my taste. And I see no reason to name it “evt” instead of event”. Sure thing. Thanks! (In reply to comment #4) > (In reply to comment #3) > > (From update of attachment 203061 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=203061&action=review > > > > > Source/WebCore/Modules/webaudio/AudioNode.h:186 > > > + // EventTarget > > > + virtual const AtomicString& interfaceName() const OVERRIDE; > > > + virtual ScriptExecutionContext* scriptExecutionContext() const OVERRIDE; > > > + virtual EventTargetData* eventTargetData() OVERRIDE { return &m_eventTargetData; } > > > + virtual EventTargetData* ensureEventTargetData() OVERRIDE { return &m_eventTargetData; } > > > > Could these overrides be private? I can’t think of any reason someone would call these on an AudioNode directly rather than through an EventTarget&, so making them private would be nice. > > Yes, these can be moved into private:. I take that back. They're called by the wrapper (JSAudioNode) directly, so they need to be public. > > > Source/WebCore/Modules/webaudio/AudioNode.idl:62 > > > + void addEventListener(DOMString type, > > > + EventListener listener, > > > + optional boolean useCapture); > > > + > > > + void removeEventListener(DOMString type, > > > + EventListener listener, > > > + optional boolean useCapture); > > > + > > > + boolean dispatchEvent(Event evt) > > > + raises(EventException); > > > > These read better on single lines, to my taste. And I see no reason to name it “evt” instead of event”. > > Sure thing. Thanks! Committed r150810: <http://trac.webkit.org/changeset/150810> |