Bug 77006 – [StorageTracker] Make one able to set the local storage (tracker) database dir's path

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Gustavo Lima Chaves]

Make one able to set the local storage (tracker) database dir's path

[Gustavo Lima Chaves]

Created attachment 124111 [details]
Patch implementing proposed new local storage API

[WebKit Review Bot]

Comment on attachment 124111 [details]
Patch implementing proposed new local storage API

Rejecting attachment 124111 [details] from review queue.

[e-mail] does not have reviewer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have reviewer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have reviewer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your reviewer rights.

[WebKit Review Bot]

Comment on attachment 124111 [details]
Patch implementing proposed new local storage API

Rejecting attachment 124111 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[Gustavo Lima Chaves]

Created attachment 124125 [details]
Patch

[Brady Eidson]

Anton needs to take a look at this.

[Brady Eidson]

Comment on attachment 124125 [details]
Patch

We don't normally check in code that is never exercised anywhere.

You say you want to add this as API.  WebCore is not an API layer.  Where is the new API?

Why are we doing this?

[Gustavo Lima Chaves]

(In reply to comment #6)
> (From update of attachment 124125 [details])
> We don't normally check in code that is never exercised anywhere.
> 
> You say you want to add this as API.  WebCore is not an API layer.  Where is the new API?
> 
> Why are we doing this?

See the bug this one is blocking for usage. Sorry, by API I meant internal webcore interface, to be used by webkit ports. We are doing this because this new call makes sense for a port -- the EFL one.

[Brady Eidson]

(In reply to comment #7)
> (In reply to comment #6)
> > (From update of attachment 124125 [details] [details])
> > We don't normally check in code that is never exercised anywhere.
> > 
> > You say you want to add this as API.  WebCore is not an API layer.  Where is the new API?
> > 
> > Why are we doing this?
> 
> See the bug this one is blocking for usage. Sorry, by API I meant internal webcore interface, to be used by webkit ports. We are doing this because this new call makes sense for a port -- the EFL one.

I am extremely dubious that it is as simple as this patch here.

And we still wouldn't want new code checked in that is "dead" like this patch would be by itself.

Still waiting on Anton who wrote the StorageTracker initially and is the most intimately familiar with it.

[Anton D'Auria]

From the blocking bug (77107), it looks like what you'd like to do is control the LocalStorage directory. StorageTracker shouldn't change LocalStorage directory. It needs to know what it is in order to manage its contents. StorageTracker is initialized in the Mac port with the LocalStorage directory. If you wish to use StorageTracker in the EFL port, you should pass the LocalStorage path to StorageTracker::initializeTracker. 

I don't see why you need to modify WebCore.

[Anton D'Auria]

If you want to change LocalStorage paths after StorageTracker initialization, then you'll need to clear m_originSet.

In setDatabaseDirectoryPath, you'll need to add:

{
    MutexLock lock(m_originSetGuard);
    m_originSet.clear();
}

Then, you can call importOriginIdentifiers(). importOriginIdentifiers writes to m_originSet and will lock m_originSetGuard again at that point.

[Anton D'Auria]

You'll need to return after saving the path string and clearing m_originSet, but before doing any importing.

// save new path
// lock m_originSetGuard and clear m_origin

if (!m_isActive)
    return;

importOriginIdentifiers();

[Gustavo Lima Chaves]

(In reply to comment #11)
> You'll need to return after saving the path string and clearing m_originSet, but before doing any importing.
> 
> // save new path
> // lock m_originSetGuard and clear m_origin
> 
> if (!m_isActive)
>     return;
> 
> importOriginIdentifiers();

Thanks for the feedback on the logic, since you're giving them, I'll assume you get our point/reason a little :) This thing is we call initializeTracker() in our port, too, with "~/.webkit". But that locks us down to a pre-set folder and, what we want is the browser being able to choose that. DatabaseTracker does that. I'll re-submit the patch with the corrections pointed out, thanks muchly.

[Gustavo Lima Chaves]

Created attachment 124172 [details]
Patch

[Anton D'Auria]

Comment on attachment 124172 [details]
Patch

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

> Source/WebCore/storage/StorageTracker.cpp:101
> +    if (!m_database.isOpen())

you mean, if (m_database.isOpen())

> Source/WebCore/storage/StorageTracker.cpp:107
> +    MutexLocker lockOrigins(m_originSetGuard);
> +    m_originSet.clear();

m_originSetGuard needs to be unlocked because it's locked again in importOriginIdentifiers()
this will work:
{
    MutexLocker lockOrigins(m_originSetGuard);
    m_originSet.clear();
}

[Gustavo Lima Chaves]

Created attachment 124304 [details]
Patch

[Gustavo Lima Chaves]

(In reply to comment #14)
> > Source/WebCore/storage/StorageTracker.cpp:101
> > +    if (!m_database.isOpen())
> 
> you mean, if (m_database.isOpen())

Oh, sorry, fixed.

> m_originSetGuard needs to be unlocked because it's locked again in importOriginIdentifiers()
> this will work:
> {
>     MutexLocker lockOrigins(m_originSetGuard);
>     m_originSet.clear();
> }

Yeah, got this lock-unlock idiom, now. Thanks for pointing out.
Is the patch acceptable now?

[Anton D'Auria]

(In reply to comment #16)
> (In reply to comment #14)
> > > Source/WebCore/storage/StorageTracker.cpp:101
> > > +    if (!m_database.isOpen())
> > 
> > you mean, if (m_database.isOpen())
> 
> Oh, sorry, fixed.
> 
> > m_originSetGuard needs to be unlocked because it's locked again in importOriginIdentifiers()
> > this will work:
> > {
> >     MutexLocker lockOrigins(m_originSetGuard);
> >     m_originSet.clear();
> > }
> 
> Yeah, got this lock-unlock idiom, now. Thanks for pointing out.
> Is the patch acceptable now?

It looks like you didn't test the previous patch before submitting it for review :) Can you please make sure this one is tested? The patch looks good to me.

[Gustavo Lima Chaves]

(In reply to comment #17)

> It looks like you didn't test the previous patch before submitting it for review :) Can you please make sure this one is tested? The patch looks good to me.

Actually I had tested with HEAD, at the time, and both creation and removal of the tracker db on a custom location was working fine. Just did it now with current HEAD and it works like a charm. If you meant any test suite inclusion, just let me know.

[Anton D'Auria]

(In reply to comment #18)
> (In reply to comment #17)
> 
> > It looks like you didn't test the previous patch before submitting it for review :) Can you please make sure this one is tested? The patch looks good to me.
> 
> Actually I had tested with HEAD, at the time, and both creation and removal of the tracker db on a custom location was working fine. Just did it now with current HEAD and it works like a charm. If you meant any test suite inclusion, just let me know.

The patch looks good, so I'm giving an unofficial r+.

[Darin Adler]

Comment on attachment 124304 [details]
Patch

Based on Anton’s informal review and my own knowledge of the code, this looks fine, so review+

[WebKit Review Bot]

Comment on attachment 124304 [details]
Patch

Clearing flags on attachment: 124304

Committed r106506: <http://trac.webkit.org/changeset/106506>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.