Promote bubble_window_style to more general dialog_style.

Promote bubble_window_style to more general dialog_style.

In order to theme certificate viewer html dialog with a different style on
chromeos, we need to promote bubble_window_style to more general dialog_style.
This way, DialogStyle can be used as a parameter for function ShowHTMLDialog.
However, only chromeos builds will use this parameter, others just ignore it.

BUG=102511
TEST=None

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=110731

[bshe, 2011-11-10 20:43:30]

Hi Rob.

Could u please take a look at this CL? I basically moved the bubble_window_style
to dialog_style. Not sure about the name dialog_style though. Especially when
using it here:
explicit BubbleWindowViews(DialogStyle style)

This constructor creates a window, not necessarily a dialog. It is not very
nature for me to use DialogStyle as its argument. 

Thanks,

Biao

[flackr, 2011-11-10 21:06:46]

I agree the naming seems a bit awkward but the BubbleWindow is being used to
display a dialog.

http://codereview.chromium.org/8469006/diff/1001/chrome/browser/chromeos/fram...
File chrome/browser/chromeos/frame/bubble_window_views.cc (left):

http://codereview.chromium.org/8469006/diff/1001/chrome/browser/chromeos/fram...
chrome/browser/chromeos/frame/bubble_window_views.cc:10: 
nit: Keep empty line here.

http://codereview.chromium.org/8469006/diff/1001/chrome/browser/ui/dialog_sty...
File chrome/browser/ui/dialog_style.cc (right):

http://codereview.chromium.org/8469006/diff/1001/chrome/browser/ui/dialog_sty...
chrome/browser/ui/dialog_style.cc:8: const SkColor kBubbleWindowBackgroundColor
= SK_ColorWHITE;
This is specific to BubbleWindow, can you put this constant in bubble_window.cc
and remove this file?

[bshe, 2011-11-14 18:58:03]

Done

http://codereview.chromium.org/8469006/diff/1001/chrome/browser/chromeos/fram...
File chrome/browser/chromeos/frame/bubble_window_views.cc (left):

http://codereview.chromium.org/8469006/diff/1001/chrome/browser/chromeos/fram...
chrome/browser/chromeos/frame/bubble_window_views.cc:10: 
On 2011/11/10 21:06:46, flackr wrote:
> nit: Keep empty line here.

Done.

http://codereview.chromium.org/8469006/diff/1001/chrome/browser/ui/dialog_sty...
File chrome/browser/ui/dialog_style.cc (right):

http://codereview.chromium.org/8469006/diff/1001/chrome/browser/ui/dialog_sty...
chrome/browser/ui/dialog_style.cc:8: const SkColor kBubbleWindowBackgroundColor
= SK_ColorWHITE;
There are more than one place using this background color. I tried to put it in
the bubble_window.h file, but we need to delete bubble_window.cc file
eventually. It is hard to initialize the background color without
bubble_window.cc files. So I put this style back to bubble_window_style.h/cc
file. Not sure if there is any better way to do it.

On 2011/11/10 21:06:46, flackr wrote:
> This is specific to BubbleWindow, can you put this constant in
bubble_window.cc
> and remove this file?

Done.

[flackr, 2011-11-15 02:09:18]

http://codereview.chromium.org/8469006/diff/4005/chrome/browser/chromeos/fram...
File chrome/browser/chromeos/frame/bubble_window_style.h (right):

http://codereview.chromium.org/8469006/diff/4005/chrome/browser/chromeos/fram...
chrome/browser/chromeos/frame/bubble_window_style.h:13: extern const SkColor
kBubbleWindowBackgroundColor;
No indentation.

http://codereview.chromium.org/8469006/diff/4005/chrome/browser/ui/dialog_sty...
File chrome/browser/ui/dialog_style.h (right):

http://codereview.chromium.org/8469006/diff/4005/chrome/browser/ui/dialog_sty...
chrome/browser/ui/dialog_style.h:19: // TODO(bshe) We probably need to use this
style for certificate viewer
s/TODO(bshe)/TODO(bshe):

http://codereview.chromium.org/8469006/diff/4005/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (left):

http://codereview.chromium.org/8469006/diff/4005/chrome/chrome_browser.gypi#o...
chrome/chrome_browser.gypi:467:
'browser/chromeos/frame/browser_frame_view_chromeos.h',
Did you mean to remove these files?

[bshe, 2011-11-15 16:05:33]

Done. Thanks for your time!

http://codereview.chromium.org/8469006/diff/4005/chrome/browser/chromeos/fram...
File chrome/browser/chromeos/frame/bubble_window_style.h (right):

http://codereview.chromium.org/8469006/diff/4005/chrome/browser/chromeos/fram...
chrome/browser/chromeos/frame/bubble_window_style.h:13: extern const SkColor
kBubbleWindowBackgroundColor;
On 2011/11/15 02:09:18, flackr wrote:
> No indentation.

Done.

http://codereview.chromium.org/8469006/diff/4005/chrome/browser/ui/dialog_sty...
File chrome/browser/ui/dialog_style.h (right):

http://codereview.chromium.org/8469006/diff/4005/chrome/browser/ui/dialog_sty...
chrome/browser/ui/dialog_style.h:19: // TODO(bshe) We probably need to use this
style for certificate viewer
On 2011/11/15 02:09:18, flackr wrote:
> s/TODO(bshe)/TODO(bshe):

Done.

http://codereview.chromium.org/8469006/diff/4005/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (left):

http://codereview.chromium.org/8469006/diff/4005/chrome/chrome_browser.gypi#o...
chrome/chrome_browser.gypi:467:
'browser/chromeos/frame/browser_frame_view_chromeos.h',
On 2011/11/15 02:09:18, flackr wrote:
> Did you mean to remove these files?

I dont mean to remvoe these files. I thought someone else did. But after a quick
check of git log. It seems I removed it by accident.

[flackr, 2011-11-15 16:45:18]

LGTM

[bshe, 2011-11-15 22:35:39]

Hi Emmanuel.

I made some changes to bubble_window_style.* files, which you are the original
author, and some related files. 

The reason is we want to get rid of the content padding for certificate viewer
in chromeos, as requested by glen in issue 102511. 

The easiest way we figure out to achieve the goal is to add a generic
DialogStyle which may only have STYLE_GENERIC for other OS's (for now), but will
have extra styles available for OS_CHROMEOS. For detail, please take a look at
code review issue 8469006.

Could you please take a look at this CL? Thanks.

[Emmanuel Saint-loubert-Bié, 2011-11-15 23:03:10]

Looks good to me! Please have sky@ review for OWNERS

[bshe, 2011-11-15 23:25:55]

On 2011/11/15 23:03:10, Emmanuel Saint-loubert wrote:
> Looks good to me! Please have sky@ review for OWNERS

Thanks! I have added sky to the reviewer.

sky: Could you please take a look at it? Thanks!

[sky, 2011-11-16 04:31:04]

http://codereview.chromium.org/8469006/diff/14001/chrome/browser/ui/dialog_st...
File chrome/browser/ui/dialog_style.h (right):

http://codereview.chromium.org/8469006/diff/14001/chrome/browser/ui/dialog_st...
chrome/browser/ui/dialog_style.h:5: #ifndef CHROME_BROWSER_UI_DIALOG_STYLE_H_
Is this going to be views specific? If so, it should live in
chrome/browser/ui/views.

[flackr, 2011-11-16 13:53:22]

http://codereview.chromium.org/8469006/diff/14001/chrome/browser/ui/dialog_st...
File chrome/browser/ui/dialog_style.h (right):

http://codereview.chromium.org/8469006/diff/14001/chrome/browser/ui/dialog_st...
chrome/browser/ui/dialog_style.h:5: #ifndef CHROME_BROWSER_UI_DIALOG_STYLE_H_
On 2011/11/16 04:31:04, sky wrote:
> Is this going to be views specific? If so, it should live in
> chrome/browser/ui/views.

Not exactly, the purpose of this change is to enable passing a generic dialog
style through browser::ShowHtmlDialog for crbug.com/102511 (See
http://codereview.chromium.org/8479042/). Ultimately we may be implementing the
styles not related to window decorations in other platforms for consistency.

[sky, 2011-11-16 16:35:46]

Thanks for clarifying. LGTM