Take out MediaPlayback UI assertion when any WebProcess is playing audible media

• Component: New Bugs
• Status: RESOLVED FIXED

[Jer Noble]

Take out MediaPlayback UI assertion when any WebProcess is playing audible media

[Jer Noble]

Created attachment 369597 [details]
Patch

[EWS Watchlist]

Attachment 369597 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/WebProcessProxy.cpp:1215:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jer Noble]

Created attachment 369604 [details]
Patch

[EWS Watchlist]

Attachment 369604 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/WebProcessProxy.cpp:1215:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Comment on attachment 369604 [details]
Patch

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

> Source/WebKit/UIProcess/WebProcessPool.h:795
> +    HashMap<WebCore::ProcessIdentifier, std::unique_ptr<ProcessAssertion>> m_processesPlayingAudibleMedia;

What makes sure this gets cleared properly in the event of a WebContent process crash?

> Source/WebKit/UIProcess/WebProcessPool.h:796
> +    std::unique_ptr<ProcessAssertion> m_uiProcessMediaPlaybackAssertion;

ditto.

> Source/WebKit/WebProcess/WebProcess.h:109
> +class WebPageProxy;

Something is wrong here. I do not think you meant to modify this file at all. There is no reason to change WebContent process code here, only UIProcess code.

[Chris Dumez]

Comment on attachment 369604 [details]
Patch

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

> Source/WebKit/UIProcess/WebProcessPool.cpp:2578
> +    RELEASE_LOG(ProcessSuspension, "Web process pid %u is now playing audible media", (unsigned)process->processIdentifier());

Why not %d ?

> Source/WebKit/UIProcess/WebProcessPool.cpp:2587
> +    auto result = m_processesWithUploads.add(processID, nullptr);

m_processesWithUploads.add LOL bad copy paste.

> Source/WebKit/UIProcess/WebProcessPool.cpp:2601
> +    if (m_processesWithUploads.isEmpty()) {

Bad copy paste.

[Chris Dumez]

Comment on attachment 369604 [details]
Patch

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

>> Source/WebKit/UIProcess/WebProcessPool.h:795
>> +    HashMap<WebCore::ProcessIdentifier, std::unique_ptr<ProcessAssertion>> m_processesPlayingAudibleMedia;
> 
> What makes sure this gets cleared properly in the event of a WebContent process crash?

I have similar concerns when a process shutdown cleanly because it no longer has any pages. Do we properly notice that we stop playing audio if the page gets closed?

[Jer Noble]

(In reply to Chris Dumez from comment #6)
> Comment on attachment 369604 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=369604&action=review
> 
> > Source/WebKit/UIProcess/WebProcessPool.cpp:2578
> > +    RELEASE_LOG(ProcessSuspension, "Web process pid %u is now playing audible media", (unsigned)process->processIdentifier());
> 
> Why not %d ?

Probably because process identifiers are unsigned, not signed integers.

> > Source/WebKit/UIProcess/WebProcessPool.cpp:2587
> > +    auto result = m_processesWithUploads.add(processID, nullptr);
> 
> m_processesWithUploads.add LOL bad copy paste.

Oh no!

> > Source/WebKit/UIProcess/WebProcessPool.cpp:2601
> > +    if (m_processesWithUploads.isEmpty()) {
> 
> Bad copy paste.


Ditto. Will fix.

(In reply to Chris Dumez from comment #5)
> Comment on attachment 369604 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=369604&action=review
> 
> > Source/WebKit/UIProcess/WebProcessPool.h:795
> > +    HashMap<WebCore::ProcessIdentifier, std::unique_ptr<ProcessAssertion>> m_processesPlayingAudibleMedia;
> 
> What makes sure this gets cleared properly in the event of a WebContent
> process crash?

Good catch, I need to add a line to resetStateAfterProcessExited().

> > Source/WebKit/UIProcess/WebProcessPool.h:796
> > +    std::unique_ptr<ProcessAssertion> m_uiProcessMediaPlaybackAssertion;
> 
> ditto.
> 
> > Source/WebKit/WebProcess/WebProcess.h:109
> > +class WebPageProxy;
> 
> Something is wrong here. I do not think you meant to modify this file at
> all. There is no reason to change WebContent process code here, only
> UIProcess code.

You're right.

[Jer Noble]

Created attachment 369612 [details]
Patch

[EWS Watchlist]

Attachment 369612 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/WebProcessProxy.cpp:1215:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Comment on attachment 369612 [details]
Patch

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

r=me

> Source/WebKit/UIProcess/WebProcessPool.cpp:2587
> +    auto result = m_processesPlayingAudibleMedia.add(processID, nullptr);

Could you ASSERT(!m_processesPlayingAudibleMedia.contains(process.coreProcessIdentifier())) in WebProcessPool::disconnectProcess() ? Just to be safe :) Maybe even RELEASE_ASSERT() since so don't often run debug on iOS.

[Jer Noble]

Created attachment 369774 [details]
Patch for landing

[EWS Watchlist]

Attachment 369774 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/WebProcessProxy.cpp:1215:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jer Noble]

Created attachment 369776 [details]
Patch for landing

[EWS Watchlist]

Attachment 369776 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/WebProcessProxy.cpp:1215:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jer Noble]

Created attachment 369777 [details]
Patch for landing

[EWS Watchlist]

Attachment 369777 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/WebProcessProxy.cpp:1215:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 369777 [details]
Patch for landing

Clearing flags on attachment: 369777

Committed r245255: <https://trac.webkit.org/changeset/245255>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/50740551>