Bug 215861

Summary:

REGRESSION(r264710): Initializing the AVPlayer Obj-C class at process start up causes a regression in power-use tests

Product:

WebKit

Reporter:

Jer Noble <jer.noble>

Component:

New Bugs

Assignee:

Jer Noble <jer.noble>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

darin, eric.carlson, peng.liu6, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	eric.carlson: review+
Patch for landing	none
Patch for landing	none

Description Jer Noble 2020-08-26 13:26:37 PDT

REGRESSION(r264710): Initializing the AVPlayer Obj-C class at process start up causes a regression in power-use tests

Comment 1 Jer Noble 2020-08-26 13:30:00 PDT

<rdar://problem/67755363>

Comment 2 Jer Noble 2020-08-26 13:31:46 PDT

Created attachment 407330 [details]
Patch

Comment 3 Peng Liu 2020-08-26 13:43:38 PDT

Comment on attachment 407330 [details]
Patch

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

> Source/WebKit/ChangeLog:8
> +        Calling +instancesRespondToSelector: will cause the underyling Obj-C class to be initialize, which in the case of

Nit. s/initialize/initialized.

> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:1048
>      // and process global.

Nit. Maybe update the comment here as well?

Comment 4 Jer Noble 2020-08-26 13:47:50 PDT

(In reply to Peng Liu from comment #3)
> Comment on attachment 407330 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=407330&action=review
> 
> > Source/WebKit/ChangeLog:8
> > +        Calling +instancesRespondToSelector: will cause the underyling Obj-C class to be initialize, which in the case of
> 
> Nit. s/initialize/initialized.

Fixed.

> > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:1048
> >      // and process global.
> 
> Nit. Maybe update the comment here as well?

I think the comment still holds. We just assume that the property is there at runtime, rather than explicitly check.

Comment 5 Jer Noble 2020-08-26 13:50:06 PDT

Created attachment 407334 [details]
Patch for landing

Comment 6 Darin Adler 2020-08-26 14:42:13 PDT

Comment on attachment 407330 [details]
Patch

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

> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:1049
> +    return;

Given that this is now a compile time switch, I suggest wrapping the entire function body in:

#if !HAVE(AVPLAYER_VIDEORANGEOVERRIDE)

Rather than compiling a function with a return at the start of it.

Comment 7 Jer Noble 2020-08-26 14:45:49 PDT

(In reply to Darin Adler from comment #6)
> Comment on attachment 407330 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=407330&action=review
> 
> > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:1049
> > +    return;
> 
> Given that this is now a compile time switch, I suggest wrapping the entire
> function body in:
> 
> #if !HAVE(AVPLAYER_VIDEORANGEOVERRIDE)
> 
> Rather than compiling a function with a return at the start of it.

I considered that (or doing a #if/#else/#endif, but thought it might be confusing for previous OSs, especially given the existing comment. But sure, I'll re-write the comment to be more explicit that the wrapped behavior is for when this API is not available.

Comment 8 Jer Noble 2020-08-26 14:57:41 PDT

Created attachment 407341 [details]
Patch for landing

Comment 9 EWS 2020-08-27 10:01:45 PDT

Committed r266240: <https://trac.webkit.org/changeset/266240>

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