Commit efc0b7c3 authored by Megan Jablonski's avatar Megan Jablonski Committed by Chromium LUCI CQ

Rename page_needs_translation to can_offer_translation

page_needs_translation() tells us if translation is allowed, not if the
page needs translation. It should be renamed to
page_level_translation_critiera_met. We can offer translation if the
page doesn't have notranslate and the determined language isn't empty.

Bug: 1152884
Change-Id: I647b17f4775787d04f34ee2076b32d53b330af55
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2561293
Commit-Queue: Megan Jablonski <megjablon@chromium.org>
Reviewed-by: default avatarColin Blundell <blundell@chromium.org>
Reviewed-by: default avatarDaniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#832683}
parent d2b1fb18
...@@ -27,9 +27,8 @@ namespace { ...@@ -27,9 +27,8 @@ namespace {
class FakeContentTranslateDriver class FakeContentTranslateDriver
: public translate::mojom::ContentTranslateDriver { : public translate::mojom::ContentTranslateDriver {
public: public:
FakeContentTranslateDriver() FakeContentTranslateDriver() = default;
: called_new_page_(false), page_needs_translation_(false) {} ~FakeContentTranslateDriver() override = default;
~FakeContentTranslateDriver() override {}
void BindHandle(mojo::ScopedMessagePipeHandle handle) { void BindHandle(mojo::ScopedMessagePipeHandle handle) {
receivers_.Add( receivers_.Add(
...@@ -41,13 +40,13 @@ class FakeContentTranslateDriver ...@@ -41,13 +40,13 @@ class FakeContentTranslateDriver
void RegisterPage( void RegisterPage(
mojo::PendingRemote<translate::mojom::TranslateAgent> translate_agent, mojo::PendingRemote<translate::mojom::TranslateAgent> translate_agent,
const translate::LanguageDetectionDetails& details, const translate::LanguageDetectionDetails& details,
bool page_needs_translation) override { bool page_level_translation_critiera_met) override {
called_new_page_ = true; called_new_page_ = true;
page_needs_translation_ = page_needs_translation; page_level_translation_critiera_met_ = page_level_translation_critiera_met;
} }
bool called_new_page_; bool called_new_page_ = false;
bool page_needs_translation_; bool page_level_translation_critiera_met_ = false;
private: private:
mojo::ReceiverSet<translate::mojom::ContentTranslateDriver> receivers_; mojo::ReceiverSet<translate::mojom::ContentTranslateDriver> receivers_;
...@@ -95,7 +94,7 @@ TEST_F(ChromeRenderFrameObserverTest, SkipCapturingSubFrames) { ...@@ -95,7 +94,7 @@ TEST_F(ChromeRenderFrameObserverTest, SkipCapturingSubFrames) {
base::RunLoop().RunUntilIdle(); base::RunLoop().RunUntilIdle();
ASSERT_TRUE(fake_translate_driver_.called_new_page_); ASSERT_TRUE(fake_translate_driver_.called_new_page_);
EXPECT_TRUE(fake_translate_driver_.page_needs_translation_) EXPECT_TRUE(fake_translate_driver_.page_level_translation_critiera_met_)
<< "Page should be translatable."; << "Page should be translatable.";
// Should have 2 samples: one for preliminary capture, one for final capture. // Should have 2 samples: one for preliminary capture, one for final capture.
// If there are more, then subframes are being captured more than once. // If there are more, then subframes are being captured more than once.
......
...@@ -33,9 +33,8 @@ namespace { ...@@ -33,9 +33,8 @@ namespace {
class FakeContentTranslateDriver class FakeContentTranslateDriver
: public translate::mojom::ContentTranslateDriver { : public translate::mojom::ContentTranslateDriver {
public: public:
FakeContentTranslateDriver() FakeContentTranslateDriver() = default;
: called_new_page_(false), page_needs_translation_(false) {} ~FakeContentTranslateDriver() override = default;
~FakeContentTranslateDriver() override {}
void BindHandle(mojo::ScopedMessagePipeHandle handle) { void BindHandle(mojo::ScopedMessagePipeHandle handle) {
receivers_.Add( receivers_.Add(
...@@ -47,21 +46,21 @@ class FakeContentTranslateDriver ...@@ -47,21 +46,21 @@ class FakeContentTranslateDriver
void RegisterPage( void RegisterPage(
mojo::PendingRemote<translate::mojom::TranslateAgent> translate_agent, mojo::PendingRemote<translate::mojom::TranslateAgent> translate_agent,
const translate::LanguageDetectionDetails& details, const translate::LanguageDetectionDetails& details,
bool page_needs_translation) override { bool page_level_translation_critiera_met) override {
called_new_page_ = true; called_new_page_ = true;
details_ = details; details_ = details;
page_needs_translation_ = page_needs_translation; page_level_translation_critiera_met_ = page_level_translation_critiera_met;
} }
void ResetNewPageValues() { void ResetNewPageValues() {
called_new_page_ = false; called_new_page_ = false;
details_ = base::nullopt; details_ = base::nullopt;
page_needs_translation_ = false; page_level_translation_critiera_met_ = false;
} }
bool called_new_page_; bool called_new_page_ = false;
bool page_level_translation_critiera_met_ = false;
base::Optional<translate::LanguageDetectionDetails> details_; base::Optional<translate::LanguageDetectionDetails> details_;
bool page_needs_translation_;
private: private:
mojo::ReceiverSet<translate::mojom::ContentTranslateDriver> receivers_; mojo::ReceiverSet<translate::mojom::ContentTranslateDriver> receivers_;
...@@ -411,7 +410,7 @@ TEST_F(TranslateAgentBrowserTest, TranslatablePage) { ...@@ -411,7 +410,7 @@ TEST_F(TranslateAgentBrowserTest, TranslatablePage) {
base::RunLoop().RunUntilIdle(); base::RunLoop().RunUntilIdle();
ASSERT_TRUE(fake_translate_driver_.called_new_page_); ASSERT_TRUE(fake_translate_driver_.called_new_page_);
EXPECT_TRUE(fake_translate_driver_.page_needs_translation_) EXPECT_TRUE(fake_translate_driver_.page_level_translation_critiera_met_)
<< "Page should be translatable."; << "Page should be translatable.";
fake_translate_driver_.ResetNewPageValues(); fake_translate_driver_.ResetNewPageValues();
...@@ -422,7 +421,7 @@ TEST_F(TranslateAgentBrowserTest, TranslatablePage) { ...@@ -422,7 +421,7 @@ TEST_F(TranslateAgentBrowserTest, TranslatablePage) {
base::RunLoop().RunUntilIdle(); base::RunLoop().RunUntilIdle();
ASSERT_TRUE(fake_translate_driver_.called_new_page_); ASSERT_TRUE(fake_translate_driver_.called_new_page_);
EXPECT_FALSE(fake_translate_driver_.page_needs_translation_) EXPECT_FALSE(fake_translate_driver_.page_level_translation_critiera_met_)
<< "Page should not be translatable."; << "Page should not be translatable.";
fake_translate_driver_.ResetNewPageValues(); fake_translate_driver_.ResetNewPageValues();
...@@ -433,7 +432,7 @@ TEST_F(TranslateAgentBrowserTest, TranslatablePage) { ...@@ -433,7 +432,7 @@ TEST_F(TranslateAgentBrowserTest, TranslatablePage) {
base::RunLoop().RunUntilIdle(); base::RunLoop().RunUntilIdle();
ASSERT_TRUE(fake_translate_driver_.called_new_page_); ASSERT_TRUE(fake_translate_driver_.called_new_page_);
EXPECT_FALSE(fake_translate_driver_.page_needs_translation_) EXPECT_FALSE(fake_translate_driver_.page_level_translation_critiera_met_)
<< "Page should not be translatable."; << "Page should not be translatable.";
} }
......
...@@ -227,7 +227,8 @@ void ContentTranslateDriver::NavigationEntryCommitted( ...@@ -227,7 +227,8 @@ void ContentTranslateDriver::NavigationEntryCommitted(
return; return;
} }
if (!translate_manager_->GetLanguageState()->page_needs_translation()) if (!translate_manager_->GetLanguageState()
->page_level_translation_critiera_met())
return; return;
// Note that we delay it as the ordering of the processing of this callback // Note that we delay it as the ordering of the processing of this callback
...@@ -286,7 +287,7 @@ void ContentTranslateDriver::AddReceiver( ...@@ -286,7 +287,7 @@ void ContentTranslateDriver::AddReceiver(
void ContentTranslateDriver::RegisterPage( void ContentTranslateDriver::RegisterPage(
mojo::PendingRemote<translate::mojom::TranslateAgent> translate_agent, mojo::PendingRemote<translate::mojom::TranslateAgent> translate_agent,
const translate::LanguageDetectionDetails& details, const translate::LanguageDetectionDetails& details,
const bool page_needs_translation) { const bool page_level_translation_critiera_met) {
base::TimeTicks language_determined_time = base::TimeTicks::Now(); base::TimeTicks language_determined_time = base::TimeTicks::Now();
ReportLanguageDeterminedDuration(finish_navigation_time_, ReportLanguageDeterminedDuration(finish_navigation_time_,
language_determined_time); language_determined_time);
...@@ -303,7 +304,7 @@ void ContentTranslateDriver::RegisterPage( ...@@ -303,7 +304,7 @@ void ContentTranslateDriver::RegisterPage(
translate_manager_->set_current_seq_no(next_page_seq_no_); translate_manager_->set_current_seq_no(next_page_seq_no_);
translate_manager_->GetLanguageState()->LanguageDetermined( translate_manager_->GetLanguageState()->LanguageDetermined(
details.adopted_language, page_needs_translation); details.adopted_language, page_level_translation_critiera_met);
if (web_contents()) { if (web_contents()) {
translate_manager_->InitiateTranslation(details.adopted_language); translate_manager_->InitiateTranslation(details.adopted_language);
......
...@@ -24,7 +24,7 @@ ...@@ -24,7 +24,7 @@
namespace content { namespace content {
class NavigationController; class NavigationController;
class WebContents; class WebContents;
} } // namespace content
namespace language { namespace language {
class UrlLanguageHistogram; class UrlLanguageHistogram;
...@@ -120,7 +120,7 @@ class ContentTranslateDriver : public TranslateDriver, ...@@ -120,7 +120,7 @@ class ContentTranslateDriver : public TranslateDriver,
void RegisterPage( void RegisterPage(
mojo::PendingRemote<translate::mojom::TranslateAgent> translate_agent, mojo::PendingRemote<translate::mojom::TranslateAgent> translate_agent,
const translate::LanguageDetectionDetails& details, const translate::LanguageDetectionDetails& details,
bool page_needs_translation) override; bool page_level_translation_critiera_met) override;
protected: protected:
const base::ObserverList<Observer, true>::Unchecked& observer_list() const { const base::ObserverList<Observer, true>::Unchecked& observer_list() const {
......
...@@ -250,7 +250,9 @@ void PerFrameContentTranslateDriver::NavigationEntryCommitted( ...@@ -250,7 +250,9 @@ void PerFrameContentTranslateDriver::NavigationEntryCommitted(
return; return;
} }
if (!translate_manager()->GetLanguageState()->page_needs_translation()) if (!translate_manager()
->GetLanguageState()
->page_level_translation_critiera_met())
return; return;
// Note that we delay it as the ordering of the processing of this callback // Note that we delay it as the ordering of the processing of this callback
...@@ -345,7 +347,7 @@ void PerFrameContentTranslateDriver::StartLanguageDetection() { ...@@ -345,7 +347,7 @@ void PerFrameContentTranslateDriver::StartLanguageDetection() {
void PerFrameContentTranslateDriver::OnPageLanguageDetermined( void PerFrameContentTranslateDriver::OnPageLanguageDetermined(
const LanguageDetectionDetails& details, const LanguageDetectionDetails& details,
bool page_needs_translation) { bool page_level_translation_critiera_met) {
language_determined_time_ = base::TimeTicks::Now(); language_determined_time_ = base::TimeTicks::Now();
ReportLanguageDeterminedDuration(finish_navigation_time_, ReportLanguageDeterminedDuration(finish_navigation_time_,
language_determined_time_); language_determined_time_);
...@@ -357,7 +359,7 @@ void PerFrameContentTranslateDriver::OnPageLanguageDetermined( ...@@ -357,7 +359,7 @@ void PerFrameContentTranslateDriver::OnPageLanguageDetermined(
if (translate_manager() && web_contents()) { if (translate_manager() && web_contents()) {
translate_manager()->GetLanguageState()->LanguageDetermined( translate_manager()->GetLanguageState()->LanguageDetermined(
details.adopted_language, page_needs_translation); details.adopted_language, page_level_translation_critiera_met);
translate_manager()->InitiateTranslation(details.adopted_language); translate_manager()->InitiateTranslation(details.adopted_language);
} }
......
...@@ -60,7 +60,7 @@ class PerFrameContentTranslateDriver : public ContentTranslateDriver { ...@@ -60,7 +60,7 @@ class PerFrameContentTranslateDriver : public ContentTranslateDriver {
void DocumentOnLoadCompletedInMainFrame() override; void DocumentOnLoadCompletedInMainFrame() override;
void OnPageLanguageDetermined(const LanguageDetectionDetails& details, void OnPageLanguageDetermined(const LanguageDetectionDetails& details,
bool page_needs_translation); bool page_level_translation_critiera_met);
private: private:
friend class PerFrameContentTranslateDriverTest; friend class PerFrameContentTranslateDriverTest;
......
...@@ -77,5 +77,5 @@ interface ContentTranslateDriver { ...@@ -77,5 +77,5 @@ interface ContentTranslateDriver {
// Notification that a new page is ready to translate, // Notification that a new page is ready to translate,
// and the language for it has been determined. // and the language for it has been determined.
RegisterPage(pending_remote<TranslateAgent> translate_agent, RegisterPage(pending_remote<TranslateAgent> translate_agent,
LanguageDetectionDetails details, bool page_needs_translation); LanguageDetectionDetails details, bool translation_critiera_met);
}; };
...@@ -12,7 +12,7 @@ namespace translate { ...@@ -12,7 +12,7 @@ namespace translate {
LanguageState::LanguageState(TranslateDriver* driver) LanguageState::LanguageState(TranslateDriver* driver)
: is_page_translated_(false), : is_page_translated_(false),
translate_driver_(driver), translate_driver_(driver),
page_needs_translation_(false), page_level_translation_critiera_met_(false),
translation_pending_(false), translation_pending_(false),
translation_error_(false), translation_error_(false),
translation_declined_(false), translation_declined_(false),
...@@ -55,15 +55,16 @@ void LanguageState::DidNavigate(bool is_same_document_navigation, ...@@ -55,15 +55,16 @@ void LanguageState::DidNavigate(bool is_same_document_navigation,
SetTranslateEnabled(false); SetTranslateEnabled(false);
} }
void LanguageState::LanguageDetermined(const std::string& page_language, void LanguageState::LanguageDetermined(
bool page_needs_translation) { const std::string& page_language,
bool page_level_translation_critiera_met) {
if (is_same_document_navigation_ && !original_lang_.empty()) { if (is_same_document_navigation_ && !original_lang_.empty()) {
// Same-document navigation, we don't expect our states to change. // Same-document navigation, we don't expect our states to change.
// Note that we'll set the languages if original_lang_ is empty. This might // Note that we'll set the languages if original_lang_ is empty. This might
// happen if the we did not get called on the top-page. // happen if the we did not get called on the top-page.
return; return;
} }
page_needs_translation_ = page_needs_translation; page_level_translation_critiera_met_ = page_level_translation_critiera_met;
original_lang_ = page_language; original_lang_ = page_language;
current_lang_ = page_language; current_lang_ = page_language;
SetIsPageTranslated(false); SetIsPageTranslated(false);
......
...@@ -35,10 +35,10 @@ class LanguageState { ...@@ -35,10 +35,10 @@ class LanguageState {
bool navigation_from_google); bool navigation_from_google);
// Should be called when the language of the page has been determined. // Should be called when the language of the page has been determined.
// |page_needs_translation| when false indicates that the browser should not // |page_level_translation_critiera_met| when false indicates that the browser
// offer to translate the page. // should not offer to translate the page.
void LanguageDetermined(const std::string& page_language, void LanguageDetermined(const std::string& page_language,
bool page_needs_translation); bool page_level_translation_critiera_met);
// Returns the language the current page should be translated to, based on the // Returns the language the current page should be translated to, based on the
// previous page languages and the transition. This should be called after // previous page languages and the transition. This should be called after
...@@ -62,7 +62,9 @@ class LanguageState { ...@@ -62,7 +62,9 @@ class LanguageState {
const std::string& current_language() const { return current_lang_; } const std::string& current_language() const { return current_lang_; }
void SetCurrentLanguage(const std::string& language); void SetCurrentLanguage(const std::string& language);
bool page_needs_translation() const { return page_needs_translation_; } bool page_level_translation_critiera_met() const {
return page_level_translation_critiera_met_;
}
// Whether the page is currently in the process of being translated. // Whether the page is currently in the process of being translated.
bool translation_pending() const { return translation_pending_; } bool translation_pending() const { return translation_pending_; }
...@@ -115,10 +117,12 @@ class LanguageState { ...@@ -115,10 +117,12 @@ class LanguageState {
// outlive this object. // outlive this object.
TranslateDriver* translate_driver_; TranslateDriver* translate_driver_;
// Whether it is OK to offer to translate the page. Some pages explictly // Whether it is OK to offer to translate the page. Translation is not offered
// specify that they should not be translated by the browser (this is the case // if we cannot determine the source language. In addition, some pages
// for GMail for example, which provides its own translation features). // explicitly specify that they should not be translated by the browser (this
bool page_needs_translation_; // is the case for GMail for example, which provides its own translation
// features).
bool page_level_translation_critiera_met_;
// Whether a translation is currently pending. // Whether a translation is currently pending.
// This is needed to avoid sending duplicate translate requests to a page. // This is needed to avoid sending duplicate translate requests to a page.
......
...@@ -813,7 +813,7 @@ void TranslateManager::FilterIsTranslatePossible( ...@@ -813,7 +813,7 @@ void TranslateManager::FilterIsTranslatePossible(
const std::string& target_lang) { const std::string& target_lang) {
// Short-circuit out if not in a state where initiating translation makes // Short-circuit out if not in a state where initiating translation makes
// sense (this method may be called multiple times for a given page). // sense (this method may be called multiple times for a given page).
if (!language_state_.page_needs_translation() || if (!language_state_.page_level_translation_critiera_met() ||
language_state_.translation_pending() || language_state_.translation_pending() ||
language_state_.translation_declined() || language_state_.translation_declined() ||
language_state_.IsPageTranslated()) { language_state_.IsPageTranslated()) {
......
...@@ -1005,7 +1005,7 @@ TEST_F(TranslateManagerTest, CanManuallyTranslate_PageNeedsTranslation) { ...@@ -1005,7 +1005,7 @@ TEST_F(TranslateManagerTest, CanManuallyTranslate_PageNeedsTranslation) {
translate_manager_->GetLanguageState()->LanguageDetermined("de", false); translate_manager_->GetLanguageState()->LanguageDetermined("de", false);
// Users should be able to manually translate the page, even when // Users should be able to manually translate the page, even when
// |page_needs_translation| is false. // |page_level_translation_critiera_met| is false.
EXPECT_TRUE(translate_manager_->CanManuallyTranslate()); EXPECT_TRUE(translate_manager_->CanManuallyTranslate());
EXPECT_TRUE(translate_manager_->CanManuallyTranslate(true)); EXPECT_TRUE(translate_manager_->CanManuallyTranslate(true));
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment