WebUI TaskManager: Add method to set minimum window size on HTMLDialog.

WebUI TaskManager: Add method to set minimum window size on HTMLDialog. 

Previously, the saved window size is used as the "minimum" size of window on HTMLDialog (cf. http://crbug.com/115403). This CL make the saved size as default size and adds the GetMinimumDialogSize() method in HTMLDialogUI to set minimum size of window separately.

This CL also enables the viwes::Widget::SaveWindowPlacement() on TaskManagerDialog by adding HtmlDialogView::GetWindowName() and HtmlDialogUIDelegate::GetDialogName() virtual functions. And this CL disables to save the window placement on TaskManagerDialog on views, since views::Widget has same feature which is enabled by this CL.

BUG=115403
TEST=manual on Windows, Linux with gtk and Linux with chromiumos=1

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

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

[yoshiki, 2012-03-01 10:24:44]

Could you take a look?

[sky, 2012-03-01 17:01:56]

https://chromiumcodereview.appspot.com/9569001/diff/1/chrome/browser/ui/views...
File chrome/browser/ui/views/html_dialog_view.cc (right):

https://chromiumcodereview.appspot.com/9569001/diff/1/chrome/browser/ui/views...
chrome/browser/ui/views/html_dialog_view.cc:210: if (delegate_)
You sure you don't want to set this to NULL before you invoke Close()?

https://chromiumcodereview.appspot.com/9569001/diff/1/chrome/browser/ui/webui...
File chrome/browser/ui/webui/html_dialog_ui.h (right):

https://chromiumcodereview.appspot.com/9569001/diff/1/chrome/browser/ui/webui...
chrome/browser/ui/webui/html_dialog_ui.h:44: // Retuens the dialog's name
identifier. Used to identify this dialog for
nit: retuens -> returns

https://chromiumcodereview.appspot.com/9569001/diff/1/ui/views/widget/widget.cc
File ui/views/widget/widget.cc (right):

https://chromiumcodereview.appspot.com/9569001/diff/1/ui/views/widget/widget....
ui/views/widget/widget.cc:452: void Widget::SetSizeAndCenterize(const gfx::Size&
size) {
The name of this should match the method you're calling in on the native widget:
CenterWindow.

[sail, 2012-03-01 18:14:59]

LGTM
cocoa/* looks good!

http://codereview.chromium.org/9569001/diff/1/chrome/browser/ui/cocoa/html_di...
File chrome/browser/ui/cocoa/html_dialog_window_controller.mm (right):

http://codereview.chromium.org/9569001/diff/1/chrome/browser/ui/cocoa/html_di...
chrome/browser/ui/cocoa/html_dialog_window_controller.mm:192: if (delegate_) {
no braces for single line statements
same above

[yoshiki, 2012-03-02 06:51:10]

Fixed reviews. PTAL.

https://chromiumcodereview.appspot.com/9569001/diff/1/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/html_dialog_window_controller.mm (right):

https://chromiumcodereview.appspot.com/9569001/diff/1/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/html_dialog_window_controller.mm:192: if (delegate_) {
On 2012/03/01 18:14:59, sail wrote:
> no braces for single line statements
> same above

Done.

https://chromiumcodereview.appspot.com/9569001/diff/1/chrome/browser/ui/views...
File chrome/browser/ui/views/html_dialog_view.cc (right):

https://chromiumcodereview.appspot.com/9569001/diff/1/chrome/browser/ui/views...
chrome/browser/ui/views/html_dialog_view.cc:210: if (delegate_)
Yes, this delegate_ is necessary to save the window placement on closing.

On 2012/03/01 17:01:57, sky wrote:
> You sure you don't want to set this to NULL before you invoke Close()?

https://chromiumcodereview.appspot.com/9569001/diff/1/chrome/browser/ui/webui...
File chrome/browser/ui/webui/html_dialog_ui.h (right):

https://chromiumcodereview.appspot.com/9569001/diff/1/chrome/browser/ui/webui...
chrome/browser/ui/webui/html_dialog_ui.h:44: // Retuens the dialog's name
identifier. Used to identify this dialog for
On 2012/03/01 17:01:57, sky wrote:
> nit: retuens -> returns

Done.

https://chromiumcodereview.appspot.com/9569001/diff/1/ui/views/widget/widget.cc
File ui/views/widget/widget.cc (right):

https://chromiumcodereview.appspot.com/9569001/diff/1/ui/views/widget/widget....
ui/views/widget/widget.cc:452: void Widget::SetSizeAndCenterize(const gfx::Size&
size) {
On 2012/03/01 17:01:57, sky wrote:
> The name of this should match the method you're calling in on the native
widget:
> CenterWindow.

Done.

[mazda, 2012-03-02 07:52:23]

http://codereview.chromium.org/9569001/diff/7001/chrome/browser/ui/webui/task...
File chrome/browser/ui/webui/task_manager_dialog.cc (right):

http://codereview.chromium.org/9569001/diff/7001/chrome/browser/ui/webui/task...
chrome/browser/ui/webui/task_manager_dialog.cc:90: size->SetSize(640, 480);
Please make constant variables.

http://codereview.chromium.org/9569001/diff/7001/ui/views/widget/widget.h
File ui/views/widget/widget.h (right):

http://codereview.chromium.org/9569001/diff/7001/ui/views/widget/widget.h#new...
ui/views/widget/widget.h:317: void CenterWindow(const gfx::Size& size);
Please add comment.

[yoshiki, 2012-03-02 10:07:57]

Thakns, PTAL

http://codereview.chromium.org/9569001/diff/7001/chrome/browser/ui/webui/task...
File chrome/browser/ui/webui/task_manager_dialog.cc (right):

http://codereview.chromium.org/9569001/diff/7001/chrome/browser/ui/webui/task...
chrome/browser/ui/webui/task_manager_dialog.cc:90: size->SetSize(640, 480);
On 2012/03/02 07:52:23, mazda wrote:
> Please make constant variables.

Done.

http://codereview.chromium.org/9569001/diff/7001/ui/views/widget/widget.h
File ui/views/widget/widget.h (right):

http://codereview.chromium.org/9569001/diff/7001/ui/views/widget/widget.h#new...
ui/views/widget/widget.h:317: void CenterWindow(const gfx::Size& size);
On 2012/03/02 07:52:23, mazda wrote:
> Please add comment.

Done.

[yoshiki, 2012-03-05 06:38:34]

sky, estade: Could you take a look of following files and approve to check-in?

sky:
chrome/browser/ui/views/html_dialog_view.h
chrome/browser/ui/views/html_dialog_view.cc
ui/views/widget/widget.h
ui/views/widget/widget.cc

estade:
chrome/browser/ui/webui/html_dialog_ui.h
chrome/browser/ui/webui/html_dialog_ui.cc
chrome/browser/ui/webui/task_manager_dialog.cc
chrome/browser/ui/gtk/html_dialog_gtk.h
chrome/browser/ui/gtk/html_dialog_gtk.cc

[sky, 2012-03-05 16:15:06]

LGTM

http://codereview.chromium.org/9569001/diff/7012/ui/views/widget/widget.h
File ui/views/widget/widget.h (right):

http://codereview.chromium.org/9569001/diff/7012/ui/views/widget/widget.h#new...
ui/views/widget/widget.h:318: // Sizes the window to the specified size and
centerizes it.
centerizes -> centers

[yoshiki, 2012-03-07 04:53:46]

estade: PTAL of following files for approve?

chrome/browser/ui/webui/html_dialog_ui.h
chrome/browser/ui/webui/html_dialog_ui.cc
chrome/browser/ui/webui/task_manager_dialog.cc
chrome/browser/ui/gtk/html_dialog_gtk.h
chrome/browser/ui/gtk/html_dialog_gtk.cc

[yoshiki, 2012-03-09 03:32:24]

estade: ping?

[yoshiki, 2012-03-12 03:45:59]

erg, jhawkins: Could you take a look of following files?

- erg: chrome/browser/ui/gtk/*
- jhawkins: chrome/browser/ui/webui/*

[yoshiki, 2012-03-12 03:46:14]

erg, jhawkins: Could you take a look of following files?

- erg: chrome/browser/ui/gtk/*
- jhawkins: chrome/browser/ui/webui/*

[Elliot Glaysher, 2012-03-12 21:16:31]

gtk lgtm

http://codereview.chromium.org/9569001/diff/20001/chrome/browser/ui/gtk/html_...
File chrome/browser/ui/gtk/html_dialog_gtk.cc (right):

http://codereview.chromium.org/9569001/diff/20001/chrome/browser/ui/gtk/html_...
chrome/browser/ui/gtk/html_dialog_gtk.cc:265: minimum_dialog_size.height());
This is fine as long as your code still works if this fails.
gtk_widget_set_size_request() (and pretty much every other window management
methods) are advisory; the window manager can completely ignore them.

[yoshiki, 2012-03-13 05:20:15]

On 2012/03/12 21:16:31, Elliot Glaysher wrote:
http://codereview.chromium.org/9569001/diff/20001/chrome/browser/ui/gtk/html_...
> chrome/browser/ui/gtk/html_dialog_gtk.cc:265: minimum_dialog_size.height());
> This is fine as long as your code still works if this fails.
> gtk_widget_set_size_request() (and pretty much every other window management
> methods) are advisory; the window manager can completely ignore them.

I don't know that, but that's no problem as you said. Thank you for the comment.

[yoshiki, 2012-03-14 11:04:08]

jhawkins: ping. PTAL of chrome/browser/ui/webui/*.

[James Hawkins, 2012-03-14 17:39:25]

http://codereview.chromium.org/9569001/diff/20001/chrome/browser/ui/webui/htm...
File chrome/browser/ui/webui/html_dialog_ui.cc (right):

http://codereview.chromium.org/9569001/diff/20001/chrome/browser/ui/webui/htm...
chrome/browser/ui/webui/html_dialog_ui.cc:110: std::string
HtmlDialogUIDelegate::GetDialogName() const {
Ordering of the methods needs to match the header.

http://codereview.chromium.org/9569001/diff/20001/chrome/browser/ui/webui/tas...
File chrome/browser/ui/webui/task_manager_dialog.cc (right):

http://codereview.chromium.org/9569001/diff/20001/chrome/browser/ui/webui/tas...
chrome/browser/ui/webui/task_manager_dialog.cc:32: #define
MINIMUM_TASKMANAGER_WIDTH 640
Use constants for values like this.

[yoshiki, 2012-03-15 08:35:07]

jhawkins: Thanks, PTAL.

http://codereview.chromium.org/9569001/diff/20001/chrome/browser/ui/webui/htm...
File chrome/browser/ui/webui/html_dialog_ui.cc (right):

http://codereview.chromium.org/9569001/diff/20001/chrome/browser/ui/webui/htm...
chrome/browser/ui/webui/html_dialog_ui.cc:110: std::string
HtmlDialogUIDelegate::GetDialogName() const {
On 2012/03/14 17:39:25, James Hawkins wrote:
> Ordering of the methods needs to match the header.

Done.

http://codereview.chromium.org/9569001/diff/20001/chrome/browser/ui/webui/tas...
File chrome/browser/ui/webui/task_manager_dialog.cc (right):

http://codereview.chromium.org/9569001/diff/20001/chrome/browser/ui/webui/tas...
chrome/browser/ui/webui/task_manager_dialog.cc:32: #define
MINIMUM_TASKMANAGER_WIDTH 640
On 2012/03/14 17:39:25, James Hawkins wrote:
> Use constants for values like this.

Done.

[James Hawkins, 2012-03-15 16:34:35]

http://codereview.chromium.org/9569001/diff/33013/chrome/browser/ui/webui/htm...
File chrome/browser/ui/webui/html_dialog_ui.cc (right):

http://codereview.chromium.org/9569001/diff/33013/chrome/browser/ui/webui/htm...
chrome/browser/ui/webui/html_dialog_ui.cc:115: std::string
HtmlDialogUIDelegate::GetDialogName() const {
Ordering still doesn't match the header.

http://codereview.chromium.org/9569001/diff/33013/chrome/browser/ui/webui/tas...
File chrome/browser/ui/webui/task_manager_dialog.cc (right):

http://codereview.chromium.org/9569001/diff/33013/chrome/browser/ui/webui/tas...
chrome/browser/ui/webui/task_manager_dialog.cc:33: const int
kMinimumTaskManagerWidth = 640;
nit: Either comment or add Px to the name that this is in pixels.

[yoshiki, 2012-03-16 05:55:10]

jhawkins: Thanks, PTAL.

https://chromiumcodereview.appspot.com/9569001/diff/33013/chrome/browser/ui/w...
File chrome/browser/ui/webui/html_dialog_ui.cc (right):

https://chromiumcodereview.appspot.com/9569001/diff/33013/chrome/browser/ui/w...
chrome/browser/ui/webui/html_dialog_ui.cc:115: std::string
HtmlDialogUIDelegate::GetDialogName() const {
Sorry, I took a mistake. I've fixed.

On 2012/03/15 16:34:36, James Hawkins wrote:
> Ordering still doesn't match the header.

https://chromiumcodereview.appspot.com/9569001/diff/33013/chrome/browser/ui/w...
File chrome/browser/ui/webui/task_manager_dialog.cc (right):

https://chromiumcodereview.appspot.com/9569001/diff/33013/chrome/browser/ui/w...
chrome/browser/ui/webui/task_manager_dialog.cc:33: const int
kMinimumTaskManagerWidth = 640;
On 2012/03/15 16:34:36, James Hawkins wrote:
> nit: Either comment or add Px to the name that this is in pixels.

Done.

[James Hawkins, 2012-03-18 03:00:04]

LGTM with nit.

https://chromiumcodereview.appspot.com/9569001/diff/34020/chrome/browser/ui/w...
File chrome/browser/ui/webui/task_manager_dialog.cc (right):

https://chromiumcodereview.appspot.com/9569001/diff/34020/chrome/browser/ui/w...
chrome/browser/ui/webui/task_manager_dialog.cc:37: }  // namespace
nit: Blank line before the brace (to match the top line).

[mazda, 2012-03-19 07:39:27]

lgtm

[commit-bot: I haz the power, 2012-03-19 08:44:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoshiki@chromium.org/9569001/41001

[commit-bot: I haz the power, 2012-03-19 09:14:08]

Try job failure for 9569001-41001 (retry) on linux_rel for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-03-19 14:05:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoshiki@chromium.org/9569001/37004

[commit-bot: I haz the power, 2012-03-19 15:12:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoshiki@chromium.org/9569001/42012

[commit-bot: I haz the power, 2012-03-19 17:31:30]

Try job failure for 9569001-42012 (retry) on win_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-03-20 07:09:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoshiki@chromium.org/9569001/42012

[commit-bot: I haz the power, 2012-03-20 09:09:48]

Change committed as 127660

[sky, 2012-03-22 16:41:48]

LGTM

[commit-bot: I haz the power, 2012-03-22 23:08:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoshiki@chromium.org/9569001/58001

[commit-bot: I haz the power, 2012-03-23 01:27:52]

Change committed as 128372