Bug 134984

Summary: Add WebSecurityOrigin "webSecurityOriginFromDatabaseIdentifier" SPI and change _websiteDataURLForContainerWithURL: SPI
Product: WebKit Reporter: Brady Eidson <beidson>
Component: WebKit APIAssignee: Brady Eidson <beidson>
Status: RESOLVED FIXED    
Severity: Normal CC: mitz
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: All   
Attachments:
Description Flags
Patch v1
none
Patch 2 v1
none
Patch 1 v2
none
Patch v3
none
Patch v4
mitz: review+
Patch v5 - Reintroduce the old removed SPI (leaving everything else about the already landed patch the same) ddkilzer: review+

Description Brady Eidson 2014-07-16 10:07:45 PDT
Add WebSecurityOrigin "webSecurityOriginFromDatabaseIdentifier" SPI

<rdar://problem/17454712>
Comment 1 Brady Eidson 2014-07-16 10:11:52 PDT
Created attachment 235002 [details]
Patch v1
Comment 2 Brady Eidson 2014-07-16 10:15:16 PDT
Created attachment 235003 [details]
Patch 2 v1

A patch for separate review to land at the same time.
Comment 3 Brady Eidson 2014-07-16 10:17:41 PDT
Created attachment 235004 [details]
Patch 1 v2

Weird encoding issue preventing review?  Trying again...
Comment 4 Brady Eidson 2014-07-16 12:38:44 PDT
An additional change is needed.

Will combine these two patches with that change when uploading a new patch soon.
Comment 5 Brady Eidson 2014-07-16 13:41:46 PDT
Retitled to:
Add WebSecurityOrigin "webSecurityOriginFromDatabaseIdentifier" SPI and change _websiteDataURLForContainerWithURL: SPI
Comment 6 Brady Eidson 2014-07-16 13:43:25 PDT
Created attachment 235018 [details]
Patch v3
Comment 7 mitz 2014-07-16 14:18:12 PDT
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:
Comment 8 Brady Eidson 2014-07-16 15:57:09 PDT
(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.
Comment 9 Brady Eidson 2014-07-16 16:13:11 PDT
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.
Comment 10 Brady Eidson 2014-07-16 20:24:08 PDT
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.
Comment 11 Brady Eidson 2014-07-16 20:28:00 PDT
Created attachment 235047 [details]
Patch v5 - Reintroduce the old removed SPI (leaving everything else about the already landed patch the same)
Comment 12 David Kilzer (:ddkilzer) 2014-07-16 20:30:01 PDT
Comment on attachment 235047 [details]
Patch v5 - Reintroduce the old removed SPI (leaving everything else about the already landed patch the same)

r=me
Comment 13 Brady Eidson 2014-07-16 20:32:14 PDT
That landed in http://trac.webkit.org/changeset/171171