Use persistent map of privet tokens for PrivetURLFetcher

Use persistent map of privet tokens for PrivetURLFetcher

Use a map of privet tokens in PrivetURLFetcher that persists between
instantiations. This should cut down on spurious /privet/info calls in cases
where spurious calls may cause performance issues (i.e. repeated frequent calls)

BUG=

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=252123

[Vitaly Buka (NO REVIEWS), 2014-02-14 22:57:45]

Do we care that device may change IP if browser alive for a long time.

What about re-connection to different network when same host maybe a different
device?

https://chromiumcodereview.appspot.com/166373006/diff/1/chrome/browser/local_...
File chrome/browser/local_discovery/privet_url_fetcher.cc (right):

https://chromiumcodereview.appspot.com/166373006/diff/1/chrome/browser/local_...
chrome/browser/local_discovery/privet_url_fetcher.cc:21: namespace {
empty line after { and before } below

https://chromiumcodereview.appspot.com/166373006/diff/1/chrome/browser/local_...
chrome/browser/local_discovery/privet_url_fetcher.cc:53: void
PrivetURLFetcher::SetTokenForHost(const  std::string& host,
double white space

https://chromiumcodereview.appspot.com/166373006/diff/1/chrome/browser/local_...
chrome/browser/local_discovery/privet_url_fetcher.cc:55: if (!g_token_map)
g_token_map = new TokenMap();
Can you please use Singleton for g_tocket_map?

[Noam Samuel, 2014-02-14 22:59:43]

On 2014/02/14 22:57:45, Vitaly Buka wrote:
> Do we care that device may change IP if browser alive for a long time.
> 
> What about re-connection to different network when same host maybe a different
> device?
> 
>
https://chromiumcodereview.appspot.com/166373006/diff/1/chrome/browser/local_...
> File chrome/browser/local_discovery/privet_url_fetcher.cc (right):
> 
>
https://chromiumcodereview.appspot.com/166373006/diff/1/chrome/browser/local_...
> chrome/browser/local_discovery/privet_url_fetcher.cc:21: namespace {
> empty line after { and before } below
> 
>
https://chromiumcodereview.appspot.com/166373006/diff/1/chrome/browser/local_...
> chrome/browser/local_discovery/privet_url_fetcher.cc:53: void
> PrivetURLFetcher::SetTokenForHost(const  std::string& host,
> double white space
> 
>
https://chromiumcodereview.appspot.com/166373006/diff/1/chrome/browser/local_...
> chrome/browser/local_discovery/privet_url_fetcher.cc:55: if (!g_token_map)
> g_token_map = new TokenMap();
> Can you please use Singleton for g_tocket_map?

In case there is a mistake in either direction, the result will just be one more
/privet/info call. I.e.: If we use the wrong X-Privet-Token, the host will just
send us a missing x privet token message, and we'll call /privet/info, and if
the same host changes IP addresses, then we'll just call /privet/info and get a
token.

[Vitaly Buka (NO REVIEWS), 2014-02-14 23:02:02]

On 2014/02/14 22:59:43, Noam Samuel wrote:
> On 2014/02/14 22:57:45, Vitaly Buka wrote:
> > Do we care that device may change IP if browser alive for a long time.
> > 
> > What about re-connection to different network when same host maybe a
different
> > device?
> > 
> >
>
https://chromiumcodereview.appspot.com/166373006/diff/1/chrome/browser/local_...
> > File chrome/browser/local_discovery/privet_url_fetcher.cc (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/166373006/diff/1/chrome/browser/local_...
> > chrome/browser/local_discovery/privet_url_fetcher.cc:21: namespace {
> > empty line after { and before } below
> > 
> >
>
https://chromiumcodereview.appspot.com/166373006/diff/1/chrome/browser/local_...
> > chrome/browser/local_discovery/privet_url_fetcher.cc:53: void
> > PrivetURLFetcher::SetTokenForHost(const  std::string& host,
> > double white space
> > 
> >
>
https://chromiumcodereview.appspot.com/166373006/diff/1/chrome/browser/local_...
> > chrome/browser/local_discovery/privet_url_fetcher.cc:55: if (!g_token_map)
> > g_token_map = new TokenMap();
> > Can you please use Singleton for g_tocket_map?
> 
> In case there is a mistake in either direction, the result will just be one
more
> /privet/info call. I.e.: If we use the wrong X-Privet-Token, the host will
just
> send us a missing x privet token message, and we'll call /privet/info, and if
> the same host changes IP addresses, then we'll just call /privet/info and get
a
> token.

Do we want to reset cache from time to time?

[Noam Samuel, 2014-02-14 23:09:08]

On 2014/02/14 23:02:02, Vitaly Buka wrote:
> On 2014/02/14 22:59:43, Noam Samuel wrote:
> > On 2014/02/14 22:57:45, Vitaly Buka wrote:
> > > Do we care that device may change IP if browser alive for a long time.
> > > 
> > > What about re-connection to different network when same host maybe a
> different
> > > device?
> > > 
> > >
> >
>
https://chromiumcodereview.appspot.com/166373006/diff/1/chrome/browser/local_...
> > > File chrome/browser/local_discovery/privet_url_fetcher.cc (right):
> > > 
> > >
> >
>
https://chromiumcodereview.appspot.com/166373006/diff/1/chrome/browser/local_...
> > > chrome/browser/local_discovery/privet_url_fetcher.cc:21: namespace {
> > > empty line after { and before } below
> > > 
> > >
> >
>
https://chromiumcodereview.appspot.com/166373006/diff/1/chrome/browser/local_...
> > > chrome/browser/local_discovery/privet_url_fetcher.cc:53: void
> > > PrivetURLFetcher::SetTokenForHost(const  std::string& host,
> > > double white space
> > > 
> > >
> >
>
https://chromiumcodereview.appspot.com/166373006/diff/1/chrome/browser/local_...
> > > chrome/browser/local_discovery/privet_url_fetcher.cc:55: if (!g_token_map)
> > > g_token_map = new TokenMap();
> > > Can you please use Singleton for g_tocket_map?
> > 
> > In case there is a mistake in either direction, the result will just be one
> more
> > /privet/info call. I.e.: If we use the wrong X-Privet-Token, the host will
> just
> > send us a missing x privet token message, and we'll call /privet/info, and
if
> > the same host changes IP addresses, then we'll just call /privet/info and
get
> a
> > token.
> 
> Do we want to reset cache from time to time?

Possibly. I'm not sure when would be a good time.

[Noam Samuel, 2014-02-14 23:26:12]

https://codereview.chromium.org/166373006/diff/1/chrome/browser/local_discove...
File chrome/browser/local_discovery/privet_url_fetcher.cc (right):

https://codereview.chromium.org/166373006/diff/1/chrome/browser/local_discove...
chrome/browser/local_discovery/privet_url_fetcher.cc:21: namespace {
On 2014/02/14 22:57:46, Vitaly Buka wrote:
> empty line after { and before } below

Done.

https://codereview.chromium.org/166373006/diff/1/chrome/browser/local_discove...
chrome/browser/local_discovery/privet_url_fetcher.cc:53: void
PrivetURLFetcher::SetTokenForHost(const  std::string& host,
On 2014/02/14 22:57:46, Vitaly Buka wrote:
> double white space

Done.

https://codereview.chromium.org/166373006/diff/1/chrome/browser/local_discove...
chrome/browser/local_discovery/privet_url_fetcher.cc:55: if (!g_token_map)
g_token_map = new TokenMap();
On 2014/02/14 22:57:46, Vitaly Buka wrote:
> Can you please use Singleton for g_tocket_map?

Done.

[Vitaly Buka (NO REVIEWS), 2014-02-14 23:34:12]

lgtm

https://chromiumcodereview.appspot.com/166373006/diff/90001/chrome/browser/lo...
File chrome/browser/local_discovery/privet_url_fetcher.cc (right):

https://chromiumcodereview.appspot.com/166373006/diff/90001/chrome/browser/lo...
chrome/browser/local_discovery/privet_url_fetcher.cc:84: if (found !=
token_map_holder->map.end()) {
return found != token_map_holder->map.end() ? found->second : std::string();

[Noam Samuel, 2014-02-15 00:22:15]

https://codereview.chromium.org/166373006/diff/90001/chrome/browser/local_dis...
File chrome/browser/local_discovery/privet_url_fetcher.cc (right):

https://codereview.chromium.org/166373006/diff/90001/chrome/browser/local_dis...
chrome/browser/local_discovery/privet_url_fetcher.cc:84: if (found !=
token_map_holder->map.end()) {
On 2014/02/14 23:34:12, Vitaly Buka wrote:
> return found != token_map_holder->map.end() ? found->second : std::string();

Done.

[Noam Samuel, 2014-02-18 19:03:27]

The CQ bit was checked by noamsml@chromium.org

[commit-bot: I haz the power, 2014-02-18 19:04:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/noamsml@chromium.org/166373006/140001

[commit-bot: I haz the power, 2014-02-18 19:04:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-18 19:04:30]

Failed to apply patch for chrome/browser/local_discovery/privet_url_fetcher.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/local_discovery/privet_url_fetcher.cc
  Hunk #1 succeeded at 8 with fuzz 1.
  Hunk #2 FAILED at 20.
  Hunk #3 FAILED at 33.
  Hunk #4 succeeded at 74 with fuzz 2 (offset 27 lines).
  Hunk #5 succeeded at 107 with fuzz 1 (offset 41 lines).
  Hunk #6 succeeded at 164 (offset 51 lines).
  Hunk #7 succeeded at 321 (offset 101 lines).
  Hunk #8 succeeded at 332 (offset 101 lines).
  2 out of 8 hunks FAILED -- saving rejects to file
chrome/browser/local_discovery/privet_url_fetcher.cc.rej

Patch:       chrome/browser/local_discovery/privet_url_fetcher.cc
Index: chrome/browser/local_discovery/privet_url_fetcher.cc
diff --git a/chrome/browser/local_discovery/privet_url_fetcher.cc
b/chrome/browser/local_discovery/privet_url_fetcher.cc
index
349555086c6c7c569d022307267fdb998d41b69a..410846f09e5227b4a3e4df802442b5e785d7a7e5
100644
--- a/chrome/browser/local_discovery/privet_url_fetcher.cc
+++ b/chrome/browser/local_discovery/privet_url_fetcher.cc
@@ -8,6 +8,7 @@
 
 #include "base/bind.h"
 #include "base/json/json_reader.h"
+#include "base/memory/singleton.h"
 #include "base/message_loop/message_loop.h"
 #include "base/rand_util.h"
 #include "chrome/browser/browser_process.h"
@@ -19,11 +20,24 @@
 namespace local_discovery {
 
 namespace {
+
+typedef std::map<std::string, std::string> TokenMap;
+
+struct TokenMapHolder {
+ public:
+  static TokenMapHolder* GetInstance() {
+    return Singleton<TokenMapHolder>::get();
+  }
+
+  TokenMap map;
+};
+
 const char kXPrivetTokenHeaderPrefix[] = "X-Privet-Token: ";
 const char kXPrivetEmptyToken[] = "\"\"";
 const int kPrivetMaxRetries = 20;
 const int kPrivetTimeoutOnError = 5;
-}
+
+}  // namespace
 
 void PrivetURLFetcher::Delegate::OnNeedPrivetToken(
     PrivetURLFetcher* fetcher,
@@ -32,12 +46,11 @@ void PrivetURLFetcher::Delegate::OnNeedPrivetToken(
 }
 
 PrivetURLFetcher::PrivetURLFetcher(
-    const std::string& token,
     const GURL& url,
     net::URLFetcher::RequestType request_type,
     net::URLRequestContextGetter* request_context,
     PrivetURLFetcher::Delegate* delegate)
-    : privet_access_token_(token), url_(url), request_type_(request_type),
+    :  url_(url), request_type_(request_type),
       request_context_(request_context), delegate_(delegate),
       do_not_retry_on_transient_error_(false),
allow_empty_privet_token_(false),
       tries_(0), weak_factory_(this) {
@@ -46,6 +59,17 @@ PrivetURLFetcher::PrivetURLFetcher(
 PrivetURLFetcher::~PrivetURLFetcher() {
 }
 
+// static
+void PrivetURLFetcher::SetTokenForHost(const std::string& host,
+                                       const std::string& token) {
+  TokenMapHolder::GetInstance()->map[host] = token;
+}
+
+// static
+void PrivetURLFetcher::ResetTokenMapForTests() {
+  TokenMapHolder::GetInstance()->map.clear();
+}
+
 void PrivetURLFetcher::DoNotRetryOnTransientError() {
   do_not_retry_on_transient_error_ = true;
 }
@@ -54,10 +78,20 @@ void PrivetURLFetcher::AllowEmptyPrivetToken() {
   allow_empty_privet_token_ = true;
 }
 
+std::string PrivetURLFetcher::GetPrivetAccessToken() {
+  TokenMapHolder* token_map_holder = TokenMapHolder::GetInstance();
+  TokenMap::iterator found = token_map_holder->map.find(GetHostString());
+  return found != token_map_holder->map.end() ? found->second : std::string();
+}
+
+std::string PrivetURLFetcher::GetHostString() {
+  return url_.GetOrigin().spec();
+}
+
 void PrivetURLFetcher::Try() {
   tries_++;
   if (tries_ < kPrivetMaxRetries) {
-    std::string token = privet_access_token_;
+    std::string token = GetPrivetAccessToken();
 
     if (token.empty())
       token = kXPrivetEmptyToken;
@@ -91,7 +125,8 @@ void PrivetURLFetcher::Try() {
 void PrivetURLFetcher::Start() {
   DCHECK_EQ(tries_, 0);  // We haven't called |Start()| yet.
 
-  if (privet_access_token_.empty() && !allow_empty_privet_token_) {
+  std::string privet_access_token = GetPrivetAccessToken();
+  if (privet_access_token.empty() && !allow_empty_privet_token_) {
     RequestTokenRefresh();
   } else {
     Try();
@@ -197,7 +232,7 @@ void PrivetURLFetcher::RefreshToken(const std::string&
token) {
   if (token.empty()) {
     delegate_->OnError(this, TOKEN_ERROR);
   } else {
-    privet_access_token_ = token;
+    SetTokenForHost(GetHostString(), token);
     Try();
   }
 }
@@ -208,20 +243,4 @@ bool PrivetURLFetcher::PrivetErrorTransient(const
std::string& error) {
          (error == kPrivetErrorPrinterBusy);
 }
 
-PrivetURLFetcherFactory::PrivetURLFetcherFactory(
-    net::URLRequestContextGetter* request_context)
-    : request_context_(request_context) {
-}
-
-PrivetURLFetcherFactory::~PrivetURLFetcherFactory() {
-}
-
-scoped_ptr<PrivetURLFetcher> PrivetURLFetcherFactory::CreateURLFetcher(
-    const GURL& url, net::URLFetcher::RequestType request_type,
-    PrivetURLFetcher::Delegate* delegate) const {
-  return scoped_ptr<PrivetURLFetcher>(
-      new PrivetURLFetcher(token_, url, request_type, request_context_.get(),
-                           delegate));
-}
-
 }  // namespace local_discovery

[Noam Samuel, 2014-02-19 20:26:18]

The CQ bit was checked by noamsml@chromium.org

[commit-bot: I haz the power, 2014-02-19 20:48:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/noamsml@chromium.org/166373006/250001

[commit-bot: I haz the power, 2014-02-19 23:13:03]

Change committed as 252123