Remove LatencyInfo::sequence_number. (May break metrics).

Remove LatencyInfo::sequence_number. (May break metrics).

sequence_number appears to be entirely useless.

It seems plausible that it's being used downstream somehow in some telemetry
tests or something, but I ran a few telemetry tests and things seemed fine.

I think we might as well land this and keep an eye on it.

If it breaks stuff, revert it!

BUG=731202
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

[tdresser, 2017-05-31 17:23:34]

Description was changed from

==========
Not for review - try removing LatencyInfo::sequence_number.

BUG=
==========

to

==========
Not for review - try removing LatencyInfo::sequence_number.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[tdresser, 2017-05-31 17:23:38]

Description was changed from

==========
Not for review - try removing LatencyInfo::sequence_number.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Not for review - try removing LatencyInfo::sequence_number.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[tdresser, 2017-05-31 17:28:36]

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

[commit-bot: I haz the power, 2017-05-31 17:29:14]

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, 2017-05-31 17:36:39]

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

[commit-bot: I haz the power, 2017-05-31 17:36:40]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  android_optional_gpu_tests_rel on master.tryserver.chromium.android
(JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_opti...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[tdresser, 2017-06-05 19:52:44]

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

[commit-bot: I haz the power, 2017-06-05 19:53:05]

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, 2017-06-05 20:07:35]

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

[commit-bot: I haz the power, 2017-06-05 20:07:36]

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

[tdresser, 2017-06-05 20:30:26]

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

[commit-bot: I haz the power, 2017-06-05 20:30:57]

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, 2017-06-05 20:45:59]

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

[commit-bot: I haz the power, 2017-06-05 20:46:00]

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

[tdresser, 2017-06-06 17:26:35]

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

[commit-bot: I haz the power, 2017-06-06 17:27:03]

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, 2017-06-06 18:00:33]

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

[commit-bot: I haz the power, 2017-06-06 18:00:34]

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_...)

[tdresser, 2017-06-07 13:55:31]

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

[commit-bot: I haz the power, 2017-06-07 13:56:32]

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, 2017-06-07 14:30:43]

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

[commit-bot: I haz the power, 2017-06-07 14:30:44]

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

[tdresser, 2017-06-08 16:55:48]

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

[commit-bot: I haz the power, 2017-06-08 16:56:12]

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, 2017-06-08 18:16:31]

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

[commit-bot: I haz the power, 2017-06-08 18:16:32]

Dry run: This issue passed the CQ dry run.

[tdresser, 2017-06-08 18:36:42]

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

[tdresser, 2017-06-08 18:36:44]

sequence_number appears to be entirely useless.

It seems plausible that it's being used downstream somehow in some telemetry
tests or something, but I ran a few telemetry tests and things seemed fine.

I think we might as well land this and keep an eye on it.

If it breaks stuff, revert it!

https://codereview.chromium.org/2914023002/diff/100001/cc/trees/latency_info_...
File cc/trees/latency_info_swap_promise_monitor.cc (left):

https://codereview.chromium.org/2914023002/diff/100001/cc/trees/latency_info_...
cc/trees/latency_info_swap_promise_monitor.cc:91: return;
This is a material change. 

I think what's happening here is that we're returning early if we've already
sent the event to this main thread.

I'm not sure when this could happen though...

https://codereview.chromium.org/2914023002/diff/100001/content/browser/render...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/2914023002/diff/100001/content/browser/render...
content/browser/renderer_host/render_widget_host_impl.cc:1840: ++snapshot_id;
Actual change.

https://codereview.chromium.org/2914023002/diff/100001/ui/latency/latency_inf...
File ui/latency/latency_info.cc (right):

https://codereview.chromium.org/2914023002/diff/100001/ui/latency/latency_inf...
ui/latency/latency_info.cc:217: static int global_trace_id = 0;
Real change.

https://codereview.chromium.org/2914023002/diff/100001/ui/latency/latency_inf...
ui/latency/latency_info.cc:220: trace_id_ = global_trace_id++;  // TODO.
I'm not super happy with this naming. Any thoughts?

https://codereview.chromium.org/2914023002/diff/100001/ui/latency/latency_inf...
File ui/latency/latency_info_unittest.cc (right):

https://codereview.chromium.org/2914023002/diff/100001/ui/latency/latency_inf...
ui/latency/latency_info_unittest.cc:78: TEST(LatencyInfoTest, TraceIdsIncrease)
{
Real change.

[tdresser, 2017-06-08 18:37:18]

Description was changed from

==========
Not for review - try removing LatencyInfo::sequence_number.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Remove LatencyInfo::sequence_number. (May break metrics).

sequence_number appears to be entirely useless.

It seems plausible that it's being used downstream somehow in some telemetry
tests or something, but I ran a few telemetry tests and things seemed fine.

I think we might as well land this and keep an eye on it.

If it breaks stuff, revert it!

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[tdresser, 2017-06-08 18:38:38]

Description was changed from

==========
Remove LatencyInfo::sequence_number. (May break metrics).

sequence_number appears to be entirely useless.

It seems plausible that it's being used downstream somehow in some telemetry
tests or something, but I ran a few telemetry tests and things seemed fine.

I think we might as well land this and keep an eye on it.

If it breaks stuff, revert it!

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Remove LatencyInfo::sequence_number. (May break metrics).

sequence_number appears to be entirely useless.

It seems plausible that it's being used downstream somehow in some telemetry
tests or something, but I ran a few telemetry tests and things seemed fine.

I think we might as well land this and keep an eye on it.

If it breaks stuff, revert it!

BUG=731202

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[dtapuska, 2017-06-09 15:15:36]

https://codereview.chromium.org/2914023002/diff/100001/ui/latency/latency_inf...
File ui/latency/latency_info.cc (right):

https://codereview.chromium.org/2914023002/diff/100001/ui/latency/latency_inf...
ui/latency/latency_info.cc:217: static int global_trace_id = 0;
On 2017/06/08 18:36:44, tdresser wrote:
> Real change.

This is kind of scary because latency_info.cc could be instantiated in multiple
processes. Should the global_trace_id be started off with some type of seed?

[dtapuska, 2017-06-26 20:51:44]

I think this is still open pending feedback from tdresser@ the static is kind of
scary because it could clash with sequences generated from other processes.

https://codereview.chromium.org/2914023002/diff/100001/ui/latency/latency_inf...
File ui/latency/latency_info.cc (right):

https://codereview.chromium.org/2914023002/diff/100001/ui/latency/latency_inf...
ui/latency/latency_info.cc:220: trace_id_ = global_trace_id++;  // TODO.
On 2017/06/08 18:36:44, tdresser wrote:
> I'm not super happy with this naming. Any thoughts?

global trace id isn't ideal because this is actually limited to this process id
and they are IPC'd.

[tdresser, 2017-06-26 20:53:37]

On 2017/06/26 20:51:44, dtapuska wrote:
> I think this is still open pending feedback from tdresser@ the static is kind
of
> scary because it could clash with sequences generated from other processes.
> 
>
https://codereview.chromium.org/2914023002/diff/100001/ui/latency/latency_inf...
> File ui/latency/latency_info.cc (right):
> 
>
https://codereview.chromium.org/2914023002/diff/100001/ui/latency/latency_inf...
> ui/latency/latency_info.cc:220: trace_id_ = global_trace_id++;  // TODO.
> On 2017/06/08 18:36:44, tdresser wrote:
> > I'm not super happy with this naming. Any thoughts?
> 
> global trace id isn't ideal because this is actually limited to this process
id
> and they are IPC'd.

Sorry, this is currently blocked on https://codereview.chromium.org/2953073002/.

[tdresser, 2017-06-27 20:52:10]

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

[commit-bot: I haz the power, 2017-06-27 20:52:31]

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, 2017-06-27 21:47:56]

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

[commit-bot: I haz the power, 2017-06-27 21:47:57]

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

[tdresser, 2017-06-28 18:55:17]

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

[commit-bot: I haz the power, 2017-06-28 18:55:40]

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, 2017-06-28 19:26:07]

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

[commit-bot: I haz the power, 2017-06-28 19:26:08]

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_...)

[tdresser, 2017-06-29 19:02:42]

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

[commit-bot: I haz the power, 2017-06-29 19:03:08]

Dry run: CQ is trying da patch.

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

[tdresser, 2017-06-29 19:04:08]

tdresser@chromium.org changed reviewers:
+ ccameron@chromium.org
- dtapuska@chromium.org

[tdresser, 2017-06-29 19:04:09]

dtapuska@ is OOO.

ccameron@, could you give this a general review, and then I'll fill in OWNERs
gaps?

[tdresser, 2017-06-29 19:04:38]

https://codereview.chromium.org/2914023002/diff/100001/ui/latency/latency_inf...
File ui/latency/latency_info.cc (right):

https://codereview.chromium.org/2914023002/diff/100001/ui/latency/latency_inf...
ui/latency/latency_info.cc:217: static int global_trace_id = 0;
On 2017/06/09 15:15:35, dtapuska wrote:
> On 2017/06/08 18:36:44, tdresser wrote:
> > Real change.
> 
> This is kind of scary because latency_info.cc could be instantiated in
multiple
> processes. Should the global_trace_id be started off with some type of seed?

Dealt with in separate patch.

[commit-bot: I haz the power, 2017-06-29 19:21:43]

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

[commit-bot: I haz the power, 2017-06-29 19:21:44]

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, 2017-06-29 20:44:54]

On 2017/06/29 19:04:38, tdresser wrote:
>
https://codereview.chromium.org/2914023002/diff/100001/ui/latency/latency_inf...
> File ui/latency/latency_info.cc (right):
> 
>
https://codereview.chromium.org/2914023002/diff/100001/ui/latency/latency_inf...
> ui/latency/latency_info.cc:217: static int global_trace_id = 0;
> On 2017/06/09 15:15:35, dtapuska wrote:
> > On 2017/06/08 18:36:44, tdresser wrote:
> > > Real change.
> > 
> > This is kind of scary because latency_info.cc could be instantiated in
> multiple
> > processes. Should the global_trace_id be started off with some type of seed?
> 
> Dealt with in separate patch.

Bot failures are real, but due to folks landing additional code which passes
empty sequence_numbers.
I'll fix them when it's time to land.

ccameron@, PTAL.

[ccameron, 2017-06-29 20:52:18]

lgtm, it looks like most (99%ish) of these numbers weren't even being updated.

https://codereview.chromium.org/2914023002/diff/160001/content/browser/render...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/2914023002/diff/160001/content/browser/render...
content/browser/renderer_host/render_widget_host_impl.cc:1835:
&window_snapshot_component)) {
Looks reasonable (first not-just-delete change).

[tdresser, 2017-06-29 21:00:35]

tdresser@chromium.org changed reviewers:
+ kbr@chromium.org, kenrb@chromium.org

[tdresser, 2017-06-29 21:00:37]

Sorry Chris, I meant to highlight the non-trivial modifications. There's only
one other - I've noted it now.

+kenrb@ for SECURITY_OWNERS (IPC/mojo stuff, including
content/common/input/input_param_traits_unittest.cc)

+kbr for 
content/common/input/event_with_latency_info_unittest.cc
content/renderer/input/render_widget_input_handler.cc
gpu/ipc/service/pass_through_image_transport_surface.cc

https://codereview.chromium.org/2914023002/diff/160001/cc/trees/latency_info_...
File cc/trees/latency_info_swap_promise_monitor.cc (left):

https://codereview.chromium.org/2914023002/diff/160001/cc/trees/latency_info_...
cc/trees/latency_info_swap_promise_monitor.cc:76: int64_t new_sequence_number =
0;
Sorry, meant to highlight this as a slightly scary location.

[ccameron, 2017-06-29 21:06:36]

Thanks for the second pointer. I think this still lgtm. You may want to verify
based on the archeology, but because this is not read anywhere, I think it's
safe to remove the complicated write.

https://codereview.chromium.org/2914023002/diff/160001/cc/trees/latency_info_...
File cc/trees/latency_info_swap_promise_monitor.cc (left):

https://codereview.chromium.org/2914023002/diff/160001/cc/trees/latency_info_...
cc/trees/latency_info_swap_promise_monitor.cc:76: int64_t new_sequence_number =
0;
On 2017/06/29 21:00:37, tdresser wrote:
> Sorry, meant to highlight this as a slightly scary location.

Missed this one. Archeology gives some context at
https://codereview.chromium.org/365463003/diff2/20001:160001/cc/base/latency_...

But, if we aren't reading the sequence number anywhere, then I don't think it
matters.

[ccameron, 2017-06-29 21:06:36]

Thanks for the second pointer. I think this still lgtm. You may want to verify
based on the archeology, but because this is not read anywhere, I think it's
safe to remove the complicated write.

https://codereview.chromium.org/2914023002/diff/160001/cc/trees/latency_info_...
File cc/trees/latency_info_swap_promise_monitor.cc (left):

https://codereview.chromium.org/2914023002/diff/160001/cc/trees/latency_info_...
cc/trees/latency_info_swap_promise_monitor.cc:76: int64_t new_sequence_number =
0;
On 2017/06/29 21:00:37, tdresser wrote:
> Sorry, meant to highlight this as a slightly scary location.

Missed this one. Archeology gives some context at
https://codereview.chromium.org/365463003/diff2/20001:160001/cc/base/latency_...

But, if we aren't reading the sequence number anywhere, then I don't think it
matters.

[Ken Russell (switch to Gerrit), 2017-06-29 21:17:58]

not lgtm

The sequence number is supposed to be placed into the LatencyInfo by the
requesting process along with the screenshot request, here:

https://cs.chromium.org/chromium/src/content/browser/devtools/protocol/page_h...

and be piped all the way through the compositing pipeline. Once the frame
actually reaches the screen, the screenshot is taken.

jbauman@ implemented this a long time ago and it's been maintained through many
iterations of DevTools' and Telemetry's screenshot code. I would hope that this
change would break the screenshot_sync_tests that are run on
win_chromium_rel_ng, linux_chromium_rel_ng, and mac_chromium_rel_ng.

Rather than blindly removing a mechanism that was known to be needed, I'd like
to see evidence that this doesn't introduce flakes of the GPU tests.

[tdresser, 2017-06-30 15:36:06]

On 2017/06/29 21:17:58, Ken Russell (switch to Gerrit) wrote:
> not lgtm
> 
> The sequence number is supposed to be placed into the LatencyInfo by the
> requesting process along with the screenshot request, here:
> 
>
https://cs.chromium.org/chromium/src/content/browser/devtools/protocol/page_h...
> 
> and be piped all the way through the compositing pipeline. Once the frame
> actually reaches the screen, the screenshot is taken.
> 
> jbauman@ implemented this a long time ago and it's been maintained through
many
> iterations of DevTools' and Telemetry's screenshot code. I would hope that
this
> change would break the screenshot_sync_tests that are run on
> win_chromium_rel_ng, linux_chromium_rel_ng, and mac_chromium_rel_ng.
> 
> Rather than blindly removing a mechanism that was known to be needed, I'd like
> to see evidence that this doesn't introduce flakes of the GPU tests.

Thanks! No one on the input team, or vmiura@ or brianderson@ had any idea what
it this was for, and removing it seemed like the easiest way to figure out if it
was in fact necessary.
I've added you and jbauman@ to the doc here
https://docs.google.com/document/d/10Ytvqz3CSKzE7ENdHaBv_zPvNuX7spPTuCQ-aDSqi...,
where we're discussing the long term plan for LatencyInfo. Sorry for missing you
earlier!

If that's the only usage, we can still clean this up significantly - there's no
reason this should be per component as opposed to per LatencyInfo object is
there?

I'm not quite following the plumbing here - where does CaptureScreenshot
establish the sequence_number?

It sounds to me like there are three steps here:
1. Make the GPU tests fail instead of flake when this mechanism doesn't work.
2. Migrate this mechanism from using information on every component to just
using something on the LatencyInfo object.
3. Remove sequence_number_.

Does that sound reasonable?
Where are the tests for screenshot capturing?

[Ken Russell (switch to Gerrit), 2017-07-07 22:30:15]

On 2017/06/30 15:36:06, tdresser (OOO until July 17) wrote:
> On 2017/06/29 21:17:58, Ken Russell (switch to Gerrit) wrote:
> > not lgtm
> > 
> > The sequence number is supposed to be placed into the LatencyInfo by the
> > requesting process along with the screenshot request, here:
> > 
> >
>
https://cs.chromium.org/chromium/src/content/browser/devtools/protocol/page_h...
> > 
> > and be piped all the way through the compositing pipeline. Once the frame
> > actually reaches the screen, the screenshot is taken.
> > 
> > jbauman@ implemented this a long time ago and it's been maintained through
> many
> > iterations of DevTools' and Telemetry's screenshot code. I would hope that
> this
> > change would break the screenshot_sync_tests that are run on
> > win_chromium_rel_ng, linux_chromium_rel_ng, and mac_chromium_rel_ng.
> > 
> > Rather than blindly removing a mechanism that was known to be needed, I'd
like
> > to see evidence that this doesn't introduce flakes of the GPU tests.
> 
> Thanks! No one on the input team, or vmiura@ or brianderson@ had any idea what
> it this was for, and removing it seemed like the easiest way to figure out if
it
> was in fact necessary.
> I've added you and jbauman@ to the doc here
>
https://docs.google.com/document/d/10Ytvqz3CSKzE7ENdHaBv_zPvNuX7spPTuCQ-aDSqi...,
> where we're discussing the long term plan for LatencyInfo. Sorry for missing
you
> earlier!

Thanks for adding us. Sorry for not replying earlier.

> If that's the only usage, we can still clean this up significantly - there's
no
> reason this should be per component as opposed to per LatencyInfo object is
> there?

I'm not sure at this point -- jbauman@ did all of this work and I'll need to
study LatencyInfo to come up to speed on it.

> I'm not quite following the plumbing here - where does CaptureScreenshot
> establish the sequence_number?

It's supposed to be established at the point the screenshot request is made in
the renderer process via DevTools. From there it is supposed to traverse all the
way through the compositing pipeline and only take the snapshot once the
rendering results have reached the screen.

> It sounds to me like there are three steps here:
> 1. Make the GPU tests fail instead of flake when this mechanism doesn't work.

This is difficult. We have tried to add stress tests to guarantee that the
mechanism is working. screenshot_sync_tests, run as part of win_chromium_rel_ng,
linux_chromium_rel_ng, and mac_chromium_rel_ng, is probably the best test to try
to catch regressions in this area. Many of the other GPU tests use the
screenshot mechanism (the pixel tests, the Maps test, and others), but they're
more likely to become flaky rather than be guaranteed to fail.

> 2. Migrate this mechanism from using information on every component to just
> using something on the LatencyInfo object.

It's possible this will work; I don't know the code well enough right now to
confirm.

> 3. Remove sequence_number_.
> 
> Does that sound reasonable?

I can't say definitively yet. Can study the code and confirm.

> Where are the tests for screenshot capturing?

As mentioned above, the screenshot_sync tests in src/content/test/gpu/gpu_tests.
Run via:
src/content/test/gpu/run_gpu_integration_test.py screenshot_sync
--browser=release

More documentation's on https://www.chromium.org/developers/testing/gpu-testing
.

[jbauman, 2017-07-07 22:46:07]

On 2017/06/30 15:36:06, tdresser (OOO until July 17) wrote:
> On 2017/06/29 21:17:58, Ken Russell (switch to Gerrit) wrote:
> > not lgtm
> > 
> > The sequence number is supposed to be placed into the LatencyInfo by the
> > requesting process along with the screenshot request, here:
> > 
> >
>
https://cs.chromium.org/chromium/src/content/browser/devtools/protocol/page_h...
> > 
> > and be piped all the way through the compositing pipeline. Once the frame
> > actually reaches the screen, the screenshot is taken.
> > 
> > jbauman@ implemented this a long time ago and it's been maintained through
> many
> > iterations of DevTools' and Telemetry's screenshot code. I would hope that
> this
> > change would break the screenshot_sync_tests that are run on
> > win_chromium_rel_ng, linux_chromium_rel_ng, and mac_chromium_rel_ng.
> > 
> > Rather than blindly removing a mechanism that was known to be needed, I'd
like
> > to see evidence that this doesn't introduce flakes of the GPU tests.
> 
> Thanks! No one on the input team, or vmiura@ or brianderson@ had any idea what
> it this was for, and removing it seemed like the easiest way to figure out if
it
> was in fact necessary.
> I've added you and jbauman@ to the doc here
>
https://docs.google.com/document/d/10Ytvqz3CSKzE7ENdHaBv_zPvNuX7spPTuCQ-aDSqi...,
> where we're discussing the long term plan for LatencyInfo. Sorry for missing
you
> earlier!
> 
> If that's the only usage, we can still clean this up significantly - there's
no
> reason this should be per component as opposed to per LatencyInfo object is
> there?
> 
> I'm not quite following the plumbing here - where does CaptureScreenshot
> establish the sequence_number?
> 
> It sounds to me like there are three steps here:
> 1. Make the GPU tests fail instead of flake when this mechanism doesn't work.

I think this code as modified will break if you try to capture screenshots of
multiple windows at the same time. I guess do 1 capture on renderer A, 1 on
renderer B, then 1 on renderer A. The second capture on renderer A could be
triggered before the contents hit the screen because snapshot_id would be too
large. This would be very difficult to fail consistently, because it would rely
on a race condition to fail.

I guess the easiest way to force a failure in that case would be to DCHECK in
RenderWidgetHostImpl::OnSnapshotReceived that snapshot_id <= the highest
snapshot id created.

> 2. Migrate this mechanism from using information on every component to just
> using something on the LatencyInfo object.
> 3. Remove sequence_number_.

Why do we need to remove this? We'd essentially be adding a sequence_number_ to
the LatencyInfo instead of to the specific component where it matters.
> 
> Does that sound reasonable?
> Where are the tests for screenshot capturing?

[tdresser, 2017-07-20 15:04:44]

Sorry for the delay, I was OOO.

> Why do we need to remove this? We'd essentially be adding a sequence_number_
to
> the LatencyInfo instead of to the specific component where it matters.

The problem is that it currently isn't added to the specific component where it
matters, it's added to EVERY component.
https://cs.chromium.org/chromium/src/ui/latency/latency_info.h?rcl=edd34538e6....

Well, the field is present in every component, the actual value is populated
inconsistently, and it's pretty unclear 
why it exists, and when it needs to be present.

We're trying to shrink the size of each component down to a single timestamp, as
part of a general cleanup of LatencyInfo.
Part of that is removing the sequence_number from being in every component.

If we were to add it to LatencyInfo, we'd also name it something specific to how
it's used, instead of something generic.

I'll set up a VC to talk this over. Thanks for your help here!