Bug 158332 – Modernize loading code

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

[Alex Christensen]

Use more SharedBuffer instead of a pointer and length

[Alex Christensen]

Created attachment 280400 [details]
Patch

[Alex Christensen]

Comment on attachment 280400 [details]
Patch

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

> Source/WebCore/loader/cache/CachedRawResource.cpp:54
> -    unsigned incrementalDataLength;
> -    const char* incrementalData = calculateIncrementalDataChunk(&data, incrementalDataLength);
> -    setEncodedSize(data.size());
> -    notifyClientsDataWasReceived(incrementalData, incrementalDataLength);
> +    notifyClientsDataWasReceived(data);

I think this may have changed behavior, but I'm not sure.

[Build Bot]

Comment on attachment 280400 [details]
Patch

Attachment 280400 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1426096

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 280415 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 280400 [details]
Patch

Attachment 280400 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1426067

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 280416 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 280400 [details]
Patch

Attachment 280400 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1426133

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 280418 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Alex Christensen]

Created attachment 280617 [details]
Patch

[Alex Christensen]

Created attachment 280645 [details]
Patch

[Alex Christensen]

Created attachment 280651 [details]
Patch

[Alex Christensen]

Created attachment 280652 [details]
Patch

[Darin Adler]

Comment on attachment 280652 [details]
Patch

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

Description says modernize, but I also see a some null checking being added that seems unrelated to the modernizing.

> Source/WebCore/loader/DocumentLoader.cpp:809
> +        if (m_substituteData.content() && m_substituteData.content()->size())
> +            dataReceived(nullptr, m_substituteData.content()->data(), m_substituteData.content()->size());

Given that we dereference it three times after checking it for null, maybe put the pointer into a local variable?

> Source/WebCore/loader/ResourceLoader.cpp:310
> +        m_resourceData->append(buffer);

Should append take a SharedBuffer& instead of a SharedBuffer*?

> Source/WebCore/loader/SubstituteData.h:58
> -        const SharedBuffer* content() const { return m_content.get(); }
> +        SharedBuffer* content() const { return m_content.get(); }

What’s the rationale behind this change?

> Source/WebCore/platform/SharedBuffer.cpp:111
>      RefPtr<SharedBuffer> buffer = create();

Should use auto.

> Source/WebCore/platform/SharedBuffer.cpp:291
> +    Ref<DataBuffer> newBuffer = adoptRef(*new DataBuffer);

Should use auto.

> Source/WebCore/platform/SharedBuffer.h:61
> +    WEBCORE_EXPORT static RefPtr<SharedBuffer> adoptVector(Vector<char>&);

This should return a Ref, not a RefPtr.

[Alex Christensen]

(In reply to comment #13)
> > Source/WebCore/loader/SubstituteData.h:58
> > -        const SharedBuffer* content() const { return m_content.get(); }
> > +        SharedBuffer* content() const { return m_content.get(); }
> 
> What’s the rationale behind this change?
This was needed because of the lack of https://bugs.webkit.org/show_bug.cgi?id=158269 at some point while working on this patch, but I don't think it's needed any more.  Removing.

[Alex Christensen]

Committed to http://trac.webkit.org/changeset/201761
Fixed some builds in http://trac.webkit.org/changeset/201767