Summary: | Implement ColorSuggestionPicker page popup | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Keishi Hattori <keishi> | ||||||||||||||
Component: | Forms | Assignee: | 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
Keishi Hattori
2012-07-24 06:15:19 PDT
Created attachment 154041 [details]
Patch
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 = ... Created attachment 154060 [details]
Patch
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 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 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. Created attachment 154222 [details]
Patch
(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 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 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 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
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. Created attachment 154317 [details]
Patch
Comment on attachment 154317 [details] Patch Clearing flags on attachment: 154317 Committed r123606: <http://trac.webkit.org/changeset/123606> All reviewed patches have been landed. Closing bug. Re-opened since this is blocked by 92247 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 (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? (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 " && ". > 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?
(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. Created attachment 154608 [details]
Patch
Comment on attachment 154608 [details]
Patch
ok
Comment on attachment 154608 [details] Patch Clearing flags on attachment: 154608 Committed r123833: <http://trac.webkit.org/changeset/123833> All reviewed patches have been landed. Closing bug. |