Bug 66918 – Chromium Win: Setting square-button appearance reaches NOTREACHED

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Keishi Hattori]

RenderThemeChromiumWin.cpp lacks cases for SquareButtonPart.

[Keishi Hattori]

Created attachment 105119 [details]
patch

[Kent Tamura]

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.

[Keishi Hattori]

Created attachment 105311 [details]
added test

[Kent Tamura]

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

[Keishi Hattori]

Created attachment 105320 [details]
moved expected files and changed test_expectations

[Keishi Hattori]

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.

[Kent Tamura]

Comment on attachment 105320 [details]
moved expected files and changed test_expectations

r- because the patch conflicts and chromium-linux will fail.

[Keishi Hattori]

Created attachment 105327 [details]
added chromium-linux expected file

[Keishi Hattori]

Do I need to add any more?

[Keishi Hattori]

Comment on attachment 105327 [details]
added chromium-linux expected file

forgot to fix conflict

[Kent Tamura]

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.

[Keishi Hattori]

Created attachment 105328 [details]
fixed conflict

[Kent Tamura]

Comment on attachment 105328 [details]
fixed conflict

r- because of a pixel test failure on Chromium-linux.

[Keishi Hattori]

Created attachment 105332 [details]
removed txt, added png

[Keishi Hattori]

Created attachment 105333 [details]
added test to webkit win test_expected.txt

[Kent Tamura]

(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.

[Keishi Hattori]

Created attachment 105337 [details]
revert changes in the last patch

[Kent Tamura]

Comment on attachment 105337 [details]
revert changes in the last patch

ok

[WebKit Review Bot]

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

[Keishi Hattori]

Created attachment 105452 [details]
rebased

[Kent Tamura]

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+.

[Keishi Hattori]

(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.

[WebKit Review Bot]

Comment on attachment 105452 [details]
rebased

Clearing flags on attachment: 105452

Committed r93953: <http://trac.webkit.org/changeset/93953>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.