Bug 225435 – Drop some unnecessary code in LocalStorageDatabase

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Sihui Liu]

...

[Sihui Liu]

Created attachment 427851 [details]
Patch

[youenn fablet]

Comment on attachment 427851 [details]
Patch

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

> Source/WebKit/NetworkProcess/WebStorage/LocalStorageDatabase.cpp:-83
> -        m_tracker->didOpenDatabaseWithOrigin(m_securityOrigin);

didOpenDatabaseWithOrigin callback is no longer called.
It does not seem that it does much right now but maybe it will in the future?

> Source/WebKit/NetworkProcess/WebStorage/LocalStorageDatabase.cpp:296
> +        SQLiteFileSystem::deleteDatabaseFile(m_databasePath);

Ditto here. Should we remove deleteDatabaseWithOrigin?

> Source/WebKit/NetworkProcess/WebStorage/StorageArea.cpp:183
> +        m_localStorageDatabase = LocalStorageDatabase::create(databasePath, m_quotaInBytes);

Could use WTFMove and take a String&&

[youenn fablet]

Might be good to provide additional details in change log as well

[Chris Dumez]

Comment on attachment 427851 [details]
Patch

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

> Source/WebKit/NetworkProcess/WebStorage/LocalStorageDatabase.cpp:-151
> -        transaction.rollback();

Why are we dropping this? This seems like a potential change in behavior but the ChangeLog says nothing about this.

> Source/WebKit/NetworkProcess/WebStorage/StorageArea.cpp:181
> +        auto* localStorageDatabaseTracker = m_localStorageNamespace->storageManager()->localStorageDatabaseTracker();

The previous code used to assume his was non-null but now you are null-checking it. Which is right?

[Sihui Liu]

(In reply to youenn fablet from comment #2)
> Comment on attachment 427851 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=427851&action=review
> 
> > Source/WebKit/NetworkProcess/WebStorage/LocalStorageDatabase.cpp:-83
> > -        m_tracker->didOpenDatabaseWithOrigin(m_securityOrigin);
> 
> didOpenDatabaseWithOrigin callback is no longer called.
> It does not seem that it does much right now but maybe it will in the future?
> 

The function is a no-op, and LocalStorageDatabaseTracker does not have client (like the comment in LocalStorageDatabaseTracker::didOpenDatabaseWithOrigin). LocalStorageDatabaseTracker used to track all origins in memory so we had the function, but it does not any more. We may just remove that function in LocalStorageDatabaseTracker but want to limit this patch to LocalStorageDatabase.

> > Source/WebKit/NetworkProcess/WebStorage/LocalStorageDatabase.cpp:296
> > +        SQLiteFileSystem::deleteDatabaseFile(m_databasePath);
> 
> Ditto here. Should we remove deleteDatabaseWithOrigin?
> 

Since LocalStorageDatabaseTracker always traverses file folder for origins and data, I It seems Okay to untie LocalStorageDatabaseTracker and LocalStorageDatabase.

> > Source/WebKit/NetworkProcess/WebStorage/StorageArea.cpp:183
> > +        m_localStorageDatabase = LocalStorageDatabase::create(databasePath, m_quotaInBytes);
> 
> Could use WTFMove and take a String&&

Sure.

[Sihui Liu]

(In reply to Chris Dumez from comment #4)
> Comment on attachment 427851 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=427851&action=review
> 
> > Source/WebKit/NetworkProcess/WebStorage/LocalStorageDatabase.cpp:-151
> > -        transaction.rollback();
> 
> Why are we dropping this? This seems like a potential change in behavior but
> the ChangeLog says nothing about this.
> 

~SQLiteTransaction will do the rollback. Will update changeling.

> > Source/WebKit/NetworkProcess/WebStorage/StorageArea.cpp:181
> > +        auto* localStorageDatabaseTracker = m_localStorageNamespace->storageManager()->localStorageDatabaseTracker();
> 
> The previous code used to assume his was non-null but now you are
> null-checking it. Which is right?

Technically it should be non-null. I just add the safe check because it returns a raw pointer; will change it back.

[Sihui Liu]

Created attachment 427893 [details]
Patch

[Chris Dumez]

Comment on attachment 427893 [details]
Patch

r=me

[EWS]

Committed r277106 (237408@main): <https://commits.webkit.org/237408@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 427893 [details].

[Radar WebKit Bug Importer]

<rdar://problem/77619330>