Get semantic capabilities from Print Backend.

Get semantic capabilities from Print Backend.
Moved OS specific code into OS specific print backends (vs. print_system_task_proxy.cc).
On Windows get capabilities using native API (vs. XPS parsing)
Some clean up on code and interfaces for Print Backend.

BUG=144031
TEST=Verify Print Preview on ALL platforms (Win XP, Win 7, Linux, Mac).

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

[Albert Bodenhamer, 2012-08-29 23:54:25]

Let's chat more tomorrow and involve Robert.  If we're going to redo a lot of
this it makes sense to make the integration with the JS side smoother at the
same time.

https://chromiumcodereview.appspot.com/10905006/diff/3004/chrome/renderer/pri...
File chrome/renderer/print_web_view_helper.cc (right):

https://chromiumcodereview.appspot.com/10905006/diff/3004/chrome/renderer/pri...
chrome/renderer/print_web_view_helper.cc:962: VLOG(1) << "Creating preview
document";
Temp debug code I assume?

https://chromiumcodereview.appspot.com/10905006/diff/3004/printing/backend/pr...
File printing/backend/print_backend.h (right):

https://chromiumcodereview.appspot.com/10905006/diff/3004/printing/backend/pr...
printing/backend/print_backend.h:86: virtual bool GetPrinterCapsAndDefaults(
Do we need to keep both?

https://chromiumcodereview.appspot.com/10905006/diff/3004/printing/backend/pr...
File printing/backend/print_backend_win.cc (right):

https://chromiumcodereview.appspot.com/10905006/diff/3004/printing/backend/pr...
printing/backend/print_backend_win.cc:23: template <typename T>
Interesting.  So this is a way to easily cast to the right version of
printer_info struct?

I'm missing why this is easier/better than just using the PRINTER_INFO_X
structures directly (with a reinterpret_cast in the GetPrinter call)

[gene, 2012-08-30 03:19:03]

Yeah, let's chat tomorrow. I would love cleaning JS integration as well.

https://chromiumcodereview.appspot.com/10905006/diff/3004/chrome/renderer/pri...
File chrome/renderer/print_web_view_helper.cc (right):

https://chromiumcodereview.appspot.com/10905006/diff/3004/chrome/renderer/pri...
chrome/renderer/print_web_view_helper.cc:962: VLOG(1) << "Creating preview
document";
yeah :), I'll remove it

https://chromiumcodereview.appspot.com/10905006/diff/3004/printing/backend/pr...
File printing/backend/print_backend.h (right):

https://chromiumcodereview.appspot.com/10905006/diff/3004/printing/backend/pr...
printing/backend/print_backend.h:86: virtual bool GetPrinterCapsAndDefaults(
Definitely yes. This function is used by CloudPrint. The idea to have 2
functions, one returning semantic subset of caps, while another return a full
list of capabilities.

https://chromiumcodereview.appspot.com/10905006/diff/3004/printing/backend/pr...
File printing/backend/print_backend_win.cc (right):

https://chromiumcodereview.appspot.com/10905006/diff/3004/printing/backend/pr...
printing/backend/print_backend_win.cc:23: template <typename T>
Actually the main idea of this class is to handle memory allocation and two
GetPrinter call that are necessary (one for memory size and another to fill it
in). Casting is kind of a nice extra. (I can do just a helper function that will
return a memory buffer for the requested level, and let caller to cast it
itself. But it makes code below more confusing with more casts)

[gene1, 2012-08-31 21:31:29]

OK, looks like code is at the point where everything is working and it is time
to request a format review.

[gene, 2012-09-04 22:01:52]

ping :)

[Albert Bodenhamer, 2012-09-04 23:09:36]

I'm setting up test runs on XP and Mac.

https://chromiumcodereview.appspot.com/10905006/diff/24009/printing/backend/c...
File printing/backend/cups_helper.cc (right):

https://chromiumcodereview.appspot.com/10905006/diff/24009/printing/backend/c...
printing/backend/cups_helper.cc:31: #if !defined(OS_MACOSX)
A large chunk of this file is ifdefed out here.  Does it make sense to pull the
non-MACOS chunk out into its own file?

https://chromiumcodereview.appspot.com/10905006/diff/24009/printing/backend/c...
File printing/backend/cups_helper_unittest.cc (right):

https://chromiumcodereview.appspot.com/10905006/diff/24009/printing/backend/c...
printing/backend/cups_helper_unittest.cc:41: // Test duplex detection code,
which regressed in http://crbug.com/103999.
Should there be more tests here?  Different ways of expressing duplex, color,
etc?

[gene, 2012-09-04 23:19:50]

https://chromiumcodereview.appspot.com/10905006/diff/24009/printing/backend/c...
File printing/backend/cups_helper.cc (right):

https://chromiumcodereview.appspot.com/10905006/diff/24009/printing/backend/c...
printing/backend/cups_helper.cc:31: #if !defined(OS_MACOSX)
2 functions are ifdef'ed for non-MACOS: parse_lpoptions() and mark_lpoptions()
and they are only about 80 lines together. I think it is ok keeping all ppd
parsing code in the same file for now. Later if we have more and more non-MACOS
code, it will make sense to have a separate cups_linux_helper.cc file

https://chromiumcodereview.appspot.com/10905006/diff/24009/printing/backend/c...
File printing/backend/cups_helper_unittest.cc (right):

https://chromiumcodereview.appspot.com/10905006/diff/24009/printing/backend/c...
printing/backend/cups_helper_unittest.cc:41: // Test duplex detection code,
which regressed in http://crbug.com/103999.
Sure. Let me add more. I've copied existing tests so far, so there should be no
regression in test coverage.

[Lei Zhang, 2012-09-05 00:32:32]

a bunch of little things:

https://chromiumcodereview.appspot.com/10905006/diff/24009/chrome/browser/pri...
File chrome/browser/printing/print_system_task_proxy.cc (right):

https://chromiumcodereview.appspot.com/10905006/diff/24009/chrome/browser/pri...
chrome/browser/printing/print_system_task_proxy.cc:10: #include <vector>
nit: you can remove this as well as histogram.h, string_split.h, and
file_util.h.

Maybe also remove string_util.h.

https://chromiumcodereview.appspot.com/10905006/diff/24009/chrome/browser/pri...
chrome/browser/printing/print_system_task_proxy.cc:148: // Old API, ugly code :(
Are we going to fix this in the future?

https://chromiumcodereview.appspot.com/10905006/diff/24009/chrome/browser/ui/...
File chrome/browser/ui/webui/print_preview/print_preview_handler.cc (left):

https://chromiumcodereview.appspot.com/10905006/diff/24009/chrome/browser/ui/...
chrome/browser/ui/webui/print_preview/print_preview_handler.cc:767:
scoped_ptr<DictionaryValue> settings(settings_info.DeepCopy());
Does removing this change our behavior w.r.t. the color setting users see when
the second time they print?

https://chromiumcodereview.appspot.com/10905006/diff/24009/printing/backend/c...
File printing/backend/cups_helper.cc (right):

https://chromiumcodereview.appspot.com/10905006/diff/24009/printing/backend/c...
printing/backend/cups_helper.cc:32: void parse_lpoptions(const FilePath&
filepath, const std::string& printer_name,
I realized this got copied over, but this should be ParseLpOptions(). Ditto with
mark_lpoptions(). I forgot why we named them this way.

https://chromiumcodereview.appspot.com/10905006/diff/24009/printing/backend/c...
printing/backend/cups_helper.cc:40: size_t kDestLen = sizeof(kDest) - 1;
const here and next line

https://chromiumcodereview.appspot.com/10905006/diff/24009/printing/backend/c...
printing/backend/cups_helper.cc:42: std::vector <std::string> lines;
no space after vector

https://chromiumcodereview.appspot.com/10905006/diff/24009/printing/backend/c...
printing/backend/cups_helper.cc:112: bool getBasicColorModelSettings(
Ditto, naming and also parameter list formatting. And the functions below as
well.

https://chromiumcodereview.appspot.com/10905006/diff/24009/printing/backend/c...
printing/backend/cups_helper.cc:280: if (!((is_color_device &&
Might be slightly better to write this as:

return (big blob of conditions);

https://chromiumcodereview.appspot.com/10905006/diff/24009/printing/backend/c...
printing/backend/cups_helper.cc:290: }  // namespace
add a blank line above this.

https://chromiumcodereview.appspot.com/10905006/diff/24009/printing/backend/c...
printing/backend/cups_helper.cc:386: }  // namespace printing
add a blank line above this.

https://chromiumcodereview.appspot.com/10905006/diff/24009/printing/backend/p...
File printing/backend/print_backend.cc (right):

https://chromiumcodereview.appspot.com/10905006/diff/24009/printing/backend/p...
printing/backend/print_backend.cc:9: static const char kSemanticCapabilities[] =
"capabilities";
These aren't used anywhere?

https://chromiumcodereview.appspot.com/10905006/diff/24009/printing/backend/p...
File printing/backend/print_backend.h (right):

https://chromiumcodereview.appspot.com/10905006/diff/24009/printing/backend/p...
printing/backend/print_backend.h:36: struct PRINTING_EXPORT
PrinterSemanticCapsAndDefaults {
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Structs_vs._Cl...

recommends making this a class due to GetAsJson(). Sorry to be a party pooper.
Maybe export GetAsJson() as a separate method instead if you want to keep this
as a struct?

https://chromiumcodereview.appspot.com/10905006/diff/24009/printing/backend/p...
printing/backend/print_backend.h:40: std::string GetAsJson() const;
In base/, they refer to Json as JSON,

https://chromiumcodereview.appspot.com/10905006/diff/24009/printing/backend/p...
File printing/backend/print_backend_cups.cc (right):

https://chromiumcodereview.appspot.com/10905006/diff/24009/printing/backend/p...
printing/backend/print_backend_cups.cc:23: #include "base/string_split.h"
not sure why these got added here.

https://chromiumcodereview.appspot.com/10905006/diff/24009/printing/backend/p...
File printing/backend/print_backend_win.cc (right):

https://chromiumcodereview.appspot.com/10905006/diff/24009/printing/backend/p...
printing/backend/print_backend_win.cc:157: // Use printer default settings
(PRINTER_INFO_9) in this case.
Did you mean PRINTER_INFO_8?

[gene, 2012-09-05 21:04:10]

Please take a look. Adding more tests in the meantime.

https://chromiumcodereview.appspot.com/10905006/diff/24009/chrome/browser/pri...
File chrome/browser/printing/print_system_task_proxy.cc (right):

https://chromiumcodereview.appspot.com/10905006/diff/24009/chrome/browser/pri...
chrome/browser/printing/print_system_task_proxy.cc:10: #include <vector>
Done. histogram.h is needed below.

https://chromiumcodereview.appspot.com/10905006/diff/24009/chrome/browser/pri...
chrome/browser/printing/print_system_task_proxy.cc:148: // Old API, ugly code :(
Yes. Plan is no make a cleaner JSON format between Chrome and JS Print Preview.
Just did not want to pollute this CL.
Let me add TODO here.

https://chromiumcodereview.appspot.com/10905006/diff/24009/chrome/browser/ui/...
File chrome/browser/ui/webui/print_preview/print_preview_handler.cc (left):

https://chromiumcodereview.appspot.com/10905006/diff/24009/chrome/browser/ui/...
chrome/browser/ui/webui/print_preview/print_preview_handler.cc:767:
scoped_ptr<DictionaryValue> settings(settings_info.DeepCopy());
Yes it does :(. Not removing it, will default to a last user color instead of
default color setting for the printer (even for the new printer). This should be
addressed by a new settings code from Print Preview, that will store settings
for default printer only.

https://chromiumcodereview.appspot.com/10905006/diff/24009/printing/backend/c...
File printing/backend/cups_helper.cc (right):

https://chromiumcodereview.appspot.com/10905006/diff/24009/printing/backend/c...
printing/backend/cups_helper.cc:32: void parse_lpoptions(const FilePath&
filepath, const std::string& printer_name,
On 2012/09/05 00:32:32, Lei Zhang wrote:
> I realized this got copied over, but this should be ParseLpOptions(). Ditto
with
> mark_lpoptions(). I forgot why we named them this way.

Done.

https://chromiumcodereview.appspot.com/10905006/diff/24009/printing/backend/c...
printing/backend/cups_helper.cc:40: size_t kDestLen = sizeof(kDest) - 1;
On 2012/09/05 00:32:32, Lei Zhang wrote:
> const here and next line

Done.

https://chromiumcodereview.appspot.com/10905006/diff/24009/printing/backend/c...
printing/backend/cups_helper.cc:42: std::vector <std::string> lines;
On 2012/09/05 00:32:32, Lei Zhang wrote:
> no space after vector

Done.

https://chromiumcodereview.appspot.com/10905006/diff/24009/printing/backend/c...
printing/backend/cups_helper.cc:112: bool getBasicColorModelSettings(
On 2012/09/05 00:32:32, Lei Zhang wrote:
> Ditto, naming and also parameter list formatting. And the functions below as
> well.

Done.

https://chromiumcodereview.appspot.com/10905006/diff/24009/printing/backend/c...
printing/backend/cups_helper.cc:280: if (!((is_color_device &&
On 2012/09/05 00:32:32, Lei Zhang wrote:
> Might be slightly better to write this as:
> 
> return (big blob of conditions);

Done.

https://chromiumcodereview.appspot.com/10905006/diff/24009/printing/backend/c...
printing/backend/cups_helper.cc:290: }  // namespace
On 2012/09/05 00:32:32, Lei Zhang wrote:
> add a blank line above this.

Done.

https://chromiumcodereview.appspot.com/10905006/diff/24009/printing/backend/c...
printing/backend/cups_helper.cc:386: }  // namespace printing
On 2012/09/05 00:32:32, Lei Zhang wrote:
> add a blank line above this.

Done.

https://chromiumcodereview.appspot.com/10905006/diff/24009/printing/backend/p...
File printing/backend/print_backend.cc (right):

https://chromiumcodereview.appspot.com/10905006/diff/24009/printing/backend/p...
printing/backend/print_backend.cc:9: static const char kSemanticCapabilities[] =
"capabilities";
Not used. Removing.
I was in the process of switching to a new json interface, but decided to do it
in a separate CL. Also, removing GetAsJson() for now.

https://chromiumcodereview.appspot.com/10905006/diff/24009/printing/backend/p...
File printing/backend/print_backend.h (right):

https://chromiumcodereview.appspot.com/10905006/diff/24009/printing/backend/p...
printing/backend/print_backend.h:36: struct PRINTING_EXPORT
PrinterSemanticCapsAndDefaults {
Removing GetAsJson() for now. I'll consider switching to class or have an
external converter once we have new json interface.

https://chromiumcodereview.appspot.com/10905006/diff/24009/printing/backend/p...
printing/backend/print_backend.h:40: std::string GetAsJson() const;
Thanks for pointer. Removing this function for now.

https://chromiumcodereview.appspot.com/10905006/diff/24009/printing/backend/p...
File printing/backend/print_backend_cups.cc (right):

https://chromiumcodereview.appspot.com/10905006/diff/24009/printing/backend/p...
printing/backend/print_backend_cups.cc:23: #include "base/string_split.h"
Good catch, thanks!
I initially moved ppd parsing code here, but then decided moving it to
cups_helper.cc

https://chromiumcodereview.appspot.com/10905006/diff/24009/printing/backend/p...
File printing/backend/print_backend_win.cc (right):

https://chromiumcodereview.appspot.com/10905006/diff/24009/printing/backend/p...
printing/backend/print_backend_win.cc:157: // Use printer default settings
(PRINTER_INFO_9) in this case.
Of course. Thanks

[Lei Zhang, 2012-09-05 21:10:15]

https://chromiumcodereview.appspot.com/10905006/diff/29001/printing/backend/c...
File printing/backend/cups_helper.cc (right):

https://chromiumcodereview.appspot.com/10905006/diff/29001/printing/backend/c...
printing/backend/cups_helper.cc:287: return (!((is_color_device &&
I think you need to get rid of the !

if (!(eval_to_false)) {
  return false;
}
return true;

--> return (eval_to_false);

[gene, 2012-09-05 21:15:10]

https://chromiumcodereview.appspot.com/10905006/diff/29001/printing/backend/c...
File printing/backend/cups_helper.cc (right):

https://chromiumcodereview.appspot.com/10905006/diff/29001/printing/backend/c...
printing/backend/cups_helper.cc:287: return (!((is_color_device &&
You are absolutely correct. Fixed.
Thanks for catching this.

[Lei Zhang, 2012-09-05 21:16:06]

lgtm

[gene, 2012-09-06 17:21:13]

Added more unittests. Found a bug. Unittests are great!
Albert, can you take a look one more time?

[Albert Bodenhamer, 2012-09-06 17:22:58]

lgtm

[commit-bot: I haz the power, 2012-09-06 17:24:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gene@chromium.org/10905006/20038

[commit-bot: I haz the power, 2012-09-06 19:13:20]

Try job failure for 10905006-20038 (retry) on mac_rel for step
"sync_integration_tests".
It's a second try, previously, step "sync_integration_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-09-06 19:20:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gene@chromium.org/10905006/20038

[commit-bot: I haz the power, 2012-09-06 22:11:31]

Try job failure for 10905006-20038 (retry) on mac_rel for step
"sync_integration_tests".
It's a second try, previously, step "sync_integration_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-09-06 22:15:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gene@chromium.org/10905006/20038

[commit-bot: I haz the power, 2012-09-07 04:21:22]

Change committed as 155310

[Dan Beam, 2012-09-07 09:11:05]

vivekg (from #chromium) and I both noticed this seems to break the compile on
Linux with the follow error message - http://pastebin.com/raw.php?i=kgAV7bR9,
btw

[gene1, 2012-09-07 17:52:52]

Damn, sorry about that :( (it failing on precise, correct?)
Fix is on it's way  http://codereview.chromium.org/10907108/

On Fri, Sep 7, 2012 at 2:11 AM, <dbeam@chromium.org> wrote:

> vivekg (from #chromium) and I both noticed this seems to break the compile
> on
> Linux with the follow error message - http://pastebin.com/raw.php?i=**
> kgAV7bR9 <http://pastebin.com/raw.php?i=kgAV7bR9>,
> btw
>
>
https://chromiumcodereview.**appspot.com/10905006/<https://chromiumcodereview...
>