Hookup the PpdProvider in the printer setup flow.

Hookup the PpdProvider in the printer setup flow.

Cache manually added PPDs and lookup PPDs with the provider in the
add printer dialog.

BUG=670037
Committed: https://crrev.com/9b008b2277810ec4638488b3bfe2fd2ef5b85eb7
Cr-Commit-Position: refs/heads/master@{#437078}

[skau, 2016-12-01 01:23:17]

skau@chromium.org changed reviewers:
+ justincarlson@chromium.org

[skau, 2016-12-01 01:23:17]

justincarlson@: Can you take a quick look?  It ended up being more code than I
was expecting.  Most of the logging should be removed still.

[Carlson, 2016-12-01 01:58:44]

https://codereview.chromium.org/2545663002/diff/1/chrome/browser/ui/webui/set...
File chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc (right):

https://codereview.chromium.org/2545663002/diff/1/chrome/browser/ui/webui/set...
chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc:227: const
Printer::PpdReference& ppd_reference = printer->ppd_reference();
Add a comment about why you're doing this?  Someone, someday is going to say
"Hey this is sort of a pointless temporary", remove it, and write
printer->ppd_reference() in the Resolve() below and there will be much
puzzlement and gnashing of teeth.

(At first glance *I* thought this was a pretty pointless temporary.  And I was
just bitten by this same sort of bug 2 weeks ago...)

https://codereview.chromium.org/2545663002/diff/1/chrome/browser/ui/webui/set...
chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc:401:
std::string ppd_path = ipp_everywhere ? "" : path.value();
Wait, what?  So if it's an ipp everywhere printer we go out and try to resolve
it and then if we do resolve it successfully, we ignore the result?  I don't
understand what you're doing here.

https://codereview.chromium.org/2545663002/diff/1/chromeos/printing/ppd_cache.cc
File chromeos/printing/ppd_cache.cc (right):

https://codereview.chromium.org/2545663002/diff/1/chromeos/printing/ppd_cache...
chromeos/printing/ppd_cache.cc:71: VLOG(1) << "Storing ppd";
Reminder to remove this logging.

https://codereview.chromium.org/2545663002/diff/1/chromeos/printing/printer_c...
File chromeos/printing/printer_configuration.h (right):

https://codereview.chromium.org/2545663002/diff/1/chromeos/printing/printer_c...
chromeos/printing/printer_configuration.h:48: static bool IsIppEverywhere(const
Printer& printer);
Seems sort of weird to have this as a static function instead of

bool IsIppEverywhere() const;

Is there a reason you're doing it this way?

[skau, 2016-12-01 21:45:12]

skau@chromium.org changed reviewers:
+ dpapad@chromium.org, xdai@chromium.org

[skau, 2016-12-01 21:45:13]

Thanks for the review.  I'll loop in relevant owners now.

xdai@ chromeos/printing/*, chrome/browser/ui/webui/settings/*
dpadpad@ chrome/browser/ui/webui/settings/*

https://codereview.chromium.org/2545663002/diff/1/chrome/browser/ui/webui/set...
File chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc (right):

https://codereview.chromium.org/2545663002/diff/1/chrome/browser/ui/webui/set...
chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc:227: const
Printer::PpdReference& ppd_reference = printer->ppd_reference();
On 2016/12/01 01:58:44, Carlson wrote:
> Add a comment about why you're doing this?  Someone, someday is going to say
> "Hey this is sort of a pointless temporary", remove it, and write
> printer->ppd_reference() in the Resolve() below and there will be much
> puzzlement and gnashing of teeth.
> 
> (At first glance *I* thought this was a pretty pointless temporary.  And I was
> just bitten by this same sort of bug 2 weeks ago...)

I really wish this was a compiler warning.  I've added a comment but the wording
isn't great.

https://codereview.chromium.org/2545663002/diff/1/chrome/browser/ui/webui/set...
chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc:401:
std::string ppd_path = ipp_everywhere ? "" : path.value();
On 2016/12/01 01:58:44, Carlson wrote:
> Wait, what?  So if it's an ipp everywhere printer we go out and try to resolve
> it and then if we do resolve it successfully, we ignore the result?  I don't
> understand what you're doing here.

What I'm doing here makes no sense.  I've moved the IPP Everywhere check to
before the resolve call since IPP Everwhere should be keyed by
effective_manufacturer and effective_model and doesn't require a Resolve call.

https://codereview.chromium.org/2545663002/diff/1/chromeos/printing/ppd_cache.cc
File chromeos/printing/ppd_cache.cc (right):

https://codereview.chromium.org/2545663002/diff/1/chromeos/printing/ppd_cache...
chromeos/printing/ppd_cache.cc:71: VLOG(1) << "Storing ppd";
On 2016/12/01 01:58:44, Carlson wrote:
> Reminder to remove this logging.

Done.

https://codereview.chromium.org/2545663002/diff/1/chromeos/printing/printer_c...
File chromeos/printing/printer_configuration.h (right):

https://codereview.chromium.org/2545663002/diff/1/chromeos/printing/printer_c...
chromeos/printing/printer_configuration.h:48: static bool IsIppEverywhere(const
Printer& printer);
On 2016/12/01 01:58:44, Carlson wrote:
> Seems sort of weird to have this as a static function instead of
> 
> bool IsIppEverywhere() const;
> 
> Is there a reason you're doing it this way?

My thought was that it might be mistaken to be a field on the object if I made
it a member function.  But if it seems odd, I'll make it a member.

[skau, 2016-12-01 21:46:38]

dpapad@ Can you also review .../print_preview/* as you're an owner of printing/

[Carlson, 2016-12-01 22:59:52]

https://codereview.chromium.org/2545663002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.h (right):

https://codereview.chromium.org/2545663002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.h:92: //
automatic configuration will be attempted.  |ppd_path| should be a path
Comment should indicate that ppd_path is ignored (or should be empty) if
ipp_everywhere is true.

[xdai1, 2016-12-01 23:18:18]

lgtm

https://codereview.chromium.org/2545663002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc (right):

https://codereview.chromium.org/2545663002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc:238:
weak_factory_.GetWeakPtr(), base::Passed(&printer)));
Is base::Passed(&printer)) preferred over base::Passed(std::move(printer))?

[skau, 2016-12-05 20:40:51]

Comments addressed.  PTAL.

https://codereview.chromium.org/2545663002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc (right):

https://codereview.chromium.org/2545663002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc:238:
weak_factory_.GetWeakPtr(), base::Passed(&printer)));
On 2016/12/01 23:18:17, xdai1 wrote:
> Is base::Passed(&printer)) preferred over base::Passed(std::move(printer))?

They are equivalent.  The post in chromium-dev that introduced unique_ptr
suggested & since it's shorter.

https://codereview.chromium.org/2545663002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.h (right):

https://codereview.chromium.org/2545663002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.h:92: //
automatic configuration will be attempted.  |ppd_path| should be a path
On 2016/12/01 22:59:52, Carlson wrote:
> Comment should indicate that ppd_path is ignored (or should be empty) if
> ipp_everywhere is true.

Done.

[dpapad, 2016-12-05 23:09:13]

https://codereview.chromium.org/2545663002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc (right):

https://codereview.chromium.org/2545663002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc:220: VLOG(1)
<< "PPD Cached: " << cached;
Is this VLOG useful enough to be checked in?

[Carlson, 2016-12-05 23:40:11]

lgtm

https://codereview.chromium.org/2545663002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.h (right):

https://codereview.chromium.org/2545663002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.h:95: const
std::string& ppd_path,
Shouldn't this be a base::FilePath?

https://codereview.chromium.org/2545663002/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.h (right):

https://codereview.chromium.org/2545663002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.h:95: //
Downloads or teh PPD Cache.
the

[skau, 2016-12-06 20:55:46]

Thanks for the review.  PTAL.

https://codereview.chromium.org/2545663002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc (right):

https://codereview.chromium.org/2545663002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc:220: VLOG(1)
<< "PPD Cached: " << cached;
On 2016/12/05 23:09:12, dpapad wrote:
> Is this VLOG useful enough to be checked in?

This actually needs to be handled as an error.  I've changed it accordingly.

https://codereview.chromium.org/2545663002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.h (right):

https://codereview.chromium.org/2545663002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.h:95: const
std::string& ppd_path,
On 2016/12/05 23:40:11, Carlson wrote:
> Shouldn't this be a base::FilePath?

It goes onto the DBus as a string so it doesn't really matter.  But FilePath is
safer.  I've changed it.

https://codereview.chromium.org/2545663002/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.h (right):

https://codereview.chromium.org/2545663002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.h:95: //
Downloads or teh PPD Cache.
On 2016/12/05 23:40:11, Carlson wrote:
> the

Done.

[dpapad, 2016-12-07 01:19:40]

lgtm

[skau, 2016-12-07 21:19:10]

The CQ bit was checked by skau@chromium.org

[skau, 2016-12-07 21:19:11]

The patchset sent to the CQ was uploaded after l-g-t-m from xdai@chromium.org,
justincarlson@chromium.org
Link to the patchset: https://codereview.chromium.org/2545663002/#ps100001
(title: "address dpadpad@ comment")

[commit-bot: I haz the power, 2016-12-07 21:19:53]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-07 22:09:28]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1481145550461140,
"parent_rev": "7df6ca470f5ef6d95c3c7e2343d81121e3e6045e", "commit_rev":
"91521ef94aea6470ae762da97c1aaaaa0b006b49"}

[commit-bot: I haz the power, 2016-12-07 22:09:55]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-12-07 22:13:17]

Description was changed from

==========
Hookup the PpdProvider in the printer setup flow.

Cache manually added PPDs and lookup PPDs with the provider in the
add printer dialog.

BUG=670037
==========

to

==========
Hookup the PpdProvider in the printer setup flow.

Cache manually added PPDs and lookup PPDs with the provider in the
add printer dialog.

BUG=670037

Committed: https://crrev.com/9b008b2277810ec4638488b3bfe2fd2ef5b85eb7
Cr-Commit-Position: refs/heads/master@{#437078}
==========

[commit-bot: I haz the power, 2016-12-07 22:13:18]

Patchset 6 (id:??) landed as
https://crrev.com/9b008b2277810ec4638488b3bfe2fd2ef5b85eb7
Cr-Commit-Position: refs/heads/master@{#437078}