Bug 87591 – Display cues in the controls area

• Component: Media
• Status: RESOLVED FIXED

[Victor Carbune]

Cues need to be displayed in the space where controls would be shown, if these are not visible.

[Victor Carbune]

Created attachment 144234 [details]
Patch

[Eric Carlson]

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?

[Victor Carbune]

Created attachment 144238 [details]
Patch

[Victor Carbune]

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

[WebKit Review Bot]

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

[Victor Carbune]

Created attachment 144619 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 144619 [details]
Patch for landing

Rejecting attachment 144619 [details] from commit-queue.

[e-mail] 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.

[Victor Carbune]

(In reply to comment #7)
> (From update of attachment 144619 [details])
> Rejecting attachment 144619 [details] from commit-queue.
> 
> [e-mail] 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+.

[WebKit Review Bot]

Comment on attachment 144619 [details]
Patch for landing

Clearing flags on attachment: 144619

Committed r118841: <http://trac.webkit.org/changeset/118841>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.