Bug 210518 – [macOS] Update ScreenTime as playback state changes

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Eric Carlson]

Update ScreenTime as playback state changes

[Eric Carlson]

<rdar://problem/61181092>

[Eric Carlson]

Created attachment 396481 [details]
WIP patch for the bots to check

[Eric Carlson]

Created attachment 396497 [details]
updated WIP patch for the bots

[Eric Carlson]

Created attachment 396528 [details]
updated WIP patch for the bots

[Eric Carlson]

Created attachment 396540 [details]
updated WIP patch for the bots

[Jer Noble]

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
> [phone]  Eric Carlson  <[e-mail]>

...

> Source/WebCore/ChangeLog:60
> [phone]  Eric Carlson  <[e-mail]>

Looks like you got a double changelong here.

> Source/WebKit/ChangeLog:52
> [phone]  Eric Carlson  <[e-mail]>

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?

[Eric Carlson]

Created attachment 396586 [details]
Patch

[Eric Carlson]

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
>> [phone]  Eric Carlson  <[e-mail]>
> 
> 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.

[Peng Liu]

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?

[Jon Lee]

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

[Jer Noble]

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?

[Eric Carlson]

Created attachment 396616 [details]
Patch for landing

[EWS]

Committed r260182: <https://trac.webkit.org/changeset/260182>

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

[Jer Noble]

Reopening to attach new patch.

[Jer Noble]

Created attachment 396672 [details]
Follow-up Patch

[Jer Noble]

Committed r260201: <https://trac.webkit.org/changeset/260201>