Bug 87591 - Display cues in the controls area
Summary: Display cues in the controls area
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Victor Carbune
URL:
Keywords:
Depends on:
Blocks: 82124
  Show dependency treegraph
 
Reported: 2012-05-27 08:52 PDT by Victor Carbune
Modified: 2012-05-29 15:28 PDT (History)
6 users (show)

See Also:


Attachments
Patch (7.96 KB, patch)
2012-05-27 09:05 PDT, Victor Carbune
no flags Details | Formatted Diff | Diff
Patch (15.32 KB, patch)
2012-05-27 11:54 PDT, Victor Carbune
no flags Details | Formatted Diff | Diff
Patch for landing (15.28 KB, patch)
2012-05-29 14:26 PDT, Victor Carbune
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Victor Carbune 2012-05-27 08:52:25 PDT
Cues need to be displayed in the space where controls would be shown, if these are not visible.
Comment 1 Victor Carbune 2012-05-27 09:05:01 PDT
Created attachment 144234 [details]
Patch
Comment 2 Eric Carlson 2012-05-27 09:47:30 PDT
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?
Comment 3 Victor Carbune 2012-05-27 11:54:21 PDT
Created attachment 144238 [details]
Patch
Comment 4 Victor Carbune 2012-05-27 12:01:06 PDT
(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 5 WebKit Review Bot 2012-05-29 10:15:56 PDT
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
Comment 6 Victor Carbune 2012-05-29 14:26:11 PDT
Created attachment 144619 [details]
Patch for landing
Comment 7 WebKit Review Bot 2012-05-29 14:26:49 PDT
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.
Comment 8 Victor Carbune 2012-05-29 14:29:41 PDT
(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 9 WebKit Review Bot 2012-05-29 15:28:32 PDT
Comment on attachment 144619 [details]
Patch for landing

Clearing flags on attachment: 144619

Committed r118841: <http://trac.webkit.org/changeset/118841>
Comment 10 WebKit Review Bot 2012-05-29 15:28:36 PDT
All reviewed patches have been landed.  Closing bug.