Articulate the cancel behavior in the WebTouchEvent.

Articulate the cancel behavior in the WebTouchEvent.

Change the cancelable flag to be an enumeration so that we
can determine if it is blocking or non-blocking and the reason.

Wheel events will add this flag in a follow up CL.

BUG=601661
Committed: https://crrev.com/afb8f54b782055923cadd711452c2448f3f7b5b4
Cr-Commit-Position: refs/heads/master@{#388585}

[dtapuska, 2016-04-14 21:10:59]

Description was changed from

==========
Articulate the cancel behavior in the WebTouchEvent.

Change the cancelable flag to be an enumeration so that we
can determine if it is blocking or non-blocking and the reason.

Wheel events will add this flag in a follow up CL.

BUG=601661
==========

to

==========
Articulate the cancel behavior in the WebTouchEvent.

Change the cancelable flag to be an enumeration so that we
can determine if it is blocking or non-blocking and the reason.

Wheel events will add this flag in a follow up CL.

BUG=601661
==========

[dtapuska, 2016-04-14 21:10:59]

dtapuska@chromium.org changed reviewers:
+ rbyers@chromium.org, tdresser@chromium.org

[dtapuska, 2016-04-14 21:11:26]

Have a look please.

I'm not sold on the variable name if you have a better suggestion please
provide.

[tdresser, 2016-04-18 15:12:03]

https://codereview.chromium.org/1888163003/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/input/input_router_impl.cc (right):

https://codereview.chromium.org/1888163003/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/input/input_router_impl.cc:373: // cancelable
(respect ACK disposition) or not except touchmove.
Can we explain the touchmove case a bit more here?

https://codereview.chromium.org/1888163003/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/input/touch_emulator_unittest.cc (right):

https://codereview.chromium.org/1888163003/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/input/touch_emulator_unittest.cc:66:
WebInputEvent::DispatchType expected_cancelable =
Rename to expected_dispatch_type

https://codereview.chromium.org/1888163003/diff/1/content/common/input/web_in...
File content/common/input/web_input_event_traits.cc (right):

https://codereview.chromium.org/1888163003/diff/1/content/common/input/web_in...
content/common/input/web_input_event_traits.cc:487: static_cast<const
WebTouchEvent&>(event).dispatchType);
Why do we DCHECK here, but for no other event types?

https://codereview.chromium.org/1888163003/diff/1/content/renderer/input/rend...
File content/renderer/input/render_widget_input_handler.cc (right):

https://codereview.chromium.org/1888163003/diff/1/content/renderer/input/rend...
content/renderer/input/render_widget_input_handler.cc:124: void
LogPassiveEventListenersUma(WebInputEventResult result,
Could we add some kind of test for this? Worst case we just expose this, and
test this method directly, using base/test/histogram_tester.h.

https://codereview.chromium.org/1888163003/diff/1/content/renderer/input/rend...
content/renderer/input/render_widget_input_handler.cc:332:
InputEventDispatchType::DISPATCH_TYPE_NON_BLOCKING_NOTIFY_MAIN ||
Would it be clearer to negate this boolean?

https://codereview.chromium.org/1888163003/diff/1/third_party/WebKit/public/w...
File third_party/WebKit/public/web/WebInputEvent.h (right):

https://codereview.chromium.org/1888163003/diff/1/third_party/WebKit/public/w...
third_party/WebKit/public/web/WebInputEvent.h:194: NonBlocking,
The difference between NonBlocking and NonBlockingPassive isn't super clear
here.

NonBlocking indicates that we don't block due to a property of the event,
whereas NonBlockingPassive indicates that we don't block due to a property of
the event listener, correct?

Should we split this up into a HandlerBlockingType and uh... maybe
EventBlockingType?

I'm not super happy with that but it would clarify that we're talking about two
separate axis here, which isn't obvious with the current enum.

Alternatively, maybe renaming to something like:

enum DispatchType {
  Blocking,
  EventNonBlocking,
  ListenersNonBlockingPassive
  ListenersForcedNonBlockingPassive
}

I'm not big on DispatchType either, but I haven't come up with anything great.

Maybe just "BlockingType"? Or "BlockOnAckType"?

https://codereview.chromium.org/1888163003/diff/1/third_party/WebKit/public/w...
third_party/WebKit/public/web/WebInputEvent.h:615: // Whether the event is
blocking or not.
This comment is a bit terse. Maybe add a bit more detail?

[Rick Byers, 2016-04-18 16:57:59]

https://codereview.chromium.org/1888163003/diff/1/third_party/WebKit/public/w...
File third_party/WebKit/public/web/WebInputEvent.h (right):

https://codereview.chromium.org/1888163003/diff/1/third_party/WebKit/public/w...
third_party/WebKit/public/web/WebInputEvent.h:194: NonBlocking,
On 2016/04/18 15:12:03, tdresser wrote:
> The difference between NonBlocking and NonBlockingPassive isn't super clear
> here.
> 
> NonBlocking indicates that we don't block due to a property of the event,
> whereas NonBlockingPassive indicates that we don't block due to a property of
> the event listener, correct?
> 
> Should we split this up into a HandlerBlockingType and uh... maybe
> EventBlockingType?
> 
> I'm not super happy with that but it would clarify that we're talking about
two
> separate axis here, which isn't obvious with the current enum.
> 
> Alternatively, maybe renaming to something like:
> 
> enum DispatchType {
>   Blocking,
>   EventNonBlocking,
>   ListenersNonBlockingPassive
>   ListenersForcedNonBlockingPassive
> }
> 
> I'm not big on DispatchType either, but I haven't come up with anything great.
> 
> Maybe just "BlockingType"? Or "BlockOnAckType"?

IMHO the important thing is that it's clear that there are really only 2
behaviors as far as the application is concerned: cancelable and not cancelable.
 The rest of the division is just for our own internal bookkeeping /
measurements (or exposed diagnostics/tooling) around WHY something is not
cancelable.  With appropriate comments to that effect, the proposal here seems
fine to me.

I could also imagine an argument for making this super explicit by keeping
cancelable a bool and adding a NonBlockingReason which is defined explicitly to
not change semantics but just inform measurements / tooling (it's not like space
is at a premium here since there's never more than a few of these alive at once
- no reason to worry about packing tightly).

[dtapuska, 2016-04-18 19:19:34]

https://codereview.chromium.org/1888163003/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/input/input_router_impl.cc (right):

https://codereview.chromium.org/1888163003/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/input/input_router_impl.cc:373: // cancelable
(respect ACK disposition) or not except touchmove.
On 2016/04/18 15:12:02, tdresser wrote:
> Can we explain the touchmove case a bit more here?

Done; moved comments into the web_input_event_traits.

https://codereview.chromium.org/1888163003/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/input/touch_emulator_unittest.cc (right):

https://codereview.chromium.org/1888163003/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/input/touch_emulator_unittest.cc:66:
WebInputEvent::DispatchType expected_cancelable =
On 2016/04/18 15:12:02, tdresser wrote:
> Rename to expected_dispatch_type

Done.

https://codereview.chromium.org/1888163003/diff/1/content/common/input/web_in...
File content/common/input/web_input_event_traits.cc (right):

https://codereview.chromium.org/1888163003/diff/1/content/common/input/web_in...
content/common/input/web_input_event_traits.cc:487: static_cast<const
WebTouchEvent&>(event).dispatchType);
On 2016/04/18 15:12:02, tdresser wrote:
> Why do we DCHECK here, but for no other event types?

This is a move of the dcheck that was in the input_router_impl before. I've also
added comments to clarify this.

https://codereview.chromium.org/1888163003/diff/1/content/renderer/input/rend...
File content/renderer/input/render_widget_input_handler.cc (right):

https://codereview.chromium.org/1888163003/diff/1/content/renderer/input/rend...
content/renderer/input/render_widget_input_handler.cc:124: void
LogPassiveEventListenersUma(WebInputEventResult result,
On 2016/04/18 15:12:02, tdresser wrote:
> Could we add some kind of test for this? Worst case we just expose this, and
> test this method directly, using base/test/histogram_tester.h.

Done.

https://codereview.chromium.org/1888163003/diff/1/content/renderer/input/rend...
content/renderer/input/render_widget_input_handler.cc:332:
InputEventDispatchType::DISPATCH_TYPE_NON_BLOCKING_NOTIFY_MAIN ||
On 2016/04/18 15:12:03, tdresser wrote:
> Would it be clearer to negate this boolean?

can I leave this for now. It is going away in a follow on patch where I add the
dispatch type for wheel events.

https://codereview.chromium.org/1888163003/diff/1/third_party/WebKit/public/w...
File third_party/WebKit/public/web/WebInputEvent.h (right):

https://codereview.chromium.org/1888163003/diff/1/third_party/WebKit/public/w...
third_party/WebKit/public/web/WebInputEvent.h:194: NonBlocking,
On 2016/04/18 16:57:58, Rick Byers wrote:
> On 2016/04/18 15:12:03, tdresser wrote:
> > The difference between NonBlocking and NonBlockingPassive isn't super clear
> > here.
> > 
> > NonBlocking indicates that we don't block due to a property of the event,
> > whereas NonBlockingPassive indicates that we don't block due to a property
of
> > the event listener, correct?
> > 
> > Should we split this up into a HandlerBlockingType and uh... maybe
> > EventBlockingType?
> > 
> > I'm not super happy with that but it would clarify that we're talking about
> two
> > separate axis here, which isn't obvious with the current enum.
> > 
> > Alternatively, maybe renaming to something like:
> > 
> > enum DispatchType {
> >   Blocking,
> >   EventNonBlocking,
> >   ListenersNonBlockingPassive
> >   ListenersForcedNonBlockingPassive
> > }
> > 
> > I'm not big on DispatchType either, but I haven't come up with anything
great.
> > 
> > Maybe just "BlockingType"? Or "BlockOnAckType"?
> 
> IMHO the important thing is that it's clear that there are really only 2
> behaviors as far as the application is concerned: cancelable and not
cancelable.
>  The rest of the division is just for our own internal bookkeeping /
> measurements (or exposed diagnostics/tooling) around WHY something is not
> cancelable.  With appropriate comments to that effect, the proposal here seems
> fine to me.
> 
> I could also imagine an argument for making this super explicit by keeping
> cancelable a bool and adding a NonBlockingReason which is defined explicitly
to
> not change semantics but just inform measurements / tooling (it's not like
space
> is at a premium here since there's never more than a few of these alive at
once
> - no reason to worry about packing tightly).

Acknowledged.

https://codereview.chromium.org/1888163003/diff/1/third_party/WebKit/public/w...
third_party/WebKit/public/web/WebInputEvent.h:194: NonBlocking,
On 2016/04/18 15:12:03, tdresser wrote:
> The difference between NonBlocking and NonBlockingPassive isn't super clear
> here.
> 
> NonBlocking indicates that we don't block due to a property of the event,
> whereas NonBlockingPassive indicates that we don't block due to a property of
> the event listener, correct?
> 
> Should we split this up into a HandlerBlockingType and uh... maybe
> EventBlockingType?
> 
> I'm not super happy with that but it would clarify that we're talking about
two
> separate axis here, which isn't obvious with the current enum.
> 
> Alternatively, maybe renaming to something like:
> 
> enum DispatchType {
>   Blocking,
>   EventNonBlocking,
>   ListenersNonBlockingPassive
>   ListenersForcedNonBlockingPassive
> }
> 
> I'm not big on DispatchType either, but I haven't come up with anything great.
> 
> Maybe just "BlockingType"? Or "BlockOnAckType"?

Done.

https://codereview.chromium.org/1888163003/diff/1/third_party/WebKit/public/w...
third_party/WebKit/public/web/WebInputEvent.h:615: // Whether the event is
blocking or not.
On 2016/04/18 15:12:03, tdresser wrote:
> This comment is a bit terse. Maybe add a bit more detail?

Done.

[tdresser, 2016-04-18 19:40:45]

LGTM

https://codereview.chromium.org/1888163003/diff/1/content/renderer/input/rend...
File content/renderer/input/render_widget_input_handler.cc (right):

https://codereview.chromium.org/1888163003/diff/1/content/renderer/input/rend...
content/renderer/input/render_widget_input_handler.cc:332:
InputEventDispatchType::DISPATCH_TYPE_NON_BLOCKING_NOTIFY_MAIN ||
On 2016/04/18 19:19:33, dtapuska wrote:
> On 2016/04/18 15:12:03, tdresser wrote:
> > Would it be clearer to negate this boolean?
> 
> can I leave this for now. It is going away in a follow on patch where I add
the
> dispatch type for wheel events.

SGTM.

https://codereview.chromium.org/1888163003/diff/20001/content/common/input/we...
File content/common/input/web_input_event_traits.cc (right):

https://codereview.chromium.org/1888163003/diff/20001/content/common/input/we...
content/common/input/web_input_event_traits.cc:493: // (aka cancelable) on the
event.
The aka here is backwards.

non-blocking -> uncancelable

https://codereview.chromium.org/1888163003/diff/20001/content/renderer/render...
File content/renderer/render_widget_unittest.cc (right):

https://codereview.chromium.org/1888163003/diff/20001/content/renderer/render...
content/renderer/render_widget_unittest.cc:155:
EXPECT_CALL(widget()->mock_webwidget(), handleInputEvent(::testing::_))
Feel free to "using testing::_".

https://codereview.chromium.org/1888163003/diff/20001/content/renderer/render...
content/renderer/render_widget_unittest.cc:278: TEST_F(RenderWidgetUnittest,
RenderWidgetInputEventUmaMetrics) {
Thanks for the test - this is great.

https://codereview.chromium.org/1888163003/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebInputEvent.h (right):

https://codereview.chromium.org/1888163003/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebInputEvent.h:192: enum DispatchType {
Let's add a comment stating that this is mirrored in platformEvent (and vice
versa). The static_asserts ensure this remains true, but it's nice to have it
stated here.

[dtapuska, 2016-04-18 20:09:09]

tdresser@; please take another look. I had to make a slight modification to
main_thread_event_queue that I'd like you to double check.

https://codereview.chromium.org/1888163003/diff/20001/content/common/input/we...
File content/common/input/web_input_event_traits.cc (right):

https://codereview.chromium.org/1888163003/diff/20001/content/common/input/we...
content/common/input/web_input_event_traits.cc:493: // (aka cancelable) on the
event.
On 2016/04/18 19:40:45, tdresser wrote:
> The aka here is backwards.
> 
> non-blocking -> uncancelable

Done.

https://codereview.chromium.org/1888163003/diff/20001/content/renderer/render...
File content/renderer/render_widget_unittest.cc (right):

https://codereview.chromium.org/1888163003/diff/20001/content/renderer/render...
content/renderer/render_widget_unittest.cc:155:
EXPECT_CALL(widget()->mock_webwidget(), handleInputEvent(::testing::_))
On 2016/04/18 19:40:45, tdresser wrote:
> Feel free to "using testing::_".

Done.

https://codereview.chromium.org/1888163003/diff/20001/content/renderer/render...
content/renderer/render_widget_unittest.cc:278: TEST_F(RenderWidgetUnittest,
RenderWidgetInputEventUmaMetrics) {
On 2016/04/18 19:40:45, tdresser wrote:
> Thanks for the test - this is great.

Acknowledged.

https://codereview.chromium.org/1888163003/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebInputEvent.h (right):

https://codereview.chromium.org/1888163003/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebInputEvent.h:192: enum DispatchType {
On 2016/04/18 19:40:45, tdresser wrote:
> Let's add a comment stating that this is mirrored in platformEvent (and vice
> versa). The static_asserts ensure this remains true, but it's nice to have it
> stated here.

Done.

[dtapuska, 2016-04-18 20:10:37]

https://codereview.chromium.org/1888163003/diff/40001/content/renderer/input/...
File content/renderer/input/main_thread_event_queue.cc (right):

https://codereview.chromium.org/1888163003/diff/40001/content/renderer/input/...
content/renderer/input/main_thread_event_queue.cc:51: PendingTouchEvent
new_touch_event =
err this is a poorly name variable. I thought I changed it before uploading.

[tdresser, 2016-04-18 20:22:40]

https://codereview.chromium.org/1888163003/diff/40001/content/renderer/input/...
File content/renderer/input/main_thread_event_queue.cc (right):

https://codereview.chromium.org/1888163003/diff/40001/content/renderer/input/...
content/renderer/input/main_thread_event_queue.cc:56:
blink::WebInputEvent::ListenersNonBlockingPassive;
Hrm, this is a bit confusing, as a non-blocking event with a blocking event
listener will now have the ListenersNonBlockingPassive DispatchType...

[tdresser, 2016-04-20 13:20:21]

https://codereview.chromium.org/1888163003/diff/80001/content/renderer/input/...
File content/renderer/input/main_thread_event_queue.cc (right):

https://codereview.chromium.org/1888163003/diff/80001/content/renderer/input/...
content/renderer/input/main_thread_event_queue.cc:56: // adjusted by the
compositor.
Maybe "since the compositor determined all event listeners are passive."?

[dtapuska, 2016-04-20 13:56:35]

https://codereview.chromium.org/1888163003/diff/80001/content/renderer/input/...
File content/renderer/input/main_thread_event_queue.cc (right):

https://codereview.chromium.org/1888163003/diff/80001/content/renderer/input/...
content/renderer/input/main_thread_event_queue.cc:56: // adjusted by the
compositor.
On 2016/04/20 13:20:21, tdresser wrote:
> Maybe "since the compositor determined all event listeners are passive."?

Done.

[dtapuska, 2016-04-20 14:05:40]

dtapuska@chromium.org changed reviewers:
+ creis@chromium.org, isherman@chromium.org

[Rick Byers, 2016-04-20 15:30:26]

third_party/WebKit LGTM with nits

https://codereview.chromium.org/1888163003/diff/100001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebInputEvent.h (right):

https://codereview.chromium.org/1888163003/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebInputEvent.h:192: // These values are direct
mappings of the values in PlatformEvent
nit: add a comment above this describing briefly the purpose of this enum.  I.e.
"indicates whether the browser needs to block on the ACK result for this event,
and if not why not (for metrics/diagnostics purposes)".

https://codereview.chromium.org/1888163003/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebInputEvent.h:196: Blocking,
nit: add comment saying "i.e. can be canceled"

[Ilya Sherman, 2016-04-20 15:37:21]

histograms.xml lgtm

[Charlie Reis, 2016-04-20 17:16:25]

I'll defer to tdresser for the actual test review, since I'm not that familiar
with the feature or Gmock.  Rubber stamp LGTM with nits for
render_widget_unittest.cc.

https://codereview.chromium.org/1888163003/diff/100001/content/renderer/rende...
File content/renderer/render_widget_unittest.cc (right):

https://codereview.chromium.org/1888163003/diff/100001/content/renderer/rende...
content/renderer/render_widget_unittest.cc:47: using testing::_;
nit: This goes between the includes and the namespaces.

https://codereview.chromium.org/1888163003/diff/100001/content/renderer/rende...
content/renderer/render_widget_unittest.cc:82: MockWebWidget& mock_webwidget() {
return mock_webwidget_; }
nit: const ref

https://codereview.chromium.org/1888163003/diff/100001/content/renderer/rende...
content/renderer/render_widget_unittest.cc:85: ~InteractiveRenderWidget()
override { webwidget_ = NULL; }
nit: nullptr

[dtapuska, 2016-04-20 19:38:12]

https://codereview.chromium.org/1888163003/diff/100001/content/renderer/rende...
File content/renderer/render_widget_unittest.cc (right):

https://codereview.chromium.org/1888163003/diff/100001/content/renderer/rende...
content/renderer/render_widget_unittest.cc:47: using testing::_;
On 2016/04/20 17:16:25, Charlie Reis wrote:
> nit: This goes between the includes and the namespaces.

Done.

https://codereview.chromium.org/1888163003/diff/100001/content/renderer/rende...
content/renderer/render_widget_unittest.cc:82: MockWebWidget& mock_webwidget() {
return mock_webwidget_; }
On 2016/04/20 17:16:25, Charlie Reis wrote:
> nit: const ref

The mocks won't accept a const ref so sadly can't do this.

https://codereview.chromium.org/1888163003/diff/100001/content/renderer/rende...
content/renderer/render_widget_unittest.cc:85: ~InteractiveRenderWidget()
override { webwidget_ = NULL; }
On 2016/04/20 17:16:25, Charlie Reis wrote:
> nit: nullptr

Done.

https://codereview.chromium.org/1888163003/diff/100001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebInputEvent.h (right):

https://codereview.chromium.org/1888163003/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebInputEvent.h:192: // These values are direct
mappings of the values in PlatformEvent
On 2016/04/20 15:30:26, Rick Byers wrote:
> nit: add a comment above this describing briefly the purpose of this enum. 
I.e.
> "indicates whether the browser needs to block on the ACK result for this
event,
> and if not why not (for metrics/diagnostics purposes)".

Done.

https://codereview.chromium.org/1888163003/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebInputEvent.h:196: Blocking,
On 2016/04/20 15:30:26, Rick Byers wrote:
> nit: add comment saying "i.e. can be canceled"

Done.

[dtapuska, 2016-04-20 19:38:20]

The CQ bit was checked by dtapuska@chromium.org

[dtapuska, 2016-04-20 19:38:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
tdresser@chromium.org, creis@chromium.org, isherman@chromium.org,
rbyers@chromium.org
Link to the patchset: https://codereview.chromium.org/1888163003/#ps140001
(title: "More nits")

[commit-bot: I haz the power, 2016-04-20 19:38:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1888163003/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1888163003/140001

[commit-bot: I haz the power, 2016-04-20 19:41:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-20 19:41:27]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_gn...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[Charlie Reis, 2016-04-20 19:53:40]

https://codereview.chromium.org/1888163003/diff/100001/content/renderer/rende...
File content/renderer/render_widget_unittest.cc (right):

https://codereview.chromium.org/1888163003/diff/100001/content/renderer/rende...
content/renderer/render_widget_unittest.cc:82: MockWebWidget& mock_webwidget() {
return mock_webwidget_; }
On 2016/04/20 19:38:11, dtapuska wrote:
> On 2016/04/20 17:16:25, Charlie Reis wrote:
> > nit: const ref
> 
> The mocks won't accept a const ref so sadly can't do this.

Then you can't use a ref.  Can you use a pointer instead?

https://google.github.io/styleguide/cppguide.html#Reference_Arguments

[Charlie Reis, 2016-04-20 19:56:13]

https://codereview.chromium.org/1888163003/diff/100001/content/renderer/rende...
File content/renderer/render_widget_unittest.cc (right):

https://codereview.chromium.org/1888163003/diff/100001/content/renderer/rende...
content/renderer/render_widget_unittest.cc:82: MockWebWidget& mock_webwidget() {
return mock_webwidget_; }
On 2016/04/20 19:53:40, Charlie Reis wrote:
> On 2016/04/20 19:38:11, dtapuska wrote:
> > On 2016/04/20 17:16:25, Charlie Reis wrote:
> > > nit: const ref
> > 
> > The mocks won't accept a const ref so sadly can't do this.
> 
> Then you can't use a ref.  Can you use a pointer instead?
> 
> https://google.github.io/styleguide/cppguide.html#Reference_Arguments

Ah, this may be a more relevant link from the style guide:
https://google.github.io/styleguide/cppguide.html#Use_of_const
"do not return a [...] non-const reference to a data member."

[dtapuska, 2016-04-20 20:09:48]

https://codereview.chromium.org/1888163003/diff/100001/content/renderer/rende...
File content/renderer/render_widget_unittest.cc (right):

https://codereview.chromium.org/1888163003/diff/100001/content/renderer/rende...
content/renderer/render_widget_unittest.cc:82: MockWebWidget& mock_webwidget() {
return mock_webwidget_; }
On 2016/04/20 19:56:13, Charlie Reis wrote:
> On 2016/04/20 19:53:40, Charlie Reis wrote:
> > On 2016/04/20 19:38:11, dtapuska wrote:
> > > On 2016/04/20 17:16:25, Charlie Reis wrote:
> > > > nit: const ref
> > > 
> > > The mocks won't accept a const ref so sadly can't do this.
> > 
> > Then you can't use a ref.  Can you use a pointer instead?
> > 
> > https://google.github.io/styleguide/cppguide.html#Reference_Arguments
> 
> Ah, this may be a more relevant link from the style guide:
> https://google.github.io/styleguide/cppguide.html#Use_of_const
> "do not return a [...] non-const reference to a data member."

Done.

[dtapuska, 2016-04-20 20:09:55]

The CQ bit was checked by dtapuska@chromium.org

[dtapuska, 2016-04-20 20:09:56]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org,
isherman@chromium.org, tdresser@chromium.org, rbyers@chromium.org
Link to the patchset: https://codereview.chromium.org/1888163003/#ps160001
(title: "Rebase and non-const ref passed back")

[commit-bot: I haz the power, 2016-04-20 20:10:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1888163003/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1888163003/160001

[commit-bot: I haz the power, 2016-04-20 21:38:29]

Description was changed from

==========
Articulate the cancel behavior in the WebTouchEvent.

Change the cancelable flag to be an enumeration so that we
can determine if it is blocking or non-blocking and the reason.

Wheel events will add this flag in a follow up CL.

BUG=601661
==========

to

==========
Articulate the cancel behavior in the WebTouchEvent.

Change the cancelable flag to be an enumeration so that we
can determine if it is blocking or non-blocking and the reason.

Wheel events will add this flag in a follow up CL.

BUG=601661
==========

[commit-bot: I haz the power, 2016-04-20 21:38:31]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-04-22 19:26:46]

Description was changed from

==========
Articulate the cancel behavior in the WebTouchEvent.

Change the cancelable flag to be an enumeration so that we
can determine if it is blocking or non-blocking and the reason.

Wheel events will add this flag in a follow up CL.

BUG=601661
==========

to

==========
Articulate the cancel behavior in the WebTouchEvent.

Change the cancelable flag to be an enumeration so that we
can determine if it is blocking or non-blocking and the reason.

Wheel events will add this flag in a follow up CL.

BUG=601661

Committed: https://crrev.com/afb8f54b782055923cadd711452c2448f3f7b5b4
Cr-Commit-Position: refs/heads/master@{#388585}
==========

[commit-bot: I haz the power, 2016-04-22 19:26:48]

Patchset 9 (id:??) landed as
https://crrev.com/afb8f54b782055923cadd711452c2448f3f7b5b4
Cr-Commit-Position: refs/heads/master@{#388585}