Use FileTypePolicies for is_archive and is_supported classifications.

chrome/common/safe_browsing/file_type_policies.h

Index: chrome/common/safe_browsing/file_type_policies.h
diff --git a/chrome/common/safe_browsing/file_type_policies.h b/chrome/common/safe_browsing/file_type_policies.h
index ff88f66922ab0496009003e41238123ea308ea9d..c12ae09032fc8091cd8a26e1d1b80c1716a47989 100644
--- a/chrome/common/safe_browsing/file_type_policies.h
+++ b/chrome/common/safe_browsing/file_type_policies.h
@@ -10,10 +10,13 @@
 
 #include "base/files/file_path.h"
 #include "base/gtest_prod_util.h"
+#include "base/synchronization/lock.h"
 #include "chrome/common/safe_browsing/download_file_types.pb.h"
 
 namespace safe_browsing {
 
+class FileTypePoliciesManager;
+
 // This holds a list of file types (aka file extensions) that we know about,
 // with policies related to how Safe Browsing and the download UI should treat
 // them.
@@ -21,18 +24,7 @@ namespace safe_browsing {
 // The data to populate it is read from a ResourceBundle and then also
 // fetched periodically from Google to get the most up-to-date policies.
 //
-// It should be setup and accessed on IO thread.
-
-// TODO(nparker): Replace the following methods' contents with calls to
-//   g_browser_process->safe_browsing_service()->file_type_policies()->***.
-//
-//   bool IsSupportedBinaryFile(const base::FilePath& file);
-//   bool IsArchiveFile(const base::FilePath& file);
-//   ClientDownloadRequest::DownloadType GetDownloadType(
-//       const base::FilePath& file);
-//   int GetSBClientDownloadTypeValueForUMA(const base::FilePath& file);
-//   bool IsAllowedToOpenAutomatically(const base::FilePath& path);
-//   DownloadDangerLevel GetFileDangerLevel(const base::FilePath& path);
+// This is thread safe. We assume it is updated at most every few hours.
 
 class FileTypePolicies {
  public:
@@ -40,6 +32,11 @@ class FileTypePolicies {
   FileTypePolicies();

[asanka, 2016/05/18 18:47:54]

If the lifetime of this class is going to be managed by a designated class, it
should probably not have a public constructor / desctructor.

[Nathan Parker, 2016/05/18 23:53:29]

Done.

   virtual ~FileTypePolicies();
 
+  // This returns the global instance. Typically its lifetime is mananged by
+  // SafeBrowingService.
+  static FileTypePolicies* GlobalInstance();

[asanka, 2016/05/18 18:47:54]

Can this return a const* ?

[Nathan Parker, 2016/05/18 23:53:28]

Nope, can't, because the PopulateFromDynanmicUpdate() will modify it.  That's
getting used in https://codereview.chromium.org/1986403002/ for
component-update.  I could make a GetInstanceMutable that's private and
friendify the component-updater class, but that seems to tangle things more than
is worth it.

+  static void SetGlobalInstance(FileTypePolicies* instance);
+
   // Read data from the main ResourceBundle. This updates the internal list
   // only if the data passes integrity checks. This is normally called once
   // after construction.
@@ -50,13 +47,30 @@ class FileTypePolicies {
   // protos.
   void PopulateFromDynamicUpdate(const std::string& binary_pb);
 
+  //
+  // Static Utils
+  //
+
+  // Returns the file extension, with the dot.
+  static const base::FilePath::StringType GetFileExtension(

[asanka, 2016/05/18 18:47:54]

const unnecessary for return type since it's being returned by value.

Also, how is this different from FilePath::GetFinalExtension()?

[Nathan Parker, 2016/05/18 23:53:28]

Done.
It strips trailing [. ]+ before calling FilePath::FinalExtension().  Maybe we
should add this to GetFinalExtension() and just call that?

I had moved this from download_protection_util.cc, since android needs this but
doesn't build that file.

+      const base::FilePath& file);
+
+  //
   // Accessors
-  const DownloadFileType& PolicyForFile(const base::FilePath& file);
-  const DownloadFileType::PlatformSettings& SettingsForFile(
+  //
+  DownloadFileType PolicyForFile(const base::FilePath& file);

[asanka, 2016/05/18 18:47:54]

All of these should be 'const' no?

[Nathan Parker, 2016/05/18 23:53:28]

Done, with the mutable lock_.

+  DownloadFileType::PlatformSettings SettingsForFile(
       const base::FilePath& file);
+  bool IsArchiveFile(const base::FilePath& file);
+
+  // SBClientDownloadExtensions UMA histogram bucket for this file's type.
   int64_t UmaValueForFile(const base::FilePath& file);
-  bool IsFileAnArchive(const base::FilePath& file);
-  float SampledPingProbability() const;
+
+  // True if download protection should send a ping to check
+  // this type of file.
+  bool IsCheckedBinaryFile(const base::FilePath& file);
+
+  float SampledPingProbability();
 
  protected:
   // Used in metrics, do not reorder.
@@ -84,19 +98,49 @@ class FileTypePolicies {
   // Return the ASCII lowercase extension w/o leading dot, or empty.
   static std::string CanonicalizedExtension(const base::FilePath& file);
 
+  // Look up the policy for a given ASCII ext.
+  virtual const DownloadFileType& PolicyForExtension(const std::string& ext);
+
  private:
+  static FileTypePolicies* global_instance_;
+
   // The latest config we've committed. Starts out null.
+  // Protected by lock_.
   std::unique_ptr<DownloadFileTypeConfig> config_;
 
   // This references entries in config_.
+  // Protected by lock_.
   std::map<std::string, const DownloadFileType*> file_type_by_ext_;
 
   // Type used if we can't load from disk.
+  // Written only in the constructor.
   DownloadFileType last_resort_default_;
+  base::Lock lock_;

[asanka, 2016/05/18 18:47:54]

Locks are often mutable, and should explain what it's protecting (I guess
everything?).

[Nathan Parker, 2016/05/18 23:53:28]

I've got "protected by lock_" on the members above.  Would it be better to have
those here?

 
   FRIEND_TEST_ALL_PREFIXES(FileTypePoliciesTest, UnpackResourceBundle);
   FRIEND_TEST_ALL_PREFIXES(FileTypePoliciesTest, BadProto);
   FRIEND_TEST_ALL_PREFIXES(FileTypePoliciesTest, BadUpdateFromExisting);
+
+  friend FileTypePoliciesManager;
+};
+
+
+// This manages the lifetime of the global FileTypePolicies.
+// If the global object already exists, this does nothing.

[asanka, 2016/05/18 18:47:54]

Shall we DCHECK if folks try to use more than one instance? This class isn't
thread safe.

[Nathan Parker, 2016/05/18 23:53:28]

I've removed this class.

+//
+// SafeBrowsingService creates the canonical object.  This can be also
+// be instantiated in unittests and in utility processes that need it.
+// If more than one is instantiated, the first one will keep ownership.
+// This is important since the ZipAnalyzer code is tested both in a
+// separate process (browser_tests) and shared with the main process
+// (unit_tests) with a SafeBrowsingService.
+class FileTypePoliciesManager {

[asanka, 2016/05/18 18:47:54]

Should mention how it's to be used (or not to be used) and the fact that it's
not thread safe.

[Nathan Parker, 2016/05/18 23:53:29]

I've removed this class.

+ public:
+  FileTypePoliciesManager();
+  virtual ~FileTypePoliciesManager();
+
+ private:
+  std::unique_ptr<FileTypePolicies> file_type_policies_;
 };
 
 }  // namespace safe_browsing