Bug 176621 - Switch MediaPlayerPrivateAVFoundation to release logging
Summary: Switch MediaPlayerPrivateAVFoundation to release logging
Status: ASSIGNED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Eric Carlson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-09-08 11:33 PDT by Eric Carlson
Modified: 2017-09-13 13:03 PDT (History)
3 users (show)

See Also:

Attachments
Proposed patch. (57.72 KB, patch)
2017-09-10 10:51 PDT, Eric Carlson 		no flags Details | Formatted Diff | Diff
Patch (57.92 KB, patch)
2017-09-10 18:55 PDT, Eric Carlson 		no flags Details | Formatted Diff | Diff
Patch. (58.50 KB, patch)
2017-09-11 08:50 PDT, Eric Carlson 		no flags Details | Formatted Diff | Diff
Patch for landing. (59.36 KB, patch)
2017-09-11 14:33 PDT, Eric Carlson 		no flags Details | Formatted Diff | Diff
Patch for landing. (59.36 KB, patch)
2017-09-12 08:59 PDT, Eric Carlson 		no flags Details | Formatted Diff | Diff
Show Obsolete (3) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Carlson 2017-09-08 11:33:46 PDT 
Switch MediaPlayerPrivateAVFoundation to release logging
Comment 1 Radar WebKit Bug Importer 2017-09-08 11:34:33 PDT 
<rdar://problem/34335311>
Comment 2 Eric Carlson 2017-09-10 10:51:03 PDT 
Created attachment 320397 [details]
Proposed patch.
Comment 3 Eric Carlson 2017-09-10 18:55:15 PDT 
Created attachment 320412 [details]
Patch
Comment 4 Eric Carlson 2017-09-11 08:50:50 PDT 
Created attachment 320430 [details]
Patch.
Comment 5 Jer Noble 2017-09-11 13:47:42 PDT 
Comment on attachment 320430 [details]
Patch.

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

r=me, with nit.

> Source/WebCore/html/HTMLMediaElement.h:904
> +    const PAL::Logger* mediaPlayerLogger() final { return &logger(); }

Can we just make this a ref rather than a pointer?

> Source/WebCore/platform/graphics/MediaPlayer.h:306
> +    virtual const PAL::Logger* mediaPlayerLogger() { return nullptr; }

You would just make this a pure virtual. (But that's ok; the HTMLMediaElement is the only MediaPlayerClient, currently.)
Comment 6 Eric Carlson 2017-09-11 14:33:33 PDT 
Comment on attachment 320430 [details]
Patch.

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

>> Source/WebCore/html/HTMLMediaElement.h:904
>> +    const PAL::Logger* mediaPlayerLogger() final { return &logger(); }
> 
> Can we just make this a ref rather than a pointer?

Fixed.

>> Source/WebCore/platform/graphics/MediaPlayer.h:306
>> +    virtual const PAL::Logger* mediaPlayerLogger() { return nullptr; }
> 
> You would just make this a pure virtual. (But that's ok; the HTMLMediaElement is the only MediaPlayerClient, currently.)

Fixed.
Comment 7 Eric Carlson 2017-09-11 14:33:59 PDT 
Created attachment 320481 [details]
Patch for landing.
Comment 8 Eric Carlson 2017-09-12 08:59:51 PDT 
Created attachment 320550 [details]
Patch for landing.
Comment 9 WebKit Commit Bot 2017-09-12 11:34:26 PDT 
Comment on attachment 320550 [details]
Patch for landing.

Clearing flags on attachment: 320550

Committed r221929: <http://trac.webkit.org/changeset/221929>