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

Disable translate url fetcher request #3293

Merged
merged 3 commits into from
Aug 29, 2019

Conversation

yrliou
Copy link
Member

@yrliou yrliou commented Aug 29, 2019

Fix brave/brave-browser#5829

Submitter Checklist:

Test Plan:

auto test: npm test brave_unit_tests -- --filter=TranslateLanguageListTest.*
manual test plan is specified in brave/brave-browser#5829, browser connections could be observed using little snitch or similar tools.

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.

@yrliou yrliou added this to the 0.71.x - Nightly milestone Aug 29, 2019
@yrliou yrliou requested a review from bridiver as a code owner August 29, 2019 00:55
@yrliou yrliou self-assigned this Aug 29, 2019
@@ -0,0 +1,6 @@
source_set("browser") {
public_deps = [
# TODO(jocelyn): move buildflags to brave/components/translate/core/browser
Copy link
Member Author

Choose a reason for hiding this comment

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

This PR will need to be uplifted to dev and beta, so I would like to separate this work into a follow up PR which only goes into master.

]
deps += [ "//components/infobars/core" ]
}
+ deps += ["//brave/components/translate/core/browser"]
Copy link
Member Author

Choose a reason for hiding this comment

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

Added this because we'll need to use our translate buildflags in the chromium_src override.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we need multiple patches anyway, why are we using chromium_src overrides?

Copy link
Member Author

@yrliou yrliou Aug 29, 2019

Choose a reason for hiding this comment

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

@pilgrim-brave If we subclass TranslateURLFetcher, we'll need 1) this one line deps patch. 2) two line patch in translate_url_fetcher.h for adding virtual keyword to its destructor and Request method. 3) one line patch to s/TranslateURLFetcher/BraveTranslateURLFetcher in translate_language_list.cc. 4) chromium_src override to s/TranslateURLFetcher/BraveTranslateURLFetcher in translate_script.cc.
Current PR has two lines less patching comparing to subclass it so I went for direct patching Request at first. I can change to subclass if that's more preferable in this case.

Copy link
Member Author

@yrliou yrliou Aug 29, 2019

Choose a reason for hiding this comment

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

Also, I don't foresee we'll need to override more methods on TranslateURLFetcher in the near future given how this class is right now. It's more likely that we might just decommission go-translate in the future at least for the desktops and then at that point we can go back to just one line patch in the Request method without this deps patch.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK I'll defer to @bridiver who might have ideas on further reducing the patches. Otherwise LGTM.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we can just replace the class .h/.cc a minimal no-op dummy class in chromium_src and avoid the second patch. We'll still need this for the buildflag dep, but at least this is an extensible patch that allows us to add more deps to this component if we need them later

Copy link
Collaborator

Choose a reason for hiding this comment

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

also @pilgrim-brave just for reference, if we don't make the change I suggested we would still use a chromium_src override for the second patch because it decreases the size of the patch and also allows us to change the code in the patch without changing the patch itself. Using a define for patches is now the preferred approach for that reason.

Copy link
Member Author

@yrliou yrliou Aug 29, 2019

Choose a reason for hiding this comment

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

Created a dummy impl as @bridiver suggested and got rid of the other patching in 9244c81.

bool TranslateURLFetcher::Request(const GURL& url,
TranslateURLFetcher::Callback callback,
bool is_incognito) {
+ BRAVE_REQUEST
Copy link
Member Author

Choose a reason for hiding this comment

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

Subclass this seems to require more patching to me, at least one line in translate_url_fetcher.h and one line in translate_language_list.cc, and the deps patch.

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed in 9244c81

@yrliou yrliou force-pushed the disable_translate_url_fetcher_request branch from f49ba19 to b4c00ae Compare August 29, 2019 01:08
@yrliou
Copy link
Member Author

yrliou commented Aug 29, 2019

Encountered known intermittent error below at windows-x64, which is not related to this PR.
This audit-dep step is green on all other platforms and it was all green in the previous try before the last commit, which should be safe enough.

13:53:42  Auditing C:\jenkins\workspace\le_translate_url_fetcher_request
13:53:42  C:\jenkins\workspace\le_translate_url_fetcher_request: npm audit
13:53:56  
13:53:56  powershell.exe : npm ERR! code ENOAUDIT
13:53:56  At C:\jenkins\workspace\le_translate_url_fetcher_request@tmp\durable-d6ca13c4\powershellWrapper.ps1:3 char:1
13:53:56  + & powershell -NoProfile -NonInteractive -ExecutionPolicy Bypass -Comm ...
13:53:56  + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
13:53:56      + CategoryInfo          : NotSpecified: (npm ERR! code ENOAUDIT:String) [], RemoteException
13:53:56      + FullyQualifiedErrorId : NativeCommandError
13:53:56   
13:53:56  npm ERR! audit Your configured registry (https://registry.npmjs.org/) does not support audit requests.

@yrliou yrliou merged commit 750d790 into master Aug 29, 2019
@yrliou yrliou deleted the disable_translate_url_fetcher_request branch August 29, 2019 23:05
brave-builds pushed a commit that referenced this pull request Aug 29, 2019
brave-builds pushed a commit that referenced this pull request Aug 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants