Bug 88623

Created attachment 146556 [details] Full patch for layout tests

Created attachment 146557 [details] Use this for review This the difference to the previous patch. It will not pass the test - refer to the full patch for that.

Comment on attachment 146557 [details] Use this for review View in context: https://bugs.webkit.org/attachment.cgi?id=146557&action=review > Source/WebCore/html/shadow/SliderThumbElement.cpp:375 > DEFINE_STATIC_LOCAL(AtomicString, sliderThumb, ("-webkit-slider-thumb")); > - return sliderThumb; > + DEFINE_STATIC_LOCAL(AtomicString, mediaSliderThumb, ("-webkit-media-slider-thumb")); These strings shouldn't be defined in both functions (even though -webkit-slider-thumb already was), can you define each in an accessor function instead? static const AtomicString& sliderThumbShadowPseudoId() { DEFINE_STATIC_LOCAL(const AtomicString, sliderThumb, ("-webkit-slider-thumb")); return sliderThumb; } etc. > Source/WebCore/rendering/RenderMediaControlsChromium.cpp:251 > + float thumbWidth = 12.0; This magic value should be a named const.

(In reply to comment #3) > (From update of attachment 146557 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=146557&action=review > > > Source/WebCore/html/shadow/SliderThumbElement.cpp:375 > > DEFINE_STATIC_LOCAL(AtomicString, sliderThumb, ("-webkit-slider-thumb")); > > - return sliderThumb; > > + DEFINE_STATIC_LOCAL(AtomicString, mediaSliderThumb, ("-webkit-media-slider-thumb")); > > These strings shouldn't be defined in both functions (even though -webkit-slider-thumb already was), can you define each in an accessor function instead? > > static const AtomicString& sliderThumbShadowPseudoId() > { > DEFINE_STATIC_LOCAL(const AtomicString, sliderThumb, ("-webkit-slider-thumb")); > return sliderThumb; > } > > etc. Sure, no problem. > > Source/WebCore/rendering/RenderMediaControlsChromium.cpp:251 > > + float thumbWidth = 12.0; > > This magic value should be a named const. OK.

Created attachment 146700 [details] Full patch for layout tests - incl Eric's comments

Created attachment 146701 [details] Patch for review with accessor functions Patch for review with accessor functions

Created attachment 147263 [details] Patch for cq

Comment on attachment 147263 [details] Patch for cq Clearing flags on attachment: 147263 Committed r120246: <http://trac.webkit.org/changeset/120246>

patch landed

It seems that 14 media tests need rebaselining for GTK and EFL ports after this patch. The diffs look like: - RenderBlock {DIV} at (0,4) size 137x12 - RenderBlock {DIV} at (137,4) size 12x12 + RenderBlock {DIV} at (0,0) size 137x20 + RenderBlock {DIV} at (0,0) size 12x12 + RenderBlock {DIV} at (137,0) size 12x12

(In reply to comment #10) > It seems that 14 media tests need rebaselining for GTK and EFL ports after this patch. > > The diffs look like: > - RenderBlock {DIV} at (0,4) size 137x12 > - RenderBlock {DIV} at (137,4) size 12x12 > + RenderBlock {DIV} at (0,0) size 137x20 > + RenderBlock {DIV} at (0,0) size 12x12 > + RenderBlock {DIV} at (137,0) size 12x12 It's more likely cause by the patches landed in bugs 87683 and 87835. Do the controls still look right? I have more patches coming for the Chromium video controls and didn't think they influenced the GTK or EFL ports video rendering. For Chromium I have turned these tests off in TestExpectations to be rebased after my patching is finished. Is there a way for me to see the layout test results on the GTK and EFL platforms?

This patch completely broke rendering of the sliders on the Mac port. I'm going to look into it for a bit, but if I can't sort it out, I will likely roll it out.

Do you have an example? A bug number?

(In reply to comment #12) > This patch completely broke rendering of the sliders on the Mac port. I'm going to look into it for a bit, but if I can't sort it out, I will likely roll it out. I just built the MiniBrowser on the Mac and I used this page for testing: http://www.html5tutorial.info/html5-range.php . It rendered ok for me.

(In reply to comment #14) > (In reply to comment #12) > > This patch completely broke rendering of the sliders on the Mac port. I'm going to look into it for a bit, but if I can't sort it out, I will likely roll it out. > > I just built the MiniBrowser on the Mac and I used this page for testing: http://www.html5tutorial.info/html5-range.php . It rendered ok for me. That one looks fine to me, too. Check out media/video-controls.html. I'll post screenshots tomorrow if need be.

(In reply to comment #15) > (In reply to comment #14) > > (In reply to comment #12) > > > This patch completely broke rendering of the sliders on the Mac port. I'm going to look into it for a bit, but if I can't sort it out, I will likely roll it out. > > > > I just built the MiniBrowser on the Mac and I used this page for testing: http://www.html5tutorial.info/html5-range.php . It rendered ok for me. > > That one looks fine to me, too. Check out media/video-controls.html. I'll post screenshots tomorrow if need be. Hmm, works for me with MiniBrowser. Do send screenshots. If it's the CSS, I think I have a better solution now that Bug 62218 is fixed.

(In reply to comment #16) > (In reply to comment #15) > > (In reply to comment #14) > > > (In reply to comment #12) > > > > This patch completely broke rendering of the sliders on the Mac port. I'm going to look into it for a bit, but if I can't sort it out, I will likely roll it out. > > > > > > I just built the MiniBrowser on the Mac and I used this page for testing: http://www.html5tutorial.info/html5-range.php . It rendered ok for me. > > > > That one looks fine to me, too. Check out media/video-controls.html. I'll post screenshots tomorrow if need be. > > Hmm, works for me with MiniBrowser. Do send screenshots. If it's the CSS, I think I have a better solution now that Bug 62218 is fixed. Bug with screenshots in https://bugs.webkit.org/show_bug.cgi?id=89280

The FIXME added in this patch will be properly addressed in https://bugs.webkit.org/show_bug.cgi?id=89090