Provide mutable members of UrlRequestContext via pure-virtual interface

net/url_request/http_user_agent_settings.h

+// Copyright (c) 2012 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_URL_REQUEST_HTTP_USER_AGENT_SETTINGS_H_
+#define NET_URL_REQUEST_HTTP_USER_AGENT_SETTINGS_H_
+
+#include <string>
+
+#include "base/basictypes.h"
+#include "net/base/net_export.h"
+
+class GURL;
+
+namespace net {
+
+// The interface used by HTTP jobs to retrieve HTTP Accept-Language,
+// Accept-Charset and User-Agent header values.
+class NET_EXPORT HttpUserAgentSettings {
+ public:
+  HttpUserAgentSettings() {}
+  virtual ~HttpUserAgentSettings() {}
+
+  // Gets the value of 'Accept-Language' header field.
+  virtual std::string GetAcceptLanguage() = 0;

[willchan no longer on Chromium, 2012/10/24 03:40:10]

Is there a reason these aren't const? I think they should be. If they're lazily
initialized, they can use mutable (safe since this should be single threaded).
Getters should generally be const.

[pauljensen, 2012/10/24 19:19:00]

Now that these are not simple accessors (hence the change in name and return
type) we want the freedom to calculate the values dynamically which may require
writing to members.  The ChromeHttpUserAgentSettings getters are examples of
this, they cannot be const as they write to members to cache the results.  They
are not lazily initialized so I don't think mutable is appropriate.

[willchan no longer on Chromium, 2012/10/24 19:30:08]

I'm not sure I agree from a logical constness perspective. I think you may be
too embroiled in the actual implementation details. Take a look at it from the
user perspective...is there an ordering or something of calls to these getters
that would alter the results? I was too strict before when I said lazy
initialization, a more generalized way to say it would be lazy evaluation. It
sounds to me like you're just doing lazy evaluation, which is what mutable is
exactly there for (but be careful with multithreaded cases, which doesn't apply
here since this should be single threaded).

+
+  // Gets the value of 'Accept-Charset' header field.
+  virtual std::string GetAcceptCharset() = 0;
+
+  // Gets the UA string to use for the given URL.  Pass an empty URL to get
+  // the default UA string.
+  virtual std::string GetUserAgent(const GURL& url) = 0;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(HttpUserAgentSettings);

[erikwright (departed), 2012/10/24 13:14:33]

I'm pretty sure we don't require this for pure-virtual classes...

[pauljensen, 2012/10/24 19:19:00]

You suggested I take this out, then mmenke suggested I put it back.  The style
guide makes a pretty blanket statement that this should be used so I went with
mmenke's suggestion.  Is there a big downside to keeping it that I'm not seeing?

[mmenke, 2012/10/24 19:25:22]

My opinion is that it's easy for pure virtual classes to become non-pure
virtual, and to overlook adding it, so simpler to have it everywhere except when
copy and assign are desired.

The less thinking one has to do, the less often one break things.  There's a
life lesson somewhere in there, I'm sure.

[erikwright (departed), 2012/10/26 21:10:45]

SGTM.

+};
+
+}  // namespace net
+
+#endif  // NET_URL_REQUEST_HTTP_USER_AGENT_SETTINGS_H_
+