Summary: | [macOS] Update ScreenTime as playback state changes | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Eric Carlson <eric.carlson> | ||||||||||||||||
Component: | Media | Assignee: | Eric Carlson <eric.carlson> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | annulen, benjamin, calvaris, cdumez, cmarcelo, esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, jer.noble, jiewen_tan, jonlee, peng.liu6, philipj, ryuan.choi, sergio, webkit-bug-importer | ||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||
Version: | Other | ||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=223731 | ||||||||||||||||||
Attachments: |
|
Description
Eric Carlson
2020-04-14 15:09:25 PDT
Created attachment 396481 [details]
WIP patch for the bots to check
Created attachment 396497 [details]
updated WIP patch for the bots
Created attachment 396528 [details]
updated WIP patch for the bots
Created attachment 396540 [details]
updated WIP patch for the bots
Comment on attachment 396540 [details] updated WIP patch for the bots View in context: https://bugs.webkit.org/attachment.cgi?id=396540&action=review > Source/WebCore/ChangeLog:1 > +2020-04-14 Eric Carlson <eric.carlson@apple.com> ... > Source/WebCore/ChangeLog:60 > +2020-04-14 Eric Carlson <eric.carlson@apple.com> Looks like you got a double changelong here. > Source/WebKit/ChangeLog:52 > +2020-04-14 Eric Carlson <eric.carlson@apple.com> Here too. > Source/WebCore/html/MediaElementSession.cpp:123 > +#if PLATFORM(MACCATALYST) || PLATFORM(MAC) Could you add a ENABLE(MEDIA_USAGE) pragma and use it in platform-independent code instead? That way other ports could adopt without having to touch all these call sites. > Source/WebKit/UIProcess/WebPageProxy.h:1739 > + void addMediaUsageManagerSession(WebCore::MediaSessionIdentifier, const String& /* bundleIdentifier */, const URL& /* pageURL */); Is there a reason these parameter names are commented out? Created attachment 396586 [details]
Patch
Comment on attachment 396540 [details] updated WIP patch for the bots View in context: https://bugs.webkit.org/attachment.cgi?id=396540&action=review >> Source/WebCore/ChangeLog:60 >> +2020-04-14 Eric Carlson <eric.carlson@apple.com> > > Looks like you got a double changelong here. Fixed. >> Source/WebCore/html/MediaElementSession.cpp:123 >> +#if PLATFORM(MACCATALYST) || PLATFORM(MAC) > > Could you add a ENABLE(MEDIA_USAGE) pragma and use it in platform-independent code instead? That way other ports could adopt without having to touch all these call sites. Good suggestion, changed. >> Source/WebKit/UIProcess/WebPageProxy.h:1739 >> + void addMediaUsageManagerSession(WebCore::MediaSessionIdentifier, const String& /* bundleIdentifier */, const URL& /* pageURL */); > > Is there a reason these parameter names are commented out? Left over from an earlier version with more parameters. Removed. Comment on attachment 396586 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=396586&action=review > Source/WebKit/UIProcess/Media/cocoa/MediaUsageManagerCocoa.h:42 > + void reset() final; Nit. Looks like these functions can be private? Comment on attachment 396586 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=396586&action=review > Source/WebCore/html/MediaElementSession.cpp:1021 > + auto* fullscreenElement = m_element.document().fullscreenManager().currentFullscreenElement(); document.fullscreenManager()... > Source/WebCore/html/MediaElementSession.cpp:1036 > + m_element.document().isMediaDocument() && (m_element.document().frame() && m_element.document().frame()->isMainFrame()), ditto Comment on attachment 396586 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=396586&action=review R=me with just a couple of nits. > Source/WebCore/testing/Internals.idl:120 > +[ > + ExportMacro=WEBCORE_TESTSUPPORT_EXPORT, > + Conditional=VIDEO, > + JSGenerateToJSObject, > +] dictionary MediaUsageState { It's unfortunate you can't just re-use MediaUsageInfo here, and not re-declare the existing struct, but I guess that the URL -> DOMString for mediaURL makes that untenable. > Source/WebKit/UIProcess/Media/cocoa/MediaUsageManagerCocoa.h:63 > + WebCore::MediaSessionIdentifier m_identifier; > + String m_bundleIdentifier; > + URL m_pageURL; > + Optional<WebCore::MediaUsageInfo> m_mediaUsageInfo; > + RetainPtr<USVideoUsage> m_usageTracker; Since structs have public members, there's no no need to prefix these with `m_`. > Source/WebKit/UIProcess/Media/cocoa/MediaUsageManagerCocoa.mm:198 > + WTFLogAlways("MediaUsageManagerCocoa::updateMediaUsage caught exception!"); Do you want to include the exception.description here? Created attachment 396616 [details]
Patch for landing
Committed r260182: <https://trac.webkit.org/changeset/260182> All reviewed patches have been landed. Closing bug and clearing flags on attachment 396616 [details]. Reopening to attach new patch. Created attachment 396672 [details]
Follow-up Patch
Committed r260201: <https://trac.webkit.org/changeset/260201> |