Add PPDProvider barebones implementation and associated cache skeleton.

chromeos/printing/ppd_provider.cc

+// 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.
+#include "chromeos/printing/ppd_provider.h"

[Lei Zhang, 2016/10/19 01:25:10]

blank line above.

[Carlson, 2016/10/19 16:38:27]

Done.  Am I misrunning lint somehow?  I would expect 'git cl lint' to turn up
stuff like this.

[Lei Zhang, 2016/10/19 23:15:46]

Indeed, rather strange.

[Carlson, 2016/10/19 23:38:56]

Acknowledged.

+
+#include <utility>
+
+#include "base/files/file_util.h"
+#include "base/json/json_parser.h"
+#include "base/memory/ptr_util.h"
+#include "base/strings/string_number_conversions.h"
+#include "base/strings/string_util.h"
+#include "base/time/time.h"
+#include "base/values.h"
+#include "chromeos/printing/ppd_cache.h"
+#include "net/base/load_flags.h"
+#include "net/http/http_status_code.h"
+#include "net/url_request/url_fetcher.h"
+#include "net/url_request/url_fetcher_delegate.h"
+#include "net/url_request/url_request_context_getter.h"
+#include "url/gurl.h"
+
+using ::base::DictionaryValue;
+using ::base::FilePath;
+using ::base::Optional;
+using ::base::Value;
+using ::net::URLFetcher;
+using ::net::URLFetcherDelegate;
+using ::std::string;
+using ::std::unique_ptr;
+
+namespace chromeos {
+namespace printing {
+namespace {
+
+// Expected fields from the quirks server.
+const char kJSONPPDKey[] = "compressedPpd";
+const char kJSONLastUpdatedKey[] = "lastUpdatedTime";
+
+class PpdProviderImpl;
+
+// URLFetcherDelegate that just forwards the complete callback back to
+// the PpdProvider that owns the delegate.
+class ForwardingURLFetcherDelegate : public URLFetcherDelegate {
+ public:
+  explicit ForwardingURLFetcherDelegate(PpdProviderImpl* owner)
+      : owner_(owner) {}
+  ~ForwardingURLFetcherDelegate() override {}
+
+  // URLFetcherDelegate API method.  Defined below since we need the
+  // PpdProviderImpl definition first.
+  void OnURLFetchComplete(const URLFetcher* source) override;
+
+ private:
+  // owner of this delegate.
+  PpdProviderImpl* owner_;
+};
+
+class PpdProviderImpl : public PpdProvider {
+ public:
+  PpdProviderImpl(
+      const string& api_key,
+      scoped_refptr<net::URLRequestContextGetter> url_context_getter,
+      unique_ptr<PpdCache> cache,
+      const PpdProvider::Options& options)
+      : api_key_(api_key),
+        forwarding_delegate_(this),
+        url_context_getter_(url_context_getter),
+        cache_(std::move(cache)),
+        options_(options) {
+    CHECK_GT(options_.max_ppd_contents_size_, static_cast<size_t>(0));

[Lei Zhang, 2016/10/19 01:25:10]

You may be able to just write "0U" and avoid the static_cast.

[Carlson, 2016/10/19 16:38:26]

Done.

+  }
+  ~PpdProviderImpl() override {}
+
+  void Resolve(const Printer::PpdReference& ppd_reference,
+               PpdProvider::ResolveCallback cb) override {
+    if (fetcher_ != nullptr) {

[Lei Zhang, 2016/10/19 01:25:10]

fop != nullptr -> foo
bar == nullptr -> !bar

[Carlson, 2016/10/19 16:38:26]

Acknowledged.

+      LOG(DFATAL) << "Can't have concurrent PpdProvider Resolve calls";
+      // Even though the caller has done something wrong, try to do something

[Lei Zhang, 2016/10/19 01:25:10]

See the "CHECK(), DCHECK(), and NOTREACHED()" philosophy on
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md -
maybe you don't want to bother handling this?

[Carlson, 2016/10/19 16:38:26]

Thanks for the link, it's good to know the consensus view here. 

Initially I didn't implement the fallback because I thought this was an
appropriate place to fail fast and crash the browser if someone violates the
threading model.  skau@ thought it was not reasonable to crash in this
circumstance (a view that I also think has significant merits -- you can
certainly envision a release that has code that triggers this check only very
infrequently, but frequently enough to be annoying for a substantial number of
users)

So whats your ultimate opinion about reasonableness-to-crash here?  If you think
it's not reasonable, I'll just LOG(ERROR) it, otherwise I'll remove the fallback
and change it to LOG(FATAL).

(semi-relatedly, is there a way in this code review tool to flag a specific
comment response as needing further followup?  I don't see any obvious way to
mark comments as resolved or not)

[Lei Zhang, 2016/10/19 23:15:46]

"A consequence of this is that you should not handle DCHECK() failures, even if
failure would result in a crash."

Just crash. If you reach this point because there is bad code, give up and
crash. Users will be annoyed. They will file bugs. Or it will show up on the
crash server and someone will take notice. Best case the bugs will get fixed
before reaching stable channel.

If you do NOTREACHED() and handle it, then in release builds, code execution
just blows through the NOTREACHED(), and the caller may or may not notice the
INTERNAL_ERROR. The bug may stay masked for a long time and developers won't
even realize there is a problem.
Don't think so.

[Carlson, 2016/10/19 23:38:56]

Done.

+      // at least semi-graceful if we're not running in debug.
+      url_context_getter_->GetNetworkTaskRunner()->PostTask(
+          FROM_HERE, base::Bind(cb, PpdProvider::INTERNAL_ERROR, FilePath()));
+      return;
+    }
+    Optional<FilePath> tmp = cache_->Find(ppd_reference);
+    if (tmp) {
+      // Cache hit.  Schedule the callback right now.
+      url_context_getter_->GetNetworkTaskRunner()->PostTask(
+          FROM_HERE, base::Bind(cb, PpdProvider::SUCCESS, tmp.value()));
+      return;
+    }
+
+    // We don't have a way to automatically resolve user-supplied ppds yet.  So
+    // if we have one specified, and it's not cached, we fail out rather than
+    // fall back to quirks-server based resolution.  The reasoning here is that
+    // if the user has specified a ppd when a quirks-server one exists, it
+    // probably means the quirks server one doesn't work for some reason, so we
+    // shouldn't silently use it.
+    if (!ppd_reference.user_supplied_ppd_url.empty()) {
+      // Cache hit.  Schedule the callback right now.
+      url_context_getter_->GetNetworkTaskRunner()->PostTask(
+          FROM_HERE, base::Bind(cb, PpdProvider::NOT_FOUND, base::FilePath()));
+      return;
+    }
+
+    active_reference_ = ppd_reference;
+    done_callback_ = cb;
+
+    fetcher_ = net::URLFetcher::Create(GetQuirksServerLookupURL(ppd_reference),
+                                       URLFetcher::GET, &forwarding_delegate_);
+    fetcher_->SetRequestContext(url_context_getter_.get());
+    fetcher_->SetLoadFlags(net::LOAD_BYPASS_CACHE | net::LOAD_DISABLE_CACHE |
+                           net::LOAD_DO_NOT_SAVE_COOKIES |
+                           net::LOAD_DO_NOT_SEND_COOKIES |
+                           net::LOAD_DO_NOT_SEND_AUTH_DATA);
+    fetcher_->Start();
+  };
+
+  void AbortResolve() override {
+    // UrlFetcher guarantees that when the object has been destroyed, no further
+    // callbacks will occur.
+    fetcher_.reset();
+  }
+
+  bool CachePpd(const Printer::PpdReference& ppd_reference,
+                const base::FilePath& ppd_path) override {
+    string buf;
+    if (!base::ReadFileToStringWithMaxSize(ppd_path, &buf,
+                                           options_.max_ppd_contents_size_)) {
+      return false;
+    }
+    return static_cast<bool>(cache_->Store(ppd_reference, buf));
+  }
+
+  // Called on the network thread when the fetcher completes its fetch.
+  void OnURLFetchComplete() {
+    // Scope the allocated fetcher_ into this function so we clean it up when

[Lei Zhang, 2016/10/19 01:25:10]

|fetcher_|

[Carlson, 2016/10/19 16:38:27]

Done.

+    // we're done here instead of leaving it around until the next Resolve call.
+    auto fetcher = std::move(fetcher_);
+    string contents;
+    if ((fetcher->GetStatus().status() != net::URLRequestStatus::SUCCESS) ||
+        (fetcher->GetResponseCode() != net::HTTP_OK) ||
+        !fetcher->GetResponseAsString(&contents)) {
+      // Something went wrong with the fetch.
+      done_callback_.Run(PpdProvider::SERVER_ERROR, FilePath());
+      return;
+    }
+
+    auto parsed_json = ::base::JSONReader::Read(contents);

[Lei Zhang, 2016/10/19 01:25:10]

You can take advantage of base::DictionaryValue::From().

[Carlson, 2016/10/19 16:38:27]

Thanks for the pointer.

Done, but I don't feel entirely ok about it; From() doesn't actually explicitly
enumerate what "if it is a dictionary" means w.r.t. nullptr.  The implementation
does seems to do what we want, though.  It just seems like borderline 'let's
depend on the implementation details and not the stated contract'.

This seems more likely to be a comment oversight than something that is likely
to change though.  (Famous last words...)

[Lei Zhang, 2016/10/19 23:15:46]

Apparently I approved the CL that added that comment, so it makes sense to me.
Nobody else has complainted about it AFAIK. I would like to hear why you are not
ok with it and what can be done to improve it. Comments / (separate) patches
welcome.

Given:

// Returns |value| if it is a dictionary, nullptr otherwise.
static std::unique_ptr<DictionaryValue> From(std::unique_ptr<Value> value);

Without peeking at the impl, my reading of the comment + signature is as
follows:

if (!value || !value->IsType(TYPE_DICTIONARY))
  return std::unique_ptr<DictionaryValue>();
return std::unique_ptr<DictionaryValue>(value.release());  // Or std::move() or
whatever to make this work.

Is the comment unclear? Do you see other interpretations?

[Carlson, 2016/10/19 23:38:56]

Ironically, I think this is a case of
special-value-conventions-leading-to-unclear-semantics.  :)  It's not clear from
the comment whether nullptr is in the set of "inputs that might reasonably be
dictionaries" or the set of "garbage input" (e.g. something that might crash the
function.

In other words, would suggest that the comment should read something like
"Returns |value| if it is non-null and a dictionary, nullptr otherwise".  I'll
probably send a patch for this when I get a minute.

[Lei Zhang, 2016/10/19 23:51:54]

Given it was added to deal with all the places where people write "if (value &&
value->IsDict() then cast..." ya, it does the nullptr check.

For fun, survey 10 Chromium developers and see if they think
base::DictionaryValue::From(std::unique_ptr<base::Value>()) would return a
st::unique_ptr<base::DictionaryValue>(), or crash?
Sure, can't hurt. Please note there's at least 1 other From() method in that
file.

+    DictionaryValue* dict;
+    if (parsed_json == nullptr ||  //

[Lei Zhang, 2016/10/19 01:25:10]

I also have not seen anyone else do this. It's an ugly hack but I don't have any
better suggestions right now, aside from base::DictionaryValue::From() will let
you avoid this particular case.

[Carlson, 2016/10/19 16:38:27]

Sorry, not clear on what 'this' refers to here.  Can you clarify?

[Lei Zhang, 2016/10/19 23:15:46]

Putting "//" at the end to avoid formatting. It, uh... sticks out.

[Carlson, 2016/10/19 23:38:56]

Acknowledged.

+        !parsed_json->GetAsDictionary(&dict)) {
+      // Malformed response.  TODO(justincarlson) - LOG something here?

[Lei Zhang, 2016/10/19 01:25:10]

Or let the caller handle it since it needs to check for SERVER_ERROR anyway?

BTW, "TODO(username): Comment" is the most common format.

[Carlson, 2016/10/19 16:38:27]

Good point, removed TODO

+      done_callback_.Run(PpdProvider::SERVER_ERROR, FilePath());
+      return;
+    }
+    string ppd_contents;
+    string last_updated_time_string;
+    uint64_t last_updated_time;
+    if (!(dict->GetString(kJSONPPDKey, &ppd_contents) &&
+          dict->GetString(kJSONLastUpdatedKey, &last_updated_time_string) &&
+          ::base::StringToUint64(last_updated_time_string,
+                                 &last_updated_time))) {
+      // Malformed response.  TODO(justincarlson) - LOG something here?
+      done_callback_.Run(PpdProvider::SERVER_ERROR, FilePath());
+      return;
+    }
+
+    if (ppd_contents.size() > options_.max_ppd_contents_size_) {
+      // PPD is too big.
+      //
+      // Note -- if we ever add shared-ppd-sourcing, e.g. we may serve a ppd to
+      // a user that's not from an explicitly trusted source, we should also
+      // check *uncompressed* size here to head off zip-bombs (e.g. let's
+      // compress 1GBs of zeros into a 900kb file and see what cups does when it
+      // tries to expand that...)
+      done_callback_.Run(PpdProvider::SERVER_ERROR, FilePath());
+      return;
+    }
+
+    auto ppd_file = cache_->Store(active_reference_, ppd_contents);
+    if (!ppd_file) {
+      // Failed to store.
+      done_callback_.Run(PpdProvider::INTERNAL_ERROR, FilePath());
+      return;
+    }
+    done_callback_.Run(PpdProvider::SUCCESS, ppd_file.value());
+  }
+
+ private:
+  // Generate a url to look up a manufacturer/model from the quirks server
+  GURL GetQuirksServerLookupURL(
+      const Printer::PpdReference& ppd_reference) const {
+    return GURL(::base::JoinString(
+        {
+            "https://",                            //

[Lei Zhang, 2016/10/19 01:25:10]

Again, I don't struggle with clang-format, so I don't know what to do here.
Maybe it would look better if you do base::StringPrintf()?

[Carlson, 2016/10/19 16:38:26]

(isn't clang-format what's underneath 'git cl format'?  Maybe I have that
wrong.)

Converted to StringPrintf, though I think it's marginally less readable than
vertically-aligned-interspersed.  Admittedly, some of the readability is
dependent on syntax highlighting though -- I think the vertical version is a lot
easier to understand immediately when the literals are a different color from
the std::string's.

+            options_.quirks_server,                //
+            "/v2/printer/manufacturers/",          //
+            ppd_reference.effective_manufacturer,  //
+            "/models/",                            //
+            ppd_reference.effective_model,         //
+            "?key=",                               //
+            api_key_                               //
+        },
+        ""));
+  }
+
+  // API key for accessing quirks server.
+  const string api_key_;
+
+  // Reference we're currently trying to resolve.
+  Printer::PpdReference active_reference_;
+
+  ForwardingURLFetcherDelegate forwarding_delegate_;
+  scoped_refptr<net::URLRequestContextGetter> url_context_getter_;
+  unique_ptr<PpdCache> cache_;
+
+  PpdProvider::ResolveCallback done_callback_;
+
+  // Fetcher for the current resolve call, if any.
+  unique_ptr<::net::URLFetcher> fetcher_;
+
+  // Construction-time options, immutable.
+  const PpdProvider::Options options_;
+};
+
+void ForwardingURLFetcherDelegate::OnURLFetchComplete(

[Lei Zhang, 2016/10/19 01:25:10]

Since you implemented PpdProviderImpl methods inline, do that for this too?
Especially since it's a one liner.

[Carlson, 2016/10/19 16:38:26]

I wish I could, but I can't because of recursive dependencies -- PpdProviderImpl
contains a ForwardingURLFetcherDelegate, and this function invokes
OnURLFetchComplete, so one of the functions needs to be out of line. 

+    const URLFetcher* source) {
+  owner_->OnURLFetchComplete();
+}
+
+}  // namespace
+
+// static
+unique_ptr<PpdProvider> PpdProvider::Create(
+    const string& api_key,
+    scoped_refptr<net::URLRequestContextGetter> url_context_getter,
+    unique_ptr<PpdCache> cache,
+    const PpdProvider::Options& options) {
+  return base::MakeUnique<PpdProviderImpl>(api_key, url_context_getter,
+                                           std::move(cache), options);
+}
+
+}  // namespace printing
+}  // namespace chromeos