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

Add GN flag to disable building cloud print and disable pdfium on non-windows #2902

Closed
wants to merge 1 commit into from

Conversation

rht
Copy link

@rht rht commented Jan 10, 2019

This partially addresses #1958 except on Windows.
cc: @fmarier

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
  • Ran git rebase master (if needed).
  • Ran git rebase -i to squash commits (if needed).
  • Tagged reviewers and labelled the pull request as needed.
  • Requested a security/privacy review as needed.

Test Plan:

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions.

lib/config.js Outdated
@@ -91,7 +91,9 @@ Config.prototype.buildArgs = function () {
chrome_version_major: chrome_version_parts[0],
safebrowsing_api_endpoint: this.safeBrowsingApiEndpoint,
brave_referrals_api_key: this.braveReferralsApiKey,
enable_hangout_services_extension: this.enable_hangout_services_extension
enable_hangout_services_extension: this.enable_hangout_services_extension,
enable_printing: false, // disable cloudprint
Copy link
Contributor

Choose a reason for hiding this comment

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

is this just cloud printing? cc @darkdh

Copy link
Author

Choose a reason for hiding this comment

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

Whoops, that's all of printing. I found the flag at https://github.com/Eloston/ungoogled-chromium/blob/master/patches/inox-patchset/0005-disable-default-extensions.patch#L19-L23. But it looks like these particular lines need to be removed in order to disable just cloud printing.

@rht
Copy link
Author

rht commented Jan 11, 2019

From the output of brave/brave-core#980 (comment), technically enable_print_preview is technically true for all platforms. So pdfium can't be disabled.

@fmarier
Copy link
Member

fmarier commented Jan 11, 2019

This latest patch runs into the same assertion failure as before:

ERROR at //printing/BUILD.gn:20:3: Assertion failed.
  assert(enable_pdf,
  ^-----
Windows basic printing or print preview needs pdf: set enable_pdf=true.
See //BUILD.gn:142:7: which caused the file to be included.
      "//printing:printing_unittests",
      ^------------------------------

@rht
Copy link
Author

rht commented Jan 11, 2019

Yeah, it is impossible to disable unless print preview uses pdf.js.

@bsclifton
Copy link
Member

Closing as we solved this issue and are now using PDFium

@bsclifton bsclifton closed this Jun 3, 2019
@rht
Copy link
Author

rht commented Jun 3, 2019

#3846

@rht rht deleted the no-cloudprint-pdf branch June 3, 2019 22:47
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.

5 participants