Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2491653002:
Add constructors for WebAudio events (Closed)
|
Created:
4 years, 1 month ago by Raymond Toy Modified:
4 years ago CC:
blink-reviews, chromium-reviews, hongchan, Raymond Toy Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd constructors for WebAudio events
Add constructor for WebAudio AudioProcessingEvent and
OfflineAudioCompletionEvent.
Intent: https://groups.google.com/a/chromium.org/d/msg/blink-dev/G3V0F7Rh0Io/MuLBeUZeAQAJ
BUG=662164
TEST=event-constructor.html
Committed: https://crrev.com/ba258a3d5e041db72953fd72f19e78747ef74602
Cr-Commit-Position: refs/heads/master@{#436647}
Patch Set 1 #Patch Set 2 : Add args for constructor and add test #Patch Set 3 : Add links to spec. #
Total comments: 23
Patch Set 4 : Address review comments. #Patch Set 5 : Add more tests for AudioProcessingEvent constructor. #Patch Set 6 : Fix typo. #
Total comments: 2
Patch Set 7 : Fix typo #Patch Set 8 : Rebase #
Total comments: 14
Patch Set 9 : Remove unnecessary blank lines #Messages
Total messages: 46 (31 generated)
The CQ bit was checked by rtoy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by rtoy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Add constructors for WebAudio events Add constructor for WebAudio AudioProcessingEvent and OfflineAudioCompletionEvent. BUG=662164 TEST=event-constructor.html ========== to ========== Add constructors for WebAudio events Add constructor for WebAudio AudioProcessingEvent and OfflineAudioCompletionEvent. Intent: https://groups.google.com/a/chromium.org/d/msg/blink-dev/G3V0F7Rh0Io/MuLBeUZe... BUG=662164 TEST=event-constructor.html ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
rtoy@chromium.org changed reviewers: + foolip@chromium.org, hongchan@chromium.org
PTAL.
rtoy@chromium.org changed reviewers: + haraken@chromium.org
haraken: PTAL at modules_idl_files.gni in particular, and the rest if you're interested.
The CQ bit was checked by rtoy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM on my side
lgtm % nits https://codereview.chromium.org/2491653002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/webaudio/event-constructor.html (right): https://codereview.chromium.org/2491653002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/event-constructor.html:7: <script src="resources/audio-testing.js"></script> It looks like the test would be more compact without this helper, but I guess all Web Audio tests are in this style? https://codereview.chromium.org/2491653002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/event-constructor.html:15: var event; event is never used in this test, so you could just call the constructors without using the result. https://codereview.chromium.org/2491653002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/event-constructor.html:20: }).throw() && success; I think 'TypeError' here and elsewhere would test that the right exception is thrown. https://codereview.chromium.org/2491653002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/event-constructor.html:37: success = Should('new OfflineAudioCompletionEvent("complete")', This doesn't match what is tested. https://codereview.chromium.org/2491653002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/event-constructor.html:57: success = Should("event.renderedBuffer !== undefined", event.renderedBuffer !== undefined) Can you assign the AudioBuffer to a variable and assert that event.renderedBuffer===buffer instead? Many things would pass this test, like leaving event.renderedBuffer as null. https://codereview.chromium.org/2491653002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/event-constructor.html:61: .summarize("correctly", Accidental newline? https://codereview.chromium.org/2491653002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/event-constructor.html:67: audit.defineTask("audio-processing", function (taskDone) { For this, since all three dictionary members are required, can you also add three tests with two members each, one member missing from each test, to test that it throws TypeError? https://codereview.chromium.org/2491653002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/event-constructor.html:82: success = Should("event.playbackTime !== undefined", event.playbackTime !== undefined) Here too, assert what the attributes should be, not just that they aren't undefined. https://codereview.chromium.org/2491653002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/AudioProcessingEvent.cpp (right): https://codereview.chromium.org/2491653002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AudioProcessingEvent.cpp:43: const AtomicString& eventType, Suggest type/initializer here too, like below and e.g. BlobEvent.cpp and many other event constructors I think. https://codereview.chromium.org/2491653002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/AudioProcessingEvent.h (right): https://codereview.chromium.org/2491653002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AudioProcessingEvent.h:50: static AudioProcessingEvent* create(const AtomicString& eventType, s/eventType/type/ for consistency https://codereview.chromium.org/2491653002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/OfflineAudioCompletionEvent.h (right): https://codereview.chromium.org/2491653002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/OfflineAudioCompletionEvent.h:62: explicit OfflineAudioCompletionEvent(const AtomicString&, Include argument name (type)
The CQ bit was checked by rtoy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2491653002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/webaudio/event-constructor.html (right): https://codereview.chromium.org/2491653002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/event-constructor.html:7: <script src="resources/audio-testing.js"></script> On 2016/11/15 19:53:33, foolip wrote: > It looks like the test would be more compact without this helper, but I guess > all Web Audio tests are in this style? Yeah, pretty much all newer tests are in this style because it provides far more information about failures than the normal test harness does. In this case, it probably doesn't matter, but I use this out of habit. https://codereview.chromium.org/2491653002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/event-constructor.html:15: var event; On 2016/11/15 19:53:33, foolip wrote: > event is never used in this test, so you could just call the constructors > without using the result. Done. https://codereview.chromium.org/2491653002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/event-constructor.html:20: }).throw() && success; On 2016/11/15 19:53:33, foolip wrote: > I think 'TypeError' here and elsewhere would test that the right exception is > thrown. Done. I meant to do this but forgot. :-( https://codereview.chromium.org/2491653002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/event-constructor.html:37: success = Should('new OfflineAudioCompletionEvent("complete")', On 2016/11/15 19:53:33, foolip wrote: > This doesn't match what is tested. Done. https://codereview.chromium.org/2491653002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/event-constructor.html:57: success = Should("event.renderedBuffer !== undefined", event.renderedBuffer !== undefined) On 2016/11/15 19:53:33, foolip wrote: > Can you assign the AudioBuffer to a variable and assert that > event.renderedBuffer===buffer instead? Many things would pass this test, like > leaving event.renderedBuffer as null. Done. https://codereview.chromium.org/2491653002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/event-constructor.html:61: .summarize("correctly", On 2016/11/15 19:53:33, foolip wrote: > Accidental newline? Done. https://codereview.chromium.org/2491653002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/event-constructor.html:67: audit.defineTask("audio-processing", function (taskDone) { On 2016/11/15 19:53:32, foolip wrote: > For this, since all three dictionary members are required, can you also add > three tests with two members each, one member missing from each test, to test > that it throws TypeError? Done. https://codereview.chromium.org/2491653002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/event-constructor.html:82: success = Should("event.playbackTime !== undefined", event.playbackTime !== undefined) On 2016/11/15 19:53:33, foolip wrote: > Here too, assert what the attributes should be, not just that they aren't > undefined. Done. https://codereview.chromium.org/2491653002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/AudioProcessingEvent.cpp (right): https://codereview.chromium.org/2491653002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AudioProcessingEvent.cpp:43: const AtomicString& eventType, On 2016/11/15 19:53:33, foolip wrote: > Suggest type/initializer here too, like below and e.g. BlobEvent.cpp and many > other event constructors I think. Done. https://codereview.chromium.org/2491653002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/AudioProcessingEvent.h (right): https://codereview.chromium.org/2491653002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AudioProcessingEvent.h:50: static AudioProcessingEvent* create(const AtomicString& eventType, On 2016/11/15 19:53:33, foolip wrote: > s/eventType/type/ for consistency Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) blimp_linux_dbg on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/blimp_linux_dbg...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by rtoy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by rtoy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
hongchang: ping
https://codereview.chromium.org/2491653002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/OfflineAudioCompletionEvent.h (right): https://codereview.chromium.org/2491653002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/OfflineAudioCompletionEvent.h:62: explicit OfflineAudioCompletionEvent(const AtomicString&, On 2016/11/15 19:53:33, foolip wrote: > Include argument name (type) What about the second one? I thought Chromium convention is to omit the variable names in the heather when it's possible. https://codereview.chromium.org/2491653002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/AudioProcessingEvent.cpp (right): https://codereview.chromium.org/2491653002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioProcessingEvent.cpp:45: return new AudioProcessingEvent(eventType, eventInit); Oh, did this ever work? evenInit is not defined. https://codereview.chromium.org/2491653002/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/webaudio/event-constructor.html (right): https://codereview.chromium.org/2491653002/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/webaudio/event-constructor.html:12: var audit = Audit.createTaskRunner(); Can we make a 'TODO' here so we can revisit this with the new test utility? https://codereview.chromium.org/2491653002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/AudioProcessingEvent.cpp (right): https://codereview.chromium.org/2491653002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioProcessingEvent.cpp:45: return new AudioProcessingEvent(type, initializer); I think |eventInit| is better. |initializer| sounds like a service, not the variable. https://codereview.chromium.org/2491653002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/AudioProcessingEvent.h (right): https://codereview.chromium.org/2491653002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioProcessingEvent.h:50: static AudioProcessingEvent* create(const AtomicString& type, Or we can just remove the variable name itself to make it consistent with the second argument. https://codereview.chromium.org/2491653002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/AudioProcessingEvent.idl (right): https://codereview.chromium.org/2491653002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioProcessingEvent.idl:28: Constructor(DOMString type, AudioProcessingEventInit eventInitDict) I would leave out 'Dict' at the end.
https://codereview.chromium.org/2491653002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/OfflineAudioCompletionEvent.h (right): https://codereview.chromium.org/2491653002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/OfflineAudioCompletionEvent.h:62: explicit OfflineAudioCompletionEvent(const AtomicString&, On 2016/12/02 18:07:17, hongchan wrote: > On 2016/11/15 19:53:33, foolip wrote: > > Include argument name (type) > > What about the second one? I thought Chromium convention is to omit the variable > names in the heather when it's possible. I think AtomicString& by itself carries no info about what the arg does. But OfflineAudioCompletionEventInit pretty clearly tells you what the arg does even without an arg name. https://codereview.chromium.org/2491653002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/AudioProcessingEvent.cpp (right): https://codereview.chromium.org/2491653002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioProcessingEvent.cpp:45: return new AudioProcessingEvent(eventType, eventInit); On 2016/12/02 18:07:17, hongchan wrote: > Oh, did this ever work? evenInit is not defined. Good question. Reviewing the wrong patch? My current code says initializer, not eventInit. https://codereview.chromium.org/2491653002/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/webaudio/event-constructor.html (right): https://codereview.chromium.org/2491653002/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/webaudio/event-constructor.html:12: var audit = Audit.createTaskRunner(); On 2016/12/02 18:07:17, hongchan wrote: > Can we make a 'TODO' here so we can revisit this with the new test utility? No, I don't want that. This CL was written before the new test facility was available, so I don't see why a TODO is needed here. You can, however, ask me to rewrite this with the new test, but, unlike other cases, I would rather not since this is fairly large. https://codereview.chromium.org/2491653002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/AudioProcessingEvent.cpp (right): https://codereview.chromium.org/2491653002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioProcessingEvent.cpp:45: return new AudioProcessingEvent(type, initializer); On 2016/12/02 18:07:17, hongchan wrote: > I think |eventInit| is better. |initializer| sounds like a service, not the > variable. I think I was following other existing code, but eventInitializer is nicer. (Preferring whole words to abbreviations.) https://codereview.chromium.org/2491653002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/AudioProcessingEvent.h (right): https://codereview.chromium.org/2491653002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioProcessingEvent.h:50: static AudioProcessingEvent* create(const AtomicString& type, On 2016/12/02 18:07:17, hongchan wrote: > Or we can just remove the variable name itself to make it consistent with the > second argument. No, I agree with foolip (in another comment). This should have a name. And I would argue "type" is useless. "eventType" or "eventName" would be a much better name. https://codereview.chromium.org/2491653002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/AudioProcessingEvent.idl (right): https://codereview.chromium.org/2491653002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioProcessingEvent.idl:28: Constructor(DOMString type, AudioProcessingEventInit eventInitDict) On 2016/12/02 18:07:17, hongchan wrote: > I would leave out 'Dict' at the end. This is what the spec has, so I think we should leave it. Feel free to file a PR on the spec to rename it.
lgtm
rtoy@chromium.org changed reviewers: + tkent@chromium.org
tkent: PTAL as API OWNER
lgtm https://codereview.chromium.org/2491653002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/AudioProcessingEvent.cpp (right): https://codereview.chromium.org/2491653002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioProcessingEvent.cpp:27: This blank line is unnecessary. https://codereview.chromium.org/2491653002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/AudioProcessingEvent.h (right): https://codereview.chromium.org/2491653002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioProcessingEvent.h:32: nit: This blink line is unnecessary. https://codereview.chromium.org/2491653002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/OfflineAudioCompletionEvent.h (right): https://codereview.chromium.org/2491653002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/OfflineAudioCompletionEvent.h:32: This blank line is unnecessary.
https://codereview.chromium.org/2491653002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/AudioProcessingEvent.cpp (right): https://codereview.chromium.org/2491653002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioProcessingEvent.cpp:27: On 2016/12/05 23:38:35, tkent wrote: > This blank line is unnecessary. Done. https://codereview.chromium.org/2491653002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/AudioProcessingEvent.h (right): https://codereview.chromium.org/2491653002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioProcessingEvent.h:32: On 2016/12/05 23:38:35, tkent wrote: > nit: This blink line is unnecessary. Done. https://codereview.chromium.org/2491653002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/OfflineAudioCompletionEvent.h (right): https://codereview.chromium.org/2491653002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/OfflineAudioCompletionEvent.h:32: On 2016/12/05 23:38:35, tkent wrote: > This blank line is unnecessary. Done.
The CQ bit was checked by rtoy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, foolip@chromium.org, tkent@chromium.org, hongchan@chromium.org Link to the patchset: https://codereview.chromium.org/2491653002/#ps160001 (title: "Remove unnecessary blank lines")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1481040244222400,
"parent_rev": "90bed947584a443672208899bae69de0baf89734", "commit_rev":
"c953d57c5dadb9d1b73740d8a53575e5cff45587"}
Message was sent while issue was closed.
Description was changed from ========== Add constructors for WebAudio events Add constructor for WebAudio AudioProcessingEvent and OfflineAudioCompletionEvent. Intent: https://groups.google.com/a/chromium.org/d/msg/blink-dev/G3V0F7Rh0Io/MuLBeUZe... BUG=662164 TEST=event-constructor.html ========== to ========== Add constructors for WebAudio events Add constructor for WebAudio AudioProcessingEvent and OfflineAudioCompletionEvent. Intent: https://groups.google.com/a/chromium.org/d/msg/blink-dev/G3V0F7Rh0Io/MuLBeUZe... BUG=662164 TEST=event-constructor.html ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Add constructors for WebAudio events Add constructor for WebAudio AudioProcessingEvent and OfflineAudioCompletionEvent. Intent: https://groups.google.com/a/chromium.org/d/msg/blink-dev/G3V0F7Rh0Io/MuLBeUZe... BUG=662164 TEST=event-constructor.html ========== to ========== Add constructors for WebAudio events Add constructor for WebAudio AudioProcessingEvent and OfflineAudioCompletionEvent. Intent: https://groups.google.com/a/chromium.org/d/msg/blink-dev/G3V0F7Rh0Io/MuLBeUZe... BUG=662164 TEST=event-constructor.html Committed: https://crrev.com/ba258a3d5e041db72953fd72f19e78747ef74602 Cr-Commit-Position: refs/heads/master@{#436647} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/ba258a3d5e041db72953fd72f19e78747ef74602 Cr-Commit-Position: refs/heads/master@{#436647} |
