Bug 134984 - Add WebSecurityOrigin "webSecurityOriginFromDatabaseIdentifier" SPI and change _websiteDataURLForContainerWithURL: SPI
Summary: Add WebSecurityOrigin "webSecurityOriginFromDatabaseIdentifier" SPI and chang...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit API (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac All
: P2 Normal
Assignee: Brady Eidson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-07-16 10:07 PDT by Brady Eidson
Modified: 2014-07-16 20:32 PDT (History)
1 user (show)

See Also:


Attachments
Patch v1 (7.48 KB, application/octet-stream)
2014-07-16 10:11 PDT, Brady Eidson
no flags Details
Patch 2 v1 (3.93 KB, patch)
2014-07-16 10:15 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch 1 v2 (7.48 KB, patch)
2014-07-16 10:17 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch v3 (13.52 KB, patch)
2014-07-16 13:43 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch v4 (13.51 KB, patch)
2014-07-16 16:13 PDT, Brady Eidson
mitz: review+
Details | Formatted Diff | Diff
Patch v5 - Reintroduce the old removed SPI (leaving everything else about the already landed patch the same) (2.03 KB, patch)
2014-07-16 20:28 PDT, Brady Eidson
ddkilzer: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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