[webkit-reviews] review requested: [Bug 74273] Constant values to set "distanceModel" are undefined : [Attachment 121724] Patch

[bugzilla-daemon at webkit.org]

Raymond Toy <rtoy at chromium.org> has asked  for review:
Bug 74273: Constant values to set "distanceModel" are undefined
https://bugs.webkit.org/show_bug.cgi?id=74273

Attachment 121724: Patch
https://bugs.webkit.org/attachment.cgi?id=121724&action=review

------- Additional Comments from Raymond Toy <rtoy at chromium.org>
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.