Language detection: Do not cache inherited languages.

Caching inherited languages in language info was a premature optimization
which makes support for dynamic detection more complex.

This change simplifies the `Label` step. Label will no longer consider
language inheritance, inheritance is handled entirely within
AXNode::GetLanguage.

After later profiling we can consider caching the GetLanguage result
separately if that ended up being a concern.

R=dmazzoni,akihiroota

Change-Id: Idd6503fbc78b3b561cb1b418dca6378bedc7ea79
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1949807
Commit-Queue: Chris Hall <chrishall@chromium.org>
Reviewed-by: Dominic Mazzoni <dmazzoni@chromium.org>
Reviewed-by: Alice Boxhall <aboxhall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#722301}

ui/accessibility/ax_language_detection.cc

@@ -125,7 +125,6 @@ void AXLanguageDetectionManager::DetectLanguageForSubtreeInternal(
       // destruction of the containing node, this is due to us treating AXNode
       // as otherwise read-only and so we store any detected language
       // information on lang info.
-
       node->SetLanguageInfo(std::make_unique<AXLanguageInfo>());
       lang_info = node->GetLanguageInfo();
     } else {
@@ -137,6 +136,10 @@ void AXLanguageDetectionManager::DetectLanguageForSubtreeInternal(
     // concatenation and bubbling up results.
     auto text = node->GetStringAttribute(ax::mojom::StringAttribute::kName);
 
+    // FindTopNMostFreqLangs will pad the results with
+    // NNetLanguageIdentifier::kUnknown in order to reach the requested number
+    // of languages, this means we cannot rely on the results' length and we
+    // have to filter the results.
     const auto results = language_identifier_.FindTopNMostFreqLangs(
         text, kMaxDetectedLanguagesPerSpan);
 
@@ -184,6 +187,12 @@ void AXLanguageDetectionManager::LabelLanguageForSubtreeInternal(AXNode* node) {
   // If the lang_info->language is already set then we have no more work to do
   // for this node.
   if (lang_info && lang_info->language.empty()) {
+    // We assign the highest probability language which is both:
+    // 1) reliably detected for this node, and
+    // 2) one of the top (kMaxDetectedLanguagesPerPage) languages on this page.
+    //
+    // This helps guard against false positives for nodes which have noisy
+    // language detection results in isolation.
     for (const auto& lang : lang_info->detected_languages) {
       if (lang_info_stats_.CheckLanguageWithinTop(lang)) {
         lang_info->language = lang;
@@ -191,43 +200,20 @@ void AXLanguageDetectionManager::LabelLanguageForSubtreeInternal(AXNode* node) {
       }
     }
 
-    // TODO(chrishall): consider obeying the author declared lang tag in some
-    // cases, either based on proximity or based on common language detection
-    // error cases.
-
-    // If language is still empty then we failed to detect a language from
-    // this node, we will instead try construct a language from other sources
-    // including any lang attribute and any language from the parent tree.
+    // After attempting labelling we no longer need the detected results in
+    // LanguageInfo, as they have no future use.
     if (lang_info->language.empty()) {
-      const auto& lang_attr =
-          node->GetStringAttribute(ax::mojom::StringAttribute::kLanguage);
-      if (!lang_attr.empty()) {
-        lang_info->language = lang_attr;
-      } else {
-        // We call GetLanguage() on our parent which will return a detected
-        // language if it has one, otherwise it will search up the tree for a
-        // kLanguage attribute.
-        //
-        // This means that lang attributes are inherited indefinitely but
-        // detected language is only inherited one level.
-        //
-        // Currently we only attach detected language to text nodes, once we
-        // start attaching detected language on other nodes we need to rethink
-        // this. We may want to attach detected language information once we
-        // consider combining multiple smaller text nodes into one larger one.
-        //
-        // TODO(chrishall): reconsider detected language inheritance.
-        AXNode* parent = node->parent();
-        if (parent) {
-          const auto& parent_lang = parent->GetLanguage();
-          if (!parent_lang.empty()) {
-            lang_info->language = parent_lang;
-          }
-        }
-      }
+      // If no language was assigned then LanguageInfo as a whole can safely be
+      // destroyed.
+      node->ClearLanguageInfo();
+    } else {
+      // Otherwise, if we assigned a language then we need to keep
+      // LanguageInfo.language, but we can clear the detected results.
+      lang_info->detected_languages.clear();
     }
   }
 
+  // Recurse into children to continue labelling.
   for (AXNode* child : node->children()) {
     LabelLanguageForSubtreeInternal(child);
   }

ui/accessibility/ax_language_detection_unittest.cc

@@ -280,19 +280,22 @@ TEST(AXLanguageDetectionTest, LangAttrInheritanceFeatureFlagOn) {
 
   {
     AXNode* node3 = tree.GetFromId(3);
-    EXPECT_NE(node3->GetLanguageInfo(), nullptr);
+    // Inherited languages are not stored in lang info.
+    EXPECT_EQ(node3->GetLanguageInfo(), nullptr);
     EXPECT_EQ(node3->GetLanguage(), "en");
   }
 
   {
     AXNode* node4 = tree.GetFromId(4);
-    EXPECT_NE(node4->GetLanguageInfo(), nullptr);
+    // Inherited languages are not stored in lang info.
+    EXPECT_EQ(node4->GetLanguageInfo(), nullptr);
     EXPECT_EQ(node4->GetLanguage(), "fr");
   }
 
   {
     AXNode* node5 = tree.GetFromId(5);
-    EXPECT_NE(node5->GetLanguageInfo(), nullptr);
+    // Inherited languages are not stored in lang info.
+    EXPECT_EQ(node5->GetLanguageInfo(), nullptr);
     EXPECT_EQ(node5->GetLanguage(), "fr");
   }
 }
@@ -406,7 +409,8 @@ TEST(AXLanguageDetectionTest, LanguageDetectionBasic) {
   {
     AXNode* node5 = tree.GetFromId(5);
     EXPECT_TRUE(node5->IsText());
-    EXPECT_NE(node5->GetLanguageInfo(), nullptr);
+    // Inherited languages are not stored in lang info.
+    EXPECT_EQ(node5->GetLanguageInfo(), nullptr);
     EXPECT_EQ(node5->GetLanguage(), "en");
   }
 }
@@ -528,8 +532,9 @@ TEST(AXLanguageDetectionTest, LanguageDetectionDetectOnly) {
   {
     AXNode* node5 = tree.GetFromId(5);
     EXPECT_TRUE(node5->IsText());
+    // Inherited languages are not stored in lang info, but removal of empty
+    // LanguageInfo(s) occurs during labelling. So we expect this to be empty.
     ASSERT_NE(node5->GetLanguageInfo(), nullptr);
-    ASSERT_EQ(node5->GetLanguageInfo()->detected_languages.size(), (unsigned)0);
     EXPECT_TRUE(node5->GetLanguageInfo()->language.empty());
     EXPECT_EQ(node5->GetLanguage(), "fr");
   }

ui/accessibility/ax_node.cc

@@ -314,7 +314,7 @@ base::string16 AXNode::GetInheritedString16Attribute(
   return base::UTF8ToUTF16(GetInheritedStringAttribute(attribute));
 }
 
-AXLanguageInfo* AXNode::GetLanguageInfo() {
+AXLanguageInfo* AXNode::GetLanguageInfo() const {
   return language_info_.get();
 }
 
@@ -322,16 +322,27 @@ void AXNode::SetLanguageInfo(std::unique_ptr<AXLanguageInfo> lang_info) {
   language_info_ = std::move(lang_info);
 }
 
+void AXNode::ClearLanguageInfo() {
+  language_info_.reset();
+}
+
 std::string AXNode::GetLanguage() {
-  // If we have been labelled with language info then rely on that.
-  const AXLanguageInfo* lang_info = GetLanguageInfo();
-  if (lang_info && !lang_info->language.empty())
-    return lang_info->language;
-
-  // Otherwise fallback to kLanguage attribute.
-  const auto& lang_attr =
-      GetInheritedStringAttribute(ax::mojom::StringAttribute::kLanguage);
-  return lang_attr;
+  // Walk up tree considering both detected and author declared languages.
+  for (const AXNode* cur = this; cur; cur = cur->parent()) {
+    // If language detection has assigned a language then we prefer that.
+    const AXLanguageInfo* lang_info = cur->GetLanguageInfo();
+    if (lang_info && !lang_info->language.empty()) {
+      return lang_info->language;
+    }
+
+    // If the page author has declared a language attribute we fallback to that.
+    const AXNodeData& data = cur->data();
+    if (data.HasStringAttribute(ax::mojom::StringAttribute::kLanguage)) {
+      return data.GetStringAttribute(ax::mojom::StringAttribute::kLanguage);
+    }
+  }
+
+  return base::EmptyString();
 }
 
 std::ostream& operator<<(std::ostream& stream, const AXNode& node) {

ui/accessibility/ax_node.h

@@ -356,19 +356,21 @@ class AX_EXPORT AXNode final {
   bool IsCellOrHeaderOfARIATable() const;
   bool IsCellOrHeaderOfARIAGrid() const;
 
-  // Return an object containing information about the languages used.
+  // Return an object containing information about the languages detected on
+  // this node.
   // Callers should not retain this pointer, instead they should request it
   // every time it is needed.
   //
-  // Clients likely want to use GetLanguage instead.
-  //
   // Returns nullptr if the node has no language info.
-  AXLanguageInfo* GetLanguageInfo();
+  AXLanguageInfo* GetLanguageInfo() const;
 
-  // This should only be called by the LabelLanguageForSubtree and is used as
-  // part of the language detection feature.
+  // This should only be called by LabelLanguageForSubtree and is used as part
+  // of the language detection feature.
   void SetLanguageInfo(std::unique_ptr<AXLanguageInfo> lang_info);
 
+  // Destroy the language info for this node.
+  void ClearLanguageInfo();
+
   // Returns true if node has ignored state or ignored role.
   bool IsIgnored() const;