Bug 117132 - 'onended' event is not received in AudioBufferSourceNode and OscillatorNode.
Summary: 'onended' event is not received in AudioBufferSourceNode and OscillatorNode.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Audio (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Praveen Jadhav
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-06-02 23:14 PDT by Praveen Jadhav
Modified: 2013-06-03 04:47 PDT (History)
9 users (show)

See Also:


Attachments
Patch (3.22 KB, patch)
2013-06-02 23:30 PDT, Praveen Jadhav
no flags Details | Formatted Diff | Diff
Patch (3.07 KB, patch)
2013-06-03 02:30 PDT, Praveen Jadhav
cdumez: review+
cdumez: commit-queue-
Details | Formatted Diff | Diff
Patch (3.29 KB, patch)
2013-06-03 03:58 PDT, Praveen Jadhav
no flags Details | Formatted Diff | Diff
Patch (3.29 KB, patch)
2013-06-03 04:17 PDT, Praveen Jadhav
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Praveen Jadhav 2013-06-02 23:14:00 PDT
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.
Comment 1 Praveen Jadhav 2013-06-02 23:30:22 PDT
Created attachment 203555 [details]
Patch
Comment 2 Chris Dumez 2013-06-03 00:39:29 PDT
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()?
Comment 3 Chris Dumez 2013-06-03 00:49:28 PDT
I skipped the related test cases in <http://trac.webkit.org/changeset/151096> for EFL port as they were failing on the bots.
Comment 4 Praveen Jadhav 2013-06-03 01:21:05 PDT
(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.
Comment 5 Praveen Jadhav 2013-06-03 02:30:45 PDT
Created attachment 203564 [details]
Patch
Comment 6 Chris Dumez 2013-06-03 03:35:03 PDT
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."
Comment 7 Chris Dumez 2013-06-03 03:37:05 PDT
(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.
Comment 8 Praveen Jadhav 2013-06-03 03:58:58 PDT
Created attachment 203570 [details]
Patch

ChangeLog updated.
Comment 9 Praveen Jadhav 2013-06-03 04:17:12 PDT
Created attachment 203573 [details]
Patch

Patch updated.
Comment 10 WebKit Commit Bot 2013-06-03 04:47:40 PDT
Comment on attachment 203573 [details]
Patch

Clearing flags on attachment: 203573

Committed r151101: <http://trac.webkit.org/changeset/151101>
Comment 11 WebKit Commit Bot 2013-06-03 04:47:43 PDT
All reviewed patches have been landed.  Closing bug.