Commit e63eab6a authored by benwells's avatar benwells Committed by Commit bot

Revert of Be explicit about forcing TouchSelectionController updates (patchset...

Revert of Be explicit about forcing TouchSelectionController updates (patchset #2 id:20001 of https://codereview.chromium.org/1127383007/)

Reason for revert:
This has caused failures on the memory bots. e.g.:http://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20MSan%20Tests/builds/6179

Sample test output:
[ RUN      ] TouchSelectionControllerTest.AllowShowingFromCurrentSelection
==6485== WARNING: MemorySanitizer: use-of-uninitialized-value
    #0 0x7f5bc93bb605 in ui::TouchSelectionController::OnSelectionBoundsChanged(ui::SelectionBound const&, ui::SelectionBound const&) ui/touch_selection/touch_selection_controller.cc:71:27
    #1 0x7f5bc922803a in ui::TouchSelectionControllerTest::ChangeSelection(gfx::RectF const&, bool, gfx::RectF const&, bool) ui/touch_selection/touch_selection_controller_unittest.cc:147:5
    #2 0x7f5bc9278c8d in ui::TouchSelectionControllerTest_AllowShowingFromCurrentSelection_Test::TestBody() ui/touch_selection/touch_selection_controller_unittest.cc:946:3
    #3 0x7f5bc93631dc in HandleExceptionsInMethodIfSupported\u003Ctesting::Test, void> testing/gtest/src/gtest.cc:2420:12
    #4 0x7f5bc93631dc in testing::Test::Run() testing/gtest/src/gtest.cc:2436:0
    #5 0x7f5bc9365a4c in testing::TestInfo::Run() testing/gtest/src/gtest.cc:2612:5
    #6 0x7f5bc93673f6 in testing::TestCase::Run() testing/gtest/src/gtest.cc:2730:5
    #7 0x7f5bc9384d03 in testing::internal::UnitTestImpl::RunAllTests() testing/gtest/src/gtest.cc:4602:11
    #8 0x7f5bc9383ce2 in HandleExceptionsInMethodIfSupported\u003Ctesting::internal::UnitTestImpl, bool> testing/gtest/src/gtest.cc:2420:12
    #9 0x7f5bc9383ce2 in testing::UnitTest::Run() testing/gtest/src/gtest.cc:4220:0
    #10 0x7f5bc93078a3 in RUN_ALL_TESTS testing/gtest/include/gtest/gtest.h:2326:10
    #11 0x7f5bc93078a3 in base::TestSuite::Run() base/test/test_suite.cc:228:0
    #12 0x7f5bc92f7b2d in (anonymous namespace)::RunTestSuite(int, char**) base/test/run_all_unittests.cc:25:10
    #13 0x7f5bc92f85ee in Run base/callback.h:396:12
    #14 0x7f5bc92f85ee in base::(anonymous namespace)::LaunchUnitTestsInternal(base::Callback\u003Cint ()> const&, int, bool, base::Callback\u003Cvoid ()> const&) base/test/launcher/unit_test_launcher.cc:184:0
    #15 0x7f5bc92f7f5b in base::LaunchUnitTests(int, char**, base::Callback\u003Cint ()> const&) base/test/launcher/unit_test_launcher.cc:423:10
    #16 0x7f5bc92f792d in main base/test/run_all_unittests.cc:37:10
    #17 0x7f5bc206a76c in __libc_start_main /build/buildd/eglibc-2.15/csu/libc-start.c:226:0
    #18 0x7f5bc9188f38 in _start ??:0:0

  Uninitialized value was created by a heap allocation
    #0 0x7f5bc91da0e2 in operator new(unsigned long) ??:0:0
    #1 0x7f5bc927c95c in ui::TouchSelectionControllerTest::SetUp() ui/touch_selection/touch_selection_controller_unittest.cc:65:23
    #2 0x7f5bc9362f57 in HandleExceptionsInMethodIfSupported\u003Ctesting::Test, void> testing/gtest/src/gtest.cc:2420:12
    #3 0x7f5bc9362f57 in testing::Test::Run() testing/gtest/src/gtest.cc:2432:0
    #4 0x7f5bc9365a4c in testing::TestInfo::Run() testing/gtest/src/gtest.cc:2612:5
    #5 0x7f5bc93673f6 in testing::TestCase::Run() testing/gtest/src/gtest.cc:2730:5
    #6 0x7f5bc9384d03 in testing::internal::UnitTestImpl::RunAllTests() testing/gtest/src/gtest.cc:4602:11
    #7 0x7f5bc9383ce2 in HandleExceptionsInMethodIfSupported\u003Ctesting::internal::UnitTestImpl, bool> testing/gtest/src/gtest.cc:2420:12
    #8 0x7f5bc9383ce2 in testing::UnitTest::Run() testing/gtest/src/gtest.cc:4220:0
    #9 0x7f5bc93078a3 in RUN_ALL_TESTS testing/gtest/include/gtest/gtest.h:2326:10
    #10 0x7f5bc93078a3 in base::TestSuite::Run() base/test/test_suite.cc:228:0
    #11 0x7f5bc92f7b2d in (anonymous namespace)::RunTestSuite(int, char**) base/test/run_all_unittests.cc:25:10
    #12 0x7f5bc92f85ee in Run base/callback.h:396:12
    #13 0x7f5bc92f85ee in base::(anonymous namespace)::LaunchUnitTestsInternal(base::Callback\u003Cint ()> const&, int, bool, base::Callback\u003Cvoid ()> const&) base/test/launcher/unit_test_launcher.cc:184:0
    #14 0x7f5bc92f7f5b in base::LaunchUnitTests(int, char**, base::Callback\u003Cint ()> const&) base/test/launcher/unit_test_launcher.cc:423:10
    #15 0x7f5bc92f792d in main base/test/run_all_unittests.cc:37:10
    #16 0x7f5bc206a76c in __libc_start_main /build/buildd/eglibc-2.15/csu/libc-start.c:226:0

SUMMARY: MemorySanitizer: use-of-uninitialized-value (/mnt/data/b/build/slave/Linux_MSan_Tests/build/src/out/Release/ui_touch_selection_unittests+0x285605)
Exiting

Original issue's description:
> Be explicit about forcing TouchSelectionController updates
>
> Previously, cached values in the TouchSelectionController would be reset
> to force future selection updates. However, these cached values can
> actually be used outside of selection update calls, e.g., when force
> showing the selection from the current cached values. Instead of
> resetting the cached values, simply set a dirty bit that forces an
> update, avoiding issues when dealing with the reset values.
>
> BUG=393025
>
> Committed: https://crrev.com/fdcf817da49ee92fe191981f7525503444f75f83
> Cr-Commit-Position: refs/heads/master@{#329325}

TBR=mohsen@chromium.org,jdduke@chromium.org
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=393025

Review URL: https://codereview.chromium.org/1139533006

Cr-Commit-Position: refs/heads/master@{#329352}
parent 21c8c94e
...@@ -68,14 +68,13 @@ TouchSelectionController::~TouchSelectionController() { ...@@ -68,14 +68,13 @@ TouchSelectionController::~TouchSelectionController() {
void TouchSelectionController::OnSelectionBoundsChanged( void TouchSelectionController::OnSelectionBoundsChanged(
const SelectionBound& start, const SelectionBound& start,
const SelectionBound& end) { const SelectionBound& end) {
if (!force_next_update_ && start == start_ && end_ == end) if (start == start_ && end_ == end)
return; return;
start_ = start; start_ = start;
end_ = end; end_ = end;
start_orientation_ = ToTouchHandleOrientation(start_.type()); start_orientation_ = ToTouchHandleOrientation(start_.type());
end_orientation_ = ToTouchHandleOrientation(end_.type()); end_orientation_ = ToTouchHandleOrientation(end_.type());
force_next_update_ = false;
if (!activate_selection_automatically_ && if (!activate_selection_automatically_ &&
!activate_insertion_automatically_) { !activate_insertion_automatically_) {
...@@ -156,7 +155,7 @@ void TouchSelectionController::OnLongPressEvent() { ...@@ -156,7 +155,7 @@ void TouchSelectionController::OnLongPressEvent() {
response_pending_input_event_ = LONG_PRESS; response_pending_input_event_ = LONG_PRESS;
ShowSelectionHandlesAutomatically(); ShowSelectionHandlesAutomatically();
ShowInsertionHandleAutomatically(); ShowInsertionHandleAutomatically();
ForceNextUpdateIfInactive(); ResetCachedValuesIfInactive();
} }
void TouchSelectionController::AllowShowingFromCurrentSelection() { void TouchSelectionController::AllowShowingFromCurrentSelection() {
...@@ -180,7 +179,7 @@ void TouchSelectionController::OnTapEvent() { ...@@ -180,7 +179,7 @@ void TouchSelectionController::OnTapEvent() {
ShowInsertionHandleAutomatically(); ShowInsertionHandleAutomatically();
if (selection_empty_ && !show_on_tap_for_empty_editable_) if (selection_empty_ && !show_on_tap_for_empty_editable_)
DeactivateInsertion(); DeactivateInsertion();
ForceNextUpdateIfInactive(); ResetCachedValuesIfInactive();
} }
void TouchSelectionController::HideAndDisallowShowingAutomatically() { void TouchSelectionController::HideAndDisallowShowingAutomatically() {
...@@ -209,7 +208,7 @@ void TouchSelectionController::OnSelectionEditable(bool editable) { ...@@ -209,7 +208,7 @@ void TouchSelectionController::OnSelectionEditable(bool editable) {
if (selection_editable_ == editable) if (selection_editable_ == editable)
return; return;
selection_editable_ = editable; selection_editable_ = editable;
ForceNextUpdateIfInactive(); ResetCachedValuesIfInactive();
if (!selection_editable_) if (!selection_editable_)
DeactivateInsertion(); DeactivateInsertion();
} }
...@@ -218,7 +217,7 @@ void TouchSelectionController::OnSelectionEmpty(bool empty) { ...@@ -218,7 +217,7 @@ void TouchSelectionController::OnSelectionEmpty(bool empty) {
if (selection_empty_ == empty) if (selection_empty_ == empty)
return; return;
selection_empty_ = empty; selection_empty_ = empty;
ForceNextUpdateIfInactive(); ResetCachedValuesIfInactive();
} }
bool TouchSelectionController::Animate(base::TimeTicks frame_time) { bool TouchSelectionController::Animate(base::TimeTicks frame_time) {
...@@ -341,14 +340,14 @@ void TouchSelectionController::ShowInsertionHandleAutomatically() { ...@@ -341,14 +340,14 @@ void TouchSelectionController::ShowInsertionHandleAutomatically() {
if (activate_insertion_automatically_) if (activate_insertion_automatically_)
return; return;
activate_insertion_automatically_ = true; activate_insertion_automatically_ = true;
ForceNextUpdateIfInactive(); ResetCachedValuesIfInactive();
} }
void TouchSelectionController::ShowSelectionHandlesAutomatically() { void TouchSelectionController::ShowSelectionHandlesAutomatically() {
if (activate_selection_automatically_) if (activate_selection_automatically_)
return; return;
activate_selection_automatically_ = true; activate_selection_automatically_ = true;
ForceNextUpdateIfInactive(); ResetCachedValuesIfInactive();
} }
void TouchSelectionController::OnInsertionChanged() { void TouchSelectionController::OnInsertionChanged() {
...@@ -464,9 +463,13 @@ void TouchSelectionController::DeactivateSelection() { ...@@ -464,9 +463,13 @@ void TouchSelectionController::DeactivateSelection() {
client_->OnSelectionEvent(SELECTION_CLEARED); client_->OnSelectionEvent(SELECTION_CLEARED);
} }
void TouchSelectionController::ForceNextUpdateIfInactive() { void TouchSelectionController::ResetCachedValuesIfInactive() {
if (active_status_ == INACTIVE) if (active_status_ != INACTIVE)
force_next_update_ = true; return;
start_ = SelectionBound();
end_ = SelectionBound();
start_orientation_ = TouchHandleOrientation::UNDEFINED;
end_orientation_ = TouchHandleOrientation::UNDEFINED;
} }
gfx::Vector2dF TouchSelectionController::GetStartLineOffset() const { gfx::Vector2dF TouchSelectionController::GetStartLineOffset() const {
......
...@@ -132,7 +132,7 @@ class UI_TOUCH_SELECTION_EXPORT TouchSelectionController ...@@ -132,7 +132,7 @@ class UI_TOUCH_SELECTION_EXPORT TouchSelectionController
void DeactivateInsertion(); void DeactivateInsertion();
void ActivateSelection(); void ActivateSelection();
void DeactivateSelection(); void DeactivateSelection();
void ForceNextUpdateIfInactive(); void ResetCachedValuesIfInactive();
gfx::Vector2dF GetStartLineOffset() const; gfx::Vector2dF GetStartLineOffset() const;
gfx::Vector2dF GetEndLineOffset() const; gfx::Vector2dF GetEndLineOffset() const;
...@@ -146,10 +146,6 @@ class UI_TOUCH_SELECTION_EXPORT TouchSelectionController ...@@ -146,10 +146,6 @@ class UI_TOUCH_SELECTION_EXPORT TouchSelectionController
const base::TimeDelta tap_timeout_; const base::TimeDelta tap_timeout_;
const float tap_slop_; const float tap_slop_;
// Whether to force an update on the next selection event even if the
// cached selection matches the new selection.
bool force_next_update_;
// Controls whether an insertion handle is shown on a tap for an empty // Controls whether an insertion handle is shown on a tap for an empty
// editable text. // editable text.
bool show_on_tap_for_empty_editable_; bool show_on_tap_for_empty_editable_;
......
...@@ -946,9 +946,6 @@ TEST_F(TouchSelectionControllerTest, AllowShowingFromCurrentSelection) { ...@@ -946,9 +946,6 @@ TEST_F(TouchSelectionControllerTest, AllowShowingFromCurrentSelection) {
ChangeSelection(start_rect, visible, end_rect, visible); ChangeSelection(start_rect, visible, end_rect, visible);
EXPECT_EQ(gfx::PointF(), GetLastEventStart()); EXPECT_EQ(gfx::PointF(), GetLastEventStart());
// A longpress should have no immediate effect.
controller().OnLongPressEvent();
// Now explicitly allow showing from the previously supplied bounds. // Now explicitly allow showing from the previously supplied bounds.
controller().AllowShowingFromCurrentSelection(); controller().AllowShowingFromCurrentSelection();
EXPECT_THAT(GetAndResetEvents(), ElementsAre(SELECTION_SHOWN)); EXPECT_THAT(GetAndResetEvents(), ElementsAre(SELECTION_SHOWN));
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment