Bug 92109 – Implement ColorSuggestionPicker page popup

• Component: Forms
• Status: RESOLVED FIXED

[Keishi Hattori]

Implement ColorSuggestionPicker page popup

[Keishi Hattori]

Created attachment 154041 [details]
Patch

[Kent Tamura]

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 = ...

[Keishi Hattori]

Created attachment 154060 [details]
Patch

[Keishi Hattori]

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.

[Kent Tamura]

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.

[Kent Tamura]

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.

[Keishi Hattori]

Created attachment 154222 [details]
Patch

[Keishi Hattori]

(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)

[Kent Tamura]

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.

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Keishi Hattori]

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.

[Keishi Hattori]

Created attachment 154317 [details]
Patch

[WebKit Review Bot]

Comment on attachment 154317 [details]
Patch

Clearing flags on attachment: 154317

Committed r123606: <http://trac.webkit.org/changeset/123606>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by 92247

[Tom Hudson]

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

[Keishi Hattori]

(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?

[Kent Tamura]

(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 " && ".

[Keishi Hattori]

> 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?

[Kent Tamura]

(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.

[Keishi Hattori]

Created attachment 154608 [details]
Patch

[Kent Tamura]

Comment on attachment 154608 [details]
Patch

ok

[WebKit Review Bot]

Comment on attachment 154608 [details]
Patch

Clearing flags on attachment: 154608

Committed r123833: <http://trac.webkit.org/changeset/123833>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.