Bug 203389 – Unused arguments in MESSAGE_CHECK_CONTEXTID() macros

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[David Kilzer (:ddkilzer)]

Unused arguments in MESSAGE_CHECK_CONTEXTID() macros due to typos (`contextID` vs. `contextId`):

PlaybackSessionManagerProxy.mm
	#define MESSAGE_CHECK_CONTEXTID(contextID) MESSAGE_CHECK_BASE(m_contextMap.isValidKey(contextId), m_page->process().connection())

VideoFullscreenManagerProxy.mm
	#define MESSAGE_CHECK_CONTEXTID(contextID) MESSAGE_CHECK_BASE(m_contextMap.isValidKey(contextId), m_page->process().connection())

This one is okay (uses `id` in both places):

UserMediaCaptureManagerProxy.cpp
	#define MESSAGE_CHECK_CONTEXTID(id) MESSAGE_CHECK_BASE(m_proxies.isValidKey(id), m_process.connection())

[David Kilzer (:ddkilzer)]

<rdar://problem/55935374>

[David Kilzer (:ddkilzer)]

Created attachment 381856 [details]
Patch v1

[Alex Christensen]

Comment on attachment 381856 [details]
Patch v1

I think this is confusing because "id" is an objc keyword.  Why not just make the capitalization consistent, or remove the parameter so we need a local variable with that name?

[David Kilzer (:ddkilzer)]

(In reply to Alex Christensen from comment #3)
> Comment on attachment 381856 [details]
> Patch v1
> 
> I think this is confusing because "id" is an objc keyword.  Why not just
> make the capitalization consistent, or remove the parameter so we need a
> local variable with that name?

Oops!  Yeah, I thought `id` looked familiar.  This is what happens when you try to rush writing a patch right before leaving work.

[David Kilzer (:ddkilzer)]

Created attachment 381907 [details]
Patch v2

[Brent Fulgham]

This patch doesn't seem quite right (yet):

In file included from /Volumes/Data/worker/iOS-13-Build-EWS/build/WebKitBuild/Release-iphoneos/DerivedSources/WebKit2/unified-sources/UnifiedSource30-mm.mm:2:
/Volumes/Data/worker/iOS-13-Build-EWS/build/Source/WebKit/UIProcess/Cocoa/PlaybackSessionManagerProxy.mm:371:29: error: use of undeclared identifier 'contextID'; did you mean 'contextId'?
    MESSAGE_CHECK_CONTEXTID(contextID);
                            ^~~~~~~~~
                            contextId
/Volumes/Data/worker/iOS-13-Build-EWS/build/Source/WebKit/UIProcess/Cocoa/PlaybackSessionManagerProxy.mm:36:88: note: expanded from macro 'MESSAGE_CHECK_CONTEXTID'
#define MESSAGE_CHECK_CONTEXTID(identifier) MESSAGE_CHECK_BASE(m_contextMap.isValidKey(identifier), m_page->process().connection())
                                                                                       ^
In file included from /Volumes/Data/worker/iOS-13-Build-EWS/build/WebKitBuild/Release-iphoneos/DerivedSources/WebKit2/unified-sources/UnifiedSource30-mm.mm:2:
In file included from /Volumes/Data/worker/iOS-13-Build-EWS/build/Source/WebKit/UIProcess/Cocoa/PlaybackSessionManagerProxy.mm:31:
In file included from /Volumes/Data/worker/iOS-13-Build-EWS/build/WebKitBuild/Release-iphoneos/DerivedSources/WebKit2/PlaybackSessionManagerMessages.h:30:
/Volumes/Data/worker/iOS-13-Build-EWS/build/Source/WebKit/Platform/IPC/Connection.h:80:11: note: expanded from macro 'MESSAGE_CHECK_BASE'
    if (!(assertion)) { \
          ^
In file included from /Volumes/Data/worker/iOS-13-Build-EWS/build/WebKitBuild/Release-iphoneos/DerivedSources/WebKit2/unified-sources/UnifiedSource30-mm.mm:2:
/Volumes/Data/worker/iOS-13-Build-EWS/build/Source/WebKit/UIProcess/Cocoa/PlaybackSessionManagerProxy.mm:369:79: note: 'contextId' declared here
void PlaybackSessionManagerProxy::setUpPlaybackControlsManagerWithID(uint64_t contextId)
                                                                              ^
/Volumes/Data/worker/iOS-13-Build-EWS/build/Source/WebKit/UIProcess/Cocoa/PlaybackSessionManagerProxy.mm:397:29: error: use of undeclared identifier 'contextID'; did you mean 'contextId'?
    MESSAGE_CHECK_CONTEXTID(contextID);
                            ^~~~~~~~~
                            contextId

[Brent Fulgham]

Comment on attachment 381907 [details]
Patch v2

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

> Source/WebKit/UIProcess/Cocoa/PlaybackSessionManagerProxy.mm:36
> +#define MESSAGE_CHECK_CONTEXTID(identifier) MESSAGE_CHECK_BASE(m_contextMap.isValidKey(identifier), m_page->process().connection())

Although you correct the macro here, it actually creates compile errors because of mistakes later in the code:

For example:
void PlaybackSessionManagerProxy::rateChanged(uint64_t contextId, bool isPlaying, double rate)
{
    MESSAGE_CHECK_CONTEXTID(contextID);
    ensureModel(contextId).rateChanged(isPlaying, rate);
}

Here, the argument 'contextID' used to be macro-expanded to this:

void PlaybackSessionManagerProxy::rateChanged(uint64_t contextId, bool isPlaying, double rate)
{
    MESSAGE_CHECK_BASE(m_contextMap.isValidKey(contextId), m_page->process().connection())
    ensureModel(contextId).rateChanged(isPlaying, rate);
}

... and the 'contextId' from the macro expansion matched the 'contextId' label in the parameter.

We need to fix the call-sites, too, so that the parameter of MESSAGE_CHECK_CONTEXTID matches an actual variable name at the call site.

[David Kilzer (:ddkilzer)]

Comment on attachment 381907 [details]
Patch v2

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

>> Source/WebKit/UIProcess/Cocoa/PlaybackSessionManagerProxy.mm:36
>> +#define MESSAGE_CHECK_CONTEXTID(identifier) MESSAGE_CHECK_BASE(m_contextMap.isValidKey(identifier), m_page->process().connection())
> 
> Although you correct the macro here, it actually creates compile errors because of mistakes later in the code:
> 
> For example:
> void PlaybackSessionManagerProxy::rateChanged(uint64_t contextId, bool isPlaying, double rate)
> {
>     MESSAGE_CHECK_CONTEXTID(contextID);
>     ensureModel(contextId).rateChanged(isPlaying, rate);
> }
> 
> Here, the argument 'contextID' used to be macro-expanded to this:
> 
> void PlaybackSessionManagerProxy::rateChanged(uint64_t contextId, bool isPlaying, double rate)
> {
>     MESSAGE_CHECK_BASE(m_contextMap.isValidKey(contextId), m_page->process().connection())
>     ensureModel(contextId).rateChanged(isPlaying, rate);
> }
> 
> ... and the 'contextId' from the macro expansion matched the 'contextId' label in the parameter.
> 
> We need to fix the call-sites, too, so that the parameter of MESSAGE_CHECK_CONTEXTID matches an actual variable name at the call site.

We live on the house of cards that we write in software.

[David Kilzer (:ddkilzer)]

Created attachment 381932 [details]
Patch v3

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 381932 [details]:

The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 381932 [details]:

imported/w3c/web-platform-tests/svg/text/visualtests/text-inline-size-003-visual.svg bug 203172 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 381932 [details]
Patch v3

Clearing flags on attachment: 381932

Committed r251616: <https://trac.webkit.org/changeset/251616>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.