[CRD iOS] Pass FeedbackData to HelpAndFeedback

This CL passes feedback data created from ChromotingSession to the
HelpAndFeedback instance, so that it can be later attached to the
feedback report.

This CL is coupled with an internal CL:
https://chrome-internal-review.googlesource.com/c/chrome/ios_internal/+/578187

Bug: 814863
Change-Id: I83c41ec7850ab5bd6852e54438ab120aab65656c
Reviewed-on: https://chromium-review.googlesource.com/940265
Commit-Queue: Yuwei Huang <yuweih@chromium.org>
Reviewed-by: Jamie Walch <jamiewalch@chromium.org>
Cr-Commit-Position: refs/heads/master@{#539987}

remoting/client/chromoting_session.h

@@ -44,6 +44,7 @@ class ChromotingSession : public ClientUserInterface,
                           public protocol::ClipboardStub,
                           public ClientInputInjector {
  public:
+  // All methods of the delegate are called on the UI threads.
   class Delegate {
    public:
     virtual ~Delegate() {}
@@ -72,6 +73,9 @@ class ChromotingSession : public ClientUserInterface,
                                         const std::string& message) = 0;
   };
 
+  using GetFeedbackDataCallback =
+      base::OnceCallback<void(std::unique_ptr<FeedbackData>)>;
+
   // Initiates a connection with the specified host. Call from the UI thread.
   ChromotingSession(
       base::WeakPtr<ChromotingSession::Delegate> delegate,
@@ -102,10 +106,14 @@ class ChromotingSession : public ClientUserInterface,
       const std::string& scope,
       const protocol::ThirdPartyTokenFetchedCallback& token_fetched_callback);
 
-  // Creates feedback data to be included in the feedback report.
-  std::unique_ptr<FeedbackData> CreateFeedbackData() const;
+  // Gets the current feedback data and returns it to the callback on the
+  // caller's thread. If the session is never connected, then an empty feedback
+  // will be returned, otherwise feedback for current session (either still
+  // connected or already disconnected) will be returned.
+  void GetFeedbackData(GetFeedbackDataCallback callback) const;
 
-  // Called by the client when the token is fetched.
+  // Called by the client when the token is fetched. Can be called on any
+  // thread.
   void HandleOnThirdPartyTokenFetched(const std::string& token,
                                       const std::string& shared_secret);
 
@@ -155,10 +163,6 @@ class ChromotingSession : public ClientUserInterface,
   // CursorShapeStub implementation.
   void InjectClipboardEvent(const protocol::ClipboardEvent& event) override;
 
-  // Get the weak pointer of the instance. Please only use it on the network
-  // thread.
-  base::WeakPtr<ChromotingSession> GetWeakPtr();
-
  private:
   void ConnectToHostOnNetworkThread();
 
@@ -176,6 +180,10 @@ class ChromotingSession : public ClientUserInterface,
   // Called on the network thread.
   void LogPerfStats();
 
+  void GetFeedbackDataOnNetworkThread(
+      GetFeedbackDataCallback callback,
+      scoped_refptr<base::SingleThreadTaskRunner> response_thread) const;
+
   // Releases the resource in the right order.
   void ReleaseResources();
 
@@ -223,10 +231,25 @@ class ChromotingSession : public ClientUserInterface,
   protocol::ConnectionToHost::State session_state_ =
       protocol::ConnectionToHost::INITIALIZING;
 
+  // The logger is created when the session is connected and destroyed when the
+  // session object is destroyed, rather than when the session is disconnected,
+  // so that caller can have a chance to see logs from a previously disconnected
+  // session.
   std::unique_ptr<ClientTelemetryLogger> logger_;
 
-  base::WeakPtr<ChromotingSession> weak_ptr_;
-  base::WeakPtrFactory<ChromotingSession> weak_factory_;
+  // These weak pointers are used on network thread.
+  base::WeakPtr<ChromotingSession> weak_ptr_per_connection_;
+  base::WeakPtr<ChromotingSession> weak_ptr_per_instance_lifetime_;
+
+  // Both weak_ptr's are constructed when the instance is created, while
+  // |weak_ptr_per_connection_| is invalidated when the session is disconnected,
+  // so that tasks do not leak after the session is disconnected, while
+  // |weak_factory_per_instance_lifetime_| is invalidated when the instace
+  // itself is invalidated.
+  // TODO(crbug/817566): Once we have a shell-core pair, make
+  // |weak_factory_per_connection_| the weak pointer of the core.
+  base::WeakPtrFactory<ChromotingSession> weak_factory_per_connection_;
+  base::WeakPtrFactory<ChromotingSession> weak_factory_per_instance_lifetime_;
 
   DISALLOW_COPY_AND_ASSIGN(ChromotingSession);
 };

remoting/client/jni/jni_client.cc

@@ -281,11 +281,9 @@ void JniClient::OnThirdPartyTokenFetched(
     const base::android::JavaParamRef<jobject>& caller,
     const JavaParamRef<jstring>& token,
     const JavaParamRef<jstring>& shared_secret) {
-  runtime_->network_task_runner()->PostTask(
-      FROM_HERE,
-      base::Bind(&ChromotingSession::HandleOnThirdPartyTokenFetched,
-                 session_->GetWeakPtr(), ConvertJavaStringToUTF8(env, token),
-                 ConvertJavaStringToUTF8(env, shared_secret)));
+  session_->HandleOnThirdPartyTokenFetched(
+      ConvertJavaStringToUTF8(env, token),
+      ConvertJavaStringToUTF8(env, shared_secret));
 }
 
 void JniClient::SendExtensionMessage(

remoting/ios/app/app_delegate.h

@@ -23,6 +23,7 @@
 // Presents the help center modally onto the topmost view controller.
 - (void)presentHelpCenter;
 
+// TODO(yuweih): Replace calls to this method with methods from HelpAndFeedback.
 // This will present the Send Feedback view controller onto the topmost view
 // controller.
 // context: a unique identifier for the user's place within the app which can be

remoting/ios/app/help_and_feedback.h

@@ -7,16 +7,24 @@
 
 #import <UIKit/UIKit.h>
 
-// This is the base class to provide help and feedback functionalities. The
-// base implementation does nothing.
+#include "remoting/client/feedback_data.h"
+
+// This is the base class to provide help and feedback functionalities.
 @interface HelpAndFeedback : NSObject
 
 // This will present the Send Feedback view controller onto the topmost view
 // controller.
 // context: a unique identifier for the user's place within the app which can be
 // used to categorize the feedback report and segment usage metrics.
+// The base implementation simply calls
+// presentFeedbackFlowWithContext:feedbackData: with empty feedback data.
 - (void)presentFeedbackFlowWithContext:(NSString*)context;
 
+// Presents a feedback view controller with extra feedback data.
+// The base implementation does nothing.
+- (void)presentFeedbackFlowWithContext:(NSString*)context
+                          feedbackData:(const remoting::FeedbackData&)data;
+
 // Instance can only be set once.
 @property(nonatomic, class) HelpAndFeedback* instance;
 

remoting/ios/app/help_and_feedback.mm

@@ -17,6 +17,12 @@ static HelpAndFeedback* g_helpAndFeedback;
 #pragma mark - Public
 
 - (void)presentFeedbackFlowWithContext:(NSString*)context {
+  [self presentFeedbackFlowWithContext:context
+                          feedbackData:remoting::FeedbackData()];
+}
+
+- (void)presentFeedbackFlowWithContext:(NSString*)context
+                          feedbackData:(const remoting::FeedbackData&)data {
   NOTIMPLEMENTED() << "This should be implemented by a subclass.";
 }
 

remoting/ios/app/host_view_controller.mm

@@ -11,6 +11,7 @@
 #include <memory>
 
 #import "ios/third_party/material_components_ios/src/components/Buttons/src/MaterialButtons.h"
+#import "remoting/ios/app/help_and_feedback.h"
 #import "remoting/ios/app/remoting_theme.h"
 #import "remoting/ios/app/settings/remoting_settings_view_controller.h"
 #import "remoting/ios/app/view_utils.h"
@@ -34,6 +35,8 @@ static const CGFloat kFabInset = 15.f;
 static const CGFloat kKeyboardAnimationTime = 0.3;
 static const CGFloat kMoveFABAnimationTime = 0.3;
 
+static NSString* const kFeedbackContext = @"InSessionFeedbackContext";
+
 @interface HostViewController ()<ClientKeyboardDelegate,
                                  ClientGesturesDelegate,
                                  RemotingSettingsViewControllerDelegate> {
@@ -370,6 +373,14 @@ static const CGFloat kMoveFABAnimationTime = 0.3;
   [self setFabIsRight:!_fabIsRight shouldLayout:YES];
 }
 
+- (void)sendFeedback {
+  [_client createFeedbackDataWithCallback:^(
+               const remoting::FeedbackData& data) {
+    [HelpAndFeedback.instance presentFeedbackFlowWithContext:kFeedbackContext
+                                                feedbackData:data];
+  }];
+}
+
 #pragma mark - Private
 
 - (void)setFabIsRight:(BOOL)fabIsRight shouldLayout:(BOOL)shouldLayout {

remoting/ios/app/settings/remoting_settings_view_controller.h

@@ -23,6 +23,8 @@
 - (void)sendCtrAltDel;
 // Sends the key Print Screen to the host.
 - (void)sendPrintScreen;
+// Sends feedback to the developers.
+- (void)sendFeedback;
 
 @end
 

remoting/ios/app/settings/remoting_settings_view_controller.mm

@@ -21,7 +21,6 @@
 #include "ui/base/l10n/l10n_util.h"
 
 static NSString* const kReusableIdentifierItem = @"remotingSettingsVCItem";
-static NSString* const kFeedbackContext = @"InSessionFeedbackContext";
 
 static const CGFloat kSectionSeparatorHeight = 1.f;
 
@@ -345,9 +344,7 @@ static const CGFloat kSectionSeparatorHeight = 1.f;
     // Dismiss self so that it can capture the screenshot of HostView.
     [weakSelf dismissViewControllerAnimated:YES
                                  completion:^{
-                                   [AppDelegate.instance
-                                       presentFeedbackFlowWithContext:
-                                           kFeedbackContext];
+                                   [weakSelf.delegate sendFeedback];
                                  }];
   };
 

remoting/ios/session/remoting_client.h

@@ -10,6 +10,7 @@
 
 #import "remoting/ios/display/gl_display_handler.h"
 
+#include "remoting/client/feedback_data.h"
 #include "remoting/protocol/connection_to_host.h"
 
 namespace remoting {
@@ -74,6 +75,10 @@ extern NSString* const kHostSessionPin;
 
 - (void)setHostResolution:(CGSize)dipsResolution scale:(int)scale;
 
+// Creates a feedback data and returns it to the callback.
+- (void)createFeedbackDataWithCallback:
+    (void (^)(const remoting::FeedbackData&))callback;
+
 // The display handler tied to the remoting client used to display the host.
 @property(nonatomic, strong) GlDisplayHandler* displayHandler;
 // The host info used to make the remoting client connection.

remoting/ios/session/remoting_client.mm

@@ -48,6 +48,18 @@ static std::string GetCurrentUserId() {
       RemotingService.instance.authentication.user.userId);
 }
 
+// Block doesn't work with rvalue passing. This function helps exposing the data
+// to the block.
+static void ResolveFeedbackDataCallback(
+    void (^callback)(const remoting::FeedbackData&),
+    std::unique_ptr<remoting::FeedbackData> data) {
+  DCHECK(remoting::ChromotingClientRuntime::GetInstance()
+             ->ui_task_runner()
+             ->BelongsToCurrentThread());
+  remoting::FeedbackData* raw_data = data.get();
+  callback(*raw_data);
+}
+
 @interface RemotingClient () {
   remoting::ChromotingClientRuntime* _runtime;
   std::unique_ptr<remoting::RemotingClientSessonDelegate> _sessonDelegate;
@@ -354,6 +366,18 @@ static std::string GetCurrentUserId() {
                                  scale);
 }
 
+- (void)createFeedbackDataWithCallback:
+    (void (^)(const remoting::FeedbackData&))callback {
+  if (!_session) {
+    // Session has never been connected. Returns an empty data.
+    callback(remoting::FeedbackData());
+    return;
+  }
+
+  _session->GetFeedbackData(
+      base::BindOnce(&ResolveFeedbackDataCallback, callback));
+}
+
 #pragma mark - GlDisplayHandlerDelegate
 
 - (void)canvasSizeChanged:(CGSize)size {