Translate: split supporting language list handling to TranslateLanguageList

chrome/browser/translate/translate_language_list.h

+// Copyright 2013 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 CHROME_BROWSER_TRANSLATE_TRANSLATE_LANGUAGE_LIST_H_
+#define CHROME_BROWSER_TRANSLATE_TRANSLATE_LANGUAGE_LIST_H_
+
+#include <set>

[MAD, 2013/05/22 14:00:04]

I don't think this is needed here. Right?

[Takashi Toyoshima, 2013/05/23 08:21:19]

Done.

+#include <string>
+#include <vector>
+
+#include "base/memory/scoped_ptr.h"
+#include "base/memory/singleton.h"
+#include "net/url_request/url_fetcher_delegate.h"
+
+class TranslateManager;
+class TranslateManagerBrowserTest;
+
+namespace net {
+class URLFetcher;
+}
+
+// The TranslateLanguageList class is responsible for maintaining the latest
+// supporting language list.
+// It is a singleton.
+

[MAD, 2013/05/22 14:00:04]

No empty line here.

[Takashi Toyoshima, 2013/05/23 08:21:19]

Done.

+class TranslateLanguageList : public net::URLFetcherDelegate {
+ public:
+  // Returns the singleton instance.
+  static TranslateLanguageList* GetInstance();
+
+  virtual ~TranslateLanguageList();
+
+  // net::URLFetcherDelegate implementation:
+  virtual void OnURLFetchComplete(const net::URLFetcher* source) OVERRIDE;
+
+  // Fills |languages| with the list of languages that the translate server can
+  // translate to and from.
+  static void GetSupportedLanguages(std::vector<std::string>* languages);
+
+  // Returns true if |language| is supported by the translation server.
+  static bool IsSupportedLanguage(const std::string& language);
+
+  // TODO(toyoshim): Add IsSupportedAlphaLanguage() here.
+
+  // Returns the language code that can be used with the Translate method for a
+  // specified |chrome_locale|.
+  static std::string GetLanguageCode(const std::string& chrome_locale);
+
+  // static const values shared with our browser tests.
+  static const char kLanguageListCallbackName[];
+  static const char kTargetLanguagesKey[];
+
+ protected:
+  TranslateLanguageList();
+
+ private:
+  friend class TranslateManager;

[MAD, 2013/05/22 14:00:04]

I'm a bit unsure about this... We usually do this, only temporary, or tests,
base helper or impl classes, but very rarely should have two separate classes
friend of eachother like this...

One alternative could be to make this class a private static member of the
translate manager, and still have the static method exposed by the translate
manager, and have this whole class a lazy initialized singleton, making sure the
translate manager's is the only instance, and then make these methods that the
translate manager needs... public...

So I'll stop the CR here and wait for a resolution on this issue before
continuing...

[Takashi Toyoshima, 2013/05/22 14:29:17]

How about just exposing  RequestLanguageList(), HasPendingRequest(), and
ResetPendingRequest() as public functions, and remove this friend line?

Currently, translate_manager.cc is a little complicated. I want to split
language list handling code to another file.

Alternative idea is make this class non-singleton class, and just make
TranslateManager own an instance for composition.

[Takashi Toyoshima, 2013/05/23 08:21:19]

Done (move these three functions to public)

+  friend struct DefaultSingletonTraits<TranslateLanguageList>;
+
+  // Following three functions, RequestLanguageList(), HasPendingRequest(),
+  // and ResetPendingRequest() should be called only by TranslateManager.
+
+  // Fetches the language list from the translate server. It will not retry
+  // more than kMaxRetryLanguageListFetch times.
+  static void RequestLanguageList();
+
+  // Returns true if there is a pending URLFetcher request.
+  static bool HasPendingRequest();
+
+  // Cleanups pending URLFetcher objects to make sure they get released in the

[MAD, 2013/05/23 15:58:13]

Cleanups -> Cleans up

+  // appropriate thread.
+  static void ResetPendingRequest();
+
+  // Set when the list of languages is currently being retrieved.
+  // NULL otherwise.
+  scoped_ptr<net::URLFetcher> url_fetcher_;
+
+  DISALLOW_COPY_AND_ASSIGN(TranslateLanguageList);
+};
+
+#endif  // CHROME_BROWSER_TRANSLATE_TRANSLATE_LANGUAGE_LIST_H_