66658 2011-08-22 03:31:18 -0700 Chrome::setSelectedColorInColorChooser shouldn't be called when color chooser sets a new color 2011-08-23 07:45:38 -0700 1 1 1 Unclassified WebKit Forms 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 keishi keishi darin tkent webkit.review.bot oldest_to_newest 454516 0 keishi 2011-08-22 03:31:18 -0700 Chrome::setSelectedColorInColorChooser is being called when the color chooser selects a color. This causes an infinite loop when the user changes the color very rapidly. 454517 1 104658 keishi 2011-08-22 03:34:23 -0700 Created attachment 104658 patch 454522 2 104658 tkent 2011-08-22 03:54:38 -0700 Comment on attachment 104658 patch ok 454532 3 104658 webkit.review.bot 2011-08-22 04:53:34 -0700 Comment on attachment 104658 patch Clearing flags on attachment: 104658 Committed r93503: <http://trac.webkit.org/changeset/93503> 454533 4 webkit.review.bot 2011-08-22 04:53:38 -0700 All reviewed patches have been landed. Closing bug. 454633 5 darin 2011-08-22 09:39:59 -0700 Normally this kind of change needs a regression test. Why no test in this case? 455125 6 keishi 2011-08-22 18:07:20 -0700 (In reply to comment #5) > Normally this kind of change needs a regression test. Why no test in this case? I can't write a layout test because I need to manipulate NSColorPanel for the problem to occur. 455131 7 darin 2011-08-22 18:10:46 -0700 (In reply to comment #6) > I can't write a layout test because I need to manipulate NSColorPanel for the problem to occur. We can easily add a feature to DumpRenderTree to call the same function that the NSColorPanel would. 455147 8 keishi 2011-08-22 19:05:41 -0700 This is my first time looking at DumpRenderTree and I have a couple of questions. 1. The WebKit part of the patch hasn't landed yet (Bug 65889, Bug 65897). So if I were to do this right now, I need to call a WebCore method from DumpRenderTree. Would it be better to do this after the patches land. 2. This infinite loop only happens in Chromium. The "set to color chooser" and "set from color chooser" IPCs get intertwined causing the loop. The loop won't happen in WebKit because the calls are from a single thread and the NSColorPanel checks if the same color is being set. Could we use DumpRenderTree to test for this? 455157 9 tkent 2011-08-22 19:24:11 -0700 (In reply to comment #8) > This is my first time looking at DumpRenderTree and I have a couple of questions. > > 1. The WebKit part of the patch hasn't landed yet (Bug 65889, Bug 65897). So if I were to do this right now, I need to call a WebCore method from DumpRenderTree. Would it be better to do this after the patches land. I think we had better introduce a mock color panel delegate object and inject it to Chrome or ChromeClient by window.internals; e.g. window.internals.useMockColorPanel(). 455402 10 keishi 2011-08-23 07:45:38 -0700 Thanks darin, tkent. I think I have an idea how to test this now. I am working on it. 104658 2011-08-22 03:34:23 -0700 2011-08-22 04:53:34 -0700 patch bug-66658-20110822193421.patch text/plain 2347 keishi U3VidmVyc2lvbiBSZXZpc2lvbjogOTMzODcKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwppbmRleCBlZDYzMTlkYTE3NWUzNDAy NmRhYzY3NzM5MmU3OTY2NGUzMWQwYTBmLi5kOWQ3YzU3ODExOWUzMGEyNmU4YzkzNzZiY2M1NmQy ZmNhOWU2YWI2IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvU291 cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTQgQEAKKzIwMTEtMDgtMjIgIEtlaXNo aSBIYXR0b3JpICA8a2Vpc2hpQHdlYmtpdC5vcmc+CisKKyAgICAgICAgQ2hyb21lOjpzZXRTZWxl Y3RlZENvbG9ySW5Db2xvckNob29zZXIgc2hvdWxkbid0IGJlIGNhbGxlZCB3aGVuIGNvbG9yIGNo b29zZXIgc2V0cyBhIG5ldyBjb2xvcgorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9z aG93X2J1Zy5jZ2k/aWQ9NjY2NTgKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMh KS4KKworICAgICAgICAqIGh0bWwvQ29sb3JJbnB1dFR5cGUuY3BwOgorICAgICAgICAoV2ViQ29y ZTo6Q29sb3JJbnB1dFR5cGU6OmNvbG9yU2VsZWN0ZWQpOiBDYWxsIHNldFZhbHVlRnJvbVJlbmRl cmVyIGluc3RlYWQgb2Ygc2V0VmFsdWUuCisgICAgICAgICogaHRtbC9Db2xvcklucHV0VHlwZS5o OgorCiAyMDExLTA4LTE4ICBTaGF3biBTaW5naCAgPHNoYXduc2luZ2hAY2hyb21pdW0ub3JnPgog CiAgICAgICAgIENvbXB1dGluZyBzY3JlZW4tc3BhY2UgdHJhbnNmb3JtIGZvciBMYXllckNocm9t aXVtIGFuZCBDQ0xheWVySW1wbApkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUvaHRtbC9Db2xv cklucHV0VHlwZS5jcHAgYi9Tb3VyY2UvV2ViQ29yZS9odG1sL0NvbG9ySW5wdXRUeXBlLmNwcApp bmRleCBhNTkyMGI3Mjg3NDZiYTU4NWZiMDk5YmZmYjRlODNhYjZjMDY1NzEzLi44ZGFkMmMzNDMw MTkyOTE4MDUyNjcwZjk1ZTU5NjQyMDk2NjMyYjFlIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29y ZS9odG1sL0NvbG9ySW5wdXRUeXBlLmNwcAorKysgYi9Tb3VyY2UvV2ViQ29yZS9odG1sL0NvbG9y SW5wdXRUeXBlLmNwcApAQCAtMTA1LDExICsxMDUsNiBAQCBDb2xvciBDb2xvcklucHV0VHlwZTo6 dmFsdWVBc0NvbG9yKCkgY29uc3QKICAgICByZXR1cm4gQ29sb3IoZWxlbWVudCgpLT52YWx1ZSgp KTsKIH0KIAotdm9pZCBDb2xvcklucHV0VHlwZTo6c2V0VmFsdWVBc0NvbG9yKGNvbnN0IENvbG9y JiBjb2xvcikgY29uc3QKLXsKLSAgICBlbGVtZW50KCktPnNldFZhbHVlKGNvbG9yLnNlcmlhbGl6 ZWQoKSwgdHJ1ZSk7Ci19Ci0KIHZvaWQgQ29sb3JJbnB1dFR5cGU6OmNyZWF0ZVNoYWRvd1N1YnRy ZWUoKQogewogICAgIERvY3VtZW50KiBkb2N1bWVudCA9IGVsZW1lbnQoKS0+ZG9jdW1lbnQoKTsK QEAgLTE3NCw3ICsxNjksOCBAQCB2b2lkIENvbG9ySW5wdXRUeXBlOjpjb2xvclNlbGVjdGVkKGNv bnN0IENvbG9yJiBjb2xvcikKIHsKICAgICBpZiAoZWxlbWVudCgpLT5kaXNhYmxlZCgpIHx8IGVs ZW1lbnQoKS0+cmVhZE9ubHkoKSkKICAgICAgICAgcmV0dXJuOwotICAgIHNldFZhbHVlQXNDb2xv cihjb2xvcik7CisgICAgZWxlbWVudCgpLT5zZXRWYWx1ZUZyb21SZW5kZXJlcihjb2xvci5zZXJp YWxpemVkKCkpOworICAgIHVwZGF0ZUNvbG9yU3dhdGNoKCk7CiB9CiAKIGJvb2wgQ29sb3JJbnB1 dFR5cGU6OmlzQ29sb3JJbnB1dFR5cGUoKSBjb25zdApkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNv cmUvaHRtbC9Db2xvcklucHV0VHlwZS5oIGIvU291cmNlL1dlYkNvcmUvaHRtbC9Db2xvcklucHV0 VHlwZS5oCmluZGV4IDM4ODhjYjUyOGZhNjlkNmUxYTljMzg0OGNkOGU4NjkzOTI5OTdlMTYuLjky OGVmODY4ZjFiN2IzOGU5MWUzOTIxMzc5Mjk5ZTRlNTAwNTRkZjYgMTAwNjQ0Ci0tLSBhL1NvdXJj ZS9XZWJDb3JlL2h0bWwvQ29sb3JJbnB1dFR5cGUuaAorKysgYi9Tb3VyY2UvV2ViQ29yZS9odG1s L0NvbG9ySW5wdXRUeXBlLmgKQEAgLTUzLDcgKzUzLDYgQEAgcHJpdmF0ZToKICAgICB2aXJ0dWFs IFN0cmluZyBmYWxsYmFja1ZhbHVlKCk7CiAgICAgdmlydHVhbCBTdHJpbmcgc2FuaXRpemVWYWx1 ZShjb25zdCBTdHJpbmcmKTsKICAgICB2aXJ0dWFsIENvbG9yIHZhbHVlQXNDb2xvcigpIGNvbnN0 OwotICAgIHZpcnR1YWwgdm9pZCBzZXRWYWx1ZUFzQ29sb3IoY29uc3QgQ29sb3ImKSBjb25zdDsK ICAgICB2aXJ0dWFsIHZvaWQgY3JlYXRlU2hhZG93U3VidHJlZSgpOwogICAgIHZpcnR1YWwgdm9p ZCB2YWx1ZUNoYW5nZWQoKTsKICAgICB2aXJ0dWFsIHZvaWQgaGFuZGxlQ2xpY2tFdmVudChNb3Vz ZUV2ZW50Kik7Cg==