Bug 66658

Summary: Chrome::setSelectedColorInColorChooser shouldn't be called when color chooser sets a new color
Product: WebKit Reporter: Keishi Hattori <keishi>
Component: FormsAssignee: Keishi Hattori <keishi>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, tkent, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch none

Description Keishi Hattori 2011-08-22 03:31:18 PDT
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.
Comment 1 Keishi Hattori 2011-08-22 03:34:23 PDT
Created attachment 104658 [details]
patch
Comment 2 Kent Tamura 2011-08-22 03:54:38 PDT
Comment on attachment 104658 [details]
patch

ok
Comment 3 WebKit Review Bot 2011-08-22 04:53:34 PDT
Comment on attachment 104658 [details]
patch

Clearing flags on attachment: 104658

Committed r93503: <http://trac.webkit.org/changeset/93503>
Comment 4 WebKit Review Bot 2011-08-22 04:53:38 PDT
All reviewed patches have been landed.  Closing bug.
Comment 5 Darin Adler 2011-08-22 09:39:59 PDT
Normally this kind of change needs a regression test. Why no test in this case?
Comment 6 Keishi Hattori 2011-08-22 18:07:20 PDT
(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.
Comment 7 Darin Adler 2011-08-22 18:10:46 PDT
(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.
Comment 8 Keishi Hattori 2011-08-22 19:05:41 PDT
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?
Comment 9 Kent Tamura 2011-08-22 19:24:11 PDT
(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().
Comment 10 Keishi Hattori 2011-08-23 07:45:38 PDT
Thanks darin, tkent. I think I have an idea how to test this now. I am working on it.