Bug 32529

Created attachment 44813 [details] patch

Comment on attachment 44813 [details] patch This looks good to me. Sooooo much more readable!

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

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

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.

(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?

Created attachment 44891 [details] patch2 Better safe than sorry, here's the patch with darin's comments addressed.

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

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.

(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?

(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”.

Created attachment 44909 [details] patch3 - reordered SecurityCheck enum

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

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.

(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 :)

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

Created attachment 44912 [details] patch4 - renamed to SecurityCheckPolicy

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