Bug 150742 – Share more code between NETWORK_SESSION and non-NETWORK_SESSION NetworkResourceLoaders

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alex Christensen]

Share more code between NETWORK_SESSION and non-NETWORK_SESSION WebResourceLoaders

[Alex Christensen]

Created attachment 264443 [details]
Patch

[Alex Christensen]

Created attachment 264446 [details]
Patch

[Alex Christensen]

Created attachment 264449 [details]
Patch

[Alex Christensen]

Created attachment 264482 [details]
Patch

[Darin Adler]

Comment on attachment 264482 [details]
Patch

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

> Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:497
> +    // The NetworkProcess should never get a didReceiveData callback.
> +    // We should always be using didReceiveBuffer.
> +    ASSERT_NOT_REACHED();

This kind of thing usually indicates a C++ object design mistake.

> Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:522
> +    ASSERT_UNUSED(handle, handle == m_handle);
> +
> +    sharedWillSendRedirectedRequest(request, redirectResponse);

Strange that there’s a blank line here, but not in all the other functions.

> Source/WebKit2/NetworkProcess/NetworkResourceLoader.h:142
> +    void sharedDidReceiveBuffer(PassRefPtr<WebCore::SharedBuffer>, int reportedEncodedDataLength);

New code should not use PassRefPtr. It should use SharedBuffer&, or SharedBuffer*, or Ref<SharedBuffer>&&, or RefPtr<SharedBuffer>&& depending on whether the buffer can be null and whether ownership is being transferred or not.

[Alex Christensen]

(In reply to comment #5)
> Comment on attachment 264482 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=264482&action=review
> 
> > Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:497
> > +    // The NetworkProcess should never get a didReceiveData callback.
> > +    // We should always be using didReceiveBuffer.
> > +    ASSERT_NOT_REACHED();
> 
> This kind of thing usually indicates a C++ object design mistake.
I agree.  Cleaning this up is outside of the scope of this patch, though.
> 
> > Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:522
> > +    ASSERT_UNUSED(handle, handle == m_handle);
> > +
> > +    sharedWillSendRedirectedRequest(request, redirectResponse);
> 
> Strange that there’s a blank line here, but not in all the other functions.
> 
> > Source/WebKit2/NetworkProcess/NetworkResourceLoader.h:142
> > +    void sharedDidReceiveBuffer(PassRefPtr<WebCore::SharedBuffer>, int reportedEncodedDataLength);
> 
> New code should not use PassRefPtr. It should use SharedBuffer&, or
> SharedBuffer*, or Ref<SharedBuffer>&&, or RefPtr<SharedBuffer>&& depending
> on whether the buffer can be null and whether ownership is being transferred
> or not.
RefPtr<SharedBuffer>&& it is, then.

[Alex Christensen]

http://trac.webkit.org/changeset/191848

[Darin Adler]

Comment on attachment 264482 [details]
Patch

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

> Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:473
> +    sharedDidReceiveBuffer(buffer, buffer->size());

The patch that was landed changed this to:

    sharedDidReceiveBuffer(WTF::move(buffer), buffer->size());

That’s wrong, because passing the first argument can modify buffer, changing it to be null, and the passing the second argument evaluates buffer. There is no guarantee of the order of evaluation of arguments, so this needs to be written out as two lines:

    auto size = buffer->size();
    sharedDidReceiveBuffer(WTF::move(buffer), size);

Please fix that.

[Alex Christensen]

(In reply to comment #8)
> Please fix that.
Followup in https://bugs.webkit.org/show_bug.cgi?id=150829