Bug 212392

Summary:

Use ObjectIdentifier<> instead of uint64_t for context IDs in VideoFullscreenManagerProxy

Product:

WebKit

Reporter:

Peng Liu <peng.liu6>

Component:

Media

Assignee:

David Kilzer (:ddkilzer) <ddkilzer>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ddkilzer, eric.carlson, ews-watchlist, glenn, jer.noble, philipj, sergio, useafterfree, webkit-bug-importer, youennf

Priority:

Keywords:

InRadar

Version:

Safari 13

Hardware:

Unspecified

OS:

Unspecified

See Also:

https://bugs.webkit.org/show_bug.cgi?id=212293

Bug Depends on:

Bug Blocks:

213576

Attachments:

Description	Flags
Patch v1	youennf: review+, ddkilzer: commit-queue-
Patch for landing	none

Peng Liu

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

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 attachment proposed patch, testcase, etc.

David Kilzer (:ddkilzer)

Comment 1 2020-06-23 20:23:15 PDT

<rdar://problem/61799040>

David Kilzer (:ddkilzer)

Comment 2 2020-06-23 20:32:02 PDT

Created attachment 402620 [details] Patch v1

youenn fablet

Comment 3 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?

David Kilzer (:ddkilzer)

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

David Kilzer (:ddkilzer)

Comment 5 2020-06-24 13:26:51 PDT

Created attachment 402679 [details] Patch for landing

David Kilzer (:ddkilzer)

Comment 6 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.)

EWS

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

Note You need to log in before you can comment on or make changes to this bug.