Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7408004:
Printer dropdown cleanup (Closed)
|
Created:
9 years, 5 months ago by Albert Bodenhamer Modified:
9 years, 5 months ago CC:
chromium-reviews, arv (Not doing code reviews) Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionPrinter dropdown cleanup
Prevents default printer value from being overridden when the printer list
comes back from GCP.
Will now populate the printer drops down with ALL printers (up to 10)
rather than just recents
Got the MORE_PRINTERS dropdown option working.
BUG=http://code.google.com/p/chromium-os/issues/detail?id=16082
TEST=
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=93216
Patch Set 1 #Patch Set 2 : Fix issue with Signin appearing twice. #Patch Set 3 : Fixed issue with signing appearing twice #
Total comments: 16
Patch Set 4 : Review related changes #
Total comments: 18
Patch Set 5 : More review feedback #
Total comments: 4
Patch Set 6 : More review feedback #
Messages
Total messages: 15 (0 generated)
Logic looks good, need someone with JS readability now. On 2011/07/19 00:45:24, Albert Bodenhamer wrote:
Adding Demetrios for JS readability.
http://codereview.chromium.org/7408004/diff/4001/chrome/browser/resources/pri... File chrome/browser/resources/print_preview/print_preview.js (right): http://codereview.chromium.org/7408004/diff/4001/chrome/browser/resources/pri... chrome/browser/resources/print_preview/print_preview.js:21: var previewRequested = false; Instead of adding another state variable, you could use the existing |lastPreviewRequestID| to determine if a preview has been requested (it will be greater than -1). I would prefer having a function function hasRequestedPreview() { return lastPreviewRequestID > -1; } to keep the number of state variables to a minimum. http://codereview.chromium.org/7408004/diff/4001/chrome/browser/resources/pri... chrome/browser/resources/print_preview/print_preview.js:68: var addedCloudPrinters = {}; Could these be moved within print_preview_cloud.js? Also it seems that there is a lot of cloud print related logic in this file. In general there has been an effort to encapsulate related options to their own separate class, please take a look at bug 88098 and the CLs that refer to that bug. I would consider creating a class like CloudPrintSettings and put everything that is cloud print related in there. I am not saying this has to be done in this CL necessarily, but if you think you can follow the same pattern, please put this in your task queue. http://codereview.chromium.org/7408004/diff/4001/chrome/browser/resources/pri... chrome/browser/resources/print_preview/print_preview.js:514: cloudprint.fetchPrinters(addCloudPrinters, true); Why fetching printers twice? Is this for getting a short list quickly and then fetch the full list? http://codereview.chromium.org/7408004/diff/4001/chrome/browser/resources/pri... chrome/browser/resources/print_preview/print_preview.js:519: var printerList = $('printer-list'); Since $('printer-list') is only used once here, no need to cache it to a local var. http://codereview.chromium.org/7408004/diff/4001/chrome/browser/resources/pri... chrome/browser/resources/print_preview/print_preview.js:598: * @param {String} id - A unique value to track this printer. Nit: No need for "-", just use a space between parameter name and comment, here and elsewhere. http://codereview.chromium.org/7408004/diff/4001/chrome/browser/resources/pri... chrome/browser/resources/print_preview/print_preview.js:600: * been passed to trackCloudPrinterAdded. Nit: Use 4 space indent as usual, here and elsewhere. http://codereview.chromium.org/7408004/diff/4001/chrome/browser/resources/pri... chrome/browser/resources/print_preview/print_preview.js:653: for (printer in printers) { Do not use for in statement with arrays, since it does not behave as expected, see JS styleguide http://www.corp.google.com/eng/doc/javascriptguide.xml?showone=for-in_loop#fo.... http://codereview.chromium.org/7408004/diff/4001/chrome/browser/resources/pri... File chrome/browser/resources/print_preview/print_preview_cloud.js (left): http://codereview.chromium.org/7408004/diff/4001/chrome/browser/resources/pri... chrome/browser/resources/print_preview/print_preview_cloud.js:133: * @param {function} callback Function to be called to process response. Document parameter |all|.
Also I think the Bug ID in the description is probably not the right one?
On 2011/07/19 18:46:58, dpapad wrote: > Also I think the Bug ID in the description is probably not the right one? Grr. Good catch. It's supposed to refer to Chromium OS issue 16082, not Chromium issue 16082.
Updated. Please take another look. http://codereview.chromium.org/7408004/diff/4001/chrome/browser/resources/pri... File chrome/browser/resources/print_preview/print_preview.js (right): http://codereview.chromium.org/7408004/diff/4001/chrome/browser/resources/pri... chrome/browser/resources/print_preview/print_preview.js:21: var previewRequested = false; On 2011/07/19 18:44:55, dpapad wrote: > Instead of adding another state variable, you could use the existing > |lastPreviewRequestID| to determine if a preview has been requested (it will be > greater than -1). I would prefer having a function > function hasRequestedPreview() { return lastPreviewRequestID > -1; } > to keep the number of state variables to a minimum. Done. http://codereview.chromium.org/7408004/diff/4001/chrome/browser/resources/pri... chrome/browser/resources/print_preview/print_preview.js:68: var addedCloudPrinters = {}; The boundary has definitely gotten muddy as things have come together. My original intent was that code that interacted directly with the print preview UI would live in print_preview.js while logic related to GCP would live in print_preview_cloud.js. Thanks for being flexible on the refactoring. I've added a todo to keep track of it. On 2011/07/19 18:44:55, dpapad wrote: > Could these be moved within print_preview_cloud.js? Also it seems that there is > a lot of cloud print related logic in this file. > In general there has been an effort to encapsulate related options to their > own separate class, please take a look at bug 88098 and the CLs that refer to > that bug. I would consider creating a class like CloudPrintSettings and put > everything that is cloud print related in there. > I am not saying this has to be done in this CL necessarily, but if you think > you can follow the same pattern, please put this in your task queue. http://codereview.chromium.org/7408004/diff/4001/chrome/browser/resources/pri... chrome/browser/resources/print_preview/print_preview.js:514: cloudprint.fetchPrinters(addCloudPrinters, true); Yep. We fetch the recents and the full list separately and then merge them to fill out the list. I've added comments to make it more clear. On 2011/07/19 18:44:55, dpapad wrote: > Why fetching printers twice? Is this for getting a short list quickly and then > fetch the full list? http://codereview.chromium.org/7408004/diff/4001/chrome/browser/resources/pri... chrome/browser/resources/print_preview/print_preview.js:519: var printerList = $('printer-list'); On 2011/07/19 18:44:55, dpapad wrote: > Since $('printer-list') is only used once here, no need to cache it to a local > var. Done. http://codereview.chromium.org/7408004/diff/4001/chrome/browser/resources/pri... chrome/browser/resources/print_preview/print_preview.js:598: * @param {String} id - A unique value to track this printer. On 2011/07/19 18:44:55, dpapad wrote: > Nit: No need for "-", just use a space between parameter name and comment, here > and elsewhere. Done. http://codereview.chromium.org/7408004/diff/4001/chrome/browser/resources/pri... chrome/browser/resources/print_preview/print_preview.js:600: * been passed to trackCloudPrinterAdded. On 2011/07/19 18:44:55, dpapad wrote: > Nit: Use 4 space indent as usual, here and elsewhere. Done. http://codereview.chromium.org/7408004/diff/4001/chrome/browser/resources/pri... chrome/browser/resources/print_preview/print_preview.js:653: for (printer in printers) { On 2011/07/19 18:44:55, dpapad wrote: > Do not use for in statement with arrays, since it does not behave as expected, > see JS styleguide > http://www.corp.google.com/eng/doc/javascriptguide.xml?showone=for-in_loop#fo.... Done.
http://codereview.chromium.org/7408004/diff/4001/chrome/browser/resources/pri... File chrome/browser/resources/print_preview/print_preview.js (right): http://codereview.chromium.org/7408004/diff/4001/chrome/browser/resources/pri... chrome/browser/resources/print_preview/print_preview.js:68: var addedCloudPrinters = {}; On 2011/07/19 21:53:19, Albert Bodenhamer wrote: > The boundary has definitely gotten muddy as things have come together. My > original intent was that code that interacted directly with the print preview UI > would live in print_preview.js while logic related to GCP would live in > print_preview_cloud.js. > > Thanks for being flexible on the refactoring. I've added a todo to keep track > of it. > > On 2011/07/19 18:44:55, dpapad wrote: > > Could these be moved within print_preview_cloud.js? Also it seems that there > is > > a lot of cloud print related logic in this file. > > In general there has been an effort to encapsulate related options to their > > own separate class, please take a look at bug 88098 and the CLs that refer to > > that bug. I would consider creating a class like CloudPrintSettings and put > > everything that is cloud print related in there. > > I am not saying this has to be done in this CL necessarily, but if you think > > you can follow the same pattern, please put this in your task queue. > The UI logic has migrated (to some degree) outside print_preview.js, which should act more like a controller (in the MVC pattern), the UI should receive generic events and update itself. Thanks for taking a note on this, please refer your CL to the same issue (88098), when you get to it. http://codereview.chromium.org/7408004/diff/10001/chrome/browser/resources/pr... chrome/browser/resources/print_preview/print_preview.js:63: var firstCloudPrintOptionPos = 0; Nit: space after //. http://codereview.chromium.org/7408004/diff/10001/chrome/browser/resources/pr... chrome/browser/resources/print_preview/print_preview.js:365: if (selectedPrinter < 0) Nit: 1 blank line between functions. http://codereview.chromium.org/7408004/diff/10001/chrome/browser/resources/pr... chrome/browser/resources/print_preview/print_preview.js:579: optionText, Nit: @param {number}, there is no type Integer in JS. http://codereview.chromium.org/7408004/diff/10001/chrome/browser/resources/pr... chrome/browser/resources/print_preview/print_preview.js:581: isDefault, Nit optional: Just for consistency (within this file and across WebUI pages) we should go with either "string" or "String". It seems that most of the existing comments use "string", but both cases occur. http://codereview.chromium.org/7408004/diff/10001/chrome/browser/resources/pr... chrome/browser/resources/print_preview/print_preview.js:618: } Change to "Returns false if adding this printer". http://codereview.chromium.org/7408004/diff/10001/chrome/browser/resources/pr... chrome/browser/resources/print_preview/print_preview.js:619: } Nit: s/maxCloudPrinters/|maxCloudPrinters| http://codereview.chromium.org/7408004/diff/10001/chrome/browser/resources/pr... chrome/browser/resources/print_preview/print_preview.js:662: cloudprint.setCloudPrint(option, Nit optional: Using a counter like "i" seems a bit more intuitive. http://codereview.chromium.org/7408004/diff/10001/chrome/browser/resources/pr... File chrome/browser/resources/print_preview/print_preview_cloud.js (right): http://codereview.chromium.org/7408004/diff/10001/chrome/browser/resources/pr... chrome/browser/resources/print_preview/print_preview_cloud.js:313: /** Returns the data necessary to serialize a cloud print printer. Please use the following format, here and elsewhere. /** * Comments go here. */ http://codereview.chromium.org/7408004/diff/10001/chrome/browser/resources/pr... chrome/browser/resources/print_preview/print_preview_cloud.js:316: * cloud print portion of the printer object, or Nit: More can fit in this line.
http://codereview.chromium.org/7408004/diff/10001/chrome/browser/resources/pr... File chrome/browser/resources/print_preview/print_preview.js (right): http://codereview.chromium.org/7408004/diff/10001/chrome/browser/resources/pr... chrome/browser/resources/print_preview/print_preview.js:63: //TODO(abodenha@chromium.org) A lot of cloud print specific logic has On 2011/07/19 22:22:58, dpapad wrote: > Nit: space after //. Done. http://codereview.chromium.org/7408004/diff/10001/chrome/browser/resources/pr... chrome/browser/resources/print_preview/print_preview.js:365: On 2011/07/19 22:22:58, dpapad wrote: > Nit: 1 blank line between functions. Done. http://codereview.chromium.org/7408004/diff/10001/chrome/browser/resources/pr... chrome/browser/resources/print_preview/print_preview.js:579: * @param {Integer} position The index in the printer-list wher the option On 2011/07/19 22:22:58, dpapad wrote: > Nit: @param {number}, there is no type Integer in JS. Done. http://codereview.chromium.org/7408004/diff/10001/chrome/browser/resources/pr... chrome/browser/resources/print_preview/print_preview.js:581: * @param {String} optionText specifies the option text content. On 2011/07/19 22:22:58, dpapad wrote: > Nit optional: Just for consistency (within this file and across WebUI pages) we > should go with either "string" or "String". It seems that most of the existing > comments use "string", but both cases occur. Done. http://codereview.chromium.org/7408004/diff/10001/chrome/browser/resources/pr... chrome/browser/resources/print_preview/print_preview.js:618: * @return {boolean} Returns false if there adding this printer On 2011/07/19 22:22:58, dpapad wrote: > Change to "Returns false if adding this printer". Done. http://codereview.chromium.org/7408004/diff/10001/chrome/browser/resources/pr... chrome/browser/resources/print_preview/print_preview.js:619: * would exceed maxCloudPrinters. On 2011/07/19 22:22:58, dpapad wrote: > Nit: s/maxCloudPrinters/|maxCloudPrinters| Done. http://codereview.chromium.org/7408004/diff/10001/chrome/browser/resources/pr... chrome/browser/resources/print_preview/print_preview.js:662: for (var printer = 0; printer < printers.length; printer++) { On 2011/07/19 22:22:58, dpapad wrote: > Nit optional: Using a counter like "i" seems a bit more intuitive. Done. http://codereview.chromium.org/7408004/diff/10001/chrome/browser/resources/pr... File chrome/browser/resources/print_preview/print_preview_cloud.js (right): http://codereview.chromium.org/7408004/diff/10001/chrome/browser/resources/pr... chrome/browser/resources/print_preview/print_preview_cloud.js:313: /** Returns the data necessary to serialize a cloud print printer. On 2011/07/19 22:22:58, dpapad wrote: > Please use the following format, here and elsewhere. > /** > * Comments go here. > */ Done. http://codereview.chromium.org/7408004/diff/10001/chrome/browser/resources/pr... chrome/browser/resources/print_preview/print_preview_cloud.js:316: * cloud print portion of the printer object, or On 2011/07/19 22:22:58, dpapad wrote: > Nit: More can fit in this line. Done.
LGTM with nits and 2 questions. 1) When I sign in order to use cloud printer, the list does not get updated, and I have to reload the preview tab to see the cloud printers. 2) When I switch from a local printer to a cloud printer, the preview data never come back, I keep seeing the "Preview loading..." animation. I used --enable-print-preview --enable-cloud-print to test. http://codereview.chromium.org/7408004/diff/16001/chrome/browser/resources/pr... File chrome/browser/resources/print_preview/print_preview.js (right): http://codereview.chromium.org/7408004/diff/16001/chrome/browser/resources/pr... chrome/browser/resources/print_preview/print_preview.js:607: * @return {boolean} Returns true if this id has previously Nit: s/Returns true/True. http://codereview.chromium.org/7408004/diff/16001/chrome/browser/resources/pr... chrome/browser/resources/print_preview/print_preview.js:617: * @return {boolean} Returns false if adding this printer would exceed Nit: s/Returns false/False
On Tue, Jul 19, 2011 at 4:01 PM, <dpapad@chromium.org> wrote: > LGTM with nits and 2 questions. > > 1) When I sign in order to use cloud printer, the list does not get > updated, and > I have to reload the preview tab to see the cloud printers. > > Expected. The current sign in functionality is just a place holder. The plan is to open a dialog for sign in and then refresh the dropdown automatically when complete. > 2) When I switch from a local printer to a cloud printer, the preview data > never > come back, I keep seeing the "Preview loading..." animation. > It's a regression that happened sometime in the last few days and is independent of this CL. Somehow I'm losing track of the request ID that gets sent with the getPreview request. When the request data comes back it gets discarded by updatePrintPreview. I'm working on it now. > > I used --enable-print-preview --enable-cloud-print to test. > > > http://codereview.chromium.**org/7408004/diff/16001/chrome/** > browser/resources/print_**preview/print_preview.js<http://codereview.chromium.org/7408004/diff/16001/chrome/browser/resources/print_preview/print_preview.js> > > File chrome/browser/resources/**print_preview/print_preview.js (right): > > http://codereview.chromium.**org/7408004/diff/16001/chrome/** > browser/resources/print_**preview/print_preview.js#**newcode607<http://codereview.chromium.org/7408004/diff/16001/chrome/browser/resources/print_preview/print_preview.js#newcode607> > chrome/browser/resources/**print_preview/print_preview.**js:607: * @return > {boolean} Returns true if this id has previously > Nit: s/Returns true/True. > > http://codereview.chromium.**org/7408004/diff/16001/chrome/** > browser/resources/print_**preview/print_preview.js#**newcode617<http://codereview.chromium.org/7408004/diff/16001/chrome/browser/resources/print_preview/print_preview.js#newcode617> > chrome/browser/resources/**print_preview/print_preview.**js:617: * @return > {boolean} Returns false if adding this printer would exceed > Nit: s/Returns false/False > > > http://codereview.chromium.**org/7408004/<http://codereview.chromium.org/7408... > -- Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com> org
And now to wait on the trybots. http://codereview.chromium.org/7408004/diff/16001/chrome/browser/resources/pr... File chrome/browser/resources/print_preview/print_preview.js (right): http://codereview.chromium.org/7408004/diff/16001/chrome/browser/resources/pr... chrome/browser/resources/print_preview/print_preview.js:607: * @return {boolean} Returns true if this id has previously On 2011/07/19 23:01:35, dpapad wrote: > Nit: s/Returns true/True. Done. http://codereview.chromium.org/7408004/diff/16001/chrome/browser/resources/pr... chrome/browser/resources/print_preview/print_preview.js:617: * @return {boolean} Returns false if adding this printer would exceed On 2011/07/19 23:01:35, dpapad wrote: > Nit: s/Returns false/False Done.
Ok. The preview ID with each request was added in http://codereview.chromium.org/7313035 by me a week ago, to fix some corner cases that had to do with timing issues.
I see it. If the user causes a new preview request to be generated while the previous one is still in progress the request ID stuff discards the data to prevent many levels of badness. Makes sense. It looks like your CL went in right after mine and added handling for the request ID to PrintWebViewHelper::UpdatePrintSettingsLocal but not PrintWebViewHelper::UpdatePrintSettingsCloud. I'll have a CL ready to fix it soon. Possibly in the AM since my build seems to be broken since my latest sync. :-/ Thanks. On Tue, Jul 19, 2011 at 4:21 PM, <dpapad@chromium.org> wrote: > Ok. The preview ID with each request was added in > http://codereview.chromium.**org/7313035<http://codereview.chromium.org/73130... me a week ago, to fix some corner > cases that had to do with timing issues. > > > > http://codereview.chromium.**org/7408004/<http://codereview.chromium.org/7408... > -- Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com> org
Change committed as 93216 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
