Bug 86235

Created attachment 141451 [details] Patch

Created attachment 141468 [details] Patch

Comment on attachment 141451 [details] Patch Attachment 141451 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12663800 New failing tests: media/media-continues-playing-after-replace-source.html

Created attachment 141482 [details] Archive of layout-test-results from ec2-cr-linux-01 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-01 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Created attachment 141492 [details] Patch

Comment on attachment 141492 [details] Patch Attachment 141492 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12666706 New failing tests: media/media-continues-playing-after-replace-source.html

Created attachment 141547 [details] Archive of layout-test-results from ec2-cr-linux-03 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-03 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Created attachment 141575 [details] Patch

Comment on attachment 141575 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=141575&action=review Seems fine though could have been slightly simpler. I'll r+ anyway. Not a substantial concern. > Source/WebCore/platform/graphics/blackberry/MediaPlayerPrivateBlackBerry.cpp:728 > + if (!element || element->paused()) This could be reduced to: return !element || element->paused();

Comment on attachment 141575 [details] Patch Clearing flags on attachment: 141575 Committed r116858: <http://trac.webkit.org/changeset/116858>

All reviewed patches have been landed. Closing bug.

Comment on attachment 141575 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=141575&action=review > LayoutTests/media/media-continues-playing-after-replace-source.html:14 > + function swapAudio() { A function's opening brace should be on a new line. > LayoutTests/media/media-continues-playing-after-replace-source.html:25 > + logResult(false, "Caught 'error' event, audio.error.code = " + this.error.code); > + endTest(); This could be simplified to: failTest("Caught 'error' event, audio.error.code = " + this.error.code); > LayoutTests/media/media-continues-playing-after-replace-source.html:45 > + if (timeupdateEventCount-skippedCount == 1) { There should be spaces around the minus sign. > LayoutTests/media/media-continues-playing-after-replace-source.html:46 > + // First time update after source replacement should be 0. I am not sure I understand this comment. Do you mean that currentTime should be 0 when the first timeUpdate event fires? It would be good to clarify the comment when this test is updated. > LayoutTests/media/media-continues-playing-after-replace-source.html:60 > + } else if (timeupdateEventCount-skippedCount >= 2) { Ditto. > LayoutTests/media/media-continues-playing-after-replace-source.html:74 > + // The ports are not consistent in regards to whether > + // the canplaythrough and playing events are triggered > + // a second time, so stop listening for them. This was > + // done to help the cr-linux test pass, and does not > + // necessarily indicate a problem. The ports should *definitely* be consistent here. Please file a bug so we can figure out what the correct behavior is and fix it. > LayoutTests/media/media-continues-playing-after-replace-source.html:91 > + //audioElement.removeChild(audioElement.childNodes[0]); The commented out code should be removed. > LayoutTests/media/media-continues-playing-after-replace-source.html:103 > + document.write("<audio controls></audio>"); Is creating the audio element with document.write necessary for the test? > LayoutTests/media/media-continues-playing-after-replace-source.html:104 > + testAudioElement(0); It doesn't look like the index passed to testAudioElement is necessary any more (eg. swapAudio is hard coded to use 0).

Reopening to get the comments addressed.

Commit 116858 has caused the EFL and GTK+ bots to fail on the tests that has been added. I have filed bug 86283 for that.

(In reply to comment #14) > Commit 116858 has caused the EFL and GTK+ bots to fail on the tests that has been added. I have filed bug 86283 for that. The bug filed was actually bug 86310.

@Max - have you addressed any of my comments?

(In reply to comment #16) > @Max - have you addressed any of my comments? Hello, is anyone home? Can you please say whether or not you have addressed the comments I made three months ago? If you have not, when will you?

(In reply to comment #17) > (In reply to comment #16) > > @Max - have you addressed any of my comments? > > Hello, is anyone home? > > Can you please say whether or not you have addressed the comments I made three months ago? If you have not, when will you? Sorry, I have not addressed your comments yet. I have been busy with higher priority work items.

Assign to myself so it shows up in my list.