Bug 46090 - [chromium] expose the filename used for a given indexed DB
Summary: [chromium] expose the filename used for a given indexed DB
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: jochen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-09-20 07:52 PDT by jochen
Modified: 2010-09-21 04:16 PDT (History)
6 users (show)

See Also:


Attachments
Patch (6.63 KB, patch)
2010-09-20 07:53 PDT, jochen
no flags Details | Formatted Diff | Diff
Patch (7.46 KB, patch)
2010-09-20 08:12 PDT, jochen
no flags Details | Formatted Diff | Diff
Patch (6.83 KB, patch)
2010-09-20 08:22 PDT, jochen
no flags Details | Formatted Diff | Diff
Patch (6.81 KB, patch)
2010-09-20 23:49 PDT, jochen
no flags Details | Formatted Diff | Diff
Patch (6.81 KB, patch)
2010-09-21 01:08 PDT, jochen
no flags Details | Formatted Diff | Diff
Patch (6.89 KB, patch)
2010-09-21 02:22 PDT, jochen
no flags Details | Formatted Diff | Diff
Patch (6.93 KB, patch)
2010-09-21 02:53 PDT, jochen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description jochen 2010-09-20 07:52:01 PDT
[chromium] expose the filename used for a given indexed DB
Comment 1 jochen 2010-09-20 07:53:27 PDT
Created attachment 68082 [details]
Patch
Comment 2 Jeremy Orlow 2010-09-20 08:00:06 PDT
Comment on attachment 68082 [details]
Patch

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

> WebCore/storage/IDBFactoryBackendInterface.cpp:48
> +String IDBFactoryBackendInterface::fileNameForIDB(const String& name, SecurityOrigin* securityOrigin)

Let's not duplicate code.  See below comment.

> WebCore/storage/IDBFactoryBackendInterface.h:57
> +    static String fileNameForIDB(const String& name, SecurityOrigin*);

Put this on the Impl.h/cpp since the interface.cpp has a chromium and a non-chromium version.  It's fine to leave the WebKit side of things as is tho.

> WebCore/storage/chromium/IDBFactoryBackendInterface.cpp:32
> +#include "FileSystem.h"

remove (per my above comment)

> WebCore/storage/chromium/IDBFactoryBackendInterface.cpp:44
> +String IDBFactoryBackendInterface::fileNameForIDB(const String& name, SecurityOrigin* securityOrigin)

remove (per my above comment)
Comment 3 WebKit Review Bot 2010-09-20 08:06:48 PDT
Attachment 68082 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4015054
Comment 4 jochen 2010-09-20 08:12:29 PDT
Created attachment 68086 [details]
Patch
Comment 5 Jeremy Orlow 2010-09-20 08:16:56 PDT
Comment on attachment 68086 [details]
Patch

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

> WebCore/storage/IDBFactoryBackendInterface.cpp:31
> +#include "FileSystem.h"

revert this file

> WebKit/chromium/public/WebIDBFactory.h:68
> +    WEBKIT_API static WebString fileNameForIDB(const WebString& name, const WebSecurityOrigin& origin);

databaseFileName seems more clear.  Also use that name in WebCore?

> WebKit/chromium/src/WebIDBFactory.cpp:36
> +#if ENABLE(INDEXED_DATABASE)

move this above the #include (the latest style these days)

> WebKit/chromium/src/WebIDBFactory.cpp:42
> +// static

remove comment


Looks close.
Comment 6 WebKit Review Bot 2010-09-20 08:20:03 PDT
Attachment 68086 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4002068
Comment 7 jochen 2010-09-20 08:22:50 PDT
Created attachment 68087 [details]
Patch
Comment 8 Jeremy Orlow 2010-09-20 08:23:42 PDT
Comment on attachment 68087 [details]
Patch

r=me
Comment 9 Darin Fisher (:fishd, Google) 2010-09-20 09:04:21 PDT
Comment on attachment 68087 [details]
Patch

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

> WebKit/chromium/public/WebIDBFactory.h:68
> +    WEBKIT_API static WebString databaseFileName(const WebString& name, const WebSecurityOrigin& origin);

nit: no need for the "origin" parameter name since it doesn't add any information.

> WebKit/chromium/src/WebIDBFactory.cpp:44
> +    return WebString(IDBFactoryBackendImpl::databaseFileName(name, origin));

nit: you should not need to call the WebString constructor explicitly here.
Comment 10 Jeremy Orlow 2010-09-20 09:08:21 PDT
(In reply to comment #9)
> (From update of attachment 68087 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=68087&action=review
> 
> > WebKit/chromium/public/WebIDBFactory.h:68
> > +    WEBKIT_API static WebString databaseFileName(const WebString& name, const WebSecurityOrigin& origin);
> 
> nit: no need for the "origin" parameter name since it doesn't add any information.
> 
> > WebKit/chromium/src/WebIDBFactory.cpp:44
> > +    return WebString(IDBFactoryBackendImpl::databaseFileName(name, origin));
> 
> nit: you should not need to call the WebString constructor explicitly here.

Ugh.  Stupid me on both counts.  Sorry.
Comment 11 WebKit Commit Bot 2010-09-20 09:12:40 PDT
Comment on attachment 68087 [details]
Patch

Rejecting patch 68087 from commit-queue.

Unexpected failure when processing patch!  Please file a bug against webkit-patch.
Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', 'land-attachment', '--force-clean', '--non-interactive', '--ignore-builders', '--quiet', '--parent-command=commit-queue', 68087]" exit_code: 1
Last 500 characters of output:
.cgi?id=68087&action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=46090&ctype=xml
Processing 1 patch from 1 bug.
Cleaning working directory
Updating working directory
Processing patch 68087 from bug 46090.
NOBODY (OOPS!) found in /Projects/CommitQueue/WebCore/ChangeLog does not appear to be a valid reviewer according to committers.py.
ERROR: /Projects/CommitQueue/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).
Comment 12 Adam Barth 2010-09-20 10:49:07 PDT
> Rejecting patch 68087 from commit-queue.

Sorry, this is a new bug in the commit-queue now that it's using multiple bots.  Will fix soon.
Comment 13 Jeremy Orlow 2010-09-20 10:52:25 PDT
(In reply to comment #12)
> > Rejecting patch 68087 from commit-queue.
> 
> Sorry, this is a new bug in the commit-queue now that it's using multiple bots.  Will fix soon.

Well, the fact that it didn't still go ahead and commit it after cq- seems to be an improvement from before!  (In which case it could commit a patch 10+ minutes after it was cq-ed.)

Anyway, Jochen, can you roll another patch?

Thanks!
Comment 14 Eric Seidel (no email) 2010-09-20 12:56:25 PDT
This should be fixed by bug 32679.
Comment 15 jochen 2010-09-20 23:49:55 PDT
Created attachment 68195 [details]
Patch
Comment 16 WebKit Review Bot 2010-09-21 01:02:40 PDT
Attachment 68195 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4101005
Comment 17 jochen 2010-09-21 01:08:37 PDT
Created attachment 68200 [details]
Patch
Comment 18 WebKit Review Bot 2010-09-21 01:19:08 PDT
Attachment 68200 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4097009
Comment 19 Jeremy Orlow 2010-09-21 02:18:35 PDT
Comment on attachment 68200 [details]
Patch

r=me
Comment 20 jochen 2010-09-21 02:22:07 PDT
Created attachment 68202 [details]
Patch
Comment 21 Jeremy Orlow 2010-09-21 02:24:00 PDT
Comment on attachment 68202 [details]
Patch

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

> WebKit/chromium/src/WebIDBFactory.cpp:45
> +    return IDBFactoryBackendImpl::databaseFileName(name, static_cast<PassRefPtr<SecurityOrigin> >(origin).get());

noooooo

You probably just need to include WebSecurityOrigin at the top.
Comment 22 jochen 2010-09-21 02:27:00 PDT
(In reply to comment #21)
> (From update of attachment 68202 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=68202&action=review
> 
> > WebKit/chromium/src/WebIDBFactory.cpp:45
> > +    return IDBFactoryBackendImpl::databaseFileName(name, static_cast<PassRefPtr<SecurityOrigin> >(origin).get());
> 
> noooooo
> 
> You probably just need to include WebSecurityOrigin at the top.

that header is already included from WebIDBFactory.h

WebSecurityOrigin defines an PassRefPtr<SecurityOrigin> operator(), the method databaseFileName wants to have a SecurityOrigin*

an alternative would be to change IDBFactoryBackendImpl.cpp:openSqliteDatabase to take a PassRefPtr instead of a SecurityOrigin*, then databaseFileName could also take a PassRefPtr?
Comment 23 jochen 2010-09-21 02:53:06 PDT
Created attachment 68203 [details]
Patch
Comment 24 Jeremy Orlow 2010-09-21 02:57:37 PDT
Comment on attachment 68203 [details]
Patch

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

please change....but looks good  :-)

> WebKit/chromium/src/WebIDBFactory.cpp:46
> +    securityOrigin = origin;

just combine these two lines?
Comment 25 jochen 2010-09-21 03:14:56 PDT
(In reply to comment #24)
> (From update of attachment 68203 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=68203&action=review
> 
> please change....but looks good  :-)
> 
> > WebKit/chromium/src/WebIDBFactory.cpp:46
> > +    securityOrigin = origin;
> 
> just combine these two lines?

That would require an explicit static_cast to PassRefPtr<SecurityOrigin>: when the compiler tries to figure out which RefPtr template to use, it doesn't take into account implicit conversion operators
Comment 26 WebKit Commit Bot 2010-09-21 04:15:54 PDT
Comment on attachment 68203 [details]
Patch

Clearing flags on attachment: 68203

Committed r67941: <http://trac.webkit.org/changeset/67941>
Comment 27 WebKit Commit Bot 2010-09-21 04:16:01 PDT
All reviewed patches have been landed.  Closing bug.