Bug 106146 – Show label automatically for plugins of significant size

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

[Jon Lee]

Snapshotted plugins that dominate a page should automatically show the label.

[Jon Lee]

<rdar://problem/12783010>

[Jon Lee]

Created attachment 181421 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 181421 [details]
Patch

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

> Source/WebCore/html/HTMLPlugInImageElement.cpp:293
> +    LayoutRect plugInClipRect = node->renderer()->absoluteClippedOverflowRect();

Is node->renderer() always non-null?

> Source/WebCore/html/HTMLPlugInImageElement.cpp:352
> +    if (shouldPlugInShowLabelAutomatically(document()->page()->mainFrame()->view()->contentsSize(), this))

Is everything in this chain always non-null?

> Source/WebCore/platform/Timer.h:143
> +    void setDelay(double delay) { m_delay = delay; }

Not sure you need this (see later comment).

> Source/WebCore/rendering/RenderSnapshottedPlugIn.cpp:276
> +    m_showLabelDelayTimer.setDelay(reason == UserMousedOver ? showLabelAfterMouseOverDelay : showLabelAutomaticallyDelay);
> +    m_showLabelDelayTimer.restart();

Can't you just call startOneShot() and avoid touching Timer.h? I don't think you even need to call stop() beforehand.

[Jon Lee]

Comment on attachment 181421 [details]
Patch

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

>> Source/WebCore/html/HTMLPlugInImageElement.cpp:293
>> +    LayoutRect plugInClipRect = node->renderer()->absoluteClippedOverflowRect();
> 
> Is node->renderer() always non-null?

I believe so. The renderer is already accessed earlier in subframeLoaderWillCreatePlugIn(), which is the caller of this function.

>> Source/WebCore/html/HTMLPlugInImageElement.cpp:352
>> +    if (shouldPlugInShowLabelAutomatically(document()->page()->mainFrame()->view()->contentsSize(), this))
> 
> Is everything in this chain always non-null?

I cannot guarantee it is. I will add checks for mainFrame() and view(). page() is already checked earlier in this function.

>> Source/WebCore/rendering/RenderSnapshottedPlugIn.cpp:276
>> +    m_showLabelDelayTimer.restart();
> 
> Can't you just call startOneShot() and avoid touching Timer.h? I don't think you even need to call stop() beforehand.

Yes, you're right. Amended.

[Jon Lee]

Committed r138927: <http://trac.webkit.org/changeset/138927>