Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2884123003:
make chrome build without any printing support (Closed)
Descriptionmake chrome build without any printing support
User complain that chrome doesn't build using their customized args.gn. See https://bugs.chromium.org/p/chromium/issues/detail?id=603559#c57.
Their args.gn set enable_basic_printing, enable_print_preview and enable_service_discovery to false. This patch makes sure this flag setting can build successfully.
BUG=603559
Review-Url: https://codereview.chromium.org/2884123003
Cr-Commit-Position: refs/heads/master@{#473057}
Committed: https://chromium.googlesource.com/chromium/src/+/f2df047e7dae8daf0f04abf9681fee1f4fcee3d9
Patch Set 1 #
Total comments: 4
Patch Set 2 : nit fix #
Messages
Total messages: 22 (14 generated)
The CQ bit was checked by jzfeng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== make chrome build when enable_basic_printing and enable_print_preview are both false BUG= ========== to ========== make chrome build when enable_basic_printing and enable_print_preview are both false user complain that chrome doesn't build using their customized args.gn. See https://bugs.chromium.org/p/chromium/issues/detail?id=603559#c57. BUG=603559 ==========
Description was changed from ========== make chrome build when enable_basic_printing and enable_print_preview are both false user complain that chrome doesn't build using their customized args.gn. See https://bugs.chromium.org/p/chromium/issues/detail?id=603559#c57. BUG=603559 ========== to ========== User complain that chrome doesn't build using their customized args.gn. See https://bugs.chromium.org/p/chromium/issues/detail?id=603559#c57. Their args.gn set enable_basic_printing, enable_print_preview and enable_service_discovery to false. This patch makes sure this flag setting can build successfully. BUG=603559 ==========
Description was changed from ========== User complain that chrome doesn't build using their customized args.gn. See https://bugs.chromium.org/p/chromium/issues/detail?id=603559#c57. Their args.gn set enable_basic_printing, enable_print_preview and enable_service_discovery to false. This patch makes sure this flag setting can build successfully. BUG=603559 ========== to ========== User complain that chrome doesn't build using their customized args.gn. See https://bugs.chromium.org/p/chromium/issues/detail?id=603559#c57. Their args.gn set enable_basic_printing, enable_print_preview and enable_service_discovery to false. This patch makes sure this flag setting can build successfully. enable_basic_printing = false, enable_print_preview = false, enable_service_discovery = true still doesn't build, since code related to enable_service_discovery is closely coupled with the printing support. Leave as is for now. BUG=603559 ==========
jzfeng@chromium.org changed reviewers: + eseckler@chromium.org, thestig@chromium.org
PTAL. Thanks!
headless lgtm.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
lgtm - The CL description is missing "make chrome build without any printing support" as the first line. - You don't need to mention the build config for: enable_basic_printing = false, enable_print_preview = false, enable_service_discovery = true - I don't think that has ever built successfully. https://codereview.chromium.org/2884123003/diff/1/headless/lib/browser/headle... File headless/lib/browser/headless_devtools_manager_delegate.cc (right): https://codereview.chromium.org/2884123003/diff/1/headless/lib/browser/headle... headless/lib/browser/headless_devtools_manager_delegate.cc:94: // The max and min value should match the ones in scaling_settings.html. Can you create another anonymous namespace for these? https://codereview.chromium.org/2884123003/diff/1/pdf/BUILD.gn File pdf/BUILD.gn (right): https://codereview.chromium.org/2884123003/diff/1/pdf/BUILD.gn#newcode53 pdf/BUILD.gn:53: "//printing", You don't need this, since pdf/ is only using printing/ headers for constant values and not actually linking against it.
Description was changed from ========== User complain that chrome doesn't build using their customized args.gn. See https://bugs.chromium.org/p/chromium/issues/detail?id=603559#c57. Their args.gn set enable_basic_printing, enable_print_preview and enable_service_discovery to false. This patch makes sure this flag setting can build successfully. enable_basic_printing = false, enable_print_preview = false, enable_service_discovery = true still doesn't build, since code related to enable_service_discovery is closely coupled with the printing support. Leave as is for now. BUG=603559 ========== to ========== make chrome build without any printing support User complain that chrome doesn't build using their customized args.gn. See https://bugs.chromium.org/p/chromium/issues/detail?id=603559#c57. Their args.gn set enable_basic_printing, enable_print_preview and enable_service_discovery to false. This patch makes sure this flag setting can build successfully. enable_basic_printing = false, enable_print_preview = false, enable_service_discovery = true still doesn't build, since code related to enable_service_discovery is closely coupled with the printing support. Leave as is for now. BUG=603559 ==========
Description was changed from ========== make chrome build without any printing support User complain that chrome doesn't build using their customized args.gn. See https://bugs.chromium.org/p/chromium/issues/detail?id=603559#c57. Their args.gn set enable_basic_printing, enable_print_preview and enable_service_discovery to false. This patch makes sure this flag setting can build successfully. enable_basic_printing = false, enable_print_preview = false, enable_service_discovery = true still doesn't build, since code related to enable_service_discovery is closely coupled with the printing support. Leave as is for now. BUG=603559 ========== to ========== make chrome build without any printing support User complain that chrome doesn't build using their customized args.gn. See https://bugs.chromium.org/p/chromium/issues/detail?id=603559#c57. Their args.gn set enable_basic_printing, enable_print_preview and enable_service_discovery to false. This patch makes sure this flag setting can build successfully. BUG=603559 ==========
Thanks for the review! https://codereview.chromium.org/2884123003/diff/1/headless/lib/browser/headle... File headless/lib/browser/headless_devtools_manager_delegate.cc (right): https://codereview.chromium.org/2884123003/diff/1/headless/lib/browser/headle... headless/lib/browser/headless_devtools_manager_delegate.cc:94: // The max and min value should match the ones in scaling_settings.html. On 2017/05/17 07:01:24, Lei Zhang wrote: > Can you create another anonymous namespace for these? Done. https://codereview.chromium.org/2884123003/diff/1/pdf/BUILD.gn File pdf/BUILD.gn (right): https://codereview.chromium.org/2884123003/diff/1/pdf/BUILD.gn#newcode53 pdf/BUILD.gn:53: "//printing", On 2017/05/17 07:01:24, Lei Zhang wrote: > You don't need this, since pdf/ is only using printing/ headers for constant > values and not actually linking against it. Looks like it also use the methods from //printing. Without "//printing",there are link errors: ../../pdf/pdfium/pdfium_engine.cc:2892: error: undefined reference to 'printing::ConvertUnitDouble(double, double, double)' ../../pdf/pdfium/pdfium_engine.cc:2894: error: undefined reference to 'printing::ConvertUnitDouble(double, double, double)' ../../pdf/pdfium/pdfium_engine.cc:1490: error: undefined reference to 'printing::ConvertUnit(double, int, int)' ../../pdf/pdfium/pdfium_engine.cc:1492: error: undefined reference to 'printing::ConvertUnit(double, int, int)' ../../pdf/pdfium/pdfium_engine.cc:1418: error: undefined reference to 'printing::ConvertUnitDouble(double, double, double)' ../../pdf/pdfium/pdfium_engine.cc:1420: error: undefined reference to 'printing::ConvertUnitDouble(double, double, double)' ../../pdf/pdfium/pdfium_engine.cc:3407: error: undefined reference to 'printing::CalculateScaleFactor(gfx::Rect const&, double, double, bool)' ../../pdf/pdfium/pdfium_engine.cc:3419: error: undefined reference to 'printing::CalculateMediaBoxAndCropBox(bool, bool, bool, printing::PdfRectangle*, printing::PdfRectangle*)' ../../pdf/pdfium/pdfium_engine.cc:3422: error: undefined reference to 'printing::CalculateClipBoxBoundary(printing::PdfRectangle const&, printing::PdfRectangle const&)' ../../pdf/pdfium/pdfium_engine.cc:3423: error: undefined reference to 'printing::ScalePdfRectangle(double, printing::PdfRectangle*)' ../../pdf/pdfium/pdfium_engine.cc:3429: error: undefined reference to 'printing::CalculateScaledClipBoxOffset(gfx::Rect const&, printing::PdfRectangle const&, double*, double*)' ../../pdf/pdfium/pdfium_engine.cc:3432: error: undefined reference to 'printing::CalculateNonScaledClipBoxOffset(gfx::Rect const&, int, int, int, printing::PdfRectangle const&, double*, double*)' ../../pdf/pdfium/pdfium_engine.cc:2546: error: undefined reference to 'printing::ConvertUnit(double, int, int)' ../../pdf/pdfium/pdfium_engine.cc:2548: error: undefined reference to 'printing::ConvertUnit(double, int, int)'
On 2017/05/19 01:33:53, jzfeng wrote: > https://codereview.chromium.org/2884123003/diff/1/pdf/BUILD.gn > File pdf/BUILD.gn (right): > > https://codereview.chromium.org/2884123003/diff/1/pdf/BUILD.gn#newcode53 > pdf/BUILD.gn:53: "//printing", > On 2017/05/17 07:01:24, Lei Zhang wrote: > > You don't need this, since pdf/ is only using printing/ headers for constant > > values and not actually linking against it. > > Looks like it also use the methods from //printing. > Without "//printing",there are link errors: Whoops. Thanks for correcting my incorrect statement. Still LGTM.
The CQ bit was checked by jzfeng@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eseckler@chromium.org Link to the patchset: https://codereview.chromium.org/2884123003/#ps20001 (title: "nit fix")
> Whoops. Thanks for correcting my incorrect statement. Still LGTM. No problem :). Thanks for the review!
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1495158009621190,
"parent_rev": "06b3dd177f562f342eac5c4bf3e541b231afeda2", "commit_rev":
"f2df047e7dae8daf0f04abf9681fee1f4fcee3d9"}
Message was sent while issue was closed.
Description was changed from ========== make chrome build without any printing support User complain that chrome doesn't build using their customized args.gn. See https://bugs.chromium.org/p/chromium/issues/detail?id=603559#c57. Their args.gn set enable_basic_printing, enable_print_preview and enable_service_discovery to false. This patch makes sure this flag setting can build successfully. BUG=603559 ========== to ========== make chrome build without any printing support User complain that chrome doesn't build using their customized args.gn. See https://bugs.chromium.org/p/chromium/issues/detail?id=603559#c57. Their args.gn set enable_basic_printing, enable_print_preview and enable_service_discovery to false. This patch makes sure this flag setting can build successfully. BUG=603559 Review-Url: https://codereview.chromium.org/2884123003 Cr-Commit-Position: refs/heads/master@{#473057} Committed: https://chromium.googlesource.com/chromium/src/+/f2df047e7dae8daf0f04abf9681f... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/f2df047e7dae8daf0f04abf9681f... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
