Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2120273002:
Initialize MaterialDesignController in NativeThemeMacTest (Closed)
|
Created:
4 years, 5 months ago by ckulakowski Modified:
4 years, 4 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionInitialize MaterialDesignController in NativeThemeMacTest
This is fix for crash (CHECK) in NativeThemeMacTest.SystemColorsExist:
FATAL:material_design_controller.cc(66)] Check failed: is_mode_initialized_
BUG=625642
Committed: https://crrev.com/dbf59fde3433f5fb453ec98c5614fb66741e3eed
Cr-Commit-Position: refs/heads/master@{#410108}
Patch Set 1 #Patch Set 2 : Fixup for component build #
Total comments: 2
Patch Set 3 : Use more generic solution #
Messages
Total messages: 34 (9 generated)
Description was changed from ========== Initialize MaterialDesignController in NativeThemeMacTest This is fix for crash (CHECK) in NativeThemeMacTest.SystemColorsExist: FATAL:material_design_controller.cc(66)] Check failed: is_mode_initialized_ BUG=625642 ========== to ========== Initialize MaterialDesignController in NativeThemeMacTest This is fix for crash (CHECK) in NativeThemeMacTest.SystemColorsExist: FATAL:material_design_controller.cc(66)] Check failed: is_mode_initialized_ BUG=625642 ==========
ckulakowski@opera.com changed reviewers: + pkasting@chromium.org
https://codereview.chromium.org/2120273002/diff/20001/ui/native_theme/native_... File ui/native_theme/native_theme_mac_unittest.cc (right): https://codereview.chromium.org/2120273002/diff/20001/ui/native_theme/native_... ui/native_theme/native_theme_mac_unittest.cc:17: MaterialDesignController::Initialize(); Wouldn't we need this in the next test too? In fact in general wouldn't we potentially need this any time we access the Mac theme APIs? I think the solution for this probably needs to be more general. You should work with a Mac engineer like shrike@ or spqchan@ to figure out the right solution.
ckulakowski@opera.com changed reviewers: + shrike@chromium.org, spqchan@chromium.org
On 2016/07/06 20:57:07, Peter Kasting wrote: > https://codereview.chromium.org/2120273002/diff/20001/ui/native_theme/native_... > File ui/native_theme/native_theme_mac_unittest.cc (right): > > https://codereview.chromium.org/2120273002/diff/20001/ui/native_theme/native_... > ui/native_theme/native_theme_mac_unittest.cc:17: > MaterialDesignController::Initialize(); > Wouldn't we need this in the next test too? > > In fact in general wouldn't we potentially need this any time we access the Mac > theme APIs? > > I think the solution for this probably needs to be more general. You should > work with a Mac engineer like shrike@ or spqchan@ to figure out the right > solution. The next test does not crash. That's the only test which has this problem (as far as I know).
On 2016/07/07 07:59:32, ckulakowski wrote: > On 2016/07/06 20:57:07, Peter Kasting wrote: > > > https://codereview.chromium.org/2120273002/diff/20001/ui/native_theme/native_... > > File ui/native_theme/native_theme_mac_unittest.cc (right): > > > > > https://codereview.chromium.org/2120273002/diff/20001/ui/native_theme/native_... > > ui/native_theme/native_theme_mac_unittest.cc:17: > > MaterialDesignController::Initialize(); > > Wouldn't we need this in the next test too? > > > > In fact in general wouldn't we potentially need this any time we access the > Mac > > theme APIs? > > > > I think the solution for this probably needs to be more general. You should > > work with a Mac engineer like shrike@ or spqchan@ to figure out the right > > solution. > > The next test does not crash. That's the only test which has this problem (as > far as I know). The test not crashing does not at all reassure me that we don't need this elsewhere. For example, is it not crashing because it simply doesn't happen to access the wrong property, but could be changed to do so later? Is it not crashing because it's happening to be run after you initialize things? Etc. There needs to be a comprehensive explanation here of exactly what's going wrong and why it would be incorrect to fix this in a more general place before I could accept this test.
On 2016/07/07 08:01:45, Peter Kasting wrote: > before I could > accept this test. ...this fix*.
On 2016/07/07 08:02:00, Peter Kasting wrote: > On 2016/07/07 08:01:45, Peter Kasting wrote: > > before I could > > accept this test. > > ...this fix*. Test SystemColorsExist crashes because it calls ui::NativeThemeMac::GetSystemColor. GetSystemColor calls GetAuraColor which assumes that material design is initialized.
On 2016/07/07 12:54:39, ckulakowski wrote: > On 2016/07/07 08:02:00, Peter Kasting wrote: > > On 2016/07/07 08:01:45, Peter Kasting wrote: > > > before I could > > > accept this test. > > > > ...this fix*. > > Test SystemColorsExist crashes because it calls > ui::NativeThemeMac::GetSystemColor. GetSystemColor calls GetAuraColor which > assumes that material design is initialized. Yes, but the next test also calls that. It's just a matter of which color constant is used. And other such tests could, in principle, call this as well. This should be initialized in general, not just for this test. P.S.: When you reply to comments on the diffs in the future please do so using the inline reply tool on the diff itself rather than by email or the global reply window, so the whole conversation stays in context. I forgot to mention this on your previous reply.
https://codereview.chromium.org/2120273002/diff/20001/ui/native_theme/native_... File ui/native_theme/native_theme_mac_unittest.cc (right): https://codereview.chromium.org/2120273002/diff/20001/ui/native_theme/native_... ui/native_theme/native_theme_mac_unittest.cc:17: MaterialDesignController::Initialize(); On 2016/07/06 20:57:07, Peter Kasting wrote: > Wouldn't we need this in the next test too? > > In fact in general wouldn't we potentially need this any time we access the Mac > theme APIs? > > I think the solution for this probably needs to be more general. You should > work with a Mac engineer like shrike@ or spqchan@ to figure out the right > solution. I agree with pkasting@ that it should be more general. I believe that if you create a test subclass, call MaterialDesignController::Initialize() from Setup(), and then convert the TEST()s to TEST_F()s you should be all set. Check out testing/gtest/include/gtest/gtest.h:348 for more info.
On 2016/07/07 19:40:47, shrike wrote: > https://codereview.chromium.org/2120273002/diff/20001/ui/native_theme/native_... > File ui/native_theme/native_theme_mac_unittest.cc (right): > > https://codereview.chromium.org/2120273002/diff/20001/ui/native_theme/native_... > ui/native_theme/native_theme_mac_unittest.cc:17: > MaterialDesignController::Initialize(); > On 2016/07/06 20:57:07, Peter Kasting wrote: > > Wouldn't we need this in the next test too? > > > > In fact in general wouldn't we potentially need this any time we access the > Mac > > theme APIs? > > > > I think the solution for this probably needs to be more general. You should > > work with a Mac engineer like shrike@ or spqchan@ to figure out the right > > solution. > > I agree with pkasting@ that it should be more general. I believe that if you > create a test subclass, call MaterialDesignController::Initialize() from > Setup(), and then convert the TEST()s to TEST_F()s you should be all set. Check > out testing/gtest/include/gtest/gtest.h:348 for more info. Ping.
On 2016/07/19 07:40:14, ckulakowski wrote: > Ping. Hello ckulakowski, Please see my paragraph at the end of comment #11 - it includes my recommendation for how you should proceed with this cl.
On 2016/07/19 16:09:51, shrike wrote: > On 2016/07/19 07:40:14, ckulakowski wrote: > > Ping. > > Hello ckulakowski, > > Please see my paragraph at the end of comment #11 - it includes my > recommendation for how you should proceed with this cl. I've already applied your recommendations. With one exception: I use SetUpTestCase() instead of Setup(). Setup is called before every test and all unittests are started in one thread (one after another) so MaterialDesignController::Initialize would be called twice (and it shouldn't).
On 2016/07/19 16:16:43, ckulakowski wrote: > I've already applied your recommendations. With one exception: I use > SetUpTestCase() instead of Setup(). Setup is called before every test and all > unittests are started in one thread (one after another) so > MaterialDesignController::Initialize would be called twice (and it shouldn't). Then instead of "ping" did you mean "PTAL"?
On 2016/07/19 16:19:33, shrike wrote: > On 2016/07/19 16:16:43, ckulakowski wrote: > > I've already applied your recommendations. With one exception: I use > > SetUpTestCase() instead of Setup(). Setup is called before every test and all > > unittests are started in one thread (one after another) so > > MaterialDesignController::Initialize would be called twice (and it shouldn't). > > Then instead of "ping" did you mean "PTAL"? Right :). PTAL at patch set 3.
lgtm (though I'm not an owner)
The CQ bit was checked by ckulakowski@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/07/19 16:27:56, shrike wrote: > lgtm (though I'm not an owner) As I said, I'm not an owner. I'm guessing that's why pkasting@ is a reviewer. You will need approval from him for this change to actually land.
The CQ bit was unchecked by ckulakowski@opera.com
I'm not actually a ui/ OWNER either, just c/b/ui/. Sorry :(
On 2016/07/19 22:46:10, Peter Kasting wrote: > I'm not actually a ui/ OWNER either, just c/b/ui/. Sorry :( Ok, so who can approve this?
On 2016/07/20 09:30:28, ckulakowski wrote: > On 2016/07/19 22:46:10, Peter Kasting wrote: > > I'm not actually a ui/ OWNER either, just c/b/ui/. Sorry :( > > Ok, so who can approve this? git cl owners ?
ckulakowski@opera.com changed reviewers: + estade@chromium.org
On 2016/07/20 16:12:17, shrike wrote: > On 2016/07/20 09:30:28, ckulakowski wrote: > > On 2016/07/19 22:46:10, Peter Kasting wrote: > > > I'm not actually a ui/ OWNER either, just c/b/ui/. Sorry :( > > > > Ok, so who can approve this? > > git cl owners ? Estade: please review.
On 2016/07/21 07:56:19, ckulakowski wrote: > On 2016/07/20 16:12:17, shrike wrote: > > On 2016/07/20 09:30:28, ckulakowski wrote: > > > On 2016/07/19 22:46:10, Peter Kasting wrote: > > > > I'm not actually a ui/ OWNER either, just c/b/ui/. Sorry :( > > > > > > Ok, so who can approve this? > > > > git cl owners ? > > Estade: please review. Estade: PTAL
lgtm
The CQ bit was checked by ckulakowski@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Initialize MaterialDesignController in NativeThemeMacTest This is fix for crash (CHECK) in NativeThemeMacTest.SystemColorsExist: FATAL:material_design_controller.cc(66)] Check failed: is_mode_initialized_ BUG=625642 ========== to ========== Initialize MaterialDesignController in NativeThemeMacTest This is fix for crash (CHECK) in NativeThemeMacTest.SystemColorsExist: FATAL:material_design_controller.cc(66)] Check failed: is_mode_initialized_ BUG=625642 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Initialize MaterialDesignController in NativeThemeMacTest This is fix for crash (CHECK) in NativeThemeMacTest.SystemColorsExist: FATAL:material_design_controller.cc(66)] Check failed: is_mode_initialized_ BUG=625642 ========== to ========== Initialize MaterialDesignController in NativeThemeMacTest This is fix for crash (CHECK) in NativeThemeMacTest.SystemColorsExist: FATAL:material_design_controller.cc(66)] Check failed: is_mode_initialized_ BUG=625642 Committed: https://crrev.com/dbf59fde3433f5fb453ec98c5614fb66741e3eed Cr-Commit-Position: refs/heads/master@{#410108} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/dbf59fde3433f5fb453ec98c5614fb66741e3eed Cr-Commit-Position: refs/heads/master@{#410108} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
