Bug 215861 - REGRESSION(r264710): Initializing the AVPlayer Obj-C class at process start up causes a regression in power-use tests
Summary: REGRESSION(r264710): Initializing the AVPlayer Obj-C class at process start u...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jer Noble
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-08-26 13:26 PDT by Jer Noble
Modified: 2020-08-27 10:01 PDT (History)
4 users (show)

See Also:


Attachments
Patch (2.07 KB, patch)
2020-08-26 13:31 PDT, Jer Noble
eric.carlson: review+
Details | Formatted Diff | Diff
Patch for landing (2.07 KB, patch)
2020-08-26 13:50 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch for landing (2.76 KB, patch)
2020-08-26 14:57 PDT, Jer Noble
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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].