Bug 203389

Summary: Unused arguments in MESSAGE_CHECK_CONTEXTID() macros
Product: WebKit Reporter: David Kilzer (:ddkilzer) <ddkilzer>
Component: WebKit2Assignee: David Kilzer (:ddkilzer) <ddkilzer>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, bfulgham, cdumez, commit-queue, eric.carlson, ews-watchlist, glenn, jer.noble, philipj, sergio, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=195289
Attachments:
Description Flags
Patch v1
none
Patch v2
none
Patch v3 none

Description David Kilzer (:ddkilzer) 2019-10-24 16:26:04 PDT
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())
Comment 1 David Kilzer (:ddkilzer) 2019-10-24 16:26:29 PDT
<rdar://problem/55935374>
Comment 2 David Kilzer (:ddkilzer) 2019-10-24 16:33:28 PDT
Created attachment 381856 [details]
Patch v1
Comment 3 Alex Christensen 2019-10-24 17:10:48 PDT
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?
Comment 4 David Kilzer (:ddkilzer) 2019-10-25 04:13:05 PDT
(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.
Comment 5 David Kilzer (:ddkilzer) 2019-10-25 04:13:41 PDT
Created attachment 381907 [details]
Patch v2
Comment 6 Brent Fulgham 2019-10-25 08:53:24 PDT
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
Comment 7 Brent Fulgham 2019-10-25 08:58:51 PDT
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.
Comment 8 David Kilzer (:ddkilzer) 2019-10-25 09:29:32 PDT
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.
Comment 9 David Kilzer (:ddkilzer) 2019-10-25 09:29:49 PDT
Created attachment 381932 [details]
Patch v3
Comment 10 WebKit Commit Bot 2019-10-25 12:59:44 PDT
The commit-queue encountered the following flaky tests while processing attachment 381932 [details]:

The commit-queue is continuing to process your patch.
Comment 11 WebKit Commit Bot 2019-10-25 12:59:51 PDT
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: sabouhallawa@apple.com)
The commit-queue is continuing to process your patch.
Comment 12 WebKit Commit Bot 2019-10-25 17:37:38 PDT
Comment on attachment 381932 [details]
Patch v3

Clearing flags on attachment: 381932

Committed r251616: <https://trac.webkit.org/changeset/251616>
Comment 13 WebKit Commit Bot 2019-10-25 17:37:40 PDT
All reviewed patches have been landed.  Closing bug.