Interrogate PpdProvider from PrintPreview.

Interrogate PpdProvider from PrintPreview.

Make use of the PpdCache from PrintPreview.  Setup downloading ppds from
the quirks server.

BUG=662755
TEST=Setup a printer.  Log out.  Rename ppd file.  Open printer from print
preview.

Committed: https://crrev.com/d96dd555f99419bee86970516993aa9631421d58
Cr-Commit-Position: refs/heads/master@{#437718}

[skau, 2016-12-02 01:12:38]

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

[skau, 2016-12-02 01:12:39]

justincarlson@ everything
xdai@ print_preview_handler.cc for general advice with WebUIMessageHandlers

[Carlson, 2016-12-02 19:10:22]

Generally looks sane to me.

https://codereview.chromium.org/2542363002/diff/1/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview/print_preview_handler.cc (right):

https://codereview.chromium.org/2542363002/diff/1/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview/print_preview_handler.cc:615:
printing::PrinterBackendProxy* PrintPreviewHandler::printer_backend_proxy() {
This should run on the UI thread, right?  Add a DCHECK?

https://codereview.chromium.org/2542363002/diff/1/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview/print_preview_handler.cc:619:
printing::PrinterBackendProxy::Create(Profile::FromWebUI(web_ui()));
The reason for the multiplexing of proxies here deserves a comment or a doc
link.

https://codereview.chromium.org/2542363002/diff/1/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview/print_preview_handler.cc:1170: base::Bind(
Is this safe?  The second bind is weak ptr based, so should do something sane in
the cancellation case, but the first Bind() takes a pointer to an internal class
member and is *not* weak ptr based.  I don't really know the guts of the
weak-ptr-cancellation mechanism very well, but this seems fishy.

https://codereview.chromium.org/2542363002/diff/1/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview/printer_backend_proxy.cc (right):

https://codereview.chromium.org/2542363002/diff/1/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview/printer_backend_proxy.cc:55: class
PrinterBackendProxyImpl : public PrinterBackendProxy {
Maybe a better suffix here would be Chrome instead of Impl?  More orthogonal to
the ChromeOS version.  Also a short comment about what this class is would be
good.

https://codereview.chromium.org/2542363002/diff/1/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview/printer_backend_proxy_chromeos.cc
(right):

https://codereview.chromium.org/2542363002/diff/1/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview/printer_backend_proxy_chromeos.cc:50:
PrinterList* list,
Aren't output parameters usually last?

https://codereview.chromium.org/2542363002/diff/1/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview/printer_backend_proxy_chromeos.cc:201:
base::Bind(&PPDResolve, base::Passed(&printer), cb));
Do we need WeakPtr semantics here?

[skau, 2016-12-05 23:13:35]

Thanks for the review.  Fixed most of the issues.  It looks like the backend
proxy might need to be ref-counted so it'll be a bit before I can update this
again.

https://codereview.chromium.org/2542363002/diff/1/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview/print_preview_handler.cc (right):

https://codereview.chromium.org/2542363002/diff/1/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview/print_preview_handler.cc:615:
printing::PrinterBackendProxy* PrintPreviewHandler::printer_backend_proxy() {
On 2016/12/02 19:10:22, Carlson wrote:
> This should run on the UI thread, right?  Add a DCHECK?

No.  The proxy is actually designed to run on the UI thread and handle making
calls into the blocking pool.

https://codereview.chromium.org/2542363002/diff/1/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview/print_preview_handler.cc:619:
printing::PrinterBackendProxy::Create(Profile::FromWebUI(web_ui()));
On 2016/12/02 19:10:22, Carlson wrote:
> The reason for the multiplexing of proxies here deserves a comment or a doc
> link.

Done.

https://codereview.chromium.org/2542363002/diff/1/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview/print_preview_handler.cc:1170: base::Bind(
On 2016/12/02 19:10:22, Carlson wrote:
> Is this safe?  The second bind is weak ptr based, so should do something sane
in
> the cancellation case, but the first Bind() takes a pointer to an internal
class
> member and is *not* weak ptr based.  I don't really know the guts of the
> weak-ptr-cancellation mechanism very well, but this seems fishy.

I need to think about this a little.  WeakPtrs aren't thread safe. I have to
double check what happens if PrintPreviewHandler is collected while
GetDefaultPrinter... is running.  Thanks for the catch.

https://codereview.chromium.org/2542363002/diff/1/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview/printer_backend_proxy.cc (right):

https://codereview.chromium.org/2542363002/diff/1/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview/printer_backend_proxy.cc:55: class
PrinterBackendProxyImpl : public PrinterBackendProxy {
On 2016/12/02 19:10:22, Carlson wrote:
> Maybe a better suffix here would be Chrome instead of Impl?  More orthogonal
to
> the ChromeOS version.  Also a short comment about what this class is would be
> good.

Renamed to 'Default'.  Chrome seems odd since ChromeOS isn't orthogonal to
Chrome.  ChromeOS is just special.

https://codereview.chromium.org/2542363002/diff/1/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview/printer_backend_proxy_chromeos.cc
(right):

https://codereview.chromium.org/2542363002/diff/1/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview/printer_backend_proxy_chromeos.cc:50:
PrinterList* list,
On 2016/12/02 19:10:22, Carlson wrote:
> Aren't output parameters usually last?

yes.  It's apparently in the style guide.
https://google.github.io/styleguide/cppguide.html#Function_Parameter_Ordering

https://codereview.chromium.org/2542363002/diff/1/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview/printer_backend_proxy_chromeos.cc:201:
base::Bind(&PPDResolve, base::Passed(&printer), cb));
On 2016/12/02 19:10:22, Carlson wrote:
> Do we need WeakPtr semantics here?

No.  We're transferring ownership of the printer.

[Carlson, 2016-12-05 23:45:58]

https://codereview.chromium.org/2542363002/diff/1/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview/print_preview_handler.cc (right):

https://codereview.chromium.org/2542363002/diff/1/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview/print_preview_handler.cc:615:
printing::PrinterBackendProxy* PrintPreviewHandler::printer_backend_proxy() {
On 2016/12/05 23:13:34, skau wrote:
> On 2016/12/02 19:10:22, Carlson wrote:
> > This should run on the UI thread, right?  Add a DCHECK?
> 
> No.  The proxy is actually designed to run on the UI thread and handle making
> calls into the blocking pool.

I don't understand your response here.  Is this function run on the UI thread or
not?

I sure hope it *is* on the UI thread because we are definitely making a
sequenced assumption with the handling of printer_backend_proxy_ initialization.

https://codereview.chromium.org/2542363002/diff/1/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview/printer_backend_proxy_chromeos.cc
(right):

https://codereview.chromium.org/2542363002/diff/1/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview/printer_backend_proxy_chromeos.cc:201:
base::Bind(&PPDResolve, base::Passed(&printer), cb));
On 2016/12/05 23:13:34, skau wrote:
> On 2016/12/02 19:10:22, Carlson wrote:
> > Do we need WeakPtr semantics here?
> 
> No.  We're transferring ownership of the printer.

I'm not sure I understand.  What guarantees do we have about the lifetime of cb?
 I don't see an obvious mechanism by which we will do the right thing here.

[skau, 2016-12-06 23:20:07]

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

[skau, 2016-12-06 23:20:08]

vitalybuka@: Please review as owner of ui/webui/print_preview/*

Rest: PTAL comments have been addressed.

https://codereview.chromium.org/2542363002/diff/1/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview/print_preview_handler.cc (right):

https://codereview.chromium.org/2542363002/diff/1/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview/print_preview_handler.cc:615:
printing::PrinterBackendProxy* PrintPreviewHandler::printer_backend_proxy() {
On 2016/12/05 23:45:58, Carlson wrote:
> On 2016/12/05 23:13:34, skau wrote:
> > On 2016/12/02 19:10:22, Carlson wrote:
> > > This should run on the UI thread, right?  Add a DCHECK?
> > 
> > No.  The proxy is actually designed to run on the UI thread and handle
making
> > calls into the blocking pool.
> 
> I don't understand your response here.  Is this function run on the UI thread
or
> not?
> 
> I sure hope it *is* on the UI thread because we are definitely making a
> sequenced assumption with the handling of printer_backend_proxy_
initialization.
> 

I misread your comment.  I've added the DCHECK.  It *is* on the UI thread.  I've
also fixed the proxy so the public methods are always called on the UI thread.

https://codereview.chromium.org/2542363002/diff/1/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview/print_preview_handler.cc:1170: base::Bind(
On 2016/12/05 23:13:34, skau wrote:
> On 2016/12/02 19:10:22, Carlson wrote:
> > Is this safe?  The second bind is weak ptr based, so should do something
sane
> in
> > the cancellation case, but the first Bind() takes a pointer to an internal
> class
> > member and is *not* weak ptr based.  I don't really know the guts of the
> > weak-ptr-cancellation mechanism very well, but this seems fishy.
> 
> I need to think about this a little.  WeakPtrs aren't thread safe. I have to
> double check what happens if PrintPreviewHandler is collected while
> GetDefaultPrinter... is running.  Thanks for the catch.

This has been fixed so that we dispatch the call from the UI thread and I've
checked that all callbacks are bound with WeakPtrs so they'll cancel safely if
PrintPreviewHandler gets collected.

https://codereview.chromium.org/2542363002/diff/1/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview/printer_backend_proxy_chromeos.cc
(right):

https://codereview.chromium.org/2542363002/diff/1/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview/printer_backend_proxy_chromeos.cc:201:
base::Bind(&PPDResolve, base::Passed(&printer), cb));
On 2016/12/05 23:45:58, Carlson wrote:
> On 2016/12/05 23:13:34, skau wrote:
> > On 2016/12/02 19:10:22, Carlson wrote:
> > > Do we need WeakPtr semantics here?
> > 
> > No.  We're transferring ownership of the printer.
> 
> I'm not sure I understand.  What guarantees do we have about the lifetime of
cb?
>  I don't see an obvious mechanism by which we will do the right thing here.

Spoke offline.  Resolve is invoked on the UI thread and by specification, it
will run the callback on the originating thread (UI thread). 
PrinterBackendProxy is always destructed on the UI thread so this should be safe
(DCHECK added to destructor to assuage concerns).

[Carlson, 2016-12-06 23:31:57]

lgtm

https://codereview.chromium.org/2542363002/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/print_preview/printer_backend_proxy_chromeos.cc
(right):

https://codereview.chromium.org/2542363002/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/print_preview/printer_backend_proxy_chromeos.cc:163:
cb.Run("");
While this conforms to my own sensibilities, I think it's probably more
idiomatic in this code base to post the callback as a task so the callback is
sequenced with respect to the invoking function.

[skau, 2016-12-07 18:19:48]

Comments addressed.  PTAL.

https://codereview.chromium.org/2542363002/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/print_preview/printer_backend_proxy_chromeos.cc
(right):

https://codereview.chromium.org/2542363002/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/print_preview/printer_backend_proxy_chromeos.cc:163:
cb.Run("");
On 2016/12/06 23:31:57, Carlson wrote:
> While this conforms to my own sensibilities, I think it's probably more
> idiomatic in this code base to post the callback as a task so the callback is
> sequenced with respect to the invoking function.

Done.

[skau, 2016-12-07 18:20:00]

The CQ bit was checked by skau@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-07 18:20:27]

Dry run: 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 19:41:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-07 19:41:02]

Dry run: This issue passed the CQ dry run.

[skau, 2016-12-08 19:39:44]

skau@chromium.org changed reviewers:
+ dpapad@chromium.org
- vitalybuka@chromium.org

[skau, 2016-12-08 19:39:45]

dpapad@: Could you review?  This needs an owner of ui/webui.

[dpapad, 2016-12-09 22:01:17]

LGTM

https://codereview.chromium.org/2542363002/diff/100001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/print_preview/printer_backend_proxy_chromeos.cc
(right):

https://codereview.chromium.org/2542363002/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/print_preview/printer_backend_proxy_chromeos.cc:52: for
(const std::unique_ptr<chromeos::Printer>& printer : printers) {
Would |auto| work here?

[skau, 2016-12-09 22:48:51]

The CQ bit was checked by skau@chromium.org

[skau, 2016-12-09 22:48:51]

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

[skau, 2016-12-09 22:49:06]

Thanks for the reviews.

https://codereview.chromium.org/2542363002/diff/100001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/print_preview/printer_backend_proxy_chromeos.cc
(right):

https://codereview.chromium.org/2542363002/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/print_preview/printer_backend_proxy_chromeos.cc:52: for
(const std::unique_ptr<chromeos::Printer>& printer : printers) {
On 2016/12/09 22:01:17, dpapad wrote:
> Would |auto| work here?

Yes it would.

[commit-bot: I haz the power, 2016-12-09 22:49: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-10 01:58:31]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1481323731253680,
"parent_rev": "79eddec1e826c79cfe976238452f2789ce3b2d89", "commit_rev":
"e8f9ba1bc4ad3088877a87443ce19b103268ec17"}

[commit-bot: I haz the power, 2016-12-10 01:58:56]

Description was changed from

==========
Interrogate PpdProvider from PrintPreview.

Make use of the PpdCache from PrintPreview.  Setup downloading ppds from
the quirks server.

BUG=662755
TEST=Setup a printer.  Log out.  Rename ppd file.  Open printer from print
preview.
==========

to

==========
Interrogate PpdProvider from PrintPreview.

Make use of the PpdCache from PrintPreview.  Setup downloading ppds from
the quirks server.

BUG=662755
TEST=Setup a printer.  Log out.  Rename ppd file.  Open printer from print
preview.

Review-Url: https://codereview.chromium.org/2542363002
==========

[commit-bot: I haz the power, 2016-12-10 01:58:58]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-12-12 14:59:29]

Description was changed from

==========
Interrogate PpdProvider from PrintPreview.

Make use of the PpdCache from PrintPreview.  Setup downloading ppds from
the quirks server.

BUG=662755
TEST=Setup a printer.  Log out.  Rename ppd file.  Open printer from print
preview.

Review-Url: https://codereview.chromium.org/2542363002
==========

to

==========
Interrogate PpdProvider from PrintPreview.

Make use of the PpdCache from PrintPreview.  Setup downloading ppds from
the quirks server.

BUG=662755
TEST=Setup a printer.  Log out.  Rename ppd file.  Open printer from print
preview.

Committed: https://crrev.com/d96dd555f99419bee86970516993aa9631421d58
Cr-Commit-Position: refs/heads/master@{#437718}
==========

[commit-bot: I haz the power, 2016-12-12 14:59:31]

Patchset 8 (id:??) landed as
https://crrev.com/d96dd555f99419bee86970516993aa9631421d58
Cr-Commit-Position: refs/heads/master@{#437718}