Offer google translate installation in translate bubbles #3103

yrliou · 2019-08-06T21:18:06Z

Below is the gif of how this PR looks like and it's easier to go through each commit when reviewing.

This PR repurposed TranslateBubbleView and TranslateIconView to offer google translate installation in translate bubbles when ENABLE_BRAVE_TRANSLATE (go-translate) is disabled.
This UI is expected to be used in all desktop channels, and the prompt of translate bubbles could be disabled through language settings by toggling Offer to translate pages that aren't in a language you read as below.

Submitter Checklist:

Test Plan:

Open brave://settings
Toggle off the Offer to translate pages that aren't in a language you read setting in Languages -> Language.
Visit https://www.deutschland.de/de, translate bubble should not be shown.
Right click and Translate to _your_language_ should not be shown in the context menu.
Toggle on the Offer to translate pages that aren't in a language you read setting in Languages -> Language.
Visit https://www.deutschland.de/de, the translate bubble should be shown to offer google translate installation.
After installation, both the icon & bubble in the location bar should be hidden and the page could be translated through Google Translate extension.
Open a new tab and visit https://www.deutschland.de/de again, translate bubble should not be shown.

Reviewer Checklist:

New files have MPL-2.0 license header.
Request a security/privacy review as needed.
Adequate test coverage exists to prevent regressions
Verify test plan is specified in PR before merging to source

After-merge Checklist:

The associated issue milestone is set to the smallest version that the
changes has landed on.
All relevant documentation has been updated.

…ed functions.

…ogle Translate extension.

…d of patching TranslateService::IsTranslatableURL.

…mpt setting in all channels.

mkarolin

LGTM, works as described.

bridiver · 2019-08-08T17:03:25Z

browser/ui/views/translate/brave_translate_bubble_view.h

+#endif
+
+ private:
+#if !BUILDFLAG(ENABLE_BRAVE_TRANSLATE) && BUILDFLAG(ENABLE_EXTENSIONS)


I don't think we want ENABLE_EXTENSIONS here. If translate is dependent on extensions then we should set enable_brave_translate = enable_extensions in gn

separate the build flags in 6acee43

bridiver · 2019-08-08T17:03:56Z

browser/ui/views/translate/brave_translate_bubble_view.cc

+void BraveTranslateBubbleView::Init() {
+  TranslateBubbleView::Init();
+  RemoveChildView(before_translate_view_);
+  before_translate_view_ = AddChildView(BraveCreateViewBeforeTranslate());


I think this is going to fail on android because it has a guard above

it seems like in general this needs to work differently for android

this google extension approach won't work in android unless we have extensions supported, so this code block shouldn't be built on android atm.

bridiver · 2019-08-08T17:10:00Z

patches/chrome-browser-ui-views-translate-translate_bubble_view.cc.patch

+   std::unique_ptr<TranslateBubbleModel> model(
+       new TranslateBubbleModelImpl(step, std::move(ui_delegate)));
+-  TranslateBubbleView* view = new TranslateBubbleView(
+  TranslateBubbleView* view = new BraveTranslateBubbleView(


rather than change this line, I think it's better to add BRAVE_SHOW_BUBBLE_ after it. Then if ENABLE_BRAVE_TRANSLATE is false BRAVE_SHOW_BUBBLE_ does nothing. If it's true then it replaces view with BraveTranslateBubbleView. This also eliminates all the guards inside BraveTranslateBubbleView and any issues with android. brave_translate_bubble_view.h/.cc should also go inside enable_brave_translate guards in gn

Could we keep this patch line but replace BraveTranslateBubbleView back to TranslateBubbleView in the chromium_src override instead if ENABLE_BRAVE_TRANSLATE_EXTENSION is false?
This would also eliminates the guards inside BraveTranslateBubbleView and guard brave_translate_bubble_view.h/.cc inside gn.
Currently did this in 6acee43, wdyt?

Updated as suggested in 60e70de

- Move adding Open Link with Tor into InitMenu and remove AppendBraveLinkItems - Fix bug when removing IDC_CONTENT_CONTEXT_TRANSLATE since it is not always existed - Define BRAVE_RENDER_VIEW_CONTEXT_MENU_H_ and BRAVE_TRANSLATE_BUBBLE_VIEW_H_ to be extensible in the header

bridiver · 2019-08-08T21:53:53Z

chromium_src/chrome/browser/ui/views/translate/translate_bubble_view.h

+  private: \
+    friend class BraveTranslateBubbleView; \
+  public:
+class BraveTranslateBubbleView;


just for readability please leave a space after the multiline macro

addresses in 352251d

- Separate translate build flags for go-translate and google translate extension approaches - Guard translate_bubble_view and translate_icon_view subclasses files in gn

…ter readability

bridiver · 2019-08-08T22:50:57Z

browser/translate/buildflags/buildflags.gni


 declare_args() {
-  enable_brave_translate = !is_release_channel
+  enable_brave_translate_go = false
+}


nit - you only need one declare_args section here

I'm actually getting below build error when doing enable_brave_translate_extension = !enable_brave_translate_go && enable_extensions if using one declare_args.
ERROR at //brave/browser/translate/buildflags/buildflags.gni:7:39: Reading a variable defined in the same declare_args() call.

bridiver · 2019-08-08T22:52:54Z

chromium_src/chrome/browser/renderer_context_menu/render_view_context_menu.cc

+    const Browser* browser = GetBrowser();
+    const bool is_app = browser && browser->is_app();
+
+    index = menu_model_.GetIndexOfCommandId(


cc @petemill - anything else she should do here? Did you find a good way to test?

bridiver · 2019-08-08T23:09:54Z

browser/ui/views/translate/brave_translate_icon_view.cc

+    return was_visible != GetVisible();
+  }
+
+  return TranslateIconView::Update();


Is this right? Won't ChromeTranslateClient::FromWebContents(GetWebContents()) return nullptr in TranslateIconView::Update? Either way does it ever make sense to call the superclass method here?

It won't be nullptr when doing ChromeTranslateClient::FromWebContents(GetWebContents()) in TranslateIconView::Update, the client still exists in our case.
What I planned to achieve here is just to behave the same way as superclass before google translate is installed, and make sure we won't still show the icon right after google translate is already installed.

bridiver · 2019-08-08T23:11:04Z

browser/ui/views/translate/brave_translate_bubble_view.cc

@@ -0,0 +1,138 @@
+/* Copyright (c) 2019 The Brave Authors. All rights reserved.


should this stuff maybe be in browser/ui/views/translate/extension?

yrliou · 2019-08-09T08:01:30Z

Added unit tests for translate bubble, latest CI test failure was only in win-x64's Copy file to S3 step.

Offer google translate installation in translate bubbles

kjozwiak · 2019-08-21T00:32:29Z

Using 0.71.21 Chromium: 76.0.3809.100, went through the following cases to ensure the above was working as expected so we can uplift into 0.69.x via #3177:

went through the original issues outlined under Offer google translate installation in translate bubbles #3103 (comment)
ensured that you can uninstall/reinstall multiple times without any issues
ensured that the translation model appears once you visit a website that can be translated even though you've uninstalled/installed several times
ensured that "Translate this Page" via the extension worked without issues
ensured that the translate context menu feature worked without issues
ensured that you can translate to different languages using the "Translate to:" feature
ensured that "Sow original" displays the original language without any issues
ensured that "Turn off translations for for this site" worked as expected

Used the following websites:

Examples of the feature:

jost-s · 2019-10-11T08:29:00Z

Guys, is there a way to disable the offer to install the extension? I don't want to install it and it constantly pops up.

bsclifton · 2019-10-11T16:29:34Z

@jost-s yes, there is 😄

Visit brave://settings/languages and click the down chevron under languages. There should be an option: Offer to translate pages that aren't in a language you read

We're rolling out a fix soon (today) that offers a Don't ask me again in this popup also 😄

jost-s · 2019-10-11T16:33:14Z

@bsclifton Brilliant, cheers man! 😃

yrliou added this to the 0.70.x - Nightly milestone Aug 6, 2019

yrliou self-assigned this Aug 6, 2019

Disable ENABLE_BRAVE_TRANSLATE in all channels for go-translate relat…

a3564ee

…ed functions.

yrliou force-pushed the offer_google_translate branch 3 times, most recently from 42ccc16 to 0657843 Compare August 6, 2019 23:10

yrliou added 4 commits August 6, 2019 16:16

Repurpose TranslateBubbleView and TranslateIconView for installing Go…

314c904

…ogle Translate extension.

Hide translate item in the context menu by overriding InitMenu instea…

2421bf8

…d of patching TranslateService::IsTranslatableURL.

Remove kTranslateUI from disabled features and show the translate pro…

bd84ba9

…mpt setting in all channels.

Fix lint errors

78dc52a

yrliou force-pushed the offer_google_translate branch from 0657843 to 78dc52a Compare August 6, 2019 23:17

yrliou changed the title ~~[WIP] Offer google translate installation in translate bubbles~~ Offer google translate installation in translate bubbles Aug 6, 2019

yrliou marked this pull request as ready for review August 6, 2019 23:30

yrliou requested a review from bridiver as a code owner August 6, 2019 23:30

yrliou requested review from mkarolin, simonhong, emerick and bbondy and removed request for emerick August 6, 2019 23:30

mkarolin previously approved these changes Aug 8, 2019

View reviewed changes

bridiver reviewed Aug 8, 2019

View reviewed changes

yrliou dismissed mkarolin’s stale review via bf5ab9e August 8, 2019 17:34

yrliou force-pushed the offer_google_translate branch from bf5ab9e to 6fb489c Compare August 8, 2019 17:39

bridiver reviewed Aug 8, 2019

View reviewed changes

yrliou added 2 commits August 8, 2019 15:07

Address review comments

31185e9

- Separate translate build flags for go-translate and google translate extension approaches - Guard translate_bubble_view and translate_icon_view subclasses files in gn

Define BRAVE_TRANSLATE_BUBBLE_VIEW_ in translate bubble patch for bet…

60e70de

…ter readability

Closing multiline macro definition with a comment

352251d

yrliou force-pushed the offer_google_translate branch from 6acee43 to 352251d Compare August 8, 2019 22:25

bridiver reviewed Aug 8, 2019

View reviewed changes

Add unit tests for translate bubble

256b4d9

yrliou force-pushed the offer_google_translate branch from 6d0cc01 to 256b4d9 Compare August 9, 2019 06:32

bridiver approved these changes Aug 9, 2019

View reviewed changes

yrliou merged commit 4efa8ef into master Aug 9, 2019

yrliou deleted the offer_google_translate branch August 9, 2019 20:55

yrliou mentioned this pull request Aug 14, 2019

Uplift "Offer google translate installation in translate bubbles" to 0.69.x #3177

Merged

yrliou added a commit that referenced this pull request Aug 14, 2019

Merge pull request #3103 from brave/offer_google_translate

8c16179

Offer google translate installation in translate bubbles

GeetaSarvadnya mentioned this pull request Aug 25, 2019

Offer to install Google Translate extension in translate bubble brave/brave-browser#5561

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Offer google translate installation in translate bubbles #3103

Offer google translate installation in translate bubbles #3103

yrliou commented Aug 6, 2019 •

edited

Loading

mkarolin left a comment

bridiver Aug 8, 2019

yrliou Aug 8, 2019

bridiver Aug 8, 2019

bridiver Aug 8, 2019

yrliou Aug 8, 2019

bridiver Aug 8, 2019

yrliou Aug 8, 2019 •

edited

Loading

yrliou Aug 8, 2019

bridiver Aug 8, 2019

yrliou Aug 8, 2019

bridiver Aug 8, 2019

yrliou Aug 8, 2019

bridiver Aug 8, 2019

bridiver Aug 8, 2019

yrliou Aug 8, 2019 •

edited

Loading

bridiver Aug 8, 2019

yrliou commented Aug 9, 2019

kjozwiak commented Aug 21, 2019

jost-s commented Oct 11, 2019

bsclifton commented Oct 11, 2019

jost-s commented Oct 11, 2019

		@@ -0,0 +1,138 @@
		/* Copyright (c) 2019 The Brave Authors. All rights reserved.

Offer google translate installation in translate bubbles #3103

Offer google translate installation in translate bubbles #3103

Conversation

yrliou commented Aug 6, 2019 • edited Loading

Submitter Checklist:

Test Plan:

Reviewer Checklist:

After-merge Checklist:

mkarolin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yrliou Aug 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yrliou Aug 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yrliou commented Aug 9, 2019

kjozwiak commented Aug 21, 2019

jost-s commented Oct 11, 2019

bsclifton commented Oct 11, 2019

jost-s commented Oct 11, 2019

yrliou commented Aug 6, 2019 •

edited

Loading

yrliou Aug 8, 2019 •

edited

Loading

yrliou Aug 8, 2019 •

edited

Loading