Change the idle logout dialog over to use views.

Change the idle logout dialog over to use views.
To keep the idle logout dialog with the rest of the popups on ChromeOS and following the policy of not using WebUI unless the UI is in a tab, we've moved the implementation from based on HtmlDialogUI to a views based dialog.

Reviews requested,
sky: Primary review
jhawkins: OWNER's review for removed code from ui/webui
zelidrag: OWNER's review for removed code from ui/webui/chromeos and resources/chromeos

R=zelidrag@chromium.org,jhawkins@chromium.org,sky@chromium.org
BUG=chromium-os:27545, chromium-os:27542
TEST=Tested with --enable-kiosk-mode flags.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=126814

[James Hawkins, 2012-03-10 03:35:50]

Remove LGTM

[sky, 2012-03-11 21:57:47]

https://chromiumcodereview.appspot.com/9665031/diff/1/chrome/browser/chromeos...
File chrome/browser/chromeos/ui/idle_logout_dialog.cc (right):

https://chromiumcodereview.appspot.com/9665031/diff/1/chrome/browser/chromeos...
chrome/browser/chromeos/ui/idle_logout_dialog.cc:7: #include <string>
Do you really need these includes in both the header and .cc?

https://chromiumcodereview.appspot.com/9665031/diff/1/chrome/browser/chromeos...
chrome/browser/chromeos/ui/idle_logout_dialog.cc:58: delete g_instance;
It's a lot safter to put this in DeleteDelegate. That way you don't have to
worry if a future code path allows closing via some other mechanism.

https://chromiumcodereview.appspot.com/9665031/diff/1/chrome/browser/chromeos...
chrome/browser/chromeos/ui/idle_logout_dialog.cc:65:
IdleLogoutDialog::IdleLogoutDialog() {
Order of methods should match header.

https://chromiumcodereview.appspot.com/9665031/diff/1/chrome/browser/chromeos...
chrome/browser/chromeos/ui/idle_logout_dialog.cc:65:
IdleLogoutDialog::IdleLogoutDialog() {
Have the constructor initialize warning_label_ and restart_label_ to NULL.

https://chromiumcodereview.appspot.com/9665031/diff/1/chrome/browser/chromeos...
chrome/browser/chromeos/ui/idle_logout_dialog.cc:71:
chromeos::KioskModeHelper::Get()->Initialize(
What happens if Close is called this returns?

https://chromiumcodereview.appspot.com/9665031/diff/1/chrome/browser/chromeos...
chrome/browser/chromeos/ui/idle_logout_dialog.cc:114: NOTIMPLEMENTED();
Do you even intend to implement this for non-aura? If not, nuke the ifdefs and
exclude this file on non-aura chromeos.

https://chromiumcodereview.appspot.com/9665031/diff/1/chrome/browser/chromeos...
chrome/browser/chromeos/ui/idle_logout_dialog.cc:131: base::Time::Now() +
kFudgeTime;
Why the kFudgeTime?

https://chromiumcodereview.appspot.com/9665031/diff/1/chrome/browser/chromeos...
chrome/browser/chromeos/ui/idle_logout_dialog.cc:168: return
gfx::Size(kIdleLogoutDialogWidth, kIdleLogoutDialogHeight);
Don't you want to make sure it can fit the content?

https://chromiumcodereview.appspot.com/9665031/diff/1/chrome/browser/chromeos...
File chrome/browser/chromeos/ui/idle_logout_dialog.h (right):

https://chromiumcodereview.appspot.com/9665031/diff/1/chrome/browser/chromeos...
chrome/browser/chromeos/ui/idle_logout_dialog.h:19: class TimeDelta;
don't indent this.

https://chromiumcodereview.appspot.com/9665031/diff/1/chrome/browser/chromeos...
chrome/browser/chromeos/ui/idle_logout_dialog.h:31: //  virtual bool CanResize()
const  OVERRIDE;
If you don't want to override this, remove it.
Also, you have two spaces in a number of places before OVERRIDE.

https://chromiumcodereview.appspot.com/9665031/diff/1/chrome/browser/chromeos...
chrome/browser/chromeos/ui/idle_logout_dialog.h:38: 
nit: only one empty line.

[tfarina, 2012-03-11 23:42:58]

http://codereview.chromium.org/9665031/diff/1/chrome/browser/chromeos/ui/idle...
File chrome/browser/chromeos/ui/idle_logout_dialog.cc (right):

http://codereview.chromium.org/9665031/diff/1/chrome/browser/chromeos/ui/idle...
chrome/browser/chromeos/ui/idle_logout_dialog.cc:31: using views::GridLayout;
nit: I'd remove this and just have views::GridLayout below, it isn't too much to
type anyway.

http://codereview.chromium.org/9665031/diff/1/chrome/browser/chromeos/ui/idle...
chrome/browser/chromeos/ui/idle_logout_dialog.cc:41: const base::TimeDelta
kFudgeTime = base::TimeDelta::FromMilliseconds(100);
I think this adds a static initializer? See
http://codereview.chromium.org/9635018/ for an example.

http://codereview.chromium.org/9665031/diff/1/chrome/browser/chromeos/ui/idle...
chrome/browser/chromeos/ui/idle_logout_dialog.cc:82:
ResourceBundle::GetSharedInstance().GetFont(ResourceBundle::BaseFont));
nit: Make a temp var, like:

ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();

and use it here and elsewhere.

http://codereview.chromium.org/9665031/diff/1/chrome/browser/chromeos/ui/idle...
chrome/browser/chromeos/ui/idle_logout_dialog.cc:86: restart_label_-> SetFont(
nit: looks like there is an extra space between -> and SetFont

http://codereview.chromium.org/9665031/diff/1/chrome/browser/chromeos/ui/idle...
chrome/browser/chromeos/ui/idle_logout_dialog.cc:88: ResourceBundle::BoldFont));
nit: ui::ResourceBundle::

[rkc, 2012-03-12 22:43:38]

http://codereview.chromium.org/9665031/diff/1/chrome/browser/chromeos/ui/idle...
File chrome/browser/chromeos/ui/idle_logout_dialog.cc (right):

http://codereview.chromium.org/9665031/diff/1/chrome/browser/chromeos/ui/idle...
chrome/browser/chromeos/ui/idle_logout_dialog.cc:7: #include <string>
On 2012/03/11 21:57:47, sky wrote:
> Do you really need these includes in both the header and .cc?

They weren't needed in either, removed.
Done.

http://codereview.chromium.org/9665031/diff/1/chrome/browser/chromeos/ui/idle...
chrome/browser/chromeos/ui/idle_logout_dialog.cc:31: using views::GridLayout;
On 2012/03/11 23:42:58, tfarina wrote:
> nit: I'd remove this and just have views::GridLayout below, it isn't too much
to
> type anyway.

Done.

http://codereview.chromium.org/9665031/diff/1/chrome/browser/chromeos/ui/idle...
chrome/browser/chromeos/ui/idle_logout_dialog.cc:41: const base::TimeDelta
kFudgeTime = base::TimeDelta::FromMilliseconds(100);
On 2012/03/11 23:42:58, tfarina wrote:
> I think this adds a static initializer? See
> http://codereview.chromium.org/9635018/ for an example.

Removed this completely.

http://codereview.chromium.org/9665031/diff/1/chrome/browser/chromeos/ui/idle...
chrome/browser/chromeos/ui/idle_logout_dialog.cc:58: delete g_instance;
On 2012/03/11 21:57:47, sky wrote:
> It's a lot safter to put this in DeleteDelegate. That way you don't have to
> worry if a future code path allows closing via some other mechanism.

Done.

http://codereview.chromium.org/9665031/diff/1/chrome/browser/chromeos/ui/idle...
chrome/browser/chromeos/ui/idle_logout_dialog.cc:65:
IdleLogoutDialog::IdleLogoutDialog() {
On 2012/03/11 21:57:47, sky wrote:
> Order of methods should match header.

Done.

http://codereview.chromium.org/9665031/diff/1/chrome/browser/chromeos/ui/idle...
chrome/browser/chromeos/ui/idle_logout_dialog.cc:65:
IdleLogoutDialog::IdleLogoutDialog() {
On 2012/03/11 21:57:47, sky wrote:
> Have the constructor initialize warning_label_ and restart_label_ to NULL.

Done.

http://codereview.chromium.org/9665031/diff/1/chrome/browser/chromeos/ui/idle...
chrome/browser/chromeos/ui/idle_logout_dialog.cc:71:
chromeos::KioskModeHelper::Get()->Initialize(
On 2012/03/11 21:57:47, sky wrote:
> What happens if Close is called this returns?

Bad things. Fixed it so it's handled correctly.
Done.

http://codereview.chromium.org/9665031/diff/1/chrome/browser/chromeos/ui/idle...
chrome/browser/chromeos/ui/idle_logout_dialog.cc:82:
ResourceBundle::GetSharedInstance().GetFont(ResourceBundle::BaseFont));
On 2012/03/11 23:42:58, tfarina wrote:
> nit: Make a temp var, like:
> 
> ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();
> 
> and use it here and elsewhere.

Done.

http://codereview.chromium.org/9665031/diff/1/chrome/browser/chromeos/ui/idle...
chrome/browser/chromeos/ui/idle_logout_dialog.cc:86: restart_label_-> SetFont(
On 2012/03/11 23:42:58, tfarina wrote:
> nit: looks like there is an extra space between -> and SetFont

Done.

http://codereview.chromium.org/9665031/diff/1/chrome/browser/chromeos/ui/idle...
chrome/browser/chromeos/ui/idle_logout_dialog.cc:88: ResourceBundle::BoldFont));
On 2012/03/11 23:42:58, tfarina wrote:
> nit: ui::ResourceBundle::

Done.

http://codereview.chromium.org/9665031/diff/1/chrome/browser/chromeos/ui/idle...
chrome/browser/chromeos/ui/idle_logout_dialog.cc:114: NOTIMPLEMENTED();
On 2012/03/11 21:57:47, sky wrote:
> Do you even intend to implement this for non-aura? If not, nuke the ifdefs and
> exclude this file on non-aura chromeos.

Code from this file is called from other places.
I could put ifdefs around all those locations and around the #include - but
since this is the 'only' place that we're doing anything aura specific, it
seemed simpler to put the #ifdefs here and get the non-aura bots to pass.

http://codereview.chromium.org/9665031/diff/1/chrome/browser/chromeos/ui/idle...
chrome/browser/chromeos/ui/idle_logout_dialog.cc:131: base::Time::Now() +
kFudgeTime;
On 2012/03/11 21:57:47, sky wrote:
> Why the kFudgeTime?

InSeconds doesn't actually round the milli/microseconds off to the nearest
second. Since we don't get Time::Now at 'exactly' -1 second from last time, we
can display the incorrect time to the user (usually -1 from what it should
show).

Adding a fudge value is a simple way to ensure that our internal processing time
is adjusted for when updating the display.

Looking at it again, it seems better if we do the ms->s conversion here rounding
correctly in the process.

Done.

http://codereview.chromium.org/9665031/diff/1/chrome/browser/chromeos/ui/idle...
chrome/browser/chromeos/ui/idle_logout_dialog.cc:168: return
gfx::Size(kIdleLogoutDialogWidth, kIdleLogoutDialogHeight);
On 2012/03/11 21:57:47, sky wrote:
> Don't you want to make sure it can fit the content?

Thought that is what 'preferred size' did? Set it to that size 'if' it fits the
content?

http://codereview.chromium.org/9665031/diff/1/chrome/browser/chromeos/ui/idle...
File chrome/browser/chromeos/ui/idle_logout_dialog.h (right):

http://codereview.chromium.org/9665031/diff/1/chrome/browser/chromeos/ui/idle...
chrome/browser/chromeos/ui/idle_logout_dialog.h:19: class TimeDelta;
On 2012/03/11 21:57:47, sky wrote:
> don't indent this.

Done.

http://codereview.chromium.org/9665031/diff/1/chrome/browser/chromeos/ui/idle...
chrome/browser/chromeos/ui/idle_logout_dialog.h:31: //  virtual bool CanResize()
const  OVERRIDE;
On 2012/03/11 21:57:47, sky wrote:
> If you don't want to override this, remove it.
> Also, you have two spaces in a number of places before OVERRIDE.

Ouch, missed this one in cleanup. Removed.
Done.

http://codereview.chromium.org/9665031/diff/1/chrome/browser/chromeos/ui/idle...
chrome/browser/chromeos/ui/idle_logout_dialog.h:38: 
On 2012/03/11 21:57:47, sky wrote:
> nit: only one empty line.

Done.

[sky, 2012-03-12 23:33:37]

http://codereview.chromium.org/9665031/diff/1/chrome/browser/chromeos/ui/idle...
File chrome/browser/chromeos/ui/idle_logout_dialog.cc (right):

http://codereview.chromium.org/9665031/diff/1/chrome/browser/chromeos/ui/idle...
chrome/browser/chromeos/ui/idle_logout_dialog.cc:168: return
gfx::Size(kIdleLogoutDialogWidth, kIdleLogoutDialogHeight);
On 2012/03/12 22:43:38, Rahul Chaturvedi wrote:
> On 2012/03/11 21:57:47, sky wrote:
> > Don't you want to make sure it can fit the content?
> 
> Thought that is what 'preferred size' did? Set it to that size 'if' it fits
the
> content?

The only way it knows is to ask for the preferred size, which you're overriding
here.

http://codereview.chromium.org/9665031/diff/6001/chrome/browser/chromeos/ui/i...
File chrome/browser/chromeos/ui/idle_logout_dialog.cc (right):

http://codereview.chromium.org/9665031/diff/6001/chrome/browser/chromeos/ui/i...
chrome/browser/chromeos/ui/idle_logout_dialog.cc:105:
IdleLogoutDialog::IdleLogoutDialog() : restart_label_(NULL),
nit: wrap to next line and indent 4.

http://codereview.chromium.org/9665031/diff/6001/chrome/browser/chromeos/ui/i...
chrome/browser/chromeos/ui/idle_logout_dialog.cc:113: if (*instance_holder)
Is instance_holder_ leaked?

http://codereview.chromium.org/9665031/diff/6001/chrome/browser/chromeos/ui/i...
chrome/browser/chromeos/ui/idle_logout_dialog.cc:157:
views::Widget::CreateWindowWithParent(this, root_window);
Why are you parenting this to the root window?

http://codereview.chromium.org/9665031/diff/6001/chrome/browser/chromeos/ui/i...
File chrome/browser/chromeos/ui/idle_logout_dialog.h (right):

http://codereview.chromium.org/9665031/diff/6001/chrome/browser/chromeos/ui/i...
chrome/browser/chromeos/ui/idle_logout_dialog.h:37: void set_closed() {
*instance_holder_ = NULL; }
Don't expose this in the public section.

[rkc, 2012-03-12 23:57:51]

http://codereview.chromium.org/9665031/diff/1/chrome/browser/chromeos/ui/idle...
File chrome/browser/chromeos/ui/idle_logout_dialog.cc (right):

http://codereview.chromium.org/9665031/diff/1/chrome/browser/chromeos/ui/idle...
chrome/browser/chromeos/ui/idle_logout_dialog.cc:168: return
gfx::Size(kIdleLogoutDialogWidth, kIdleLogoutDialogHeight);
On 2012/03/12 23:33:38, sky wrote:
> On 2012/03/12 22:43:38, Rahul Chaturvedi wrote:
> > On 2012/03/11 21:57:47, sky wrote:
> > > Don't you want to make sure it can fit the content?
> > 
> > Thought that is what 'preferred size' did? Set it to that size 'if' it fits
> the
> > content?
> 
> The only way it knows is to ask for the preferred size, which you're
overriding
> here.
I think the user should keep the size adjusted to the text; we have a certain
amount of white space also around the text  also which makes the dialog look
nicer.

http://codereview.chromium.org/9665031/diff/6001/chrome/browser/chromeos/ui/i...
File chrome/browser/chromeos/ui/idle_logout_dialog.cc (right):

http://codereview.chromium.org/9665031/diff/6001/chrome/browser/chromeos/ui/i...
chrome/browser/chromeos/ui/idle_logout_dialog.cc:105:
IdleLogoutDialog::IdleLogoutDialog() : restart_label_(NULL),
On 2012/03/12 23:33:38, sky wrote:
> nit: wrap to next line and indent 4.

Done.

http://codereview.chromium.org/9665031/diff/6001/chrome/browser/chromeos/ui/i...
chrome/browser/chromeos/ui/idle_logout_dialog.cc:113: if (*instance_holder)
On 2012/03/12 23:33:38, sky wrote:
> Is instance_holder_ leaked?

Was, fixed.

http://codereview.chromium.org/9665031/diff/6001/chrome/browser/chromeos/ui/i...
chrome/browser/chromeos/ui/idle_logout_dialog.cc:157:
views::Widget::CreateWindowWithParent(this, root_window);
On 2012/03/12 23:33:38, sky wrote:
> Why are you parenting this to the root window?

The IdleLogout can come up at any point; whether or not a browser window even
exists at that time. It made the most sense to have it as a child of the root
window.

http://codereview.chromium.org/9665031/diff/6001/chrome/browser/chromeos/ui/i...
File chrome/browser/chromeos/ui/idle_logout_dialog.h (right):

http://codereview.chromium.org/9665031/diff/6001/chrome/browser/chromeos/ui/i...
chrome/browser/chromeos/ui/idle_logout_dialog.h:37: void set_closed() {
*instance_holder_ = NULL; }
On 2012/03/12 23:33:38, sky wrote:
> Don't expose this in the public section.

Done.

[tfarina, 2012-03-13 00:04:31]

http://codereview.chromium.org/9665031/diff/9002/chrome/browser/chromeos/ui/i...
File chrome/browser/chromeos/ui/idle_logout_dialog.cc (right):

http://codereview.chromium.org/9665031/diff/9002/chrome/browser/chromeos/ui/i...
chrome/browser/chromeos/ui/idle_logout_dialog.cc:28: using views::GridLayout;
you can remove this now.

http://codereview.chromium.org/9665031/diff/9002/chrome/browser/chromeos/ui/i...
chrome/browser/chromeos/ui/idle_logout_dialog.cc:100:
GetLayoutManager()->Layout(this);
Do you really need to override this? Doesn't the default View::Layout()
implementation do this for you?

http://codereview.chromium.org/9665031/diff/9002/chrome/browser/chromeos/ui/i...
chrome/browser/chromeos/ui/idle_logout_dialog.cc:112: Init();
nit: Can you call Init() outside of the ctor? Not a big deal though.

http://codereview.chromium.org/9665031/diff/9002/chrome/browser/chromeos/ui/i...
chrome/browser/chromeos/ui/idle_logout_dialog.cc:116: void
CallInit(IdleLogoutDialog** instance_holder) {
Can you move this to the unnamed namespace above?

http://codereview.chromium.org/9665031/diff/9002/chrome/browser/chromeos/ui/i...
chrome/browser/chromeos/ui/idle_logout_dialog.cc:193:
string16(base::Int64ToString16(seconds_left))));
nit: isn't Int64ToString16 already returning a string16? Do you really need to
do string16(...)?

[rkc, 2012-03-13 00:36:38]

http://codereview.chromium.org/9665031/diff/9002/chrome/browser/chromeos/ui/i...
File chrome/browser/chromeos/ui/idle_logout_dialog.cc (right):

http://codereview.chromium.org/9665031/diff/9002/chrome/browser/chromeos/ui/i...
chrome/browser/chromeos/ui/idle_logout_dialog.cc:28: using views::GridLayout;
On 2012/03/13 00:04:32, tfarina wrote:
> you can remove this now.

Done.

http://codereview.chromium.org/9665031/diff/9002/chrome/browser/chromeos/ui/i...
chrome/browser/chromeos/ui/idle_logout_dialog.cc:100:
GetLayoutManager()->Layout(this);
On 2012/03/13 00:04:32, tfarina wrote:
> Do you really need to override this? Doesn't the default View::Layout()
> implementation do this for you?

Nope.

http://codereview.chromium.org/9665031/diff/9002/chrome/browser/chromeos/ui/i...
chrome/browser/chromeos/ui/idle_logout_dialog.cc:112: Init();
On 2012/03/13 00:04:32, tfarina wrote:
> nit: Can you call Init() outside of the ctor? Not a big deal though.
> 

Done.

http://codereview.chromium.org/9665031/diff/9002/chrome/browser/chromeos/ui/i...
chrome/browser/chromeos/ui/idle_logout_dialog.cc:116: void
CallInit(IdleLogoutDialog** instance_holder) {
On 2012/03/13 00:04:32, tfarina wrote:
> Can you move this to the unnamed namespace above?

Made the function a static method; didn't seem to be any good reason why this
function needs to remain outside the class.

http://codereview.chromium.org/9665031/diff/9002/chrome/browser/chromeos/ui/i...
chrome/browser/chromeos/ui/idle_logout_dialog.cc:193:
string16(base::Int64ToString16(seconds_left))));
On 2012/03/13 00:04:32, tfarina wrote:
> nit: isn't Int64ToString16 already returning a string16? Do you really need to
> do string16(...)?

Done.

[sky, 2012-03-13 03:43:54]

http://codereview.chromium.org/9665031/diff/1/chrome/browser/chromeos/ui/idle...
File chrome/browser/chromeos/ui/idle_logout_dialog.cc (right):

http://codereview.chromium.org/9665031/diff/1/chrome/browser/chromeos/ui/idle...
chrome/browser/chromeos/ui/idle_logout_dialog.cc:168: return
gfx::Size(kIdleLogoutDialogWidth, kIdleLogoutDialogHeight);
On 2012/03/12 23:57:51, Rahul Chaturvedi wrote:
> On 2012/03/12 23:33:38, sky wrote:
> > On 2012/03/12 22:43:38, Rahul Chaturvedi wrote:
> > > On 2012/03/11 21:57:47, sky wrote:
> > > > Don't you want to make sure it can fit the content?
> > > 
> > > Thought that is what 'preferred size' did? Set it to that size 'if' it
fits
> > the
> > > content?
> > 
> > The only way it knows is to ask for the preferred size, which you're
> overriding
> > here.
> I think the user should keep the size adjusted to the text; we have a certain
> amount of white space also around the text  also which makes the dialog look
> nicer.

I agree, but this isn't adjusting to the size of the text. You're returning a
hard coded size which may not be enough in some locales.

http://codereview.chromium.org/9665031/diff/6001/chrome/browser/chromeos/ui/i...
File chrome/browser/chromeos/ui/idle_logout_dialog.cc (right):

http://codereview.chromium.org/9665031/diff/6001/chrome/browser/chromeos/ui/i...
chrome/browser/chromeos/ui/idle_logout_dialog.cc:157:
views::Widget::CreateWindowWithParent(this, root_window);
On 2012/03/12 23:57:51, Rahul Chaturvedi wrote:
> On 2012/03/12 23:33:38, sky wrote:
> > Why are you parenting this to the root window?
> 
> The IdleLogout can come up at any point; whether or not a browser window even
> exists at that time. It made the most sense to have it as a child of the root
> window.

I believe that means it could go in from of the lock screen, which isn't right.
I think you want to create it always on top and let the system parent it for
you.

[tfarina, 2012-03-14 00:07:58]

On 2012/03/13 03:43:54, sky wrote:
>
http://codereview.chromium.org/9665031/diff/1/chrome/browser/chromeos/ui/idle...
> File chrome/browser/chromeos/ui/idle_logout_dialog.cc (right):
> 
>
http://codereview.chromium.org/9665031/diff/1/chrome/browser/chromeos/ui/idle...
> chrome/browser/chromeos/ui/idle_logout_dialog.cc:168: return
> gfx::Size(kIdleLogoutDialogWidth, kIdleLogoutDialogHeight);
> On 2012/03/12 23:57:51, Rahul Chaturvedi wrote:
> > On 2012/03/12 23:33:38, sky wrote:
> > > On 2012/03/12 22:43:38, Rahul Chaturvedi wrote:
> > > > On 2012/03/11 21:57:47, sky wrote:
> > > > > Don't you want to make sure it can fit the content?
> > > > 
> > > > Thought that is what 'preferred size' did? Set it to that size 'if' it
> fits
> > > the
> > > > content?
> > > 
> > > The only way it knows is to ask for the preferred size, which you're
> > overriding
> > > here.
> > I think the user should keep the size adjusted to the text; we have a
certain
> > amount of white space also around the text  also which makes the dialog look
> > nicer.
> 
> I agree, but this isn't adjusting to the size of the text. You're returning a
> hard coded size which may not be enough in some locales.
> 
I think Scott is saying that you need to use
views::Widget::GetLocalizedContentsWidth() and
GetLayoutManager()->GetPreferredHeightForWidth().

[tfarina, 2012-03-14 00:15:22]

On Tue, Mar 13, 2012 at 9:07 PM,  <tfarina@chromium.org> wrote:
>> I agree, but this isn't adjusting to the size of the text. You're
>> returning a
>> hard coded size which may not be enough in some locales.
>
>
> I think Scott is saying that you need to use
> views::Widget::GetLocalizedContentsWidth() and
> GetLayoutManager()->GetPreferredHeightForWidth().
>
You also have views::Widget::GetLocalizedContentsSize().


-- 
Thiago

[tfarina, 2012-03-14 16:45:47]

http://codereview.chromium.org/9665031/diff/9003/chrome/browser/chromeos/ui/i...
File chrome/browser/chromeos/ui/idle_logout_dialog.h (right):

http://codereview.chromium.org/9665031/diff/9003/chrome/browser/chromeos/ui/i...
chrome/browser/chromeos/ui/idle_logout_dialog.h:9: #include
"base/memory/scoped_ptr.h"
nit: remove, unused here.

http://codereview.chromium.org/9665031/diff/9003/chrome/browser/chromeos/ui/i...
chrome/browser/chromeos/ui/idle_logout_dialog.h:10: #include "base/string16.h"
nit: this comes from from dialog_delegate.h below, so you can remove this.

http://codereview.chromium.org/9665031/diff/9003/chrome/browser/chromeos/ui/i...
chrome/browser/chromeos/ui/idle_logout_dialog.h:12: #include
"ui/views/controls/label.h"
nit: just forward declare this instead.

http://codereview.chromium.org/9665031/diff/9003/chrome/browser/chromeos/ui/i...
chrome/browser/chromeos/ui/idle_logout_dialog.h:20: class IdleLogoutDialog :
public views::DialogDelegateView {
nit: As many other views dialog, I'd name this as IdleLogoutDialogView and
rename the file_name as well.

http://codereview.chromium.org/9665031/diff/9003/chrome/browser/chromeos/ui/i...
chrome/browser/chromeos/ui/idle_logout_dialog.h:22: static void
ShowIdleLogoutDialog();
nit: This names is redundant. I think it's better just Show() and Close().

Instead of having:

IdleLogoutDialog::ShowIdleLogoutDialog();

Have an:

IdleLogoutDialog::Show();

Same for CloseIdleLogoutDialog();

http://codereview.chromium.org/9665031/diff/9003/chrome/browser/chromeos/ui/i...
chrome/browser/chromeos/ui/idle_logout_dialog.h:52: bool is_closed() { return
NULL == *instance_holder_; }
nit: bool is_closed() const { ... };

[rkc, 2012-03-14 22:35:32]

http://codereview.chromium.org/9665031/diff/1/chrome/browser/chromeos/ui/idle...
File chrome/browser/chromeos/ui/idle_logout_dialog.cc (right):

http://codereview.chromium.org/9665031/diff/1/chrome/browser/chromeos/ui/idle...
chrome/browser/chromeos/ui/idle_logout_dialog.cc:168: return
gfx::Size(kIdleLogoutDialogWidth, kIdleLogoutDialogHeight);
On 2012/03/13 03:43:54, sky wrote:
> On 2012/03/12 23:57:51, Rahul Chaturvedi wrote:
> > On 2012/03/12 23:33:38, sky wrote:
> > > On 2012/03/12 22:43:38, Rahul Chaturvedi wrote:
> > > > On 2012/03/11 21:57:47, sky wrote:
> > > > > Don't you want to make sure it can fit the content?
> > > > 
> > > > Thought that is what 'preferred size' did? Set it to that size 'if' it
> fits
> > > the
> > > > content?
> > > 
> > > The only way it knows is to ask for the preferred size, which you're
> > overriding
> > > here.
> > I think the user should keep the size adjusted to the text; we have a
certain
> > amount of white space also around the text  also which makes the dialog look
> > nicer.
> 
> I agree, but this isn't adjusting to the size of the text. You're returning a
> hard coded size which may not be enough in some locales. 

I realized the main thing we really want is that our text doesn't go too wide
and end up giving us a thin horizontal dialog. So setting just the max width on
the label elements and letting GetPreferredSize handle the rest gives us the
desirable result.

Done.

http://codereview.chromium.org/9665031/diff/6001/chrome/browser/chromeos/ui/i...
File chrome/browser/chromeos/ui/idle_logout_dialog.cc (right):

http://codereview.chromium.org/9665031/diff/6001/chrome/browser/chromeos/ui/i...
chrome/browser/chromeos/ui/idle_logout_dialog.cc:157:
views::Widget::CreateWindowWithParent(this, root_window);
On 2012/03/13 03:43:54, sky wrote:
> On 2012/03/12 23:57:51, Rahul Chaturvedi wrote:
> > On 2012/03/12 23:33:38, sky wrote:
> > > Why are you parenting this to the root window?
> > 
> > The IdleLogout can come up at any point; whether or not a browser window
even
> > exists at that time. It made the most sense to have it as a child of the
root
> > window.
> 
> I believe that means it could go in from of the lock screen, which isn't
right.
> I think you want to create it always on top and let the system parent it for
> you.

Done.

http://codereview.chromium.org/9665031/diff/9003/chrome/browser/chromeos/ui/i...
File chrome/browser/chromeos/ui/idle_logout_dialog.h (right):

http://codereview.chromium.org/9665031/diff/9003/chrome/browser/chromeos/ui/i...
chrome/browser/chromeos/ui/idle_logout_dialog.h:9: #include
"base/memory/scoped_ptr.h"
On 2012/03/14 16:45:47, tfarina wrote:
> nit: remove, unused here.

Done.

http://codereview.chromium.org/9665031/diff/9003/chrome/browser/chromeos/ui/i...
chrome/browser/chromeos/ui/idle_logout_dialog.h:10: #include "base/string16.h"
On 2012/03/14 16:45:47, tfarina wrote:
> nit: this comes from from dialog_delegate.h below, so you can remove this.

Done.

http://codereview.chromium.org/9665031/diff/9003/chrome/browser/chromeos/ui/i...
chrome/browser/chromeos/ui/idle_logout_dialog.h:12: #include
"ui/views/controls/label.h"
On 2012/03/14 16:45:47, tfarina wrote:
> nit: just forward declare this instead.

Done.

http://codereview.chromium.org/9665031/diff/9003/chrome/browser/chromeos/ui/i...
chrome/browser/chromeos/ui/idle_logout_dialog.h:20: class IdleLogoutDialog :
public views::DialogDelegateView {
On 2012/03/14 16:45:47, tfarina wrote:
> nit: As many other views dialog, I'd name this as IdleLogoutDialogView and
> rename the file_name as well.

Done.

http://codereview.chromium.org/9665031/diff/9003/chrome/browser/chromeos/ui/i...
chrome/browser/chromeos/ui/idle_logout_dialog.h:22: static void
ShowIdleLogoutDialog();
On 2012/03/14 16:45:47, tfarina wrote:
> nit: This names is redundant. I think it's better just Show() and Close().
> 
> Instead of having:
> 
> IdleLogoutDialog::ShowIdleLogoutDialog();
> 
> Have an:
> 
> IdleLogoutDialog::Show();
> 
> Same for CloseIdleLogoutDialog();

Show/CloseDialog seem more appropriate; renaming to those.
Done.

http://codereview.chromium.org/9665031/diff/9003/chrome/browser/chromeos/ui/i...
chrome/browser/chromeos/ui/idle_logout_dialog.h:52: bool is_closed() { return
NULL == *instance_holder_; }
On 2012/03/14 16:45:47, tfarina wrote:
> nit: bool is_closed() const { ... };

Done.

[tfarina, 2012-03-14 22:55:34]

http://codereview.chromium.org/9665031/diff/14001/chrome/browser/chromeos/ui/...
File chrome/browser/chromeos/ui/idle_logout_dialog_view.cc (right):

http://codereview.chromium.org/9665031/diff/14001/chrome/browser/chromeos/ui/...
chrome/browser/chromeos/ui/idle_logout_dialog_view.cc:10: #include
"base/timer.h"
nit: already included in the header, remove.

http://codereview.chromium.org/9665031/diff/14001/chrome/browser/chromeos/ui/...
chrome/browser/chromeos/ui/idle_logout_dialog_view.cc:14: #include
"chrome/common/logging_chrome.h"
can you remove this as well?

http://codereview.chromium.org/9665031/diff/14001/chrome/browser/chromeos/ui/...
chrome/browser/chromeos/ui/idle_logout_dialog_view.cc:25: #include "ash/shell.h"
What do you need these includes for?

http://codereview.chromium.org/9665031/diff/14001/chrome/browser/chromeos/ui/...
chrome/browser/chromeos/ui/idle_logout_dialog_view.cc:40: // IdleLogoutDialog
public static methods
nit: IdleLogoutDialogView

http://codereview.chromium.org/9665031/diff/14001/chrome/browser/chromeos/ui/...
chrome/browser/chromeos/ui/idle_logout_dialog_view.cc:97: void
IdleLogoutDialogView::Layout() {
Do you still need to override this?

http://codereview.chromium.org/9665031/diff/14001/chrome/browser/chromeos/ui/...
chrome/browser/chromeos/ui/idle_logout_dialog_view.cc:120:
base::Bind(&IdleLogoutDialogView::CallInit, instance_holder_));
nit: indent 4 spaces only.

http://codereview.chromium.org/9665031/diff/14001/chrome/browser/chromeos/ui/...
chrome/browser/chromeos/ui/idle_logout_dialog_view.cc:136:
warning_label_->SizeToFit(kIdleLogoutDialogMaxWidth);
nit: called twice? first in line 131 and then now here. Is this intentional?

http://codereview.chromium.org/9665031/diff/14001/chrome/browser/chromeos/ui/...
File chrome/browser/chromeos/ui/idle_logout_dialog_view.h (right):

http://codereview.chromium.org/9665031/diff/14001/chrome/browser/chromeos/ui/...
chrome/browser/chromeos/ui/idle_logout_dialog_view.h:28: virtual string16
GetWindowTitle() const  OVERRIDE;
nit: two spaces between const and OVERRIDE, below too.

[rkc, 2012-03-14 23:19:29]

http://codereview.chromium.org/9665031/diff/14001/chrome/browser/chromeos/ui/...
File chrome/browser/chromeos/ui/idle_logout_dialog_view.cc (right):

http://codereview.chromium.org/9665031/diff/14001/chrome/browser/chromeos/ui/...
chrome/browser/chromeos/ui/idle_logout_dialog_view.cc:10: #include
"base/timer.h"
On 2012/03/14 22:55:34, tfarina wrote:
> nit: already included in the header, remove.

Done.

http://codereview.chromium.org/9665031/diff/14001/chrome/browser/chromeos/ui/...
chrome/browser/chromeos/ui/idle_logout_dialog_view.cc:14: #include
"chrome/common/logging_chrome.h"
On 2012/03/14 22:55:34, tfarina wrote:
> can you remove this as well?

Done.

http://codereview.chromium.org/9665031/diff/14001/chrome/browser/chromeos/ui/...
chrome/browser/chromeos/ui/idle_logout_dialog_view.cc:25: #include "ash/shell.h"
On 2012/03/14 22:55:34, tfarina wrote:
> What do you need these includes for?

Already removed in the latest patch.

http://codereview.chromium.org/9665031/diff/14001/chrome/browser/chromeos/ui/...
chrome/browser/chromeos/ui/idle_logout_dialog_view.cc:40: // IdleLogoutDialog
public static methods
On 2012/03/14 22:55:34, tfarina wrote:
> nit: IdleLogoutDialogView

Done.

http://codereview.chromium.org/9665031/diff/14001/chrome/browser/chromeos/ui/...
chrome/browser/chromeos/ui/idle_logout_dialog_view.cc:97: void
IdleLogoutDialogView::Layout() {
On 2012/03/14 22:55:34, tfarina wrote:
> Do you still need to override this?

Doesn't seem like it anymore.
Done.

http://codereview.chromium.org/9665031/diff/14001/chrome/browser/chromeos/ui/...
chrome/browser/chromeos/ui/idle_logout_dialog_view.cc:120:
base::Bind(&IdleLogoutDialogView::CallInit, instance_holder_));
On 2012/03/14 22:55:34, tfarina wrote:
> nit: indent 4 spaces only.

Done.

http://codereview.chromium.org/9665031/diff/14001/chrome/browser/chromeos/ui/...
chrome/browser/chromeos/ui/idle_logout_dialog_view.cc:136:
warning_label_->SizeToFit(kIdleLogoutDialogMaxWidth);
On 2012/03/14 22:55:34, tfarina wrote:
> nit: called twice? first in line 131 and then now here. Is this intentional?

Done.

http://codereview.chromium.org/9665031/diff/14001/chrome/browser/chromeos/ui/...
File chrome/browser/chromeos/ui/idle_logout_dialog_view.h (right):

http://codereview.chromium.org/9665031/diff/14001/chrome/browser/chromeos/ui/...
chrome/browser/chromeos/ui/idle_logout_dialog_view.h:28: virtual string16
GetWindowTitle() const  OVERRIDE;
On 2012/03/14 22:55:34, tfarina wrote:
> nit: two spaces between const and OVERRIDE, below too.

Done.

[sky, 2012-03-15 00:05:01]

LGTM

[tfarina, 2012-03-15 00:10:43]

On Wed, Mar 14, 2012 at 9:05 PM,  <sky@chromium.org> wrote:
> LGTM
>
You just need the Scott lgtm, but it lgtm too now.

Thanks.

-- 
Thiago

[rkc, 2012-03-15 00:12:23]

On 2012/03/15 00:10:43, tfarina wrote:
> On Wed, Mar 14, 2012 at 9:05 PM,  <mailto:sky@chromium.org> wrote:
> > LGTM
> >
> You just need the Scott lgtm, but it lgtm too now.
> 
> Thanks.
> 
> -- 
> Thiago

Thanks for the meticulous review!