Bug 30548 – Remove the dependency on OriginQuotaManager from Database.cpp

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dumitru Daniliuc]

Chromium needs to manage per-origin quotas in the browser process, and therefore, cannot use the OriginQuotaManager/OriginUsageRecord classes. We need to remove the dependency on those classes from the rest of the database classes.

[Dumitru Daniliuc]

Created attachment 41471 [details]
patch

It's OK to have DatabaseTracker.cpp depend on OriginQuotaManager/OriginUsageRecord since we will have a Chromium-specific implementation for DatabaseTracker.

Also, in order to keep everything thread-safe, we needed to introduce a SecurityOrigin copy for each Database instance, that is safe to use on the DB thread.

[Eric Seidel (no email)]

We should CC whoever does Database stuff @ Apple on this bug.

[Dumitru Daniliuc]

(In reply to comment #2)
> We should CC whoever does Database stuff @ Apple on this bug.

added aroben, andersca and beidson. haven't added them initially because they never reply to my patches.

[Adam Barth]

Comment on attachment 41471 [details]
patch

Looks reasonable.  A few minor points about RefPtrs:

+ PassRefPtr<SecurityOrigin> Database::databaseThreadSecurityOrigin() const
+ PassRefPtr<SecurityOrigin> Database::threadSafeSecurityOrigin() const

No need to use a PassRefPtr here because we're not transferring ownership.  We can just use a raw ptr.  For more information, please see http://webkit.org/coding/RefPtr.html

+ RefPtr<SecurityOrigin> origin = database->databaseThreadSecurityOrigin();

No need to churn ref counter here.  |database| will keep origin alive for you during this call.

[Dumitru Daniliuc]

Created attachment 41917 [details]
patch

Removed all SecurityOrigin related methods in Database in favor of a single securityOrigin() method that returns a thread-safe SecurityOrigin instance.

[Adam Barth]

Comment on attachment 41917 [details]
patch

In DatabaseTracker::getMaxSizeForDatabase, do we need to addref the origin?

+ RefPtr<SecurityOrigin> origin = database->securityOrigin();

It seems like we can use a raw pointer here and avoid the churning the ref count.

[Dumitru Daniliuc]

Created attachment 41970 [details]
patch

Addressed Adam's comment. Please re-r+ the patch.

[Adam Barth]

Comment on attachment 41970 [details]
patch

Thanks!

[WebKit Commit Bot]

Comment on attachment 41970 [details]
patch

Rejecting patch 41970 from commit-queue.

Failed to run "WebKitTools/Scripts/build-webkit" exit_code: 1
Last 500 characters of output:
xfyyua/WebCorePrefix.h -c /Users/eseidel/Projects/CommitQueue/WebCore/svg/SVGAElement.cpp -o /Users/eseidel/Projects/build/WebCore.build/Release/WebCore.build/Objects-normal/i386/SVGAElement.o
** BUILD FAILED **

The following build commands failed:
WebCore:
	Distributed-CompileC /Users/eseidel/Projects/build/WebCore.build/Release/WebCore.build/Objects-normal/i386/Database.o /Users/eseidel/Projects/CommitQueue/WebCore/storage/Database.cpp normal i386 c++ com.apple.compilers.gcc.4_2
(1 failure)

[Eric Seidel (no email)]

Comment on attachment 41970 [details]
patch

This appears to be a false rejection.

[Eric Seidel (no email)]

Comment on attachment 41970 [details]
patch

Actually, this patch will fail due to tabs in the ChangeLog.  so cq-

[Dumitru Daniliuc]

ChangeLog had some tabs. I fixed that and manually submitted the patch. Landed as r50169.