Extract two methods by refactoring IndexedDBBackingStore::Open.

content/browser/indexed_db/indexed_db_backing_store.cc

 // Copyright (c) 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.
 
 #include "content/browser/indexed_db/indexed_db_backing_store.h"
 
 #include "base/file_util.h"
 #include "base/logging.h"
 #include "base/metrics/histogram.h"
 #include "base/strings/string_piece.h"
@@ skipping 366 matching lines @@
     : primary_key_(primary_key), version_(version) {
   DCHECK(!primary_key.empty());
 }
 IndexedDBBackingStore::RecordIdentifier::RecordIdentifier()
     : primary_key_(), version_(-1) {}
 IndexedDBBackingStore::RecordIdentifier::~RecordIdentifier() {}
 
 IndexedDBBackingStore::Cursor::CursorOptions::CursorOptions() {}
 IndexedDBBackingStore::Cursor::CursorOptions::~CursorOptions() {}
 
-enum IndexedDBLevelDBBackingStoreOpenResult {
-  INDEXED_DB_LEVEL_DB_BACKING_STORE_OPEN_MEMORY_SUCCESS,
-  INDEXED_DB_LEVEL_DB_BACKING_STORE_OPEN_SUCCESS,
-  INDEXED_DB_LEVEL_DB_BACKING_STORE_OPEN_FAILED_DIRECTORY,
-  INDEXED_DB_LEVEL_DB_BACKING_STORE_OPEN_FAILED_UNKNOWN_SCHEMA,
-  INDEXED_DB_LEVEL_DB_BACKING_STORE_OPEN_CLEANUP_DESTROY_FAILED,
-  INDEXED_DB_LEVEL_DB_BACKING_STORE_OPEN_CLEANUP_REOPEN_FAILED,
-  INDEXED_DB_LEVEL_DB_BACKING_STORE_OPEN_CLEANUP_REOPEN_SUCCESS,
-  INDEXED_DB_LEVEL_DB_BACKING_STORE_OPEN_FAILED_IO_ERROR_CHECKING_SCHEMA,
-  INDEXED_DB_LEVEL_DB_BACKING_STORE_OPEN_FAILED_UNKNOWN_ERR,
-  INDEXED_DB_LEVEL_DB_BACKING_STORE_OPEN_MEMORY_FAILED,
-  INDEXED_DB_LEVEL_DB_BACKING_STORE_OPEN_ATTEMPT_NON_ASCII,
-  INDEXED_DB_LEVEL_DB_BACKING_STORE_OPEN_DISK_FULL,
-  INDEXED_DB_LEVEL_DB_BACKING_STORE_OPEN_ORIGIN_TOO_LONG,
-  INDEXED_DB_LEVEL_DB_BACKING_STORE_OPEN_NO_RECOVERY,
-  INDEXED_DB_LEVEL_DB_BACKING_STORE_OPEN_MAX,
+enum IndexedDBBackingStoreOpenResult {
+  INDEXED_DB_BACKING_STORE_OPEN_MEMORY_SUCCESS,
+  INDEXED_DB_BACKING_STORE_OPEN_SUCCESS,
+  INDEXED_DB_BACKING_STORE_OPEN_FAILED_DIRECTORY,
+  INDEXED_DB_BACKING_STORE_OPEN_FAILED_UNKNOWN_SCHEMA,
+  INDEXED_DB_BACKING_STORE_OPEN_CLEANUP_DESTROY_FAILED,
+  INDEXED_DB_BACKING_STORE_OPEN_CLEANUP_REOPEN_FAILED,
+  INDEXED_DB_BACKING_STORE_OPEN_CLEANUP_REOPEN_SUCCESS,
+  INDEXED_DB_BACKING_STORE_OPEN_FAILED_IO_ERROR_CHECKING_SCHEMA,
+  INDEXED_DB_BACKING_STORE_OPEN_FAILED_UNKNOWN_ERR,
+  INDEXED_DB_BACKING_STORE_OPEN_MEMORY_FAILED,
+  INDEXED_DB_BACKING_STORE_OPEN_ATTEMPT_NON_ASCII,
+  INDEXED_DB_BACKING_STORE_OPEN_DISK_FULL,
+  INDEXED_DB_BACKING_STORE_OPEN_ORIGIN_TOO_LONG,
+  INDEXED_DB_BACKING_STORE_OPEN_NO_RECOVERY,
+  INDEXED_DB_BACKING_STORE_OPEN_MAX,
 };
 
 // TODO(dgrogan): Move to leveldb_env.
 bool RecoveryCouldBeFruitful(leveldb::Status status) {
   leveldb_env::MethodID method;
   int error = -1;
   leveldb_env::ErrorParsingResult result = leveldb_env::ParseMethodAndError(
       status.ToString().c_str(), &method, &error);
   switch (result) {
     case leveldb_env::NONE:
@@ skipping 34 matching lines @@
   *data_loss = WebKit::WebIDBCallbacks::DataLossNone;
   DefaultLevelDBFactory leveldb_factory;
   return IndexedDBBackingStore::Open(origin_identifier,
                                      path_base,
                                      file_identifier,
                                      data_loss,
                                      disk_full,
                                      &leveldb_factory);
 }
 
+static void HistogramOpenStatus(IndexedDBBackingStoreOpenResult result) {
+  UMA_HISTOGRAM_ENUMERATION("WebCore.IndexedDB.BackingStore.OpenStatus",
+                            result,
+                            INDEXED_DB_BACKING_STORE_OPEN_MAX);
+}
+
+// TODO(dgrogan): This only checks the lengths of the path components. It should
+// also check the total path length against MAX_PATH.

[jsbell, 2013/10/01 23:56:46]

I believe this is done implicitly by the call to GetMaximumPathComponentLength.
The function calculates the whole path limit as MAX_PATH - 1 (separator) -
prefix (the path specified, minus separator).

[dgrogan, 2013/10/02 00:54:24]

True. Comment removed.

+static bool IsPathTooLong(const base::FilePath& leveldb_dir) {
+  int limit = file_util::GetMaximumPathComponentLength(leveldb_dir);

[jsbell, 2013/10/01 23:56:46]

Based on the windows impl of GetMaximumPathComponentLength, I think this is
wrong - it will end up double-counting the length of the last component in the
path.

Previously, this was called with only path_base and identifier_path was appended
later. Now, leveldb_dir has both parts, which will count against the total. i.e.
we'll get limit = MAX_PATH - length(leveldb_dir), and then later on check that
length(leveldb_dir.BaseName()) < limit. If leveldb_dir was > half of MAX_PATH it
would always fail, whereas before this patch the test would pass.

[dgrogan, 2013/10/02 00:54:24]

Good catch. Changed back to only using the base directory name.

+  if (limit == -1) {
+    DLOG(WARNING) << "GetMaximumPathComponentLength returned -1";
+    // In limited testing, ChromeOS returns 143, other OSes 255.

[jsbell, 2013/10/01 23:56:46]

Bonus points if you can make clang-format not want to change the indentation of
this comment. (Or maybe we should file a clang-format bug.)

+#if defined(OS_CHROMEOS)
+    limit = 143;
+#else
+    limit = 255;
+#endif
+  }
+  size_t component_length = leveldb_dir.BaseName().value().length();
+  if (component_length > static_cast<uint32_t>(limit)) {
+    DLOG(WARNING) << "Path component length (" << component_length
+                  << ") exceeds maximum (" << limit
+                  << ") allowed by this filesystem.";
+    const int min = 140;
+    const int max = 300;
+    const int num_buckets = 12;
+    UMA_HISTOGRAM_CUSTOM_COUNTS(
+        "WebCore.IndexedDB.BackingStore.OverlyLargeOriginLength",
+        component_length,
+        min,
+        max,
+        num_buckets);
+    return true;
+  }
+  return false;
+}
+
 scoped_refptr<IndexedDBBackingStore> IndexedDBBackingStore::Open(
     const std::string& origin_identifier,
     const base::FilePath& path_base,
     const std::string& file_identifier,
     WebKit::WebIDBCallbacks::DataLoss* data_loss,
     bool* is_disk_full,
     LevelDBFactory* leveldb_factory) {
   IDB_TRACE("IndexedDBBackingStore::Open");
   DCHECK(!path_base.empty());
   *data_loss = WebKit::WebIDBCallbacks::DataLossNone;
   *is_disk_full = false;
 
   scoped_ptr<LevelDBComparator> comparator(new Comparator());
 
   if (!IsStringASCII(path_base.AsUTF8Unsafe())) {
-    base::Histogram::FactoryGet("WebCore.IndexedDB.BackingStore.OpenStatus",
-                                1,
-                                INDEXED_DB_LEVEL_DB_BACKING_STORE_OPEN_MAX,
-                                INDEXED_DB_LEVEL_DB_BACKING_STORE_OPEN_MAX + 1,
-                                base::HistogramBase::kUmaTargetedHistogramFlag)
-        ->Add(INDEXED_DB_LEVEL_DB_BACKING_STORE_OPEN_ATTEMPT_NON_ASCII);
+    HistogramOpenStatus(INDEXED_DB_BACKING_STORE_OPEN_ATTEMPT_NON_ASCII);
   }
   if (!file_util::CreateDirectory(path_base)) {
     LOG(ERROR) << "Unable to create IndexedDB database path "
                << path_base.AsUTF8Unsafe();
-    base::Histogram::FactoryGet("WebCore.IndexedDB.BackingStore.OpenStatus",
-                                1,
-                                INDEXED_DB_LEVEL_DB_BACKING_STORE_OPEN_MAX,
-                                INDEXED_DB_LEVEL_DB_BACKING_STORE_OPEN_MAX + 1,
-                                base::HistogramBase::kUmaTargetedHistogramFlag)
-        ->Add(INDEXED_DB_LEVEL_DB_BACKING_STORE_OPEN_FAILED_DIRECTORY);
+    HistogramOpenStatus(INDEXED_DB_BACKING_STORE_OPEN_FAILED_DIRECTORY);
     return scoped_refptr<IndexedDBBackingStore>();
   }
 
-  base::FilePath identifier_path =
-      base::FilePath().AppendASCII(origin_identifier)
+  base::FilePath file_path =
+      path_base.AppendASCII(origin_identifier)
           .AddExtension(FILE_PATH_LITERAL(".indexeddb.leveldb"));
 
-  int limit = file_util::GetMaximumPathComponentLength(path_base);
-  if (limit == -1) {
-    DLOG(WARNING) << "GetMaximumPathComponentLength returned -1";
-    // In limited testing, ChromeOS returns 143, other OSes 255.
-#if defined(OS_CHROMEOS)
-    limit = 143;
-#else
-    limit = 255;
-#endif
-  }
-  if (identifier_path.value().length() > static_cast<uint32_t>(limit)) {
-    DLOG(WARNING) << "Path component length ("
-                  << identifier_path.value().length() << ") exceeds maximum ("
-                  << limit << ") allowed by this filesystem.";
-    const int min = 140;
-    const int max = 300;
-    const int num_buckets = 12;
-    // TODO(dgrogan): Remove WebCore from these histogram names.
-    UMA_HISTOGRAM_CUSTOM_COUNTS(
-        "WebCore.IndexedDB.BackingStore.OverlyLargeOriginLength",
-        identifier_path.value().length(),
-        min,
-        max,
-        num_buckets);
-    // TODO(dgrogan): Translate the FactoryGet calls to
-    // UMA_HISTOGRAM_ENUMERATION. FactoryGet was the most direct translation
-    // from the WebCore code.
-    base::Histogram::FactoryGet("WebCore.IndexedDB.BackingStore.OpenStatus",
-                                1,
-                                INDEXED_DB_LEVEL_DB_BACKING_STORE_OPEN_MAX,
-                                INDEXED_DB_LEVEL_DB_BACKING_STORE_OPEN_MAX + 1,
-                                base::HistogramBase::kUmaTargetedHistogramFlag)
-        ->Add(INDEXED_DB_LEVEL_DB_BACKING_STORE_OPEN_ORIGIN_TOO_LONG);
+  if (IsPathTooLong(file_path)) {
+    HistogramOpenStatus(INDEXED_DB_BACKING_STORE_OPEN_ORIGIN_TOO_LONG);
     return scoped_refptr<IndexedDBBackingStore>();
   }
 
-  base::FilePath file_path = path_base.Append(identifier_path);
-
   scoped_ptr<LevelDBDatabase> db;
   leveldb::Status status = leveldb_factory->OpenLevelDB(
       file_path, comparator.get(), &db, is_disk_full);
 
   if (!status.ok() && leveldb_env::IndicatesDiskFull(status)) {
     DCHECK(!db);
     *is_disk_full = true;
   }
 
   if (db) {
     bool known = false;
     bool ok = IsSchemaKnown(db.get(), &known);
     if (!ok) {
       LOG(ERROR) << "IndexedDB had IO error checking schema, treating it as "
                     "failure to open";
-      base::Histogram::FactoryGet(
-          "WebCore.IndexedDB.BackingStore.OpenStatus",
-          1,
-          INDEXED_DB_LEVEL_DB_BACKING_STORE_OPEN_MAX,
-          INDEXED_DB_LEVEL_DB_BACKING_STORE_OPEN_MAX + 1,
-          base::HistogramBase::kUmaTargetedHistogramFlag)
-          ->Add(INDEXED_DB_LEVEL_DB_BACKING_STORE_OPEN_FAILED_IO_ERROR_CHECKING_SCHEMA);
+      HistogramOpenStatus(
+          INDEXED_DB_BACKING_STORE_OPEN_FAILED_IO_ERROR_CHECKING_SCHEMA);
       db.reset();
     } else if (!known) {
       LOG(ERROR) << "IndexedDB backing store had unknown schema, treating it "
                     "as failure to open";
-      base::Histogram::FactoryGet(
-          "WebCore.IndexedDB.BackingStore.OpenStatus",
-          1,
-          INDEXED_DB_LEVEL_DB_BACKING_STORE_OPEN_MAX,
-          INDEXED_DB_LEVEL_DB_BACKING_STORE_OPEN_MAX + 1,
-          base::HistogramBase::kUmaTargetedHistogramFlag)
-          ->Add(INDEXED_DB_LEVEL_DB_BACKING_STORE_OPEN_FAILED_UNKNOWN_SCHEMA);
+      HistogramOpenStatus(INDEXED_DB_BACKING_STORE_OPEN_FAILED_UNKNOWN_SCHEMA);
       db.reset();
     }
   }
 
   if (db) {
-    base::Histogram::FactoryGet("WebCore.IndexedDB.BackingStore.OpenStatus",
-                                1,
-                                INDEXED_DB_LEVEL_DB_BACKING_STORE_OPEN_MAX,
-                                INDEXED_DB_LEVEL_DB_BACKING_STORE_OPEN_MAX + 1,
-                                base::HistogramBase::kUmaTargetedHistogramFlag)
-        ->Add(INDEXED_DB_LEVEL_DB_BACKING_STORE_OPEN_SUCCESS);
+    HistogramOpenStatus(INDEXED_DB_BACKING_STORE_OPEN_SUCCESS);
   } else if (!RecoveryCouldBeFruitful(status)) {
     LOG(ERROR) << "Unable to open backing store, not trying to recover - "
                << status.ToString();
-    base::Histogram::FactoryGet("WebCore.IndexedDB.BackingStore.OpenStatus",
-                                1,
-                                INDEXED_DB_LEVEL_DB_BACKING_STORE_OPEN_MAX,
-                                INDEXED_DB_LEVEL_DB_BACKING_STORE_OPEN_MAX + 1,
-                                base::HistogramBase::kUmaTargetedHistogramFlag)
-        ->Add(INDEXED_DB_LEVEL_DB_BACKING_STORE_OPEN_NO_RECOVERY);
+    HistogramOpenStatus(INDEXED_DB_BACKING_STORE_OPEN_NO_RECOVERY);
     return scoped_refptr<IndexedDBBackingStore>();
   } else if (*is_disk_full) {
     LOG(ERROR) << "Unable to open backing store - disk is full.";
-    base::Histogram::FactoryGet("WebCore.IndexedDB.BackingStore.OpenStatus",
-                                1,
-                                INDEXED_DB_LEVEL_DB_BACKING_STORE_OPEN_MAX,
-                                INDEXED_DB_LEVEL_DB_BACKING_STORE_OPEN_MAX + 1,
-                                base::HistogramBase::kUmaTargetedHistogramFlag)
-        ->Add(INDEXED_DB_LEVEL_DB_BACKING_STORE_OPEN_DISK_FULL);
+    HistogramOpenStatus(INDEXED_DB_BACKING_STORE_OPEN_DISK_FULL);
     return scoped_refptr<IndexedDBBackingStore>();
   } else {
     LOG(ERROR) << "IndexedDB backing store open failed, attempting cleanup";
     *data_loss = WebKit::WebIDBCallbacks::DataLossTotal;
     bool success = leveldb_factory->DestroyLevelDB(file_path);
     if (!success) {
       LOG(ERROR) << "IndexedDB backing store cleanup failed";
-      base::Histogram::FactoryGet(
-          "WebCore.IndexedDB.BackingStore.OpenStatus",
-          1,
-          INDEXED_DB_LEVEL_DB_BACKING_STORE_OPEN_MAX,
-          INDEXED_DB_LEVEL_DB_BACKING_STORE_OPEN_MAX + 1,
-          base::HistogramBase::kUmaTargetedHistogramFlag)
-          ->Add(INDEXED_DB_LEVEL_DB_BACKING_STORE_OPEN_CLEANUP_DESTROY_FAILED);
+      HistogramOpenStatus(INDEXED_DB_BACKING_STORE_OPEN_CLEANUP_DESTROY_FAILED);
       return scoped_refptr<IndexedDBBackingStore>();
     }
 
     LOG(ERROR) << "IndexedDB backing store cleanup succeeded, reopening";
     leveldb_factory->OpenLevelDB(file_path, comparator.get(), &db, NULL);
     if (!db) {
       LOG(ERROR) << "IndexedDB backing store reopen after recovery failed";
-      base::Histogram::FactoryGet(
-          "WebCore.IndexedDB.BackingStore.OpenStatus",
-          1,
-          INDEXED_DB_LEVEL_DB_BACKING_STORE_OPEN_MAX,
-          INDEXED_DB_LEVEL_DB_BACKING_STORE_OPEN_MAX + 1,
-          base::HistogramBase::kUmaTargetedHistogramFlag)
-          ->Add(INDEXED_DB_LEVEL_DB_BACKING_STORE_OPEN_CLEANUP_REOPEN_FAILED);
+      HistogramOpenStatus(INDEXED_DB_BACKING_STORE_OPEN_CLEANUP_REOPEN_FAILED);
       return scoped_refptr<IndexedDBBackingStore>();
     }
-    base::Histogram::FactoryGet("WebCore.IndexedDB.BackingStore.OpenStatus",
-                                1,
-                                INDEXED_DB_LEVEL_DB_BACKING_STORE_OPEN_MAX,
-                                INDEXED_DB_LEVEL_DB_BACKING_STORE_OPEN_MAX + 1,
-                                base::HistogramBase::kUmaTargetedHistogramFlag)
-        ->Add(INDEXED_DB_LEVEL_DB_BACKING_STORE_OPEN_CLEANUP_REOPEN_SUCCESS);
+    HistogramOpenStatus(INDEXED_DB_BACKING_STORE_OPEN_CLEANUP_REOPEN_SUCCESS);
   }
 
   if (!db) {
     NOTREACHED();
-    base::Histogram::FactoryGet("WebCore.IndexedDB.BackingStore.OpenStatus",
-                                1,
-                                INDEXED_DB_LEVEL_DB_BACKING_STORE_OPEN_MAX,
-                                INDEXED_DB_LEVEL_DB_BACKING_STORE_OPEN_MAX + 1,
-                                base::HistogramBase::kUmaTargetedHistogramFlag)
-        ->Add(INDEXED_DB_LEVEL_DB_BACKING_STORE_OPEN_FAILED_UNKNOWN_ERR);
+    HistogramOpenStatus(INDEXED_DB_BACKING_STORE_OPEN_FAILED_UNKNOWN_ERR);
     return scoped_refptr<IndexedDBBackingStore>();
   }
 
   return Create(file_identifier, db.Pass(), comparator.Pass());
 }
 
 scoped_refptr<IndexedDBBackingStore> IndexedDBBackingStore::OpenInMemory(
     const std::string& file_identifier) {
   DefaultLevelDBFactory leveldb_factory;
   return IndexedDBBackingStore::OpenInMemory(file_identifier, &leveldb_factory);
 }
 
 scoped_refptr<IndexedDBBackingStore> IndexedDBBackingStore::OpenInMemory(
     const std::string& file_identifier,
     LevelDBFactory* leveldb_factory) {
   IDB_TRACE("IndexedDBBackingStore::OpenInMemory");
 
   scoped_ptr<LevelDBComparator> comparator(new Comparator());
   scoped_ptr<LevelDBDatabase> db =
       LevelDBDatabase::OpenInMemory(comparator.get());
   if (!db) {
     LOG(ERROR) << "LevelDBDatabase::OpenInMemory failed.";
-    base::Histogram::FactoryGet("WebCore.IndexedDB.BackingStore.OpenStatus",
-                                1,
-                                INDEXED_DB_LEVEL_DB_BACKING_STORE_OPEN_MAX,
-                                INDEXED_DB_LEVEL_DB_BACKING_STORE_OPEN_MAX + 1,
-                                base::HistogramBase::kUmaTargetedHistogramFlag)
-        ->Add(INDEXED_DB_LEVEL_DB_BACKING_STORE_OPEN_MEMORY_FAILED);
+    HistogramOpenStatus(INDEXED_DB_BACKING_STORE_OPEN_MEMORY_FAILED);
     return scoped_refptr<IndexedDBBackingStore>();
   }
-  base::Histogram::FactoryGet("WebCore.IndexedDB.BackingStore.OpenStatus",
-                              1,
-                              INDEXED_DB_LEVEL_DB_BACKING_STORE_OPEN_MAX,
-                              INDEXED_DB_LEVEL_DB_BACKING_STORE_OPEN_MAX + 1,
-                              base::HistogramBase::kUmaTargetedHistogramFlag)
-      ->Add(INDEXED_DB_LEVEL_DB_BACKING_STORE_OPEN_MEMORY_SUCCESS);
+  HistogramOpenStatus(INDEXED_DB_BACKING_STORE_OPEN_MEMORY_SUCCESS);
 
   return Create(file_identifier, db.Pass(), comparator.Pass());
 }
 
 scoped_refptr<IndexedDBBackingStore> IndexedDBBackingStore::Create(
     const std::string& identifier,
     scoped_ptr<LevelDBDatabase> db,
     scoped_ptr<LevelDBComparator> comparator) {
   // TODO(jsbell): Handle comparator name changes.
   scoped_refptr<IndexedDBBackingStore> backing_store(
@@ skipping 1951 matching lines @@
 }
 
 void IndexedDBBackingStore::Transaction::Rollback() {
   IDB_TRACE("IndexedDBBackingStore::Transaction::Rollback");
   DCHECK(transaction_.get());
   transaction_->Rollback();
   transaction_ = NULL;
 }
 
 }  // namespace content