Bug 165923 - Arguments called in wrong order
Summary: Arguments called in wrong order
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Brent Fulgham
URL:
Keywords:
Depends on:
Blocks: 104114
  Show dependency treegraph
 
Reported: 2016-12-15 14:56 PST by Brent Fulgham
Modified: 2017-03-08 06:48 PST (History)
8 users (show)

See Also:


Attachments
Patch (8.08 KB, patch)
2016-12-15 16:57 PST, Brent Fulgham
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brent Fulgham 2016-12-15 14:56:36 PST
Static analysis has identified several places where the order of arguments to functions does not match the expected signature.
Comment 1 Brent Fulgham 2016-12-15 16:57:05 PST
I corrected a few other small bugs at the same time.
Comment 2 Brent Fulgham 2016-12-15 16:57:22 PST
Created attachment 297257 [details]
Patch
Comment 3 Simon Fraser (smfr) 2016-12-15 17:12:17 PST
Comment on attachment 297257 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=297257&action=review

> Source/WebCore/ChangeLog:3
> +        Arguments called in wrong order

That doesn't really cover all the problems.

> Source/WebCore/platform/graphics/GraphicsContext3D.cpp:332
> -    if (!computeFormatAndTypeParameters(format, type, &bytesPerComponent, &componentsPerPixel))
> +    if (!computeFormatAndTypeParameters(format, type, &componentsPerPixel, &bytesPerComponent))

It seems like this should be testable.
Comment 4 Tim Horton 2016-12-15 17:24:36 PST
Comment on attachment 297257 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=297257&action=review

> Tools/TestWebKitAPI/Tests/WebKit2/UserMedia.cpp:54
> -        WKUserMediaPermissionRequestAllow(permissionRequest, videoDeviceUID.get(), audioDeviceUID.get());
> +        WKUserMediaPermissionRequestAllow(permissionRequest, audioDeviceUID.get(), videoDeviceUID.get());

Seems bad that the test didn't catch this?
Comment 5 Brent Fulgham 2016-12-16 10:31:20 PST
Comment on attachment 297257 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=297257&action=review

>> Source/WebCore/platform/graphics/GraphicsContext3D.cpp:332
>> +    if (!computeFormatAndTypeParameters(format, type, &componentsPerPixel, &bytesPerComponent))
> 
> It seems like this should be testable.

It's actually not, because the only use of 'bytesPerComponent' and 'componentsPerPixel' is to multiply by each-other and use that result.

So this change is more about correctness than any actual behavior problem.

I kind of wonder why we bother returning both values, since they are only ever used to do this one calculation.

>> Tools/TestWebKitAPI/Tests/WebKit2/UserMedia.cpp:54
>> +        WKUserMediaPermissionRequestAllow(permissionRequest, audioDeviceUID.get(), videoDeviceUID.get());
> 
> Seems bad that the test didn't catch this?

Agreed!
Comment 6 Brent Fulgham 2016-12-16 10:33:03 PST
Committed r209924: <http://trac.webkit.org/changeset/209924>
Comment 7 David Kilzer (:ddkilzer) 2016-12-19 16:19:04 PST
Antti:  FYI, note the cache validation fix in Source/WebCore/platform/network/CacheValidation.cpp.