Bug 212392 - Use ObjectIdentifier<> instead of uint64_t for context IDs in VideoFullscreenManagerProxy
Summary: Use ObjectIdentifier<> instead of uint64_t for context IDs in VideoFullscreen...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: Safari 13
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Kilzer (:ddkilzer)
URL:
Keywords: InRadar
Depends on:
Blocks: 213576
  Show dependency tree / graph
 
Reported: 2020-05-26 16:41 PDT by Peng Liu
Modified: 2020-06-24 15:09 PDT (History)
10 users (show)

See Also:

Attachments
Patch v1 (121.89 KB, patch)
2020-06-23 20:32 PDT, David Kilzer (:ddkilzer) 		youennf: review+
ddkilzer: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (123.71 KB, patch)
2020-06-24 13:26 PDT, David Kilzer (:ddkilzer) 		no flags Details | Formatted Diff | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Peng Liu 2020-05-26 16:41:33 PDT 
A follow-up task based on Youenn's comment: https://bugs.webkit.org/show_bug.cgi?id=212293#c3.
Comment 1 David Kilzer (:ddkilzer) 2020-06-23 20:23:15 PDT 
<rdar://problem/61799040>
Comment 2 David Kilzer (:ddkilzer) 2020-06-23 20:32:02 PDT 
Created attachment 402620 [details]
Patch v1
Comment 3 youenn fablet 2020-06-24 01:33:36 PDT 
Comment on attachment 402620 [details]
Patch v1

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

> Source/WebKit/UIProcess/Cocoa/PlaybackSessionManagerProxy.mm:371
>      MESSAGE_CHECK_CONTEXTID(contextId);

We can remove this one.

> Source/WebKit/UIProcess/Cocoa/PlaybackSessionManagerProxy.mm:504
>      MESSAGE_CHECK_CONTEXTID(contextId);

Ditto here and above.

> Source/WebKit/UIProcess/Cocoa/VideoFullscreenManagerProxy.mm:512
>      MESSAGE_CHECK_CONTEXTID(contextId);

Do we need to remove this one as well? Ditto below.

> Source/WebKit/UIProcess/Cocoa/VideoFullscreenManagerProxy.mm:513
>      MESSAGE_CHECK(videoLayerID);

Should we also use ObjectIdentifier for videoLayerID in the future?
Comment 4 David Kilzer (:ddkilzer) 2020-06-24 13:17:30 PDT 
Comment on attachment 402620 [details]
Patch v1

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

Will remove the MESSAGE_CHECK_CONTEXTID() checks as they're redundant now.

Thanks!

>> Source/WebKit/UIProcess/Cocoa/VideoFullscreenManagerProxy.mm:513
>>      MESSAGE_CHECK(videoLayerID);
> 
> Should we also use ObjectIdentifier for videoLayerID in the future?

Yes, tracked by <rdar://problem/61806253>.
Comment 5 David Kilzer (:ddkilzer) 2020-06-24 13:26:51 PDT 
Created attachment 402679 [details]
Patch for landing
Comment 6 David Kilzer (:ddkilzer) 2020-06-24 14:42:13 PDT 
Comment on attachment 402679 [details]
Patch for landing

Marking cq+ since changes are only in Source/WebKit, and thus mac-debug-wk1 can't be affected by failed tests.  (I also built macOS Debug locally.)
Comment 7 EWS 2020-06-24 15:09:38 PDT 
Committed r263476: <https://trac.webkit.org/changeset/263476>

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