REGRESSION(r142191): Closed caption buttons no longer appear on non-Mac ports

• Component: Media
• Status: RESOLVED FIXED

[Andrew Scherkus]

http://trac.webkit.org/changeset/142191 removed RenderTheme::paintMediaToggleClosedCaptionsButton(RenderObject*, const PaintInfo&, const IntRect&) as the Mac port switched to inlined SVG graphics, however it appears the Efl, Chromium, and Win ports still make use of that method.

[Andrew Scherkus]

Created attachment 190813 [details]
Patch

[Eric Carlson]

Comment on attachment 190813 [details]
Patch

You are missing a LayoutTests/ChangeLog entry, test results, and test expectations.

[Andrew Scherkus]

I looked through the existing set of layout tests and it doesn't look like we have a pixel test that cover the controls-with-captions case that would have caught this regression (please correct me if I'm wrong!)

I'll fix the ChangeLog bit, but what's the preferred method for landing a layout test requiring pixel results for each port?

[WebKit Review Bot]

Comment on attachment 190813 [details]
Patch

Attachment 190813 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/16849189

New failing tests:
media/controls-captions.html
media/track/track-cue-rendering-horizontal.html

[Andrew Scherkus]

Created attachment 190827 [details]
Patch

[Andrew Scherkus]

Test added for chromium-linux.

Seeing as I don't own a machine for each port I'll rebaseline as new results are generated after landing.

[WebKit Review Bot]

Attachment 190827 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/media/controls-captions.html', u'LayoutTests/platform/chromium-linux/media/controls-captions-expected.png', u'LayoutTests/platform/chromium-linux/media/controls-captions-expected.txt', u'Source/WebCore/ChangeLog', u'Source/WebCore/rendering/RenderTheme.cpp', u'Source/WebCore/rendering/RenderTheme.h']" exit_code: 1
LayoutTests/platform/chromium-linux/media/controls-captions-expected.png:0:  Have to enable auto props in the subversion config file (/home/alancutter/.subversion/config "enable-auto-props = yes"). Have to set the svn:mime-type in the subversion config file (/home/alancutter/.subversion/config "*.png = svn:mime-type=image/png").  [image/png] [5]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Andrew Scherkus]

Created attachment 190830 [details]
Patch

[WebKit Review Bot]

Attachment 190830 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/media/controls-captions.html', u'LayoutTests/platform/chromium-linux/media/controls-captions-expected.png', u'LayoutTests/platform/chromium-linux/media/controls-captions-expected.txt', u'Source/WebCore/ChangeLog', u'Source/WebCore/rendering/RenderTheme.cpp', u'Source/WebCore/rendering/RenderTheme.h']" exit_code: 1
LayoutTests/platform/chromium-linux/media/controls-captions-expected.png:0:  Have to enable auto props in the subversion config file (/home/alancutter/.subversion/config "enable-auto-props = yes"). Have to set the svn:mime-type in the subversion config file (/home/alancutter/.subversion/config "*.png = svn:mime-type=image/png").  [image/png] [5]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 190830 [details]
Patch

Attachment 190830 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/16863093

New failing tests:
media/track/track-cue-rendering-horizontal.html
media/track/track-cue-rendering-vertical.html

[WebKit Review Bot]

Comment on attachment 190830 [details]
Patch

Attachment 190830 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/16810303

New failing tests:
media/track/track-cue-rendering-horizontal.html
media/track/track-cue-rendering-vertical.html

[Build Bot]

Comment on attachment 190830 [details]
Patch

Attachment 190830 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/16815219

New failing tests:
media/controls-captions.html

[Andrew Scherkus]

Created attachment 191365 [details]
Patch

[Andrew Scherkus]

Silly me. Turns out there are existing tests that cover this situation. The sad bit is they currently have the incorrect baseline :(

Please take another look.

[Eric Carlson]

It looks like you need to rebase your patch.

[Andrew Scherkus]

Crud. I just rebased it -- I guess not enough :~(

Thanks for the heads up.

[WebKit Review Bot]

Comment on attachment 191365 [details]
Patch

Rejecting attachment 191365 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', '--bot-id=gce-cq-04', 'apply-attachment', '--no-update', '--non-interactive', 191365, '--port=chromium-xvfb']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue

Last 500 characters of output:
/rendering/RenderTheme.cpp
patching file Source/WebCore/rendering/RenderTheme.h
patching file LayoutTests/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file LayoutTests/platform/chromium/TestExpectations
Hunk #1 FAILED at 4437.
1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/platform/chromium/TestExpectations.rej

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', '--force', '--reviewer', 'Eric Carlson']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue

Full output: http://webkit-commit-queue.appspot.com/results/16988064

[Andrew Scherkus]

Committed r144789: <http://trac.webkit.org/changeset/144789>