Bug 117132 – 'onended' event is not received in AudioBufferSourceNode and OscillatorNode.

• Component: Web Audio
• Status: RESOLVED FIXED

[Praveen Jadhav]

m_hasEndedListener is unset even though setOnended() passes a valid reference pointer(tested on EFL port). As a result, onended event is not received by JS.

[Praveen Jadhav]

Created attachment 203555 [details]
Patch

[Chris Dumez]

Comment on attachment 203555 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=203555&action=review

> Source/WebCore/ChangeLog:3
> +        [EFL] onended Event is not received in AudioBufferSourceNode and OscillatorNode.

Patch is not specific to EFL so please remove [EFL] tag.

> Source/WebCore/ChangeLog:11
> +        No new tests. Present layout tests will cover all the scenarios.

Then why doesn't this patch unskip / rebaseline any existing test?

> Source/WebCore/Modules/webaudio/AudioScheduledSourceNode.cpp:-172
> -    m_hasEndedListener = listener;

Could you be more explicit about what was wrong with this assignment? Was the issue that listener becomes NULL after the call to setAttributeEventListener()?

[Chris Dumez]

I skipped the related test cases in <http://trac.webkit.org/changeset/151096> for EFL port as they were failing on the bots.

[Praveen Jadhav]

(In reply to comment #2)
> (From update of attachment 203555 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=203555&action=review
> 
> > Source/WebCore/ChangeLog:3
> > +        [EFL] onended Event is not received in AudioBufferSourceNode and OscillatorNode.
> 
> Patch is not specific to EFL so please remove [EFL] tag.

Hmm, will update the title.

> 
> > Source/WebCore/ChangeLog:11
> > +        No new tests. Present layout tests will cover all the scenarios.
> 
> Then why doesn't this patch unskip / rebaseline any existing test?

Din't see your patch for tests rebaseline. :) Will update this as well.

> 
> > Source/WebCore/Modules/webaudio/AudioScheduledSourceNode.cpp:-172
> > -    m_hasEndedListener = listener;
> 
> Could you be more explicit about what was wrong with this assignment? Was the issue that listener becomes NULL after the call to setAttributeEventListener()?

As you said, "listener" becomes NULL after the call to setAttributeEventListener(). Will update the patch as discussed shortly.

[Praveen Jadhav]

Created attachment 203564 [details]
Patch

[Chris Dumez]

Comment on attachment 203564 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=203564&action=review

Looks good, r=me. But please improve the Changelogs before landing.

> Source/WebCore/ChangeLog:3
> +        onended Event is not received in AudioBufferSourceNode and OscillatorNode.

'onevent' event...

> Source/WebCore/ChangeLog:8
> +        PassRefPtr is set to null after usage in setAttributeEventListener().

This would be better IMHO: "The listener argument is a PassRefPtr which becomes NULL after the call to setAttributeEventListener() in setOnended(). This causes the m_hasEndedListener boolean to be incorrectly initialized. This patch reverses the two statements so that the m_hasEndedListener is updated before the call to setAttributeEventListener()."

> Source/WebCore/ChangeLog:11
> +        No new tests. Present layout tests will cover all the scenarios.

No new tests, already covered by existing tests.

> LayoutTests/ChangeLog:8
> +        Re-enabling failed webaudio tests due to 'onended' event not being

"Unskip webaudio test cases that were failing due to the 'onended' event not being fired."

[Chris Dumez]

(In reply to comment #6)
> (From update of attachment 203564 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=203564&action=review
> 
> Looks good, r=me. But please improve the Changelogs before landing.
> 
> > Source/WebCore/ChangeLog:3
> > +        onended Event is not received in AudioBufferSourceNode and OscillatorNode.
> 
> 'onevent' event...

I meant 'onended' event...", sorry.

[Praveen Jadhav]

Created attachment 203570 [details]
Patch

ChangeLog updated.

[Praveen Jadhav]

Created attachment 203573 [details]
Patch

Patch updated.

[WebKit Commit Bot]

Comment on attachment 203573 [details]
Patch

Clearing flags on attachment: 203573

Committed r151101: <http://trac.webkit.org/changeset/151101>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.