[webkit-reviews] review granted: [Bug 131541] Some small loader refinements and refactoring : [Attachment 229136] Patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted Darin Adler <darin at apple.com>'s
request for review:
Bug 131541: Some small loader refinements and refactoring
https://bugs.webkit.org/show_bug.cgi?id=131541

Attachment 229136: Patch
https://bugs.webkit.org/attachment.cgi?id=229136&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
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.