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

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

Only views::LabelButton uses disabled colors on a views::Label in a
meaningful way. Move the logic into a subclass.

BUG=691891
Review-Url: https://codereview.chromium.org/2913933002
Cr-Commit-Position: refs/heads/master@{#481374}
Committed: https://chromium.googlesource.com/chromium/src/+/e699d8e27d888b832000c598fa10e8788d1042ef

[tapted, 2017-05-31 01:58:05]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-31 01:58:52]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-31 02:02:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-31 02:02:53]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[tapted, 2017-05-31 03:19:26]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-31 03:19:41]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-31 03:22:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-31 03:22:15]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[tapted, 2017-05-31 05:23:29]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-31 05:23:51]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-31 05:55:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-31 05:55:42]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tapted, 2017-05-31 06:42:53]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-31 06:43:07]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-31 07:26:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-31 07:26:40]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tapted, 2017-05-31 08:47:49]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-31 08:48:04]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-31 10:14:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-31 10:14:26]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[tapted, 2017-06-01 07:26:58]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-01 07:27:06]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-01 09:00:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-01 09:00:29]

Dry run: This issue passed the CQ dry run.

[tapted, 2017-06-01 23:39:18]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-01 23:39:59]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-01 23:44:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-01 23:44:50]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[tapted, 2017-06-02 00:51:13]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-02 00:52:08]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-02 02:55:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-02 02:55:48]

Dry run: This issue passed the CQ dry run.

[tapted, 2017-06-06 06:09:58]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-06 06:10:20]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tapted, 2017-06-06 06:41:06]

Patchset #1 (id:1) has been deleted

[tapted, 2017-06-06 06:41:16]

Patchset #1 (id:20001) has been deleted

[tapted, 2017-06-06 06:41:26]

Patchset #1 (id:40001) has been deleted

[tapted, 2017-06-06 06:41:37]

Patchset #1 (id:60001) has been deleted

[tapted, 2017-06-06 06:41:51]

Patchset #3 (id:120001) has been deleted

[tapted, 2017-06-06 06:41:59]

Patchset #3 (id:140001) has been deleted

[tapted, 2017-06-06 06:42:08]

Patchset #3 (id:160001) has been deleted

[tapted, 2017-06-06 06:48:56]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-06 06:49:17]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tapted, 2017-06-06 07:15:48]

Description was changed from

==========
Make views::LabelButtonLabel

BUG=691891
==========

to

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

Only views::LabelButton uses disabled colors on a views::Label in a
meaningful way. Move the logic into a subclass.

BUG=691891
==========

[commit-bot: I haz the power, 2017-06-06 08:24:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-06 08:24:19]

Dry run: This issue passed the CQ dry run.

[tapted, 2017-06-06 12:30:23]

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

[tapted, 2017-06-06 12:30:24]

Hi sky, please take a look

https://codereview.chromium.org/2913933002/diff/200001/ui/views/controls/labe...
File ui/views/controls/label.cc (left):

https://codereview.chromium.org/2913933002/diff/200001/ui/views/controls/labe...
ui/views/controls/label.cc:437: ApplyTextColors();
I tested and ran jobs in earlier patchsets with this just NOTREACHED(). (and
with LabelButtonLabel jumping over it). There's just
LabelTest.NoSchedulePaintInOnPaint that calls SetEnabled(false) on a
non-LabelButton Label as far as I can tell - nothing interesting.

[sky, 2017-06-06 17:12:25]

https://codereview.chromium.org/2913933002/diff/200001/ui/views/controls/butt...
File ui/views/controls/button/label_button_label.h (right):

https://codereview.chromium.org/2913933002/diff/200001/ui/views/controls/butt...
ui/views/controls/button/label_button_label.h:16: class LabelButtonLabel :
public Label {
Please document this is an implementation detail, and not public. Also, can you
move the header into a section in the BUILD.gn so that only views code can use
it?

https://codereview.chromium.org/2913933002/diff/200001/ui/views/controls/label.h
File ui/views/controls/label.h (right):

https://codereview.chromium.org/2913933002/diff/200001/ui/views/controls/labe...
ui/views/controls/label.h:235: virtual SkColor GetForegroundColor() const;
I would prefer not to have this virtual. With this function virtual it's easy to
miss that you need to trigger a paint when the value changes. For this reason I
prefer an explicit setter.

[tapted, 2017-06-07 08:25:11]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-07 08:25:21]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tapted, 2017-06-07 08:49:56]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-07 08:50:12]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-07 08:53:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-07 08:53:58]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[tapted, 2017-06-07 08:58:43]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-07 08:59:11]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-07 09:02:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-07 09:02:41]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[tapted, 2017-06-07 11:31:06]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-07 11:31:14]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tapted, 2017-06-07 11:43:06]

Patchset #5 (id:220001) has been deleted

[tapted, 2017-06-07 12:52:54]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-07 12:53:39]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tapted, 2017-06-07 13:32:27]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-07 13:32:42]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-07 14:58:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-07 14:58:22]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[tapted, 2017-06-08 01:42:58]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-08 01:43:22]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-08 03:32:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-08 03:32:36]

Dry run: This issue passed the CQ dry run.

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

https://codereview.chromium.org/2913933002/diff/200001/ui/views/controls/butt...
File ui/views/controls/button/label_button_label.h (right):

https://codereview.chromium.org/2913933002/diff/200001/ui/views/controls/butt...
ui/views/controls/button/label_button_label.h:16: class LabelButtonLabel :
public Label {
On 2017/06/06 17:12:24, sky wrote:
> Please document this is an implementation detail, and not public. Also, can
you
> move the header into a section in the BUILD.gn so that only views code can use
> it?

Done. Although the second bit was tricky - see later comments and comments in
views/BUILD.gn

https://codereview.chromium.org/2913933002/diff/200001/ui/views/controls/label.h
File ui/views/controls/label.h (right):

https://codereview.chromium.org/2913933002/diff/200001/ui/views/controls/labe...
ui/views/controls/label.h:235: virtual SkColor GetForegroundColor() const;
On 2017/06/06 17:12:24, sky wrote:
> I would prefer not to have this virtual. With this function virtual it's easy
to
> miss that you need to trigger a paint when the value changes. For this reason
I
> prefer an explicit setter.

Done. Also added a test (seemed almost too simple ;)... ) - which also allowed
me to ensure `views_internal` was set up in a way that doesn't prohibit testing.

https://codereview.chromium.org/2913933002/diff/340001/ui/views/BUILD.gn
File ui/views/BUILD.gn (right):

https://codereview.chromium.org/2913933002/diff/340001/ui/views/BUILD.gn#newc...
ui/views/BUILD.gn:703: }
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.
"""

https://codereview.chromium.org/2913933002/diff/340001/ui/views/BUILD.gn#newc...
ui/views/BUILD.gn:956: ":views",
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.

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

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

[sky, 2017-06-08 19:06:15]

+brettw

https://codereview.chromium.org/2913933002/diff/340001/ui/views/BUILD.gn
File ui/views/BUILD.gn (right):

https://codereview.chromium.org/2913933002/diff/340001/ui/views/BUILD.gn#newc...
ui/views/BUILD.gn:672: static_library("views_internal") {
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.

[tapted, 2017-06-13 21:08:24]

brettw: ping?

[brettw, 2017-06-13 22:33:58]

https://codereview.chromium.org/2913933002/diff/340001/ui/views/BUILD.gn
File ui/views/BUILD.gn (right):

https://codereview.chromium.org/2913933002/diff/340001/ui/views/BUILD.gn#newc...
ui/views/BUILD.gn:701: "controls/button/label_button_label.h",
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 00:53:50]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-14 00:54:05]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-14 00:59:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-14 00:59:37]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tapted, 2017-06-14 02:45:56]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-14 02:46:15]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-14 02:50:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-14 02:50:59]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tapted, 2017-06-14 03:48:28]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-14 03:48:45]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-14 03:54:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-14 03:54:09]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)

[tapted, 2017-06-14 05:11:17]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-14 05:11:26]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-14 06:05:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-14 06:05:22]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tapted, 2017-06-14 06:37:22]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-14 06:37:36]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-14 08:35:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-14 08:36:00]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

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

https://codereview.chromium.org/2913933002/diff/340001/ui/views/BUILD.gn
File ui/views/BUILD.gn (right):

https://codereview.chromium.org/2913933002/diff/340001/ui/views/BUILD.gn#newc...
ui/views/BUILD.gn:701: "controls/button/label_button_label.h",
On 2017/06/13 22:33:57, brettw wrote:
> 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?

Yup - exactly.

> 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.

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.

https://codereview.chromium.org/2913933002/diff/480001/ui/views/BUILD.gn
File ui/views/BUILD.gn (right):

https://codereview.chromium.org/2913933002/diff/480001/ui/views/BUILD.gn#newc...
ui/views/BUILD.gn:448: ":views_vector_icons",
I had to move this.. but for generated headers this is often the right place
anyway

ui/views/vector_icons.h is added by

sources += get_target_outputs(":views_vector_icons")

adding a public =[] list means that it's not usable by dependants and gn check
fails in a bunch of places. But just adding `vector_icons.h` to the public =[]
list causes a different error in check_gn_headers.py, saying it should be
removed:

"""Running ['/usr/bin/python', '/b/c/b/linux/src/build/check_gn_headers.py',
'--out-dir', u'/b/c/b/linux/src/out/Release', '--whitelist',
'/b/c/b/linux/src/build/check_gn_headers_whitelist.txt', '--json',
'/tmp/tmpsQjx3x', '--verbose'] in '/b/c/b/linux/src' (env: None)
The following non-existing files should be removed from gn files:
ui/views/vector_icons.h
"""

Hence, this.

https://codereview.chromium.org/2913933002/diff/480001/ui/views/BUILD.gn#newc...
ui/views/BUILD.gn:475: ]
not sure whether it's necessary to maintain removals from `public` this way.
I've gone for consistency for now.

https://codereview.chromium.org/2913933002/diff/480001/ui/views/BUILD.gn#newc...
ui/views/BUILD.gn:683: "accessibility/native_view_accessibility_base.h",
If a section didn't raise a gn check I didn't split it, but this is one used in
tests, so it appears in views_unittests, under the same conditions.

[tapted, 2017-06-21 02:30:53]

Patchset #2 (id:100001) has been deleted

[tapted, 2017-06-21 02:31:04]

Patchset #2 (id:180001) has been deleted

[tapted, 2017-06-21 02:31:35]

Patchset #3 (id:240001) has been deleted

[tapted, 2017-06-21 02:31:45]

Patchset #3 (id:260001) has been deleted

[tapted, 2017-06-21 02:32:34]

Patchset #10 (id:420001) has been deleted

[tapted, 2017-06-21 02:32:44]

Patchset #10 (id:440001) has been deleted

[tapted, 2017-06-21 02:32:58]

Patchset #10 (id:460001) has been deleted

[tapted, 2017-06-21 02:39:38]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-21 02:39:49]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tapted, 2017-06-21 02:47:51]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-21 02:48:09]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-21 04:16:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-21 04:16:11]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tapted, 2017-06-21 07:08:30]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-21 07:08:43]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-21 07:55:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-21 07:55:23]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[tapted, 2017-06-21 09:18:43]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-21 09:19:00]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-21 10:17:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-21 10:17:10]

Dry run: This issue passed the CQ dry run.

[tapted, 2017-06-21 10:33:18]

sky: wdyt? I've rebased and copied the comments from patchset 10 for the
approach using `public` sources lists. This is patchset 13.

I've put an alternative in patchset 12 as well. I simplified it a bit versus the
one reviewed earlier in patchset 6 (removed the ":test_headers" target and just
listed the required things to ":views_unittests_sources" along with a comment
pointing to the bug brettw filed).


https://codereview.chromium.org/2913933002/diff/340001/ui/views/BUILD.gn#newc...
is the earlier thread with brettw

https://codereview.chromium.org/2913933002/diff/540001/ui/views/BUILD.gn
File ui/views/BUILD.gn (right):

https://codereview.chromium.org/2913933002/diff/540001/ui/views/BUILD.gn#newc...
ui/views/BUILD.gn:447: ":views_vector_icons",
(copied from patchset 10)
I had to move this.. but for generated headers this is often the right place
anyway

ui/views/vector_icons.h is added by

sources += get_target_outputs(":views_vector_icons")

adding a public =[] list means that it's not usable by dependants and gn check
fails in a bunch of places. But just adding `vector_icons.h` to the public =[]
list causes a different error in check_gn_headers.py, saying it should be
removed:

"""Running ['/usr/bin/python', '/b/c/b/linux/src/build/check_gn_headers.py',
'--out-dir', u'/b/c/b/linux/src/out/Release', '--whitelist',
'/b/c/b/linux/src/build/check_gn_headers_whitelist.txt', '--json',
'/tmp/tmpsQjx3x', '--verbose'] in '/b/c/b/linux/src' (env: None)
The following non-existing files should be removed from gn files:
ui/views/vector_icons.h
"""

Hence, this.

https://codereview.chromium.org/2913933002/diff/540001/ui/views/BUILD.gn#newc...
ui/views/BUILD.gn:474: ]
(copied from patchset 10) not sure whether it's necessary to maintain removals
from `public` this way.
I've gone for consistency for now.

https://codereview.chromium.org/2913933002/diff/540001/ui/views/BUILD.gn#newc...
ui/views/BUILD.gn:683: "accessibility/native_view_accessibility_mac.h",
(copied from patchset 10) If a section didn't raise a gn check I didn't split
it, but this is one used in
tests, so it appears in views_unittests, under the same conditions.

[sky, 2017-06-21 15:00:12]

LGTM

[tapted, 2017-06-22 00:23:18]

The CQ bit was checked by tapted@chromium.org

[commit-bot: I haz the power, 2017-06-22 00:23:37]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-22 00:28:17]

CQ is committing da patch.

Bot data: {"patchset_id": 540001, "attempt_start_ts": 1498090998857620,
"parent_rev": "e0d1cc35ae732bd9dabc6393900c278dbb045245", "commit_rev":
"e699d8e27d888b832000c598fa10e8788d1042ef"}

[commit-bot: I haz the power, 2017-06-22 00:28:33]

Description was changed from

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

Only views::LabelButton uses disabled colors on a views::Label in a
meaningful way. Move the logic into a subclass.

BUG=691891
==========

to

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

Only views::LabelButton uses disabled colors on a views::Label in a
meaningful way. Move the logic into a subclass.

BUG=691891

Review-Url: https://codereview.chromium.org/2913933002
Cr-Commit-Position: refs/heads/master@{#481374}
Committed:
https://chromium.googlesource.com/chromium/src/+/e699d8e27d888b832000c598fa10...
==========

[commit-bot: I haz the power, 2017-06-22 00:28:35]

Committed patchset #13 (id:540001) as
https://chromium.googlesource.com/chromium/src/+/e699d8e27d888b832000c598fa10...