Enable WebUI ScreenLocker for aura by abstracting lock window.

Enable WebUI ScreenLocker for aura by abstracting lock window.

This CL abstracts the actual lock window allowing for a GTK implementation and an aura implementation. The aura screenlocker stubs are removed and the screenlocker is enabled on aura using the aura lock window.

BUG=100723
TEST=Build chrome with use_aura=1, trigger screen lock and test unlocking screen.


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

[flackr, 2011-11-27 01:20:12]

Hi oshima,

Please take a look, I've tested the screen locker now with aura.

Thanks!

[oshima, 2011-11-29 20:18:06]

http://codereview.chromium.org/8711003/diff/2001/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros_stubs_aura.cc (right):

http://codereview.chromium.org/8711003/diff/2001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros_stubs_aura.cc:6: #include
"chrome/browser/chromeos/login/user.h"
these two are no longer necessary?

http://codereview.chromium.org/8711003/diff/2001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros_stubs_aura.cc:10: #include
"chrome/browser/ui/views/frame/browser_non_client_frame_view.h"
ditto (for above three)

http://codereview.chromium.org/8711003/diff/2001/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/lock_window.h (right):

http://codereview.chromium.org/8711003/diff/2001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/lock_window.h:15: 
nit: remove new lines

http://codereview.chromium.org/8711003/diff/2001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/lock_window.h:25: // about its status.
I think observer is more appropriate here than "a delegate".

A delegate is an object that does certain task on behalf of the calling class.
(thus, there should be only one delegate), while an observer is an object that
wants to take action in response to certain events. (thus can be multiple
observers).

http://codereview.chromium.org/8711003/diff/2001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/lock_window.h:33: LockWindow() : observer_(NULL)
{}
virtual. please move impl to .cc

http://codereview.chromium.org/8711003/diff/2001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/lock_window.h:36: virtual void
SetContentsView(views::View* content) = 0;
Sets |content| ...

http://codereview.chromium.org/8711003/diff/2001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/lock_window.h:38: // Show the lock window,
|dom_view| is the actual view displaying the lock
Shows/Sets/Creates (this is chromium style)

http://codereview.chromium.org/8711003/diff/2001/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/lock_window_aura.cc (right):

http://codereview.chromium.org/8711003/diff/2001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/lock_window_aura.cc:46: params.bounds = bounds;
nit: or just = gfx::Rect(aura::Desktop...)
up to you.

http://codereview.chromium.org/8711003/diff/2001/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/lock_window_aura.h (right):

http://codereview.chromium.org/8711003/diff/2001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/lock_window_aura.h:9: #include <cstddef>
do we need this here?

http://codereview.chromium.org/8711003/diff/2001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/lock_window_aura.h:10: 
base/compiler_specific.h (for iwyu)

http://codereview.chromium.org/8711003/diff/2001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/lock_window_aura.h:17: public LockWindow {
When we need different impl for different platform, we typically put them in
NativeWidget (see TabContentsView and NativeTabContentsViewXXX as an example).
It may not work well in this case though. Can you look into it?

http://codereview.chromium.org/8711003/diff/2001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/lock_window_aura.h:19: LockWindowAura();
move this to private and add friend to LockWindow.

http://codereview.chromium.org/8711003/diff/2001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/lock_window_aura.h:27: virtual ~LockWindowAura()
{}
move to .cc

http://codereview.chromium.org/8711003/diff/2001/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/lock_window_gtk.h (right):

http://codereview.chromium.org/8711003/diff/2001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/lock_window_gtk.h:9: #include <string>
not necessary here?

http://codereview.chromium.org/8711003/diff/2001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/lock_window_gtk.h:31: LockWindowGtk();
ditto

http://codereview.chromium.org/8711003/diff/2001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/lock_window_gtk.h:47: virtual ~LockWindowGtk() {}
move impl to .cc

http://codereview.chromium.org/8711003/diff/2001/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/screen_locker.cc (right):

http://codereview.chromium.org/8711003/diff/2001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/screen_locker.cc:366: #if defined(TOUCH_UI)
I thought TOUCH_UI has switched to AURA, correct? If that's the case, can we
simply exclude UseWebUILockScreen for aura?

http://codereview.chromium.org/8711003/diff/2001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/8711003/diff/2001/chrome/chrome_browser.gypi#n...
chrome/chrome_browser.gypi:5026: ['exclude',
'^browser/chromeos/login/lock_window_aura.h'],
I believe we exclude _aura files for use_aura==0 by default.

http://codereview.chromium.org/8711003/diff/2001/chrome/chrome_browser.gypi#n...
chrome/chrome_browser.gypi:5144: ['exclude',
'^browser/chromeos/login/lock_window_gtk.h'],
same _gtk is excluded for non gtk build, unless it's included explicitly
somewhere.

[flackr, 2011-11-30 20:16:54]

http://codereview.chromium.org/8711003/diff/2001/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros_stubs_aura.cc (right):

http://codereview.chromium.org/8711003/diff/2001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros_stubs_aura.cc:6: #include
"chrome/browser/chromeos/login/user.h"
On 2011/11/29 20:18:06, oshima wrote:
> these two are no longer necessary?

Done.

http://codereview.chromium.org/8711003/diff/2001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros_stubs_aura.cc:10: #include
"chrome/browser/ui/views/frame/browser_non_client_frame_view.h"
On 2011/11/29 20:18:06, oshima wrote:
> ditto (for above three)

Done.

http://codereview.chromium.org/8711003/diff/2001/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/lock_window.h (right):

http://codereview.chromium.org/8711003/diff/2001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/lock_window.h:15: 
On 2011/11/29 20:18:06, oshima wrote:
> nit: remove new lines

Done.

http://codereview.chromium.org/8711003/diff/2001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/lock_window.h:25: // about its status.
On 2011/11/29 20:18:06, oshima wrote:
> I think observer is more appropriate here than "a delegate".
> 
> A delegate is an object that does certain task on behalf of the calling class.
> (thus, there should be only one delegate), while an observer is an object that
> wants to take action in response to certain events. (thus can be multiple
> observers).

Done. I missed this instance of the word delegate :-).

http://codereview.chromium.org/8711003/diff/2001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/lock_window.h:33: LockWindow() : observer_(NULL)
{}
On 2011/11/29 20:18:06, oshima wrote:
> virtual. please move impl to .cc

Virtual constructor? Implementation moved.

http://codereview.chromium.org/8711003/diff/2001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/lock_window.h:36: virtual void
SetContentsView(views::View* content) = 0;
On 2011/11/29 20:18:06, oshima wrote:
> Sets |content| ...

Done.

http://codereview.chromium.org/8711003/diff/2001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/lock_window.h:38: // Show the lock window,
|dom_view| is the actual view displaying the lock
On 2011/11/29 20:18:06, oshima wrote:
> Shows/Sets/Creates (this is chromium style)

Done.

http://codereview.chromium.org/8711003/diff/2001/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/lock_window_aura.cc (right):

http://codereview.chromium.org/8711003/diff/2001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/lock_window_aura.cc:46: params.bounds = bounds;
On 2011/11/29 20:18:06, oshima wrote:
> nit: or just = gfx::Rect(aura::Desktop...)
> up to you.

Done.

http://codereview.chromium.org/8711003/diff/2001/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/lock_window_aura.h (right):

http://codereview.chromium.org/8711003/diff/2001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/lock_window_aura.h:9: #include <cstddef>
On 2011/11/29 20:18:06, oshima wrote:
> do we need this here?

This include was for NULL. Not needed here.

http://codereview.chromium.org/8711003/diff/2001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/lock_window_aura.h:10: 
On 2011/11/29 20:18:06, oshima wrote:
> base/compiler_specific.h (for iwyu)

Done.

http://codereview.chromium.org/8711003/diff/2001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/lock_window_aura.h:17: public LockWindow {
On 2011/11/29 20:18:06, oshima wrote:
> When we need different impl for different platform, we typically put them in
> NativeWidget (see TabContentsView and NativeTabContentsViewXXX as an example).
> It may not work well in this case though. Can you look into it?

I'm not exactly sure what you mean. Do you mean to subclass NativeWidget as
LockWindowGtk does? TabContentsView seems to have TabContentsView(Win|Mac|Gtk),
and  the NativeTabContentsView interface is very similar to LockWindow interface
except that all implementations subclass NativeWidgetXXX.

http://codereview.chromium.org/8711003/diff/2001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/lock_window_aura.h:19: LockWindowAura();
On 2011/11/29 20:18:06, oshima wrote:
> move this to private and add friend to LockWindow.

Done.

http://codereview.chromium.org/8711003/diff/2001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/lock_window_aura.h:27: virtual ~LockWindowAura()
{}
On 2011/11/29 20:18:06, oshima wrote:
> move to .cc

Done.

http://codereview.chromium.org/8711003/diff/2001/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/lock_window_gtk.h (right):

http://codereview.chromium.org/8711003/diff/2001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/lock_window_gtk.h:9: #include <string>
On 2011/11/29 20:18:06, oshima wrote:
> not necessary here?

Done.

http://codereview.chromium.org/8711003/diff/2001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/lock_window_gtk.h:31: LockWindowGtk();
On 2011/11/29 20:18:06, oshima wrote:
> ditto

Done.

http://codereview.chromium.org/8711003/diff/2001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/lock_window_gtk.h:47: virtual ~LockWindowGtk() {}
On 2011/11/29 20:18:06, oshima wrote:
> move impl to .cc

Done.

http://codereview.chromium.org/8711003/diff/2001/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/screen_locker.cc (right):

http://codereview.chromium.org/8711003/diff/2001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/screen_locker.cc:366: #if defined(TOUCH_UI)
On 2011/11/29 20:18:06, oshima wrote:
> I thought TOUCH_UI has switched to AURA, correct? If that's the case, can we
> simply exclude UseWebUILockScreen for aura?

It either has or will soon. Done.

http://codereview.chromium.org/8711003/diff/2001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/8711003/diff/2001/chrome/chrome_browser.gypi#n...
chrome/chrome_browser.gypi:5026: ['exclude',
'^browser/chromeos/login/lock_window_aura.h'],
On 2011/11/29 20:18:06, oshima wrote:
> I believe we exclude _aura files for use_aura==0 by default.

Done.

http://codereview.chromium.org/8711003/diff/2001/chrome/chrome_browser.gypi#n...
chrome/chrome_browser.gypi:5144: ['exclude',
'^browser/chromeos/login/lock_window_gtk.h'],
On 2011/11/29 20:18:06, oshima wrote:
> same _gtk is excluded for non gtk build, unless it's included explicitly
> somewhere.

Done.

[oshima, 2011-11-30 23:41:07]

can you run it through trybots?

http://codereview.chromium.org/8711003/diff/2001/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/lock_window.h (right):

http://codereview.chromium.org/8711003/diff/2001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/lock_window.h:33: LockWindow() : observer_(NULL)
{}
On 2011/11/30 20:16:54, flackr wrote:
> On 2011/11/29 20:18:06, oshima wrote:
> > virtual. please move impl to .cc
> 
> Virtual constructor? Implementation moved.

Sorry, I was going to say you need virtual detor as this is abstract superclass.

http://codereview.chromium.org/8711003/diff/2001/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/lock_window_aura.h (right):

http://codereview.chromium.org/8711003/diff/2001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/lock_window_aura.h:17: public LockWindow {
On 2011/11/30 20:16:54, flackr wrote:
> On 2011/11/29 20:18:06, oshima wrote:
> > When we need different impl for different platform, we typically put them in
> > NativeWidget (see TabContentsView and NativeTabContentsViewXXX as an
example).
> > It may not work well in this case though. Can you look into it?
> 
> I'm not exactly sure what you mean. Do you mean to subclass NativeWidget as
> LockWindowGtk does? TabContentsView seems to have
TabContentsView(Win|Mac|Gtk),
> and  the NativeTabContentsView interface is very similar to LockWindow
interface
> except that all implementations subclass NativeWidgetXXX.

I was thinking something like LockWindow being just a Widget and provide
difference in NativeWidget via params.native_widget (like you did in gtk)

I thought this may actually make the code simpler/smaller because
you don't need SetContents/GetWidget , although I may be wrong.

http://codereview.chromium.org/8711003/diff/8001/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/lock_window.h (right):

http://codereview.chromium.org/8711003/diff/8001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/lock_window.h:54: Observer* observer_;
DISALLOW_COPY_AND_ASSIGN

http://codereview.chromium.org/8711003/diff/8001/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/lock_window_aura.cc (right):

http://codereview.chromium.org/8711003/diff/8001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/lock_window_aura.cc:5: #include
"chrome/browser/chromeos/login/lock_window_aura.h"
new line after this

http://codereview.chromium.org/8711003/diff/8001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/lock_window_aura.cc:49:
aura_shell::internal::kShellWindowId_LockScreenContainer);
You probably should use a property (or invent a way) to tell it's lock screen,
instead of depending on shell implementation.
You can do it in separate CL though.

http://codereview.chromium.org/8711003/diff/8001/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/lock_window_aura.h (right):

http://codereview.chromium.org/8711003/diff/8001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/lock_window_aura.h:30: void Init();
DISALLOW_COPY_AND_ASSIGN

[flackr, 2011-12-01 21:40:40]

Try runs are coming.

http://codereview.chromium.org/8711003/diff/2001/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/lock_window_aura.h (right):

http://codereview.chromium.org/8711003/diff/2001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/lock_window_aura.h:17: public LockWindow {
On 2011/11/30 23:41:07, oshima wrote:
> On 2011/11/30 20:16:54, flackr wrote:
> > On 2011/11/29 20:18:06, oshima wrote:
> > > When we need different impl for different platform, we typically put them
in
> > > NativeWidget (see TabContentsView and NativeTabContentsViewXXX as an
> example).
> > > It may not work well in this case though. Can you look into it?
> > 
> > I'm not exactly sure what you mean. Do you mean to subclass NativeWidget as
> > LockWindowGtk does? TabContentsView seems to have
> TabContentsView(Win|Mac|Gtk),
> > and  the NativeTabContentsView interface is very similar to LockWindow
> interface
> > except that all implementations subclass NativeWidgetXXX.
> 
> I was thinking something like LockWindow being just a Widget and provide
> difference in NativeWidget via params.native_widget (like you did in gtk)
> 
> I thought this may actually make the code simpler/smaller because
> you don't need SetContents/GetWidget , although I may be wrong.

To do this I will likely have to introduce "if defined" into the lock_window
code. Only the GTK version requires observing grab and render status, as well as
grabbing a particular widget which means selectively calling functions on the
NativeWidget. I can avoid having to define SetContents by using the actual
widget as I have done in patch 7.

http://codereview.chromium.org/8711003/diff/8001/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/lock_window.h (right):

http://codereview.chromium.org/8711003/diff/8001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/lock_window.h:54: Observer* observer_;
On 2011/11/30 23:41:07, oshima wrote:
> DISALLOW_COPY_AND_ASSIGN

All of the interface/virtual classes I looked at do not have this. Also:
./chrome/browser/chromeos/login/lock_window.h:56: error: ISO C++ forbids
declaration of 'DISALLOW_COPY_AND_ASSIGN' with no type
./chrome/browser/chromeos/login/lock_window.h:56: error: cannot declare
parameter '<anonymous>' to be of abstract type 'chromeos::LockWindow'
./chrome/browser/chromeos/login/lock_window.h:20: note:   because the following
virtual functions are pure within 'chromeos::LockWindow':

http://codereview.chromium.org/8711003/diff/8001/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/lock_window_aura.cc (right):

http://codereview.chromium.org/8711003/diff/8001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/lock_window_aura.cc:5: #include
"chrome/browser/chromeos/login/lock_window_aura.h"
On 2011/11/30 23:41:07, oshima wrote:
> new line after this

Done.

http://codereview.chromium.org/8711003/diff/8001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/lock_window_aura.cc:49:
aura_shell::internal::kShellWindowId_LockScreenContainer);
On 2011/11/30 23:41:07, oshima wrote:
> You probably should use a property (or invent a way) to tell it's lock screen,
> instead of depending on shell implementation.
> You can do it in separate CL though.

Wrote TODO.

http://codereview.chromium.org/8711003/diff/8001/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/lock_window_aura.h (right):

http://codereview.chromium.org/8711003/diff/8001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/lock_window_aura.h:30: void Init();
On 2011/11/30 23:41:07, oshima wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

[oshima, 2011-12-01 22:45:35]

http://codereview.chromium.org/8711003/diff/2001/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/lock_window_aura.h (right):

http://codereview.chromium.org/8711003/diff/2001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/lock_window_aura.h:17: public LockWindow {
On 2011/12/01 21:40:40, flackr wrote:
> On 2011/11/30 23:41:07, oshima wrote:
> > On 2011/11/30 20:16:54, flackr wrote:
> > > On 2011/11/29 20:18:06, oshima wrote:
> > > > When we need different impl for different platform, we typically put
them
> in
> > > > NativeWidget (see TabContentsView and NativeTabContentsViewXXX as an
> > example).
> > > > It may not work well in this case though. Can you look into it?
> > > 
> > > I'm not exactly sure what you mean. Do you mean to subclass NativeWidget
as
> > > LockWindowGtk does? TabContentsView seems to have
> > TabContentsView(Win|Mac|Gtk),
> > > and  the NativeTabContentsView interface is very similar to LockWindow
> > interface
> > > except that all implementations subclass NativeWidgetXXX.
> > 
> > I was thinking something like LockWindow being just a Widget and provide
> > difference in NativeWidget via params.native_widget (like you did in gtk)
> > 
> > I thought this may actually make the code simpler/smaller because
> > you don't need SetContents/GetWidget , although I may be wrong.
> 
> To do this I will likely have to introduce "if defined" into the lock_window
> code. Only the GTK version requires observing grab and render status, as well
as
> grabbing a particular widget which means selectively calling functions on the
> NativeWidget. I can avoid having to define SetContents by using the actual
> widget as I have done in patch 7.

It it doesn't help, then I'm fine with current approach.

http://codereview.chromium.org/8711003/diff/8001/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/lock_window.h (right):

http://codereview.chromium.org/8711003/diff/8001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/lock_window.h:54: Observer* observer_;
On 2011/12/01 21:40:40, flackr wrote:
> On 2011/11/30 23:41:07, oshima wrote:
> > DISALLOW_COPY_AND_ASSIGN
> 
> All of the interface/virtual classes I looked at do not have this.

This is not interface class because it has instance variable.

 Also:
> ./chrome/browser/chromeos/login/lock_window.h:56: error: ISO C++ forbids
> declaration of 'DISALLOW_COPY_AND_ASSIGN' with no type
> ./chrome/browser/chromeos/login/lock_window.h:56: error: cannot declare
> parameter '<anonymous>' to be of abstract type 'chromeos::LockWindow'
> ./chrome/browser/chromeos/login/lock_window.h:20: note:   because the
following
> virtual functions are pure within 'chromeos::LockWindow':

you need "base/basictypes.h"

http://codereview.chromium.org/8711003/diff/18007/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/lock_window.cc (right):

http://codereview.chromium.org/8711003/diff/18007/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/lock_window.cc:7: #include
"chrome/browser/chromeos/login/lock_window.h"
lock_window.h first.

http://codereview.chromium.org/8711003/diff/18007/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/webui_screen_locker.h (right):

http://codereview.chromium.org/8711003/diff/18007/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/webui_screen_locker.h:66: virtual void
OnLockWindowReady();
OVERRIDE; 

(note that this is now required and forgetting will break clang)

[flackr, 2011-12-01 23:16:34]

Looking into cros_x86 failure.

http://codereview.chromium.org/8711003/diff/8001/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/lock_window.h (right):

http://codereview.chromium.org/8711003/diff/8001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/lock_window.h:54: Observer* observer_;
On 2011/12/01 22:45:35, oshima wrote:
> On 2011/12/01 21:40:40, flackr wrote:
> > On 2011/11/30 23:41:07, oshima wrote:
> > > DISALLOW_COPY_AND_ASSIGN
> > 
> > All of the interface/virtual classes I looked at do not have this.
> 
> This is not interface class because it has instance variable.
> 
>  Also:
> > ./chrome/browser/chromeos/login/lock_window.h:56: error: ISO C++ forbids
> > declaration of 'DISALLOW_COPY_AND_ASSIGN' with no type
> > ./chrome/browser/chromeos/login/lock_window.h:56: error: cannot declare
> > parameter '<anonymous>' to be of abstract type 'chromeos::LockWindow'
> > ./chrome/browser/chromeos/login/lock_window.h:20: note:   because the
> following
> > virtual functions are pure within 'chromeos::LockWindow':
> 
> you need "base/basictypes.h"

Thanks, sorry for the confusion! Done.

http://codereview.chromium.org/8711003/diff/18007/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/lock_window.cc (right):

http://codereview.chromium.org/8711003/diff/18007/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/lock_window.cc:7: #include
"chrome/browser/chromeos/login/lock_window.h"
On 2011/12/01 22:45:35, oshima wrote:
> lock_window.h first.

Done.

http://codereview.chromium.org/8711003/diff/18007/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/webui_screen_locker.h (right):

http://codereview.chromium.org/8711003/diff/18007/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/webui_screen_locker.h:66: virtual void
OnLockWindowReady();
On 2011/12/01 22:45:35, oshima wrote:
> OVERRIDE; 
> 
> (note that this is now required and forgetting will break clang)

Done.

[oshima, 2011-12-02 00:50:46]

LGTM

You may want to run linux_chromeos_clang before commit.

[commit-bot: I haz the power, 2011-12-02 14:45:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/flackr@chromium.org/8711003/15057

[commit-bot: I haz the power, 2011-12-02 16:05:03]

Change committed as 112709