Fix URL of feedback invoked by alt-shift-i from an active lacros window.

Add a GetActiveTabUrl crosapi in LacrosChromeService to allow ash
to get the URL of active lacros tab. Show active lacros tab's
URL in feedback dialog when user press alt-shift-i from an active
lacros window.

TEST:
1. Launch lacros chrome, invoke feedback by alt-shift-i. Make sure
the URL field of the feedback dialog shows the url of active lacros
tab.
2. Launch lacros chrome, deactivate it by minimizing it, or activate
another app, invoke feedback by alt-shift-i. Make sure the URL field
of the feedback dialog is empty.
3. Launch both lacros chrome and ash chrome, minimize lacros chrome
or keep it in background, have ash chrome in the foreground, invoke
feedback by alt-shift-i. Make sure the URL field of the feedback
dialog shows the url of the active ash chrome tab.

Bug: 1142541
Change-Id: I3edc15db3fdda22214f3b849b24d4b619b219f59
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2505736
Commit-Queue: Jenny Zhang <jennyz@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Reviewed-by: Miriam Zimmerman <mutexlox@chromium.org>
Cr-Commit-Position: refs/heads/master@{#822087}

chrome/browser/chromeos/crosapi/browser_manager.cc

@@ -63,6 +63,9 @@ constexpr uint32_t kGetFeedbackDataMinVersion = 6;
 // The min version of LacrosChromeService mojo interface that supports
 // GetHistograms API.
 constexpr uint32_t kGetHistogramsMinVersion = 7;
+// The min version of LacrosChromeService mojo interface that supports
+// GetActiveTabUrl API.
+constexpr uint32_t kGetActiveTabUrlMinVersion = 8;
 
 base::FilePath LacrosLogPath() {
   return browser_util::GetUserDataDir().Append("lacros.log");
@@ -247,6 +250,16 @@ void BrowserManager::GetHistograms(GetHistogramsCallback callback) {
   lacros_chrome_service_->GetHistograms(std::move(callback));
 }
 
+bool BrowserManager::GetActiveTabUrlSupported() const {
+  return lacros_chrome_service_version_ >= kGetActiveTabUrlMinVersion;
+}
+
+void BrowserManager::GetActiveTabUrl(GetActiveTabUrlCallback callback) {
+  DCHECK(lacros_chrome_service_.is_connected());
+  DCHECK(GetActiveTabUrlSupported());
+  lacros_chrome_service_->GetActiveTabUrl(std::move(callback));
+}
+
 void BrowserManager::AddObserver(BrowserManagerObserver* observer) {
   observers_.AddObserver(observer);
 }

chrome/browser/chromeos/crosapi/browser_manager.h

@@ -90,6 +90,14 @@ class BrowserManager : public session_manager::SessionManagerObserver {
   // Gets Lacros histograms.
   void GetHistograms(GetHistogramsCallback callback);
 
+  // Returns true if crosapi interface supports GetActiveTabUrl API.
+  bool GetActiveTabUrlSupported() const;
+
+  using GetActiveTabUrlCallback =
+      base::OnceCallback<void(const base::Optional<GURL>&)>;
+  // Gets Url of the active tab from lacros if there is any.
+  void GetActiveTabUrl(GetActiveTabUrlCallback callback);
+
   void AddObserver(BrowserManagerObserver* observer);
   void RemoveObserver(BrowserManagerObserver* observer);
 

chrome/browser/chromeos/crosapi/test_mojo_connection_manager_unittest.cc

@@ -56,6 +56,7 @@ class TestLacrosChromeService : public crosapi::mojom::LacrosChromeService {
   void NewWindow(NewWindowCallback callback) override {}
   void GetFeedbackData(GetFeedbackDataCallback callback) override {}
   void GetHistograms(GetHistogramsCallback callback) override {}
+  void GetActiveTabUrl(GetActiveTabUrlCallback callback) override {}
 
   bool init_is_called() { return init_is_called_; }
 

chrome/browser/feedback/show_feedback_page.cc

@@ -19,6 +19,8 @@
 #include "extensions/browser/api/feedback_private/feedback_private_api.h"
 
 #if defined(OS_CHROMEOS)
+#include "base/bind.h"
+#include "chrome/browser/chromeos/crosapi/browser_manager.h"
 #include "chrome/browser/signin/identity_manager_factory.h"
 #include "components/signin/public/identity_manager/identity_manager.h"
 #include "google_apis/gaia/gaia_auth_util.h"
@@ -63,7 +65,23 @@ bool IsFromUserInteraction(FeedbackSource source) {
       return false;
   }
 }
-#endif
+
+void OnLacrosActiveTabUrlFeteched(
+    Profile* profile,
+    chrome::FeedbackSource source,
+    const std::string& description_template,
+    const std::string& description_placeholder_text,
+    const std::string& category_tag,
+    const std::string& extra_diagnostics,
+    const base::Optional<GURL>& active_tab_url) {
+  GURL page_url;
+  if (active_tab_url)
+    page_url = *active_tab_url;
+  chrome::ShowFeedbackPage(page_url, profile, source, description_template,
+                           description_placeholder_text, category_tag,
+                           extra_diagnostics);
+}
+#endif  // defined(OS_CHROMEOS)
 
 // TODO(http://crbug.com/1132106): Include the following code only in
 // non-lacros builds after M87 beta when Feedback crosapi is available in all
@@ -125,9 +143,26 @@ void ShowFeedbackPage(const Browser* browser,
   }
 
   Profile* profile = GetFeedbackProfile(browser);
+
+#if defined(OS_CHROMEOS)
+  // When users invoke the feedback dialog by pressing alt-shift-i without
+  // an active ash window, we need to check if there is an active lacros window
+  // and show its Url in the feedback dialog if there is any.
+  if (!browser && crosapi::BrowserManager::Get()->IsRunning() &&
+      crosapi::BrowserManager::Get()->GetActiveTabUrlSupported()) {
+    crosapi::BrowserManager::Get()->GetActiveTabUrl(base::BindOnce(
+        &OnLacrosActiveTabUrlFeteched, profile, source, description_template,
+        description_placeholder_text, category_tag, extra_diagnostics));
+  } else {
+    ShowFeedbackPage(page_url, profile, source, description_template,
+                     description_placeholder_text, category_tag,
+                     extra_diagnostics);
+  }
+#else
   ShowFeedbackPage(page_url, profile, source, description_template,
                    description_placeholder_text, category_tag,
                    extra_diagnostics);
+#endif  // defined(OS_CHROMEOS)
 }
 
 void ShowFeedbackPage(const GURL& page_url,
@@ -169,7 +204,7 @@ void ShowFeedbackPage(const GURL& page_url,
   RequestFeedbackFlow(page_url, profile, source, description_template,
                       description_placeholder_text, category_tag,
                       extra_diagnostics);
-#endif  //  #if BUILDFLAG(IS_LACROS)
+#endif  //  BUILDFLAG(IS_LACROS)
 }
 
 }  // namespace chrome

chrome/browser/lacros/lacros_chrome_service_delegate_impl.cc

@@ -6,11 +6,14 @@
 
 #include "base/logging.h"
 #include "base/metrics/statistics_recorder.h"
+#include "chrome/browser/feedback/feedback_dialog_utils.h"
 #include "chrome/browser/lacros/feedback_util.h"
 #include "chrome/browser/lacros/system_logs/lacros_system_log_fetcher.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/profiles/profile_manager.h"
+#include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_commands.h"
+#include "chrome/browser/ui/browser_finder.h"
 #include "chrome/common/channel_info.h"
 #include "components/feedback/feedback_report.h"
 #include "components/feedback/feedback_util.h"
@@ -66,6 +69,21 @@ void LacrosChromeServiceDelegateImpl::GetHistograms(
   }
 }
 
+void LacrosChromeServiceDelegateImpl::GetActiveTabUrl(
+    GetActiveTabUrlCallback callback) {
+  Browser* browser = chrome::FindBrowserWithActiveWindow();
+  if (browser) {
+    GURL page_url;
+    page_url = chrome::GetTargetTabUrl(
+        browser->session_id(), browser->tab_strip_model()->active_index());
+    if (page_url.is_valid()) {
+      std::move(callback).Run(std::move(page_url));
+      return;
+    }
+  }
+  std::move(callback).Run(base::nullopt);
+}
+
 void LacrosChromeServiceDelegateImpl::OnSystemInformationReady(
     std::unique_ptr<system_logs::SystemLogsResponse> sys_info) {
   base::Value system_log_entries(base::Value::Type::DICTIONARY);

chrome/browser/lacros/lacros_chrome_service_delegate_impl.h

@@ -25,6 +25,7 @@ class LacrosChromeServiceDelegateImpl
   std::string GetChromeVersion() override;
   void GetFeedbackData(GetFeedbackDataCallback callback) override;
   void GetHistograms(GetHistogramsCallback callback) override;
+  void GetActiveTabUrl(GetActiveTabUrlCallback callback) override;
 
  private:
   void OnSystemInformationReady(

chromeos/crosapi/mojom/crosapi.mojom

@@ -15,6 +15,7 @@ import "mojo/public/mojom/base/big_string.mojom";
 import "mojo/public/mojom/base/token.mojom";
 import "mojo/public/mojom/base/values.mojom";
 import "services/device/public/mojom/hid.mojom";
+import "url/mojom/url.mojom";
 
 // LacrosInfo is a set of parameters passed to ash from lacros-chrome
 // upon lacros startup, which contains the lacros information such as version,
@@ -194,4 +195,8 @@ interface LacrosChromeService {
   // The histograms returned is zip compressed and is typically around 100KB.
   [MinVersion=7]
   GetHistograms@4() => (mojo_base.mojom.BigString compressed_histograms);
+
+  // Returns Url of the active tab from lacros if there is any.
+  [MinVersion=8]
+  GetActiveTabUrl@5() => (url.mojom.Url? url);
 };

chromeos/lacros/lacros_chrome_service_delegate.h

@@ -10,6 +10,8 @@
 #include "base/callback.h"
 #include "base/values.h"
 
+class GURL;
+
 namespace chromeos {
 
 // Interface to inject Chrome dependent behavior into LacrosChromeServiceImpl
@@ -34,6 +36,11 @@ class LacrosChromeServiceDelegate {
   using GetHistogramsCallback = base::OnceCallback<void(const std::string&)>;
   // Gets lacros histograms.
   virtual void GetHistograms(GetHistogramsCallback callback) = 0;
+
+  using GetActiveTabUrlCallback =
+      base::OnceCallback<void(const base::Optional<GURL>&)>;
+  // Gets Url of the active tab if there is any.
+  virtual void GetActiveTabUrl(GetActiveTabUrlCallback callback) = 0;
 };
 
 }  // namespace chromeos

chromeos/lacros/lacros_chrome_service_impl.cc

@@ -12,6 +12,7 @@
 #include "base/task/thread_pool.h"
 #include "chromeos/lacros/lacros_chrome_service_delegate.h"
 #include "mojo/public/cpp/bindings/pending_receiver.h"
+#include "url/gurl.h"
 
 namespace chromeos {
 namespace {
@@ -89,6 +90,13 @@ class LacrosChromeServiceNeverBlockingState
                        owner_, std::move(callback)));
   }
 
+  void GetActiveTabUrl(GetActiveTabUrlCallback callback) override {
+    owner_sequence_->PostTask(
+        FROM_HERE,
+        base::BindOnce(&LacrosChromeServiceImpl::GetActiveTabUrlAffineSequence,
+                       owner_, std::move(callback)));
+  }
+
   // Unlike most of other methods of this class, this is called on the
   // affined thread. Specifically, it is intended to be called before starting
   // the message pumping of the affined thread to pass the initialization
@@ -434,6 +442,12 @@ void LacrosChromeServiceImpl::GetHistogramsAffineSequence(
   delegate_->GetHistograms(std::move(callback));
 }
 
+void LacrosChromeServiceImpl::GetActiveTabUrlAffineSequence(
+    GetActiveTabUrlCallback callback) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(affine_sequence_checker_);
+  delegate_->GetActiveTabUrl(std::move(callback));
+}
+
 int LacrosChromeServiceImpl::AshChromeServiceVersion() {
   if (g_disable_all_crosapi_for_tests)
     return -1;

chromeos/lacros/lacros_chrome_service_impl.h

@@ -24,6 +24,8 @@
 #include "mojo/public/cpp/bindings/remote.h"
 #include "services/device/public/mojom/hid.mojom.h"
 
+class GURL;
+
 namespace chromeos {
 
 class LacrosChromeServiceDelegate;
@@ -198,6 +200,11 @@ class COMPONENT_EXPORT(CHROMEOS_LACROS) LacrosChromeServiceImpl {
   // Gets histograms on the affine sequence.
   void GetHistogramsAffineSequence(GetHistogramsCallback callback);
 
+  using GetActiveTabUrlCallback =
+      base::OnceCallback<void(const base::Optional<GURL>&)>;
+  // Gets Url of the active tab on the affine sequence.
+  void GetActiveTabUrlAffineSequence(GetActiveTabUrlCallback callback);
+
   // Returns ash's version of the AshChromeService mojo interface version. This
   // determines which interface methods are available. This is safe to call from
   // any sequence. This can only be called after BindReceiver().