webrtc-internals: highlight failures

webrtc-internals: highlight failures

Highlights both ICE failure (iceconnectionstatechange -> failed)
and any error callbacks (indicated by ...OnFailure) so that they
are more visible.

BUG=678292
Review-Url: https://codereview.chromium.org/2621493002
Cr-Commit-Position: refs/heads/master@{#443523}
Committed: https://chromium.googlesource.com/chromium/src/+/90c70c7ac3a6f823ec083829fad97016ef4d24b6

[fippo, 2017-01-08 15:57:00]

Description was changed from

==========
webrtc-internals: highlight failures

Highlights both ICE failure (iceconnectionstatechange -> failed)
and any error callbacks (indicated by ...OnFailure) so that they
are more visible.

BUG=678292
==========

to

==========
webrtc-internals: highlight failures

Highlights both ICE failure (iceconnectionstatechange -> failed)
and any error callbacks (indicated by ...OnFailure) so that they
are more visible.

BUG=678292
==========

[fippo, 2017-01-08 15:57:01]

philipp.hancke@googlemail.com changed reviewers:
+ hta@chromium.org, tommi@chromium.org

[fippo, 2017-01-08 15:57:59]

if I recall correctly that red was picked from the WebRTC logo. It looks pretty
ugly but the page lacks proper styling anyway ;-)

[tommi (sloooow) - chröme, 2017-01-08 16:48:05]

good idea. lgtm

https://codereview.chromium.org/2621493002/diff/1/content/browser/resources/m...
File content/browser/resources/media/peer_connection_update_table.js (right):

https://codereview.chromium.org/2621493002/diff/1/content/browser/resources/m...
content/browser/resources/media/peer_connection_update_table.js:111:
update.value === 'ICEConnectionStateFailed') ||
nit: indent one more space to group with the above condition

[fippo, 2017-01-08 17:10:02]

https://codereview.chromium.org/2621493002/diff/1/content/browser/resources/m...
File content/browser/resources/media/peer_connection_update_table.js (right):

https://codereview.chromium.org/2621493002/diff/1/content/browser/resources/m...
content/browser/resources/media/peer_connection_update_table.js:111:
update.value === 'ICEConnectionStateFailed') ||
On 2017/01/08 16:48:05, tommi (chrömium) wrote:
> nit: indent one more space to group with the above condition

Done.

[fippo, 2017-01-11 18:55:16]

Hrmpf. The failure callback for addIceCandidate is addIceCandidateFailed, not
addIceCandidateOnFailure.
Should I add that case the highlight code or change the name to
addIceCandidateOnFailure?

[hta - Chromium, 2017-01-12 22:24:58]

On 2017/01/11 18:55:16, fippo wrote:
> Hrmpf. The failure callback for addIceCandidate is addIceCandidateFailed, not
> addIceCandidateOnFailure.
> Should I add that case the highlight code or change the name to
> addIceCandidateOnFailure?

Yes. This is code that works with what is, not what should be.

[hta - Chromium, 2017-01-12 22:26:59]

lgtm

On 2017/01/12 22:24:58, hta - Chromium wrote:
> On 2017/01/11 18:55:16, fippo wrote:
> > Hrmpf. The failure callback for addIceCandidate is addIceCandidateFailed,
not
> > addIceCandidateOnFailure.
> > Should I add that case the highlight code or change the name to
> > addIceCandidateOnFailure?
> 
> Yes. This is code that works with what is, not what should be.

Hrmf. Neither string is in the code now. Is this a matter for a future cl?
Anyway, for this CL as it stands: lgtm

[fippo, 2017-01-13 07:19:58]

On 2017/01/12 22:26:59, hta - Chromium wrote:
> lgtm
> 
> On 2017/01/12 22:24:58, hta - Chromium wrote:
> > On 2017/01/11 18:55:16, fippo wrote:
> > > Hrmpf. The failure callback for addIceCandidate is addIceCandidateFailed,
> not
> > > addIceCandidateOnFailure.
> > > Should I add that case the highlight code or change the name to
> > > addIceCandidateOnFailure?
> > 
> > Yes. This is code that works with what is, not what should be.
> 
> Hrmf. Neither string is in the code now. Is this a matter for a future cl?
> Anyway, for this CL as it stands: lgtm

That string lives in
https://codesearch.chromium.org/chromium/src/content/renderer/media/peer_conn...

[tommi (sloooow) - chröme, 2017-01-13 09:47:19]

The CQ bit was checked by tommi@chromium.org

[tommi (sloooow) - chröme, 2017-01-13 09:47:19]

The patchset sent to the CQ was uploaded after l-g-t-m from tommi@chromium.org,
hta@chromium.org
Link to the patchset: https://codereview.chromium.org/2621493002/#ps40001
(title: "highlight addicecandidatefailed")

[commit-bot: I haz the power, 2017-01-13 09:47:44]

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-01-13 09:58:12]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1484300839557430,
"parent_rev": "2b57920cbb778f03295ae1c7e0cee380667b6d5a", "commit_rev":
"90c70c7ac3a6f823ec083829fad97016ef4d24b6"}

[commit-bot: I haz the power, 2017-01-13 09:58:45]

Description was changed from

==========
webrtc-internals: highlight failures

Highlights both ICE failure (iceconnectionstatechange -> failed)
and any error callbacks (indicated by ...OnFailure) so that they
are more visible.

BUG=678292
==========

to

==========
webrtc-internals: highlight failures

Highlights both ICE failure (iceconnectionstatechange -> failed)
and any error callbacks (indicated by ...OnFailure) so that they
are more visible.

BUG=678292

Review-Url: https://codereview.chromium.org/2621493002
Cr-Commit-Position: refs/heads/master@{#443523}
Committed:
https://chromium.googlesource.com/chromium/src/+/90c70c7ac3a6f823ec083829fad9...
==========

[commit-bot: I haz the power, 2017-01-13 09:58:48]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/90c70c7ac3a6f823ec083829fad9...