Add PPDProvider barebones implementation and associated cache skeleton.

chromeos/printing/ppd_cache.h

Index: chromeos/printing/ppd_cache.h
diff --git a/chromeos/printing/ppd_cache.h b/chromeos/printing/ppd_cache.h
new file mode 100644
index 0000000000000000000000000000000000000000..f3f051fad918492ebe53f7003f30f2519d7b984a
--- /dev/null
+++ b/chromeos/printing/ppd_cache.h
@@ -0,0 +1,62 @@
+// 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 CHROMEOS_PRINTING_PPD_CACHE_H_
+#define CHROMEOS_PRINTING_PPD_CACHE_H_
+
+#include <memory>
+#include <string>
+
+#include "base/files/file_path.h"
+#include "base/optional.h"
+#include "chromeos/chromeos_export.h"
+#include "chromeos/printing/printer_configuration.h"
+
+namespace chromeos {
+namespace printing {
+
+// PpdCache manages a cache of locally-stored ppd files.  At its core,

[Lei Zhang, 2016/10/17 17:54:06]

PPD

[Carlson, 2016/10/18 19:05:01]

Done.

+// it operates like a persistent hash from PpdReference to files.  If
+// you give the same PpdReference to Find that was previously passed

[Lei Zhang, 2016/10/17 17:54:06]

Find()

[Carlson, 2016/10/18 19:05:01]

Done.

+// to
+class CHROMEOS_EXPORT PpdCache {
+ public:
+  // Construction-time optional parameters.  These should all have defaults that
+  // are sane.
+  struct Options {

[Lei Zhang, 2016/10/17 17:54:06]

Can you add this once you actually have options?

[Carlson, 2016/10/18 19:05:01]

Done.

+    // Nothing here yet.  We may want to add ttl-style options here eventually

[Lei Zhang, 2016/10/17 17:54:06]

Some people do not like using "we" in comments because it is unclear who "we"
refers to. e.g. Here it may refer to the developers, but the "we" on line 38"
refers to the code.

What may be better is:

TODO(username): Consider adding options for ...

Otherwise, Find() will miss in the cache...

[Carlson, 2016/10/18 19:05:01]

Not sure if use of vague "some people" in comment about vague pronoun was meant
to be ironic or not.  :)

Anyways, removed options.

+    // so the cache doesn't grow without bound.
+  };
+
+  // Create and return a Ppdcache that uses cache_dir to store state.
+  static std::unique_ptr<PpdCache> Create(const base::FilePath& cache_base_dir,
+                                          const Options& options = Options());
+  virtual ~PpdCache() {}
+
+  // Find a PPD that was previously cached with the given reference.  Note that
+  // all fields of the reference must be the same, otherwise we'll miss in the
+  // cache and re-run resolution for the PPD.
+  //
+  // If a FilePath is returned, it is guaranteed to remain valid until the next
+  // Store() call.
+  virtual base::Optional<base::FilePath> Find(

[Lei Zhang, 2016/10/17 17:54:07]

Do you need the Optional? Can an empty FilePath represent a failure instead?
Ditto below.

[Carlson, 2016/10/18 19:05:01]

We could do that, but I'm generally against
special-return-values-meaning-failure as a software engineering practice.

My first choice here was StatusOr<FilePath>, but we don't seem to have that in
this code base, alas.

[Lei Zhang, 2016/10/18 19:37:00]

Where do you draw the line there? Does malloc() returning NULL count as
special-return-values-meaning-failure?

Another way to think about it is: when does it make sense for Find() to return
an empty FilePath? Wouldn't you want to check for that as an error condition?

We didn't get base::Optional until 5 months ago. Somehow we survived without it
before.

[Carlson, 2016/10/18 20:06:28]

If you want to litigate Optional as a useful tool, this is probably not the
right forum.  Optional is a perfectly useful and accepted tool.  The fact that
it is new to the chromium code base is more or less irrelevant -- we also
survived without unique_ptr for a long time, as it was only accepted less than a
year ago, but that doesn't mean it's a bad idea.  The question of relevance here
is whether this API makes sense.

The biggest thing Optional buys us here is making it harder to accidentally use
an error condition return value.  If we use the "empty-file-path-means-error"
convention, a naive caller that blindly uses the returned FilePath might get
weird results that may or may not be easily trackable back to failing to check
the result of this call.

It also makes it immediately obvious from the function signature that you may
*not* get a valid FilePath back from this function, which is also valuable,
otherwise you are relying on users noticing documentation to call the function
in the right way. 

The only "cost" to using Optional here is that the caller has to say .value() to
get the FilePath out, which seems pretty minimal to me.

If you want to draw a line in the sand because it's not your preference, so be
it, but frankly I think you've straying pretty far from "is this reasonable
code" and well into "does this code precisely match how I would implement it
myself". 

[Lei Zhang, 2016/10/18 22:25:34]

You wrote "happy to take any suggestions" so I'm giving you suggestions. You
wrote "I'm generally against ..." so I'm trying to understand your broad
generalizations.

It's your code here. If you feel Optional is correct here, and the other folks
who have to work with your code are ok with it, then great, have fun. If you
want to use Optional everywhere, then maybe you will have more litigation with
other reviewers in the future.

https://chromium.googlesource.com/chromium/src/+/master/docs/optional.md is
relevant here, but my interpretation doesn't sway the argument in either
direction.

Also, we have had unique_ptr as scoped_ptr in Chromium for ~10 years. It's not
new, whereas base::Optional is.
By convention, in many places, an empty FilePath means "failure", "false", or
"the end." See base::FileEnumerator::Next() and drive::util::ExtractDrivePath()
as a couple selected examples.

Basically anything in Chromium that deals with base::FilePath tends to use an
empty FilePath to indicate failure. Yes, I'm sure at some point someone forgot
to check the return result and created a bug, but in general, we are not losing
sleep over this. Yes, this may corrolate with the fact that we didn't have
Optional for a long time. But then again, the various FilePath API authors also
didn't go out of their way make their code return a FilePath pointer.
Without Optional, there is one less state to be concerned about:

non-empty FilePath -> probably a valid path
empty FilePath -> failure

but with Optional, now we have:

nullopt_t -> failure
non-empty FilePath -> probably a valid path
empty FilePath -> also a failure???

The caller now has one more possibility to worry about. If you add a comment and
say "returned FilePath will never be empty" then ok, but see my comment above
about convention.
I never said you can't do this. Once again, I'm just trying to understand your
position and point out what's more commonly done. To go with my probably not
perfect UK analogy, if everyone else drives on the left, go with the flow.

[Carlson, 2016/10/18 23:54:31]

Social subtleties don't always come across well in code review, and it sounds
like I misinterpreted the spirit of this discussion from your end.  Apologies
for that.  I do appreciate and recognize that we both have the same goal, that
is making APIs sane from the get-go.
I think it's fair to summarize the issue you bring up here as one of judging
whether the benefits of using the New Thing (Optional) outweigh the implicit
cost of having an API that deviates from the common usage
(empty-FilePath-as-error-value).  

My hope is that, as better structs for these things become available, we *will*
see APIs move to more explicit, type-enforced error values, including FilePath
libraries.  As I mentioned earlier, I think the *best* answer here would be
StatusOr, as it has precisely the desired semantics -- "Either a FilePath or an
error will be returned and you're going to get a compile error if you don't
check that error...", but Optional gets us most of the way there by demuxing the
error value from the return type.

(As an aside, I was, until relatively recently, pretty unconvinced that StatusOr
and friends were good ideas; mostly because they do tend to be quite verbose,
especially in headers.  But having worked for a while with code that uses them
extensively, I'm firmly convinced that the added compiler support for ensuring
that callsites are correct is well worth the syntactic overhead).

Anyways, to make a long story short (too late) I'm quite convinced that,
although the case is not clear-cut, Optional here is sufficiently better than
bare FilePath that it's worth breaking with wider codebase convention.
The latter case would definitely be considered a library bug.  Updated comment
to be more explicit about this.  
Again, apologies for the overly aggressive reply.

+      const Printer::PpdReference& reference) const = 0;
+
+  // Take the contents of a ppd file, store it to the cache, and return the
+  // path to the stored file keyed on reference.
+  //
+  // If a different ppd was previously Stored for the given reference, it
+  // will be replaced.
+  //
+  // On success, the returned FilePath is guaranteed to remain valid until the
+  // next Store* call.
+  virtual base::Optional<base::FilePath> Store(
+      const Printer::PpdReference& reference,
+      const std::string& ppd_contents) = 0;
+};
+
+}  // namespace printing
+}  // namespace chromeos
+
+#endif  // CHROMEOS_PRINTING_PPD_CACHE_H_