[webkit-reviews] review denied: [Bug 208528] UIProcess needs mechanism to specify AppBound domains : [Attachment 392324] Patch

[bugzilla-daemon at webkit.org]

John Wilander <wilander at apple.com> has denied katherine_cheney at apple.com's
request for review:
Bug 208528: UIProcess needs mechanism to specify AppBound domains
https://bugs.webkit.org/show_bug.cgi?id=208528

Attachment 392324: Patch

https://bugs.webkit.org/attachment.cgi?id=392324&action=review




--- Comment #4 from John Wilander <wilander at apple.com> ---
Comment on attachment 392324
  --> https://bugs.webkit.org/attachment.cgi?id=392324
Patch

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

> Source/WebCore/ChangeLog:3
> +	   UIProcess needs mechanism to specify AppBound domains

It's a little weird to refer to UIProcess without further qualifications in the
WebCore change log. Not sure how this could be done better.

> Source/WebCore/platform/RuntimeApplicationChecks.h:28
> +#include "RegistrableDomain.h"

I believe this could just be forward-declared instead of included. Then you
include in the implementation file instead. We try to do that to shorten build
times.

> Source/WebCore/platform/cocoa/RuntimeApplicationChecksCocoa.mm:78
> +Vector<WebCore::RegistrableDomain>& getAppBoundDomains()

I wonder if there's a way to make sure that callers of this function cannot
change the app-bound domains.

> Source/WebCore/platform/cocoa/RuntimeApplicationChecksCocoa.mm:84
> +    NSArray *domains = [[NSBundle mainBundle]
objectForInfoDictionaryKey:@"AppBoundDomains"];

Is there a guarantee that these domains will indeed be registrable domains? I
don't think there even can be since the Public Suffix List which is the basis
for RegistrableDomain changes and thus an array entry may be a registrable
domain one day and not another, or vice versa. See comment below.

> Source/WebCore/platform/cocoa/RuntimeApplicationChecksCocoa.mm:85
> +    int appBoundDomainsCount = std::min(maxAppBoundDomainCount,
static_cast<int>([domains count]));

Where does maxAppBoundDomainCount come from? It doesn't look like a constant.

> Source/WebCore/platform/cocoa/RuntimeApplicationChecksCocoa.mm:88
> +	  
appBoundDomains->append(WebCore::RegistrableDomain::uncheckedCreateFromRegistra
bleDomainString([domains objectAtIndex:i]));

You should use a checked constructor here by creating a URL object from the
array entry and then letting RegistrableDomain figure out the registrable
domain through its use of the Public Suffix List. Then you also need to check
that the returned RegistrableDomain object is not empty since an array entry
that does not have a registrable domain will result in an empty object.

> Source/WebCore/platform/cocoa/RuntimeApplicationChecksCocoa.mm:90
> +    return appBoundDomains;

I wonder if we should return a deep copy of this Vector? Otherwise other code
can change its content, right? (Not 100% sure about function statics.) It goes
back to my thoughts that we should make sure callers of this function cannot
change the set of app-bound domains. Another way of doing it is to have a void
return type and instead have the caller supply a Vector that this function
fills with copies of the registrable domains.

> Source/WebKit/UIProcess/WebProcessProxy.cpp:1770
> +	   m_appBoundDomains.uncheckedAppend(domain);

Since this is that way we're using WebCore::getAppBoundDomains(), it looks like
supplying the function with the Vector that WebProcessProxy wants filled is a
better pattern.

> Source/WebKit/UIProcess/WebProcessProxy.h:379
> +    Vector<WebCore::RegistrableDomain>& appBoundDomains() { return
m_appBoundDomains; }

First, this can be made const. Second, the same worry applies here in changing
the contents of the Vector.

> Tools/TestWebKitAPI/Info.plist:7
> +		<string>testDomain1</string>

You need to do full testing here. Real registrable domains, real domains with
subdomains, bogus domains (such as the ones you have now), empty entries,
whitespace entries, full URLs, and IP addresses. Then you need to test below
max, at max, and above max number of entries.