Bug 131541 – Some small loader refinements and refactoring

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Darin Adler]

Some small loader refinements and refactoring

[Darin Adler]

Created attachment 229136 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 229136 [details]
Patch

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

> Source/WebCore/loader/ResourceLoader.cpp:400
> +    if (m_handle)
> +        m_handle->didChangePriority(loadPriority);

Eventually we'll need a way to change priority with NetworkProcess too.

At the moment, ResourceHandle::didChangePriority is a no-op though.

> Source/WebCore/loader/ResourceLoader.cpp:597
> +    m_handle->schedule(pair);

Perhaps a comment saying that this code is only used with WebKit1 would explain why there is no null check.

I really wish we could kill custom scheduling, but looks like it's currently used by at least two clients (it's an SPI).

> Source/WebCore/loader/ResourceLoader.h:130
> +    virtual void receivedCancellation(ResourceHandle*, const AuthenticationChallenge& challenge) override { receivedCancellation(challenge); }

I don't think that moving this function here is helpful. The biggest challenge when dealing with authentication (to me) is figuring out which of the similarly named functions are called by AuthenticationClient, and which are called by ResourceHandle. Moving them into one block makes it more confusing.

> Source/WebCore/loader/ResourceLoader.h:174
> +    void schedule(WTF::SchedulePair&);
> +    void unschedule(WTF::SchedulePair&);
> +#endif

No need for the WTF:: prefix.

> Source/WebCore/loader/mac/DocumentLoaderMac.cpp:42
> +    for (auto& loader : loadersCopy)

I would much prefer ResourceLoader& to auto&. We have lots and lots of loader classes, so the variable name is not sufficient for easy reading.

> Source/WebCore/loader/mac/DocumentLoaderMac.cpp:50
> +    for (auto& loader : loadersCopy)

Ditto.

> Source/WebCore/platform/network/ResourceHandle.h:139
> +    void schedule(WTF::SchedulePair&);
> +    void unschedule(WTF::SchedulePair&);

The WTF:: prefix is not needed.

> Source/WebCore/platform/network/ResourceHandle.h:287
> +#if PLATFORM(COCOA) && !USE(CFNETWORK)

These days, USE(FOUNDATION) may be a better macro to use here than PLATFORM(COCOA). Or maybe we should just add USE(NSURLCONNECTION), so that adding other Foundation based network back-ends would be easier.

Not something for this patch, of course.

[Darin Adler]

Comment on attachment 229136 [details]
Patch

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

Thanks for the review, Alexey!

>> Source/WebCore/loader/ResourceLoader.cpp:597
>> +    m_handle->schedule(pair);
> 
> Perhaps a comment saying that this code is only used with WebKit1 would explain why there is no null check.
> 
> I really wish we could kill custom scheduling, but looks like it's currently used by at least two clients (it's an SPI).

Actually, I meant to include a null check. I’ll add the null check rather than the comment.

>> Source/WebCore/loader/ResourceLoader.h:130
>> +    virtual void receivedCancellation(ResourceHandle*, const AuthenticationChallenge& challenge) override { receivedCancellation(challenge); }
> 
> I don't think that moving this function here is helpful. The biggest challenge when dealing with authentication (to me) is figuring out which of the similarly named functions are called by AuthenticationClient, and which are called by ResourceHandle. Moving them into one block makes it more confusing.

I am not sure how your comment tells me what is the best action. I can leave this function below, in between only-tangentially-related function declarations that are inside various conditionals.

But I was moving it up because it was unconditional and a forwarder, just like the other forwarders just above, not because of any other logic to the grouping.

>> Source/WebCore/loader/ResourceLoader.h:174
>> +#endif
> 
> No need for the WTF:: prefix.

Actually, here I do need the prefix, because this file forward-declares SchedulePair.

I have four choices:

1) Leave the prefix in this declaration.
2) Add SchedulePair to <wtf/Forward.h> so I don’t need the prefix or the forward declaration in this header.
3) Add “using WTF::SchedulePair” to the forward declaration in this header.
4) Include <wtf/SchedulePair.h> instead of forward declaring.

I chose (1), but maybe you would prefer (2) or (3)?

>> Source/WebCore/loader/mac/DocumentLoaderMac.cpp:42
>> +    for (auto& loader : loadersCopy)
> 
> I would much prefer ResourceLoader& to auto&. We have lots and lots of loader classes, so the variable name is not sufficient for easy reading.

I fear this is going to be a constant debate between the two of us. I find no value in specifically stating the type, my patch did not introduce the auto&, and so in this patch I am not going to change this to ResourceLoader&. I am looking forward to C++14 where this can just be written without the “auto&”.

>> Source/WebCore/platform/network/ResourceHandle.h:287
>> +#if PLATFORM(COCOA) && !USE(CFNETWORK)
> 
> These days, USE(FOUNDATION) may be a better macro to use here than PLATFORM(COCOA). Or maybe we should just add USE(NSURLCONNECTION), so that adding other Foundation based network back-ends would be easier.
> 
> Not something for this patch, of course.

Good point.

[Darin Adler]

Committed r167183: <http://trac.webkit.org/changeset/167183>

[Andy Estes]

iOS build fix: <http://trac.webkit.org/changeset/167190>

[Darin Adler]

I’m pretty sure we don’t need the SchedulePair feature at all for iOS.

[Darin Adler]

So the build fix is fine for future USE(CFNETWORK) on Mac, but we should change both of those conditionals to PLATFORM(MAC) instead of PLATFORM(COCOA).

[Alexey Proskuryakov]

> I fear this is going to be a constant debate between the two of us. I find no value in specifically stating the type, my patch did not introduce the auto&, and so in this patch I am not going to change this to ResourceLoader&. I am looking forward to C++14 where this can just be written without the “auto&”.

Per webkit-dev thread, multiple WebKit hackers find significant value in seeing type names. If it's just a question of others not seeing the value, but also not seeing harm, it seems that using explicit types would accommodate both camps.

These autos are a problem for me.