Use different source types for each ConnectJob subclass.

Use different source types for each ConnectJob subclass.

This makes it easier to distinguish the different CONNECT_JOB entries
that get spawned in the more complex cases. Notably, SSL produces two of
them.

BUG=669700
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/b7048f096092bcb293216921c35df413258dbd57
Cr-Commit-Position: refs/heads/master@{#435419}

[davidben, 2016-11-29 23:52:50]

Description was changed from

==========
Use different source types for each ConnectJob subclass.

This makes it easier to distinguish the different CONNECT_JOB entries
that get spawned in the more complex cases. Notably, SSL produces two of
them.

BUG=669700
==========

to

==========
Use different source types for each ConnectJob subclass.

This makes it easier to distinguish the different CONNECT_JOB entries
that get spawned in the more complex cases. Notably, SSL produces two of
them.

BUG=669700
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[davidben, 2016-11-29 23:52:52]

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

[commit-bot: I haz the power, 2016-11-29 23:53:59]

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

[davidben, 2016-11-29 23:54:24]

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

[commit-bot: I haz the power, 2016-11-29 23:54:54]

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

[davidben, 2016-11-30 00:03:54]

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

[davidben, 2016-11-30 00:04:16]

davidben@chromium.org changed reviewers:
+ mmenke@chromium.org

[commit-bot: I haz the power, 2016-11-30 00:05:09]

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-11-30 02:08:25]

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

[commit-bot: I haz the power, 2016-11-30 02:08:26]

Dry run: Try jobs failed on following builders:
  blimp_linux_dbg on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  closure_compilation on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)

[mmenke, 2016-11-30 15:09:24]

https://codereview.chromium.org/2537373002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/net_internals/source_entry.js (right):

https://codereview.chromium.org/2537373002/diff/40001/chrome/browser/resource...
chrome/browser/resources/net_internals/source_entry.js:88: case
EventSourceType.CONNECT_JOB:
This works, even if EventSourceType.CONNECT_JOB is undefined (And even when
everything but EventSourceType.CONNECT_JOB is undefined)?

https://codereview.chromium.org/2537373002/diff/40001/net/cert/multi_log_ct_v...
File net/cert/multi_log_ct_verifier_unittest.cc (right):

https://codereview.chromium.org/2537373002/diff/40001/net/cert/multi_log_ct_v...
net/cert/multi_log_ct_verifier_unittest.cc:126: &test_net_log,
NetLogSourceType::SSL_CONNECT_JOB);
Do these actually matter, or could you just use NetLotWithSource() inline,
below?

[davidben, 2016-11-30 17:37:46]

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

[davidben, 2016-11-30 17:37:49]

https://codereview.chromium.org/2537373002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/net_internals/source_entry.js (right):

https://codereview.chromium.org/2537373002/diff/40001/chrome/browser/resource...
chrome/browser/resources/net_internals/source_entry.js:88: case
EventSourceType.CONNECT_JOB:
On 2016/11/30 15:09:24, mmenke wrote:
> This works, even if EventSourceType.CONNECT_JOB is undefined (And even when
> everything but EventSourceType.CONNECT_JOB is undefined)?

Apparently. EventSourceType.SOCKET_STREAM is also undefined. JavaScript just
spits out the value |undefined| and I guess e.source.type is not that value. For
better or worse, we seem to rely on this everywhere. :-/

(I would probably suggest that, if we cared, we post-process EventSourceType and
friends to set all unknown keys to -1 or something.)

https://codereview.chromium.org/2537373002/diff/40001/net/cert/multi_log_ct_v...
File net/cert/multi_log_ct_verifier_unittest.cc (right):

https://codereview.chromium.org/2537373002/diff/40001/net/cert/multi_log_ct_v...
net/cert/multi_log_ct_verifier_unittest.cc:126: &test_net_log,
NetLogSourceType::SSL_CONNECT_JOB);
On 2016/11/30 15:09:24, mmenke wrote:
> Do these actually matter, or could you just use NetLotWithSource() inline,
> below?

This one doesn't seem to, but the one below checks test_net_log.

[commit-bot: I haz the power, 2016-11-30 17:38:26]

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-11-30 19:57:33]

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

[commit-bot: I haz the power, 2016-11-30 19:57:34]

Dry run: This issue passed the CQ dry run.

[mmenke, 2016-11-30 20:51:59]

https://codereview.chromium.org/2537373002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/net_internals/source_entry.js (right):

https://codereview.chromium.org/2537373002/diff/40001/chrome/browser/resource...
chrome/browser/resources/net_internals/source_entry.js:88: case
EventSourceType.CONNECT_JOB:
On 2016/11/30 17:37:49, davidben (on triage til 11-30) wrote:
> On 2016/11/30 15:09:24, mmenke wrote:
> > This works, even if EventSourceType.CONNECT_JOB is undefined (And even when
> > everything but EventSourceType.CONNECT_JOB is undefined)?
> 
> Apparently. EventSourceType.SOCKET_STREAM is also undefined. JavaScript just
> spits out the value |undefined| and I guess e.source.type is not that value.
For
> better or worse, we seem to rely on this everywhere. :-/
> 
> (I would probably suggest that, if we cared, we post-process EventSourceType
and
> friends to set all unknown keys to -1 or something.)

But....then we'd have multiple "case -1:", instead of multiple "case unknowns:".
 Just to verify, you have checked this works with old logs?  Multiple cases with
the same value (even if it's unknown) seems even weirder than one case with an
unknown.

[davidben, 2016-11-30 20:58:38]

https://codereview.chromium.org/2537373002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/net_internals/source_entry.js (right):

https://codereview.chromium.org/2537373002/diff/40001/chrome/browser/resource...
chrome/browser/resources/net_internals/source_entry.js:88: case
EventSourceType.CONNECT_JOB:
On 2016/11/30 20:51:59, mmenke wrote:
> On 2016/11/30 17:37:49, davidben (on triage til 11-30) wrote:
> > On 2016/11/30 15:09:24, mmenke wrote:
> > > This works, even if EventSourceType.CONNECT_JOB is undefined (And even
when
> > > everything but EventSourceType.CONNECT_JOB is undefined)?
> > 
> > Apparently. EventSourceType.SOCKET_STREAM is also undefined. JavaScript just
> > spits out the value |undefined| and I guess e.source.type is not that value.
> For
> > better or worse, we seem to rely on this everywhere. :-/
> > 
> > (I would probably suggest that, if we cared, we post-process EventSourceType
> and
> > friends to set all unknown keys to -1 or something.)
> 
> But....then we'd have multiple "case -1:", instead of multiple "case
unknowns:".
>  Just to verify, you have checked this works with old logs?  Multiple cases
with
> the same value (even if it's unknown) seems even weirder than one case with an
> unknown.

Yup, works fine. JavaScript is fine with duplicate switch-cases and picks the
first match.

[mmenke, 2016-11-30 21:13:44]

Thanks, LGTM!

[davidben, 2016-11-30 21:15:58]

The CQ bit was checked by davidben@chromium.org

[commit-bot: I haz the power, 2016-11-30 21:16:35]

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-11-30 21:20:29]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1480540558779380,
"parent_rev": "db8fd526fc38b41a45f08e2ac493992e8fc16559", "commit_rev":
"5d58307b31bed0b705a3a8d18154bd93daa1c433"}

[commit-bot: I haz the power, 2016-11-30 21:20:56]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-11-30 21:24:51]

Description was changed from

==========
Use different source types for each ConnectJob subclass.

This makes it easier to distinguish the different CONNECT_JOB entries
that get spawned in the more complex cases. Notably, SSL produces two of
them.

BUG=669700
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Use different source types for each ConnectJob subclass.

This makes it easier to distinguish the different CONNECT_JOB entries
that get spawned in the more complex cases. Notably, SSL produces two of
them.

BUG=669700
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

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

[commit-bot: I haz the power, 2016-11-30 21:24:52]

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