Refactor ViewsDelegate and MD-ify the icon-to-text spacing for checkbox and radiobutton

chrome/browser/ui/views/chrome_views_delegate_win.cc

 // Copyright 2017 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/views/chrome_views_delegate.h"
 
 #include <dwmapi.h>
 #include <shellapi.h>
 
 #include "base/profiler/scoped_tracker.h"
@@ skipping 86 matching lines @@
     edges |= views::ViewsDelegate::EDGE_LEFT;
   if (MonitorHasAutohideTaskbarForEdge(ABE_TOP, monitor))
     edges |= views::ViewsDelegate::EDGE_TOP;
   if (MonitorHasAutohideTaskbarForEdge(ABE_RIGHT, monitor))
     edges |= views::ViewsDelegate::EDGE_RIGHT;
   if (MonitorHasAutohideTaskbarForEdge(ABE_BOTTOM, monitor))
     edges |= views::ViewsDelegate::EDGE_BOTTOM;
   return edges;
 }
 
+ChromeViewsDelegate* views_delegate = nullptr;
+
 }  // namespace
 
 ChromeViewsDelegate::ChromeViewsDelegate()
-    : in_autohide_edges_callback_(false), weak_factory_(this) {}
+    : in_autohide_edges_callback_(false), weak_factory_(this) {

[Peter Kasting, 2017/03/09 05:52:11]

For this function, it looks like the only reason it needs to be in this file is
because of the member references to win-only members.  But as a consequence, we
need to duplicate the body of the function and the file-local |views_delegate|
object into this file.  I suggest removing this, and instead in the header that
declares these members, using default member initializers to avoid the need to
give a member initializer list in any C++ file.

[kylix_rd, 2017/03/09 15:02:31]

Can weak_factory_ be initialized with "this" using the default member
initializers? Because of the difference constructor and that it references
views_delegate, the other methods which also reference it must also be moved
since views_delegate has no public linkage.

I added the GetInstance() static and the views_delegate reference in order to
avoid performing a very unsafe static_cast<> of the ViewsDelegate::GetInstance()
return value. Having separate ChromeViewsDelegate::GetInstance() ensures that it
will actually return an instance of ChromeViewsDelegate or nullptr if it weren't
constructed.

[Peter Kasting, 2017/03/09 18:26:48]

It should be able to, AFAIK.  Such initializers are basically expressions that
run in the context of the constructor when it executes, unless they're
overridden by an explicit member initializer.
Ah, right.  That answers my other question.

Yes, I'd try to do "= this" in the header, to avoid the need for all this.  If
that doesn't work, I'd use an #ifdef in the cross-platform .cc, since on balance
that seems better than having to hoist so much other duplicated stuff to this
file.

[kylix_rd, 2017/03/10 15:10:40]

weak_factory_ takes "this" as a constructor parameter, so a simple assignment
won't work (tried it). It will work with in_autohide_edges_callback_, but since
weak_factory_ must still be handled in the constructor initializer list, it
might as well remain there as well.

I've moved the duplicated code back to chrome_views_delegate.cc and merely added
an #if..#else around the different constructor definition and initializers.

[Peter Kasting, 2017/03/10 20:44:54]

Oh, due to the constructor being marked explicit?

If so it might have worked to do "=
base::WeakPtrFactory<ChromeViewsDelegate>(this)".  But that's kind of ugly.  Up
to you if you think it would be a win and want to try it in a future CL :)

+  DCHECK(!views_delegate);
+  views_delegate = this;
+}
+
+ChromeViewsDelegate::~ChromeViewsDelegate() {

[Peter Kasting, 2017/03/09 05:52:11]

For these two functions, the implementations look the same as in the
cross-platform class.  Why do they have to be pulled out to the _win file?

[kylix_rd, 2017/03/09 15:02:31]

Since views_delegate is also defined in chrome_views_delegate.cc, it doesn't
have public linkage (to ensure it is only obtained via the GetInstance()
method). The constructor for the Windows version of the delegate must also
initialize some extra fields and initialize the views_delegate global. It looks
like the chrome_views_delegate_xxx.cc modules were added in order to minimize
the #ifdef's within chrome_views_delegate.cc.

+  DCHECK_EQ(0u, ref_count_);
+
+  DCHECK_EQ(this, views_delegate);
+  views_delegate = nullptr;
+}
+
+ChromeViewsDelegate* ChromeViewsDelegate::GetInstance() {
+  return views_delegate;
+}
 
 HICON ChromeViewsDelegate::GetDefaultWindowIcon() const {
   return GetAppIcon();
 }
 
 HICON ChromeViewsDelegate::GetSmallWindowIcon() const {
   return GetSmallAppIcon();
 }
 
 views::NativeWidget* ChromeViewsDelegate::CreateNativeWidget(
@@ skipping 65 matching lines @@
     HMONITOR monitor,
     int returned_edges,
     int edges) {
   appbar_autohide_edge_map_[monitor] = edges;
   if (returned_edges == edges)
     return;
 
   base::AutoReset<bool> in_callback_setter(&in_autohide_edges_callback_, true);
   callback.Run();
 }