Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2872393002:
Add layout-test-tidy to LayoutTests/webaudio/tools (Closed)
|
Created:
3 years, 7 months ago by hongchan Modified:
3 years, 7 months ago Reviewers:
Raymond Toy CC:
chromium-reviews, blink-reviews Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd layout-test-tidy to LayoutTests/webaudio/tools
This tool, a Node.js CLI utility, performs a set of clean-up tasks for layout
test files in LayoutTests/webaudio. (Potentially it can be applied to any kind
of layout test files in HTML or JS format.)
The clean-up tasks includes:
- Sanitize missing or incorrect HTML elements: reordering |script| elements or
adding missing |title| element.
- Apply [html-tidy](http://www.html-tidy.org/).
- Apply [clang-format](https://clang.llvm.org/docs/ClangFormat.html).
- Perform RegExp substitution based on predefined dictionary. (e.g. |var| to
|let|, or redundant empty lines in markup.)
For every step of clean-up processing, the tool collects warning, diagnostics or
notes and print out the logs at the end of the processing.
For more info, refer README.md file included.
BUG=720378
Review-Url: https://codereview.chromium.org/2872393002
Cr-Commit-Position: refs/heads/master@{#471857}
Committed: https://chromium.googlesource.com/chromium/src/+/1dd94e01da63c05f3c6855ac25a911f2d43868ae
Patch Set 1 #
Total comments: 38
Patch Set 2 : Addressing feedback (1) #
Total comments: 10
Patch Set 3 : Addressing feedback (2) #
Total comments: 1
Patch Set 4 : Remove indentation hack after l-g-t-m #
Messages
Total messages: 17 (8 generated)
hongchan@chromium.org changed reviewers: + rtoy@chromium.org
PTAL.
https://codereview.chromium.org/2872393002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/webaudio/tools/README.md (right): https://codereview.chromium.org/2872393002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/webaudio/tools/README.md:3: This tool, a Node.js CLI utility, performs a set of clean-up tasks for layout test files in LayoutTests/webaudio. (Potentially it can be applied to any kind of layout test files in HTML or JS format.) These lines can be wrapped to 80 columns for markdown files without changing the output, right? https://codereview.chromium.org/2872393002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/webaudio/tools/README.md:29: # Dry run and piping. Add a note that you need to inspect result.txt because it contains error/warning/info messages https://codereview.chromium.org/2872393002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/webaudio/tools/README.md:30: node layout-test-tidy ${TARGET_PATH} --dryrun > result.txt I would really like it if layout-test-tidy would read from stdin if TARGET_PATH is not given. I think this would automatically imply --dryrun. But then I think you also need to add an option to indicate whether the input is html or just javascript. https://codereview.chromium.org/2872393002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/webaudio/tools/README.md:36: - If there is a file to be skipped, simply add its absolute file path to |skip-tidy| file. Do we really want to support a skip-tidy file? I'm thinking we don't because there shouldn't be any reason for us to skip any file. https://codereview.chromium.org/2872393002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/webaudio/tools/layout-test-tidy.js (right): https://codereview.chromium.org/2872393002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/webaudio/tools/layout-test-tidy.js:38: {regexp: /var /, replace: 'let '}, I think you need to tighten up the regexp here: let message = "This var should be safe to use" will become "This let should be safe to use". https://codereview.chromium.org/2872393002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/webaudio/tools/layout-test-tidy.js:41: // Clean up redundant braces at the beginning/end. Expand on why this is needed. Presumably for clang format? How do you prevent code containing three braces from getting deleted? https://codereview.chromium.org/2872393002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/webaudio/tools/layout-test-tidy.js:42: {regexp: /test\-code\"\>(\n\s*\{){3}/, replace: 'test-code">'}, Why is the replacement "test-code>"? https://codereview.chromium.org/2872393002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/webaudio/tools/layout-test-tidy.js:51: const Bar80 = { What is this for? https://codereview.chromium.org/2872393002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/webaudio/tools/layout-test-tidy.js:87: * @param {Array} swapCollection Array of key-value pairs. {regexp: replace} "{regexp: replace}" looks weird for what you're trying to describe? https://codereview.chromium.org/2872393002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/webaudio/tools/layout-test-tidy.js:133: let clangFormatBinary = __dirname + '/node_modules/clang-format/bin/'; Does this mean we actually install our own copy of clang-format? If so, does it matter if the installed version doesn't match the version from depot_tools? I think we should just use depot_tools/clang-format, if possible. https://codereview.chromium.org/2872393002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/webaudio/tools/layout-test-tidy.js:139: let echo = cp.spawn('echo', [codeString]); IIUC, this basically runs a shell with the echo command whose argument is the code string. If so, I think this is wrong. Shell quoting will mess things up and command lines have a relatively short line length limit. https://codereview.chromium.org/2872393002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/webaudio/tools/layout-test-tidy.js:214: // If the title is missing, add one with the file name. "with the" -> "from the" https://codereview.chromium.org/2872393002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/webaudio/tools/layout-test-tidy.js:228: // section. Do we really want to do this? Can't think of anything that would break with this, though. https://codereview.chromium.org/2872393002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/webaudio/tools/layout-test-tidy.js:257: element.id = 'layout-test-code'; Is this safe? What if another element has the same id? https://codereview.chromium.org/2872393002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/webaudio/tools/layout-test-tidy.js:258: // If the element was belong to something else than body, move it. "was belong" -> "belongs", I think. "else than" -> "else other than" Move it where? https://codereview.chromium.org/2872393002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/webaudio/tools/layout-test-tidy.js:265: task.addLog('DOMUtil.getElementWithTestCodeSync', Is it really bad to have more than one? What if we do? https://codereview.chromium.org/2872393002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/webaudio/tools/layout-test-tidy.js:331: // Extract JS code string with additional braces for indentation hack. Add note that the number of braces depends on the indent size and the expected script tag level. (It would be cool if this could be computed based on the level of the script tag. https://codereview.chromium.org/2872393002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/webaudio/tools/layout-test-tidy.js:332: let codeString = '{\n{\n{' + scriptElement.textContent + '}\n}\n}'; When I did this by hand, I inserted "{{{\n" and appended "\n}}}". I think that's better than separating each by \n, and it's nice to have the actual code start a new line rather than being smashed up against "{". (Not that that is really visible anywhere, but might be useful for debugging.) https://codereview.chromium.org/2872393002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/webaudio/tools/layout-test-tidy.js:334: // Start with clang-foramt and text-based operation. What does "Start" mean here? What will you end with?
Description was changed from
==========
Add layout-test-tidy to LayoutTests/webaudio/tools
This tool, a Node.js CLI utility, performs a set of clean-up tasks for layout
test files in LayoutTests/webaudio. (Potentially it can be applied to any kind
of layout test files in HTML or JS format.)
The clean-up tasks includes:
- Sanitize missing or incorrect HTML elements: reordering |script| elements or
adding missing |title| element.
- Apply [html-tidy](http://www.html-tidy.org/).
- Apply [clang-format](https://clang.llvm.org/docs/ClangFormat.html).
- Perform RegExp substitution based on predefined dictionary. (e.g. |var| to
|let|, or redundant empty lines in markup.)
For every step of clean-up processing, the tool collects warning, diagnostics or
notes and print out the logs at the end of the processing.
For more info, refer README.md file included.
BUG=720378
==========
to
==========
Add layout-test-tidy to LayoutTests/webaudio/tools
This tool, a Node.js CLI utility, performs a set of clean-up tasks for layout
test files in LayoutTests/webaudio. (Potentially it can be applied to any kind
of layout test files in HTML or JS format.)
The clean-up tasks includes:
- Sanitize missing or incorrect HTML elements: reordering |script| elements or
adding missing |title| element.
- Apply [html-tidy](http://www.html-tidy.org/).
- Apply [clang-format](https://clang.llvm.org/docs/ClangFormat.html).
- Perform RegExp substitution based on predefined dictionary. (e.g. |var| to
|let|, or redundant empty lines in markup.)
For every step of clean-up processing, the tool collects warning, diagnostics or
notes and print out the logs at the end of the processing.
For more info, refer README.md file included.
The following is the example output from test-basic.html
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/webaudio/...
---
<!DOCTYPE html>
<!--
This is a very basic test to make sure testRunner.setAudioData() works
correctly.
It generates a 2 seconds long stereo result @44.1KHz
The left channel will be a 880Hz tone, while the right will be 440Hz.
-->
<html>
<head>
<title>
test-basic.html
</title>
<script src="../resources/testharness.js"></script>
<script src="../resources/testharnessreport.js"></script>
<script src="resources/audit-util.js"></script>
<script src="resources/audit.js"></script>
</head>
<body>
<script id="layout-test-code">
let audit = Audit.createTaskRunner();
let sampleRate = 44100.0;
let lengthInSeconds = 2;
function generateSinWave(audioBuffer) {
let n = audioBuffer.length;
let channelL = audioBuffer.getChannelData(0);
let channelR = audioBuffer.getChannelData(1);
let sampleRate = audioBuffer.sampleRate;
for (let i = 0; i < n; ++i) {
channelL[i] = Math.sin(880.0 * 2.0 * Math.PI * i / sampleRate);
channelR[i] = Math.sin(440.0 * 2.0 * Math.PI * i / sampleRate);
}
}
audit.define(
{
label: 'test',
description:
'Basic audio test infrastructure: testRunner.setAudioData()'
},
(task, should) => {
if (!window.testRunner) {
should(!window.testRunner, 'window.testRunner is defined')
.beTrue();
task.done();
return;
}
let context = new AudioContext();
let audioBuffer = context.createBuffer(
2, lengthInSeconds * sampleRate, sampleRate);
generateSinWave(audioBuffer);
let audioData = createAudioData(audioBuffer);
testRunner.setAudioData(audioData);
testRunner.notifyDone();
task.done();
});
audit.run();
</script>
</body>
</html>
---
BUG=720378
==========
Patchset #2 (id:20001) has been deleted
Sorry for the messy patch. Now you can specify two options: * --inplace or -i: in-place processing. |false| by default. * --verbose or -v: verbose output for warnings/logs. |false| by default. https://codereview.chromium.org/2872393002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/webaudio/tools/README.md (right): https://codereview.chromium.org/2872393002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/webaudio/tools/README.md:3: This tool, a Node.js CLI utility, performs a set of clean-up tasks for layout test files in LayoutTests/webaudio. (Potentially it can be applied to any kind of layout test files in HTML or JS format.) On 2017/05/10 21:28:43, Raymond Toy wrote: > These lines can be wrapped to 80 columns for markdown files without changing the > output, right? "When you do want to insert a break tag using Markdown, you end a line with two or more spaces, then type return." Just learned a new thing. I originally had it in wrapped, but unrolled it because I thought it was a right way for the markdown. done. https://codereview.chromium.org/2872393002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/webaudio/tools/README.md:29: # Dry run and piping. On 2017/05/10 21:28:43, Raymond Toy wrote: > Add a note that you need to inspect result.txt because it contains > error/warning/info messages Done. https://codereview.chromium.org/2872393002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/webaudio/tools/README.md:30: node layout-test-tidy ${TARGET_PATH} --dryrun > result.txt On 2017/05/10 21:28:43, Raymond Toy wrote: > I would really like it if layout-test-tidy would read from stdin if TARGET_PATH > is not given. I think this would automatically imply --dryrun. But then I > think you also need to add an option to indicate whether the input is html or > just javascript. Make sense, but it sounds like a big change for the current CL. I would like to punt this. https://codereview.chromium.org/2872393002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/webaudio/tools/README.md:36: - If there is a file to be skipped, simply add its absolute file path to |skip-tidy| file. On 2017/05/10 21:28:43, Raymond Toy wrote: > Do we really want to support a skip-tidy file? I'm thinking we don't because > there shouldn't be any reason for us to skip any file. If you want to leave out certain files for some reason, this is useful. I had to do that few times when I was testing. https://codereview.chromium.org/2872393002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/webaudio/tools/layout-test-tidy.js (right): https://codereview.chromium.org/2872393002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/webaudio/tools/layout-test-tidy.js:38: {regexp: /var /, replace: 'let '}, On 2017/05/10 21:28:44, Raymond Toy wrote: > I think you need to tighten up the regexp here: > > let message = "This var should be safe to use" > > will become "This let should be safe to use". > Please advise if you have a better idea. https://codereview.chromium.org/2872393002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/webaudio/tools/layout-test-tidy.js:41: // Clean up redundant braces at the beginning/end. On 2017/05/10 21:28:44, Raymond Toy wrote: > Expand on why this is needed. Presumably for clang format? > > How do you prevent code containing three braces from getting deleted? This is a hack for clang-format module. The result of clang-format always starts from 0 col, no matter when the original code was at. So the clang-format modules adds 3 braces top and bottom, and this takes them away at the end of process. https://codereview.chromium.org/2872393002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/webaudio/tools/layout-test-tidy.js:42: {regexp: /test\-code\"\>(\n\s*\{){3}/, replace: 'test-code">'}, On 2017/05/10 21:28:44, Raymond Toy wrote: > Why is the replacement "test-code>"? Basically |test-code>| is the beginning of the test code. Just trying to be as specific as possible. Otherwise, this will be applied to all the script elements. https://codereview.chromium.org/2872393002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/webaudio/tools/layout-test-tidy.js:42: {regexp: /test\-code\"\>(\n\s*\{){3}/, replace: 'test-code">'}, On 2017/05/10 21:28:44, Raymond Toy wrote: > Why is the replacement "test-code>"? Fixed with more specific patterns. https://codereview.chromium.org/2872393002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/webaudio/tools/layout-test-tidy.js:51: const Bar80 = { On 2017/05/10 21:28:44, Raymond Toy wrote: > What is this for? This prints out the 80 col ruler top and bottom, only for --dryrun. Now the latest patch has the auto detection of overflowed lines, so I am removing this. https://codereview.chromium.org/2872393002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/webaudio/tools/layout-test-tidy.js:87: * @param {Array} swapCollection Array of key-value pairs. {regexp: replace} On 2017/05/10 21:28:44, Raymond Toy wrote: > "{regexp: replace}" looks weird for what you're trying to describe? Done. https://codereview.chromium.org/2872393002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/webaudio/tools/layout-test-tidy.js:133: let clangFormatBinary = __dirname + '/node_modules/clang-format/bin/'; On 2017/05/10 21:28:44, Raymond Toy wrote: > Does this mean we actually install our own copy of clang-format? If so, does it > matter if the installed version doesn't match the version from depot_tools? > > I think we should just use depot_tools/clang-format, if possible. Until the weird depot_tools issue is resolved, I prefer to use clang-format that we can control the version of it. https://bugs.chromium.org/p/chromium/issues/detail?id=558447 https://codereview.chromium.org/2872393002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/webaudio/tools/layout-test-tidy.js:139: let echo = cp.spawn('echo', [codeString]); On 2017/05/10 21:28:44, Raymond Toy wrote: > IIUC, this basically runs a shell with the echo command whose argument is the > code string. If so, I think this is wrong. Shell quoting will mess things up > and command lines have a relatively short line length limit. Well, that I don't understand and this worked perfectly on both my mac machines. Feel free to advise alternatives if you have! https://codereview.chromium.org/2872393002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/webaudio/tools/layout-test-tidy.js:214: // If the title is missing, add one with the file name. On 2017/05/10 21:28:43, Raymond Toy wrote: > "with the" -> "from the" Done. https://codereview.chromium.org/2872393002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/webaudio/tools/layout-test-tidy.js:228: // section. On 2017/05/10 21:28:44, Raymond Toy wrote: > Do we really want to do this? Can't think of anything that would break with > this, though. Yes, we do want to have this. All the scripts in the head is synchronous/blocking whereas the script in the body is not. We do want to have all the dependencies loaded first. We actually move all the script element to the head and make the body empty, but I didn't do that. https://codereview.chromium.org/2872393002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/webaudio/tools/layout-test-tidy.js:257: element.id = 'layout-test-code'; On 2017/05/10 21:28:44, Raymond Toy wrote: > Is this safe? What if another element has the same id? If there are two or more script elements with the code, this will throw. https://codereview.chromium.org/2872393002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/webaudio/tools/layout-test-tidy.js:258: // If the element was belong to something else than body, move it. On 2017/05/10 21:28:44, Raymond Toy wrote: > "was belong" -> "belongs", I think. > "else than" -> "else other than" > > Move it where? Done. https://codereview.chromium.org/2872393002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/webaudio/tools/layout-test-tidy.js:265: task.addLog('DOMUtil.getElementWithTestCodeSync', On 2017/05/10 21:28:43, Raymond Toy wrote: > Is it really bad to have more than one? What if we do? Yes, it's bad. If there are two script tags with actual code, that means refactoring is necessary. I believe only the high-level code should be used in the html file within one element. Other codes need to be in the separate JS files. https://codereview.chromium.org/2872393002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/webaudio/tools/layout-test-tidy.js:331: // Extract JS code string with additional braces for indentation hack. On 2017/05/10 21:28:44, Raymond Toy wrote: > Add note that the number of braces depends on the indent size and the expected > script tag level. > > (It would be cool if this could be computed based on the level of the script > tag. Currently the main script is always two level lower from the <html> and one level lower from the <body>. Are we expecting to have something else in the future? https://codereview.chromium.org/2872393002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/webaudio/tools/layout-test-tidy.js:334: // Start with clang-foramt and text-based operation. On 2017/05/10 21:28:43, Raymond Toy wrote: > What does "Start" mean here? What will you end with? Done.
Description was changed from
==========
Add layout-test-tidy to LayoutTests/webaudio/tools
This tool, a Node.js CLI utility, performs a set of clean-up tasks for layout
test files in LayoutTests/webaudio. (Potentially it can be applied to any kind
of layout test files in HTML or JS format.)
The clean-up tasks includes:
- Sanitize missing or incorrect HTML elements: reordering |script| elements or
adding missing |title| element.
- Apply [html-tidy](http://www.html-tidy.org/).
- Apply [clang-format](https://clang.llvm.org/docs/ClangFormat.html).
- Perform RegExp substitution based on predefined dictionary. (e.g. |var| to
|let|, or redundant empty lines in markup.)
For every step of clean-up processing, the tool collects warning, diagnostics or
notes and print out the logs at the end of the processing.
For more info, refer README.md file included.
The following is the example output from test-basic.html
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/webaudio/...
---
<!DOCTYPE html>
<!--
This is a very basic test to make sure testRunner.setAudioData() works
correctly.
It generates a 2 seconds long stereo result @44.1KHz
The left channel will be a 880Hz tone, while the right will be 440Hz.
-->
<html>
<head>
<title>
test-basic.html
</title>
<script src="../resources/testharness.js"></script>
<script src="../resources/testharnessreport.js"></script>
<script src="resources/audit-util.js"></script>
<script src="resources/audit.js"></script>
</head>
<body>
<script id="layout-test-code">
let audit = Audit.createTaskRunner();
let sampleRate = 44100.0;
let lengthInSeconds = 2;
function generateSinWave(audioBuffer) {
let n = audioBuffer.length;
let channelL = audioBuffer.getChannelData(0);
let channelR = audioBuffer.getChannelData(1);
let sampleRate = audioBuffer.sampleRate;
for (let i = 0; i < n; ++i) {
channelL[i] = Math.sin(880.0 * 2.0 * Math.PI * i / sampleRate);
channelR[i] = Math.sin(440.0 * 2.0 * Math.PI * i / sampleRate);
}
}
audit.define(
{
label: 'test',
description:
'Basic audio test infrastructure: testRunner.setAudioData()'
},
(task, should) => {
if (!window.testRunner) {
should(!window.testRunner, 'window.testRunner is defined')
.beTrue();
task.done();
return;
}
let context = new AudioContext();
let audioBuffer = context.createBuffer(
2, lengthInSeconds * sampleRate, sampleRate);
generateSinWave(audioBuffer);
let audioData = createAudioData(audioBuffer);
testRunner.setAudioData(audioData);
testRunner.notifyDone();
task.done();
});
audit.run();
</script>
</body>
</html>
---
BUG=720378
==========
to
==========
Add layout-test-tidy to LayoutTests/webaudio/tools
This tool, a Node.js CLI utility, performs a set of clean-up tasks for layout
test files in LayoutTests/webaudio. (Potentially it can be applied to any kind
of layout test files in HTML or JS format.)
The clean-up tasks includes:
- Sanitize missing or incorrect HTML elements: reordering |script| elements or
adding missing |title| element.
- Apply [html-tidy](http://www.html-tidy.org/).
- Apply [clang-format](https://clang.llvm.org/docs/ClangFormat.html).
- Perform RegExp substitution based on predefined dictionary. (e.g. |var| to
|let|, or redundant empty lines in markup.)
For every step of clean-up processing, the tool collects warning, diagnostics or
notes and print out the logs at the end of the processing.
For more info, refer README.md file included.
BUG=720378
==========
The following is the example output from test-basic.html https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/webaudio/... --- <!DOCTYPE html> <!-- This is a very basic test to make sure testRunner.setAudioData() works correctly. It generates a 2 seconds long stereo result @44.1KHz The left channel will be a 880Hz tone, while the right will be 440Hz. --> <html> <head> <title> test-basic.html </title> <script src="../resources/testharness.js"></script> <script src="../resources/testharnessreport.js"></script> <script src="resources/audit-util.js"></script> <script src="resources/audit.js"></script> </head> <body> <script id="layout-test-code"> let audit = Audit.createTaskRunner(); let sampleRate = 44100.0; let lengthInSeconds = 2; function generateSinWave(audioBuffer) { let n = audioBuffer.length; let channelL = audioBuffer.getChannelData(0); let channelR = audioBuffer.getChannelData(1); let sampleRate = audioBuffer.sampleRate; for (let i = 0; i < n; ++i) { channelL[i] = Math.sin(880.0 * 2.0 * Math.PI * i / sampleRate); channelR[i] = Math.sin(440.0 * 2.0 * Math.PI * i / sampleRate); } } audit.define( { label: 'test', description: 'Basic audio test infrastructure: testRunner.setAudioData()' }, (task, should) => { if (!window.testRunner) { should(!window.testRunner, 'window.testRunner is defined') .beTrue(); task.done(); return; } let context = new AudioContext(); let audioBuffer = context.createBuffer( 2, lengthInSeconds * sampleRate, sampleRate); generateSinWave(audioBuffer); let audioData = createAudioData(audioBuffer); testRunner.setAudioData(audioData); testRunner.notifyDone(); task.done(); }); audit.run(); </script> </body> </html> ---
Everything looks good. I have just one major issue. https://codereview.chromium.org/2872393002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/webaudio/tools/layout-test-tidy.js (right): https://codereview.chromium.org/2872393002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/tools/layout-test-tidy.js:25: {'indent': 'yes', 'indent-spaces': '2', 'wrap': '80', 'tidy-mark': 'no'}, Haha. I assume you ran clang-format on this file which moved everything on to one line. Bummer. https://codereview.chromium.org/2872393002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/tools/layout-test-tidy.js:126: // Be sure to pipe the result, not to stdio. What does this mean? https://codereview.chromium.org/2872393002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/tools/layout-test-tidy.js:149: clangFormat.stdin.setEncoding('utf-8'); As we mentioned offline, when we read the file, we probably want to make sure it's read as utf-8 too. (This is not a blocker for this CL.) https://codereview.chromium.org/2872393002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/tools/layout-test-tidy.js:239: scriptElements.forEach(function(element) { Not element => { ? https://codereview.chromium.org/2872393002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/tools/layout-test-tidy.js:346: 0, formattedCodeString.indexOf('/*end*/')); This is not very robust. What if the code actually had /*end*/ somewhere in the middle? You'd get the wrong string result. I really think you should just delete the first 3 lines and the last 3 lines because that's exactly what you added (after formatting).
https://codereview.chromium.org/2872393002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/webaudio/tools/layout-test-tidy.js (right): https://codereview.chromium.org/2872393002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/tools/layout-test-tidy.js:25: {'indent': 'yes', 'indent-spaces': '2', 'wrap': '80', 'tidy-mark': 'no'}, On 2017/05/11 21:50:28, Raymond Toy wrote: > Haha. I assume you ran clang-format on this file which moved everything on to > one line. Bummer. :| https://codereview.chromium.org/2872393002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/tools/layout-test-tidy.js:126: // Be sure to pipe the result, not to stdio. On 2017/05/11 21:50:28, Raymond Toy wrote: > What does this mean? Expanded the comment. https://codereview.chromium.org/2872393002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/tools/layout-test-tidy.js:149: clangFormat.stdin.setEncoding('utf-8'); On 2017/05/11 21:50:28, Raymond Toy wrote: > As we mentioned offline, when we read the file, we probably want to make sure > it's read as utf-8 too. (This is not a blocker for this CL.) Done. https://codereview.chromium.org/2872393002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/tools/layout-test-tidy.js:239: scriptElements.forEach(function(element) { On 2017/05/11 21:50:28, Raymond Toy wrote: > Not element => { ? Done. Also renamed variables for clarification. https://codereview.chromium.org/2872393002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/tools/layout-test-tidy.js:346: 0, formattedCodeString.indexOf('/*end*/')); On 2017/05/11 21:50:28, Raymond Toy wrote: > This is not very robust. What if the code actually had /*end*/ somewhere in the > middle? You'd get the wrong string result. > > I really think you should just delete the first 3 lines and the last 3 lines > because that's exactly what you added (after formatting). Done.
lgtm with nit https://codereview.chromium.org/2872393002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/webaudio/tools/layout-test-tidy.js (right): https://codereview.chromium.org/2872393002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/tools/layout-test-tidy.js:274: if (numberOfScriptElementsWithCode !== 1) { I think I'd add a comment here that says it's perfectly valid to have multiple script tags, but we explicitly disallow that in our test code.
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/2872393002/#ps80001 (title: "Remove indentation hack 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...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1494869547345990,
"parent_rev": "0712667572b158d0613b5f5a2ec8fa6f61cada34", "commit_rev":
"1dd94e01da63c05f3c6855ac25a911f2d43868ae"}
Message was sent while issue was closed.
Description was changed from
==========
Add layout-test-tidy to LayoutTests/webaudio/tools
This tool, a Node.js CLI utility, performs a set of clean-up tasks for layout
test files in LayoutTests/webaudio. (Potentially it can be applied to any kind
of layout test files in HTML or JS format.)
The clean-up tasks includes:
- Sanitize missing or incorrect HTML elements: reordering |script| elements or
adding missing |title| element.
- Apply [html-tidy](http://www.html-tidy.org/).
- Apply [clang-format](https://clang.llvm.org/docs/ClangFormat.html).
- Perform RegExp substitution based on predefined dictionary. (e.g. |var| to
|let|, or redundant empty lines in markup.)
For every step of clean-up processing, the tool collects warning, diagnostics or
notes and print out the logs at the end of the processing.
For more info, refer README.md file included.
BUG=720378
==========
to
==========
Add layout-test-tidy to LayoutTests/webaudio/tools
This tool, a Node.js CLI utility, performs a set of clean-up tasks for layout
test files in LayoutTests/webaudio. (Potentially it can be applied to any kind
of layout test files in HTML or JS format.)
The clean-up tasks includes:
- Sanitize missing or incorrect HTML elements: reordering |script| elements or
adding missing |title| element.
- Apply [html-tidy](http://www.html-tidy.org/).
- Apply [clang-format](https://clang.llvm.org/docs/ClangFormat.html).
- Perform RegExp substitution based on predefined dictionary. (e.g. |var| to
|let|, or redundant empty lines in markup.)
For every step of clean-up processing, the tool collects warning, diagnostics or
notes and print out the logs at the end of the processing.
For more info, refer README.md file included.
BUG=720378
Review-Url: https://codereview.chromium.org/2872393002
Cr-Commit-Position: refs/heads/master@{#471857}
Committed:
https://chromium.googlesource.com/chromium/src/+/1dd94e01da63c05f3c6855ac25a9...
==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001) as https://chromium.googlesource.com/chromium/src/+/1dd94e01da63c05f3c6855ac25a9... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
