Bug 66658 - Chrome::setSelectedColorInColorChooser shouldn't be called when color chooser sets a new color
Summary: Chrome::setSelectedColorInColorChooser shouldn't be called when color chooser...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Keishi Hattori
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-08-22 03:31 PDT by Keishi Hattori
Modified: 2011-08-23 07:45 PDT (History)
3 users (show)

See Also:

Attachments
patch (2.29 KB, patch)
2011-08-22 03:34 PDT, Keishi Hattori 		no flags Details | Formatted Diff | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
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.