Bug 203389

<rdar://problem/55935374>

Created attachment 381856 [details] Patch v1

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?

(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.

Created attachment 381907 [details] Patch v2

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 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 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.

Created attachment 381932 [details] Patch v3

The commit-queue encountered the following flaky tests while processing attachment 381932 [details]: The commit-queue is continuing to process your patch.

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 on attachment 381932 [details] Patch v3 Clearing flags on attachment: 381932 Committed r251616: <https://trac.webkit.org/changeset/251616>

All reviewed patches have been landed. Closing bug.