Bug 213546 - Use ObjectIdentifier<>instead of WebCore::nextPlaybackTargetClientContextId() in Document.cpp
Summary: Use ObjectIdentifier<>instead of WebCore::nextPlaybackTargetClientContextId()...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Kilzer (:ddkilzer)
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-06-23 21:50 PDT by David Kilzer (:ddkilzer)
Modified: 2020-06-24 21:25 PDT (History)
16 users (show)

See Also:


Attachments
Patch v1 (65.09 KB, patch)
2020-06-23 21:55 PDT, David Kilzer (:ddkilzer)
youennf: review+
Details | Formatted Diff | Diff
Patch for landing (66.22 KB, patch)
2020-06-24 14:01 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Kilzer (:ddkilzer) 2020-06-23 21:50:05 PDT
Use ObjectIdentifier<>instead of WebCore::nextPlaybackTargetClientContextId() in Document.cpp.

<rdar://problem/61803576>
Comment 1 David Kilzer (:ddkilzer) 2020-06-23 21:55:51 PDT
Created attachment 402621 [details]
Patch v1
Comment 2 David Kilzer (:ddkilzer) 2020-06-23 21:57:13 PDT
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.
Comment 3 youenn fablet 2020-06-24 01:25:57 PDT
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:://
Comment 4 David Kilzer (:ddkilzer) 2020-06-24 14:01:50 PDT
Created attachment 402683 [details]
Patch for landing
Comment 5 David Kilzer (:ddkilzer) 2020-06-24 21:09:45 PDT
Comment on attachment 402683 [details]
Patch for landing

Marking cq+ as all bubbles are green.
Comment 6 EWS 2020-06-24 21:25:09 PDT
Committed r263498: <https://trac.webkit.org/changeset/263498>

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