|
|
DescriptionMove 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
Patch Set 1 : NoSchedulePaintInOnPaint #Patch Set 2 : missed-some #
Total comments: 5
Patch Set 3 : respond to comments, self review, fix DLLs? Also text is not black on non-MD Aura :o #Patch Set 4 : source_set? Pretty sure that will not help #Patch Set 5 : Maybe this is the magic incantation #Patch Set 6 : OK.. I think I found a solution #
Total comments: 5
Patch Set 7 : reabse #Patch Set 8 : Use public #Patch Set 9 : public does not work that way #Patch Set 10 : Fix win, linux. Nit. TODO. Fix vector icons #
Total comments: 3
Patch Set 11 : Rebase for r479616 #Patch Set 12 : Simpler alternative? #Patch Set 13 : Back to PatchSet 11 #
Total comments: 3
Messages
Total messages: 126 (115 generated)
The CQ bit was checked by tapted@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by tapted@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by tapted@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by tapted@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by tapted@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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...)
The CQ bit was checked by tapted@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by tapted@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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...)
The CQ bit was checked by tapted@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by tapted@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #3 (id:120001) has been deleted
Patchset #3 (id:140001) has been deleted
Patchset #3 (id:160001) has been deleted
The CQ bit was checked by tapted@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
tapted@chromium.org changed reviewers: + sky@chromium.org
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.
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.
The CQ bit was checked by tapted@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by tapted@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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...)
The CQ bit was checked by tapted@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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...)
The CQ bit was checked by tapted@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #5 (id:220001) has been deleted
The CQ bit was checked by tapted@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by tapted@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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...)
The CQ bit was checked by tapted@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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@chromium.org changed reviewers: + brettw@chromium.org
+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.
brettw: ping?
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.
The CQ bit was checked by tapted@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by tapted@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by tapted@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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-...)
The CQ bit was checked by tapted@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by tapted@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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...)
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.
Patchset #2 (id:100001) has been deleted
Patchset #2 (id:180001) has been deleted
Patchset #3 (id:240001) has been deleted
Patchset #3 (id:260001) has been deleted
Patchset #10 (id:420001) has been deleted
Patchset #10 (id:440001) has been deleted
Patchset #10 (id:460001) has been deleted
The CQ bit was checked by tapted@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by tapted@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by tapted@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by tapted@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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.
LGTM
The CQ bit was checked by tapted@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 540001, "attempt_start_ts": 1498090998857620, "parent_rev": "e0d1cc35ae732bd9dabc6393900c278dbb045245", "commit_rev": "e699d8e27d888b832000c598fa10e8788d1042ef"}
Message was sent while issue was closed.
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... ==========
Message was sent while issue was closed.
Committed patchset #13 (id:540001) as https://chromium.googlesource.com/chromium/src/+/e699d8e27d888b832000c598fa10... |