The constants required to set an specific distance model on an AudioPannerNode are not defined, there is a comment "TODO: add constants" on the specification document: https://dvcs.w3.org/hg/audio/raw-file/tip/webaudio/specification.html#attributes-AudioPannerNode Looking at the objects created at runtime there are no properties defined that could be used to set a required model. At the moment you have to look at the source code to figure out what values set which model.
Created attachment 118890 [details] Patch
Comment on attachment 118890 [details] Patch This patch requires tests.
Comment on attachment 118890 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=118890&action=review > Source/WebCore/webaudio/AudioPannerNode.idl:39 > + const unsigned short EXPONENTIALDISTANCE = 2; Can we add an "_" before "DISTANCE"?
Created attachment 119037 [details] Patch
Comment on attachment 118890 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=118890&action=review >> Source/WebCore/webaudio/AudioPannerNode.idl:39 >> + const unsigned short EXPONENTIALDISTANCE = 2; > > Can we add an "_" before "DISTANCE"? Done.
Comment on attachment 119037 [details] Patch Attachment 119037 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10850436
Looks good to me. I just checked the OpenAL names and it looks like we're somewhat consistent: #define AL_DISTANCE_MODEL 0xD000 #define AL_INVERSE_DISTANCE 0xD001 #define AL_INVERSE_DISTANCE_CLAMPED 0xD002 #define AL_LINEAR_DISTANCE 0xD003 #define AL_LINEAR_DISTANCE_CLAMPED 0xD004 #define AL_EXPONENT_DISTANCE 0xD005 #define AL_EXPONENT_DISTANCE_CLAMPED 0xD006 By the way, it looks like the "clamped" versions need to be handled by a separate bool attribute called "distanceClamped" so we wouldn't define specific constants for them...
Adam is right that we need tests
(In reply to comment #8) > Adam is right that we need tests What should the tests test? That the constants are correct?
No, I think we need proper layout tests that each distance model is calculating the expected attenuation now that the distance models are being formally exposed as an API. I can explain to you offline how we can effectively do this.
(In reply to comment #10) > No, I think we need proper layout tests that each distance model is calculating the expected attenuation now that the distance models are being formally exposed as an API. I can explain to you offline how we can effectively do this. The behavior of the existing code is unchanged by this patch, so I think that's outside the scope of this particular patch. I do agree that tests need to be written, as a different bug.
Comment on attachment 119037 [details] Patch At a minimum, you need to write tests that check that these constants are exposed with the correct values.
As a general rule, WebKit does not accept patches that change web-visible behavior without tests demonstrating that change. Specifically, this patch makes some constants visible to JavaScript, so we need a test that demonstrates that.
Created attachment 121724 [details] Patch
Tests added, but the tests can't pass until bug 75767 is landed.
Comment on attachment 121724 [details] Patch Attachment 121724 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11113642 New failing tests: webaudio/distance-inverse.html webaudio/distance-linear.html webaudio/distance-exponential.html
Comment on attachment 121724 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=121724&action=review Nice tests! Just a couple of comments to simplify + nits > Source/WebCore/webaudio/AudioPannerNode.idl:39 > + const unsigned short EXPONENTIAL_DISTANCE = 2; We need a layout test specifically to test the "distance-constants.html" to test that the constant names match the expected number. Something like: // test that ... panner.LINEAR_DISTANCE == 0 // test that ... panner.INVERSE_DISTANCE == 1 // test that ... panner.EXPONENTIAL_DISTANCE == 2 > LayoutTests/webaudio/distance-exponential.html:33 > + var time; Lines 16:33 appear to be duplicated in all three .html test files. Can we move these lines to the .js file as a simplification? > LayoutTests/webaudio/distance-exponential.html:53 > + context.oncomplete = checkDistanceResult(tempPanner.EXPONENTIAL_DISTANCE, threshold, false); Indentation seems off here. > LayoutTests/webaudio/distance-exponential.html:55 > + } Lines 43:54 appear to be duplicated in all three .html test *except* for the distance model constant. This could be made a function in the .js file which takes the specific constant to test. This will simplify the .html files quite a bit. > LayoutTests/webaudio/resources/distance-model-testing.js:21 > + var gain = (1 - rolloff*(distance - panner.refDistance)/(panner.maxDistance - panner.refDistance)); WebKit style nit: spacing between * and / operators here and in similar places
Comment on attachment 121724 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=121724&action=review >> Source/WebCore/webaudio/AudioPannerNode.idl:39 >> + const unsigned short EXPONENTIAL_DISTANCE = 2; > > We need a layout test specifically to test the "distance-constants.html" to test that the constant names match the expected number. > > Something like: > > // test that ... panner.LINEAR_DISTANCE == 0 > // test that ... panner.INVERSE_DISTANCE == 1 > // test that ... panner.EXPONENTIAL_DISTANCE == 2 These values are indirectly tested because each tests prints out the model constant as a number which is compared against the expected result. Do you still want a separate test for that?
Created attachment 121867 [details] Patch
Comment on attachment 121724 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=121724&action=review >>> Source/WebCore/webaudio/AudioPannerNode.idl:39 >>> + const unsigned short EXPONENTIAL_DISTANCE = 2; >> >> We need a layout test specifically to test the "distance-constants.html" to test that the constant names match the expected number. >> >> Something like: >> >> // test that ... panner.LINEAR_DISTANCE == 0 >> // test that ... panner.INVERSE_DISTANCE == 1 >> // test that ... panner.EXPONENTIAL_DISTANCE == 2 > > These values are indirectly tested because each tests prints out the model constant as a number which is compared against the expected result. > > Do you still want a separate test for that? Latest patch tests each constant now, instead of a separate test. >> LayoutTests/webaudio/distance-exponential.html:33 >> + var time; > > Lines 16:33 appear to be duplicated in all three .html test files. Can we move these lines to the .js file as a simplification? Done. >> LayoutTests/webaudio/distance-exponential.html:53 >> + context.oncomplete = checkDistanceResult(tempPanner.EXPONENTIAL_DISTANCE, threshold, false); > > Indentation seems off here. Removed inadvertent tabs. >> LayoutTests/webaudio/distance-exponential.html:55 >> + } > > Lines 43:54 appear to be duplicated in all three .html test *except* for the distance model constant. This could be made a function in the .js file which takes the specific constant to test. > This will simplify the .html files quite a bit. Done. >> LayoutTests/webaudio/resources/distance-model-testing.js:21 >> + var gain = (1 - rolloff*(distance - panner.refDistance)/(panner.maxDistance - panner.refDistance)); > > WebKit style nit: spacing between * and / operators here and in similar places Fixed.
Comment on attachment 121867 [details] Patch Attachment 121867 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11198136 New failing tests: webaudio/distance-inverse.html webaudio/distance-linear.html webaudio/distance-exponential.html
Created attachment 122089 [details] Patch
Minor update to include audio-testing.js and to fix the expected results to include a line about having the right value for the constants.
Comment on attachment 122089 [details] Patch Attachment 122089 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11212172 New failing tests: webaudio/distance-inverse.html webaudio/distance-linear.html webaudio/distance-exponential.html
Comment on attachment 122089 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=122089&action=review > LayoutTests/webaudio/distance-exponential.html:28 > + // Threshold experimentally determined. Can you use a more descriptive variable name and comment than simply threshold. What type of value is being compared, etc... > LayoutTests/webaudio/distance-inverse.html:29 > + var threshold = 1.3e-7; Now that I look at this, let's just settle on a single threshold value for all three tests (I guess the largest of the values). And then get rid of this code in all the .html files and remove the "threshold" parameter to createTestAndRun() > LayoutTests/webaudio/resources/distance-model-testing.js:3 > +var renderLengthSeconds = 8; Why do we need to render 8 seconds? That seems like a really long time and wasteful. For the convolution test, it makes more sense, but this could be fractions of a second, test could complete more quickly, use less memory, etc. > LayoutTests/webaudio/resources/distance-model-testing.js:5 > +var pulseLengthFrames = pulseLengthSeconds * sampleRate; lines 4:5 look like they can be removed > LayoutTests/webaudio/resources/distance-model-testing.js:21 > +function createImpulseBuffer(context, sampleFrameLength) { Seems like this function would be nice to put in audio-testing.js since it seems to be useful for several different tests. > LayoutTests/webaudio/resources/distance-model-testing.js:35 > +// spec, not the code. Instead, can we say that the Web Audio spec follows the OpenAL formulas and provide a link? > LayoutTests/webaudio/resources/distance-model-testing.js:54 > +function inverseDistance(panner, x, y, z) { nit: the order these functions are defined is different than the distanceModelFunction array below -- slightly confusing > LayoutTests/webaudio/resources/distance-model-testing.js:72 > + impulse = createImpulseBuffer(context, pulseLengthFrames); Why are we creating a separate impulse for each bufferSource? Please create just one above the loop > LayoutTests/webaudio/resources/distance-model-testing.js:88 > + for (var k = 0; k < nodeCount; ++k) { Please consolidate code from loop on line 70 into this loop. There's no need to have two separate loops. Then your comments 76:85 can move down to just above 89 > LayoutTests/webaudio/resources/distance-model-testing.js:93 > + // distance. strange line wrapping - nearby line 94 is long enough to not wrap this comment > LayoutTests/webaudio/resources/distance-model-testing.js:100 > + for (var k = 0; k < nodeCount; ++k) { Once again following my comment on line 88, please consolidate this third loop into the above loop > LayoutTests/webaudio/resources/distance-model-testing.js:111 > + bufferSource[k].noteOn(time[k]); Please just consolidate startSources() function into the above createGraph() function, and you can use the same loop above. This will read more smoothly having a single loop will all of the graph connection, and the noteOn() one line after the next instead of split out into separate functions and loops > LayoutTests/webaudio/resources/distance-model-testing.js:120 > + startSources(); As per comment above, please just consolidate startSources() into the createGraph() function > LayoutTests/webaudio/resources/distance-model-testing.js:122 > + context.oncomplete = checkDistanceResult(distanceModel, expectedModel, threshold, false); Please remove final argument (false) since we can simply comment out lines for the bug below > LayoutTests/webaudio/resources/distance-model-testing.js:132 > +function checkDistanceResult(model, expectedModel, threshold, debug) { Please remove "debug" param > LayoutTests/webaudio/resources/distance-model-testing.js:142 > + testPassed("Distance model value matched expected value."); Please update text for failure case -- it's the same text as the passed case > LayoutTests/webaudio/resources/distance-model-testing.js:151 > + // (For debugging.) remove (for debugging) comment > LayoutTests/webaudio/resources/distance-model-testing.js:154 > + // expected location. (For debugging.) remove (for debugging) comment > LayoutTests/webaudio/resources/distance-model-testing.js:158 > + // so we can find where our impulses are. "impulses" -> "distance-attenuated impulses" I'd add a further comment here saying that we then check each of these values with the expected distance attenuation for that distance > LayoutTests/webaudio/resources/distance-model-testing.js:163 > + expected *= equalPowerGain(); Please add comment explaining that this compensates for the "center-panning" value using the EQUALPOWERPANNING model > LayoutTests/webaudio/resources/distance-model-testing.js:165 > + var error = Math.abs(renderedData[k] - expected)/Math.abs(expected); nit: spacing with / > LayoutTests/webaudio/resources/distance-model-testing.js:168 > + } Let's get rid of this debug code for code we actually commit > LayoutTests/webaudio/resources/distance-model-testing.js:170 > + if (k != Math.round(sampleRate * time[impulseCount])) { I'd add a comment before line 170, saying that we keep track of the exact sample-frame offsets compared with their expected locations > LayoutTests/webaudio/resources/distance-model-testing.js:179 > + testPassed("Number of nodes is correct."); Can we be a bit more specific what this test is, something like: 'Number of impulses found matches number of panner nodes" > LayoutTests/webaudio/resources/distance-model-testing.js:196 > + // FIXME: File a bug about this. We actually have a specific bug for this now, so we can provide the exact link > LayoutTests/webaudio/resources/distance-model-testing.js:198 > + console.log(timeErrors.length + " timing errors found"); Please convert from console.log() format to testPassed/testFailed format, get rid of the "debug" parameter to this function and simply comment out these specific tests Then once the bug is fixed, these lines can simply be uncommented and the expected results .txt files updated
Comment on attachment 122089 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=122089&action=review >> LayoutTests/webaudio/distance-inverse.html:29 >> + var threshold = 1.3e-7; > > Now that I look at this, let's just settle on a single threshold value for all three tests (I guess the largest of the values). And then get rid of this code in all the .html files and remove the "threshold" parameter > to createTestAndRun() They vary a bit (from 1.3e-7 to 2.3e-6). Do you want to use just one? >> LayoutTests/webaudio/resources/distance-model-testing.js:72 >> + impulse = createImpulseBuffer(context, pulseLengthFrames); > > Why are we creating a separate impulse for each bufferSource? Please create just one above the loop Is it possible to turn a single bufferSource on at multiple times?
Comment on attachment 122089 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=122089&action=review >>> LayoutTests/webaudio/resources/distance-model-testing.js:72 >>> + impulse = createImpulseBuffer(context, pulseLengthFrames); >> >> Why are we creating a separate impulse for each bufferSource? Please create just one above the loop > > Is it possible to turn a single bufferSource on at multiple times? Sorry, I misread your comment. Only one impulse is needed.
(In reply to comment #26) > (From update of attachment 122089 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=122089&action=review > > >> LayoutTests/webaudio/distance-inverse.html:29 > >> + var threshold = 1.3e-7; > > > > Now that I look at this, let's just settle on a single threshold value for all three tests (I guess the largest of the values). And then get rid of this code in all the .html files and remove the "threshold" parameter > > to createTestAndRun() > > They vary a bit (from 1.3e-7 to 2.3e-6). Do you want to use just one? Yes, I was hoping you could pick a threshold which is the least stringent of them all and go with that, and then just define that threshold inside the .js file instead of separately for each .html
(In reply to comment #28) > (In reply to comment #26) > > (From update of attachment 122089 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=122089&action=review > > > > >> LayoutTests/webaudio/distance-inverse.html:29 > > >> + var threshold = 1.3e-7; > > > > > > Now that I look at this, let's just settle on a single threshold value for all three tests (I guess the largest of the values). And then get rid of this code in all the .html files and remove the "threshold" parameter > > > to createTestAndRun() > > > > They vary a bit (from 1.3e-7 to 2.3e-6). Do you want to use just one? > > Yes, I was hoping you could pick a threshold which is the least stringent of them all and go with that, and then just define that threshold inside the .js file instead of separately for each .html I'll update this once the bug 75767 has landed due to the change in audio-testing.js.
Created attachment 122818 [details] Patch
Comment on attachment 122089 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=122089&action=review >> LayoutTests/webaudio/distance-exponential.html:28 >> + // Threshold experimentally determined. > > Can you use a more descriptive variable name and comment than simply threshold. What type of value is being compared, etc... Done. >>>>> LayoutTests/webaudio/distance-inverse.html:29 >>>>> + var threshold = 1.3e-7; >>>> >>>> Now that I look at this, let's just settle on a single threshold value for all three tests (I guess the largest of the values). And then get rid of this code in all the .html files and remove the "threshold" parameter >>>> to createTestAndRun() >>> >>> They vary a bit (from 1.3e-7 to 2.3e-6). Do you want to use just one? >> >> Yes, I was hoping you could pick a threshold which is the least stringent of them all and go with that, and then just define that threshold inside the .js file instead of separately for each .html > > I'll update this once the bug 75767 has landed due to the change in audio-testing.js. Done. >> LayoutTests/webaudio/resources/distance-model-testing.js:3 >> +var renderLengthSeconds = 8; > > Why do we need to render 8 seconds? That seems like a really long time and wasteful. For the convolution test, it makes more sense, but this could be fractions of a second, test could complete more quickly, use less memory, etc. Too much copying and pasting from the convolution test. The time is much smaller now. >> LayoutTests/webaudio/resources/distance-model-testing.js:5 >> +var pulseLengthFrames = pulseLengthSeconds * sampleRate; > > lines 4:5 look like they can be removed Line 4 deleted, but 5 is used to set the length of the impulse. >> LayoutTests/webaudio/resources/distance-model-testing.js:21 >> +function createImpulseBuffer(context, sampleFrameLength) { > > Seems like this function would be nice to put in audio-testing.js since it seems to be useful for several different tests. Already done in the equalpower panner test. >> LayoutTests/webaudio/resources/distance-model-testing.js:35 >> +// spec, not the code. > > Instead, can we say that the Web Audio spec follows the OpenAL formulas and provide a link? Done. >> LayoutTests/webaudio/resources/distance-model-testing.js:54 >> +function inverseDistance(panner, x, y, z) { > > nit: the order these functions are defined is different than the distanceModelFunction array below -- slightly confusing That was the order of the implementation of the tests. Order is changed now. >> LayoutTests/webaudio/resources/distance-model-testing.js:88 >> + for (var k = 0; k < nodeCount; ++k) { > > Please consolidate code from loop on line 70 into this loop. There's no need to have two separate loops. Then your comments 76:85 can move down to just above 89 Done. >> LayoutTests/webaudio/resources/distance-model-testing.js:93 >> + // distance. > > strange line wrapping - nearby line 94 is long enough to not wrap this comment Fixed. >> LayoutTests/webaudio/resources/distance-model-testing.js:100 >> + for (var k = 0; k < nodeCount; ++k) { > > Once again following my comment on line 88, please consolidate this third loop into the above loop Done. >> LayoutTests/webaudio/resources/distance-model-testing.js:111 >> + bufferSource[k].noteOn(time[k]); > > Please just consolidate startSources() function into the above createGraph() function, and you can use the same loop above. > > This will read more smoothly having a single loop will all of the graph connection, and the noteOn() one line after the next instead of split out into separate functions and loops Done >> LayoutTests/webaudio/resources/distance-model-testing.js:120 >> + startSources(); > > As per comment above, please just consolidate startSources() into the createGraph() function Done >> LayoutTests/webaudio/resources/distance-model-testing.js:132 >> +function checkDistanceResult(model, expectedModel, threshold, debug) { > > Please remove "debug" param Done >> LayoutTests/webaudio/resources/distance-model-testing.js:142 >> + testPassed("Distance model value matched expected value."); > > Please update text for failure case -- it's the same text as the passed case Done. >> LayoutTests/webaudio/resources/distance-model-testing.js:154 >> + // expected location. (For debugging.) > > remove (for debugging) comment Done. >> LayoutTests/webaudio/resources/distance-model-testing.js:163 >> + expected *= equalPowerGain(); > > Please add comment explaining that this compensates for the "center-panning" value using the EQUALPOWERPANNING model Done. >> LayoutTests/webaudio/resources/distance-model-testing.js:165 >> + var error = Math.abs(renderedData[k] - expected)/Math.abs(expected); > > nit: spacing with / Fixed. >> LayoutTests/webaudio/resources/distance-model-testing.js:168 >> + } > > Let's get rid of this debug code for code we actually commit Removed. >> LayoutTests/webaudio/resources/distance-model-testing.js:179 >> + testPassed("Number of nodes is correct."); > > Can we be a bit more specific what this test is, something like: > > 'Number of impulses found matches number of panner nodes" Done. >> LayoutTests/webaudio/resources/distance-model-testing.js:196 >> + // FIXME: File a bug about this. > > We actually have a specific bug for this now, so we can provide the exact link The noteOn bug has been commited so this comment and the following code has been updated appropriately. >> LayoutTests/webaudio/resources/distance-model-testing.js:198 >> + console.log(timeErrors.length + " timing errors found"); > > Please convert from console.log() format to testPassed/testFailed format, get rid of the "debug" parameter to this function and simply comment out these specific tests > > Then once the bug is fixed, these lines can simply be uncommented and the expected results .txt files updated Done
Looks good to me.
(In reply to comment #32) > Looks good to me. Unfortunately, the tests don't pass on Windows. I have no idea why, but the expected values are way off. but we know the panner is working because the panner demos sound just fine. Yuck.
Wait for bug 76659 to land before continuing with this.
Comment on attachment 121724 [details] Patch Cleared review? from obsolete attachment 121724 [details] so that this bug does not appear in http://webkit.org/pending-review. If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).
Comment on attachment 122089 [details] Patch Cleared review? from obsolete attachment 122089 [details] so that this bug does not appear in http://webkit.org/pending-review. If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).
Created attachment 124760 [details] Patch
Now that the fix for bug 76659 has landed, this test has been updated to use the new timeToSampleFrame function to get the correct expected sample frame for each impulse. Manually ran tests on OSX and Windows, where they pass.
Attachment 124760 [details] did not pass style-queue: Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9 Updating OpenSource From git://git.webkit.org/WebKit a72c8a9..11be23e master -> origin/master Partial-rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ... Currently at 106364 = a72c8a9b2ed7c202860de7e4733e96d7f60cdc14 r106366 = 11be23e25c7011a2675e260f91a1b3cbfa7052f9 Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc First, rewinding head to replay your work on top of it... Applying: Fix compilation errors on build-webkit --debug --no-workers on mac. Using index info to reconstruct a base tree... Falling back to patching base and 3-way merge... Auto-merging LayoutTests/ChangeLog CONFLICT (content): Merge conflict in LayoutTests/ChangeLog Auto-merging LayoutTests/platform/qt/Skipped CONFLICT (content): Merge conflict in LayoutTests/platform/qt/Skipped Auto-merging Source/WebCore/ChangeLog CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog Failed to merge in the changes. Patch failed at 0001 Fix compilation errors on build-webkit --debug --no-workers on mac. When you have resolved this problem run "git rebase --continue". If you would prefer to skip this patch, instead run "git rebase --skip". To restore the original branch and stop rebasing run "git rebase --abort". rebase refs/remotes/origin/master: command returned error: 1 Died at Tools/Scripts/update-webkit line 164. If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 124763 [details] Patch
Attachment 124763 [details] did not pass style-queue: Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9 Updating OpenSource First, rewinding head to replay your work on top of it... Applying: Fix compilation errors on build-webkit --debug --no-workers on mac. Using index info to reconstruct a base tree... Falling back to patching base and 3-way merge... Auto-merging LayoutTests/ChangeLog CONFLICT (content): Merge conflict in LayoutTests/ChangeLog Auto-merging LayoutTests/platform/qt/Skipped CONFLICT (content): Merge conflict in LayoutTests/platform/qt/Skipped Auto-merging Source/WebCore/ChangeLog CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog Failed to merge in the changes. Patch failed at 0001 Fix compilation errors on build-webkit --debug --no-workers on mac. When you have resolved this problem run "git rebase --continue". If you would prefer to skip this patch, instead run "git rebase --skip". To restore the original branch and stop rebasing run "git rebase --abort". rebase refs/remotes/origin/master: command returned error: 1 Died at Tools/Scripts/update-webkit line 164. If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 124774 [details] Patch
Attachment 124774 [details] did not pass style-queue: Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9 Updating OpenSource First, rewinding head to replay your work on top of it... Applying: Fix compilation errors on build-webkit --debug --no-workers on mac. Using index info to reconstruct a base tree... Falling back to patching base and 3-way merge... Auto-merging LayoutTests/ChangeLog CONFLICT (content): Merge conflict in LayoutTests/ChangeLog Auto-merging LayoutTests/platform/qt/Skipped CONFLICT (content): Merge conflict in LayoutTests/platform/qt/Skipped Auto-merging Source/WebCore/ChangeLog CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog Failed to merge in the changes. Patch failed at 0001 Fix compilation errors on build-webkit --debug --no-workers on mac. When you have resolved this problem run "git rebase --continue". If you would prefer to skip this patch, instead run "git rebase --skip". To restore the original branch and stop rebasing run "git rebase --abort". rebase refs/remotes/origin/master: command returned error: 1 Died at Tools/Scripts/update-webkit line 164. If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 124778 [details] Patch
Attachment 124778 [details] did not pass style-queue: Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9 Updating OpenSource First, rewinding head to replay your work on top of it... Applying: Fix compilation errors on build-webkit --debug --no-workers on mac. Using index info to reconstruct a base tree... Falling back to patching base and 3-way merge... Auto-merging LayoutTests/ChangeLog CONFLICT (content): Merge conflict in LayoutTests/ChangeLog Auto-merging LayoutTests/platform/qt/Skipped CONFLICT (content): Merge conflict in LayoutTests/platform/qt/Skipped Auto-merging Source/WebCore/ChangeLog CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog Failed to merge in the changes. Patch failed at 0001 Fix compilation errors on build-webkit --debug --no-workers on mac. When you have resolved this problem run "git rebase --continue". If you would prefer to skip this patch, instead run "git rebase --skip". To restore the original branch and stop rebasing run "git rebase --abort". rebase refs/remotes/origin/master: command returned error: 1 Died at Tools/Scripts/update-webkit line 164. If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 125020 [details] Patch
Comment on attachment 125020 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=125020&action=review > LayoutTests/webaudio/resources/distance-model-testing.js:201 > + // following if it enable this test. Please land 75996 first and remove the comments about these bugs (lines 199:201)
Created attachment 125038 [details] Patch
Looks good.
Comment on attachment 125038 [details] Patch rs=me
Comment on attachment 125038 [details] Patch Thanks for the review.
Comment on attachment 125038 [details] Patch Clearing flags on attachment: 125038 Committed r106580: <http://trac.webkit.org/changeset/106580>
All reviewed patches have been landed. Closing bug.