Bug 92109

Summary: Implement ColorSuggestionPicker page popup
Product: WebKit Reporter: Keishi Hattori <keishi>
Component: FormsAssignee: Keishi Hattori <keishi>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, tkent, tomhudson, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 92247    
Bug Blocks: 92075    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from gce-cr-linux-08
none
Patch
none
Patch none

Description Keishi Hattori 2012-07-24 06:15:19 PDT
Implement ColorSuggestionPicker page popup
Comment 1 Keishi Hattori 2012-07-24 06:30:57 PDT
Created attachment 154041 [details]
Patch
Comment 2 Kent Tamura 2012-07-24 07:34:51 PDT
Comment on attachment 154041 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=154041&action=review

> Source/WebCore/Resources/colorSuggestionPicker.js:97
> +"#ffffff", "#980000", "#ff0000", "#ff9900", "#ffff00", "#00ff00", "#00ffff",
> +"#4a86e8", "#0000ff", "#9900ff", "#ff00ff"];

nit: need indentation

> Source/WebCore/Resources/colorSuggestionPicker.js:144
> +    this.element = element;
> +    this.config = config;

private members should be start with _

this.element -> this._element
this.config -> this._config
this.layout() -> this._layout()

> Source/WebCore/Resources/colorSuggestionPicker.js:150
> +var SwatchSize = 24; // keep in sync with CSS

'Size' is unclear.  This should be SwatchBorderBoxWidth or something like that.  The comment or the name should have its unit.

> Source/WebCore/Resources/colorSuggestionPicker.js:157
> +    for (var i = 0; i  < this.config.values.length; i ++) {

- extra space between i and <.
- i ++ should be ++i

> Source/WebCore/Resources/colorSuggestionPicker.js:159
> +        swatch.value = this.config.values[i];

Adding an application data to a DOM object isn't a good idea.
We should use data-* attribute like
    swatch.dataset.value = ...
Comment 3 Keishi Hattori 2012-07-24 07:49:39 PDT
Created attachment 154060 [details]
Patch
Comment 4 Keishi Hattori 2012-07-24 07:52:44 PDT
Comment on attachment 154041 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=154041&action=review

>> Source/WebCore/Resources/colorSuggestionPicker.js:97
>> +"#4a86e8", "#0000ff", "#9900ff", "#ff00ff"];
> 
> nit: need indentation

Done.

>> Source/WebCore/Resources/colorSuggestionPicker.js:144
>> +    this.config = config;
> 
> private members should be start with _
> 
> this.element -> this._element
> this.config -> this._config
> this.layout() -> this._layout()

Done.

>> Source/WebCore/Resources/colorSuggestionPicker.js:150
>> +var SwatchSize = 24; // keep in sync with CSS
> 
> 'Size' is unclear.  This should be SwatchBorderBoxWidth or something like that.  The comment or the name should have its unit.

I was using it as width and height so I made two constants SwatchBorderBoxWidth and SwatchBorderBoxHeight.

>> Source/WebCore/Resources/colorSuggestionPicker.js:157
>> +    for (var i = 0; i  < this.config.values.length; i ++) {
> 
> - extra space between i and <.
> - i ++ should be ++i

Done.

>> Source/WebCore/Resources/colorSuggestionPicker.js:159
>> +        swatch.value = this.config.values[i];
> 
> Adding an application data to a DOM object isn't a good idea.
> We should use data-* attribute like
>     swatch.dataset.value = ...

Done.
Comment 5 Kent Tamura 2012-07-24 09:10:13 PDT
Comment on attachment 154060 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=154060&action=review

> Source/WebCore/WebCore.gyp/WebCore.gyp:930
> +          'action_name': 'ColorSuggestionPicker',

we should exclude this action for Android.
Comment 6 Kent Tamura 2012-07-24 17:52:51 PDT
Comment on attachment 154060 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=154060&action=review

>> Source/WebCore/WebCore.gyp/WebCore.gyp:930
>> +          'action_name': 'ColorSuggestionPicker',
> 
> we should exclude this action for Android.

Probably, it would be simpler to add "&& ENABLE(PAGE_POPUP)" or "&& !OS(ANDROID)" to --condition argument.
Comment 7 Keishi Hattori 2012-07-24 19:14:06 PDT
Created attachment 154222 [details]
Patch
Comment 8 Keishi Hattori 2012-07-24 19:14:15 PDT
(In reply to comment #6)
> (From update of attachment 154060 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=154060&action=review
> 
> >> Source/WebCore/WebCore.gyp/WebCore.gyp:930
> >> +          'action_name': 'ColorSuggestionPicker',
> > 
> > we should exclude this action for Android.
> 
> Probably, it would be simpler to add "&& ENABLE(PAGE_POPUP)" or "&& !OS(ANDROID)" to --condition argument.

Added && ENABLE(PAGE_POPUP)
Comment 9 Kent Tamura 2012-07-24 20:27:05 PDT
Comment on attachment 154222 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=154222&action=review

> Source/WebCore/Resources/colorSuggestionPicker.js:28
> +var global = {
> +    argumentsReceived: false
> +};

Making 'global' object only for 'argumentsReceived' flag is overkill.
We had better remove global, or add scrollbarWidth to global.
Comment 10 WebKit Review Bot 2012-07-24 21:50:59 PDT
Comment on attachment 154222 [details]
Patch

Attachment 154222 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13343112

New failing tests:
fast/forms/range/slider-onchange-event.html
fast/forms/range/slider-mouse-events.html
fast/forms/range/slider-delete-while-dragging-thumb.html
Comment 11 WebKit Review Bot 2012-07-24 21:51:02 PDT
Created attachment 154246 [details]
Archive of layout-test-results from gce-cr-linux-08

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-08  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment 12 Keishi Hattori 2012-07-25 04:01:21 PDT
scrollbarWidth(In reply to comment #9)
> (From update of attachment 154222 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=154222&action=review
> 
> > Source/WebCore/Resources/colorSuggestionPicker.js:28
> > +var global = {
> > +    argumentsReceived: false
> > +};
> 
> Making 'global' object only for 'argumentsReceived' flag is overkill.
> We had better remove global, or add scrollbarWidth to global.

Added scrollbarWidth to global.
Comment 13 Keishi Hattori 2012-07-25 04:01:45 PDT
Created attachment 154317 [details]
Patch
Comment 14 WebKit Review Bot 2012-07-25 04:57:42 PDT
Comment on attachment 154317 [details]
Patch

Clearing flags on attachment: 154317

Committed r123606: <http://trac.webkit.org/changeset/123606>
Comment 15 WebKit Review Bot 2012-07-25 04:57:47 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 WebKit Review Bot 2012-07-25 06:57:47 PDT
Re-opened since this is blocked by 92247
Comment 17 Tom Hudson 2012-07-25 07:19:38 PDT
Breaks the Chromium Windows Canary bots with the following error message:

72>------ Build started: Project: webcore_bindings_sources, Configuration: Release Win32 ------
...
72>  ColorSuggestionPicker
72>  bash: -c: line 0: unexpected EOF while looking for matching `"'
72>  bash: -c: line 1: syntax error: unexpected end of file
Comment 18 Keishi Hattori 2012-07-25 13:12:41 PDT
(In reply to comment #17)
> Breaks the Chromium Windows Canary bots with the following error message:
> 
> 72>------ Build started: Project: webcore_bindings_sources, Configuration: Release Win32 ------
> ...
> 72>  ColorSuggestionPicker
> 72>  bash: -c: line 0: unexpected EOF while looking for matching `"'
> 72>  bash: -c: line 1: syntax error: unexpected end of file

I looked at webcore_bindings_sources.vcxproj but the quotes seemed to be balanced. Any ideas?
Comment 19 Kent Tamura 2012-07-26 00:19:27 PDT
(In reply to comment #18)
> (In reply to comment #17)
> > Breaks the Chromium Windows Canary bots with the following error message:
> > 
> > 72>------ Build started: Project: webcore_bindings_sources, Configuration: Release Win32 ------
> > ...
> > 72>  ColorSuggestionPicker
> > 72>  bash: -c: line 0: unexpected EOF while looking for matching `"'
> > 72>  bash: -c: line 1: syntax error: unexpected end of file
> 
> I looked at webcore_bindings_sources.vcxproj but the quotes seemed to be balanced. Any ideas?

The generated command is:

call call "$(ProjectDir)..\..\..\..\..\third_party\cygwin\setup_env.bat" && set CYGWIN=nontsec&& set OUTDIR=$(OutDir)&& bash -c "\"python\" \"../make-file-arrays.py\" \"--condition=ENABLE(INPUT_TYPE_COLOR) && ENABLE(DATALIST_ELEMENT) && ENABLE(PAGE_POPUP)\" \"--out-h=`cygpath -m \\\"${OUTDIR}\\\"`/obj/global_intermediate/webkit/ColorSuggestionPicker.h\" \"--out-cpp=`cygpath -m \\\"${OUTDIR}\\\"`/obj/global_intermediate/webkit/ColorSuggestionPicker.cpp\" \"../Resources/colorSuggestionPicker.css\" \"../Resources/colorSuggestionPicker.js\""

I guess CMD's command-line parser split it by '&&' first. I have no idea to avoid this issue.
How about changing the --condition processing of make-file-arrays.py?  e.g. make-file-arrays.py replaces " AND " with " && ".
Comment 20 Keishi Hattori 2012-07-26 01:08:57 PDT
> I guess CMD's command-line parser split it by '&&' first. I have no idea to avoid this issue.
> How about changing the --condition processing of make-file-arrays.py?  e.g. make-file-arrays.py replaces " AND " with " && ".

How about '--condition=!!ENABLE(INPUT_TYPE_COLOR) & !!ENABLE(DATALIST_ELEMENT) & !!ENABLE(PAGE_POPUP)', ? Which is better?
Comment 21 Kent Tamura 2012-07-26 02:29:27 PDT
(In reply to comment #20)
> > I guess CMD's command-line parser split it by '&&' first. I have no idea to avoid this issue.
> > How about changing the --condition processing of make-file-arrays.py?  e.g. make-file-arrays.py replaces " AND " with " && ".
> 
> How about '--condition=!!ENABLE(INPUT_TYPE_COLOR) & !!ENABLE(DATALIST_ELEMENT) & !!ENABLE(PAGE_POPUP)', ? Which is better?

'&' is also a special character in CMD. I confirmed it didn't work.
Comment 22 Keishi Hattori 2012-07-26 04:27:22 PDT
Created attachment 154608 [details]
Patch
Comment 23 Kent Tamura 2012-07-26 04:31:56 PDT
Comment on attachment 154608 [details]
Patch

ok
Comment 24 WebKit Review Bot 2012-07-26 20:22:11 PDT
Comment on attachment 154608 [details]
Patch

Clearing flags on attachment: 154608

Committed r123833: <http://trac.webkit.org/changeset/123833>
Comment 25 WebKit Review Bot 2012-07-26 20:22:16 PDT
All reviewed patches have been landed.  Closing bug.