| Summary: | [GTK] Volume slider shows below the panel with videos in certain cases | ||
|---|---|---|---|
| Product: | WebKit | Reporter: | Xabier Rodríguez Calvar <calvaris> |
| Component: | New Bugs | Assignee: | Xabier Rodríguez Calvar <calvaris> |
| Status: | RESOLVED FIXED | ||
| Severity: | Normal | CC: | buildbot, commit-queue, eric.carlson, glenn, jer.noble, mrobinson, philipj, rniwa, sergio |
| Priority: | P2 | ||
| Version: | 528+ (Nightly build) | ||
| Hardware: | Unspecified | ||
| OS: | Unspecified | ||
| Attachments: | |||
|
Description
Xabier Rodríguez Calvar
2014-03-21 10:56:06 PDT
Created attachment 227458 [details]
Patch
Comment on attachment 227458 [details]
Patch
Can you explain why this was failing before?
Created attachment 227463 [details]
Patch
(In reply to comment #2) > (From update of attachment 227458 [details]) > Can you explain why this was failing before? Sure, I forgot to explain it in the changelog, so reuploaded the patch. Comment on attachment 227463 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=227463&action=review Looks good. A couple small nits for landing: > Source/WebCore/ChangeLog:9 > + We need to delay the moment we check if the volume slider show > + show up or down because if the video was not visible when we were Double "show" > LayoutTests/media/video-initially-hidden-volume-slider-up-expected.txt:5 > +** Test that the volume slider is rendered correctly if video is brought to foreground ** > +** Move mouse on top of the mute button ** > +** Ensure layout is done after mouse move ** > +** The volume slider should not be positioned below the panel ** I think these can just be code comments. Comment on attachment 227463 [details] Patch Attachment 227463 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/6752269651410944 New failing tests: media/video-initially-hidden-volume-slider-up.html Created attachment 227479 [details]
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-12 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Comment on attachment 227463 [details] Patch Attachment 227463 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/4879779337601024 New failing tests: media/video-initially-hidden-volume-slider-up.html Created attachment 227482 [details]
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-07 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Comment on attachment 227463 [details] Patch Attachment 227463 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5351491837624320 New failing tests: media/video-initially-hidden-volume-slider-up.html Created attachment 227486 [details]
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-05 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Created attachment 227546 [details]
Patch
(In reply to comment #5) > > Source/WebCore/ChangeLog:9 > > + We need to delay the moment we check if the volume slider show > > + show up or down because if the video was not visible when we were > > Double "show" I rephrased it. > > LayoutTests/media/video-initially-hidden-volume-slider-up-expected.txt:5 > > +** Test that the volume slider is rendered correctly if video is brought to foreground ** > > +** Move mouse on top of the mute button ** > > +** Ensure layout is done after mouse move ** > > +** The volume slider should not be positioned below the panel ** > > I think these can just be code comments. Done. I also fixed the problem with the test in Mac. (In reply to comment #13) > (In reply to comment #5) > > > Source/WebCore/ChangeLog:9 > > > + We need to delay the moment we check if the volume slider show > > > + show up or down because if the video was not visible when we were > > > > Double "show" > > I rephrased it. > > > > LayoutTests/media/video-initially-hidden-volume-slider-up-expected.txt:5 > > > +** Test that the volume slider is rendered correctly if video is brought to foreground ** > > > +** Move mouse on top of the mute button ** > > > +** Ensure layout is done after mouse move ** > > > +** The volume slider should not be positioned below the panel ** > > > > I think these can just be code comments. > > Done. > > I also fixed the problem with the test in Mac. Apart from this changes, the fix for Mac 'required' to change some common Mac code, I think a review for that would be necessary as I think the other r+ is not enough for this. Comment on attachment 227546 [details]
Patch
r=me, based on the Mac changes & Martin's earlier r+ and
(In reply to comment #15) > (From update of attachment 227546 [details]) > r=me, based on the Mac changes & Martin's earlier r+ and s/ and/. Comment on attachment 227546 [details] Patch Clearing flags on attachment: 227546 Committed r166227: <http://trac.webkit.org/changeset/166227> All reviewed patches have been landed. Closing bug. |