Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(624)

Issue 1410563006: [Chromoting] SessionLogger refactor. (Closed)

Created:
5 years, 1 month ago by kelvinp
Modified:
5 years, 1 month ago
Reviewers:
Jamie, garykac
CC:
chromium-reviews, chromoting-reviews_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

[Chromoting] SessionLogger refactor. Motivation: When a remoting connection fails, we often need to collect extra debugging information in additional to the error tag. For example, when the host goes offline, we would like to report the corresponding XMPP error stanza as well. In the current design, we pass the XMPP error stanza to the SessionLogger as a separate argument in logSessionStateChange(). In this pattern, we need to add a new argument to logSessionStateChange() for each error specific detail. As we collect more and more extra info (e.g. PNaCl error, script error callstack), this pattern quickly becomes unmanageable. Summary of changes: 1. When the connection fails, the caller set the |detail| field in the remoting.Error object. When logSessionStateChange() is called, SessionLogger calls error.fillLogEntry() to translate the |detail| field into the corresponding fields in the log entry. 2. Consolidates logClientSessionStateChange() and logSessionStateChange() 3. Remove the dependency of remoting.ClientSession.State from SessionLogger.js 4. Report bad version as a connection failure instead of user cancellation BUG=552693 Committed: https://crrev.com/bca7ea20b1b8d03163427ff3dd97df02bd4922e5 Cr-Commit-Position: refs/heads/master@{#358726}

Patch Set 1 : #

Total comments: 12

Patch Set 2 : Reviewer's feedback #

Patch Set 3 : Rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+140 lines, -194 lines) Patch
M remoting/webapp/base/js/chromoting_event.js View 1 1 chunk +23 lines, -0 lines 0 comments Download
M remoting/webapp/base/js/client_session.js View 1 3 chunks +42 lines, -12 lines 0 comments Download
M remoting/webapp/base/js/client_session_factory.js View 3 chunks +4 lines, -32 lines 0 comments Download
M remoting/webapp/base/js/client_session_unittest.js View 3 chunks +6 lines, -6 lines 0 comments Download
M remoting/webapp/base/js/session_logger.js View 1 6 chunks +16 lines, -74 lines 0 comments Download
M remoting/webapp/base/js/session_logger_unittest.js View 8 chunks +14 lines, -16 lines 0 comments Download
M remoting/webapp/base/js/telemetry_event_writer_unittest.js View 4 chunks +11 lines, -12 lines 0 comments Download
M remoting/webapp/base/js/xmpp_error_cache.js View 3 chunks +8 lines, -9 lines 0 comments Download
M remoting/webapp/base/js/xmpp_error_cache_unittest.js View 1 chunk +2 lines, -2 lines 0 comments Download
M remoting/webapp/crd/js/desktop_remoting_activity.js View 1 chunk +1 line, -2 lines 0 comments Download
M remoting/webapp/crd/js/host_screen.js View 4 chunks +5 lines, -15 lines 0 comments Download
M remoting/webapp/crd/js/it2me_activity.js View 1 chunk +1 line, -2 lines 0 comments Download
M remoting/webapp/crd/js/me2me_activity.js View 3 chunks +7 lines, -12 lines 0 comments Download

Dependent Patchsets:

Messages

Total messages: 16 (8 generated)
kelvinp
PTAL https://codereview.chromium.org/1410563006/diff/20001/remoting/webapp/base/js/error.js File remoting/webapp/base/js/error.js (right): https://codereview.chromium.org/1410563006/diff/20001/remoting/webapp/base/js/error.js#newcode115 remoting/webapp/base/js/error.js:115: var detail = /** @type{string} */ (this.getDetail()); This ...
5 years, 1 month ago (2015-11-07 04:44:11 UTC) #7
Jamie
https://codereview.chromium.org/1410563006/diff/20001/remoting/webapp/base/js/client_session.js File remoting/webapp/base/js/client_session.js (right): https://codereview.chromium.org/1410563006/diff/20001/remoting/webapp/base/js/client_session.js#newcode567 remoting/webapp/base/js/client_session.js:567: this.logger_.logSessionStateChange(toSessionState(this.state_), this.error_); This is a change in semantics. Previously, ...
5 years, 1 month ago (2015-11-09 18:46:03 UTC) #8
kelvinp
PTAL https://codereview.chromium.org/1410563006/diff/20001/remoting/webapp/base/js/client_session.js File remoting/webapp/base/js/client_session.js (right): https://codereview.chromium.org/1410563006/diff/20001/remoting/webapp/base/js/client_session.js#newcode567 remoting/webapp/base/js/client_session.js:567: this.logger_.logSessionStateChange(toSessionState(this.state_), this.error_); On 2015/11/09 18:46:02, Jamie wrote: > ...
5 years, 1 month ago (2015-11-09 22:59:10 UTC) #9
Jamie
LGTM as long as my remaining comment is correct. https://codereview.chromium.org/1410563006/diff/20001/remoting/webapp/base/js/client_session_factory.js File remoting/webapp/base/js/client_session_factory.js (left): https://codereview.chromium.org/1410563006/diff/20001/remoting/webapp/base/js/client_session_factory.js#oldcode146 remoting/webapp/base/js/client_session_factory.js:146: ...
5 years, 1 month ago (2015-11-09 23:23:09 UTC) #10
kelvinp
FYI https://codereview.chromium.org/1410563006/diff/20001/remoting/webapp/base/js/client_session_factory.js File remoting/webapp/base/js/client_session_factory.js (left): https://codereview.chromium.org/1410563006/diff/20001/remoting/webapp/base/js/client_session_factory.js#oldcode146 remoting/webapp/base/js/client_session_factory.js:146: logger.setPluginError(pluginError); On 2015/11/09 23:23:09, Jamie wrote: > On ...
5 years, 1 month ago (2015-11-10 00:02:45 UTC) #13
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1410563006/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1410563006/60001
5 years, 1 month ago (2015-11-10 00:05:32 UTC) #14
commit-bot: I haz the power
Committed patchset #3 (id:60001)
5 years, 1 month ago (2015-11-10 01:17:48 UTC) #15
commit-bot: I haz the power
5 years, 1 month ago (2015-11-10 01:18:57 UTC) #16
Message was sent while issue was closed.
Patchset 3 (id:??) landed as
https://crrev.com/bca7ea20b1b8d03163427ff3dd97df02bd4922e5
Cr-Commit-Position: refs/heads/master@{#358726}

Powered by Google App Engine
This is Rietveld 408576698