Bug 215237 - Add AudioProcessingEvent Constructor
Summary: Add AudioProcessingEvent Constructor
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Audio (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Clark Wang
URL:
Keywords: InRadar
Depends on:
Blocks: 212611
  Show dependency treegraph
 
Reported: 2020-08-06 13:36 PDT by Clark Wang
Modified: 2020-08-10 12:40 PDT (History)
13 users (show)

See Also:


Attachments
Patch (21.31 KB, patch)
2020-08-06 15:37 PDT, Clark Wang
no flags Details | Formatted Diff | Diff
Patch (26.92 KB, patch)
2020-08-10 08:32 PDT, Clark Wang
no flags Details | Formatted Diff | Diff
Patch (28.99 KB, patch)
2020-08-10 09:54 PDT, Clark Wang
no flags Details | Formatted Diff | Diff
Patch (28.09 KB, patch)
2020-08-10 10:07 PDT, Clark Wang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Clark Wang 2020-08-06 13:36:08 PDT
Add constructor according to spec: https://www.w3.org/TR/webaudio/#audioprocessingevent
Comment 1 Clark Wang 2020-08-06 15:37:19 PDT
Created attachment 406123 [details]
Patch
Comment 2 Chris Dumez 2020-08-07 12:09:52 PDT
Comment on attachment 406123 [details]
Patch

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

> Source/WebCore/Modules/webaudio/AudioProcessingEvent.cpp:56
> +    : Event(eventType, CanBubble::Yes, IsCancelable::No)

You need to be calling this other Event constructor:
Event(eventType, eventInitDict, IsTrusted::No)

AudioProcessingEventInit subclasses EventInit which allows the client to set bubbles / cancelable to something else.

Note that this indicates you don't have enough test coverage, please add a test.

> Source/WebCore/Modules/webaudio/AudioProcessingEvent.h:46
>      static Ref<AudioProcessingEvent> createForBindings()

I don't think we need this one.

> Source/WebCore/Modules/webaudio/AudioProcessingEvent.h:60
>      AudioProcessingEvent();

I don't think we need this constructor.

> Source/WebCore/Modules/webaudio/AudioProcessingEvent.idl:30
>      readonly attribute unrestricted double playbackTime;

You can remove unrestricted.
Comment 3 Clark Wang 2020-08-10 08:32:51 PDT
Created attachment 406297 [details]
Patch
Comment 4 Chris Dumez 2020-08-10 09:06:40 PDT
Comment on attachment 406297 [details]
Patch

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

> LayoutTests/webaudio/audioprocessingevent-constructor.html:10
> +shouldThrowErrorName("event = new AudioProcessingEvent('foo', { renderedBuffer: null });", "TypeError");

renderedBuffer is not part of your dictionary. Please add more checks for cases that are supposed to throw. For example:
- playbackTime is missing  (but other members are present)
- inputBuffer is missing (but other members are present)
- outputBuffer is missing (but other members are present)
- inputBuffer/outputBuffer is null
- inputBuffer/outputBuffer has wrong type

> LayoutTests/webaudio/audioprocessingevent-constructor.html:19
> +shouldBeFalse("event.composed");

Please add shouldBeFalse("event.isTrusted");
Comment 5 Clark Wang 2020-08-10 09:54:42 PDT
Created attachment 406307 [details]
Patch
Comment 6 Clark Wang 2020-08-10 09:55:16 PDT
(In reply to Chris Dumez from comment #4)
> Comment on attachment 406297 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=406297&action=review
> 
> > LayoutTests/webaudio/audioprocessingevent-constructor.html:10
> > +shouldThrowErrorName("event = new AudioProcessingEvent('foo', { renderedBuffer: null });", "TypeError");
> 
> renderedBuffer is not part of your dictionary. Please add more checks for
> cases that are supposed to throw. For example:
> - playbackTime is missing  (but other members are present)
> - inputBuffer is missing (but other members are present)
> - outputBuffer is missing (but other members are present)
> - inputBuffer/outputBuffer is null
> - inputBuffer/outputBuffer has wrong type
> 
> > LayoutTests/webaudio/audioprocessingevent-constructor.html:19
> > +shouldBeFalse("event.composed");
> 
> Please add shouldBeFalse("event.isTrusted");

Added these test cases, thanks.
Comment 7 Chris Dumez 2020-08-10 10:05:25 PDT
Looks like your patch failed to apply.
Comment 8 Clark Wang 2020-08-10 10:07:25 PDT
Created attachment 406308 [details]
Patch
Comment 9 Clark Wang 2020-08-10 10:07:51 PDT
(In reply to Chris Dumez from comment #7)
> Looks like your patch failed to apply.

Just rebased, hopefully works now.
Comment 10 EWS 2020-08-10 12:39:31 PDT
Committed r265443: <https://trac.webkit.org/changeset/265443>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 406308 [details].
Comment 11 Radar WebKit Bug Importer 2020-08-10 12:40:21 PDT
<rdar://problem/66794462>