Bug 134984 – Add WebSecurityOrigin "webSecurityOriginFromDatabaseIdentifier" SPI and change _websiteDataURLForContainerWithURL: SPI

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit API

[Brady Eidson]

Add WebSecurityOrigin "webSecurityOriginFromDatabaseIdentifier" SPI

<rdar://problem/17454712>

[Brady Eidson]

Created attachment 235002 [details]
Patch v1

[Brady Eidson]

Created attachment 235003 [details]
Patch 2 v1

A patch for separate review to land at the same time.

[Brady Eidson]

Created attachment 235004 [details]
Patch 1 v2

Weird encoding issue preventing review?  Trying again...

[Brady Eidson]

An additional change is needed.

Will combine these two patches with that change when uploading a new patch soon.

[Brady Eidson]

Retitled to:
Add WebSecurityOrigin "webSecurityOriginFromDatabaseIdentifier" SPI and change _websiteDataURLForContainerWithURL: SPI

[Brady Eidson]

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:

[Brady Eidson]

(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.

[Brady Eidson]

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.

[Brady Eidson]

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.

[Brady Eidson]

Created attachment 235047 [details]
Patch v5 - Reintroduce the old removed SPI (leaving everything else about the already landed patch the same)

[David Kilzer (:ddkilzer)]

Comment on attachment 235047 [details]
Patch v5 - Reintroduce the old removed SPI (leaving everything else about the already landed patch the same)

r=me

[Brady Eidson]

That landed in http://trac.webkit.org/changeset/171171