Bug 106146 - Show label automatically for plugins of significant size
Summary: Show label automatically for plugins of significant size
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.8
: P2 Normal
Assignee: Jon Lee
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2013-01-04 16:21 PST by Jon Lee
Modified: 2013-01-06 23:30 PST (History)
7 users (show)

See Also:

Attachments
Patch (15.05 KB, patch)
2013-01-04 19:10 PST, Jon Lee 		simon.fraser: review+
Details | Formatted Diff | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Jon Lee 2013-01-04 16:21:50 PST 
Snapshotted plugins that dominate a page should automatically show the label.
Comment 1 Jon Lee 2013-01-04 16:22:22 PST 
<rdar://problem/12783010>
Comment 2 Jon Lee 2013-01-04 19:10:47 PST 
Created attachment 181421 [details]
Patch
Comment 3 Simon Fraser (smfr) 2013-01-04 19:40:50 PST 
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.
Comment 4 Jon Lee 2013-01-06 23:12:03 PST 
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.
Comment 5 Jon Lee 2013-01-06 23:30:12 PST 
Committed r138927: <http://trac.webkit.org/changeset/138927>