[webkit-reviews] review granted: [Bug 210518] [macOS] Update ScreenTime as playback state changes : [Attachment 396586] Patch

[bugzilla-daemon at webkit.org]

Jer Noble <jer.noble at apple.com> has granted Eric Carlson
<eric.carlson at apple.com>'s request for review:
Bug 210518: [macOS] Update ScreenTime as playback state changes
https://bugs.webkit.org/show_bug.cgi?id=210518

Attachment 396586: Patch

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




--- Comment #11 from Jer Noble <jer.noble at apple.com> ---
Comment on attachment 396586
  --> https://bugs.webkit.org/attachment.cgi?id=396586
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?