Bug 162146 – Clean-up CachedResourceLoader::requestResource after r206016

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

[youenn fablet]

CachedResourceRequest is moved to construct a CachedResource.
This makes CachedResourceLoader::requestResource currently storing some fields of CachedResourceRequest before moving it.

[youenn fablet]

Created attachment 289223 [details]
Patch

[Alex Christensen]

Comment on attachment 289223 [details]
Patch

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

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:1178
> +    CachedResourceHandle<CachedResource> resource = requestResource(type, WTFMove(request), true);

This value of true should be given a name.

> Source/WebCore/loader/cache/CachedResourceLoader.h:151
> +    enum class ForPreload { True, False };

Use this instead of bools.  In WebKit we use No, Yes for things like this.

[youenn fablet]

Thanks for the review.

(In reply to comment #2)
> Comment on attachment 289223 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=289223&action=review
> 
> > Source/WebCore/loader/cache/CachedResourceLoader.cpp:1178
> > +    CachedResourceHandle<CachedResource> resource = requestResource(type, WTFMove(request), true);
> 
> This value of true should be given a name.

OK, I will use an enum class.
The main issue I see is that instead of writing "if (forPreload)"
We need to write "if (forPreload == ...)"

Maybe we can find a way to have both advantages?

[youenn fablet]

Created attachment 289440 [details]
Patch

[youenn fablet]

(In reply to comment #4)
> Created attachment 289440 [details]
> Patch

Patch also removed redirectResponse boolean.
This improves over bug 162144.

[Darin Adler]

Our typical pattern is this:

- If we use constants for an argument with a generic type like "bool" that makes it hard to understand the meaning of the argument at the call site we do one of the following:

1) Break into two separate functions rather than using a boolean argument to create two separate behaviors. This underused option is often quite effective in making code easy to read and even can push us toward better factoring. The pattern of “do a big job and then look at a boolean in the middle to do two variations” is not really a great one.

2) Move the boolean into an object that can either be a high level policy object like a security origin or just a simple structure with well-named fields; also helps make code that has to move multiple arguments along from one function to another easier to read.

3) Use a named enum value instead of a boolean. Can be tricky to decide whether to use enum class or enum and whether to make the enum a class member or at the namespace level.

- If the value passed in is not a constant, but rather computed, then we are willing to use a boolean and we try not to change to an enum.

The time we run into trouble is when we have a function that often takes a constant but sometimes takes a computed value, and the value is not just the enum passed through, rather is computed some other way. Then we often find we have to convert a boolean expression into an enum value and then back to a boolean. Super ugly. But keep the rules above in mind because there are often alternate paths.

[youenn fablet]

This was fixed in another of my patch