Summary: | Add WebSecurityOrigin "webSecurityOriginFromDatabaseIdentifier" SPI and change _websiteDataURLForContainerWithURL: SPI | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Brady Eidson <beidson> | ||||||||||||||
Component: | WebKit API | Assignee: | Brady Eidson <beidson> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | mitz | ||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | Mac | ||||||||||||||||
OS: | All | ||||||||||||||||
Attachments: |
|
Description
Brady Eidson
2014-07-16 10:07:45 PDT
Created attachment 235002 [details]
Patch v1
Created attachment 235003 [details]
Patch 2 v1
A patch for separate review to land at the same time.
Created attachment 235004 [details]
Patch 1 v2
Weird encoding issue preventing review? Trying again...
An additional change is needed. Will combine these two patches with that change when uploading a new patch soon. Retitled to: Add WebSecurityOrigin "webSecurityOriginFromDatabaseIdentifier" SPI and change _websiteDataURLForContainerWithURL: SPI Created attachment 235018 [details]
Patch v3
Comment on attachment 235018 [details] Patch v3 View in context: https://bugs.webkit.org/attachment.cgi?id=235018&action=review > Source/WebKit/mac/WebCoreSupport/WebSecurityOrigin.mm:48 > + return [[WebSecurityOrigin alloc] _initWithWebCoreSecurityOrigin:origin.get()]; Based on the method’s name (and lack of annotations in its declaration), this should be autoreleased. > Source/WebKit/mac/WebCoreSupport/WebSecurityOriginPrivate.h:37 > ++ (id)webSecurityOriginFromDatabaseIdentifier:(NSString *)databaseIdentifier; Could use a newline above this. > Source/WebKit2/DatabaseProcess/DatabaseProcess.cpp:102 > + // ********* > + // IMPORTANT: Do not change the directory structure for indexed databases on disk without first consulting a reviewer from Apple (<rdar://problem/17454712>) > + // ********* I don’t think this comment is appropriate. In principle, we shouldn’t change the location and structure of any persistent user data between WebKit versions. > Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.cpp:67 > + // ********* > + // IMPORTANT: Do not change the directory structure for indexed databases on disk without first consulting a reviewer from Apple (<rdar://problem/17454712>) > + // ********* Ditto. > Source/WebKit2/UIProcess/API/Cocoa/WKProcessPoolPrivate.h:47 > -+ (NSURL *)_websiteDataURLForContainerWithURL:(NSURL *)containerURL; > ++ (NSURL *)_websiteDataURLForContainerWithURL:(NSURL *)containerURL bundleIdentifierIfNotInContainer:(NSString *)bundleIdentifier; Why cram two APIs into one? I think you want to add something like + (NSURL *)_websiteDataURLForApplicationWithBundleIdentifier: (In reply to comment #7) > (From update of attachment 235018 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=235018&action=review > > > Source/WebKit/mac/WebCoreSupport/WebSecurityOrigin.mm:48 > > + return [[WebSecurityOrigin alloc] _initWithWebCoreSecurityOrigin:origin.get()]; > > Based on the method’s name (and lack of annotations in its declaration), this should be autoreleased. Yup. > > Source/WebKit/mac/WebCoreSupport/WebSecurityOriginPrivate.h:37 > > ++ (id)webSecurityOriginFromDatabaseIdentifier:(NSString *)databaseIdentifier; > > Could use a newline above this. Sounds good. > > Source/WebKit2/DatabaseProcess/DatabaseProcess.cpp:102 > > + // ********* > > + // IMPORTANT: Do not change the directory structure for indexed databases on disk without first consulting a reviewer from Apple (<rdar://problem/17454712>) > > + // ********* > > I don’t think this comment is appropriate. In principle, we shouldn’t change the location and structure of any persistent user data between WebKit versions. > > > Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.cpp:67 > > + // ********* > > + // IMPORTANT: Do not change the directory structure for indexed databases on disk without first consulting a reviewer from Apple (<rdar://problem/17454712>) > > + // ********* > > Ditto. These comments were directly requested by another Apple reviewer that I agree with. > > Source/WebKit2/UIProcess/API/Cocoa/WKProcessPoolPrivate.h:47 > > -+ (NSURL *)_websiteDataURLForContainerWithURL:(NSURL *)containerURL; > > ++ (NSURL *)_websiteDataURLForContainerWithURL:(NSURL *)containerURL bundleIdentifierIfNotInContainer:(NSString *)bundleIdentifier; > > Why cram two APIs into one? I think you want to add something like > + (NSURL *)_websiteDataURLForApplicationWithBundleIdentifier: The new form still needs to take the container URL and bundleID. I can add a second form instead of overriding the first, but it does need both arguments. Created attachment 235033 [details]
Patch v4
On IRC Dan and I hashed out the _websiteDataURLForContainerWithURL: change.
Made the other requested changes here in Patch v4.
The comments need to stay, but don't need to remain in the exact same form they're currently in.
Landed that patch in http://trac.webkit.org/changeset/171160 But we're going to keep the old SPI around, too. Patch for that forthcoming. Created attachment 235047 [details]
Patch v5 - Reintroduce the old removed SPI (leaving everything else about the already landed patch the same)
Comment on attachment 235047 [details]
Patch v5 - Reintroduce the old removed SPI (leaving everything else about the already landed patch the same)
r=me
That landed in http://trac.webkit.org/changeset/171171 |