|
|
DescriptionPrint Preview: Add the list entire list of cloud print media sizes.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/263321a3c439d9872438f1904d1570573b7e7d9b
Cr-Commit-Position: refs/heads/master@{#427206}
Patch Set 1 #
Total comments: 5
Patch Set 2 : commas #Patch Set 3 : decl once #
Total comments: 4
Patch Set 4 : address comments, put back 'Tabloid' #Messages
Total messages: 29 (16 generated)
Description was changed from ========== Print Preview: Add the list entire list of cloud print media sizes. ========== to ========== Print Preview: Add the list entire list of cloud print media sizes. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by thestig@chromium.org to run a CQ dry run
thestig@chromium.org changed reviewers: + dpapad@chromium.org
https://codereview.chromium.org/2444643002/diff/1/chrome/browser/resources/pr... File chrome/browser/resources/print_preview/data/destination_store.js (left): https://codereview.chromium.org/2444643002/diff/1/chrome/browser/resources/pr... chrome/browser/resources/print_preview/data/destination_store.js:357: 'NA_LEDGER': 'Tabloid', I asked on https://codereview.chromium.org/294923005/ why this is. https://codereview.chromium.org/2444643002/diff/1/chrome/browser/resources/pr... File chrome/browser/resources/print_preview/data/destination_store.js (right): https://codereview.chromium.org/2444643002/diff/1/chrome/browser/resources/pr... chrome/browser/resources/print_preview/data/destination_store.js:510: 'ROC_8K': 'ROC 8k' Can I add a trailing comma?
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Print Preview: Add the list entire list of cloud print media sizes. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Print Preview: Add the list entire list of cloud print media sizes. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/2444643002/diff/1/chrome/browser/resources/pr... File chrome/browser/resources/print_preview/data/destination_store.js (right): https://codereview.chromium.org/2444643002/diff/1/chrome/browser/resources/pr... chrome/browser/resources/print_preview/data/destination_store.js:345: var mediaDisplayNames = { Now that this object got significantly larger, can we move it outside of localizeCapabilities_ such that it is only created once? https://codereview.chromium.org/2444643002/diff/1/chrome/browser/resources/pr... chrome/browser/resources/print_preview/data/destination_store.js:510: 'ROC_8K': 'ROC 8k' On 2016/10/22 at 02:11:06, Lei Zhang wrote: > Can I add a trailing comma? Yes, unless you care about IE6 and such ;>
The CQ bit was checked by thestig@chromium.org to run a CQ dry run
https://codereview.chromium.org/2444643002/diff/1/chrome/browser/resources/pr... File chrome/browser/resources/print_preview/data/destination_store.js (right): https://codereview.chromium.org/2444643002/diff/1/chrome/browser/resources/pr... chrome/browser/resources/print_preview/data/destination_store.js:345: var mediaDisplayNames = { On 2016/10/24 18:14:56, dpapad wrote: > Now that this object got significantly larger, can we move it outside of > localizeCapabilities_ such that it is only created once? Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM with nits. https://codereview.chromium.org/2444643002/diff/40001/chrome/browser/resource... File chrome/browser/resources/print_preview/data/destination_store.js (right): https://codereview.chromium.org/2444643002/diff/40001/chrome/browser/resource... chrome/browser/resources/print_preview/data/destination_store.js:311: * @const {number} I think the correct annotation (if we were to actually compile this file with JS Compiler, which I don't think it happens), would be @private {number} @const https://codereview.chromium.org/2444643002/diff/40001/chrome/browser/resource... chrome/browser/resources/print_preview/data/destination_store.js:332: * Maximum amount of time spent searching for extension destinations, in Wrong comment?
https://codereview.chromium.org/2444643002/diff/40001/chrome/browser/resource... File chrome/browser/resources/print_preview/data/destination_store.js (right): https://codereview.chromium.org/2444643002/diff/40001/chrome/browser/resource... chrome/browser/resources/print_preview/data/destination_store.js:311: * @const {number} On 2016/10/24 20:53:53, dpapad wrote: > I think the correct annotation (if we were to actually compile this file with JS > Compiler, which I don't think it happens), would be > > @private {number} > @const Done. https://codereview.chromium.org/2444643002/diff/40001/chrome/browser/resource... chrome/browser/resources/print_preview/data/destination_store.js:332: * Maximum amount of time spent searching for extension destinations, in On 2016/10/24 20:53:53, dpapad wrote: > Wrong comment? Copy + paste error.
The CQ bit was checked by thestig@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpapad@chromium.org Link to the patchset: https://codereview.chromium.org/2444643002/#ps60001 (title: "address comments, put back 'Tabloid'")
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
Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by thestig@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Print Preview: Add the list entire list of cloud print media sizes. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Print Preview: Add the list entire list of cloud print media sizes. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Print Preview: Add the list entire list of cloud print media sizes. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Print Preview: Add the list entire list of cloud print media sizes. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/263321a3c439d9872438f1904d1570573b7e7d9b Cr-Commit-Position: refs/heads/master@{#427206} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/263321a3c439d9872438f1904d1570573b7e7d9b Cr-Commit-Position: refs/heads/master@{#427206}
Message was sent while issue was closed.
alekseys@chromium.org changed reviewers: + alekseys@chromium.org
Message was sent while issue was closed.
As a suggestion, with media set this big, I'd start planning on structuring media selection dropdown. For example, GCP groups media by standard/region (ISO, NA, JIS etc.)
Message was sent while issue was closed.
On 2016/10/25 16:33:58, Aleksey Shlyapnikov wrote: > As a suggestion, with media set this big, I'd start planning on structuring > media selection dropdown. For example, GCP groups media by standard/region (ISO, > NA, JIS etc.) Yes, I noticed the GCP website does this. I implemented the sorting earlier in https://crrev.com/426311 .
Message was sent while issue was closed.
On 2016/10/25 17:13:46, Lei Zhang wrote: > On 2016/10/25 16:33:58, Aleksey Shlyapnikov wrote: > > As a suggestion, with media set this big, I'd start planning on structuring > > media selection dropdown. For example, GCP groups media by standard/region > (ISO, > > NA, JIS etc.) > > Yes, I noticed the GCP website does this. I implemented the sorting earlier in > https://crrev.com/426311 . Oh, sorry, my bad, should've checked the code out first. Thank you! |