[webkit-reviews] review requested: [Bug 170355] fast/mediastream/MediaStream-page-muted.html times out and asserts : [Attachment 312300] Proposed patch.

[bugzilla-daemon at webkit.org]

youenn fablet <youennf at gmail.com> has asked  for review:
Bug 170355: fast/mediastream/MediaStream-page-muted.html times out and asserts
https://bugs.webkit.org/show_bug.cgi?id=170355

Attachment 312300: Proposed patch.

https://bugs.webkit.org/attachment.cgi?id=312300&action=review




--- Comment #23 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 312300
  --> https://bugs.webkit.org/attachment.cgi?id=312300
Proposed patch.

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

>> Source/WebCore/Modules/mediastream/MediaStream.cpp:521
>> +	return true;
> 
> If you return true here, shouldn't you override void
ActiveDOMObject::suspend(ReasonForSuspension) ? Or does nothing need
suspending?

Default implementation of returning false seems good to me.

>>> Source/WebCore/Modules/mediastream/MediaStream.cpp:526
>>> +	 return hasEventListeners(eventNames().activeEvent) ||
hasEventListeners(eventNames().inactiveEvent);
>> 
>> Can you explain why having a listener counts as having activity? This seems
odd.
>> 
>> Normally, it is about something being active / playing or having pending
events.
> 
> I suspect this would cause leaks if there is any event listener registered.

If we have listeners, the JS stream wrapper might get collected while the
stream is still active and ready to fire events.
Using setPendingActivity/unsetPendingActivity seems like overkill here.

hasPendingActivity() should probably return false if m_isActive is false.