Bug 43750

Summary:

Fix regression in Mac Chromium UI for audio/video controls

Product:

WebKit

Reporter:

Victoria Kirst <vrk>

Component:

New Bugs

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

commit-queue, hclam, levin

Priority:

Version:

528+ (Nightly build)

Hardware:

Other

OS:

OS X 10.5

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none

Victoria Kirst

Reported 2010-08-09 15:11:51 PDT

Fix regression in Mac Chromium UI for audio/video controls

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
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Victoria Kirst

Comment 1 2010-08-09 16:48:44 PDT

Created attachment 63951 [details] Patch

Hin-Chung Lam

Comment 2 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.

Victoria Kirst

Comment 3 2010-08-09 16:56:48 PDT

Created attachment 63953 [details] Patch

David Levin

Comment 4 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.

Hin-Chung Lam

Comment 5 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!

Eric Seidel (no email)

Comment 6 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?

Victoria Kirst

Comment 7 2010-08-11 17:36:45 PDT

Created attachment 64176 [details] Patch

Victoria Kirst

Comment 8 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.

WebKit Commit Bot

Comment 9 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>

WebKit Commit Bot

Comment 10 2010-08-11 20:57:13 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.