Bug 46543

Created attachment 68794 [details] Proposed Patch

I'll look at this. Jian and I already discussed one issue.

Comment on attachment 68794 [details] Proposed Patch View in context: https://bugs.webkit.org/attachment.cgi?id=68794&action=review > WebCore/fileapi/BlobBuilder.cpp:52 > + DEFINE_STATIC_LOCAL(AtomicString, transparent, ("transparent")); > + DEFINE_STATIC_LOCAL(AtomicString, native, ("native")); Why AtomicString? You're comparing against a non-Atomic string. > WebCore/fileapi/BlobBuilder.cpp:59 > + CString ctext = UTF8Encoding().encode(text.characters(), text.length(), EntitiesForUnencodables); ctext? Can we use a more descriptive variable name? > WebCore/fileapi/BlobBuilder.cpp:73 > + buffer.grow(buffer.size() + ctext.length()); > + memcpy(buffer.data() + oldSize, ctext.data(), ctext.length()); Why not just use buffer.append() ? > WebCore/platform/network/BlobData.cpp:46 > + : m_data(data.leakPtr()) leakPtr ? So sad. > WebCore/platform/network/BlobData.h:62 > + Vector<char>* m_data; Why can't this be an OwnPtr? > WebCore/platform/text/LineEnding.cpp:73 > +#if OS(WINDOWS) Why is this windows-specific? > WebCore/platform/text/LineEnding.cpp:93 > + unsigned oldSize = m_buffer.size(); > + m_buffer.grow(oldSize + src.length()); > + memcpy(m_buffer.data() + oldSize, src.data(), src.length()); again, why not just m_buffer.append() ?

Comment on attachment 68794 [details] Proposed Patch View in context: https://bugs.webkit.org/attachment.cgi?id=68794&action=review > WebCore/fileapi/BlobBuilder.cpp:66 > + Vector<char>& buffer = *m_items[m_items.size() - 1].data.mutableData(); Is this the only use case for mutableData? Maybe instead of mutating an existing 'DataItem', just dump new data into a plain old Vector<char> that's owned by the builder and defer adding a DataItem to the list until you have a fully formed thing... buffer stuff up in a Vector, then flush it out as a DataItem. > WebCore/platform/network/BlobData.h:63 > + RefPtr<SharedRawData> m_sharedData; Why have the m_data ptr at all, you can access the vector thru m_sharedData as needed, maybe a helper to return a const Vector<char>*

Comment on attachment 68794 [details] Proposed Patch View in context: https://bugs.webkit.org/attachment.cgi?id=68794&action=review There are enough things in here to be addressed at the moment that I'm r-'ing. I think once they are fixed some items will be a little clearer and there may be a few more comments. > WebCore/platform/network/BlobData.h:36 > +#include <wtf/CrossThreadRefCounted.h> I'd strongly discourage using CrossThreadRefCounted. It has a very special use case when the ref counting must live outside of the class (and probably a few other things that I don't remember right off). Instead create a class that derives from ThreadSafeShared. I suspect this change will make this code much easier to read and may clear up some issues that others were concerned about. > WebCore/platform/network/BlobData.h:54 > + void makeCrossThread(); makeCrossThread is a terrible name that I think I started. I like the suggestion of detachFromCurrentThread (or something like that). > WebCore/platform/text/LineEnding.cpp:89 > + virtual void copy(const CString& src) avoid abbreviations "src" > WebKit/chromium/public/WebBlobData.h:31 > #ifndef WebBlobData_h If you break out this part of the change in to a separate patch, it would be better as Darin could review just that patch and likely do it quickly.

Created attachment 70179 [details] Proposed Patch

All others are fixed. (In reply to comment #3) > > WebCore/platform/text/LineEnding.cpp:73 > > +#if OS(WINDOWS) > > Why is this windows-specific? > This is because if the ending type is specified as "native", we need to do the line ending normalization per the platform-specific logic.

(In reply to comment #4) > (From update of attachment 68794 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=68794&action=review > > > WebCore/fileapi/BlobBuilder.cpp:66 > > + Vector<char>& buffer = *m_items[m_items.size() - 1].data.mutableData(); > > Is this the only use case for mutableData? Maybe instead of mutating an existing 'DataItem', just dump new data into a plain old Vector<char> that's owned by the builder and defer adding a DataItem to the list until you have a fully formed thing... buffer stuff up in a Vector, then flush it out as a DataItem. Since now I put a Vector<char> directly in RawData, it will add up some more complexity to change as suggest, though it does simply the logic in BlobBuilder, but it would add up the complexity in BlobData. I rather keep it as is now. > > > WebCore/platform/network/BlobData.h:63 > > + RefPtr<SharedRawData> m_sharedData; > > Why have the m_data ptr at all, you can access the vector thru m_sharedData as needed, maybe a helper to return a const Vector<char>* Removed due to the new structure change.

All fixed. WebBlobData is stripped out and it will be included in another patch for review. (In reply to comment #5) > (From update of attachment 68794 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=68794&action=review > > There are enough things in here to be addressed at the moment that I'm r-'ing. I think once they are fixed some items will be a little clearer and there may be a few more comments. >

Attachment 70179 [details] did not build on chromium: Build output: http://queues.webkit.org/results/4259033

Comment on attachment 70179 [details] Proposed Patch View in context: https://bugs.webkit.org/attachment.cgi?id=70179&action=review > WebCore/fileapi/BlobBuilder.cpp:53 > + if (!endingType.isEmpty() && !isEndingTypeTransparent && !isEndingTypeNative) { This looks like an interesting error. Is there a test case for it? Could there be one? > WebCore/platform/text/LineEnding.cpp:73 > +#if OS(WINDOWS) I wouldn't ifdef this because it make it confusing to try to guess why it is ifdef'ed out. > WebCore/platform/text/LineEnding.cpp:128 > + char* q = buffer.allocate(newLen); It would be nice to have a more descriptive variable name than "q".

http://trac.webkit.org/changeset/69610 might have broken Chromium Win Release

Committed as http://trac.webkit.org/changeset/69610.