Add INSERTION_DRAG_STOPPED to SelectionEventType

Add INSERTION_DRAG_STOPPED to SelectionEventType

In order to use ui::TouchSelectionController in Aura, we need to be notified
when handle drag ends. Aura's touch selection quick menu is hidden on drag start
and should be shown back on drag end.

BUG=399721
Committed: https://crrev.com/5600e359878b61a6ab0f96361c731ea694064ed9
Cr-Commit-Position: refs/heads/master@{#317864}

[mohsen, 2015-02-18 18:46:45]

mohsen@chromium.org changed reviewers:
+ jdduke@chromium.org

[mohsen, 2015-02-18 18:46:46]

jdduke@: Please take a look...

[jdduke (slow), 2015-02-18 19:09:14]

https://codereview.chromium.org/937773003/diff/20001/ui/touch_selection/touch...
File ui/touch_selection/touch_selection_controller_unittest.cc (right):

https://codereview.chromium.org/937773003/diff/20001/ui/touch_selection/touch...
ui/touch_selection/touch_selection_controller_unittest.cc:230: EXPECT_EQ(0,
GetAndResetEventCount(INSERTION_CLEARED));
This is a lot of boilerplate for every check.

What if we instead do |GetAndResetLastEventType()|, and have it return either a
(potentially empty) scoped_ptr, or a pointer to the received type (or nullptr if
no event has been received since the last reset).

I think there are only 2 or 3 places currently where the result should be null.

[mohsen, 2015-02-18 21:53:36]

https://codereview.chromium.org/937773003/diff/20001/ui/touch_selection/touch...
File ui/touch_selection/touch_selection_controller_unittest.cc (right):

https://codereview.chromium.org/937773003/diff/20001/ui/touch_selection/touch...
ui/touch_selection/touch_selection_controller_unittest.cc:230: EXPECT_EQ(0,
GetAndResetEventCount(INSERTION_CLEARED));
On 2015/02/18 19:09:14, jdduke wrote:
> This is a lot of boilerplate for every check.
> 
> What if we instead do |GetAndResetLastEventType()|, and have it return either
a
> (potentially empty) scoped_ptr, or a pointer to the received type (or nullptr
if
> no event has been received since the last reset).
> 
> I think there are only 2 or 3 places currently where the result should be
null.

There are some location where we test more than last event (e.g. line 365).
Also, using scoped_ptr would still be almost the same amount of code for each
test. Something like:

scoped_ptr<SelectionEventType> last_event = GetAndResetLastEventType();
ASSERT_NE(nullptr, last_event);
EXPECT_EQ(INSERTION_SHOWN, *last_event);

Do you have something cleaner in mind?

[jdduke (slow), 2015-02-18 22:03:49]

Keeping counts is fine, but what if we only check the counts for the type we
either expect an event to have occurred, or explicitly want to make sure no new
event has occurred.

https://codereview.chromium.org/937773003/diff/20001/ui/touch_selection/touch...
File ui/touch_selection/touch_selection_controller_unittest.cc (right):

https://codereview.chromium.org/937773003/diff/20001/ui/touch_selection/touch...
ui/touch_selection/touch_selection_controller_unittest.cc:96:
event_count_[event]++;
Hmm, you could use a bool + the type, |bool has_received_event_|, then return
nullptr if it's false, or the address of the |last_event_type_| otherwise, and
just inline the pointer deref in the EXPECT_EQ. Crashing is as good as a failure
anyway.

e.g.,

SelectionEventType* GetAndResetLastEventType() {
  if (!has_received_event_)
    return nullptr;
  has_received_event_ = false;
  return &last_event_type_;
}

Or use different queries:

bool HasReceivedEvent() const {...}
SelectionEventType GetAndResetLastEventType()  {
  ASSERT_TRUE(HasReceivedEvent());
  has_received_event_ = false; ...
}

And use the former when you don't expect there to be a new event.

[mohsen, 2015-02-18 23:31:17]

Patchset #3 (id:40001) has been deleted

[mohsen, 2015-02-18 23:35:22]

https://codereview.chromium.org/937773003/diff/20001/ui/touch_selection/touch...
File ui/touch_selection/touch_selection_controller_unittest.cc (right):

https://codereview.chromium.org/937773003/diff/20001/ui/touch_selection/touch...
ui/touch_selection/touch_selection_controller_unittest.cc:96:
event_count_[event]++;
On 2015/02/18 22:03:49, jdduke wrote:
> Hmm, you could use a bool + the type, |bool has_received_event_|, then return
> nullptr if it's false, or the address of the |last_event_type_| otherwise, and
> just inline the pointer deref in the EXPECT_EQ. Crashing is as good as a
failure
> anyway.
> 
> e.g.,
> 
> SelectionEventType* GetAndResetLastEventType() {
>   if (!has_received_event_)
>     return nullptr;
>   has_received_event_ = false;
>   return &last_event_type_;
> }
> 
> Or use different queries:
> 
> bool HasReceivedEvent() const {...}
> SelectionEventType GetAndResetLastEventType()  {
>   ASSERT_TRUE(HasReceivedEvent());
>   has_received_event_ = false; ...
> }
> 
> And use the former when you don't expect there to be a new event.

I ended up adding a total count for events to be able to make sure no extra
event is happening, without checking all other events. This is similar to your
2nd suggestion (HasReceivedEvent + GetAndResetLastEventType). Does it look
reasonable to you?

[jdduke (slow), 2015-02-19 22:53:48]

https://codereview.chromium.org/937773003/diff/60001/ui/touch_selection/touch...
File ui/touch_selection/touch_selection_controller_unittest.cc (right):

https://codereview.chromium.org/937773003/diff/60001/ui/touch_selection/touch...
ui/touch_selection/touch_selection_controller_unittest.cc:566: EXPECT_EQ(2,
total_event_count());
This is still too much boiler plate. If we use Gmock (#include
"testing/gmock/include/gmock/gmock.h"), we can probably do something like:

EXPECT_THAT(PopEvents(), ElementsAre(A, B, ...));

Can you give that a try?

[mohsen, 2015-02-20 18:16:00]

https://codereview.chromium.org/937773003/diff/60001/ui/touch_selection/touch...
File ui/touch_selection/touch_selection_controller_unittest.cc (right):

https://codereview.chromium.org/937773003/diff/60001/ui/touch_selection/touch...
ui/touch_selection/touch_selection_controller_unittest.cc:566: EXPECT_EQ(2,
total_event_count());
On 2015/02/19 22:53:48, jdduke wrote:
> This is still too much boiler plate. If we use Gmock (#include
> "testing/gmock/include/gmock/gmock.h"), we can probably do something like:
> 
> EXPECT_THAT(PopEvents(), ElementsAre(A, B, ...));
> 
> Can you give that a try?

Nice, I was looking for something like that. Please see how it looks now.

[jdduke (slow), 2015-02-23 16:19:27]

lgtm, thanks.

https://codereview.chromium.org/937773003/diff/80001/ui/touch_selection/touch...
File ui/touch_selection/touch_selection_controller_unittest.cc (right):

https://codereview.chromium.org/937773003/diff/80001/ui/touch_selection/touch...
ui/touch_selection/touch_selection_controller_unittest.cc:187:
std::vector<SelectionEventType> events = events_;
std::vector<SelectionEventType> events;
events.swap(events_);
return events;

https://codereview.chromium.org/937773003/diff/80001/ui/touch_selection/touch...
ui/touch_selection/touch_selection_controller_unittest.cc:886:
EXPECT_THAT(GetAndResetEvents(), ElementsAre(SELECTION_SHOWN));
This is a great improvement overall, thanks.

[mohsen, 2015-02-24 07:29:36]

https://codereview.chromium.org/937773003/diff/80001/ui/touch_selection/touch...
File ui/touch_selection/touch_selection_controller_unittest.cc (right):

https://codereview.chromium.org/937773003/diff/80001/ui/touch_selection/touch...
ui/touch_selection/touch_selection_controller_unittest.cc:187:
std::vector<SelectionEventType> events = events_;
On 2015/02/23 16:19:27, jdduke wrote:
> std::vector<SelectionEventType> events;
> events.swap(events_);
> return events;

Done.

[mohsen, 2015-02-24 19:45:42]

The CQ bit was checked by mohsen@chromium.org

[mohsen, 2015-02-24 19:45:42]

The patchset sent to the CQ was uploaded after l-g-t-m from jdduke@chromium.org
Link to the patchset: https://codereview.chromium.org/937773003/#ps100001
(title: "Rebased and Addressed Reviews")

[commit-bot: I haz the power, 2015-02-24 19:46:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/937773003/100001

[commit-bot: I haz the power, 2015-02-24 20:03:57]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2015-02-24 20:04:31]

Patchset 5 (id:??) landed as
https://crrev.com/5600e359878b61a6ab0f96361c731ea694064ed9
Cr-Commit-Position: refs/heads/master@{#317864}