Bug 101139 – Expose security origin to BundleFrame

• Component: WebKit2
• Status: RESOLVED FIXED

[Jon Lee]

Allow clients to get the origin for a given frame.

[Jon Lee]

<rdar://problem/12629900>

[Jon Lee]

Created attachment 172192 [details]
Patch

[Adam Barth]

Comment on attachment 172192 [details]
Patch

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

> Source/WebKit2/WebProcess/WebPage/WebFrame.cpp:809
> +    return WebSecurityOrigin::createFromString(m_coreFrame->document()->securityOrigin()->toString());

Round-tripping through a string is lossy.  Are you sure that's what you want?

[Jon Lee]

The last time I dealt with this the origins were sent across the wire as a string, so the reconstruction of the origin was lossy, as you said. In this case, maybe we can share the webcore origin across WebSecurityOrigin instances. Let me think about this more.

[Sam Weinig]

(In reply to comment #4)
> The last time I dealt with this the origins were sent across the wire as a string, so the reconstruction of the origin was lossy, as you said. In this case, maybe we can share the webcore origin across WebSecurityOrigin instances. Let me think about this more.

We should definitely not be serializing to string only to reconstruct the origin.  Instead, you should add a new WebSecurityOrigin create function that takes a WebCore::SecurityOrigin.

[Jon Lee]

Created attachment 172211 [details]
Patch

[Sam Weinig]

Comment on attachment 172211 [details]
Patch

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

Please add an API test.

> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundleFrame.cpp:294
> +WKSecurityOriginRef WKBundleFrameGetSecurityOrigin(WKBundleFrameRef frameRef)
> +{
> +    return toAPI(toImpl(frameRef)->securityOrigin().leakRef());
> +}

This needs to be called WKBundleFrameCopySecurityOrigin and follow copy rules.

> Source/WebKit2/WebProcess/WebPage/WebFrame.cpp:807
> +    if (!m_coreFrame)
> +        return WebSecurityOrigin::createFromString(String());

I don't understand why this makes sense.  Why not just return null?

[Adam Barth]

Comment on attachment 172211 [details]
Patch

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

>> Source/WebKit2/WebProcess/WebPage/WebFrame.cpp:807
>> +        return WebSecurityOrigin::createFromString(String());
> 
> I don't understand why this makes sense.  Why not just return null?

Or, if for some reason you can't return null, you can create a unique origin via SecurityOrigin::createUnique().

[Jon Lee]

Created attachment 172235 [details]
Patch

[Darin Adler]

Comment on attachment 172235 [details]
Patch

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

> Source/WebKit2/Shared/API/c/WKSharedAPICast.h:207
> +    RefPtr<WebSecurityOrigin> webSecurityOrigin = WebSecurityOrigin::create(origin);
> +    return toAPI(webSecurityOrigin.release().leakRef());

I think these two lines would read better as one, without the local variable.

> Source/WebKit2/Shared/WebSecurityOrigin.h:63
> +    static PassRefPtr<WebSecurityOrigin> create(WebCore::SecurityOrigin* securityOrigin)

This could use PassRefPtr instead of a raw pointer as the argument. It would compile down to the same code at call sites, since the function is inlined and the pointer ends up inside a PassRefPtr eventually. And it would make it possible to use this when the caller has a PassRefPtr. In fact, I think it would be slightly better. If this took a PassRefPtr, you could rewrite all the other create functions to call this one which would make them all into one-liners:

    return create(WebCore::SecurityOrigin::createFromString(string));

    return create(WebCore::SecurityOrigin::createFromDatabaseIdentifier(identifier));

    return create(WebCore::SecurityOrigin::create(protocol, host, port));

[Jon Lee]

Comment on attachment 172235 [details]
Patch

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

>> Source/WebKit2/Shared/API/c/WKSharedAPICast.h:207
>> +    return toAPI(webSecurityOrigin.release().leakRef());
> 
> I think these two lines would read better as one, without the local variable.

Fixed.

>> Source/WebKit2/Shared/WebSecurityOrigin.h:63
>> +    static PassRefPtr<WebSecurityOrigin> create(WebCore::SecurityOrigin* securityOrigin)
> 
> This could use PassRefPtr instead of a raw pointer as the argument. It would compile down to the same code at call sites, since the function is inlined and the pointer ends up inside a PassRefPtr eventually. And it would make it possible to use this when the caller has a PassRefPtr. In fact, I think it would be slightly better. If this took a PassRefPtr, you could rewrite all the other create functions to call this one which would make them all into one-liners:
> 
>     return create(WebCore::SecurityOrigin::createFromString(string));
> 
>     return create(WebCore::SecurityOrigin::createFromDatabaseIdentifier(identifier));
> 
>     return create(WebCore::SecurityOrigin::create(protocol, host, port));

Done. Thanks!

[Jon Lee]

Committed r133425: <http://trac.webkit.org/changeset/133425>