As soon as a page attempts to use localstorage, StorageAreaSync will create an empty database if one does not already exist. This can lead to lots of unnecessary database files. In particular, they are created even when the privacy settings or private browsing mode disallow localstorage data, which may seem odd to the user. For example, visit http://people.w3.org/mike/localstorage.html and note that ~/Library/Safari/LocalStorage/http_people.w3.org_0.localstorage is created even though no attempt to store any data has been made. Database creation should be put off in StorageAreaSync until it is time to actually write something to the database.
Created attachment 58140 [details] Patch
Comment on attachment 58140 [details] Patch WebCore/storage/StorageAreaSync.cpp:306 + if (!m_database.isOpen()) Is it safe to do this? What happens if it fails the first 2 times (the HD is full or something) but later starts succeeding. Please verify it'll do the right thing. I'd lean towards a m_initialized flag. But is any of this needed? Isn't the initial import guaranteed to run first? WebCore/storage/StorageAreaSync.cpp:202 + void StorageAreaSync::openDatabase(bool create) Create an enum instead of a bool for create and don't create. It makes code easier to read. WebCore/ChangeLog:12 + Tested by visiting e.g. http://people.w3.org/mike/localstorage.html and verifying that no file is created in ~/Library/Safari/LocalStorage until something is stored. Create a manual-test in WebCore/manual-tests
(In reply to comment #2) > (From update of attachment 58140 [details]) > WebCore/storage/StorageAreaSync.cpp:306 > + if (!m_database.isOpen()) > Is it safe to do this? What happens if it fails the first 2 times (the HD is full or something) but later starts succeeding. Please verify it'll do the right thing. It's not safe in that if it first fails to open the database, and later starts succeeding, it will have dropped some sync items and the database contents might not correctly represent the state of the storage. Changing so that if it fails to open the database once, it will not retry. > > I'd lean towards a m_initialized flag. > > But is any of this needed? Isn't the initial import guaranteed to run first? > Yes, but the initial import no longer always opens the database; that's why it's needed. > WebCore/storage/StorageAreaSync.cpp:202 > + void StorageAreaSync::openDatabase(bool create) > Create an enum instead of a bool for create and don't create. It makes code easier to read. Done. Suggestions for better naming welcome. > > WebCore/ChangeLog:12 > + Tested by visiting e.g. http://people.w3.org/mike/localstorage.html and verifying that no file is created in ~/Library/Safari/LocalStorage until something is stored. > Create a manual-test in WebCore/manual-tests Done.
Created attachment 58154 [details] Patch
Comment on attachment 58154 [details] Patch WebCore/storage/StorageAreaSync.cpp:66 + , m_triedOpeningDatabase(false) How about m_databaseOpenFailed WebCore/storage/StorageAreaSync.cpp:206 + ASSERT(!m_database.isOpen()); Assert we haven't tried yet. WebCore/storage/StorageAreaSync.h:79 + CreateDatabase, These names are not very clear. The point is that creation is allowed, but this sounds like you're saying always create it. WebCore/storage/StorageAreaSync.cpp:307 + if (!m_database.isOpen() && !m_triedOpeningDatabase) If you change the name as I suggested, then you can simply do "if m_databaseOpenFailed return;" before this statement. I think it's more clear.
(In reply to comment #5) > (From update of attachment 58154 [details]) > WebCore/storage/StorageAreaSync.cpp:66 > + , m_triedOpeningDatabase(false) > How about m_databaseOpenFailed Done. > WebCore/storage/StorageAreaSync.cpp:206 > + ASSERT(!m_database.isOpen()); > Assert we haven't tried yet. Done. > WebCore/storage/StorageAreaSync.h:79 > + CreateDatabase, > These names are not very clear. The point is that creation is allowed, but this sounds like you're saying always create it. Done. Trying new names. > WebCore/storage/StorageAreaSync.cpp:307 > + if (!m_database.isOpen() && !m_triedOpeningDatabase) > If you change the name as I suggested, then you can simply do "if m_databaseOpenFailed return;" before this statement. I think it's more clear. Done.
Created attachment 58233 [details] Patch
Comment on attachment 58233 [details] Patch WebCore/ChangeLog:8 + As soon as a page attempts to use localstorage, StorageAreaSync will create an empty database if one doesn't already exist. This can lead to lots of unnecessary database files. In particular, they are created even when the privacy settings or private browsing mode disallow localstorage data, which may seem odd to the user. This is all on one line. Please wrap. WebCore/manual-tests/localstorage-empty-database.html:6 + </body> Why end the body before the body? :-) WebCore/storage/StorageAreaSync.cpp:203 + void StorageAreaSync::openDatabase(OpenDatabaseParamType create) Create is probably not the best name for this.
(In reply to comment #8) > (From update of attachment 58233 [details]) > WebCore/ChangeLog:8 > + As soon as a page attempts to use localstorage, StorageAreaSync will create an empty database if one doesn't already exist. This can lead to lots of unnecessary database files. In particular, they are created even when the privacy settings or private browsing mode disallow localstorage data, which may seem odd to the user. > This is all on one line. Please wrap. Done. > WebCore/manual-tests/localstorage-empty-database.html:6 > + </body> > Why end the body before the body? :-) Fixed. > WebCore/storage/StorageAreaSync.cpp:203 > + void StorageAreaSync::openDatabase(OpenDatabaseParamType create) > Create is probably not the best name for this. Trying new name.
Created attachment 58463 [details] Patch
Comment on attachment 58463 [details] Patch r=me
Comment on attachment 58463 [details] Patch Clearing flags on attachment: 58463 Committed r61023: <http://trac.webkit.org/changeset/61023>
All reviewed patches have been landed. Closing bug.