Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2346153002:
Save most recent 3 destinations across multiple sessions (Closed)
DescriptionSave most recent 3 destinations for print across multiple
prints and multiple browser sessions. Note: can increase from
3 easily if needed.
Add browser test to check behavior.
BUG=601858
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/6d799882e49984de8620d41c53893101eaed92f6
Cr-Commit-Position: refs/heads/master@{#420238}
Patch Set 1 #
Total comments: 14
Patch Set 2 : Factor out to helper functions, small fixes #
Total comments: 15
Patch Set 3 : Fix errors, remove commented code #
Total comments: 18
Patch Set 4 : Fix readability and extra checks #
Total comments: 2
Patch Set 5 : Change to array of objects #
Total comments: 10
Patch Set 6 : Fix nits and define object elements #Patch Set 7 : Add check for bad struct read in #
Messages
Total messages: 51 (36 generated)
Description was changed from ========== Save most recent 3 destinations across multiple prints and multiple browser sessions. Note: can increase from 3 easily if needed. Add browser test to check behavior. BUG=601858 ========== to ========== Save most recent 3 destinations across multiple prints and multiple browser sessions. Note: can increase from 3 easily if needed. Add browser test to check behavior. BUG=601858 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== Save most recent 3 destinations across multiple prints and multiple browser sessions. Note: can increase from 3 easily if needed. Add browser test to check behavior. BUG=601858 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Save most recent 3 destinations for print across multiple prints and multiple browser sessions. Note: can increase from 3 easily if needed. Add browser test to check behavior. BUG=601858 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...
rbpotter@chromium.org changed reviewers: + dpapad@chromium.org, thestig@chromium.org
Saving print destinations across browser sessions.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2346153002/diff/1/chrome/browser/resources/pr... File chrome/browser/resources/print_preview/data/app_state.js (right): https://codereview.chromium.org/2346153002/diff/1/chrome/browser/resources/pr... chrome/browser/resources/print_preview/data/app_state.js:21: this.numDestinations_ = 3; Let's add type annotation to this. @private {number} Also, if this is meant to be a constant, you could move it a few lines below and change it as follows: /** * Explanatory comment. * @const {number} */ AppState.NUM_DESTINATIONS = 3; https://codereview.chromium.org/2346153002/diff/1/chrome/browser/resources/pr... chrome/browser/resources/print_preview/data/app_state.js:77: /** @return {?string} ID of the selected destination. */ The type annotation here seems no longer accurate. It seems that this function now always returns a string, never null (same below). https://codereview.chromium.org/2346153002/diff/1/chrome/browser/resources/pr... chrome/browser/resources/print_preview/data/app_state.js:88: if (this.state_[AppState.Field.RECENT_DESTINATION_ACCOUNTS] && This logic is repeated 7 times. Can you package it in a helper function? Something like getStateValue_: function(name) { return this.state_[name] && this.state_[name].length > 0 ? this.state_[name][0] : ''; }, get selectedDestinationAccount() { return this.getStateValue_(AppState.Field.RECENT_DESTINATION_ACCOUNTS); } ... https://codereview.chromium.org/2346153002/diff/1/chrome/browser/resources/pr... chrome/browser/resources/print_preview/data/app_state.js:152: /** @return {?array of strings} IDs of the recent destinations. */ @return {?Array<string>} IDs of the recent destinations. (here and elsewhere) https://codereview.chromium.org/2346153002/diff/1/chrome/browser/resources/pr... chrome/browser/resources/print_preview/data/app_state.js:252: if (!this.state_[AppState.Field.RECENT_DESTINATION_IDS]) Is there a way to package redundant logic into a helper? https://codereview.chromium.org/2346153002/diff/1/chrome/browser/resources/pr... chrome/browser/resources/print_preview/data/app_state.js:255: AppState.Field.RECENT_DESTINATION_IDS].constructor != Array) { instanceof is more suited for this type of checks. var a = []; if (a instanceof Array) {...} https://codereview.chromium.org/2346153002/diff/1/chrome/browser/resources/pr... chrome/browser/resources/print_preview/data/app_state.js:347: for (var i = this.numDestinations_ - 1; i > 0; i--) { Can you explain a bit what this logic does? Perhaps there is an easier way to do it using the Array#shift or splice() methods?
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/2346153002/diff/1/chrome/browser/resources/pr... File chrome/browser/resources/print_preview/data/app_state.js (right): https://codereview.chromium.org/2346153002/diff/1/chrome/browser/resources/pr... chrome/browser/resources/print_preview/data/app_state.js:21: this.numDestinations_ = 3; On 2016/09/17 01:53:02, dpapad wrote: > Let's add type annotation to this. > > @private {number} > > Also, if this is meant to be a constant, you could move it a few lines below and > change it as follows: > > /** > * Explanatory comment. > * @const {number} > */ > AppState.NUM_DESTINATIONS = 3; Done. https://codereview.chromium.org/2346153002/diff/1/chrome/browser/resources/pr... chrome/browser/resources/print_preview/data/app_state.js:77: /** @return {?string} ID of the selected destination. */ On 2016/09/17 01:53:02, dpapad wrote: > The type annotation here seems no longer accurate. It seems that this function > now always returns a string, never null (same below). Done. https://codereview.chromium.org/2346153002/diff/1/chrome/browser/resources/pr... chrome/browser/resources/print_preview/data/app_state.js:88: if (this.state_[AppState.Field.RECENT_DESTINATION_ACCOUNTS] && On 2016/09/17 01:53:02, dpapad wrote: > This logic is repeated 7 times. Can you package it in a helper function? > Something like > > getStateValue_: function(name) { > return this.state_[name] && this.state_[name].length > 0 ? > this.state_[name][0] : ''; > }, > > get selectedDestinationAccount() { > return this.getStateValue_(AppState.Field.RECENT_DESTINATION_ACCOUNTS); > } > > ... Done. https://codereview.chromium.org/2346153002/diff/1/chrome/browser/resources/pr... chrome/browser/resources/print_preview/data/app_state.js:152: /** @return {?array of strings} IDs of the recent destinations. */ On 2016/09/17 01:53:02, dpapad wrote: > @return {?Array<string>} IDs of the recent destinations. > (here and elsewhere) Done. https://codereview.chromium.org/2346153002/diff/1/chrome/browser/resources/pr... chrome/browser/resources/print_preview/data/app_state.js:252: if (!this.state_[AppState.Field.RECENT_DESTINATION_IDS]) On 2016/09/17 01:53:02, dpapad wrote: > Is there a way to package redundant logic into a helper? Done. https://codereview.chromium.org/2346153002/diff/1/chrome/browser/resources/pr... chrome/browser/resources/print_preview/data/app_state.js:255: AppState.Field.RECENT_DESTINATION_IDS].constructor != Array) { On 2016/09/17 01:53:02, dpapad wrote: > instanceof is more suited for this type of checks. > > var a = []; > if (a instanceof Array) {...} Done. https://codereview.chromium.org/2346153002/diff/1/chrome/browser/resources/pr... chrome/browser/resources/print_preview/data/app_state.js:347: for (var i = this.numDestinations_ - 1; i > 0; i--) { On 2016/09/17 01:53:02, dpapad wrote: > Can you explain a bit what this logic does? Perhaps there is an easier way to do > it using the Array#shift or splice() methods? Acknowledged - see new code in the next patchset. Not sure if it is simpler/easier or not, let me know what you think. https://codereview.chromium.org/2346153002/diff/20001/chrome/browser/resource... File chrome/browser/resources/print_preview/data/app_state.js (right): https://codereview.chromium.org/2346153002/diff/20001/chrome/browser/resource... chrome/browser/resources/print_preview/data/app_state.js:315: persistSelectedDestination: function(dest) { Does this function (with the helper above) look better? If so I will remove the commented code below.
Some more comments. Looks cleaner than before, but still I don't have a clear picture of the desired change. Perhaps we can briefly talk about this CL in person, it would make reviewing easier. https://codereview.chromium.org/2346153002/diff/20001/chrome/browser/resource... File chrome/browser/resources/print_preview/data/app_state.js (right): https://codereview.chromium.org/2346153002/diff/20001/chrome/browser/resource... chrome/browser/resources/print_preview/data/app_state.js:88: * @param {?print_preview.AppState.field} The state field to return the Type annotation is missing the parameter name, and has a small typo. Should be: * @param {!print_preview.AppState.Field} fieldName The state field to return the https://codereview.chromium.org/2346153002/diff/20001/chrome/browser/resource... chrome/browser/resources/print_preview/data/app_state.js:90: * @return {?string or ?print_preview.Cdd} The most recent value of the This is not a valid type annotation. I am not sure if print_preview code is actually compiled by the JS compiler, or if these annotations server mostly as documentation, but a valid annotation for returning a union of types looks as follows {?string|?print_preview.Cdd} If this code, was actually compiled, the compiler would throw a "Bad type annotation" error. https://codereview.chromium.org/2346153002/diff/20001/chrome/browser/resource... chrome/browser/resources/print_preview/data/app_state.js:219: * @param {print_preview.AppState.field} stateFieldName The state field to !print_preview.AppState.Field https://codereview.chromium.org/2346153002/diff/20001/chrome/browser/resource... chrome/browser/resources/print_preview/data/app_state.js:223: if (!this.state_[stateFieldName]) { Is this check robust? Could this.state_[stateFieldName] have a valid value of 0 or '' (empty string) for example? If so this check would succeed, but accidentally it seems. Or is this.state_[stateFieldName] expected to only have Array instances as values? https://codereview.chromium.org/2346153002/diff/20001/chrome/browser/resource... chrome/browser/resources/print_preview/data/app_state.js:227: this.state_[stateFieldName][0] = tmp; This assignment seems a odd but also wrong. Are you simply trying to convert the single value to an array with a single element that has that value? If so could you do the following instead: var tmp = this.state_[stateFieldName]; this.state_[stateFieldName] = [tmp]; https://codereview.chromium.org/2346153002/diff/20001/chrome/browser/resource... chrome/browser/resources/print_preview/data/app_state.js:264: Nit: Remove blank line. https://codereview.chromium.org/2346153002/diff/20001/chrome/browser/resource... chrome/browser/resources/print_preview/data/app_state.js:306: this.state_[fieldName].reverse(); No need to reverse, push, reverse. You can insert a single element in the front of an array with splice(), as follows myArray.splice(0, 0, 'foo');
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 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...
Fixed errors. The general goal is to save the 3 (or possibly some other number, but 3 was proposed in the bug) most recent destinations across different sessions. Currently the app state only saves 1 destination's information (selected destination) and uses that as the initial destination when the dialog is brought up again. I am modifying the app state to save arrays containing the information from 3 destinations instead, and am maintaining the arrays so that the nth element of each corresponds to the nth most recently used destination. These destinations are also set as recent in the destination store at startup so that when the user looks for destinations they appear in the "Recent" destinations top portion of the dialog. We can talk in person whenever you are free. https://codereview.chromium.org/2346153002/diff/20001/chrome/browser/resource... File chrome/browser/resources/print_preview/data/app_state.js (right): https://codereview.chromium.org/2346153002/diff/20001/chrome/browser/resource... chrome/browser/resources/print_preview/data/app_state.js:88: * @param {?print_preview.AppState.field} The state field to return the On 2016/09/19 22:16:19, dpapad wrote: > Type annotation is missing the parameter name, and has a small typo. Should be: > > * @param {!print_preview.AppState.Field} fieldName The state field to return the Done. https://codereview.chromium.org/2346153002/diff/20001/chrome/browser/resource... chrome/browser/resources/print_preview/data/app_state.js:90: * @return {?string or ?print_preview.Cdd} The most recent value of the On 2016/09/19 22:16:19, dpapad wrote: > This is not a valid type annotation. I am not sure if print_preview code is > actually compiled by the JS compiler, or if these annotations server mostly as > documentation, but a valid annotation for returning a union of types looks as > follows > > {?string|?print_preview.Cdd} > > If this code, was actually compiled, the compiler would throw a "Bad type > annotation" error. Done - I did not realize there was actually a compiler, thought that these comments were just for documentation... sorry. https://codereview.chromium.org/2346153002/diff/20001/chrome/browser/resource... chrome/browser/resources/print_preview/data/app_state.js:219: * @param {print_preview.AppState.field} stateFieldName The state field to On 2016/09/19 22:16:19, dpapad wrote: > !print_preview.AppState.Field Done. https://codereview.chromium.org/2346153002/diff/20001/chrome/browser/resource... chrome/browser/resources/print_preview/data/app_state.js:223: if (!this.state_[stateFieldName]) { On 2016/09/19 22:16:19, dpapad wrote: > Is this check robust? Could this.state_[stateFieldName] have a valid value of 0 > or '' (empty string) for example? If so this check would succeed, but > accidentally it seems. Or is this.state_[stateFieldName] expected to only have > Array instances as values? Done - fields other than IDs could be a single element with a value of '' or null. IDs should always be non empty strings or an array of nonempty strings. https://codereview.chromium.org/2346153002/diff/20001/chrome/browser/resource... chrome/browser/resources/print_preview/data/app_state.js:227: this.state_[stateFieldName][0] = tmp; On 2016/09/19 22:16:19, dpapad wrote: > This assignment seems a odd but also wrong. Are you simply trying to convert the > single value to an array with a single element that has that value? If so could > you do the following instead: > > var tmp = this.state_[stateFieldName]; > this.state_[stateFieldName] = [tmp]; Done - yes, I was trying to convert the value to an array with an element that has that value. https://codereview.chromium.org/2346153002/diff/20001/chrome/browser/resource... chrome/browser/resources/print_preview/data/app_state.js:264: On 2016/09/19 22:16:19, dpapad wrote: > Nit: Remove blank line. Done. https://codereview.chromium.org/2346153002/diff/20001/chrome/browser/resource... chrome/browser/resources/print_preview/data/app_state.js:306: this.state_[fieldName].reverse(); On 2016/09/19 22:16:19, dpapad wrote: > No need to reverse, push, reverse. You can insert a single element in the front > of an array with splice(), as follows > > myArray.splice(0, 0, 'foo'); Done.
Thanks, for the explanation. I understand now what the previous and new state of the code is doing. Few more questions, mostly nits. https://codereview.chromium.org/2346153002/diff/40001/chrome/browser/resource... File chrome/browser/resources/print_preview/data/app_state.js (right): https://codereview.chromium.org/2346153002/diff/40001/chrome/browser/resource... chrome/browser/resources/print_preview/data/app_state.js:145: /** @return {?Array<string>} IDs of the recent destinations. */ Nit: Indentation off by 1. https://codereview.chromium.org/2346153002/diff/40001/chrome/browser/resource... chrome/browser/resources/print_preview/data/app_state.js:224: this.state_[AppState.Field.RECENT_DESTINATION_IDS].length == 0) { Should these comparisons use stateFieldName instead of RECENT_DESTINATION_IDS? It is not clear if this is intentional. https://codereview.chromium.org/2346153002/diff/40001/chrome/browser/resource... chrome/browser/resources/print_preview/data/app_state.js:233: this.state_[stateFieldName].splice(AppState.NUM_DESTINATIONS_, Could you simply truncate the array by setting the length, as follows? this.state_[stateFieldName].length = AppState.NUM_DESTINATIONS_; This is a valid way to truncate an array (see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje...). https://codereview.chromium.org/2346153002/diff/40001/chrome/browser/resource... chrome/browser/resources/print_preview/data/app_state.js:300: * @param {string or print_preview.Cdd} the value that should be added as {string|printPreview.Cdd} destVal The value that ... https://codereview.chromium.org/2346153002/diff/40001/chrome/browser/resource... chrome/browser/resources/print_preview/data/app_state.js:317: if (!this.isInitialized_ || !dest) Can dest be null/undefined? If so line 314 should depict that by specifying {?print_preview.Destination} https://codereview.chromium.org/2346153002/diff/40001/chrome/browser/resource... File chrome/browser/resources/print_preview/data/destination_store.js (right): https://codereview.chromium.org/2346153002/diff/40001/chrome/browser/resource... chrome/browser/resources/print_preview/data/destination_store.js:469: this.appState_.recentDestinationOrigins[i]) { Are those two checks necessary? https://codereview.chromium.org/2346153002/diff/40001/chrome/browser/resource... chrome/browser/resources/print_preview/data/destination_store.js:486: } else { Can this be simplified as follows? } else { foundDestination = this.fetchPreselectedDestionation_(...); } https://codereview.chromium.org/2346153002/diff/40001/chrome/browser/resource... chrome/browser/resources/print_preview/data/destination_store.js:1163: destinationIdNum < this.appState_.recentDestinationIds.length; Probably more readable to simply name the iteration counter just "i". https://codereview.chromium.org/2346153002/diff/40001/chrome/browser/resource... chrome/browser/resources/print_preview/data/destination_store.js:1165: if (this.appState_.recentDestinationIds && Is this check necessary? Does this.appState_.recentDestinationIds hold null/undefined values?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2346153002/diff/40001/chrome/browser/resource... File chrome/browser/resources/print_preview/data/app_state.js (right): https://codereview.chromium.org/2346153002/diff/40001/chrome/browser/resource... chrome/browser/resources/print_preview/data/app_state.js:145: /** @return {?Array<string>} IDs of the recent destinations. */ On 2016/09/21 00:34:30, dpapad wrote: > Nit: Indentation off by 1. Done. https://codereview.chromium.org/2346153002/diff/40001/chrome/browser/resource... chrome/browser/resources/print_preview/data/app_state.js:224: this.state_[AppState.Field.RECENT_DESTINATION_IDS].length == 0) { On 2016/09/21 00:34:30, dpapad wrote: > Should these comparisons use stateFieldName instead of RECENT_DESTINATION_IDS? > It is not clear if this is intentional. This is intentional. The other states can have empty strings or, in the case of capabilities, nullptr as values. If there is only one destination and it happens to have one of these values this could lead to the check succeeding incorrectly. Ids is a required value, so the check will only succeed if there are really no destinations stored. https://codereview.chromium.org/2346153002/diff/40001/chrome/browser/resource... chrome/browser/resources/print_preview/data/app_state.js:233: this.state_[stateFieldName].splice(AppState.NUM_DESTINATIONS_, On 2016/09/21 00:34:30, dpapad wrote: > Could you simply truncate the array by setting the length, as follows? > > this.state_[stateFieldName].length = AppState.NUM_DESTINATIONS_; > > This is a valid way to truncate an array (see > https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje...). Done. https://codereview.chromium.org/2346153002/diff/40001/chrome/browser/resource... chrome/browser/resources/print_preview/data/app_state.js:300: * @param {string or print_preview.Cdd} the value that should be added as On 2016/09/21 00:34:30, dpapad wrote: > {string|printPreview.Cdd} destVal The value that ... Done. https://codereview.chromium.org/2346153002/diff/40001/chrome/browser/resource... chrome/browser/resources/print_preview/data/app_state.js:317: if (!this.isInitialized_ || !dest) On 2016/09/21 00:34:30, dpapad wrote: > Can dest be null/undefined? If so line 314 should depict that by specifying > {?print_preview.Destination} Done - No, it shouldn't be. This was accidentally left over from testing. https://codereview.chromium.org/2346153002/diff/40001/chrome/browser/resource... File chrome/browser/resources/print_preview/data/destination_store.js (right): https://codereview.chromium.org/2346153002/diff/40001/chrome/browser/resource... chrome/browser/resources/print_preview/data/destination_store.js:469: this.appState_.recentDestinationOrigins[i]) { On 2016/09/21 00:34:30, dpapad wrote: > Are those two checks necessary? Done. https://codereview.chromium.org/2346153002/diff/40001/chrome/browser/resource... chrome/browser/resources/print_preview/data/destination_store.js:486: } else { On 2016/09/21 00:34:30, dpapad wrote: > Can this be simplified as follows? > > } else { > foundDestination = this.fetchPreselectedDestionation_(...); > } Done. https://codereview.chromium.org/2346153002/diff/40001/chrome/browser/resource... chrome/browser/resources/print_preview/data/destination_store.js:1163: destinationIdNum < this.appState_.recentDestinationIds.length; On 2016/09/21 00:34:30, dpapad wrote: > Probably more readable to simply name the iteration counter just "i". Done. https://codereview.chromium.org/2346153002/diff/40001/chrome/browser/resource... chrome/browser/resources/print_preview/data/destination_store.js:1165: if (this.appState_.recentDestinationIds && On 2016/09/21 00:34:30, dpapad wrote: > Is this check necessary? Does this.appState_.recentDestinationIds hold > null/undefined values? 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...
LGTM, with one potential future improvement suggestion. Does one id, correspond to one account, one origin, one capability, one name etc? If yes, it would be cleaner to create an object to hold all related state, and only use a single array of such objects to store the most recent ones. My understanding is that currently the index within the array is used to group related things that are spread across different arrays. This creates the burden of having to update all those separate arrays in a consistent way (shifting all of them, instead of shifting just one). https://codereview.chromium.org/2346153002/diff/60001/chrome/browser/resource... File chrome/browser/resources/print_preview/data/destination_store.js (right): https://codereview.chromium.org/2346153002/diff/60001/chrome/browser/resource... chrome/browser/resources/print_preview/data/destination_store.js:1157: for (var i = 0; i < this.appState_.recentDestinationIds.length; i++) { It seems that the array is being searched for a specific element. Once that element is found, iterating through the remaining of the array is unnecessary. I think this can be simplified using Array#some() method. destination.isRecent = this.appState.recentDestinationIds.some( function(id, i) { return destionation.id == id && destination.origin == this.appState_.recentDestinationOrigins[i]; }, this);
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 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...
I switched this to an array of objects instead of lots of arrays. I think this made things cleaner and it eliminated the need for some of the helper functions. https://codereview.chromium.org/2346153002/diff/60001/chrome/browser/resource... File chrome/browser/resources/print_preview/data/destination_store.js (right): https://codereview.chromium.org/2346153002/diff/60001/chrome/browser/resource... chrome/browser/resources/print_preview/data/destination_store.js:1157: for (var i = 0; i < this.appState_.recentDestinationIds.length; i++) { On 2016/09/21 02:22:03, dpapad wrote: > It seems that the array is being searched for a specific element. Once that > element is found, iterating through the remaining of the array is unnecessary. I > think this can be simplified using Array#some() method. > > destination.isRecent = this.appState.recentDestinationIds.some( > function(id, i) { > return destionation.id == id && > destination.origin == this.appState_.recentDestinationOrigins[i]; > }, this); Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks for following the suggestion. Indeed it is easier to follow now. A few nits, but otherwise still LGTM. https://codereview.chromium.org/2346153002/diff/80001/chrome/browser/resource... File chrome/browser/resources/print_preview/data/app_state.js (right): https://codereview.chromium.org/2346153002/diff/80001/chrome/browser/resource... chrome/browser/resources/print_preview/data/app_state.js:18: this.extension_id_ = destination.extension_id || ''; Nit: Per JS styleguide naming, this should be this.extensionId_ (similar for extensionName below). https://codereview.chromium.org/2346153002/diff/80001/chrome/browser/resource... chrome/browser/resources/print_preview/data/app_state.js:163: * @return {?Array<RecentDestination>} The AppState.NUM_DESTINATIONS_ most Nit: ?Array<!RecentDestination> https://codereview.chromium.org/2346153002/diff/80001/chrome/browser/resource... chrome/browser/resources/print_preview/data/app_state.js:269: }, newDestination); No need to use newDestination as "this" here. Just refer to newDestination directly, it already exists in the scope. https://codereview.chromium.org/2346153002/diff/80001/chrome/browser/resource... chrome/browser/resources/print_preview/data/app_state.js:289: newDestination); Nit: Probably more readable to break line right after "(".
https://codereview.chromium.org/2346153002/diff/80001/chrome/browser/resource... File chrome/browser/resources/print_preview/data/destination_store.js (right): https://codereview.chromium.org/2346153002/diff/80001/chrome/browser/resource... chrome/browser/resources/print_preview/data/destination_store.js:1159: return (destination.id == recent.id_ && Since you are accessing id_, origin_ from here, make them public. The '_' suffix implies that they are private (and usually such member vars are accompanied by an @private annotation).
Patchset #6 (id:100001) 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...
https://codereview.chromium.org/2346153002/diff/80001/chrome/browser/resource... File chrome/browser/resources/print_preview/data/app_state.js (right): https://codereview.chromium.org/2346153002/diff/80001/chrome/browser/resource... chrome/browser/resources/print_preview/data/app_state.js:18: this.extension_id_ = destination.extension_id || ''; On 2016/09/21 20:44:18, dpapad wrote: > Nit: Per JS styleguide naming, this should be this.extensionId_ (similar for > extensionName below). Done. https://codereview.chromium.org/2346153002/diff/80001/chrome/browser/resource... chrome/browser/resources/print_preview/data/app_state.js:163: * @return {?Array<RecentDestination>} The AppState.NUM_DESTINATIONS_ most On 2016/09/21 20:44:18, dpapad wrote: > Nit: ?Array<!RecentDestination> Done. https://codereview.chromium.org/2346153002/diff/80001/chrome/browser/resource... chrome/browser/resources/print_preview/data/app_state.js:269: }, newDestination); On 2016/09/21 20:44:18, dpapad wrote: > No need to use newDestination as "this" here. Just refer to newDestination > directly, it already exists in the scope. Done. https://codereview.chromium.org/2346153002/diff/80001/chrome/browser/resource... chrome/browser/resources/print_preview/data/app_state.js:289: newDestination); On 2016/09/21 20:44:18, dpapad wrote: > Nit: Probably more readable to break line right after "(". Done. https://codereview.chromium.org/2346153002/diff/80001/chrome/browser/resource... File chrome/browser/resources/print_preview/data/destination_store.js (right): https://codereview.chromium.org/2346153002/diff/80001/chrome/browser/resource... chrome/browser/resources/print_preview/data/destination_store.js:1159: return (destination.id == recent.id_ && On 2016/09/21 20:46:32, dpapad wrote: > Since you are accessing id_, origin_ from here, make them public. The '_' suffix > implies that they are private (and usually such member vars are accompanied by > an @private annotation). Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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: 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 dpapad@chromium.org Link to the patchset: https://codereview.chromium.org/2346153002/#ps140001 (title: "Add check for bad struct read in")
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 ========== Save most recent 3 destinations for print across multiple prints and multiple browser sessions. Note: can increase from 3 easily if needed. Add browser test to check behavior. BUG=601858 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Save most recent 3 destinations for print across multiple prints and multiple browser sessions. Note: can increase from 3 easily if needed. Add browser test to check behavior. BUG=601858 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #7 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Save most recent 3 destinations for print across multiple prints and multiple browser sessions. Note: can increase from 3 easily if needed. Add browser test to check behavior. BUG=601858 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Save most recent 3 destinations for print across multiple prints and multiple browser sessions. Note: can increase from 3 easily if needed. Add browser test to check behavior. BUG=601858 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/6d799882e49984de8620d41c53893101eaed92f6 Cr-Commit-Position: refs/heads/master@{#420238} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/6d799882e49984de8620d41c53893101eaed92f6 Cr-Commit-Position: refs/heads/master@{#420238} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
