Bug 166632 – [GTK] Expose WebKitSecurityOrigin API

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Michael Catanzaro]

Expose WebKitSecurityOrigin API

[Michael Catanzaro]

Note that discussion for this is mostly in bug #163366.

[Michael Catanzaro]

Created attachment 297871 [details]
Patch

[Carlos Garcia Campos]

I'm not going to use SecurityOrigin for website data api in the end.

[Carlos Garcia Campos]

Comment on attachment 297871 [details]
Patch

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

Thanks!

> Source/WebKit2/ChangeLog:9
> +        permissions and exposing new functionality of WebKitWebsiteDataManager.

I'm not going to use this in WebKitWebsiteDataManager in the end.

> Source/WebKit2/UIProcess/API/gtk/WebKitSecurityOrigin.cpp:61
> +    int referenceCount = 1;

I've never seen this = 1 initialization, shouldn't it be { 1 }?

> Source/WebKit2/UIProcess/API/gtk/WebKitSecurityOrigin.cpp:77
> +WebCore::SecurityOrigin& webkitSecurityOriginGetSecurityOrigin(WebKitSecurityOrigin* origin)
> +{
> +    ASSERT(origin);
> +    return origin->securityOrigin.get();
> +}

As I said in the other review if you use to_string/as_string we don't need this.

> Source/WebKit2/UIProcess/API/gtk/WebKitSecurityOrigin.cpp:179
> +    if (origin->securityOrigin->protocol().isEmpty())
> +        return nullptr;

You should document that this can return %NULL. There's allow-none annotation, but we should explain in which cases a security origin can have a null protocol.

> Source/WebKit2/UIProcess/API/gtk/WebKitSecurityOrigin.cpp:201
> +    if (origin->securityOrigin->host().isEmpty())
> +        return nullptr;

Ditoo.

> Source/WebKit2/UIProcess/API/gtk/WebKitSecurityOrigin.cpp:216
> + * #WebKitSecurityOrigin constructed from either string.

string -> URI

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitSecurityOrigin.cpp:2
> + * Copyright (C) 2016 Igalia S.L.

Happy new year!

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitSecurityOrigin.cpp:30
> +    GUniquePtr<char> asString(webkit_security_origin_to_string(origin));

I still think as_string could be a good name and return const char*, but I don't have a strong opinion.

[Michael Catanzaro]

(In reply to comment #4)
> > Source/WebKit2/ChangeLog:9
> > +        permissions and exposing new functionality of WebKitWebsiteDataManager.
> 
> I'm not going to use this in WebKitWebsiteDataManager in the end.

OK.

> > Source/WebKit2/UIProcess/API/gtk/WebKitSecurityOrigin.cpp:61
> > +    int referenceCount = 1;
> 
> I've never seen this = 1 initialization, shouldn't it be { 1 }?

Good point, that is WebKit style. I usually use = in my own projects.

> > Source/WebKit2/UIProcess/API/gtk/WebKitSecurityOrigin.cpp:77
> > +WebCore::SecurityOrigin& webkitSecurityOriginGetSecurityOrigin(WebKitSecurityOrigin* origin)
> > +{
> > +    ASSERT(origin);
> > +    return origin->securityOrigin.get();
> > +}
> 
> As I said in the other review if you use to_string/as_string we don't need
> this.

Oops, missed that.

> > Source/WebKit2/UIProcess/API/gtk/WebKitSecurityOrigin.cpp:179
> > +    if (origin->securityOrigin->protocol().isEmpty())
> > +        return nullptr;
> 
> You should document that this can return %NULL. There's allow-none
> annotation, but we should explain in which cases a security origin can have
> a null protocol.

I did and then removed it, because it can only return NULL if the security origin is opaque, but I decided not to expose opaqueness in the API. I guess I have to briefly explain that here.

> > Source/WebKit2/UIProcess/API/gtk/WebKitSecurityOrigin.cpp:201
> > +    if (origin->securityOrigin->host().isEmpty())
> > +        return nullptr;
> 
> Ditoo.

This case is easier to explain, since some protocols like file just don't have hosts.

> > Source/WebKit2/UIProcess/API/gtk/WebKitSecurityOrigin.cpp:216
> > + * #WebKitSecurityOrigin constructed from either string.
> 
> string -> URI

OK

> > Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitSecurityOrigin.cpp:2
> > + * Copyright (C) 2016 Igalia S.L.
> 
> Happy new year!

Apparently.

> > Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitSecurityOrigin.cpp:30
> > +    GUniquePtr<char> asString(webkit_security_origin_to_string(origin));
> 
> I still think as_string could be a good name and return const char*, but I
> don't have a strong opinion.

I only see two uses of _as_string in devhelp, g_file_info_get_attribute_as_string, and g_mime_message_get_date_as_string, and they're both transfer full, so I think we should stick with _to_string.

[Michael Catanzaro]

I will expose _is_opaque but not any of the other methods.

[Michael Catanzaro]

Created attachment 297874 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 297874 [details]
Patch

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

> Source/WebKit2/UIProcess/API/gtk/WebKitSecurityOrigin.cpp:34
> +using WebCore::URL;

What is this for?

> Source/WebKit2/UIProcess/API/gtk/WebKitSecurityOrigin.cpp:163
> + * Gets the protocol of @origin, or %NULL if @origin is opaque.

opaque? why not unique?

> Source/WebKit2/UIProcess/API/gtk/WebKitSecurityOrigin.cpp:185
> + * Gets the hostname of @origin, or %NULL if @origin is opaque or if its

Ditto.

> Source/WebKit2/UIProcess/API/gtk/WebKitSecurityOrigin.cpp:230
> + * Gets whether @origin is an opaque security origin.

As I said in previous reviews, we should explain here what a unique origin is.

> Source/WebKit2/UIProcess/API/gtk/WebKitSecurityOrigin.cpp:238
> +    g_return_val_if_fail(origin, FALSE);

I would return TRUE in this case.

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitSecurityOrigin.cpp:118
> +static void testSecurityOriginDataURI(Test*, gconstpointer)

Now that we expose unique origins, I would call this testSecurityOriginUnique, since what we want to test here is unique origins,m not data URIs specifically.

[Michael Catanzaro]

(In reply to comment #8)
> Comment on attachment 297874 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=297874&action=review
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitSecurityOrigin.cpp:34
> > +using WebCore::URL;
> 
> What is this for?

To make the URL constructor simpler:

URL(URL(), ...)

instead of:

WebCore::URL(WebCore::URL(), ...)

> > Source/WebKit2/UIProcess/API/gtk/WebKitSecurityOrigin.cpp:163
> > + * Gets the protocol of @origin, or %NULL if @origin is opaque.
> 
> opaque? why not unique?

To match the terminology that is used by HTML, which developers might already be familiar with. Our "unique" security origins are the same as HTML opaque origins, while an HTML unique opaque origin would be an opaque origin that is equivalent to no other opaque origin.

At least, I think so. :)

> > Source/WebKit2/UIProcess/API/gtk/WebKitSecurityOrigin.cpp:230
> > + * Gets whether @origin is an opaque security origin.
> 
> As I said in previous reviews, we should explain here what a unique origin
> is.

I explained it in the section block; that's probably enough, right?

> > Source/WebKit2/UIProcess/API/gtk/WebKitSecurityOrigin.cpp:238
> > +    g_return_val_if_fail(origin, FALSE);
> 
> I would return TRUE in this case.

Hm, we normally always return FALSE from these assertion failures, but you're right, that does seem safer to do here.

> > Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitSecurityOrigin.cpp:118
> > +static void testSecurityOriginDataURI(Test*, gconstpointer)
> 
> Now that we expose unique origins, I would call this
> testSecurityOriginUnique, since what we want to test here is unique
> origins,m not data URIs specifically.

OK.

[Michael Catanzaro]

Created attachment 297902 [details]
Patch

[Michael Catanzaro]

I think we could rename WebCore::SecurityOrigin::isUnique to isOpaque if you doesn't like the terminology mismatch, but I don't think it's terribly unusual to have different terminology for the public API than the internal API. We surely don't want to rename WebKitWebContext to WebKitWebProcessPool, for example.

[Carlos Garcia Campos]

(In reply to comment #9)
> (In reply to comment #8)
> > Comment on attachment 297874 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=297874&action=review
> > 
> > > Source/WebKit2/UIProcess/API/gtk/WebKitSecurityOrigin.cpp:34
> > > +using WebCore::URL;
> > 
> > What is this for?
> 
> To make the URL constructor simpler:
> 
> URL(URL(), ...)
> 
> instead of:
> 
> WebCore::URL(WebCore::URL(), ...)

You can do that also with using namespace WebCore which is what we usually do.

> > > Source/WebKit2/UIProcess/API/gtk/WebKitSecurityOrigin.cpp:163
> > > + * Gets the protocol of @origin, or %NULL if @origin is opaque.
> > 
> > opaque? why not unique?
> 
> To match the terminology that is used by HTML, which developers might
> already be familiar with. Our "unique" security origins are the same as HTML
> opaque origins, while an HTML unique opaque origin would be an opaque origin
> that is equivalent to no other opaque origin.
> 
> At least, I think so. :)

Maybe I'm too used to WebKit terminology. The RFC-6454 refers to "globally unique identifier". In HTML5 docs I've seen "unique opaque origin". If people are used to opaque, it's ok then.

> > > Source/WebKit2/UIProcess/API/gtk/WebKitSecurityOrigin.cpp:230
> > > + * Gets whether @origin is an opaque security origin.
> > 
> > As I said in previous reviews, we should explain here what a unique origin
> > is.
> 
> I explained it in the section block; that's probably enough, right?

I missed that.

> > > Source/WebKit2/UIProcess/API/gtk/WebKitSecurityOrigin.cpp:238
> > > +    g_return_val_if_fail(origin, FALSE);
> > 
> > I would return TRUE in this case.
> 
> Hm, we normally always return FALSE from these assertion failures, but
> you're right, that does seem safer to do here.

Because normally FALSE prevents from doing more things with the object or taking further actions, in this case, that happens with TRUE.

> > > Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitSecurityOrigin.cpp:118
> > > +static void testSecurityOriginDataURI(Test*, gconstpointer)
> > 
> > Now that we expose unique origins, I would call this
> > testSecurityOriginUnique, since what we want to test here is unique
> > origins,m not data URIs specifically.
> 
> OK.

[Michael Catanzaro]

Seems commit-queue is stuck on flaky tests?

[Michael Catanzaro]

Committed r210238: <http://trac.webkit.org/changeset/210238>