Bug 30529

Summary: Hide Chromium's media slider thumb if no source has been loaded.
Product: WebKit Reporter: Andrew Scherkus <scherkus>
Component: MediaAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, eric.carlson
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Round 1
levin: review-
Round 2 none

Description Andrew Scherkus 2009-10-19 12:51:12 PDT
Since the slider is non-interactive, it does not make sense to display an active-appearing slider thumb.
Comment 1 Andrew Scherkus 2009-10-19 12:53:38 PDT
Created attachment 41440 [details]
Round 1
Comment 2 Eric Seidel (no email) 2009-10-19 13:37:01 PDT
Comment on attachment 41440 [details]
Round 1

It is much better to list the layout tests rather than just saying "there are some".  Eventually all of chromiums tests will be upstream and we won't have untestable patches like this. :(

I thought "false" here meant "no theme-specific painting, webcore should paint if it want to"?
Comment 3 Eric Seidel (no email) 2009-10-19 13:37:51 PDT
Comment on attachment 41440 [details]
Round 1

It seems there was at least one other:
if (mediaElement->networkState() == HTMLMediaElement::NETWORK_EMPTY ||
 154         mediaElement->networkState() == HTMLMediaElement::NETWORK_NO_SOURCE)
check, maybe this check should be unified into a static method which can be used in multiple places in this file?
Comment 4 Andrew Scherkus 2009-10-19 13:40:20 PDT
I like the suggestions -- I'll upload something new.
Comment 5 David Levin 2009-10-19 14:02:16 PDT
Comment on attachment 41440 [details]
Round 1

r- to move out of the queue since Andrew said he is uploading something new.


> diff --git a/WebCore/rendering/RenderMediaControlsChromium.cpp b/WebCore/rendering/RenderMediaControlsChromium.cpp
> +    if (mediaElement->networkState() == HTMLMediaElement::NETWORK_EMPTY ||
> +        mediaElement->networkState() == HTMLMediaElement::NETWORK_NO_SOURCE)
fyi, the || should be on the next line.
Comment 6 Andrew Scherkus 2009-10-19 14:17:49 PDT
Created attachment 41449 [details]
Round 2

Here ya go!

P.S. I can't wait for the layout tests to be in WebKit... doing two-sided change + rebaselines are killing me!
Comment 7 Eric Seidel (no email) 2009-10-20 11:59:20 PDT
Comment on attachment 41449 [details]
Round 2

LGTM.
Comment 8 WebKit Commit Bot 2009-10-20 12:08:27 PDT
Comment on attachment 41449 [details]
Round 2

Clearing flags on attachment: 41449

Committed r49878: <http://trac.webkit.org/changeset/49878>
Comment 9 WebKit Commit Bot 2009-10-20 12:08:31 PDT
All reviewed patches have been landed.  Closing bug.