Readability review for joaodasilva

chrome/browser/policy/url_blacklist_manager.h

 // Copyright (c) 2011 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_POLICY_URL_BLACKLIST_MANAGER_H_
 #define CHROME_BROWSER_POLICY_URL_BLACKLIST_MANAGER_H_
 #pragma once
 
 #include <string>
 #include <vector>
@@ skipping 12 matching lines @@
 class NotificationSource;
 class PrefService;
 
 namespace base {
 class ListValue;
 }
 
 namespace policy {
 
 // Contains a set of filters to block and allow certain URLs, and matches GURLs
 // against this set. The filters are currently kept in memory.

[zunger, 2011/10/20 22:24:03]

What's the thread-safety of this class?

[Joao da Silva, 2011/10/21 10:19:47]

It's not thread safe. After setting up all the filters with Allow and Block,
concurrent calls to IsURLBlocked are safe though.

Instances are currently only owned by a URLBlacklistManager on a specific thread
(IO). This class should be private and in the .cc file, but the declaration is
here for testing.

 class URLBlacklist {
  public:
   URLBlacklist();
   virtual ~URLBlacklist();
 
   // URLs matching |filter| will be blocked.

[zunger, 2011/10/20 22:24:03]

Are these exact URLs? Is any normalization done?

[Joao da Silva, 2011/10/21 10:19:47]

Most URLs are valid filters, with a couple of exceptions. The format is
documented for administrators at
http://www.chromium.org/administrators/url-blacklist-filter-format ; I've left a
comment here pointing to that document.

   void Block(const std::string& filter);
 
   // URLs matching |filter| will be allowed. If |filter| is both Blocked and
   // Allowed, Allow takes precedence.
   void Allow(const std::string& filter);
 
   // Returns true if the URL is blocked.
   bool IsURLBlocked(const GURL& url) const;
 
   // A constant mapped to a scheme that can be filtered.
@@ skipping 16 matching lines @@
   // |port| is 0 if none is explicitly defined.
   // |path| does not include query parameters.
   static bool FilterToComponents(const std::string& filter,
                                  std::string* scheme,
                                  std::string* host,
                                  uint16* port,
                                  std::string* path);
  private:
   void AddFilter(const std::string& filter, bool block);
 
   struct PathFilter {

[zunger, 2011/10/20 22:24:03]

You can move this entire declaration into the .cc file and just forward-declare
here.

[Joao da Silva, 2011/10/21 10:19:47]

Done.

     explicit PathFilter(const std::string& path, uint16 port, bool match)
         : path_prefix(path),
           port(port),
           blocked_schemes(0),
           allowed_schemes(0),
           match_subdomains(match) {}
 
     std::string path_prefix;
     uint16 port;
     uint8 blocked_schemes;
@@ skipping 83 matching lines @@
 
   // The current blacklist.
   scoped_ptr<URLBlacklist> blacklist_;
 
   DISALLOW_COPY_AND_ASSIGN(URLBlacklistManager);
 };
 
 }  // namespace policy
 
 #endif  // CHROME_BROWSER_POLICY_URL_BLACKLIST_MANAGER_H_
+