Bug 112224 – IndexedDB: Record attempts to open a path with non-ascii characters

• Component: New Bugs
• Status: RESOLVED FIXED

[David Grogan]

IndexedDB: Record when we try to open a path with non-ascii characters

[David Grogan]

Created attachment 192861 [details]
Patch

[David Grogan]

Created attachment 192862 [details]
Patch

[David Grogan]

Josh, could you take a look at this?

[Joshua Bell]

Comment on attachment 192862 [details]
Patch

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

[Mike West]

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`.

[Mike West]

(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... :) )

[David Grogan]

(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.

[David Grogan]

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.

[David Grogan]

Tony, could you review this?

[WebKit Review Bot]

Comment on attachment 192862 [details]
Patch

Clearing flags on attachment: 192862

Committed r145760: <http://trac.webkit.org/changeset/145760>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.