Summary: | [MediaStream] push media stream state to the UI process | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Eric Carlson <eric.carlson> | ||||||||
Component: | Media | Assignee: | Eric Carlson <eric.carlson> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | buildbot, commit-queue, rniwa, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Eric Carlson
2016-03-09 18:24:07 PST
Created attachment 273525 [details]
Proposed patch.
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 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
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"? (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. Created attachment 273555 [details]
Patch for landing.
Comment on attachment 273555 [details] Patch for landing. Clearing flags on attachment: 273555 Committed r197929: <http://trac.webkit.org/changeset/197929> Removed some changes accidentally committed in r197931: <http://trac.webkit.org/changeset/197931> One more change committed in https://trac.webkit.org/r197945. |