Bug 46090 – [chromium] expose the filename used for a given indexed DB

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[chromium] expose the filename used for a given indexed DB

Created attachment 68082 [details]
Patch

[Jeremy Orlow]

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)

[WebKit Review Bot]

Attachment 68082 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4015054

Created attachment 68086 [details]
Patch

[Jeremy Orlow]

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.

[WebKit Review Bot]

Attachment 68086 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4002068

Created attachment 68087 [details]
Patch

[Jeremy Orlow]

Comment on attachment 68087 [details]
Patch

r=me

[Darin Fisher (:fishd, Google)]

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.

[Jeremy Orlow]

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

[WebKit Commit Bot]

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

[Adam Barth]

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

[Jeremy Orlow]

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

[Eric Seidel (no email)]

This should be fixed by bug 32679.

Created attachment 68195 [details]
Patch

[WebKit Review Bot]

Attachment 68195 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4101005

Created attachment 68200 [details]
Patch

[WebKit Review Bot]

Attachment 68200 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4097009

[Jeremy Orlow]

Comment on attachment 68200 [details]
Patch

r=me

Created attachment 68202 [details]
Patch

[Jeremy Orlow]

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.

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

Created attachment 68203 [details]
Patch

[Jeremy Orlow]

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?

(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

[WebKit Commit Bot]

Comment on attachment 68203 [details]
Patch

Clearing flags on attachment: 68203

Committed r67941: <http://trac.webkit.org/changeset/67941>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.