Extract two methods by refactoring IndexedDBBackingStore::Open.

Extract two methods by refactoring IndexedDBBackingStore::Open.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=226528

[dgrogan, 2013-10-01 22:41:53]

Josh or Alec, could you review this?

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

not lgtm

If I'm reading this right, it looks like there's a Windows-specific problem with
how the usage of GetMaximumPathComponentLength is changed in this patch -
previously, was called with only the directory part of the path, now it's being
called with the whole path.

https://codereview.chromium.org/25541006/diff/1/content/browser/indexed_db/in...
File content/browser/indexed_db/indexed_db_backing_store.cc (right):

https://codereview.chromium.org/25541006/diff/1/content/browser/indexed_db/in...
content/browser/indexed_db/indexed_db_backing_store.cc:464: // also check the
total path length against MAX_PATH.
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).

https://codereview.chromium.org/25541006/diff/1/content/browser/indexed_db/in...
content/browser/indexed_db/indexed_db_backing_store.cc:466: int limit =
file_util::GetMaximumPathComponentLength(leveldb_dir);
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.

https://codereview.chromium.org/25541006/diff/1/content/browser/indexed_db/in...
content/browser/indexed_db/indexed_db_backing_store.cc:469: // In limited
testing, ChromeOS returns 143, other OSes 255.
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.)

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

Josh, ptal?

https://codereview.chromium.org/25541006/diff/1/content/browser/indexed_db/in...
File content/browser/indexed_db/indexed_db_backing_store.cc (right):

https://codereview.chromium.org/25541006/diff/1/content/browser/indexed_db/in...
content/browser/indexed_db/indexed_db_backing_store.cc:464: // also check the
total path length against MAX_PATH.
On 2013/10/01 23:56:46, jsbell wrote:
> 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).

True. Comment removed.

https://codereview.chromium.org/25541006/diff/1/content/browser/indexed_db/in...
content/browser/indexed_db/indexed_db_backing_store.cc:466: int limit =
file_util::GetMaximumPathComponentLength(leveldb_dir);
Good catch. Changed back to only using the base directory name.

[jsbell, 2013-10-02 15:44:00]

lgtm

[commit-bot: I haz the power, 2013-10-02 16:42:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dgrogan@chromium.org/25541006/6001

[commit-bot: I haz the power, 2013-10-02 18:35:08]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...

[commit-bot: I haz the power, 2013-10-02 18:43:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dgrogan@chromium.org/25541006/6001

[commit-bot: I haz the power, 2013-10-02 19:42:08]

Change committed as 226528