Make one able to set the local storage (tracker) database dir's path
Created attachment 124111 [details] Patch implementing proposed new local storage API
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 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.
Created attachment 124125 [details] Patch
Anton needs to take a look at this.
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?
(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.
(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.
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.
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.
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();
(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.
Created attachment 124172 [details] Patch
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(); }
Created attachment 124304 [details] Patch
(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?
(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.
(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.
(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 on attachment 124304 [details] Patch Based on Anton’s informal review and my own knowledge of the code, this looks fine, so review+
Comment on attachment 124304 [details] Patch Clearing flags on attachment: 124304 Committed r106506: <http://trac.webkit.org/changeset/106506>
All reviewed patches have been landed. Closing bug.