Bug 104107 – [WK2] Move button image to injected bundle

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Jon Lee]

Allow the bundle to specify the images to use.

[Radar WebKit Bug Importer]

<rdar://problem/12813842>

[Jon Lee]

Created attachment 177713 [details]
Patch

[Early Warning System Bot]

Comment on attachment 177713 [details]
Patch

Attachment 177713 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15159206

[Early Warning System Bot]

Comment on attachment 177713 [details]
Patch

Attachment 177713 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15160179

[Jon Lee]

Created attachment 177720 [details]
Patch

[Early Warning System Bot]

Comment on attachment 177720 [details]
Patch

Attachment 177720 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15133796

[WebKit Review Bot]

Comment on attachment 177720 [details]
Patch

Attachment 177720 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15152360

[Peter Beverloo (cr-android ews)]

Comment on attachment 177720 [details]
Patch

Attachment 177720 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/15152358

[Build Bot]

Comment on attachment 177720 [details]
Patch

Attachment 177720 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/15148445

[EFL EWS Bot]

Comment on attachment 177720 [details]
Patch

Attachment 177720 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15152374

[kov's GTK+ EWS bot]

Comment on attachment 177720 [details]
Patch

Attachment 177720 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/15159276

[Jon Lee]

Created attachment 177766 [details]
Patch

[Jon Lee]

Created attachment 178195 [details]
Patch

[Jon Lee]

Created attachment 178226 [details]
Patch

[Jon Lee]

Created attachment 178228 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 178228 [details]
Patch

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?

[Jon Lee]

Comment on attachment 178228 [details]
Patch

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() ?

Renamed. This was originally connected to the idea that via the WK2 API we have to copy the data to a bitmap image every time we make this call, but since the code already caches those images, this is less important.

>> Source/WebCore/rendering/RenderSnapshottedPlugIn.cpp:138
>> +        return 0;
> 
> Is this just to avoid multiple aborted attempts to get the image? Do we expect that ever to happen?

If a client decides not to provide an image, then we will already ask for it. So I wanted to avoid that. Clients that implement this probably will provide all buttons.

>> Source/WebCore/rendering/RenderSnapshottedPlugIn.cpp:154
>> +        buttonImage = startButtonImage(m_buttonType);
> 
> This logic is a bit odd. Why not just early return if not hovered?

Yes, I've updated the logic here.

>> Source/WebCore/rendering/RenderSnapshottedPlugIn.cpp:245
>> +    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?

I thought it might be less performant to do this on every paint, but I can move it. Moving it would also obviate the need for these instance variables.

[Jon Lee]

Committed r137067: <http://trac.webkit.org/changeset/137067>