GTK: Add a dialog for printing.

GTK: Add a dialog for printing.

Add an infobar directing users to tell us if they have problems with printing.

Hide printing behind --enable-printing flag on linux/gtk.

BUG=9847
TEST=none

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

[Evan Stade, 2009-09-16 01:29:36]

this works for me with the one printer I tested, but I've decided it has several
problems:

1) the dialog is popped up *after* the pdf is created. It should pop up
beforehand (or in parallel), so that if the user presses ctrl+p by mistake he
doesn't have to wait for the pdf to be generated before he can cancel.
2) we should optionally create a PS instead of a PDF if the printer supports one
but not the other
3) if the printer supports neither one, ??? (can this even happen?)

solving 1 and 2 shouldn't be hard but before I proceed I was curious about your
opinion on whether we care about 1 and 2 and what to do for 3.

[Evan Stade, 2009-09-16 01:30:51]

+maruel

[M-A Ruel, 2009-09-16 01:47:23]

lgtm

http://codereview.chromium.org/200138/diff/1/2
File chrome/browser/printing/print_dialog_gtk.cc (right):

http://codereview.chromium.org/200138/diff/1/2#newcode34
Line 34: // FIXME: window title
TODO(estade)

[Evan Stade, 2009-09-16 02:18:31]

did you have any opinion about the problems I pointed out in my first message?

[M-A Ruel, 2009-09-16 11:28:55]

On 2009/09/16 02:18:31, Evan Stade wrote:
> did you have any opinion about the problems I pointed out in my first message?

1) This is a side-effect of how it works on Windows. I'd say to refactory the
behavior as soon as you can. There are deadlocks issues but that could be much
simpler on linux. Bug 21555 solves all known issues.

2) That would be specified on the GetDefaultPrinterSettings. PS support can be
added later. File a bug if you think it's important and make 9847 dependent on
this one.

3) I guess so, it would be determined on GetDefaultPrinterSettings and would
return that no printer is avail.

Also, please fix the description, you should use BUG=9847.

[M-A Ruel, 2009-09-16 11:29:26]

And please sync and try again, the patch failed to apply on all platforms.

[tony, 2009-09-16 17:32:38]

2) There were problems with PS.  The problem was that the cairo PS backend uses
temp files while writing the PS file.  Since this happens in the renderer, it
will fail with the sandbox.  It's in cairo itself, so I'm not sure we can work
around this.

3) Technically, don't we send the pdf to CUPS and CUPS does something?  Min-yu
said there were some pdfs that CUPS (or the printer in the office) couldn't
handle.  I guess we need to figure out why some pdf files don't work or if some
printers can't handle pdf or what.

(Taking a quick look at the change now...)

[Evan Stade, 2009-09-16 17:34:23]

On 2009/09/16 11:28:55, Marc-Antoine Ruel wrote:
> On 2009/09/16 02:18:31, Evan Stade wrote:
> > did you have any opinion about the problems I pointed out in my first
message?
> 
> 1) This is a side-effect of how it works on Windows. I'd say to refactory the
> behavior as soon as you can. There are deadlocks issues but that could be much
> simpler on linux. Bug 21555 solves all known issues.
> 
> 2) That would be specified on the GetDefaultPrinterSettings. PS support can be
> added later. File a bug if you think it's important and make 9847 dependent on
> this one.

We can't know if PS is supported until the printer is chosen, so fixing this is
dependent on fixing (1) as well.

> 
> 3) I guess so, it would be determined on GetDefaultPrinterSettings and would
> return that no printer is avail.

my question is more, what do we do? presumably an error dialog or infobar would
be in order but if it doesn't happen very often it might not be worth it.

> 
> Also, please fix the description, you should use BUG=9847.

done.

[tony, 2009-09-16 17:39:07]

http://codereview.chromium.org/200138/diff/1/2
File chrome/browser/printing/print_dialog_gtk.cc (right):

http://codereview.chromium.org/200138/diff/1/2#newcode7
Line 7: #include <gtk-unix-print-2.0/gtk/gtkprinter.h>
Out of curiosity, is this provided by gtk-2.0? (ie, there are no new library
dependencies?)

http://codereview.chromium.org/200138/diff/1/2#newcode53
Line 53: LOG(ERROR) << "Selected printer doesn't accept pdf.";
I think a good way to test this is to push it in a dev build and ask people for
feedback (maybe on the splash page?).  Maybe we can even pop up a dialog with a
bug number for people to report to.

http://codereview.chromium.org/200138/diff/1/2#newcode54
Line 54: return;
Do you need to gtk_widget_destroy and delete this here?

http://codereview.chromium.org/200138/diff/1/2#newcode72
Line 72: break;
Do you need to gtk_widget_destroy and delete this here?

http://codereview.chromium.org/200138/diff/1/3
File chrome/browser/printing/print_dialog_gtk.h (right):

http://codereview.chromium.org/200138/diff/1/3#newcode24
Line 24: PrintDialogGtk(const FilePath& path_to_pdf);
Nit: explicit

http://codereview.chromium.org/200138/diff/1/3#newcode47
Line 47: };
Nit: disallow_copy_and_assign

[M-A Ruel, 2009-09-16 17:42:11]

On 2009/09/16 17:34:23, Evan Stade wrote:
> On 2009/09/16 11:28:55, Marc-Antoine Ruel wrote:
> > 1) This is a side-effect of how it works on Windows. I'd say to refactory
the
> > behavior as soon as you can. There are deadlocks issues but that could be
much
> > simpler on linux. Bug 21555 solves all known issues.
> > 
> > 2) That would be specified on the GetDefaultPrinterSettings. PS support can
be
> > added later. File a bug if you think it's important and make 9847 dependent
on
> > this one.
> 
> We can't know if PS is supported until the printer is chosen, so fixing this
is
> dependent on fixing (1) as well.

See the thread on chromium-dev about print preview.


> > 3) I guess so, it would be determined on GetDefaultPrinterSettings and would
> > return that no printer is avail.
> 
> my question is more, what do we do? presumably an error dialog or infobar
would
> be in order but if it doesn't happen very often it might not be worth it.

It's currently a javascript's alert()-style dialog box on Windows. See
PrintWebViewHelper::Print() in print_web_view_helper_win.cc


> > Also, please fix the description, you should use BUG=9847.
> 
> done.

I'm surprised the presubmit check doesn't complain about TEST too on your side.

[M-A Ruel, 2009-09-16 17:42:42]

Oh, and you still need to sync to see if you patch pass on try slaves.

[Evan Stade, 2009-09-16 17:51:43]

I honestly don't see the point of writing TEST=none or TEST=print something.

[Evan Stade, 2009-09-16 18:11:40]

http://codereview.chromium.org/200138/diff/1/2
File chrome/browser/printing/print_dialog_gtk.cc (right):

http://codereview.chromium.org/200138/diff/1/2#newcode7
Line 7: #include <gtk-unix-print-2.0/gtk/gtkprinter.h>
On 2009/09/16 17:39:07, tony wrote:
> Out of curiosity, is this provided by gtk-2.0? (ie, there are no new library
> dependencies?)

yes

http://codereview.chromium.org/200138/diff/1/2#newcode53
Line 53: LOG(ERROR) << "Selected printer doesn't accept pdf.";
On 2009/09/16 17:39:07, tony wrote:
> I think a good way to test this is to push it in a dev build and ask people
for
> feedback (maybe on the splash page?).  Maybe we can even pop up a dialog with
a
> bug number for people to report to.

that sounds like a good idea. Do you think we should also hide it behind a
--enable-printing flag?

http://codereview.chromium.org/200138/diff/1/2#newcode54
Line 54: return;
On 2009/09/16 17:39:07, tony wrote:
> Do you need to gtk_widget_destroy and delete this here?

yea, sorry. This was not intended to be reviewed yet. Fixed here and below.

[tony, 2009-09-16 18:19:43]

http://codereview.chromium.org/200138/diff/1/2
File chrome/browser/printing/print_dialog_gtk.cc (right):

http://codereview.chromium.org/200138/diff/1/2#newcode53
Line 53: LOG(ERROR) << "Selected printer doesn't accept pdf.";
On 2009/09/16 18:11:40, Evan Stade wrote:
> On 2009/09/16 17:39:07, tony wrote:
> > I think a good way to test this is to push it in a dev build and ask people
> for
> > feedback (maybe on the splash page?).  Maybe we can even pop up a dialog
with
> a
> > bug number for people to report to.
> 
> that sounds like a good idea. Do you think we should also hide it behind a
> --enable-printing flag?

Yes, I think that's fine as long as we call it out on the splash page like we
did for plugins.

[Evan Stade, 2009-09-16 20:08:37]

updated.

[M-A Ruel, 2009-09-16 20:20:52]

On 2009/09/16 20:08:37, Evan Stade wrote:
> updated.

Still compile failure on mac. Also do a run with:
gcl try print --bot linux_view
just to be sure you don't break TOOLKIT_VIEWS

[Evan Stade, 2009-09-16 20:24:05]

On Wed, Sep 16, 2009 at 1:20 PM,  <maruel@chromium.org> wrote:
> On 2009/09/16 20:08:37, Evan Stade wrote:
>>
>> updated.
>
> Still compile failure on mac. Also do a run with:
> gcl try print --bot linux_view
> just to be sure you don't break TOOLKIT_VIEWS
>
> http://codereview.chromium.org/200138
>

yea, compile failure on linux as well. For some reason the bot is
complaining about headers included from gtkprintjob.h, although it
works fine on my machine

[tony, 2009-09-16 20:31:04]

Overall, LGTM, just some small nits.

http://codereview.chromium.org/200138/diff/9001/3011
File chrome/browser/browser.cc (right):

http://codereview.chromium.org/200138/diff/9001/3011#newcode2262
Line 2262: #if defined(TOOLKIT_GTK)
Why does toolkit matter here?  It's not going to work in TOOLKIT_VIEWS either.

http://codereview.chromium.org/200138/diff/9001/3007
File chrome/browser/renderer_host/resource_message_filter_gtk.cc (right):

http://codereview.chromium.org/200138/diff/9001/3007#newcode276
Line 276: #endif
#else NOTIMPLEMENTED?

http://codereview.chromium.org/200138/diff/9001/3010
File chrome/browser/resources/linux-splash-chrome.html (right):

http://codereview.chromium.org/200138/diff/9001/3010#newcode62
Line 62: Basic printing support is now available with the --enable-printing
flag. There
Nit: Lots of lines over 80 cols.  It seems like above it's only when the line
can't wrap (long links).

http://codereview.chromium.org/200138/diff/9001/3009
File chrome/browser/resources/linux-splash.html (right):

http://codereview.chromium.org/200138/diff/9001/3009#newcode60
Line 60: Basic printing support is now available with the --enable-printing
flag. There
80 cols.

[Evan Stade, 2009-09-16 20:42:09]

http://codereview.chromium.org/200138/diff/9001/3011
File chrome/browser/browser.cc (right):

http://codereview.chromium.org/200138/diff/9001/3011#newcode2262
Line 2262: #if defined(TOOLKIT_GTK)
On 2009/09/16 20:31:04, tony wrote:
> Why does toolkit matter here?  It's not going to work in TOOLKIT_VIEWS either.

the difference is between being always disabled and being sometimes disabled

http://codereview.chromium.org/200138/diff/9001/3007
File chrome/browser/renderer_host/resource_message_filter_gtk.cc (right):

http://codereview.chromium.org/200138/diff/9001/3007#newcode276
Line 276: #endif
On 2009/09/16 20:31:04, tony wrote:
> #else NOTIMPLEMENTED?

well, these two functions are really TOOLKIT_GTK only, I just put the ifdef here
so it would compile on views, but I think their printing solution is going to be
vastly different.

On views this shouldn't be hit because the print command is disabled, although
now that I think about it I guess javascript can trigger a print so I should put
a NOTIMPLEMENTED() after all.

[tony, 2009-09-16 20:49:22]

I would just enable this on TOOLKIT_VIEWS.  I think for now using this code is
better than nothing.  When they bring up printing, they can decide where the
#ifs go.

But I don't feel strongly either way, so feel free to checkin.

[M-A Ruel, 2009-09-16 20:55:38]

please run a try run before committing (especially linux_view)

http://codereview.chromium.org/200138/diff/5008/5015
File chrome/browser/browser.cc (right):

http://codereview.chromium.org/200138/diff/5008/5015#newcode2265
Line 2265: #else  // defined(TOOLKIT_VIEWS)
nit: If you don't decide to enable for both, I'd say make it explicit. I was
confused at first glance.
#elif defined(TOOLKIT_VIEWS)

[Evan Stade, 2009-09-16 21:39:15]

4 platforms report try success

[myhuang, 2009-09-16 22:33:08]

On 2009/09/16 01:29:36, Evan Stade wrote:
> this works for me with the one printer I tested, but I've decided it has
several
> problems:
> 
> 1) the dialog is popped up *after* the pdf is created. It should pop up
> beforehand (or in parallel), so that if the user presses ctrl+p by mistake he
> doesn't have to wait for the pdf to be generated before he can cancel.
> 2) we should optionally create a PS instead of a PDF if the printer supports
one
> but not the other
> 3) if the printer supports neither one, ??? (can this even happen?)
> 
> solving 1 and 2 shouldn't be hard but before I proceed I was curious about
your
> opinion on whether we care about 1 and 2 and what to do for 3.

I agree that we should solve issue #1.

Just like what Tony mentioned, Cairo will create a temporary file with a PS
surface. I addressed this issue in the wiki page, and you can find other issues
in the wiki page, too.

I am not sure what would happen on the machine which does not install CUPS.
However, if CUPS is installed, we always have
a) At least a valid printer. (I forgot it is /dev/null, or print to file)
b) Newer version of CUPS (> 1.3.11?) always supports PDF. It will call pdftopdf,
pdftops and/or something like that.

If you would have any question, please email me or put me in the CC list. Thank
you very much!

Min-Yu

[myhuang, 2009-09-16 22:38:22]

I forgot to mention that we might need to disable some tabs and/or some
component in the dialog. For example, page range and pages per paper.

[Evan Stade, 2009-09-16 22:38:22]

Min: I landed this patch, so if you want, you could announce to
chromium-dev that basic printing now works in linux (to a certain
extent)

-- Evan Stade



On Wed, Sep 16, 2009 at 3:33 PM,  <minyu.huang@gmail.com> wrote:
> On 2009/09/16 01:29:36, Evan Stade wrote:
>>
>> this works for me with the one printer I tested, but I've decided it has
>
> several
>>
>> problems:
>
>> 1) the dialog is popped up *after* the pdf is created. It should pop up
>> beforehand (or in parallel), so that if the user presses ctrl+p by mistake
>> he
>> doesn't have to wait for the pdf to be generated before he can cancel.
>> 2) we should optionally create a PS instead of a PDF if the printer
>> supports
>
> one
>>
>> but not the other
>> 3) if the printer supports neither one, ??? (can this even happen?)
>
>> solving 1 and 2 shouldn't be hard but before I proceed I was curious about
>
> your
>>
>> opinion on whether we care about 1 and 2 and what to do for 3.
>
> I agree that we should solve issue #1.
>
> Just like what Tony mentioned, Cairo will create a temporary file with a PS
> surface. I addressed this issue in the wiki page, and you can find other
> issues
> in the wiki page, too.
>
> I am not sure what would happen on the machine which does not install CUPS.
> However, if CUPS is installed, we always have
> a) At least a valid printer. (I forgot it is /dev/null, or print to file)
> b) Newer version of CUPS (> 1.3.11?) always supports PDF. It will call
> pdftopdf,
> pdftops and/or something like that.
>
> If you would have any question, please email me or put me in the CC list.
> Thank
> you very much!
>
> Min-Yu
>
> http://codereview.chromium.org/200138
>

[myhuang, 2009-09-16 22:44:08]

On 2009/09/16 22:38:22, Evan Stade wrote:
> Min: I landed this patch, so if you want, you could announce to
> chromium-dev that basic printing now works in linux (to a certain
> extent)
> 
> -- Evan Stade

Hi Even,

 Yes, I notice that you have landed the patch. I appreciate very much for you
help and time. My internship at Google has ended last week (I am sorry that I
could not finish this on time) and I do not have a chromium account. Tony, could
you help us announce this? Thank you very much!


Min-Yu

[Evan Stade, 2009-09-16 22:54:39]

anyone can post to chromium-dev.

-- Evan Stade



On Wed, Sep 16, 2009 at 3:44 PM,  <minyu.huang@gmail.com> wrote:
> On 2009/09/16 22:38:22, Evan Stade wrote:
>>
>> Min: I landed this patch, so if you want, you could announce to
>> chromium-dev that basic printing now works in linux (to a certain
>> extent)
>
>> -- Evan Stade
>
> Hi Even,
>
> =A0Yes, I notice that you have landed the patch. I appreciate very much f=
or
> you
> help and time. My internship at Google has ended last week (I am sorry th=
at
> I
> could not finish this on time) and I do not have a chromium account. Tony=
,
> could
> you help us announce this? Thank you very much!
>
>
> Min-Yu
>
> http://codereview.chromium.org/200138
>