Bug 43750 - Fix regression in Mac Chromium UI for audio/video controls
Summary: Fix regression in Mac Chromium UI for audio/video controls
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-08-09 15:11 PDT by Victoria Kirst
Modified: 2010-08-11 20:57 PDT (History)
3 users (show)

See Also:


Attachments
Patch (3.29 KB, patch)
2010-08-09 16:48 PDT, Victoria Kirst
no flags Details | Formatted Diff | Diff
Patch (3.27 KB, patch)
2010-08-09 16:56 PDT, Victoria Kirst
no flags Details | Formatted Diff | Diff
Patch (4.56 KB, patch)
2010-08-11 17:36 PDT, Victoria Kirst
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Victoria Kirst 2010-08-09 15:11:51 PDT
Fix regression in Mac Chromium UI for audio/video controls
Comment 1 Victoria Kirst 2010-08-09 16:48:44 PDT
Created attachment 63951 [details]
Patch
Comment 2 Hin-Chung Lam 2010-08-09 16:56:41 PDT
The function change in this patch looks good to me.

It'll break chromium layout tests, but it's ok because those are the correct results.

Please ping jianli@ as he's the webkit sheriff on duty.
Comment 3 Victoria Kirst 2010-08-09 16:56:48 PDT
Created attachment 63953 [details]
Patch
Comment 4 David Levin 2010-08-09 21:39:26 PDT
(In reply to comment #2)
> The function change in this patch looks good to me.
> 
> It'll break chromium layout tests, but it's ok because those are the correct results.
> 
> Please ping jianli@ as he's the webkit sheriff on duty.

This isn't a great answer. WK gardeners have plenty to do without more being added.

You have two much better answers:
Method 1: Provide a new baseline as part of this patch.
Method 2:
 * Mark the tests as failing the tests in this patch.
 * When you are able (to pull the baseline from the canaries), get the new baseline 
 * Verify it.
 * Submit a new patch with the new baselines and unmark the tests as failing.
Comment 5 Hin-Chung Lam 2010-08-09 23:10:45 PDT
(In reply to comment #4)
> (In reply to comment #2)
> > The function change in this patch looks good to me.
> > 
> > It'll break chromium layout tests, but it's ok because those are the correct results.
> > 
> > Please ping jianli@ as he's the webkit sheriff on duty.
> 
> This isn't a great answer. WK gardeners have plenty to do without more being added.
> 
> You have two much better answers:
> Method 1: Provide a new baseline as part of this patch.
> Method 2:
>  * Mark the tests as failing the tests in this patch.
>  * When you are able (to pull the baseline from the canaries), get the new baseline 
>  * Verify it.
>  * Submit a new patch with the new baselines and unmark the tests as failing.

Sorry, I meant to reply to Victoria. I forgot we can use test_expactations.txt to expect failure, thanks for reminding me!
Comment 6 Eric Seidel (no email) 2010-08-10 09:55:56 PDT
Comment on attachment 63953 [details]
Patch

r- based on dave's comment?  This needs text_expectations.txt changes I guess?
Comment 7 Victoria Kirst 2010-08-11 17:36:45 PDT
Created attachment 64176 [details]
Patch
Comment 8 Victoria Kirst 2010-08-11 17:38:35 PDT
Comment on attachment 64176 [details]
Patch

Added expected failures to test_expectations.txt, so this passes all media layout tests.
Comment 9 WebKit Commit Bot 2010-08-11 20:57:08 PDT
Comment on attachment 64176 [details]
Patch

Clearing flags on attachment: 64176

Committed r65215: <http://trac.webkit.org/changeset/65215>
Comment 10 WebKit Commit Bot 2010-08-11 20:57:13 PDT
All reviewed patches have been landed.  Closing bug.