Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2442223002:
Views: Give ScrollViewTest a test harness. (Closed)
DescriptionViews: Give ScrollViewTest a test harness.
Currently the tests are all TEST rather than TEST_F.
Add a test harness (TEST_F) to reduce some boilerplate and encapsulate
some subtle setup code required for overlay scrollers on Mac.
Part of this is to do with ScopedPreferredScrollerStyle: Interleaving
swizzlers like `std::unique_ptr x(new X1); x.reset(new X2);` causes
incorrect behaviour and leakage of the swizzled methods across tests run
in the same process. Specifically, X1's destructor restores the original
methods, but then X2's destructor will restore X1's swizzled methods,
since they were in effect when X2 was constructed. Make it harder for
devs to shoot themselves in the foot with this.
BUG=615948
Committed: https://crrev.com/5a1cee4f8acd9731b7af2c616b42aa892fdfdd2e
Cr-Commit-Position: refs/heads/master@{#427253}
Patch Set 1 #
Total comments: 5
Patch Set 2 : respond to comments #
Dependent Patchsets: Messages
Total messages: 21 (15 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: This issue passed the CQ dry run.
Description was changed from ========== Views: Give ScrollViewTest a test harness. Reduce some boilerplate and encapsulate some subtle setup code required for overlay scrollers on Mac. BUG=615948 ========== to ========== Views: Give ScrollViewTest a test harness. Currently the tests are all TEST rather than TEST_F. Add a test harness (TEST_F) to reduce some boilerplate and encapsulate some subtle setup code required for overlay scrollers on Mac. Part of this is to do with ScopedPreferredScrollerStyle: interleaving swizzlers like `std::unique_ptr x(new X1); x.reset(new X2);` Causes incorrect behaviour and leakage of the swizzled methods across tests run in the same process. Specifically, X1's destructor restores the original methods, but then X2's destructor will restore X1's swizzled methods, since they were in effect when X2 was constructed. Make it harder for devs to shoot themselves in the foot with this. BUG=615948 ==========
tapted@chromium.org changed reviewers: + sky@chromium.org
Description was changed from ========== Views: Give ScrollViewTest a test harness. Currently the tests are all TEST rather than TEST_F. Add a test harness (TEST_F) to reduce some boilerplate and encapsulate some subtle setup code required for overlay scrollers on Mac. Part of this is to do with ScopedPreferredScrollerStyle: interleaving swizzlers like `std::unique_ptr x(new X1); x.reset(new X2);` Causes incorrect behaviour and leakage of the swizzled methods across tests run in the same process. Specifically, X1's destructor restores the original methods, but then X2's destructor will restore X1's swizzled methods, since they were in effect when X2 was constructed. Make it harder for devs to shoot themselves in the foot with this. BUG=615948 ========== to ========== Views: Give ScrollViewTest a test harness. Currently the tests are all TEST rather than TEST_F. Add a test harness (TEST_F) to reduce some boilerplate and encapsulate some subtle setup code required for overlay scrollers on Mac. Part of this is to do with ScopedPreferredScrollerStyle: Interleaving swizzlers like `std::unique_ptr x(new X1); x.reset(new X2);` causes incorrect behaviour and leakage of the swizzled methods across tests run in the same process. Specifically, X1's destructor restores the original methods, but then X2's destructor will restore X1's swizzled methods, since they were in effect when X2 was constructed. Make it harder for devs to shoot themselves in the foot with this. BUG=615948 ==========
Hi sky, please take a look. Thanks! https://codereview.chromium.org/2442223002/diff/1/ui/views/controls/scroll_vi... File ui/views/controls/scroll_view_unittest.cc (right): https://codereview.chromium.org/2442223002/diff/1/ui/views/controls/scroll_vi... ui/views/controls/scroll_view_unittest.cc:158: // change modes, which requires a MessageLoop to exist. Tests should only It's possible that later on I'll need a MessageLoop.. (or I'll need to migrate tests to use WidgetScrollViewTest just below). But the existing tests were getting weird failures in the next round of things because of this other strangeness.
LGTM https://codereview.chromium.org/2442223002/diff/1/ui/views/controls/scroll_vi... File ui/views/controls/scroll_view_unittest.cc (right): https://codereview.chromium.org/2442223002/diff/1/ui/views/controls/scroll_vi... ui/views/controls/scroll_view_unittest.cc:151: scroller_style_.reset(); It's worth a comment as to why you need two resets rather than one. https://codereview.chromium.org/2442223002/diff/1/ui/views/controls/scroll_vi... ui/views/controls/scroll_view_unittest.cc:152: scroller_style_.reset(new ui::test::ScopedPreferredScrollerStyle(enabled)); MakeUnique?
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/2442223002/diff/1/ui/views/controls/scroll_vi... File ui/views/controls/scroll_view_unittest.cc (right): https://codereview.chromium.org/2442223002/diff/1/ui/views/controls/scroll_vi... ui/views/controls/scroll_view_unittest.cc:151: scroller_style_.reset(); On 2016/10/24 16:09:49, sky wrote: > It's worth a comment as to why you need two resets rather than one. Done. https://codereview.chromium.org/2442223002/diff/1/ui/views/controls/scroll_vi... ui/views/controls/scroll_view_unittest.cc:152: scroller_style_.reset(new ui::test::ScopedPreferredScrollerStyle(enabled)); On 2016/10/24 16:09:49, sky wrote: > MakeUnique? Done. There's actually a way to omit the template arguments with a weird C++ trick. I.e. instead of scroller_style_ = base::MakeUnique<ui::test::ScopedPreferredScrollerStyle>(true); it could be scroller_style_ = base::EmplaceUnique(true); But it's probably too much C++ voodoo (CL for it looks like https://codereview.chromium.org/2449753002 ).
The CQ bit was checked by tapted@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org Link to the patchset: https://codereview.chromium.org/2442223002/#ps20001 (title: "respond to comments")
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 ========== Views: Give ScrollViewTest a test harness. Currently the tests are all TEST rather than TEST_F. Add a test harness (TEST_F) to reduce some boilerplate and encapsulate some subtle setup code required for overlay scrollers on Mac. Part of this is to do with ScopedPreferredScrollerStyle: Interleaving swizzlers like `std::unique_ptr x(new X1); x.reset(new X2);` causes incorrect behaviour and leakage of the swizzled methods across tests run in the same process. Specifically, X1's destructor restores the original methods, but then X2's destructor will restore X1's swizzled methods, since they were in effect when X2 was constructed. Make it harder for devs to shoot themselves in the foot with this. BUG=615948 ========== to ========== Views: Give ScrollViewTest a test harness. Currently the tests are all TEST rather than TEST_F. Add a test harness (TEST_F) to reduce some boilerplate and encapsulate some subtle setup code required for overlay scrollers on Mac. Part of this is to do with ScopedPreferredScrollerStyle: Interleaving swizzlers like `std::unique_ptr x(new X1); x.reset(new X2);` causes incorrect behaviour and leakage of the swizzled methods across tests run in the same process. Specifically, X1's destructor restores the original methods, but then X2's destructor will restore X1's swizzled methods, since they were in effect when X2 was constructed. Make it harder for devs to shoot themselves in the foot with this. BUG=615948 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Views: Give ScrollViewTest a test harness. Currently the tests are all TEST rather than TEST_F. Add a test harness (TEST_F) to reduce some boilerplate and encapsulate some subtle setup code required for overlay scrollers on Mac. Part of this is to do with ScopedPreferredScrollerStyle: Interleaving swizzlers like `std::unique_ptr x(new X1); x.reset(new X2);` causes incorrect behaviour and leakage of the swizzled methods across tests run in the same process. Specifically, X1's destructor restores the original methods, but then X2's destructor will restore X1's swizzled methods, since they were in effect when X2 was constructed. Make it harder for devs to shoot themselves in the foot with this. BUG=615948 ========== to ========== Views: Give ScrollViewTest a test harness. Currently the tests are all TEST rather than TEST_F. Add a test harness (TEST_F) to reduce some boilerplate and encapsulate some subtle setup code required for overlay scrollers on Mac. Part of this is to do with ScopedPreferredScrollerStyle: Interleaving swizzlers like `std::unique_ptr x(new X1); x.reset(new X2);` causes incorrect behaviour and leakage of the swizzled methods across tests run in the same process. Specifically, X1's destructor restores the original methods, but then X2's destructor will restore X1's swizzled methods, since they were in effect when X2 was constructed. Make it harder for devs to shoot themselves in the foot with this. BUG=615948 Committed: https://crrev.com/5a1cee4f8acd9731b7af2c616b42aa892fdfdd2e Cr-Commit-Position: refs/heads/master@{#427253} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/5a1cee4f8acd9731b7af2c616b42aa892fdfdd2e Cr-Commit-Position: refs/heads/master@{#427253} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
