Restructure SDCH layering to allow more separation (observer/1->[0,n]

chrome/browser/net/chrome_sdch_policy.h

+// Copyright 2014 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.
+
+// Implementation of chrome embedder policy for SDCH.  Handles fetches.
+// TODO(rdsmith): Implement dictionary prioritization.

[mmenke, 2014/11/06 15:30:12]

nit:  Comments like this generally go above the class, not at the top of the
file.  I know that I, at least, always miss comments up here, because I've been
trained to ignore the license comments at the top of files.

[Randy Smith (Not in Mondays), 2014/11/06 16:39:04]

Done.

+
+#ifndef CHROME_BROWSER_NET_CHROME_SDCH_POLICY_H_
+#define CHROME_BROWSER_NET_CHROME_SDCH_POLICY_H_
+
+#include "net/base/sdch_observer.h"
+#include "net/url_request/sdch_dictionary_fetcher.h"
+
+namespace net {
+class SdchManager;
+class URLRequestContext;
+}
+
+class ChromeSdchPolicy : public net::SdchObserver {

[mmenke, 2014/11/06 15:30:12]

I wonder about the name...  "Policy" indicates to me it does something, rather
than just forward everything along to another class.  I guess it's because of
your TODO about prioritization?

[Randy Smith (Not in Mondays), 2014/11/06 16:39:04]

Yeah.  The context here is that we want an embedder class that determines
policy, which this one will.  More pedantically, "blindly accept all dictionary
suggestions until full up" counts as a policy.  I hope to get a bit more nuanced
in future CLs :-}.

+ public:
+  // Consumer must guarantee that |sdch_manager| and |context| outlive
+  // this object.
+  ChromeSdchPolicy(net::SdchManager* sdch_manager,
+                   net::URLRequestContext* context);
+  virtual ~ChromeSdchPolicy();

[mmenke, 2014/11/06 15:30:12]

nit:  -virtual +override

[Randy Smith (Not in Mondays), 2014/11/06 16:39:04]

Done.

+
+  void OnDictionaryFetched(const std::string& dictionary_text,

[mmenke, 2014/11/06 15:30:12]

#include  <string>

[Randy Smith (Not in Mondays), 2014/11/06 16:39:04]

Done.

+                           const GURL& dictionary_url);
+
+  // SdchObserver implementation.
+  virtual void OnGetDictionary(net::SdchManager* manager,
+                               const GURL& request_url,

[mmenke, 2014/11/06 15:30:12]

Maybe forward declare GURL?  Know it's part of the interface, so it always has
to have been declared, but then, so is SdchManager.  Not something I have much
opinion one, as long as we're consistent.

[Randy Smith (Not in Mondays), 2014/11/06 16:39:04]

I think if we really want consistency, we'd need to get something into the style
guide, but what makes sense to me to aim for: If something's in the .h file, no
need to put it in the .cc file.  But the .h file should forward declare or
include everything that it needs.  I've tried to do that here; let me know if
you see any gaps (or object to the goal).

+                               const GURL& dictionary_url) override;
+  virtual void OnClearDictionaries(net::SdchManager* manager) override;

[mmenke, 2014/11/06 15:30:11]

nit:  -virtual +override (x2)

[Randy Smith (Not in Mondays), 2014/11/06 16:39:04]

Done.

+
+ private:
+  net::SdchManager* manager_;
+  net::SdchDictionaryFetcher fetcher_;
+};
+
+#endif  // CHROME_BROWSER_NET_CHROME_SDCH_POLICY_H_