[webkit-reviews] review granted: [Bug 170355] fast/mediastream/MediaStream-page-muted.html times out and asserts : [Attachment 310427] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Per Arne Vollan <pvollan at apple.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 310427: Patch

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




--- Comment #16 from Darin Adler <darin at apple.com> ---
Comment on attachment 310427
  --> https://bugs.webkit.org/attachment.cgi?id=310427
Patch

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

> Source/WebCore/Modules/mediastream/MediaStreamTrack.cpp:343
> +    if (!ended())
> +	   return true;
> +    return ActiveDOMObject::hasPendingActivity();

Looks OK like this. Could also just write this:

    return !ended() || ActiveDOMObject::hasPendingActivity();

I find that form quite readable for a simple case like this, maybe more than
the early exit style.

> Source/WebCore/Modules/mediastream/MediaStreamTrack.h:125
> +    // ActiveDOMObject API.
> +    bool hasPendingActivity() const final;

I suggest making this override private rather than public. I also don’t think
we need the "ActiveDOMObject API" comment. For one thing, an interface like
this inside WebCore is not "API".