Simplified print preview printer selection to be more consistent with Chrome

Refactored code to move cloudprint specific code into print_preview_cloud.js.
Removed "Local" and "Cloud" labels from dropdown
The "More printers" label is now more clear and shares behavior with the cloud print printer option in Chrome.
The system print dialog link is replaced with a similar link referencing cloud print in chrome os.
Choosing cloud print from the printers dropdown now requires the user to hit print before opening the cloud print dialog.
Fixed broken tests.

BUG=88098, 97175
, http://code.google.com/p/chromium-os/issues/detail?id=16082, http://code.google.com/p/chromium-os/issues/detail?id=20121, http://code.google.com/p/chromium-os/issues/detail?id=20119
TEST=Enable print preview in about:flags.  Printing should work with cloud print printers in much the same way as local printers in Chrome

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=103010

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=103173

[kmadhusu, 2011-09-21 21:57:40]

http://codereview.chromium.org/7976017/diff/3001/chrome/app/generated_resourc...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/7976017/diff/3001/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:6094: desc="Option shown in printer drop-down
list to allow the user to print using cloud print.">
Update the desc.

http://codereview.chromium.org/7976017/diff/3001/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:6123: Print using <ph
name="CLOUD_PRINT_NAME">Google Cloud Print</ph> dialog...
I think we have associated a shortcut key for the cloud print dialog. You may
want to add it to your string?

Eg: Print using Google Cloud Print dialog... (Shift+Ctrl+P)

http://codereview.chromium.org/7976017/diff/3001/chrome/browser/resources/pri...
File chrome/browser/resources/print_preview/print_preview.js (right):

http://codereview.chromium.org/7976017/diff/3001/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/print_preview.js:232: selectedValue ==
PRINT_WITH_CLOUD_PRINT) {
Adding this option here will hide copies, color and layout settings.  Some users
may want these options.

http://codereview.chromium.org/7976017/diff/3001/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/print_preview.js:317:
getSelectedPrinterName() == PRINT_WITH_CLOUD_PRINT);
var selectedPrinter = getSelectedPrinterName();
and reuse "selectedPrinter" here.

http://codereview.chromium.org/7976017/diff/3001/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/print_preview.js:429:
showCustomMessage(localStrings.getString('printWithCloudPrintWait'));
Along with the progress message, you also want to show a throbber near the link
so that the user can know that we are loading the dialog.

Eg: On linux, try to open a long document and press Ctrl+P. While the preview is
loading, click the "Print using system dialog" link. You will see a throbber
near the link till the system dialog is completely loaded.

http://codereview.chromium.org/7976017/diff/3001/chrome/browser/ui/webui/prin...
File chrome/browser/ui/webui/print_preview_data_source.cc (right):

http://codereview.chromium.org/7976017/diff/3001/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_data_source.cc:97:
AddLocalizedString("systemDialogOption",
"systemDialogOption" is a just a escape hatch to use the native print system
dialog. Once the print preview is stable and has all the advanced options of
native dialog, we may remove this link forever. 

Is it the same for cloud print dialog option? It's better if you can create a
new link for this cloud print dialog link.  You may also want to document this
in the html file.

http://codereview.chromium.org/7976017/diff/3001/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_data_source.cc:106:
IDS_PRINT_PREVIEW_PRINT_WITH_CLOUD_PRINT);
IDS_PRINT_PREVIEW_PRINT_WITH_CLOUD_PRINT is a formatted string, Do you need to
pass the string values for the place holders?

[dpapad, 2011-09-22 18:11:57]

http://codereview.chromium.org/7976017/diff/3001/chrome/browser/resources/pri...
File chrome/browser/resources/print_preview/print_preview.js (right):

http://codereview.chromium.org/7976017/diff/3001/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/print_preview.js:328: var
printWithCloudPrint = (deviceName == PRINT_WITH_CLOUD_PRINT);
Nit: No need for parenthesis here, same for line 327. I t does enhance
readability but it is not consistent with lines 423, 424.

http://codereview.chromium.org/7976017/diff/3001/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/print_preview.js:429:
showCustomMessage(localStrings.getString('printWithCloudPrintWait'));
On 2011/09/21 21:57:41, kmadhusu wrote:
> Along with the progress message, you also want to show a throbber near the
link
> so that the user can know that we are loading the dialog.
> 
> Eg: On linux, try to open a long document and press Ctrl+P. While the preview
is
> loading, click the "Print using system dialog" link. You will see a throbber
> near the link till the system dialog is completely loaded. 

We only show the throbber if the link was clicked and the metafile is not ready.
In this case the "Print" button was clicked, right? So I don't think a throbber
should be displayed.

http://codereview.chromium.org/7976017/diff/3001/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/print_preview.js:799: selectedPrinter ==
PRINT_WITH_CLOUD_PRINT;
Nit: Do regular 4 space indent, since the styleguide does not specify any
exceptions for aligning conditionals. Same for line 906.

http://codereview.chromium.org/7976017/diff/3001/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/print_preview.js:908:
!isPrintReadyMetafileReady) ||
This can fit in the previous line now.

http://codereview.chromium.org/7976017/diff/3001/chrome/browser/resources/pri...
File chrome/browser/resources/print_preview/print_preview_cloud.js (right):

http://codereview.chromium.org/7976017/diff/3001/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/print_preview_cloud.js:122:
addCloudPrinters(printerList, callback);
Fix indentation.

http://codereview.chromium.org/7976017/diff/3001/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/print_preview_cloud.js:127:
addCloudPrinters(null, callback);
Could you restructure the if blocks as follows?
if (xhr.status == 200) {
  var searchResult = JSON.parse(xhr.responseText);
  if (searchResult['success']) {
    var printerList = searchResult['printers'];
    addCloudPrinters(printerList, callback);
    return;
  }
}
addCloudPrinters(null, callback);

http://codereview.chromium.org/7976017/diff/3001/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/print_preview_cloud.js:358: * @return
{boolean} True if this id has previously been passed to
Nit: True if |id| has ...

http://codereview.chromium.org/7976017/diff/3001/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/print_preview_cloud.js:362: return
(addedCloudPrinters[id]);
Nit: No need for parenthesis.

[Albert Bodenhamer, 2011-09-22 23:23:38]

http://codereview.chromium.org/7976017/diff/3001/chrome/app/generated_resourc...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/7976017/diff/3001/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:6094: desc="Option shown in printer drop-down
list to allow the user to print using cloud print.">
On 2011/09/21 21:57:41, kmadhusu wrote:
> Update the desc.

Done.

http://codereview.chromium.org/7976017/diff/3001/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:6123: Print using <ph
name="CLOUD_PRINT_NAME">Google Cloud Print</ph> dialog...
On 2011/09/21 21:57:41, kmadhusu wrote:
> I think we have associated a shortcut key for the cloud print dialog. You may
> want to add it to your string?
> 
> Eg: Print using Google Cloud Print dialog... (Shift+Ctrl+P)

Done.

http://codereview.chromium.org/7976017/diff/3001/chrome/browser/resources/pri...
File chrome/browser/resources/print_preview/print_preview.js (right):

http://codereview.chromium.org/7976017/diff/3001/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/print_preview.js:232: selectedValue ==
PRINT_WITH_CLOUD_PRINT) {
Layout works fine.  Did you mean duplex?
Copies, color, and duplex must be specified via the cloud print dialog if they
are supported on the specific printer so it makes no sense to ask for them here.

On 2011/09/21 21:57:41, kmadhusu wrote:
> Adding this option here will hide copies, color and layout settings.  Some
users
> may want these options.

http://codereview.chromium.org/7976017/diff/3001/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/print_preview.js:317:
getSelectedPrinterName() == PRINT_WITH_CLOUD_PRINT);
On 2011/09/21 21:57:41, kmadhusu wrote:
> var selectedPrinter = getSelectedPrinterName();
> and reuse "selectedPrinter" here.

Done.

http://codereview.chromium.org/7976017/diff/3001/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/print_preview.js:328: var
printWithCloudPrint = (deviceName == PRINT_WITH_CLOUD_PRINT);
On 2011/09/22 18:11:57, dpapad wrote:
> Nit: No need for parenthesis here, same for line 327. I t does enhance
> readability but it is not consistent with lines 423, 424.

Done.

http://codereview.chromium.org/7976017/diff/3001/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/print_preview.js:429:
showCustomMessage(localStrings.getString('printWithCloudPrintWait'));
I'm going with dpapad@ on this one.  The option should behave very much like a
normal printer.  There are other status messages that indicate the wait
properly.

On 2011/09/22 18:11:57, dpapad wrote:
> On 2011/09/21 21:57:41, kmadhusu wrote:
> > Along with the progress message, you also want to show a throbber near the
> link
> > so that the user can know that we are loading the dialog.
> > 
> > Eg: On linux, try to open a long document and press Ctrl+P. While the
preview
> is
> > loading, click the "Print using system dialog" link. You will see a throbber
> > near the link till the system dialog is completely loaded. 
> 
> We only show the throbber if the link was clicked and the metafile is not
ready.
> In this case the "Print" button was clicked, right? So I don't think a
throbber
> should be displayed.

http://codereview.chromium.org/7976017/diff/3001/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/print_preview.js:799: selectedPrinter ==
PRINT_WITH_CLOUD_PRINT;
On 2011/09/22 18:11:57, dpapad wrote:
> Nit: Do regular 4 space indent, since the styleguide does not specify any
> exceptions for aligning conditionals. Same for line 906.

Done.

http://codereview.chromium.org/7976017/diff/3001/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/print_preview.js:908:
!isPrintReadyMetafileReady) ||
On 2011/09/22 18:11:57, dpapad wrote:
> This can fit in the previous line now.

Done.

http://codereview.chromium.org/7976017/diff/3001/chrome/browser/resources/pri...
File chrome/browser/resources/print_preview/print_preview_cloud.js (right):

http://codereview.chromium.org/7976017/diff/3001/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/print_preview_cloud.js:122:
addCloudPrinters(printerList, callback);
On 2011/09/22 18:11:57, dpapad wrote:
> Fix indentation.

Done.

http://codereview.chromium.org/7976017/diff/3001/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/print_preview_cloud.js:127:
addCloudPrinters(null, callback);
On 2011/09/22 18:11:57, dpapad wrote:
> Could you restructure the if blocks as follows?
> if (xhr.status == 200) {
>   var searchResult = JSON.parse(xhr.responseText);
>   if (searchResult['success']) {
>     var printerList = searchResult['printers'];
>     addCloudPrinters(printerList, callback);
>     return;
>   }
> }
> addCloudPrinters(null, callback);

Done.

http://codereview.chromium.org/7976017/diff/3001/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/print_preview_cloud.js:358: * @return
{boolean} True if this id has previously been passed to
On 2011/09/22 18:11:57, dpapad wrote:
> Nit: True if |id| has ...

Done.

http://codereview.chromium.org/7976017/diff/3001/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/print_preview_cloud.js:362: return
(addedCloudPrinters[id]);
On 2011/09/22 18:11:57, dpapad wrote:
> Nit: No need for parenthesis.

Done.

http://codereview.chromium.org/7976017/diff/3001/chrome/browser/ui/webui/prin...
File chrome/browser/ui/webui/print_preview_data_source.cc (right):

http://codereview.chromium.org/7976017/diff/3001/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_data_source.cc:97:
AddLocalizedString("systemDialogOption",
On Chrome OS, cloud print IS the system print dialog.  This is just an attempt
to make it more clear by changing the string but keeping the behavior consistent
with other platforms.

Eventually we want a single dialog that seemlessly does everything both dialogs
do today and we'll want to remove the link at that point.

I've pushed the string selection into the html.  Hopefully that will make things
more clear.

On 2011/09/21 21:57:41, kmadhusu wrote:
> "systemDialogOption" is a just a escape hatch to use the native print system
> dialog. Once the print preview is stable and has all the advanced options of
> native dialog, we may remove this link forever. 
> 
> Is it the same for cloud print dialog option? It's better if you can create a
> new link for this cloud print dialog link.  You may also want to document this
> in the html file.

http://codereview.chromium.org/7976017/diff/3001/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_data_source.cc:106:
IDS_PRINT_PREVIEW_PRINT_WITH_CLOUD_PRINT);
It has a non-translatable section for the service name, but it isn't
parameterized.  Am I missing something?

On 2011/09/21 21:57:41, kmadhusu wrote:
> IDS_PRINT_PREVIEW_PRINT_WITH_CLOUD_PRINT is a formatted string, Do you need to
> pass the string values for the place holders?

[kmadhusu, 2011-09-23 01:27:19]

http://codereview.chromium.org/7976017/diff/3001/chrome/browser/ui/webui/prin...
File chrome/browser/ui/webui/print_preview_data_source.cc (right):

http://codereview.chromium.org/7976017/diff/3001/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_data_source.cc:97:
AddLocalizedString("systemDialogOption",
On 2011/09/22 23:23:39, Albert Bodenhamer wrote:
> On Chrome OS, cloud print IS the system print dialog.  This is just an attempt
> to make it more clear by changing the string but keeping the behavior
consistent
> with other platforms.
> 
> Eventually we want a single dialog that seemlessly does everything both
dialogs
> do today and we'll want to remove the link at that point.
> 
> I've pushed the string selection into the html.  Hopefully that will make
things
> more clear.
> 
> On 2011/09/21 21:57:41, kmadhusu wrote:
> > "systemDialogOption" is a just a escape hatch to use the native print system
> > dialog. Once the print preview is stable and has all the advanced options of
> > native dialog, we may remove this link forever. 
> > 
> > Is it the same for cloud print dialog option? It's better if you can create
a
> > new link for this cloud print dialog link.  You may also want to document
this
> > in the html file. 
> 

Thanks for the clarification. On Chrome OS, if you have a link to print using
cloud print dialog, how are we going to handle the page range errors? (Having a
link does not solve this issue crbug.com/97175)

http://codereview.chromium.org/7976017/diff/3001/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_data_source.cc:106:
IDS_PRINT_PREVIEW_PRINT_WITH_CLOUD_PRINT);
On 2011/09/22 23:23:39, Albert Bodenhamer wrote:
> It has a non-translatable section for the service name, but it isn't
> parameterized.  Am I missing something?
> 
> On 2011/09/21 21:57:41, kmadhusu wrote:
> > IDS_PRINT_PREVIEW_PRINT_WITH_CLOUD_PRINT is a formatted string, Do you need
to
> > pass the string values for the place holders?
> 

I thought <ph> tags (place holder tags) allows us to add dynamic strings at run
time. I am not sure whether it is used to specify non-translatable section for
the service name.  I may be wrong. Can you provide some reference link?

Eg: Custom string example <ph name=TEST_NAME>$1<eg>Google</eg></ph>

'$1' refers to the argument passed along with this IDS... string.

[Albert Bodenhamer, 2011-09-23 16:34:25]

http://codereview.chromium.org/7976017/diff/3001/chrome/browser/ui/webui/prin...
File chrome/browser/ui/webui/print_preview_data_source.cc (right):

http://codereview.chromium.org/7976017/diff/3001/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_data_source.cc:97:
AddLocalizedString("systemDialogOption",
The link actually triggers the system print dialog behavior which generates a
new PDF based on default values.  The page range value is discarded so illegal
values aren't a problem.

The intent was to be more consistent with the system dialog option on other
platforms which starts clean.

On 2011/09/23 01:27:19, kmadhusu wrote:
> On 2011/09/22 23:23:39, Albert Bodenhamer wrote:
> > On Chrome OS, cloud print IS the system print dialog.  This is just an
attempt
> > to make it more clear by changing the string but keeping the behavior
> consistent
> > with other platforms.
> > 
> > Eventually we want a single dialog that seemlessly does everything both
> dialogs
> > do today and we'll want to remove the link at that point.
> > 
> > I've pushed the string selection into the html.  Hopefully that will make
> things
> > more clear.
> > 
> > On 2011/09/21 21:57:41, kmadhusu wrote:
> > > "systemDialogOption" is a just a escape hatch to use the native print
system
> > > dialog. Once the print preview is stable and has all the advanced options
of
> > > native dialog, we may remove this link forever. 
> > > 
> > > Is it the same for cloud print dialog option? It's better if you can
create
> a
> > > new link for this cloud print dialog link.  You may also want to document
> this
> > > in the html file. 
> > 
> 
> Thanks for the clarification. On Chrome OS, if you have a link to print using
> cloud print dialog, how are we going to handle the page range errors? (Having
a
> link does not solve this issue crbug.com/97175) 
>

http://codereview.chromium.org/7976017/diff/3001/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_data_source.cc:106:
IDS_PRINT_PREVIEW_PRINT_WITH_CLOUD_PRINT);
Strangely, I'm having a hard time finding public documentation on GRIT. I'll
send you an internal link separately. (And bug joi@ to get the docs live)

<ph> specifies a non-translatable element.  $1, $2, etc are parameters that can
be dynamically replaced at runtime.  Usually, parameters occur inside of a
non-translatable element, but not all non-translatable elements contain
parameters.

In this case, the <ph> marks the name of the Google Cloud Print service, but
doesn't contain any parameters.

On 2011/09/23 01:27:19, kmadhusu wrote:
> On 2011/09/22 23:23:39, Albert Bodenhamer wrote:
> > It has a non-translatable section for the service name, but it isn't
> > parameterized.  Am I missing something?
> > 
> > On 2011/09/21 21:57:41, kmadhusu wrote:
> > > IDS_PRINT_PREVIEW_PRINT_WITH_CLOUD_PRINT is a formatted string, Do you
need
> to
> > > pass the string values for the place holders?
> > 
> 
> I thought <ph> tags (place holder tags) allows us to add dynamic strings at
run
> time. I am not sure whether it is used to specify non-translatable section for
> the service name.  I may be wrong. Can you provide some reference link?
> 
> Eg: Custom string example <ph name=TEST_NAME>$1<eg>Google</eg></ph>
> 
> '$1' refers to the argument passed along with this IDS... string.

[Jói, 2011-09-23 16:37:59]

> Strangely, I'm having a hard time finding public documentation on GRIT.
> I'll send you an internal link separately. (And bug joi@ to get the docs
> live)

http://code.google.com/p/grit-i18n/wiki/GritUsersGuide

This has been live all of 3 days but is more or less a straight port
of documentation from another branch of grit that is somewhat older
:-)  It documents some features that have not yet made it to the
version in the grit-i18n project, but that I plan to port over very
soon.  For most features though, it should be good as an overview.

Cheers,
Jói


On Fri, Sep 23, 2011 at 4:34 PM, <abodenha@chromium.org> wrote:
>
>
http://codereview.chromium.org/7976017/diff/3001/chrome/browser/ui/webui/prin...
> File chrome/browser/ui/webui/print_preview_data_source.cc (right):
>
>
http://codereview.chromium.org/7976017/diff/3001/chrome/browser/ui/webui/prin...
> chrome/browser/ui/webui/print_preview_data_source.cc:97:
> AddLocalizedString("systemDialogOption",
> The link actually triggers the system print dialog behavior which
> generates a new PDF based on default values.  The page range value is
> discarded so illegal values aren't a problem.
>
> The intent was to be more consistent with the system dialog option on
> other platforms which starts clean.
>
> On 2011/09/23 01:27:19, kmadhusu wrote:
>>
>> On 2011/09/22 23:23:39, Albert Bodenhamer wrote:
>> > On Chrome OS, cloud print IS the system print dialog.  This is just
>
> an attempt
>>
>> > to make it more clear by changing the string but keeping the
>
> behavior
>>
>> consistent
>> > with other platforms.
>> >
>> > Eventually we want a single dialog that seemlessly does everything
>
> both
>>
>> dialogs
>> > do today and we'll want to remove the link at that point.
>> >
>> > I've pushed the string selection into the html.  Hopefully that will
>
> make
>>
>> things
>> > more clear.
>> >
>> > On 2011/09/21 21:57:41, kmadhusu wrote:
>> > > "systemDialogOption" is a just a escape hatch to use the native
>
> print system
>>
>> > > dialog. Once the print preview is stable and has all the advanced
>
> options of
>>
>> > > native dialog, we may remove this link forever.
>> > >
>> > > Is it the same for cloud print dialog option? It's better if you
>
> can create
>>
>> a
>> > > new link for this cloud print dialog link.  You may also want to
>
> document
>>
>> this
>> > > in the html file.
>> >
>
>> Thanks for the clarification. On Chrome OS, if you have a link to
>
> print using
>>
>> cloud print dialog, how are we going to handle the page range errors?
>
> (Having a
>>
>> link does not solve this issue crbug.com/97175)
>
>
>
http://codereview.chromium.org/7976017/diff/3001/chrome/browser/ui/webui/prin...
> chrome/browser/ui/webui/print_preview_data_source.cc:106:
> IDS_PRINT_PREVIEW_PRINT_WITH_CLOUD_PRINT);
> Strangely, I'm having a hard time finding public documentation on GRIT.
> I'll send you an internal link separately. (And bug joi@ to get the docs
> live)
>
> <ph> specifies a non-translatable element.  $1, $2, etc are parameters
> that can be dynamically replaced at runtime.  Usually, parameters occur
> inside of a non-translatable element, but not all non-translatable
> elements contain parameters.
>
> In this case, the <ph> marks the name of the Google Cloud Print service,
> but doesn't contain any parameters.
>
> On 2011/09/23 01:27:19, kmadhusu wrote:
>>
>> On 2011/09/22 23:23:39, Albert Bodenhamer wrote:
>> > It has a non-translatable section for the service name, but it isn't
>> > parameterized.  Am I missing something?
>> >
>> > On 2011/09/21 21:57:41, kmadhusu wrote:
>> > > IDS_PRINT_PREVIEW_PRINT_WITH_CLOUD_PRINT is a formatted string, Do
>
> you need
>>
>> to
>> > > pass the string values for the place holders?
>> >
>
>> I thought <ph> tags (place holder tags) allows us to add dynamic
>
> strings at run
>>
>> time. I am not sure whether it is used to specify non-translatable
>
> section for
>>
>> the service name.  I may be wrong. Can you provide some reference
>
> link?
>
>> Eg: Custom string example <ph name=TEST_NAME>$1<eg>Google</eg></ph>
>
>> '$1' refers to the argument passed along with this IDS... string.
>
> http://codereview.chromium.org/7976017/

[dpapad, 2011-09-23 19:57:18]

One comment about print_preview_cloud.js. Some logic has been moved there but
not in an object-oriented way (no class CloudPrintSettings) as done with other
settings like CopiesSettings etc. I am not saying that it should definitely be
done, but it is something to keep in mind for a future CL. I would not want to
delay landing this CL which is actually addressing bug 97175.

Also when I compile with GYP_DEFINES="chromeos=1" I always get "The selected
printer is not available or not installed correctly.  Check your printer or try
selecting another printer." while "Print To PDF" is selected.

http://codereview.chromium.org/7976017/diff/12001/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/print_preview.js (right):

http://codereview.chromium.org/7976017/diff/12001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:424: var printToPDF =
getSelectedPrinterName() == PRINT_TO_PDF;
Nit: No need to invoke getSelectedPrinterName() twice. use a local var
|selectedPrinterName|.

[kmadhusu, 2011-09-23 20:30:07]

Thanks for providing the reference link for .grd file description.

http://codereview.chromium.org/7976017/diff/12001/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/print_preview.js (right):

http://codereview.chromium.org/7976017/diff/12001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:317: selectedPrinter()
== PRINT_TO_PDF ||
selectedPrinter() => selectedPrinter

http://codereview.chromium.org/7976017/diff/12001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:425: var
printWithCloudPrint = getSelectedPrinterName() == PRINT_WITH_CLOUD_PRINT;
nit:

var deviceName = getSelectedPrinterName();
var printToPDF = deviceName == PRINT_TO_PDF;	  
var printWithCloudPrint = deviceName == PRINT_WITH_CLOUD_PRINT;

http://codereview.chromium.org/7976017/diff/12001/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_data_source.cc (right):

http://codereview.chromium.org/7976017/diff/12001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_data_source.cc:96:
AddString("cloudPrintDialogOption", l10n_util::GetStringFUTF16(

Add line 96 within the #if defined(OS_CHROMEOS)

Eg:
#if defined(OS_CHROMEOS)
   AddString("cloudPrintDialogOption" ,... )
#endif

[Albert Bodenhamer, 2011-09-23 22:10:44]

On 2011/09/23 19:57:18, dpapad wrote:
> One comment about print_preview_cloud.js. Some logic has been moved there but
> not in an object-oriented way (no class CloudPrintSettings) as done with other
> settings like CopiesSettings etc. I am not saying that it should definitely be
> done, but it is something to keep in mind for a future CL. I would not want to
> delay landing this CL which is actually addressing bug 97175.
> 

It's definitely worth looking at.  I'm not going to add a todo since this is
more of a "keep this in mind" rather than a "todo".

> Also when I compile with GYP_DEFINES="chromeos=1" I always get "The selected
> printer is not available or not installed correctly.  Check your printer or
try
> selecting another printer." while "Print To PDF" is selected.

It's working fine for me.  I'm unable to actually save the PDF since it can't
access a chrome os file system, but the dialog comes up properly and if I
hardwire a path it saves properly.

At what point are you getting that message?  Does the dialog display?

> 
>
http://codereview.chromium.org/7976017/diff/12001/chrome/browser/resources/pr...
> File chrome/browser/resources/print_preview/print_preview.js (right):
> 
>
http://codereview.chromium.org/7976017/diff/12001/chrome/browser/resources/pr...
> chrome/browser/resources/print_preview/print_preview.js:424: var printToPDF =
> getSelectedPrinterName() == PRINT_TO_PDF;
> Nit: No need to invoke getSelectedPrinterName() twice. use a local var
> |selectedPrinterName|.

[Albert Bodenhamer, 2011-09-23 22:10:56]

http://codereview.chromium.org/7976017/diff/12001/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/print_preview.js (right):

http://codereview.chromium.org/7976017/diff/12001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:317: selectedPrinter()
== PRINT_TO_PDF ||
On 2011/09/23 20:30:07, kmadhusu wrote:
> selectedPrinter() => selectedPrinter

Done.

http://codereview.chromium.org/7976017/diff/12001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:424: var printToPDF =
getSelectedPrinterName() == PRINT_TO_PDF;
On 2011/09/23 19:57:18, dpapad wrote:
> Nit: No need to invoke getSelectedPrinterName() twice. use a local var
> |selectedPrinterName|.

Done.

http://codereview.chromium.org/7976017/diff/12001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:425: var
printWithCloudPrint = getSelectedPrinterName() == PRINT_WITH_CLOUD_PRINT;
On 2011/09/23 20:30:07, kmadhusu wrote:
> nit:
> 
> var deviceName = getSelectedPrinterName();
> var printToPDF = deviceName == PRINT_TO_PDF;	  
> var printWithCloudPrint = deviceName == PRINT_WITH_CLOUD_PRINT;

Done.

http://codereview.chromium.org/7976017/diff/12001/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_data_source.cc (right):

http://codereview.chromium.org/7976017/diff/12001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_data_source.cc:96:
AddString("cloudPrintDialogOption", l10n_util::GetStringFUTF16(
On 2011/09/23 20:30:07, kmadhusu wrote:
> 
> Add line 96 within the #if defined(OS_CHROMEOS)
> 
> Eg:
> #if defined(OS_CHROMEOS)
>    AddString("cloudPrintDialogOption" ,... )
> #endif

Done.

[dpapad, 2011-09-23 22:16:47]

On 2011/09/23 22:10:44, Albert Bodenhamer wrote:
> On 2011/09/23 19:57:18, dpapad wrote:
> > One comment about print_preview_cloud.js. Some logic has been moved there
but
> > not in an object-oriented way (no class CloudPrintSettings) as done with
other
> > settings like CopiesSettings etc. I am not saying that it should definitely
be
> > done, but it is something to keep in mind for a future CL. I would not want
to
> > delay landing this CL which is actually addressing bug 97175.
> > 
> 
> It's definitely worth looking at.  I'm not going to add a todo since this is
> more of a "keep this in mind" rather than a "todo".
> 
> > Also when I compile with GYP_DEFINES="chromeos=1" I always get "The selected
> > printer is not available or not installed correctly.  Check your printer or
> try
> > selecting another printer." while "Print To PDF" is selected.
> 
> It's working fine for me.  I'm unable to actually save the PDF since it can't
> access a chrome os file system, but the dialog comes up properly and if I
> hardwire a path it saves properly.
> 
> At what point are you getting that message?  Does the dialog display?
> 
> > 
> >
>
http://codereview.chromium.org/7976017/diff/12001/chrome/browser/resources/pr...
> > File chrome/browser/resources/print_preview/print_preview.js (right):
> > 
> >
>
http://codereview.chromium.org/7976017/diff/12001/chrome/browser/resources/pr...
> > chrome/browser/resources/print_preview/print_preview.js:424: var printToPDF
=
> > getSelectedPrinterName() == PRINT_TO_PDF;
> > Nit: No need to invoke getSelectedPrinterName() twice. use a local var
> > |selectedPrinterName|.

When I hit ctrl+p the print preview tab opens, and instead of the preview
document on the right, I see the error message and therefore the print button is
disabled. If I try 10 times it might succeed 1 time on average and then the
dialog will be displayed after clicking print.

[kmadhusu, 2011-09-23 22:36:35]

lgtm

[Albert Bodenhamer, 2011-09-23 22:45:47]

On Fri, Sep 23, 2011 at 3:16 PM, <dpapad@chromium.org> wrote:

> On 2011/09/23 22:10:44, Albert Bodenhamer wrote:
>
>> On 2011/09/23 19:57:18, dpapad wrote:
>> > One comment about print_preview_cloud.js. Some logic has been moved
>> there
>>
> but
>
>> > not in an object-oriented way (no class CloudPrintSettings) as done with
>>
> other
>
>> > settings like CopiesSettings etc. I am not saying that it should
>> definitely
>>
> be
>
>> > done, but it is something to keep in mind for a future CL. I would not
>> want
>>
> to
>
>> > delay landing this CL which is actually addressing bug 97175.
>> >
>>
>
>  It's definitely worth looking at.  I'm not going to add a todo since this
>> is
>> more of a "keep this in mind" rather than a "todo".
>>
>
>  > Also when I compile with GYP_DEFINES="chromeos=1" I always get "The
>> selected
>> > printer is not available or not installed correctly.  Check your printer
>> or
>> try
>> > selecting another printer." while "Print To PDF" is selected.
>>
>
>  It's working fine for me.  I'm unable to actually save the PDF since it
>> can't
>> access a chrome os file system, but the dialog comes up properly and if I
>> hardwire a path it saves properly.
>>
>
>  At what point are you getting that message?  Does the dialog display?
>>
>
>  >
>> >
>>
>
> http://codereview.chromium.**org/7976017/diff/12001/chrome/**
>
browser/resources/print_**preview/print_preview.js<http://codereview.chromium.org/7976017/diff/12001/chrome/browser/resources/print_preview/print_preview.js>
>
>> > File chrome/browser/resources/**print_preview/print_preview.js (right):
>> >
>> >
>>
>
> http://codereview.chromium.**org/7976017/diff/12001/chrome/**
>
browser/resources/print_**preview/print_preview.js#**newcode424<http://codereview.chromium.org/7976017/diff/12001/chrome/browser/resources/print_preview/print_preview.js#newcode424>
>
>> > chrome/browser/resources/**print_preview/print_preview.**js:424: var
>> printToPDF
>>
> =
>
>> > getSelectedPrinterName() == PRINT_TO_PDF;
>>
>> > Nit: No need to invoke getSelectedPrinterName() twice. use a local var
>> > |selectedPrinterName|.
>>
>
> When I hit ctrl+p the print preview tab opens, and instead of the preview
> document on the right, I see the error message and therefore the print
> button is
> disabled. If I try 10 times it might succeed 1 time on average and then the
> dialog will be displayed after clicking print.
>
>
http://codereview.chromium.**org/7976017/<http://codereview.chromium.org/7976...
>

That message fires if PrintWebViewHelper::InitPrintSettings fails.  That's
just trying to grab settings from the "native" print system to serve as a
starting point.  On Chrome OS that should just be returning a sensible set
of hardwired default values.  I'll dig a bit further to see how this could
fail.

I'm baffled how that could be related to this CL.  It only happens with my
CL patched in, right?


-- 
Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com>
org

[dpapad, 2011-09-23 22:52:03]

On 2011/09/23 22:45:47, Albert Bodenhamer wrote:
> On Fri, Sep 23, 2011 at 3:16 PM, <mailto:dpapad@chromium.org> wrote:
> 
> > On 2011/09/23 22:10:44, Albert Bodenhamer wrote:
> >
> >> On 2011/09/23 19:57:18, dpapad wrote:
> >> > One comment about print_preview_cloud.js. Some logic has been moved
> >> there
> >>
> > but
> >
> >> > not in an object-oriented way (no class CloudPrintSettings) as done with
> >>
> > other
> >
> >> > settings like CopiesSettings etc. I am not saying that it should
> >> definitely
> >>
> > be
> >
> >> > done, but it is something to keep in mind for a future CL. I would not
> >> want
> >>
> > to
> >
> >> > delay landing this CL which is actually addressing bug 97175.
> >> >
> >>
> >
> >  It's definitely worth looking at.  I'm not going to add a todo since this
> >> is
> >> more of a "keep this in mind" rather than a "todo".
> >>
> >
> >  > Also when I compile with GYP_DEFINES="chromeos=1" I always get "The
> >> selected
> >> > printer is not available or not installed correctly.  Check your printer
> >> or
> >> try
> >> > selecting another printer." while "Print To PDF" is selected.
> >>
> >
> >  It's working fine for me.  I'm unable to actually save the PDF since it
> >> can't
> >> access a chrome os file system, but the dialog comes up properly and if I
> >> hardwire a path it saves properly.
> >>
> >
> >  At what point are you getting that message?  Does the dialog display?
> >>
> >
> >  >
> >> >
> >>
> >
> > http://codereview.chromium.**org/7976017/diff/12001/chrome/**
> >
>
browser/resources/print_**preview/print_preview.js<http://codereview.chromium.org/7976017/diff/12001/chrome/browser/resources/print_preview/print_preview.js>
> >
> >> > File chrome/browser/resources/**print_preview/print_preview.js (right):
> >> >
> >> >
> >>
> >
> > http://codereview.chromium.**org/7976017/diff/12001/chrome/**
> >
>
browser/resources/print_**preview/print_preview.js#**newcode424<http://codereview.chromium.org/7976017/diff/12001/chrome/browser/resources/print_preview/print_preview.js#newcode424>
> >
> >> > chrome/browser/resources/**print_preview/print_preview.**js:424: var
> >> printToPDF
> >>
> > =
> >
> >> > getSelectedPrinterName() == PRINT_TO_PDF;
> >>
> >> > Nit: No need to invoke getSelectedPrinterName() twice. use a local var
> >> > |selectedPrinterName|.
> >>
> >
> > When I hit ctrl+p the print preview tab opens, and instead of the preview
> > document on the right, I see the error message and therefore the print
> > button is
> > disabled. If I try 10 times it might succeed 1 time on average and then the
> > dialog will be displayed after clicking print.
> >
> >
>
http://codereview.chromium.**org/7976017/%3Chttp://codereview.chromium.org/79...>
> >
> 
> That message fires if PrintWebViewHelper::InitPrintSettings fails.  That's
> just trying to grab settings from the "native" print system to serve as a
> starting point.  On Chrome OS that should just be returning a sensible set
> of hardwired default values.  I'll dig a bit further to see how this could
> fail.
> 
> I'm baffled how that could be related to this CL.  It only happens with my
> CL patched in, right?
> 
> 
> -- 
> Albert Bodenhamer | Software Engineer |
mailto:abodenha@chromium.%3Cabodenha@google.com>
> org

Negative, it happens on trunk also for me. I had not tried without your changes
before. So it is not related to this CL.

[Albert Bodenhamer, 2011-09-23 22:57:14]

On Fri, Sep 23, 2011 at 3:52 PM, <dpapad@chromium.org> wrote:

> On 2011/09/23 22:45:47, Albert Bodenhamer wrote:
>
>  On Fri, Sep 23, 2011 at 3:16 PM, <mailto:dpapad@chromium.org> wrote:
>>
>
>  > On 2011/09/23 22:10:44, Albert Bodenhamer wrote:
>> >
>> >> On 2011/09/23 19:57:18, dpapad wrote:
>> >> > One comment about print_preview_cloud.js. Some logic has been moved
>> >> there
>> >>
>> > but
>> >
>> >> > not in an object-oriented way (no class CloudPrintSettings) as done
>> with
>> >>
>> > other
>> >
>> >> > settings like CopiesSettings etc. I am not saying that it should
>> >> definitely
>> >>
>> > be
>> >
>> >> > done, but it is something to keep in mind for a future CL. I would
>> not
>> >> want
>> >>
>> > to
>> >
>> >> > delay landing this CL which is actually addressing bug 97175.
>> >> >
>> >>
>> >
>> >  It's definitely worth looking at.  I'm not going to add a todo since
>> this
>> >> is
>> >> more of a "keep this in mind" rather than a "todo".
>> >>
>> >
>> >  > Also when I compile with GYP_DEFINES="chromeos=1" I always get "The
>> >> selected
>> >> > printer is not available or not installed correctly.  Check your
>> printer
>> >> or
>> >> try
>> >> > selecting another printer." while "Print To PDF" is selected.
>> >>
>> >
>> >  It's working fine for me.  I'm unable to actually save the PDF since it
>> >> can't
>> >> access a chrome os file system, but the dialog comes up properly and if
>> I
>> >> hardwire a path it saves properly.
>> >>
>> >
>> >  At what point are you getting that message?  Does the dialog display?
>> >>
>> >
>> >  >
>> >> >
>> >>
>> >
>> > http://codereview.chromium.****org/7976017/diff/12001/chrome/****
>> >
>>
>
> browser/resources/print_****preview/print_preview.js<http:**
> //codereview.chromium.org/**7976017/diff/12001/chrome/**
>
browser/resources/print_**preview/print_preview.js<http://codereview.chromium.org/7976017/diff/12001/chrome/browser/resources/print_preview/print_preview.js>
> >
>
>> >
>> >> > File chrome/browser/resources/****print_preview/print_preview.js
>> (right):
>> >> >
>> >> >
>> >>
>> >
>> > http://codereview.chromium.****org/7976017/diff/12001/chrome/****
>> >
>>
>
> browser/resources/print_****preview/print_preview.js#****newcode424<
> http://codereview.**chromium.org/7976017/diff/**12001/chrome/browser/**
>
resources/print_preview/print_**preview.js#newcode424<http://codereview.chromium.org/7976017/diff/12001/chrome/browser/resources/print_preview/print_preview.js#newcode424>
> >
>
>> >
>> >> > chrome/browser/resources/****print_preview/print_preview.****js:424:
>> var
>>
>> >> printToPDF
>> >>
>> > =
>> >
>> >> > getSelectedPrinterName() == PRINT_TO_PDF;
>> >>
>> >> > Nit: No need to invoke getSelectedPrinterName() twice. use a local
>> var
>> >> > |selectedPrinterName|.
>> >>
>> >
>> > When I hit ctrl+p the print preview tab opens, and instead of the
>> preview
>> > document on the right, I see the error message and therefore the print
>> > button is
>> > disabled. If I try 10 times it might succeed 1 time on average and then
>> the
>> > dialog will be displayed after clicking print.
>> >
>> >
>>
>
> http://codereview.chromium.****org/7976017/%3Chttp://coderevi**
> ew.chromium.org/7976017/ <http://codereview.chromium.org/7976017/>>
>
>  >
>>
>
>  That message fires if PrintWebViewHelper::**InitPrintSettings fails.
>>  That's
>> just trying to grab settings from the "native" print system to serve as a
>> starting point.  On Chrome OS that should just be returning a sensible set
>> of hardwired default values.  I'll dig a bit further to see how this could
>> fail.
>>
>
>  I'm baffled how that could be related to this CL.  It only happens with my
>> CL patched in, right?
>>
>
>
>  --
>> Albert Bodenhamer | Software Engineer |
>>
> mailto:abodenha@chromium.%3Cab**odenha@google.com <3Cabodenha@google.com>>
>
>> org
>>
>
> Negative, it happens on trunk also for me. I had not tried without your
> changes
> before. So it is not related to this CL.
>
>
Ah.  Ok.  Do you mind if I go ahead and submit this and investigate the
failure you're seeing as a separate issue?


>
http://codereview.chromium.**org/7976017/<http://codereview.chromium.org/7976...
>



-- 
Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com>
org

[dpapad, 2011-09-23 23:02:49]

Go ahead. LGTM

[Albert Bodenhamer, 2011-09-28 02:21:58]

Lei discovered an issue where printing to a regular printer in Linux would
trigger the GCP dialog.  He rolled back the change.

It turned out to be an issue in print_preview_handler.cc where I checked for the
existence of a key rather than getting the associated value.

I've uploaded a new patch with the fix and started a new set of try jobs.  I'll
test thoroughly when I get in in the AM and commit again if there are no
problems/objections.

Thanks

[kmadhusu, 2011-09-28 17:08:34]

LGTM

[dpapad, 2011-09-28 18:26:30]

LGTM

http://codereview.chromium.org/7976017/diff/27001/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/7976017/diff/27001/chrome/app/generated_resour...
chrome/app/generated_resources.grd:6080: <message
name="IDS_PRINT_PREVIEW_MORE_PRINTERS"
Remove whitespace at the end.

http://codereview.chromium.org/7976017/diff/27001/chrome/app/generated_resour...
chrome/app/generated_resources.grd:6081: desc="Option shown in printer drop-down
list that displays an interface for finding/using additional cloud based
printers.">
Same here.