[Chromium]Change menu list background fallback value to transparent to work background:none for HTML select tag.
Created attachment 88187 [details] Patch
Still the patch is not matured. But I want to know the direction is correct or not. Windows has the same problem, so once Linux is fine, I will fix Windows.
Comment on attachment 88187 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=88187&action=review Ideally test cases shoudl be reduced as much as possible. > LayoutTests/fast/forms/select-background-none.html:1 > +<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd"> Not needed (or really wanted). > LayoutTests/fast/forms/select-background-none.html:13 > +<html xmlns="http://www.w3.org/1999/xhtml"> > +<head> > +<meta http-equiv="Content-Type" content="text/html; charset=utf-8" /> > +<style type="text/css"> > +body{ > + background:#666; > +} > +.test{ > + background:none; > +} > +</style> > +</head> Most of this isn't needed. > LayoutTests/fast/forms/select-background-none.html:19 > + <option>2</option> > + <option>3</option> > + <option>4</option> Only really need one.
Thanks, Eric. How about C++ side? (In reply to comment #3) > (From update of attachment 88187 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=88187&action=review > > Ideally test cases shoudl be reduced as much as possible. > > > LayoutTests/fast/forms/select-background-none.html:1 > > +<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd"> > > Not needed (or really wanted). > > > LayoutTests/fast/forms/select-background-none.html:13 > > +<html xmlns="http://www.w3.org/1999/xhtml"> > > +<head> > > +<meta http-equiv="Content-Type" content="text/html; charset=utf-8" /> > > +<style type="text/css"> > > +body{ > > + background:#666; > > +} > > +.test{ > > + background:none; > > +} > > +</style> > > +</head> > > Most of this isn't needed. > > > LayoutTests/fast/forms/select-background-none.html:19 > > + <option>2</option> > > + <option>3</option> > > + <option>4</option> > > Only really need one.
Created attachment 88481 [details] Patch
Comment on attachment 88481 [details] Patch Please review again.
Could you please review my patch?
Comment on attachment 88481 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=88481&action=review In either case, this looks fine. > Source/WebCore/rendering/RenderThemeChromiumLinux.cpp:278 > + extraParams.menuList.backgroundColor = SkColorSetARGB(0, 0, 0, 0); Do we not have a named SkColor for this? Color::transparent would be the WebCore way.
I see. I'll do that. (In reply to comment #8) > (From update of attachment 88481 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=88481&action=review > > In either case, this looks fine. > > > Source/WebCore/rendering/RenderThemeChromiumLinux.cpp:278 > > + extraParams.menuList.backgroundColor = SkColorSetARGB(0, 0, 0, 0); > > Do we not have a named SkColor for this? Color::transparent would be the WebCore way.
To be clear: I don't need to see the change agian. As is is fine, or changed your call. If you want to use the cq, you can just fill in the reviewer line and upload a patch marked with cq? (or cq+ if your'e a committer) and anyone can cq+ it without further review.
Cool. But actually, as I wrote here, it is Ok for Linux but not for Win. Because Windows has the same problem. Or should I make another patch for Windows? Thanks, (In reply to comment #10) > To be clear: I don't need to see the change agian. As is is fine, or changed your call. If you want to use the cq, you can just fill in the reviewer line and upload a patch marked with cq? (or cq+ if your'e a committer) and anyone can cq+ it without further review.
You could fix both in one patch, assuming your test case works for both. Up to you. :)
Yes, But I have to generate Windows test results because it includes font information... Can I ask you to generate expected results, once I'm ready for Windows patch? BTW, are you in #webkit irc channel? I can ping you if I'm ready. Thanks, (In reply to comment #12) > You could fix both in one patch, assuming your test case works for both. Up to you. :)
Does this change the color of the background when the drop down menu is open? What happens if background is not specified for the <select>?
No, this change affects only closed menu. If there no background, the original default color is specified, not transparent color. (In reply to comment #14) > Does this change the color of the background when the drop down menu is open? > > What happens if background is not specified for the <select>?
Created attachment 88948 [details] Patch
Comment on attachment 88948 [details] Patch As Eris Seidel wrote here, he doesn't need review again. Originally I wanted to add Windows fix, but not so easy compared to Linux, so I'll work on with separate patch. Please commit.
Comment on attachment 88948 [details] Patch I'm entertained that you reverted back to your earlier test case. Looks fine.
Comment on attachment 88948 [details] Patch Rejecting attachment 88948 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-8', 'build-..." exit_code: 2 Last 500 characters of output: mlmp . http/tests/xmlhttprequest ................................................................................................................................................................................ http/tests/xmlhttprequest/web-apps ............... http/tests/xmlhttprequest/workers ........... http/tests/xmlviewer . http/tests/xmlviewer/dumpAsText ............ 751.63s total testing time 23211 test cases (99%) succeeded 1 test case (<1%) was new 16 test cases (<1%) had stderr output Full output: http://queues.webkit.org/results/8381530
Eric, Thank you for your commit. But it looks failed. I guess the patch option looks wrong, right? Because I added new test and result (only Linux though) but the option specified Failed to run "['Tools/Scripts/run-webkit-tests', '--no-new-test-results', '--no-launch-safari', '--exit-after-n..." exit_code: 1 Could you give me your thought? Thanks, (In reply to comment #19) > (From update of attachment 88948 [details]) > Rejecting attachment 88948 [details] from commit-queue. > > Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-8', 'build-..." exit_code: 2 > > Last 500 characters of output: > mlmp . > http/tests/xmlhttprequest ................................................................................................................................................................................ > http/tests/xmlhttprequest/web-apps ............... > http/tests/xmlhttprequest/workers ........... > http/tests/xmlviewer . > http/tests/xmlviewer/dumpAsText ............ > 751.63s total testing time > > 23211 test cases (99%) succeeded > 1 test case (<1%) was new > 16 test cases (<1%) had stderr output > > Full output: http://queues.webkit.org/results/8381530
Your patch doesn't have any results for the Mac port. So the commit-queue can't land it (since it would make the mac bot turn red): fast/forms/select-background-none.html -> new
Ok, Maybe I can generate Mac expected results but how about Windows? As I wrote here, Windows still has problem. Do I have to treat special care for Windows? Otherwise, doesn't it fail again? (In reply to comment #21) > Your patch doesn't have any results for the Mac port. So the commit-queue can't land it (since it would make the mac bot turn red): > > fast/forms/select-background-none.html -> new
We don't want to land patches which would make the bots red. So if you expect your patch to do that (which it sounds like you do), it's not ready for landing. :)
I see. But my Windows dev environment is extremely slow... So it is very painful to debug on Windows. Anyway, I want to continue to figure out the windows problem also. Thanks, (In reply to comment #23) > We don't want to land patches which would make the bots red. So if you expect your patch to do that (which it sounds like you do), it's not ready for landing. :)
Takano-san, You need a Mac result in the patch because commit-queue runs on Mac. You don't need a Windows result at this moment. Port maintainers will commit Windows results later.
Kent-san, Thank you for your comment. So can I land my patch without Windows porting? That's great news for me;-) Tomorrow, I'll do that. Thanks, (In reply to comment #25) > Takano-san, > You need a Mac result in the patch because commit-queue runs on Mac. You don't need a Windows result at this moment. Port maintainers will commit Windows results later.
(In reply to comment #26) > So can I land my patch without Windows porting? Yes. I think we may land a test without non-Mac results. Ideally we should land a test with results of all platforms. But it's almost impossible.
Created attachment 89344 [details] Patch
Comment on attachment 89344 [details] Patch I added mac expected results. Please try commit again. Thanks,
Comment on attachment 89344 [details] Patch These only have the chromium-mac expectations, not the apple mac or "mac" expectations, so the cq will fail again. However all non-chromium ports don't bother to run the pixel tests, so all you need are the -expected.txt results, which I would expect to be identical between mac and chromium mac. As an experiment, I'm going to cq+ this, as the cq should now be smart enough to upload the results.zip file including the failure information for you (as of yesterday).
Comment on attachment 89344 [details] Patch Rejecting attachment 89344 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-3', 'build-..." exit_code: 2 Last 500 characters of output: mlmp . http/tests/xmlhttprequest ................................................................................................................................................................................ http/tests/xmlhttprequest/web-apps ............... http/tests/xmlhttprequest/workers ........... http/tests/xmlviewer . http/tests/xmlviewer/dumpAsText ............ 723.47s total testing time 23260 test cases (99%) succeeded 1 test case (<1%) was new 16 test cases (<1%) had stderr output Full output: http://queues.webkit.org/results/8399298
Created attachment 89387 [details] Archive of layout-test-results from cr-jail-3 The attached test failures were seen while running run-webkit-tests on the commit-queue. Bot: cr-jail-3 Port: Mac Platform: Mac OS X 10.6.6
(In reply to comment #32) > Created an attachment (id=89387) [details] > Archive of layout-test-results from cr-jail-3 > > The attached test failures were seen while running run-webkit-tests on the commit-queue. > Bot: cr-jail-3 Port: Mac Platform: Mac OS X 10.6.6 This failed because the (Apple) mac bot wasn't able to find results for the new test. I suspect the chromium-mac results should be put in platform/mac, although you might want to verify that the tests are in fact the same with mac webkit.
So exiciting! My new code works! That's soooo useful!
Ok, I'll copy only select-background-none-expected.txt into platform/mac/fast/forms/. Is this correct? I assumed "mac" meant "chromium-mac", but there is another "mac" inside... Thanks, (In reply to comment #34) > So exiciting! My new code works! That's soooo useful! (In reply to comment #30) > (From update of attachment 89344 [details]) > These only have the chromium-mac expectations, not the apple mac or "mac" expectations, so the cq will fail again. > > However all non-chromium ports don't bother to run the pixel tests, so all you need are the -expected.txt results, which I would expect to be identical between mac and chromium mac. > > As an experiment, I'm going to cq+ this, as the cq should now be smart enough to upload the results.zip file including the failure information for you (as of yesterday).
(In reply to comment #35) > I'll copy only select-background-none-expected.txt into platform/mac/fast/forms/. Is this correct? Yes. Please copy select-background-none-actual.txt in Attachment #89387 [details] into platform/mac/fast/forms/select-background-none-expected.txt.
Created attachment 89518 [details] Patch
Comment on attachment 89518 [details] Patch Please commit again. I hope this is the last try!!
Comment on attachment 89518 [details] Patch Rejecting attachment 89518 [details] from review queue. takano.naoki@gmail.com does not have reviewer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py. - If you do not have reviewer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags. - If you have reviewer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed). The commit-queue restarts itself every 2 hours. After restart the commit-queue will correctly respect your reviewer rights.
Hmm... The patch is not landed. Is the bot very busy?
The commit-queue encountered the following flaky tests while processing attachment 89518 [details]: http/tests/misc/favicon-loads-with-icon-loading-override.html bug 58412 (author: alice.liu@apple.com) The commit-queue is continuing to process your patch.
Comment on attachment 89518 [details] Patch Clearing flags on attachment: 89518 Committed r83930: <http://trac.webkit.org/changeset/83930>
All reviewed patches have been landed. Closing bug.