Issue 7690004: Implement input type=color UI (win part)

Keyboard Shortcuts

	File
u :	up to issue
j / k :	jump to file after / before current file
J / K :	jump to next file with a comment after / before current file
	Side-by-side diff
i :	toggle intra-line diffs
e :	expand all comments
c :	collapse all comments
s :	toggle showing all comments
n / p :	next / previous diff chunk or comment
N / P :	next / previous comment
<Up> / <Down> :	next / previous line

	Issue
u :	up to list of issues
j / k :	jump to patch after / before current patch
o / <Enter> :	open current patch in side-by-side view
i :	open current patch in unified diff view

	Issue List
j / k :	jump to issue after / before current issue
o / <Enter> :	open current issue

Issue 7690004: Implement input type=color UI (win part) (Closed)

Created:
9 years, 4 months ago by keishi

Modified:
8 years, 11 months ago

Reviewers:
keishi, yosin_UTC9

Base URL:
http://git.chromium.org/git/chromium.git@trunk

Visibility:
Public.

Description

Implement input type=color UI (win part) Apply "Issue 7685006 Implement input type=color UI (common part)" first. BUG=92608 TEST=none

Patch Set 1 #

Patch Set 2 : Forgot to turn off flag #

Patch Set 3 : renamed closeColorChooser and colorSelected to match WebKit side #

Patch Set 4 : rebased #

Total comments: 7

Created: 9 years, 2 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+152 lines, -0 lines)			Patch
A	chrome/browser/ui/color_chooser_dialog.h	View	1 2	1 chunk	+40 lines, -0 lines	3 comments	Download
M	chrome/browser/ui/views/shell_dialogs_win.cc	View	1 2	2 chunks	+111 lines, -0 lines	4 comments	Download
M	chrome/chrome_browser.gypi	View	1 2 3	1 chunk	+1 line, -0 lines	0 comments	Download

Messages

Total messages: 1 (0 generated)

Expand Messages | Collapse Messages

yosin_UTC9

9 years ago (2011-12-06 07:31:50 UTC) #1

Few minor style comments.

http://codereview.chromium.org/7690004/diff/6001/chrome/browser/ui/color_choo...
File chrome/browser/ui/color_chooser_dialog.h (right):

http://codereview.chromium.org/7690004/diff/6001/chrome/browser/ui/color_choo...
chrome/browser/ui/color_chooser_dialog.h:15: // Gtk and Mac use ColorChooser.
Windows uses ColorChooserDialog instead
This comment may be:

This class wraps Windows's color chooser dialog and make it singleton.

http://codereview.chromium.org/7690004/diff/6001/chrome/browser/ui/color_choo...
chrome/browser/ui/color_chooser_dialog.h:30: static ColorChooserDialog*
Create(Listener* listener);
It is better to have private ctor since this class has static creator class
method.

http://codereview.chromium.org/7690004/diff/6001/chrome/browser/ui/color_choo...
chrome/browser/ui/color_chooser_dialog.h:35: };
Please put
DISALLOW_COPY_AND_ASSIGN(ColorChooserDialog)

http://codereview.chromium.org/7690004/diff/6001/chrome/browser/ui/views/shel...
File chrome/browser/ui/views/shell_dialogs_win.cc (right):

http://codereview.chromium.org/7690004/diff/6001/chrome/browser/ui/views/shel...
chrome/browser/ui/views/shell_dialogs_win.cc:948: uint8 r = (color >> 16) &
0xFF;
Could you use static_cast<uint8>(color >> 16) instead of (color >> 16) & 0xFF.

To prevent warning from MSVC. It warns narrow casting.

http://codereview.chromium.org/7690004/diff/6001/chrome/browser/ui/views/shel...
chrome/browser/ui/views/shell_dialogs_win.cc:955: uint8 b = (color >> 16) &
0xFF;
It is better to use BYTE GetBValue(COLORREF), GetGValue(COLORREF) and
GetRValue(COLORREF) macros even if MSDN documentation describes packing scheme
of COLORREF.

http://codereview.chromium.org/7690004/diff/6001/chrome/browser/ui/views/shel...
chrome/browser/ui/views/shell_dialogs_win.cc:967: virtual bool IsRunning(HWND
owning_hwnd) const;
Please put "OVERRIDE" before semi-colon for override methods.

http://codereview.chromium.org/7690004/diff/6001/chrome/browser/ui/views/shel...
chrome/browser/ui/views/shell_dialogs_win.cc:991: COLORREF custom_colors_[16];
Where does number 16 come from? We may want to have description of this "16".

Expand Messages | Collapse Messages