Bug 66918 - Chromium Win: Setting square-button appearance reaches NOTREACHED
Summary: Chromium Win: Setting square-button appearance reaches NOTREACHED
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows 7
: P2 Normal
Assignee: Keishi Hattori
URL:
Keywords:
Depends on:
Blocks: 66750
  Show dependency treegraph
 
Reported: 2011-08-24 19:03 PDT by Keishi Hattori
Modified: 2011-08-28 20:49 PDT (History)
2 users (show)

See Also:


Attachments
patch (2.33 KB, patch)
2011-08-24 19:08 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff
added test (25.23 KB, patch)
2011-08-25 22:20 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff
moved expected files and changed test_expectations (12.77 KB, patch)
2011-08-25 23:44 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff
added chromium-linux expected file (13.49 KB, patch)
2011-08-26 00:58 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff
fixed conflict (13.59 KB, patch)
2011-08-26 01:03 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff
removed txt, added png (13.86 KB, patch)
2011-08-26 01:55 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff
added test to webkit win test_expected.txt (14.64 KB, patch)
2011-08-26 02:00 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff
revert changes in the last patch (13.86 KB, patch)
2011-08-26 02:24 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff
rebased (13.79 KB, patch)
2011-08-28 18:23 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Keishi Hattori 2011-08-24 19:03:52 PDT
RenderThemeChromiumWin.cpp lacks cases for SquareButtonPart.
Comment 1 Keishi Hattori 2011-08-24 19:08:59 PDT
Created attachment 105119 [details]
patch
Comment 2 Kent Tamura 2011-08-24 19:23:03 PDT
Comment on attachment 105119 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=105119&action=review

> Source/WebCore/ChangeLog:8
> +        No new tests.

We should have a test.
Comment 3 Keishi Hattori 2011-08-25 22:20:43 PDT
Created attachment 105311 [details]
added test
Comment 4 Kent Tamura 2011-08-25 22:39:02 PDT
Comment on attachment 105311 [details]
added test

View in context: https://bugs.webkit.org/attachment.cgi?id=105311&action=review

> LayoutTests/ChangeLog:8
> +        * fast/css/square-button-appearance.html: Added. Test rendering of square-button appearance.

You should add this test to test_expectations.txt.  Otherwise, layout tests on ports other than chromium-win and mac-snowleopard fail.

> LayoutTests/ChangeLog:12
> +        * platform/mac-snowleopard/fast/css/square-button-appearance-expected.png: Added.
> +        * platform/mac-snowleopard/fast/css/square-button-appearance-expected.txt: Added.

Do you think these results are not compatible with Lion?  If you're not sure about it, I recommend you don't add results for Mac.

> LayoutTests/fast/css/square-button-appearance.html:18
> +<h1>Test if square-button appearance is rendered properly.</h1>

Please change this line to a comment.  Texts prevent test result sharing.
<!-- Test if square-button appearance is rendered properly. -->
Comment 5 Keishi Hattori 2011-08-25 23:44:44 PDT
Created attachment 105320 [details]
moved expected files and changed test_expectations
Comment 6 Keishi Hattori 2011-08-25 23:46:26 PDT
I'm still not sure how this system works. Do I need to add expected results for chromium-mac and chromium-gtk?

(In reply to comment #4)
> (From update of attachment 105311 [details](apply) [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=105311&action=review
> 
> > LayoutTests/ChangeLog:8
> > +        * fast/css/square-button-appearance.html: Added. Test rendering of square-button appearance.
> 
> You should add this test to test_expectations.txt.  Otherwise, layout tests on ports other than chromium-win and mac-snowleopard fail.

I added to platform/qt/test_expectations.txt and platform/gtk/test_expectations.txt.

> > LayoutTests/ChangeLog:12
> > +        * platform/mac-snowleopard/fast/css/square-button-appearance-expected.png: Added.
> > +        * platform/mac-snowleopard/fast/css/square-button-appearance-expected.txt: Added.
> 
> Do you think these results are not compatible with Lion?  If you're not sure about it, I recommend you don't add results for Mac.

The script, run-webkit-tests, placed it there. It should be the same so I moved it to platform/mac/

> > LayoutTests/fast/css/square-button-appearance.html:18
> > +<h1>Test if square-button appearance is rendered properly.</h1>
> 
> Please change this line to a comment.  Texts prevent test result sharing.
> <!-- Test if square-button appearance is rendered properly. -->

Done.
Comment 7 Kent Tamura 2011-08-26 00:17:22 PDT
Comment on attachment 105320 [details]
moved expected files and changed test_expectations

r- because the patch conflicts and chromium-linux will fail.
Comment 8 Keishi Hattori 2011-08-26 00:58:36 PDT
Created attachment 105327 [details]
added chromium-linux expected file
Comment 9 Keishi Hattori 2011-08-26 00:59:12 PDT
Do I need to add any more?
Comment 10 Keishi Hattori 2011-08-26 00:59:49 PDT
Comment on attachment 105327 [details]
added chromium-linux expected file

forgot to fix conflict
Comment 11 Kent Tamura 2011-08-26 01:01:43 PDT
Comment on attachment 105327 [details]
added chromium-linux expected file

View in context: https://bugs.webkit.org/attachment.cgi?id=105327&action=review

> LayoutTests/ChangeLog:10
> +        * platform/chromium-linux/fast/css/square-button-appearance-expected.txt: Added.

No, we need a PNG result to make all of Chromium bots happy.
You don't need to prepare all of expected files.  You may just add an entry to test_expectations.txt like other ports.
Comment 12 Keishi Hattori 2011-08-26 01:03:55 PDT
Created attachment 105328 [details]
fixed conflict
Comment 13 Kent Tamura 2011-08-26 01:19:43 PDT
Comment on attachment 105328 [details]
fixed conflict

r- because of a pixel test failure on Chromium-linux.
Comment 14 Keishi Hattori 2011-08-26 01:55:01 PDT
Created attachment 105332 [details]
removed txt, added png
Comment 15 Keishi Hattori 2011-08-26 02:00:03 PDT
Created attachment 105333 [details]
added test to webkit win test_expected.txt
Comment 16 Kent Tamura 2011-08-26 02:07:15 PDT
(In reply to comment #15)
> Created an attachment (id=105333) [details]
> added test to webkit win test_expected.txt

It makes no sense.  Apple Windows doesn't use new-run-webkit-tests.
Comment 17 Keishi Hattori 2011-08-26 02:24:32 PDT
Created attachment 105337 [details]
revert changes in the last patch
Comment 18 Kent Tamura 2011-08-26 02:39:15 PDT
Comment on attachment 105337 [details]
revert changes in the last patch

ok
Comment 19 WebKit Review Bot 2011-08-28 18:11:46 PDT
Comment on attachment 105337 [details]
revert changes in the last patch

Rejecting attachment 105337 [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:
ayoutTests/platform/gtk/test_expectations.txt
Hunk #1 FAILED at 16.
1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/platform/gtk/test_expectations.txt.rej
patching file LayoutTests/platform/qt/test_expectations.txt
patching file Source/WebCore/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/WebCore/rendering/RenderThemeChromiumWin.cpp

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Kent Tamura', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/9564013
Comment 20 Keishi Hattori 2011-08-28 18:23:58 PDT
Created attachment 105452 [details]
rebased
Comment 21 Kent Tamura 2011-08-28 19:23:10 PDT
Comment on attachment 105452 [details]
rebased

You may fill out the "Reviewed by" lines in the patch, and may commit it yourself or may enqueue it to commit-queue because the patch already had r+.
Comment 22 Keishi Hattori 2011-08-28 19:47:56 PDT
(In reply to comment #21)
> (From update of attachment 105452 [details](apply) [details])
> You may fill out the "Reviewed by" lines in the patch, and may commit it yourself or may enqueue it to commit-queue because the patch already had r+.

Will do so next time.
Comment 23 WebKit Review Bot 2011-08-28 20:49:18 PDT
Comment on attachment 105452 [details]
rebased

Clearing flags on attachment: 105452

Committed r93953: <http://trac.webkit.org/changeset/93953>
Comment 24 WebKit Review Bot 2011-08-28 20:49:24 PDT
All reviewed patches have been landed.  Closing bug.