Summary: | REGRESSION(r83561): doesn't pause in-window playback during fullscreen playback | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Philippe Normand <pnormand> | ||||
Component: | Media | Assignee: | Nobody <webkit-unassigned> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | mrobinson | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | PC | ||||||
OS: | OS X 10.5 | ||||||
Attachments: |
|
Description
Philippe Normand
2011-04-12 03:29:54 PDT
Created attachment 89183 [details]
proposed patch
Comment on attachment 89183 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=89183&action=review Is it possible to include a test with this change? > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1714 > if (!m_fpsSink) { > gst_bin_add(GST_BIN(m_videoSinkBin), m_webkitVideoSink); > + actualVideoSink = m_webkitVideoSink; > + } > + I think it makes sense to turn if (!m_fpsSink) into an else statement here. (In reply to comment #2) > (From update of attachment 89183 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=89183&action=review > > Is it possible to include a test with this change? > > > I don't think this can be tested. The fact that we have 2 video-sinks is internal to the private media-player :/ Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1714 > > if (!m_fpsSink) { > > gst_bin_add(GST_BIN(m_videoSinkBin), m_webkitVideoSink); > > + actualVideoSink = m_webkitVideoSink; > > + } > > + > > I think it makes sense to turn if (!m_fpsSink) into an else statement here. OK. I can attach a new patch or do that tweak before landing if you r+ this. (In reply to comment #2) > (From update of attachment 89183 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=89183&action=review > > Is it possible to include a test with this change? > > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1714 > > if (!m_fpsSink) { > > gst_bin_add(GST_BIN(m_videoSinkBin), m_webkitVideoSink); > > + actualVideoSink = m_webkitVideoSink; > > + } > > + > > I think it makes sense to turn if (!m_fpsSink) into an else statement here. Hum sorry but I don't think so, finally :) m_fpsSink can be set to 0 in 2 cases just above this code, in the if (m_fpsSink) test. Turning this if (!m_fpsSink) to a else would require to duplicate the code, I think. (In reply to comment #3) > (In reply to comment #2) > > (From update of attachment 89183 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=89183&action=review > > Is it possible to include a test with this change? > I don't think this can be tested. The fact that we have 2 video-sinks is internal to the private media-player :/ Alright. Please include a line in the ChangeLog explaining the lack of test, per typical WebCore ChangeLog style. :) Committed r83741: <http://trac.webkit.org/changeset/83741> |