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

in-page translation #2006

Merged
merged 11 commits into from
Apr 4, 2019
Merged

in-page translation #2006

merged 11 commits into from
Apr 4, 2019

Conversation

yrliou
Copy link
Member

@yrliou yrliou commented Mar 19, 2019

Fix brave/brave-browser#208

This PR re-enable entries of translate service, all incurred requests will be sent to go-translate server (https://github.com/brave/go-translate) to either be relayed to MS server or be proxied through Brave to avoid introducing direct connections to google.

go-translate is now running locally at localhost:8195 manually, the URL will be updated to a brave server once go-translate is deployed by devops.

Related PR:
brave/go-translate#1

Entry points of translation service

  1. Through the translate bubble in the location bar

Screen Shot 2019-03-19 at 3 09 27 PM

  1. Through the context menu

Screen Shot 2019-03-19 at 3 09 35 PM

Submitter Checklist:

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Verified that these changes build without errors on
    • Windows
    • macOS
    • Linux
  • Verified that these changes pass automated tests (npm test brave_unit_tests && npm test brave_browser_tests) on
    • Windows
    • macOS
    • Linux
  • Verified that all lint errors/warnings are resolved (npm run lint)
  • Ran git rebase master (if needed).
  • Ran git rebase -i to squash commits (if needed).
  • Tagged reviewers and labelled the pull request as needed.
  • Request a security/privacy review as needed.
  • Add appropriate QA labels (QA/Yes or QA/No) to include the closed issue in milestone

Test Plan:

  1. Navigate to https://www.deutschland.de/de and click translate when the translate bubble shows up. Continue surfing through this website and pages should be translated.

  2. Open a new tab to navigate to https://www.deutschland.de/de, click "Translate to English" (depends on your local language) in the context menu, current page should be translated, when click away through links in this page, they will not be translated.

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

@yrliou yrliou self-assigned this Mar 19, 2019
@yrliou yrliou changed the title [WIP] in-page translate WIP: in-page translate Mar 19, 2019
@yrliou yrliou force-pushed the page-translate branch 3 times, most recently from f07daee to 5f54528 Compare March 19, 2019 06:32
@yrliou yrliou changed the title WIP: in-page translate WIP: in-page translation Mar 19, 2019
@yrliou yrliou added this to the 0.64.x - Nightly milestone Mar 19, 2019
@yrliou yrliou force-pushed the page-translate branch 3 times, most recently from dd87208 to c456ea9 Compare March 20, 2019 03:11
@yrliou yrliou marked this pull request as ready for review March 20, 2019 03:13
@yrliou yrliou requested a review from bbondy March 20, 2019 03:13
@yrliou yrliou changed the title WIP: in-page translation in-page translation Mar 20, 2019
@yrliou yrliou requested review from emerick and mkarolin March 20, 2019 16:39
emerick
emerick previously approved these changes Mar 20, 2019
Copy link
Contributor

@emerick emerick left a comment

Choose a reason for hiding this comment

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

LGTM

@yrliou
Copy link
Member Author

yrliou commented Mar 20, 2019

Updated wording in the bubble title, now looks like this:
Screen Shot 2019-03-20 at 1 05 06 PM

@rebron
Copy link
Collaborator

rebron commented Mar 20, 2019

Updated wording in the bubble title, now looks like this:
Screen Shot 2019-03-20 at 1 05 06 PM

The language here looks good.

@yrliou
Copy link
Member Author

yrliou commented Mar 21, 2019

rebased on C74.

@@ -396,6 +396,10 @@ By installing this extension, you are agreeing to the Google Widevine Terms of U
<message name="IDS_SETTINGS_BRAVE_ON_EXIT" desc="Clear Browsing Data dialog On exit tab label">
On exit
</message>
<!-- In-page Translation -->
<message name="IDS_BRAVE_TRANSLATE_BUBBLE_BEFORE_TRANSLATE_TITLE" desc="Title text for the translate bubble when asking to translate a page.">
Translate this page? This will send all text on this page to Microsoft for translation.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we plan to have a wiki that highlights the text will not be associated with any user identifiable information?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

not yet; maybe https://github.com/brave/brave-browser/wiki/Proxy-redirected-URLs could mention what data is sent in each of the proxied requests

@jumde jumde self-requested a review March 25, 2019 19:27
jumde
jumde previously approved these changes Mar 25, 2019
@yrliou yrliou modified the milestones: 0.64.x - Dev, 0.65.x - Nightly Apr 3, 2019
@yrliou yrliou requested a review from emerick April 4, 2019 00:19
emerick
emerick previously approved these changes Apr 4, 2019
Copy link
Contributor

@emerick emerick left a comment

Choose a reason for hiding this comment

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

LGTM

@yrliou
Copy link
Member Author

yrliou commented Apr 4, 2019

linux and mac passed: https://staging.ci.brave.com/job/brave-browser-build-pr/job/page-translate/3/flowGraphTable/
windows-x64 passed after widevine PRs reverted: https://staging.ci.brave.com/job/brave-browser-build-pr/job/page-translate/4/flowGraphTable/

@emerick sorry the review is being dismissed again because I have to push again to try windows after reverting widevine PRs, no code changes here, please approve again then I'll merge.

@yrliou yrliou requested a review from emerick April 4, 2019 05:02
Copy link
Contributor

@emerick emerick left a comment

Choose a reason for hiding this comment

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

LGTM

@yrliou yrliou merged commit 4c18291 into master Apr 4, 2019
@bbondy
Copy link
Member

bbondy commented Apr 5, 2019

🎉

@bsclifton bsclifton deleted the page-translate branch April 5, 2019 16:39
@bsclifton
Copy link
Member

Great job with this, @yrliou! And thanks for being vigilant to make CI passes 😄

@kmichelizzi
Copy link

Sorry, this is not working on MacOS 10.14.5 going to www.yandex.ru - it never shows the translate bubble. Doesn't have Translate in the context menu either. Neither does going to the specified verification webpage (deutschland.de/de) - if it is going to be difficult to enable, it isn't working in my book. This is with Brave Version 0.65.118 Chromium: 75.0.3770.80 (Official Build) (64-bit).

yandex

@yrliou
Copy link
Member Author

yrliou commented Jun 6, 2019

@kmichelizzi Translation service is disabled on Release channel currently due to cost performance, we're still working on enabling it on Release channel.
In the mean time it is enabled on Nightly, Dev, and Beta. which you could grab and install if you would like to use the translation service for now.

@kmichelizzi
Copy link

Thanks, @yrliou - the Nightly, Dev, and Beta don't run on MacOS 10.14.5. All 3 versions just spin until crash on loading.
Brave Browser Beta [23658] crash report.txt

@yrliou
Copy link
Member Author

yrliou commented Jun 6, 2019

@kmichelizzi I'm also on MacOS 10.14.5, Nightly, Dev, and Beta all works fine for me.
You might need to create a new issue in https://github.com/brave/brave-browser with details in that case so we could help identify what's the problem, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

In page translation for content
8 participants