Bug 116871 - Made AudioNode an EventTarget
Summary: Made AudioNode an EventTarget
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jer Noble
URL:
Keywords:
Depends on:
Blocks: 116798
  Show dependency treegraph
 
Reported: 2013-05-28 09:15 PDT by Jer Noble
Modified: 2013-05-28 10:54 PDT (History)
5 users (show)

See Also:


Attachments
Patch (11.86 KB, patch)
2013-05-28 09:22 PDT, Jer Noble
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jer Noble 2013-05-28 09:15:56 PDT
Made AudioNode an EventTarget
Comment 1 Jer Noble 2013-05-28 09:22:36 PDT
Created attachment 203061 [details]
Patch
Comment 2 WebKit Commit Bot 2013-05-28 09:24:31 PDT
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 3 Darin Adler 2013-05-28 09:29:37 PDT
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”.
Comment 4 Jer Noble 2013-05-28 09:46:30 PDT
(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!
Comment 5 Jer Noble 2013-05-28 09:47:37 PDT
(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!
Comment 6 Jer Noble 2013-05-28 10:54:17 PDT
Committed r150810: <http://trac.webkit.org/changeset/150810>