Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
942113004:
Telemetry: Refactor inspector_websocket to create cleaner interface. (Closed)
DescriptionTelemetry: 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}
Patch Set 1 : #
Total comments: 33
Patch Set 2 : Comments from slamm. #
Messages
Total messages: 22 (8 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
erikchen@chromium.org changed reviewers: + nednguyen@google.com
nednguyen: Please review. Let me know if you have any high level questions. https://codereview.chromium.org/942113004/diff/60001/tools/telemetry/telemetr... File tools/telemetry/telemetry/core/backends/chrome_inspector/inspector_backend.py (right): https://codereview.chromium.org/942113004/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome_inspector/inspector_backend.py:131: except (socket.error, websocket.WebSocketException) as e: This is a lot of duplicate code. All I'm doing is reproducing the existing behavior in this CL. In a future CL, some of the try/except loops are going to have different logic. (Notably WaitForNavigate(), Navigate() - I haven't looked too deeply into the other callsites yet). https://codereview.chromium.org/942113004/diff/60001/tools/telemetry/telemetr... File tools/telemetry/telemetry/core/backends/chrome_inspector/inspector_page.py (left): https://codereview.chromium.org/942113004/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome_inspector/inspector_page.py:92: except util.TimeoutException: This exception is never raised.
Is it possible to split up this patch into smaller ones for the ease of review & revert if anything goes wrong? i.e: Removal of DispatchNotificationsUntilDone in 1 patch. Removal of _error_handler in another patch. Removal of all exception handling from inspector_page.py and inspector_websocket.py in the last one.
nednguyen@google.com changed reviewers: + sullivan@chromium.org
On 2015/02/24 04:52:54, nednguyen wrote: > Is it possible to split up this patch into smaller ones for the ease of review & > revert if anything goes wrong? > > i.e: > Removal of DispatchNotificationsUntilDone in 1 patch. > Removal of _error_handler in another patch. > Removal of all exception handling from inspector_page.py and > inspector_websocket.py in the last one. nednguyen: PTAL As per out-of-band discussion, it is possible to split up the patch, but it involves adding hacky intermediate state. As I mentioned in the CL description, the code I'm changing is tightly coupled.
Except this comment, the rest looks solid to me. https://codereview.chromium.org/942113004/diff/60001/tools/telemetry/telemetr... File tools/telemetry/telemetry/core/backends/chrome_inspector/inspector_backend.py (right): https://codereview.chromium.org/942113004/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome_inspector/inspector_backend.py:131: except (socket.error, websocket.WebSocketException) as e: On 2015/02/23 23:01:49, erikchen wrote: > This is a lot of duplicate code. All I'm doing is reproducing the existing > behavior in this CL. > > In a future CL, some of the try/except loops are going to have different logic. > (Notably WaitForNavigate(), Navigate() - I haven't looked too deeply into the > other callsites yet). Given the amount of boiler-plate code, I am not sure whether removing the _HandleError param make it better.
https://codereview.chromium.org/942113004/diff/60001/tools/telemetry/telemetr... File tools/telemetry/telemetry/core/backends/chrome_inspector/inspector_backend.py (right): https://codereview.chromium.org/942113004/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome_inspector/inspector_backend.py:131: except (socket.error, websocket.WebSocketException) as e: On 2015/02/24 19:50:46, nednguyen wrote: > On 2015/02/23 23:01:49, erikchen wrote: > > This is a lot of duplicate code. All I'm doing is reproducing the existing > > behavior in this CL. > > > > In a future CL, some of the try/except loops are going to have different > logic. > > (Notably WaitForNavigate(), Navigate() - I haven't looked too deeply into the > > other callsites yet). > > Given the amount of boiler-plate code, I am not sure whether removing the > _HandleError param make it better. As I mentioned in the CL description, the problem with passing _HandleError as a param is that it loses the context of the error. If you look at the implementation of _HandleError, you'll see that its error messages actually make assumptions about the context. "assuming it timed out" "assuming it crashed". The assumptions are necessary because it doesn't have sufficient context. The error messages themselves are vague, and not very helpful. Furthermore, _HandleError is only called for some websocket exceptions, not others. (In fact, it's only called for exceptions in _Receive). This is confusing, since callers need to know which exceptions are handled by _HandleError, and which are not. We could extend _HandleError be called for all websocket exceptions, but once again, we get more loss of context. An exception while trying to connect a websocket is very different from a timeout while waiting for a response from a javascript evaluation. Finally, while the previous implementations for many of these methods was short, the actual behavior and side effects were unclear. e.g. It is unclear what exceptions GetCookieByName will throw. It is unclear that an exception in GetCookieByName will actually call back into _HandleError, which converts the exception into a different exception, and then reraises the new exception.
nednguyen@google.com changed reviewers: + slamm@chromium.org
lgtm slamm@ can you take another pass as this? https://codereview.chromium.org/942113004/diff/60001/tools/telemetry/telemetr... File tools/telemetry/telemetry/core/backends/chrome_inspector/inspector_backend.py (right): https://codereview.chromium.org/942113004/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome_inspector/inspector_backend.py:131: except (socket.error, websocket.WebSocketException) as e: On 2015/02/24 21:08:53, erikchen wrote: > On 2015/02/24 19:50:46, nednguyen wrote: > > On 2015/02/23 23:01:49, erikchen wrote: > > > This is a lot of duplicate code. All I'm doing is reproducing the existing > > > behavior in this CL. > > > > > > In a future CL, some of the try/except loops are going to have different > > logic. > > > (Notably WaitForNavigate(), Navigate() - I haven't looked too deeply into > the > > > other callsites yet). > > > > Given the amount of boiler-plate code, I am not sure whether removing the > > _HandleError param make it better. > > As I mentioned in the CL description, the problem with passing _HandleError as a > param is that it loses the context of the error. > > If you look at the implementation of _HandleError, you'll see that its error > messages actually make assumptions about the context. "assuming it timed out" > "assuming it crashed". The assumptions are necessary because it doesn't have > sufficient context. The error messages themselves are vague, and not very > helpful. > > Furthermore, _HandleError is only called for some websocket exceptions, not > others. (In fact, it's only called for exceptions in _Receive). This is > confusing, since callers need to know which exceptions are handled by > _HandleError, and which are not. We could extend _HandleError be called for all > websocket exceptions, but once again, we get more loss of context. An exception > while trying to connect a websocket is very different from a timeout while > waiting for a response from a javascript evaluation. > > Finally, while the previous implementations for many of these methods was short, > the actual behavior and side effects were unclear. e.g. It is unclear what > exceptions GetCookieByName will throw. It is unclear that an exception in > GetCookieByName will actually call back into _HandleError, which converts the > exception into a different exception, and then reraises the new exception. Good arguments. Given this is also nested deep inside telemetry/core and not something public users have to deal with, I like this.
This is a nice cleanup. I have found this code confusing in the past. https://codereview.chromium.org/942113004/diff/60001/tools/telemetry/telemetr... File tools/telemetry/telemetry/core/backends/chrome_inspector/inspector_backend.py (right): https://codereview.chromium.org/942113004/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome_inspector/inspector_backend.py:96: def Screenshot(self, timeout): FWIW, I do not see Screenshot getting used. https://code.google.com/p/chromium/codesearch#search/&q=%5C.Screenshot%20file... Thought jeremy@ did recently file a bug saying that we should capture the screen on failures: https://code.google.com/p/chromium/issues/detail?id=454155 https://codereview.chromium.org/942113004/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome_inspector/inspector_backend.py:131: except (socket.error, websocket.WebSocketException) as e: On 2015/02/24 21:28:49, nednguyen wrote: > On 2015/02/24 21:08:53, erikchen wrote: > > On 2015/02/24 19:50:46, nednguyen wrote: > > > On 2015/02/23 23:01:49, erikchen wrote: > > > > This is a lot of duplicate code. All I'm doing is reproducing the existing > > > > behavior in this CL. > > > > > > > > In a future CL, some of the try/except loops are going to have different > > > logic. > > > > (Notably WaitForNavigate(), Navigate() - I haven't looked too deeply into > > the > > > > other callsites yet). > > > > > > Given the amount of boiler-plate code, I am not sure whether removing the > > > _HandleError param make it better. > > > > As I mentioned in the CL description, the problem with passing _HandleError as > a > > param is that it loses the context of the error. > > > > If you look at the implementation of _HandleError, you'll see that its error > > messages actually make assumptions about the context. "assuming it timed out" > > "assuming it crashed". The assumptions are necessary because it doesn't have > > sufficient context. The error messages themselves are vague, and not very > > helpful. > > > > Furthermore, _HandleError is only called for some websocket exceptions, not > > others. (In fact, it's only called for exceptions in _Receive). This is > > confusing, since callers need to know which exceptions are handled by > > _HandleError, and which are not. We could extend _HandleError be called for > all > > websocket exceptions, but once again, we get more loss of context. An > exception > > while trying to connect a websocket is very different from a timeout while > > waiting for a response from a javascript evaluation. > > > > Finally, while the previous implementations for many of these methods was > short, > > the actual behavior and side effects were unclear. e.g. It is unclear what > > exceptions GetCookieByName will throw. It is unclear that an exception in > > GetCookieByName will actually call back into _HandleError, which converts the > > exception into a different exception, and then reraises the new exception. > > Good arguments. Given this is also nested deep inside telemetry/core and not > something public users have to deal with, I like this. It is possible to drop the boilerplate by writing a decorator: @_HandleWebSocketError def WaitForNavigate(self, timeout): self._page.WaitForNavigate(timeout) However, it is unlikely worth the effort. https://codereview.chromium.org/942113004/diff/60001/tools/telemetry/telemetr... File tools/telemetry/telemetry/core/backends/chrome_inspector/inspector_page.py (right): https://codereview.chromium.org/942113004/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome_inspector/inspector_page.py:14: inspector_websocket exceptions must be handled by the caller.""" """Class comments start with a one-liner. The comments have more lines as a separate block if needed. Plus, for multi-line comments, the final triple-quotes goes on its own line. """ (This is on the Google-internal Python style guide.) https://codereview.chromium.org/942113004/diff/60001/tools/telemetry/telemetr... File tools/telemetry/telemetry/core/backends/chrome_inspector/inspector_websocket.py (right): https://codereview.chromium.org/942113004/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome_inspector/inspector_websocket.py:93: return result How about the following? def _Receive(self, timeout=10): self._SetTimeout(timeout) if not self._socket: return None data = self._socket.recv() ... return result https://codereview.chromium.org/942113004/diff/60001/tools/telemetry/telemetr... File tools/telemetry/telemetry/core/backends/chrome_inspector/tracing_backend.py (right): https://codereview.chromium.org/942113004/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome_inspector/tracing_backend.py:42: self._received_all_tracing_data = False _received_all_tracing_data -> _has_received_all_tracing_data https://codereview.chromium.org/942113004/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome_inspector/tracing_backend.py:128: 'Trace data was not fully received due to timeout after %s ' Only received partial trace data due to timeout after %s seconds. https://codereview.chromium.org/942113004/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome_inspector/tracing_backend.py:130: 'time out amount.' % elapsed_time) time out -> timeout https://codereview.chromium.org/942113004/diff/60001/tools/telemetry/telemetr... File tools/telemetry/telemetry/core/backends/chrome_inspector/tracing_backend_unittest.py (right): https://codereview.chromium.org/942113004/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome_inspector/tracing_backend_unittest.py:26: '''A fake inspector websocket that allows test to send random data. Normal Use triple double quotes and start with a one-liner. https://codereview.chromium.org/942113004/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome_inspector/tracing_backend_unittest.py:39: if len(self._responses) > 0: if self._responses: https://codereview.chromium.org/942113004/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome_inspector/tracing_backend_unittest.py:42: params = {'value' : value} No space before the colon. https://codereview.chromium.org/942113004/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome_inspector/tracing_backend_unittest.py:43: response = {'method' : method, Put it all on one line? response = {'method': method, 'params': params} Otherwise, break at the open curly: response = { 'method': method, 'params': params, } https://codereview.chromium.org/942113004/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome_inspector/tracing_backend_unittest.py:52: if len(self._responses) == 0: if not self._responses: https://codereview.chromium.org/942113004/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome_inspector/tracing_backend_unittest.py:120: 'inspector_websocket.InspectorWebsocket') as mock_class: This indenting style is a style guide violation. How about indenting the second line more: with mock.patch('telemetry.core.backends.chrome_inspector.' 'inspector_websocket.InspectorWebsocket') as mock_class: https://codereview.chromium.org/942113004/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome_inspector/tracing_backend_unittest.py:138: 'inspector_websocket.InspectorWebsocket') as mock_class: indent here too
https://codereview.chromium.org/942113004/diff/60001/tools/telemetry/telemetr... File tools/telemetry/telemetry/core/backends/chrome_inspector/inspector_backend.py (right): https://codereview.chromium.org/942113004/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome_inspector/inspector_backend.py:96: def Screenshot(self, timeout): On 2015/02/25 00:26:54, slamm wrote: > FWIW, I do not see Screenshot getting used. > > https://code.google.com/p/chromium/codesearch#search/&q=%5C.Screenshot%20file... > > Thought jeremy@ did recently file a bug saying that we should capture the screen > on failures: > > https://code.google.com/p/chromium/issues/detail?id=454155 There may be some benchmark that uses screenshot. So it's best to keep this here for now. https://code.google.com/p/chromium/issues/detail?id=242405 https://codereview.chromium.org/942113004/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome_inspector/inspector_backend.py:131: except (socket.error, websocket.WebSocketException) as e: On 2015/02/25 00:26:54, slamm wrote: > On 2015/02/24 21:28:49, nednguyen wrote: > > On 2015/02/24 21:08:53, erikchen wrote: > > > On 2015/02/24 19:50:46, nednguyen wrote: > > > > On 2015/02/23 23:01:49, erikchen wrote: > > > > > This is a lot of duplicate code. All I'm doing is reproducing the > existing > > > > > behavior in this CL. > > > > > > > > > > In a future CL, some of the try/except loops are going to have different > > > > logic. > > > > > (Notably WaitForNavigate(), Navigate() - I haven't looked too deeply > into > > > the > > > > > other callsites yet). > > > > > > > > Given the amount of boiler-plate code, I am not sure whether removing the > > > > _HandleError param make it better. > > > > > > As I mentioned in the CL description, the problem with passing _HandleError > as > > a > > > param is that it loses the context of the error. > > > > > > If you look at the implementation of _HandleError, you'll see that its error > > > messages actually make assumptions about the context. "assuming it timed > out" > > > "assuming it crashed". The assumptions are necessary because it doesn't have > > > sufficient context. The error messages themselves are vague, and not very > > > helpful. > > > > > > Furthermore, _HandleError is only called for some websocket exceptions, not > > > others. (In fact, it's only called for exceptions in _Receive). This is > > > confusing, since callers need to know which exceptions are handled by > > > _HandleError, and which are not. We could extend _HandleError be called for > > all > > > websocket exceptions, but once again, we get more loss of context. An > > exception > > > while trying to connect a websocket is very different from a timeout while > > > waiting for a response from a javascript evaluation. > > > > > > Finally, while the previous implementations for many of these methods was > > short, > > > the actual behavior and side effects were unclear. e.g. It is unclear what > > > exceptions GetCookieByName will throw. It is unclear that an exception in > > > GetCookieByName will actually call back into _HandleError, which converts > the > > > exception into a different exception, and then reraises the new exception. > > > > Good arguments. Given this is also nested deep inside telemetry/core and not > > something public users have to deal with, I like this. > > It is possible to drop the boilerplate by writing a decorator: > > @_HandleWebSocketError > def WaitForNavigate(self, timeout): > self._page.WaitForNavigate(timeout) > > However, it is unlikely worth the effort. I would try to avoid the decorator syntactic sugar. As Erik say, how exception should be handle could be different and it's good to be explicit in this context.
slamm: PTAL https://codereview.chromium.org/942113004/diff/60001/tools/telemetry/telemetr... File tools/telemetry/telemetry/core/backends/chrome_inspector/inspector_page.py (right): https://codereview.chromium.org/942113004/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome_inspector/inspector_page.py:14: inspector_websocket exceptions must be handled by the caller.""" On 2015/02/25 00:26:54, slamm wrote: > """Class comments start with a one-liner. > > The comments have more lines as a separate block if needed. > Plus, for multi-line comments, the final triple-quotes goes on its own line. > """ > > (This is on the Google-internal Python style guide.) Got it. Fixed. https://codereview.chromium.org/942113004/diff/60001/tools/telemetry/telemetr... File tools/telemetry/telemetry/core/backends/chrome_inspector/inspector_websocket.py (right): https://codereview.chromium.org/942113004/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome_inspector/inspector_websocket.py:93: return result On 2015/02/25 00:26:54, slamm wrote: > How about the following? > > def _Receive(self, timeout=10): > self._SetTimeout(timeout) > if not self._socket: > return None > data = self._socket.recv() > ... > return result I'm a big fan of early returns. Done. https://codereview.chromium.org/942113004/diff/60001/tools/telemetry/telemetr... File tools/telemetry/telemetry/core/backends/chrome_inspector/tracing_backend.py (right): https://codereview.chromium.org/942113004/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome_inspector/tracing_backend.py:42: self._received_all_tracing_data = False On 2015/02/25 00:26:55, slamm wrote: > _received_all_tracing_data -> _has_received_all_tracing_data Done. https://codereview.chromium.org/942113004/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome_inspector/tracing_backend.py:128: 'Trace data was not fully received due to timeout after %s ' On 2015/02/25 00:26:54, slamm wrote: > Only received partial trace data due to timeout after %s seconds. Done. https://codereview.chromium.org/942113004/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome_inspector/tracing_backend.py:130: 'time out amount.' % elapsed_time) On 2015/02/25 00:26:55, slamm wrote: > time out -> timeout Done. https://codereview.chromium.org/942113004/diff/60001/tools/telemetry/telemetr... File tools/telemetry/telemetry/core/backends/chrome_inspector/tracing_backend_unittest.py (right): https://codereview.chromium.org/942113004/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome_inspector/tracing_backend_unittest.py:26: '''A fake inspector websocket that allows test to send random data. Normal On 2015/02/25 00:26:55, slamm wrote: > Use triple double quotes and start with a one-liner. Done. https://codereview.chromium.org/942113004/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome_inspector/tracing_backend_unittest.py:39: if len(self._responses) > 0: On 2015/02/25 00:26:55, slamm wrote: > if self._responses: Done. https://codereview.chromium.org/942113004/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome_inspector/tracing_backend_unittest.py:42: params = {'value' : value} On 2015/02/25 00:26:55, slamm wrote: > No space before the colon. Done. https://codereview.chromium.org/942113004/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome_inspector/tracing_backend_unittest.py:43: response = {'method' : method, On 2015/02/25 00:26:55, slamm wrote: > Put it all on one line? > > response = {'method': method, 'params': params} > > Otherwise, break at the open curly: > > response = { > 'method': method, > 'params': params, > } I put everything on one line. https://codereview.chromium.org/942113004/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome_inspector/tracing_backend_unittest.py:52: if len(self._responses) == 0: On 2015/02/25 00:26:55, slamm wrote: > if not self._responses: Done. https://codereview.chromium.org/942113004/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome_inspector/tracing_backend_unittest.py:120: 'inspector_websocket.InspectorWebsocket') as mock_class: On 2015/02/25 00:26:55, slamm wrote: > This indenting style is a style guide violation. How about indenting the second > line more: > > with mock.patch('telemetry.core.backends.chrome_inspector.' > 'inspector_websocket.InspectorWebsocket') as mock_class: Fixed the style. https://codereview.chromium.org/942113004/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome_inspector/tracing_backend_unittest.py:138: 'inspector_websocket.InspectorWebsocket') as mock_class: On 2015/02/25 00:26:55, slamm wrote: > indent here too Fixed the style.
lgtm
The CQ bit was checked by erikchen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nednguyen@google.com Link to the patchset: https://codereview.chromium.org/942113004/#ps80001 (title: "Comments from slamm.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/942113004/80001
Message was sent while issue was closed.
Committed patchset #2 (id:80001)
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/64cf2ab4a6a348954d46d9d758c5be53aa1c1220 Cr-Commit-Position: refs/heads/master@{#317972}
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:80001) has been created in https://codereview.chromium.org/962443002/ by nednguyen@google.com. The reason for reverting is: This possibly is causing chrome tracing data leakage (https://code.google.com/p/chromium/issues/detail?id=461874).. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
