Bug 213546 – Use ObjectIdentifier<>instead of WebCore::nextPlaybackTargetClientContextId() in Document.cpp

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[David Kilzer (:ddkilzer)]

Use ObjectIdentifier<>instead of WebCore::nextPlaybackTargetClientContextId() in Document.cpp.

<rdar://problem/61803576>

[David Kilzer (:ddkilzer)]

Created attachment 402621 [details]
Patch v1

[David Kilzer (:ddkilzer)]

Comment on attachment 402621 [details]
Patch v1

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

> Source/WebCore/ChangeLog:10
> +        Switch from uint64_t to WebCore::MediaSessionContextIdentifier
> +        for contextId values.

Would a better name be WebCore::PlaybackTargetClientContextIdentifier?

Also, there were a few uses of parameters named `clientId` that I changed to `contextId` since the majority of the names were already `contextId`.  If there is a strong desire to change them back to `clientId`, let me know.

[youenn fablet]

Comment on attachment 402621 [details]
Patch v1

LGTM, modulo naming that would be good to get feedback from Eric or Jer.

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

>> Source/WebCore/ChangeLog:10
>> +        for contextId values.
> 
> Would a better name be WebCore::PlaybackTargetClientContextIdentifier?
> 
> Also, there were a few uses of parameters named `clientId` that I changed to `contextId` since the majority of the names were already `contextId`.  If there is a strong desire to change them back to `clientId`, let me know.

PlaybackTargetClientContextIdentifier seems to align better with existing usage.
MediaSessionContextIdentifier makes me think of identifying MediaSession or PlatformMediaSessionClient.

> Source/WebCore/dom/Document.cpp:7402
> +void Document::playbackTargetAvailabilityDidChange(MediaSessionContextIdentifier contentId, bool available)

s/content/context/
Here and below

> Source/WebCore/dom/Document.h:1953
> +    typedef HashMap<MediaSessionContextIdentifier, WebCore::MediaPlaybackTargetClient*> TargetIdToClientMap;

s/typedef/using
s/TargetIdToClientMap/ContextIdToClientMap

> Source/WebKit/UIProcess/WebPageProxy.cpp:9147
> +void WebPageProxy::addPlaybackTargetPickerClient(WebCore::MediaSessionContextIdentifier contextId)

s/WebCore::// here and below

> Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.cpp:1259
> +void WebChromeClient::addPlaybackTargetPickerClient(WebCore::MediaSessionContextIdentifier contextId)

s/WebCore:://

> Source/WebKit/WebProcess/WebPage/mac/WebPageMac.mm:1011
> +void WebPage::playbackTargetSelected(WebCore::MediaSessionContextIdentifier contextId, const WebCore::MediaPlaybackTargetContext& targetContext) const

s/WebCore:://

[David Kilzer (:ddkilzer)]

Created attachment 402683 [details]
Patch for landing

[David Kilzer (:ddkilzer)]

Comment on attachment 402683 [details]
Patch for landing

Marking cq+ as all bubbles are green.

[EWS]

Committed r263498: <https://trac.webkit.org/changeset/263498>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 402683 [details].