[webkit-reviews] review granted: [Bug 213546] Use ObjectIdentifier<>instead of WebCore::nextPlaybackTargetClientContextId() in Document.cpp : [Attachment 402621] Patch v1

[bugzilla-daemon at webkit.org]

youenn fablet <youennf at gmail.com> has granted David Kilzer (:ddkilzer)
<ddkilzer at webkit.org>'s request for review:
Bug 213546: Use ObjectIdentifier<>instead of
WebCore::nextPlaybackTargetClientContextId() in Document.cpp
https://bugs.webkit.org/show_bug.cgi?id=213546

Attachment 402621: Patch v1

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




--- Comment #3 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 402621
  --> https://bugs.webkit.org/attachment.cgi?id=402621
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::MediaSessionContextIdentif
ier contextId)

s/WebCore::// here and below

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

s/WebCore:://

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

s/WebCore:://