New issue
Advanced search Search tips

Issue 817566 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Aug 10
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: iOS
Pri: 2
Type: Bug



Sign in to add a comment

[Remoting Mobile] Refactoring ChromotingSession

Project Member Reported by yuweih@chromium.org, Feb 28 2018

Issue description

ChromotingSession is a super class that does multiple things and receives calls from both UI and network thread. Some of its components becomes invalidated after the session is disconnected, while others are valid until the session object is destroyed. This makes memory management very painful and error-prone.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Mar 20 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0ebacacbcb0ad87308107a870450674397a152b8

commit 0ebacacbcb0ad87308107a870450674397a152b8
Author: Yuwei Huang <yuweih@chromium.org>
Date: Tue Mar 20 01:06:39 2018

[Remoting Mobile] Refactoring ChromotingSession

ChromotingSession is a complex class that receives calls from both UI
and network thread, with some components being invalidated once the
connection is disconnected, while other components are invalidated when
the session instance is destroyed.

This CL refactors the ChromotingSession class by moving session-bound
and network thread logic into a core class. It also allows
TelemetryLogWriter and ClientTelemetryLogger to be created on a thread
different than the thread they are later used and destroyed, to make
the refactoring a bit easier.

Bug:  817566 
Change-Id: I909113cb70fcb15bac2e8a506eb4f32d63f7ad47
Reviewed-on: https://chromium-review.googlesource.com/953597
Commit-Queue: Yuwei Huang <yuweih@chromium.org>
Reviewed-by: Joe Downing <joedow@chromium.org>
Cr-Commit-Position: refs/heads/master@{#544243}
[modify] https://crrev.com/0ebacacbcb0ad87308107a870450674397a152b8/remoting/base/chromoting_event_log_writer.h
[modify] https://crrev.com/0ebacacbcb0ad87308107a870450674397a152b8/remoting/base/telemetry_log_writer.cc
[modify] https://crrev.com/0ebacacbcb0ad87308107a870450674397a152b8/remoting/base/telemetry_log_writer.h
[modify] https://crrev.com/0ebacacbcb0ad87308107a870450674397a152b8/remoting/base/telemetry_log_writer_unittest.cc
[modify] https://crrev.com/0ebacacbcb0ad87308107a870450674397a152b8/remoting/client/chromoting_client_runtime.cc
[modify] https://crrev.com/0ebacacbcb0ad87308107a870450674397a152b8/remoting/client/chromoting_client_runtime.h
[modify] https://crrev.com/0ebacacbcb0ad87308107a870450674397a152b8/remoting/client/chromoting_session.cc
[modify] https://crrev.com/0ebacacbcb0ad87308107a870450674397a152b8/remoting/client/chromoting_session.h
[modify] https://crrev.com/0ebacacbcb0ad87308107a870450674397a152b8/remoting/client/client_telemetry_logger.cc
[modify] https://crrev.com/0ebacacbcb0ad87308107a870450674397a152b8/remoting/client/client_telemetry_logger.h
[modify] https://crrev.com/0ebacacbcb0ad87308107a870450674397a152b8/remoting/client/client_telemetry_logger_unittest.cc
[modify] https://crrev.com/0ebacacbcb0ad87308107a870450674397a152b8/remoting/client/connect_to_host_info.cc
[modify] https://crrev.com/0ebacacbcb0ad87308107a870450674397a152b8/remoting/client/connect_to_host_info.h
[modify] https://crrev.com/0ebacacbcb0ad87308107a870450674397a152b8/remoting/client/jni/jni_client.cc
[modify] https://crrev.com/0ebacacbcb0ad87308107a870450674397a152b8/remoting/ios/session/remoting_client.mm

Project Member

Comment 2 by bugdroid1@chromium.org, Mar 22 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/35a5c82faf8e96fb224d1b20bada488d96a2a7ad

commit 35a5c82faf8e96fb224d1b20bada488d96a2a7ad
Author: Yuwei Huang <yuweih@chromium.org>
Date: Thu Mar 22 00:19:29 2018

[Remoting Mobile] Rename ChromotingSession::ProvideSecret

This method doesn't use the PIN. This CL renames this method to
RequestPairing.

Bug:  817566 
Change-Id: I50cc8da936ae7242c2f227e8b4f78f013f26f6b1
Reviewed-on: https://chromium-review.googlesource.com/974461
Reviewed-by: Joe Downing <joedow@chromium.org>
Commit-Queue: Yuwei Huang <yuweih@chromium.org>
Cr-Commit-Position: refs/heads/master@{#544917}
[modify] https://crrev.com/35a5c82faf8e96fb224d1b20bada488d96a2a7ad/remoting/client/chromoting_session.cc
[modify] https://crrev.com/35a5c82faf8e96fb224d1b20bada488d96a2a7ad/remoting/client/chromoting_session.h
[modify] https://crrev.com/35a5c82faf8e96fb224d1b20bada488d96a2a7ad/remoting/client/jni/jni_client.cc
[modify] https://crrev.com/35a5c82faf8e96fb224d1b20bada488d96a2a7ad/remoting/ios/session/remoting_client.mm

Project Member

Comment 3 by bugdroid1@chromium.org, Mar 22 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e5d9939ec98d7c714fc9c95e0a35f08ce71ec035

commit e5d9939ec98d7c714fc9c95e0a35f08ce71ec035
Author: Yuwei Huang <yuweih@chromium.org>
Date: Thu Mar 22 02:33:31 2018

[Remoting Mobile] Remove unused weak_ptr from ChromotingSession

The weak_ptr_ in ChromotingSession seems to be unused. This CL removes
it.

Bug:  817566 
Change-Id: I4f07be8e8047b76ce36f5fe658b17bf6cf45ec72
Reviewed-on: https://chromium-review.googlesource.com/974563
Reviewed-by: Joe Downing <joedow@chromium.org>
Commit-Queue: Yuwei Huang <yuweih@chromium.org>
Cr-Commit-Position: refs/heads/master@{#544956}
[modify] https://crrev.com/e5d9939ec98d7c714fc9c95e0a35f08ce71ec035/remoting/client/chromoting_session.cc
[modify] https://crrev.com/e5d9939ec98d7c714fc9c95e0a35f08ce71ec035/remoting/client/chromoting_session.h

Status: Fixed (was: Assigned)

Sign in to add a comment