Start link generation with context menu.

Start link generation when context menu is requested for every new
selection. If link generation is requested, while there is one ongoing,
the previous one will be cancelled.

In this CL we also cancel the link generation from the client side if it
takes too long(timeout is different on different platforms).

This CL also fixes typo and moves preemptive generation feature from
chrome/ to components/ so that it can be used from blink/

Bug: 1177503
Change-Id: Iceacf5dfb88009bd6979a7f7e44b7725a3019724
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2716611
Reviewed-by: Chris Palmer <palmer@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: David Bokan <bokan@chromium.org>
Commit-Queue: Gayane Petrosyan <gayane@chromium.org>
Cr-Commit-Position: refs/heads/master@{#858349}

chrome/browser/about_flags.cc

@@ -123,6 +123,7 @@
 #include "components/security_state/core/security_state.h"
 #include "components/send_tab_to_self/features.h"
 #include "components/services/heap_profiling/public/cpp/switches.h"
+#include "components/shared_highlighting/core/common/features.h"
 #include "components/shared_highlighting/core/common/shared_highlighting_features.h"
 #include "components/signin/core/browser/dice_account_reconcilor_delegate.h"
 #include "components/signin/public/base/signin_buildflags.h"
@@ -3328,10 +3329,10 @@ const FeatureEntry kFeatureEntries[] = {
      flag_descriptions::kExtensionContentVerificationName,
      flag_descriptions::kExtensionContentVerificationDescription, kOsDesktop,
      MULTI_VALUE_TYPE(kExtensionContentVerificationChoices)},
-    {"preemtive-link-to-text-generation",
-     flag_descriptions::kPreemtiveLinkToTextGenerationName,
-     flag_descriptions::kPreemtiveLinkToTextGenerationDescription, kOsAll,
-     FEATURE_VALUE_TYPE(features::kPreemtiveLinkToTextGeneration)},
+    {"preemptive-link-to-text-generation",
+     flag_descriptions::kPreemptiveLinkToTextGenerationName,
+     flag_descriptions::kPreemptiveLinkToTextGenerationDescription, kOsAll,
+     FEATURE_VALUE_TYPE(features::kPreemptiveLinkToTextGeneration)},
 #if BUILDFLAG(IS_CHROMEOS_ASH)
     {"keyboard-based-display-arrangement-in-settings",
      flag_descriptions::kKeyboardBasedDisplayArrangementInSettingsName,

chrome/browser/flag-metadata.json

@@ -4190,7 +4190,7 @@
     "expiry_milestone": 100
   },
   {
-    "name": "preemtive-link-to-text-generation",
+    "name": "preemptive-link-to-text-generation",
     "owners": [ "cheickcisse@google.com" ],
     "expiry_milestone": 92
   },

chrome/browser/flag_descriptions.cc

@@ -2681,9 +2681,9 @@ const char kSharedHighlightingUseBlocklistDescription[] =
     "restrictions make it unlikely that a URL can be generated and actually "
     "work when shared.";
 
-const char kPreemtiveLinkToTextGenerationName[] =
+const char kPreemptiveLinkToTextGenerationName[] =
     "Preemptive generation of link to text";
-const char kPreemtiveLinkToTextGenerationDescription[] =
+const char kPreemptiveLinkToTextGenerationDescription[] =
     "Enables link to text to be generated in advance.";
 
 const char kDraw1PredictedPoint12Ms[] = "1 point 12ms ahead.";

chrome/browser/flag_descriptions.h

@@ -1558,8 +1558,8 @@ extern const char kEnableVulkanDescription[];
 extern const char kSharedHighlightingUseBlocklistName[];
 extern const char kSharedHighlightingUseBlocklistDescription[];
 
-extern const char kPreemtiveLinkToTextGenerationName[];
-extern const char kPreemtiveLinkToTextGenerationDescription[];
+extern const char kPreemptiveLinkToTextGenerationName[];
+extern const char kPreemptiveLinkToTextGenerationDescription[];
 
 extern const char kDraw1PredictedPoint12Ms[];
 extern const char kDraw2PredictedPoints6Ms[];

chrome/browser/flags/android/chrome_feature_list.cc

@@ -44,6 +44,7 @@
 #include "components/reading_list/features/reading_list_switches.h"
 #include "components/safe_browsing/core/features.h"
 #include "components/security_state/core/features.h"
+#include "components/shared_highlighting/core/common/features.h"
 #include "components/signin/public/base/account_consistency_method.h"
 #include "components/signin/public/base/signin_switches.h"
 #include "components/subresource_filter/core/browser/subresource_filter_features.h"
@@ -104,7 +105,7 @@ const base::Feature* kFeaturesExposedToJava[] = {
     &features::kMetricsSettingsAndroid,
     &features::kNetworkServiceInProcess,
     &features::kPredictivePrefetchingAllowedOnAllConnectionTypes,
-    &features::kPreemtiveLinkToTextGeneration,
+    &features::kPreemptiveLinkToTextGeneration,
     &features::kPrivacyReorderedAndroid,
     &features::kPrivacySandboxSettings,
     &features::kPrioritizeBootstrapTasks,

chrome/browser/flags/android/java/src/org/chromium/chrome/browser/flags/ChromeFeatureList.java

@@ -389,7 +389,8 @@ public static boolean getFieldTrialParamByFeatureAsBoolean(
     public static final String PHOTO_PICKER_VIDEO_SUPPORT = "PhotoPickerVideoSupport";
     public static final String PORTALS = "Portals";
     public static final String PORTALS_CROSS_ORIGIN = "PortalsCrossOrigin";
-    public static final String PREEMTIVE_LINK_TO_TEXT_GENERATION = "PreemtiveLinkToTextGeneration";
+    public static final String PREEMPTIVE_LINK_TO_TEXT_GENERATION =
+            "PreemptiveLinkToTextGeneration";
     public static final String PREDICTIVE_PREFETCHING_ALLOWED_ON_ALL_CONNECTION_TYPES =
             "PredictivePrefetchingAllowedOnAllConnectionTypes";
     public static final String PREFETCH_NOTIFICATION_SCHEDULING_INTEGRATION =

chrome/browser/renderer_context_menu/copy_link_to_text_menu_observer.cc

@@ -7,11 +7,12 @@
 #include <memory>
 
 #include "base/strings/utf_string_conversions.h"
+#include "base/time/time.h"
 #include "chrome/app/chrome_command_ids.h"
-#include "chrome/common/chrome_features.h"
 #include "chrome/grit/generated_resources.h"
 #include "components/renderer_context_menu/render_view_context_menu_proxy.h"
 #include "components/shared_highlighting/core/common/disabled_sites.h"
+#include "components/shared_highlighting/core/common/features.h"
 #include "components/shared_highlighting/core/common/shared_highlighting_metrics.h"
 #include "content/public/browser/context_menu_params.h"
 #include "content/public/browser/render_view_host.h"
@@ -23,6 +24,9 @@
 
 namespace {
 constexpr char kTextFragmentUrlClassifier[] = "#:~:text=";
+
+// Indicates how long context menu should wait for link generation result.
+constexpr base::TimeDelta kTimeoutMs = base::TimeDelta::FromMilliseconds(500);
 }
 
 // static
@@ -59,8 +63,14 @@ void CopyLinkToTextMenuObserver::InitMenu(
       IDC_CONTENT_CONTEXT_COPYLINKTOTEXT,
       l10n_util::GetStringUTF16(IDS_CONTENT_CONTEXT_COPYLINKTOTEXT));
 
-  if (ShouldPreemptivelyGenerateLink())
+  if (ShouldPreemptivelyGenerateLink()) {
     RequestLinkGeneration();
+    base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
+        FROM_HERE,
+        base::BindOnce(&CopyLinkToTextMenuObserver::Timeout,
+                       weak_ptr_factory_.GetWeakPtr()),
+        kTimeoutMs);
+  }
 }
 
 bool CopyLinkToTextMenuObserver::IsCommandIdSupported(int command_id) {
@@ -117,7 +127,8 @@ void CopyLinkToTextMenuObserver::OverrideGeneratedSelectorForTesting(
 }
 
 bool CopyLinkToTextMenuObserver::ShouldPreemptivelyGenerateLink() {
-  return base::FeatureList::IsEnabled(features::kPreemtiveLinkToTextGeneration);
+  return base::FeatureList::IsEnabled(
+      features::kPreemptiveLinkToTextGeneration);
 }
 
 void CopyLinkToTextMenuObserver::RequestLinkGeneration() {
@@ -152,11 +163,11 @@ void CopyLinkToTextMenuObserver::RequestLinkGeneration() {
 
   // Make a call to the renderer to generate a string that uniquely represents
   // the selected text and any context around the text to distinguish it from
-  // the rest of the contents. GenerateSelector will call a callback with
+  // the rest of the contents. Get will call a callback with
   // the generated string if it succeeds or an empty string if it fails.
   main_frame->GetRemoteInterfaces()->GetInterface(
       remote_.BindNewPipeAndPassReceiver());
-  remote_->GenerateSelector(base::BindOnce(
+  remote_->RequestSelector(base::BindOnce(
       &CopyLinkToTextMenuObserver::OnRequestLinkGenerationCompleted,
       weak_ptr_factory_.GetWeakPtr()));
 }
@@ -166,3 +177,13 @@ void CopyLinkToTextMenuObserver::CopyLinkToClipboard() {
                                 std::move(data_transfer_endpoint_));
   scw.WriteText(base::UTF8ToUTF16(generated_link_.value()));
 }
+
+void CopyLinkToTextMenuObserver::Timeout() {
+  DCHECK(ShouldPreemptivelyGenerateLink());
+  if (generated_link_.has_value())
+    return;
+  remote_->Cancel();
+  remote_.reset();
+  shared_highlighting::LogGenerateErrorTimeout();
+  OnRequestLinkGenerationCompleted(std::string());
+}

chrome/browser/renderer_context_menu/copy_link_to_text_menu_observer.h

@@ -54,12 +54,16 @@ class CopyLinkToTextMenuObserver : public RenderViewContextMenuObserver {
   // Copies the generated link to the user's clipboard.
   void CopyLinkToClipboard();
 
+  // Cancels link generation if we are still waiting for it.
+  void Timeout();
+
   mojo::Remote<blink::mojom::TextFragmentSelectorProducer> remote_;
   RenderViewContextMenuProxy* proxy_;
   GURL url_;
   base::Optional<std::string> generated_link_;
   base::Optional<std::string> generated_selector_for_testing_;
   std::unique_ptr<ui::DataTransferEndpoint> data_transfer_endpoint_;
+
   base::WeakPtrFactory<CopyLinkToTextMenuObserver> weak_ptr_factory_{this};
 };
 

chrome/browser/renderer_context_menu/copy_link_to_text_menu_observer_interactive_uitest.cc

@@ -11,10 +11,10 @@
 #include "chrome/app/chrome_command_ids.h"
 #include "chrome/browser/renderer_context_menu/mock_render_view_context_menu.h"
 #include "chrome/browser/ui/browser.h"
-#include "chrome/common/chrome_features.h"
 #include "chrome/test/base/chrome_test_utils.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/test/base/ui_test_utils.h"
+#include "components/shared_highlighting/core/common/features.h"
 #include "content/public/browser/context_menu_params.h"
 #include "content/public/test/browser_test.h"
 #include "net/dns/mock_host_resolver.h"
@@ -35,10 +35,10 @@ class CopyLinkToTextMenuObserverTest
 
     if (GetParam()) {
       scoped_feature_list.InitAndEnableFeature(
-          features::kPreemtiveLinkToTextGeneration);
+          features::kPreemptiveLinkToTextGeneration);
     } else {
       scoped_feature_list.InitAndDisableFeature(
-          features::kPreemtiveLinkToTextGeneration);
+          features::kPreemptiveLinkToTextGeneration);
     }
   }
 
@@ -75,7 +75,7 @@ class CopyLinkToTextMenuObserverTest
 
   bool ShouldPreemptivelyGenerateLink() {
     return base::FeatureList::IsEnabled(
-        features::kPreemtiveLinkToTextGeneration);
+        features::kPreemptiveLinkToTextGeneration);
   }
 
   ~CopyLinkToTextMenuObserverTest() override;

chrome/browser/share/android/java/src/org/chromium/chrome/browser/share/link_to_text/LinkToTextBridge.java

@@ -31,6 +31,10 @@ public static void logGenerateErrorBlockList() {
         LinkToTextBridgeJni.get().logGenerateErrorBlockList();
     }
 
+    public static void logGenerateErrorTimeout() {
+        LinkToTextBridgeJni.get().logGenerateErrorTimeout();
+    }
+
     // TODO(gayane): Update the name whenever |shared_highlighting::ShouldOfferLinkToText| updated
     // to moredescriptive name.
     public static boolean shouldOfferLinkToText(GURL url) {
@@ -44,6 +48,7 @@ interface Natives {
         void logGenerateErrorTabCrash();
         void logGenerateErrorIFrame();
         void logGenerateErrorBlockList();
+        void logGenerateErrorTimeout();
         boolean shouldOfferLinkToText(GURL url);
     }
 }

chrome/browser/share/android/java/src/org/chromium/chrome/browser/share/link_to_text/LinkToTextCoordinator.java

@@ -9,6 +9,7 @@
 
 import androidx.annotation.IntDef;
 
+import org.chromium.base.task.PostTask;
 import org.chromium.blink.mojom.TextFragmentSelectorProducer;
 import org.chromium.chrome.R;
 import org.chromium.chrome.browser.flags.ChromeFeatureList;
@@ -18,6 +19,7 @@
 import org.chromium.chrome.browser.tab.Tab;
 import org.chromium.chrome.browser.tab.TabHidingType;
 import org.chromium.components.browser_ui.share.ShareParams;
+import org.chromium.content_public.browser.UiThreadTaskTraits;
 import org.chromium.ui.widget.Toast;
 import org.chromium.url.GURL;
 
@@ -35,6 +37,7 @@ public class LinkToTextCoordinator extends EmptyTabObserver {
     private static final String SHARE_TEXT_TEMPLATE = "\"%s\"\n";
     private static final String TEXT_FRAGMENT_PREFIX = ":~:text=";
     private static final String INVALID_SELECTOR = "";
+    private static final long TIMEOUT_MS = 50;
     private final Context mContext;
     private final ChromeOptionShareCallback mChromeOptionShareCallback;
     private final String mVisibleUrl;
@@ -81,6 +84,7 @@ public LinkToTextCoordinator(ShareParams shareParams, Tab tab,
         mContext = null;
 
         requestSelector();
+        PostTask.postDelayedTask(UiThreadTaskTraits.DEFAULT, () -> timeout(), TIMEOUT_MS);
     }
 
     public ShareParams getShareParams(@LinkGeneration int linkGeneration) {
@@ -95,7 +99,7 @@ public ShareParams getShareParams(@LinkGeneration int linkGeneration) {
     public void onSelectorReady(String selector) {
         if (mCancelRequest) return;
 
-        if (ChromeFeatureList.isEnabled(ChromeFeatureList.PREEMTIVE_LINK_TO_TEXT_GENERATION)) {
+        if (ChromeFeatureList.isEnabled(ChromeFeatureList.PREEMPTIVE_LINK_TO_TEXT_GENERATION)) {
             mShareLinkParams = selector.isEmpty()
                     ? null
                     : new ShareParams
@@ -153,7 +157,7 @@ public void requestSelector() {
 
         mProducer = mTab.getWebContents().getMainFrame().getInterfaceToRendererFrame(
                 TextFragmentSelectorProducer.MANAGER);
-        mProducer.generateSelector(new TextFragmentSelectorProducer.GenerateSelectorResponse() {
+        mProducer.requestSelector(new TextFragmentSelectorProducer.RequestSelectorResponse() {
             @Override
             public void call(String selector) {
                 onSelectorReady(selector);
@@ -193,9 +197,18 @@ public void onCrash(Tab tab) {
     }
 
     private void cleanup() {
-        // TODO(gayane): Consider canceling request in renderer.
-        if (mProducer != null) mProducer.close();
+        if (mProducer != null) {
+            mProducer.cancel();
+            mProducer.close();
+        }
         mCancelRequest = true;
         mTab.removeObserver(this);
     }
+
+    private void timeout() {
+        if (!mCancelRequest) {
+            LinkToTextBridge.logGenerateErrorTimeout();
+            onSelectorReady(INVALID_SELECTOR);
+        }
+    }
 }

chrome/browser/share/android/java/src/org/chromium/chrome/browser/share/share_sheet/ChromeProvidedSharingOptionsProvider.java

@@ -238,7 +238,7 @@ private void initializeFirstPartyOptionsInOrder() {
         if (ChromeFeatureList.isEnabled(ChromeFeatureList.CHROME_SHARING_HUB_V15)
                 && ChromeFeatureList.isEnabled(ChromeFeatureList.CHROME_SHARE_HIGHLIGHTS_ANDROID)
                 && !ChromeFeatureList.isEnabled(
-                        ChromeFeatureList.PREEMTIVE_LINK_TO_TEXT_GENERATION)) {
+                        ChromeFeatureList.PREEMPTIVE_LINK_TO_TEXT_GENERATION)) {
             mOrderedFirstPartyOptions.add(createHighlightsFirstPartyOption());
         }
         if (ChromeFeatureList.isEnabled(ChromeFeatureList.CHROME_SHARE_QRCODE)

chrome/browser/share/android/java/src/org/chromium/chrome/browser/share/share_sheet/ShareSheetBottomSheetContent.java

@@ -224,7 +224,7 @@ private void createPreview(Set<Integer> contentTypes, String fileContentType) {
             fetchFavicon(mParams.getUrl());
         }
 
-        if (ChromeFeatureList.isEnabled(ChromeFeatureList.PREEMTIVE_LINK_TO_TEXT_GENERATION)
+        if (ChromeFeatureList.isEnabled(ChromeFeatureList.PREEMPTIVE_LINK_TO_TEXT_GENERATION)
                 && contentTypes.contains(ContentType.HIGHLIGHTED_TEXT)) {
             setLinkImageViewForPreview();
         }

chrome/browser/share/android/java/src/org/chromium/chrome/browser/share/share_sheet/ShareSheetCoordinator.java

@@ -169,7 +169,7 @@ public void showShareSheet(
      * @param state The state from {@link LinkGeneration} to which ShareParams should be updated.
      */
     void updateShareSheetForLinkToText(@LinkGeneration int state) {
-        if (!ChromeFeatureList.isEnabled(ChromeFeatureList.PREEMTIVE_LINK_TO_TEXT_GENERATION)
+        if (!ChromeFeatureList.isEnabled(ChromeFeatureList.PREEMPTIVE_LINK_TO_TEXT_GENERATION)
                 || mLinkToTextCoordinator == null) {
             return;
         }
@@ -202,7 +202,7 @@ private void updateShareSheet() {
      */
     public void showInitialShareSheet(
             ShareParams params, ChromeShareExtras chromeShareExtras, long shareStartTime) {
-        if (ChromeFeatureList.isEnabled(ChromeFeatureList.PREEMTIVE_LINK_TO_TEXT_GENERATION)
+        if (ChromeFeatureList.isEnabled(ChromeFeatureList.PREEMPTIVE_LINK_TO_TEXT_GENERATION)
                 && chromeShareExtras.isUserHighlightedText()) {
             String tabUrl =
                     mTabProvider.get().isInitialized() ? mTabProvider.get().getUrl().getSpec() : "";

chrome/browser/share/android/javatests/src/org/chromium/chrome/browser/share/link_to_text/LinkToTextCoordinatorTest.java

@@ -131,7 +131,7 @@ public void getUrlToShareTest_EmptySelector() {
 
     @Test
     @SmallTest
-    @Features.DisableFeatures({ChromeFeatureList.PREEMTIVE_LINK_TO_TEXT_GENERATION})
+    @Features.DisableFeatures({ChromeFeatureList.PREEMPTIVE_LINK_TO_TEXT_GENERATION})
     public void onSelectorReadyTest() {
         MockLinkToTextCoordinator coordinator = new MockLinkToTextCoordinator(
                 mAcivity, mTab, mShareCallback, VISIBLE_URL, SELECTED_TEXT);
@@ -142,7 +142,7 @@ public void onSelectorReadyTest() {
 
     @Test
     @SmallTest
-    @Features.DisableFeatures({ChromeFeatureList.PREEMTIVE_LINK_TO_TEXT_GENERATION})
+    @Features.DisableFeatures({ChromeFeatureList.PREEMPTIVE_LINK_TO_TEXT_GENERATION})
     public void onSelectorReadyTest_EmptySelector() {
         MockLinkToTextCoordinator coordinator = new MockLinkToTextCoordinator(
                 mAcivity, mTab, mShareCallback, VISIBLE_URL, SELECTED_TEXT);
@@ -153,8 +153,8 @@ public void onSelectorReadyTest_EmptySelector() {
 
     @Test
     @SmallTest
-    @Features.EnableFeatures({ChromeFeatureList.PREEMTIVE_LINK_TO_TEXT_GENERATION})
-    public void onSelectorReadyTest_PreemtiveLinkToTextGeneration() {
+    @Features.EnableFeatures({ChromeFeatureList.PREEMPTIVE_LINK_TO_TEXT_GENERATION})
+    public void onSelectorReadyTest_PreemptiveLinkToTextGeneration() {
         MockLinkToTextCoordinator coordinator = new MockLinkToTextCoordinator(
                 mAcivity, mTab, mShareCallback, VISIBLE_URL, SELECTED_TEXT);
         // OnSelectorReady should call back the share sheet.
@@ -164,8 +164,8 @@ public void onSelectorReadyTest_PreemtiveLinkToTextGeneration() {
 
     @Test
     @SmallTest
-    @Features.EnableFeatures({ChromeFeatureList.PREEMTIVE_LINK_TO_TEXT_GENERATION})
-    public void onSelectorReadyTest_EmptySelector_PreemtiveLinkToTextGeneration() {
+    @Features.EnableFeatures({ChromeFeatureList.PREEMPTIVE_LINK_TO_TEXT_GENERATION})
+    public void onSelectorReadyTest_EmptySelector_PreemptiveLinkToTextGeneration() {
         MockLinkToTextCoordinator coordinator = new MockLinkToTextCoordinator(
                 mAcivity, mTab, mShareCallback, VISIBLE_URL, SELECTED_TEXT);
         // OnSelectorReady should call back the share sheet.
@@ -175,8 +175,8 @@ public void onSelectorReadyTest_EmptySelector_PreemtiveLinkToTextGeneration() {
 
     @Test
     @SmallTest
-    @Features.EnableFeatures({ChromeFeatureList.PREEMTIVE_LINK_TO_TEXT_GENERATION})
-    public void showShareSheetTest_PreemtiveLinkToTextGeneration_LinkGeneration() {
+    @Features.EnableFeatures({ChromeFeatureList.PREEMPTIVE_LINK_TO_TEXT_GENERATION})
+    public void showShareSheetTest_PreemptiveLinkToTextGeneration_LinkGeneration() {
         ShareParams shareParams = new ShareParams.Builder(/*window=*/null, "", VISIBLE_URL)
                                           .setText(SELECTED_TEXT)
                                           .build();
@@ -189,8 +189,8 @@ public void showShareSheetTest_PreemtiveLinkToTextGeneration_LinkGeneration() {
 
     @Test
     @SmallTest
-    @Features.EnableFeatures({ChromeFeatureList.PREEMTIVE_LINK_TO_TEXT_GENERATION})
-    public void showShareSheetTest_EmptySelector_PreemtiveLinkToTextGeneration() {
+    @Features.EnableFeatures({ChromeFeatureList.PREEMPTIVE_LINK_TO_TEXT_GENERATION})
+    public void showShareSheetTest_EmptySelector_PreemptiveLinkToTextGeneration() {
         ShareParams shareParams = new ShareParams.Builder(/*window=*/null, "", VISIBLE_URL)
                                           .setText(SELECTED_TEXT)
                                           .build();