webbookmarksd needs to use the same AppCache directory as MobileSafari

[Alex Christensen]

webbookmarksd needs to use the same AppCache directory as MobileSafari

[Alex Christensen]

Created attachment 283967 [details]
Patch

[WebKit Commit Bot]

Attachment 283967 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 283967 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=283967&action=review

> Source/WebKit2/UIProcess/API/Cocoa/APIWebsiteDataStoreCocoa.mm:43
>      // FIXME: Ideally we should just have Safari and WebApp create a data store with

You should add webbookmarksd to the comment. It would be good to also have a reference to the Apple bug tracking fixing this.

[Brady Eidson]

(In reply to comment #2)
> Attachment 283967 [details] did not pass style-queue:
> 
> 
> ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and
> either add and list tests, or explain why no new tests were possible. 
> [changelog/nonewtests] [5]
> Total errors found: 1 in 5 files

This is actually testable - We've recently started adding API tests where we manually put SQLite and data files in place, then do something with WK API to remove them, then manually check that they were removed.

[Alex Christensen]

https://trac.webkit.org/changeset/203392

[Alex Christensen]

(In reply to comment #4)
> (In reply to comment #2)
> > Attachment 283967 [details] did not pass style-queue:
> > 
> > 
> > ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and
> > either add and list tests, or explain why no new tests were possible. 
> > [changelog/nonewtests] [5]
> > Total errors found: 1 in 5 files
> 
> This is actually testable - We've recently started adding API tests where we
> manually put SQLite and data files in place, then do something with WK API
> to remove them, then manually check that they were removed.
Can we change the main bundle identifier to com.apple.webbookmarksd in the tests?

[Brady Eidson]

(In reply to comment #6)
> (In reply to comment #4)
> > (In reply to comment #2)
> > > Attachment 283967 [details] did not pass style-queue:
> > > 
> > > 
> > > ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and
> > > either add and list tests, or explain why no new tests were possible. 
> > > [changelog/nonewtests] [5]
> > > Total errors found: 1 in 5 files
> > 
> > This is actually testable - We've recently started adding API tests where we
> > manually put SQLite and data files in place, then do something with WK API
> > to remove them, then manually check that they were removed.
> Can we change the main bundle identifier to com.apple.webbookmarksd in the
> tests?

Alexey - Is there a way too spoof bundle IDs in API tests? Or do you have an alternate idea here?

[Alexey Proskuryakov]

Swizzling should work I guess.

We need to do something to make API tests scalable, I'm skeptical of adding more when they run sequentially and each one is so slow.

[Brady Eidson]

(In reply to comment #8)
> Swizzling should work I guess.
> 
> We need to do something to make API tests scalable, I'm skeptical of adding
> more when they run sequentially and each one is so slow.

This *almost* sounds like you're messaging "We shouldn't add any more API tests until we find a way to make them run in parallel", but I'm sure that's not your actual point ;)

I don't see why run-api-tests couldn't run multiple parallel instances of TestWebKitAPI just like run-webkit-tests does, but that's clearly out of scope for this bug.

[Alex Christensen]

cmake ports make multiple api test executables.  We could even make one per test.

I would like to add a test for this, but I wasn't aware of this "Swizzling".  If someone could point me in the right direction, I'll add a test.

[Brady Eidson]

(In reply to comment #10)
> cmake ports make multiple api test executables.  We could even make one per
> test.
> 
> I would like to add a test for this, but I wasn't aware of this "Swizzling".
> If someone could point me in the right direction, I'll add a test.

http://darkdust.net/index.php/writings/objective-c/method-swizzling

So, what we want to change is this:
static String applicationBundleIdentifier()
{
    // The override only gets set in WebKit2's WebProcess and NetworkProcess. If unset, we use the main bundle identifier.
    const auto& identifier = applicationBundleIdentifierOverride();
    ASSERT(identifier.isNull() || RunLoop::isMain());
    return identifier.isNull() ? String([[NSBundle mainBundle] bundleIdentifier]) : identifier;
}

So you'd swizzle the -[NSBundle bundleIdentifier] method to be the fake one, run the test, then swizzle it back.

[Brady Eidson]

Also https://blog.newrelic.com/2014/04/16/right-way-to-swizzle/

[Alex Christensen]

Oh, THAT swizzling.  I verified this fix manually.  I'm not sure how much value it would add to have a test that uses swizzling to verify the existence of an iOS-specific hack that we want to get rid of.  My complete lack of testing isn't the best, either...

[Brady Eidson]

(In reply to comment #13)
> Oh, THAT swizzling.  I verified this fix manually.  I'm not sure how much
> value it would add to have a test that uses swizzling to verify the
> existence of an iOS-specific hack that we want to get rid of.  

The value is that we don't regress again in the meantime.

"Regression tests" <--- It's right there in the name.

[Alex Christensen]

Regression tests added in https://bugs.webkit.org/show_bug.cgi?id=159949