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

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has denied youenn fablet <youennf at gmail.com>'s
request 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 #24 from Chris Dumez <cdumez at apple.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.

Please stop blindly returning false in those methods :( You will make PageCache
a lot worse. At the very least, we should be able to suspend if we're not
active (e.g. !hasPendingActivity() or !m_isActive).