Bug 212415

<rdar://problem/63679095>

Created attachment 400345 [details]
Patch

Comment on attachment 400345 [details]
Patch

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

> Source/WebCore/ChangeLog:7
> +

Need some explanation.

> Source/WebCore/ChangeLog:8
> +        No new tests (OOPS!).

Need justification for why there are no tests

> Source/WebCore/platform/graphics/filters/FilterEffect.cpp:272
> +        LOG_WITH_STREAM(Filters, stream << " m_premultipliedImageResult " << m_premultipliedImageResult->data());

I think this will create new lines, which probably isn't the intention. "FilterEffect" looks like it was an intentional prefix, to aid for things like grepping through logs.

How about using StringBuilder to reduce this giant line into something more readable? Perhaps putting this stuff in its own function, too?

Comment on attachment 400345 [details]
Patch

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

> Source/WebCore/platform/graphics/filters/FilterEffect.cpp:269
> -    LOG_WITH_STREAM(Filters, stream << "FilterEffect " << filterName() << " " << this << " imageBufferResult(). Existing image buffer " << m_imageBufferResult.get() <<  " m_premultipliedImageResult " << m_premultipliedImageResult->data() << " m_unmultipliedImageResult " << m_unmultipliedImageResult->data());
> +    LOG_WITH_STREAM(Filters, stream << "FilterEffect " << filterName() << " " << this << " imageBufferResult(). Existing image buffer " << m_imageBufferResult.get());

You want to do:
LOG_WITH_STREAM(Filters, stream << "FilterEffect " << filterName() << " " << this << " imageBufferResult(). Existing image buffer " << m_imageBufferResult.get() << " m_premultipliedImageResult " << (m_premultipliedImageResult  ? m_premultipliedImageResult->data() : nullptr) << " m_unmultipliedImageResult " << (m_unmultipliedImageResult  ? m_unmultipliedImageResult->data() : nullptr));

Created attachment 401045 [details]
Patch

Comment on attachment 401045 [details]
Patch

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

> Source/WebCore/platform/graphics/filters/FilterEffect.cpp:284
> +    StringBuilder builder;
> +    StringPrintStream logStream;
> +    logStream.printf("FilterEffect %s %p %s(). Existing image buffer %p", filterName(), this, resultType, m_imageBufferResult.get());
> +    
> +    if (m_premultipliedImageResult)
> +        logStream.printf(" m_premultipliedImageResult %p ", m_premultipliedImageResult->data());
> +    else
> +        logStream.printf(" m_premultipliedImageResult %p", nullptr);
> +        
> +    if (m_unmultipliedImageResult)
> +        logStream.printf(" m_unmultipliedImageResult %p", m_unmultipliedImageResult->data());
> +    else
> +        logStream.printf(" m_unmultipliedImageResult %p", nullptr);
> +    builder.append(logStream.toString());
> +    
> +    return builder.toString();

This seems worse than the original. 

Implement TextStream& operator<<(TextStream&, const ImageBuffer&)

then use ValueOrNull() in the logging.

Created attachment 401456 [details]
Patch

Created attachment 401463 [details]
Patch

Created attachment 401474 [details]
Patch - updated reviewer name

guowei_yang@apple.com does not have committer permissions according to https://trac.webkit.org/browser/webkit/trunk/Tools/Scripts/webkitpy/common/config/contributors.json.

Rejecting attachment 401474 [details] from commit queue.

Committed r262813: <https://trac.webkit.org/changeset/262813>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 401463 [details].

Comment on attachment 401463 [details]
Patch

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

> Source/WebCore/html/ImageData.cpp:122
> +    // Print out the address of the pixel data array
> +    return ts << imageData.data();

This comment can be removed.

This should also output the size.