Bug 87591

Created attachment 144234 [details] Patch

Comment on attachment 144234 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=144234&action=review The WebCore changes look fine, but please consider the comments about the test. > LayoutTests/ChangeLog:8 > + * media/video-controls-rendering-toggle-display-none.html: Is this test necessary now that we don't control visibility with "display" style? If the test is necessary, it seems that at least the test name and some of the contents (comments, text logged) should be updated. > LayoutTests/media/video-controls-rendering-toggle-display-none.html:40 > // Ensure paint with display property set to "none". Is this comment incorrect now?

Created attachment 144238 [details] Patch

(In reply to comment #2) > (From update of attachment 144234 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=144234&action=review > > The WebCore changes look fine, but please consider the comments about the test. > > > LayoutTests/ChangeLog:8 > > + * media/video-controls-rendering-toggle-display-none.html: > > Is this test necessary now that we don't control visibility with "display" style? If the test is necessary, it seems that at least the test name and some of the contents (comments, text logged) should be updated. We still use the "display" property to toggle the controls (this is tested in video-controls-toggling). I would keep this test as it does some extra interaction testing (comparing it with the pixel test video-volume-slider). However, since the test doesn't directly relate now to the use of display:none I have renamed the test file and the comments within it). > > LayoutTests/media/video-controls-rendering-toggle-display-none.html:40 > > // Ensure paint with display property set to "none". > > Is this comment incorrect now? I changed to video.controls = false, instead of directly settings display:none, because that's how most likely a developer will interact with the controls. So the comment was correct about what's actually happening, but it's not explicitly setting that property (hence, I changed it to something else). Thank you for the review.

Comment on attachment 144238 [details] Patch Rejecting attachment 144238 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: ests/platform/chromium/test_expectations.txt Hunk #1 succeeded at 3022 (offset -2 lines). patching file LayoutTests/platform/efl/test_expectations.txt Hunk #1 FAILED at 70. 1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/platform/efl/test_expectations.txt.rej patching file LayoutTests/platform/mac/test_expectations.txt Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Eric Carls..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue/ Full output: http://queues.webkit.org/results/12847549

Created attachment 144619 [details] Patch for landing

Comment on attachment 144619 [details] Patch for landing Rejecting attachment 144619 [details] from commit-queue. victor@rosedu.org does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py. - If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags. - If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed). The commit-queue restarts itself every 2 hours. After restart the commit-queue will correctly respect your committer rights.

(In reply to comment #7) > (From update of attachment 144619 [details]) > Rejecting attachment 144619 [details] from commit-queue. > > victor@rosedu.org does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py. Oups, I used land-safely. I have rebased the patch, it should be safe to cq+.

Comment on attachment 144619 [details] Patch for landing Clearing flags on attachment: 144619 Committed r118841: <http://trac.webkit.org/changeset/118841>

All reviewed patches have been landed. Closing bug.