Move views::Label DisabledColor logic into views::LabelButtonLabel

ui/views/BUILD.gn

Index: ui/views/BUILD.gn
diff --git a/ui/views/BUILD.gn b/ui/views/BUILD.gn
index 80cdae18bfa651b749538015a445e428422bb21f..b514011d74cb91a584d5f0612497605a1b982c32 100644
--- a/ui/views/BUILD.gn
+++ b/ui/views/BUILD.gn
@@ -31,6 +31,7 @@ aggregate_vector_icons("views_vector_icons") {
 }
 
 component("views") {
+  allow_circular_includes_from = [ ":views_internal" ]
   all_dependent_configs = [ ":flags" ]
   sources = [
     "accessibility/native_view_accessibility.h",
@@ -412,6 +413,7 @@ component("views") {
   defines = [ "VIEWS_IMPLEMENTATION" ]
 
   deps = [
+    ":views_internal",
     ":views_vector_icons",
     "//base:i18n",
     "//base/third_party/dynamic_annotations",
@@ -667,6 +669,39 @@ component("views") {
   }
 }
 
+static_library("views_internal") {

[sky, 2017/06/08 19:06:14]

This seems overly complex, but I could certainly be wrong. I talked with Brett
briefly about it and he felt there *may* be a cleaner way. He tried explaining
it, but I told him to comment on this bug.

+  # External code should depend upon "views".
+  visibility = [ "./*" ]
+
+  sources = [
+    "controls/button/label_button_label.cc",
+    "controls/button/label_button_label.h",
+  ]
+
+  defines = [ "VIEWS_IMPLEMENTATION" ]
+
+  deps = [
+    "//base",
+    "//skia:skia",
+    "//ui/accessibility:ax_gen",
+    "//ui/gfx",
+  ]
+}
+
+# Lists headers in :views_internal that are used in :views_unittests. This is
+# needed to satisfy gn check because :views_unittests can't depend on both
+# :views and :views_internal without linking in :views_internal twice and
+# causing duplicate symbols. This happens because when :views is a DLL and
+# :views_internal is a static library, the usual symbol resolution breaks.
+source_set("test_headers") {
+  visibility = [ "./*" ]
+  check_includes = false  # Deps are checked in :views_internal.
+
+  sources = [
+    "controls/button/label_button_label.h",

[brettw, 2017/06/13 22:33:57]

What's the ever higher level goal here? You want views and views_unittests to be
able to include label_button_label.h but not external code? If this is your
goal:

The build support for this kind of sucks, unfortunately. Theoretically "public"
is for this, but without some support for "friend", the case you're running into
with tests is difficult.

I've thought about this before but didn't do anything. I filed
https://bugs.chromium.org/p/chromium/issues/detail?id=732993 describing what I
think the right solution for GN is.

In this case, I would add this as a private header to "views" by renaming the
current sources list "public" and adding your new ones in "sources" below it.
Then add the label_button_label.h as a source in the unit test target (listing
it again) with a TODO that references by above bug.

[tapted, 2017/06/14 09:14:09]

Yup - exactly.
Done.. I think I'm holding it right. `public = []` seems designed only for
header files (at least, if I put a .cc in there, it doesn't get linked), so I've
split the lists. But, apart from the new file, I didn't put too much effort into
trying to categorize them as public/internal at this point.

+  ]
+}

[tapted, 2017/06/08 04:11:32]

You can see the other things I attempted in earlier patchsets... I noted the
approach for test_support_internal, but I didn't copy it. I don't think it
achieves the desired goal (see below).

Listing the header in multiple targets is annoying, but the gn reference alludes
to this approach, e.g., at 

https://chromium.googlesource.com/chromium/src/tools/gn/+/HEAD/docs/reference...

"""
 For an include to be valid:
 ...
    - There can be multiple targets with an included file: only one needs to be
      valid for the include to be allowed.
"""

and

https://chromium.googlesource.com/chromium/src/tools/gn/+/HEAD/docs/reference...

"""
  In rare cases it makes sense to list a header in more than one target if it
  could be considered conceptually a member of both. libraries.
"""

+
 static_library("test_support_internal") {
   testonly = true
   sources = [
@@ -857,6 +892,7 @@ source_set("views_unittests_sources") {
     "controls/button/custom_button_unittest.cc",
     "controls/button/image_button_factory_unittest.cc",
     "controls/button/image_button_unittest.cc",
+    "controls/button/label_button_label_unittest.cc",
     "controls/button/label_button_unittest.cc",
     "controls/button/menu_button_unittest.cc",
     "controls/button/toggle_button_unittest.cc",
@@ -915,7 +951,9 @@ source_set("views_unittests_sources") {
   # Make all deps in this target public so both views_unittests and
   # views_mus_unittests will get them.
   public_deps = [
+    ":test_headers",
     ":test_support",
+    ":views",

[tapted, 2017/06/08 04:11:32]

One problem with the approach used by ":test_support" / ":test_support_internal"
can be seen here. I.e. clearly the files above should have a dependency on
:views, but `gn check` does not complain.

Anything depending on //ui/views/views:test_support gets a "free" :views
dependency because test_support_*internal*_ lists it as a public dep, and it
propagates up. I don't think test_support_internal is enforcing any kind of
implementation hiding right now.

I think `allow_circular_includes_from` is the right approach. Splitting the
:views files in two lists creates a cycle which is unavoidable without
unnecessary additional layering.

That still leaves the DLL problem. test_support files are all statically linked
so don't encounter this issue. I couldn't find any solution that would make
Windows happy that involved any target apart from views.dll depending on
views_internal.
 views.dll _must_ depend on views_internal in order for gn to permit naming it
in `allow_circular_includes_from`. But adding views_internal to any other
targets causes link errors. We still want to enforce `gn check` for
views_unittests, hence the extra "test_headers" target.


Maybe there's something I've missed.. Or maybe this is already all obvious to
you and I needn't have explained so much :). It did seem a bit non-intuitive
though.

     "//base",
     "//base:i18n",
     "//base/test:test_support",