Use BubbleWindow for ChromeOS windows/dialogs.

Use BubbleWindow for ChromeOS windows/dialogs.

BUG=chromium-os:7216
TEST=Verify all windows/dialogs use new bubble design, e.g. JS dialog, page info window, certificate view, extension install confirmation, save as dialog etc.

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

[zel, 2010-10-06 18:08:01]

http://codereview.chromium.org/3538012/diff/1/11
File chrome/browser/views/frame/browser_view.cc (right):

http://codereview.chromium.org/3538012/diff/1/11#newcode1174
chrome/browser/views/frame/browser_view.cc:1174:
views::Window::CreateChromeWindow(GetNativeHandle(), gfx::Rect(),
couldn't we change this factory method to return BubbleWindow on ChromeOS and
Window in any other case? This would greately reduce number of changes needed
here.

[xiyuan, 2010-10-06 19:49:59]

http://codereview.chromium.org/3538012/diff/1/11
File chrome/browser/views/frame/browser_view.cc (right):

http://codereview.chromium.org/3538012/diff/1/11#newcode1174
chrome/browser/views/frame/browser_view.cc:1174:
views::Window::CreateChromeWindow(GetNativeHandle(), gfx::Rect(),
On 2010/10/06 18:08:01, zel wrote:
> couldn't we change this factory method to return BubbleWindow on ChromeOS and
> Window in any other case? This would greately reduce number of changes needed
> here.

In chrome/browser/chromeos/login/login_html_dialog.cc LoginHtmlDialog::Show uses
both CreateChromeWindow and BubbleWindow based on its style. Can I disregard
this?

CreateChromeWindow is in views and does not have any dependency on chrome,
However, BubbleWindow has a dependency on chrome's bubble image resource. It is
a bit trickier than just replace the implementation.

[zel, 2010-10-06 19:56:22]

On 2010/10/06 19:49:59, xiyuan wrote:
> http://codereview.chromium.org/3538012/diff/1/11
> File chrome/browser/views/frame/browser_view.cc (right):
> 
> http://codereview.chromium.org/3538012/diff/1/11#newcode1174
> chrome/browser/views/frame/browser_view.cc:1174:
> views::Window::CreateChromeWindow(GetNativeHandle(), gfx::Rect(),
> On 2010/10/06 18:08:01, zel wrote:
> > couldn't we change this factory method to return BubbleWindow on ChromeOS
and
> > Window in any other case? This would greately reduce number of changes
needed
> > here.
> 
> In chrome/browser/chromeos/login/login_html_dialog.cc LoginHtmlDialog::Show
uses
> both CreateChromeWindow and BubbleWindow based on its style. Can I disregard
> this?
> 
> CreateChromeWindow is in views and does not have any dependency on chrome,
> However, BubbleWindow has a dependency on chrome's bubble image resource. It
is
> a bit trickier than just replace the implementation.

OK, how about we create another factory method in some UI helper class that is
part of browser already? I hope we can make this being one-two line change in
these files instead of 7 lines :)

[xiyuan, 2010-10-06 19:59:44]

On 2010/10/06 19:56:22, zel wrote:
> On 2010/10/06 19:49:59, xiyuan wrote:
> > http://codereview.chromium.org/3538012/diff/1/11
> > File chrome/browser/views/frame/browser_view.cc (right):
> > 
> > http://codereview.chromium.org/3538012/diff/1/11#newcode1174
> > chrome/browser/views/frame/browser_view.cc:1174:
> > views::Window::CreateChromeWindow(GetNativeHandle(), gfx::Rect(),
> > On 2010/10/06 18:08:01, zel wrote:
> > > couldn't we change this factory method to return BubbleWindow on ChromeOS
> and
> > > Window in any other case? This would greately reduce number of changes
> needed
> > > here.
> > 
> > In chrome/browser/chromeos/login/login_html_dialog.cc LoginHtmlDialog::Show
> uses
> > both CreateChromeWindow and BubbleWindow based on its style. Can I disregard
> > this?
> > 
> > CreateChromeWindow is in views and does not have any dependency on chrome,
> > However, BubbleWindow has a dependency on chrome's bubble image resource. It
> is
> > a bit trickier than just replace the implementation.
> 
> OK, how about we create another factory method in some UI helper class that is
> part of browser already? I hope we can make this being one-two line change in
> these files instead of 7 lines :)

That should work. I am thinking that the window frame probably should be part of
a theme. Let me see if I can find anything to leverage. But we still need to
have a big change to hook things up.

[xiyuan, 2010-10-06 21:36:13]

CL update.

Added a factory function browser::CreateWindow and wire the windows/dialogs
through it.

[zel, 2010-10-06 21:43:58]

LGTM

[tfarina, 2010-10-06 22:08:55]

http://codereview.chromium.org/3538012/diff/10001/11015
File chrome/browser/views/modal_dialog_delegate.cc (right):

http://codereview.chromium.org/3538012/diff/10001/11015#newcode1
chrome/browser/views/modal_dialog_delegate.cc:1: // Copyright (c) 2006-2008 The
Chromium Authors. All rights reserved.
2010

http://codereview.chromium.org/3538012/diff/10001/11015#newcode8
chrome/browser/views/modal_dialog_delegate.cc:8: #include "base/logging.h"
sort alphabetical.

http://codereview.chromium.org/3538012/diff/10001/11018
File chrome/browser/views/window.h (right):

http://codereview.chromium.org/3538012/diff/10001/11018#newcode9
chrome/browser/views/window.h:9: #include "views/window/window.h"
Can't you forward declare for Window and WindowDelegate and Rect? And include
gfx/native_widget_types.h for NativeWindow?

[xiyuan, 2010-10-07 19:06:49]

http://codereview.chromium.org/3538012/diff/10001/11015
File chrome/browser/views/modal_dialog_delegate.cc (right):

http://codereview.chromium.org/3538012/diff/10001/11015#newcode1
chrome/browser/views/modal_dialog_delegate.cc:1: // Copyright (c) 2006-2008 The
Chromium Authors. All rights reserved.
On 2010/10/06 22:08:55, tfarina wrote:
> 2010

Done.

http://codereview.chromium.org/3538012/diff/10001/11015#newcode8
chrome/browser/views/modal_dialog_delegate.cc:8: #include "base/logging.h"
On 2010/10/06 22:08:55, tfarina wrote:
> sort alphabetical.

Done.

http://codereview.chromium.org/3538012/diff/10001/11018
File chrome/browser/views/window.h (right):

http://codereview.chromium.org/3538012/diff/10001/11018#newcode9
chrome/browser/views/window.h:9: #include "views/window/window.h"
On 2010/10/06 22:08:55, tfarina wrote:
> Can't you forward declare for Window and WindowDelegate and Rect? And include
> gfx/native_widget_types.h for NativeWindow?

The code that calls CreateWindow would need to call views::Window methods (e.g.
Show). So if I don't include it here, then the caller would need to include the
that file. So which way do you prefer?

[tfarina, 2010-10-07 19:13:01]

http://codereview.chromium.org/3538012/diff/10001/11018
File chrome/browser/views/window.h (right):

http://codereview.chromium.org/3538012/diff/10001/11018#newcode9
chrome/browser/views/window.h:9: #include "views/window/window.h"
On 2010/10/07 19:06:49, xiyuan wrote:
> On 2010/10/06 22:08:55, tfarina wrote:
> > Can't you forward declare for Window and WindowDelegate and Rect? And
include
> > gfx/native_widget_types.h for NativeWindow?
> 
> The code that calls CreateWindow would need to call views::Window methods
(e.g.
> Show). So if I don't include it here, then the caller would need to include
the
> that file. So which way do you prefer?

I'd prefer that the caller include the window.h. Forward declaring helps us
speeding up the compilation.

LGTM with that.

[xiyuan, 2010-10-07 19:32:56]

On 2010/10/07 19:13:01, tfarina wrote:
> http://codereview.chromium.org/3538012/diff/10001/11018
> File chrome/browser/views/window.h (right):
> 
> http://codereview.chromium.org/3538012/diff/10001/11018#newcode9
> chrome/browser/views/window.h:9: #include "views/window/window.h"
> On 2010/10/07 19:06:49, xiyuan wrote:
> > On 2010/10/06 22:08:55, tfarina wrote:
> > > Can't you forward declare for Window and WindowDelegate and Rect? And
> include
> > > gfx/native_widget_types.h for NativeWindow?
> > 
> > The code that calls CreateWindow would need to call views::Window methods
> (e.g.
> > Show). So if I don't include it here, then the caller would need to include
> the
> > that file. So which way do you prefer?
> 
> I'd prefer that the caller include the window.h. Forward declaring helps us
> speeding up the compilation.
> 
> LGTM with that.

Done.