Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(1)

Issue 349973006: HTMLMediaElement::webMediaPlayer() should never be null if m_readyState >= HAVE_METADATA (multiple … (Closed)

Created:
3 years, 4 months ago by Srirama
Modified:
3 years, 4 months ago
CC:
blink-reviews, blink-reviews-html_chromium.org, gasubic, fs, eric.carlson_apple.com, feature-media-reviews_chromium.org, dglazkov+blink, nessy, vcarbune.chromium
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Project:
blink
Visibility:
Public.

Description

HTMLMediaElement::webMediaPlayer() should never be null if m_readyState >= HAVE_METADATA (multiple source) BUG=382721 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=177353

Patch Set 1 : #

Total comments: 14

Patch Set 2 : Fixed review comments #

Total comments: 6

Patch Set 3 : updated final patch #

Patch Set 4 : Updated as per the layout test failures #

Patch Set 5 : updated descriptions in the test case #

Patch Set 6 : Fixed review comments #

Messages

Total messages: 22 (0 generated)
Srirama
This is to fix another case where webMediaPlayer() will be null when m_readyState >= HAVE_METADATA. ...
3 years, 4 months ago (2014-06-23 15:00:49 UTC) #1
acolwell GONE FROM CHROMIUM
https://codereview.chromium.org/349973006/diff/60001/LayoutTests/media/media-source-append-multiple.html File LayoutTests/media/media-source-append-multiple.html (right): https://codereview.chromium.org/349973006/diff/60001/LayoutTests/media/media-source-append-multiple.html#newcode8 LayoutTests/media/media-source-append-multiple.html:8: function logEvent(audio, name) { Remove. You should be using ...
3 years, 4 months ago (2014-06-23 16:49:41 UTC) #2
Srirama
PTAL. Is the expected output correct with respect to the url check? it prints the ...
3 years, 4 months ago (2014-06-23 19:19:26 UTC) #3
acolwell GONE FROM CHROMIUM
lgtm % nits https://codereview.chromium.org/349973006/diff/100001/LayoutTests/media/media-source-append-multiple-expected.txt File LayoutTests/media/media-source-append-multiple-expected.txt (right): https://codereview.chromium.org/349973006/diff/100001/LayoutTests/media/media-source-append-multiple-expected.txt#newcode9 LayoutTests/media/media-source-append-multiple-expected.txt:9: EXPECTED (audio.currentSrc == 'file:///E:/chrome/winchrome/src/third_party/WebKit/LayoutTests/media/content/test.wav') OK You ...
3 years, 4 months ago (2014-06-23 20:30:50 UTC) #4
Srirama
Cleaned up as per the comments before final checkin. https://codereview.chromium.org/349973006/diff/100001/LayoutTests/media/media-source-append-multiple-expected.txt File LayoutTests/media/media-source-append-multiple-expected.txt (right): https://codereview.chromium.org/349973006/diff/100001/LayoutTests/media/media-source-append-multiple-expected.txt#newcode9 LayoutTests/media/media-source-append-multiple-expected.txt:9: ...
3 years, 4 months ago (2014-06-24 07:04:47 UTC) #5
Srirama
The CQ bit was checked by srirama.m@samsung.com
3 years, 4 months ago (2014-06-24 07:05:07 UTC) #6
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/srirama.m@samsung.com/349973006/120001
3 years, 4 months ago (2014-06-24 07:05:28 UTC) #7
commit-bot: I haz the power
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_blink_dbg on tryserver.blink ...
3 years, 4 months ago (2014-06-24 08:09:44 UTC) #8
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
3 years, 4 months ago (2014-06-24 08:32:56 UTC) #9
commit-bot: I haz the power
Try jobs failed on following builders: linux_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/13051)
3 years, 4 months ago (2014-06-24 08:32:57 UTC) #10
Srirama
Updated with test case failures. Modified the logic now. If there is no current source ...
3 years, 4 months ago (2014-06-27 15:51:38 UTC) #11
philipj_slow
Sorry I'm late to the party, but I don't think I understand this patch. Can ...
3 years, 4 months ago (2014-06-27 22:06:14 UTC) #12
Srirama
On 2014/06/27 22:06:14, philipj wrote: > Sorry I'm late to the party, but I don't ...
3 years, 4 months ago (2014-06-28 06:23:04 UTC) #13
philipj_slow
On 2014/06/28 06:23:04, Srirama wrote: > On 2014/06/27 22:06:14, philipj wrote: > > Sorry I'm ...
3 years, 4 months ago (2014-06-30 11:35:51 UTC) #14
Srirama
On 2014/06/30 11:35:51, philipj wrote: > On 2014/06/28 06:23:04, Srirama wrote: > > On 2014/06/27 ...
3 years, 4 months ago (2014-06-30 14:13:54 UTC) #15
philipj_slow
On 2014/06/30 14:13:54, Srirama wrote: > On 2014/06/30 11:35:51, philipj wrote: > > On 2014/06/28 ...
3 years, 4 months ago (2014-06-30 21:43:26 UTC) #16
Srirama
On 2014/06/30 21:43:26, philipj wrote: > On 2014/06/30 14:13:54, Srirama wrote: > > On 2014/06/30 ...
3 years, 4 months ago (2014-07-01 02:55:00 UTC) #17
Srirama
The suggested changes looks fine. All the tests are passing. PTAL.
3 years, 4 months ago (2014-07-01 10:52:39 UTC) #18
philipj_slow
LGTM!
3 years, 4 months ago (2014-07-01 21:37:59 UTC) #19
Srirama
The CQ bit was checked by srirama.m@samsung.com
3 years, 4 months ago (2014-07-02 02:38:44 UTC) #20
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/srirama.m@samsung.com/349973006/180001
3 years, 4 months ago (2014-07-02 02:39:24 UTC) #21
commit-bot: I haz the power
3 years, 4 months ago (2014-07-02 06:59:54 UTC) #22
Message was sent while issue was closed.
Change committed as 177353

Powered by Google App Engine
This is Rietveld efc10ee0f