Refactor ViewsDelegate singleton

Refactor ViewsDelegate singleton

Currently, the variable containing ViewsDelegate singleton object can be
set by anyone and there are cases (at least in tests) where it is freely
replaced without properly deleting the old one which can lead to
situations where multiple ViewsDelegate objects are in existence. This
patch tries to ensure that there is at most one instance available at
each time.

Also, cleaned up some ViewsDelegate related includes.

BUG=492991
Committed: https://crrev.com/0ff8c0e1c8c805bce8e216c74c66ccc8fd137a6f
Cr-Commit-Position: refs/heads/master@{#333692}

[mohsen, 2015-06-05 21:01:47]

mohsen@chromium.org changed reviewers:
+ sadrul@chromium.org

[mohsen, 2015-06-05 21:01:49]

sadrul@: This is my stab at fixing ViewsDelegate situation. What do you think?

[sadrul, 2015-06-08 03:27:43]

LGTM

https://codereview.chromium.org/1159033008/diff/20001/ui/views/controls/textf...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/1159033008/diff/20001/ui/views/controls/textf...
ui/views/controls/textfield/textfield.cc:290:
->GetDefaultTextfieldObscuredRevealDuration();
Does 'git cl format' do this?

https://codereview.chromium.org/1159033008/diff/20001/ui/views/controls/webvi...
File ui/views/controls/webview/webview_unittest.cc (right):

https://codereview.chromium.org/1159033008/diff/20001/ui/views/controls/webvi...
ui/views/controls/webview/webview_unittest.cc:135:
set_views_delegate(views_delegate.Pass());
You can also do:
  set_views_delegate(make_scoped_ptr(new WebViewTestViewsDelegate));

(this applies to the rest of the set_views_delegate() calls too).

https://codereview.chromium.org/1159033008/diff/20001/ui/views/views_delegate.h
File ui/views/views_delegate.h (right):

https://codereview.chromium.org/1159033008/diff/20001/ui/views/views_delegate...
ui/views/views_delegate.h:63: // XXX
?

https://codereview.chromium.org/1159033008/diff/20001/ui/views/views_delegate...
ui/views/views_delegate.h:79: static ViewsDelegate* GetInstance();
Clarify here that GetInstance() can return null if an instance hasn't been
created yet.

https://codereview.chromium.org/1159033008/diff/20001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_native_widget_aura_unittest.cc
(right):

https://codereview.chromium.org/1159033008/diff/20001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_native_widget_aura_unittest.cc:368:
set_views_delegate(nullptr);
Maybe DesktopAuraWidgetTest could do this in its SetUp() override?

[mohsen, 2015-06-08 19:27:35]

+sky@: for OWNERS in ash/, chrome/browser/ui/, and chrome/test/.

https://codereview.chromium.org/1159033008/diff/20001/ui/views/controls/textf...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/1159033008/diff/20001/ui/views/controls/textf...
ui/views/controls/textfield/textfield.cc:290:
->GetDefaultTextfieldObscuredRevealDuration();
On 2015/06/08 03:27:43, sadrul wrote:
> Does 'git cl format' do this?

Yes.

https://codereview.chromium.org/1159033008/diff/20001/ui/views/controls/webvi...
File ui/views/controls/webview/webview_unittest.cc (right):

https://codereview.chromium.org/1159033008/diff/20001/ui/views/controls/webvi...
ui/views/controls/webview/webview_unittest.cc:135:
set_views_delegate(views_delegate.Pass());
On 2015/06/08 03:27:43, sadrul wrote:
> You can also do:
>   set_views_delegate(make_scoped_ptr(new WebViewTestViewsDelegate));
> 
> (this applies to the rest of the set_views_delegate() calls too).

Done.

https://codereview.chromium.org/1159033008/diff/20001/ui/views/views_delegate.h
File ui/views/views_delegate.h (right):

https://codereview.chromium.org/1159033008/diff/20001/ui/views/views_delegate...
ui/views/views_delegate.h:63: // XXX
On 2015/06/08 03:27:43, sadrul wrote:
> ?

Sorry! This was here to remind me to update the comment, and... it didn't! Done.

https://codereview.chromium.org/1159033008/diff/20001/ui/views/views_delegate...
ui/views/views_delegate.h:79: static ViewsDelegate* GetInstance();
On 2015/06/08 03:27:43, sadrul wrote:
> Clarify here that GetInstance() can return null if an instance hasn't been
> created yet.

Done.

https://codereview.chromium.org/1159033008/diff/20001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_native_widget_aura_unittest.cc
(right):

https://codereview.chromium.org/1159033008/diff/20001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_native_widget_aura_unittest.cc:368:
set_views_delegate(nullptr);
On 2015/06/08 03:27:43, sadrul wrote:
> Maybe DesktopAuraWidgetTest could do this in its SetUp() override?

The last two tests did not change the existing ViewsDelegate and I thought that
they might be dependent on it.

[mohsen, 2015-06-08 19:28:25]

mohsen@chromium.org changed reviewers:
+ sky@chromium.org

[mohsen, 2015-06-08 19:28:25]

+sky@: for OWNERS in ash/, chrome/browser/ui/, and chrome/test/.

[mohsen, 2015-06-08 19:34:22]

mohsen@chromium.org changed reviewers:
+ benwells@chromium.org

[mohsen, 2015-06-08 19:34:25]

+benwells@: for OWNERS in apps/.

[sky, 2015-06-08 20:22:42]

LGTM

https://codereview.chromium.org/1159033008/diff/40001/ash/shell/content_clien...
File ash/shell/content_client/shell_browser_main_parts.h (right):

https://codereview.chromium.org/1159033008/diff/40001/ash/shell/content_clien...
ash/shell/content_client/shell_browser_main_parts.h:61:
scoped_ptr<views::ViewsDelegate> views_delegate_;
Seems like this should be before window_watcher_.

https://codereview.chromium.org/1159033008/diff/40001/ui/views/views_delegate.cc
File ui/views/views_delegate.cc (right):

https://codereview.chromium.org/1159033008/diff/40001/ui/views/views_delegate...
ui/views/views_delegate.cc:17: ViewsDelegate* g_views_delegate = nullptr;
This isn't global, just views_delegate.

[mohsen, 2015-06-08 20:46:09]

https://codereview.chromium.org/1159033008/diff/40001/ash/shell/content_clien...
File ash/shell/content_client/shell_browser_main_parts.h (right):

https://codereview.chromium.org/1159033008/diff/40001/ash/shell/content_clien...
ash/shell/content_client/shell_browser_main_parts.h:61:
scoped_ptr<views::ViewsDelegate> views_delegate_;
On 2015/06/08 20:22:42, sky wrote:
> Seems like this should be before window_watcher_.

Done, but is it because it is set before window_wathcer_ in the .cc file? Or
there is some other reason?

https://codereview.chromium.org/1159033008/diff/40001/ui/views/views_delegate.cc
File ui/views/views_delegate.cc (right):

https://codereview.chromium.org/1159033008/diff/40001/ui/views/views_delegate...
ui/views/views_delegate.cc:17: ViewsDelegate* g_views_delegate = nullptr;
On 2015/06/08 20:22:42, sky wrote:
> This isn't global, just views_delegate.

Interesting. I've seen several variables like this prefixed with g_. Done.

[sky, 2015-06-08 20:48:03]

https://codereview.chromium.org/1159033008/diff/40001/ash/shell/content_clien...
File ash/shell/content_client/shell_browser_main_parts.h (right):

https://codereview.chromium.org/1159033008/diff/40001/ash/shell/content_clien...
ash/shell/content_client/shell_browser_main_parts.h:61:
scoped_ptr<views::ViewsDelegate> views_delegate_;
On 2015/06/08 20:46:09, mohsen wrote:
> On 2015/06/08 20:22:42, sky wrote:
> > Seems like this should be before window_watcher_.
> 
> Done, but is it because it is set before window_wathcer_ in the .cc file? Or
> there is some other reason?

I would think anything using ash many be using aura/views, and you really want
the delegate destroyed after that.

[mohsen, 2015-06-09 02:17:23]

https://codereview.chromium.org/1159033008/diff/40001/ash/shell/content_clien...
File ash/shell/content_client/shell_browser_main_parts.h (right):

https://codereview.chromium.org/1159033008/diff/40001/ash/shell/content_clien...
ash/shell/content_client/shell_browser_main_parts.h:61:
scoped_ptr<views::ViewsDelegate> views_delegate_;
On 2015/06/08 20:48:03, sky wrote:
> On 2015/06/08 20:46:09, mohsen wrote:
> > On 2015/06/08 20:22:42, sky wrote:
> > > Seems like this should be before window_watcher_.
> > 
> > Done, but is it because it is set before window_wathcer_ in the .cc file? Or
> > there is some other reason?
> 
> I would think anything using ash many be using aura/views, and you really want
> the delegate destroyed after that.

I see. Now, I destroy the delegate in PostMainMessageLoopRun. Please take a
look...

[sky, 2015-06-09 13:13:52]

LGTM

[benwells, 2015-06-10 04:09:22]

lgtm

[mohsen, 2015-06-10 04:45:42]

The CQ bit was checked by mohsen@chromium.org

[mohsen, 2015-06-10 04:45:43]

The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org
Link to the patchset: https://codereview.chromium.org/1159033008/#ps80001
(title: "Destroy ash ViewsDelegate")

[commit-bot: I haz the power, 2015-06-10 04:45:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1159033008/80001

[commit-bot: I haz the power, 2015-06-10 06:49:19]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-06-10 06:51:02]

Patchset 5 (id:??) landed as
https://crrev.com/0ff8c0e1c8c805bce8e216c74c66ccc8fd137a6f
Cr-Commit-Position: refs/heads/master@{#333692}