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!