Bug 77006 - [StorageTracker] Make one able to set the local storage (tracker) database dir's path
Summary: [StorageTracker] Make one able to set the local storage (tracker) database di...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 77107
  Show dependency treegraph
 
Reported: 2012-01-25 06:47 PST by Gustavo Lima Chaves
Modified: 2012-02-01 15:32 PST (History)
16 users (show)

See Also:


Attachments
Patch implementing proposed new local storage API (2.16 KB, patch)
2012-01-26 05:52 PST, Gustavo Lima Chaves
no flags Details | Formatted Diff | Diff
Patch (2.37 KB, patch)
2012-01-26 09:02 PST, Gustavo Lima Chaves
no flags Details | Formatted Diff | Diff
Patch (2.53 KB, patch)
2012-01-26 13:39 PST, Gustavo Lima Chaves
no flags Details | Formatted Diff | Diff
Patch (2.55 KB, patch)
2012-01-27 06:25 PST, Gustavo Lima Chaves
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gustavo Lima Chaves 2012-01-25 06:47:14 PST
Make one able to set the local storage (tracker) database dir's path
Comment 1 Gustavo Lima Chaves 2012-01-26 05:52:59 PST
Created attachment 124111 [details]
Patch implementing proposed new local storage API
Comment 2 WebKit Review Bot 2012-01-26 05:53:31 PST
Comment on attachment 124111 [details]
Patch implementing proposed new local storage API

Rejecting attachment 124111 [details] from review queue.

glima@profusion.mobi 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.
Comment 3 WebKit Review Bot 2012-01-26 05:54:11 PST
Comment on attachment 124111 [details]
Patch implementing proposed new local storage API

Rejecting attachment 124111 [details] from commit-queue.

glima@profusion.mobi 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.
Comment 4 Gustavo Lima Chaves 2012-01-26 09:02:30 PST
Created attachment 124125 [details]
Patch
Comment 5 Brady Eidson 2012-01-26 10:03:20 PST
Anton needs to take a look at this.
Comment 6 Brady Eidson 2012-01-26 10:04:40 PST
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?
Comment 7 Gustavo Lima Chaves 2012-01-26 10:25:12 PST
(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.
Comment 8 Brady Eidson 2012-01-26 10:39:42 PST
(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.
Comment 9 Anton D'Auria 2012-01-26 10:41:39 PST
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.
Comment 10 Anton D'Auria 2012-01-26 11:02:09 PST
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.
Comment 11 Anton D'Auria 2012-01-26 11:09:00 PST
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();
Comment 12 Gustavo Lima Chaves 2012-01-26 12:49:15 PST
(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.
Comment 13 Gustavo Lima Chaves 2012-01-26 13:39:11 PST
Created attachment 124172 [details]
Patch
Comment 14 Anton D'Auria 2012-01-26 13:48:53 PST
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();
}
Comment 15 Gustavo Lima Chaves 2012-01-27 06:25:29 PST
Created attachment 124304 [details]
Patch
Comment 16 Gustavo Lima Chaves 2012-01-31 05:11:15 PST
(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?
Comment 17 Anton D'Auria 2012-01-31 13:58:52 PST
(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.
Comment 18 Gustavo Lima Chaves 2012-02-01 07:38:55 PST
(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.
Comment 19 Anton D'Auria 2012-02-01 14:08:00 PST
(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+.
Comment 20 Darin Adler 2012-02-01 14:12:13 PST
Comment on attachment 124304 [details]
Patch

Based on Anton’s informal review and my own knowledge of the code, this looks fine, so review+
Comment 21 WebKit Review Bot 2012-02-01 15:32:50 PST
Comment on attachment 124304 [details]
Patch

Clearing flags on attachment: 124304

Committed r106506: <http://trac.webkit.org/changeset/106506>
Comment 22 WebKit Review Bot 2012-02-01 15:32:57 PST
All reviewed patches have been landed.  Closing bug.