Bug 130608 – [GTK] Volume slider shows below the panel with videos in certain cases

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Xabier Rodríguez Calvar]

[GTK] Volume slider shows below the panel with videos in certain cases

[Xabier Rodríguez Calvar]

Created attachment 227458 [details]
Patch

[Martin Robinson]

Comment on attachment 227458 [details]
Patch

Can you explain why this was failing before?

[Xabier Rodríguez Calvar]

Created attachment 227463 [details]
Patch

[Xabier Rodríguez Calvar]

(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.

[Martin Robinson]

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.

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Xabier Rodríguez Calvar]

Created attachment 227546 [details]
Patch

[Xabier Rodríguez Calvar]

(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.

[Xabier Rodríguez Calvar]

(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.

[Jer Noble]

Comment on attachment 227546 [details]
Patch

r=me, based on the Mac changes & Martin's earlier r+ and

[Jer Noble]

(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/.

[WebKit Commit Bot]

Comment on attachment 227546 [details]
Patch

Clearing flags on attachment: 227546

Committed r166227: <http://trac.webkit.org/changeset/166227>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.