[webkit-reviews] review denied: [Bug 42292] Add NetworkingContext to avoid layer violations : [Attachment 62106] NetworkingContext v2

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Jesus Sanchez-Palencia
<jesus at webkit.org>'s request for review:
Bug 42292: Add NetworkingContext to avoid layer violations
https://bugs.webkit.org/show_bug.cgi?id=42292

Attachment 62106: NetworkingContext v2
https://bugs.webkit.org/attachment.cgi?id=62106&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> -	   m_handle = ResourceHandle::create(r, this, m_frame.get(), false,
true);
> +	   m_handle = ResourceHandle::create(r, this,
m_frame.get()->loader()->client()->networkingContext(), false, true);

Should not need get() here any more.

> -    m_handle = ResourceHandle::create(clientRequest, this, m_frame.get(),
m_defersLoading, m_shouldContentSniff);
> +    m_handle = ResourceHandle::create(clientRequest, this,
m_frame.get()->loader()->client()->networkingContext(), m_defersLoading,
m_shouldContentSniff);

Or here.

> diff --git a/WebCore/loader/ResourceLoader.h
b/WebCore/loader/ResourceLoader.h
> index cbdd8c2..1b097c6 100644
> --- a/WebCore/loader/ResourceLoader.h
> +++ b/WebCore/loader/ResourceLoader.h
> @@ -29,6 +29,7 @@
>  #ifndef ResourceLoader_h
>  #define ResourceLoader_h
>  
> +#include "FrameLoaderClient.h"

Why does this need to be added to this header? Doesn't make sense to me.
Perhaps should be added to some .cpp files, but not the header.

> +class NetworkingContext : public Noncopyable {
> +public:
> +    static PassOwnPtr<NetworkingContext> create(Frame *frame);

The networking context class cannot include a pointer to the Frame*. That’s the
same layering violation we are trying to fix. Instead, it needs to take
whatever arguments make sense on each platform that needs it. But never a
Frame*. This means you have to plumb the NetworkingContext in deeper, because
you can't just go from NetworkingContext* to Frame*. But maybe you could
temporarily have it work this way as a staging step.

> +    , m_networkingContext(0)

No need for this. An OwnPtr automatically starts at 0.

> +#include "OwnPtr.h"

Should be <wtf/OwnPtr.h>.

review- because of the "includes a pointer to Frame*" issue.