Popup changes

Popup changes

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

[brettw, 2009-07-06 14:46:27]

Random style comments. Scott should check the Views stuff.

http://codereview.chromium.org/155075/diff/1069/64
File chrome/browser/gtk/browser_window_gtk.h (right):

http://codereview.chromium.org/155075/diff/1069/64#newcode34
Line 34: #ifdef LINUX2
This file should probably include build_config.h

http://codereview.chromium.org/155075/diff/1069/64#newcode133
Line 133: 
You probably want to delete this addition.

http://codereview.chromium.org/155075/diff/1069/65
File chrome/browser/gtk/panel_controller.cc (right):

http://codereview.chromium.org/155075/diff/1069/65#newcode61
Line 61: }
Put a blank line before this and "  // namespace" after

http://codereview.chromium.org/155075/diff/1069/61
File chrome/browser/gtk/panel_controller.h (right):

http://codereview.chromium.org/155075/diff/1069/61#newcode99
Line 99: DISALLOW_EVIL_CONSTRUCTORS(PanelController);
...EVIL... has been changed to DISALLOW_COPY_AND_ASSIGN

http://codereview.chromium.org/155075/diff/1069/61#newcode102
Line 102: #endif // CHROME_BROWSER_PANEL_CONTROLLER_H_
Two spaces before the //

[sky, 2009-07-06 15:38:18]

The approach you've taken looks good. I mostly have style nits. Adding more
documentation would be good too:)

http://codereview.chromium.org/155075/diff/1069/63
File chrome/browser/gtk/browser_window_gtk.cc (right):

http://codereview.chromium.org/155075/diff/1069/63#newcode60
Line 60: #include "chrome/browser/gtk/panel_controller.h"
nit: your include should be before the other LINUX2 include.

http://codereview.chromium.org/155075/diff/1069/63#newcode1049
Line 1049: GTK_WIDGET(window_), bounds_.width(), bounds_.height());
The docs for this say "Sets the minimum size of a widget". Are you sure you want
to set this to the minimum size?

http://codereview.chromium.org/155075/diff/1069/64
File chrome/browser/gtk/browser_window_gtk.h (right):

http://codereview.chromium.org/155075/diff/1069/64#newcode302
Line 302: PanelController* panel_controller_;
nit: description

http://codereview.chromium.org/155075/diff/1069/65
File chrome/browser/gtk/panel_controller.cc (right):

http://codereview.chromium.org/155075/diff/1069/65#newcode67
Line 67: expanded_(true) {
I would recommend initializing all the fields here. For example, I think it's
possible now for you to test mouse_down_ without it ever have been set.

http://codereview.chromium.org/155075/diff/1069/65#newcode97
Line 97: if (title_content_->title_label()) {
do you really need this check? It looks like title_label() is always non-null.

http://codereview.chromium.org/155075/diff/1069/65#newcode152
Line 152: if (abs(abs_location.x() - mouse_down_abs_x_) >= kDragThreshold ||
use views::View::ExceededDragThreshold

http://codereview.chromium.org/155075/diff/1069/65#newcode161
Line 161: msg.set_param(2, abs_location.y() - mouse_down_offset_y_);
you sure you want these coordinates? It looks like you're subtracting screen
coordinates from view coordinates.

http://codereview.chromium.org/155075/diff/1069/65#newcode193
Line 193: : panel_controller_(panel_controller) {
nit: this line should be indented 4 spaces.

http://codereview.chromium.org/155075/diff/1069/65#newcode201
Line 201: title_label_ = new views::Label(L"", *title_font);
nit: L"" -> std::wstring()

http://codereview.chromium.org/155075/diff/1069/61
File chrome/browser/gtk/panel_controller.h (right):

http://codereview.chromium.org/155075/diff/1069/61#newcode5
Line 5: #ifndef CHROME_BROWSER_PANEL_CONTROLLER_H_
This file should be in browser/views, and the guard should match that.

http://codereview.chromium.org/155075/diff/1069/61#newcode16
Line 16: class MouseEvent;
nit: sort these.

http://codereview.chromium.org/155075/diff/1069/61#newcode22
Line 22: class PanelController : public views::ButtonListener {
nit: description of the class.

http://codereview.chromium.org/155075/diff/1069/61#newcode54
Line 54: DISALLOW_EVIL_CONSTRUCTORS(TitleContentView);
DISALLOW_COPY_AND_ASSIGN

http://codereview.chromium.org/155075/diff/1069/60
File views/widget/widget_gtk.cc (right):

http://codereview.chromium.org/155075/diff/1069/60#newcode442
Line 442: gtk_widget_set_size_request(widget_, bounds.width(), bounds.height());
again, are you sure you want to set the minimum size? Last I messed with this
code the gtk_widget_size_allocate call above had the effect of setting the
default size the window manager gets.

[DaveMoore, 2009-07-06 18:24:32]

http://codereview.chromium.org/155075/diff/1069/63
File chrome/browser/gtk/browser_window_gtk.cc (right):

http://codereview.chromium.org/155075/diff/1069/63#newcode60
Line 60: #include "chrome/browser/gtk/panel_controller.h"
On 2009/07/06 15:38:18, sky wrote:
> nit: your include should be before the other LINUX2 include.

Done.

http://codereview.chromium.org/155075/diff/1069/63#newcode1049
Line 1049: GTK_WIDGET(window_), bounds_.width(), bounds_.height());
Popups needs their size set explicitly for the WM to handle things right. I've
limited the calls to that case.
On 2009/07/06 15:38:18, sky wrote:
> The docs for this say "Sets the minimum size of a widget". Are you sure you
want
> to set this to the minimum size?

http://codereview.chromium.org/155075/diff/1069/64
File chrome/browser/gtk/browser_window_gtk.h (right):

http://codereview.chromium.org/155075/diff/1069/64#newcode34
Line 34: #ifdef LINUX2
On 2009/07/06 14:46:28, brettw wrote:
> This file should probably include build_config.h

Done.

http://codereview.chromium.org/155075/diff/1069/64#newcode133
Line 133: 
On 2009/07/06 14:46:28, brettw wrote:
> You probably want to delete this addition.

Done.

http://codereview.chromium.org/155075/diff/1069/64#newcode302
Line 302: PanelController* panel_controller_;
On 2009/07/06 15:38:18, sky wrote:
> nit: description

Done.

http://codereview.chromium.org/155075/diff/1069/65
File chrome/browser/gtk/panel_controller.cc (right):

http://codereview.chromium.org/155075/diff/1069/65#newcode67
Line 67: expanded_(true) {
There's some initialization that needs to happen in a particular order. I think
it's easier to read if the title stuff is handled below.

On 2009/07/06 15:38:18, sky wrote:
> I would recommend initializing all the fields here. For example, I think it's
> possible now for you to test mouse_down_ without it ever have been set.

http://codereview.chromium.org/155075/diff/1069/65#newcode97
Line 97: if (title_content_->title_label()) {
On 2009/07/06 15:38:18, sky wrote:
> do you really need this check? It looks like title_label() is always non-null.

Done.

http://codereview.chromium.org/155075/diff/1069/65#newcode152
Line 152: if (abs(abs_location.x() - mouse_down_abs_x_) >= kDragThreshold ||
On 2009/07/06 15:38:18, sky wrote:
> use views::View::ExceededDragThreshold

Done.

http://codereview.chromium.org/155075/diff/1069/65#newcode161
Line 161: msg.set_param(2, abs_location.y() - mouse_down_offset_y_);
These are the coordinates that the WM is expecting
On 2009/07/06 15:38:18, sky wrote:
> you sure you want these coordinates? It looks like you're subtracting screen
> coordinates from view coordinates.

http://codereview.chromium.org/155075/diff/1069/65#newcode193
Line 193: : panel_controller_(panel_controller) {
On 2009/07/06 15:38:18, sky wrote:
> nit: this line should be indented 4 spaces.

Done.

http://codereview.chromium.org/155075/diff/1069/65#newcode201
Line 201: title_label_ = new views::Label(L"", *title_font);
On 2009/07/06 15:38:18, sky wrote:
> nit: L"" -> std::wstring()

Done.

http://codereview.chromium.org/155075/diff/1069/61
File chrome/browser/gtk/panel_controller.h (right):

http://codereview.chromium.org/155075/diff/1069/61#newcode5
Line 5: #ifndef CHROME_BROWSER_PANEL_CONTROLLER_H_
On 2009/07/06 15:38:18, sky wrote:
> This file should be in browser/views, and the guard should match that.

Done.

http://codereview.chromium.org/155075/diff/1069/61#newcode16
Line 16: class MouseEvent;
On 2009/07/06 15:38:18, sky wrote:
> nit: sort these.

Done.

http://codereview.chromium.org/155075/diff/1069/61#newcode22
Line 22: class PanelController : public views::ButtonListener {
On 2009/07/06 15:38:18, sky wrote:
> nit: description of the class.

Done.

http://codereview.chromium.org/155075/diff/1069/61#newcode54
Line 54: DISALLOW_EVIL_CONSTRUCTORS(TitleContentView);
On 2009/07/06 15:38:18, sky wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

http://codereview.chromium.org/155075/diff/1069/61#newcode99
Line 99: DISALLOW_EVIL_CONSTRUCTORS(PanelController);
On 2009/07/06 14:46:28, brettw wrote:
> ...EVIL... has been changed to DISALLOW_COPY_AND_ASSIGN

Done.

http://codereview.chromium.org/155075/diff/1069/61#newcode102
Line 102: #endif // CHROME_BROWSER_PANEL_CONTROLLER_H_
On 2009/07/06 14:46:28, brettw wrote:
> Two spaces before the //

Done.

[sky, 2009-07-06 19:46:34]

http://codereview.chromium.org/155075/diff/1108/1115
File chrome/browser/views/panel_controller.cc (right):

http://codereview.chromium.org/155075/diff/1108/1115#newcode65
Line 65: expanded_(true) {
you need to at least add mouse_down_(false), but I would add all here to be
sure.

http://codereview.chromium.org/155075/diff/1108/1116
File chrome/browser/views/panel_controller.h (right):

http://codereview.chromium.org/155075/diff/1108/1116#newcode22
Line 22: // Controls interactions with the WM for popops / panels.
nit: popops -> popups

http://codereview.chromium.org/155075/diff/1108/1116#newcode34
Line 34: virtual void Close();
Does Close need to be virtual?

http://codereview.chromium.org/155075/diff/1108/1109
File views/widget/widget_gtk.cc (right):

http://codereview.chromium.org/155075/diff/1108/1109#newcode109
Line 109: root_view_->SetBounds(0, 0, bounds.width(), bounds.height());
Do you really need this? The root view should eventually be size, so I'm curious
why you need it here.

http://codereview.chromium.org/155075/diff/1108/1109#newcode442
Line 442: gtk_widget_set_size_request(widget_, bounds.width(), bounds.height());
This still concerns me. If you remove it, what happens?

[DaveMoore, 2009-07-06 21:18:10]

http://codereview.chromium.org/155075/diff/1108/1115
File chrome/browser/views/panel_controller.cc (right):

http://codereview.chromium.org/155075/diff/1108/1115#newcode65
Line 65: expanded_(true) {
On 2009/07/06 19:46:34, sky wrote:
> you need to at least add mouse_down_(false), but I would add all here to be
> sure.

Done.

http://codereview.chromium.org/155075/diff/1108/1116
File chrome/browser/views/panel_controller.h (right):

http://codereview.chromium.org/155075/diff/1108/1116#newcode22
Line 22: // Controls interactions with the WM for popops / panels.
On 2009/07/06 19:46:34, sky wrote:
> nit: popops -> popups

Done.

http://codereview.chromium.org/155075/diff/1108/1116#newcode34
Line 34: virtual void Close();
On 2009/07/06 19:46:34, sky wrote:
> Does Close need to be virtual?

Done.

http://codereview.chromium.org/155075/diff/1108/1109
File views/widget/widget_gtk.cc (right):

http://codereview.chromium.org/155075/diff/1108/1109#newcode109
Line 109: root_view_->SetBounds(0, 0, bounds.width(), bounds.height());
Removed
On 2009/07/06 19:46:34, sky wrote:
> Do you really need this? The root view should eventually be size, so I'm
curious
> why you need it here.

http://codereview.chromium.org/155075/diff/1108/1109#newcode442
Line 442: gtk_widget_set_size_request(widget_, bounds.width(), bounds.height());
Removed
On 2009/07/06 19:46:34, sky wrote:
> This still concerns me. If you remove it, what happens?

[sky, 2009-07-06 21:23:50]

LGTM

[brettw, 2009-07-06 21:43:39]

LGTM2