Bug 28631

Created attachment 38404 [details] patch

Created attachment 43741 [details] updated Adam seems to have forgotten this patch, so I have merged it to ToT and changed the logic to center rather than flushing bottom right (this matches what firefox appears to do). I am sending a run to the chromium layout test try bots to see if any layout test expectations need updating. I also tested to make sure that checkboxes are unchanged in the default case (e.g., gmail login screen).

Comment on attachment 43741 [details] updated Style: 226 IntRect center(const IntRect& original, int width, int height) { Does this change match other webkit ports behaviors? I would assume it does if it matches IE.

it matches win safari, win chrome, and win ie (which are all the same presumably because they all draw checkboxes via windows system calls)

Comment on attachment 43741 [details] updated r- for If a WebKit layout test already covers this functionality, just list it in the ChangeLog. If not, please add a simple one. > Index: WebCore/rendering/RenderThemeChromiumSkia.cpp > +IntRect center(const IntRect& original, int width, int height) { Brace needs to go on the next line for functions.

Created attachment 43819 [details] plus test

Created attachment 43820 [details] html cleanup

Comment on attachment 43820 [details] html cleanup I'm not a big fan of the name "center", but I'm not sure I have a better suggestion. The patch looks fine. We'll need results for other platforms. I'm surprised that this isn't already covered by other tests. Also, WebKit has no firm wrapping rule: + Make the default size for checkboxes/radio buttons also the maximum + size. We generally just wrap when it's convenient. :)

Comment on attachment 43820 [details] html cleanup Evan asked me via email if this could be cq'd. We could cq this, but then there are results that will be missing on the various bots and someone else will have to commit them in a separate run. It might be easier to have a committer commit this manually and grab the results off the bot. We really need some sort of try-server setup which could produce the various platform results to avoid this sort of pain. :( You can of course always request a cq by setting commit-queue=?, but I think this particular patch should be landed by hand, unless someone wants to watch the bots after the commit-queue lands it, but at that point one might as well just land it by hand. :)

Comment on attachment 43820 [details] html cleanup I'll pull the results off of the windows bot once this lands.

Comment on attachment 43820 [details] html cleanup Rejecting patch 43820 from commit-queue. Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--quiet']" exit_code: 1 Running build-dumprendertree Running tests from /Users/eseidel/Projects/CommitQueueSVN/LayoutTests Testing 11853 test cases. fast/css/non-standard-checkbox-size.html -> failed Exiting early after 1 failures. 5218 tests run. 78.53s total testing time 5217 test cases (99%) succeeded 1 test case (<1%) had incorrect layout 1 test case (<1%) had stderr output Full output: http://webkit-commit-queue.appspot.com/results/151590

Comment on attachment 43820 [details] html cleanup Test results are incorrect per the commit-queue. Sorry you got a-run-around here, but the change you're attempting to make is not one which can be made drive-by. Because it requires test results on many platforms it requires the engagement of an active committer.

I'll give landing this a whirl.

Comment on attachment 43820 [details] html cleanup Marking r+ again so the apply scripts work.

Committed r53289: <http://trac.webkit.org/changeset/53289>

I ended up just borrowing a trick from Chromium's book and skipping the test on platforms for which I don't have results. I should have just encouraged you to do that in the first place. I think WebKit is too scared of using the Skipped list. Yes, this particular use of it isn't very good, but it's better than turning the bots red. :) It's impractical to expect you, or anyone else, to be able to generate pixel results for all platforms with our current infrastructure.