<rdar://problem/22056719>

Created attachment 257777 [details] Patch

Comment on attachment 257777 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=257777&action=review > LayoutTests/media/session/track-media-events-in-content-sessions.html:16 > + controls.addEventListener("nexttrack", skipToNextTrack, false); > + controls.addEventListener("previoustrack", skipToPreviousTrack, false); Nit: in case you don't know you can use waitForEvent on any object to get the automatic logging it adds. For example: waitForEvent('nexttrack', skipToNextTrack, false, false, controls) > LayoutTests/media/session/track-media-events-in-content-sessions.html:30 > + function beginPlaying(event) > + { > + if (window.internals) > + testExpected('internals.mediaSessionCurrentState(session)', "idle"); > + > + testExpected('video.paused', true); Nit: If you want to add the event listener with "video.onplaying", I think it will make the test results easier to understand if you log the event name as you would if you used waitForEvent, eg: consoleWrite("EVENT(" + event.type + ")"); > LayoutTests/media/session/track-media-events-in-content-sessions.html:33 > + consoleWrite("Playing media."); > + video.play(); Nit: "Begin playing media" would be more correct. Alternatively you could use "run('video.play()')" instead. > LayoutTests/media/session/track-media-events-in-content-sessions.html:38 > + video.onplaying = null; Nit: "waitForEvent('playing', beganPlaying, false, true)" would make this unnecessary. > LayoutTests/media/session/track-media-events-in-content-sessions.html:43 > + consoleWrite("Active Media Sessions should respond to Previous/Next Track events."); Nit: This is essentially the same as the paragraph in the body so I don't think it is necessary. > LayoutTests/media/session/track-media-events-in-content-sessions.html:57 > + function skipToNextTrack(event) > + { > + consoleWrite("Sending Previous Track media event."); > + run('internals.sendMediaControlEvent("previous-track")'); > + } Nit: If you want to add the event listener with addEventListener(), I think it will make the test results easier to understand if you log the event name as you would if you used waitForEvent, eg: consoleWrite("EVENT(" + event.type + ")"); > LayoutTests/media/session/track-media-events-in-content-sessions.html:62 > + function skipToPreviousTrack(event) > + { > + endTest(); > + } Ditto.

(In reply to comment #3) > Comment on attachment 257777 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=257777&action=review > > > LayoutTests/media/session/track-media-events-in-content-sessions.html:16 > > + controls.addEventListener("nexttrack", skipToNextTrack, false); > > + controls.addEventListener("previoustrack", skipToPreviousTrack, false); > > Nit: in case you don't know you can use waitForEvent on any object to get > the automatic logging it adds. For example: > > waitForEvent('nexttrack', skipToNextTrack, false, false, controls) I switched to `waitForEvent`. > > > LayoutTests/media/session/track-media-events-in-content-sessions.html:30 > > + function beginPlaying(event) > > + { > > + if (window.internals) > > + testExpected('internals.mediaSessionCurrentState(session)', "idle"); > > + > > + testExpected('video.paused', true); > > Nit: If you want to add the event listener with "video.onplaying", I think > it will make the test results easier to understand if you log the event name > as you would if you used waitForEvent, eg: > > consoleWrite("EVENT(" + event.type + ")"); Switched to `waitForEvent`. > > > LayoutTests/media/session/track-media-events-in-content-sessions.html:33 > > + consoleWrite("Playing media."); > > + video.play(); > > Nit: "Begin playing media" would be more correct. Fixed. > Alternatively you could use "run('video.play()')" instead. > > > LayoutTests/media/session/track-media-events-in-content-sessions.html:38 > > + video.onplaying = null; > > Nit: "waitForEvent('playing', beganPlaying, false, true)" would make this > unnecessary. Switched for `waitForEvent`. > > > LayoutTests/media/session/track-media-events-in-content-sessions.html:43 > > + consoleWrite("Active Media Sessions should respond to Previous/Next Track events."); > > Nit: This is essentially the same as the paragraph in the body so I don't > think it is necessary. Removed. > > > LayoutTests/media/session/track-media-events-in-content-sessions.html:57 > > + function skipToNextTrack(event) > > + { > > + consoleWrite("Sending Previous Track media event."); > > + run('internals.sendMediaControlEvent("previous-track")'); > > + } > > Nit: If you want to add the event listener with addEventListener(), I think > it will make the test results easier to understand if you log the event name > as you would if you used waitForEvent, eg: > > consoleWrite("EVENT(" + event.type + ")"); Switched to `waitForEvent`. > > > LayoutTests/media/session/track-media-events-in-content-sessions.html:62 > > + function skipToPreviousTrack(event) > > + { > > + endTest(); > > + } > > Ditto. Switched to `waitForEvent`.

Committed r187589: <http://trac.webkit.org/changeset/187589>