Bug 162149 – Rename SecurityOrigin::canRequest to SecurityOrigin::shouldTreatAsSameOrigin

• Component: WebCore Misc.
• Status: NEW

[youenn fablet]

This will better match the use of SecurityOrigin::canRequest.

[youenn fablet]

Created attachment 289227 [details]
Patch

[youenn fablet]

Created attachment 289229 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 289229 [details]
Patch

SecurityOrigin::canRequest returns true in many cases other than same origin URL, so it can't be called isSameOrigin.

[Alexey Proskuryakov]

Perhaps "shouldBeTreatedAsSameOrigin" or something else would be a good name.

[youenn fablet]

Created attachment 289327 [details]
shoudTreatAsSameOrigin renaming

[youenn fablet]

(In reply to comment #4)
> Perhaps "shouldBeTreatedAsSameOrigin" or something else would be a good name.

OK. How about shouldTreatAsSameOrigin?

[Alexey Proskuryakov]

This is a member function of SecurityOrigin, and saying origin-> shouldTreatAsSameOrigin(...) seems confusing - the origin doesn't "treat" the URL passed.

There is a large number of options, all the way to making this a non-member function. Discussing on webkit-dev wouldn't be a bad idea I think.

[youenn fablet]

(In reply to comment #7)
> This is a member function of SecurityOrigin, and saying origin->
> shouldTreatAsSameOrigin(...) seems confusing - the origin doesn't "treat"
> the URL passed.

Before the patch, the code could be read as:  can the security origin request that URL ?
With the patch, it read as: should the security origin treat as same origin the URL?

> There is a large number of options, all the way to making this a non-member
> function. Discussing on webkit-dev wouldn't be a bad idea I think.

Let's start with this bug entry and we will seek webkit-dev if needed.

[Darin Adler]

What language is used in the relevant standards and specifications for this concept? Could you point me at some of the key passages in the standards so I can see what kind of wording they use and how consistent our internal terminology is with that?

[youenn fablet]

HTML spec is using CORS-same-origin and CORS-cross-origin terms. This allows handling canvas tainting, script error message filtering, css rule set access from JS...
See https://html.spec.whatwg.org/#fetching-resources for a definition of those terms. The definition refers to fetch spec.

In fetch spec, https://fetch.spec.whatwg.org/#main-fetch is interesting, in particular step 11 when doing "request's current url's origin is request's origin".
That is what canRequest is really all about.
When looking at the place where canRequest is used (cf. the patch), this matches pretty well this same question: is it the same origin (not CORS-same-origin)?

Ideally, after refactoring is done, I would like code to check the HTML CORS-same-origin/CORS-cross-origin information on CachedResource directly, since a CachedResource is origin-specific. This should remove several uses of canRequest.

[youenn fablet]

Equality of origin is also defined here: https://html.spec.whatwg.org/multipage/browsers.html#same-origin

[Darin Adler]

This function is about a policy, not a check if two origins are the same. I think “should treat as same origin” is not great, because there could be many places that want to check if two origins are the same and maybe they should not use this function.

[Darin Adler]

I am going to think about other name suggestions. A useful exercise is to look at the different call sites and try to think about what words you would use to explain the check to a person. Maybe that will end up being "should treat as same origin", but maybe not.

[youenn fablet]

canRequest name makes it clear that we are testing an URL (an instance) against an origin (a set of URLs). We probably should keep that information. How about:

bool isURLSameOrigin(const URL&, const SecurityOrigin&);

[Brady Eidson]

Comment on attachment 289327 [details]
shoudTreatAsSameOrigin renaming

Patches that have been up for review since 2016 are almost certainly too stale to be relevant to trunk in their current form.

If this patch is still important please rebase it and post it for review again.