Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support separate lists for origin and target languages #10338

Merged
merged 16 commits into from Oct 8, 2021
Expand Up @@ -8,8 +8,10 @@
#include <memory>
#include <string>
#include <vector>

#include "brave/common/translate_network_constants.h"
#include "extensions/common/url_pattern.h"
#include "net/base/net_errors.h"

namespace {
const char kTranslateElementLibQuery[] = "client=te_lib";
Expand Down
Expand Up @@ -15,22 +15,20 @@
#include "net/traffic_annotation/network_traffic_annotation_test_helper.h"
#include "net/url_request/url_request_test_util.h"
#include "url/gurl.h"
// #include "url/url_constants.h"

namespace {

class BraveTranslateRedirectNetworkDelegateHelperTest
: public testing::Test {
public:
BraveTranslateRedirectNetworkDelegateHelperTest()
: thread_bundle_(content::TestBrowserThreadBundle::IO_MAINLOOP),
context_(new net::TestURLRequestContext(true)) {}
: context_(new net::TestURLRequestContext(true)) {}
~BraveTranslateRedirectNetworkDelegateHelperTest() override {}
void SetUp() override { context_->Init(); }
net::TestURLRequestContext* context() { return context_.get(); }

private:
content::TestBrowserThreadBundle thread_bundle_;
content::BrowserTaskEnvironment task_environment_;
std::unique_ptr<net::TestURLRequestContext> context_;
};

Expand All @@ -52,8 +50,8 @@ TEST_F(BraveTranslateRedirectNetworkDelegateHelperTest,

std::shared_ptr<brave::BraveRequestInfo> before_url_context(
new brave::BraveRequestInfo());
brave::BraveRequestInfo::FillCTXFromRequest(request.get(),
before_url_context);
before_url_context->initiator_url = GURL(kTranslateInitiatorURL);
before_url_context->request_url = url;
brave::ResponseCallback callback;

GURL expected_url(kBraveTranslateServer + path_string);
Expand Down Expand Up @@ -81,8 +79,7 @@ TEST_F(BraveTranslateRedirectNetworkDelegateHelperTest,

std::shared_ptr<brave::BraveRequestInfo> before_url_context(
new brave::BraveRequestInfo());
brave::BraveRequestInfo::FillCTXFromRequest(request.get(),
before_url_context);
before_url_context->request_url = url;
brave::ResponseCallback callback;

int ret = OnBeforeURLRequest_TranslateRedirectWork(callback,
Expand Down Expand Up @@ -110,8 +107,7 @@ TEST_F(BraveTranslateRedirectNetworkDelegateHelperTest,

std::shared_ptr<brave::BraveRequestInfo> before_url_context(
new brave::BraveRequestInfo());
brave::BraveRequestInfo::FillCTXFromRequest(request.get(),
before_url_context);
before_url_context->request_url = url;
brave::ResponseCallback callback;

GURL expected_url(kBraveTranslateServer + url.path());
Expand All @@ -137,8 +133,8 @@ TEST_F(BraveTranslateRedirectNetworkDelegateHelperTest,

std::shared_ptr<brave::BraveRequestInfo> before_url_context(
new brave::BraveRequestInfo());
brave::BraveRequestInfo::FillCTXFromRequest(request.get(),
before_url_context);
before_url_context->initiator_url = GURL(kTranslateInitiatorURL);
before_url_context->request_url = url;
brave::ResponseCallback callback;

GURL expected_url(kBraveTranslateEndpoint + std::string("?") + url.query());
Expand All @@ -162,8 +158,7 @@ TEST_F(BraveTranslateRedirectNetworkDelegateHelperTest,

std::shared_ptr<brave::BraveRequestInfo> before_url_context(
new brave::BraveRequestInfo());
brave::BraveRequestInfo::FillCTXFromRequest(request.get(),
before_url_context);
before_url_context->request_url = url;
brave::ResponseCallback callback;

GURL expected_url(kBraveTranslateServer + url.path());
Expand All @@ -186,8 +181,7 @@ TEST_F(BraveTranslateRedirectNetworkDelegateHelperTest,

std::shared_ptr<brave::BraveRequestInfo> before_url_context(
new brave::BraveRequestInfo());
brave::BraveRequestInfo::FillCTXFromRequest(request.get(),
before_url_context);
before_url_context->request_url = url;
brave::ResponseCallback callback;

int ret = OnBeforeURLRequest_TranslateRedirectWork(callback,
Expand All @@ -210,8 +204,7 @@ TEST_F(BraveTranslateRedirectNetworkDelegateHelperTest,

std::shared_ptr<brave::BraveRequestInfo> before_url_context(
new brave::BraveRequestInfo());
brave::BraveRequestInfo::FillCTXFromRequest(request.get(),
before_url_context);
before_url_context->request_url = url;
brave::ResponseCallback callback;

int ret = OnBeforeURLRequest_TranslateRedirectWork(callback,
Expand Down
3 changes: 3 additions & 0 deletions chromium_src/chrome/browser/ui/translate/DEPS
@@ -0,0 +1,3 @@
include_rules = [
"+../../../../../../chrome/browser/ui/translate",
]
@@ -0,0 +1,135 @@
/* Copyright (c) 2021 The Brave Authors. All rights reserved.
* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this file,
* You can obtain one at http://mozilla.org/MPL/2.0/. */

#include "chrome/browser/ui/translate/translate_bubble_model_impl.h"

#define TranslateBubbleModelImpl ChromiumTranslateBubbleModelImpl
#include "../../../../../../chrome/browser/ui/translate/translate_bubble_model_impl.cc"
#undef TranslateBubbleModelImpl

#include "base/containers/contains.h"
#include "brave/components/translate/core/browser/brave_translate_language_filter.h"

namespace {
const int kNoIndex = static_cast<int>(translate::TranslateUIDelegate::kNoIndex);
} // namespace

// A mapping between a chromium languages list in |ui_delegate| and a brave
// languages list (a limited subset of chromium list preassigned by |filter|).
// Note: mixin of int/size_t types is used because of chromium implementation.
class BraveLanguageMap {
atuchin-m marked this conversation as resolved.
Show resolved Hide resolved
public:
BraveLanguageMap(const translate::TranslateUIDelegate* ui_delegate,
base::RepeatingCallback<bool(const std::string&)> filter) {
int ui_index = 0;
for (size_t core_index = 0;
core_index < ui_delegate->GetNumberOfLanguages(); ++core_index) {
const auto lang = ui_delegate->GetLanguageCodeAt(core_index);
if (!filter.Run(lang))
continue;
to_core_index_[ui_index] = core_index;
to_ui_index_[core_index] = ui_index;
++ui_index;
}
DCHECK_EQ(to_ui_index_.size(), to_core_index_.size());
#if DCHECK_IS_ON()
for (int i = 0; i < static_cast<int>(to_core_index_.size()); ++i) {
const auto core_ind = to_core_index_[i];
DCHECK_EQ(to_ui_index_[to_core_index_[i]], i);
DCHECK_EQ(to_core_index_[to_ui_index_[core_ind]], core_ind);
}
#endif // DCHECK_IS_ON()
}

// Coverts index in brave list (index in [0, GetSize() - 1]) to a correspoding
// chromium index.
size_t ToCoreIndex(int index) const {
if (index == kNoIndex)
return translate::TranslateUIDelegate::kNoIndex;
const auto it = to_core_index_.find(index);
return it != to_core_index_.end()
? it->second
: translate::TranslateUIDelegate::kNoIndex;
}

// An inverse function to ToCoreIndex(). Coverts chromium index
// (form [0, ui_delegate->GetNumberOfLanguages()] to a
// correspoding index in brave list.
int FromCoreIndex(size_t index) const {
if (index == translate::TranslateUIDelegate::kNoIndex)
return kNoIndex;
const auto it = to_ui_index_.find(index);
return it != to_ui_index_.end() ? it->second : kNoIndex;
}

int GetSize() const { return to_core_index_.size(); }

private:
std::map<int, size_t> to_core_index_;
std::map<size_t, int> to_ui_index_;
};

TranslateBubbleModelImpl::TranslateBubbleModelImpl(
translate::TranslateStep step,
std::unique_ptr<translate::TranslateUIDelegate> ui_delegate)
: ChromiumTranslateBubbleModelImpl(step, std::move(ui_delegate)) {
source_language_map_ = std::make_unique<BraveLanguageMap>(
ui_delegate_.get(),
base::BindRepeating(&translate::IsSourceLanguageCodeSupported));
target_language_map_ = std::make_unique<BraveLanguageMap>(
ui_delegate_.get(),
base::BindRepeating(&translate::IsTargetLanguageCodeSupported));

// If the source language is unsupported then drop it to "und".
// Theoretically it isn't the same as creating ui_delegate with source_lang ==
// und, because |initial_source_language_index_| hasn't been updated. But in
// fact chromium doesn't use |initial_source_language_index_| at all.
if (!translate::IsSourceLanguageCodeSupported(
ui_delegate_->GetSourceLanguageCode())) {
ui_delegate_->UpdateSourceLanguageIndex(0u);
}
}

TranslateBubbleModelImpl::~TranslateBubbleModelImpl() = default;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can't we accomplish the same thing here by subclass/override TranslateUIDelegate
in TranslateBubbleView? Seems like that would be simpler and cleaner, no?

Copy link
Collaborator Author

@atuchin-m atuchin-m Oct 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea @bridiver, thanks.
Would you mind if I did it in the next PR?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow up is fine


int TranslateBubbleModelImpl::GetNumberOfSourceLanguages() const {
return source_language_map_->GetSize();
}

int TranslateBubbleModelImpl::GetNumberOfTargetLanguages() const {
return target_language_map_->GetSize();
}

std::u16string TranslateBubbleModelImpl::GetSourceLanguageNameAt(
int index) const {
return ui_delegate_->GetLanguageNameAt(
source_language_map_->ToCoreIndex(index));
}

std::u16string TranslateBubbleModelImpl::GetTargetLanguageNameAt(
int index) const {
return ui_delegate_->GetLanguageNameAt(
target_language_map_->ToCoreIndex(index));
}

int TranslateBubbleModelImpl::GetSourceLanguageIndex() const {
return source_language_map_->FromCoreIndex(
ui_delegate_->GetSourceLanguageIndex());
}

int TranslateBubbleModelImpl::GetTargetLanguageIndex() const {
return target_language_map_->FromCoreIndex(
ui_delegate_->GetTargetLanguageIndex());
}

void TranslateBubbleModelImpl::UpdateSourceLanguageIndex(int index) {
ui_delegate_->UpdateSourceLanguageIndex(
source_language_map_->ToCoreIndex(index));
}

void TranslateBubbleModelImpl::UpdateTargetLanguageIndex(int index) {
ui_delegate_->UpdateTargetLanguageIndex(
target_language_map_->ToCoreIndex(index));
}
@@ -0,0 +1,47 @@
/* Copyright (c) 2021 The Brave Authors. All rights reserved.
* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this file,
* You can obtain one at http://mozilla.org/MPL/2.0/. */

#ifndef BRAVE_CHROMIUM_SRC_CHROME_BROWSER_UI_TRANSLATE_TRANSLATE_BUBBLE_MODEL_IMPL_H_
#define BRAVE_CHROMIUM_SRC_CHROME_BROWSER_UI_TRANSLATE_TRANSLATE_BUBBLE_MODEL_IMPL_H_

class TranslateBubbleModelImpl;
using BraveTranslateBubbleModelImpl = TranslateBubbleModelImpl;

#define TranslateBubbleModelImpl ChromiumTranslateBubbleModelImpl
#define translate_executed_ \
translate_executed_; \
friend BraveTranslateBubbleModelImpl
#include "../../../../../../chrome/browser/ui/translate/translate_bubble_model_impl.h"
#undef translate_executed_
#undef TranslateBubbleModelImpl

class BraveLanguageMap;

// Brave customization of TranslateBubbleModelImpl that uses separated lists for
// source and target languages. Holds two mappings between chromium languages
// list in |ui_delegate| and brave lists.
class TranslateBubbleModelImpl : public ChromiumTranslateBubbleModelImpl {
public:
TranslateBubbleModelImpl(
translate::TranslateStep step,
std::unique_ptr<translate::TranslateUIDelegate> ui_delegate);

~TranslateBubbleModelImpl() override;

int GetNumberOfSourceLanguages() const override;
int GetNumberOfTargetLanguages() const override;
std::u16string GetSourceLanguageNameAt(int index) const override;
std::u16string GetTargetLanguageNameAt(int index) const override;
int GetSourceLanguageIndex() const override;
int GetTargetLanguageIndex() const override;
void UpdateSourceLanguageIndex(int index) override;
void UpdateTargetLanguageIndex(int index) override;

private:
std::unique_ptr<BraveLanguageMap> source_language_map_;
std::unique_ptr<BraveLanguageMap> target_language_map_;
};

#endif // BRAVE_CHROMIUM_SRC_CHROME_BROWSER_UI_TRANSLATE_TRANSLATE_BUBBLE_MODEL_IMPL_H_
3 changes: 3 additions & 0 deletions chromium_src/components/translate/core/browser/DEPS
Expand Up @@ -14,4 +14,7 @@ specific_include_rules = {
"translate_url_fetcher(\.cc|\.h)": [
"+brave/browser/translate/buildflags/buildflags.h",
],
"translate_manager(\.cc|\.h)": [
"+brave/browser/translate/buildflags/buildflags.h",
],
}
Expand Up @@ -5,9 +5,84 @@

#include "components/translate/core/browser/translate_manager.h"

#include "brave/browser/translate/buildflags/buildflags.h"
#include "brave/components/translate/core/browser/brave_translate_language_filter.h"
#include "components/translate/core/browser/translate_download_manager.h"
#include "components/translate/core/browser/translate_prefs.h"

namespace translate {

class BraveIsSupportedTargetLanguageProxy : public TranslateDownloadManager {
public:
#if BUILDFLAG(ENABLE_BRAVE_TRANSLATE_GO)
static bool IsSupportedLanguage(const std::string& lang) {
return IsTargetLanguageCodeSupported(lang);
}
#endif // BUILDFLAG(ENABLE_BRAVE_TRANSLATE_GO)
~BraveIsSupportedTargetLanguageProxy() override;
};

} // namespace translate
#define ASSERT_INSIDE_FUNCTION(fname) \
static_assert(base::StringPiece(__FUNCTION__) == fname, \
"should work only within " fname " function");

#define GetRecentTargetLanguage \
GetRecentTargetLanguage(); \
ASSERT_INSIDE_FUNCTION("GetTargetLanguage") \
using TranslateDownloadManager = BraveIsSupportedTargetLanguageProxy; \
void
#define HasAPIKeyConfigured BraveHasAPIKeyConfigured
#include "../../../../../../components/translate/core/browser/translate_manager.cc" // NOLINT
#define TranslateManager ChromiumTranslateManager
#include "../../../../../../components/translate/core/browser/translate_manager.cc"
#undef HasAPIKeyConfigured
#undef TranslateManager
#undef ASSERT_INSIDE_FUNCTION

namespace translate {

void TranslateManager::FilterIsTranslatePossible(
TranslateTriggerDecision* decision,
TranslatePrefs* translate_prefs,
const std::string& page_language_code,
const std::string& target_lang) {
ChromiumTranslateManager::FilterIsTranslatePossible(
decision, translate_prefs, page_language_code, target_lang);
#if BUILDFLAG(ENABLE_BRAVE_TRANSLATE_GO)
// The source language is not supported by Brave backend. Currently we allow a
// user to trigger a manual translation to have a chance to change the
// incorrectly recognized source language to the correct one.
if (!IsSourceLanguageCodeSupported(page_language_code)) {
atuchin-m marked this conversation as resolved.
Show resolved Hide resolved
decision->PreventAutoTranslate();
decision->PreventShowingUI();
decision->initiation_statuses.push_back(
TranslateBrowserMetrics::INITIATION_STATUS_LANGUAGE_IS_NOT_SUPPORTED);
decision->ranker_events.push_back(
metrics::TranslateEventProto::UNSUPPORTED_LANGUAGE);
GetActiveTranslateMetricsLogger()->LogTriggerDecision(
TriggerDecision::kDisabledUnsupportedLanguage);
}

// Just in case. In general a user can't trigger a translation to an
// unsupported target language. But some new entry points can be added so
// block translation in that case.
if (!IsTargetLanguageCodeSupported(target_lang)) {
decision->PreventAllTriggering();
decision->initiation_statuses.push_back(
TranslateBrowserMetrics::INITIATION_STATUS_LANGUAGE_IS_NOT_SUPPORTED);
decision->ranker_events.push_back(
metrics::TranslateEventProto::UNSUPPORTED_LANGUAGE);
GetActiveTranslateMetricsLogger()->LogTriggerDecision(
TriggerDecision::kDisabledUnsupportedLanguage);
}
#endif // BUILDFLAG(ENABLE_BRAVE_TRANSLATE_GO)
}

base::WeakPtr<TranslateManager> TranslateManager::GetWeakPtr() {
return AsWeakPtr();
}

} // namespace translate

namespace google_apis {

Expand Down