Readability review for joaodasilva

chrome/browser/policy/url_blacklist_manager.cc

 // 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.
 
 #include "chrome/browser/policy/url_blacklist_manager.h"
 
 #include "base/bind.h"
 #include "base/stl_util.h"
 #include "base/string_number_conversions.h"
 #include "base/string_util.h"
@@ skipping 16 matching lines @@
 const int64 kUpdateDelayMs = 1000;
 
 // Maximum filters per policy. Filters over this index are ignored.
 const size_t kMaxFiltersPerPolicy = 100;
 
 typedef std::vector<std::string> StringVector;
 
 StringVector* ListValueToStringVector(const base::ListValue* list) {
   StringVector* vector = new StringVector;
 
   if (!list)

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

Better to say 'list == NULL' explicitly.

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

Done.

     return vector;
 
   vector->reserve(list->GetSize());
   std::string s;
   for (base::ListValue::const_iterator it = list->begin();
        it != list->end() && vector->size() < kMaxFiltersPerPolicy; ++it) {
     if ((*it)->GetAsString(&s))
       vector->push_back(s);
   }
 
@@ skipping 17 matching lines @@
     blacklist->Allow(*it);
   }
 }
 
 // A task that owns the URLBlacklist, and passes it to the URLBlacklistManager
 // on the IO thread, if the URLBlacklistManager still exists.
 void SetBlacklistOnIO(base::WeakPtr<URLBlacklistManager> blacklist_manager,
                       URLBlacklist* blacklist) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   if (blacklist_manager)
     blacklist_manager->SetBlacklist(blacklist);

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

Use explicit { }'s when the composite if statement takes up more than two lines.

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

Done.

   else
     delete blacklist;
 }
 
 }  // namespace
 
 URLBlacklist::URLBlacklist() {
 }
 
 URLBlacklist::~URLBlacklist() {
   STLDeleteValues(&host_filters_);
 }
 
 void URLBlacklist::AddFilter(const std::string& filter, bool block) {
   std::string scheme;
   std::string host;
   uint16 port;
   std::string path;
   SchemeFlag flag;

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

Declare variables immediately before first use.

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

Done.

   bool match_subdomains = true;
 
   if (!FilterToComponents(filter, &scheme, &host, &port, &path)) {
     LOG(WARNING) << "Invalid filter, ignoring: " << filter;
     return;
   }
 
   if (!SchemeToFlag(scheme, &flag)) {
     LOG(WARNING) << "Unsupported scheme in filter, ignoring filter: " << filter;
     return;
@@ skipping 11 matching lines @@
   HostFilterTable::iterator host_filter = host_filters_.find(host);
   if (host_filter == host_filters_.end()) {
     list = new PathFilterList;
     host_filters_[host] = list;
   } else {
     list = host_filter->second;
   }
   PathFilterList::iterator it;
   for (it = list->begin(); it != list->end(); ++it) {
     if (it->port == port && it->match_subdomains == match_subdomains &&
         it->path_prefix == path)

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

Use explicit { }'s when the hypothesis takes up more than 1 line.

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

Done.

       break;
   }
   PathFilter* path_filter;
   if (it == list->end()) {
     list->push_back(PathFilter(path, port, match_subdomains));
     path_filter = &list->back();
   } else {
     path_filter = &(*it);
   }
 
   if (block)

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

&c

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

Not clear what is meant here; I've added explicit { } since the composite if
statement takes more than two lines, as mentioned above.

     path_filter->blocked_schemes |= flag;
   else
     path_filter->allowed_schemes |= flag;
 }
 
 void URLBlacklist::Block(const std::string& filter) {
   AddFilter(filter, true);
 }
 
 void URLBlacklist::Allow(const std::string& filter) {
@@ skipping 10 matching lines @@
   std::string host(url.host());
   int int_port = url.EffectiveIntPort();
   const uint16 port = int_port > 0 ? int_port : 0;
   const std::string& path = url.path();
 
   // The first iteration through the loop will be an exact host match.
   // Subsequent iterations are subdomain matches, and some filters don't apply
   // to those.
   bool is_matching_subdomains = false;
   const bool host_is_ip = url.HostIsIPAddress();
   for (;;) {

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

while (1) is a bit more common in google3 code.

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

Done.

     HostFilterTable::const_iterator host_filter = host_filters_.find(host);
     if (host_filter != host_filters_.end()) {
       const PathFilterList* list = host_filter->second;
       size_t longest_length = 0;
       bool is_blocked = false;
       bool has_match = false;
       bool has_exact_host_match = false;
       for (PathFilterList::const_iterator it = list->begin();
            it != list->end(); ++it) {
         // Filters that apply to an exact hostname only take precedence over
@@ skipping 52 matching lines @@
       ++i;
     host.erase(0, i);
   }
 
   // Default is to allow.
   return false;
 }
 
 // static
 bool URLBlacklist::SchemeToFlag(const std::string& scheme, SchemeFlag* flag) {
   if (scheme.empty())

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

{ }'s

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

Done.

     *flag = SCHEME_ALL;
   else if (scheme == "http")
     *flag = SCHEME_HTTP;
   else if (scheme == "https")
     *flag = SCHEME_HTTPS;
   else if (scheme == "ftp")
     *flag = SCHEME_FTP;
   else
     return false;
   return true;
@@ skipping 141 matching lines @@
 
 // static
 void URLBlacklistManager::RegisterPrefs(PrefService* pref_service) {
   pref_service->RegisterListPref(prefs::kUrlBlacklist,
                                  PrefService::UNSYNCABLE_PREF);
   pref_service->RegisterListPref(prefs::kUrlWhitelist,
                                  PrefService::UNSYNCABLE_PREF);
 }
 
 }  // namespace policy
+