Summary: | Quota for IndexedDB should be per origin not per database | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jeremy Orlow <jorlow> | ||||||||||
Component: | New Bugs | Assignee: | Jeremy Orlow <jorlow> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | andreip, dglazkov, eric, joepeck, steveblock | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Other | ||||||||||||
OS: | OS X 10.5 | ||||||||||||
Attachments: |
|
Description
Jeremy Orlow
2010-10-21 05:49:31 PDT
Created attachment 71428 [details]
fix
Please review. Created attachment 71429 [details]
oops
Looks good, some small nits below: > WebCore/manual-tests/indexeddb-persists.html > var result = event.result.transaction([]).objectStore("test").get("key"); You don't need to pass the [] array. Not passing anything at all is equivalent. > WebCore/storage/IDBDatabaseBackendImpl.cpp > if (databaseQuery.prepare() != SQLResultOk) > ASSERT_NOT_REACHED(); why this and not (also) return false like we do in other error cases? > if (databaseQuery.step() == SQLResultRow) ditto > if (query.prepare() != SQLResultOk) > ASSERT_NOT_REACHED(); ditto > , m_id(-1) Make this a constant like we've done everywhere else? Attachment 71429 [details] did not build on chromium: Build output: http://queues.webkit.org/results/4718053 (In reply to comment #4) > Looks good, some small nits below: > > > > WebCore/manual-tests/indexeddb-persists.html > > var result = event.result.transaction([]).objectStore("test").get("key"); > > You don't need to pass the [] array. Not passing anything at all is equivalent. As specced today, I believe null implies dynamic transaction, which is not what we're looking for here. Hopefully we get this sorted out next week at TPAC. > > WebCore/storage/IDBDatabaseBackendImpl.cpp > > if (databaseQuery.prepare() != SQLResultOk) > > ASSERT_NOT_REACHED(); Done. > why this and not (also) return false like we do in other error cases? > > > if (databaseQuery.step() == SQLResultRow) > > ditto Because this isn't fatal. It's hardly even worth the check. > > if (query.prepare() != SQLResultOk) > > ASSERT_NOT_REACHED(); > > ditto Done. > > , m_id(-1) > > Make this a constant like we've done everywhere else? Will do. Created attachment 71761 [details]
Patch
(In reply to comment #7) > Created an attachment (id=71761) [details] > Patch LGTM Comment on attachment 71761 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=71761&action=review > WebCore/ChangeLog:23 > + Add a comment about the map by db name not origin? > WebCore/storage/IDBFactoryBackendImpl.cpp:122 > + String uniqueIdentifier = fileIdentifier + "_" + name; Use @ for safety? Comment on attachment 71761 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=71761&action=review >> WebCore/storage/IDBFactoryBackendImpl.cpp:122 >> + String uniqueIdentifier = fileIdentifier + "_" + name; > > Use @ for safety? If we allow underscores in the domain name, I think it's unsafe to use an underscore to join the two components, since databaseIdentifier() doesn't escape underscores ... http://www.foo.com, ".com_80_name" -> http_www.foo.com_80_.com_80_name http://www.foo.com_80_.com, "name" -> http_www.foo.com_80_.com_80_name Using '@' avoids the problem as the hostname can't contain an '@'. Created attachment 71869 [details]
Patch
(In reply to comment #10) > (From update of attachment 71761 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=71761&action=review > > >> WebCore/storage/IDBFactoryBackendImpl.cpp:122 > >> + String uniqueIdentifier = fileIdentifier + "_" + name; > > > > Use @ for safety? > > If we allow underscores in the domain name, I think it's unsafe to use an underscore to join the two components, since databaseIdentifier() doesn't escape underscores ... > > http://www.foo.com, ".com_80_name" -> http_www.foo.com_80_.com_80_name > http://www.foo.com_80_.com, "name" -> http_www.foo.com_80_.com_80_name > > Using '@' avoids the problem as the hostname can't contain an '@'. I agree this is better. Fixed other issue. Please take another look? Comment on attachment 71869 [details]
Patch
r=me
Comment on attachment 71869 [details] Patch Clearing flags on attachment: 71869 Committed r70522: <http://trac.webkit.org/changeset/70522> All reviewed patches have been landed. Closing bug. |