Bug 166632 - [GTK] Expose WebKitSecurityOrigin API
Summary: [GTK] Expose WebKitSecurityOrigin API
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: PC Linux
: P2 Normal
Assignee: Michael Catanzaro
URL:
Keywords:
Depends on: 166624
Blocks: 163366
  Show dependency treegraph
 
Reported: 2016-12-31 14:06 PST by Michael Catanzaro
Modified: 2017-01-03 04:00 PST (History)
4 users (show)

See Also:


Attachments
Patch (27.31 KB, patch)
2016-12-31 14:21 PST, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (28.37 KB, patch)
2017-01-01 09:55 PST, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (28.40 KB, patch)
2017-01-02 10:08 PST, Michael Catanzaro
cgarcia: commit-queue+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Catanzaro 2016-12-31 14:06:34 PST
Expose WebKitSecurityOrigin API
Comment 1 Michael Catanzaro 2016-12-31 14:09:50 PST
Note that discussion for this is mostly in bug #163366.
Comment 2 Michael Catanzaro 2016-12-31 14:21:40 PST
Created attachment 297871 [details]
Patch
Comment 3 Carlos Garcia Campos 2017-01-01 01:57:48 PST
I'm not going to use SecurityOrigin for website data api in the end.
Comment 4 Carlos Garcia Campos 2017-01-01 02:23:27 PST
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.
Comment 5 Michael Catanzaro 2017-01-01 08:09:30 PST
(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.
Comment 6 Michael Catanzaro 2017-01-01 08:51:03 PST
I will expose _is_opaque but not any of the other methods.
Comment 7 Michael Catanzaro 2017-01-01 09:55:26 PST
Created attachment 297874 [details]
Patch
Comment 8 Carlos Garcia Campos 2017-01-02 00:15:30 PST
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.
Comment 9 Michael Catanzaro 2017-01-02 07:27:58 PST
(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.
Comment 10 Michael Catanzaro 2017-01-02 10:08:06 PST
Created attachment 297902 [details]
Patch
Comment 11 Michael Catanzaro 2017-01-02 10:09:14 PST
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.
Comment 12 Carlos Garcia Campos 2017-01-03 00:08:43 PST
(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.
Comment 13 Michael Catanzaro 2017-01-03 03:59:25 PST
Seems commit-queue is stuck on flaky tests?
Comment 14 Michael Catanzaro 2017-01-03 04:00:26 PST
Committed r210238: <http://trac.webkit.org/changeset/210238>