Bug 155281 – [MediaStream] push media stream state to the UI process

• Component: Media
• Status: RESOLVED FIXED

[Eric Carlson]

Make it possible for the UI process to know when a page has active media streams, and to mute all streams in a page.

[Eric Carlson]

Created attachment 273525 [details]
Proposed patch.

[Eric Carlson]

<rdar://problem/24309604>

[Build Bot]

Comment on attachment 273525 [details]
Proposed patch.

Attachment 273525 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/951503

New failing tests:
fast/mediastream/MediaDevices-enumerateDevices.html

[Build Bot]

Created attachment 273530 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Darin Adler]

Comment on attachment 273525 [details]
Proposed patch.

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

> Source/WebCore/Modules/mediastream/MediaStream.cpp:86
> +    downcast<Document>(&context)->addAudioProducer(this);

Should not convert a reference to a pointer:

    downcast<Document>(context).addAudioProducer(this);

Also seems lame that addAudioProducer takes a pointer, but I guess not new.

> Source/WebCore/Modules/mediastream/MediaStream.cpp:104
> +    downcast<Document>(&context)->addAudioProducer(this);

Ditto.

> Source/WebCore/Modules/mediastream/MediaStream.cpp:109
> +    m_active = false;

This line of code needs a comment. It’s peculiar to set the value of a data member while destroying an object.

> Source/WebCore/Modules/mediastream/MediaStream.cpp:114
> +    if (Document* document = downcast<Document>(scriptExecutionContext()))

If we can always downcast scriptExecutionContext() to Document* in this class, maybe we should add a helper function named document() that does that.

> Source/WebCore/Modules/mediastream/MediaStream.cpp:271
> +    MediaStateFlags state = IsNotPlaying;

This seems strange. Even when playing the state is equal to IsNotPlaying. Maybe that’s just a fancy name for zero?

> Source/WebCore/Modules/mediastream/MediaStream.cpp:277
> +        state |= MediaProducer::HasActiveMediaCaptureDevice;

Peculiar that we need MediaProducer:: here but not in the rest of this function.

> Source/WebCore/Modules/mediastream/MediaStream.h:138
> +    bool m_active { false };

Why rename m_isActive to m_active? Does not seem like an improvement. Generally we like to name boolean data members and even functions so the phrase “stream <xxx>” makes grammatical sense. Hence "is active", "is muted", "is externally muted".

> Source/WebCore/platform/mediastream/MediaStreamPrivate.h:96
> +    bool muted() const;

isMuted would be better (same reason as above)

> Source/WebKit2/UIProcess/WebPageProxy.cpp:6013
> +    playingMediaMask |= MediaProducer::HasActiveMediaCaptureDevice;

This should be:

    MediaProducer::MediaStateFlags playingAudioMask = playingMediaMask | MediaProducer::HasActiveMediaCaptureDevice;

But that makes no sense to me! How is that the correct mask to use to check "audio did change"?

[Eric Carlson]

(In reply to comment #5)
> Comment on attachment 273525 [details]
> Proposed patch.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=273525&action=review
> 
> > Source/WebCore/Modules/mediastream/MediaStream.cpp:86
> > +    downcast<Document>(&context)->addAudioProducer(this);
> 
> Should not convert a reference to a pointer:
> 
>     downcast<Document>(context).addAudioProducer(this);
> 
  Added a document() method as you suggested.

> Also seems lame that addAudioProducer takes a pointer, but I guess not new.
> 
> > Source/WebCore/Modules/mediastream/MediaStream.cpp:104
> > +    downcast<Document>(&context)->addAudioProducer(this);
> 
> Ditto.
> 
  Ditto.

> > Source/WebCore/Modules/mediastream/MediaStream.cpp:109
> > +    m_active = false;
> 
> This line of code needs a comment. It’s peculiar to set the value of a data
> member while destroying an object.
> 
  Commented.

> > Source/WebCore/Modules/mediastream/MediaStream.cpp:114
> > +    if (Document* document = downcast<Document>(scriptExecutionContext()))
> 
> If we can always downcast scriptExecutionContext() to Document* in this
> class, maybe we should add a helper function named document() that does that.
> 
  Added.

> > Source/WebCore/Modules/mediastream/MediaStream.cpp:271
> > +    MediaStateFlags state = IsNotPlaying;
> 
> This seems strange. Even when playing the state is equal to IsNotPlaying.
> Maybe that’s just a fancy name for zero?
> 
  Indeed, it is the enum's name for zero. The MediaProducer class needs a cleanup, I will do this in a follow up patch.

> > Source/WebCore/Modules/mediastream/MediaStream.cpp:277
> > +        state |= MediaProducer::HasActiveMediaCaptureDevice;
> 
> Peculiar that we need MediaProducer:: here but not in the rest of this
> function.
> 
  Fixed.

> > Source/WebCore/Modules/mediastream/MediaStream.h:138
> > +    bool m_active { false };
> 
> Why rename m_isActive to m_active? Does not seem like an improvement.
> Generally we like to name boolean data members and even functions so the
> phrase “stream <xxx>” makes grammatical sense. Hence "is active", "is
> muted", "is externally muted".
> 
  Fixed.

> > Source/WebCore/platform/mediastream/MediaStreamPrivate.h:96
> > +    bool muted() const;
> 
> isMuted would be better (same reason as above)
> 
  Fixed.

> > Source/WebKit2/UIProcess/WebPageProxy.cpp:6013
> > +    playingMediaMask |= MediaProducer::HasActiveMediaCaptureDevice;
> 
> This should be:
> 
>     MediaProducer::MediaStateFlags playingAudioMask = playingMediaMask |
> MediaProducer::HasActiveMediaCaptureDevice;
> 
> But that makes no sense to me! How is that the correct mask to use to check
> "audio did change"?
  The purpose of isPlayingAudioDidChange has morphed, I will clean this up in a follow up.

[Eric Carlson]

Created attachment 273555 [details]
Patch for landing.

[WebKit Commit Bot]

Comment on attachment 273555 [details]
Patch for landing.

Clearing flags on attachment: 273555

Committed r197929: <http://trac.webkit.org/changeset/197929>

[Eric Carlson]

Removed some changes accidentally committed in r197931: <http://trac.webkit.org/changeset/197931>

[Eric Carlson]

One more change committed in https://trac.webkit.org/r197945.