SourceEventType added to LatencyInfo

SourceEventType added to LatencyInfo

The source event type information will be used in a follow up patch to compute scroll latency metrics for touch and wheel, separately.

BUG=622827
TEST=RenderWidgetHostViewAuraTest.SourceEventTypeExistsInLatencyInfo, RenderWidgetHostViewMacTest.SourceEventTypeExistsInLatencyInfo, EventTest.PointerEventSourceEventTypeExistsInLatencyInfo

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/69895c817b673b47db19ca948ab0e6249e5bd699
Cr-Commit-Position: refs/heads/master@{#419204}

[sahel, 2016-09-08 14:24:26]

Description was changed from

==========
New latency metrics for Touch and Wheel

The new histograms compute latency info by subtracting the last event
time of one component from the first event time of its previous
component. (Deprecated histograms compute the latency by using average
event time of both.)


BUG=622827
TEST=RenderWidgetHostLatencyTrackerTest.WheelTestHistograms,
RenderWidgetHostLatencyTrackerTest.TouchTestHistograms
==========

to

==========
New latency metrics for Touch and Wheel

The new histograms compute latency info by subtracting the last event
time of one component from the first event time of its previous
component. (Deprecated histograms compute the latency by using average
event time of both.)


BUG=622827
TEST=RenderWidgetHostLatencyTrackerTest.WheelTestHistograms,
RenderWidgetHostLatencyTrackerTest.TouchTestHistograms
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[sahel, 2016-09-08 14:29:12]

The CQ bit was checked by sahel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-08 14:29:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sahel, 2016-09-08 14:32:11]

sahel@chromium.org changed reviewers:
+ tdresser@chromium.org

[sahel, 2016-09-08 14:32:12]

I haven't added any comments for deprecating the old histograms, yet. We might
need to change the new histogram names (I couldn't come up with more proper
names.)
And I don't know if I should put you as their owner or myself.

[commit-bot: I haz the power, 2016-09-08 14:50:27]

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

[commit-bot: I haz the power, 2016-09-08 14:50:28]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[tdresser, 2016-09-08 15:07:40]

On 2016/09/08 14:32:12, sahel wrote:
> I haven't added any comments for deprecating the old histograms, yet. We might
> need to change the new histogram names (I couldn't come up with more proper
> names.)
> And I don't know if I should put you as their owner or myself.

I'd recommend plumbing the source type through, and adding tests for that in one
patch, and then landing a followup patch which adds the new histograms. In
general, if you can divide a patch up into unrelated parts with separate tests,
it makes sense to do so.

If you'd prefer, I can review this though.

List me as an owner, but feel free to list yourself as well if you want.

I think this is going to have nasty merge conflicts with
https://codereview.chromium.org/2268163002/.

[sahel, 2016-09-08 15:22:07]

The CQ bit was checked by sahel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-08 15:22:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-08 16:00:54]

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

[commit-bot: I haz the power, 2016-09-08 16:00:55]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[sahel, 2016-09-08 20:41:13]

The CQ bit was checked by sahel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-08 20:41:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-08 21:19:55]

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

[commit-bot: I haz the power, 2016-09-08 21:19:55]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[sahel, 2016-09-09 16:45:07]

The CQ bit was checked by sahel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-09 16:45:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sahel, 2016-09-09 17:29:57]

Description was changed from

==========
New latency metrics for Touch and Wheel

The new histograms compute latency info by subtracting the last event
time of one component from the first event time of its previous
component. (Deprecated histograms compute the latency by using average
event time of both.)


BUG=622827
TEST=RenderWidgetHostLatencyTrackerTest.WheelTestHistograms,
RenderWidgetHostLatencyTrackerTest.TouchTestHistograms
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
SourceEventType added to LatencyInfo 

The source event type information will be used in a follow up patch to compute
scroll latency metrics for touch and wheel, separately. 

BUG=622827
TEST=RenderWidgetHostViewAuraTest.SourceEventTypeExistsInLatencyInfo,
RenderWidgetHostViewMacTest.SourceEventTypeExistsInLatencyInfo
==========

[commit-bot: I haz the power, 2016-09-09 18:03:08]

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

[commit-bot: I haz the power, 2016-09-09 18:03:09]

Dry run: This issue passed the CQ dry run.

[tdresser, 2016-09-12 14:23:09]

https://codereview.chromium.org/2317253005/diff/60001/content/browser/browser...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://codereview.chromium.org/2317253005/diff/60001/content/browser/browser...
content/browser/browser_plugin/browser_plugin_guest.cc:469: //
https://crbug.com/613628
Remove TODO, update linked bug, cc James.

Does this have a test?

https://codereview.chromium.org/2317253005/diff/60001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/2317253005/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:5: #include
"content/browser/renderer_host/render_widget_host_view_android.h"
Should we add a test for logic in this file?

https://codereview.chromium.org/2317253005/diff/60001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura_unittest.cc
(right):

https://codereview.chromium.org/2317253005/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:448: 
I think this would be clearer if you removed the space, and included the comment
above both Forward*EventWithLatencyInfo methods.

https://codereview.chromium.org/2317253005/diff/60001/ui/events/blink/web_inp...
File ui/events/blink/web_input_event_traits.cc (right):

https://codereview.chromium.org/2317253005/diff/60001/ui/events/blink/web_inp...
ui/events/blink/web_input_event_traits.cc:262:
blink::WebGestureDevice::WebGestureDeviceTouchscreen)
I prefer using {} when the condition is multiline (though this isn't technically
required by the style guide, so your call.

https://codereview.chromium.org/2317253005/diff/60001/ui/events/event.cc
File ui/events/event.cc (right):

https://codereview.chromium.org/2317253005/diff/60001/ui/events/event.cc#newc...
ui/events/event.cc:903:
latency()->set_source_event_type(ui::SourceEventType::TOUCH);
Should it be set to "OTHER" if it isn't touch?

https://codereview.chromium.org/2317253005/diff/60001/ui/events/latency_info.cc
File ui/events/latency_info.cc (right):

https://codereview.chromium.org/2317253005/diff/60001/ui/events/latency_info....
ui/events/latency_info.cc:146: source_event_type_(SourceEventType::UNKOWN) {}
Use a delegated constructor here.

(https://www.ibm.com/developerworks/community/blogs/5894415f-be62-4bc0-81c5-39...)

https://codereview.chromium.org/2317253005/diff/60001/ui/events/latency_info.h
File ui/events/latency_info.h (right):

https://codereview.chromium.org/2317253005/diff/60001/ui/events/latency_info....
ui/events/latency_info.h:260: SourceEventType source_event_type_;
Add comment.

[tdresser, 2016-09-12 15:36:37]

https://codereview.chromium.org/2317253005/diff/60001/content/browser/browser...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://codereview.chromium.org/2317253005/diff/60001/content/browser/browser...
content/browser/browser_plugin/browser_plugin_guest.cc:469: //
https://crbug.com/613628
On 2016/09/12 14:23:08, tdresser wrote:
> Remove TODO, update linked bug, cc James.
> 
> Does this have a test?

Whoops, you're right, this isn't done.

We should still cc james though.

[sahel, 2016-09-12 16:05:38]

Description was changed from

==========
SourceEventType added to LatencyInfo 

The source event type information will be used in a follow up patch to compute
scroll latency metrics for touch and wheel, separately. 

BUG=622827
TEST=RenderWidgetHostViewAuraTest.SourceEventTypeExistsInLatencyInfo,
RenderWidgetHostViewMacTest.SourceEventTypeExistsInLatencyInfo
==========

to

==========
SourceEventType added to LatencyInfo 

The source event type information will be used in a follow up patch to compute
scroll latency metrics for touch and wheel, separately. 

BUG=622827
TEST=RenderWidgetHostViewAuraTest.SourceEventTypeExistsInLatencyInfo,
RenderWidgetHostViewMacTest.SourceEventTypeExistsInLatencyInfo
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[sahel, 2016-09-12 17:02:04]

The CQ bit was checked by sahel@chromium.org to run a CQ dry run

[sahel, 2016-09-12 17:02:26]

https://codereview.chromium.org/2317253005/diff/60001/content/browser/browser...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://codereview.chromium.org/2317253005/diff/60001/content/browser/browser...
content/browser/browser_plugin/browser_plugin_guest.cc:469: //
https://crbug.com/613628
On 2016/09/12 15:36:37, tdresser wrote:
> On 2016/09/12 14:23:08, tdresser wrote:
> > Remove TODO, update linked bug, cc James.
> > 
> > Does this have a test?
> 
> Whoops, you're right, this isn't done.
> 
> We should still cc james though.

Acknowledged.

https://codereview.chromium.org/2317253005/diff/60001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/2317253005/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:5: #include
"content/browser/renderer_host/render_widget_host_view_android.h"
On 2016/09/12 14:23:08, tdresser wrote:
> Should we add a test for logic in this file?

There is no unittest files available for render_widget_host_view_android.cc and
that's why I couldn't add a test.

https://codereview.chromium.org/2317253005/diff/60001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura_unittest.cc
(right):

https://codereview.chromium.org/2317253005/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:448: 
On 2016/09/12 14:23:08, tdresser wrote:
> I think this would be clearer if you removed the space, and included the
comment
> above both Forward*EventWithLatencyInfo methods.

Done.

https://codereview.chromium.org/2317253005/diff/60001/ui/events/blink/web_inp...
File ui/events/blink/web_input_event_traits.cc (right):

https://codereview.chromium.org/2317253005/diff/60001/ui/events/blink/web_inp...
ui/events/blink/web_input_event_traits.cc:262:
blink::WebGestureDevice::WebGestureDeviceTouchscreen)
On 2016/09/12 14:23:08, tdresser wrote:
> I prefer using {} when the condition is multiline (though this isn't
technically
> required by the style guide, so your call.

Done.

https://codereview.chromium.org/2317253005/diff/60001/ui/events/event.cc
File ui/events/event.cc (right):

https://codereview.chromium.org/2317253005/diff/60001/ui/events/event.cc#newc...
ui/events/event.cc:903:
latency()->set_source_event_type(ui::SourceEventType::TOUCH);
On 2016/09/12 14:23:08, tdresser wrote:
> Should it be set to "OTHER" if it isn't touch?

We decided to check the source and set it only in the case of touch. If I set it
to OTHER, it means it's neither TOUCH nor WHEEL, which is not true in all cases.

https://codereview.chromium.org/2317253005/diff/60001/ui/events/latency_info.cc
File ui/events/latency_info.cc (right):

https://codereview.chromium.org/2317253005/diff/60001/ui/events/latency_info....
ui/events/latency_info.cc:146: source_event_type_(SourceEventType::UNKOWN) {}
On 2016/09/12 14:23:08, tdresser wrote:
> Use a delegated constructor here.
> 
>
(https://www.ibm.com/developerworks/community/blogs/5894415f-be62-4bc0-81c5-39...)

Done.

https://codereview.chromium.org/2317253005/diff/60001/ui/events/latency_info.h
File ui/events/latency_info.h (right):

https://codereview.chromium.org/2317253005/diff/60001/ui/events/latency_info....
ui/events/latency_info.h:260: SourceEventType source_event_type_;
On 2016/09/12 14:23:09, tdresser wrote:
> Add comment.

Done.

[commit-bot: I haz the power, 2016-09-12 17:02:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tdresser, 2016-09-12 18:03:43]

tdresser@chromium.org changed reviewers:
+ wjmaclean@chromium.org

[tdresser, 2016-09-12 18:03:45]

+James

LGTM with nits.

https://codereview.chromium.org/2317253005/diff/60001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/2317253005/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:5: #include
"content/browser/renderer_host/render_widget_host_view_android.h"
On 2016/09/12 17:02:26, sahel wrote:
> On 2016/09/12 14:23:08, tdresser wrote:
> > Should we add a test for logic in this file?
> 
> There is no unittest files available for render_widget_host_view_android.cc
and
> that's why I couldn't add a test.

Bah, of course.

https://codereview.chromium.org/2317253005/diff/60001/ui/events/blink/web_inp...
File ui/events/blink/web_input_event_traits.cc (right):

https://codereview.chromium.org/2317253005/diff/60001/ui/events/blink/web_inp...
ui/events/blink/web_input_event_traits.cc:262:
blink::WebGestureDevice::WebGestureDeviceTouchscreen)
On 2016/09/12 17:02:26, sahel wrote:
> On 2016/09/12 14:23:08, tdresser wrote:
> > I prefer using {} when the condition is multiline (though this isn't
> technically
> > required by the style guide, so your call.
> 
> Done.

Sorry, this means you need {} on the previous clause as well.

"However, if one part of an if-else statement uses curly braces, the other part
must too:"

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

https://codereview.chromium.org/2317253005/diff/60001/ui/events/event.cc
File ui/events/event.cc (right):

https://codereview.chromium.org/2317253005/diff/60001/ui/events/event.cc#newc...
ui/events/event.cc:903:
latency()->set_source_event_type(ui::SourceEventType::TOUCH);
On 2016/09/12 17:02:26, sahel wrote:
> On 2016/09/12 14:23:08, tdresser wrote:
> > Should it be set to "OTHER" if it isn't touch?
> 
> We decided to check the source and set it only in the case of touch. If I set
it
> to OTHER, it means it's neither TOUCH nor WHEEL, which is not true in all
cases.

Don't we only make pointer events for mouse and touch? This will never be wheel,
will it?

https://codereview.chromium.org/2317253005/diff/80001/ui/events/latency_info.h
File ui/events/latency_info.h (right):

https://codereview.chromium.org/2317253005/diff/80001/ui/events/latency_info....
ui/events/latency_info.h:122: UNKOWN,
UNKOWN -> UNKNOWN

[tdresser, 2016-09-12 18:39:37]

+Lan and Deep as FYI

[commit-bot: I haz the power, 2016-09-12 19:10:34]

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

[commit-bot: I haz the power, 2016-09-12 19:10:35]

Dry run: This issue passed the CQ dry run.

[sahel, 2016-09-12 20:56:35]

sahel@chromium.org changed reviewers:
+ jochen@chromium.org, nasko@chromium.org, sky@chromium.org

[sahel, 2016-09-12 20:56:36]

jochen@chromium.org: Please review changes in 

content/browser/frame_host/render_widget_host_view_guest.cc
content/browser/site_per_process_browsertest.cc

nasko@chromium.org: Please review changes in

ui/events/ipc/latency_info_param_traits.cc
ui/events/ipc/latency_info_param_traits_macros.h 

sky@chromium.org: Please review changes in 
ui/events/event.cc
ui/events/event_unittest.cc

https://codereview.chromium.org/2317253005/diff/80001/ui/events/latency_info.h
File ui/events/latency_info.h (right):

https://codereview.chromium.org/2317253005/diff/80001/ui/events/latency_info....
ui/events/latency_info.h:122: UNKOWN,
On 2016/09/12 18:03:44, tdresser wrote:
> UNKOWN -> UNKNOWN

Done.

[sahel, 2016-09-12 20:57:30]

Description was changed from

==========
SourceEventType added to LatencyInfo 

The source event type information will be used in a follow up patch to compute
scroll latency metrics for touch and wheel, separately. 

BUG=622827
TEST=RenderWidgetHostViewAuraTest.SourceEventTypeExistsInLatencyInfo,
RenderWidgetHostViewMacTest.SourceEventTypeExistsInLatencyInfo
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
SourceEventType added to LatencyInfo 

The source event type information will be used in a follow up patch to compute
scroll latency metrics for touch and wheel, separately. 

BUG=622827
TEST=RenderWidgetHostViewAuraTest.SourceEventTypeExistsInLatencyInfo,
RenderWidgetHostViewMacTest.SourceEventTypeExistsInLatencyInfo,
EventTest.PointerEventSourceEventTypeExistsInLatencyInfo

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[sahel, 2016-09-12 21:04:32]

Patchset #6 (id:100001) has been deleted

[nasko, 2016-09-12 21:08:45]

IPC LGTM

[sahel, 2016-09-12 21:53:22]

The CQ bit was checked by sahel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-12 21:53:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-12 22:59:09]

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

[commit-bot: I haz the power, 2016-09-12 22:59:10]

Dry run: This issue passed the CQ dry run.

[sky, 2016-09-13 00:12:18]

https://codereview.chromium.org/2317253005/diff/120001/ui/events/event.cc
File ui/events/event.cc (right):

https://codereview.chromium.org/2317253005/diff/120001/ui/events/event.cc#new...
ui/events/event.cc:1118:
latency()->set_source_event_type(ui::SourceEventType::OTHER);
Why do you have to explicitly set the source type for OTHER? Shouldn't that be
the default?

[sahel, 2016-09-13 14:29:53]

https://codereview.chromium.org/2317253005/diff/120001/ui/events/event.cc
File ui/events/event.cc (right):

https://codereview.chromium.org/2317253005/diff/120001/ui/events/event.cc#new...
ui/events/event.cc:1118:
latency()->set_source_event_type(ui::SourceEventType::OTHER);
On 2016/09/13 00:12:18, sky wrote:
> Why do you have to explicitly set the source type for OTHER? Shouldn't that be
> the default?

The default is UNKNOWN, OTHER is for the cases that we know the source is not
WHEEL or TOUCH. We decided two have both values because in many places the
default constructor is called and it doesn't set the source.

[tdresser, 2016-09-13 14:46:34]

On 2016/09/13 14:29:53, sahel wrote:
> https://codereview.chromium.org/2317253005/diff/120001/ui/events/event.cc
> File ui/events/event.cc (right):
> 
>
https://codereview.chromium.org/2317253005/diff/120001/ui/events/event.cc#new...
> ui/events/event.cc:1118:
> latency()->set_source_event_type(ui::SourceEventType::OTHER);
> On 2016/09/13 00:12:18, sky wrote:
> > Why do you have to explicitly set the source type for OTHER? Shouldn't that
be
> > the default?
> 
> The default is UNKNOWN, OTHER is for the cases that we know the source is not
> WHEEL or TOUCH. We decided two have both values because in many places the
> default constructor is called and it doesn't set the source.

We'd like to eventually be able to DCHECK that the type != UNKNOWN, despite the
fact that the type will sometimes be OTHER.

[wjmaclean, 2016-09-13 15:11:39]

lgtm for webview, rwhier

[sky, 2016-09-13 18:22:01]

Why are we going with an incomplete mapping? For example, we have
TOUCH, but not MOUSE? I believe there are other pointer types we're
going to support shortly as well.

On Tue, Sep 13, 2016 at 7:46 AM,  <tdresser@chromium.org> wrote:
> On 2016/09/13 14:29:53, sahel wrote:
>> https://codereview.chromium.org/2317253005/diff/120001/ui/events/event.cc
>> File ui/events/event.cc (right):
>>
>>
>
https://codereview.chromium.org/2317253005/diff/120001/ui/events/event.cc#new...
>> ui/events/event.cc:1118:
>> latency()->set_source_event_type(ui::SourceEventType::OTHER);
>> On 2016/09/13 00:12:18, sky wrote:
>> > Why do you have to explicitly set the source type for OTHER? Shouldn't
>> > that
> be
>> > the default?
>>
>> The default is UNKNOWN, OTHER is for the cases that we know the source is
>> not
>> WHEEL or TOUCH. We decided two have both values because in many places the
>> default constructor is called and it doesn't set the source.
>
> We'd like to eventually be able to DCHECK that the type != UNKNOWN, despite
> the
> fact that the type will sometimes be OTHER.
>
> https://codereview.chromium.org/2317253005/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sahel, 2016-09-13 18:31:32]

On 2016/09/13 18:22:01, sky wrote:
> Why are we going with an incomplete mapping? For example, we have
> TOUCH, but not MOUSE? I believe there are other pointer types we're
> going to support shortly as well.
> 
> On Tue, Sep 13, 2016 at 7:46 AM,  <mailto:tdresser@chromium.org> wrote:
> > On 2016/09/13 14:29:53, sahel wrote:
> >> https://codereview.chromium.org/2317253005/diff/120001/ui/events/event.cc
> >> File ui/events/event.cc (right):
> >>
> >>
> >
>
https://codereview.chromium.org/2317253005/diff/120001/ui/events/event.cc#new...
> >> ui/events/event.cc:1118:
> >> latency()->set_source_event_type(ui::SourceEventType::OTHER);
> >> On 2016/09/13 00:12:18, sky wrote:
> >> > Why do you have to explicitly set the source type for OTHER? Shouldn't
> >> > that
> > be
> >> > the default?
> >>
> >> The default is UNKNOWN, OTHER is for the cases that we know the source is
> >> not
> >> WHEEL or TOUCH. We decided two have both values because in many places the
> >> default constructor is called and it doesn't set the source.
> >
> > We'd like to eventually be able to DCHECK that the type != UNKNOWN, despite
> > the
> > fact that the type will sometimes be OTHER.
> >
> > https://codereview.chromium.org/2317253005/
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.
> 

The sources that we are interested in are only TOUCH and WHEEL, because we need
it
for scroll latency metrics. That's why all other sources fall into category of
OTHER
without specifying the type.

[sky, 2016-09-13 20:03:37]

On 2016/09/13 18:31:32, sahel wrote:
> On 2016/09/13 18:22:01, sky wrote:
> > Why are we going with an incomplete mapping? For example, we have
> > TOUCH, but not MOUSE? I believe there are other pointer types we're
> > going to support shortly as well.
> > 
> > On Tue, Sep 13, 2016 at 7:46 AM,  <mailto:tdresser@chromium.org> wrote:
> > > On 2016/09/13 14:29:53, sahel wrote:
> > >> https://codereview.chromium.org/2317253005/diff/120001/ui/events/event.cc
> > >> File ui/events/event.cc (right):
> > >>
> > >>
> > >
> >
>
https://codereview.chromium.org/2317253005/diff/120001/ui/events/event.cc#new...
> > >> ui/events/event.cc:1118:
> > >> latency()->set_source_event_type(ui::SourceEventType::OTHER);
> > >> On 2016/09/13 00:12:18, sky wrote:
> > >> > Why do you have to explicitly set the source type for OTHER? Shouldn't
> > >> > that
> > > be
> > >> > the default?
> > >>
> > >> The default is UNKNOWN, OTHER is for the cases that we know the source is
> > >> not
> > >> WHEEL or TOUCH. We decided two have both values because in many places
the
> > >> default constructor is called and it doesn't set the source.
> > >
> > > We'd like to eventually be able to DCHECK that the type != UNKNOWN,
despite
> > > the
> > > fact that the type will sometimes be OTHER.
> > >
> > > https://codereview.chromium.org/2317253005/
> > 
> > -- 
> > You received this message because you are subscribed to the Google Groups
> > "Chromium-reviews" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.
> > 
> 
> The sources that we are interested in are only TOUCH and WHEEL, because we
need
> it
> for scroll latency metrics. That's why all other sources fall into category of
> OTHER
> without specifying the type.

Ok, fair enough.
Doesn't Event::type_ contain enough information to set the source event type?
I'm wondering if you can move the logic of figuring out the type to Event's
constructors and not set it everywhere else?

[chromium-reviews, 2016-09-13 20:17:52]

If you meant checking only type instead of pointer type in pointer events,
there are many touch events and it's easier to check the pointertype.
I didn't do that for WHEEL, because the pointer type is MOUSE in that case
and
I had to check the event type as well. If this wan't what you meant, pleas
explain more.


On Tue, Sep 13, 2016 at 4:04 PM <sky@chromium.org> wrote:

> On 2016/09/13 18:31:32, sahel wrote:
> > On 2016/09/13 18:22:01, sky wrote:
> > > Why are we going with an incomplete mapping? For example, we have
> > > TOUCH, but not MOUSE? I believe there are other pointer types we're
> > > going to support shortly as well.
> > >
> > > On Tue, Sep 13, 2016 at 7:46 AM, <mailto:tdresser@chromium.org> wrote:
> > > > On 2016/09/13 14:29:53, sahel wrote:
> > > >>
> https://codereview.chromium.org/2317253005/diff/120001/ui/events/event.cc
> > > >> File ui/events/event.cc (right):
> > > >>
> > > >>
> > > >
> > >
> >
>
>
https://codereview.chromium.org/2317253005/diff/120001/ui/events/event.cc#new...
> > > >> ui/events/event.cc:1118:
> > > >> latency()->set_source_event_type(ui::SourceEventType::OTHER);
> > > >> On 2016/09/13 00:12:18, sky wrote:
> > > >> > Why do you have to explicitly set the source type for OTHER?
> Shouldn't
> > > >> > that
> > > > be
> > > >> > the default?
> > > >>
> > > >> The default is UNKNOWN, OTHER is for the cases that we know the
> source is
> > > >> not
> > > >> WHEEL or TOUCH. We decided two have both values because in many
> places
> the
> > > >> default constructor is called and it doesn't set the source.
> > > >
> > > > We'd like to eventually be able to DCHECK that the type != UNKNOWN,
> despite
> > > > the
> > > > fact that the type will sometimes be OTHER.
> > > >
> > > > https://codereview.chromium.org/2317253005/
> > >
> > > --
> > > You received this message because you are subscribed to the Google
> Groups
> > > "Chromium-reviews" group.
> > > To unsubscribe from this group and stop receiving emails from it, send
> an
> > email
> > > to mailto:chromium-reviews+unsubscribe@chromium.org.
> > >
> >
> > The sources that we are interested in are only TOUCH and WHEEL, because
> we
> need
> > it
> > for scroll latency metrics. That's why all other sources fall into
> category of
> > OTHER
> > without specifying the type.
>
> Ok, fair enough.
> Doesn't Event::type_ contain enough information to set the source event
> type?
> I'm wondering if you can move the logic of figuring out the type to Event's
> constructors and not set it everywhere else?
>
> https://codereview.chromium.org/2317253005/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sky, 2016-09-14 02:52:43]

https://codereview.chromium.org/2317253005/diff/120001/ui/events/event.cc
File ui/events/event.cc (right):

https://codereview.chromium.org/2317253005/diff/120001/ui/events/event.cc#new...
ui/events/event.cc:336: if (type_ < ET_LAST)
I'm suggesting this, and the other Event constructors have something like:

latency()->set_source_event_type(EventTypeToLatencySourceEventType(type));

EventtypeToLatencySourceEventType is a big switch statement. PointerEvents sound
still set the source type as type_ isn't enough for them.

[sahel, 2016-09-14 18:56:18]

The CQ bit was checked by sahel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-14 18:57:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-14 20:09:00]

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

[commit-bot: I haz the power, 2016-09-14 20:09:01]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sahel, 2016-09-14 20:26:18]

Patchset #7 (id:140001) has been deleted

[sahel, 2016-09-14 20:28:43]

The CQ bit was checked by sahel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-14 20:29:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-14 21:20:56]

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

[commit-bot: I haz the power, 2016-09-14 21:20:57]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[sahel, 2016-09-15 15:31:20]

Patchset #7 (id:160001) has been deleted

[sahel, 2016-09-15 15:33:22]

The CQ bit was checked by sahel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-15 15:33:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-15 16:48:38]

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

[commit-bot: I haz the power, 2016-09-15 16:48:39]

Dry run: This issue passed the CQ dry run.

[sahel, 2016-09-15 17:07:21]

On 2016/09/14 02:52:43, sky wrote:
> https://codereview.chromium.org/2317253005/diff/120001/ui/events/event.cc
> File ui/events/event.cc (right):
> 
>
https://codereview.chromium.org/2317253005/diff/120001/ui/events/event.cc#new...
> ui/events/event.cc:336: if (type_ < ET_LAST)
> I'm suggesting this, and the other Event constructors have something like:
> 
> latency()->set_source_event_type(EventTypeToLatencySourceEventType(type));
> 
> EventtypeToLatencySourceEventType is a big switch statement. PointerEvents
sound
> still set the source type as type_ isn't enough for them.

I changed the code to use the helper function. For gestureEvents/scrollEvents/
PointerEvents the function returns UNKNOWN, and proper values are assigned in
constructor of the subclasses rather than the Event class.

[sky, 2016-09-15 17:56:34]

https://codereview.chromium.org/2317253005/diff/180001/ui/events/event.cc
File ui/events/event.cc (right):

https://codereview.chromium.org/2317253005/diff/180001/ui/events/event.cc#new...
ui/events/event.cc:146: // WHEEL. The proper value is assigned in the
Constructors.
nit: 'C' -> 'c'.

https://codereview.chromium.org/2317253005/diff/180001/ui/events/event.cc#new...
ui/events/event.cc:202: default:
If you have this and we add a new type it'll be missed. Remove the default so
it's a compile error if someone adds a new type without updating this.

https://codereview.chromium.org/2317253005/diff/180001/ui/events/event.cc#new...
ui/events/event.cc:479: }
Shouldn't you handle the else case here? What if type_ goes from < ET_LAST to >=
ET_LAST?

[sahel, 2016-09-15 18:58:08]

https://codereview.chromium.org/2317253005/diff/120001/ui/events/event.cc
File ui/events/event.cc (right):

https://codereview.chromium.org/2317253005/diff/120001/ui/events/event.cc#new...
ui/events/event.cc:336: if (type_ < ET_LAST)
On 2016/09/14 02:52:43, sky wrote:
> I'm suggesting this, and the other Event constructors have something like:
> 
> latency()->set_source_event_type(EventTypeToLatencySourceEventType(type));
> 
> EventtypeToLatencySourceEventType is a big switch statement. PointerEvents
sound
> still set the source type as type_ isn't enough for them.

Done.

https://codereview.chromium.org/2317253005/diff/180001/ui/events/event.cc
File ui/events/event.cc (right):

https://codereview.chromium.org/2317253005/diff/180001/ui/events/event.cc#new...
ui/events/event.cc:146: // WHEEL. The proper value is assigned in the
Constructors.
On 2016/09/15 17:56:34, sky wrote:
> nit: 'C' -> 'c'.

Done.

https://codereview.chromium.org/2317253005/diff/180001/ui/events/event.cc#new...
ui/events/event.cc:202: default:
On 2016/09/15 17:56:34, sky wrote:
> If you have this and we add a new type it'll be missed. Remove the default so
> it's a compile error if someone adds a new type without updating this.

Done.

https://codereview.chromium.org/2317253005/diff/180001/ui/events/event.cc#new...
ui/events/event.cc:479: }
On 2016/09/15 17:56:34, sky wrote:
> Shouldn't you handle the else case here? What if type_ goes from < ET_LAST to
>=
> ET_LAST?

The default value is UNKNOWN, and it would take care of the else case.

[sky, 2016-09-15 22:44:52]

LGTM

[sahel, 2016-09-16 14:47:09]

The CQ bit was checked by sahel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-16 14:47:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-16 14:50:13]

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

[commit-bot: I haz the power, 2016-09-16 14:50:14]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[sahel, 2016-09-16 15:29:07]

The CQ bit was checked by sahel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-16 15:29:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-16 15:43:38]

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

[commit-bot: I haz the power, 2016-09-16 15:43:39]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[sahel, 2016-09-16 16:06:25]

The CQ bit was checked by sahel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-16 16:06:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-16 17:06:45]

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

[commit-bot: I haz the power, 2016-09-16 17:06:47]

Dry run: This issue passed the CQ dry run.

[sahel, 2016-09-16 17:30:17]

The CQ bit was checked by sahel@chromium.org

[sahel, 2016-09-16 17:30:18]

The patchset sent to the CQ was uploaded after l-g-t-m from
tdresser@chromium.org, wjmaclean@chromium.org, nasko@chromium.org,
sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2317253005/#ps240001
(title: "Added a fake return to the end of EventTypeToLatencySourceEventType,
for compilers that don't know …")

[commit-bot: I haz the power, 2016-09-16 17:30:50]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-16 17:37:06]

Description was changed from

==========
SourceEventType added to LatencyInfo 

The source event type information will be used in a follow up patch to compute
scroll latency metrics for touch and wheel, separately. 

BUG=622827
TEST=RenderWidgetHostViewAuraTest.SourceEventTypeExistsInLatencyInfo,
RenderWidgetHostViewMacTest.SourceEventTypeExistsInLatencyInfo,
EventTest.PointerEventSourceEventTypeExistsInLatencyInfo

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
SourceEventType added to LatencyInfo 

The source event type information will be used in a follow up patch to compute
scroll latency metrics for touch and wheel, separately. 

BUG=622827
TEST=RenderWidgetHostViewAuraTest.SourceEventTypeExistsInLatencyInfo,
RenderWidgetHostViewMacTest.SourceEventTypeExistsInLatencyInfo,
EventTest.PointerEventSourceEventTypeExistsInLatencyInfo

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-09-16 17:37:11]

Committed patchset #10 (id:240001)

[commit-bot: I haz the power, 2016-09-16 17:41:56]

Description was changed from

==========
SourceEventType added to LatencyInfo 

The source event type information will be used in a follow up patch to compute
scroll latency metrics for touch and wheel, separately. 

BUG=622827
TEST=RenderWidgetHostViewAuraTest.SourceEventTypeExistsInLatencyInfo,
RenderWidgetHostViewMacTest.SourceEventTypeExistsInLatencyInfo,
EventTest.PointerEventSourceEventTypeExistsInLatencyInfo

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
SourceEventType added to LatencyInfo

The source event type information will be used in a follow up patch to compute
scroll latency metrics for touch and wheel, separately.

BUG=622827
TEST=RenderWidgetHostViewAuraTest.SourceEventTypeExistsInLatencyInfo,
RenderWidgetHostViewMacTest.SourceEventTypeExistsInLatencyInfo,
EventTest.PointerEventSourceEventTypeExistsInLatencyInfo

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/69895c817b673b47db19ca948ab0e6249e5bd699
Cr-Commit-Position: refs/heads/master@{#419204}
==========

[commit-bot: I haz the power, 2016-09-16 17:41:59]

Patchset 10 (id:??) landed as
https://crrev.com/69895c817b673b47db19ca948ab0e6249e5bd699
Cr-Commit-Position: refs/heads/master@{#419204}

[brucedawson, 2016-09-19 17:40:21]

brucedawson@chromium.org changed reviewers:
+ brucedawson@chromium.org

[brucedawson, 2016-09-19 17:40:22]

https://codereview.chromium.org/2317253005/diff/240001/ui/events/event.cc
File ui/events/event.cc (right):

https://codereview.chromium.org/2317253005/diff/240001/ui/events/event.cc#new...
ui/events/event.cc:1420: if ((flags | EF_FROM_TOUCH) ||
This line of code appears to be incorrect. This was detected through a warning
from the experimental VC++ /analyze static code analysis bot:

ui\events\event.cc(1420) : warning C6316: Incorrect operator:  tested expression
is constant and non-zero.  Use bitwise-and to determine whether bits are set.

As the warning says, this test is always true and '&' was almost certainly
intended.

The tracking bug number for /analyze-found bugs is 427616.

[sahel, 2016-09-20 19:10:32]

I fixed the issue.

https://codereview.chromium.org/2317253005/diff/240001/ui/events/event.cc
File ui/events/event.cc (right):

https://codereview.chromium.org/2317253005/diff/240001/ui/events/event.cc#new...
ui/events/event.cc:1420: if ((flags | EF_FROM_TOUCH) ||
On 2016/09/19 17:40:21, brucedawson wrote:
> This line of code appears to be incorrect. This was detected through a warning
> from the experimental VC++ /analyze static code analysis bot:
> 
> ui\events\event.cc(1420) : warning C6316: Incorrect operator:  tested
expression
> is constant and non-zero.  Use bitwise-and to determine whether bits are set.
> 
> As the warning says, this test is always true and '&' was almost certainly
> intended.
> 
> The tracking bug number for /analyze-found bugs is 427616.

Acknowledged.