| Summary: | Slew of minor edits to media controls on OS X | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Roger Fong <roger_fong> | ||||||||||||||
| Component: | Media | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||
| Severity: | Normal | CC: | bfulgham, buildbot, darin, dino, rniwa, roger_fong | ||||||||||||||
| Priority: | P2 | ||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||
| OS: | Unspecified | ||||||||||||||||
| Attachments: |
|
||||||||||||||||
|
Description
Roger Fong
2015-03-10 17:23:58 PDT
Need to confirm that my patch still looks okay on a non-retina machine before I upload it. Created attachment 248374 [details]
patch
(In reply to comment #1) > Need to confirm that my patch still looks okay on a non-retina machine > before I upload it. Eh, will confirm with non-retina displays tmrw. The patch wouldn't change much anyways aside from a few pixel shifts here and there (if any). Created attachment 248375 [details]
inline
Created attachment 248376 [details]
fullscreen
Comment on attachment 248374 [details] patch Attachment 248374 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/6047502721613824 New failing tests: http/tests/media/hls/video-controls-live-stream.html Created attachment 248378 [details]
Archive of layout-test-results from ews100 for mac-mavericks
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100 Port: mac-mavericks Platform: Mac OS X 10.9.5
Comment on attachment 248374 [details]
patch
http/tests/media/hls/video-controls-live-stream.html failed; I suggest testing that locally to see why it failed
Created attachment 248380 [details]
patch
(In reply to comment #8) > Comment on attachment 248374 [details] > patch > > http/tests/media/hls/video-controls-live-stream.html failed; I suggest > testing that locally to see why it failed Fixed it, showed the controls unnecessarily early, which did generate the intended visual effect but wasn't the best way to go about it. I made it so that we draw the timeline simply when the 'canplay' event is received instead since we don't show the controls until we get that event anyways. I think 'loadedData' might work too, though I think 'canplay' might be a bit more conclusive that we're ready to draw the timeline. Comment on attachment 248380 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=248380&action=review > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:578 > + this.updateProgress(event.type == 'canplay'); Are you sure this is exactly what we want? What if this is called again later for another event? It would be strange to call updateProgress(false) then. Also, I suggest using === instead of ==. Created attachment 248462 [details] patch I ended up taking a safer route and show the controls when we hide the loading status UI, which is really what I want (instead of my old patch which waited on an event that in theory coincided with the need to show the controls). Committed here: http://trac.webkit.org/changeset/181413 But let me know if you disagree. |