Deletes keyboard container before recreating the controller.

Updates RootWindowController to delete the keyboard container before recreating the keyboard controller since the controller creates a new window in GetWindowContainer().


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

[bshe, 2013-09-27 18:25:04]

On 2013/09/27 18:14:29, rsadam wrote:

lgtm

[sadrul, 2013-09-27 20:25:16]

https://codereview.chromium.org/25105002/diff/1/ui/keyboard/keyboard_controll...
File ui/keyboard/keyboard_controller.cc (right):

https://codereview.chromium.org/25105002/diff/1/ui/keyboard/keyboard_controll...
ui/keyboard/keyboard_controller.cc:137: container_->SetLayoutManager(NULL);
The container window takes ownership of the layout-manager created below (line
149), and the layout-manager is not tied to the lifetime of this controller. So
it should not be necessary to reset the layout-manager here.

I left a comment on the bug explaining what I think would be better fixes.

[rsadam, 2013-10-01 14:23:49]

Re-implemented the fix as you suggested!

[sadrul, 2013-10-01 14:28:28]

LGTM

Please add an OWNER reviewer in the next iteration.

https://codereview.chromium.org/25105002/diff/7001/ash/root_window_controller.cc
File ash/root_window_controller.cc (right):

https://codereview.chromium.org/25105002/diff/7001/ash/root_window_controller...
ash/root_window_controller.cc:534: // Deletes the old container if it exists.
Add a comment here that the KeyboardController recreates a container window
(e.g. 'Delete the old container since |keyboard_controller_| creates a new
container window in GetContainerWindow()').

[bshe, 2013-10-01 14:29:49]

On 2013/10/01 14:28:28, sadrul wrote:
> LGTM
> 
> Please add an OWNER reviewer in the next iteration.
> 
>
https://codereview.chromium.org/25105002/diff/7001/ash/root_window_controller.cc
> File ash/root_window_controller.cc (right):
> 
>
https://codereview.chromium.org/25105002/diff/7001/ash/root_window_controller...
> ash/root_window_controller.cc:534: // Deletes the old container if it exists.
> Add a comment here that the KeyboardController recreates a container window
> (e.g. 'Delete the old container since |keyboard_controller_| creates a new
> container window in GetContainerWindow()').

lgtm
you also want to change your issue description to reflect the new change. We
dont do the
clean up in destructor anymore.

[rsadam, 2013-10-01 14:44:00]

Fixed the comment as suggested.

+jamescook for OWNERS

https://codereview.chromium.org/25105002/diff/7001/ash/root_window_controller.cc
File ash/root_window_controller.cc (right):

https://codereview.chromium.org/25105002/diff/7001/ash/root_window_controller...
ash/root_window_controller.cc:534: // Deletes the old container if it exists.
On 2013/10/01 14:28:28, sadrul wrote:
> Add a comment here that the KeyboardController recreates a container window
> (e.g. 'Delete the old container since |keyboard_controller_| creates a new
> container window in GetContainerWindow()').

Done.

[James Cook, 2013-10-01 15:45:29]

Can we have a test for this function?  Then the memory bots will run it and
we'll be protected from future devs messing up memory management here.

[rsadam, 2013-10-01 18:09:57]

Added a unit test to root_window_controller_unittest to test this.

[James Cook, 2013-10-01 20:15:33]

LGTM

[commit-bot: I haz the power, 2013-10-01 20:20:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rsadam@chromium.org/25105002/18001

[commit-bot: I haz the power, 2013-10-01 22:49:02]

Change committed as 226324