Fixed the note creation from stylus tool.

Noticed that the stylus tool was not launching android apps due to a
permission error with the content provider.  Updated the ARC++
container to a more preferred method of launching the Android note
taking app.

Bug: b/138960936
Change-Id: If9d5e5fbdce610df24bfe94a5c40c160b7f7a347
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1783221
Commit-Queue: Alexander Nohe <nohe@chromium.org>
Reviewed-by: Hidehiko Abe <hidehiko@chromium.org>
Reviewed-by: Dominick Ng <dominickn@chromium.org>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#704212}

chrome/browser/chromeos/note_taking_helper.cc

@@ -10,6 +10,7 @@
 #include "apps/launcher.h"
 #include "ash/public/cpp/stylus_utils.h"
 #include "base/bind.h"
+#include "base/bind_helpers.h"
 #include "base/command_line.h"
 #include "base/files/file_path.h"
 #include "base/logging.h"
@@ -29,6 +30,7 @@
 #include "components/arc/arc_service_manager.h"
 #include "components/arc/intent_helper/arc_intent_helper_bridge.h"
 #include "components/arc/metrics/arc_metrics_constants.h"
+#include "components/arc/mojom/file_system.mojom.h"
 #include "components/arc/session/arc_bridge_service.h"
 #include "components/prefs/pref_service.h"
 #include "components/prefs/scoped_user_pref_update.h"
@@ -478,6 +480,20 @@ void NoteTakingHelper::UpdateAndroidApps() {
                                            weak_ptr_factory_.GetWeakPtr()));
 }
 
+arc::mojom::ActivityNamePtr AppIdToActivityName(const std::string& id) {
+  auto name = arc::mojom::ActivityName::New();
+
+  const size_t separator = id.find('/');
+  if (separator == std::string::npos) {
+    name->package_name = id;
+    name->activity_name = std::string();
+  } else {
+    name->package_name = id.substr(0, separator);
+    name->activity_name = id.substr(separator + 1);
+  }
+  return name;
+}
+
 void NoteTakingHelper::OnGotAndroidApps(
     std::vector<arc::mojom::IntentHandlerInfoPtr> handlers) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
@@ -497,6 +513,21 @@ void NoteTakingHelper::OnGotAndroidApps(
     observer.OnAvailableNoteTakingAppsUpdated();
 }
 
+arc::mojom::OpenUrlsRequestPtr CreateArcNoteRequest(const std::string& app_id,
+                                                    const GURL& clip_data_uri) {
+  auto request = arc::mojom::OpenUrlsRequest::New();
+  request->action_type = arc::mojom::ActionType::CREATE_NOTE;
+  request->activity_name = AppIdToActivityName(app_id);
+  if (!clip_data_uri.is_empty()) {
+    auto url_with_type = arc::mojom::ContentUrlWithMimeType::New();
+    url_with_type->content_url = clip_data_uri;
+    url_with_type->mime_type = "image/png";
+    request->urls.push_back(std::move(url_with_type));
+  }
+
+  return request;
+}
+
 NoteTakingHelper::LaunchResult NoteTakingHelper::LaunchAppInternal(
     Profile* profile,
     const std::string& app_id,
@@ -531,7 +562,15 @@ NoteTakingHelper::LaunchResult NoteTakingHelper::LaunchAppInternal(
 
     // TODO(derat): Is there some way to detect whether this fails due to the
     // package no longer being available?
-    helper->HandleIntent(CreateIntentInfo(clip_data_uri), std::move(activity));
+    auto request = CreateArcNoteRequest(app_id, clip_data_uri);
+    arc::mojom::FileSystemInstance* arc_file_system =
+        ARC_GET_INSTANCE_FOR_METHOD(
+            arc::ArcServiceManager::Get()->arc_bridge_service()->file_system(),
+            OpenUrlsWithPermission);
+    if (!arc_file_system)
+      return LaunchResult::ANDROID_NOT_RUNNING;
+    arc_file_system->OpenUrlsWithPermission(std::move(request),
+                                            base::DoNothing());
 
     UMA_HISTOGRAM_ENUMERATION(
         "Arc.UserInteraction",

chrome/browser/chromeos/note_taking_helper_unittest.cc

@@ -17,6 +17,7 @@
 #include "base/strings/utf_string_conversions.h"
 #include "base/test/metrics/histogram_tester.h"
 #include "chrome/browser/chrome_notification_types.h"
+#include "chrome/browser/chromeos/arc/fileapi/arc_file_system_bridge.h"
 #include "chrome/browser/chromeos/file_manager/path_util.h"
 #include "chrome/browser/chromeos/note_taking_controller_client.h"
 #include "chrome/browser/extensions/extension_service.h"
@@ -33,9 +34,11 @@
 #include "components/arc/arc_service_manager.h"
 #include "components/arc/arc_util.h"
 #include "components/arc/intent_helper/arc_intent_helper_bridge.h"
+#include "components/arc/mojom/file_system.mojom.h"
 #include "components/arc/mojom/intent_helper.mojom.h"
 #include "components/arc/session/arc_bridge_service.h"
 #include "components/arc/test/connection_holder_util.h"
+#include "components/arc/test/fake_file_system_instance.h"
 #include "components/arc/test/fake_intent_helper_instance.h"
 #include "components/crx_file/id_util.h"
 #include "components/sync_preferences/testing_pref_service_syncable.h"
@@ -82,21 +85,6 @@ std::string GetAppString(const NoteTakingAppInfo& info) {
                       info.lock_screen_support);
 }
 
-// Helper functions returning strings that can be used to compare launched
-// intents.
-std::string GetIntentString(const std::string& package,
-                            const std::string& clip_data_uri) {
-  return base::StringPrintf(
-      "%s %s", package.c_str(),
-      clip_data_uri.empty() ? "[unset]" : clip_data_uri.c_str());
-}
-std::string GetIntentString(const HandledIntent& intent) {
-  EXPECT_EQ(NoteTakingHelper::kIntentAction, intent.intent->action);
-  return GetIntentString(
-      intent.activity->package_name,
-      (intent.intent->clip_data_uri ? *intent.intent->clip_data_uri : ""));
-}
-
 // Creates an ARC IntentHandlerInfo object.
 IntentHandlerInfoPtr CreateIntentHandlerInfo(const std::string& name,
                                              const std::string& package) {
@@ -165,8 +153,13 @@ class NoteTakingHelperTest : public BrowserWithTestWindowTest {
           ->arc_bridge_service()
           ->intent_helper()
           ->CloseInstance(&intent_helper_);
+      arc::ArcServiceManager::Get()
+          ->arc_bridge_service()
+          ->file_system()
+          ->CloseInstance(file_system_.get());
       NoteTakingHelper::Shutdown();
       intent_helper_bridge_.reset();
+      file_system_bridge_.reset();
       arc_test_.TearDown();
     }
     extensions::ExtensionSystem::Get(profile())->Shutdown();
@@ -218,6 +211,18 @@ class NoteTakingHelperTest : public BrowserWithTestWindowTest {
     WaitForInstanceReady(
         arc::ArcServiceManager::Get()->arc_bridge_service()->intent_helper());
 
+    file_system_bridge_ = std::make_unique<arc::ArcFileSystemBridge>(
+        profile(), arc::ArcServiceManager::Get()->arc_bridge_service());
+    file_system_ = std::make_unique<arc::FakeFileSystemInstance>();
+
+    arc::ArcServiceManager::Get()
+        ->arc_bridge_service()
+        ->file_system()
+        ->SetInstance(file_system_.get());
+    WaitForInstanceReady(
+        arc::ArcServiceManager::Get()->arc_bridge_service()->file_system());
+    ASSERT_TRUE(file_system_->InitCalled());
+
     if (flags & ENABLE_PALETTE) {
       base::CommandLine::ForCurrentProcess()->AppendSwitch(
           ash::switches::kAshForceEnableStylusTools);
@@ -405,6 +410,10 @@ class NoteTakingHelperTest : public BrowserWithTestWindowTest {
 
   arc::FakeIntentHelperInstance intent_helper_;
 
+  std::unique_ptr<arc::ArcFileSystemBridge> file_system_bridge_;
+
+  std::unique_ptr<arc::FakeFileSystemInstance> file_system_;
+
   // Pointer to the primary profile (returned by |profile()|) prefs - owned by
   // the profile.
   sync_preferences::TestingPrefServiceSyncable* profile_prefs_ = nullptr;
@@ -903,9 +912,16 @@ TEST_F(NoteTakingHelperTest, LaunchAndroidApp) {
   // The installed app should be launched.
   std::unique_ptr<HistogramTester> histogram_tester(new HistogramTester());
   helper()->LaunchAppForNewNote(profile(), base::FilePath());
-  ASSERT_EQ(1u, intent_helper_.handled_intents().size());
-  EXPECT_EQ(GetIntentString(kPackage1, ""),
-            GetIntentString(intent_helper_.handled_intents()[0]));
+  ASSERT_EQ(1u, file_system_->handledUrlRequests().size());
+  EXPECT_EQ(arc::mojom::ActionType::CREATE_NOTE,
+            file_system_->handledUrlRequests().at(0)->action_type);
+  EXPECT_EQ(
+      kPackage1,
+      file_system_->handledUrlRequests().at(0)->activity_name->package_name);
+  EXPECT_EQ(
+      std::string(),
+      file_system_->handledUrlRequests().at(0)->activity_name->activity_name);
+  ASSERT_EQ(0u, file_system_->handledUrlRequests().at(0)->urls.size());
 
   histogram_tester->ExpectUniqueSample(
       NoteTakingHelper::kPreferredLaunchResultHistogramName,
@@ -926,11 +942,19 @@ TEST_F(NoteTakingHelperTest, LaunchAndroidApp) {
 
   // The second app should be launched now.
   intent_helper_.clear_handled_intents();
+  file_system_->clear_handled_requests();
   histogram_tester.reset(new HistogramTester());
   helper()->LaunchAppForNewNote(profile(), base::FilePath());
-  ASSERT_EQ(1u, intent_helper_.handled_intents().size());
-  EXPECT_EQ(GetIntentString(kPackage2, ""),
-            GetIntentString(intent_helper_.handled_intents()[0]));
+  ASSERT_EQ(1u, file_system_->handledUrlRequests().size());
+  EXPECT_EQ(arc::mojom::ActionType::CREATE_NOTE,
+            file_system_->handledUrlRequests().at(0)->action_type);
+  EXPECT_EQ(
+      kPackage2,
+      file_system_->handledUrlRequests().at(0)->activity_name->package_name);
+  EXPECT_EQ(
+      std::string(),
+      file_system_->handledUrlRequests().at(0)->activity_name->activity_name);
+  ASSERT_EQ(0u, file_system_->handledUrlRequests().at(0)->urls.size());
 
   histogram_tester->ExpectUniqueSample(
       NoteTakingHelper::kPreferredLaunchResultHistogramName,
@@ -954,25 +978,45 @@ TEST_F(NoteTakingHelperTest, LaunchAndroidAppWithPath) {
       file_manager::util::GetDownloadsFolderForProfile(profile()).Append(
           "image.jpg"));
   helper()->LaunchAppForNewNote(profile(), kDownloadedPath);
-  ASSERT_EQ(1u, intent_helper_.handled_intents().size());
-  EXPECT_EQ(GetIntentString(kPackage, GetArcUrl(kDownloadedPath)),
-            GetIntentString(intent_helper_.handled_intents()[0]));
+  ASSERT_EQ(1u, file_system_->handledUrlRequests().size());
+  EXPECT_EQ(arc::mojom::ActionType::CREATE_NOTE,
+            file_system_->handledUrlRequests().at(0)->action_type);
+  EXPECT_EQ(
+      kPackage,
+      file_system_->handledUrlRequests().at(0)->activity_name->package_name);
+  EXPECT_EQ(
+      std::string(),
+      file_system_->handledUrlRequests().at(0)->activity_name->activity_name);
+  ASSERT_EQ(1u, file_system_->handledUrlRequests().at(0)->urls.size());
+  ASSERT_EQ(GetArcUrl(kDownloadedPath),
+            file_system_->handledUrlRequests().at(0)->urls.at(0)->content_url);
 
   const base::FilePath kRemovablePath =
       base::FilePath(file_manager::util::kRemovableMediaPath)
           .Append("image.jpg");
   intent_helper_.clear_handled_intents();
+  file_system_->clear_handled_requests();
   helper()->LaunchAppForNewNote(profile(), kRemovablePath);
-  ASSERT_EQ(1u, intent_helper_.handled_intents().size());
-  EXPECT_EQ(GetIntentString(kPackage, GetArcUrl(kRemovablePath)),
-            GetIntentString(intent_helper_.handled_intents()[0]));
+  ASSERT_EQ(1u, file_system_->handledUrlRequests().size());
+  EXPECT_EQ(arc::mojom::ActionType::CREATE_NOTE,
+            file_system_->handledUrlRequests().at(0)->action_type);
+  EXPECT_EQ(
+      kPackage,
+      file_system_->handledUrlRequests().at(0)->activity_name->package_name);
+  EXPECT_EQ(
+      std::string(),
+      file_system_->handledUrlRequests().at(0)->activity_name->activity_name);
+  ASSERT_EQ(1u, file_system_->handledUrlRequests().at(0)->urls.size());
+  ASSERT_EQ(GetArcUrl(kRemovablePath),
+            file_system_->handledUrlRequests().at(0)->urls.at(0)->content_url);
 
   // When a path that isn't accessible to ARC is passed, the request should be
   // dropped.
   HistogramTester histogram_tester;
   intent_helper_.clear_handled_intents();
+  file_system_->clear_handled_requests();
   helper()->LaunchAppForNewNote(profile(), base::FilePath("/bad/path.jpg"));
-  EXPECT_TRUE(intent_helper_.handled_intents().empty());
+  EXPECT_TRUE(file_system_->handledUrlRequests().empty());
 
   histogram_tester.ExpectUniqueSample(
       NoteTakingHelper::kPreferredLaunchResultHistogramName,

components/arc/mojom/intent_common.mojom

@@ -10,6 +10,7 @@ enum ActionType {
   VIEW,           // Can handle only one URL.
   SEND,           // Can handle only one URL.
   SEND_MULTIPLE,  // Can handle multiple URLs.
+  CREATE_NOTE,    // Can handle only one URL.
 };
 
 // Describes an activity.

components/arc/test/fake_file_system_instance.cc

@@ -565,6 +565,7 @@ void FakeFileSystemInstance::OpenUrlsWithPermission(
     mojom::OpenUrlsRequestPtr request,
     OpenUrlsWithPermissionCallback callback) {
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+  handled_url_requests_.emplace_back(std::move(request));
 }
 
 std::string FakeFileSystemInstance::FindChildDocumentId(

components/arc/test/fake_file_system_instance.h

@@ -217,6 +217,14 @@ class FakeFileSystemInstance : public mojom::FileSystemInstance {
   // |bytes| bytes.
   std::string GetFileContent(const std::string& url, size_t bytes);
 
+  // For clearing the list of handled requests
+  void clear_handled_requests() { handled_url_requests_.clear(); }
+
+  // For validating the url requests in testing.
+  const std::vector<mojom::OpenUrlsRequestPtr>& handledUrlRequests() const {
+    return handled_url_requests_;
+  }
+
   // mojom::FileSystemInstance:
   void AddWatcher(const std::string& authority,
                   const std::string& document_id,
@@ -326,6 +334,9 @@ class FakeFileSystemInstance : public mojom::FileSystemInstance {
   // Mapping from a watcher ID to a document key.
   std::map<int64_t, DocumentKey> watcher_to_document_;
 
+  // List of all OpenUrlsRequests made to the fake_file_system_instance
+  std::vector<mojom::OpenUrlsRequestPtr> handled_url_requests_;
+
   // List of roots added by AddRoot().
   std::vector<Root> roots_;