[webkit-reviews] review granted: [Bug 174122] HTMLVideoElement with a broken poster image will take square dimension : [Attachment 378581] A updated patch after discussion with Daniel

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Sep 13 15:33:23 PDT 2019


Daniel Bates <dbates at webkit.org> has granted Peng Liu <peng.liu6 at apple.com>'s
request for review:
Bug 174122: HTMLVideoElement with a broken poster image will take square
dimension
https://bugs.webkit.org/show_bug.cgi?id=174122

Attachment 378581: A updated patch after discussion with Daniel

https://bugs.webkit.org/attachment.cgi?id=378581&action=review




--- Comment #12 from Daniel Bates <dbates at webkit.org> ---
Comment on attachment 378581
  --> https://bugs.webkit.org/attachment.cgi?id=378581
A updated patch after discussion with Daniel

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

Your change log message is very descriptive. I like it.

> Source/WebCore/rendering/RenderImage.cpp:463
> +		   RefPtr<Image> image = brokenImageAndImageScaleFactor.first;

It would be slightly more optimal to use auto& here instead of RefPtr<> (you'll
need to * the right-hand side) because brokenImageAndImageScaleFactor.first is
the broken image, which is guaranteed never be nullptr and exist for the entire
program lifetime. So, no need to ref it.

Optional: This doesn't need to be done in this patch callers of
CachedImage::brokenImage() would benefit if it returned a
std::pair<std::reference_wrapper<const Image>, float> instead of
std::pair<Image*, float>. If std::pair<std::reference_wrapper<const Image>,
float> doesn't work, then std::pair<std::reference_wrapper<Image>, float>.


More information about the webkit-reviews mailing list