Implement ShellWindow in Views.

Implement ShellWindow in Views.

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

[Mihai Parparita -not on Chrome, 2012-01-06 23:26:49]

http://codereview.chromium.org/9030017/diff/1/chrome/browser/ui/views/extensi...
File chrome/browser/ui/views/extensions/shell_window_views.cc (right):

http://codereview.chromium.org/9030017/diff/1/chrome/browser/ui/views/extensi...
chrome/browser/ui/views/extensions/shell_window_views.cc:46: // TODO(jeremya):
destroy when window closes?
I think you'll want to override DeleteDelegate and delete the the
ShellWindowViews instance in that case (similar to the way the GTK version does
this in OnMainWindowDeleteEvent)

[jeremya, 2012-01-12 01:46:04]

http://codereview.chromium.org/9030017/diff/1/chrome/browser/ui/views/extensi...
File chrome/browser/ui/views/extensions/shell_window_views.cc (right):

http://codereview.chromium.org/9030017/diff/1/chrome/browser/ui/views/extensi...
chrome/browser/ui/views/extensions/shell_window_views.cc:46: // TODO(jeremya):
destroy when window closes?
On 2012/01/06 23:26:50, Mihai Parparita wrote:
> I think you'll want to override DeleteDelegate and delete the the
> ShellWindowViews instance in that case (similar to the way the GTK version
does
> this in OnMainWindowDeleteEvent)

Done.

[Mihai Parparita -not on Chrome, 2012-01-12 02:09:09]

Can you also enable the tests in platform_app_browsertest.cc that are currently
disabled (and send tryjobs for them)?

http://codereview.chromium.org/9030017/diff/4002/chrome/browser/ui/views/exte...
File chrome/browser/ui/views/extensions/shell_window_views.cc (right):

http://codereview.chromium.org/9030017/diff/4002/chrome/browser/ui/views/exte...
chrome/browser/ui/views/extensions/shell_window_views.cc:11: #include
"ui/gfx/canvas_skia.h"
This seems unused.

http://codereview.chromium.org/9030017/diff/4002/chrome/browser/ui/views/exte...
chrome/browser/ui/views/extensions/shell_window_views.cc:15: void
GetContentsMask(const gfx::Rect& rect, gfx::Path* path);
I think this can be deleted.

http://codereview.chromium.org/9030017/diff/4002/chrome/browser/ui/views/exte...
chrome/browser/ui/views/extensions/shell_window_views.cc:36: delete this;
Will this close the window?

http://codereview.chromium.org/9030017/diff/4002/chrome/browser/ui/views/exte...
chrome/browser/ui/views/extensions/shell_window_views.cc:58: const
views::Widget* ShellWindowViews::GetWidget() const {
Add a newline between functions..

http://codereview.chromium.org/9030017/diff/4002/chrome/browser/ui/views/exte...
File chrome/browser/ui/views/extensions/shell_window_views.h (right):

http://codereview.chromium.org/9030017/diff/4002/chrome/browser/ui/views/exte...
chrome/browser/ui/views/extensions/shell_window_views.h:1: // Copyright (c) 2011
The Chromium Authors. All rights reserved.
Presubmit check will most likely yell about non-2012 copyright dates.

http://codereview.chromium.org/9030017/diff/4002/chrome/browser/ui/views/exte...
chrome/browser/ui/views/extensions/shell_window_views.h:35: views::Widget*
window_;
Seems like this should be a scoped_ptr

[jeremya, 2012-01-12 18:28:46]

Tries underway.

http://codereview.chromium.org/9030017/diff/4002/chrome/browser/ui/views/exte...
File chrome/browser/ui/views/extensions/shell_window_views.cc (right):

http://codereview.chromium.org/9030017/diff/4002/chrome/browser/ui/views/exte...
chrome/browser/ui/views/extensions/shell_window_views.cc:11: #include
"ui/gfx/canvas_skia.h"
On 2012/01/12 02:09:09, Mihai Parparita wrote:
> This seems unused.

Done.

http://codereview.chromium.org/9030017/diff/4002/chrome/browser/ui/views/exte...
chrome/browser/ui/views/extensions/shell_window_views.cc:15: void
GetContentsMask(const gfx::Rect& rect, gfx::Path* path);
On 2012/01/12 02:09:09, Mihai Parparita wrote:
> I think this can be deleted.

Done.

http://codereview.chromium.org/9030017/diff/4002/chrome/browser/ui/views/exte...
chrome/browser/ui/views/extensions/shell_window_views.cc:36: delete this;
On 2012/01/12 02:09:09, Mihai Parparita wrote:
> Will this close the window?

Nope. Good catch.

http://codereview.chromium.org/9030017/diff/4002/chrome/browser/ui/views/exte...
chrome/browser/ui/views/extensions/shell_window_views.cc:58: const
views::Widget* ShellWindowViews::GetWidget() const {
On 2012/01/12 02:09:09, Mihai Parparita wrote:
> Add a newline between functions..

Done.

http://codereview.chromium.org/9030017/diff/4002/chrome/browser/ui/views/exte...
File chrome/browser/ui/views/extensions/shell_window_views.h (right):

http://codereview.chromium.org/9030017/diff/4002/chrome/browser/ui/views/exte...
chrome/browser/ui/views/extensions/shell_window_views.h:1: // Copyright (c) 2011
The Chromium Authors. All rights reserved.
On 2012/01/12 02:09:09, Mihai Parparita wrote:
> Presubmit check will most likely yell about non-2012 copyright dates.

My local one didn't... but done anyway :)

http://codereview.chromium.org/9030017/diff/4002/chrome/browser/ui/views/exte...
chrome/browser/ui/views/extensions/shell_window_views.h:35: views::Widget*
window_;
On 2012/01/12 02:09:09, Mihai Parparita wrote:
> Seems like this should be a scoped_ptr

It shouldn't, because the default ownership model of widgets is
NATIVE_WIDGET_OWNS_WIDGET, so the native widget should destroy it when it goes.

[Mihai Parparita -not on Chrome, 2012-01-12 22:00:07]

LGTM, but you'll also need someone from chrome/browser/ui/OWNERS to review.

http://codereview.chromium.org/9030017/diff/6008/chrome/browser/sync/engine/s...
File chrome/browser/sync/engine/syncer.cc (left):

http://codereview.chromium.org/9030017/diff/6008/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncer.cc:147:
session->mutable_status_controller()->set_last_download_updates_result(
Did you really mean to change this file? Merge error?

[jeremya, 2012-01-12 23:30:46]

Ben- could you have a look?

Thanks :)

http://codereview.chromium.org/9030017/diff/6008/chrome/browser/sync/engine/s...
File chrome/browser/sync/engine/syncer.cc (left):

http://codereview.chromium.org/9030017/diff/6008/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncer.cc:147:
session->mutable_status_controller()->set_last_download_updates_result(
On 2012/01/12 22:00:07, Mihai Parparita wrote:
> Did you really mean to change this file? Merge error?

Oops, that was a windows file handle brokenness thing on 'git checkout'. Fixed.

[Mihai Parparita -not on Chrome, 2012-01-12 23:37:08]

Still LGTM

http://codereview.chromium.org/9030017/diff/8006/chrome/browser/extensions/pl...
File chrome/browser/extensions/platform_app_browsertest.cc (right):

http://codereview.chromium.org/9030017/diff/8006/chrome/browser/extensions/pl...
chrome/browser/extensions/platform_app_browsertest.cc:53:
ui_test_utils::WindowedNotificationObserver popup_observer(
Calling this app_loaded_observer is probably more appropriate than
popup_observer.

http://codereview.chromium.org/9030017/diff/8006/chrome/browser/extensions/pl...
chrome/browser/extensions/platform_app_browsertest.cc:75: popup_observer.Wait();
Indentation seems off here.

[jeremya, 2012-01-12 23:38:45]

http://codereview.chromium.org/9030017/diff/8006/chrome/browser/extensions/pl...
File chrome/browser/extensions/platform_app_browsertest.cc (right):

http://codereview.chromium.org/9030017/diff/8006/chrome/browser/extensions/pl...
chrome/browser/extensions/platform_app_browsertest.cc:53:
ui_test_utils::WindowedNotificationObserver popup_observer(
On 2012/01/12 23:37:08, Mihai Parparita wrote:
> Calling this app_loaded_observer is probably more appropriate than
> popup_observer.

Done.

http://codereview.chromium.org/9030017/diff/8006/chrome/browser/extensions/pl...
chrome/browser/extensions/platform_app_browsertest.cc:75: popup_observer.Wait();
On 2012/01/12 23:37:08, Mihai Parparita wrote:
> Indentation seems off here.

Done.

[Ben Goodger (Google), 2012-01-12 23:42:38]

http://codereview.chromium.org/9030017/diff/8006/chrome/browser/extensions/pl...
File chrome/browser/extensions/platform_app_browsertest.cc (right):

http://codereview.chromium.org/9030017/diff/8006/chrome/browser/extensions/pl...
chrome/browser/extensions/platform_app_browsertest.cc:116: #if
defined(TOOLKIT_GTK) | defined(TOOLKIT_VIEWS)
Convention is to do double pipe || or.

http://codereview.chromium.org/9030017/diff/8006/chrome/browser/ui/views/exte...
File chrome/browser/ui/views/extensions/shell_window_views.h (right):

http://codereview.chromium.org/9030017/diff/8006/chrome/browser/ui/views/exte...
chrome/browser/ui/views/extensions/shell_window_views.h:25: views::View*
GetContentsView() OVERRIDE;
make all these virtual

http://codereview.chromium.org/9030017/diff/8006/chrome/browser/ui/views/exte...
chrome/browser/ui/views/extensions/shell_window_views.h:36: };
DISALLOW_COPY_AND_ASSIGN(ShellWindowViews);

[jeremya, 2012-01-12 23:48:51]

http://codereview.chromium.org/9030017/diff/8006/chrome/browser/extensions/pl...
File chrome/browser/extensions/platform_app_browsertest.cc (right):

http://codereview.chromium.org/9030017/diff/8006/chrome/browser/extensions/pl...
chrome/browser/extensions/platform_app_browsertest.cc:116: #if
defined(TOOLKIT_GTK) | defined(TOOLKIT_VIEWS)
On 2012/01/12 23:42:38, Ben Goodger (Google) wrote:
> Convention is to do double pipe || or.

Done.

http://codereview.chromium.org/9030017/diff/8006/chrome/browser/ui/views/exte...
File chrome/browser/ui/views/extensions/shell_window_views.h (right):

http://codereview.chromium.org/9030017/diff/8006/chrome/browser/ui/views/exte...
chrome/browser/ui/views/extensions/shell_window_views.h:25: views::View*
GetContentsView() OVERRIDE;
On 2012/01/12 23:42:38, Ben Goodger (Google) wrote:
> make all these virtual

Done.

http://codereview.chromium.org/9030017/diff/8006/chrome/browser/ui/views/exte...
chrome/browser/ui/views/extensions/shell_window_views.h:36: };
On 2012/01/12 23:42:38, Ben Goodger (Google) wrote:
> DISALLOW_COPY_AND_ASSIGN(ShellWindowViews);

Done.

[Ben Goodger (Google), 2012-01-12 23:53:57]

http://codereview.chromium.org/9030017/diff/8013/chrome/browser/ui/extensions...
File chrome/browser/ui/extensions/shell_window.cc (right):

http://codereview.chromium.org/9030017/diff/8013/chrome/browser/ui/extensions...
chrome/browser/ui/extensions/shell_window.cc:49: #if defined(TOOLKIT_GTK) |
defined(TOOLKIT_VIEWS)
double pipe

http://codereview.chromium.org/9030017/diff/8013/chrome/browser/ui/views/exte...
File chrome/browser/ui/views/extensions/shell_window_views.cc (right):

http://codereview.chromium.org/9030017/diff/8013/chrome/browser/ui/views/exte...
chrome/browser/ui/views/extensions/shell_window_views.cc:17: params.can_activate
= true;
This should be implied by TYPE_WINDOW.

http://codereview.chromium.org/9030017/diff/8013/chrome/browser/ui/views/exte...
chrome/browser/ui/views/extensions/shell_window_views.cc:18: params.parent =
NULL;
default is NULL

http://codereview.chromium.org/9030017/diff/8013/chrome/browser/ui/views/exte...
chrome/browser/ui/views/extensions/shell_window_views.cc:22: params.top_level =
true;
I am not sure what this parameter does but I am pretty sure you shouldn't be
setting it.

[jeremya, 2012-01-12 23:58:51]

http://codereview.chromium.org/9030017/diff/8013/chrome/browser/ui/extensions...
File chrome/browser/ui/extensions/shell_window.cc (right):

http://codereview.chromium.org/9030017/diff/8013/chrome/browser/ui/extensions...
chrome/browser/ui/extensions/shell_window.cc:49: #if defined(TOOLKIT_GTK) |
defined(TOOLKIT_VIEWS)
On 2012/01/12 23:53:57, Ben Goodger (Google) wrote:
> double pipe

Done.

http://codereview.chromium.org/9030017/diff/8013/chrome/browser/ui/views/exte...
File chrome/browser/ui/views/extensions/shell_window_views.cc (right):

http://codereview.chromium.org/9030017/diff/8013/chrome/browser/ui/views/exte...
chrome/browser/ui/views/extensions/shell_window_views.cc:17: params.can_activate
= true;
On 2012/01/12 23:53:57, Ben Goodger (Google) wrote:
> This should be implied by TYPE_WINDOW.

Done.

http://codereview.chromium.org/9030017/diff/8013/chrome/browser/ui/views/exte...
chrome/browser/ui/views/extensions/shell_window_views.cc:18: params.parent =
NULL;
On 2012/01/12 23:53:57, Ben Goodger (Google) wrote:
> default is NULL

Done.

http://codereview.chromium.org/9030017/diff/8013/chrome/browser/ui/views/exte...
chrome/browser/ui/views/extensions/shell_window_views.cc:22: params.top_level =
true;
On 2012/01/12 23:53:57, Ben Goodger (Google) wrote:
> I am not sure what this parameter does but I am pretty sure you shouldn't be
> setting it.

Done.

[Ben Goodger (Google), 2012-01-13 00:01:22]

lgtm

[commit-bot: I haz the power, 2012-01-13 00:13:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jeremya@chromium.org/9030017/6024

[commit-bot: I haz the power, 2012-01-13 03:00:50]

Change committed as 117591