|
|
DescriptionAdd 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 #Messages
Total messages: 139 (98 generated)
Description was changed from ========== Start on print preview scaling. Hardcoded value in native_layer.js (tested with several values, current value is 0.8) used to set print scaling. Uploading now to check that approach is reasonable while working on changes to UI code. Note: extra logging still present, will remove in final version. BUG= 96456 ========== to ========== Start on print preview scaling. Hardcoded value in native_layer.js (tested with several values, current value is 0.8) used to set print scaling. Uploading now to check that approach is reasonable while working on changes to UI code. Note: extra logging still present, will remove in final version. BUG= 96456 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== Start on print preview scaling. Hardcoded value in native_layer.js (tested with several values, current value is 0.8) used to set print scaling. Uploading now to check that approach is reasonable while working on changes to UI code. Note: extra logging still present, will remove in final version. BUG= 96456 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Start on print preview scaling. Hardcoded value in native_layer.js (tested with several values, current value is 0.8) used to set print scaling. Uploading now to check that approach is reasonable while working on changes to UI code. Note: extra logging still present, will remove in final version. BUG= 96456 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
rbpotter@chromium.org changed reviewers: + thestig@chromium.org
rbpotter@chromium.org changed reviewers: - thestig@chromium.org
rbpotter@chromium.org changed reviewers: + thestig@chromium.org
Lei, When you get a chance, could you take a look at this patch and tell me if the approach looks reasonable? I am particularly interested in having you check what I am doing in components/printing/renderer/print_view_helper.cc, as everything else is just passing the parameter from native_layer.js (where it is currently hardcoded) to that section of code. In the meantime, I am going to try to work on the javascript side of things so that the scaling parameter can come from the preview dialog rather than being hardcoded like it is currently. I am planning to largely duplicate the "copies" setting, with an additional check that if "Fit To Page" is showing and selected the scaling factor will have to be 100%. -Rebekah
Patchset #1 (id:1) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #7 (id:160001) has been deleted
Description was changed from ========== Start on print preview scaling. Hardcoded value in native_layer.js (tested with several values, current value is 0.8) used to set print scaling. Uploading now to check that approach is reasonable while working on changes to UI code. Note: extra logging still present, will remove in final version. BUG= 96456 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Add print scaling to printing/print_preview. ==========
The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run
rbpotter@chromium.org changed reviewers: + dpapad@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Description was changed from ========== Add print scaling to printing/print_preview. ========== to ========== Add print scaling to printing/print_preview. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by rbpotter@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by rbpotter@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Description was changed from ========== Add print scaling to printing/print_preview. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Add print scaling to printing/print_preview. Design Doc for context: https://docs.google.com/a/google.com/document/d/18PAkA1CVokrJdv05KQ75-xC_DiNv... CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== Add print scaling to printing/print_preview. Design Doc for context: https://docs.google.com/a/google.com/document/d/18PAkA1CVokrJdv05KQ75-xC_DiNv... CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Add print scaling to printing/print_preview. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Posting some initial comments. https://codereview.chromium.org/2240433003/diff/240001/chrome/browser/resourc... File chrome/browser/resources/print_preview/data/ticket_items/scaling.js (right): https://codereview.chromium.org/2240433003/diff/240001/chrome/browser/resourc... chrome/browser/resources/print_preview/data/ticket_items/scaling.js:50: if (scaling < Scaling.MIN_VAL || scaling > Scaling.MAX_VAL) { return scaling >= Scaling.MIN_VAL && scaling <= Scaling.MAX_VAL; https://codereview.chromium.org/2240433003/diff/240001/chrome/browser/resourc... chrome/browser/resources/print_preview/data/ticket_items/scaling.js:58: return (this.getValueAsNumber() == value); No need for parentheses. https://codereview.chromium.org/2240433003/diff/240001/chrome/browser/resourc... chrome/browser/resources/print_preview/data/ticket_items/scaling.js:66: getNearestValidValue: function(value) { Equivalent to return Math.min(Math.max(value, Scaling.MIN_VAL), Scaling.MAX_VAL); https://codereview.chromium.org/2240433003/diff/240001/chrome/browser/resourc... File chrome/browser/resources/print_preview/previewarea/preview_area.js (right): https://codereview.chromium.org/2240433003/diff/240001/chrome/browser/resourc... chrome/browser/resources/print_preview/previewarea/preview_area.js:319: this.tracker.add( Nit: This function is very verbose and mostly holds repetitive code. Can we clean this up as follows (on a separate CL)? var TicketStoreEvent = print_preview.PrintTicketStore.EventType; [ TicketStoreEvent.INITIALIZE, TicketStoreEvent.TICKET_CHANGE, TicketStoreEvent.CAPABILITIES_CHANGE TicketStoreEvent.DOCUMENT_CHANGE ].forEach(function(event) { this.tracker.add(this.printTicketStore_, event, this.onTicketChange_.bind(this)); }.bind(this)); [ this.printTicketStore_.color, this.printTicketStore_.cssBackground, this.printTicketStore_.customMargins, ... /// more settings here. ].forEach(function(setting) { this.tracker.add( setting, print_preview.ticket_items.TicketItem.EventType.CHANGE, this.onTicketChange_.bind(this)); }.bind(this)); https://codereview.chromium.org/2240433003/diff/240001/chrome/browser/resourc... File chrome/browser/resources/print_preview/settings/scaling_settings.css (right): https://codereview.chromium.org/2240433003/diff/240001/chrome/browser/resourc... chrome/browser/resources/print_preview/settings/scaling_settings.css:5: #scaling-settings input.scaling { Is copying the CSS from #copies-settings necessary? It seems almost identical. Can we simply re-use it (perhaps by using some common CSS class like settings-box)? Then the CSS would look as follows .settings-box { margin: 10px 0; } etc Both #copies-settings and #scaling-settings would have class="settings-box" in their HTML markup. https://codereview.chromium.org/2240433003/diff/240001/chrome/browser/resourc... File chrome/browser/resources/print_preview/settings/scaling_settings.html (right): https://codereview.chromium.org/2240433003/diff/240001/chrome/browser/resourc... chrome/browser/resources/print_preview/settings/scaling_settings.html:18: <span class="hint" i18n-content="scalingInstruction" aria-live="polite"> Could you try out using the newer $i18n{} mechanism instead of the old i18n-content, i18n-values instead? For example <span class="hint"aria-live="polite">$i18n{scalingInstruction}</span> If that does not work, I suggest contacting @dschuyler who implemented this mechanism. In general, substituting translations in C++ (before page is served) is an improvement over substituting translations in JS after the page has loaded (which is what i18n-content mechanism does, see https://cs.chromium.org/chromium/src/ui/webui/resources/js/i18n_template_no_p...). https://codereview.chromium.org/2240433003/diff/240001/chrome/browser/resourc... File chrome/browser/resources/print_preview/settings/scaling_settings.js (right): https://codereview.chromium.org/2240433003/diff/240001/chrome/browser/resourc... chrome/browser/resources/print_preview/settings/scaling_settings.js:22: * @type {!print_preview.ticket_items.Scaling} Since this is a new file, let's use the shorthand type declaration @private {!TypeGoesHere} https://codereview.chromium.org/2240433003/diff/240001/chrome/browser/resourc... chrome/browser/resources/print_preview/settings/scaling_settings.js:61: ScalingSettings.TEXTFIELD_DELAY_ = 250; Nit: TEXTFIELD_DELAY_MS_. It is fairly common to suffix ms time values with _ms_, see https://cs.chromium.org/search/?q=_MS_&sq=package:chromium&type=cs. https://codereview.chromium.org/2240433003/diff/240001/chrome/browser/ui/webu... File chrome/browser/ui/webui/print_preview/print_preview_ui.cc (right): https://codereview.chromium.org/2240433003/diff/240001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_ui.cc:501: Nit: Revert this?
The CQ bit was checked by rbpotter@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
rbpotter@chromium.org changed reviewers: + wfh@chromium.org
https://codereview.chromium.org/2240433003/diff/240001/chrome/browser/resourc... File chrome/browser/resources/print_preview/data/ticket_items/scaling.js (right): https://codereview.chromium.org/2240433003/diff/240001/chrome/browser/resourc... chrome/browser/resources/print_preview/data/ticket_items/scaling.js:50: if (scaling < Scaling.MIN_VAL || scaling > Scaling.MAX_VAL) { On 2016/10/06 23:15:45, dpapad wrote: > return scaling >= Scaling.MIN_VAL && scaling <= Scaling.MAX_VAL; Done. https://codereview.chromium.org/2240433003/diff/240001/chrome/browser/resourc... chrome/browser/resources/print_preview/data/ticket_items/scaling.js:58: return (this.getValueAsNumber() == value); On 2016/10/06 23:15:45, dpapad wrote: > No need for parentheses. Done. https://codereview.chromium.org/2240433003/diff/240001/chrome/browser/resourc... chrome/browser/resources/print_preview/data/ticket_items/scaling.js:66: getNearestValidValue: function(value) { On 2016/10/06 23:15:45, dpapad wrote: > Equivalent to > > return Math.min(Math.max(value, Scaling.MIN_VAL), Scaling.MAX_VAL); Done. https://codereview.chromium.org/2240433003/diff/240001/chrome/browser/resourc... File chrome/browser/resources/print_preview/previewarea/preview_area.js (right): https://codereview.chromium.org/2240433003/diff/240001/chrome/browser/resourc... chrome/browser/resources/print_preview/previewarea/preview_area.js:319: this.tracker.add( On 2016/10/06 23:15:45, dpapad wrote: > Nit: This function is very verbose and mostly holds repetitive code. Can we > clean this up as follows (on a separate CL)? > > var TicketStoreEvent = print_preview.PrintTicketStore.EventType; > > [ > TicketStoreEvent.INITIALIZE, > TicketStoreEvent.TICKET_CHANGE, > TicketStoreEvent.CAPABILITIES_CHANGE > TicketStoreEvent.DOCUMENT_CHANGE > ].forEach(function(event) { > this.tracker.add(this.printTicketStore_, event, > this.onTicketChange_.bind(this)); > }.bind(this)); > > > [ > this.printTicketStore_.color, > this.printTicketStore_.cssBackground, > this.printTicketStore_.customMargins, > ... /// more settings here. > ].forEach(function(setting) { > this.tracker.add( > setting, > print_preview.ticket_items.TicketItem.EventType.CHANGE, > this.onTicketChange_.bind(this)); > }.bind(this)); Acknowledged - will create a new CL for this later. https://codereview.chromium.org/2240433003/diff/240001/chrome/browser/resourc... File chrome/browser/resources/print_preview/settings/scaling_settings.css (right): https://codereview.chromium.org/2240433003/diff/240001/chrome/browser/resourc... chrome/browser/resources/print_preview/settings/scaling_settings.css:5: #scaling-settings input.scaling { On 2016/10/06 23:15:45, dpapad wrote: > Is copying the CSS from #copies-settings necessary? It seems almost identical. > Can we simply re-use it (perhaps by using some common CSS class like > settings-box)? > > Then the CSS would look as follows > > .settings-box { > margin: 10px 0; > } > etc > > Both #copies-settings and #scaling-settings would have class="settings-box" in > their HTML markup. Done. https://codereview.chromium.org/2240433003/diff/240001/chrome/browser/resourc... File chrome/browser/resources/print_preview/settings/scaling_settings.html (right): https://codereview.chromium.org/2240433003/diff/240001/chrome/browser/resourc... chrome/browser/resources/print_preview/settings/scaling_settings.html:18: <span class="hint" i18n-content="scalingInstruction" aria-live="polite"> On 2016/10/06 23:15:45, dpapad wrote: > Could you try out using the newer $i18n{} mechanism instead of the old > i18n-content, i18n-values instead? For example > > <span class="hint"aria-live="polite">$i18n{scalingInstruction}</span> > > If that does not work, I suggest contacting @dschuyler who implemented this > mechanism. In general, substituting translations in C++ (before page is served) > is an improvement over substituting translations in JS after the page has loaded > (which is what i18n-content mechanism does, see > https://cs.chromium.org/chromium/src/ui/webui/resources/js/i18n_template_no_p...). Done. https://codereview.chromium.org/2240433003/diff/240001/chrome/browser/resourc... File chrome/browser/resources/print_preview/settings/scaling_settings.js (right): https://codereview.chromium.org/2240433003/diff/240001/chrome/browser/resourc... chrome/browser/resources/print_preview/settings/scaling_settings.js:22: * @type {!print_preview.ticket_items.Scaling} On 2016/10/06 23:15:45, dpapad wrote: > Since this is a new file, let's use the shorthand type declaration > > @private {!TypeGoesHere} Done. https://codereview.chromium.org/2240433003/diff/240001/chrome/browser/resourc... chrome/browser/resources/print_preview/settings/scaling_settings.js:61: ScalingSettings.TEXTFIELD_DELAY_ = 250; On 2016/10/06 23:15:45, dpapad wrote: > Nit: TEXTFIELD_DELAY_MS_. It is fairly common to suffix ms time values with > _ms_, see https://cs.chromium.org/search/?q=_MS_&sq=package:chromium&type=cs. Done. https://codereview.chromium.org/2240433003/diff/240001/chrome/browser/ui/webu... File chrome/browser/ui/webui/print_preview/print_preview_ui.cc (right): https://codereview.chromium.org/2240433003/diff/240001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_ui.cc:501: On 2016/10/06 23:15:45, dpapad wrote: > Nit: Revert this? Done.
is this ready for review yet, I was added to this 2hrs ago?
https://codereview.chromium.org/2240433003/diff/260001/chrome/browser/resourc... File chrome/browser/resources/print_preview/data/ticket_items/scaling.js (right): https://codereview.chromium.org/2240433003/diff/260001/chrome/browser/resourc... chrome/browser/resources/print_preview/data/ticket_items/scaling.js:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. 2016 https://codereview.chromium.org/2240433003/diff/260001/chrome/browser/resourc... chrome/browser/resources/print_preview/data/ticket_items/scaling.js:94: Nit: Remove \n https://codereview.chromium.org/2240433003/diff/260001/chrome/browser/resourc... File chrome/browser/resources/print_preview/preview_generator.js (right): https://codereview.chromium.org/2240433003/diff/260001/chrome/browser/resourc... chrome/browser/resources/print_preview/preview_generator.js:97: * Ignored if fit to page is true. Nit (optional): Maximize 80col usage before breaking line. https://codereview.chromium.org/2240433003/diff/260001/chrome/browser/resourc... chrome/browser/resources/print_preview/preview_generator.js:98: * @type {int32_t} No such types exist for the JS compiler. @type {number} https://codereview.chromium.org/2240433003/diff/260001/chrome/browser/resourc... chrome/browser/resources/print_preview/preview_generator.js:290: !ticketStore.fitToPage.isValueEqual(this.isFitToPageEnabled_) || How come this line is not checking for fitToPage.isCapabilityAvailable? Is it necessary to check? https://codereview.chromium.org/2240433003/diff/260001/chrome/browser/resourc... File chrome/browser/resources/print_preview/previewarea/preview_area.js (right): https://codereview.chromium.org/2240433003/diff/260001/chrome/browser/resourc... chrome/browser/resources/print_preview/previewarea/preview_area.js:575: onTicketChange_: function(id) { Revert this? https://codereview.chromium.org/2240433003/diff/260001/chrome/browser/resourc... File chrome/browser/resources/print_preview/print_preview.js (right): https://codereview.chromium.org/2240433003/diff/260001/chrome/browser/resourc... chrome/browser/resources/print_preview/print_preview.js:127: * Component that renders the scaling settings. I think this entire comment block should be after the if(loadTimeData) line. https://codereview.chromium.org/2240433003/diff/260001/chrome/browser/resourc... chrome/browser/resources/print_preview/print_preview.js:134: this.printTicketStore_.fitToPage); Nit: Per styleguide, when passing args in multiple lines, start at the same column. new print_preview.ScalingSettings( this.printTicketStore_.scaling, this.printTicketStore_.fitToPage); OR new print_preview.ScalingSettings(this.printTicketStore_.scaling, this.printTicketStore_.fitToPage); This is actually from the C++ styleguide (which JS styleguide used to inherit), https://google.github.io/styleguide/cppguide.html#Function_Calls. https://codereview.chromium.org/2240433003/diff/260001/chrome/browser/resourc... chrome/browser/resources/print_preview/print_preview.js:206: var settingsSections = [ Let's not repeat the entire array. Just create the array once and insert scalingSettings_ at the right index. var settingsSections = [...]; if (loadTimeData.getBoolean('scalingEnabled')) settingsSections.splice(4, 0, this.scalingSettings_); https://codereview.chromium.org/2240433003/diff/260001/chrome/browser/resourc... chrome/browser/resources/print_preview/print_preview.js:380: this.tracker.add( Can we wrap this add() call in an if (loadTimeData....) check? Then we don't need to do this check in the listener, because the listener is not registered unnecessarily. https://codereview.chromium.org/2240433003/diff/260001/chrome/browser/resourc... chrome/browser/resources/print_preview/print_preview.js:1045: onPageCountReady_: function(event) { Why is this listener added in print_preview.js, to update something in scalingSettings_? Can we simply add the listener inside scalingSettings_ itself? Do other settings components do this? https://codereview.chromium.org/2240433003/diff/260001/chrome/browser/resourc... chrome/browser/resources/print_preview/print_preview.js:1046: if (loadTimeData.getBoolean('scalingEnabled') && Per comment at line 380, this check here would be unnecessary. https://codereview.chromium.org/2240433003/diff/260001/chrome/browser/resourc... File chrome/browser/resources/print_preview/settings/copies_settings.js (right): https://codereview.chromium.org/2240433003/diff/260001/chrome/browser/resourc... chrome/browser/resources/print_preview/settings/copies_settings.js:72: this.getChildElement('input.user-value').disabled = !isEnabled; This looks wrong. The two lines are now identical, I don't think that was the intention. (collate <input> is no longer enabled/disabled?) https://codereview.chromium.org/2240433003/diff/260001/chrome/browser/resourc... File chrome/browser/resources/print_preview/settings/scaling_settings.js (right): https://codereview.chromium.org/2240433003/diff/260001/chrome/browser/resourc... chrome/browser/resources/print_preview/settings/scaling_settings.js:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. 2016
On 2016/10/14 16:36:01, Will Harris wrote: > is this ready for review yet, I was added to this 2hrs ago? I imagine the changes to components/printing/common/print_messages.* is final-ish?
https://codereview.chromium.org/2240433003/diff/260001/components/printing/co... File components/printing/common/print_messages.h (right): https://codereview.chromium.org/2240433003/diff/260001/components/printing/co... components/printing/common/print_messages.h:273: // Scaling % to fit to page where is this in the struct above?
I do not expect there will be any changes to the IPC messages, but there will likely be some other changes as other reviewers provide feedback. I added you as due to the message changes this needs a security review. Not sure when in the process exactly that should happen; should I notify you when all other reviews/changes are complete? On Fri, Oct 14, 2016 at 9:36 AM, <wfh@chromium.org> wrote: > is this ready for review yet, I was added to this 2hrs ago? > > 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.
Patchset #12 (id:280001) has been deleted
The CQ bit was checked by rbpotter@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Exceeded global retry quota
The CQ bit was checked by rbpotter@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
https://codereview.chromium.org/2240433003/diff/260001/chrome/browser/resourc... File chrome/browser/resources/print_preview/data/ticket_items/scaling.js (right): https://codereview.chromium.org/2240433003/diff/260001/chrome/browser/resourc... chrome/browser/resources/print_preview/data/ticket_items/scaling.js:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2016/10/14 21:11:38, dpapad wrote: > 2016 Done. https://codereview.chromium.org/2240433003/diff/260001/chrome/browser/resourc... chrome/browser/resources/print_preview/data/ticket_items/scaling.js:94: On 2016/10/14 21:11:38, dpapad wrote: > Nit: Remove \n Done. https://codereview.chromium.org/2240433003/diff/260001/chrome/browser/resourc... File chrome/browser/resources/print_preview/preview_generator.js (right): https://codereview.chromium.org/2240433003/diff/260001/chrome/browser/resourc... chrome/browser/resources/print_preview/preview_generator.js:97: * Ignored if fit to page is true. On 2016/10/14 21:11:39, dpapad wrote: > Nit (optional): Maximize 80col usage before breaking line. Done. https://codereview.chromium.org/2240433003/diff/260001/chrome/browser/resourc... chrome/browser/resources/print_preview/preview_generator.js:98: * @type {int32_t} On 2016/10/14 21:11:39, dpapad wrote: > No such types exist for the JS compiler. > > @type {number} Done. https://codereview.chromium.org/2240433003/diff/260001/chrome/browser/resourc... chrome/browser/resources/print_preview/preview_generator.js:290: !ticketStore.fitToPage.isValueEqual(this.isFitToPageEnabled_) || On 2016/10/14 21:11:38, dpapad wrote: > How come this line is not checking for fitToPage.isCapabilityAvailable? Is it > necessary to check? This line isn't checking because it indicates the preview should be updated if fit to page has changed, regardless of whether that is because fit to page is now available or because it was changed by the user. I checked it in 288 because the "CapabilityNotAvailableValue" that is returned is often true, and I only wanted to prevent scaling from updating the preview if fit to page capability is available and has been selected by the user. However, I think I now have updated things so that the scaling ticket item will only change if fit to page is unavailable/off, so will remove the fit to page checks. https://codereview.chromium.org/2240433003/diff/260001/chrome/browser/resourc... File chrome/browser/resources/print_preview/previewarea/preview_area.js (right): https://codereview.chromium.org/2240433003/diff/260001/chrome/browser/resourc... chrome/browser/resources/print_preview/previewarea/preview_area.js:575: onTicketChange_: function(id) { On 2016/10/14 21:11:39, dpapad wrote: > Revert this? Done. https://codereview.chromium.org/2240433003/diff/260001/chrome/browser/resourc... File chrome/browser/resources/print_preview/print_preview.js (right): https://codereview.chromium.org/2240433003/diff/260001/chrome/browser/resourc... chrome/browser/resources/print_preview/print_preview.js:127: * Component that renders the scaling settings. On 2016/10/14 21:11:39, dpapad wrote: > I think this entire comment block should be after the if(loadTimeData) line. Done. https://codereview.chromium.org/2240433003/diff/260001/chrome/browser/resourc... chrome/browser/resources/print_preview/print_preview.js:134: this.printTicketStore_.fitToPage); On 2016/10/14 21:11:39, dpapad wrote: > Nit: Per styleguide, when passing args in multiple lines, start at the same > column. > > new print_preview.ScalingSettings( > this.printTicketStore_.scaling, > this.printTicketStore_.fitToPage); > > OR > > new print_preview.ScalingSettings(this.printTicketStore_.scaling, > this.printTicketStore_.fitToPage); > > This is actually from the C++ styleguide (which JS styleguide used to inherit), > https://google.github.io/styleguide/cppguide.html#Function_Calls. Done. https://codereview.chromium.org/2240433003/diff/260001/chrome/browser/resourc... chrome/browser/resources/print_preview/print_preview.js:206: var settingsSections = [ On 2016/10/14 21:11:39, dpapad wrote: > Let's not repeat the entire array. Just create the array once and insert > scalingSettings_ at the right index. > > var settingsSections = [...]; > > if (loadTimeData.getBoolean('scalingEnabled')) > settingsSections.splice(4, 0, this.scalingSettings_); Done. https://codereview.chromium.org/2240433003/diff/260001/chrome/browser/resourc... chrome/browser/resources/print_preview/print_preview.js:380: this.tracker.add( On 2016/10/14 21:11:39, dpapad wrote: > Can we wrap this add() call in an if (loadTimeData....) check? Then we don't > need to do this check in the listener, because the listener is not registered > unnecessarily. Done. https://codereview.chromium.org/2240433003/diff/260001/chrome/browser/resourc... chrome/browser/resources/print_preview/print_preview.js:1045: onPageCountReady_: function(event) { On 2016/10/14 21:11:39, dpapad wrote: > Why is this listener added in print_preview.js, to update something in > scalingSettings_? Can we simply add the listener inside scalingSettings_ itself? > Do other settings components do this? Other settings components don't do either as they don't require information from messages sent to the native layer, like the PageCount message. The reasoning for putting the listener here was to avoid linking the native layer directly to the scaling settings section as none of the other settings sections are linked directly to the native layer, while print_preview already receives messages from native layer events. I can try moving it to scaling settings if you think that is the better option; what would you recommend? https://codereview.chromium.org/2240433003/diff/260001/chrome/browser/resourc... chrome/browser/resources/print_preview/print_preview.js:1046: if (loadTimeData.getBoolean('scalingEnabled') && On 2016/10/14 21:11:39, dpapad wrote: > Per comment at line 380, this check here would be unnecessary. Done. https://codereview.chromium.org/2240433003/diff/260001/chrome/browser/resourc... File chrome/browser/resources/print_preview/settings/copies_settings.js (right): https://codereview.chromium.org/2240433003/diff/260001/chrome/browser/resourc... chrome/browser/resources/print_preview/settings/copies_settings.js:72: this.getChildElement('input.user-value').disabled = !isEnabled; On 2016/10/14 21:11:39, dpapad wrote: > This looks wrong. The two lines are now identical, I don't think that was the > intention. (collate <input> is no longer enabled/disabled?) Done. https://codereview.chromium.org/2240433003/diff/260001/chrome/browser/resourc... File chrome/browser/resources/print_preview/settings/scaling_settings.js (right): https://codereview.chromium.org/2240433003/diff/260001/chrome/browser/resourc... chrome/browser/resources/print_preview/settings/scaling_settings.js:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2016/10/14 21:11:39, dpapad wrote: > 2016 Done. https://codereview.chromium.org/2240433003/diff/260001/components/printing/co... File components/printing/common/print_messages.h (right): https://codereview.chromium.org/2240433003/diff/260001/components/printing/co... components/printing/common/print_messages.h:273: // Scaling % to fit to page On 2016/10/15 02:00:04, Will Harris wrote: > where is this in the struct above? Sorry, not sure I understand the question. This struct appears to only be defined here as far as I can tell, and I'm not sure why this parameter would need to be in other structs. Am I missing something?
components/printing/common/print_messages.[cc|h] lgtm https://codereview.chromium.org/2240433003/diff/260001/components/printing/co... File components/printing/common/print_messages.h (right): https://codereview.chromium.org/2240433003/diff/260001/components/printing/co... components/printing/common/print_messages.h:273: // Scaling % to fit to page On 2016/10/17 22:29:41, rbpotter wrote: > On 2016/10/15 02:00:04, Will Harris wrote: > > where is this in the struct above? > > Sorry, not sure I understand the question. This struct appears to only be > defined here as far as I can tell, and I'm not sure why this parameter would > need to be in other structs. Am I missing something? sorry my mistake, from eyeballing the diff I didn't notice this was in another IPC traits ("skipping nnn matching lines")
https://codereview.chromium.org/2240433003/diff/320001/chrome/browser/ui/webu... File chrome/browser/ui/webui/print_preview/print_preview_ui.cc (right): https://codereview.chromium.org/2240433003/diff/320001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_ui.cc:416: bool scaling_enabled = base::FeatureList::IsEnabled(kPrintScalingFeature); How does an end user enable this feature? There's no FieldTrial here so it's not obvious how the changes to testing/variations/fieldtrial_testing_config.json are suppose to work. https://codereview.chromium.org/2240433003/diff/320001/components/printing/re... File components/printing/renderer/print_web_view_helper.cc (right): https://codereview.chromium.org/2240433003/diff/320001/components/printing/re... components/printing/renderer/print_web_view_helper.cc:222: PageSizeMargins* page_layout_in_points, Put the out parameter last. https://codereview.chromium.org/2240433003/diff/320001/components/printing/re... components/printing/renderer/print_web_view_helper.cc:449: static_cast<double>(params.content_size.height()) / (*scale_factor))); You may want to write a helper function (or see if one already exists) to do the non-integer division + rounding. It'll make many of the changes here a bit easier to read. https://codereview.chromium.org/2240433003/diff/320001/printing/print_job_con... File printing/print_job_constants.h (right): https://codereview.chromium.org/2240433003/diff/320001/printing/print_job_con... printing/print_job_constants.h:29: PRINTING_EXPORT extern const char kSettingFitToPageScaling[]; Alphabetical order please.
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/resourc... File chrome/browser/resources/print_preview/native_layer.js (right): https://codereview.chromium.org/2240433003/diff/320001/chrome/browser/resourc... chrome/browser/resources/print_preview/native_layer.js:670: var pageCountChangeEvent = new Event( Nothing to do here, just a suggestion for a future cleanup. Creating an Event instance, and augmenting properties on that instance after construction is not great (for various reasons, for example what if a property collides with an already existing member var of Event?). A better approach is to use the more appropriate class CustomEvent (see https://developer.mozilla.org/en-US/docs/Web/API/CustomEvent/CustomEvent), where it has a |detail| field that can be populated at will as follows var myEvent = new CustomEvent(PAGE_COUNT_READY, {detail: { pageCount: pageCount, previewResponseId = previewResponseId, previewResponseId = previewResponseId, }}); Then when you receive the event in a listener you can dereference as e.detail.pageCount etc. https://codereview.chromium.org/2240433003/diff/320001/chrome/browser/resourc... File chrome/browser/resources/print_preview/print_preview.js (right): https://codereview.chromium.org/2240433003/diff/320001/chrome/browser/resourc... chrome/browser/resources/print_preview/print_preview.js:127: /** Indentation off by one. https://codereview.chromium.org/2240433003/diff/320001/chrome/browser/resourc... File chrome/browser/resources/print_preview/settings/scaling_settings.js (right): https://codereview.chromium.org/2240433003/diff/320001/chrome/browser/resourc... chrome/browser/resources/print_preview/settings/scaling_settings.js:34: * @private {!number} No need for "!" when declaring primitive types (string,number,boolean), since it is implied. https://codereview.chromium.org/2240433003/diff/320001/chrome/browser/resourc... chrome/browser/resources/print_preview/settings/scaling_settings.js:39: * Timeout used to delay processing of the scaling input. Mention the unit, sec? ms? https://codereview.chromium.org/2240433003/diff/320001/chrome/browser/resourc... chrome/browser/resources/print_preview/settings/scaling_settings.js:62: * @private {number} Add @const
https://chromiumcodereview.appspot.com/2240433003/diff/320001/chrome/browser/... File chrome/browser/resources/print_preview/settings/scaling_settings.html (right): https://chromiumcodereview.appspot.com/2240433003/diff/320001/chrome/browser/... chrome/browser/resources/print_preview/settings/scaling_settings.html:7: <input class="user-value" type="text" value="100" maxlength="3" Have you considered using a native numerical textfield which comes with its own increment/decrement buttons and disallows non-numerical input, out of the box? See example at https://jsfiddle.net/m5zo82br/1/. Not sure if the buttons can be styled with CSS, but perhaps worth exploring since it seems it would save quite a bit of code. https://chromiumcodereview.appspot.com/2240433003/diff/320001/chrome/browser/... File chrome/browser/resources/print_preview/settings/scaling_settings.js (right): https://chromiumcodereview.appspot.com/2240433003/diff/320001/chrome/browser/... chrome/browser/resources/print_preview/settings/scaling_settings.js:95: this.getChildElement('input.user-value'), Can this.getChildElement('input.user-value') be cached as a member var, given that it is called very often? https://chromiumcodereview.appspot.com/2240433003/diff/320001/chrome/browser/... chrome/browser/resources/print_preview/settings/scaling_settings.js:131: if (this.fitToPageTicketItem_.getValue() && The condition this.fitToPageTicketItem_.getValue() && this.fitToPageTicketItem_.isCapabilityAvailable() is repeated about 4-5 times in this class, if you also take into account the reverse !this.fitToPageTicketItem_.isCapabilityAvailable() || !this.fitToPageTicketItem_.isCapabilityAvailable() => !(this.fitToPageTicketItem_.getValue() && this.fitToPageTicketItem_.isCapabilityAvailable()) Can we package it in a helper private method with an appropriate name and re-use that? https://chromiumcodereview.appspot.com/2240433003/diff/320001/chrome/browser/... chrome/browser/resources/print_preview/settings/scaling_settings.js:132: this.fitToPageTicketItem_.isCapabilityAvailable()) { Perhaps change the order of those checks? It makes more sense to check if it is available first, before trying to inspect the value. https://chromiumcodereview.appspot.com/2240433003/diff/320001/chrome/browser/... chrome/browser/resources/print_preview/settings/scaling_settings.js:148: this.scalingTicketItem_.wouldValueBeValid(displayedValue + '') && What is the purpose of the + ''? Are you just corverting to a string? If so let's use the more explicit displayedValue.toString() instead (here and elsewhere). https://chromiumcodereview.appspot.com/2240433003/diff/320001/chrome/browser/... chrome/browser/resources/print_preview/settings/scaling_settings.js:151: // Display should be updated to match the ticket item unless displayed Nit: Move this comment outside before the if() at line 147, since it is more helpful to the reader at that point (same below). https://chromiumcodereview.appspot.com/2240433003/diff/320001/chrome/browser/... chrome/browser/resources/print_preview/settings/scaling_settings.js:220: var currentValue = parseInt( Let's make a helper and re-use, something to the effect of getUserInput_: function() { return this.getChildElement('input.user-value').value, 10); } https://chromiumcodereview.appspot.com/2240433003/diff/320001/chrome/browser/... chrome/browser/resources/print_preview/settings/scaling_settings.js:272: onTextfieldKeyDown_: function(event) { Nit: Instead of wrapping the entire function contents in an if, reverse the condition and return early onTextfieldKeyDown_: function(event) { if (event.keyCode != 13 /*enter*/) return; if (this.textfieldTimeout_) clearTimeout(this.textfieldTimeout_); this.onTextfieldTimeout_(); }, https://chromiumcodereview.appspot.com/2240433003/diff/320001/chrome/browser/... chrome/browser/resources/print_preview/settings/scaling_settings.js:273: if (event.keyCode == 13 /*enter*/) { Can you use event.key == 'Enter' instead?
Patchset #14 (id:340001) has been deleted
Patchset #14 (id:360001) has been deleted
Patchset #14 (id:380001) has been deleted
https://codereview.chromium.org/2240433003/diff/320001/chrome/browser/resourc... File chrome/browser/resources/print_preview/native_layer.js (right): https://codereview.chromium.org/2240433003/diff/320001/chrome/browser/resourc... chrome/browser/resources/print_preview/native_layer.js:670: var pageCountChangeEvent = new Event( On 2016/10/18 01:35:52, dpapad wrote: > Nothing to do here, just a suggestion for a future cleanup. Creating an Event > instance, and augmenting properties on that instance after construction is not > great (for various reasons, for example what if a property collides with an > already existing member var of Event?). > > A better approach is to use the more appropriate class CustomEvent (see > https://developer.mozilla.org/en-US/docs/Web/API/CustomEvent/CustomEvent), where > it has a |detail| field that can be populated at will as follows > > var myEvent = new CustomEvent(PAGE_COUNT_READY, {detail: { > pageCount: pageCount, > previewResponseId = previewResponseId, > previewResponseId = previewResponseId, > }}); > > Then when you receive the event in a listener you can dereference as > e.detail.pageCount etc. Acknowledged. https://codereview.chromium.org/2240433003/diff/320001/chrome/browser/resourc... File chrome/browser/resources/print_preview/print_preview.js (right): https://codereview.chromium.org/2240433003/diff/320001/chrome/browser/resourc... chrome/browser/resources/print_preview/print_preview.js:127: /** On 2016/10/18 01:35:52, dpapad wrote: > Indentation off by one. Done. https://codereview.chromium.org/2240433003/diff/320001/chrome/browser/resourc... File chrome/browser/resources/print_preview/settings/scaling_settings.html (right): https://codereview.chromium.org/2240433003/diff/320001/chrome/browser/resourc... chrome/browser/resources/print_preview/settings/scaling_settings.html:7: <input class="user-value" type="text" value="100" maxlength="3" On 2016/10/18 18:57:33, dpapad wrote: > Have you considered using a native numerical textfield which comes with its own > increment/decrement buttons and disallows non-numerical input, out of the box? > See example at https://jsfiddle.net/m5zo82br/1/. > > Not sure if the buttons can be styled with CSS, but perhaps worth exploring > since it seems it would save quite a bit of code. Done. I am not sure exactly how to get the buttons to have labels/aria labels, but otherwise this seems to work. I have also modified the copies settings so that the two fields will be consistent - if the scaling flag is on the copies settings will use the native field, otherwise they will look the same as they do currently. For now, I have left the old code, revised per other comments, commented out in scaling_settings.js in case UI does not like the new buttons. https://codereview.chromium.org/2240433003/diff/320001/chrome/browser/resourc... File chrome/browser/resources/print_preview/settings/scaling_settings.js (right): https://codereview.chromium.org/2240433003/diff/320001/chrome/browser/resourc... chrome/browser/resources/print_preview/settings/scaling_settings.js:34: * @private {!number} On 2016/10/18 01:35:52, dpapad wrote: > No need for "!" when declaring primitive types (string,number,boolean), since it > is implied. Done. https://codereview.chromium.org/2240433003/diff/320001/chrome/browser/resourc... chrome/browser/resources/print_preview/settings/scaling_settings.js:39: * Timeout used to delay processing of the scaling input. On 2016/10/18 01:35:52, dpapad wrote: > Mention the unit, sec? ms? Done. https://codereview.chromium.org/2240433003/diff/320001/chrome/browser/resourc... chrome/browser/resources/print_preview/settings/scaling_settings.js:62: * @private {number} On 2016/10/18 01:35:52, dpapad wrote: > Add @const Done. https://codereview.chromium.org/2240433003/diff/320001/chrome/browser/resourc... chrome/browser/resources/print_preview/settings/scaling_settings.js:95: this.getChildElement('input.user-value'), On 2016/10/18 18:57:33, dpapad wrote: > Can this.getChildElement('input.user-value') be cached as a member var, given > that it is called very often? Done. https://codereview.chromium.org/2240433003/diff/320001/chrome/browser/resourc... chrome/browser/resources/print_preview/settings/scaling_settings.js:131: if (this.fitToPageTicketItem_.getValue() && On 2016/10/18 18:57:33, dpapad wrote: > The condition > this.fitToPageTicketItem_.getValue() && > this.fitToPageTicketItem_.isCapabilityAvailable() > > is repeated about 4-5 times in this class, if you also take into account the > reverse > > !this.fitToPageTicketItem_.isCapabilityAvailable() || > !this.fitToPageTicketItem_.isCapabilityAvailable() => > !(this.fitToPageTicketItem_.getValue() && > this.fitToPageTicketItem_.isCapabilityAvailable()) > > Can we package it in a helper private method with an appropriate name and re-use > that? Done. https://codereview.chromium.org/2240433003/diff/320001/chrome/browser/resourc... chrome/browser/resources/print_preview/settings/scaling_settings.js:132: this.fitToPageTicketItem_.isCapabilityAvailable()) { On 2016/10/18 18:57:33, dpapad wrote: > Perhaps change the order of those checks? It makes more sense to check if it is > available first, before trying to inspect the value. Done. https://codereview.chromium.org/2240433003/diff/320001/chrome/browser/resourc... chrome/browser/resources/print_preview/settings/scaling_settings.js:148: this.scalingTicketItem_.wouldValueBeValid(displayedValue + '') && On 2016/10/18 18:57:33, dpapad wrote: > What is the purpose of the + ''? Are you just corverting to a string? If so > let's use the more explicit displayedValue.toString() instead (here and > elsewhere). Done. https://codereview.chromium.org/2240433003/diff/320001/chrome/browser/resourc... chrome/browser/resources/print_preview/settings/scaling_settings.js:151: // Display should be updated to match the ticket item unless displayed On 2016/10/18 18:57:33, dpapad wrote: > Nit: Move this comment outside before the if() at line 147, since it is more > helpful to the reader at that point (same below). Done. https://codereview.chromium.org/2240433003/diff/320001/chrome/browser/resourc... chrome/browser/resources/print_preview/settings/scaling_settings.js:220: var currentValue = parseInt( On 2016/10/18 18:57:33, dpapad wrote: > Let's make a helper and re-use, something to the effect of > > getUserInput_: function() { > return this.getChildElement('input.user-value').value, 10); > } Done. https://codereview.chromium.org/2240433003/diff/320001/chrome/browser/resourc... chrome/browser/resources/print_preview/settings/scaling_settings.js:272: onTextfieldKeyDown_: function(event) { On 2016/10/18 18:57:33, dpapad wrote: > Nit: Instead of wrapping the entire function contents in an if, reverse the > condition and return early > > onTextfieldKeyDown_: function(event) { > if (event.keyCode != 13 /*enter*/) > return; > > if (this.textfieldTimeout_) > clearTimeout(this.textfieldTimeout_); > this.onTextfieldTimeout_(); > }, Done. https://codereview.chromium.org/2240433003/diff/320001/chrome/browser/resourc... chrome/browser/resources/print_preview/settings/scaling_settings.js:273: if (event.keyCode == 13 /*enter*/) { On 2016/10/18 18:57:33, dpapad wrote: > Can you use event.key == 'Enter' instead? Done. https://codereview.chromium.org/2240433003/diff/320001/chrome/browser/ui/webu... File chrome/browser/ui/webui/print_preview/print_preview_ui.cc (right): https://codereview.chromium.org/2240433003/diff/320001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_ui.cc:416: bool scaling_enabled = base::FeatureList::IsEnabled(kPrintScalingFeature); On 2016/10/17 22:56:53, Lei Zhang wrote: > How does an end user enable this feature? There's no FieldTrial here so it's not > obvious how the changes to testing/variations/fieldtrial_testing_config.json are > suppose to work. Done - added to chrome flags so it now shows up as a feature in chrome://flags so can be enabled/disabled that way. https://codereview.chromium.org/2240433003/diff/320001/components/printing/re... File components/printing/renderer/print_web_view_helper.cc (right): https://codereview.chromium.org/2240433003/diff/320001/components/printing/re... components/printing/renderer/print_web_view_helper.cc:222: PageSizeMargins* page_layout_in_points, On 2016/10/17 22:56:53, Lei Zhang wrote: > Put the out parameter last. Done. https://codereview.chromium.org/2240433003/diff/320001/components/printing/re... components/printing/renderer/print_web_view_helper.cc:449: static_cast<double>(params.content_size.height()) / (*scale_factor))); On 2016/10/17 22:56:53, Lei Zhang wrote: > You may want to write a helper function (or see if one already exists) to do the > non-integer division + rounding. It'll make many of the changes here a bit > easier to read. Done. https://codereview.chromium.org/2240433003/diff/320001/printing/print_job_con... File printing/print_job_constants.h (right): https://codereview.chromium.org/2240433003/diff/320001/printing/print_job_con... printing/print_job_constants.h:29: PRINTING_EXPORT extern const char kSettingFitToPageScaling[]; On 2016/10/17 22:56:53, Lei Zhang wrote: > Alphabetical order please. Done.
The CQ bit was checked by rbpotter@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by rbpotter@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...
Patchset #15 (id:420001) has been deleted
The CQ bit was checked by rbpotter@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
https://codereview.chromium.org/2240433003/diff/440001/chrome/browser/resourc... File chrome/browser/resources/print_preview/data/ticket_items/scaling.js (right): https://codereview.chromium.org/2240433003/diff/440001/chrome/browser/resourc... chrome/browser/resources/print_preview/data/ticket_items/scaling.js:48: if (/[^\d]+/.test(value)) { I don't think this check is needed anymore, since the <input type="number"> element only allows integer numbers to be entered. https://codereview.chromium.org/2240433003/diff/440001/chrome/browser/resourc... chrome/browser/resources/print_preview/data/ticket_items/scaling.js:61: * Returns the nearest valid scaling percentage to value. Nit: You can probably delete this comment, since it is redundant with the @return comment. https://codereview.chromium.org/2240433003/diff/440001/chrome/browser/resourc... chrome/browser/resources/print_preview/data/ticket_items/scaling.js:63: * @return {number} Nearest valid scailng percentage Typo: s/scailng/scaling https://codereview.chromium.org/2240433003/diff/440001/chrome/browser/resourc... File chrome/browser/resources/print_preview/settings/scaling_settings.js (right): https://codereview.chromium.org/2240433003/diff/440001/chrome/browser/resourc... chrome/browser/resources/print_preview/settings/scaling_settings.js:116: /* this.tracker.add( Can we seek for an answer from UX to see if they have any objections with the small UI changes for the increment/decrement buttons. Perhaps send them before/after screenshots? If you can't get a hold of them, can we delete all the commented out code for now? In general I think getting an early approval for this UI change would simplify things a bit, because then we could land the "copies" related change independently of the "scaling" related changes (no need for copies_simple and copies). https://codereview.chromium.org/2240433003/diff/440001/chrome/browser/resourc... chrome/browser/resources/print_preview/settings/scaling_settings.js:136: * @return {bool} true if fit to page is available and selected. s/bool/boolean Nit (optional): Consider rephrasing comment as Whether fit to page is available and selected. This matches the example of the styleguide for documenting returned booleans, see https://google.github.io/styleguide/javascriptguide.xml?showone=Comments#Comm... ("Method and Function Comments" subsection). https://codereview.chromium.org/2240433003/diff/440001/chrome/browser/resourc... chrome/browser/resources/print_preview/settings/scaling_settings.js:190: var currentValue = this.scalingTicketItem_.getValueAsNumber(); currentValue no longer used. https://codereview.chromium.org/2240433003/diff/440001/chrome/browser/resourc... chrome/browser/resources/print_preview/settings/scaling_settings.js:230: scalingTicketItem_.getValue(); Nit: Fits in previous line? https://codereview.chromium.org/2240433003/diff/440001/chrome/browser/resourc... chrome/browser/resources/print_preview/settings/scaling_settings.js:268: if (scalingVal != '') { Nit: Reverse logic and return early if scalingVal == '', to make it more readable. https://codereview.chromium.org/2240433003/diff/440001/chrome/browser/resourc... chrome/browser/resources/print_preview/settings/scaling_settings.js:270: if (scalingVal == this.scalingTicketItem_.getValue()) Use {} if the body of the if extends more than one line. https://codereview.chromium.org/2240433003/diff/440001/chrome/browser/resourc... chrome/browser/resources/print_preview/settings/scaling_settings.js:275: parseInt(scalingVal, 10) != this.fitToPageScaling_) Same here. https://codereview.chromium.org/2240433003/diff/440001/chrome/browser/resourc... chrome/browser/resources/print_preview/settings/scaling_settings.js:288: * @param {Event} event Contains the key that was pressed. !Event https://codereview.chromium.org/2240433003/diff/440001/chrome/browser/resourc... File chrome/browser/resources/print_preview/settings/settings_box.css (right): https://codereview.chromium.org/2240433003/diff/440001/chrome/browser/resourc... chrome/browser/resources/print_preview/settings/settings_box.css:26: .settings-box button.increment, My understanding is that now that ScalingSettings is using <input type="number"> and class="simple-settings-box", all the rules in this file are only used by the "old" copies UI. Can we revert (move those rules back to copies_settings.css and delete this file)?
The CQ bit was checked by rbpotter@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
https://codereview.chromium.org/2240433003/diff/440001/chrome/browser/resourc... File chrome/browser/resources/print_preview/data/ticket_items/scaling.js (right): https://codereview.chromium.org/2240433003/diff/440001/chrome/browser/resourc... chrome/browser/resources/print_preview/data/ticket_items/scaling.js:48: if (/[^\d]+/.test(value)) { On 2016/10/19 20:56:46, dpapad wrote: > I don't think this check is needed anymore, since the <input type="number"> > element only allows integer numbers to be entered. It actually also allows floating point from what I can tell. So this check prevents any floating point numbers from getting through. I modified the scaling hint so people can understand that only integers are allowed. We could also make scaling floating point at some point if desired; obviously copies would need to remain whole numbers only. https://codereview.chromium.org/2240433003/diff/440001/chrome/browser/resourc... chrome/browser/resources/print_preview/data/ticket_items/scaling.js:61: * Returns the nearest valid scaling percentage to value. On 2016/10/19 20:56:47, dpapad wrote: > Nit: You can probably delete this comment, since it is redundant with the > @return comment. Done. https://codereview.chromium.org/2240433003/diff/440001/chrome/browser/resourc... chrome/browser/resources/print_preview/data/ticket_items/scaling.js:63: * @return {number} Nearest valid scailng percentage On 2016/10/19 20:56:46, dpapad wrote: > Typo: s/scailng/scaling Done. https://codereview.chromium.org/2240433003/diff/440001/chrome/browser/resourc... File chrome/browser/resources/print_preview/settings/scaling_settings.js (right): https://codereview.chromium.org/2240433003/diff/440001/chrome/browser/resourc... chrome/browser/resources/print_preview/settings/scaling_settings.js:116: /* this.tracker.add( On 2016/10/19 20:56:47, dpapad wrote: > Can we seek for an answer from UX to see if they have any objections with the > small UI changes for the increment/decrement buttons. Perhaps send them > before/after screenshots? > > If you can't get a hold of them, can we delete all the commented out code for > now? > > In general I think getting an early approval for this UI change would simplify > things a bit, because then we could land the "copies" related change > independently of the "scaling" related changes (no need for copies_simple and > copies). Sent them an e-mail. Will see when they have time to get to it. I did track down a previous e-mail they sent with some concepts for a new print preview in which the copies field looked much more similar to the input number than it did to its current appearance with the +/- buttons, so it seems likely they will be okay with it. https://codereview.chromium.org/2240433003/diff/440001/chrome/browser/resourc... chrome/browser/resources/print_preview/settings/scaling_settings.js:136: * @return {bool} true if fit to page is available and selected. On 2016/10/19 20:56:47, dpapad wrote: > s/bool/boolean > > Nit (optional): Consider rephrasing comment as > Whether fit to page is available and selected. > > This matches the example of the styleguide for documenting returned booleans, > see > https://google.github.io/styleguide/javascriptguide.xml?showone=Comments#Comm... > ("Method and Function Comments" subsection). Done. https://codereview.chromium.org/2240433003/diff/440001/chrome/browser/resourc... chrome/browser/resources/print_preview/settings/scaling_settings.js:190: var currentValue = this.scalingTicketItem_.getValueAsNumber(); On 2016/10/19 20:56:47, dpapad wrote: > currentValue no longer used. Done. https://codereview.chromium.org/2240433003/diff/440001/chrome/browser/resourc... chrome/browser/resources/print_preview/settings/scaling_settings.js:230: scalingTicketItem_.getValue(); On 2016/10/19 20:56:47, dpapad wrote: > Nit: Fits in previous line? Done. https://codereview.chromium.org/2240433003/diff/440001/chrome/browser/resourc... chrome/browser/resources/print_preview/settings/scaling_settings.js:268: if (scalingVal != '') { On 2016/10/19 20:56:47, dpapad wrote: > Nit: Reverse logic and return early if scalingVal == '', to make it more > readable. Done. https://codereview.chromium.org/2240433003/diff/440001/chrome/browser/resourc... chrome/browser/resources/print_preview/settings/scaling_settings.js:270: if (scalingVal == this.scalingTicketItem_.getValue()) On 2016/10/19 20:56:47, dpapad wrote: > Use {} if the body of the if extends more than one line. Done. https://codereview.chromium.org/2240433003/diff/440001/chrome/browser/resourc... chrome/browser/resources/print_preview/settings/scaling_settings.js:275: parseInt(scalingVal, 10) != this.fitToPageScaling_) On 2016/10/19 20:56:47, dpapad wrote: > Same here. Done. https://codereview.chromium.org/2240433003/diff/440001/chrome/browser/resourc... chrome/browser/resources/print_preview/settings/scaling_settings.js:288: * @param {Event} event Contains the key that was pressed. On 2016/10/19 20:56:47, dpapad wrote: > !Event Done. https://codereview.chromium.org/2240433003/diff/440001/chrome/browser/resourc... File chrome/browser/resources/print_preview/settings/settings_box.css (right): https://codereview.chromium.org/2240433003/diff/440001/chrome/browser/resourc... chrome/browser/resources/print_preview/settings/settings_box.css:26: .settings-box button.increment, On 2016/10/19 20:56:47, dpapad wrote: > My understanding is that now that ScalingSettings is using <input type="number"> > and class="simple-settings-box", all the rules in this file are only used by the > "old" copies UI. Can we revert (move those rules back to copies_settings.css and > delete this file)? Done.
The CQ bit was checked by rbpotter@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...
Getting pretty close on the JS side. Few more questions. https://codereview.chromium.org/2240433003/diff/460001/chrome/browser/resourc... File chrome/browser/resources/print_preview/settings/copies_settings.css (right): https://codereview.chromium.org/2240433003/diff/460001/chrome/browser/resourc... chrome/browser/resources/print_preview/settings/copies_settings.css:5: #copies-settings input.user-value { Would things still work if we fully revert copies_settings.css,html,js? https://codereview.chromium.org/2240433003/diff/460001/chrome/browser/resourc... File chrome/browser/resources/print_preview/settings/scaling_settings.js (right): https://codereview.chromium.org/2240433003/diff/460001/chrome/browser/resourc... chrome/browser/resources/print_preview/settings/scaling_settings.js:114: * @return {bool} true if fit to page is available and selected. s/bool/boolean https://codereview.chromium.org/2240433003/diff/460001/chrome/browser/resourc... chrome/browser/resources/print_preview/settings/scaling_settings.js:124: * @return {number} current input field value Remove redundant comment? @return {number} The current input field value as an integer. https://codereview.chromium.org/2240433003/diff/460001/chrome/browser/resourc... chrome/browser/resources/print_preview/settings/scaling_settings.js:128: return this.inputField_.value; The <input>'s value is always string, even if type="number". See example of this and how it can cause a subtle bug at https://jsfiddle.net/dc2dns9w/1/. Let's return parseInt(this.inputField_.value, 10); https://codereview.chromium.org/2240433003/diff/460001/chrome/browser/resourc... chrome/browser/resources/print_preview/settings/scaling_settings.js:154: if (displayedValue != this.scalingTicketItem_.getValue() && Are we comparing a string to a number here? If so, this is brittle. Can you use '!==' instead of '!=' and see if this still works? (Same for other places where we call scalingTicketItem_.getValue()). Or alternatively, should we be calling scalingTicketItem_.getValueAsNumber() instead?
On 2016/10/20 02:28:58, rbpotter wrote: > https://codereview.chromium.org/2240433003/diff/440001/chrome/browser/resourc... > File chrome/browser/resources/print_preview/data/ticket_items/scaling.js > (right): > > https://codereview.chromium.org/2240433003/diff/440001/chrome/browser/resourc... > chrome/browser/resources/print_preview/data/ticket_items/scaling.js:48: if > (/[^\d]+/.test(value)) { > On 2016/10/19 20:56:46, dpapad wrote: > > I don't think this check is needed anymore, since the <input type="number"> > > element only allows integer numbers to be entered. > > It actually also allows floating point from what I can tell. So this check > prevents any floating point numbers from getting through. I modified the scaling > hint so people can understand that only integers are allowed. We could also make > scaling floating point at some point if desired; obviously copies would need to > remain whole numbers only. > > https://codereview.chromium.org/2240433003/diff/440001/chrome/browser/resourc... > chrome/browser/resources/print_preview/data/ticket_items/scaling.js:61: * > Returns the nearest valid scaling percentage to value. > On 2016/10/19 20:56:47, dpapad wrote: > > Nit: You can probably delete this comment, since it is redundant with the > > @return comment. > > Done. > > https://codereview.chromium.org/2240433003/diff/440001/chrome/browser/resourc... > chrome/browser/resources/print_preview/data/ticket_items/scaling.js:63: * > @return {number} Nearest valid scailng percentage > On 2016/10/19 20:56:46, dpapad wrote: > > Typo: s/scailng/scaling > > Done. > > https://codereview.chromium.org/2240433003/diff/440001/chrome/browser/resourc... > File chrome/browser/resources/print_preview/settings/scaling_settings.js > (right): > > https://codereview.chromium.org/2240433003/diff/440001/chrome/browser/resourc... > chrome/browser/resources/print_preview/settings/scaling_settings.js:116: /* > this.tracker.add( > On 2016/10/19 20:56:47, dpapad wrote: > > Can we seek for an answer from UX to see if they have any objections with the > > small UI changes for the increment/decrement buttons. Perhaps send them > > before/after screenshots? > > > > If you can't get a hold of them, can we delete all the commented out code for > > now? > > > > In general I think getting an early approval for this UI change would simplify > > things a bit, because then we could land the "copies" related change > > independently of the "scaling" related changes (no need for copies_simple and > > copies). > > Sent them an e-mail. Will see when they have time to get to it. I did track down > a previous e-mail they sent with some concepts for a new print preview in which > the copies field looked much more similar to the input number than it did to its > current appearance with the +/- buttons, so it seems likely they will be okay > with it. > > https://codereview.chromium.org/2240433003/diff/440001/chrome/browser/resourc... > chrome/browser/resources/print_preview/settings/scaling_settings.js:136: * > @return {bool} true if fit to page is available and selected. > On 2016/10/19 20:56:47, dpapad wrote: > > s/bool/boolean > > > > Nit (optional): Consider rephrasing comment as > > Whether fit to page is available and selected. > > > > This matches the example of the styleguide for documenting returned booleans, > > see > > > https://google.github.io/styleguide/javascriptguide.xml?showone=Comments#Comm... > > ("Method and Function Comments" subsection). > > Done. > > https://codereview.chromium.org/2240433003/diff/440001/chrome/browser/resourc... > chrome/browser/resources/print_preview/settings/scaling_settings.js:190: var > currentValue = this.scalingTicketItem_.getValueAsNumber(); > On 2016/10/19 20:56:47, dpapad wrote: > > currentValue no longer used. > > Done. > > https://codereview.chromium.org/2240433003/diff/440001/chrome/browser/resourc... > chrome/browser/resources/print_preview/settings/scaling_settings.js:230: > scalingTicketItem_.getValue(); > On 2016/10/19 20:56:47, dpapad wrote: > > Nit: Fits in previous line? > > Done. > > https://codereview.chromium.org/2240433003/diff/440001/chrome/browser/resourc... > chrome/browser/resources/print_preview/settings/scaling_settings.js:268: if > (scalingVal != '') { > On 2016/10/19 20:56:47, dpapad wrote: > > Nit: Reverse logic and return early if scalingVal == '', to make it more > > readable. > > Done. > > https://codereview.chromium.org/2240433003/diff/440001/chrome/browser/resourc... > chrome/browser/resources/print_preview/settings/scaling_settings.js:270: if > (scalingVal == this.scalingTicketItem_.getValue()) > On 2016/10/19 20:56:47, dpapad wrote: > > Use {} if the body of the if extends more than one line. > > Done. > > https://codereview.chromium.org/2240433003/diff/440001/chrome/browser/resourc... > chrome/browser/resources/print_preview/settings/scaling_settings.js:275: > parseInt(scalingVal, 10) != this.fitToPageScaling_) > On 2016/10/19 20:56:47, dpapad wrote: > > Same here. > > Done. > > https://codereview.chromium.org/2240433003/diff/440001/chrome/browser/resourc... > chrome/browser/resources/print_preview/settings/scaling_settings.js:288: * > @param {Event} event Contains the key that was pressed. > On 2016/10/19 20:56:47, dpapad wrote: > > !Event > > Done. > > https://codereview.chromium.org/2240433003/diff/440001/chrome/browser/resourc... > File chrome/browser/resources/print_preview/settings/settings_box.css (right): > > https://codereview.chromium.org/2240433003/diff/440001/chrome/browser/resourc... > chrome/browser/resources/print_preview/settings/settings_box.css:26: > .settings-box button.increment, > On 2016/10/19 20:56:47, dpapad wrote: > > My understanding is that now that ScalingSettings is using <input > type="number"> > > and class="simple-settings-box", all the rules in this file are only used by > the > > "old" copies UI. Can we revert (move those rules back to copies_settings.css > and > > delete this file)? > > Done. Received e-mail from UX approving change to native number element for copies & scaling. They prefer the default element (no visible buttons until mouse is hovered). Latest patchset removes the old CSS/HTML/JS for copies and replaces it with only the new code (native number element), so copies appearance will change regardless of the status of the scaling feature flag.
> Received e-mail from UX approving change to native number element for copies & scaling. They prefer the default element (no visible buttons until mouse is hovered). Latest patchset removes the old CSS/HTML/JS for copies and replaces it with only the new code (native number element), so copies appearance will change regardless of the status of the scaling feature flag. That is great news. Thanks for following up with UX. I agree that the native look is more appealing, and the way we were lining up the buttons with the textfield, felt a bit odd. Can we land the copies change on a separate CL then, before this CL lands? Then you can simply rebase this CL, which would only contain changes related to print scaling.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
On 2016/10/20 19:18:50, dpapad wrote: > Can we land the copies change on a separate CL then, before this CL lands? Then > you can simply rebase this CL, which would only contain changes related to print > scaling. +1 BTW, there's still some red bots...
The CQ bit was checked by rbpotter@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...
On 2016/10/20 22:22:29, Lei Zhang wrote: > On 2016/10/20 19:18:50, dpapad wrote: > > Can we land the copies change on a separate CL then, before this CL lands? > Then > > you can simply rebase this CL, which would only contain changes related to > print > > scaling. > > +1 > > BTW, there's still some red bots... This is http://crrev.com/2441493004 I broke the bots as I forgot to rename the element back to normal for the copies browser test when I changed copies back to only 1 version in the last patch. Should be fixed now.
https://codereview.chromium.org/2240433003/diff/480001/chrome/browser/about_f... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2240433003/diff/480001/chrome/browser/about_f... 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/webu... File chrome/browser/ui/webui/print_preview/print_preview_ui.cc (right): https://codereview.chromium.org/2240433003/diff/480001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_ui.cc:57: const base::Feature kPrintScaling { Can we move this into chrome/common/chrome_features.cc? https://codereview.chromium.org/2240433003/diff/480001/chrome/common/cloud_pr... File chrome/common/cloud_print/cloud_print_cdd_conversion.cc (right): https://codereview.chromium.org/2240433003/diff/480001/chrome/common/cloud_pr... chrome/common/cloud_print/cloud_print_cdd_conversion.cc:126: ScalingCapability scaling; I'm curious what happens if we don't add this here and if we leave components/cloud_devices/common/printer_description.* alone. https://codereview.chromium.org/2240433003/diff/480001/components/printing/re... File components/printing/renderer/print_web_view_helper.cc (right): https://codereview.chromium.org/2240433003/diff/480001/components/printing/re... components/printing/renderer/print_web_view_helper.cc:231: if (scale_factor > 0 &&(fit_to_page || params.print_to_pdf)) { nit: missing space after "&&" https://codereview.chromium.org/2240433003/diff/480001/components/printing/re... components/printing/renderer/print_web_view_helper.cc:232: LOG(ERROR) << "Scaing content. Scale factor = " << scale_factor; Debug message. https://codereview.chromium.org/2240433003/diff/480001/components/printing/re... components/printing/renderer/print_web_view_helper.cc:1259: static_cast<double>(preset_options.uniformPageSize.width); Does accessing |uniformPageSize| work here if the PDF has mixed page sizes?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Patchset #19 (id:520001) has been deleted
Patchset #19 (id:540001) has been deleted
Patchset #19 (id:560001) has been deleted
The CQ bit was checked by rbpotter@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...
Patchset #19 (id:580001) has been deleted
The CQ bit was checked by rbpotter@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...
Addressed multiple page size case based on advice from UX. Rebased CL so copies changes are no longer included. https://codereview.chromium.org/2240433003/diff/460001/chrome/browser/resourc... File chrome/browser/resources/print_preview/settings/copies_settings.css (right): https://codereview.chromium.org/2240433003/diff/460001/chrome/browser/resourc... chrome/browser/resources/print_preview/settings/copies_settings.css:5: #copies-settings input.user-value { On 2016/10/20 19:13:08, dpapad wrote: > Would things still work if we fully revert copies_settings.css,html,js? Think this no longer applies. https://codereview.chromium.org/2240433003/diff/460001/chrome/browser/resourc... File chrome/browser/resources/print_preview/settings/scaling_settings.js (right): https://codereview.chromium.org/2240433003/diff/460001/chrome/browser/resourc... chrome/browser/resources/print_preview/settings/scaling_settings.js:114: * @return {bool} true if fit to page is available and selected. On 2016/10/20 19:13:08, dpapad wrote: > s/bool/boolean Done. https://codereview.chromium.org/2240433003/diff/460001/chrome/browser/resourc... chrome/browser/resources/print_preview/settings/scaling_settings.js:124: * @return {number} current input field value On 2016/10/20 19:13:08, dpapad wrote: > Remove redundant comment? > > @return {number} The current input field value as an integer. Done. https://codereview.chromium.org/2240433003/diff/460001/chrome/browser/resourc... chrome/browser/resources/print_preview/settings/scaling_settings.js:128: return this.inputField_.value; On 2016/10/20 19:13:08, dpapad wrote: > The <input>'s value is always string, even if type="number". See example of this > and how it can cause a subtle bug at https://jsfiddle.net/dc2dns9w/1/. > > Let's > return parseInt(this.inputField_.value, 10); Done. https://codereview.chromium.org/2240433003/diff/460001/chrome/browser/resourc... chrome/browser/resources/print_preview/settings/scaling_settings.js:154: if (displayedValue != this.scalingTicketItem_.getValue() && On 2016/10/20 19:13:08, dpapad wrote: > Are we comparing a string to a number here? If so, this is brittle. Can you use > '!==' instead of '!=' and see if this still works? (Same for other places where > we call scalingTicketItem_.getValue()). > > Or alternatively, should we be calling scalingTicketItem_.getValueAsNumber() > instead? Done. https://codereview.chromium.org/2240433003/diff/480001/chrome/browser/about_f... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2240433003/diff/480001/chrome/browser/about_f... chrome/browser/about_flags.cc:2076: #if defined(OS_CHROMEOS)||defined(OS_WIN)||defined(OS_LINUX)||defined(OS_MACOSX) On 2016/10/20 22:47:12, Lei Zhang wrote: > Let's just do: > > #if defined(ENABLE_PRINT_PREVIEW) Done. https://codereview.chromium.org/2240433003/diff/480001/chrome/browser/ui/webu... File chrome/browser/ui/webui/print_preview/print_preview_ui.cc (right): https://codereview.chromium.org/2240433003/diff/480001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_ui.cc:57: const base::Feature kPrintScaling { On 2016/10/20 22:47:12, Lei Zhang wrote: > Can we move this into chrome/common/chrome_features.cc? Done. https://codereview.chromium.org/2240433003/diff/480001/chrome/common/cloud_pr... File chrome/common/cloud_print/cloud_print_cdd_conversion.cc (right): https://codereview.chromium.org/2240433003/diff/480001/chrome/common/cloud_pr... chrome/common/cloud_print/cloud_print_cdd_conversion.cc:126: ScalingCapability scaling; On 2016/10/20 22:47:12, Lei Zhang wrote: > I'm curious what happens if we don't add this here and if we leave > components/cloud_devices/common/printer_description.* alone. Done - don't seem to need it since scaling isn't a capability determined by the destination. Not sure why fit to page is in here either. https://codereview.chromium.org/2240433003/diff/480001/components/printing/re... File components/printing/renderer/print_web_view_helper.cc (right): https://codereview.chromium.org/2240433003/diff/480001/components/printing/re... components/printing/renderer/print_web_view_helper.cc:231: if (scale_factor > 0 &&(fit_to_page || params.print_to_pdf)) { On 2016/10/20 22:47:12, Lei Zhang wrote: > nit: missing space after "&&" Done. https://codereview.chromium.org/2240433003/diff/480001/components/printing/re... components/printing/renderer/print_web_view_helper.cc:232: LOG(ERROR) << "Scaing content. Scale factor = " << scale_factor; On 2016/10/20 22:47:12, Lei Zhang wrote: > Debug message. Done. https://codereview.chromium.org/2240433003/diff/480001/components/printing/re... components/printing/renderer/print_web_view_helper.cc:1259: static_cast<double>(preset_options.uniformPageSize.width); On 2016/10/20 22:47:12, Lei Zhang wrote: > Does accessing |uniformPageSize| work here if the PDF has mixed page sizes? No. Fixed this in new patch - returns 0 (invalid value) for mixed page sizes, will display blank scaling field in this case per advice from UX.
JS LGTM, with few (optional mostly) nits. https://codereview.chromium.org/2240433003/diff/600001/chrome/browser/resourc... File chrome/browser/resources/print_preview/print_preview.js (right): https://codereview.chromium.org/2240433003/diff/600001/chrome/browser/resourc... chrome/browser/resources/print_preview/print_preview.js:126: if (loadTimeData.getBoolean('scalingEnabled')) { Nit: loadTimeData.getBoolean('scalingEnabled') is repeated 5 times in this file. How about we either store it as a this.scalingEnabled_ or make a private helper this.isScalingEnabled_(). https://codereview.chromium.org/2240433003/diff/600001/chrome/browser/resourc... File chrome/browser/resources/print_preview/settings/scaling_settings.js (right): https://codereview.chromium.org/2240433003/diff/600001/chrome/browser/resourc... chrome/browser/resources/print_preview/settings/scaling_settings.js:114: * @return {boolean} true if fit to page is available and selected. Nit: Keep only @return comment. https://codereview.chromium.org/2240433003/diff/600001/chrome/browser/resourc... chrome/browser/resources/print_preview/settings/scaling_settings.js:127: if (/[^\d]+/.test(this.inputField_.value)) Optional: return /[^\d]+/.test(this.inputField_.value) ? 0 : // Return an invalid scaling so that the hint will display. parseInt(this.inputField_.value, 10); https://codereview.chromium.org/2240433003/diff/600001/chrome/browser/resourc... chrome/browser/resources/print_preview/settings/scaling_settings.js:138: if (this.fitToPageScaling_) Equivalent to: this.inputField_.value = this.fitToPageScaling_ || ''; https://codereview.chromium.org/2240433003/diff/600001/chrome/browser/resourc... chrome/browser/resources/print_preview/settings/scaling_settings.js:202: (displayedValue == this.fitToPageScaling_ || Nit: |displayedValue| only used here, so perhaps no benefit of storing it as local var, just inline this.getInputAsNumber(). https://codereview.chromium.org/2240433003/diff/600001/chrome/browser/resourc... chrome/browser/resources/print_preview/settings/scaling_settings.js:215: var scalingVal = this.inputField_.value; Non established shorthands like |Val| are discouraged by the styleguide (example of established shorthands are net,db,dir). Let's just s/Val/Value https://codereview.chromium.org/2240433003/diff/600001/chrome/browser/resourc... chrome/browser/resources/print_preview/settings/scaling_settings.js:273: setTimeout((function() { Is this setTimeout really needed? Have you been able to repro what the comment describes? Asking since this is coped from copies, and might not be relevant anymore.
Patchset #20 (id:620001) has been deleted
https://codereview.chromium.org/2240433003/diff/600001/chrome/browser/ui/webu... File chrome/browser/ui/webui/print_preview/print_preview_ui.cc (right): https://codereview.chromium.org/2240433003/diff/600001/chrome/browser/ui/webu... 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_f... File chrome/common/chrome_features.cc (right): https://codereview.chromium.org/2240433003/diff/600001/chrome/common/chrome_f... chrome/common/chrome_features.cc:101: #endif // ENABLE_PRINT_PREVIEW I wouldn't bother with the comment when it is (a) very close to the #if and (b) there are no nested #ifs. https://codereview.chromium.org/2240433003/diff/600001/components/printing/re... File components/printing/renderer/print_web_view_helper.cc (right): https://codereview.chromium.org/2240433003/diff/600001/components/printing/re... components/printing/renderer/print_web_view_helper.cc:454: double* scale_factor) { Do you think this function will be less messy if this is always valid? https://codereview.chromium.org/2240433003/diff/600001/components/printing/re... components/printing/renderer/print_web_view_helper.cc:747: static_cast<int>(static_cast<double>(print_layout_size.height()) * Another place to apply ScaleAndRound(). https://codereview.chromium.org/2240433003/diff/600001/components/printing/re... components/printing/renderer/print_web_view_helper.cc:1264: fit_to_page_scale_factor = 0.0f; Can this go back to 1.0f / can we default to 1 in all places, or do we need 0 to indicate this as a special case? https://codereview.chromium.org/2240433003/diff/600001/components/printing/re... components/printing/renderer/print_web_view_helper.cc:1583: CalculatePrintParamsForCss(frame, page_index, page_params, Both CalculatePrintParamsForCss() and CalculatePageLayoutFromPrintParams() can drop the |fit_to_page| parameter. They can calculate it from |page_params| and |params|, respectively. Add something like: bool IsWebPrintScalingOptionFitToPage(blink::WebPrintScalingOption option) { return option == blink::WebPrintScalingOptionFitToPrintableArea; } https://codereview.chromium.org/2240433003/diff/600001/testing/variations/fie... File testing/variations/fieldtrial_testing_config.json (right): https://codereview.chromium.org/2240433003/diff/600001/testing/variations/fie... testing/variations/fieldtrial_testing_config.json:1408: "PrintScaling": [ Do we actually need this? I don't think we are adding a field trial anymore?
The CQ bit was checked by rbpotter@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...
https://codereview.chromium.org/2240433003/diff/600001/chrome/browser/resourc... File chrome/browser/resources/print_preview/print_preview.js (right): https://codereview.chromium.org/2240433003/diff/600001/chrome/browser/resourc... chrome/browser/resources/print_preview/print_preview.js:126: if (loadTimeData.getBoolean('scalingEnabled')) { On 2016/10/22 00:30:25, dpapad wrote: > Nit: loadTimeData.getBoolean('scalingEnabled') is repeated 5 times in this file. > How about we either store it as a this.scalingEnabled_ or make a private helper > this.isScalingEnabled_(). Done. https://codereview.chromium.org/2240433003/diff/600001/chrome/browser/resourc... File chrome/browser/resources/print_preview/settings/scaling_settings.js (right): https://codereview.chromium.org/2240433003/diff/600001/chrome/browser/resourc... chrome/browser/resources/print_preview/settings/scaling_settings.js:114: * @return {boolean} true if fit to page is available and selected. On 2016/10/22 00:30:25, dpapad wrote: > Nit: Keep only @return comment. Done. https://codereview.chromium.org/2240433003/diff/600001/chrome/browser/resourc... chrome/browser/resources/print_preview/settings/scaling_settings.js:127: if (/[^\d]+/.test(this.inputField_.value)) On 2016/10/22 00:30:25, dpapad wrote: > Optional: > > return /[^\d]+/.test(this.inputField_.value) ? > 0 : // Return an invalid scaling so that the hint will display. > parseInt(this.inputField_.value, 10); Done. https://codereview.chromium.org/2240433003/diff/600001/chrome/browser/resourc... chrome/browser/resources/print_preview/settings/scaling_settings.js:138: if (this.fitToPageScaling_) On 2016/10/22 00:30:25, dpapad wrote: > Equivalent to: > > this.inputField_.value = this.fitToPageScaling_ || ''; Done. https://codereview.chromium.org/2240433003/diff/600001/chrome/browser/resourc... chrome/browser/resources/print_preview/settings/scaling_settings.js:202: (displayedValue == this.fitToPageScaling_ || On 2016/10/22 00:30:25, dpapad wrote: > Nit: |displayedValue| only used here, so perhaps no benefit of storing it as > local var, just inline this.getInputAsNumber(). Done. https://codereview.chromium.org/2240433003/diff/600001/chrome/browser/resourc... chrome/browser/resources/print_preview/settings/scaling_settings.js:215: var scalingVal = this.inputField_.value; On 2016/10/22 00:30:25, dpapad wrote: > Non established shorthands like |Val| are discouraged by the styleguide (example > of established shorthands are net,db,dir). Let's just s/Val/Value Done. https://codereview.chromium.org/2240433003/diff/600001/chrome/browser/resourc... chrome/browser/resources/print_preview/settings/scaling_settings.js:273: setTimeout((function() { On 2016/10/22 00:30:25, dpapad wrote: > Is this setTimeout really needed? Have you been able to repro what the comment > describes? Asking since this is coped from copies, and might not be relevant > anymore. It doesn't look like that issue occurs any more, so I have removed the timeout. However I did notice a few other problems while testing: (1) some weird animation going on with the hint appearing and then disappearing (2) an edge case (also present for the original copies) in which editing the field from the default value to empty and then shifting focus actually left the field blank, and (3) realized if fit to page is checked we don't want to update the ticket item to 100, just fill in the fit to page value again. Have made some modifications to fix all of this. I will create another CL to get rid of the timeout and fix the edge case for copies. https://codereview.chromium.org/2240433003/diff/600001/chrome/browser/ui/webu... File chrome/browser/ui/webui/print_preview/print_preview_ui.cc (right): https://codereview.chromium.org/2240433003/diff/600001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_ui.cc:406: features::kPrintScaling); On 2016/10/22 01:45:57, Lei Zhang wrote: > nit: fits on previous line. Done. https://codereview.chromium.org/2240433003/diff/600001/chrome/common/chrome_f... File chrome/common/chrome_features.cc (right): https://codereview.chromium.org/2240433003/diff/600001/chrome/common/chrome_f... chrome/common/chrome_features.cc:101: #endif // ENABLE_PRINT_PREVIEW On 2016/10/22 01:45:57, Lei Zhang wrote: > I wouldn't bother with the comment when it is (a) very close to the #if and (b) > there are no nested #ifs. Done. https://codereview.chromium.org/2240433003/diff/600001/components/printing/re... File components/printing/renderer/print_web_view_helper.cc (right): https://codereview.chromium.org/2240433003/diff/600001/components/printing/re... components/printing/renderer/print_web_view_helper.cc:454: double* scale_factor) { On 2016/10/22 01:45:57, Lei Zhang wrote: > Do you think this function will be less messy if this is always valid? Actually with the new modifications for fixing the android bot I think it is always valid (the call in PrepareFrameAndViewForPrint always sends a valid value and both calls to ComputePageLayoutInPointsForCss now have guaranteed valid values). Only place that is still suspect is in the mac code, which I am fixing. Is there anywhere else this could be called from? Removing scale_factor checks for now, will add them back if bots start failing again. https://codereview.chromium.org/2240433003/diff/600001/components/printing/re... components/printing/renderer/print_web_view_helper.cc:747: static_cast<int>(static_cast<double>(print_layout_size.height()) * On 2016/10/22 01:45:57, Lei Zhang wrote: > Another place to apply ScaleAndRound(). Done. https://codereview.chromium.org/2240433003/diff/600001/components/printing/re... components/printing/renderer/print_web_view_helper.cc:1264: fit_to_page_scale_factor = 0.0f; On 2016/10/22 01:45:57, Lei Zhang wrote: > Can this go back to 1.0f / can we default to 1 in all places, or do we need 0 to > indicate this as a special case? We need 0 (or some other non-valid scaling value) to indicate this as a special case, so that the UI knows to make the scaling field blank. 1.0f is a valid scaling (100%), so the UI would just think the fit to page scaling happened to be 100%. https://codereview.chromium.org/2240433003/diff/600001/components/printing/re... components/printing/renderer/print_web_view_helper.cc:1583: CalculatePrintParamsForCss(frame, page_index, page_params, On 2016/10/22 01:45:57, Lei Zhang wrote: > Both CalculatePrintParamsForCss() and CalculatePageLayoutFromPrintParams() can > drop the |fit_to_page| parameter. They can calculate it from |page_params| and > |params|, respectively. > > Add something like: > > bool IsWebPrintScalingOptionFitToPage(blink::WebPrintScalingOption option) { > return option == blink::WebPrintScalingOptionFitToPrintableArea; > } CalculatePrintParamsForCss() had the fit to page input before, and it is calculated slightly differently here vs in the other call to the function (line 728), so I'm leaving it alone for now. Changed CalculatePageLayoutFromPrintParams and added a helper function. https://codereview.chromium.org/2240433003/diff/600001/testing/variations/fie... File testing/variations/fieldtrial_testing_config.json (right): https://codereview.chromium.org/2240433003/diff/600001/testing/variations/fie... testing/variations/fieldtrial_testing_config.json:1408: "PrintScaling": [ On 2016/10/22 01:45:57, Lei Zhang wrote: > Do we actually need this? I don't think we are adding a field trial anymore? My understanding from the documentation is that adding this makes it enabled by default in Chromium (but not Chrome) builds, which means it is on for all the trybots. I wanted to have it on the trybots so that it gets tested and so that the new test added to check scaling will not fail. Is there another way to turn the feature on for trybots?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2240433003/diff/600001/components/printing/re... File components/printing/renderer/print_web_view_helper.cc (right): https://codereview.chromium.org/2240433003/diff/600001/components/printing/re... components/printing/renderer/print_web_view_helper.cc:454: double* scale_factor) { On 2016/10/22 04:57:20, rbpotter wrote: > On 2016/10/22 01:45:57, Lei Zhang wrote: > > Do you think this function will be less messy if this is always valid? > > Actually with the new modifications for fixing the android bot I think it is > always valid (the call in PrepareFrameAndViewForPrint always sends a valid value > and both calls to ComputePageLayoutInPointsForCss now have guaranteed valid > values). Only place that is still suspect is in the mac code, which I am fixing. > Is there anywhere else this could be called from? Removing scale_factor checks > for now, will add them back if bots start failing again. ComputePageLayoutInPointsForCss() looks like the only interesting caller, and I think you got all the places where it is called. https://codereview.chromium.org/2240433003/diff/600001/testing/variations/fie... File testing/variations/fieldtrial_testing_config.json (right): https://codereview.chromium.org/2240433003/diff/600001/testing/variations/fie... testing/variations/fieldtrial_testing_config.json:1408: "PrintScaling": [ On 2016/10/22 04:57:20, rbpotter wrote: > On 2016/10/22 01:45:57, Lei Zhang wrote: > > Do we actually need this? I don't think we are adding a field trial anymore? > > My understanding from the documentation is that adding this makes it enabled by > default in Chromium (but not Chrome) builds, which means it is on for all the > trybots. I wanted to have it on the trybots so that it gets tested and so that > the new test added to check scaling will not fail. Is there another way to turn > the feature on for trybots? Can you check with testing/variations/OWNERS ? You need them to review this anyway. https://codereview.chromium.org/2240433003/diff/660001/components/printing/re... File components/printing/renderer/print_web_view_helper.cc (right): https://codereview.chromium.org/2240433003/diff/660001/components/printing/re... components/printing/renderer/print_web_view_helper.cc:725: #if defined(ENABLE_PRINT_PREVIEW) How about: double scale_factor = 1.0f; #if defined(ENABLE_PRINT_PREVIEW) if (print_params.scale_factor > 0.0f) scale_factor = print_params.scale_factor; #endif
https://codereview.chromium.org/2240433003/diff/600001/components/printing/re... File components/printing/renderer/print_web_view_helper.cc (right): https://codereview.chromium.org/2240433003/diff/600001/components/printing/re... components/printing/renderer/print_web_view_helper.cc:1583: CalculatePrintParamsForCss(frame, page_index, page_params, On 2016/10/22 04:57:20, rbpotter wrote: > On 2016/10/22 01:45:57, Lei Zhang wrote: > > Both CalculatePrintParamsForCss() and CalculatePageLayoutFromPrintParams() can > > drop the |fit_to_page| parameter. They can calculate it from |page_params| and > > |params|, respectively. > > > > Add something like: > > > > bool IsWebPrintScalingOptionFitToPage(blink::WebPrintScalingOption option) { > > return option == blink::WebPrintScalingOptionFitToPrintableArea; > > } > > CalculatePrintParamsForCss() had the fit to page input before, and it is > calculated slightly differently here vs in the other call to the function (line > 728), so I'm leaving it alone for now. Changed > CalculatePageLayoutFromPrintParams and added a helper function. I looked at CalculatePrintParamsForCss() and I still think you can omit the |fit_to_page| parameter. Basically: bool fit_to_page = ignore_css_margins && IsWebPrintScalingOptionFitToPage(print_params); can be moved into CalculatePrintParamsForCss(), and it has all the inputs necessary to calculate the boolean. https://codereview.chromium.org/2240433003/diff/660001/components/printing/re... File components/printing/renderer/print_web_view_helper.cc (right): https://codereview.chromium.org/2240433003/diff/660001/components/printing/re... components/printing/renderer/print_web_view_helper.cc:230: { BTW, this look out of place. Can you just run "git cl format" and let it deal with the formatting? Also, no blank lines at the start of functions. https://codereview.chromium.org/2240433003/diff/660001/components/printing/re... components/printing/renderer/print_web_view_helper.cc:238: if (scale_factor > 0 && (fit_to_page || params.print_to_pdf)) { Here, you have scale_factor > 0, but in a bunch of places below, it's > 0.0f. Can't they just all be > 0? https://codereview.chromium.org/2240433003/diff/660001/components/printing/re... components/printing/renderer/print_web_view_helper.cc:1238: double scale_factor = print_params.scale_factor > 0 ? nit: There's an extra space in front of '>'. https://codereview.chromium.org/2240433003/diff/660001/components/printing/re... components/printing/renderer/print_web_view_helper.cc:1586: double input_scale_factor = scale_factor ? *scale_factor : 1.0f; At this point, I think |scale_factor| is never a nullptr.
rbpotter@chromium.org changed reviewers: + rkaplow@chromium.org
https://codereview.chromium.org/2240433003/diff/600001/components/printing/re... File components/printing/renderer/print_web_view_helper.cc (right): https://codereview.chromium.org/2240433003/diff/600001/components/printing/re... 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: > On 2016/10/22 04:57:20, rbpotter wrote: > > On 2016/10/22 01:45:57, Lei Zhang wrote: > > > Both CalculatePrintParamsForCss() and CalculatePageLayoutFromPrintParams() > can > > > drop the |fit_to_page| parameter. They can calculate it from |page_params| > and > > > |params|, respectively. > > > > > > Add something like: > > > > > > bool IsWebPrintScalingOptionFitToPage(blink::WebPrintScalingOption option) { > > > return option == blink::WebPrintScalingOptionFitToPrintableArea; > > > } > > > > CalculatePrintParamsForCss() had the fit to page input before, and it is > > calculated slightly differently here vs in the other call to the function > (line > > 728), so I'm leaving it alone for now. Changed > > CalculatePageLayoutFromPrintParams and added a helper function. > > I looked at CalculatePrintParamsForCss() and I still think you can omit the > |fit_to_page| parameter. Basically: > > bool fit_to_page = ignore_css_margins && > IsWebPrintScalingOptionFitToPage(print_params); > > can be moved into CalculatePrintParamsForCss(), and it has all the inputs > necessary to calculate the boolean. In this function, fit_to_page is computed independently of ignore_css_margins, and I am not sure that we can guarantee ignore_css_margins == true here, since it seems to just depend on whether or not the margins are set to default in print preview. So there would be a risk of changing the behavior if we change the computation to check ignore_css_margins_ in CalculatePrintParamsForCss. Not sure why the computation is different here vs in PrepareFrameAndViewForPrint though, will try to look into it a bit more. https://codereview.chromium.org/2240433003/diff/660001/components/printing/re... File components/printing/renderer/print_web_view_helper.cc (right): https://codereview.chromium.org/2240433003/diff/660001/components/printing/re... components/printing/renderer/print_web_view_helper.cc:230: { On 2016/10/25 00:58:00, Lei Zhang wrote: > BTW, this look out of place. Can you just run "git cl format" and let it deal > with the formatting? Also, no blank lines at the start of functions. Done. https://codereview.chromium.org/2240433003/diff/660001/components/printing/re... components/printing/renderer/print_web_view_helper.cc:238: if (scale_factor > 0 && (fit_to_page || params.print_to_pdf)) { On 2016/10/25 00:58:00, Lei Zhang wrote: > Here, you have scale_factor > 0, but in a bunch of places below, it's > 0.0f. > Can't they just all be > 0? Done. https://codereview.chromium.org/2240433003/diff/660001/components/printing/re... components/printing/renderer/print_web_view_helper.cc:725: #if defined(ENABLE_PRINT_PREVIEW) On 2016/10/24 23:56:38, Lei Zhang wrote: > How about: > > double scale_factor = 1.0f; > #if defined(ENABLE_PRINT_PREVIEW) > if (print_params.scale_factor > 0.0f) > scale_factor = print_params.scale_factor; > #endif Done. https://codereview.chromium.org/2240433003/diff/660001/components/printing/re... components/printing/renderer/print_web_view_helper.cc:1238: double scale_factor = print_params.scale_factor > 0 ? On 2016/10/25 00:58:00, Lei Zhang wrote: > nit: There's an extra space in front of '>'. Done. https://codereview.chromium.org/2240433003/diff/660001/components/printing/re... components/printing/renderer/print_web_view_helper.cc:1586: double input_scale_factor = scale_factor ? *scale_factor : 1.0f; On 2016/10/25 00:58:00, Lei Zhang wrote: > At this point, I think |scale_factor| is never a nullptr. Done. https://codereview.chromium.org/2240433003/diff/680001/testing/variations/fie... File testing/variations/fieldtrial_testing_config.json (right): https://codereview.chromium.org/2240433003/diff/680001/testing/variations/fie... testing/variations/fieldtrial_testing_config.json:1408: "PrintScaling": [ Question to rkaplow: Do we need this addition or not? Would like to have the feature on for buildbots so that it gets tested and so that the new test for this feature can be enabled. Is there another way of doing this other than modifying this file?
lgtm histogram lg
The CQ bit was checked by rbpotter@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) closure_compilation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by rbpotter@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...
rkaplow: Do we need the modifications to fieldtrial_testing_config.json or not? We are not clear on whether the changes to histograms and about_flags are sufficient on their own. In addition to the flag being available, I would like to have the feature on for the trybots so that it gets tested and so that the new test for the feature can be enabled. Is there a way to achieve this without modifying fieldtrial_testing_config? Thanks! On Tue, Oct 25, 2016 at 8:31 AM, <rkaplow@chromium.org> wrote: > lgtm > > histogram lg > > 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.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm Please check with rkaplow and get a clarification on the field trial config. https://codereview.chromium.org/2240433003/diff/700001/components/printing/re... File components/printing/renderer/print_web_view_helper.cc (right): https://codereview.chromium.org/2240433003/diff/700001/components/printing/re... components/printing/renderer/print_web_view_helper.cc:473: // and margins) if we are not fitting to page. This is worded a bit funny. https://codereview.chromium.org/2240433003/diff/700001/components/printing/re... components/printing/renderer/print_web_view_helper.cc:475: if (!fit_to_page) braces https://codereview.chromium.org/2240433003/diff/700001/components/printing/re... components/printing/renderer/print_web_view_helper.cc:514: if (fit_to_page) { This can only evaluate to true when |ignore_css_margins| is false. Move inside the block above? https://codereview.chromium.org/2240433003/diff/700001/components/printing/re... components/printing/renderer/print_web_view_helper.cc:1834: double css_scale_factor = 1.0f; Do the same pattern as line 723?
On 2016/10/25 21:42:53, Lei Zhang wrote: > lgtm > > Please check with rkaplow and get a clarification on the field trial config. > > https://codereview.chromium.org/2240433003/diff/700001/components/printing/re... > File components/printing/renderer/print_web_view_helper.cc (right): > > https://codereview.chromium.org/2240433003/diff/700001/components/printing/re... > components/printing/renderer/print_web_view_helper.cc:473: // and margins) if we > are not fitting to page. > This is worded a bit funny. > > https://codereview.chromium.org/2240433003/diff/700001/components/printing/re... > components/printing/renderer/print_web_view_helper.cc:475: if (!fit_to_page) > braces > > https://codereview.chromium.org/2240433003/diff/700001/components/printing/re... > components/printing/renderer/print_web_view_helper.cc:514: if (fit_to_page) { > This can only evaluate to true when |ignore_css_margins| is false. Move inside > the block above? > > https://codereview.chromium.org/2240433003/diff/700001/components/printing/re... > components/printing/renderer/print_web_view_helper.cc:1834: double > css_scale_factor = 1.0f; > Do the same pattern as line 723? Apologizes for terseness before. The logic looks fine overall. You've set it up so regular users will not have it enabled by default, but the field trial config will make it so the trybots have the feature enabled. This is the right way to do it if that's what you expect. This is also the natural approach if you're planning to run an experiment.
The CQ bit was checked by rbpotter@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...
https://codereview.chromium.org/2240433003/diff/700001/components/printing/re... File components/printing/renderer/print_web_view_helper.cc (right): https://codereview.chromium.org/2240433003/diff/700001/components/printing/re... components/printing/renderer/print_web_view_helper.cc:473: // and margins) if we are not fitting to page. On 2016/10/25 21:42:53, Lei Zhang wrote: > This is worded a bit funny. Done. https://codereview.chromium.org/2240433003/diff/700001/components/printing/re... components/printing/renderer/print_web_view_helper.cc:475: if (!fit_to_page) On 2016/10/25 21:42:53, Lei Zhang wrote: > braces Done. https://codereview.chromium.org/2240433003/diff/700001/components/printing/re... components/printing/renderer/print_web_view_helper.cc:514: if (fit_to_page) { On 2016/10/25 21:42:53, Lei Zhang wrote: > This can only evaluate to true when |ignore_css_margins| is false. Move inside > the block above? Done. https://codereview.chromium.org/2240433003/diff/700001/components/printing/re... components/printing/renderer/print_web_view_helper.cc:1834: double css_scale_factor = 1.0f; On 2016/10/25 21:42:53, Lei Zhang wrote: > Do the same pattern as line 723? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by rbpotter@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wfh@chromium.org, dpapad@chromium.org, rkaplow@chromium.org, thestig@chromium.org Link to the patchset: https://codereview.chromium.org/2240433003/#ps720001 (title: "Final small fixes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Add print scaling to printing/print_preview. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Add print scaling to printing/print_preview. BUG=96456 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Description was changed from ========== Add print scaling to printing/print_preview. BUG=96456 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Add print scaling to printing/print_preview. BUG=96456 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #24 (id:720001)
Message was sent while issue was closed.
Description was changed from ========== Add print scaling to printing/print_preview. BUG=96456 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Add print scaling to printing/print_preview. BUG=96456 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/769ffdf779d83399a6f219e2637c6469ab94da0e Cr-Commit-Position: refs/heads/master@{#427556} ==========
Message was sent while issue was closed.
Patchset 24 (id:??) landed as https://crrev.com/769ffdf779d83399a6f219e2637c6469ab94da0e Cr-Commit-Position: refs/heads/master@{#427556}
Message was sent while issue was closed.
Description was changed from ========== Add print scaling to printing/print_preview. BUG=96456 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/769ffdf779d83399a6f219e2637c6469ab94da0e Cr-Commit-Position: refs/heads/master@{#427556} ========== to ========== 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} ==========
Message was sent while issue was closed.
thakis@chromium.org changed reviewers: + thakis@chromium.org
Message was sent while issue was closed.
It looks like the tests this adds fail in official builds, see e.g. https://build.chromium.org/p/chromium.fyi/builders/ClangToTWin%20tester/build... Probably related to the finch config somehow. Can you please take a look, and if it takes a while to figure out, revert while you figure it out?
Message was sent while issue was closed.
SourceIsPDFCapabilities is still broken. Final request for you to take action on this...
Message was sent while issue was closed.
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.
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. |