Bug 112224 - IndexedDB: Record attempts to open a path with non-ascii characters
Summary: IndexedDB: Record attempts to open a path with non-ascii characters
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Grogan
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-03-12 20:20 PDT by David Grogan
Modified: 2013-03-13 15:01 PDT (History)
5 users (show)

See Also:

Attachments
Patch (2.23 KB, patch)
2013-03-12 20:21 PDT, David Grogan 		no flags Details | Formatted Diff | Diff
Patch (2.30 KB, patch)
2013-03-12 20:22 PDT, David Grogan 		no flags Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description David Grogan 2013-03-12 20:20:06 PDT 
IndexedDB: Record when we try to open a path with non-ascii characters
Comment 1 David Grogan 2013-03-12 20:21:33 PDT 
Created attachment 192861 [details]
Patch
Comment 2 David Grogan 2013-03-12 20:22:46 PDT 
Created attachment 192862 [details]
Patch
Comment 3 David Grogan 2013-03-12 20:26:13 PDT 
Josh, could you take a look at this?
Comment 4 Joshua Bell 2013-03-12 21:45:30 PDT 
Comment on attachment 192862 [details]
Patch

Looks fine. We should also do local testing on Windows to rule out anything obvious.
Comment 5 Mike West 2013-03-13 04:56:59 PDT 
Comment on attachment 192862 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=192862&action=review

Quick drive-by comment.

> Source/WebCore/Modules/indexeddb/IDBBackingStore.cpp:396
> +            HistogramSupport::histogramEnumeration("WebCore.IndexedDB.BackingStore.OpenStatus", IDBLevelDBBackingStoreOpenAttemptNonASCII, IDBLevelDBBackingStoreOpenMax);

It looks like you're calling `HistogramSupport::histogramEnumeration("WebCore.IndexedDB.BackingStore.OpenStatus", ???, IDBLevelDBBackingStoreOpenMax);` ~10 times in this file. It might be worth creating a static helper function for clarity, similar to what you've already done with `recordInternalError`.
Comment 6 Mike West 2013-03-13 04:57:15 PDT 
(In reply to comment #5)
> (From update of attachment 192862 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=192862&action=review
> 
> Quick drive-by comment.
> 
> > Source/WebCore/Modules/indexeddb/IDBBackingStore.cpp:396
> > +            HistogramSupport::histogramEnumeration("WebCore.IndexedDB.BackingStore.OpenStatus", IDBLevelDBBackingStoreOpenAttemptNonASCII, IDBLevelDBBackingStoreOpenMax);
> 
> It looks like you're calling `HistogramSupport::histogramEnumeration("WebCore.IndexedDB.BackingStore.OpenStatus", ???, IDBLevelDBBackingStoreOpenMax);` ~10 times in this file. It might be worth creating a static helper function for clarity, similar to what you've already done with `recordInternalError`.

(in a future patch, of course... :) )
Comment 7 David Grogan 2013-03-13 12:08:28 PDT 
(In reply to comment #4)
> (From update of attachment 192862 [details])
> Looks fine.

> We should also do local testing on Windows to rule out anything obvious.

I did my best with rdesktop, http://download-chromium.appspot.com/ and --enable-dcheck. cursor-advance.html ran fine, I shutdown and reopened, cursor-advance ran fine again. This isn't a perfect test because, among other reasons, cursor-advance starts with a a deleteDatabase. Though deleteDatabase goes through the IDBBackingStore::open path, so it probably would have caught anything obvious.
Comment 8 David Grogan 2013-03-13 12:09:04 PDT 
Comment on attachment 192862 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=192862&action=review

>>> Source/WebCore/Modules/indexeddb/IDBBackingStore.cpp:396
>>> +            HistogramSupport::histogramEnumeration("WebCore.IndexedDB.BackingStore.OpenStatus", IDBLevelDBBackingStoreOpenAttemptNonASCII, IDBLevelDBBackingStoreOpenMax);
>> 
>> It looks like you're calling `HistogramSupport::histogramEnumeration("WebCore.IndexedDB.BackingStore.OpenStatus", ???, IDBLevelDBBackingStoreOpenMax);` ~10 times in this file. It might be worth creating a static helper function for clarity, similar to what you've already done with `recordInternalError`.
> 
> (in a future patch, of course... :) )

You're right, readers would have an easier time realizing that it all goes to the same place.
Comment 9 David Grogan 2013-03-13 12:12:46 PDT 
Tony, could you review this?
Comment 10 WebKit Review Bot 2013-03-13 15:01:38 PDT 
Comment on attachment 192862 [details]
Patch

Clearing flags on attachment: 192862

Committed r145760: <http://trac.webkit.org/changeset/145760>
Comment 11 WebKit Review Bot 2013-03-13 15:01:42 PDT 
All reviewed patches have been landed.  Closing bug.