Bug 237991

Created attachment 454919 [details] Patch

Looks like some of this was already fixed in bug 238010.

Comment on attachment 454919 [details] Patch I've been told that though this built for me, I have used ASSERT_UNUSED incorrectly.

Comment on attachment 454919 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=454919&action=review > Source/WebCore/platform/audio/mac/AudioOutputUnitAdaptorMac.cpp:58 > ASSERT(!result); > + ASSERT_UNUSED(result, result); This is not correct. Should be just one line, ASSERT_UNUSED(result, !result); I'm surprised that we haven't seen this until now.

Comment on attachment 454919 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=454919&action=review > Source/WebCore/platform/graphics/avfoundation/objc/MediaSampleAVFObjC.mm:380 > + ASSERT_UNUSED(status, status); Per Alexey's comment above, shouldn't this be: ASSERT_UNUSED(status, status == noErr); > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:3543 > + ASSERT_UNUSED(valuesOK, valuesOK); Should replace the above line, right? > Source/WebKit/Shared/cf/ArgumentCodersCF.cpp:654 > + ASSERT_UNUSED(numConvertedBytes, numConvertedBytes); Should replace the above line, right? > Source/WebKit/Shared/mac/AuxiliaryProcessMac.mm:441 > + ASSERT_UNUSED(copied, copied); Should replace the above line, right? > Tools/TestWebKitAPI/Tests/WebKitCocoa/WKWebViewCloseAllMediaPresentations.mm:51 > + ASSERT_UNUSED(presentationModeChanged, presentationModeChanged); Feels wrong to have this assertion here, right? Because this variable is used on the line right below this.

Created attachment 454989 [details] Patch

(In reply to Jonathan Bedard from comment #5) > Comment on attachment 454919 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=454919&action=review > > > Source/WebCore/platform/graphics/avfoundation/objc/MediaSampleAVFObjC.mm:380 > > + ASSERT_UNUSED(status, status); > > Per Alexey's comment above, shouldn't this be: > ASSERT_UNUSED(status, status == noErr); > > > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:3543 > > + ASSERT_UNUSED(valuesOK, valuesOK); > > Should replace the above line, right? > > > Source/WebKit/Shared/cf/ArgumentCodersCF.cpp:654 > > + ASSERT_UNUSED(numConvertedBytes, numConvertedBytes); > > Should replace the above line, right? > > > Source/WebKit/Shared/mac/AuxiliaryProcessMac.mm:441 > > + ASSERT_UNUSED(copied, copied); > > Should replace the above line, right? > > > Tools/TestWebKitAPI/Tests/WebKitCocoa/WKWebViewCloseAllMediaPresentations.mm:51 > > + ASSERT_UNUSED(presentationModeChanged, presentationModeChanged); > > Feels wrong to have this assertion here, right? Because this variable is > used on the line right below this. I encountered a build failure without this, let me re-check with my corrected patch.

Oh, most of these were fixed by https://bugs.webkit.org/show_bug.cgi?id=238010, duping. *** This bug has been marked as a duplicate of bug 238010 ***

Comment on attachment 454989 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=454989&action=review > Tools/TestWebKitAPI/Tests/WebKitCocoa/WKWebViewCloseAllMediaPresentations.mm:51 > + ASSERT_UNUSED(presentationModeChanged, presentationModeChanged); This should be UNUSED_VARIABLE. ASSERT_UNUSED would fail anyway, as we just set it to false above. But looking further into the code, the real fix is to change an ASSERT below to ASSERT_UNUSED. And in fact, this was also done in r291403!