Clean up Blob code

[Alex Christensen]

Clean up Blob code

[Alex Christensen]

Created attachment 270725 [details]
Patch

[Alex Christensen]

Created attachment 270736 [details]
Patch

[Alex Christensen]

Created attachment 270737 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 270737 [details]
Patch

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

> Source/WebCore/fileapi/Blob.h:79
> +#ifndef NDEBUG

If this is for assertions only, then it should be using ASSERT_DISABLED macro (name from memory).

> Source/WebCore/platform/network/BlobRegistry.h:69
> +    virtual ~BlobRegistry() { };

This is not an improvement - constructors and destructors of polymorphic classes should not be inline, because of code size bloat.

> Source/WebCore/platform/network/BlobRegistryImpl.cpp:78
> +        return nullptr;

Does return value optimization work in functions with multiple exit points?

> Source/WebCore/platform/network/BlobRegistryImpl.h:70
> +    HashMap<URL, RefPtr<BlobData>> m_blobs;

Why is this better? URL is a large object. Do we waste time reparsing them later?

[Alex Christensen]

Comment on attachment 270737 [details]
Patch

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

>> Source/WebCore/platform/network/BlobRegistryImpl.cpp:78
>> +        return nullptr;
> 
> Does return value optimization work in functions with multiple exit points?

Yes.

> Source/WebCore/platform/network/BlobRegistryImpl.cpp:149
> -            if (!m_blobs.contains(part.url().string()))
> +            if (!m_blobs.contains(part.url()))
>                  return;
> -            for (const BlobDataItem& item : m_blobs.get(part.url().string())->items())
> +            for (const BlobDataItem& item : m_blobs.get(part.url())->items())

I am going to make this only lookup the url once in the HashMap, though.

>> Source/WebCore/platform/network/BlobRegistryImpl.h:70
>> +    HashMap<URL, RefPtr<BlobData>> m_blobs;
> 
> Why is this better? URL is a large object. Do we waste time reparsing them later?

I thought it was more clear, and we don't reparse them, but you're right: this is a memory regression.

[Alex Christensen]

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