Skip to content

Commit

Permalink
Revert "[LanguageDetection] Pass language model to TranslateAgent."
Browse files Browse the repository at this point in the history
This reverts commit 5a849c9.

Reason for revert: https://bugs.chromium.org/p/chromium/issues/detail?id=1164560

Original change's description:
> [LanguageDetection] Pass language model to TranslateAgent.
>
> This change provides the file for the language detection model needed
> in the TranslateAgent.
>
> Bug: 1151422
> Change-Id: I24870758adf8b00ee378ca7ccc55bd45edb164d1
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2599580
> Reviewed-by: John Abd-El-Malek <jam@chromium.org>
> Reviewed-by: Scott Little <sclittle@chromium.org>
> Reviewed-by: Daniel Cheng <dcheng@chromium.org>
> Reviewed-by: Trevor  Perrier <perrier@chromium.org>
> Reviewed-by: Sophie Chang <sophiechang@chromium.org>
> Commit-Queue: Michael Crouse <mcrouse@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#841678}

TBR=dcheng@chromium.org,jam@chromium.org,sclittle@chromium.org,sophiechang@chromium.org,mcrouse@chromium.org,chromium-scoped@luci-project-accounts.iam.gserviceaccount.com,perrier@chromium.org

Change-Id: I7d33ec2acd918141ae310911550e3ffb86f336b8
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 1151422
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2618475
Reviewed-by: Dale Curtis <dalecurtis@chromium.org>
Commit-Queue: Dale Curtis <dalecurtis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#841723}
  • Loading branch information
dalecurtis authored and Chromium LUCI CQ committed Jan 9, 2021
1 parent 2ac5521 commit 6a15d50
Show file tree
Hide file tree
Showing 11 changed files with 10 additions and 137 deletions.
8 changes: 1 addition & 7 deletions chrome/browser/translate/chrome_translate_client.cc
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,7 @@
#include "chrome/browser/language/language_model_manager_factory.h"
#include "chrome/browser/language/url_language_histogram_factory.h"
#include "chrome/browser/profiles/profile.h"
#include "chrome/browser/profiles/profile_key.h"
#include "chrome/browser/translate/translate_accept_languages_factory.h"
#include "chrome/browser/translate/translate_model_service_factory.h"
#include "chrome/browser/translate/translate_ranker_factory.h"
#include "chrome/browser/translate/translate_service.h"
#include "chrome/browser/ui/translate/translate_bubble_factory.h"
Expand All @@ -33,7 +31,6 @@
#include "components/language/core/browser/language_model_manager.h"
#include "components/language/core/browser/pref_names.h"
#include "components/prefs/pref_service.h"
#include "components/translate/content/browser/translate_model_service.h"
#include "components/translate/core/browser/language_state.h"
#include "components/translate/core/browser/page_translated_details.h"
#include "components/translate/core/browser/translate_accept_languages.h"
Expand Down Expand Up @@ -102,10 +99,7 @@ ChromeTranslateClient::ChromeTranslateClient(content::WebContents* web_contents)
translate_driver_ = std::make_unique<translate::ContentTranslateDriver>(
&web_contents->GetController(),
UrlLanguageHistogramFactory::GetForBrowserContext(
web_contents->GetBrowserContext()),
TranslateModelServiceFactory::GetOrBuildForKey(
Profile::FromBrowserContext(web_contents->GetBrowserContext())
->GetProfileKey()));
web_contents->GetBrowserContext()));
}
translate_manager_ = std::make_unique<translate::TranslateManager>(
this,
Expand Down
40 changes: 1 addition & 39 deletions chrome/browser/translate/translate_model_service_browsertest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,8 @@
#include "components/optimization_guide/core/optimization_guide_features.h"
#include "components/optimization_guide/proto/models.pb.h"
#include "components/translate/core/common/translate_util.h"
#include "components/translate/core/language_detection/language_detection_model.h"
#include "content/public/test/browser_test.h"
#include "content/public/test/browser_test_utils.h"
#include "net/test/embedded_test_server/embedded_test_server.h"
#include "testing/gtest/include/gtest/gtest.h"

namespace {
Expand Down Expand Up @@ -121,28 +119,15 @@ class TranslateModelServiceBrowserTest
{});
}

void SetUp() override {
origin_server_ = std::make_unique<net::EmbeddedTestServer>(
net::EmbeddedTestServer::TYPE_HTTPS);
origin_server_->ServeFilesFromSourceDirectory("chrome/test/data/translate");
ASSERT_TRUE(origin_server_->Start());
english_url_ = origin_server_->GetURL("/english_page.html");
InProcessBrowserTest::SetUp();
}

~TranslateModelServiceBrowserTest() override = default;

translate::TranslateModelService* translate_model_service() {
return TranslateModelServiceFactory::GetOrBuildForKey(
browser()->profile()->GetProfileKey());
}

const GURL& english_url() const { return english_url_; }

private:
base::test::ScopedFeatureList scoped_feature_list_;
GURL english_url_;
std::unique_ptr<net::EmbeddedTestServer> origin_server_;
};

base::FilePath model_file_path() {
Expand Down Expand Up @@ -237,33 +222,10 @@ IN_PROC_BROWSER_TEST_F(TranslateModelServiceBrowserTest,
IN_PROC_BROWSER_TEST_F(TranslateModelServiceBrowserTest,
LanguageDetectionModelCreated) {
base::HistogramTester histogram_tester;
ui_test_utils::NavigateToURL(browser(), english_url());
ui_test_utils::NavigateToURL(browser(), GURL("https://test.com"));
RetryForHistogramUntilCountReached(
&histogram_tester,
"LanguageDetection.TFLiteModel.WasModelAvailableForDetection", 1);
histogram_tester.ExpectUniqueSample(
"LanguageDetection.TFLiteModel.WasModelAvailableForDetection", false, 1);
}

IN_PROC_BROWSER_TEST_F(TranslateModelServiceBrowserTest,
LanguageDetectionModelAvailableForDetection) {
base::HistogramTester histogram_tester;
OptimizationGuideKeyedServiceFactory::GetForProfile(browser()->profile())
->OverrideTargetModelFileForTesting(
optimization_guide::proto::OPTIMIZATION_TARGET_LANGUAGE_DETECTION,
model_file_path());
RetryForHistogramUntilCountReached(
&histogram_tester,
"LanguageDetection.TFLiteModel.LanguageDetectionModelState", 1);
histogram_tester.ExpectUniqueSample(
"LanguageDetection.TFLiteModel.LanguageDetectionModelState",
translate::LanguageDetectionModelState::kModelFileValidAndMemoryMapped,
1);

ui_test_utils::NavigateToURL(browser(), english_url());
RetryForHistogramUntilCountReached(
&histogram_tester,
"LanguageDetection.TFLiteModel.WasModelAvailableForDetection", 1);
histogram_tester.ExpectBucketCount(
"LanguageDetection.TFLiteModel.WasModelAvailableForDetection", true, 1);
}
2 changes: 0 additions & 2 deletions chrome/renderer/chrome_render_frame_observer_browsertest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,6 @@ class FakeContentTranslateDriver
called_new_page_ = true;
page_level_translation_critiera_met_ = page_level_translation_critiera_met;
}
void GetLanguageDetectionModel(
GetLanguageDetectionModelCallback callback) override {}

bool called_new_page_ = false;
bool page_level_translation_critiera_met_ = false;
Expand Down
2 changes: 0 additions & 2 deletions chrome/renderer/translate/translate_agent_browsertest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,6 @@ class FakeContentTranslateDriver
details_ = details;
page_level_translation_critiera_met_ = page_level_translation_critiera_met;
}
void GetLanguageDetectionModel(
GetLanguageDetectionModelCallback callback) override {}

void ResetNewPageValues() {
called_new_page_ = false;
Expand Down
25 changes: 2 additions & 23 deletions components/translate/content/browser/content_translate_driver.cc
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
#include "components/google/core/common/google_util.h"
#include "components/language/core/browser/url_language_histogram.h"
#include "components/translate/content/browser/content_record_page_language.h"
#include "components/translate/content/browser/translate_model_service.h"
#include "components/translate/core/browser/translate_download_manager.h"
#include "components/translate/core/browser/translate_manager.h"
#include "components/translate/core/browser/translate_metrics_logger.h"
Expand Down Expand Up @@ -58,15 +57,13 @@ const base::Feature kAutoHrefTranslateAllOrigins{

ContentTranslateDriver::ContentTranslateDriver(
content::NavigationController* nav_controller,
language::UrlLanguageHistogram* url_language_histogram,
translate::TranslateModelService* translate_model_service)
language::UrlLanguageHistogram* url_language_histogram)
: content::WebContentsObserver(nav_controller->GetWebContents()),
navigation_controller_(nav_controller),
translate_manager_(nullptr),
max_reload_check_attempts_(kMaxTranslateLoadCheckAttempts),
next_page_seq_no_(0),
language_histogram_(url_language_histogram),
translate_model_service_(translate_model_service) {
language_histogram_(url_language_histogram) {
DCHECK(navigation_controller_);
}

Expand Down Expand Up @@ -342,22 +339,4 @@ void ContentTranslateDriver::OnPageTranslated(
observer.OnPageTranslated(original_lang, translated_lang, error_type);
}

void ContentTranslateDriver::GetLanguageDetectionModel(
GetLanguageDetectionModelCallback callback) {
if (!translate_model_service_) {
std::move(callback).Run(base::File());
return;
}
translate_model_service_->GetLanguageDetectionModelFile(
base::BindOnce(&ContentTranslateDriver::OnLanguageDetectionModelFile,
weak_pointer_factory_.GetWeakPtr(), std::move(callback)));
}

void ContentTranslateDriver::OnLanguageDetectionModelFile(
GetLanguageDetectionModelCallback callback,
base::File model_file) {
DCHECK(model_file.IsValid());
std::move(callback).Run(std::move(model_file));
}

} // namespace translate
21 changes: 3 additions & 18 deletions components/translate/content/browser/content_translate_driver.h
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ namespace translate {

struct LanguageDetectionDetails;
class TranslateManager;
class TranslateModelService;

// Content implementation of TranslateDriver.
class ContentTranslateDriver : public TranslateDriver,
Expand All @@ -56,9 +55,9 @@ class ContentTranslateDriver : public TranslateDriver,
}
};

ContentTranslateDriver(content::NavigationController* nav_controller,
language::UrlLanguageHistogram* url_language_histogram,
TranslateModelService* translate_model_service);
ContentTranslateDriver(
content::NavigationController* nav_controller,
language::UrlLanguageHistogram* url_language_histogram);
~ContentTranslateDriver() override;

// Adds or removes observers.
Expand Down Expand Up @@ -109,17 +108,12 @@ class ContentTranslateDriver : public TranslateDriver,
// Adds a receiver in |receivers_| for the passed |receiver|.
void AddReceiver(
mojo::PendingReceiver<translate::mojom::ContentTranslateDriver> receiver);

// Called when a page has been loaded and can be potentially translated.
void RegisterPage(
mojo::PendingRemote<translate::mojom::TranslateAgent> translate_agent,
const translate::LanguageDetectionDetails& details,
bool page_level_translation_critiera_met) override;

// translate::mojom::ContentTranslateDriver implementation:
void GetLanguageDetectionModel(
GetLanguageDetectionModelCallback callback) override;

protected:
const base::ObserverList<TranslationObserver, true>& translation_observers()
const {
Expand All @@ -137,11 +131,6 @@ class ContentTranslateDriver : public TranslateDriver,
private:
void OnPageAway(int page_seq_no);

// Runs the provided callback with the loaded model file
// to pass it to the connected translate agent.
void OnLanguageDetectionModelFile(GetLanguageDetectionModelCallback callback,
base::File model_file);

// The navigation controller of the tab we are associated with.
content::NavigationController* navigation_controller_;

Expand Down Expand Up @@ -173,10 +162,6 @@ class ContentTranslateDriver : public TranslateDriver,
// page language is determined.
base::TimeTicks finish_navigation_time_;

// The service that provides the model files needed for translate. Not owned
// but guaranteed to outlive |this|.
TranslateModelService* const translate_model_service_;

base::WeakPtrFactory<ContentTranslateDriver> weak_pointer_factory_{this};

DISALLOW_COPY_AND_ASSIGN(ContentTranslateDriver);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -130,9 +130,7 @@ void PerFrameContentTranslateDriver::PendingRequestStats::Report() {
PerFrameContentTranslateDriver::PerFrameContentTranslateDriver(
content::NavigationController* nav_controller,
language::UrlLanguageHistogram* url_language_histogram)
: ContentTranslateDriver(nav_controller,
url_language_histogram,
/*translate_model_service=*/nullptr) {}
: ContentTranslateDriver(nav_controller, url_language_histogram) {}

PerFrameContentTranslateDriver::~PerFrameContentTranslateDriver() = default;

Expand Down
7 changes: 0 additions & 7 deletions components/translate/content/common/translate.mojom
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@

module translate.mojom;

import "mojo/public/mojom/base/file.mojom";
import "mojo/public/mojom/base/time.mojom";
import "mojo/public/mojom/base/string16.mojom";
import "url/mojom/url.mojom";
Expand Down Expand Up @@ -79,10 +78,4 @@ interface ContentTranslateDriver {
// and the language for it has been determined.
RegisterPage(pending_remote<TranslateAgent> translate_agent,
LanguageDetectionDetails details, bool translation_critiera_met);

// Request that the language detection model being loaded and returned
// for use by the TranslateAgent.
GetLanguageDetectionModel()
=> (mojo_base.mojom.File? model_file);

};
28 changes: 1 addition & 27 deletions components/translate/content/renderer/translate_agent.cc
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@
#include "third_party/blink/public/web/web_local_frame.h"
#include "third_party/blink/public/web/web_script_source.h"
#include "url/gurl.h"
#include "url/url_constants.h"
#include "v8/include/v8.h"

using blink::WebDocument;
Expand Down Expand Up @@ -83,19 +82,6 @@ TranslateAgent::TranslateAgent(content::RenderFrame* render_frame,
extension_scheme_(extension_scheme) {
translate_task_runner_ = this->render_frame()->GetTaskRunner(
blink::TaskType::kInternalTranslation);

if (translate::IsTFLiteLanguageDetectionEnabled()) {
translate::LanguageDetectionModel& language_detection_model =
GetLanguageDetectionModel();
if (!language_detection_model.IsAvailable()) {
// TODO(crbug.com/1160948): Consider tracking if another agent associated
// with the same LanguageDetectionModel has already requested a model be
// provided by the translate host.
GetTranslateHandler()->GetLanguageDetectionModel(
base::BindOnce(&TranslateAgent::UpdateLanguageDetectionModel,
weak_pointer_factory_.GetWeakPtr()));
}
}
}

TranslateAgent::~TranslateAgent() {}
Expand All @@ -113,7 +99,7 @@ void TranslateAgent::PageCaptured(const base::string16& contents) {
// original intent of http-equiv to be an equivalent) with the former
// being the language of the document and the latter being the
// language of the intended audience (a distinction really only
// relevant for things like language textbooks). This distinction
// relevant for things like langauge textbooks). This distinction
// shouldn't affect translation.
WebLocalFrame* main_frame = render_frame()->GetWebFrame();
if (!main_frame)
Expand All @@ -129,12 +115,6 @@ void TranslateAgent::PageCaptured(const base::string16& contents) {

std::string language;
if (translate::IsTFLiteLanguageDetectionEnabled()) {
if (!document.Url().ProtocolIs(url::kHttpsScheme) &&
!document.Url().ProtocolIs(url::kHttpScheme)) {
// TFLite-based language detection only supports HTTP/HTTPS pages.
// Others should be ignored, for example the New Tab Page.
return;
}
translate::LanguageDetectionModel& language_detection_model =
GetLanguageDetectionModel();
bool is_available = language_detection_model.IsAvailable();
Expand Down Expand Up @@ -522,10 +502,4 @@ std::string TranslateAgent::BuildTranslationScript(
base::GetQuotedJSONString(target_lang) + ")";
}

void TranslateAgent::UpdateLanguageDetectionModel(base::File model_file) {
translate::LanguageDetectionModel& language_detection_model =
GetLanguageDetectionModel();
language_detection_model.UpdateWithFile(std::move(model_file));
}

} // namespace translate
7 changes: 0 additions & 7 deletions components/translate/content/renderer/translate_agent.h
Original file line number Diff line number Diff line change
Expand Up @@ -154,10 +154,6 @@ class TranslateAgent : public content::RenderFrameObserver,
// if the page is being closed.
blink::WebLocalFrame* GetMainFrame();

// Called by the translate host when a new language detection model file
// has been loaded and is available.
void UpdateLanguageDetectionModel(base::File model_file);

// The states associated with the current translation.
TranslateFrameCallback translate_callback_pending_;
std::string source_lang_;
Expand Down Expand Up @@ -188,9 +184,6 @@ class TranslateAgent : public content::RenderFrameObserver,
// Method factory used to make calls to TranslatePageImpl.
base::WeakPtrFactory<TranslateAgent> weak_method_factory_{this};

// Weak pointer factory used to provide references to the translate host.
base::WeakPtrFactory<TranslateAgent> weak_pointer_factory_{this};

DISALLOW_COPY_AND_ASSIGN(TranslateAgent);
};

Expand Down
3 changes: 1 addition & 2 deletions weblayer/browser/translate_client_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,7 @@ std::unique_ptr<translate::TranslatePrefs> CreateTranslatePrefs(
TranslateClientImpl::TranslateClientImpl(content::WebContents* web_contents)
: content::WebContentsObserver(web_contents),
translate_driver_(&web_contents->GetController(),
/*url_language_histogram=*/nullptr,
/*translate_model_service=*/nullptr),
/*url_language_histogram=*/nullptr),
translate_manager_(new translate::TranslateManager(
this,
TranslateRankerFactory::GetForBrowserContext(
Expand Down

0 comments on commit 6a15d50

Please sign in to comment.