Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2697733004:
Refactor decode-audio-data-basic.html to use testharness (Closed)
|
Created:
3 years, 10 months ago by hongchan Modified:
3 years, 10 months ago Reviewers:
Raymond Toy CC:
chromium-reviews, blink-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRefactor decode-audio-data-basic.html to use testharness
- Replace js-test and old Audit with testharness and new Audit.
- Promisefied file loader |Audit.loadFileAtUrl(fileUrl)| is added.
- Refactored complex promise/callback code.
- should().beResolved() now bypassing its argument to the next promise.
For the first pass, I focused on preserving the test behavior. If this looks good,
I will move on to the clean-up and more refactoring.
BUG=688064
TEST=
LayoutTests/webaudio/decodeAudioData/decode-audio-data-basic.html
Review-Url: https://codereview.chromium.org/2697733004
Cr-Commit-Position: refs/heads/master@{#451011}
Committed: https://chromium.googlesource.com/chromium/src/+/0e7af755c6ff3377abb22925f319c8cfb699461d
Patch Set 1 : Initial Commit #
Total comments: 4
Patch Set 2 : Addressing comments #Patch Set 3 : More refactoring (clean up #1) #Patch Set 4 : Comments and clang-format (clean up #2) #
Total comments: 18
Patch Set 5 : Addressing feedback #
Total comments: 1
Patch Set 6 : Fixed type after l-g-t-m #
Messages
Total messages: 26 (11 generated)
Description was changed from ========== out# Enter a description of the change. Refactor decode-audio-data-basic.html to adapt testharness - Replace js-test and old Audit with testharness and new Audit. - Promisefied file loader |Audit.loadFileAtUrl(fileUrl)| is added. - Refactored complex promise/callback code. - should().beResolved() now bypassing its argument to the next promise. BUG=688064 TEST= LayoutTests/webaudio/decodeAudioData/decode-audio-data-basic.html ========== to ========== out# Enter a description of the change. Refactor decode-audio-data-basic.html to adapt testharness - Replace js-test and old Audit with testharness and new Audit. - Promisefied file loader |Audit.loadFileAtUrl(fileUrl)| is added. - Refactored complex promise/callback code. - should().beResolved() now bypassing its argument to the next promise. For the first pass, I focused on preserving the test behavior. If this looks good, I will move on to the clean-up and more refactoring. BUG=688064 TEST= LayoutTests/webaudio/decodeAudioData/decode-audio-data-basic.html ==========
hongchan@chromium.org changed reviewers: + rtoy@chromium.org
Description was changed from ========== out# Enter a description of the change. Refactor decode-audio-data-basic.html to adapt testharness - Replace js-test and old Audit with testharness and new Audit. - Promisefied file loader |Audit.loadFileAtUrl(fileUrl)| is added. - Refactored complex promise/callback code. - should().beResolved() now bypassing its argument to the next promise. For the first pass, I focused on preserving the test behavior. If this looks good, I will move on to the clean-up and more refactoring. BUG=688064 TEST= LayoutTests/webaudio/decodeAudioData/decode-audio-data-basic.html ========== to ========== Refactor decode-audio-data-basic.html to adapt testharness - Replace js-test and old Audit with testharness and new Audit. - Promisefied file loader |Audit.loadFileAtUrl(fileUrl)| is added. - Refactored complex promise/callback code. - should().beResolved() now bypassing its argument to the next promise. For the first pass, I focused on preserving the test behavior. If this looks good, I will move on to the clean-up and more refactoring. BUG=688064 TEST= LayoutTests/webaudio/decodeAudioData/decode-audio-data-basic.html ==========
Description was changed from ========== Refactor decode-audio-data-basic.html to adapt testharness - Replace js-test and old Audit with testharness and new Audit. - Promisefied file loader |Audit.loadFileAtUrl(fileUrl)| is added. - Refactored complex promise/callback code. - should().beResolved() now bypassing its argument to the next promise. For the first pass, I focused on preserving the test behavior. If this looks good, I will move on to the clean-up and more refactoring. BUG=688064 TEST= LayoutTests/webaudio/decodeAudioData/decode-audio-data-basic.html ========== to ========== Refactor decode-audio-data-basic.html to use testharness - Replace js-test and old Audit with testharness and new Audit. - Promisefied file loader |Audit.loadFileAtUrl(fileUrl)| is added. - Refactored complex promise/callback code. - should().beResolved() now bypassing its argument to the next promise. For the first pass, I focused on preserving the test behavior. If this looks good, I will move on to the clean-up and more refactoring. BUG=688064 TEST= LayoutTests/webaudio/decodeAudioData/decode-audio-data-basic.html ==========
PTAL.
Looks good with just a few comments. Maybe want to add a comment somewhere that this currently depends on a certain order for promise and callback handling. https://codereview.chromium.org/2697733004/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/webaudio/resources/audit.js (right): https://codereview.chromium.org/2697733004/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/webaudio/resources/audit.js:334: return this._actual.then(function (result) { This needs to be documented somehow. https://codereview.chromium.org/2697733004/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/webaudio/resources/audit.js:1111: function loadFileAtUrl (fileUrl) { Documentation says loadFile, but the function is loadFileAtUrl. I think I actually prefer loadFileFromUrl. Would this support data uri's? If not, that's ok.
This is the result from the test. ------------------------------------------- This is a testharness.js-based test. PASS # AUDIT TASK RUNNER STARTED. PASS Decoding null AudioBuffer rejected correctly with TypeError: Failed to execute 'decodeAudioData' on 'BaseAudioContext': parameter 1 is not of type 'ArrayBuffer'.. PASS < [null-audiobuffer] All assertions passed. (total 1 assertions) PASS Decoding a valid audio file resolved correctly. PASS < [decode-valid-file] All assertions passed. (total 1 assertions) PASS Decoding an invalid audio file rejected correctly with EncodingError: Unable to decode audio data. PASS < [decode-invalid-file] All assertions passed. (total 1 assertions) PASS Decoding valid file by callback function successCallback invoked correctly PASS Decoding a file via promise resolved correctly. PASS Two buffers decoded by callback function and promise are identical PASS < [promise-and-success-callback] All assertions passed. (total 3 assertions) PASS Decoding invalid file with promise and callback: errorCallback invoked correctly with EncodingError: Unable to decode audio data PASS decodeAudioData promise rejected correctly with EncodingError: Unable to decode audio data. PASS < [promise-and-error-callback] All assertions passed. (total 2 assertions) PASS < [load-data] All assertions passed. (total 0 assertions) PASS Decoded buffer length (frames) is equal to 44100. PASS Decoded buffer duration (sec) is equal to 1. PASS Decoded buffer sample rate (Hz) is equal to 44100. PASS Number of channels in decoded buffer is equal to 1. PASS Decoded buffer channel #0 is identical to the array the expected channel #0. PASS The buffer correctly decoded after the context has been closed PASS Decoding valid file completed successfully PASS < [close-context-with-pending-decode] All assertions passed. (total 7 assertions) PASS # AUDIT TASK RUNNER FINISHED: 7 tasks ran successfully. Harness: the test ran to completion. ------------------------------------------- https://codereview.chromium.org/2697733004/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/webaudio/resources/audit.js (right): https://codereview.chromium.org/2697733004/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/webaudio/resources/audit.js:334: return this._actual.then(function (result) { On 2017/02/14 22:47:56, Raymond Toy wrote: > This needs to be documented somehow. Done. https://codereview.chromium.org/2697733004/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/webaudio/resources/audit.js:1111: function loadFileAtUrl (fileUrl) { On 2017/02/14 22:47:56, Raymond Toy wrote: > Documentation says loadFile, but the function is loadFileAtUrl. I think I > actually prefer loadFileFromUrl. > > Would this support data uri's? If not, that's ok. Done. A good question. I have no idea for now.
On 2017/02/14 22:54:39, hongchan wrote: > This is the result from the test. > > ------------------------------------------- > This is a testharness.js-based test. > PASS # AUDIT TASK RUNNER STARTED. > PASS Decoding null AudioBuffer rejected correctly with TypeError: Failed to > execute 'decodeAudioData' on 'BaseAudioContext': parameter 1 is not of type > 'ArrayBuffer'.. > PASS < [null-audiobuffer] All assertions passed. (total 1 assertions) > PASS Decoding a valid audio file resolved correctly. > PASS < [decode-valid-file] All assertions passed. (total 1 assertions) > PASS Decoding an invalid audio file rejected correctly with EncodingError: > Unable to decode audio data. > PASS < [decode-invalid-file] All assertions passed. (total 1 assertions) > PASS Decoding valid file by callback function successCallback invoked > correctly > PASS Decoding a file via promise resolved correctly. > PASS Two buffers decoded by callback function and promise are identical > PASS < [promise-and-success-callback] All assertions passed. (total 3 > assertions) > PASS Decoding invalid file with promise and callback: errorCallback invoked > correctly with EncodingError: Unable to decode audio data > PASS decodeAudioData promise rejected correctly with EncodingError: Unable to > decode audio data. > PASS < [promise-and-error-callback] All assertions passed. (total 2 assertions) > PASS < [load-data] All assertions passed. (total 0 assertions) > PASS Decoded buffer length (frames) is equal to 44100. > PASS Decoded buffer duration (sec) is equal to 1. > PASS Decoded buffer sample rate (Hz) is equal to 44100. > PASS Number of channels in decoded buffer is equal to 1. > PASS Decoded buffer channel #0 is identical to the array the expected channel > #0. > PASS The buffer correctly decoded after the context has been closed > PASS Decoding valid file completed successfully > PASS < [close-context-with-pending-decode] All assertions passed. (total 7 > assertions) > PASS # AUDIT TASK RUNNER FINISHED: 7 tasks ran successfully. > Harness: the test ran to completion. > ------------------------------------------- > > https://codereview.chromium.org/2697733004/diff/1/third_party/WebKit/LayoutTe... > File third_party/WebKit/LayoutTests/webaudio/resources/audit.js (right): > > https://codereview.chromium.org/2697733004/diff/1/third_party/WebKit/LayoutTe... > third_party/WebKit/LayoutTests/webaudio/resources/audit.js:334: return > this._actual.then(function (result) { > On 2017/02/14 22:47:56, Raymond Toy wrote: > > This needs to be documented somehow. > > Done. > > https://codereview.chromium.org/2697733004/diff/1/third_party/WebKit/LayoutTe... > third_party/WebKit/LayoutTests/webaudio/resources/audit.js:1111: function > loadFileAtUrl (fileUrl) { > On 2017/02/14 22:47:56, Raymond Toy wrote: > > Documentation says loadFile, but the function is loadFileAtUrl. I think I > > actually prefer loadFileFromUrl. > > > > Would this support data uri's? If not, that's ok. > > Done. > > A good question. I have no idea for now. Just found out that the last task is not doing what it supposed to do. > // If the context is closing before decodeAudioData has finished decoding, we should reject the > // promise from decodeAudioData. The original test does not check if it rejects. In fact, it just compares two decoded buffers. The spec does not say about the decoding behavior on the closed context. Do you have any reference for this?
On 2017/02/15 18:11:17, hongchan wrote: > On 2017/02/14 22:54:39, hongchan wrote: > > This is the result from the test. > > > > ------------------------------------------- > > This is a testharness.js-based test. > > PASS # AUDIT TASK RUNNER STARTED. > > PASS Decoding null AudioBuffer rejected correctly with TypeError: Failed to > > execute 'decodeAudioData' on 'BaseAudioContext': parameter 1 is not of type > > 'ArrayBuffer'.. > > PASS < [null-audiobuffer] All assertions passed. (total 1 assertions) > > PASS Decoding a valid audio file resolved correctly. > > PASS < [decode-valid-file] All assertions passed. (total 1 assertions) > > PASS Decoding an invalid audio file rejected correctly with EncodingError: > > Unable to decode audio data. > > PASS < [decode-invalid-file] All assertions passed. (total 1 assertions) > > PASS Decoding valid file by callback function successCallback invoked > > correctly > > PASS Decoding a file via promise resolved correctly. > > PASS Two buffers decoded by callback function and promise are identical > > PASS < [promise-and-success-callback] All assertions passed. (total 3 > > assertions) > > PASS Decoding invalid file with promise and callback: errorCallback invoked > > correctly with EncodingError: Unable to decode audio data > > PASS decodeAudioData promise rejected correctly with EncodingError: Unable > to > > decode audio data. > > PASS < [promise-and-error-callback] All assertions passed. (total 2 > assertions) > > PASS < [load-data] All assertions passed. (total 0 assertions) > > PASS Decoded buffer length (frames) is equal to 44100. > > PASS Decoded buffer duration (sec) is equal to 1. > > PASS Decoded buffer sample rate (Hz) is equal to 44100. > > PASS Number of channels in decoded buffer is equal to 1. > > PASS Decoded buffer channel #0 is identical to the array the expected > channel > > #0. > > PASS The buffer correctly decoded after the context has been closed > > PASS Decoding valid file completed successfully > > PASS < [close-context-with-pending-decode] All assertions passed. (total 7 > > assertions) > > PASS # AUDIT TASK RUNNER FINISHED: 7 tasks ran successfully. > > Harness: the test ran to completion. > > ------------------------------------------- > > > > > https://codereview.chromium.org/2697733004/diff/1/third_party/WebKit/LayoutTe... > > File third_party/WebKit/LayoutTests/webaudio/resources/audit.js (right): > > > > > https://codereview.chromium.org/2697733004/diff/1/third_party/WebKit/LayoutTe... > > third_party/WebKit/LayoutTests/webaudio/resources/audit.js:334: return > > this._actual.then(function (result) { > > On 2017/02/14 22:47:56, Raymond Toy wrote: > > > This needs to be documented somehow. > > > > Done. > > > > > https://codereview.chromium.org/2697733004/diff/1/third_party/WebKit/LayoutTe... > > third_party/WebKit/LayoutTests/webaudio/resources/audit.js:1111: function > > loadFileAtUrl (fileUrl) { > > On 2017/02/14 22:47:56, Raymond Toy wrote: > > > Documentation says loadFile, but the function is loadFileAtUrl. I think I > > > actually prefer loadFileFromUrl. > > > > > > Would this support data uri's? If not, that's ok. > > > > Done. > > > > A good question. I have no idea for now. > > Just found out that the last task is not doing what it supposed to do. > > > // If the context is closing before decodeAudioData has finished decoding, we > should reject the > > // promise from decodeAudioData. > > The original test does not check if it rejects. In fact, it just compares two > decoded buffers. > The spec does not say about the decoding behavior on the closed context. Do you > have any reference for this? I wonder if this is an artifact of the fact that a closed context has a sample rate of 0? Then we can't resample the buffer to the appropriate sample rate. We have a bug filed on this, so maybe the best course here is to fix the comment, add a TODO on why (if my theory is right), and move on with the conversion.
On 2017/02/15 18:11:17, hongchan wrote: > On 2017/02/14 22:54:39, hongchan wrote: > > This is the result from the test. > > > > ------------------------------------------- > > This is a testharness.js-based test. > > PASS # AUDIT TASK RUNNER STARTED. > > PASS Decoding null AudioBuffer rejected correctly with TypeError: Failed to > > execute 'decodeAudioData' on 'BaseAudioContext': parameter 1 is not of type > > 'ArrayBuffer'.. > > PASS < [null-audiobuffer] All assertions passed. (total 1 assertions) > > PASS Decoding a valid audio file resolved correctly. > > PASS < [decode-valid-file] All assertions passed. (total 1 assertions) > > PASS Decoding an invalid audio file rejected correctly with EncodingError: > > Unable to decode audio data. > > PASS < [decode-invalid-file] All assertions passed. (total 1 assertions) > > PASS Decoding valid file by callback function successCallback invoked > > correctly > > PASS Decoding a file via promise resolved correctly. > > PASS Two buffers decoded by callback function and promise are identical > > PASS < [promise-and-success-callback] All assertions passed. (total 3 > > assertions) > > PASS Decoding invalid file with promise and callback: errorCallback invoked > > correctly with EncodingError: Unable to decode audio data > > PASS decodeAudioData promise rejected correctly with EncodingError: Unable > to > > decode audio data. > > PASS < [promise-and-error-callback] All assertions passed. (total 2 > assertions) > > PASS < [load-data] All assertions passed. (total 0 assertions) > > PASS Decoded buffer length (frames) is equal to 44100. > > PASS Decoded buffer duration (sec) is equal to 1. > > PASS Decoded buffer sample rate (Hz) is equal to 44100. > > PASS Number of channels in decoded buffer is equal to 1. > > PASS Decoded buffer channel #0 is identical to the array the expected > channel > > #0. > > PASS The buffer correctly decoded after the context has been closed > > PASS Decoding valid file completed successfully > > PASS < [close-context-with-pending-decode] All assertions passed. (total 7 > > assertions) > > PASS # AUDIT TASK RUNNER FINISHED: 7 tasks ran successfully. > > Harness: the test ran to completion. > > ------------------------------------------- > > > > > https://codereview.chromium.org/2697733004/diff/1/third_party/WebKit/LayoutTe... > > File third_party/WebKit/LayoutTests/webaudio/resources/audit.js (right): > > > > > https://codereview.chromium.org/2697733004/diff/1/third_party/WebKit/LayoutTe... > > third_party/WebKit/LayoutTests/webaudio/resources/audit.js:334: return > > this._actual.then(function (result) { > > On 2017/02/14 22:47:56, Raymond Toy wrote: > > > This needs to be documented somehow. > > > > Done. > > > > > https://codereview.chromium.org/2697733004/diff/1/third_party/WebKit/LayoutTe... > > third_party/WebKit/LayoutTests/webaudio/resources/audit.js:1111: function > > loadFileAtUrl (fileUrl) { > > On 2017/02/14 22:47:56, Raymond Toy wrote: > > > Documentation says loadFile, but the function is loadFileAtUrl. I think I > > > actually prefer loadFileFromUrl. > > > > > > Would this support data uri's? If not, that's ok. > > > > Done. > > > > A good question. I have no idea for now. > > Just found out that the last task is not doing what it supposed to do. > > > // If the context is closing before decodeAudioData has finished decoding, we > should reject the > > // promise from decodeAudioData. > > The original test does not check if it rejects. In fact, it just compares two > decoded buffers. > The spec does not say about the decoding behavior on the closed context. Do you > have any reference for this? Actually the comment in the test is incorrect. This is what the spec says: "The system shall ensure that the AudioContext is not garbage collected before the promise is resolved or rejected and any callback function is called and completes." See: https://webaudio.github.io/web-audio-api/#widl-BaseAudioContext-decodeAudioDa... Fortunately what the test is doing is correct. I'll keep work on it.
For the next step (clean up #2): - Add/edit some comments. - Run clang-format on the test file. That should wrap up this CL.
Patchset #4 (id:60001) has been deleted
Patchset #4 (id:80001) has been deleted
Patchset #4 (id:100001) has been deleted
Please take one last look.
Just a few minor questions. https://codereview.chromium.org/2697733004/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/webaudio/decodeAudioData/decode-audio-data-basic.html (right): https://codereview.chromium.org/2697733004/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/webaudio/decodeAudioData/decode-audio-data-basic.html:11: // Use offline context for decoding because we want a fixed know sample Typo: "know" -> "known" https://codereview.chromium.org/2697733004/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/webaudio/decodeAudioData/decode-audio-data-basic.html:62: ]) This works because should().beResolved() and .beRejected() return promises that resolve successfully? https://codereview.chromium.org/2697733004/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/webaudio/decodeAudioData/decode-audio-data-basic.html:76: 'Decoding valid file by callback function') A comment stating what callbackArg should be is useful. An AudioBuffer implies this is the success callback and anything else means it's the error callback. https://codereview.chromium.org/2697733004/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/webaudio/decodeAudioData/decode-audio-data-basic.html:94: // Step 3: compare two buffers from Step 1 and Step 2. Is it guaranteed that the callbacks have fired by the time we get here to do the comparison? https://codereview.chromium.org/2697733004/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/webaudio/decodeAudioData/decode-audio-data-basic.html:124: // See crbug.com/692650 I think you can say TODO(crbug.com/692650). https://codereview.chromium.org/2697733004/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/webaudio/decodeAudioData/decode-audio-data-basic.html:126: let resolveOrReject = (promiseArg) => { A comment on what resolveOrReject means would be useful, like you did above for the success/error callbacks. https://codereview.chromium.org/2697733004/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/webaudio/decodeAudioData/decode-audio-data-basic.html:147: 'the expected channel #' + c); I didn't know you could do this! Nice! https://codereview.chromium.org/2697733004/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/webaudio/resources/audit.js (right): https://codereview.chromium.org/2697733004/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/webaudio/resources/audit.js:1140: 'loadFile: Network failure when loading ' + fileUrl + '.'; Is this the only way for xhr to fail? (Network failure)
https://codereview.chromium.org/2697733004/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/webaudio/decodeAudioData/decode-audio-data-basic.html (right): https://codereview.chromium.org/2697733004/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/webaudio/decodeAudioData/decode-audio-data-basic.html:11: // Use offline context for decoding because we want a fixed know sample On 2017/02/15 23:15:52, Raymond Toy wrote: > Typo: "know" -> "known" Done. https://codereview.chromium.org/2697733004/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/webaudio/decodeAudioData/decode-audio-data-basic.html:62: ]) On 2017/02/15 23:15:52, Raymond Toy wrote: > This works because should().beResolved() and .beRejected() return promises that > resolve successfully? Yes. The order can be flaky as noted, but it should not matter for WPT. https://codereview.chromium.org/2697733004/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/webaudio/decodeAudioData/decode-audio-data-basic.html:76: 'Decoding valid file by callback function') On 2017/02/15 23:15:52, Raymond Toy wrote: > A comment stating what callbackArg should be is useful. An AudioBuffer implies > this is the success callback and anything else means it's the error callback. Done. Yes, you're correct. https://codereview.chromium.org/2697733004/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/webaudio/decodeAudioData/decode-audio-data-basic.html:94: // Step 3: compare two buffers from Step 1 and Step 2. On 2017/02/15 23:15:52, Raymond Toy wrote: > Is it guaranteed that the callbacks have fired by the time we get here to do the > comparison? Not guaranteed, and that needs to be fixed in a follow up CL. Or do you want it to be addressed here? https://codereview.chromium.org/2697733004/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/webaudio/decodeAudioData/decode-audio-data-basic.html:124: // See crbug.com/692650 On 2017/02/15 23:15:53, Raymond Toy wrote: > I think you can say TODO(crbug.com/692650). Done. Is this style conventional? https://codereview.chromium.org/2697733004/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/webaudio/decodeAudioData/decode-audio-data-basic.html:126: let resolveOrReject = (promiseArg) => { On 2017/02/15 23:15:52, Raymond Toy wrote: > A comment on what resolveOrReject means would be useful, like you did above for > the success/error callbacks. Done. https://codereview.chromium.org/2697733004/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/webaudio/decodeAudioData/decode-audio-data-basic.html:147: 'the expected channel #' + c); On 2017/02/15 23:15:53, Raymond Toy wrote: > I didn't know you could do this! Nice! :D https://codereview.chromium.org/2697733004/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/webaudio/resources/audit.js (right): https://codereview.chromium.org/2697733004/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/webaudio/resources/audit.js:1140: 'loadFile: Network failure when loading ' + fileUrl + '.'; On 2017/02/15 23:15:53, Raymond Toy wrote: > Is this the only way for xhr to fail? (Network failure) Nope. See l.1130. If the request status is not 200, it is considered an invalid response. Also this diff should have not been included here. I think these are unwanted artifacts of clang-format.
lgtm lgtm with nits https://codereview.chromium.org/2697733004/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/webaudio/decodeAudioData/decode-audio-data-basic.html (right): https://codereview.chromium.org/2697733004/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/webaudio/decodeAudioData/decode-audio-data-basic.html:94: // Step 3: compare two buffers from Step 1 and Step 2. On 2017/02/15 23:40:36, hongchan wrote: > On 2017/02/15 23:15:52, Raymond Toy wrote: > > Is it guaranteed that the callbacks have fired by the time we get here to do > the > > comparison? > > Not guaranteed, and that needs to be fixed in a follow up CL. > > Or do you want it to be addressed here? A TODO with a crbug link is good enough. https://codereview.chromium.org/2697733004/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/webaudio/decodeAudioData/decode-audio-data-basic.html:124: // See crbug.com/692650 On 2017/02/15 23:40:35, hongchan wrote: > On 2017/02/15 23:15:53, Raymond Toy wrote: > > I think you can say TODO(crbug.com/692650). > > Done. Is this style conventional? I've seen this in other places, perhaps in chromium. I kind of like it because the bug can have discussions and such on how to resolve the issue. https://codereview.chromium.org/2697733004/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/webaudio/decodeAudioData/decode-audio-data-basic.html (right): https://codereview.chromium.org/2697733004/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/webaudio/decodeAudioData/decode-audio-data-basic.html:127: // handlers; it is an decoded audio buffer for success case and an error Typo: "an" -> "a"
> https://codereview.chromium.org/2697733004/diff/120001/third_party/WebKit/Lay... > third_party/WebKit/LayoutTests/webaudio/decodeAudioData/decode-audio-data-basic.html:94: > // Step 3: compare two buffers from Step 1 and Step 2. > On 2017/02/15 23:40:36, hongchan wrote: > > On 2017/02/15 23:15:52, Raymond Toy wrote: > > > Is it guaranteed that the callbacks have fired by the time we get here to do > > the > > > comparison? > > > > Not guaranteed, and that needs to be fixed in a follow up CL. > > > > Or do you want it to be addressed here? > > A TODO with a crbug link is good enough. So is this something that we want to fix? The order does not really matter.
The CQ bit was checked by hongchan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rtoy@chromium.org Link to the patchset: https://codereview.chromium.org/2697733004/#ps160001 (title: "Fixed type after l-g-t-m")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/02/16 16:35:11, hongchan wrote: > > > https://codereview.chromium.org/2697733004/diff/120001/third_party/WebKit/Lay... > > > third_party/WebKit/LayoutTests/webaudio/decodeAudioData/decode-audio-data-basic.html:94: > > // Step 3: compare two buffers from Step 1 and Step 2. > > On 2017/02/15 23:40:36, hongchan wrote: > > > On 2017/02/15 23:15:52, Raymond Toy wrote: > > > > Is it guaranteed that the callbacks have fired by the time we get here to > do > > > the > > > > comparison? > > > > > > Not guaranteed, and that needs to be fixed in a follow up CL. > > > > > > Or do you want it to be addressed here? > > > > A TODO with a crbug link is good enough. > > So is this something that we want to fix? The order does not really matter. Well, you said it needs to be fixed. So does it?
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1487263348016590,
"parent_rev": "74feab800a38e1cc133c3406d19f966844881e42", "commit_rev":
"0e7af755c6ff3377abb22925f319c8cfb699461d"}
Message was sent while issue was closed.
Description was changed from ========== Refactor decode-audio-data-basic.html to use testharness - Replace js-test and old Audit with testharness and new Audit. - Promisefied file loader |Audit.loadFileAtUrl(fileUrl)| is added. - Refactored complex promise/callback code. - should().beResolved() now bypassing its argument to the next promise. For the first pass, I focused on preserving the test behavior. If this looks good, I will move on to the clean-up and more refactoring. BUG=688064 TEST= LayoutTests/webaudio/decodeAudioData/decode-audio-data-basic.html ========== to ========== Refactor decode-audio-data-basic.html to use testharness - Replace js-test and old Audit with testharness and new Audit. - Promisefied file loader |Audit.loadFileAtUrl(fileUrl)| is added. - Refactored complex promise/callback code. - should().beResolved() now bypassing its argument to the next promise. For the first pass, I focused on preserving the test behavior. If this looks good, I will move on to the clean-up and more refactoring. BUG=688064 TEST= LayoutTests/webaudio/decodeAudioData/decode-audio-data-basic.html Review-Url: https://codereview.chromium.org/2697733004 Cr-Commit-Position: refs/heads/master@{#451011} Committed: https://chromium.googlesource.com/chromium/src/+/0e7af755c6ff3377abb22925f319... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:160001) as https://chromium.googlesource.com/chromium/src/+/0e7af755c6ff3377abb22925f319... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
