Bug 32529 – Rename skipCanLoadCheck to skipSecurityCheck

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Nate Chapin]

First step of https://bugs.webkit.org/show_bug.cgi?id=30457.  The next step will include separating the can load check into the security check and a check as to whether the current subresource load should be allowed to outlive the page that spawned it.

[Nate Chapin]

Created attachment 44813 [details]
patch

[Eric Seidel (no email)]

Comment on attachment 44813 [details]
patch

This looks good to me.  Sooooo much more readable!

[Brady Eidson]

I think Sam should look at this before it lands, he's the most likely to have an opinion on it.

[Adam Barth]

I'm not sure this is the optimal name, but it's lightyears better than what we have currently.

[Darin Adler]

Comment on attachment 44813 [details]
patch

> +        void load(DocLoader*, CachedResource*, bool incremental = true, SecurityCheck securityCheck = DoSecurityCheck, bool sendResourceLoadCallbacks = true);

One of the great things about using a named type is that you can, and should, leave out the argument name in cases like this one.
>  
> +    void load(DocLoader*, bool incremental, SecurityCheck securityCheck, bool sendResourceLoadCallbacks);

And this one.
>  
> +        Request(DocLoader*, CachedResource*, bool incremental, SecurityCheck securityCheck, bool sendResourceLoadCallbacks);

And this one.

> +        SecurityCheck shouldDoSecurityCheck() const { return m_securityCheck; }

The data member and the function member should have the same name.

> +        void loadRequest(const ResourceRequest&, SecurityCheck securityCheck);

Another place where you can omit the argument name.

> +    if ((securityCheck == DoSecurityCheck) && (fl->state() == FrameStateProvisional || fl->activeDocumentLoader()->isStopping()))

We normally would not include the parentheses around the "==" expression. It's so idiomatic to combine "==" with "&&" and "||" that we don't feel we need parentheses to make it clear.

> +    if ((securityCheck == DoSecurityCheck)

Ditto.

> +    // ok to skip load check since we already asked about the preflight request

Would be nice to format this comment as a sentence, which is our preferred style.

> +        static PassRefPtr<SubresourceLoader> create(Frame*, SubresourceLoaderClient*, const ResourceRequest&, SecurityCheck securityCheck = DoSecurityCheck, bool sendResourceLoadCallbacks = true, bool shouldContentSniff = true);

Another place to omit the argument name.

I feel bad about suggesting FrameLoaderTypes for this. Wish I could have thought of a better header.

[Nate Chapin]

(In reply to comment #5)
> (From update of attachment 44813 [details])
> > +        void load(DocLoader*, CachedResource*, bool incremental = true, SecurityCheck securityCheck = DoSecurityCheck, bool sendResourceLoadCallbacks = true);
> 
> One of the great things about using a named type is that you can, and should,
> leave out the argument name in cases like this one.
> >  
> > +    void load(DocLoader*, bool incremental, SecurityCheck securityCheck, bool sendResourceLoadCallbacks);
> 
> And this one.
> >  
> > +        Request(DocLoader*, CachedResource*, bool incremental, SecurityCheck securityCheck, bool sendResourceLoadCallbacks);
> 
> And this one.
> 
> > +        SecurityCheck shouldDoSecurityCheck() const { return m_securityCheck; }
> 
> The data member and the function member should have the same name.
> 
> > +        void loadRequest(const ResourceRequest&, SecurityCheck securityCheck);
> 
> Another place where you can omit the argument name.
> 
> > +    if ((securityCheck == DoSecurityCheck) && (fl->state() == FrameStateProvisional || fl->activeDocumentLoader()->isStopping()))
> 
> We normally would not include the parentheses around the "==" expression. It's
> so idiomatic to combine "==" with "&&" and "||" that we don't feel we need
> parentheses to make it clear.
> 
> > +    if ((securityCheck == DoSecurityCheck)
> 
> Ditto.
> 
> > +    // ok to skip load check since we already asked about the preflight request
> 
> Would be nice to format this comment as a sentence, which is our preferred
> style.
> 
> > +        static PassRefPtr<SubresourceLoader> create(Frame*, SubresourceLoaderClient*, const ResourceRequest&, SecurityCheck securityCheck = DoSecurityCheck, bool sendResourceLoadCallbacks = true, bool shouldContentSniff = true);
> 
> Another place to omit the argument name.
> 
> I feel bad about suggesting FrameLoaderTypes for this. Wish I could have
> thought of a better header.

All are now done locally.

These changes seem pretty trivial to me, do you want me to upload and go through another review cycle or just submit?

[Nate Chapin]

Created attachment 44891 [details]
patch2

Better safe than sorry, here's the patch with darin's comments addressed.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44891 [details] without any errors.

[Darin Adler]

Comment on attachment 44891 [details]
patch2

Sorry, I'm not trying to be difficult, but I see a problem now introduced by the changes I suggested.

> +    enum SecurityCheck {
> +        DoSecurityCheck,
> +        SkipSecurityCheck
> +    };

> -        bool shouldSkipCanLoadCheck() const { return m_shouldSkipCanLoadCheck; }
> +        SecurityCheck shouldDoSecurityCheck() const { return m_shouldDoSecurityCheck; }

This is a risky combination. The value of DoSecurityCheck is 0 and SkipSecurityCheck is 1, so this code:

    if (object->shouldDoSecurityCheck())

would look right, but be wrong. We should make the name of the function and data member be shouldSkipSecurityCheck or something neutral like securityCheckPolicy that does not imply a true/false mapping.

[Nate Chapin]

(In reply to comment #9)
> (From update of attachment 44891 [details])
> Sorry, I'm not trying to be difficult, but I see a problem now introduced by
> the changes I suggested.
> 
> > +    enum SecurityCheck {
> > +        DoSecurityCheck,
> > +        SkipSecurityCheck
> > +    };
> 
> > -        bool shouldSkipCanLoadCheck() const { return m_shouldSkipCanLoadCheck; }
> > +        SecurityCheck shouldDoSecurityCheck() const { return m_shouldDoSecurityCheck; }
> 
> This is a risky combination. The value of DoSecurityCheck is 0 and
> SkipSecurityCheck is 1, so this code:
> 
>     if (object->shouldDoSecurityCheck())
> 
> would look right, but be wrong. We should make the name of the function and
> data member be shouldSkipSecurityCheck or something neutral like
> securityCheckPolicy that does not imply a true/false mapping.

Good point.  Do you have a preference on renaming vs. switching the ordering in the enum so SkipSecurityCheck is 0 and DoSecurityCheck is 1?

[Darin Adler]

(In reply to comment #10)
> Good point. Do you have a preference on renaming vs. switching the ordering in
> the enum so SkipSecurityCheck is 0 and DoSecurityCheck is 1?

I slightly prefer switching the ordering, because I think an affirmative “do the check” is an easier concept than “skip the check”.

[Nate Chapin]

Created attachment 44909 [details]
patch3 - reordered SecurityCheck enum

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44909 [details] without any errors.

[Darin Adler]

Comment on attachment 44909 [details]
patch3 - reordered SecurityCheck enum

This is great. r=me

Next time we name a type like this, we should consider how the type name works conceptually. The type is named SecurityCheck, yet the object is not a security check. It's a decision about whether security checks should be performed. I think I would have called the type SecurityCheckPolicy.

[Nate Chapin]

(In reply to comment #14)
> (From update of attachment 44909 [details])
> This is great. r=me
> 
> Next time we name a type like this, we should consider how the type name works
> conceptually. The type is named SecurityCheck, yet the object is not a security
> check. It's a decision about whether security checks should be performed. I
> think I would have called the type SecurityCheckPolicy.

Yeah, SecurityCheckPolicy is probably a better name.

I'll rename it if you're willing to suffer through another round of review :)

[Darin Adler]

(In reply to comment #15)
> I'll rename it if you're willing to suffer through another round of review :)

I'm game.

[Nate Chapin]

Created attachment 44912 [details]
patch4 - renamed to SecurityCheckPolicy

[Nate Chapin]

http://trac.webkit.org/changeset/52177

Thanks!