[webkit-reviews] review granted: [Bug 104107] [WK2] Move button image to injected bundle : [Attachment 178228] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Jon Lee
<jonlee at apple.com>'s request for review:
Bug 104107: [WK2] Move button image to injected bundle
https://bugs.webkit.org/show_bug.cgi?id=104107

Attachment 178228: Patch
https://bugs.webkit.org/attachment.cgi?id=178228&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=178228&action=review


> Source/WebCore/page/ChromeClient.h:376
> +	   virtual PassRefPtr<Image>
generatePlugInStartButtonImage(RenderSnapshottedPlugIn::ButtonSize) const {
return 0; }

Why 'generate'? Why not just plugInStartButtonImage() ?

> Source/WebCore/rendering/RenderSnapshottedPlugIn.cpp:138
> +    if (initializedImages[arrayIndex])
> +	   return 0;

Is this just to avoid multiple aborted attempts to get the image? Do we expect
that ever to happen?

> Source/WebCore/rendering/RenderSnapshottedPlugIn.cpp:154
> +    Image* buttonImage = 0;
> +    if (plugInImageElement()->hovered())
> +	   buttonImage = startButtonImage(m_buttonType);

This logic is a bit odd. Why not just early return if not hovered?

> Source/WebCore/rendering/RenderSnapshottedPlugIn.cpp:245
> +    m_buttonRect = tryToFitStartButton(ButtonSizeLarge, rect);
> +    if (!m_buttonRect.isEmpty()) {
> +	   m_buttonType = ButtonSizeLarge;
> +	   return;
> +    }
> +
> +    m_buttonRect = tryToFitStartButton(ButtonSizeSmall, rect);
> +    if (!m_buttonRect.isEmpty()) {
> +	   m_buttonType = ButtonSizeSmall;
> +	   return;
> +    }
> +
> +    m_buttonType = NoButton;

I'm not sure why you have to run this logic at layout time. Can't you just
choose what to paint at paint time?