Bug 30548

Summary: Remove the dependency on OriginQuotaManager from Database.cpp
Product: WebKit Reporter: Dumitru Daniliuc <dumi>
Component: New BugsAssignee: Dumitru Daniliuc <dumi>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, andersca, aroben, beidson, eric, fishd, michaeln
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
patch
abarth: review-
patch
abarth: review+
patch abarth: review+, eric: commit-queue-

Dumitru Daniliuc
Reported 2009-10-19 18:41:46 PDT
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.
Attachments
patch (10.68 KB, patch)
2009-10-19 19:37 PDT, Dumitru Daniliuc
abarth: review-
patch (14.44 KB, patch)
2009-10-26 17:13 PDT, Dumitru Daniliuc
abarth: review+
patch (14.42 KB, patch)
2009-10-27 11:51 PDT, Dumitru Daniliuc
abarth: review+
eric: commit-queue-
Dumitru Daniliuc
Comment 1 2009-10-19 19:37:33 PDT
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)
Comment 2 2009-10-21 10:39:32 PDT
We should CC whoever does Database stuff @ Apple on this bug.
Dumitru Daniliuc
Comment 3 2009-10-21 10:43:48 PDT
(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 4 2009-10-24 09:44:48 PDT
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
Comment 5 2009-10-26 17:13:56 PDT
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 6 2009-10-27 09:04:12 PDT
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
Comment 7 2009-10-27 11:51:36 PDT
Created attachment 41970 [details] patch Addressed Adam's comment. Please re-r+ the patch.
Adam Barth
Comment 8 2009-10-27 12:48:15 PDT
Comment on attachment 41970 [details] patch Thanks!
WebKit Commit Bot
Comment 9 2009-10-27 13:09:31 PDT
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 10 2009-10-27 13:15:03 PDT
Comment on attachment 41970 [details] patch This appears to be a false rejection.
Eric Seidel (no email)
Comment 11 2009-10-27 13:15:29 PDT
Comment on attachment 41970 [details] patch Actually, this patch will fail due to tabs in the ChangeLog. so cq-
Dumitru Daniliuc
Comment 12 2009-10-27 13:16:39 PDT
ChangeLog had some tabs. I fixed that and manually submitted the patch. Landed as r50169.
Note You need to log in before you can comment on or make changes to this bug.