Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(246)

Issue 2240433003: Print Scaling (Closed)

Created:
4 years, 4 months ago by rbpotter
Modified:
4 years, 1 month ago
CC:
arv+watch_chromium.org, chromium-reviews
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Add print scaling to printing/print_preview. BUG=659430 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/769ffdf779d83399a6f219e2637c6469ab94da0e Cr-Commit-Position: refs/heads/master@{#427556}

Patch Set 1 #

Patch Set 2 : Add UI. #

Patch Set 3 : Persist scaling in app state. #

Patch Set 4 : Clean up code, remove printing. #

Patch Set 5 : Diff reduction #

Patch Set 6 : Change fit to page/scaling interaction #

Patch Set 7 : Final fit to page fixes + cleanup #

Patch Set 8 : Clean up #

Patch Set 9 : Fix mac build error #

Patch Set 10 : Change inc/dec per UI feedback. Add flag. Fix tests. #

Total comments: 18

Patch Set 11 : Initial changes, add increment flexibility #

Total comments: 31

Patch Set 12 : More fixes, fix aria test failures #

Patch Set 13 : Set up flag #

Total comments: 36

Patch Set 14 : Simplify copies and scaling and fix issues from 13 #

Patch Set 15 : Merge #

Total comments: 24

Patch Set 16 : Updates for comments. #

Total comments: 10

Patch Set 17 : Change copies to native number element only #

Total comments: 12

Patch Set 18 : Fix JS issues. Fix test. #

Patch Set 19 : Rebase, move flag, fix multiple size case #

Total comments: 32

Patch Set 20 : Fix android #

Patch Set 21 : Clean up, fix animation issues for blank field #

Total comments: 10

Patch Set 22 : Small edits. #

Total comments: 1

Patch Set 23 : Merge #

Total comments: 8

Patch Set 24 : Final small fixes #

Unified diffs Side-by-side diffs Delta from patch set Stats (+742 lines, -28 lines) Patch
M chrome/app/generated_resources.grd View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 3 chunks +14 lines, -0 lines 0 comments Download
M chrome/browser/about_flags.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 1 chunk +5 lines, -0 lines 0 comments Download
M chrome/browser/resources/print_preview/data/app_state.js View 1 2 3 4 5 6 7 8 9 2 chunks +4 lines, -0 lines 0 comments Download
M chrome/browser/resources/print_preview/data/print_ticket_store.js View 1 2 3 4 5 6 7 4 chunks +21 lines, -0 lines 0 comments Download
A chrome/browser/resources/print_preview/data/ticket_items/scaling.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +101 lines, -0 lines 0 comments Download
M chrome/browser/resources/print_preview/native_layer.js View 1 2 3 4 5 6 7 3 chunks +7 lines, -2 lines 0 comments Download
M chrome/browser/resources/print_preview/preview_generator.js View 1 2 3 4 5 6 7 8 9 10 11 3 chunks +10 lines, -0 lines 0 comments Download
M chrome/browser/resources/print_preview/previewarea/preview_area.js View 1 7 8 9 10 11 1 chunk +4 lines, -0 lines 0 comments Download
M chrome/browser/resources/print_preview/print_preview.html View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/resources/print_preview/print_preview.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 9 chunks +49 lines, -0 lines 0 comments Download
A chrome/browser/resources/print_preview/settings/scaling_settings.html View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +13 lines, -0 lines 0 comments Download
A chrome/browser/resources/print_preview/settings/scaling_settings.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +289 lines, -0 lines 0 comments Download
M chrome/browser/ui/webui/print_preview/print_preview_ui.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 6 chunks +11 lines, -1 line 0 comments Download
M chrome/common/chrome_features.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +5 lines, -1 line 0 comments Download
M chrome/common/chrome_features.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 1 chunk +6 lines, -0 lines 0 comments Download
M chrome/test/data/webui/print_preview.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 5 chunks +48 lines, -1 line 0 comments Download
M components/printing/browser/print_manager_utils.cc View 1 chunk +1 line, -0 lines 0 comments Download
M components/printing/common/print_messages.h View 1 2 3 4 5 6 3 chunks +7 lines, -0 lines 0 comments Download
M components/printing/common/print_messages.cc View 1 2 3 4 5 6 7 3 chunks +3 lines, -2 lines 0 comments Download
M components/printing/renderer/print_web_view_helper.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 12 chunks +102 lines, -19 lines 0 comments Download
M components/printing/renderer/print_web_view_helper_mac.mm View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 1 chunk +1 line, -1 line 0 comments Download
M printing/print_job_constants.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 2 chunks +2 lines, -0 lines 0 comments Download
M printing/print_job_constants.cc View 1 2 3 4 5 2 chunks +6 lines, -0 lines 0 comments Download
M printing/print_settings.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 2 chunks +6 lines, -0 lines 0 comments Download
M printing/print_settings_conversion.cc View 1 2 3 2 chunks +4 lines, -1 line 0 comments Download
M printing/printing_context.cc View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M testing/variations/fieldtrial_testing_config.json View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 1 chunk +18 lines, -0 lines 0 comments Download
M tools/metrics/histograms/histograms.xml View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 3 chunks +3 lines, -0 lines 0 comments Download

Messages

Total messages: 139 (98 generated)
rbpotter
Lei, When you get a chance, could you take a look at this patch and ...
4 years, 4 months ago (2016-08-12 03:10:39 UTC) #6
dpapad
Posting some initial comments. https://codereview.chromium.org/2240433003/diff/240001/chrome/browser/resources/print_preview/data/ticket_items/scaling.js File chrome/browser/resources/print_preview/data/ticket_items/scaling.js (right): https://codereview.chromium.org/2240433003/diff/240001/chrome/browser/resources/print_preview/data/ticket_items/scaling.js#newcode50 chrome/browser/resources/print_preview/data/ticket_items/scaling.js:50: if (scaling < Scaling.MIN_VAL || ...
4 years, 2 months ago (2016-10-06 23:15:46 UTC) #27
rbpotter
https://codereview.chromium.org/2240433003/diff/240001/chrome/browser/resources/print_preview/data/ticket_items/scaling.js File chrome/browser/resources/print_preview/data/ticket_items/scaling.js (right): https://codereview.chromium.org/2240433003/diff/240001/chrome/browser/resources/print_preview/data/ticket_items/scaling.js#newcode50 chrome/browser/resources/print_preview/data/ticket_items/scaling.js:50: if (scaling < Scaling.MIN_VAL || scaling > Scaling.MAX_VAL) { ...
4 years, 2 months ago (2016-10-14 14:14:06 UTC) #33
Will Harris
is this ready for review yet, I was added to this 2hrs ago?
4 years, 2 months ago (2016-10-14 16:36:01 UTC) #34
dpapad
https://codereview.chromium.org/2240433003/diff/260001/chrome/browser/resources/print_preview/data/ticket_items/scaling.js File chrome/browser/resources/print_preview/data/ticket_items/scaling.js (right): https://codereview.chromium.org/2240433003/diff/260001/chrome/browser/resources/print_preview/data/ticket_items/scaling.js#newcode1 chrome/browser/resources/print_preview/data/ticket_items/scaling.js:1: // Copyright (c) 2012 The Chromium Authors. All rights ...
4 years, 2 months ago (2016-10-14 21:11:39 UTC) #35
Lei Zhang
On 2016/10/14 16:36:01, Will Harris wrote: > is this ready for review yet, I was ...
4 years, 2 months ago (2016-10-14 21:14:24 UTC) #36
Will Harris
https://codereview.chromium.org/2240433003/diff/260001/components/printing/common/print_messages.h File components/printing/common/print_messages.h (right): https://codereview.chromium.org/2240433003/diff/260001/components/printing/common/print_messages.h#newcode273 components/printing/common/print_messages.h:273: // Scaling % to fit to page where is ...
4 years, 2 months ago (2016-10-15 02:00:04 UTC) #37
rbpotter
I do not expect there will be any changes to the IPC messages, but there ...
4 years, 2 months ago (2016-10-16 20:09:52 UTC) #38
rbpotter
https://codereview.chromium.org/2240433003/diff/260001/chrome/browser/resources/print_preview/data/ticket_items/scaling.js File chrome/browser/resources/print_preview/data/ticket_items/scaling.js (right): https://codereview.chromium.org/2240433003/diff/260001/chrome/browser/resources/print_preview/data/ticket_items/scaling.js#newcode1 chrome/browser/resources/print_preview/data/ticket_items/scaling.js:1: // Copyright (c) 2012 The Chromium Authors. All rights ...
4 years, 2 months ago (2016-10-17 22:29:41 UTC) #48
Will Harris
components/printing/common/print_messages.[cc|h] lgtm https://codereview.chromium.org/2240433003/diff/260001/components/printing/common/print_messages.h File components/printing/common/print_messages.h (right): https://codereview.chromium.org/2240433003/diff/260001/components/printing/common/print_messages.h#newcode273 components/printing/common/print_messages.h:273: // Scaling % to fit to page ...
4 years, 2 months ago (2016-10-17 22:34:30 UTC) #49
Lei Zhang
https://codereview.chromium.org/2240433003/diff/320001/chrome/browser/ui/webui/print_preview/print_preview_ui.cc File chrome/browser/ui/webui/print_preview/print_preview_ui.cc (right): https://codereview.chromium.org/2240433003/diff/320001/chrome/browser/ui/webui/print_preview/print_preview_ui.cc#newcode416 chrome/browser/ui/webui/print_preview/print_preview_ui.cc:416: bool scaling_enabled = base::FeatureList::IsEnabled(kPrintScalingFeature); How does an end user ...
4 years, 2 months ago (2016-10-17 22:56:53 UTC) #50
dpapad
fYI, I am still going through the JS changes. Will continue tomorrow with copies_settings.js. https://codereview.chromium.org/2240433003/diff/320001/chrome/browser/resources/print_preview/native_layer.js ...
4 years, 2 months ago (2016-10-18 01:35:52 UTC) #51
dpapad
https://chromiumcodereview.appspot.com/2240433003/diff/320001/chrome/browser/resources/print_preview/settings/scaling_settings.html File chrome/browser/resources/print_preview/settings/scaling_settings.html (right): https://chromiumcodereview.appspot.com/2240433003/diff/320001/chrome/browser/resources/print_preview/settings/scaling_settings.html#newcode7 chrome/browser/resources/print_preview/settings/scaling_settings.html:7: <input class="user-value" type="text" value="100" maxlength="3" Have you considered using ...
4 years, 2 months ago (2016-10-18 18:57:33 UTC) #52
rbpotter
https://codereview.chromium.org/2240433003/diff/320001/chrome/browser/resources/print_preview/native_layer.js File chrome/browser/resources/print_preview/native_layer.js (right): https://codereview.chromium.org/2240433003/diff/320001/chrome/browser/resources/print_preview/native_layer.js#newcode670 chrome/browser/resources/print_preview/native_layer.js:670: var pageCountChangeEvent = new Event( On 2016/10/18 01:35:52, dpapad ...
4 years, 2 months ago (2016-10-19 02:54:04 UTC) #56
dpapad
https://codereview.chromium.org/2240433003/diff/440001/chrome/browser/resources/print_preview/data/ticket_items/scaling.js File chrome/browser/resources/print_preview/data/ticket_items/scaling.js (right): https://codereview.chromium.org/2240433003/diff/440001/chrome/browser/resources/print_preview/data/ticket_items/scaling.js#newcode48 chrome/browser/resources/print_preview/data/ticket_items/scaling.js:48: if (/[^\d]+/.test(value)) { I don't think this check is ...
4 years, 2 months ago (2016-10-19 20:56:47 UTC) #68
rbpotter
https://codereview.chromium.org/2240433003/diff/440001/chrome/browser/resources/print_preview/data/ticket_items/scaling.js File chrome/browser/resources/print_preview/data/ticket_items/scaling.js (right): https://codereview.chromium.org/2240433003/diff/440001/chrome/browser/resources/print_preview/data/ticket_items/scaling.js#newcode48 chrome/browser/resources/print_preview/data/ticket_items/scaling.js:48: if (/[^\d]+/.test(value)) { On 2016/10/19 20:56:46, dpapad wrote: > ...
4 years, 2 months ago (2016-10-20 02:28:58 UTC) #73
dpapad
Getting pretty close on the JS side. Few more questions. https://codereview.chromium.org/2240433003/diff/460001/chrome/browser/resources/print_preview/settings/copies_settings.css File chrome/browser/resources/print_preview/settings/copies_settings.css (right): https://codereview.chromium.org/2240433003/diff/460001/chrome/browser/resources/print_preview/settings/copies_settings.css#newcode5 ...
4 years, 2 months ago (2016-10-20 19:13:09 UTC) #76
rbpotter
On 2016/10/20 02:28:58, rbpotter wrote: > https://codereview.chromium.org/2240433003/diff/440001/chrome/browser/resources/print_preview/data/ticket_items/scaling.js > File chrome/browser/resources/print_preview/data/ticket_items/scaling.js > (right): > > https://codereview.chromium.org/2240433003/diff/440001/chrome/browser/resources/print_preview/data/ticket_items/scaling.js#newcode48 ...
4 years, 2 months ago (2016-10-20 19:14:44 UTC) #77
dpapad
> Received e-mail from UX approving change to native number element for copies & scaling. ...
4 years, 2 months ago (2016-10-20 19:18:50 UTC) #78
Lei Zhang
On 2016/10/20 19:18:50, dpapad wrote: > Can we land the copies change on a separate ...
4 years, 2 months ago (2016-10-20 22:22:29 UTC) #81
rbpotter
On 2016/10/20 22:22:29, Lei Zhang wrote: > On 2016/10/20 19:18:50, dpapad wrote: > > Can ...
4 years, 2 months ago (2016-10-20 22:29:30 UTC) #84
Lei Zhang
https://codereview.chromium.org/2240433003/diff/480001/chrome/browser/about_flags.cc File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2240433003/diff/480001/chrome/browser/about_flags.cc#newcode2076 chrome/browser/about_flags.cc:2076: #if defined(OS_CHROMEOS)||defined(OS_WIN)||defined(OS_LINUX)||defined(OS_MACOSX) Let's just do: #if defined(ENABLE_PRINT_PREVIEW) https://codereview.chromium.org/2240433003/diff/480001/chrome/browser/ui/webui/print_preview/print_preview_ui.cc File ...
4 years, 2 months ago (2016-10-20 22:47:12 UTC) #85
rbpotter
Addressed multiple page size case based on advice from UX. Rebased CL so copies changes ...
4 years, 2 months ago (2016-10-21 22:14:39 UTC) #96
dpapad
JS LGTM, with few (optional mostly) nits. https://codereview.chromium.org/2240433003/diff/600001/chrome/browser/resources/print_preview/print_preview.js File chrome/browser/resources/print_preview/print_preview.js (right): https://codereview.chromium.org/2240433003/diff/600001/chrome/browser/resources/print_preview/print_preview.js#newcode126 chrome/browser/resources/print_preview/print_preview.js:126: if (loadTimeData.getBoolean('scalingEnabled')) ...
4 years, 2 months ago (2016-10-22 00:30:25 UTC) #97
Lei Zhang
https://codereview.chromium.org/2240433003/diff/600001/chrome/browser/ui/webui/print_preview/print_preview_ui.cc File chrome/browser/ui/webui/print_preview/print_preview_ui.cc (right): https://codereview.chromium.org/2240433003/diff/600001/chrome/browser/ui/webui/print_preview/print_preview_ui.cc#newcode406 chrome/browser/ui/webui/print_preview/print_preview_ui.cc:406: features::kPrintScaling); nit: fits on previous line. https://codereview.chromium.org/2240433003/diff/600001/chrome/common/chrome_features.cc File chrome/common/chrome_features.cc ...
4 years, 2 months ago (2016-10-22 01:45:57 UTC) #99
rbpotter
https://codereview.chromium.org/2240433003/diff/600001/chrome/browser/resources/print_preview/print_preview.js File chrome/browser/resources/print_preview/print_preview.js (right): https://codereview.chromium.org/2240433003/diff/600001/chrome/browser/resources/print_preview/print_preview.js#newcode126 chrome/browser/resources/print_preview/print_preview.js:126: if (loadTimeData.getBoolean('scalingEnabled')) { On 2016/10/22 00:30:25, dpapad wrote: > ...
4 years, 2 months ago (2016-10-22 04:57:20 UTC) #102
Lei Zhang
https://codereview.chromium.org/2240433003/diff/600001/components/printing/renderer/print_web_view_helper.cc File components/printing/renderer/print_web_view_helper.cc (right): https://codereview.chromium.org/2240433003/diff/600001/components/printing/renderer/print_web_view_helper.cc#newcode454 components/printing/renderer/print_web_view_helper.cc:454: double* scale_factor) { On 2016/10/22 04:57:20, rbpotter wrote: > ...
4 years, 1 month ago (2016-10-24 23:56:38 UTC) #105
Lei Zhang
https://codereview.chromium.org/2240433003/diff/600001/components/printing/renderer/print_web_view_helper.cc File components/printing/renderer/print_web_view_helper.cc (right): https://codereview.chromium.org/2240433003/diff/600001/components/printing/renderer/print_web_view_helper.cc#newcode1583 components/printing/renderer/print_web_view_helper.cc:1583: CalculatePrintParamsForCss(frame, page_index, page_params, On 2016/10/22 04:57:20, rbpotter wrote: > ...
4 years, 1 month ago (2016-10-25 00:58:00 UTC) #106
rbpotter
https://codereview.chromium.org/2240433003/diff/600001/components/printing/renderer/print_web_view_helper.cc File components/printing/renderer/print_web_view_helper.cc (right): https://codereview.chromium.org/2240433003/diff/600001/components/printing/renderer/print_web_view_helper.cc#newcode1583 components/printing/renderer/print_web_view_helper.cc:1583: CalculatePrintParamsForCss(frame, page_index, page_params, On 2016/10/25 00:58:00, Lei Zhang wrote: ...
4 years, 1 month ago (2016-10-25 02:17:06 UTC) #108
rkaplow
lgtm histogram lg
4 years, 1 month ago (2016-10-25 15:31:45 UTC) #109
rbpotter
rkaplow: Do we need the modifications to fieldtrial_testing_config.json or not? We are not clear on ...
4 years, 1 month ago (2016-10-25 17:59:42 UTC) #116
Lei Zhang
lgtm Please check with rkaplow and get a clarification on the field trial config. https://codereview.chromium.org/2240433003/diff/700001/components/printing/renderer/print_web_view_helper.cc ...
4 years, 1 month ago (2016-10-25 21:42:53 UTC) #119
rkaplow
On 2016/10/25 21:42:53, Lei Zhang wrote: > lgtm > > Please check with rkaplow and ...
4 years, 1 month ago (2016-10-25 21:54:33 UTC) #120
rbpotter
https://codereview.chromium.org/2240433003/diff/700001/components/printing/renderer/print_web_view_helper.cc File components/printing/renderer/print_web_view_helper.cc (right): https://codereview.chromium.org/2240433003/diff/700001/components/printing/renderer/print_web_view_helper.cc#newcode473 components/printing/renderer/print_web_view_helper.cc:473: // and margins) if we are not fitting to ...
4 years, 1 month ago (2016-10-25 23:57:14 UTC) #123
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2240433003/720001
4 years, 1 month ago (2016-10-26 00:45:52 UTC) #128
commit-bot: I haz the power
Committed patchset #24 (id:720001)
4 years, 1 month ago (2016-10-26 00:54:22 UTC) #131
commit-bot: I haz the power
Patchset 24 (id:??) landed as https://crrev.com/769ffdf779d83399a6f219e2637c6469ab94da0e Cr-Commit-Position: refs/heads/master@{#427556}
4 years, 1 month ago (2016-10-26 00:58:24 UTC) #133
Nico
It looks like the tests this adds fail in official builds, see e.g. https://build.chromium.org/p/chromium.fyi/builders/ClangToTWin%20tester/builds/6500 Probably ...
4 years, 1 month ago (2016-10-31 14:44:36 UTC) #136
Nico
SourceIsPDFCapabilities is still broken. Final request for you to take action on this...
4 years, 1 month ago (2016-11-01 13:30:46 UTC) #137
rbpotter
On 2016/11/01 13:30:46, Nico (afk Tue Nov 1) wrote: > SourceIsPDFCapabilities is still broken. Final ...
4 years, 1 month ago (2016-11-01 19:12:03 UTC) #138
Nico
4 years, 1 month ago (2016-11-01 19:26:19 UTC) #139
Message was sent while issue was closed.
Thanks!

On Nov 1, 2016 3:12 PM, <rbpotter@chromium.org> wrote:

> On 2016/11/01 13:30:46, Nico (afk Tue Nov 1) wrote:
> > SourceIsPDFCapabilities is still broken. Final request for you to take
> action
> on
> > this...
>
> Missed a line on the first patch, sorry. This patch
> http://crrev.com/2466193002
> should fix it and has actually been in the works since last night. Landed
> this
> morning. This bot
> https://build.chromium.org/p/chromium.fyi/builders/CrWinClang%20tester
> was failing before and appears to now be passing all browser tests since
> picking
> up that revision.
>
> https://codereview.chromium.org/2240433003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

Powered by Google App Engine
This is Rietveld 408576698