Add net::SdchSourceStream and net::SdchPolicyDelegate

net/filter/sdch_policy_delegate.h

Index: net/filter/sdch_policy_delegate.h
diff --git a/net/filter/sdch_policy_delegate.h b/net/filter/sdch_policy_delegate.h
new file mode 100644
index 0000000000000000000000000000000000000000..ade2db8db63a337da13f1290d36512400c1e70eb
--- /dev/null
+++ b/net/filter/sdch_policy_delegate.h
@@ -0,0 +1,84 @@
+// 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 NET_FILTER_SDCH_POLICY_DELEGATE_H_
+#define NET_FILTER_SDCH_POLICY_DELEGATE_H_
+
+#include <memory>
+
+#include "base/macros.h"
+#include "base/supports_user_data.h"
+#include "net/base/net_export.h"
+#include "net/base/sdch_manager.h"
+#include "net/filter/sdch_source_stream.h"
+#include "net/log/net_log.h"
+#include "url/gurl.h"
+
+namespace net {
+
+class SdchSourceStream;
+
+// This class ascribes to the SdchSourceStream::Delegate interface to implement

[Randy Smith (Not in Mondays), 2016/09/27 20:09:13]

nit, suggestion "ascribes to" -> "implements" (this may be just a language
difference between us--I'm not familiar with "ascribes to" as a phrase).

[xunjieli, 2016/09/30 15:32:17]

Done.

+// the SDCH error-handling and stats-gathering policy. The Context object
+// supplies details about the request needed for this object to make
+// error-handling decisions. See the SdchSourceStream::Delegate documentation
+// for more details.
+class NET_EXPORT_PRIVATE SdchPolicyDelegate
+    : public SdchSourceStream::Delegate {
+ public:
+  class Context {
+   public:
+    enum StatisticSelector {
+      SDCH_DECODE,
+      SDCH_PASSTHROUGH,
+      SDCH_EXPERIMENT_DECODE,
+      SDCH_EXPERIMENT_HOLDBACK,
+    };
+
+    virtual ~Context() {}
+    virtual bool GetMimeType(std::string* mime_type) const = 0;
+    virtual bool GetURL(GURL* url) const = 0;
+    virtual bool IsCachedContent() const = 0;
+    virtual SdchManager* GetSdchManager() const = 0;
+    virtual SdchManager::DictionarySet* SdchDictionariesAdvertised() const = 0;
+    // virtual int64_t GetByteReadCount() const = 0;
+    virtual int GetResponseCode() const = 0;
+    virtual const NetLogWithSource& GetNetLog() const = 0;
+  };
+
+  SdchPolicyDelegate(std::unique_ptr<Context> context);

[Randy Smith (Not in Mondays), 2016/09/27 20:09:12]

I'd really like to explore the possibility of getting rid of the Context object
entirely, and just initializing the SdchPolicyDelegate with the information it
needs.  Would you be willing to see if that's a possible change?  (This is
partially because I hate FilterContext and want it to die rather than being
further baked into the code, and partially because this seems like an excess
amount of indirection, given that we already have the policy delegate.)

I'd think that mime_type, url, IsCachedContent(), SdchManager (with lifetime
guarantees), response code, and netlog could just be passed in.  I'd *think*
that the dictionaries advertised could be owned directly by this class, but I
could imagine that being a tricky change.  But could you see how hard it would
be?

This might be most easily explored in the context of a CL that included the
instantiation of these classes; such a CL wouldn't land (that'd be part of the
CL that Matt's reviewing) but would make it easier to evaluate this change.

[xunjieli, 2016/09/30 15:32:17]

I don't like the extra indirection either. However, many of the fields that the
Delegate requires are private members of URLRequestHttpJob. If we remove the
Context, we need to change these private members (e.g. GetMimeType()) to public
and add accessors for those that are not exposed (e.g. |is_cached_content_|)

How about we revisit this in a follow-up? I can add a TODO.

[Randy Smith (Not in Mondays), 2016/10/04 20:06:28]

That's fine; thanks.

+
+  ~SdchPolicyDelegate() override;
+
+  // SdchSourceStream::Delegate implementation.
+  bool OnDictionaryError(SdchSourceStream* source) override;
+  bool OnDecodingError(SdchSourceStream* source) override;
+  bool OnGetDictionary(const std::string& server_id,
+                       const std::string** text) override;
+
+ private:
+  // Issues a meta-refresh if the context's MIME type supports it, and returns
+  // whether a refresh was issued. As a side-effect, blacklists the context's
+  // domain either temporarily or permanently.
+  bool IssueMetaRefreshIfPossible(SdchSourceStream* source);
+
+  // Logs an SDCH failure. This logs events in UMA and the net log in
+  // |context_|, but has no side-effects on this class or the stream.
+  void LogSdchProblem(SdchProblemCode problem);
+
+  // Whether OnGetDictionary is invoked.
+  bool dictionary_requested_;

[Randy Smith (Not in Mondays), 2016/09/27 20:09:13]

Suggestion: As you know, I'm a bit allergic to these type of "state variables". 
It looks to me as if this is only used to distinguish between the case where
SdchSourceStream decides it's got a bogus dictionary id and the case where it
has a good dictionary id and tries and fails to get the dictionary.  Would it be
reasonable to simply split those two cases into two separate delegate calls and
get rid of this boolean?  It might result in simpler code for the delegate (--
code where fewer pieces of information need to be tracked in reading it).

[xunjieli, 2016/09/30 15:32:17]

Done.

+
+  std::unique_ptr<Context> context_;
+  // If the response was encoded with a dictionary different than those
+  // advertised (e.g. a cached response using an old dictionary), this
+  // variable preserves that dictionary from deletion during decoding.
+  std::unique_ptr<SdchManager::DictionarySet> unexpected_dictionary_set_;
+
+  DISALLOW_COPY_AND_ASSIGN(SdchPolicyDelegate);
+};
+
+}  // namespace net
+
+#endif  // NET_FILTER_SDCH_POLICY_DELEGATE_H_