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

Issue 962443002: Revert of Telemetry: Refactor inspector_websocket to create cleaner interface. (Closed)

Created:
5 years, 10 months ago by nednguyen
Modified:
5 years, 10 months ago
Reviewers:
sullivan, erikchen, slamm
CC:
chromium-reviews, telemetry-reviews_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Revert of Telemetry: Refactor inspector_websocket to create cleaner interface. (patchset #2 id:80001 of https://codereview.chromium.org/942113004/) Reason for revert: This possibly is causing chrome tracing data leakage: BUG=461874 Original issue's description: > Telemetry: Refactor inspector_websocket to create cleaner interface. > > This CL contains several improvements which are tightly coupled. > - The method DispatchNotificationsUntilDone is only used by tracing_backend. I > moved all the logic into tracing_backend. This has several side effects: > - The exception DispatchNotificationsUntilDone is no longer needed. > - The method _Receive no longer handles exceptions. It used to handle some, > but not all exceptions. This change is very important for sane exception > handling of the Chrome/Telemetry bindings. > - The method _HandleNotification never has a return value. It used to > sometimes return a boolean, and sometimes return nothing. > - I removed the member _error_handler. This member was only used in two > places: inspector_backend and tracing_backend. Of these two places, > tracing_backend only used it to emit a not-so-useful log statement. > - The biggest problem with _error_handler is that it decouples the exception > handling from the context of the try/catch loop that triggered the > exception. > - The _error_handler only was called during _Receive(). This is very > confusing, since that is not the only method that raises Exceptions. So some > exceptions get handled by _error_handler, and some do not. :( > - I removed all exception handling from inspector_page.py and > inspector_websocket.py. It used to be that some exceptions would be handled in > inspector_websocket, some would trickle up to inspector_page, and some would > trickle out past inspector_backend.py. The eventual goal is for > inspector_backend to catch all exceptions and translate them into > {Recoverable/Unrecoverable}Exceptions. > > BUG=460625 > > Committed: https://crrev.com/64cf2ab4a6a348954d46d9d758c5be53aa1c1220 > Cr-Commit-Position: refs/heads/master@{#317972} TBR=sullivan@chromium.org,slamm@chromium.org,erikchen@chromium.org BUG=460625 Committed: https://crrev.com/179e300754aeda5ffeae3ed165324b2913487dd5 Cr-Commit-Position: refs/heads/master@{#318205}

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+205 lines, -225 lines) Patch
M tools/telemetry/telemetry/core/backends/chrome_inspector/inspector_backend.py View 10 chunks +22 lines, -63 lines 0 comments Download
M tools/telemetry/telemetry/core/backends/chrome_inspector/inspector_page.py View 2 chunks +9 lines, -9 lines 0 comments Download
M tools/telemetry/telemetry/core/backends/chrome_inspector/inspector_websocket.py View 3 chunks +78 lines, -15 lines 0 comments Download
M tools/telemetry/telemetry/core/backends/chrome_inspector/inspector_websocket_unittest.py View 5 chunks +81 lines, -4 lines 0 comments Download
M tools/telemetry/telemetry/core/backends/chrome_inspector/tracing_backend.py View 5 chunks +15 lines, -43 lines 0 comments Download
M tools/telemetry/telemetry/core/backends/chrome_inspector/tracing_backend_unittest.py View 2 chunks +0 lines, -91 lines 0 comments Download

Messages

Total messages: 7 (2 generated)
nednguyen
Created Revert of Telemetry: Refactor inspector_websocket to create cleaner interface.
5 years, 10 months ago (2015-02-26 02:13:58 UTC) #1
erikchen
lgtm
5 years, 10 months ago (2015-02-26 02:15:36 UTC) #4
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/962443002/1
5 years, 10 months ago (2015-02-26 02:15:55 UTC) #5
commit-bot: I haz the power
Committed patchset #1 (id:1)
5 years, 10 months ago (2015-02-26 07:17:31 UTC) #6
commit-bot: I haz the power
5 years, 10 months ago (2015-02-26 07:18:03 UTC) #7
Message was sent while issue was closed.
Patchset 1 (id:??) landed as
https://crrev.com/179e300754aeda5ffeae3ed165324b2913487dd5
Cr-Commit-Position: refs/heads/master@{#318205}

Powered by Google App Engine
This is Rietveld 408576698