Create a new print backend for the updated CUPS APIs

printing/backend/cups_connection.h

+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef PRINTING_BACKEND_CUPS_CONNECTION_H_
+#define PRINTING_BACKEND_CUPS_CONNECTION_H_
+
+#include <cups/cups.h>
+
+#include <memory>
+#include <string>
+#include <vector>
+
+#include "base/macros.h"
+#include "base/memory/weak_ptr.h"
+#include "printing/backend/cups_printer.h"
+#include "printing/printing_export.h"
+#include "url/gurl.h"
+
+namespace printing {
+
+class HttpDeleter {
+ public:
+  void operator()(http_t* http) const;
+};
+
+// Represents a connection to a CUPS server.
+class PRINTING_EXPORT CupsConnection {
+ public:
+  explicit CupsConnection(const GURL& print_server_url,

[Lei Zhang, 2016/07/13 01:08:42]

Not explicit

[skau, 2016/07/14 20:43:05]

Done.

+                          http_encryption_t encryption,
+                          bool blocking);
+
+  CupsConnection(CupsConnection&& connection);
+
+  ~CupsConnection();
+
+  // Returns the number of destinations and populates |destinations|.

[Lei Zhang, 2016/07/13 01:08:42]

Comment doesn't match code.

[skau, 2016/07/14 20:43:05]

Done.

+  std::vector<CupsPrinter> GetDests();
+
+  // Returns a printer for |printer_name| from the connected server.
+  CupsPrinter* GetPrinter(const std::string& printer_name);

[Lei Zhang, 2016/07/13 01:08:42]

Caller owns the returned pointer, right? Make it return an unique_ptr.

[skau, 2016/07/14 20:43:05]

Yes it does.

Done.

+
+  std::string server_name() const;
+
+  int last_error() const;
+
+ private:
+  // lazily initialize http connection
+  bool Connect();
+
+  GURL print_server_url_;
+  http_encryption_t cups_encryption_;
+  bool blocking_;
+
+  class Impl {

[Lei Zhang, 2016/07/13 01:08:42]

Put this as the first thing in the private section, so all the member variables
are together.

[skau, 2016/07/14 20:43:05]

Done.

+   public:
+    explicit Impl(http_t* http);
+    ~Impl();
+
+    base::WeakPtr<http_t> GetHttp();

[Lei Zhang, 2016/07/13 01:08:42]

Can you explain the ownership model? Why do we need WeakPtrs?

[skau, 2016/07/14 20:43:05]

CupsConnection owns the http_t so I can make sure that it gets closed.  However,
CupsPrinters don't have the same lifetime as the CupsConnection so I wanted to
make sure that a CupsPrinter object would fail gracefully in that situation.  I
would make it shared except that the style guide recommends to avoid shared refs
if possible.

Thoughts?

[Lei Zhang, 2016/07/19 22:49:17]

Can you show we where CupsPrinters can outlive CupsConnection? I'm not seeing
it.
Look for "counted" in
https://www.chromium.org/developers/smart-pointer-guidelines and
https://www.chromium.org/developers/coding-style/important-abstractions-and-d...
to see why std::shared_ptr, base::RefCounted, and friends are discouraged.

[skau, 2016/07/20 23:58:22]

Per offline discussion, building printers with raw pointers and documented the
requirement.

+
+   private:
+    std::unique_ptr<http_t, HttpDeleter> cups_http_;
+    base::WeakPtrFactory<http_t> http_factory_;
+
+    DISALLOW_COPY_AND_ASSIGN(Impl);
+  };
+
+  std::unique_ptr<Impl> impl_;
+  base::WeakPtr<http_t> cups_http_;
+
+  DISALLOW_COPY_AND_ASSIGN(CupsConnection);
+};
+
+}  // namespace printing
+
+#endif  // PRINTING_BACKEND_CUPS_CONNECTION_H_